threads / patch / 26701

patch, 5 partssupport --exclude for diff/log commands

Subject: [WIP PATCH 0/5] support --exclude for diff/log commands

## tl;dr

20 messages between Mar 10, 2011 and Mar 23, 2011. Diffs are folded; open one to read it.

replies: 19people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Mar 10, 2011, 03:13 UTC · lore

This is not really ready for review or use, but I'd like to have feedback if any, where this series is heading because it requires some code refactor in diff/rev machinery. It does not feel absolutely right to me.

On the other hand, if we go with ':' as a mark of special pathspecs, then
 - ":/" mark pathspecs relative to worktree root
 - ":!" may mark negative pathspecs
Less changes in diff/rev this way.
Nguyễn Thái Ngọc Duy (5):
  tree-walk: support negative pathspec
  match_pathspec_depth: support negative pathspec

match_pathspec_depth needs work. tree_entry_interesting() can use some optimization, but leave it for now.

  revision.c: get rid of struct rev_info.prune_data
  diff: refactor init/release API

These two facilitate post pathspec manipulation to transform --exclude to pathspecs with to_exclude = 1.

  diff: support --exclude
 Documentation/technical/api-diff.txt |    2 +-
 builtin/add.c                        |    3 +-
 builtin/blame.c                      |   22 +++++----------
 builtin/diff.c                       |    6 ++--
 builtin/fast-export.c                |    2 +-
 builtin/merge.c                      |    2 +-
 builtin/reset.c                      |    4 +-
 cache.h                              |    1 +
 diff-lib.c                           |    6 ++--
 diff-no-index.c                      |    6 ++--
 diff.c                               |   49 ++++++++++++++++++++++++++++++++-
 diff.h                               |    6 ++--
 dir.c                                |    5 +++
 merge-recursive.c                    |    2 +-
 notes-merge.c                        |    8 +++---
 patch-ids.c                          |    2 +-
 revision.c                           |   22 ++++++++-------
 revision.h                           |    1 -
 tree-diff.c                          |   21 +++------------
 tree-walk.c                          |   37 +++++++++++++++++++------
 wt-status.c                          |    6 +---
 21 files changed, 130 insertions(+), 83 deletions(-)
-- 
1.7.3.1.256.g2539c.dirty
Nguyễn Thái Ngọc Duy· Mar 10, 2011, 03:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/5] tree-walk: support negative pathspec

While the UI will be simple, where negative pathspecs are always appended in the end, this could should be able to handle a mixed set of negative and positive pathspecs (ie. overlapping must be handled correctly).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache.h     |    1 +
 tree-walk.c |   37 ++++++++++++++++++++++++++++---------
 2 files changed, 29 insertions(+), 9 deletions(-)
Show changes to 2 files +29 −9

cache.h, tree-walk.c

diff --git a/cache.h b/cache.h
index 08a9022..2189366 100644
--- a/cache.h
+++ b/cache.h
@@ -510,6 +510,7 @@ struct pathspec {
 		const char *match;
 		int len;
 		int has_wildcard:1;
+		int to_exclude:1;
 	} *items;
 };
 
