From: Mark Jamsek Subject: Re: three bugs related to merge commits To: James Cook , gameoftrees@openbsd.org Date: Sun, 16 Apr 2023 22:43:23 +1000 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 > GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68 -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68