Re: [PATCH v5] t2000: modernize overall structure and path checks

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 7, 2026, 16:10 UTC
Message-ID: <xmqqzf3e69at.fsf@gitster.g>
In-Reply-To: <xmqqmrze7sj9.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> The description of this v5 patch looks suspiciously similar, as its
> patch text, so I suspect it won't apply to my tree.

So, I applied this "v5" to a slightly older 'master', immediately before the za/t2000-modernise topic was merged, and compared the result with what we already have in 'master'.

    $ git log -1 --oneline 0713d3b7f6
    0713d3b7f6 Merge branch 'za/t2000-modernise'
    $ git checkout 0713d3b7f6~1
    $ git am patch-v5.mbox
    $ git diff 0713d3b7f6 HEAD

There are some things that are better, and some that do not look improvements.

Show 32 quoted lines

> diff --git a/t/t2000-conflict-when-checking-files-out.sh b/t/t2000-conflict-when-checking-files-out.sh
> index af199d8191..44728329f3 100755
> --- a/t/t2000-conflict-when-checking-files-out.sh
> +++ b/t/t2000-conflict-when-checking-files-out.sh
> @@ -35,19 +35,18 @@ show_files() {
>  	sed -e 's/^\([0-9]*\)	[^ ]*	[0-9a-f]*	/tr: \1 /'
>  }
>  
> -test_expect_success 'prepare files path0 and path1/file1' '
> -	date >path0 &&
> -	mkdir path1 &&
> -	date >path1/file1 &&
> -	git update-index --add path0 path1/file1
> -'
> +date >path0
> +mkdir path1
> +date >path1/file1
>  
> -test_expect_success 'prepare working tree files with D/F conflicts' '
> -	rm -fr path0 path1 &&
> -	mkdir path0 &&
> -	date >path0/file0 &&
> -	date >path1
> -'
> +test_expect_success \
> +    'git update-index --add various paths.' \
> +    'git update-index --add path0 path1/file1'
> +
> +rm -fr path0 path1
> +mkdir path0
> +date >path0/file0
> +date >path1

All of the above look regression to me, for the purpose of "modernization" effort. We want the steps to prepare for tests (e.g., creation of test files and directories and preparation of their contents), the steps of actual tests (e.g., running git commands and ensuring that they succeed or fail as expected), and the steps to verify the results, all contained inside a single test_expect_success for each step of the test.

On the other hand, the below looks like moving things in a better direction. The original has a logically single test split into multiple pieces and code to debug tests sprinkled all over, like ...

Show 16 quoted lines

> @@ -83,59 +82,22 @@ test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' '
>  # path path3 is occupied by a non-directory.  With "-f" it should remove
>  # the symlink path3 and create directory path3 and file path3/file1.
>  
> -test_expect_success 'prepare path2/file0 and index' '
> +test_expect_success 'checkout-index -f resolves symlink conflict on leading path' '
>  	mkdir path2 &&
>  	date >path2/file0 &&
> -	git update-index --add path2/file0
> -'
> -
> -test_expect_success 'write tree with path2/file0' '
> -	tree1=$(git write-tree)
> -'
> -
> -test_debug 'show_files $tree1'

... this one. And consolidating them into a single logical piece may make sense.

But it seems not quite complete and needs a bit more cleaning. For example, ...

Show 31 quoted lines

> -test_expect_success 'prepare path3/file1 and index' '
> +	git update-index --add path2/file0 &&
> +	tree1=$(git write-tree) &&
>  	mkdir path3 &&
>  	date >path3/file1 &&
> -	git update-index --add path3/file1
> -'
> -
> -test_expect_success 'write tree with path3/file1' '
> -	tree2=$(git write-tree)
> -'
> -
> -test_debug 'show_files $tree2'
> -
> -test_expect_success 'read previously written tree and checkout.' '
> +	git update-index --add path3/file1 &&
> +	tree2=$(git write-tree) &&
>  	rm -fr path3 &&
>  	git read-tree -m $tree1 &&
> -	git checkout-index -f -a
> -'
> -
> -test_debug 'show_files $tree1'
> -
> -test_expect_success 'add a symlink' '
> -	test_ln_s_add path2 path3
> -'
> -
> -test_expect_success 'write tree with symlink path3' '
> -	tree3=$(git write-tree)
> -'

... we used to write out $tree3 here only because ...

> -
> -test_debug 'show_files $tree3'

... we use it to debug that tree here. In the updated version, we still write out ...

Show 7 quoted lines

> -# Morten says "Got that?" here.
> -# Test begins.
> -
> -test_expect_success 'read previously written tree and checkout.' '
> +	git checkout-index -f -a &&
> +	test_ln_s_add path2 path3 &&
> +	tree3=$(git write-tree) &&

... the same tree3 here, but because we lost the test debug, we no longer use the resulting tree object name.

Show 11 quoted lines

>  	git read-tree $tree2 &&
> -	git checkout-index -f -a
> -'
> -
> -test_debug 'show_files $tree2'
> -
> -test_expect_success 'checking out conflicting path with -f' '
> +	git checkout-index -f -a &&
>  	test_path_is_dir_not_symlink path2 &&
>  	test_path_is_dir_not_symlink path3 &&
>  	test_path_is_file_not_symlink path2/file0 &&

I didn't go through the updated version with fine toothed comb, so there may be other "why is this thing left?" and/or "this update changes what is being tested, no?" gotchas that I missed.

In any case, can you update the patch so that it applies cleanly to a more recent "master" to resurrect the good bits out of what you have?

Thanks.