[PATCH 0/6] Negation magic pathspec

[PATCH 0/6] Negation magic pathspec

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

After the last round toying with .gitignore mechanism as a way to
exclude paths, I have finally got back to the negative pathspec.

I'm still struggling with read_directory() rewrite so that struct
pathspec can be used throughout git, but now realized we can at least
enable magic for certain commands and die() on those that don't.
This may help move magic pathspec patches forward.

The nice thing about this series is that negative pathspec patch is
small and simple, much less headache to review than the previous
version (and as a consequence, not as powerful).

So here it is to gather comments whether we should go this way. Very
much WIP, I have not even run "make test".

Nguyễn Thái Ngọc Duy (6):
  Recognize magic pathspec as filenames
  Replace has_wildcard with PATHSPEC_NOGLOB
  Convert prefix_pathspec() to produce struct pathspec_item
  Implement parse_pathspec()
  Convert simple init_pathspec() cases to parse_pathspec()
  Implement negative pathspec

 Documentation/glossary-content.txt |    8 ++--
 builtin/grep.c                     |    4 +-
 builtin/ls-files.c                 |    2 +-
 builtin/ls-tree.c                  |    6 +-
 builtin/reset.c                    |    2 +-
 cache.h                            |   29 +++++++++++-
 dir.c                              |   85 +++++++++++++++++++++++++++--------
 revision.c                         |    9 ++--
 setup.c                            |   56 +++++++++++-------------
 tree-walk.c                        |   44 ++++++++++++++++---
 10 files changed, 169 insertions(+), 76 deletions(-)

-- 
1.7.3.1.256.g2539c.dirty

[PATCH 1/6] Recognize magic pathspec as filenames

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 .. so that "git log :/" works, not so sure this is correct though

 setup.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/setup.c b/setup.c
index 27c1d47..08f605b 100644
--- a/setup.c
+++ b/setup.c
@@ -58,15 +58,8 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
 	unsigned char sha1[20];
 	unsigned mode;
 
-	/*
-	 * Saying "'(icase)foo' does not exist in the index" when the
-	 * user gave us ":(icase)foo" is just stupid.  A magic pathspec
-	 * begins with a colon and is followed by a non-alnum; do not
-	 * let get_sha1_with_mode_1(only_to_die=1) to even trigger.
-	 */
-	if (!(arg[0] == ':' && !isalnum(arg[1])))
-		/* try a detailed diagnostic ... */
-		get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
+	/* try a detailed diagnostic ... */
+	get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
 
 	/* ... or fall back the most general message. */
 	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
@@ -85,6 +78,11 @@ void verify_filename(const char *prefix, const char *arg)
 {
 	if (*arg == '-')
 		die("bad flag '%s' used after filename", arg);
+
+	/* If it's magic pathspec, just assume it's file name */
+	if (arg[0] == ':' && is_pathspec_magic(arg[1]))
+		return;
+
 	if (check_filename(prefix, arg))
 		return;
 	die_verify_filename(prefix, arg);
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH 1/6] Recognize magic pathspec as filenames

[Junio C Hamano]

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

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  .. so that "git log :/" works, not so sure this is correct though

At least the first half should be in the commit log message, and also it
should contrast it against "git log -- :/".

I doubt the approach taken by this particular patch (I do not know about
the rest of the series) is correct, as it breaks the symmetry between
verify_filename() and verify_non_filename().

When given a list of command line arguments, we do:

 (1) If there is "--", then no verification is needed. Everything before
     the double-dash that is not a "-flag" will be interpreted as revs
     (and get_sha1() will error out otherwise), and everything after the
     double-dash will be used as an pathspec element.

 (2) If there is no "--", then earlier ones must be all revs and cannot be
     pathnames in the working tree. The first argument that is not a rev
     and everything after that must be a pathname in the working tree, and
     must not be interpretable as revs.

That "must be a pathname in the working tree" is what verify_filename()
does. and you say that ":/foo" is OK to be a pathname in this patch.

But shouldn't the opposite "cannot be pathnames in the working tree" check
done by verify_non_filename() also be told the same logic? If ":/foo" is
OK to be a pathname, shouldn't check_filename() call in that function also
be avoided the same way?

