On 23-04-16 10:40PM, Mark Jamsek wrote:
> On 23-04-14 03:52PM, Stefan Sperling wrote:
> > On Mon, Mar 13, 2023 at 12:01:42AM +0000, James Cook wrote:
> > > I recently noticed the following bugs. I just verified that they still
> > > happen with the lastest got, commit 264c43dd.
> > > 
> > > Two of them have scripts to reproduce; the other's pretty simple.
> >  
> > I would like to fix these issues but haven't found time to take
> > a closer look yet, sorry.
> 
> I just had a cursory look at the case for the first two bugs and it
> seems that at this point in the reproducer:
> 
> ---
> # In the worktree called "linear", add a commit *without updating*.
> cd ../linear
> echo 2 > f
> got commit -m 2
> ---
> 
> the commit _should_ fail because the work tree's base commit is not
> up-to-date (i.e., does not match the work tree's head ref).
> 
> Looking at the code, this appears to be the responsibility of
> lib/worktree.c:6128:check_out_of_date(); in particular, should this
> check be switched from:
> 
> 	if (got_object_id_cmp(base_commit_id, head_commit_id) == 0)
> 		return NULL;
> 
> to:
> 
> 	if (got_object_id_cmp(base_commit_id, head_commit_id) != 0)
> 		return got_error(ood_errcode);
> 
> This change appears to correct the behaviour of the first two bugs;
> however, I'm unsure of the original intent behind allowing out-of-date
> base commits to continue here.

Nevermind. I should have let regress finish first. This breaks other
things so clearly the check is correct. I'll keep looking into these
bugs though.

> diff /home/mark/src/got
> commit - e1dd38749cb5548ac21464ee05e4a3a492c7add1
> path + /home/mark/src/got
> blob - df29e65f90431310315474962e5c738ec544f38d
> file + lib/worktree.c
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -6136,9 +6136,9 @@ check_out_of_date(const char *in_repo_path, unsigned c
>  	struct got_object_id *id = NULL;
>  
>  	if (status != GOT_STATUS_ADD && staged_status != GOT_STATUS_ADD) {
> -		/* Trivial case: base commit == head commit */
> -		if (got_object_id_cmp(base_commit_id, head_commit_id) == 0)
> -			return NULL;
> +		/* Trivial case: base commit != head commit */
> +		if (got_object_id_cmp(base_commit_id, head_commit_id) != 0)
> +			return got_error(ood_errcode);
>  		/*
>  		 * Ensure file content which local changes were based
>  		 * on matches file content in the branch head.
> 
> > 
> > > ** First bug: got ci doesn't properly check for updated-ness, and
> > >    clobbers merge commit with surprising results
> > > 
> > > A script to reproduce follows. First, though, a description. We set up
> > > the following four commits, where a commit "branch" from another branch
> > > got merged into the main branch.
> > > 
> > > merge
> > > |   \
> > > 1   branch
> > > |  /
> > > init
> > > 
> > > Now, in another worktree pointing to the "1" commit, we make a change
> > > and run "got ci". This should fail because the worktree is not updated.
> > > Instead, the merge commit is forgotten, and we end up with:
> > > 
> > > 2
> > > |
> > > 1
> > > |
> > > init
> > > 
> > > As fallout from this strange situation, two weird things happen:
> > > - The commit "2" includes the changes from the commit "branch", even
> > >   though they weren't in the work tree when we ran "got ci" to create
> > >   that commit.
> > > - In the worktree pointing to the commit "merge", got up refuses to run,
> > >   saying "got: work tree's head reference now points to a different
> > >   branch; new head reference and/or update -b required"
> > > 
> > > ---- BEGIN script ----
> > > #!/bin/sh
> > > set -e
> > > 
> > > mkdir bug;cd bug
> > > 
> > > # Create a repo with an initial commit.
> > > gotadmin init repo.git
> > > mkdir init;touch init/f
> > > got import -m init -r repo.git init
> > > 
> > > # For convenience, we'll use three work trees.
> > > for name in branch merge linear; do got checkout repo.git $name; done
> > > 
> > > # Make a branch with one new commit.
> > > cd branch
> > > got branch aux
> > > echo 1 > on_branch
> > > got add on_branch
> > > got commit -m branch
> > > 
> > > # Commit to the main branch; put that in the other two work trees.
> > > cd ../linear
> > > echo 1 > f
> > > got commit -m 1
> > > cd ../merge
> > > got up
> > > 
> > > # In the worktree called "merge", add a merge commit to main.
> > > got merge aux
> > > 
> > > # In the worktree called "linear", add a commit *without updating*.
> > > cd ../linear
> > > echo 2 > f
> > > got commit -m 2
> > > 
> > > # First problem:
> > > #
> > > # Examine the commit that was created in the worktree at bug/linear.
> > > # Surprisingly, it adds the file "on_branch", but does not include
> > > # either the commit called "branch" or the merge commit in its history.
> > > 
> > > # Second problem:
> > > cd ../merge
> > > got up
> > > # got complains: "got: work tree's head reference now points to a different branch; new head reference and/or update -b required"
> > > ---- END script ----
> > > 
> > > ** Second bug: can't send after merging origin/main
> > > 
> > > This one's simpler. An argument could be made it's not a bug, but the
> > > behaviour is certainly different from git.
> > > 
> > > - Clone a remote repo.
> > > - Make a change in the remote repo.
> > > - Separately, make a change in your local clone.
> > > - In the local clone, run got fetch then got merge origin/main.
> > > - Run got send.
> > > 
> > > got send says:
> > > 	got: refs/heads/main: fetch and rebase required
> > > Coming from git, my expectation is that I can send the new commit since
> > > it includes the remote head in its history. If the goal of "got merge"
> > > is only to support vendor branches, then this behaviour might be
> > > reasonable, since it indicates a departure from that work flow: the
> > > remote main should appear in the "linear history" ignoring second merge
> > > children. Still, though, a different error message would help.
> > > 
> > > ** Third bug: Can't merge a branch with identical content.
> > > 
> > > Create two branches A and B, where A includes all of B's changes, but as
> > > different commit objects. Then try to merge B into A. Got complains:
> > > 	got: merge of refs/heads/aux cannot proceed: no changes to commit
> > > 
> > > I do occasionally want to create merge commits like this. For example,
> > > it happened while I was cleaning up after I encountered the first bug
> > > above: I ended up with divergent commits making some of the same
> > > changes, and wanted to incorporate all of them into my commit graph for
> > > a historical record.
> > > 
> > > Here's a script to reproduce; for simplicity, the changes are not just a
> > > subset but exactly the same.
> > > ---- BEGIN script ----
> > > #!/bin/sh
> > > set -e
> > > 
> > > mkdir bug;cd bug
> > > 
> > > # Create and check out a repo with an initial commit.
> > > gotadmin init repo.git
> > > mkdir init;touch init/f
> > > got import -m init -r repo.git init
> > > got checkout repo.git; cd repo
> > > 
> > > # Create a branch with a commit.
> > > got branch aux
> > > echo 1 > f
> > > got ci -m branch
> > > 
> > > # Commit the same change (but different commit object) to main.
> > > got up -b main
> > > echo 1 > f
> > > got ci -m main
> > > 
> > > # Try to merge.
> > > got merge aux
> > > ---- END script ----
> > > 
> > > -- 
> > > James
> > > 
> > > 
> > 
> 
> -- 
> Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
> GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68

-- 
Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68