builtin/ls-files.c:add git ls-file --dedup option

[...]
Therefore, I added the --dedup option to git ls-files.
1. It can be achieved that only the deleted file name is displayed when using -m, -d, and --dedup at the same time.
2. Add --dedup when merging branches to remove duplicate file names. (unless -s, -u are used)

Signed-off-by ZheNing Hu <[email protected]>

---
 builtin/ls-files.c | 43 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 38 insertions(+), 5 deletions(-)

Quoted 1 line

+ if (show_deleted && err)/* you can't find it,so it's actually removed at all! */

Add space before /* comment */.
Add space in "...it, so...".

builtin:ls-files.c:add git ls-file --dedup option

This commit standardizes the code format.

builtin/ls-files.c +2 -2

-       const struct cache_entry *last_stage=NULL;
+       const struct cache_entry *last_stage = NULL;
-                       if(show_cached && delete_dup){
+                       if (show_cached && delete_dup) {

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

t/t3012-ls-files-dedup.sh +1 -0

+       cat <<-EOF >expect1 &&

t/t3012-ls-files-dedup.sh +9 -0

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

Quoted 1 line · ZheNing Hu via GitGitGadget

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

Quoted 1 line

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.

Documentation/git-ls-files.txt OPTIONS · line 84

Quoted 7 lines

Signed-off-by ZheNing Hu <[email protected]>

---

Documentation/git-ls-files.txt +3 -0

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

Quoted 5 lines

builtin/ls-files.c +2 -2

-       const struct cache_entry *last_stage=NULL;
+       const struct cache_entry *last_stage = NULL;
-                       if(show_cached && delete_dup){
+                       if (show_cached && delete_dup) {

builtin/ls-files.c

Quoted 10 lines

builtin/ls-files.c +5 -5

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

t/t3012-ls-files-dedup.sh line 1

Quoted 3 lines

t/t3012-ls-files-dedup.sh +1 -0

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

t/t3012-ls-files-dedup.sh line 22

Quoted 3 lines

t/t3012-ls-files-dedup.sh +3 -0

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

Quoted 3 lines

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

t/t3012-ls-files-dedup.sh line 33

Quoted 11 lines

t/t3012-ls-files-dedup.sh +11 -0

+       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

Quoted 9 lines

t/t3012-ls-files-dedup.sh +9 -0

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

t/t3012-ls-files-dedup.sh line 52

Quoted 10 lines

t/t3012-ls-files-dedup.sh +10 -0

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

t/t3012-ls-files-dedup.sh line 61

Quoted 8 lines

t/t3012-ls-files-dedup.sh +8 -0

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

From: ZheNing Hu <[email protected]>

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.

In a merge conflict, one item of "git ls-files" output may appear multiple times. For example,now the file a.c has a conflict,a.c will appear three times in the output of "git ls-files".We can use "git ls-files --dedup" to output a.c only one time.(unless --stage or --unmerged is used to view all the detailed information in the index)

In addition, if you use both --delete and --modify in the same time, The --dedup option can also suppress modified

entries output.

--dedup option relevant descriptions in
Documentation/git-ls-files.txt,

the test script in t/t3012-ls-files-dedup.sh prove the correctness of the --dedup option.

Quoted 7 lines · 阿德烈 via GitGitGadget

From: ZheNing Hu <[email protected]>

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.

Quoted 6 lines

In a merge conflict, one item of "git ls-files" output may appear multiple times. For example,now the file a.c has a conflict,a.c will appear three times in the output of "git ls-files".We can use "git ls-files --dedup" to output a.c only one time.(unless --stage or --unmerged is used to view all the detailed information in the index)

Unlike these option names we see in the description, "dedup" is not a full word. Perhaps spell it fully "--deduplicate" while letting parse-options machinery to accept unique prefix (including "--dedup"?

Quoted 2 lines

In addition, if you use both --delete and --modify in the same time, The --dedup option can also suppress modified

"at the same time", I think.

Quoted 1 line

entries output.

[let's call this point "point A"]

Quoted 2 lines

--dedup option relevant descriptions in
Documentation/git-ls-files.txt,

I am not sure what this means.

Quoted 2 lines

the test script in t/t3012-ls-files-dedup.sh prove the correctness of the --dedup option.

No amount of tests "proves" any correctness, but that is OK. I think you meant to say "a few tests have been added to t3012 to protect the new feature from future breakage" or something like that.

In any case, I think everything after "point A" and before your sign off does not belong to the log message. The diffstat shows that documentation and tests have been added already.

Quoted 2 lines

+--dedup::
+     Suppress duplicate entries when conflict happen

"conflict happen" -> "there are unmerged paths", as the term "unmerged" is already shown to readers of "ls-files --help".

Quoted 1 line

+ or --deleted and --modified are combined.

I somehow thought that you refrained from deduping when you are showing the stages with "ls-files -u" and "ls-files -s", or you are showing status with "ls-files -t", because you will otherwise lose information. In other words, showing only one cache entry out of many that share the same name makes sense only when we are showing name and nothing else.

Having said all that, I suspect that we may be much better off if we can somehow merge the two loops into one. You may be dedup adjacent entries in each loop separately with the approach taken by this patch, but I do not think the patch would work to deduplicate across two loops. For example, what happens if you do this?

    $ git reset --hard
    $ echo >>builtin/ls-files.c
    $ git ls-files -c -m builtin/ls-files.c
    $ git ls-files -t -c -m builtin/ls-files.c

I think you see the path twice in the output, with or without your --dedup option (remember what I said about proving, by the way? ;-)).

#2: consolidate two for loops into one.

The two loops have slightly different condition to skip a ce, and different logic on what tag each path is shown with. When --cached and --modified or --deleted are asked for at the same time, we'd show them multiple times (this is done inside the loop for each ce)

if (show_cached || show_stage)
show_ce(... ce_stage(ce) ? tag_unmerged : ...);
err = lstat(fullname.buf, &st);
if (err) {
/* deleted? */
... code that corresponds to the
... illustration in #1 above come here.
} else if (...)
show_ce(..., tag_modified);

This changes the semantics. The original iterates the index twice, so you may see the same entry from --cached once and then again from --modified. The updated one still will show the same entry twice but next to each other.

#3: optionally deduplicate.

Once we have a single loop, deduplicationg based on names is trivial, as we seen before.

Hmm?

Quoted 7 lines · 阿德烈 via GitGitGadget

From: ZheNing Hu <[email protected]>

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.

Quoted 6 lines

In a merge conflict, one item of "git ls-files" output may appear multiple times. For example,now the file a.c has a conflict,a.c will appear three times in the output of "git ls-files".We can use "git ls-files --dedup" to output a.c only one time.(unless --stage or --unmerged is used to view all the detailed information in the index)

Unlike these option names we see in the description, "dedup" is not a full word. Perhaps spell it fully "--deduplicate" while letting parse-options machinery to accept unique prefix (including "--dedup"?

Quoted 2 lines

In addition, if you use both --delete and --modify in the same time, The --dedup option can also suppress modified

"at the same time", I think.

Quoted 1 line

entries output.

[let's call this point "point A"]

Quoted 2 lines

--dedup option relevant descriptions in
Documentation/git-ls-files.txt,

I am not sure what this means.

Quoted 2 lines

the test script in t/t3012-ls-files-dedup.sh prove the correctness of the --dedup option.

No amount of tests "proves" any correctness, but that is OK. I think you meant to say "a few tests have been added to t3012 to protect the new feature from future breakage" or something like that.

In any case, I think everything after "point A" and before your sign off does not belong to the log message. The diffstat shows that documentation and tests have been added already.

Quoted 2 lines

+--dedup::
+     Suppress duplicate entries when conflict happen

"conflict happen" -> "there are unmerged paths", as the term "unmerged" is already shown to readers of "ls-files --help".

Quoted 1 line

+ or --deleted and --modified are combined.

I somehow thought that you refrained from deduping when you are showing the stages with "ls-files -u" and "ls-files -s", or you are showing status with "ls-files -t", because you will otherwise lose information. In other words, showing only one cache entry out of many that share the same name makes sense only when we are showing name and nothing else.

Has that been changed from the previous rounds?

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 325

Quoted 13 lines

builtin/ls-files.c +5 -0

index c8eae899b82..bc4eded19ab 100644
@@ -316,6 +318,20 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
              for (i = 0; i < repo->index->cache_nr; i++) {
                      const struct cache_entry *ce = repo->index->cache[i];
 
+                     if (show_cached && delete_dup) {
+                             switch (ce_stage(ce)) {
+                             case 0:
+                             default:
+                                     break;

This part looks somewhat strange for two reasons:

- The code enumerates ALL the possible stage numbers from 0 to 3; if we were to have "default", I'd expect it would be a separate switch arm from the possible values that calls out an programming error, e.g. BUG("at stage #%d???", ce_stage(ce)). Simply removing the "default" arm would be another way out of this strangeness.

- When we see a stage #0 entry, we know we will not have higher stage entries with the same name. Not clearing last_stage here feels wrong, as the primary reason why last_stage variable is used is to remember the last ce that was shown, so that other entries with the same name can be skipped.

By the way, "last_shown_ce" may be a much better name for the variable, as you do not really care what stage number the ce you showed last was at (you care about its name).

Also, I do not see a good reason why the last_shown_ce variable should have lifetime longer than the block that contains this for() loop (and the other for loop for deleted/modified codepath we see later). Especially since you initialize the variable that you made visible to the entire function to NULL before entering the first for loop, but you do not set it back to NULL before entering the second for loop, it is inviting a subtle bug. You may have been given show_cached and show_modified at the same time, so you enter the first loop and have shown the first stage of the last conflicted path, whose cache entry is left in the last_stage variable. Since the variable has longer lifespan than necessary, when the second loop is entered, it still points at the cache entry for the highest stage of the last conflicted path. That is because the code forgets to clear it to NULL before entering the second for loop.

Having said all that, I suspect that we may be much better off if we can somehow merge the two loops into one. You may be dedup adjacent entries in each loop separately with the approach taken by this patch, but I do not think the patch would work to deduplicate across two loops. For example, what happens if you do this?

    $ git reset --hard
    $ echo >>builtin/ls-files.c
    $ git ls-files -c -m builtin/ls-files.c
    $ git ls-files -t -c -m builtin/ls-files.c

I think you see the path twice in the output, with or without your --dedup option (remember what I said about proving, by the way? ;-)).

Once we successfully merged two loops into one, the part that shows tracked paths in the function would have only one loop, and it would become a lot cleaner to add the logic to "skip showing the ce if it has the same name as the previously shown one, only when doing so won't lose information", by doing something like this:

        static void show_files(....)
        {
                /* show_others || show_killed done here */
                ...

                /* leave early if not showing anything */
                if (! (show_cached || show_stage || show_deleted || show_modified))
                        return;

                last_shown_ce = NULL;
                for (i = 0; i < repo->index->cache_nr; i++) {
                        const struct cache_entry *ce = repo->index->cache[i];

                        if (skipping_duplicates && last_shown_ce)
                                if (!strcmp(ce->name, last_shown_ce->name))
                                        continue;

                        construct_fullname();

                        /* various reasons to skip the entry tested */
                        if (showing ignored directory and ce is excluded)
                                continue;
                        if (show_unmerged && !ce_stage(ce))
                                continue;
                        if (ce->ce_flags & CE_UPDATE)
                                continue;
                        ... other reasons may appear here ...

                        /* now we are committed to show it */
                        last_shown_ce = ce;

                        ... various different ways to show ce come here ...
                        show_ce(...);
                }
        }

where "skipping_duplicates" would be set when "--deduplicate" is asked and we are not showing information other than the pathname via various options e.g. the tags (-t) or stages (-s/-u).

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 381

Quoted 2 lines

builtin/ls-files.c +1 -0

+                     if (delete_dup && show_deleted && show_modified && err)
                              show_ce(repo, dir, ce, fullname.buf, tag_removed);

I actually think the original code that is still shown here ...

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 385

Quoted 4 lines

builtin/ls-files.c +4 -0

+                     else {
+                             if (show_deleted && err)
+                                     show_ce(repo, dir, ce, fullname.buf, tag_removed);
+                             if (show_modified && ie_modified(repo->index, ce, &st, 0))

... about modified file is buggy. If lstat() failed, then &st has no useful information, so it is wrong to feed it to ie_modified().

Perhaps a three-patch series that is structured like this may be in order?

#1: bugfix for --deleted and --modified.

        err = lstat(fullname.buf, &st);
        if (err) {
                /* deleted? */
                if (errno != E_NOENT)
                        error_errno("cannot lstat '%s'", fullname.buf);
                else {
                        if (show_deleted)
                                show_ce(..., tag_removed);
                        if (show_modified)
                                show_ce(..., tag_modified);
                }
        } else if (show_modified && ie_modified(...))
                show_ce(..., tag_modified);

     This hopefully should not change the semantics.  If you ask
     --deleted and --modified, a deleted path would be listed twice.

#2: consolidate two for loops into one.

The two loops have slightly different condition to skip a ce, and different logic on what tag each path is shown with. When --cached and --modified or --deleted are asked for at the same time, we'd show them multiple times (this is done inside the loop for each ce)

if (show_cached || show_stage)
show_ce(... ce_stage(ce) ? tag_unmerged : ...);
err = lstat(fullname.buf, &st);
if (err) {
/* deleted? */
... code that corresponds to the
... illustration in #1 above come here.
} else if (...)
show_ce(..., tag_modified);

This changes the semantics. The original iterates the index twice, so you may see the same entry from --cached once and then again from --modified. The updated one still will show the same entry twice but next to each other.

#3: optionally deduplicate.

Once we have a single loop, deduplicationg based on names is trivial, as we seen before.

Hmm?

Here I may disagree with your point of view:

                 if (errno != E_NOENT)
                         error_errno("cannot lstat '%s'", fullname.buf);

With this sentence included, the patch will fail the test:
t/t3010-ls-files-killed-modified.sh.
the errno maybe ENOTDIR when you try to lstat a filer with lstat("r/f",&st); So I temporarily removed the judgment of errno.

Quoted 7 lines

#2: consolidate two for loops into one.
...
This changes the semantics. The original iterates the index twice, so you may see the same entry from --cached once and then again from --modified. The updated one still will show the same entry twice but next to each other.

Well,This does change the semantics. I think people who used two for loops before may want to separate different outputs. Now, if you don’t use "--deduplicate", You may see six consecutive items under a combination of multiple options.

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

Signed-off-by ZheNing Hu <[email protected]>

---

t/t3012-ls-files-dedup.sh +3 -0

@@ -0,0 +1,54 @@
+test_description='git ls-files --dedup test.
+
+This test prepares the following in the cache:

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

+ cat >expect2<<-EOF &&

Quoted 5 lines · 阿德烈 via GitGitGadget

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

Quoted 7 lines

Signed-off-by ZheNing Hu <[email protected]>

---

t/t3012-ls-files-dedup.sh +3 -0

@@ -0,0 +1,54 @@
+test_description='git ls-files --dedup test.
+
+This test prepares the following in the cache:

t/t3012-ls-files-dedup.sh line 13

Quoted 10 lines

t/t3012-ls-files-dedup.sh +10 -0

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

t/t3012-ls-files-dedup.sh line 23

Quoted 5 lines

t/t3012-ls-files-dedup.sh +5 -0

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

Quoted 3 lines

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

    cat >expect1 <<-\EOF &&

Quoted 1 line

+ cat >expect2<<-EOF &&

Nit: missing the backslash (and wrong spacing):

    cat >expect2 <<-\EOF &&

t/t3012-ls-files-dedup.sh line 37

Quoted 2 lines

t/t3012-ls-files-dedup.sh +2 -0

+       echo a>a.txt &&
+       echo b>b.txt &&

Style:

    echo a >a.txt &&
    echo b >b.txt &&

t/t3012-ls-files-dedup.sh line 42

Quoted 5 lines

t/t3012-ls-files-dedup.sh +5 -0

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

t/t3012-ls-files-dedup.sh line 51

Quoted 9 lines

t/t3012-ls-files-dedup.sh +9 -0

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

Quoted 13 lines · Eric Sunshine <[email protected]> 于2021年1月16日周六 下午3:13写道：

Quoted 1 line

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

Quoted 4 lines

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.

I have little confuse about I can use test_when_finished "git switch master" , but I can't use test_when_finished "git switch -" ,
why？

Quoted 18 lines · 胡哲宁

Quoted 17 lines · Eric Sunshine <[email protected]> 于2021年1月16日周六 下午3:13写道：

Quoted 1 line

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

Quoted 3 lines

I have little confuse about I can use test_when_finished "git switch master" , but I can't use test_when_finished "git switch -" ,
why？

You may use either one. I presented both as alternative approaches.

Quoted 12 lines · Eric Sunshine

Quoted 10 lines · 胡哲宁

Quoted 6 lines · Eric Sunshine <[email protected]> 于2021年1月16日周六 下午3:13写道：

    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.

I have little confuse about I can use test_when_finished "git switch master" , but I can't use test_when_finished "git switch -" ,
why？

Quoted 1 line

You may use either one. I presented both as alternative approaches.

I am sensing a bit of miscommunication here. You sound like you still believe either would work OK, but to me, it sounds like that the author claims the one of them does not work for him/her.

From: ZheNing Hu <[email protected]>

This situation may occur in the original code: lstat() failed but we use &st to feed ie_modified() later.

It's buggy!

Therefore, we can directly execute show_ce without the judgment of ie_modified() when lstat() has failed.

Signed-off-by ZheNing Hu <[email protected]>

---
 builtin/ls-files.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

builtin/ls-files.c +6 -3

index c8eae899b82..6f97a23c2dc 100644
@@ -347,9 +347,12 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 			if (ce_skip_worktree(ce))
 				continue;
 			err = lstat(fullname.buf, &st);
-			if (show_deleted && err)
-				show_ce(repo, dir, ce, fullname.buf, tag_removed);
-			if (show_modified && ie_modified(repo->index, ce, &st, 0))
+			if (err) {
+					if (show_deleted)
+						show_ce(repo, dir, ce, fullname.buf, tag_removed);
+					if (show_modified)
+						show_ce(repo, dir, ce, fullname.buf, tag_modified);
+			}else if (show_modified && ie_modified(repo->index, ce, &st, 0))
 				show_ce(repo, dir, ce, fullname.buf, tag_modified);
 		}
 	}

t/t3012-ls-files-dedup.sh new file +13 -0

+	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

t/t3012-ls-files-dedup.sh new file +3 -0

+	git merge --abort
+'
+test_done

t/t3012-ls-files-dedup.sh line 5

Quoted 11 lines · ZheNing Hu via GitGitGadget

t/t3012-ls-files-dedup.sh new file +5 -0

new file mode 100755
index 00000000000..75877255c2c
@@ -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.

t/t3012-ls-files-dedup.sh line 12

Quoted 6 lines

t/t3012-ls-files-dedup.sh new file +6 -0

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

t/t3012-ls-files-dedup.sh line 19

Quoted 8 lines

t/t3012-ls-files-dedup.sh new file +7 -0

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

t/t3012-ls-files-dedup.sh line 23

Quoted 4 lines

t/t3012-ls-files-dedup.sh new file +4 -0

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

Quoted 1 line

+ test_must_fail git merge master &&

This will become "git merge tip" instead of 'master'.

t/t3012-ls-files-dedup.sh line 31

Quoted 7 lines

t/t3012-ls-files-dedup.sh new file +7 -0

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

Quoted 13 lines

t/t3012-ls-files-dedup.sh new file +13 -0

+     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

t/t3012-ls-files-dedup.sh line 54

Quoted 10 lines

t/t3012-ls-files-dedup.sh new file +10 -0

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

Quoted 3 lines

t/t3012-ls-files-dedup.sh new file +3 -0

+     git merge --abort
+'
+test_done

Quoted 1 line

OPT_BOOL(0,"deduplicate",&skipping_duplicates,N_("suppress duplicate entries")),

Quoted 18 lines · Junio C Hamano <[email protected]> 于2021年1月18日周一 上午7:34写道：

t/t3012-ls-files-dedup.sh line 5

Quoted 11 lines · ZheNing Hu via GitGitGadget

t/t3012-ls-files-dedup.sh new file +5 -0

new file mode 100755
index 00000000000..75877255c2c
@@ -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.

Fine,It might be easier for me to write a test file myself for the time being. But I will learn slowly.

Quoted 16 lines

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.

t/t3012-ls-files-dedup.sh line 12

Quoted 6 lines

t/t3012-ls-files-dedup.sh new file +6 -0

+test_expect_success 'setup' '
+     >a.txt &&
+     >b.txt &&
+     >delete.txt &&
+     git add a.txt b.txt delete.txt &&
+     git commit -m master:1 &&

Quoted 7 lines

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.

Well, I will try my best to use less "master".

Quoted 17 lines

t/t3012-ls-files-dedup.sh line 19

Quoted 8 lines

t/t3012-ls-files-dedup.sh new file +7 -0

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

Quoted 9 lines

t/t3012-ls-files-dedup.sh line 23

Quoted 4 lines

t/t3012-ls-files-dedup.sh new file +4 -0

+     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

I understand it now...setup is for serve as a basis for other tests.

Quoted 6 lines

        git tag tip &&
        git reset --hard HEAD^ &&
        echo change >a.txt &&
        git commit -a -m side &&
        git tag side

Quoted 9 lines

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.

Quoted 1 line

+ test_must_fail git merge master &&

This will become "git merge tip" instead of 'master'.

use tag instead of use branch name...

Quoted 10 lines

t/t3012-ls-files-dedup.sh line 31

Quoted 7 lines

t/t3012-ls-files-dedup.sh new file +7 -0

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

Quoted 8 lines

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.

Well,I understand now that the a test_success should allow other programmers to skip this test,so that we should reset to a known state at the beginning of each test.

Quoted 24 lines

Quoted 13 lines

t/t3012-ls-files-dedup.sh new file +13 -0

+     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

t/t3012-ls-files-dedup.sh line 54

Quoted 10 lines

t/t3012-ls-files-dedup.sh new file +10 -0

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

Quoted 4 lines

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.

indeed so.

Quoted 5 lines

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.

Thanks:)

Quoted 3 lines

Quoted 3 lines

t/t3012-ls-files-dedup.sh new file +3 -0

+     git merge --abort
+'
+test_done

Here I am thinking about the role of this "--deduplicate" is to suppress duplicate filenames rather than duplicate entries. Do you think I should modify this sentence?

Quoted 1 line

Quoted 1 line

OPT_BOOL(0,"deduplicate",&skipping_duplicates,N_("suppress duplicate entries")),

Quoted 5 lines · 胡哲宁

Here I am thinking about the role of this "--deduplicate" is to suppress duplicate filenames rather than duplicate entries. Do you think I should modify this sentence?

Quoted 1 line

Quoted 1 line

OPT_BOOL(0,"deduplicate",&skipping_duplicates,N_("suppress duplicate entries")),

I see no strong need to. One set of output entries from "ls-files" may say

    $ git ls-files -u
    100644 536e55524db72bd2acf175208aef4f3dfc148d41 1   COPYING
    100644 536e55524db72bd2acf175208aef4f3dfc148d43 3   COPYING

and these three "entries" are not duplicates. Another set of output entries may say

    $ git ls-files COPYING
    COPYING
    COPYING
    COPYING

and these output entries are duplicates. If you deduplicate the latter but not the former, then "suppress duplicate entries" is exactly what you are doing, I would think.

And if you are asked to show entries that would look like this in a not-deduplicated form:

    $ git ls-files -u
    100644 536e55524db72bd2acf175208aef4f3dfc148d41 1   COPYING
    100644 536e55524db72bd2acf175208aef4f3dfc148d41 1   COPYING
    100644 536e55524db72bd2acf175208aef4f3dfc148d43 3   COPYING

"suppressing duplicates" would give us the first entry and drop the second entry that is identical to the second entry, I would think [*1*].

So "duplicate entries" would probably be more correct description of what we want to happen than "duplicate filenames".

[Footnote]

*1* Multiple "common ancestor" versions at stage #1 for the same
path is not an error. That is how "merge-resolve" expresses criss-cross merge where multiple merge-bases exist.

Multiple "their" versions at stage #3 for the same path is not an error, and "merge-octopus" should use it to express contents from histories being merged into ours, but the implementation of the octopus strategy does not use this feature of the index.

Multiple "our" versions at stage #2 by definition should not happen ;-)

From: ZheNing Hu <[email protected]>

This situation may occur in the original code: lstat() failed but we use &st to feed ie_modified() later.

Therefore, we can directly execute show_ce without the judgment of ie_modified() when lstat() has failed.

Signed-off-by ZheNing Hu <[email protected]>

---

Quoted 8 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

This situation may occur in the original code: lstat() failed but we use &st to feed ie_modified() later.

Therefore, we can directly execute show_ce without the judgment of ie_modified() when lstat() has failed.

Quoted 2 lines

Signed-off-by ZheNing Hu <[email protected]>

---

Thanks. A few comments:

* The error_errno() line is not indented correctly; I'll fix it up while queuing, but it would conflict with 2/3 as you'll be moving that line around.

* When we say "error", we do not even know if the thing got removed or modified at all, so it is somewhat strange to report it as such (the path may be intact and the only issue may be that we cannot read the containing directory). It is equally strange not to say anything on the path, and between the two, there isn't clearly a more correct answer. What you implemented here does not change the traditional behaviour of reporting it as deleted/modified to "alert" the user, which I think is good.

* The logic for modified entry looks a bit duplicated. I wonder if the one at the end of this message reads better. Renaming err to stat_err is optional, but I think the name makes it clear why it is sensible that these two places use the variable as a sign that the path was deleted and/or modified.

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 358

Quoted 10 lines

                      err = lstat(fullname.buf, &st);

builtin/ls-files.c +8 -0

+                     if (err) {
+                             if (errno != ENOENT && errno != ENOTDIR)
+                                 error_errno("cannot lstat '%s'", fullname.buf);
+                             if (show_deleted)
+                                     show_ce(repo, dir, ce, fullname.buf, tag_removed);
+                             if (show_modified)
+                                     show_ce(repo, dir, ce, fullname.buf, tag_modified);
+                     } else if (show_modified && ie_modified(repo->index, ce, &st, 0))
                              show_ce(repo, dir, ce, fullname.buf, tag_modified);

                        stat_err = lstat(...);
                        if (stat_err && (errno != ENOENT && errno != ENOTDIR))
                                error_errno("cannot lstat '%s'", fullname.buf);

                        if (show_deleted && stat_err)
                                show_ce(..., tag_removed);
                        if (show_modified &&
                            (stat_err || ie_modified(..., &st, 0)))
                                show_ce(..., tag_modified);

From: ZheNing Hu <[email protected]>

Refactor the two for loops into one,skip showing the ce if it has the same name as the previously shown one, only when doing so won't lose information.

Signed-off-by ZheNing Hu <[email protected]>

---
 builtin/ls-files.c | 70 +++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 41 deletions(-)

builtin/ls-files.c +4 -0

+		if (ce_skip_worktree(ce))
+			continue;
+		err = lstat(fullname.buf, &st);
+		if (err) {

Quoted 10 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Refactor the two for loops into one,skip showing the ce if it has the same name as the previously shown one, only when doing so won't lose information.

Signed-off-by ZheNing Hu <[email protected]>

---
 builtin/ls-files.c | 70 +++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 41 deletions(-)

This one needs a bit more work, but I like the basic structure of the rewritten loop.

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 316

Quoted 13 lines

builtin/ls-files.c +2 -3

index f1617260064..1454ab1ae6f 100644
@@ -312,51 +312,39 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
              if (show_killed)
                      show_killed_files(repo->index, dir);
      }
-     if (show_cached || show_stage) {
-             for (i = 0; i < repo->index->cache_nr; i++) {
-                     const struct cache_entry *ce = repo->index->cache[i];
+     if (! (show_cached || show_stage || show_deleted || show_modified))
+             return;

If none of these four are given, nothing will be given after this point, so returning early is good.

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 328

Quoted 12 lines

builtin/ls-files.c +10 -0

+     for (i = 0; i < repo->index->cache_nr; i++) {
+             const struct cache_entry *ce = repo->index->cache[i];
+             struct stat st;
+             int err;
 
+             construct_fullname(&fullname, repo, ce);
 
+             if ((dir->flags & DIR_SHOW_IGNORED) &&
+                     !ce_excluded(dir, repo->index, fullname.buf, ce))
+                     continue;
+             if (ce->ce_flags & CE_UPDATE)
+                     continue;

The above two are common between the original two codepaths, and merging them is good.

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 335

Quoted 7 lines

builtin/ls-files.c +6 -0

+             if (show_cached || show_stage) {
+                     if (!show_unmerged || ce_stage(ce))
+                             show_ce(repo, dir, ce, fullname.buf,
+                                     ce_stage(ce) ? tag_unmerged :
+                                     (ce_skip_worktree(ce) ? tag_skip_worktree :
+                                             tag_cached));
              }

We would want to reduce the indentation level of the show_ce() by consolidating the nested if/if to

                if ((show_cached || show_stage) &&
                    (!show_unmerged || ce_stage(ce)))
                        show_ce(...);

Everything below from this point should be skipped (especially, the call to lstat()) unless show_modified and/or show_deleted was asked by the caller, i.e. we want to insert

                if (!(show_deleted || show_modified))
                        continue;

here, before we call ce_skip_worktree(), I think.

Quoted 4 lines

builtin/ls-files.c +4 -0

+             if (ce_skip_worktree(ce))
+                     continue;
+             err = lstat(fullname.buf, &st);
+             if (err) {

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 348

Quoted 10 lines

builtin/ls-files.c +7 -1

+                     if (errno != ENOENT && errno != ENOTDIR)
+                             error_errno("cannot lstat '%s'", fullname.buf);
+                     if (show_deleted)
+                             show_ce(repo, dir, ce, fullname.buf, tag_removed);
+                     if (show_modified)
                              show_ce(repo, dir, ce, fullname.buf, tag_modified);
-             }
+             } else if (show_modified && ie_modified(repo->index, ce, &st, 0))
+                     show_ce(repo, dir, ce, fullname.buf, tag_modified);
      }

And this part would look somewhat different if we take my earlier suggestion for [1/3].

Thanks.

builtin/ls-files.c +1 -5

 		err = lstat(fullname.buf, &st);
 		if (err) {
-			if (errno != ENOENT && errno != ENOTDIR)
-				error_errno("cannot lstat '%s'", fullname.buf);
-			if (show_deleted)
+			if (skipping_duplicates && show_deleted && show_modified)
 				show_ce(repo, dir, ce, fullname.buf, tag_removed);
-			if (show_modified)
-				show_ce(repo, dir, ce, fullname.buf, tag_modified);

builtin/ls-files.c +2 -0

+		last_shown_ce = ce;
 	}
 
 	strbuf_release(&fullname);

@@ -571,6 +590,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 			N_("pretend that paths removed since <tree-ish> are still present")),
 		OPT__ABBREV(&abbrev),
 		OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
+		OPT_BOOL(0,"deduplicate",&skipping_duplicates,N_("suppress duplicate entries")),
 		OPT_END()
 	};
 

Quoted 2 lines · ZheNing Hu via GitGitGadget

(quoted diff) +0 -0

@@ -321,30 +324,46 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 329

Quoted 5 lines

builtin/ls-files.c +3 -0

              construct_fullname(&fullname, repo, ce);
 
+             if (skipping_duplicates && last_shown_ce &&
+                     !strcmp(last_shown_ce->name,ce->name))
+                             continue;

Style. Missing SP after comma.

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 338

Quoted 10 lines

builtin/ls-files.c +3 -0

              if ((dir->flags & DIR_SHOW_IGNORED) &&
                      !ce_excluded(dir, repo->index, fullname.buf, ce))
                      continue;
              if (ce->ce_flags & CE_UPDATE)
                      continue;
              if (show_cached || show_stage) {
+                     if (skipping_duplicates && last_shown_ce &&
+                             !strcmp(last_shown_ce->name,ce->name))
+                                     continue;

OK. When show_stage is set, skipping_duplicates is automatically turned off (and show_unmerged is automatically covered as it turns show_stage on automatically). So this feature has really become "are we showing only names, and if so, did we show an entry of the same name before?".

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 345

Quoted 8 lines

builtin/ls-files.c +2 -0

                      if (!show_unmerged || ce_stage(ce))
                              show_ce(repo, dir, ce, fullname.buf,
                                      ce_stage(ce) ? tag_unmerged :
                                      (ce_skip_worktree(ce) ? tag_skip_worktree :
                                              tag_cached));
+                     if (show_cached && skipping_duplicates)
+                             last_shown_ce = ce;

The code that calls show_ce() belonging to a totally separate if() statement makes my stomach hurt---how are we going to guarantee that "last shown" really will keep track of what was shown last?

Shouldn't the above be more like this?

It does maintain last_shown_ce even when skipping_duplicates is not set, but I think that is overall win. Assigning unconditionally would be cheaper than making a conditional jump on the variable and make assignment (or not).

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 351

Quoted 6 lines

builtin/ls-files.c +3 -0

              }
              if (ce_skip_worktree(ce))
                      continue;
+             if (skipping_duplicates && last_shown_ce &&
+                     !strcmp(last_shown_ce->name,ce->name))
+                             continue;

Style. Missing SP after comma.

OK, if we've shown an entry of the same name under skip-duplicates mode, and the code that follows will show the same entry (if they decide to show it), so we can go to the next entry early.

Quoted 9 lines

builtin/ls-files.c +1 -5

              err = lstat(fullname.buf, &st);
              if (err) {
-                     if (errno != ENOENT && errno != ENOTDIR)
-                             error_errno("cannot lstat '%s'", fullname.buf);
-                     if (show_deleted)
+                     if (skipping_duplicates && show_deleted && show_modified)
                              show_ce(repo, dir, ce, fullname.buf, tag_removed);
-                     if (show_modified)
-                             show_ce(repo, dir, ce, fullname.buf, tag_modified);

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 365

Quoted 10 lines

builtin/ls-files.c +8 -0

+                     else {
+                             if (errno != ENOENT && errno != ENOTDIR)
+                                     error_errno("cannot lstat '%s'", fullname.buf);
+                             if (show_deleted)
+                                     show_ce(repo, dir, ce, fullname.buf, tag_removed);
+                             if (show_modified)
+                                     show_ce(repo, dir, ce, fullname.buf, tag_modified);
+                     }
              } else if (show_modified && ie_modified(repo->index, ce, &st, 0))
                      show_ce(repo, dir, ce, fullname.buf, tag_modified);

This part will change shape quite a bit when we follow the suggestion I made on 1/3, so I won't analyze how correct this version is.

Quoted 13 lines

builtin/ls-files.c +2 -0

+             last_shown_ce = ce;
      }
 
      strbuf_release(&fullname);

@@ -571,6 +590,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
                      N_("pretend that paths removed since <tree-ish> are still present")),
              OPT__ABBREV(&abbrev),
              OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
+             OPT_BOOL(0,"deduplicate",&skipping_duplicates,N_("suppress duplicate entries")),
              OPT_END()
      };
 

builtin/ls-files.c int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) · line 634

Quoted 5 lines

builtin/ls-files.c +2 -0

@@ -610,6 +630,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
               * you also show the stage information.
               */
              show_stage = 1;
+     if (show_tag || show_stage)
+             skipping_duplicates = 0;

OK.

builtin/ls-files.c int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) · line 637

Quoted 3 lines

builtin/ls-files.c +0 -0

      if (dir.exclude_per_dir)
              exc_given = 1;
 

Thanks.

Quoted 5 lines

OK. When show_stage is set, skipping_duplicates is automatically turned off (and show_unmerged is automatically covered as it turns show_stage on automatically). So this feature has really become "are we showing only names, and if so, did we show an entry of the same name before?".

Yeah,showing only names,so I yesterday ask such question :)