And once that happens, I think you will be back to square one.

"git log :/foo" is ambiguous no matter how you slice it, if you are going
to look at only the first character in the string. It could be asking to
show only commits that touch the path in the top-level directory "foo" and
its subdirectories, or it could be asking to start traversal at a commit
with "foo" in the log message.

Longhand magic pathspecs e.g. ":(icase)foo" might have better chance, but
not by a wide margin. It can be a rev that names the blob object in the
index registered for the literal path "'(icase)foo", or it could be an
element in the pathspec to match [Ff][Oo][[Oo].

>  setup.c |   16 +++++++---------
>  1 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 27c1d47..08f605b 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -58,15 +58,8 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg)
>  	unsigned char sha1[20];
>  	unsigned mode;
>  
> -	/*
> -	 * Saying "'(icase)foo' does not exist in the index" when the
> -	 * user gave us ":(icase)foo" is just stupid.  A magic pathspec
> -	 * begins with a colon and is followed by a non-alnum; do not
> -	 * let get_sha1_with_mode_1(only_to_die=1) to even trigger.
> -	 */
> -	if (!(arg[0] == ':' && !isalnum(arg[1])))
> -		/* try a detailed diagnostic ... */
> -		get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
> +	/* try a detailed diagnostic ... */
> +	get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
>  
>  	/* ... or fall back the most general message. */
>  	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
> @@ -85,6 +78,11 @@ void verify_filename(const char *prefix, const char *arg)
>  {
>  	if (*arg == '-')
>  		die("bad flag '%s' used after filename", arg);
> +
> +	/* If it's magic pathspec, just assume it's file name */
> +	if (arg[0] == ':' && is_pathspec_magic(arg[1]))
> +		return;
> +
>  	if (check_filename(prefix, arg))
>  		return;
>  	die_verify_filename(prefix, arg);

Re: [PATCH 1/6] Recognize magic pathspec as filenames

[Nguyen Thai Ngoc Duy]

2011/10/13 Junio C Hamano <gitster@pobox.com>:
> "git log :/foo" is ambiguous no matter how you slice it, if you are going
> to look at only the first character in the string. It could be asking to
> show only commits that touch the path in the top-level directory "foo" and
> its subdirectories, or it could be asking to start traversal at a commit
> with "foo" in the log message.

Yeah, I forgot the ":/" ref syntax. But because it's ambiguous anyway,
should we disallow "git log :/foo"? Either "git log :/foo --" or "git
log -- :/foo" is OK.

> Longhand magic pathspecs e.g. ":(icase)foo" might have better chance, but
> not by a wide margin. It can be a rev that names the blob object in the
> index registered for the literal path "'(icase)foo", or it could be an
> element in the pathspec to match [Ff][Oo][[Oo].

It's unfortunate that ":" has so many meanings attached to it. I hope
that our ambiguation detection code can be smart, based on context, to
avoid unnecessary "--". For example, "git log :(icase)foo" can only
mean pathspec magic (I hope..)
-- 
Duy

Re: [PATCH 1/6] Recognize magic pathspec as filenames

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> It's unfortunate that ":" has so many meanings attached to it.

Yeah, I still recall that there were people who said ":/$path looks
similar to the way how the object at the $path in the index is named" as
if it were an advantage.  Maybe they were all misguided ;-).

[PATCH 2/6] Replace has_wildcard with PATHSPEC_NOGLOB

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

Note though "noglob" magic is not implemented yet, only its definition
for internal use.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/ls-files.c |    2 +-
 builtin/ls-tree.c  |    4 ++--
 cache.h            |   22 ++++++++++++++++++++--
 dir.c              |   11 +++++++----
 setup.c            |   17 -----------------
 tree-walk.c        |    7 +++----
 6 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e8a800d..e687157 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -326,7 +326,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 		matchbuf[0] = prefix;
 		matchbuf[1] = NULL;
 		init_pathspec(&pathspec, matchbuf);
-		pathspec.items[0].use_wildcard = 0;
+		pathspec.items[0].magic = PATHSPEC_NOGLOB;
 	} else
 		init_pathspec(&pathspec, NULL);
 	if (read_tree(tree, 1, &pathspec))
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 6b666e1..f0fa7dd 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -168,8 +168,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 
 	init_pathspec(&pathspec, get_pathspec(prefix, argv + 1));
 	for (i = 0; i < pathspec.nr; i++)