diff --git a/tree-walk.c b/tree-walk.c
index 322becc..acbee1b 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -560,6 +560,7 @@ int tree_entry_interesting(const struct name_entry *entry,
 	int i;
 	int pathlen, baselen = base->len - base_offset;
 	int never_interesting = ps->has_wildcard ? 0 : -1;
+	int all_interesting_ok = !ps->recursive || ps->max_depth == -1;
 
 	if (!ps->nr) {
 		if (!ps->recursive || ps->max_depth == -1)
@@ -569,6 +570,19 @@ int tree_entry_interesting(const struct name_entry *entry,
 				      ps->max_depth);
 	}
 
+	/*
+	 * TODO: add prepare_pathspec() to scan for pathspecs
+	 * overlapped with negative ones. The ones that do not overlap
+	 * can still return 2.
+	 */
+	for (i = 0; i < ps->nr; i++) {
+		const struct pathspec_item *item = ps->items+i;
+		if (item->to_exclude) {
+			all_interesting_ok = 0;
+			break;
+		}
+	}
+
 	pathlen = tree_entry_len(entry->path, entry->sha1);
 
 	for (i = ps->nr - 1; i >= 0; i--) {
@@ -578,17 +592,22 @@ int tree_entry_interesting(const struct name_entry *entry,
 		int matchlen = item->len;
 
 		if (baselen >= matchlen) {
+			int ret;
+
 			/* If it doesn't match, move along... */
 			if (!match_dir_prefix(base_str, baselen, match, matchlen))
 				goto match_wildcards;
 
-			if (!ps->recursive || ps->max_depth == -1)
+			if (all_interesting_ok)
 				return 2;
 
-			return !!within_depth(base_str + matchlen + 1,
-					      baselen - matchlen - 1,
-					      !!S_ISDIR(entry->mode),
-					      ps->max_depth);
+			ret = !!within_depth(base_str + matchlen + 1,
+					     baselen - matchlen - 1,
+					     !!S_ISDIR(entry->mode),
+					     ps->max_depth);
+			if (item->to_exclude)
+				ret = !ret;
+			return ret;
 		}
 
 		/* Does the base match? */
@@ -596,18 +615,18 @@ int tree_entry_interesting(const struct name_entry *entry,
 			if (match_entry(entry, pathlen,
 					match + baselen, matchlen - baselen,
 					&never_interesting))
-				return 1;
+				return item->to_exclude ? 0 : 1;
 
 			if (ps->items[i].has_wildcard) {
 				if (!fnmatch(match + baselen, entry->path, 0))
-					return 1;
+					return item->to_exclude ? 0 : 1;
 
 				/*
 				 * Match all directories. We'll try to
 				 * match files later on.
 				 */
 				if (ps->recursive && S_ISDIR(entry->mode))
-					return 1;
+					return item->to_exclude ? 0 : 1;
 			}
 
 			continue;
@@ -626,7 +645,7 @@ match_wildcards:
 
 		if (!fnmatch(match, base->buf + base_offset, 0)) {
 			strbuf_setlen(base, base_offset + baselen);
-			return 1;
+			return item->to_exclude ? 0 : 1;
 		}
 		strbuf_setlen(base, base_offset + baselen);
 

-- 
1.7.3.1.256.g2539c.dirty
Nguyễn Thái Ngọc Duy· Mar 10, 2011, 03:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/5] match_pathspec_depth: support negative pathspec

The return value of this function is bugging me, as well as seen[] array. Not really sure how to do with them when a negative pathspec is matched.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 dir.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Show changes to dir.c +5 −0
diff --git a/dir.c b/dir.c
index 168dad6..75c4a8f 100644
--- a/dir.c
+++ b/dir.c
@@ -279,6 +279,11 @@ int match_pathspec_depth(const struct pathspec *ps,
 				how = 0;
 		}
 		if (how) {
+			if (ps->items[i].to_exclude) {
+				if (seen && seen[i] < how)
+					seen[i] = how;
+				return 0;
+			}
 			if (retval < how)
 				retval = how;
 			if (seen && seen[i] < how)

-- 
1.7.3.1.256.g2539c.dirty
Nguyễn Thái Ngọc Duy· Mar 10, 2011, 03:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 3/5] revision.c: get rid of struct rev_info.prune_data

There are three struct pathspec(s) in struct rev_info:
 - prune_data
 - pruning.pathspec
 - diffopt.pathspec

In some places, this pathspec is used, in other places another one. I'd like to have only one pathspec in struct rev_info, but i'm not sure why diffopt can't be used in place of pruning. So in the meantime, prune_data will be gone.

The driving force behind this is now diff_options.pathspec can be manipulated behind the scene by diff_setup_done() to support --exclude. Rev machinery does not know about this and does not need to. Remove prune_data in favor of diffopt.pathspec, handing complex pathspec manipulation to diff machinery.

As part of the changes, setup_revisions() now can take pathspecs directly by setting argc = 0, argv = pathspec

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/add.c         |    3 +--
 builtin/diff.c        |    6 +++---
 builtin/fast-export.c |    2 +-
 diff-lib.c            |    6 +++---
 revision.c            |   20 +++++++++++---------
 revision.h            |    1 -
 wt-status.c           |    6 ++----
 7 files changed, 21 insertions(+), 23 deletions(-)
Show changes to 7 files +21 −23

builtin/add.c, builtin/diff.c, builtin/fast-export.c, diff-lib.c, revision.c, revision.h, wt-status.c

diff --git a/builtin/add.c b/builtin/add.c
index f7a17e4..904db6b 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -85,8 +85,7 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 	struct update_callback_data data;
 	struct rev_info rev;
 	init_revisions(&rev, prefix);
-	setup_revisions(0, NULL, &rev, NULL);
-	init_pathspec(&rev.prune_data, pathspec);
+	setup_revisions(0, pathspec, &rev, NULL);
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
 	data.flags = flags;
diff --git a/builtin/diff.c b/builtin/diff.c
index 4c9deb2..1c69da2 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -374,10 +374,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		}
 		die("unhandled object '%s' given.", name);
 	}
-	if (rev.prune_data.nr) {
+	if (rev.diffopt.pathspec.nr) {
 		if (!path)
-			path = rev.prune_data.items[0].match;
-		paths += rev.prune_data.nr;
+			path = rev.diffopt.pathspec.items[0].match;
+		paths += rev.diffopt.pathspec.nr;
 	}
 
 	/*
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index daf1945..b225427 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -651,7 +651,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (import_filename)
 		import_marks(import_filename);
 
-	if (import_filename && revs.prune_data.nr)
+	if (import_filename && revs.diffopt.pathspec.nr)
 		full_tree = 1;
 
 	get_tags_and_duplicates(&revs.pending, &extra_refs);
diff --git a/diff-lib.c b/diff-lib.c
index 1e22992..b3ac269 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -106,7 +106,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))
 			break;
 
-		if (!ce_path_match(ce, &revs->prune_data))
+		if (!ce_path_match(ce, &revs->diffopt.pathspec))
 			continue;
 
 		if (ce_stage(ce)) {
@@ -427,7 +427,7 @@ static int oneway_diff(struct cache_entry **src, struct unpack_trees_options *o)
 	if (tree == o->df_conflict_entry)
 		tree = NULL;
 
-	if (ce_path_match(idx ? idx : tree, &revs->prune_data))
+	if (ce_path_match(idx ? idx : tree, &revs->diffopt.pathspec))
 		do_oneway_diff(o, idx, tree);
 
 	return 0;
@@ -501,7 +501,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	active_nr = dst - active_cache;
 
 	init_revisions(&revs, NULL);
-	init_pathspec(&revs.prune_data, opt->pathspec.raw);
+	init_pathspec(&revs.diffopt.pathspec, opt->pathspec.raw);
 	tree = parse_tree_indirect(tree_sha1);
 	if (!tree)
 		die("bad tree object %s", sha1_to_hex(tree_sha1));
diff --git a/revision.c b/revision.c
index 86d2470..f9f66de 100644
--- a/revision.c
+++ b/revision.c
@@ -323,7 +323,7 @@ static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct
 		 * tagged commit by specifying both --simplify-by-decoration
 		 * and pathspec.
 		 */
-		if (!revs->prune_data.nr)
+		if (!revs->diffopt.pathspec.nr)
 			return REV_TREE_SAME;
 	}
 
@@ -969,7 +969,7 @@ static void prepare_show_merge(struct rev_info *revs)
 		struct cache_entry *ce = active_cache[i];
 		if (!ce_stage(ce))
 			continue;
-		if (ce_path_match(ce, &revs->prune_data)) {
+		if (ce_path_match(ce, &revs->diffopt.pathspec)) {
 			prune_num++;
 			prune = xrealloc(prune, sizeof(*prune) * prune_num);
 			prune[prune_num-2] = ce->name;
@@ -979,8 +979,8 @@ static void prepare_show_merge(struct rev_info *revs)
 		       ce_same_name(ce, active_cache[i+1]))
 			i++;
 	}
-	free_pathspec(&revs->prune_data);
-	init_pathspec(&revs->prune_data, prune);
+	free_pathspec(&revs->diffopt.pathspec);
+	init_pathspec(&revs->diffopt.pathspec, prune);
 	revs->limited = 1;
 }
 
@@ -1488,6 +1488,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (opt)
 		submodule = opt->submodule;
 
+	/* argv is a list of pathspec */
+	if (!argc && argv)
+		prune_data = argv;
+
 	/* First, search for "--" */
 	seen_dashdash = 0;
 	for (i = 1; i < argc; i++) {
@@ -1617,7 +1621,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	}
 
 	if (prune_data)
-		init_pathspec(&revs->prune_data, get_pathspec(revs->prefix, prune_data));
+		init_pathspec(&revs->diffopt.pathspec, get_pathspec(revs->prefix, prune_data));
 
 	if (revs->def == NULL)
 		revs->def = opt ? opt->def : NULL;
@@ -1648,19 +1652,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->topo_order)
 		revs->limited = 1;
 
-	if (revs->prune_data.nr) {
-		diff_tree_setup_paths(revs->prune_data.raw, &revs->pruning);
+	if (revs->diffopt.pathspec.nr) {
 		/* Can't prune commits with rename following: the paths change.. */
 		if (!DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))
 			revs->prune = 1;
-		if (!revs->full_diff)
-			diff_tree_setup_paths(revs->prune_data.raw, &revs->diffopt);
 	}
 	if (revs->combine_merges)
 		revs->ignore_merges = 0;
 	revs->diffopt.abbrev = revs->abbrev;
 	if (diff_setup_done(&revs->diffopt) < 0)
 		die("diff_setup_done failed");
+	diff_tree_setup_paths(revs->diffopt.pathspec.raw, &revs->pruning);
 
 	compile_grep_patterns(&revs->grep_filter);
 
diff --git a/revision.h b/revision.h
index 82509dd..066e407 100644
--- a/revision.h
+++ b/revision.h
@@ -34,7 +34,6 @@ struct rev_info {
 	/* Basic information */
 	const char *prefix;
 	const char *def;
-	struct pathspec prune_data;
 	unsigned int early_output;
 
 	/* Traversal flags */
diff --git a/wt-status.c b/wt-status.c
index a82b11d..7c4171e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -312,7 +312,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 	struct rev_info rev;
 
 	init_revisions(&rev, NULL);
-	setup_revisions(0, NULL, &rev, NULL);
+	setup_revisions(0, s->pathspec, &rev, NULL);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	DIFF_OPT_SET(&rev.diffopt, DIRTY_SUBMODULES);
 	if (!s->show_untracked_files)
@@ -323,7 +323,6 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
     }
 	rev.diffopt.format_callback = wt_status_collect_changed_cb;
 	rev.diffopt.format_callback_data = s;
-	init_pathspec(&rev.prune_data, s->pathspec);
 	run_diff_files(&rev, 0);
 }
 