Quoted 16 lines

Review

Quoted 8 lines

if (!show_unmerged || ce_stage(ce))
show_ce(repo, dir, ce, fullname.buf,
ce_stage(ce) ? tag_unmerged :
(ce_skip_worktree(ce) ? tag_skip_worktree :
tag_cached));

(quoted diff) +2 -0

+                     if (show_cached && skipping_duplicates)
+                             last_shown_ce = ce;

The code that calls show_ce() belonging to a totally separate if() statement makes my stomach hurt---how are we going to guarantee that "last shown" really will keep track of what was shown last?

Shouldn't the above be more like this?

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 363

Quoted 8 lines

builtin/ls-files.c +3 -1

-                       if (!show_unmerged || ce_stage(ce))
+                       if (!show_unmerged || ce_stage(ce)) {
                                show_ce(repo, dir, ce, fullname.buf,
                                        ce_stage(ce) ? tag_unmerged :
                                        (ce_skip_worktree(ce) ? tag_skip_worktree :
                                                tag_cached));
+                               last_shown_ce = ce;
+                       }

well,I am also thinking about this question :"last_shown_ce" is not true last shown ce,but may be If "last_shown_ce" truly seen every last shown ce ,We may need more cumbersome logic to make the program correct. I have tried the processing method of your above code before, but found
that some errors may have occurred.