-		pathspec.items[i].use_wildcard = 0;
-	pathspec.has_wildcard = 0;
+		pathspec.items[i].magic = PATHSPEC_NOGLOB;
+	pathspec.magic |= PATHSPEC_NOGLOB;
 	tree = parse_tree_indirect(sha1);
 	if (!tree)
 		die("not a tree object");
diff --git a/cache.h b/cache.h
index 82e12c8..fe46f1e 100644
--- a/cache.h
+++ b/cache.h
@@ -521,16 +521,34 @@ extern int index_name_is_other(const struct index_state *, const char *, int);
 extern int ie_match_stat(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);
 extern int ie_modified(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);
 
+/*
+ * Magic pathspec
+ *
+ * NEEDSWORK: These need to be moved to dir.h or even to a new
+ * pathspec.h when we restructure get_pathspec() users to use the
+ * "struct pathspec" interface.
+ *
+ * Possible future magic semantics include stuff like:
+ *
+ *	{ PATHSPEC_NOGLOB, '!', "noglob" },
+ *	{ PATHSPEC_ICASE, '\0', "icase" },
+ *	{ PATHSPEC_RECURSIVE, '*', "recursive" },
+ *	{ PATHSPEC_REGEXP, '\0', "regexp" },
+ *
+ */
+#define PATHSPEC_FROMTOP    (1<<0)
+#define PATHSPEC_NOGLOB     (1<<1)
+
 struct pathspec {
 	const char **raw; /* get_pathspec() result, not freed by free_pathspec() */
 	int nr;
-	unsigned int has_wildcard:1;
+	unsigned magic;
 	unsigned int recursive:1;
 	int max_depth;
 	struct pathspec_item {
 		const char *match;
 		int len;
-		unsigned int use_wildcard:1;
+		unsigned magic;
 	} *items;
 };
 
diff --git a/dir.c b/dir.c
index 08281d2..6c82615 100644
--- a/dir.c
+++ b/dir.c
@@ -230,7 +230,7 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 			return MATCHED_RECURSIVELY;
 	}
 
-	if (item->use_wildcard && !fnmatch(match, name, 0))
+	if (!(item->magic & PATHSPEC_NOGLOB) && !fnmatch(match, name, 0))
 		return MATCHED_FNMATCH;
 
 	return 0;
@@ -1264,19 +1264,22 @@ int init_pathspec(struct pathspec *pathspec, const char **paths)
 		p++;
 	pathspec->raw = paths;
 	pathspec->nr = p - paths;
+	pathspec->magic = PATHSPEC_NOGLOB;
 	if (!pathspec->nr)
 		return 0;
 
 	pathspec->items = xmalloc(sizeof(struct pathspec_item)*pathspec->nr);
+	memset(pathspec->items, 0, sizeof(struct pathspec_item)*pathspec->nr);
 	for (i = 0; i < pathspec->nr; i++) {
 		struct pathspec_item *item = pathspec->items+i;
 		const char *path = paths[i];
 
 		item->match = path;
 		item->len = strlen(path);
-		item->use_wildcard = !no_wildcard(path);
-		if (item->use_wildcard)
-			pathspec->has_wildcard = 1;
+		if (no_wildcard(path))
+			item->magic |= PATHSPEC_NOGLOB;
+		else
+			pathspec->magic &= ~PATHSPEC_NOGLOB;
 	}
 
 	qsort(pathspec->items, pathspec->nr,
diff --git a/setup.c b/setup.c
index 08f605b..35db910 100644
--- a/setup.c
+++ b/setup.c
@@ -105,23 +105,6 @@ void verify_non_filename(const char *prefix, const char *arg)
 	    "Use '--' to separate filenames from revisions", arg);
 }
 
