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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 16, 2021, 07:13 UTC
Message-ID: <CAPig+cQBi7jdq64==U630Ht1YDcH+9komLNv-hZMnEhQ1Q-V9A@mail.gmail.com>
In-Reply-To: <pull.832.v3.git.1610626942677.gitgitgadget@gmail.com>

On Thu, Jan 14, 2021 at 7:22 AM 阿德烈 via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 5 quoted lines

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

Show 17 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 9 quoted lines

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