| Message ID | 802ff802be884349b4a63c0ae6e4b783e6c7aedb.1611037846.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/ls-files.c:add git ls-file --dedup option | expand |
"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]. Thanks.
Junio C Hamano <gitster@pobox.com> 于2021年1月21日周四 上午4:27写道： > > "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. > 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.
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; + 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); + construct_fullname(&fullname, repo, ce); - if ((dir->flags & DIR_SHOW_IGNORED) && - !ce_excluded(dir, repo->index, fullname.buf, ce)) - continue; - if (show_unmerged && !ce_stage(ce)) - continue; - if (ce->ce_flags & CE_UPDATE) - continue; - show_ce(repo, dir, ce, fullname.buf, - ce_stage(ce) ? tag_unmerged : - (ce_skip_worktree(ce) ? tag_skip_worktree : - tag_cached)); + 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 (!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_deleted || show_modified) { - 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; - 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)) + 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); } strbuf_release(&fullname);