-/*
- * Magic pathspec
- *
- * NEEDSWORK: These need to be moved to dir.h or even to a new
- * pathspec.h when we restructure get_pathspec() users to use the
- * "struct pathspec" interface.
- *
- * Possible future magic semantics include stuff like:
- *
- *	{ PATHSPEC_NOGLOB, '!', "noglob" },
- *	{ PATHSPEC_ICASE, '\0', "icase" },
- *	{ PATHSPEC_RECURSIVE, '*', "recursive" },
- *	{ PATHSPEC_REGEXP, '\0', "regexp" },
- *
- */
-#define PATHSPEC_FROMTOP    (1<<0)
-
 static struct pathspec_magic {
 	unsigned bit;
 	char mnemonic; /* this cannot be ':'! */
diff --git a/tree-walk.c b/tree-walk.c
index 808bb55..db07fd6 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -586,7 +586,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 never_interesting = ps->magic & PATHSPEC_NOGLOB ? -1 : 0;
 
 	if (!ps->nr) {
 		if (!ps->recursive || ps->max_depth == -1)
@@ -625,7 +625,7 @@ int tree_entry_interesting(const struct name_entry *entry,
 					&never_interesting))
 				return 1;
 
-			if (ps->items[i].use_wildcard) {
+			if (!(ps->items[i].magic & PATHSPEC_NOGLOB)) {
 				if (!fnmatch(match + baselen, entry->path, 0))
 					return 1;
 
@@ -636,12 +636,11 @@ int tree_entry_interesting(const struct name_entry *entry,
 				if (ps->recursive && S_ISDIR(entry->mode))
 					return 1;
 			}
-
 			continue;
 		}
 
 match_wildcards:
-		if (!ps->items[i].use_wildcard)
+		if (ps->items[i].magic & PATHSPEC_NOGLOB)
 			continue;
 
 		/*
-- 
1.7.3.1.256.g2539c.dirty

[PATCH 3/6] Convert prefix_pathspec() to produce struct pathspec_item

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

New field plain_len is used to mark the first part of 'match' where no
magic can be applied. It's not used though. tree_entry_interesting()
should learn to utilize it.

Now we can show get_pathspec() what magic a pathspec has. Make sure
only certain magic is accepted because more will come in future and
not all of them can be converted to plain string like
PATHSPEC_FROMTOP.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 match_pathspec_depth() should also check for unsupported magic..

 cache.h |    1 +
 setup.c |   22 +++++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index fe46f1e..17df06f 100644
--- a/cache.h
+++ b/cache.h
@@ -548,6 +548,7 @@ struct pathspec {
 	struct pathspec_item {
 		const char *match;
 		int len;
+		int plain_len;	/* match[0..plain_len-1] does not have any kind of magic*/
 		unsigned magic;
 	} *items;
 };
