On Thu, Jan 14, 2021 at 7:22 AM 阿德烈 via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
> In order to provide users a better experience
> when viewing information about files in the index
> and the working tree, the `--dedup` option will suppress
> some duplicate options under some conditions.
> [...]
I have a few very minor comments alongside Junio's review comments...
> Signed-off-by: ZheNing Hu <adlternative@xxxxxxxxx>
> ---
> diff --git a/t/t3012-ls-files-dedup.sh b/t/t3012-ls-files-dedup.sh
> @@ -0,0 +1,54 @@
> +test_description='git ls-files --dedup test.
> +
> +This test prepares the following in the cache:
> +
> + a.txt - a file(base)
> + a.txt - a file(master)
> + a.txt - a file(dev)
> + b.txt - a file
> + delete.txt - a file
> + expect1 - a file
> + expect2 - a file
> +
> +'
This test script description is outdated now. Perhaps shorten it to:
test_description='ls-files dedup tests'
Or, it might be suitable to simply add the new test to the existing
t3004-ls-files-basic.sh instead.
> +test_expect_success 'setup' '
> + > a.txt &&
> + > b.txt &&
> + > delete.txt &&
> + cat >expect1<<-\EOF &&
Style nits: no space after redirection operator and a space before
redirection operator:
>a.txt &&
>b.txt &&
>delete.txt &&
cat >expect1 <<-\EOF &&
> + cat >expect2<<-EOF &&
Nit: missing the backslash (and wrong spacing):
cat >expect2 <<-\EOF &&
> + echo a>a.txt &&
> + echo b>b.txt &&
Style:
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 &&
If someone adds a new test after this test, then that new test will
run in the "dev" branch, which might be unexpected or undesirable. It
often is a good idea to ensure that tests do certain types of cleanup
to avoid breaking subsequent tests. Here, it would be a good idea to
ensure that the test switches back to the original branch when it
finishes (regardless of whether it finishes successfully or
unsuccessfully).
git switch -c dev &&
test_when_finished "git switch master" &&
Or you could use `git switch -` if you don't want to hard-code the
name "master" in the test (since there has been effort lately to
remove that name from tests.
> + echo change >a.txt &&
> + git add a.txt &&
> + git commit -m dev:1 &&
> + test_must_fail git merge master &&
> + git ls-files -t --dedup >actual1 &&
> + test_cmp expect1 actual1 &&
> + rm delete.txt &&
> + git ls-files -d -m -t --dedup >actual2 &&
> + test_cmp expect2 actual2
We usually don't bother giving temporary files unique names like
"actual1" and "actual2" unless those files must exist at the same
time. This is because unique names like this may confuse readers into
wondering if there is some hidden interdependency between the files.
In this case, the files don't need to exist at the same time, so it
may be better simply to use the names "actual" and "expect", like
this:
...other stuff...
cat >expect <<-\EOF &&
...
EOF
git ls-files -t --dedup >actual &&
test_cmp expect actual &&
rm delete.txt &&
cat >expect <<-\EOF &&
...
EOF
git ls-files -d -m -t --dedup >actual &&
test_cmp expect actual
(It also has the benefit that the "expect" content is closer to the
place where it is actually used, which may make it a bit easier for a
person reading the test to understand what is supposed to be
produced.)
