Re: [PATCH 5/5] t8020: fix test failure due to indeterministic tag sorting

[Toon Claes <toon@iotcl.com>]

Patrick Steinhardt <ps@pks.im> writes:

> In e6c06e87a2 (last-modified: fix bug when some paths remain unhandled,
> 2025-09-18), we have fixed a bug where under certain circumstances,
> git-last-modified(1) would BUG because there's still some unhandled
> paths. The fix claims that the root cause here is criss-cross merges,
> and it adds a test case that seemingly exercises this.
>
> Curiously, this test case fails on some systems because the actual
> output does not match our expectations:
>
>     diff --git a/expect b/actual
>     index 5271607..bdc620e 100644
>     --- a/expect
>     --- b/actual
>     @@ -1,3 +1,3 @@
>      km3 a
>     -k2 k
>     +km2 k
>      1 file
>     error: last command exited with $?=1
>     not ok 15 - last-modified with subdir and criss-cross merge
>
> The output we see is git-name-rev(1) with `--annotate-stdin`. What it
> does is to take the output of git-last-modified(1), which contains
> object IDs of the blamed commits, and convert those object IDs into the
> names of the corresponding tags. Interestingly, we indeed have both "k2"
> and "km2" as tags, and even more interestingly both of these tags point
> to the same commit. So the output we get isn't _wrong_, as the tags are
> ambiguous.

Ahha, I see...

> But why do both of these tags point to the same commit? "km2" really is
> supposed to be a merge, but due to the way the test is constructed the
> merge turns into a fast-forward merge.

Ohw :o

> Which means that the resulting
> does not even contain a criss-cross merge in the first place! A quick
> test though shows that the test indeed triggers the bug, so the initial
> analysis that the behaviour is triggered by such merges must be wrong.

I've tried various things, this felt like the simplest case to
reproduce. Apparantly not realizing it was wrong.

> And it is: seemingly, the issue isn't with criss-cross merges, but
> rather with a graph where different files in the same directory were
> modified on both sides of a merge.

Well yes, it felt like it was something like this. But like said, it
_seemed_ I needed a criss-cross merge.

> Refactor the test so that we explicitly test for this specific situation
> instead of mentioning the "criss-cross merge" red herring.

Well, thanks for this cleanup!

> As the test
> is very specific to the actual layout of the repository we also adapt it
> to use its own standalone repository.

I should have done that in the first place. Trying to shoe horn this
test in the existing repo might have guided me to take incorrect
conclusions.
> Note that this requires us to drop the `test_when_finished` call in
> `check_last_modified` because it's not supported to execute that
> function in a subshell.

This is not an issue because these files are always overwritten before
comparing, so this the `rm` wasn't strictly needed.

> This refactoring also fixes the original tag ambiguity that caused us to
> fail on some platforms.

Awesome! 

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t8020-last-modified.sh | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
> index e13aad1439..61f00bc15c 100755
> --- a/t/t8020-last-modified.sh
> +++ b/t/t8020-last-modified.sh
> @@ -33,7 +33,6 @@ check_last_modified() {
>  	done &&
>  
>  	cat >expect &&
> -	test_when_finished "rm -f tmp.*" &&
>  	git ${indir:+-C "$indir"} last-modified "$@" >tmp.1 &&
>  	git name-rev --annotate-stdin --name-only --tags \
>  		<tmp.1 >tmp.2 &&
> @@ -128,20 +127,25 @@ test_expect_success 'only last-modified files in the current tree' '
>  	EOF
>  '
>  
> -test_expect_success 'last-modified with subdir and criss-cross merge' '
> -	git checkout -b branch-k1 1 &&
> -	mkdir -p a k &&
> -	test_commit k1 a/file2 &&
> -	git checkout -b branch-k2 &&
> -	test_commit k2 k/file2 &&
> -	git checkout branch-k1 &&
> -	test_merge km2 branch-k2 &&
> -	test_merge km3 3 &&
> -	check_last_modified <<-\EOF
> -	km3 a
> -	k2 k
> -	1 file
> -	EOF
> +test_expect_success 'subdirectory modified via merge' '
> +	test_when_finished rm -rf repo &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit base &&
> +		git switch --create left &&
> +		mkdir subdir &&
> +		test_commit left subdir/left &&
> +		git switch --create right base &&
> +		mkdir subdir &&
> +		test_commit right subdir/right &&
> +		git switch - &&
> +		test_merge merge right &&
> +		check_last_modified <<-\EOF
> +		merge subdir
> +		base base.t
> +		EOF
> +	)
>  '

Looks good to me.

-- 
Cheers,
Toon