diff --git a/setup.c b/setup.c
index 35db910..8f1c2c0 100644
--- a/setup.c
+++ b/setup.c
@@ -126,7 +126,8 @@ static struct pathspec_magic {
  * the prefix part must always match literally, and a single stupid
  * string cannot express such a case.
  */
-static const char *prefix_pathspec(const char *prefix, int prefixlen, const char *elt)
+static const char *prefix_pathspec(struct pathspec_item *item, const char *prefix,
+				   int prefixlen, const char *elt)
 {
 	unsigned magic = 0;
 	const char *copyfrom = elt;
@@ -181,10 +182,17 @@ static const char *prefix_pathspec(const char *prefix, int prefixlen, const char
 			copyfrom++;
 	}
 
+	memset(item, 0, sizeof(*item));
+	item->magic = magic;
+
 	if (magic & PATHSPEC_FROMTOP)
-		return xstrdup(copyfrom);
-	else
-		return prefix_path(prefix, prefixlen, copyfrom);
+		item->match = xstrdup(copyfrom);
+	else {
+		item->match = prefix_path(prefix, prefixlen, copyfrom);
+		item->plain_len = prefixlen;
+	}
+	item->len = strlen(item->match);
+	return 0;
 }
 
 const char **get_pathspec(const char *prefix, const char **pathspec)
@@ -208,7 +216,11 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	dst = pathspec;
 	prefixlen = prefix ? strlen(prefix) : 0;
 	while (*src) {
-		*(dst++) = prefix_pathspec(prefix, prefixlen, *src);
+		struct pathspec_item item;
+		prefix_pathspec(&item, prefix, prefixlen, *src);
+		if (item.magic & ~PATHSPEC_FROMTOP)
+			die("Unsupported magic pathspec %s", *src);
+		*(dst++) = item.match;
 		src++;
 	}
 	*dst = NULL;
-- 
1.7.3.1.256.g2539c.dirty

[PATCH 4/6] Implement parse_pathspec()

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

This function is the same as get_pathspec() except that it produces
struct pathspec directly.

no_prefix code path is necessary because init_pathspec() can be called
without get_pathspec_old() in "diff --no-index" case. Without this
exception, prefix_path() will be eventually called and die because
there is not worktree.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache.h |    5 ++++
 dir.c   |   82 ++++++++++++++++++++++++++++++++++++++++++++++----------------
 setup.c |    4 +-
 3 files changed, 68 insertions(+), 23 deletions(-)

diff --git a/cache.h b/cache.h
index 17df06f..719d4a3 100644
--- a/cache.h
+++ b/cache.h
@@ -443,6 +443,9 @@ extern void set_git_work_tree(const char *tree);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
+struct pathspec_item;
+extern const char *prefix_pathspec(struct pathspec_item *item, const char *prefix,
+				   int prefixlen, const char *elt);
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
 extern const char *pathspec_prefix(const char *prefix, const char **pathspec);
 extern void setup_work_tree(void);
@@ -554,6 +557,8 @@ struct pathspec {
 };
 
 extern int init_pathspec(struct pathspec *, const char **);
+extern int parse_pathspec(struct pathspec *pathspec, const char *prefix,
+			  int argc, const char **argv);
 extern void free_pathspec(struct pathspec *);
 extern int ce_path_match(const struct cache_entry *ce, const struct pathspec *pathspec);
 
diff --git a/dir.c b/dir.c
index 6c82615..d38af0f 100644
--- a/dir.c
+++ b/dir.c
@@ -18,6 +18,8 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, in
 	int check_only, const struct path_simplify *simplify);
 static int get_dtype(struct dirent *de, const char *path, int len);
 
+static const char *no_prefix = "We do not want outside repository check.";
+
 /* helper string functions with support for the ignore_case flag */
 int strcmp_icase(const char *a, const char *b)
 {
@@ -1252,34 +1254,62 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
 	return strcmp(a->match, b->match);
 }
 
