Re: [PATCH v2 2/2] builtin:ls-files.c:add git ls-file --dedup option

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jan 8, 2021 at 9:36 AM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> builtin:ls-files.c:add git ls-file --dedup option

This subject concisely explains the purpose of the patch. That's good.
A more typical way to write it would be:

    ls-files: add --dedup option

> This commit standardizes the code format.

Fixing problems pointed out by reviewers is good. Normally, however,
when you submit a new version of your patch or patch series, you
should apply these fixes directly to the patch(es) which introduced
the problems in the first place rather than adding one or more
additional patches to fix problems introduced in earlier patches. To
do this, you typically would use `git rebase -i` or `git commit
--amend` to squash the fixes into the problematic patches. Thus, when
you re-submit the patches, they will appear to be "perfect".

For this particular two-patch series, patch [2/2] is doing two things:
(1) fixing style problems from patch [1/2], and (2) adding
documentation and tests which logically belong with the feature added
by patch [1/2]. Taking the above advice into account, a better
presentation when you re-submit this series would be to squash these
two patches into a single patch.

> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> @@ -81,6 +82,9 @@ OPTIONS
> +--dedup::
> +       Suppress duplicates entries when conflicts happen or
> +       specify -d -m at the same time.

For consistency with typesetting elsewhere in this file, use backticks
around the command-line options. It also often is a good idea to spell
the options using long form since it is typically easier to search for
the long form of an option in documentation. So, perhaps the above can
be written like this:

    Suppress duplicate entries when `--deleted` and `--modified` are
    combined.

> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> -       const struct cache_entry *last_stage=NULL;
> +       const struct cache_entry *last_stage = NULL;
> -                       if(show_cached && delete_dup){
> +                       if (show_cached && delete_dup) {
> -                                       last_stage=ce;
> +                                       last_stage = ce;
> -                       if(delete_dup){
> +                       if (delete_dup) {
> -                       if(delete_dup && show_deleted && show_modified && err)
> +                       if (delete_dup && show_deleted && show_modified && err)
> -                       else{
> -                               if (show_deleted && err)/* you can't find it,so it's actually removed at all! */
> +                       else {
> +                               if (show_deleted && err)

As mentioned above, these style fixes should be squashed into the
first patch, rather than being done in a separate patch, so that
reviewers see a nicely polished patch rather than a patch which
requires later fixing up.

> diff --git a/t/t3012-ls-files-dedup.sh b/t/t3012-ls-files-dedup.sh
> @@ -0,0 +1,63 @@
> +test_expect_success 'master branch setup and write expect1 expect2 and commit' '

We usually give this test a simple title such as "setup" so that we
don't have to worry about the title becoming outdated as people make
changes to the test itself.

> +       touch a.txt &&
> +       touch b.txt &&
> +       touch delete.txt &&

On this project, we use `touch` when the timestamp of the empty files
is important to the test. If the timestamp is not important, then we
just use `>`, like this:

    >a.txt &&
    >b.txt &&
    >delete.txt &&

> +       cat <<-EOF >expect1 &&
> +       M a.txt
> +       H b.txt
> +       H delete.txt
> +       H expect1
> +       H expect2
> +       EOF
> +       cat <<-EOF >expect2 &&
> +       C a.txt
> +       R delete.txt
> +       EOF

When no variables are being interpolated in the here-doc content, we
use -\EOF to let readers know that the here-doc body is literal. So:

    cat >expect1 <<-\EOF &&
    ...
    EOF

> +       git add a.txt b.txt delete.txt expect1 expect2 &&
> +       git commit -m master:1
> +'
> +
> +test_expect_success 'main commit again' '
> +       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
> +'
> +
> +test_expect_success 'dev commit' '
> +       git checkout HEAD~ &&
> +       git switch -c dev &&
> +       echo change>a.txt &&
> +       git add a.txt &&
> +       git commit -m dev:1
> +'

These two tests following the "setup" test also seem to be doing setup
tasks rather than testing the new --dedup functionality. If this is
the case, then it probably would make sense to combine all three tests
into a single "setup" test.

> +test_expect_success 'dev merge master' '
> +       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
> +'

Do you foresee that people will add more tests to this file which will
use the files and branches set up by the "setup" test(s)? If not, if
those branches and files are only ever going to be used by this one
test, then it probably would be better to combine all the above code
into a single test.