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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 28, 2021, 19:12 UTC
Message-ID: <xmqqmtunks8q.fsf@gitster.g>
In-Reply-To: <20210328154532.23803-1-133706+stdedos@users.noreply.github.com>

Stavros Ntentos <stdedos@gmail.com> writes:

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.

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

Show 17 quoted lines

> @@ -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;