Re: [PATCH v5 1/3] ls_files.c: bugfix for --deleted and --modified

Junio C Hamano [off-list ref] 于2021年1月21日周四 上午4:26写道：

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

quoted

From: ZheNing Hu <redacted>

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 <redacted>
---

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.

quoted

                      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:)