-int init_pathspec(struct pathspec *pathspec, const char **paths)
+extern const char *prefix_pathspec(struct pathspec_item *item, const char *prefix,
+				   int prefixlen, const char *elt);
+int parse_pathspec(struct pathspec *pathspec, const char *prefix,
+		   int argc, const char **argv)
 {
-	const char **p = paths;
-	int i;
+	struct pathspec_item *pitem;
+	const char **dst;
+	int prefixlen;
 
 	memset(pathspec, 0, sizeof(*pathspec));
-	if (!p)
-		return 0;
-	while (*p)
-		p++;
-	pathspec->raw = paths;
-	pathspec->nr = p - paths;
-	pathspec->magic = PATHSPEC_NOGLOB;
-	if (!pathspec->nr)
+
+	if (argc == -1) {
+		argc = 0;
+		for (dst = argv; *dst; dst++)
+			argc++;
+	}
+
+	if ((!prefix || prefix == no_prefix) && !argc)
 		return 0;
 
-	pathspec->items = xmalloc(sizeof(struct pathspec_item)*pathspec->nr);
-	memset(pathspec->items, 0, sizeof(struct pathspec_item)*pathspec->nr);
-	for (i = 0; i < pathspec->nr; i++) {
-		struct pathspec_item *item = pathspec->items+i;
-		const char *path = paths[i];
+	if (!argc) {
+		static const char *spec[2];
+		spec[0] = prefix;
+		spec[1] = NULL;
+		init_pathspec(pathspec, spec);
+		pathspec->items[0].plain_len = pathspec->items[0].len;
+		return 0;
+	}
 
-		item->match = path;
-		item->len = strlen(path);
-		if (no_wildcard(path))
-			item->magic |= PATHSPEC_NOGLOB;
+	prefixlen = prefix && prefix != no_prefix ? strlen(prefix) : 0;
+	pathspec->nr = argc; /* be optimistic, lower it later if necessary */
+	pathspec->items = xmalloc(sizeof(struct pathspec_item) * argc);
+	pathspec->raw = argv;
+	pathspec->magic = PATHSPEC_NOGLOB;
+	pitem = pathspec->items;
+	dst = argv;
+
+	while (*argv) {
+		if (prefix == no_prefix) {
+			memset(pitem, 0, sizeof(*pitem));
+			pitem->match = *argv;
+			pitem->len = strlen(pitem->match);
+		}
+		else
+			prefix_pathspec(pitem, prefix, prefixlen, *argv);
+		*dst = *argv++;
+		if (pitem->match && pitem->len) {
+			if (no_wildcard(pitem->match + pitem->plain_len))
+				pitem->magic |= PATHSPEC_NOGLOB;
+			else
+				pathspec->magic &= ~PATHSPEC_NOGLOB;
+			pitem++;
+			dst++;
+		}
 		else
-			pathspec->magic &= ~PATHSPEC_NOGLOB;
+			pathspec->nr--;
 	}
 
 	qsort(pathspec->items, pathspec->nr,
@@ -1288,8 +1318,18 @@ int init_pathspec(struct pathspec *pathspec, const char **paths)
 	return 0;
 }
 
+int init_pathspec(struct pathspec *pathspec, const char **paths)
+{
+	const char **p = paths;
+	while (p && *p)
+		p++;
+	return parse_pathspec(pathspec, no_prefix, p - paths, paths);
+}
+
 void free_pathspec(struct pathspec *pathspec)
 {
+	/* memory leak: pathspec_item->match likely be xstrdup'd by
+	   prefix_pathspec */
 	free(pathspec->items);
 	pathspec->items = NULL;
 }
diff --git a/setup.c b/setup.c
index 8f1c2c0..b074210 100644
--- a/setup.c
+++ b/setup.c
@@ -126,8 +126,8 @@ static struct pathspec_magic {
  * the prefix part must always match literally, and a single stupid
  * string cannot express such a case.
  */
-static const char *prefix_pathspec(struct pathspec_item *item, const char *prefix,
-				   int prefixlen, const char *elt)
+const char *prefix_pathspec(struct pathspec_item *item, const char *prefix,
+			    int prefixlen, const char *elt)
 {
 	unsigned magic = 0;
 	const char *copyfrom = elt;
-- 
1.7.3.1.256.g2539c.dirty

[PATCH 5/6] Convert simple init_pathspec() cases to parse_pathspec()

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

These commands can now take advantage of new pathspec magic, if both
tree_entry_interesting() and match_pathspec_depth() support them properly

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c    |    4 +---
 builtin/ls-tree.c |    2 +-
 builtin/reset.c   |    2 +-
 revision.c        |    9 +++++----
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index a286692..e171a9d 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -759,7 +759,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	const char *show_in_pager = NULL, *default_pager = "dummy";
 	struct grep_opt opt;
 	struct object_array list = OBJECT_ARRAY_INIT;
-	const char **paths = NULL;
 	struct pathspec pathspec;
 	struct string_list path_list = STRING_LIST_INIT_NODUP;
 	int i;
@@ -1020,8 +1019,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			verify_filename(prefix, argv[j]);
 	}
 
-	paths = get_pathspec(prefix, argv + i);
-	init_pathspec(&pathspec, paths);
+	parse_pathspec(&pathspec, prefix, -1, argv + i);
 	pathspec.max_depth = opt.max_depth;
 	pathspec.recursive = 1;
 
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index f0fa7dd..b717bb2 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -166,7 +166,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	if (get_sha1(argv[0], sha1))
 		die("Not a valid object name %s", argv[0]);
 
-	init_pathspec(&pathspec, get_pathspec(prefix, argv + 1));
+	parse_pathspec(&pathspec, prefix, -1, argv + 1);
 	for (i = 0; i < pathspec.nr; i++)
 		pathspec.items[i].magic = PATHSPEC_NOGLOB;
 	pathspec.magic |= PATHSPEC_NOGLOB;
diff --git a/builtin/reset.c b/builtin/reset.c
index 811e8e2..8126e69 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -176,7 +176,7 @@ 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);
+	parse_pathspec(&opt.pathspec, prefix, -1, argv);
 	opt.output_format = DIFF_FORMAT_CALLBACK;
 	opt.format_callback = update_index_from_diff;
 	opt.format_callback_data = &index_was_discarded;
diff --git a/revision.c b/revision.c
index 9bae329..cba32e8 100644
--- a/revision.c
+++ b/revision.c
@@ -1770,8 +1770,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		 */
 		ALLOC_GROW(prune_data.path, prune_data.nr+1, prune_data.alloc);
 		prune_data.path[prune_data.nr++] = NULL;
-		init_pathspec(&revs->prune_data,
-			      get_pathspec(revs->prefix, prune_data.path));
+		parse_pathspec(&revs->prune_data, revs->prefix, -1, prune_data.path);
 	}
 
 	if (revs->def == NULL)
@@ -1804,12 +1803,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		revs->limited = 1;
 
 	if (revs->prune_data.nr) {
-		diff_tree_setup_paths(revs->prune_data.raw, &revs->pruning);
+		/* Careful, we share a lot of pointers here, do not free 1st arg */
+		memcpy(&revs->pruning.pathspec, &revs->prune_data, sizeof(struct pathspec));
 		/* 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);
+			/* Careful, we share a lot of pointers here, do not free 1st arg */
+			memcpy(&revs->diffopt.pathspec, &revs->prune_data, sizeof(struct pathspec));
 	}
 	if (revs->combine_merges)
 		revs->ignore_merges = 0;
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH 5/6] Convert simple init_pathspec() cases to parse_pathspec()

