threads / patch / 23409

patch, 5 partswt-status: remove unused workdir_untracked field

Subject: [PATCH 1/5] wt-status: remove unused workdir_untracked field

## tl;dr

14 messages between Apr 10, 2010 and Apr 11, 2010. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Junio C Hamano· Apr 10, 2010, 07:40 UTC · lore

[PATCH 0/5] "status --ignored"

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
>> If ignored files are not listed, that's another problem. Even more
>> serious, actually.
>
> See above.
>
> It wouldn't be too hard to add them in, and would look something like
> the patch below.
As I didn't see a patch, I did a rough outline just for fun.
Junio C Hamano (5):
      wt-status: remove unused workdir_untracked field
      wt-status: plug memory leak while collecting untracked files
      wt-status: collect ignored files
      wt-status: rename and restructure status-print-untracked
      status: --ignored option shows ignored files
Junio C Hamano· Apr 10, 2010, 07:40 UTC · re: Junio C Hamano · lore
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wt-status.c |    1 -
 wt-status.h |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)
Show changes to 2 files +0 −2

wt-status.c, wt-status.h

diff --git a/wt-status.c b/wt-status.c
index 8ca59a2..db20b86 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -378,7 +378,6 @@ static void wt_status_collect_untracked(struct wt_status *s)
 			continue;
 		if (!match_pathspec(s->pathspec, ent->name, ent->len, 0, NULL))
 			continue;
-		s->workdir_untracked = 1;
 		string_list_insert(ent->name, &s->untracked);
 	}
 }
diff --git a/wt-status.h b/wt-status.h
index 9120673..2c49f46 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -47,7 +47,6 @@ struct wt_status {
 	/* These are computed during processing of the individual sections */
 	int commitable;
 	int workdir_dirty;
-	int workdir_untracked;
 	const char *index_file;
 	FILE *fp;
 	const char *prefix;

-- 
1.7.1.rc0.264.g94f6e
Junio C Hamano· Apr 10, 2010, 07:40 UTC · re: Junio C Hamano · lore

[PATCH 2/5] wt-status: plug memory leak while collecting untracked files

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wt-status.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Show changes to wt-status.c +3 −0
diff --git a/wt-status.c b/wt-status.c
index db20b86..c88159a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -379,7 +379,10 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		if (!match_pathspec(s->pathspec, ent->name, ent->len, 0, NULL))
 			continue;
 		string_list_insert(ent->name, &s->untracked);
+		free(ent);
 	}
+
+	free(dir.entries);
 }
 
 void wt_status_collect(struct wt_status *s)

-- 
1.7.1.rc0.264.g94f6e
Junio C Hamano· Apr 10, 2010, 07:40 UTC · re: Junio C Hamano · lore

[PATCH 3/5] wt-status: collect ignored files

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wt-status.c |   16 ++++++++++++++++
 wt-status.h |    2 ++
 2 files changed, 18 insertions(+), 0 deletions(-)
Show changes to 2 files +18 −0

wt-status.c, wt-status.h

diff --git a/wt-status.c b/wt-status.c
index c88159a..f13c7da 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -42,6 +42,7 @@ void wt_status_prepare(struct wt_status *s)
 	s->index_file = get_index_file();
 	s->change.strdup_strings = 1;
 	s->untracked.strdup_strings = 1;
+	s->ignored.strdup_strings = 1;
 }
 
 static void wt_status_print_unmerged_header(struct wt_status *s)
@@ -382,6 +383,21 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		free(ent);
 	}
 
+	if (s->show_ignored_files) {
+		dir.nr = 0;
+		dir.flags = DIR_SHOW_IGNORED | DIR_SHOW_OTHER_DIRECTORIES;
+		fill_directory(&dir, s->pathspec);
+		for (i = 0; i < dir.nr; i++) {
+			struct dir_entry *ent = dir.entries[i];
+			if (!cache_name_is_other(ent->name, ent->len))
+				continue;
+			if (!match_pathspec(s->pathspec, ent->name, ent->len, 0, NULL))
+				continue;
+			string_list_insert(ent->name, &s->ignored);
+			free(ent);
+		}
+	}
+
 	free(dir.entries);
 }
 