Quoted 30 lines · 胡哲宁

Quoted 5 lines

OK. When show_stage is set, skipping_duplicates is automatically turned off (and show_unmerged is automatically covered as it turns show_stage on automatically). So this feature has really become "are we showing only names, and if so, did we show an entry of the same name before?".

Yeah,showing only names,so I yesterday ask such question :)

Quoted 16 lines

Review

Quoted 8 lines

if (!show_unmerged || ce_stage(ce))
show_ce(repo, dir, ce, fullname.buf,
ce_stage(ce) ? tag_unmerged :
(ce_skip_worktree(ce) ? tag_skip_worktree :
tag_cached));

(quoted diff) +2 -0

+                     if (show_cached && skipping_duplicates)
+                             last_shown_ce = ce;

The code that calls show_ce() belonging to a totally separate if() statement makes my stomach hurt---how are we going to guarantee that "last shown" really will keep track of what was shown last?

Shouldn't the above be more like this?

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 363

Quoted 8 lines

builtin/ls-files.c +3 -1

-                       if (!show_unmerged || ce_stage(ce))
+                       if (!show_unmerged || ce_stage(ce)) {
                                show_ce(repo, dir, ce, fullname.buf,
                                        ce_stage(ce) ? tag_unmerged :
                                        (ce_skip_worktree(ce) ? tag_skip_worktree :
                                                tag_cached));
+                               last_shown_ce = ce;
+                       }

