Re: What's cooking in git.git (Jan 2013, #06; Mon, 14)

Adam Spiers [off-list ref] writes:

On Mon, Jan 14, 2013 at 10:23 PM, Junio C Hamano [off-list ref] wrote:

quoted

* as/check-ignore (2013-01-10) 12 commits
  (merged to 'next' on 2013-01-14 at 9df2afc)
 + t0008: avoid brace expansion
 + add git-check-ignore sub-command
 + setup.c: document get_pathspec()
 + add.c: extract new die_if_path_beyond_symlink() for reuse
 + add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse
 + pathspec.c: rename newly public functions for clarity
 + add.c: move pathspec matchers into new pathspec.c for reuse
 + add.c: remove unused argument from validate_pathspec()
 + dir.c: improve docs for match_pathspec() and match_pathspec_depth()
 + dir.c: provide clear_directory() for reclaiming dir_struct memory
 + dir.c: keep track of where patterns came from
 + dir.c: use a single struct exclude_list per source of excludes

 Add a new command "git check-ignore" for debugging .gitignore
 files.

The above is v4 plus the "t0008: avoid brace expansion" fix.  v4 is
slightly outdated and not quite the right version to merge to 'next'.

Sigh.

The "What's cooking" is a report of what _has_ already happened.  I
would have appreciated if you said the above _before_ this happened.

I'll post a v5 re-roll as per:

Now the series is in 'next', it is too late to _replace_ it X-<.
Could you instead make an incremental updates on top?  That way, we
do not have to re-review the whole thing; we only need to review the
changes relative to the old one, making sure that the fixes in the
updates are better than the v4 version.

Thanks.

On Wed, Jan 16, 2013 at 2:37 AM, Junio C Hamano [off-list ref] wrote:

Adam Spiers [off-list ref] writes:

quoted

On Mon, Jan 14, 2013 at 10:23 PM, Junio C Hamano [off-list ref] wrote:

quoted

* as/check-ignore (2013-01-10) 12 commits
  (merged to 'next' on 2013-01-14 at 9df2afc)
 + t0008: avoid brace expansion
 + add git-check-ignore sub-command
 + setup.c: document get_pathspec()
 + add.c: extract new die_if_path_beyond_symlink() for reuse
 + add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse
 + pathspec.c: rename newly public functions for clarity
 + add.c: move pathspec matchers into new pathspec.c for reuse
 + add.c: remove unused argument from validate_pathspec()
 + dir.c: improve docs for match_pathspec() and match_pathspec_depth()
 + dir.c: provide clear_directory() for reclaiming dir_struct memory
 + dir.c: keep track of where patterns came from
 + dir.c: use a single struct exclude_list per source of excludes

 Add a new command "git check-ignore" for debugging .gitignore
 files.

The above is v4 plus the "t0008: avoid brace expansion" fix.  v4 is
slightly outdated and not quite the right version to merge to 'next'.

Sigh.

The "What's cooking" is a report of what _has_ already happened.  I
would have appreciated if you said the above _before_ this happened.

I did, 8 days ago in the link which you just trimmed from your reply:

  http://thread.gmane.org/gmane.comp.version-control.git/212184/focus=212856

The additional issues with t0008 were discovered after I posted v4, as
reflected in last Wednesday's "What's cooking" (#04):

    "The test it adds seems to break under dash.
     Expecting a reroll or fixup."

I assumed that for brevity you had deliberately omitted mentioning the
outstanding dir.exclude_list_group[EXC_CMDL].el[0] encapsulation
issue linked above, so I thought we were aligned at this point.

However I see now that you changed the status to an intention to merge
this to 'next' in last Friday's "What's cooking" (#05).  That gave me
a window of under 72 hours in which to reiterate the need for a final
re-roll.  Unfortunately with other commitments and illness over the
weekend, I didn't catch this in time.

However, the damage is very small:

quoted

I'll post a v5 re-roll as per:

