Junio C Hamano <gitster@xxxxxxxxx> 于2021年1月21日周四 上午4:27写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: ZheNing Hu <adlternative@xxxxxxxxx>
> >
> > 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 <adlternative@xxxxxxxxx>
> > ---
> > 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.
>
> > diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> > index f1617260064..1454ab1ae6f 100644
> > --- a/builtin/ls-files.c
> > +++ b/builtin/ls-files.c
> > @@ -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.
>
I understand.
> > + 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.
>
> > + 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(...);
>
>
The reason for this may be I gave
"if(show_cached || show_stage)" in 3/3
Added some logic.
> 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;
>
I agree.
> here, before we call ce_skip_worktree(), I think.
>
> > + if (ce_skip_worktree(ce))
> > + continue;
> > + err = lstat(fullname.buf, &st);
> > + 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);
> > }
>
> And this part would look somewhat different if we take my earlier
> suggestion for [1/3].
>
Fine.
> Thanks.