Quoted 5 lines

well,I am also thinking about this question :"last_shown_ce" is not true last shown ce,but may be If "last_shown_ce" truly seen every last shown ce ,We may need more cumbersome logic to make the program correct. I have tried the processing method of your above code before, but found
that some errors may have occurred.

I think judicious use of "goto" without introducing the last_shown would probably result in a much more maintainable code. It may look somewhat like so:

        for (i = 0; i < repo->index->cache_nr; i++) {
                const struct cache_entry *ce = repo->index->cache[i];
                struct stat st;
                int stat_err;

                construct_fullname(&fullname, repo, ce);

                if ((dir->flags & DIR_SHOW_IGNORED) &&
                        !ce_excluded(dir, repo->index, fullname.buf, ce))
                        continue;
                if (ce->ce_flags & CE_UPDATE)
                        continue;
                if ((show_cached || show_stage) &&
                    (!show_unmerged || ce_stage(ce))) {
                        show_ce(repo, dir, ce, fullname.buf,
                                ce_stage(ce) ? tag_unmerged :
                                (ce_skip_worktree(ce) ? tag_skip_worktree :
                                 tag_cached));
                        if (skip_duplicates)
                                goto skip_to_next_name;
                }

                if (!show_deleted && !show_modified)
                        continue;
                if (ce_skip_worktree(ce))
                        continue;
                stat_err = lstat(fullname.buf, &st);
                if (stat_err && (errno != ENOENT && errno != ENOTDIR))
                        error_errno("cannot lstat '%s'", fullname.buf);

                if (show_deleted) {
                        show_ce(repo, dir, ce, fullname.buf, tag_removed);
                        if (skip_duplicates)
                                goto skip_to_next_name;
                }
                if (show_modified &&
                    (stat_err || ie_modified(repo->index, ce, &st, 0)))
                        show_ce(repo, dir, ce, fullname.buf, tag_modified);
                continue;

        skip_to_next_name:
                {

                        int j;
                        const struct cache_entry **cache = repo->index->cache;
                        for (j = i + 1; j < repo->index->cache_nr; j++)
                                if (strcmp(ce->ce_name, cache[j]->ce_name))
                                        break;
                        i = j - 1; /* compensate for outer for loop */
                }
        }