Now the series is in 'next', it is too late to _replace_ it X-<.
Could you instead make an incremental updates on top?  That way, we
do not have to re-review the whole thing; we only need to review the
changes relative to the old one, making sure that the fixes in the
updates are better than the v4 version.

Sure, that's easy to do.  It'll be a single small patch very similar
to this one:

  http://article.gmane.org/gmane.comp.version-control.git/212852

minus the superfluous printf() debug statements.  I'll do that now.

Thanks,
Adam

Consumers of the dir.c traversal API should avoid assuming knowledge
of the internal implementation of exclude_list_groups.  Therefore
when adding items to an exclude list, it should be accessed via the
pointer returned from add_exclude_list(), rather than by referencing
a location within dir.exclude_list_groups[EXC_CMDL].

Signed-off-by: Adam Spiers <redacted>
---
 builtin/clean.c    |  6 +++---
 builtin/ls-files.c | 15 ++++++++++-----
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index b098288..b9cb7ad 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c

@@ -45,6 +45,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	static const char **pathspec;
 	struct strbuf buf = STRBUF_INIT;
 	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
+	struct exclude_list *el;
 	const char *qname;
 	char *seen = NULL;
 	struct option options[] = {

@@ -97,10 +98,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	if (!ignored)
 		setup_standard_excludes(&dir);
 
-	add_exclude_list(&dir, EXC_CMDL, "--exclude option");
+	el = add_exclude_list(&dir, EXC_CMDL, "--exclude option");
 	for (i = 0; i < exclude_list.nr; i++)
-		add_exclude(exclude_list.items[i].string, "", 0,
-			    &dir.exclude_list_group[EXC_CMDL].el[0], -(i+1));
+		add_exclude(exclude_list.items[i].string, "", 0, el, -(i+1));
 
 	pathspec = get_pathspec(prefix, argv);
 

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index fa9ccb8..b4d8b01 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c

@@ -421,10 +421,10 @@ static int option_parse_z(const struct option *opt,
 static int option_parse_exclude(const struct option *opt,
 				const char *arg, int unset)
 {
-	struct exclude_list_group *group = opt->value;
+	struct string_list *exclude_list = opt->value;
 
 	exc_given = 1;
-	add_exclude(arg, "", 0, &group->el[0], --exclude_args);
+	string_list_append(exclude_list, arg);
 
 	return 0;
 }

@@ -453,9 +453,11 @@ static int option_parse_exclude_standard(const struct option *opt,
 
 int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 {
-	int require_work_tree = 0, show_tag = 0;
+	int require_work_tree = 0, show_tag = 0, i;
 	const char *max_prefix;
 	struct dir_struct dir;
+	struct exclude_list *el;
+	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 	struct option builtin_ls_files_options[] = {
 		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
 			"paths are separated with NUL character",

@@ -490,7 +492,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		OPT_BOOLEAN(0, "resolve-undo", &show_resolve_undo,
 			    "show resolve-undo information"),
 		{ OPTION_CALLBACK, 'x', "exclude",
-			&dir.exclude_list_group[EXC_CMDL], "pattern",
+			&exclude_list, "pattern",
 			"skip files matching pattern",
 			0, option_parse_exclude },
 		{ OPTION_CALLBACK, 'X', "exclude-from", &dir, "file",

@@ -525,9 +527,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (read_cache() < 0)
 		die("index file corrupt");
 
-	add_exclude_list(&dir, EXC_CMDL, "--exclude option");
 	argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
 			ls_files_usage, 0);
+	el = add_exclude_list(&dir, EXC_CMDL, "--exclude option");
+	for (i = 0; i < exclude_list.nr; i++) {
+		add_exclude(exclude_list.items[i].string, "", 0, el, --exclude_args);
+	}
 	if (show_tag || show_valid_bit) {
 		tag_cached = "H ";
 		tag_unmerged = "M ";

-- 
1.8.1.291.g0730ed6