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

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 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.

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

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.

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

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

;-).

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

On Thu, Nov 21, 2013 at 6:48 AM, Junio C Hamano [off-list ref] wrote:

quoted

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

quoted

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

quoted

+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