Re: [PATCH v5 3/3] ls-files.c: add --deduplicate option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 20, 2021, 21:26 UTC
- Message-ID
- <xmqq7do7fggn.fsf@gitster.c.googlers.com>
- In-Reply-To
- <e9c5318670658b032ba921129859f9fb3b2ca017.1611037846.git.gitgitgadget@gmail.com>
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 7 quoted lines
> @@ -321,30 +324,46 @@ static void show_files(struct repository *repo, struct dir_struct *dir) > > construct_fullname(&fullname, repo, ce); > > + if (skipping_duplicates && last_shown_ce && > + !strcmp(last_shown_ce->name,ce->name)) > + continue;
Style. Missing SP after comma.
Show 9 quoted lines
> 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?".
Show 7 quoted 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)); > + 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?
- 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;
+ }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).
Show 6 quoted lines
> } > 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.
Show 19 quoted lines
> 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);
> + 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.
Show 18 quoted lines
> + 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()
> };
>
> @@ -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.
> if (dir.exclude_per_dir) > exc_given = 1; >
Thanks.