Re: [PATCH v3] ls-files.c: add --dedup option

>
> On Thu, Jan 14, 2021 at 7:22 AM 阿德烈 via GitGitGadget
> <gitgitgadget@gmail.com> 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@gmail.com>
> > ---
> > 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.)