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

[Junio C Hamano <gitster@pobox.com>]

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.

> 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 ...

> @@ -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, ...

> -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 ...

> -# 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.

>  	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.