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

[Junio C Hamano <gitster@pobox.com>]

Stavros Ntentos <stdedos@gmail.com> writes:

> It can be a "reasonable" mistake to mix short and long forms,
> e.g. `:!(glob)`, instead of the (correct) `:(exclude,glob)`.

The word "form" may be sufficient for today's us because we have
been so focused on this particular pathspec magic issue.  

But reviewers are not, and those who read the "git log" output later
are not, either.  Let's be friendly and helpful to them by saying
"form of pathspec magic" or somesuch.

The same comment applies to the patch title.

It also might be more friendly to readers what the mistaken form
would do, too.

Here is my attempt, taking all of the above into account.

    It is a hard-to-notice mistake to try mixing short and long
    forms of pathspec magic, e.g. instead of ':(exclude,glob)', it
    may be tempting to write ':!(glob)', which stops at ":!",
    i.e. the end of the short-form pathspec magic, and the "(glob)"
    is taken as the beginning part of the pathspec, wanting to match
    a file or a directory whose name begins with that literal string.

> Teach git to issue an advice when such a pathspec is given.
>         i.e.: While in short form parsing:
>         * if the string contains an open parenthesis [`(`], and
>         * without having explicitly terminated magic parsing (`:`)
>         issue an advice hinting to that fact.

OK.

> Based on "Junio C Hamano <gitster@pobox.com>"'s code suggestions:
> * https://lore.kernel.org/git/xmqqa6qoqw9n.fsf@gitster.g/
> * https://lore.kernel.org/git/xmqqo8f0cr9z.fsf@gitster.g/

It is sufficient to just write a single line

	Helped-by: Junio C Hamano <gitster@pobox.com>

immediately before your sign-off, I would think.

> Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>

The sign-off name and address must match the name and address of the
patch author (i.e. "Stavros Ntentos <stdedos@gmail.com>").

> +	mixedPathspecMagic::
> +		Advice shown if a user tries to mix short- and
> +		longform pathspec magic.

Good.  Here the phrase "pathspec magic" is used.

> +/*
> + * Give hint for a common mistake of mixing short and long
> + * form of pathspec magic, as well as possible corrections
> + */
> +static void warn_mixed_magic(unsigned magic, const char *elem, const char *pos)
> +{
> +	struct strbuf longform = STRBUF_INIT;
> +	int i;
> +
> +	if (!advice_enabled(ADVICE_MIXED_PATHSPEC_MAGIC))
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> +		if (pathspec_magic[i].bit & magic) {
> +			if (longform.len)
> +				strbuf_addch(&longform, ',');
> +			strbuf_addstr(&longform, pathspec_magic[i].name);
> +		}
> +	}

OK, so we are collecting the ones given by the short form so far.
For e.g. ":!(glob)", we write "exclude" in the longform buffer.  If
we had more than one, then before adding the second one, we add a
comma, so we may see "top,exclude" for ":/!(glob)".  Good.

> +	advise_if_enabled(ADVICE_MIXED_PATHSPEC_MAGIC,
> +	                  _("'%.*s(...': cannot mix short- and longform pathspec magic!\n"

Do we need to "shout!"?  I think a normal full-stop would be sufficient.

'elem' is pointing at ':', 'pos' is where we read '(' from, so
the above gives us "':/!(...': cannot..." for "':/!(glob)".  OK.

> +	                    "Either spell the shortform magic '%.*s' as ':(%s,...'\n"

Here, the shortform %.*s excludes the ':' that introduced the magic,
so we would see "shortform magic '/!' as ':(top,exclude,...'".  Good.

> +	                    "or end magic pathspec matching with '%.*s:'."),

This one I am not sure about.  Something like

    $ git add -- ":!(0) preface.txt" \*.txt

may be plausible, albeit rare, and it may be a good advice to
explicitly terminate the shortform pathspec magic before the '(' in
such a case.

But presumably it is much rarer for '(' to be a part of a pathspec
element than an attempt to introduce a longform magic, it might be
worth spending an extra line to explain in what narrow cases the
latter choice may make sense.  Here is my attempt.

    or if '(...' is indeed the beginning of a pathname, end the shortform
    magic sequence explicitly with another ':' before it, e.g. '%.*s:(...'


> +	                  (int)(pos - elem), elem,
> +	                  (int)(pos - (elem + 1)), elem + 1,
> +	                  longform.buf,
> +	                  (int)(pos - elem), elem);
> +}
>  /*
>   * Parse the pathspec element looking for short magic
>   *
> @@ -356,6 +386,9 @@ static const char *parse_short_magic(unsigned *magic, const char *elem)
>  			continue;
>  		}
>  
> +		if (ch == '(')
> +			warn_mixed_magic(*magic, elem, pos);
> +
>  		if (!is_pathspec_magic(ch))
>  			break;
>  
> diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
> index 30328b87f0..8b9d543e1e 100755
> --- a/t/t6132-pathspec-exclude.sh
> +++ b/t/t6132-pathspec-exclude.sh
> @@ -244,4 +244,42 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '
>  	test_cmp expect-grep actual-grep
>  '
>  
> +cat > expected_warn <<"EOF"
> +hint: ':!(...': cannot mix short- and longform pathspec magic!
> +hint: Either spell the shortform magic '!' as ':(exclude,...'
> +hint: or end magic pathspec matching with ':!:'.
> +hint: Disable this message with "git config advice.mixedPathspecMagic false"
> +EOF
> +test_expect_success 'warn pathspec not matching longform magic in :!(...)' '
> +	git log --oneline --format=%s -- '"'"':!(glob)**/file'"'"' >actual 2>warn &&
> +	cat <<EOF >expect &&
> +sub2/file
> +sub/sub/sub/file
> +sub/file2
> +sub/sub/file
> +sub/file
> +file
> +EOF
> +	cat actual &&
> +	cat warn &&

Are these leftover debugging aid?

> +	test_cmp expect actual &&
> +	test_cmp expected_warn warn
> +'
> +
> +test_expect_success 'do not warn pathspec not matching longform magic in :!:(...) (i.e. if magic parsing is explicitly stopped)' '
> +	git log --oneline --format=%s -- '"'"':!:(glob)**/file'"'"' >actual 2>warn &&
> +	cat <<EOF >expect &&
> +sub2/file
> +sub/sub/sub/file
> +sub/file2
> +sub/sub/file
> +sub/file
> +file
> +EOF
> +	cat actual &&
> +	cat warn &&

Are these leftover debugging aid?

> +	test_cmp expect actual &&
> +	! test_cmp expected_warn warn
> +'
> +
>  test_done


Thanks.