Hello there, Implemented the last fixes > > 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 "). The author *is* Stavros Ntentos <133706+stdedos@users.noreply.github.com>; I don't know why it is messed up. Maybe if I send the patch as an attachment instead. With regards, Ntentos Stavros On Sun, 4 Apr 2021 at 10:19, Junio C Hamano wrote: > > Stavros Ntentos 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 "'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 > > 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 "). > > > + 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 <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 <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.