From: Mark Jamsek <mark@jamsek.com>
Subject: Re: three bugs related to merge commits
To: James Cook <falsifian@falsifian.org>, gameoftrees@openbsd.org
Date: Mon, 17 Apr 2023 15:20:51 +1000

On 23-04-16 05:00PM, Stefan Sperling wrote:
> On Mon, Apr 17, 2023 at 12:38:41AM +1000, Mark Jamsek wrote:
> > On 23-04-17 12:02AM, Mark Jamsek wrote:
> > > On 23-04-16 10:43PM, Mark Jamsek wrote:
> > > > 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.
> > > > > 
> > > 
> > > In regards to the first two bugs, the second branch and merge commit
> > > appear to be a red herring: simply committing to main in another work
> > > tree has the same effect:
> > 
> > No luck debugging this yet, but here's a quick regress test:
> 
> Thanks!
> Below is a fix with your test adjusted to expect correct behaviour.

Nice! 2-for-1. This fixes the first two issues reported by James :)

ok

> The bug is that the first parent's object ID was derived from the work
> tree's base commit. It should be derived from the branch tip commit.
> The comitted tree is already based on the branch tip's tree which is
> why the added file did not disappear.

You never cease to amaze how you manage to keep all this in your head!
I don't touch the code for a month and forget so many even basic Got
procedures. For example, misremembering only some--not all--operations
require a consistent base commit for all files in the tree. Instead it's
replaced with what I've been studying. It takes me days to properly
context switch; I find it difficult to work effectively on different
things concurrently.

> It is unfortunate that the test suite didn't already catch such a basic
> problem, but here we are. All other tests are still passing for me.

Yes, true. I'm surprised I hadn't stumbled on this because I often have
multiple work trees. I must have been (un)lucky to have not triggered
it sooner. In any case, we have coverage now. Thank you for the fix!

> diff /home/stsp/src/got
> commit - c736b84ab8efb53399d58afe57a2e40c4c7dd1b5
> path + /home/stsp/src/got
> blob - df29e65f90431310315474962e5c738ec544f38d
> file + lib/worktree.c
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -6252,7 +6252,7 @@ commit_worktree(struct got_object_id **new_commit_id,
>  	if (err)
>  		goto done;
>  
> -	err = got_object_qid_alloc(&pid, worktree->base_commit_id);
> +	err = got_object_qid_alloc(&pid, head_commit_id);
>  	if (err)
>  		goto done;
>  	STAILQ_INSERT_TAIL(&parent_ids, pid, entry);
> blob - 26a31c050dfc034e8c024f4e945dafbc2d72cf28
> file + regress/cmdline/commit.sh
> --- regress/cmdline/commit.sh
> +++ regress/cmdline/commit.sh
> @@ -2077,6 +2077,41 @@ test_parseargs "$@"
>  	test_done "$testroot" "$ret"
>  }
>  
> +test_commit_from_different_worktrees() {
> +	local testroot=$(test_init commit_from_different_worktrees)
> +
> +	got checkout $testroot/repo $testroot/wt > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	got checkout $testroot/repo $testroot/wt2 > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo "new file" > $testroot/wt2/new
> +	(cd $testroot/wt2 && got add new >/dev/null)
> +	(cd $testroot/wt2 && got commit -m 'add new file from wt2' > \
> +	    $testroot/stdout)
> +	local wt2_head_id=$(git_show_head $testroot/repo)
> +
> +	echo "modified alpha" > $testroot/wt/alpha
> +	(cd $testroot/wt && got commit -m 'mod alpha in wt' > $testroot/stdout)
> +	local wt1_parent_id=$(git_show_parent_commit $testroot/repo)
> +
> +	if [ $wt2_head_id != $wt1_parent_id ]; then
> +		echo "commit lost from different work tree" >&2
> +		test_done "$testroot" "1"
> +	fi
> +
> +	test_done "$testroot" "0"
> +}
> +
>  test_parseargs "$@"
>  run_test test_commit_basic
>  run_test test_commit_new_subdir
> @@ -2108,3 +2143,4 @@ run_test test_commit_logmsg_ref
>  run_test test_commit_gitignore
>  run_test test_commit_bad_author
>  run_test test_commit_logmsg_ref
> +run_test test_commit_from_different_worktrees
> 

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