Re: [PATCH v5 2/3] ls_files.c: consolidate two for loops into one

>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > 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@gmail.com>
> > ---
> >  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.
>

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

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