[Junio C Hamano]

I gave a cursory look up to this one. I didn't read 4/6 very carefully
though.

I think teaching the users of "raw[]" field of "struct pathspec" to use
corresponding "items[]" element should come before this series (up to
5/6). After that, once parse_pathspec() API stabilizes, we should teach
users of get_pathspec() to get and pass around "struct pathspec" and when
we reach the point where we can get rid of use of "raw[]" field (the field
itself can stay to serve as a debugging aid if we choose to), everybody
that uses pathspec would be ready to start taking more magic pathspecs.

Having said that, I think the series itself is going in the right
direction.

Thanks for working on this.

[PATCH 6/6] Implement negative pathspec

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I really like the mnemonic ^ but it's regex. ":^Documentation" looks
 nicer than ":~Documentation". Do we plan on supporting regex in
 pathspec too?

 We should mention these magic in a less obscure document. Glossary is
 mostly for developer discussions. git-diff may be a good place
 because it's one of the two frequently used commands (the other one
 is grep) that benefit magic the most (with a short reference from
 git.txt)

 Documentation/glossary-content.txt |    8 +++---
 cache.h                            |    1 +
 dir.c                              |    2 +
 setup.c                            |    1 +
 tree-walk.c                        |   37 +++++++++++++++++++++++++++++++++--
 5 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 3595b58..9a2765d 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -319,12 +319,12 @@ top `/`;;
 	The magic word `top` (mnemonic: `/`) makes the pattern match
 	from the root of the working tree, even when you are running
 	the command from inside a subdirectory.
+
+exclude `~`;;
+	The magic word `exclude` (mnemonic: `~`) excludes paths that
+	match the pattern.
 --
 +
-Currently only the slash `/` is recognized as the "magic signature",
-but it is envisioned that we will support more types of magic in later
-versions of git.
-+
 A pathspec with only a colon means "there is no pathspec". This form
 should not be combined with other pathspec.
 
