diff mbox series

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

Message ID 8b02367a359e62d7721b9078ac8393a467d83724.1611485667.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. 24, 2021, 10:54 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>
[jc: fixed misindented code]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/ls-files.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Jan. 24, 2021, 10:04 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>
> [jc: fixed misindented code]

I noticed that you reverted my fix in this version, when this is
compared with the one I sent last night.

Comparing the result of applying all three with what I sent last
night, this v7 looks worse (see below).  Let's discard this round
and declare victory with what is already on 'seen'.

Thanks.


---

comparison between what these three patches would produce (preimage)
and what is on 'seen' (postimage)is shown here.

diff --git w/builtin/ls-files.c c/builtin/ls-files.c
index fb9cf50d76..f6f9e483b2 100644
--- w/builtin/ls-files.c
+++ c/builtin/ls-files.c
@@ -313,7 +313,8 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 		if (show_killed)
 			show_killed_files(repo->index, dir);
 	}
-	if (! (show_cached || show_stage || show_deleted || show_modified))
+
+	if (!(show_cached || show_stage || show_deleted || show_modified))
 		return;
 	for (i = 0; i < repo->index->cache_nr; i++) {
 		const struct cache_entry *ce = repo->index->cache[i];
@@ -328,15 +329,16 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 		if (ce->ce_flags & CE_UPDATE)
 			continue;
 		if ((show_cached || show_stage) &&
-			(!show_unmerged || ce_stage(ce))) {
-				show_ce(repo, dir, ce, fullname.buf,
-					ce_stage(ce) ? tag_unmerged :
-					(ce_skip_worktree(ce) ? tag_skip_worktree :
-						tag_cached));
+		    (!show_unmerged || ce_stage(ce))) {
+			show_ce(repo, dir, ce, fullname.buf,
+				ce_stage(ce) ? tag_unmerged :
+				(ce_skip_worktree(ce) ? tag_skip_worktree :
+				 tag_cached));
 			if (skipping_duplicates)
 				goto skip_to_next_name;
 		}
-		if (!show_deleted && !show_modified)
+
+		if (!(show_deleted || show_modified))
 			continue;
 		if (ce_skip_worktree(ce))
 			continue;
@@ -349,12 +351,13 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 				goto skip_to_next_name;
 		}
 		if (show_modified &&
-			(stat_err || ie_modified(repo->index, ce, &st, 0))) {
-				show_ce(repo, dir, ce, fullname.buf, tag_modified);
+		    (stat_err || ie_modified(repo->index, ce, &st, 0))) {
+			show_ce(repo, dir, ce, fullname.buf, tag_modified);
 			if (skipping_duplicates)
 				goto skip_to_next_name;
 		}
 		continue;
+
 skip_to_next_name:
 		{
 			int j;
@@ -362,7 +365,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 			for (j = i + 1; j < repo->index->cache_nr; j++)
 				if (strcmp(ce->name, cache[j]->name))
 					break;
-			i = j - 1; /* compensate for outer for loop */
+			i = j - 1; /* compensate for the for loop */
 		}
 	}
 
@@ -590,7 +593,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 			N_("pretend that paths removed since <tree-ish> are still present")),
 		OPT__ABBREV(&abbrev),
 		OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
-		OPT_BOOL(0,"deduplicate",&skipping_duplicates,N_("suppress duplicate entries")),
+		OPT_BOOL(0, "deduplicate", &skipping_duplicates,
+			 N_("suppress duplicate entries")),
 		OPT_END()
 	};
ZheNing Hu Jan. 25, 2021, 6:05 a.m. UTC | #2
OK,I didn’t notice any formatting changes before.

Am I free from this patch now?I should probably
look for other issues.

Junio, thank you for all your patient help.
I may often make some low-level mistakes.
I am grateful.

Cheers.

