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

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

[Nguyễn Thái Ngọc Duy]

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

[PATCH 2/5] match_pathspec_depth: support negative pathspec

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 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.

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

[Nguyen Thai Ngoc Duy]

2011/3/10 Junio C Hamano <gitster@pobox.com>:
>> 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

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

[Nguyen Thai Ngoc Duy]

2011/3/10 Junio C Hamano <gitster@pobox.com>:
> 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.

> 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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

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

[Nguyen Thai Ngoc Duy]

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.

> 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.

> 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.

> 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

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

[Nguyen Thai Ngoc Duy]

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.
-- 
Duy

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

[Michael J Gruber]

Nguyen Thai Ngoc Duy venit, vidit, dixit 21.03.2011 05:02:
> 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

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

[Nguyen Thai Ngoc Duy]

On Mon, Mar 21, 2011 at 4:45 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
>> 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

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>> 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).

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

[Nguyen Thai Ngoc Duy]

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< --
-- 
Duy

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

[Nguyen Thai Ngoc Duy]

On Wed, Mar 23, 2011 at 7:10 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> 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).

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

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

[Michael J Gruber]

Nguyen Thai Ngoc Duy venit, vidit, dixit 23.03.2011 13:10:
> 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

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

[Nguyen Thai Ngoc Duy]

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