@@ -335,7 +334,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	init_revisions(&rev, NULL);
 	memset(&opt, 0, sizeof(opt));
 	opt.def = s->is_initial ? EMPTY_TREE_SHA1_HEX : s->reference;
-	setup_revisions(0, NULL, &rev, &opt);
+	setup_revisions(0, s->pathspec, &rev, &opt);
 
 	if (s->ignore_submodule_arg) {
 		DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
@@ -348,7 +347,6 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 200;
 	rev.diffopt.break_opt = 0;
-	init_pathspec(&rev.prune_data, s->pathspec);
 	run_diff_index(&rev, 1);
 }
 

-- 
1.7.3.1.256.g2539c.dirty
Junio C Hamano· Mar 10, 2011, 06:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 3/5] revision.c: get rid of struct rev_info.prune_data

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 9 quoted lines
> There are three struct pathspec(s) in struct rev_info:
>
>  - prune_data
>  - pruning.pathspec
>  - diffopt.pathspec
>
> In some places, this pathspec is used, in other places another
> ... but i'm
> not sure why diffopt can't be used in place of pruning.

A command in the "log" family uses revision traversal and uses one pathspec (pruning.pathspec) to cull side branches and skip irrelevant commits, and another pathspec (diffopt.pathspec) to specify how the found commit is shown. Off the top of my head, currently the only case the pathspec of these diff options differ is when --full-diff is used, but they are independent concepts.

I would suspect this would matter even more once somebody wants to redo the --follow hack to actually follow different renamed-from path per merged branches, but I haven't thought through the issues.

Nguyen Thai Ngoc Duy· Mar 10, 2011, 06:52 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/5] revision.c: get rid of struct rev_info.prune_data

2011/3/10 Junio C Hamano <gitster@pobox.com>:
Show 10 quoted lines
>> In some places, this pathspec is used, in other places another
>> ... but i'm
>> not sure why diffopt can't be used in place of pruning.
>
> A command in the "log" family uses revision traversal and uses one
> pathspec (pruning.pathspec) to cull side branches and skip irrelevant
> commits, and another pathspec (diffopt.pathspec) to specify how the found
> commit is shown.  Off the top of my head, currently the only case the
> pathspec of these diff options differ is when --full-diff is used, but
> they are independent concepts.

OK then I did it wrong. I should have removed prune_data in favor of pruning.pathspec, leaving diffopt.pathspec intact.

-- 
Duy
Nguyễn Thái Ngọc Duy· Mar 10, 2011, 03:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 4/5] diff: refactor init/release API

This patch:
 - renames diff_setup() to init_diff()
 - gets rid of diff_tree_setup_paths()
 - renames diff_tree_release_paths() to release_diff()

The first one makes it probably just personal taste. I find "init" better name for where input can be garbage, as opposed to "setup", where the input is at least sane.

release_diff() is supposed to take care of more cleanup stuff as struct diff_options grows.

diff_tree_release_paths() is not just a simple wrapper around init_pathspec(). Remove it and manipulate diff_options->pathspec directly.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/technical/api-diff.txt |    2 +-
 builtin/blame.c                      |   22 +++++++---------------
 builtin/merge.c                      |    2 +-
 builtin/reset.c                      |    4 ++--
 diff-no-index.c                      |    6 +++---
 diff.c                               |    8 +++++++-
 diff.h                               |    5 ++---
 merge-recursive.c                    |    2 +-
 notes-merge.c                        |    8 ++++----
 patch-ids.c                          |    2 +-
 revision.c                           |    4 ++--
 tree-diff.c                          |   21 ++++-----------------
 12 files changed, 35 insertions(+), 51 deletions(-)
Show changes to 12 files +35 −51

Documentation/technical/api-diff.txt, builtin/blame.c, builtin/merge.c, builtin/reset.c, diff-no-index.c, diff.c, diff.h, merge-recursive.c, notes-merge.c, patch-ids.c, revision.c, tree-diff.c

diff --git a/Documentation/technical/api-diff.txt b/Documentation/technical/api-diff.txt
index 20b0241..b88ee9e 100644
--- a/Documentation/technical/api-diff.txt
+++ b/Documentation/technical/api-diff.txt
@@ -18,7 +18,7 @@ Calling sequence
 ----------------
 
 * Prepare `struct diff_options` to record the set of diff options, and
-  then call `diff_setup()` to initialize this structure.  This sets up
+  then call `init_diff()` to initialize this structure.  This sets up
   the vanilla default.
 
 * Fill in the options structure to specify desired output format, rename
diff --git a/builtin/blame.c b/builtin/blame.c
index aa30ec5..f58e6e4 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -383,14 +383,13 @@ static struct origin *find_origin(struct scoreboard *sb,
 	 * and origin first.  Most of the time they are the
 	 * same and diff-tree is fairly efficient about this.
 	 */
-	diff_setup(&diff_opts);
+	init_diff(&diff_opts);
 	DIFF_OPT_SET(&diff_opts, RECURSIVE);
 	diff_opts.detect_rename = 0;
 	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	paths[0] = origin->path;
 	paths[1] = NULL;
-
-	diff_tree_setup_paths(paths, &diff_opts);
+	init_pathspec(&diff_opts.pathspec, paths);
 	if (diff_setup_done(&diff_opts) < 0)
 		die("diff-setup");
 
@@ -441,7 +440,7 @@ static struct origin *find_origin(struct scoreboard *sb,
 		}
 	}
 	diff_flush(&diff_opts);
-	diff_tree_release_paths(&diff_opts);
+	release_diff(&diff_opts);
 	if (porigin) {
 		/*
 		 * Create a freestanding copy that is not part of
@@ -469,15 +468,12 @@ static struct origin *find_rename(struct scoreboard *sb,
 	struct origin *porigin = NULL;
 	struct diff_options diff_opts;
 	int i;
-	const char *paths[2];
 
-	diff_setup(&diff_opts);
+	init_diff(&diff_opts);
 	DIFF_OPT_SET(&diff_opts, RECURSIVE);
 	diff_opts.detect_rename = DIFF_DETECT_RENAME;
 	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	diff_opts.single_follow = origin->path;
-	paths[0] = NULL;
-	diff_tree_setup_paths(paths, &diff_opts);
 	if (diff_setup_done(&diff_opts) < 0)
 		die("diff-setup");
 
@@ -500,7 +496,7 @@ static struct origin *find_rename(struct scoreboard *sb,
 		}
 	}
 	diff_flush(&diff_opts);
-	diff_tree_release_paths(&diff_opts);
+	release_diff(&diff_opts);
 	return porigin;
 }
 
@@ -1048,7 +1044,6 @@ static int find_copy_in_parent(struct scoreboard *sb,
 			       int opt)
 {
 	struct diff_options diff_opts;
-	const char *paths[1];
 	int i, j;
 	int retval;
 	struct blame_list *blame_list;
@@ -1058,12 +1053,9 @@ static int find_copy_in_parent(struct scoreboard *sb,
 	if (!blame_list)
 		return 1; /* nothing remains for this target */
 
-	diff_setup(&diff_opts);
+	init_diff(&diff_opts);
 	DIFF_OPT_SET(&diff_opts, RECURSIVE);
 	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
-
-	paths[0] = NULL;
-	diff_tree_setup_paths(paths, &diff_opts);
 	if (diff_setup_done(&diff_opts) < 0)
 		die("diff-setup");
 
@@ -1147,7 +1139,7 @@ static int find_copy_in_parent(struct scoreboard *sb,
 	}
 	reset_scanned_flag(sb);
 	diff_flush(&diff_opts);
-	diff_tree_release_paths(&diff_opts);
+	release_diff(&diff_opts);
 	return retval;
 }
 
