Re: [PATCH v4 3/3] ls-files: add --deduplicate option

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

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/t/t3012-ls-files-dedup.sh b/t/t3012-ls-files-dedup.sh
> new file mode 100755
> index 00000000000..75877255c2c
> --- /dev/null
> +++ b/t/t3012-ls-files-dedup.sh
> @@ -0,0 +1,57 @@
> +#!/bin/sh
> +
> +test_description='git ls-files --deduplicate test'
> +
> +. ./test-lib.sh

We should already have a ls-files test so that we can add a handful
new tests to it, instead of dedicating a whole new test script.

Also, don't do everything in a single 'setup'.  There are various
scenarios you want to make sure ls-files to work (grep for ls-files
in the following you added---I count 4 of them), and when a future
developer touches the code, he or she may break one but not other
three.  The purpose you write tests is to protect your new feature
from such a developer *AND* help such a developer to debug and fix
his or her changes.  For that, it would be a lot more sensible to
have one set-up that is common, and then four separate tests.

> +test_expect_success 'setup' '
> +	>a.txt &&
> +	>b.txt &&
> +	>delete.txt &&
> +	git add a.txt b.txt delete.txt &&
> +	git commit -m master:1 &&

Needless use of the word "master".  Observe what is going on in the
project around you and avoid stepping other peoples' toes.  One of
the ongoing effort is to grep for the phrase master in t/ directory
and examine what happens when the default initial branch name
becomes something other than 'master', so adding a needless hit like
this is most unwelcome.

> +	echo a >a.txt &&
> +	echo b >b.txt &&
> +	echo delete >delete.txt &&
> +	git add a.txt b.txt delete.txt &&
> +	git commit -m master:2 &&

> +	git checkout HEAD~ &&
> +	git switch -c dev &&

Needless mixture of checkout/switch.  If you switch branches using
"git checkout", for example, consistently do so, i.e.

	git checkout -b dev HEAD~1 

It's not like these new tests are to test checkout and switch; your
mission is to protect "ls-files --dedup" feature here.

> +	test_when_finished "git switch master" &&
> +	echo change >a.txt &&
> +	git add a.txt &&
> +	git commit -m dev:1 &&

I'd consider all of the above to be 'setup' that is common for
subsequent tests.  It may make sense to actually do everything
on the initial branch, i.e. after creating two commits, do

	git tag tip &&
	git reset --hard HEAD^ &&
	echo change >a.txt &&
	git commit -a -m side &&
	git tag side

You are always on the initial branch without ever switching, so
there is no need for the when_finished stuff.

Then the first of your test is to show the index with conflicts.

> +	test_must_fail git merge master &&

This will become "git merge tip" instead of 'master'.

> +	git ls-files --deduplicate >actual &&
> +	cat >expect <<-\EOF &&
> +	a.txt
> +	b.txt
> +	delete.txt
> +	EOF
> +	test_cmp expect actual &&

And up to this point is the first test after 'setup'.

The next test should begin with:

	git reset --hard side &&
	test_must_fail git merge tip &&

so that even when the first test is skipped, or left unmerged,
you'll begin with a known state.

> +	rm delete.txt &&
> +	git ls-files -d -m --deduplicate >actual &&
> +	cat >expect <<-\EOF &&
> +	a.txt
> +	delete.txt
> +	EOF
> +	test_cmp expect actual &&
> +	git ls-files -d -m -t  --deduplicate >actual &&
> +	cat >expect <<-\EOF &&
> +	C a.txt
> +	C a.txt
> +	C a.txt
> +	R delete.txt
> +	C delete.txt
> +	EOF
> +	test_cmp expect actual &&
> +	git ls-files -d -m -c  --deduplicate >actual &&
> +	cat >expect <<-\EOF &&
> +	a.txt
> +	b.txt
> +	delete.txt
> +	EOF
> +	test_cmp expect actual &&

These three can be kept in the same test_expect_success, as they are
exercising read-only operation on the same state but with different
display options.

But in this case, the preparation is not too tedious (just a failed
merge plus a deletion), so you probably would prefer to split it
into 3 independent tests---that may make it more helpful to future
developers.

> +	git merge --abort
> +'
> +test_done