Re: [RFC PATCH v1 1/2] pathspec: warn: long and short forms are incompatible

Jeff King [off-list ref] writes:

It also feels like any checks like this should be relying on the
existing pathspec-magic parser a bit more. I don't know the pathspec
code that well, but surely at some point it has a notion of which parts
are magic flags (e.g., after parse_element_magic in init_pathspec_item).

Absolutely.  parse_element_magic() decides, by looking at the
character after the initial ':', the we are looking at the longhand
form when magic sequence is introduced by ":(".

Otherwise, : must be followed by shorthand form, where the only
usable ones are '/' (synonym for top), '!' and '^' (synonym for
exclude), and ':' (end of short-form marker), but most importantly,
'(' will *never* be a valid shorthand form (because it is not part
of the GIT_PATHSPEC_MAGIC class of bytes).

So, presumably, inside parse_short_magic(), you could detect '('
and complain it as an attempt to switch to longform.

Here is to illustrate where to hook the check.  With this, I can get
something like this:

    $ git show -s --oneline -- ':!/(foobar)frotz' ':/(bar)nitfol' .
    warning: :!/: cannot mix shortform magic with longform like (exclude)
    warning: :/: cannot mix shortform magic with longform like (exclude)
    84d06cdc06 Sync with v2.31.1

Note that this illustration does not show the best warning message.
To improve it to a bit more useful level, I think two things need to
happen on top.

 * Use advice to allow users sequelch.

 * Show a few letters after '(' (but pay attention to the end of the
   string, which ends with either '\0' or ':'), and lose the
   substring "like (exclude)" from the message.

That would have given

    hint: ":!/(foo...": cannot mix shortform and longform magic

for the above example.

I initially thought it might make it even better if we looked ahead
of what comes after '(', took the substring before ',' or ')', and
looked up pathspec_magic[] array, BUT I do not think it is a good
idea.  It would be confusing if we do not give the same advice to
":!(literol)" when the user does get one for ":!(literal)".  So the
above "two things need to happen" list deliberately excludes an
"improvement" doing so.


 pathspec.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git i/pathspec.c w/pathspec.c
index 18b3be362a..cd343d5b54 100644
--- i/pathspec.c
+++ w/pathspec.c

@@ -336,6 +336,9 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len,
 	return pos;
 }
 
+static const char mixed_pathspec_magic[] =
+	N_("%.*s: cannot mix shortform magic with longform like (exclude)");
+
 /*
  * Parse the pathspec element looking for short magic
  *

@@ -356,6 +359,16 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
 			continue;
 		}
 
+		if (ch == '(') {
+			/*
+			 * a possible common mistake: once you started
+			 * a shortform you cannot add (longform), e.g.
+			 * ":!(top)"
+			 */
+			warning(_(mixed_pathspec_magic),
+				(int)(pos-elem), elem);
+		}
+
 		if (!is_pathspec_magic(ch))
 			break;
 

Junio C Hamano [off-list ref] writes:

Jeff King [off-list ref] writes:

quoted

It also feels like any checks like this should be relying on the
existing pathspec-magic parser a bit more. I don't know the pathspec
code that well, but surely at some point it has a notion of which parts
are magic flags (e.g., after parse_element_magic in init_pathspec_item).

Absolutely.  parse_element_magic() decides, by looking at the
character after the initial ':', the we are looking at the longhand
form when magic sequence is introduced by ":(".

Absolutely from me too! I would want to re-use more of the existing code.
In response to that, patch
https://lore.kernel.org/git/20210328152629.16486-1-133706+stdedos@users.noreply.github.com/
clearly shows that it is possible to re-use the de-facto long form magic parsing
code to attempt to parse the string that continues as a long form magic.

This could theoretically pave the road for mixed short/long form parsing
(in that order), if would be so desired.

However, as probably suspected, it suffers from one big problem:
	if (ARRAY_SIZE(pathspec_magic) <= i)
		die(_("Invalid pathspec magic '%.*s' in '%s'"),
		    (int) len, pos, elem);

which is an unconditional `exit(128)`.

I don't know if it's possible to rescue (or redirect) that `die` call.
In any case, considering that, this again deviates from the
"small informative" change this commit was supposed to be.

Having said that, I have cooked a working patch with the proposed code.