diff --git a/builtin/merge.c b/builtin/merge.c
index a89ddbb..c74ed1f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -379,7 +379,7 @@ static void finish(const unsigned char *new_head, const char *msg)
 	}
 	if (new_head && show_diffstat) {
 		struct diff_options opts;
-		diff_setup(&opts);
+		init_diff(&opts);
 		opts.output_format |=
 			DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
 		opts.detect_rename = DIFF_DETECT_RENAME;
diff --git a/builtin/reset.c b/builtin/reset.c
index 5de2bce..36b0605 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -195,10 +195,10 @@ static int read_from_tree(const char *prefix, const char **argv,
 	struct diff_options opt;
 
 	memset(&opt, 0, sizeof(opt));
-	diff_tree_setup_paths(get_pathspec(prefix, (const char **)argv), &opt);
 	opt.output_format = DIFF_FORMAT_CALLBACK;
 	opt.format_callback = update_index_from_diff;
 	opt.format_callback_data = &index_was_discarded;
+	init_pathspec(&opt.pathspec, get_pathspec(prefix, (const char **)argv));
 
 	index_fd = hold_locked_index(lock, 1);
 	index_was_discarded = 0;
@@ -207,7 +207,7 @@ static int read_from_tree(const char *prefix, const char **argv,
 		return 1;
 	diffcore_std(&opt);
 	diff_flush(&opt);
-	diff_tree_release_paths(&opt);
+	release_diff(&opt);
 
 	if (!index_was_discarded)
 		/* The index is still clobbered from do_diff_cache() */
diff --git a/diff-no-index.c b/diff-no-index.c
index 3a36144..a60eda7 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -203,7 +203,7 @@ void diff_no_index(struct rev_info *revs,
 		usagef("git diff %s <path> <path>",
 		       no_index ? "--no-index" : "[--no-index]");
 
-	diff_setup(&revs->diffopt);
+	init_diff(&revs->diffopt);
 	for (i = 1; i < argc - 2; ) {
 		int j;
 		if (!strcmp(argv[i], "--no-index"))
@@ -245,10 +245,10 @@ void diff_no_index(struct rev_info *revs,
 			     : p);
 			paths[i] = p;
 		}
-		diff_tree_setup_paths(paths, &revs->diffopt);
+		init_pathspec(&revs->diffopt.pathspec, paths);
 	}
 	else
-		diff_tree_setup_paths(argv + argc - 2, &revs->diffopt);
+		init_pathspec(&revs->diffopt.pathspec, argv + argc - 2);
 	revs->diffopt.skip_stat_unmatch = 1;
 	if (!revs->diffopt.output_format)
 		revs->diffopt.output_format = DIFF_FORMAT_PATCH;
diff --git a/diff.c b/diff.c
index 6640857..6f206a9 100644
--- a/diff.c
+++ b/diff.c
@@ -2847,7 +2847,7 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 	builtin_checkdiff(name, other, attr_path, p->one, p->two, o);
 }
 
-void diff_setup(struct diff_options *options)
+void init_diff(struct diff_options *options)
 {
 	memcpy(options, &default_diff_options, sizeof(*options));
 
@@ -2976,6 +2976,12 @@ int diff_setup_done(struct diff_options *options)
 	return 0;
 }
 
+
+void release_diff(struct diff_options *o)
+{
+	free_pathspec(&o->pathspec);
+}
+
 static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
 {
 	char c, *eq;
diff --git a/diff.h b/diff.h
index 310bd6b..12a9907 100644
--- a/diff.h
+++ b/diff.h
@@ -160,8 +160,6 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix);
 
 extern const char mime_boundary_leader[];
 
-extern void diff_tree_setup_paths(const char **paths, struct diff_options *);
-extern void diff_tree_release_paths(struct diff_options *);
 extern int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
 		     const char *base, struct diff_options *opt);
 extern int diff_tree_sha1(const unsigned char *old, const unsigned char *new,
@@ -226,9 +224,10 @@ extern int parse_long_opt(const char *opt, const char **argv,
 extern int git_diff_basic_config(const char *var, const char *value, void *cb);
 extern int git_diff_ui_config(const char *var, const char *value, void *cb);
 extern int diff_use_color_default;
-extern void diff_setup(struct diff_options *);
+extern void init_diff(struct diff_options *);
 extern int diff_opt_parse(struct diff_options *, const char **, int);
 extern int diff_setup_done(struct diff_options *);
+extern void release_diff(struct diff_options *);
 
 #define DIFF_DETECT_RENAME	1
 #define DIFF_DETECT_COPY	2
diff --git a/merge-recursive.c b/merge-recursive.c
index 16c2dbe..bbf48c9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -429,7 +429,7 @@ static struct string_list *get_renames(struct merge_options *o,
 	struct diff_options opts;
 
 	renames = xcalloc(1, sizeof(struct string_list));
-	diff_setup(&opts);
+	init_diff(&opts);
 	DIFF_OPT_SET(&opts, RECURSIVE);
 	opts.detect_rename = DIFF_DETECT_RENAME;
 	opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
diff --git a/notes-merge.c b/notes-merge.c
index 1467ad3..acf98e5 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -131,7 +131,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,
 	trace_printf("\tdiff_tree_remote(base = %.7s, remote = %.7s)\n",
 	       sha1_to_hex(base), sha1_to_hex(remote));
 
-	diff_setup(&opt);
+	init_diff(&opt);
 	DIFF_OPT_SET(&opt, RECURSIVE);
 	opt.output_format = DIFF_FORMAT_NO_OUTPUT;
 	if (diff_setup_done(&opt) < 0)
@@ -178,7 +178,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,
 		       sha1_to_hex(mp->remote));
 	}
 	diff_flush(&opt);
-	diff_tree_release_paths(&opt);
+	release_diff(&opt);
 
 	*num_changes = len;
 	return changes;
@@ -195,7 +195,7 @@ static void diff_tree_local(struct notes_merge_options *o,
 	trace_printf("\tdiff_tree_local(len = %i, base = %.7s, local = %.7s)\n",
 	       len, sha1_to_hex(base), sha1_to_hex(local));
 
-	diff_setup(&opt);
+	init_diff(&opt);
 	DIFF_OPT_SET(&opt, RECURSIVE);
 	opt.output_format = DIFF_FORMAT_NO_OUTPUT;
 	if (diff_setup_done(&opt) < 0)
@@ -265,7 +265,7 @@ static void diff_tree_local(struct notes_merge_options *o,
 		       sha1_to_hex(mp->local));
 	}
 	diff_flush(&opt);
-	diff_tree_release_paths(&opt);
+	release_diff(&opt);
 }
 
 static void check_notes_merge_worktree(struct notes_merge_options *o)
diff --git a/patch-ids.c b/patch-ids.c
index 5717257..e8ff156 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -37,7 +37,7 @@ struct patch_id_bucket {
 int init_patch_ids(struct patch_ids *ids)
 {
 	memset(ids, 0, sizeof(*ids));
-	diff_setup(&ids->diffopts);
+	init_diff(&ids->diffopts);
 	DIFF_OPT_SET(&ids->diffopts, RECURSIVE);
 	if (diff_setup_done(&ids->diffopts) < 0)
 		return error("diff_setup_done failed");
diff --git a/revision.c b/revision.c
index f9f66de..014b723 100644
--- a/revision.c
+++ b/revision.c
@@ -925,7 +925,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->grep_filter.header_tail = &(revs->grep_filter.header_list);
 	revs->grep_filter.regflags = REG_NEWLINE;
 
-	diff_setup(&revs->diffopt);
+	init_diff(&revs->diffopt);
 	if (prefix && !revs->diffopt.prefix) {
 		revs->diffopt.prefix = prefix;
 		revs->diffopt.prefix_length = strlen(prefix);
@@ -1662,7 +1662,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	revs->diffopt.abbrev = revs->abbrev;
 	if (diff_setup_done(&revs->diffopt) < 0)
 		die("diff_setup_done failed");
-	diff_tree_setup_paths(revs->diffopt.pathspec.raw, &revs->pruning);
+	init_pathspec(&revs->pruning.pathspec, revs->diffopt.pathspec.raw);
 
 	compile_grep_patterns(&revs->grep_filter);
 
diff --git a/tree-diff.c b/tree-diff.c
index 3954281..7df9064 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -209,26 +209,23 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 	struct diff_options diff_opts;
 	struct diff_queue_struct *q = &diff_queued_diff;
 	struct diff_filepair *choice;
-	const char *paths[1];
 	int i;
 
 	/* Remove the file creation entry from the diff queue, and remember it */
 	choice = q->queue[0];
 	q->nr = 0;
 
-	diff_setup(&diff_opts);
+	init_diff(&diff_opts);
 	DIFF_OPT_SET(&diff_opts, RECURSIVE);
 	DIFF_OPT_SET(&diff_opts, FIND_COPIES_HARDER);
 	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	diff_opts.single_follow = opt->pathspec.raw[0];
 	diff_opts.break_opt = opt->break_opt;
-	paths[0] = NULL;
-	diff_tree_setup_paths(paths, &diff_opts);
 	if (diff_setup_done(&diff_opts) < 0)
 		die("unable to set up diff options to follow renames");
 	diff_tree(t1, t2, base, &diff_opts);
 	diffcore_std(&diff_opts);
-	diff_tree_release_paths(&diff_opts);
+	release_diff(&diff_opts);
 
 	/* Go through the new set of filepairing, and see if we find a more interesting one */
 	opt->found_follow = 0;
@@ -247,9 +244,9 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 			choice = p;
 
 			/* Update the path we use from now on.. */
-			diff_tree_release_paths(opt);
+			release_diff(opt);
 			opt->pathspec.raw[0] = xstrdup(p->one->path);
-			diff_tree_setup_paths(opt->pathspec.raw, opt);
+			init_pathspec(&opt->pathspec, opt->pathspec.raw);
 
 			/*
 			 * The caller expects us to return a set of vanilla
@@ -323,13 +320,3 @@ int diff_root_tree_sha1(const unsigned char *new, const char *base, struct diff_
 	free(tree);
 	return retval;
 }
-
-void diff_tree_release_paths(struct diff_options *opt)
-{
-	free_pathspec(&opt->pathspec);
-}
-
-void diff_tree_setup_paths(const char **p, struct diff_options *opt)
-{
-	init_pathspec(&opt->pathspec, p);
-}

-- 
1.7.3.1.256.g2539c.dirty
Nguyễn Thái Ngọc Duy· Mar 10, 2011, 03:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 5/5] diff: support --exclude

diff_setup_done() will update diff_options.pathspec, so if you give
--exclude '*.h' -- src/
then the final pathspec looks like
 - src/
 - *.h (negated)

The code is protected against multiple call because setup_revisions() may call it once, then the outer code calls it again. I'm not sure if doing it this way is correct though.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c |   41 ++++++++++++++++++++++++++++++++++++++++-
 diff.h |    1 +
 2 files changed, 41 insertions(+), 1 deletions(-)
Show changes to 2 files +41 −1

diff.c, diff.h

diff --git a/diff.c b/diff.c
index 6f206a9..14f1cfc 100644
--- a/diff.c
+++ b/diff.c
@@ -2973,6 +2973,34 @@ int diff_setup_done(struct diff_options *options)
 		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	}
 
+	/*
+	 * Adjust options->pathspec. All --exclude will be appended to
+	 * options->pathspec with to_exclude set.
+	 */
+	if (options->paths &&
+	    options->pathspec.raw != options->paths) { /* avoid being called twice */
+		int i, nr, new_nr, exc;
+
+		nr = options->pathspec.nr ? options->pathspec.nr : 1;
+		for (exc = 0; options->paths[exc]; exc++)
+			;			  /* nothing */
+		new_nr = nr + exc;
+
+		options->paths = xrealloc(options->paths, sizeof(char*) * (new_nr + 1));
+		memmove(options->paths + nr, options->paths, sizeof(char*) * (exc + 1));
+		if (options->pathspec.nr)
+			memcpy(options->paths, options->pathspec.raw, sizeof(char*) * nr);
+		else
+			options->paths[0] = "";
+
+		free_pathspec(&options->pathspec);
+		init_pathspec(&options->pathspec, options->paths);
+		for (i = nr; i < new_nr; i++) {
+			struct pathspec_item *item = options->pathspec.items + i;
+			item->to_exclude = 1;
+		}
+	}
+
 	return 0;
 }
 
@@ -2980,6 +3008,7 @@ int diff_setup_done(struct diff_options *options)
 void release_diff(struct diff_options *o)
 {
 	free_pathspec(&o->pathspec);
+	free(o->paths);
 }
 
 static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
@@ -3347,7 +3376,17 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 			die_errno("Could not open '%s'", optarg);
 		options->close_file = 1;
 		return argcount;
-	} else
+	}
+	else if ((argcount = parse_long_opt("exclude", av, &optarg))) {
+		int i = 1;
+		while (options->paths && options->paths[i])
+			i++;
+		options->paths = xrealloc(options->paths, sizeof(char*) * (i + 1));
+		options->paths[i - 1] = optarg;
+		options->paths[i] = 0;
+		return argcount;
+	}
+	else
 		return 0;
 	return 1;
 }
