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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2021, 23:34 UTC
Message-ID: <xmqqbldnkuja.fsf@gitster.c.googlers.com>
In-Reply-To: <0c7830d07db0aa1ec055b97de52bd873d05e3ab1.1610856136.git.gitgitgadget@gmail.com>

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

Show 11 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 7 quoted lines

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

Show 23 quoted lines

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