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.

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