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

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 ?

Hi,

Quoted 3 lines · Toralf Förster

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

Quoted 2 lines

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.

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.

Quoted 3 lines · Christian Couder

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

Quoted 6 lines · Matthieu Moy

Quoted 3 lines · Christian Couder

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.

Quoted 4 lines

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/

Quoted 8 lines · Christian Couder

Quoted 6 lines · Matthieu Moy

Quoted 3 lines · Christian Couder

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.

Quoted 8 lines

Quoted 4 lines

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

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?

Quoted 3 lines · Piotr Krukowiecki

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.

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

Quoted 1 line

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.

Quoted 5 lines

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.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

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

Documentation/glossary-content.txt +0 -0

index e470661..f7d7d8c 100644
@@ -377,6 +377,11 @@ full pathname may have special meaning:
  - Other consecutive asterisks are considered invalid.

builtin/add.c +0 -1

index 226f758..0df73ae 100644
@@ -540,10 +540,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       PATHSPEC_FROMTOP |
 			       PATHSPEC_LITERAL |
 			       PATHSPEC_GLOB |
-			       PATHSPEC_ICASE);

dir.c +0 -1

index 23b6de4..e2df82f 100644
@@ -126,10 +126,13 @@ static size_t common_prefix_len(const struct pathspec *pathspec)
 		       PATHSPEC_MAXDEPTH |
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
-		       PATHSPEC_ICASE);

dir.c +1 -1

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

dir.c +11 -0

+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

dir.c +15 -0

+	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 */
+}

Quoted 3 lines

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.

pathspec.c void parse_pathspec(struct pathspec *pathspec, · line 435

Quoted 7 lines

pathspec.c +3 -0

@@ -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 '.' ?"));

;-).

Quoted 15 lines

tree-walk.c +15 -0

+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

tree-walk.c match_wildcards: · line 812

Quoted 6 lines

tree-walk.c +6 -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...

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

Quoted 5 lines

pathspec.c void parse_pathspec(struct pathspec *pathspec, · line 435

Quoted 3 lines

pathspec.c +3 -0

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

Quoted 15 lines

Quoted 14 lines

tree-walk.c +14 -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
+      */

Quoted 1 line

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.