> Administrivia.
>
> If "Stavros Ntentos <133706+stdedos@xxxxxxxxxxxxxxxxxxxxxxxx>" 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).
> > +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)
