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

From: Junio C Hamano <gitster@pobox.com>
To: "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Eric Sunshine" <sunshine@sunshineco.com>,
	胡哲宁 <adlternative@gmail.com>
Subject: Re: [PATCH v5 1/3] ls_files.c: bugfix for --deleted and --modified
Date: Wed, 20 Jan 2021 12:26:21 -0800	[thread overview]
Message-ID: <xmqqlfcnfj82.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: ec9464f6094f111bc7b6dc1dc07ecc9997d366d4.1611037846.git.gitgitgadget@gmail.com

"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);