RFC: git bisect should accept "paths-to-be-excluded"

RFC: git bisect should accept "paths-to-be-excluded"

[Toralf Förster]

I'm bisecting a linux kernel issue and want to ignore all commits just
touching something in ./drives/staging.

Currently the only way would be to specify all dir/subdir combination
under ./linux except that particular directory, right ?

-- 
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Christian Couder]

Hi,

On Mon, Sep 16, 2013 at 2:39 PM, Toralf Förster <toralf.foerster@gmx.de> wrote:
> I'm bisecting a linux kernel issue and want to ignore all commits just
> touching something in ./drives/staging.
>
> Currently the only way would be to specify all dir/subdir combination
> under ./linux except that particular directory, right ?

Yeah, you are right, currently the only way would be to specify all
dir/subdir combination
under ./linux except the particular directory you want to exclude.

It might indeed be useful to have a way to exclude some directories or files.

In practice though, as git bisect is a kind of binary search, if what
you want to exclude is exclusively touched by half the commits, it
will only add one more bisection step if you don't exclude it.

Best regards,
Christian.

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Matthieu Moy]

Christian Couder <christian.couder@gmail.com> writes:

> In practice though, as git bisect is a kind of binary search, if what
> you want to exclude is exclusively touched by half the commits, it
> will only add one more bisection step if you don't exclude it.

Actually, I think the same remark would apply to any other Git command
that deal with a set of revisions. If you want to review code with "git
log -p", but you don't care about a subdirectory, you may want a "git
log -p --ignore-dir foo/" or so, too.

And then, the "it's logarithmic" argument doesn't work anymore ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Christian Couder]

On Tue, Sep 17, 2013 at 10:21 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> In practice though, as git bisect is a kind of binary search, if what
>> you want to exclude is exclusively touched by half the commits, it
>> will only add one more bisection step if you don't exclude it.
>
> Actually, I think the same remark would apply to any other Git command
> that deal with a set of revisions. If you want to review code with "git
> log -p", but you don't care about a subdirectory, you may want a "git
> log -p --ignore-dir foo/" or so, too.

Yeah, and there was a patch series about that 2 years ago:

http://thread.gmane.org/gmane.comp.version-control.git/182830/

> And then, the "it's logarithmic" argument doesn't work anymore ;-).

Sure.

Best regards,
Christian.

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Duy Nguyen]

On Tue, Sep 17, 2013 at 4:03 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Sep 17, 2013 at 10:21 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> In practice though, as git bisect is a kind of binary search, if what
>>> you want to exclude is exclusively touched by half the commits, it
>>> will only add one more bisection step if you don't exclude it.
>>
>> Actually, I think the same remark would apply to any other Git command
>> that deal with a set of revisions. If you want to review code with "git
>> log -p", but you don't care about a subdirectory, you may want a "git
>> log -p --ignore-dir foo/" or so, too.
>
> Yeah, and there was a patch series about that 2 years ago:
>
> http://thread.gmane.org/gmane.comp.version-control.git/182830/

And that's just one of the few attempts if I remember correctly. I
guess it's time revisit it. A few things to sort out before we get to
the implementation:

Support flat or nested negation (i.e.include A, ignore A/B, but
include A/B/C..). Nested thing complicates things so I'm towards the
flat exclusion (exclude B means all inside B, no buts nor excepts) and
probably cover most use cases

Interaction with "git grep --depth"

Syntax. I guess --ignore (or --exclude) is more intuitive than
":(exclude)something" but then it might collide with existing options
(I did not check if --ignore or --exclude is used anywhere though).
The latter also enables combining with other filters, such as
case-insensitive matching..
-- 
Duy

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Toralf Förster]

On 09/17/2013 09:26 AM, Christian Couder wrote:
> Hi,
> 
> On Mon, Sep 16, 2013 at 2:39 PM, Toralf Förster <toralf.foerster@gmx.de> wrote:
>> I'm bisecting a linux kernel issue and want to ignore all commits just
>> touching something in ./drives/staging.
>>
>> Currently the only way would be to specify all dir/subdir combination
>> under ./linux except that particular directory, right ?
> 
> Yeah, you are right, currently the only way would be to specify all
> dir/subdir combination
> under ./linux except the particular directory you want to exclude.
> 
> It might indeed be useful to have a way to exclude some directories or files.

Great to hear

> In practice though, as git bisect is a kind of binary search, if what
> you want to exclude is exclusively touched by half the commits, it
> will only add one more bisection step if you don't exclude it.

