git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Apr 4, 2021, 07:19 UTC
Message-ID
<xmqqr1jq7bzc.fsf@gitster.g>
In-Reply-To
<20210403122604.19203-1-133706+stdedos@users.noreply.github.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.
Show 5 quoted lines
> 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.
Show 19 quoted lines
> +/*
> + * 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:(...'
Show 44 quoted lines
> +	                  (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?
Show 16 quoted lines
> +	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?
Show 5 quoted lines
> +	test_cmp expect actual &&
> +	! test_cmp expected_warn warn
> +'
> +
>  test_done
Thanks.
Previous: Stavros NtentosNext: Σταύρος Ντέντος
Message 32 of 43 in “Pathspec does not accept / does not warn for specific pathspecs”
  1. Σταύρος ΝτέντοςFeb 26, 2021
  2. Junio C HamanoFeb 26, 2021
  3. 0/1 pathspec: warn for a no-glob entry that contains `**`Σταύρος Ντέντος, Mar 25, 2021
  4. 1/1 pathspec: warn for a no-glob entry that contains `**`Σταύρος Ντέντος, Mar 25, 2021
  5. Bagas SanjayaMar 25, 2021
  6. Bagas SanjayaMar 25, 2021
  7. 1/1 pathspec: warn for a no-glob entry that contains `**`Stavros Ntentos, Mar 25, 2021
  8. Junio C HamanoMar 25, 2021
  9. 0/1 pathspec: warn for a no-glob entry that contains `**`Σταύρος Ντέντος, Mar 25, 2021
  10. 1/1 pathspec: warn for a no-glob entry that contains `**`Σταύρος Ντέντος, Mar 25, 2021
  11. Junio C HamanoMar 26, 2021
  12. Eric SunshineMar 26, 2021
  13. Junio C HamanoMar 26, 2021
  14. Jeff KingMar 26, 2021
  15. Stavros NtentosMar 26, 2021
  16. 1/2 pathspec: warn for a no-glob entry that contains `**`Stavros Ntentos, Mar 26, 2021
  17. 2/2 pathspec: convert no-glob warn to adviceStavros Ntentos, Mar 26, 2021
  18. 0/1 pathspec: warn: long and short forms are incompatibleΣταύρος Ντέντος, Mar 26, 2021
  19. 2/2 fixup! pathspec: warn: long and short forms are incompatibleΣταύρος Ντέντος, Mar 26, 2021
  20. Bagas SanjayaMar 26, 2021
  21. Stavros NtentosMar 26, 2021
  22. 3/3 squash! fixup! pathspec: warn: long and short forms are incompatibleStavros Ntentos, Mar 28, 2021
  23. 1/2 pathspec: warn: long and short forms are incompatibleΣταύρος Ντέντος, Mar 26, 2021
  24. Jeff KingMar 26, 2021
  25. Stavros NtentosMar 26, 2021
  26. Jeff KingMar 27, 2021
  27. Junio C HamanoMar 27, 2021
  28. Stavros NtentosMar 28, 2021
  29. 0/1 pathspec: warn: long and short forms are incompatibleΣταύρος Ντέντος, Mar 26, 2021
  30. pathspec: warn: long and short forms are incompatibleΣταύρος Ντέντος, Mar 26, 2021
  31. pathspec: advice: long and short forms are incompatibleStavros Ntentos, Apr 3, 2021
  32. Junio C HamanoApr 4, 2021
  33. Σταύρος ΝτέντοςApr 11, 2021
  34. Junio C HamanoApr 11, 2021
  35. Σταύρος ΝτέντοςApr 11, 2021
  36. pathspec: advice: long and short forms are incompatibleStavros Ntentos, Mar 28, 2021
  37. Junio C HamanoMar 28, 2021
  38. Junio C HamanoMar 30, 2021
  39. Stavros NtentosMar 30, 2021
  40. Stavros NtentosMar 30, 2021
  41. Junio C HamanoMar 30, 2021
  42. pathspec: advice: long and short forms are incompatibleStavros Ntentos, Apr 3, 2021
  43. pathspec: advice: long and short forms are incompatibleStavros Ntentos, Apr 3, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.