Stavros Ntentos [off-list ref] writes:
Administrivia.
If "Stavros Ntentos [off-list ref]" is an
address that is not meant to receive any e-mail, please do not
include it on the Cc line and force those who respond to you to
remove it when replying.
+static const char mixed_pathspec_magic[] = N_(
+ "'%.*s...': cannot mix shortform magic with longform [e.g. like :(glob)].\n"
OK. Just a bit of bikeshedding.
cannot mix short and long form magic
cannot mix shortform magic with longform
The former is a bit shorter. Also, if we show (with %.*s) the
actual beginning of their attempt, e.g. when they gave us [*]
git show -- ':!(global,icase)foo'
if we show
':!(glo...': cannot mix short and long form pathspec magic
or even just
':!(...': cannot mix short and long form pathspec magic
it may be sufficiently clear where the problem is.
+ "If '%.*s...' is a valid path, explicitly terminate magic parsing with ':'; or");
The seemingly-stray '; or' at the end aside, I am not sure what this
is trying to say. If the sample input were [*] above, what are we
asking? "if 'foo' is a valid path"? No, we are showing 7 chars
starting at pos, so "if 'global,i...' is a valid path"?
If ':(global,icase)foo' were the exact path the user wants to match
with, then "prefix the whole thing with ":(literal)" would be an
understandable hint, but that is not what you are suggesting.
In short, I do not quite agree with the second line of the message.
It may be more helpful if, rather than looking at what comes after
'(', we looked at what came before '(' and helped the user write
them out in the longform, i.e. perhaps we can tell them the moral
equivalent of:
If you meant by to use the pathspec magic '!' with other
longform magic after '(' with ":!(...", use ":(exclude,..."
instead. Short and long form of pathspec magic do not mix.
+static int extra_lookahead_chars = 7;
A few problems:
- This is not something we want to configure. It does not need to
be a variable.
- This is not something anybody other than the code in the new
block "if (ch == '(')" in parse_short_magic() needs to know. It
does not need to be a file-scope static.
- 7 is way too long for warning against something like ":!(glob)",
no?
But with the above "It may be more helpful" suggestion, notice that
I am deliberately refraining from looking at what comes after '(',
so extra-lookahead may not be necessary after all, and nitpicking
about it may be moot.
Thanks.
quoted hunk
@@ -356,6 +361,17 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
continue;
}
+ if (ch == '(') {
+ /*
+ * a possible mistake: once you started a shortform
+ * you cannot add a longform, e.g. ":!(top)"
+ */
+ advise_if_enabled(ADVICE_MIXED_SHORT_LONG_MAGIC_PATHSPEC,
+ _(mixed_pathspec_magic),
+ (int)(pos-elem+extra_lookahead_chars), elem,
+ extra_lookahead_chars, pos);
+ }
+
if (!is_pathspec_magic(ch))
break;
Administrivia.
If "Stavros Ntentos [off-list ref]" is an
address that is not meant to receive any e-mail, please do not
include it on the Cc line and force those who respond to you to
remove it when replying.
I am trying. However, git-send-email keeps pulling that no-reply address, and
git-send-email does not offer any `--exclude-addresses="*glob*"`-like option.
or even just
':!(...': cannot mix short and long form pathspec magic
it may be sufficiently clear where the problem is.
I slightly disagree, and prefer the `extra_lookahead_chars`. Just 3 characters
[`:!(`] is a bit too little, and the total message sits below the "you can
disable this message" hint.
The seemingly-stray '; or' at the end aside, I am not sure what this
is trying to say.
See the testcase:
hint: If '(glob)*...' is a valid path, explicitly terminate magic parsing with ':'; or
hint: Disable this message with "git config advice.mixedShortLongMagicPathspec false"
I am segway-ing from the "explicitly stop parsing" to the "disable this message" sentence.
If ':(global,icase)foo' were the exact path the user wants to match
with, then "prefix the whole thing with ":(literal)" would be an
understandable hint, but that is not what you are suggesting.
I am siding with the "user entered this situation by mistake", and not with the
"user is explicitly trying to match a file named `:(global,icase)foo`" side.
Offering a more complete message, will become more complex. I disagree with that.
I can settle by offering examples (mine and yours) in the documentation.
It may be more helpful if, rather than looking at what comes after
'(', we looked at what came before '(' and helped the user write
them out in the longform
I don't see any explicit code in parsing the shortform magics, except:
/* Special case alias for '!' */
if (ch == '^') {
*magic |= PATHSPEC_EXCLUDE;
continue;
}
and therefore I would like to avoid such task (although I love well-written
DWIMs-or-close-to-them).
quoted
+static int extra_lookahead_chars = 7;
A few problems:
- This is not something we want to configure. It does not need to
be a variable.
I hate macros, only because I cannot expand or modify them during gdb.
(Suggestions welcome! :-D)
- This is not something anybody other than the code in the new
block "if (ch == '(')" in parse_short_magic() needs to know. It
does not need to be a file-scope static.
True, but the message was explicitly referred to with i18n code
specifically targeted for such initialization.
I like code doing the same job, sitting together.
I'd prefer to either move both inside (since no one else will ever
refer to this message either), or keep them as-is.
- 7 is way too long for warning against something like ":!(glob)",
no?
GRRRRR C :-p
(I'll push the changes on the next iteration; including the `like glob`
removed, and whatever comes from our discussion.)
But with the above "It may be more helpful" suggestion, notice that
I am deliberately refraining from looking at what comes after '(',
so extra-lookahead may not be necessary after all, and nitpicking
about it may be moot.
Lookahead is simply to inform user what git will do with the current
state of affairs, i.e.:
git log --oneline --format=%s -- ':!(glob)**/file'
will filter with
NOT '(glob)**/file'
path (truncated for brevity)
quoted
- 7 is way too long for warning against something like ":!(glob)",
no?
GRRRRR C :-p
(I'll push the changes on the next iteration; including the `like glob`
removed, and whatever comes from our discussion.)
Actually ... C does not have a problem with it:
root@94ae60990b85:/usr/src/git/t# ../git log -- ':!(glob)'
hint: ':!(glob)...': cannot mix shortform magic with longform [e.g. like :(glob)].
hint: If '(glob)...' is a valid path, explicitly terminate magic parsing with ':'; or
hint: Disable this message with "git config advice.mixedShortLongMagicPathspec false"
commit 14b8580dde326a0bbf9abacac7e09dbe4c38c25e (HEAD -> fix/mixed-short-long-magic)
True, it does not look nice; theoretically, any number (even 1!) could be
too much to contain ellipsis after that text.
Given the size of the change, I would want to say that it is acceptable to
leave this as-is.
Unless you'd like me to write a generic
truncate_at_chars(char *text, int width, char *ellipsis_char_or_default)
function.