| Message ID | ec9464f6094f111bc7b6dc1dc07ecc9997d366d4.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> > > 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> > --- Thanks. A few comments: * The error_errno() line is not indented correctly; I'll fix it up while queuing, but it would conflict with 2/3 as you'll be moving that line around. * When we say "error", we do not even know if the thing got removed or modified at all, so it is somewhat strange to report it as such (the path may be intact and the only issue may be that we cannot read the containing directory). It is equally strange not to say anything on the path, and between the two, there isn't clearly a more correct answer. What you implemented here does not change the traditional behaviour of reporting it as deleted/modified to "alert" the user, which I think is good. * The logic for modified entry looks a bit duplicated. I wonder if the one at the end of this message reads better. Renaming err to stat_err is optional, but I think the name makes it clear why it is sensible that these two places use the variable as a sign that the path was deleted and/or modified. > 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); stat_err = lstat(...); if (stat_err && (errno != ENOENT && errno != ENOTDIR)) error_errno("cannot lstat '%s'", fullname.buf); if (show_deleted && stat_err) show_ce(..., tag_removed); if (show_modified && (stat_err || ie_modified(..., &st, 0))) show_ce(..., tag_modified);
Junio C Hamano <gitster@pobox.com> 于2021年1月21日周四 上午4:26写道： > > "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> > > --- > > Thanks. A few comments: > > * The error_errno() line is not indented correctly; I'll fix it up > while queuing, but it would conflict with 2/3 as you'll be moving > that line around. > I might not have noticed,very Sorry. > * When we say "error", we do not even know if the thing got removed > or modified at all, so it is somewhat strange to report it as > such (the path may be intact and the only issue may be that we > cannot read the containing directory). It is equally strange not > to say anything on the path, and between the two, there isn't > clearly a more correct answer. What you implemented here does > not change the traditional behaviour of reporting it as > deleted/modified to "alert" the user, which I think is good. > Haha,thanks! > * The logic for modified entry looks a bit duplicated. I wonder if > the one at the end of this message reads better. Renaming err to > stat_err is optional, but I think the name makes it clear why it > is sensible that these two places use the variable as a sign that > the path was deleted and/or modified. > > > 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); > > > stat_err = lstat(...); > if (stat_err && (errno != ENOENT && errno != ENOTDIR)) > error_errno("cannot lstat '%s'", fullname.buf); > > if (show_deleted && stat_err) > show_ce(..., tag_removed); > if (show_modified && > (stat_err || ie_modified(..., &st, 0))) > show_ce(..., tag_modified); > Yes,"stat_err" may better then "err". And putting the conditions together will make the code more compact. Thanks for your comment:)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c index c8eae899b82..f1617260064 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -347,9 +347,14 @@ static void show_files(struct repository *repo, struct dir_struct *dir) if (ce_skip_worktree(ce)) continue; err = lstat(fullname.buf, &st); - if (show_deleted && err) - show_ce(repo, dir, ce, fullname.buf, tag_removed); - if (show_modified && ie_modified(repo->index, ce, &st, 0)) + 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); } }