From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got log -b path gets confused by merge commits
To: James Cook <falsifian@falsifian.org>
Cc: gameoftrees@openbsd.org
Date: Thu, 1 Feb 2024 20:52:54 +0100

On Thu, Feb 01, 2024 at 06:54:48PM +0000, James Cook wrote:
> On Mon, Jan 29, 2024 at 03:07:43PM +0100, Stefan Sperling wrote:
> > On Sun, Jan 28, 2024 at 06:34:17PM +0000, James Cook wrote:
> > > I've managed to trigger the error
> > > 
> > > 	got: //d/g: no such entry found in tree
> > > 
> > > by running got log -b d/g or tog log -b d/g in a specially crafted repo.
> > 
> > Thanks! This is an edge case I had overlooked.
> > Below is a fix which I tested manually. Does it work for you?
> > 
> > The patch repeats an if-condition in a slightly awkward way because the
> > diff becomes easier to read, avoiding indentation of a lot of lines by
> > one more level. Essentially we need to guard the STAILQ_FOREACH loop
> > with if (merged_id != NULL).
> > 
> > > I have a script (at bottom) to reproduce and am happy to turn that into a
> > > patch for regress/cmdline.
> > 
> > Yes, please do turn it into a regression test if you have time.
> 
> Thanks, your patch fixes the problem for me, including in the original
> private repo where I discovered it.

Thanks, committed.

> Comparing the output of "got log -b path" with "git log path", I notice got
> likes to show some merge commits that git skips over, but that doesn't seem
> like a big problem.
> 
> Below is a patch adding a test case to log.sh based on my repro script. I
> named the test ..._corner_case because it's pretty specific. I followed the
> pattern I see of using git commands instead of got commands to set up the
> repo.

Ok for this test, thanks! Fails as expected without the fix in place,
and passes with the fix applied.

"Corner case" sounds too generic as many test cases check corner cases.
I would suggest a name that contains some of the relevant terms like
"test_log_merge_commit_nonexistent_path" or similar.


> 
> -- 
> James
> 
> 
> diff fc9b745fd3b1a01f7e89f269600db36ad5222b3e 88456a98463ce990d4f4df3c98274b43fd148eda
> commit - fc9b745fd3b1a01f7e89f269600db36ad5222b3e
> commit + 88456a98463ce990d4f4df3c98274b43fd148eda
> blob - 4721d314c761fef6ff7f1dfe3ff55d52e6d39216
> blob + 2382dfe2d37e40510f2213eafd7c5177407f3df6
> --- regress/cmdline/log.sh
> +++ regress/cmdline/log.sh
> @@ -769,6 +769,60 @@ test_log_changed_paths() {
>  	test_done "$testroot" "$ret"
>  }
> +test_log_merge_commit_corner_case() {
> +	local testroot=`test_init log_merge_commit_corner_case 1`
> +
> +        # Create the following commit graph (most recent commit shown first):
> +        #
> +        #   o  create dir/beta
> +        #   |
> +        #   o  merge (does not touch dir)
> +        #  / \
> +        # o   o  changes which don't touch the directory "dir"
> +        #  \ /
> +        #   o  initial commit, which includes directory "dir" but not dir/beta
> +
> +
> +	mkdir $testroot/repo/dir
> +	touch $testroot/repo/dir/alpha
> +	git -C $testroot/repo add dir/alpha
> +	git_commit $testroot/repo -m "initial commit"
> +
> +	git -C $testroot/repo checkout -q -b aux
> +	touch $testroot/repo/gamma
> +	git -C $testroot/repo add gamma
> +	git_commit $testroot/repo -m "change on aux"
> +
> +	git -C $testroot/repo checkout -q master
> +	touch $testroot/repo/delta
> +	git -C $testroot/repo add delta
> +	git_commit $testroot/repo -m "change on master"
> +
> +	git -C $testroot/repo merge -q -m "merge" aux
> +
> +	touch $testroot/repo/dir/beta
> +	git -C $testroot/repo add dir/beta
> +	git_commit $testroot/repo -m "add beta"
> +
> +	head_commit=`git_show_head $testroot/repo`
> +
> +	got log -r $testroot/repo -b dir/beta | grep ^commit > $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "log command failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	echo "commit $head_commit (master)" > $testroot/stdout.expected
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_log_submodule() {
>  	local testroot=`test_init log_submodule`
> @@ -1201,6 +1255,7 @@ run_test test_log_end_at_commit
>  run_test test_log_reverse_display
>  run_test test_log_in_worktree_different_repo
>  run_test test_log_changed_paths
> +run_test test_log_merge_commit_corner_case
>  run_test test_log_submodule
>  run_test test_log_diffstat
>  run_test test_log_commit_keywords
> 
>