threads / patch / 28653

patch, 6 partsNegation magic pathspec

Subject: [PATCH 0/6] Negation magic pathspec

## tl;dr

12 messages between Oct 11, 2011 and Oct 13, 2011. Diffs are folded; open one to read it.

replies: 11people: 2as markdown or json

Nguyễn Thái Ngọc Duy· Oct 11, 2011, 22:44 UTC · lore

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
Nguyễn Thái Ngọc Duy· Oct 11, 2011, 22:44 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/6] Recognize magic pathspec as filenames

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(-)
Show changes to setup.c +7 −9
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
Junio C Hamano· Oct 12, 2011, 20:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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

Show 37 quoted lines
>  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);
Nguyen Thai Ngoc Duy· Oct 13, 2011, 04:23 UTC · re: Junio C Hamano · lore

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

2011/10/13 Junio C Hamano <gitster@pobox.com>:
Show 5 quoted lines
> "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
Junio C Hamano· Oct 13, 2011, 06:06 UTC · re: Nguyen Thai Ngoc Duy · lore

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

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

Nguyễn Thái Ngọc Duy· Oct 11, 2011, 22:44 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/6] Replace has_wildcard with PATHSPEC_NOGLOB

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(-)
Show changes to 6 files +33 −30

builtin/ls-files.c, builtin/ls-tree.c, cache.h, dir.c, setup.c, tree-walk.c

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
Nguyễn Thái Ngọc Duy· Oct 11, 2011, 22:44 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +18 −5

cache.h, setup.c

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
Nguyễn Thái Ngọc Duy· Oct 11, 2011, 22:44 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 4/6] Implement parse_pathspec()

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(-)
Show changes to 3 files +68 −23

cache.h, dir.c, setup.c

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
Nguyễn Thái Ngọc Duy· Oct 11, 2011, 22:44 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 4 files +8 −9

builtin/grep.c, builtin/ls-tree.c, builtin/reset.c, revision.c

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
Junio C Hamano· Oct 13, 2011, 00:29 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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.
Nguyễn Thái Ngọc Duy· Oct 11, 2011, 22:44 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 6/6] Implement negative pathspec

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(-)
Show changes to 5 files +42 −7

Documentation/glossary-content.txt, cache.h, dir.c, setup.c, tree-walk.c

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
Junio C Hamano· Oct 11, 2011, 23:17 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 0/6] Negation magic pathspec

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.

← back to recent threads