diff --git a/wt-status.h b/wt-status.h
index 2c49f46..1093e65 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -41,6 +41,7 @@ struct wt_status {
 	int use_color;
 	int relative_paths;
 	int submodule_summary;
+	int show_ignored_files;
 	enum untracked_status_type show_untracked_files;
 	char color_palette[WT_STATUS_UNMERGED+1][COLOR_MAXLEN];
 
@@ -52,6 +53,7 @@ struct wt_status {
 	const char *prefix;
 	struct string_list change;
 	struct string_list untracked;
+	struct string_list ignored;
 };
 
 void wt_status_prepare(struct wt_status *s);

-- 
1.7.1.rc0.264.g94f6e
Jeff King· Apr 10, 2010, 07:48 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/5] wt-status: collect ignored files

On Sat, Apr 10, 2010 at 12:40:54AM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> +	if (s->show_ignored_files) {
> +		dir.nr = 0;
> +		dir.flags = DIR_SHOW_IGNORED | DIR_SHOW_OTHER_DIRECTORIES;
> +		fill_directory(&dir, s->pathspec);
> +		for (i = 0; i < dir.nr; i++) {
> +			struct dir_entry *ent = dir.entries[i];
> +			if (!cache_name_is_other(ent->name, ent->len))
> +				continue;
> +			if (!match_pathspec(s->pathspec, ent->name, ent->len, 0, NULL))
> +				continue;
> +			string_list_insert(ent->name, &s->ignored);
> +			free(ent);
> +		}
> +	}
> +

Why not use DIR_COLLECT_IGNORED, which would mean we only have to traverse the directory tree once?

-Peff
Junio C Hamano· Apr 10, 2010, 07:40 UTC · re: Junio C Hamano · lore

[PATCH 4/5] wt-status: rename and restructure status-print-untracked

I will be reusing this to show ignored stuff in the next patch.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wt-status.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)
Show changes to wt-status.c +14 −8
diff --git a/wt-status.c b/wt-status.c
index f13c7da..2c9a05d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -97,13 +97,15 @@ static void wt_status_print_dirty_header(struct wt_status *s,
 	color_fprintf_ln(s->fp, c, "#");
 }
 
-static void wt_status_print_untracked_header(struct wt_status *s)
+static void wt_status_print_other_header(struct wt_status *s,
+					 const char *what,
+					 const char *how)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
-	color_fprintf_ln(s->fp, c, "# Untracked files:");
+	color_fprintf_ln(s->fp, c, "# %s files:", what);
 	if (!advice_status_hints)
 		return;
-	color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to include in what will be committed)");
+	color_fprintf_ln(s->fp, c, "#   (use \"git %s <file>...\" to include in what will be committed)", how);
 	color_fprintf_ln(s->fp, c, "#");
 }
 
@@ -541,7 +543,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 	run_command(&sm_summary);
 }
 
