| Message ID | 8b02367a359e62d7721b9078ac8393a467d83724.1611485667.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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> > > This situation may occur in the original code: lstat() failed > but we use `&st` to feed ie_modified() later. > > Therefore, we can directly execute show_ce without the judgment of > ie_modified() when lstat() has failed. > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > [jc: fixed misindented code] I noticed that you reverted my fix in this version, when this is compared with the one I sent last night. Comparing the result of applying all three with what I sent last night, this v7 looks worse (see below). Let's discard this round and declare victory with what is already on 'seen'. Thanks. --- comparison between what these three patches would produce (preimage) and what is on 'seen' (postimage)is shown here. diff --git w/builtin/ls-files.c c/builtin/ls-files.c index fb9cf50d76..f6f9e483b2 100644 --- w/builtin/ls-files.c +++ c/builtin/ls-files.c @@ -313,7 +313,8 @@ 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 || show_deleted || show_modified)) + + 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]; @@ -328,15 +329,16 @@ static void show_files(struct repository *repo, struct dir_struct *dir) if (ce->ce_flags & CE_UPDATE) continue; if ((show_cached || show_stage) && - (!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)); + (!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 (skipping_duplicates) goto skip_to_next_name; } - if (!show_deleted && !show_modified) + + if (!(show_deleted || show_modified)) continue; if (ce_skip_worktree(ce)) continue; @@ -349,12 +351,13 @@ static void show_files(struct repository *repo, struct dir_struct *dir) goto skip_to_next_name; } if (show_modified && - (stat_err || ie_modified(repo->index, ce, &st, 0))) { - show_ce(repo, dir, ce, fullname.buf, tag_modified); + (stat_err || ie_modified(repo->index, ce, &st, 0))) { + show_ce(repo, dir, ce, fullname.buf, tag_modified); if (skipping_duplicates) goto skip_to_next_name; } continue; + skip_to_next_name: { int j; @@ -362,7 +365,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir) for (j = i + 1; j < repo->index->cache_nr; j++) if (strcmp(ce->name, cache[j]->name)) break; - i = j - 1; /* compensate for outer for loop */ + i = j - 1; /* compensate for the for loop */ } } @@ -590,7 +593,8 @@ 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_BOOL(0, "deduplicate", &skipping_duplicates, + N_("suppress duplicate entries")), OPT_END() };
OK,I didn’t notice any formatting changes before. Am I free from this patch now?I should probably look for other issues. Junio, thank you for all your patient help. I may often make some low-level mistakes. I am grateful. Cheers. Junio C Hamano <gitster@pobox.com> 于2021年1月25日周一 上午6:04写道： > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: ZheNing Hu <adlternative@gmail.com> > > > > This situation may occur in the original code: lstat() failed > > but we use `&st` to feed ie_modified() later. > > > > Therefore, we can directly execute show_ce without the judgment of > > ie_modified() when lstat() has failed. > > > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > > [jc: fixed misindented code] > > I noticed that you reverted my fix in this version, when this is > compared with the one I sent last night. > > Comparing the result of applying all three with what I sent last > night, this v7 looks worse (see below). Let's discard this round > and declare victory with what is already on 'seen'. > > Thanks. > > > --- > > comparison between what these three patches would produce (preimage) > and what is on 'seen' (postimage)is shown here. > > diff --git w/builtin/ls-files.c c/builtin/ls-files.c > index fb9cf50d76..f6f9e483b2 100644 > --- w/builtin/ls-files.c > +++ c/builtin/ls-files.c > @@ -313,7 +313,8 @@ 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 || show_deleted || show_modified)) > + > + 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]; > @@ -328,15 +329,16 @@ static void show_files(struct repository *repo, struct dir_struct *dir) > if (ce->ce_flags & CE_UPDATE) > continue; > if ((show_cached || show_stage) && > - (!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)); > + (!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 (skipping_duplicates) > goto skip_to_next_name; > } > - if (!show_deleted && !show_modified) > + > + if (!(show_deleted || show_modified)) > continue; > if (ce_skip_worktree(ce)) > continue; > @@ -349,12 +351,13 @@ static void show_files(struct repository *repo, struct dir_struct *dir) > goto skip_to_next_name; > } > if (show_modified && > - (stat_err || ie_modified(repo->index, ce, &st, 0))) { > - show_ce(repo, dir, ce, fullname.buf, tag_modified); > + (stat_err || ie_modified(repo->index, ce, &st, 0))) { > + show_ce(repo, dir, ce, fullname.buf, tag_modified); > if (skipping_duplicates) > goto skip_to_next_name; > } > continue; > + > skip_to_next_name: > { > int j; > @@ -362,7 +365,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir) > for (j = i + 1; j < repo->index->cache_nr; j++) > if (strcmp(ce->name, cache[j]->name)) > break; > - i = j - 1; /* compensate for outer for loop */ > + i = j - 1; /* compensate for the for loop */ > } > } > > @@ -590,7 +593,8 @@ 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_BOOL(0, "deduplicate", &skipping_duplicates, > + N_("suppress duplicate entries")), > OPT_END() > }; >
胡哲宁 <adlternative@gmail.com> writes: > OK,I didn’t notice any formatting changes before. > > Am I free from this patch now?I should probably > look for other issues. I think we are pretty much done with it. Thanks for working on the topic so patiently.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c index c8eae899b82..1e264bd1329 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -335,7 +335,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir) for (i = 0; i < repo->index->cache_nr; i++) { const struct cache_entry *ce = repo->index->cache[i]; struct stat st; - int err; + int stat_err; construct_fullname(&fullname, repo, ce); @@ -346,11 +346,14 @@ static void show_files(struct repository *repo, struct dir_struct *dir) continue; if (ce_skip_worktree(ce)) continue; - err = lstat(fullname.buf, &st); - if (show_deleted && err) + stat_err = lstat(fullname.buf, &st); + if (stat_err && (errno != ENOENT && errno != ENOTDIR)) + error_errno("cannot lstat '%s'", fullname.buf); + if (stat_err && show_deleted) show_ce(repo, dir, ce, fullname.buf, tag_removed); - if (show_modified && ie_modified(repo->index, ce, &st, 0)) - show_ce(repo, dir, ce, fullname.buf, tag_modified); + if (show_modified && + (stat_err || ie_modified(repo->index, ce, &st, 0))) + show_ce(repo, dir, ce, fullname.buf, tag_modified); } }