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