-static void wt_status_print_untracked(struct wt_status *s)
+static void wt_status_print_other(struct wt_status *s,
+				  struct string_list *l,
+				  const char *what,
+				  const char *how)
 {
 	int i;
 	struct strbuf buf = STRBUF_INIT;
@@ -549,10 +554,11 @@ static void wt_status_print_untracked(struct wt_status *s)
 	if (!s->untracked.nr)
 		return;
 
-	wt_status_print_untracked_header(s);
-	for (i = 0; i < s->untracked.nr; i++) {
+	wt_status_print_other_header(s, what, how);
+
+	for (i = 0; i < l->nr; i++) {
 		struct string_list_item *it;
-		it = &(s->untracked.items[i]);
+		it = &(l->items[i]);
 		color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "#\t");
 		color_fprintf_ln(s->fp, color(WT_STATUS_UNTRACKED, s), "%s",
 				 quote_path(it->string, strlen(it->string),
@@ -641,7 +647,7 @@ void wt_status_print(struct wt_status *s)
 		wt_status_print_submodule_summary(s, 1);  /* unstaged */
 	}
 	if (s->show_untracked_files)
-		wt_status_print_untracked(s);
+		wt_status_print_other(s, &s->untracked, "Untracked", "add");
 	else if (s->commitable)
 		 fprintf(s->fp, "# Untracked files not listed (use -u option to show untracked files)\n");
 

-- 
1.7.1.rc0.264.g94f6e
Junio C Hamano· Apr 10, 2010, 07:40 UTC · re: Junio C Hamano · lore

[PATCH 5/5] status: --ignored option shows ignored files

There is no stronger reason behind the choice of "!!" than just I happened to have typed them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c |    6 +++++-
 wt-status.c      |   22 +++++++++++++++-------
 2 files changed, 20 insertions(+), 8 deletions(-)
Show changes to 2 files +20 −8

builtin/commit.c, wt-status.c

diff --git a/builtin/commit.c b/builtin/commit.c
index c5ab683..761ca07 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -83,6 +83,7 @@ static enum {
 static char *cleanup_arg;
 
 static int use_editor = 1, initial_commit, in_merge, include_status = 1;
+static int show_ignored_in_status;
 static const char *only_include_assumed;
 static struct strbuf message;
 
@@ -1031,6 +1032,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		  "mode",
 		  "show untracked files, optional modes: all, normal, no. (Default: all)",
 		  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
+		OPT_BOOLEAN(0, "ignored", &show_ignored_in_status,
+			    "show ignored files"),
 		OPT_END(),
 	};
 
@@ -1044,7 +1047,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			     builtin_status_options,
 			     builtin_status_usage, 0);
 	handle_untracked_files_arg(&s);
-
+	if (show_ignored_in_status)
+		s.show_ignored_files = 1;
 	if (*argv)
 		s.pathspec = get_pathspec(prefix, argv);
 
diff --git a/wt-status.c b/wt-status.c
index 2c9a05d..7bda995 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -646,9 +646,11 @@ void wt_status_print(struct wt_status *s)
 		wt_status_print_submodule_summary(s, 0);  /* staged */
 		wt_status_print_submodule_summary(s, 1);  /* unstaged */
 	}
-	if (s->show_untracked_files)
+	if (s->show_untracked_files) {
 		wt_status_print_other(s, &s->untracked, "Untracked", "add");
-	else if (s->commitable)
+		if (s->show_ignored_files)
+			wt_status_print_other(s, &s->ignored, "Ignored", "add -f");
+	} else if (s->commitable)
 		 fprintf(s->fp, "# Untracked files not listed (use -u option to show untracked files)\n");
 
 	if (s->verbose)
@@ -730,16 +732,16 @@ static void wt_shortstatus_status(int null_termination, struct string_list_item
 	}
 }
 
-static void wt_shortstatus_untracked(int null_termination, struct string_list_item *it,
-			    struct wt_status *s)
+static void wt_shortstatus_other(int null_termination, struct string_list_item *it,
+				 struct wt_status *s, const char *sign)
 {
 	if (null_termination) {
-		fprintf(stdout, "?? %s%c", it->string, 0);
+		fprintf(stdout, "%s %s%c", sign, it->string, 0);
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
 		one = quote_path(it->string, -1, &onebuf, s->prefix);
-		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "??");
+		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), sign);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
 	}
@@ -763,7 +765,13 @@ void wt_shortstatus_print(struct wt_status *s, int null_termination)
 		struct string_list_item *it;
 
 		it = &(s->untracked.items[i]);
-		wt_shortstatus_untracked(null_termination, it, s);
+		wt_shortstatus_other(null_termination, it, s, "??");
+	}
+	for (i = 0; i < s->ignored.nr; i++) {
+		struct string_list_item *it;
+
+		it = &(s->ignored.items[i]);
+		wt_shortstatus_other(null_termination, it, s, "!!");
 	}
 }
 

-- 
1.7.1.rc0.264.g94f6e
Jeff King· Apr 10, 2010, 07:44 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/5] "status --ignored"

On Sat, Apr 10, 2010 at 12:40:51AM -0700, Junio C Hamano wrote:
> > It wouldn't be too hard to add them in, and would look something like
> > the patch below.
> 
> As I didn't see a patch, I did a rough outline just for fun.
Gah. Yours is undoubtedly less ugly, but here was mine for reference.
Show changes to 3 files +22 −5

dir.c, wt-status.c, wt-status.h

diff --git a/dir.c b/dir.c
index cb83332..deee7c8 100644
--- a/dir.c
+++ b/dir.c
@@ -619,7 +619,9 @@ static int exclude_matches_pathspec(const char *path, int len,
 				return 1;
 		}
 	}
