Re: [PATCH v2] pathspec: advice: long and short forms are incompatible

From: Stavros Ntentos <stdedos@gmail.com>
Date: Mar 30, 2021, 19:05 UTC
Message-ID: <20210330190517.21138-1-133706+stdedos@users.noreply.github.com>
In-Reply-To: <xmqqmtunks8q.fsf@gitster.g>

Show 6 quoted lines

> Administrivia.
>
> If "Stavros Ntentos <133706+stdedos@users.noreply.github.com>" 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.

Show 5 quoted lines

> 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).

Show 6 quoted lines

> > +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)