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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 27, 2021, 18:36 UTC
Message-ID: <xmqqa6qoqw9n.fsf@gitster.g>
In-Reply-To: <YF792/TkS3Ssw9NS@coredump.intra.peff.net>

Jeff King <peff@peff.net> 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;