-	return 0;
+	/* ugh, I think this may be a long-standing bug, but
+	 * this code was never reachable before. */
+	return 1;
 }
 
 static int get_index_dtype(const char *path, int len)
diff --git a/wt-status.c b/wt-status.c
index 8ca59a2..53a7a5a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -366,6 +366,8 @@ static void wt_status_collect_untracked(struct wt_status *s)
 	if (!s->show_untracked_files)
 		return;
 	memset(&dir, 0, sizeof(dir));
+	/* should be conditional on s->show_ignored_files */
+	dir.flags |= DIR_COLLECT_IGNORED;
 	if (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)
 		dir.flags |=
 			DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
@@ -381,6 +383,14 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		s->workdir_untracked = 1;
 		string_list_insert(ent->name, &s->untracked);
 	}
+	for (i = 0; i < dir.ignored_nr; i++) {
+		struct dir_entry *ent = dir.ignored[i];
+		if (!cache_name_is_other(ent->name, ent->len))
+			continue;
+		if (!match_pathspec(s->pathspec, ent->name, ent->len, 0, NULL))
+			continue;
+		string_list_insert(ent->name, &s->ignored);
+	}
 }
 
 void wt_status_collect(struct wt_status *s)
@@ -707,15 +717,15 @@ static void wt_shortstatus_status(int null_termination, struct string_list_item
 }
 
 static void wt_shortstatus_untracked(int null_termination, struct string_list_item *it,
-			    struct wt_status *s)
+			    struct wt_status *s, const char *marker)
 {
 	if (null_termination) {
-		fprintf(stdout, "?? %s%c", it->string, 0);
+		fprintf(stdout, "%s %s%c", marker, it->string, 0);
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
 		one = quote_path(it->string, -1, &onebuf, s->prefix);
-		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "??");
+		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), marker);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
 	}
@@ -739,7 +749,11 @@ void wt_shortstatus_print(struct wt_status *s, int null_termination)
 		struct string_list_item *it;
 
 		it = &(s->untracked.items[i]);
-		wt_shortstatus_untracked(null_termination, it, s);
+		wt_shortstatus_untracked(null_termination, it, s, "??");
+	}
+	for (i = 0; i < s->ignored.nr; i++) {
+		struct string_list_item *it = &(s->ignored.items[i]);
+		wt_shortstatus_untracked(null_termination, it, s, "II");
 	}
 }
 
diff --git a/wt-status.h b/wt-status.h
index 9120673..0f5cf38 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -53,6 +53,7 @@ struct wt_status {
 	const char *prefix;
 	struct string_list change;
 	struct string_list untracked;
+	struct string_list ignored;
 };
 
 void wt_status_prepare(struct wt_status *s);
Junio C Hamano· Apr 10, 2010, 07:48 UTC · re: Jeff King · lore

Re: [PATCH 0/5] "status --ignored"

Jeff King <peff@peff.net> writes:
> +	dir.flags |= DIR_COLLECT_IGNORED;

I thought about using collect-ignored so that fill_directory() does not have to be run twice, but IIRC it can short-circuit an entire directory using the "simplify" logic (as it was designed to be used for "git add"), no?

Mine is not "less ugly", but just "simple and stupid" ;-).
Jeff King· Apr 10, 2010, 08:40 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/5] "status --ignored"

On Sat, Apr 10, 2010 at 12:48:49AM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > +	dir.flags |= DIR_COLLECT_IGNORED;
> 
> I thought about using collect-ignored so that fill_directory() does not
> have to be run twice, but IIRC it can short-circuit an entire directory
> using the "simplify" logic (as it was designed to be used for "git add"),
> no?

Hmm. I didn't consider that. It may be sufficient to say "foo/ is ignored" and let callers decide for themselves that "foo/bar" is ignored, too (unless, of course, it appears as tracked earlier in the list).

