From: 胡哲宁 <adlternative@gmail.com>
Date: Thu, 21 Jan 2021 10:02:26 GMT
Subject: Re: [PATCH v5 1/3] ls_files.c: bugfix for --deleted and --modified
Message-ID: <CAOLTT8QN=qLUb7HSCeufbo_zQyhjgQXE4T6x_XCg9Y8t_kT2Ag@mail.gmail.com>
In-Reply-To: <xmqqlfcnfj82.fsf@gitster.c.googlers.com>

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