Unfortunately not. Linus pulls from Greg's staging tree usually once in
a merge window. It is not uncommon to have hundreds of commits in that
merge. If now (by accident) the merge point is marked as "BAD" and the
base is "GOOD", then git bisect falls into that trap and wastes about
ld(few hundreds) steps - and this happened here for me and each bisect
step took hours ...


> Best regards,
> Christian.
> 


-- 
MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Sep 17, 2013 at 4:03 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Tue, Sep 17, 2013 at 10:21 AM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Christian Couder <christian.couder@gmail.com> writes:
>>>
>>>> In practice though, as git bisect is a kind of binary search, if what
>>>> you want to exclude is exclusively touched by half the commits, it
>>>> will only add one more bisection step if you don't exclude it.
>>>
>>> Actually, I think the same remark would apply to any other Git command
>>> that deal with a set of revisions. If you want to review code with "git
>>> log -p", but you don't care about a subdirectory, you may want a "git
>>> log -p --ignore-dir foo/" or so, too.
>>
>> Yeah, and there was a patch series about that 2 years ago:
>>
>> http://thread.gmane.org/gmane.comp.version-control.git/182830/
>
> And that's just one of the few attempts if I remember correctly. I
> guess it's time revisit it. A few things to sort out before we get to
> the implementation:
>
> Support flat or nested negation (i.e.include A, ignore A/B, but
> include A/B/C..). Nested thing complicates things so I'm towards the
> flat exclusion (exclude B means all inside B, no buts nor excepts) and
> probably cover most use cases

Yeah, it is easy to say that

	git log -- A ':(exclude)A/B' A/B/C

has two positive (A, A/B/C) and one negative (A/B), and then the
most specific one A/B/C matches a path A/B/C/D and hence A/B/C/D is
included.

But to actually _design_ it, there are ambiguities that makes
understanding and explaining the semantics, especially given
pathspecs can have wildcards, icase matches, etc.  For example, is
":(exclude,icase)A/B/?"  more specific than "A/?/C" or less?

So I tend to agree that we should aim for an easier to explain, if
less capable, approach.

> Interaction with "git grep --depth"

I am not sure how that affects anything.  Conceptually, isn't
"--depth" an independent axis to filter out paths that have too many
components after given positive pathspec elements?  E.g. given

	git grep --depth=2 pattern -- A B/C

we will grab paths from two levels starting at A and B/C (so A/1/2
and B/C/1/2 may hit but not A/1/2/3 nor B/C/1/2/3).  Shouldn't
negative pathspecs just filter that depth filtering, i.e. if you
have ":(exclude)*/1/*", even though both "A/1/2" and "A/a/b" may
pass the --depth=2 filter, the former is excluded while the latter
is not.

> Syntax. I guess --ignore (or --exclude) is more intuitive than
> ":(exclude)something" but then it might collide with existing options
> (I did not check if --ignore or --exclude is used anywhere though).
> The latter also enables combining with other filters, such as
> case-insensitive matching..

I do not think it is an option to do this with any mechanism other
than negative pathspecs.

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Piotr Krukowiecki]

Junio C Hamano <gitster@pobox.com> napisał:
>Yeah, it is easy to say that
>
>	git log -- A ':(exclude)A/B' A/B/C
>
>has two positive (A, A/B/C) and one negative (A/B), and then the
>most specific one A/B/C matches a path A/B/C/D and hence A/B/C/D is
>included.
>
>But to actually _design_ it, there are ambiguities that makes
>understanding and explaining the semantics, especially given
>pathspecs can have wildcards, icase matches, etc.  For example, is
>":(exclude,icase)A/B/?"  more specific than "A/?/C" or less?

 What about simply iterating over options in order in which they are specified and the last option that matches specifies the result? 

-- 
Piotr Krukowiecki 

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Junio C Hamano]

Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:

>  What about simply iterating over options in order in which they
>  are specified and the last option that matches specifies the
>  result?

But isn't it very inconsistent from the way normal pathspec works?
"git log -- A B" and "git log -- B A" would give the same result.

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Piotr Krukowiecki]

On Tue, Sep 17, 2013 at 9:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:
>
>>  What about simply iterating over options in order in which they
>>  are specified and the last option that matches specifies the
>>  result?
>
> But isn't it very inconsistent from the way normal pathspec works?
> "git log -- A B" and "git log -- B A" would give the same result.

Both are include-type filters. "--include A --include B" will give the
same result as "--include B --include A" too.

Are there existing include/exclude filters where order does not
matter? For example gitattributes(5) says "When more than one pattern
matches the path, a later line overrides an earlier line."