I just worry that your version will perform less well. I'm not that worried about the double traversal (on any reasonable system everything will be cached after the first traversal). But finding every file means we have to traverse areas that git otherwise wouldn't look at, which might mean going to disk to pull in all of the "foo/" directory structure (which is less likely to be cached, since the rest of git isn't touching it), even though it may not even be of interest to us.

-Peff
Eric Raymond· Apr 10, 2010, 14:41 UTC · re: Jeff King · lore

Re: [PATCH 0/5] "status --ignored"

Jeff King <peff@peff.net>:
Show 15 quoted lines
> On Sat, Apr 10, 2010 at 12:48:49AM -0700, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > +	dir.flags |= DIR_COLLECT_IGNORED;
> > 
> > I thought about using collect-ignored so that fill_directory() does not
> > have to be run twice, but IIRC it can short-circuit an entire directory
> > using the "simplify" logic (as it was designed to be used for "git add"),
> > no?
> 
> Hmm.  I didn't consider that. It may be sufficient to say "foo/ is
> ignored" and let callers decide for themselves that "foo/bar" is
> ignored, too (unless, of course, it appears as tracked earlier in the
> list).

Speaking as a front-end author, I would prefer that all ignored paths be listed so I don't have to do globbing.

-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
Junio C Hamano· Apr 10, 2010, 18:27 UTC · re: Jeff King · lore

Re: [PATCH 0/5] "status --ignored"

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> ... But finding every file means
> we have to traverse areas that git otherwise wouldn't look at, which
> might mean going to disk to pull in all of the "foo/" directory
> structure (which is less likely to be cached, since the rest of git
> isn't touching it), even though it may not even be of interest to us.

Yes, that is why it is adequate for us use COLLECT_IGNORED in "git add" and give "foo/ is outside---you as an intelligent human should be able to infer that your foo/bar is also" without double traversal.

Eric's tool might want the same abbreviated information if it is just relaying our output to an intelligent human. Or it might want to get all paths if it wants to operate on them itself. Knowing VC I chose to illustrate how to do the latter, but in the real implementation, we may want --show-ignored=normal vs --show-ignored=expanded to support both uses.

Jakub Narebski· Apr 10, 2010, 19:20 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/5] "status --ignored"

Junio C Hamano <gitster@pobox.com> writes:
Show 18 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > ... But finding every file means
> > we have to traverse areas that git otherwise wouldn't look at, which
> > might mean going to disk to pull in all of the "foo/" directory
> > structure (which is less likely to be cached, since the rest of git
> > isn't touching it), even though it may not even be of interest to us.
> 
> Yes, that is why it is adequate for us use COLLECT_IGNORED in "git add"
> and give "foo/ is outside---you as an intelligent human should be able to
> infer that your foo/bar is also" without double traversal.
> 
> Eric's tool might want the same abbreviated information if it is just
> relaying our output to an intelligent human.  Or it might want to get all
> paths if it wants to operate on them itself.  Knowing VC I chose to
> illustrate how to do the latter, but in the real implementation, we may
> want --show-ignored=normal vs --show-ignored=expanded to support both
> uses.

Or rather -i / --ignored-files[=(no|normal|all)], default to 'normal' in the '--ignored-files' form (without optional <mode>), similarly to currently existing -u / --untracked-files option to git-status.

Let's not introduce yet another CLI inconsistency^TM in Git... ;-)
-- 
Jakub Narebski
Poland
ShadeHawk on #git
Jeff King· Apr 11, 2010, 10:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/5] "status --ignored"

On Sat, Apr 10, 2010 at 11:27:39AM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> > ... But finding every file means
> > we have to traverse areas that git otherwise wouldn't look at, which
> > might mean going to disk to pull in all of the "foo/" directory
> > structure (which is less likely to be cached, since the rest of git
> > isn't touching it), even though it may not even be of interest to us.
> 
> Yes, that is why it is adequate for us use COLLECT_IGNORED in "git add"
> and give "foo/ is outside---you as an intelligent human should be able to
> infer that your foo/bar is also" without double traversal.

It would be adequate here, too, if we want to know whether a specific file is ignored. It just takes more work from the caller. But Eric has already said he would prefer to just get all files, so let's go with what you wrote.

If another caller wants a more restricted but efficient interface later, we can add it as you suggest (or more likely, they want to check a particular _set_ of files, so we don't want to do a full traversal anyway. We would want to get their list of candidates and traverse just enough of the .gitignore stack to get answers for their set).

-Peff

← back to recent threads