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