Ignoring (possible) inconsistency thing, I think they are easy to
understand and use.


-- 
Piotr Krukowiecki

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Junio C Hamano]

Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:

> Ignoring (possible) inconsistency thing, I think they are easy to
> understand and use.

Probably you are right (in the sense that I do not offhand think of
a confusing and ambiguous set of positive and negative pathspecs;
others may find holes in my/our thinking).

I am not sure if it will fit well to the current "struct pathspec"
design, though.  We could start from "when there is any negative
pathspec, disable the 'optimize away the common leading prefix'
thing", I guess.

Re: RFC: git bisect should accept "paths-to-be-excluded"

[Duy Nguyen]

On Wed, Sep 18, 2013 at 12:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Interaction with "git grep --depth"
>
> I am not sure how that affects anything.  Conceptually, isn't
> "--depth" an independent axis to filter out paths that have too many
> components after given positive pathspec elements?  E.g. given
>
>         git grep --depth=2 pattern -- A B/C
>
> we will grab paths from two levels starting at A and B/C (so A/1/2
> and B/C/1/2 may hit but not A/1/2/3 nor B/C/1/2/3).  Shouldn't
> negative pathspecs just filter that depth filtering, i.e. if you
> have ":(exclude)*/1/*", even though both "A/1/2" and "A/a/b" may
> pass the --depth=2 filter, the former is excluded while the latter
> is not.

Implementation details leaked into the design thoughts. I was worried
that the qsort() in pathspec() might make it incompatible with the
:(exclude). Or I was thinking that --depth should be part of this new
filter.. Never mind.

>> Syntax. I guess --ignore (or --exclude) is more intuitive than
>> ":(exclude)something" but then it might collide with existing options
>> (I did not check if --ignore or --exclude is used anywhere though).
>> The latter also enables combining with other filters, such as
>> case-insensitive matching..
>
> I do not think it is an option to do this with any mechanism other
> than negative pathspecs.

Under the hood, a new pathspec magic must be introduced (else we can't
pass them from "git add -u" to git-add--interactive then some other
commands that take pathspec). So --exclude would be transformed to the
pathspec magic, similar to "git grep --depth". But we could add that
later if :(exclude)something is too long to type.
-- 
Duy