And should I still use gitgitgadget PR on my origin branch "dedup"or send patch on branch "zh/ls-files-deduplicate"?

Quoted 2 lines

And should I still use gitgitgadget PR on my origin branch "dedup"or send patch on branch "zh/ls-files-deduplicate"?

The way GitGitGadget is designed asks for contributors to adjust their patch(es) via interactive rebase, implementing the suggestions and addressing the concerns while doing so, then force-pushing, optionally amending the first PR comment (i.e. the description) with a list of those changes, and then submitting a new iteration via /submit.

Just being curious, but when a series hits 'next', would the way in which the user interacts with GGG change?

With or without GGG, what is done on the local side is not all that different---you build new commits on top without disturbing the commits that are in 'next'. Then what? Push it again (this time there is no need to force) and submit the additional ones via /submit?

Quoted 2 lines · Junio C Hamano

Just being curious, but when a series hits 'next', would the way in which the user interacts with GGG change?

My hunch is that we should probably tell new users (for who GitGitGadget now uses the "new user" PR label) about the expectations of only adding patches on top (i.e. in a new PR), unless the branch gets kicked out of next.

Quoted 4 lines

With or without GGG, what is done on the local side is not all that different---you build new commits on top without disturbing the commits that are in 'next'. Then what? Push it again (this time there is no need to force) and submit the additional ones via /submit?

