| Message ID | 20210403122604.19203-1-133706+stdedos@users.noreply.github.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] pathspec: advice: long and short forms are incompatible | expand |
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.
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 <stdedos@gmail.com>"). 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 <gitster@pobox.com> wrote: > > 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.
Σταύρος Ντέντος <stdedos@gmail.com> writes: > The author *is* Stavros Ntentos <133706+stdedos@users.noreply.github.com>; > I don't know why it is messed up. Hmph, that is unfortunate. As authorship and sign-off is a part of the mechanism we use to keep track of provenance of our codebase, we'd prefer to be talking to somebody whom we can reach when needed; the noreply address apparently is designed to be unreachable, no?
On Sun, 11 Apr 2021 at 22:10, Junio C Hamano <gitster@pobox.com> wrote: > > Hmph, that is unfortunate. As authorship and sign-off is a part of > the mechanism we use to keep track of provenance of our codebase, > we'd prefer to be talking to somebody whom we can reach when needed; > the noreply address apparently is designed to be unreachable, no? > I would like to "just" contribute, if possible. If someone *really* needs to track me down, I assume the mailing list is anyway already public, and the patchset title already will directly link to the thread. With regards, Ntentos Stavros
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index acbd0c09aa..05a3cbc164 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -119,4 +119,7 @@ advice.*:: addEmptyPathspec:: Advice shown if a user runs the add command without providing the pathspec parameter. + mixedPathspecMagic:: + Advice shown if a user tries to mix short- and + longform pathspec magic. -- diff --git a/advice.c b/advice.c index 164742305f..f94505e0d6 100644 --- a/advice.c +++ b/advice.c @@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1; int advice_submodule_alternate_error_strategy_die = 1; int advice_add_ignored_file = 1; int advice_add_empty_pathspec = 1; +int advice_mixed_pathspec_magic = 1; static int advice_use_color = -1; static char advice_colors[][COLOR_MAXLEN] = { @@ -95,6 +96,7 @@ static struct { { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die }, { "addIgnoredFile", &advice_add_ignored_file }, { "addEmptyPathspec", &advice_add_empty_pathspec }, + { "mixedPathspecMagic", &advice_mixed_pathspec_magic }, /* make this an alias for backward compatibility */ { "pushNonFastForward", &advice_push_update_rejected } @@ -137,6 +139,7 @@ static struct { [ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 }, [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 }, [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 }, + [ADVICE_MIXED_PATHSPEC_MAGIC] = { "mixedPathspecMagic", 1 }, }; static const char turn_off_instructions[] = diff --git a/advice.h b/advice.h index bc2432980a..aa229fded8 100644 --- a/advice.h +++ b/advice.h @@ -33,6 +33,7 @@ extern int advice_checkout_ambiguous_remote_branch_name; extern int advice_submodule_alternate_error_strategy_die; extern int advice_add_ignored_file; extern int advice_add_empty_pathspec; +extern int advice_mixed_pathspec_magic; /* * To add a new advice, you need to: @@ -72,6 +73,7 @@ extern int advice_add_empty_pathspec; ADVICE_STATUS_U_OPTION, ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE, ADVICE_WAITING_FOR_EDITOR, + ADVICE_MIXED_PATHSPEC_MAGIC, }; int git_default_advice_config(const char *var, const char *value); diff --git a/pathspec.c b/pathspec.c index 18b3be362a..5f77e2e8d8 100644 --- a/pathspec.c +++ b/pathspec.c @@ -336,6 +336,36 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len, return pos; } + +/* + * 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); + } + } + advise_if_enabled(ADVICE_MIXED_PATHSPEC_MAGIC, + _("'%.*s(...': cannot mix short- and longform pathspec magic!\n" + "Either spell the shortform magic '%.*s' as ':(%s,...'\n" + "or end magic pathspec matching with '%.*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 && + 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 && + test_cmp expect actual && + ! test_cmp expected_warn warn +' + test_done
It can be a "reasonable" mistake to mix short and long forms, e.g. `:!(glob)`, instead of the (correct) `:(exclude,glob)`. 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. 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/ Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com> --- Documentation/config/advice.txt | 3 +++ advice.c | 3 +++ advice.h | 2 ++ pathspec.c | 33 ++++++++++++++++++++++++++++ t/t6132-pathspec-exclude.sh | 38 +++++++++++++++++++++++++++++++++ 5 files changed, 79 insertions(+)