[PATCH] Support pathspec magic :(exclude) and its short form :-

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 This is yet another stab at the negative pathspec thing. It's not
 ready yet (there are a few XXXs) but I could use some feedback
 regarding the interface, or the behavior. It looks better this time
 now that pathspec magic is supported (or maybe I'm just biased).

 For :(glob) or :(icase) you're more likely to enable it for all
 pathspec, i.e. --glob-pathspecs. But I expect :(exclude) to be typed
 more often (it does not make sense to add --exclude-pathspecs to
 exclude everything), which is why I add the short form for it.

 We don't have many options that say "negative" in short form.
 Either '!', '-' or '~'. '!' is already used for bash history expansion.
 ~ looks more like $HOME expansion. Which left me '-'.

 Documentation/glossary-content.txt |  5 ++++
 builtin/add.c                      |  5 +++-
 dir.c                              | 50 +++++++++++++++++++++++++++++++-----
 pathspec.c                         |  9 ++++++-
 pathspec.h                         |  4 ++-
 tree-walk.c                        | 52 +++++++++++++++++++++++++++++++++++---
 6 files changed, 112 insertions(+), 13 deletions(-)

diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index e470661..f7d7d8c 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -377,6 +377,11 @@ full pathname may have special meaning:
  - Other consecutive asterisks are considered invalid.
 +
 Glob magic is incompatible with literal magic.
+
+exclude `-`;;
+	After a path matches any non-exclude pathspec, it will be run
+	through all exclude pathspec. If it matches, the path is
+	ignored.
 --
 +
 Currently only the slash `/` is recognized as the "magic signature",
diff --git a/builtin/add.c b/builtin/add.c
index 226f758..0df73ae 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -540,10 +540,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_FROMTOP |
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
-			       PATHSPEC_ICASE);
+			       PATHSPEC_ICASE |
+			       PATHSPEC_EXCLUDE);
 
 		for (i = 0; i < pathspec.nr; i++) {
 			const char *path = pathspec.items[i].match;
+			if (pathspec.items[i].magic & PATHSPEC_EXCLUDE)
+				continue;
 			if (!seen[i] &&
 			    ((pathspec.items[i].magic &
 			      (PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
diff --git a/dir.c b/dir.c
index 23b6de4..e2df82f 100644
--- a/dir.c
+++ b/dir.c
@@ -126,10 +126,13 @@ static size_t common_prefix_len(const struct pathspec *pathspec)
 		       PATHSPEC_MAXDEPTH |
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
-		       PATHSPEC_ICASE);
+		       PATHSPEC_ICASE |
+		       PATHSPEC_EXCLUDE);
 
 	for (n = 0; n < pathspec->nr; n++) {
 		size_t i = 0, len = 0, item_len;
+		if (pathspec->items[n].magic & PATHSPEC_EXCLUDE)
+			continue;
 		if (pathspec->items[n].magic & PATHSPEC_ICASE)
 			item_len = pathspec->items[n].prefix;
 		else
@@ -279,9 +282,10 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
  * pathspec did not match any names, which could indicate that the
  * user mistyped the nth pathspec.
  */
-int match_pathspec_depth(const struct pathspec *ps,
-			 const char *name, int namelen,
-			 int prefix, char *seen)
+static int match_pathspec_depth_1(const struct pathspec *ps,
+				  const char *name, int namelen,
+				  int prefix, char *seen,
+				  int exclude)
 {
 	int i, retval = 0;
 
@@ -290,7 +294,8 @@ int match_pathspec_depth(const struct pathspec *ps,
 		       PATHSPEC_MAXDEPTH |
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
-		       PATHSPEC_ICASE);
+		       PATHSPEC_ICASE |
+		       PATHSPEC_EXCLUDE);
 
 	if (!ps->nr) {
 		if (!ps->recursive ||
@@ -309,6 +314,11 @@ int match_pathspec_depth(const struct pathspec *ps,
 
 	for (i = ps->nr - 1; i >= 0; i--) {
 		int how;
+
+		if ((!exclude &&   ps->items[i].magic & PATHSPEC_EXCLUDE) ||
+		    ( exclude && !(ps->items[i].magic & PATHSPEC_EXCLUDE)))
+			continue;
+
 		if (seen && seen[i] == MATCHED_EXACTLY)
 			continue;
 		how = match_pathspec_item(ps->items+i, prefix, name, namelen);
@@ -327,6 +337,16 @@ int match_pathspec_depth(const struct pathspec *ps,
 		if (how) {
 			if (retval < how)
 				retval = how;
+			/*
+			 * seen[i] is used for detecting unused
+			 * pathspec. For excluded pathspec, it's less
+			 * obvious if seen[] should be set if the
+			 * pathspec matches.
+			 *
+			 * XXX: perhaps we should also set seen[] for
+			 * exclude patterns and stop e.g. "git add"
+			 * from complaining?
+			 */
 			if (seen && seen[i] < how)
 				seen[i] = how;
 		}
@@ -334,6 +354,18 @@ int match_pathspec_depth(const struct pathspec *ps,
 	return retval;
 }
 
+int match_pathspec_depth(const struct pathspec *ps,
+			 const char *name, int namelen,
+			 int prefix, char *seen)
+{
+	int positive, negative;
+	positive = match_pathspec_depth_1(ps, name, namelen, prefix, seen, 0);
+	if (!(ps->magic & PATHSPEC_EXCLUDE) || !positive)
+		return positive;
+	negative = match_pathspec_depth_1(ps, name, namelen, prefix, seen, 1);
+	return negative ? 0 : positive;
+}
+
 /*
  * Return the length of the "simple" part of a path match limiter.
  */
@@ -1375,11 +1407,17 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const stru
 			       PATHSPEC_MAXDEPTH |
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
-			       PATHSPEC_ICASE);
+			       PATHSPEC_ICASE |
+			       PATHSPEC_EXCLUDE);
 
 	if (has_symlink_leading_path(path, len))
 		return dir->nr;
 
+	/*
+	 * XXX: exclude patterns are treated like positive ones in
+	 * create_simplify! This is not wrong, but may make path
+	 * filtering less efficient.
+	 */
 	simplify = create_simplify(pathspec ? pathspec->_raw : NULL);
 	if (!len || treat_leading_path(dir, path, len, simplify))
 		read_directory_recursive(dir, path, len, 0, simplify);
diff --git a/pathspec.c b/pathspec.c
index 4cf2bd3..a021959 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -71,6 +71,7 @@ static struct pathspec_magic {
 	{ PATHSPEC_LITERAL,   0, "literal" },
 	{ PATHSPEC_GLOB,   '\0', "glob" },
 	{ PATHSPEC_ICASE,  '\0', "icase" },
+	{ PATHSPEC_EXCLUDE, '-', "exclude" },
 };
 
 /*
@@ -355,7 +356,7 @@ void parse_pathspec(struct pathspec *pathspec,
 {
 	struct pathspec_item *item;
 	const char *entry = argv ? *argv : NULL;
-	int i, n, prefixlen;
+	int i, n, prefixlen, nr_exclude = 0;
 
 	memset(pathspec, 0, sizeof(*pathspec));
 
@@ -412,6 +413,8 @@ void parse_pathspec(struct pathspec *pathspec,
 		if ((flags & PATHSPEC_LITERAL_PATH) &&
 		    !(magic_mask & PATHSPEC_LITERAL))
 			item[i].magic |= PATHSPEC_LITERAL;
+		if (item[i].magic & PATHSPEC_EXCLUDE)
+			nr_exclude++;
 		if (item[i].magic & magic_mask)
 			unsupported_magic(entry,
 					  item[i].magic & magic_mask,
@@ -427,6 +430,10 @@ void parse_pathspec(struct pathspec *pathspec,
 		pathspec->magic |= item[i].magic;
 	}
 
+	if (nr_exclude == n)
+		die(_("There is nothing to exclude from by :(exclude) patterns.\n"
+		      "Perhaps you forgot to add either ':/' or '.' ?"));
+
 
 	if (pathspec->magic & PATHSPEC_MAXDEPTH) {
 		if (flags & PATHSPEC_KEEP_ORDER)
diff --git a/pathspec.h b/pathspec.h
index a75e924..0c11262 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -7,12 +7,14 @@
 #define PATHSPEC_LITERAL	(1<<2)
 #define PATHSPEC_GLOB		(1<<3)
 #define PATHSPEC_ICASE		(1<<4)
+#define PATHSPEC_EXCLUDE	(1<<5)
 #define PATHSPEC_ALL_MAGIC	  \
 	(PATHSPEC_FROMTOP	| \
 	 PATHSPEC_MAXDEPTH	| \
 	 PATHSPEC_LITERAL	| \
 	 PATHSPEC_GLOB		| \
-	 PATHSPEC_ICASE)
+	 PATHSPEC_ICASE		| \
+	 PATHSPEC_EXCLUDE)
 
 #define PATHSPEC_ONESTAR 1	/* the pathspec pattern satisfies GFNM_ONESTAR */
 
diff --git a/tree-walk.c b/tree-walk.c
index 5ece8c3..9011f87 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -662,9 +662,10 @@ static int match_wildcard_base(const struct pathspec_item *item,
  * Pre-condition: either baselen == base_offset (i.e. empty path)
  * or base[baselen-1] == '/' (i.e. with trailing slash).
  */
-enum interesting tree_entry_interesting(const struct name_entry *entry,
-					struct strbuf *base, int base_offset,
-					const struct pathspec *ps)
+static enum interesting tree_entry_interesting_1(const struct name_entry *entry,
+						 struct strbuf *base, int base_offset,
+						 const struct pathspec *ps,
+						 int exclude)
 {
 	int i;
 	int pathlen, baselen = base->len - base_offset;
@@ -676,7 +677,8 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
 		       PATHSPEC_MAXDEPTH |
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
-		       PATHSPEC_ICASE);
+		       PATHSPEC_ICASE |
+		       PATHSPEC_EXCLUDE);
 
 	if (!ps->nr) {
 		if (!ps->recursive ||
@@ -697,6 +699,10 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
 		const char *base_str = base->buf + base_offset;
 		int matchlen = item->len, matched = 0;
 
+		if ((!exclude &&   item->magic & PATHSPEC_EXCLUDE) ||
+		    ( exclude && !(item->magic & PATHSPEC_EXCLUDE)))
+			continue;
+
 		if (baselen >= matchlen) {
 			/* If it doesn't match, move along... */
 			if (!match_dir_prefix(item, base_str, match, matchlen))
@@ -782,3 +788,41 @@ match_wildcards:
 	}
 	return never_interesting; /* No matches */
 }
+
+enum interesting tree_entry_interesting(const struct name_entry *entry,
+					struct strbuf *base, int base_offset,
+					const struct pathspec *ps)
+{
+	enum interesting positive, negative;
+	positive = tree_entry_interesting_1(entry, base, base_offset, ps, 0);
+
+	/*
+	 *   #  | positive | negative | result
+	 * -----+----------+----------+-------
+	 * 1..4 |   -1     |    *     |  -1
+	 * 5..8 |    0     |    *     |   0
+	 *   9  |    1     |   -1     |   1
+	 *  10  |    1     |    0     |   1
+	 *  11  |    1     |    1     |   0
+	 *  12  |    1     |    2     |   0
+	 *  13  |    2     |   -1     |   2
+	 *  14  |    2     |    0     |   2
+	 *  15  |    2     |    1     |   0
+	 *  16  |    2     |    2     |  -1
+	 */
+
+	if (!(ps->magic & PATHSPEC_EXCLUDE) ||
+	    positive <= entry_not_interesting) /* #1..#8 */
+		return positive;
+
+	negative = tree_entry_interesting_1(entry, base, base_offset, ps, 1);
+
+	if (negative <= entry_not_interesting)	 /* #9, #10, #13, #14 */
+		return positive;
+	if ((positive == entry_interesting &&
+	     negative >= entry_interesting) || /* #11, #12 */
+	    (positive == all_entries_interesting &&
+	     negative == entry_interesting)) /* #15 */
+		return entry_not_interesting;
+	return all_entries_not_interesting; /* #16 */
+}
-- 
1.8.2.82.gc24b958

Re: [PATCH] Support pathspec magic :(exclude) and its short form :-

[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>
> ---
>  This is yet another stab at the negative pathspec thing. It's not
>  ready yet (there are a few XXXs) but I could use some feedback
>  regarding the interface, or the behavior. It looks better this time
>  now that pathspec magic is supported (or maybe I'm just biased).
>
>  For :(glob) or :(icase) you're more likely to enable it for all
>  pathspec, i.e. --glob-pathspecs. But I expect :(exclude) to be typed
>  more often (it does not make sense to add --exclude-pathspecs to
>  exclude everything), which is why I add the short form for it.
>
>  We don't have many options that say "negative" in short form.
>  Either '!', '-' or '~'. '!' is already used for bash history expansion.
>  ~ looks more like $HOME expansion. Which left me '-'.

I agree with your decision to reject ~, but "!not-this-pattern" is
very much consistent with the patterns used in .gitignore (and the
"--exclude <pattern>" option), so avoiding "!" and introducing an
inconsistent "-" only to appease bash leaves somewhat a funny taste
in my mouth.

>  Documentation/glossary-content.txt |  5 ++++
>  builtin/add.c                      |  5 +++-
>  dir.c                              | 50 +++++++++++++++++++++++++++++++-----
>  pathspec.c                         |  9 ++++++-
>  pathspec.h                         |  4 ++-
>  tree-walk.c                        | 52 +++++++++++++++++++++++++++++++++++---
>  6 files changed, 112 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> index e470661..f7d7d8c 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -377,6 +377,11 @@ full pathname may have special meaning:
>   - Other consecutive asterisks are considered invalid.
>  +
>  Glob magic is incompatible with literal magic.
> +
> +exclude `-`;;
> +	After a path matches any non-exclude pathspec, it will be run
> +	through all exclude pathspec. If it matches, the path is
> +	ignored.
>  --
>  +
>  Currently only the slash `/` is recognized as the "magic signature",

No longer, no?  "magic signature" is a non-alphanumeric that follows
the ':' introducer, as opposed to "magic words" that are in ":(...)".

> diff --git a/builtin/add.c b/builtin/add.c
> index 226f758..0df73ae 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -540,10 +540,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  			       PATHSPEC_FROMTOP |
>  			       PATHSPEC_LITERAL |
>  			       PATHSPEC_GLOB |
> -			       PATHSPEC_ICASE);
> +			       PATHSPEC_ICASE |
> +			       PATHSPEC_EXCLUDE);
>  
>  		for (i = 0; i < pathspec.nr; i++) {
>  			const char *path = pathspec.items[i].match;
> +			if (pathspec.items[i].magic & PATHSPEC_EXCLUDE)
> +				continue;
>  			if (!seen[i] &&
>  			    ((pathspec.items[i].magic &
>  			      (PATHSPEC_GLOB | PATHSPEC_ICASE)) ||

So "git add ':(exclude)junk/' '*.c'" to add all .c files except for
the ones in the 'junk/' directory may find that ':(exclude)junk/'
matched nothing (because there is no .c file in there), and that is
not an error.  It makes sense to me.

> diff --git a/dir.c b/dir.c
> index 23b6de4..e2df82f 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -126,10 +126,13 @@ static size_t common_prefix_len(const struct pathspec *pathspec)
>  		       PATHSPEC_MAXDEPTH |
>  		       PATHSPEC_LITERAL |
>  		       PATHSPEC_GLOB |
> -		       PATHSPEC_ICASE);
> +		       PATHSPEC_ICASE |
> +		       PATHSPEC_EXCLUDE);
>  
>  	for (n = 0; n < pathspec->nr; n++) {
>  		size_t i = 0, len = 0, item_len;
> +		if (pathspec->items[n].magic & PATHSPEC_EXCLUDE)
> +			continue;
>  		if (pathspec->items[n].magic & PATHSPEC_ICASE)
>  			item_len = pathspec->items[n].prefix;
>  		else

Likewise.  Exclusion does not participate in the early culling with
the common prefix.

> @@ -1375,11 +1407,17 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const stru
>  			       PATHSPEC_MAXDEPTH |
>  			       PATHSPEC_LITERAL |
>  			       PATHSPEC_GLOB |
> -			       PATHSPEC_ICASE);
> +			       PATHSPEC_ICASE |
> +			       PATHSPEC_EXCLUDE);
>  
>  	if (has_symlink_leading_path(path, len))
>  		return dir->nr;
>  
> +	/*
> +	 * XXX: exclude patterns are treated like positive ones in
> +	 * create_simplify! This is not wrong, but may make path
> +	 * filtering less efficient.
> +	 */

True, but "git add ':(exclude)a/b/c' a/b" would not suffer.  And
those who do "git add ':(exclude)a/b' a/b/c" deserve it, no ;-)?

> @@ -427,6 +430,10 @@ void parse_pathspec(struct pathspec *pathspec,
>  		pathspec->magic |= item[i].magic;
>  	}
>  
> +	if (nr_exclude == n)
> +		die(_("There is nothing to exclude from by :(exclude) patterns.\n"
> +		      "Perhaps you forgot to add either ':/' or '.' ?"));

;-).

> +enum interesting tree_entry_interesting(const struct name_entry *entry,
> +					struct strbuf *base, int base_offset,
> +					const struct pathspec *ps)
> +{
> +	enum interesting positive, negative;
> +	positive = tree_entry_interesting_1(entry, base, base_offset, ps, 0);
> +
> +	/*
> +	 *   #  | positive | negative | result
> +	 * -----+----------+----------+-------
> +	 * 1..4 |   -1     |    *     |  -1
> +	 * 5..8 |    0     |    *     |   0
> +	 *   9  |    1     |   -1     |   1
> +	 *  10  |    1     |    0     |   1
> +	 *  11  |    1     |    1     |   0
> +	 *  12  |    1     |    2     |   0
> +	 *  13  |    2     |   -1     |   2
> +	 *  14  |    2     |    0     |   2
> +	 *  15  |    2     |    1     |   0
> +	 *  16  |    2     |    2     |  -1
> +	 */

Not sure what this case-table means...

> +	if (!(ps->magic & PATHSPEC_EXCLUDE) ||
> +	    positive <= entry_not_interesting) /* #1..#8 */
> +		return positive;
> +
> +	negative = tree_entry_interesting_1(entry, base, base_offset, ps, 1);
> +
> +	if (negative <= entry_not_interesting)	 /* #9, #10, #13, #14 */
> +		return positive;
> +	if ((positive == entry_interesting &&
> +	     negative >= entry_interesting) || /* #11, #12 */
> +	    (positive == all_entries_interesting &&
> +	     negative == entry_interesting)) /* #15 */
> +		return entry_not_interesting;
> +	return all_entries_not_interesting; /* #16 */
> +}

Re: [PATCH] Support pathspec magic :(exclude) and its short form :-

[Duy Nguyen]

On Thu, Nov 21, 2013 at 6:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>  We don't have many options that say "negative" in short form.
>>  Either '!', '-' or '~'. '!' is already used for bash history expansion.
>>  ~ looks more like $HOME expansion. Which left me '-'.
>
> I agree with your decision to reject ~, but "!not-this-pattern" is
> very much consistent with the patterns used in .gitignore (and the
> "--exclude <pattern>" option), so avoiding "!" and introducing an
> inconsistent "-" only to appease bash leaves somewhat a funny taste
> in my mouth.

The thing about '!'  is it's history expansion in bash and I suspect
not many people are aware of it. So "git log -- :!something" may
recall the last command that has "something" in it, which is confusing
for those new people and may potentially be dangerous (multiple
command in one line, separated by semicolon). Compared to ":git log --
(exclude)somethign" the worst that could happen is a syntax error
message from bash.

Other than that I'm fine with '!' being the shortcut.

Btw I'm thinking of extending pathspec magic syntax a bit to allow
path completion. Right now the user has to write

git log -- :-Documentation

which does not play well with path completion. I'm thinking of accepting

git log -- :- Documentation

In other words, if there's no path (or pattern) component after the
magic, then the next argument must contain the path. This enables path
completion and I haven't seen any drawbacks yet..

>> @@ -427,6 +430,10 @@ void parse_pathspec(struct pathspec *pathspec,
>>               pathspec->magic |= item[i].magic;
>>       }
>>
>> +     if (nr_exclude == n)
>> +             die(_("There is nothing to exclude from by :(exclude) patterns.\n"
>> +                   "Perhaps you forgot to add either ':/' or '.' ?"));
>
> ;-).

Hey it was originally not there, then I made a mistake of typing "git
log -- :-po" and wondered why it shows nothing. Intuitively, if "git
log" shows every path, then "git log -- :-po" should show every path
except 'po' and the user should not be required to type "git log -- :/
:-po". parse_pathspec() can do that, but it's more work and I'm lazy
so I push that back to the user until they scream :)

>> +enum interesting tree_entry_interesting(const struct name_entry *entry,
>> +                                     struct strbuf *base, int base_offset,
>> +                                     const struct pathspec *ps)
>> +{
>> +     enum interesting positive, negative;
>> +     positive = tree_entry_interesting_1(entry, base, base_offset, ps, 0);
>> +
>> +     /*
>> +      *   #  | positive | negative | result
>> +      * -----+----------+----------+-------
>> +      * 1..4 |   -1     |    *     |  -1
>> +      * 5..8 |    0     |    *     |   0
>> +      *   9  |    1     |   -1     |   1
>> +      *  10  |    1     |    0     |   1
>> +      *  11  |    1     |    1     |   0
>> +      *  12  |    1     |    2     |   0
>> +      *  13  |    2     |   -1     |   2
>> +      *  14  |    2     |    0     |   2
>> +      *  15  |    2     |    1     |   0
>> +      *  16  |    2     |    2     |  -1
>> +      */
>
> Not sure what this case-table means...

Sorry, because tree_entry_interesting_1() returns more than "match or
not", we need to combine the result from positive pathspec with the
negative one to correctly handle all_not_interesting and
all_interesting. This table sums it up. I'll add more explanation in
the next patch.
-- 
Duy

Re: [PATCH] Support pathspec magic :(exclude) and its short form :-

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Btw I'm thinking of extending pathspec magic syntax a bit to allow
> path completion. Right now the user has to write
>
> git log -- :-Documentation
>
> which does not play well with path completion. I'm thinking of accepting
>
> git log -- :- Documentation

Please don't.  That does not help our users, but actively harm them.
They have to stop and wonder why a single pathspec is spelled as two
tokens on the command line of some other people.

Doing that stupidity only to help those who polish the tool (namely,
"bash completion") to be lazy is doubly wrong (in the meantime, the
users can type your second variant and then edit the result).

For the same reason why I do not think rewriting

	echo "hello, world!"

to

	echo "hello, world-"

only to work around a pitfall of a particular tool (namely "bash")
makes any sense, I do not think it makes sense to make _our_ tool
inconsistent by using "!excluded" in the files (and --exclude) and
"-not this pattern" only here.

>>> +     if (nr_exclude == n)
>>> +             die(_("There is nothing to exclude from by :(exclude) patterns.\n"
>>> +                   "Perhaps you forgot to add either ':/' or '.' ?"));
>>
>> ;-).
>
> Hey it was originally not there,...

I am not objecting. I noticed it and was commending on it as "a nice
touch" ;-)

>>> +     /*
>>> +      *   #  | positive | negative | result
>>> +      * -----+----------+----------+-------
>>> +      * 1..4 |   -1     |    *     |  -1
>>> +      * 5..8 |    0     |    *     |   0
>>> +      *   9  |    1     |   -1     |   1
>>> +      *  10  |    1     |    0     |   1
>>> +      *  11  |    1     |    1     |   0
>>> +      *  12  |    1     |    2     |   0
>>> +      *  13  |    2     |   -1     |   2
>>> +      *  14  |    2     |    0     |   2
>>> +      *  15  |    2     |    1     |   0
>>> +      *  16  |    2     |    2     |  -1
>>> +      */
>>
>> Not sure what this case-table means...
>
> Sorry, because tree_entry_interesting_1() returns more than "match
> or not", we need to combine the result from positive pathspec with
> the negative one to correctly handle all_not_interesting and
> all_interesting. This table sums it up. I'll add more explanation
> in the next patch.

I managed to have guessed what the three columns on the right meant;
I was wondering about the meaning of the "#" column and where it is
defined/explained.