GitGitGadget would send the entire patch series, which is probably not a good idea.

Hi 胡哲宁,

Quoted 2 lines · 胡哲宁

And should I still use gitgitgadget PR on my origin branch "dedup"or send patch on branch "zh/ls-files-deduplicate"?

The way GitGitGadget is designed asks for contributors to adjust their patch(es) via interactive rebase, implementing the suggestions and addressing the concerns while doing so, then force-pushing, optionally amending the first PR comment (i.e. the description) with a list of those changes, and then submitting a new iteration via /submit.

Ciao,
Johannes

In order to provide users a better experience
when viewing information about files in the index and the working tree, the --deduplicate option will suppress some duplicate name under some conditions.

builtin/ls-files.c +1 -1

@@ -317,7 +318,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 	for (i = 0; i < repo->index->cache_nr; i++) {
 		const struct cache_entry *ce = repo->index->cache[i];
 		struct stat st;
-		int err;
+		int stat_err;
 
 		construct_fullname(&fullname, repo, ce);
 

@@ -326,25 +327,43 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 			continue;
 		if (ce->ce_flags & CE_UPDATE)
 			continue;

Quoted 4 lines · ZheNing Hu

In order to provide users a better experience
when viewing information about files in the index and the working tree, the --deduplicate option will suppress some duplicate name under some conditions.

Now is it just a single patch squashing everything together? That does not look like it.

Quoted 18 lines

builtin/ls-files.c +3 -3

@@ -317,7 +318,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
      for (i = 0; i < repo->index->cache_nr; i++) {
              const struct cache_entry *ce = repo->index->cache[i];
              struct stat st;
-             int err;
+             int stat_err;
 
              construct_fullname(&fullname, repo, ce);
 

@@ -326,25 +327,43 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
                      continue;
              if (ce->ce_flags & CE_UPDATE)
                      continue;
-             if (show_cached || show_stage) {
-                     if (!show_unmerged || ce_stage(ce))
+             if ((show_cached || show_stage) &&
+                     (!show_unmerged || ce_stage(ce))) {
                              show_ce(repo, dir, ce, fullname.buf,

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 337

Quoted 5 lines

builtin/ls-files.c +2 -0

                                      ce_stage(ce) ? tag_unmerged :
                                      (ce_skip_worktree(ce) ? tag_skip_worktree :
                                              tag_cached));
+                     if (show_cached && skipping_duplicates)
+                             goto skip_to_next_name;

Why should this be so complex? You are dropping skipping_duplicates when the output is not name-only, so shouldn't this look more like

                if ((show_cached || show_stage) &&
                    (!show_unmerged || ce_stage(ce)) {
                        show_ce(...);
                        if (skipping_duplicates)
                                goto skip_to_next_name;
                }

It seems that this still depends on the 2/3 from the previous iteration, against which I suggested to merge the conditions of nested if statements into one. That should be done in the updated 2/3, not in this step, no?

builtin/ls-files.c static void show_files(struct repository *repo, struct dir_struct *dir) · line 340

Quoted 3 lines

builtin/ls-files.c +2 -0

              }
+             if (!show_deleted && !show_modified)
+                     continue;

And this one also belongs to the step 2/3 that consolidates the two loops into one.

I think you'd need to start from the three patches in v5, "rebase -i" not just [3/3] but at least [2/3], too.

Thanks.

From: ZheNing Hu <[email protected]>

This situation may occur in the original code: lstat() failed but we use &st to feed ie_modified() later.

Therefore, we can directly execute show_ce without the judgment of ie_modified() when lstat() has failed.

Signed-off-by ZheNing Hu <[email protected]>

---
 builtin/ls-files.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

From: ZheNing Hu <[email protected]>

Refactor the two for loops into one,skip showing the ce if it has the same name as the previously shown one, only when doing so won't lose information.

Signed-off-by ZheNing Hu <[email protected]>

---
 builtin/ls-files.c | 70 ++++++++++++++++++++--------------------------
 1 file changed, 30 insertions(+), 40 deletions(-)

builtin/ls-files.c +30 -40

index 1e264bd1329..966c0ab0296 100644
@@ -312,49 +312,39 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 		if (show_killed)
 			show_killed_files(repo->index, dir);
 	}
-	if (show_cached || show_stage) {
-		for (i = 0; i < repo->index->cache_nr; i++) {
-			const struct cache_entry *ce = repo->index->cache[i];
-
-			construct_fullname(&fullname, repo, ce);
-
-			if ((dir->flags & DIR_SHOW_IGNORED) &&
-			    !ce_excluded(dir, repo->index, fullname.buf, ce))
-				continue;
-			if (show_unmerged && !ce_stage(ce))
-				continue;
-			if (ce->ce_flags & CE_UPDATE)
-				continue;
-			show_ce(repo, dir, ce, fullname.buf,
-				ce_stage(ce) ? tag_unmerged :
-				(ce_skip_worktree(ce) ? tag_skip_worktree :
-				 tag_cached));
-		}
-	}
-	if (show_deleted || show_modified) {
-		for (i = 0; i < repo->index->cache_nr; i++) {
-			const struct cache_entry *ce = repo->index->cache[i];
-			struct stat st;
-			int stat_err;
+	if (! (show_cached || show_stage || show_deleted || show_modified))
+		return;
+	for (i = 0; i < repo->index->cache_nr; i++) {
+		const struct cache_entry *ce = repo->index->cache[i];
+		struct stat st;
+		int stat_err;
 
-			construct_fullname(&fullname, repo, ce);
+		construct_fullname(&fullname, repo, ce);
 
-			if ((dir->flags & DIR_SHOW_IGNORED) &&
-			    !ce_excluded(dir, repo->index, fullname.buf, ce))
-				continue;
-			if (ce->ce_flags & CE_UPDATE)
-				continue;
-			if (ce_skip_worktree(ce))
-				continue;
-			stat_err = lstat(fullname.buf, &st);
-			if (stat_err && (errno != ENOENT && errno != ENOTDIR))
-				error_errno("cannot lstat '%s'", fullname.buf);
-			if (stat_err && show_deleted)
-				show_ce(repo, dir, ce, fullname.buf, tag_removed);
-			if (show_modified &&
-				(stat_err || ie_modified(repo->index, ce, &st, 0)))
-					show_ce(repo, dir, ce, fullname.buf, tag_modified);
+		if ((dir->flags & DIR_SHOW_IGNORED) &&
+			!ce_excluded(dir, repo->index, fullname.buf, ce))
+			continue;
+		if (ce->ce_flags & CE_UPDATE)
+			continue;
+		if (show_cached || show_stage) {
+			if (!show_unmerged || ce_stage(ce))
+				show_ce(repo, dir, ce, fullname.buf,
+					ce_stage(ce) ? tag_unmerged :
+					(ce_skip_worktree(ce) ? tag_skip_worktree :
+						tag_cached));
 		}
+		if (!show_deleted && !show_modified)
+			continue;
+		if (ce_skip_worktree(ce))
+			continue;
+		stat_err = lstat(fullname.buf, &st);
+		if (stat_err && (errno != ENOENT && errno != ENOTDIR))
+			error_errno("cannot lstat '%s'", fullname.buf);
+		if (stat_err && show_deleted)
+			show_ce(repo, dir, ce, fullname.buf, tag_removed);
+		if (show_modified &&
+			(stat_err || ie_modified(repo->index, ce, &st, 0)))
+				show_ce(repo, dir, ce, fullname.buf, tag_modified);
 	}
 
 	strbuf_release(&fullname);