diff --git a/diff.h b/diff.h
index 12a9907..b60057c 100644
--- a/diff.h
+++ b/diff.h
@@ -134,6 +134,7 @@ struct diff_options {
 	int close_file;
 
 	struct pathspec pathspec;
+	const char **paths;
 	change_fn_t change;
 	add_remove_fn_t add_remove;
 	diff_format_fn_t format_callback;

-- 
1.7.3.1.256.g2539c.dirty
Junio C Hamano· Mar 10, 2011, 06:11 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> On the other hand, if we go with ':' as a mark of special pathspecs, then
>
>  - ":/" mark pathspecs relative to worktree root
>  - ":!" may mark negative pathspecs

I am still moderately negative on this "negative pathspec" stuff, as it will complicate the semantics (just one example: would a path that is covered by both positive and negative pathspecs included or excluded? would the last one win? would the more specific one win?) and makes the design harder to explain to the users. Depending on the semantics chosen, it may also make the implementation less efficient and more complex.

As the choice of the syntax goes, in the recent "grep --full-tree" discussion, I thought people are more or less happy with the colon prefixed "magic pathspec" syntax, and something along the lines of the above two seems to be a good design.

The two most important things to consider are to make sure that people with funny pathnames can work it around by quoting, and the prefixing scheme is extensible so that other types of magic can later be introduced with the same kind of escape hatch for people with funny pathnames that begin with or contain new magic characters used to trigger the new magic.

I said "something along the lines" above because ":/ for root, :! for negative" does not yet specify how the scheme would satisfy the above two consideration very well.

Nguyen Thai Ngoc Duy· Mar 10, 2011, 07:03 UTC · re: Junio C Hamano · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

2011/3/10 Junio C Hamano <gitster@pobox.com>:
Show 13 quoted lines
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> On the other hand, if we go with ':' as a mark of special pathspecs, then
>>
>>  - ":/" mark pathspecs relative to worktree root
>>  - ":!" may mark negative pathspecs
>
> I am still moderately negative on this "negative pathspec" stuff, as it
> will complicate the semantics (just one example: would a path that is
> covered by both positive and negative pathspecs included or excluded?
> would the last one win? would the more specific one win?) and makes the
> design harder to explain to the users. Depending on the semantics chosen,
> it may also make the implementation less efficient and more complex.

The semantics resembles .gitignore, or at least that's my intention. But yes, this kind of notion is more complex than simple '--exclude' option, which would suit 90% cases, I guess.

Show 9 quoted lines
> The two most important things to consider are to make sure that people
> with funny pathnames can work it around by quoting, and the prefixing
> scheme is extensible so that other types of magic can later be introduced
> with the same kind of escape hatch for people with funny pathnames that
> begin with or contain new magic characters used to trigger the new magic.
>
> I said "something along the lines" above because ":/ for root, :! for
> negative" does not yet specify how the scheme would satisfy the above
> two consideration very well.

The quoting part may go through another painful migration plan if you don't want to surprise users.

I have already suggested to reserve the next character after ':' (or any char chosen as the magic one) as control character, for this extensibility. Any unrecognized control character will be rejected. Even if we limit ourselves to punctuation letters only, there's still plenty room for future after '/' and '!' are taken.

-- 
Duy
Junio C Hamano· Mar 10, 2011, 08:41 UTC · re: Junio C Hamano · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

Junio C Hamano <gitster@pobox.com> writes:
Show 22 quoted lines
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> On the other hand, if we go with ':' as a mark of special pathspecs, then
>>
>>  - ":/" mark pathspecs relative to worktree root
>>  - ":!" may mark negative pathspecs
> ...
>
> As the choice of the syntax goes, in the recent "grep --full-tree"
> discussion, I thought people are more or less happy with the colon
> prefixed "magic pathspec" syntax, and something along the lines of the
> above two seems to be a good design.
>
> The two most important things to consider are to make sure that people
> with funny pathnames can work it around by quoting, and the prefixing
> scheme is extensible so that other types of magic can later be introduced
> with the same kind of escape hatch for people with funny pathnames that
> begin with or contain new magic characters used to trigger the new magic.
>
> I said "something along the lines" above because ":/ for root, :! for
> negative" does not yet specify how the scheme would satisfy the above
> two consideration very well.
Let's step back a bit.

We chose to use ":/<regexp>" as one new form of extended SHA-1 expression to name an object for two reasons: (1) no normal <ref> can have a colon in it, because of check-ref-format restriction; (2) ":" is an unlikely letter to appear at the beginning of a pathname, and people with such a path can work around by saying "./:frotz" or "\:xyzzy".

There is a disambiguation logic to check a list of arguments that lacks an explicit "--" separator to make sure that each element early on the list can only be interpreted as an object name but not as a pathname that exists on the filesystem, and all the remaining elements are pathnames that exist on the filesystem.

If we introduce an extended syntax for pathspec and make the prefix magic character ":", and if we choose to use ":/" as one kind of magic, I was a bit worried that this may affect the disambiguation. The users must use an explicit "--" when feeding a pathspec with the magic so that the parser knows which kind of magic (either object name magic or pathspec magic) they are talking about.

I however realized that it is not an issue at all, because the users already need to disambiguate with "--" when using wildcards in their pathspecs (e.g. "git log 'Makefil*'" will give you an ambiguity error). Admittedly, wildcard pathspecs are lessor kind of magic, but they are magic nevertheless.

So my tentative conclusion is that there is no problem using the same ":" as the magic introducer for pathspecs, just like we do for object names.

	Side note: please point out flaws in the above train of thought
        that would make the end result non-workable.

Although I like the general approach Michael took with his "alternative setup.c that understands tree-wide pathspec" patch (Cf. $gmane/168207), I agree ":/" would be better than saying "':' is root, and we don't allow any other magic under ':'" for the sake of extensibility.

I am not very happy with ":!" as negative from syntactical point of view, because traditionally "!" is a tad cumbersome to quote in shells with history support (e.g. "csh"), but unfortunately I don't think of any good alternative that is easy to type. Pathspecs are globs by nature, and "!" is a good choice of nagation indicator for that reason (think "[!aeiou]" not "[^aeiou]" the latter of which is a regexp and not glob), though.

Nguyen Thai Ngoc Duy· Mar 10, 2011, 10:05 UTC · re: Junio C Hamano · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

2011/3/10 Junio C Hamano <gitster@pobox.com>:
Show 20 quoted lines
> Let's step back a bit.
>
> We chose to use ":/<regexp>" as one new form of extended SHA-1 expression
> to name an object for two reasons: (1) no normal <ref> can have a colon in
> it, because of check-ref-format restriction; (2) ":" is an unlikely letter
> to appear at the beginning of a pathname, and people with such a path can
> work around by saying "./:frotz" or "\:xyzzy".
>
> There is a disambiguation logic to check a list of arguments that lacks an
> explicit "--" separator to make sure that each element early on the list
> can only be interpreted as an object name but not as a pathname that
> exists on the filesystem, and all the remaining elements are pathnames
> that exist on the filesystem.
>
> If we introduce an extended syntax for pathspec and make the prefix magic
> character ":", and if we choose to use ":/" as one kind of magic, I was a
> bit worried that this may affect the disambiguation.  The users must use
> an explicit "--" when feeding a pathspec with the magic so that the parser
> knows which kind of magic (either object name magic or pathspec magic)
> they are talking about.

Or.. we can consider this ':' a special form of wildcard and interpret the same way:

 - first try exact match. If it matches, the leading ':' is
interpreted literally as part of file name.
 - magic.
Show 5 quoted lines
> I however realized that it is not an issue at all, because the users
> already need to disambiguate with "--" when using wildcards in their
> pathspecs (e.g. "git log 'Makefil*'" will give you an ambiguity error).
> Admittedly, wildcard pathspecs are lessor kind of magic, but they are
> magic nevertheless.
Yes, ':' is on the same side with wildcard pathspecs.
Show 5 quoted lines
> So my tentative conclusion is that there is no problem using the same ":"
> as the magic introducer for pathspecs, just like we do for object names.
>
>        Side note: please point out flaws in the above train of thought
>        that would make the end result non-workable.
Huh? I failed.
Show 6 quoted lines
> I am not very happy with ":!" as negative from syntactical point of view,
> because traditionally "!" is a tad cumbersome to quote in shells with
> history support (e.g. "csh"), but unfortunately I don't think of any good
> alternative that is easy to type.  Pathspecs are globs by nature, and "!"
> is a good choice of nagation indicator for that reason (think "[!aeiou]"
> not "[^aeiou]" the latter of which is a regexp and not glob), though.

We don't need to stick to ':!'. I just needed something to start coding. Other candidates can be '^' or '-' or '~'. '^' looks best in my opinion.

-- 
Duy
Nguyen Thai Ngoc Duy· Mar 21, 2011, 04:02 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

On Thu, Mar 10, 2011 at 5:05 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
Show 28 quoted lines
> 2011/3/10 Junio C Hamano <gitster@pobox.com>:
>> Let's step back a bit.
>>
>> We chose to use ":/<regexp>" as one new form of extended SHA-1 expression
>> to name an object for two reasons: (1) no normal <ref> can have a colon in
>> it, because of check-ref-format restriction; (2) ":" is an unlikely letter
>> to appear at the beginning of a pathname, and people with such a path can
>> work around by saying "./:frotz" or "\:xyzzy".
>>
>> There is a disambiguation logic to check a list of arguments that lacks an
>> explicit "--" separator to make sure that each element early on the list
>> can only be interpreted as an object name but not as a pathname that
>> exists on the filesystem, and all the remaining elements are pathnames
>> that exist on the filesystem.
>>
>> If we introduce an extended syntax for pathspec and make the prefix magic
>> character ":", and if we choose to use ":/" as one kind of magic, I was a
>> bit worried that this may affect the disambiguation.  The users must use
>> an explicit "--" when feeding a pathspec with the magic so that the parser
>> knows which kind of magic (either object name magic or pathspec magic)
>> they are talking about.
>
> Or.. we can consider this ':' a special form of wildcard and interpret
> the same way:
>
>  - first try exact match. If it matches, the leading ':' is
> interpreted literally as part of file name.
>  - magic.

One thing that makes it different from Michael's approach is, :/foo will match ':/foo' literally in every directories and foo at top-tree. I feel mildly uncomfortable with it, but it makes it consistent with other wildcards. If no one objects, I'll try to make a patch with this approach.

-- 
Duy
Michael J Gruber· Mar 21, 2011, 09:45 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

Nguyen Thai Ngoc Duy venit, vidit, dixit 21.03.2011 05:02:
Show 35 quoted lines
> On Thu, Mar 10, 2011 at 5:05 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>> 2011/3/10 Junio C Hamano <gitster@pobox.com>:
>>> Let's step back a bit.
>>>
>>> We chose to use ":/<regexp>" as one new form of extended SHA-1 expression
>>> to name an object for two reasons: (1) no normal <ref> can have a colon in
>>> it, because of check-ref-format restriction; (2) ":" is an unlikely letter
>>> to appear at the beginning of a pathname, and people with such a path can
>>> work around by saying "./:frotz" or "\:xyzzy".
>>>
>>> There is a disambiguation logic to check a list of arguments that lacks an
>>> explicit "--" separator to make sure that each element early on the list
>>> can only be interpreted as an object name but not as a pathname that
>>> exists on the filesystem, and all the remaining elements are pathnames
>>> that exist on the filesystem.
>>>
>>> If we introduce an extended syntax for pathspec and make the prefix magic
>>> character ":", and if we choose to use ":/" as one kind of magic, I was a
>>> bit worried that this may affect the disambiguation.  The users must use
>>> an explicit "--" when feeding a pathspec with the magic so that the parser
>>> knows which kind of magic (either object name magic or pathspec magic)
>>> they are talking about.
>>
>> Or.. we can consider this ':' a special form of wildcard and interpret
>> the same way:
>>
>>  - first try exact match. If it matches, the leading ':' is
>> interpreted literally as part of file name.
>>  - magic.
> 
> One thing that makes it different from Michael's approach is, :/foo
> will match ':/foo' literally in every directories and foo at top-tree.
> I feel mildly uncomfortable with it, but it makes it consistent with
> other wildcards. If no one objects, I'll try to make a patch with this
> approach.

Maybe I misunderstand which topic you are referring to. I have a patch for ":" (to denote repo-root in pathspecs), there's no need to make another one for ":/". (I would really prefer to do it myself since I brought it up.) I was just waiting for agreement to settle in about which prefix to use. Also, I have checked with J&J of mingw/msysgit fame meanwhile to make sure that the notation does not create problems on the windows side of git.

Michael
Nguyen Thai Ngoc Duy· Mar 21, 2011, 10:02 UTC · re: Michael J Gruber · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

On Mon, Mar 21, 2011 at 4:45 PM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 10 quoted lines
>> One thing that makes it different from Michael's approach is, :/foo
>> will match ':/foo' literally in every directories and foo at top-tree.
>> I feel mildly uncomfortable with it, but it makes it consistent with
>> other wildcards. If no one objects, I'll try to make a patch with this
>> approach.
>
> Maybe I misunderstand which topic you are referring to. I have a patch
> for ":" (to denote repo-root in pathspecs), there's no need to make
> another one for ":/". (I would really prefer to do it myself since I
> brought it up.)

If you make it a wildcard as I mentioned above, the code would be different. Your patch will match only files at top, while wildcards can also match any files literally. You can do it, I don't mind at all.

> I was just waiting for agreement to settle in about
> which prefix to use. Also, I have checked with J&J of mingw/msysgit fame
> meanwhile to make sure that the notation does not create problems on the
> windows side of git.

The nice (or not) thing about wildcard behavior is that because it would match both cases, we would not need to worry too much. People who don't know the magic can still match files (although more, not less).

-- 
Duy
Junio C Hamano· Mar 22, 2011, 23:59 UTC · re: Michael J Gruber · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 11 quoted lines
>> One thing that makes it different from Michael's approach is, :/foo
>> will match ':/foo' literally in every directories and foo at top-tree.
>> I feel mildly uncomfortable with it, but it makes it consistent with
>> other wildcards. If no one objects, I'll try to make a patch with this
>> approach.
>
> Maybe I misunderstand which topic you are referring to. I have a patch
> for ":" (to denote repo-root in pathspecs), there's no need to make
> another one for ":/". (I would really prefer to do it myself since I
> brought it up.) I was just waiting for agreement to settle in about
> which prefix to use.

I am not sure if there is any disagreement in the desired semantics between the two.

Let's clarify by dumping my understanding of what we aim to achieve.
 - The user gives ':/<frotz>' and '<xyzzy>' from the command line, in a
   subdirectory.
 - Internally we chdir() up to the top of the working tree, and the prefix
   is set to the path to the subdirectory (with trailing slash);
   get_pathspec(prefix, pathspec) is called, where pathspec is often a
   later part of argv[] we got from the command line.
 - Currently, get_pathspec() gives either:
    - the same as incoming pathspec, if <prefix> is NULL (this happens at
      the root of the working tree);
    - { <prefix>, NULL }, if the incoming pathspec array is empty; or
    - an array of strings taken from pathspec, each prefixed with <prefix>.
   The list of the resulting strings is the pathspec, relative to the root
   of the working tree, and is used to determine if a path matches the
   given pathspec.  The match traditionally had two semantics (diff-tree
   family and everybody else, only the latter group knew about globs), but
   an earlier work by Nguyen has taught globs to everybody (at least it
   aimed to; we might have leftover cases that we haven't uncovered).

Both the ":/<path>" proposal (or your ":<path>" proposal) changes only a very small part of the above, namely, "each prefixed with '<prefix>'" is changed to if the element in original pathspec has the magic colon prefix, the magic is stripped away, and the remainder becomes the element in the resulting pathspec array without additional <prefix> in front.

If Nguyen's proposal is to also match ":/<path>" (or ":<path>") literally, that part should be scrapped. If somebody wants to match such an unusual path component, it can always be expressed by quoting it as a glob, i.e. "[:]/<path>" (or "[:]<path").

I am slightly in favor of ":<path>" syntax (than ":/<path>"), but I do not care too deeply. Either has the same problem that it will be confusing with existing and well-established syntax (the former would conflict with "name of the blob in the index", the latter with "name of the commit that match the regexp).

Nguyen Thai Ngoc Duy· Mar 23, 2011, 12:10 UTC · re: Junio C Hamano · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

On Wed, Mar 23, 2011 at 6:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> Let's clarify by dumping my understanding of what we aim to achieve.
>
> ...
>
> Both the ":/<path>" proposal (or your ":<path>" proposal) changes only a
> very small part of the above, namely, "each prefixed with '<prefix>'" is
> changed to if the element in original pathspec has the magic colon prefix,
> the magic is stripped away, and the remainder becomes the element in the
> resulting pathspec array without additional <prefix> in front.
Correct.
> If Nguyen's proposal is to also match ":/<path>" (or ":<path>") literally,
> that part should be scrapped.  If somebody wants to match such an unusual
> path component, it can always be expressed by quoting it as a glob,
> i.e. "[:]/<path>" (or "[:]<path").
OK.
Show 5 quoted lines
> I am slightly in favor of ":<path>" syntax (than ":/<path>"), but I do not
> care too deeply.  Either has the same problem that it will be confusing
> with existing and well-established syntax (the former would conflict with
> "name of the blob in the index", the latter with "name of the commit that
> match the regexp).

How about ":<path>" for root pathspecs, but reserve ':[^0-9A-Za-z]*' for future use?

(on top of Michael's patch)
-- 8< --
Show changes to setup.c +6 −2
diff --git a/setup.c b/setup.c
index ef55e5d..1ebe1d2 100644
--- a/setup.c
+++ b/setup.c
@@ -146,8 +146,13 @@ const char **get_pathspec(const char *prefix,
const char **pathspec)
 	while (*src) {
 		const char *p;

-		if ((*src)[0] == ':')
+		if ((*src)[0] == ':') {
+			const char *reserved = "~`!@#$%^&*()-_=+[{]}\\|;:'\",<.>/?";
+			if (strchr(reserved, (*src)[1]))
+				die("':%c' syntax is not supported. "
+				    "Quote it to match literally.", (*src)[1]);
 			p = prefix_path(NULL, 0, (*src)+1);
+		}
 		else
 			p = prefix_path(prefix, prefixlen, *src);
 		*(dst++) = p;
-- 8< --

-- 
Duy
Nguyen Thai Ngoc Duy· Mar 23, 2011, 12:18 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

On Wed, Mar 23, 2011 at 7:10 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
Show 6 quoted lines
> How about ":<path>" for root pathspecs, but reserve ':[^0-9A-Za-z]*'
> for future use?
> ..
> +                       if (strchr(reserved, (*src)[1]))
> +                               die("':%c' syntax is not supported. "
> +                                   "Quote it to match literally.", (*src)[1]);

Hmm.. I need a letter to become quoter. Let's take '\' for that. I also leave '.' out of reserved range because it may be used frequently (hidden files).

Show changes to setup.c +10 −2
diff --git a/setup.c b/setup.c
index ef55e5d..b6294be 100644
--- a/setup.c
+++ b/setup.c
@@ -146,8 +146,16 @@ const char **get_pathspec(const char *prefix,
const char **pathspec)
 	while (*src) {
 		const char *p;

-		if ((*src)[0] == ':')
-			p = prefix_path(NULL, 0, (*src)+1);
+		if ((*src)[0] == ':') {
+			const char *reserved = "~`!@#$%^&*()-_=+[{]}|;:'\",<>/?";
+			const char *s = *src + 1;
+			if (*s == '\\')
+				s++;
+			else if (strchr(reserved, *s))
+				die("':%c' syntax is not supported. "
+				    "Quote it to match literally.", *s);
+			p = prefix_path(NULL, 0, s);
+		}
 		else
 			p = prefix_path(prefix, prefixlen, *src);
 		*(dst++) = p;

-- 
Duy
Michael J Gruber· Mar 23, 2011, 12:51 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

Nguyen Thai Ngoc Duy venit, vidit, dixit 23.03.2011 13:10:
Show 53 quoted lines
> On Wed, Mar 23, 2011 at 6:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Let's clarify by dumping my understanding of what we aim to achieve.
>>
>> ...
>>
>> Both the ":/<path>" proposal (or your ":<path>" proposal) changes only a
>> very small part of the above, namely, "each prefixed with '<prefix>'" is
>> changed to if the element in original pathspec has the magic colon prefix,
>> the magic is stripped away, and the remainder becomes the element in the
>> resulting pathspec array without additional <prefix> in front.
> 
> Correct.
> 
>> If Nguyen's proposal is to also match ":/<path>" (or ":<path>") literally,
>> that part should be scrapped.  If somebody wants to match such an unusual
>> path component, it can always be expressed by quoting it as a glob,
>> i.e. "[:]/<path>" (or "[:]<path").
> 
> OK.
> 
>> I am slightly in favor of ":<path>" syntax (than ":/<path>"), but I do not
>> care too deeply.  Either has the same problem that it will be confusing
>> with existing and well-established syntax (the former would conflict with
>> "name of the blob in the index", the latter with "name of the commit that
>> match the regexp).
> 
> How about ":<path>" for root pathspecs, but reserve ':[^0-9A-Za-z]*'
> for future use?
> 
> (on top of Michael's patch)
> 
> -- 8< --
> diff --git a/setup.c b/setup.c
> index ef55e5d..1ebe1d2 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -146,8 +146,13 @@ const char **get_pathspec(const char *prefix,
> const char **pathspec)
>  	while (*src) {
>  		const char *p;
> 
> -		if ((*src)[0] == ':')
> +		if ((*src)[0] == ':') {
> +			const char *reserved = "~`!@#$%^&*()-_=+[{]}\\|;:'\",<.>/?";
> +			if (strchr(reserved, (*src)[1]))
> +				die("':%c' syntax is not supported. "
> +				    "Quote it to match literally.", (*src)[1]);
>  			p = prefix_path(NULL, 0, (*src)+1);
> +		}
>  		else
>  			p = prefix_path(prefix, prefixlen, *src);
>  		*(dst++) = p;
> -- 8< --
Sounds good to me so far.

Note that there is also possible notational overlap with the WIP "attr: make attributes depend on file type" at

http://permalink.gmane.org/gmane.comp.version-control.git/168116
which suggested something like
:symlink:* diff=symlinkdiffdriverofyourchoice

as a notation for attributes which depend on the file type. Is that a problem, should I do something different there? (Also, I still need help with that...)

Michael
Nguyen Thai Ngoc Duy· Mar 23, 2011, 13:34 UTC · re: Michael J Gruber · lore

Re: [WIP PATCH 0/5] support --exclude for diff/log commands

On Wed, Mar 23, 2011 at 7:51 PM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 12 quoted lines
> Note that there is also possible notational overlap with the WIP "attr:
> make attributes depend on file type" at
>
> http://permalink.gmane.org/gmane.comp.version-control.git/168116
>
> which suggested something like
>
> :symlink:* diff=symlinkdiffdriverofyourchoice
>
> as a notation for attributes which depend on the file type. Is that a
> problem, should I do something different there? (Also, I still need help
> with that...)
I would do
:{symlink}* diff=symlinkdiffdriverofyourchoice

There's also blank quote problem in gitattr. Tricks like 'hello.world' would match path 'hello world' but I'd rather have something better. If ':' is the special char, I could teach gitattr to recognize <colon>+<quote> as a mark to do C quotation without regressions.

-- 
Duy

← back to recent threads