Re: [PATCH v6] ls-files.c: add --deduplicate option

ZheNing Hu [off-list ref] writes:

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.

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?

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

Junio C Hamano [off-list ref] 于2021年1月23日周六 上午4:52写道：

ZheNing Hu [off-list ref] writes:

quoted

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

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

Truly,I may have considered too much,if I have
"show_stage","skipping_duplicates"
must be false.

                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?

quoted

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

Thanks,I am rewriting.