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