Junio C Hamano <gitster@pobox.com> 于2021年1月25日周一 上午6:04写道:
>
> "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>
> > [jc: fixed misindented code]
>
> I noticed that you reverted my fix in this version, when this is
> compared with the one I sent last night.
>
> Comparing the result of applying all three with what I sent last
> night, this v7 looks worse (see below).  Let's discard this round
> and declare victory with what is already on 'seen'.
>
> Thanks.
>
>
> ---
>
> comparison between what these three patches would produce (preimage)
> and what is on 'seen' (postimage)is shown here.
>
> diff --git w/builtin/ls-files.c c/builtin/ls-files.c
> index fb9cf50d76..f6f9e483b2 100644
> --- w/builtin/ls-files.c
> +++ c/builtin/ls-files.c
> @@ -313,7 +313,8 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
>                 if (show_killed)
>                         show_killed_files(repo->index, dir);
>         }
> -       if (! (show_cached || show_stage || show_deleted || show_modified))
> +
> +       if (!(show_cached || show_stage || show_deleted || show_modified))
>                 return;
>         for (i = 0; i < repo->index->cache_nr; i++) {
>                 const struct cache_entry *ce = repo->index->cache[i];
> @@ -328,15 +329,16 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
>                 if (ce->ce_flags & CE_UPDATE)
>                         continue;
>                 if ((show_cached || show_stage) &&
> -                       (!show_unmerged || ce_stage(ce))) {
> -                               show_ce(repo, dir, ce, fullname.buf,
> -                                       ce_stage(ce) ? tag_unmerged :
> -                                       (ce_skip_worktree(ce) ? tag_skip_worktree :
> -                                               tag_cached));
> +                   (!show_unmerged || ce_stage(ce))) {
> +                       show_ce(repo, dir, ce, fullname.buf,
> +                               ce_stage(ce) ? tag_unmerged :
> +                               (ce_skip_worktree(ce) ? tag_skip_worktree :
> +                                tag_cached));
>                         if (skipping_duplicates)
>                                 goto skip_to_next_name;
>                 }
> -               if (!show_deleted && !show_modified)
> +
> +               if (!(show_deleted || show_modified))
>                         continue;
>                 if (ce_skip_worktree(ce))
>                         continue;
> @@ -349,12 +351,13 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
>                                 goto skip_to_next_name;
>                 }
>                 if (show_modified &&
> -                       (stat_err || ie_modified(repo->index, ce, &st, 0))) {
> -                               show_ce(repo, dir, ce, fullname.buf, tag_modified);
> +                   (stat_err || ie_modified(repo->index, ce, &st, 0))) {
> +                       show_ce(repo, dir, ce, fullname.buf, tag_modified);
>                         if (skipping_duplicates)
>                                 goto skip_to_next_name;
>                 }
>                 continue;
> +
>  skip_to_next_name:
>                 {
>                         int j;
> @@ -362,7 +365,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
>                         for (j = i + 1; j < repo->index->cache_nr; j++)
>                                 if (strcmp(ce->name, cache[j]->name))
>                                         break;
> -                       i = j - 1; /* compensate for outer for loop */
> +                       i = j - 1; /* compensate for the for loop */
>                 }
>         }
>
> @@ -590,7 +593,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>                         N_("pretend that paths removed since <tree-ish> are still present")),
>                 OPT__ABBREV(&abbrev),
>                 OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
> -               OPT_BOOL(0,"deduplicate",&skipping_duplicates,N_("suppress duplicate entries")),
> +               OPT_BOOL(0, "deduplicate", &skipping_duplicates,
> +                        N_("suppress duplicate entries")),
>                 OPT_END()
>         };
>
Junio C Hamano Jan. 25, 2021, 7:05 p.m. UTC | #3
胡哲宁 <adlternative@gmail.com> writes:

> OK,I didn’t notice any formatting changes before.
>
> Am I free from this patch now?I should probably
> look for other issues.

I think we are pretty much done with it.  Thanks for working on the
topic so patiently.
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);
 		}
 	}