diff mbox series

[v6,1/3] ls_files.c: bugfix for --deleted and --modified

Message ID fbc38ce9075d3d28187351b0fb6b34a27ec431fd.1611397210.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series builtin/ls-files.c:add git ls-file --dedup option | expand

Commit Message

ZheNing Hu Jan. 23, 2021, 10:20 a.m. UTC
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>
---
 builtin/ls-files.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Jan. 23, 2021, 5:55 p.m. UTC | #1
"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>
> ---
>  builtin/ls-files.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)

Looks good.  I think we are finished with this part, except for one
nit.

> +			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);
>  		}

The last line is misindented by having one leading horizontal tab
too many.  show_ce() for modified files and show_ce() for deleted
files are done independently under different conditions and stand as
equals, so the beginning of them should align to show that.

Perhaps format the last three lines more like so:

			if (show_modified &&
			    (stat_err || ie_modified(repo->index, ce, &st, 0)))
				show_ce(repo, dir, ce, fullname.buf, tag_modified);

Again this would cascade throughout the sreies, but let's see if
there are other things we may want to change in the rest of the
series first.  Otherwise, instead of having you rebase, I probably
have time to tweak the series on my end while queuing.

Thanks.
diff mbox series

Patch

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