diff --git a/cache.h b/cache.h
index 719d4a3..75fe589 100644
--- a/cache.h
+++ b/cache.h
@@ -541,6 +541,7 @@ extern int ie_modified(const struct index_state *, struct cache_entry *, struct
  */
 #define PATHSPEC_FROMTOP    (1<<0)
 #define PATHSPEC_NOGLOB     (1<<1)
+#define PATHSPEC_NEGATE     (1<<2)
 
 struct pathspec {
 	const char **raw; /* get_pathspec() result, not freed by free_pathspec() */
diff --git a/dir.c b/dir.c
index d38af0f..46dd35f 100644
--- a/dir.c
+++ b/dir.c
@@ -1305,6 +1305,8 @@ int parse_pathspec(struct pathspec *pathspec, const char *prefix,
 				pitem->magic |= PATHSPEC_NOGLOB;
 			else
 				pathspec->magic &= ~PATHSPEC_NOGLOB;
+			if (pitem->magic & PATHSPEC_NEGATE)
+				pathspec->magic |= PATHSPEC_NEGATE;
 			pitem++;
 			dst++;
 		}
diff --git a/setup.c b/setup.c
index b074210..42beb9b 100644
--- a/setup.c
+++ b/setup.c
@@ -111,6 +111,7 @@ static struct pathspec_magic {
 	const char *name;
 } pathspec_magic[] = {
 	{ PATHSPEC_FROMTOP, '/', "top" },
+	{ PATHSPEC_NEGATE, '~', "exclude" },
 };
 
 /*
diff --git a/tree-walk.c b/tree-walk.c
index db07fd6..936b5da 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -580,15 +580,17 @@ static int match_dir_prefix(const char *base, int baselen,
  *  - zero for no
  *  - negative for "no, and no subsequent entries will be either"
  */
-int tree_entry_interesting(const struct name_entry *entry,
-			   struct strbuf *base, int base_offset,
-			   const struct pathspec *ps)
+static int tree_entry_interesting_1(const struct name_entry *entry,
+				    struct strbuf *base, int base_offset,
+				    const struct pathspec *ps, int negative_magic)
 {
 	int i;
 	int pathlen, baselen = base->len - base_offset;
 	int never_interesting = ps->magic & PATHSPEC_NOGLOB ? -1 : 0;
+	int has_effective_pathspec = 0;
 
 	if (!ps->nr) {
+no_pathspec:
 		if (!ps->recursive || ps->max_depth == -1)
 			return 2;
 		return !!within_depth(base->buf + base_offset, baselen,
@@ -604,6 +606,12 @@ int tree_entry_interesting(const struct name_entry *entry,
 		const char *base_str = base->buf + base_offset;
 		int matchlen = item->len;
 
+		if ((!negative_magic && !(item->magic & PATHSPEC_NEGATE)) ||
+		    ( negative_magic &&  (item->magic & PATHSPEC_NEGATE)))
+			has_effective_pathspec = 1;
+		else
+			continue;
+
 		if (baselen >= matchlen) {
 			/* If it doesn't match, move along... */
 			if (!match_dir_prefix(base_str, baselen, match, matchlen))
@@ -663,5 +671,28 @@ match_wildcards:
 		if (ps->recursive && S_ISDIR(entry->mode))
 			return 1;
 	}
+
+	/* the same effect with ps->nr == 0 */
+	if (!has_effective_pathspec)
+		goto no_pathspec;
+
 	return never_interesting; /* No matches */
 }
+
+int tree_entry_interesting(const struct name_entry *entry,
+			   struct strbuf *base, int base_offset,
+			   const struct pathspec *ps)
+{
+	int next_ret, ret;
+
+	ret = tree_entry_interesting_1(entry, base, base_offset, ps, 0);
+	if (ps->magic & PATHSPEC_NEGATE) {
+		next_ret = tree_entry_interesting_1(entry, base, base_offset, ps, 1);
+		switch (next_ret) {
+		case 2: ret = -1; break;
+		case 1: ret = 0; break;
+		default: break;
+		}
+	}
+	return ret;
+}
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH 0/6] Negation magic pathspec

[Junio C Hamano]

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

> I'm still struggling with read_directory() rewrite so that struct
> pathspec can be used throughout git, but now realized we can at least
> enable magic for certain commands and die() on those that don't.
> This may help move magic pathspec patches forward.

I actually was thinking that teaching those that take bare "const char **"
to take "struct pathspec *" is much more important conversion before
adding more magic to those that already do take "struct pathspec *". The
ones that ask "does this path match the pathspec" would automatically
start honoring negative or quantum or whatever magic once we teach
the magic to match_pathspec(), but the ones that take "const char **" will
have no way of doing so before getting converted to "struct pathspec *"
interface.  If some parts of the system knows more magic and people start
playing with them, the lack of the support in codepaths that haven't been
converted will become real pain point.

That is not to say that these 6 patch series are wasted patches. I just
think it may be doing things in a wrong order.