| Message ID | 20210328154532.23803-1-133706+stdedos@users.noreply.github.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] pathspec: advice: long and short forms are incompatible | expand |
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. > @@ -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;
Junio C Hamano <gitster@pobox.com> writes: >> +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 > ... > ':!(...': cannot mix short and long form pathspec magic > > it may be sufficiently clear where the problem is. I meant "clear where the problem is, without adding [e.g. like :(glob)]". Thanks.
> 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. I am trying. However, git-send-email keeps pulling that no-reply address, and git-send-email does not offer any `--exclude-addresses="*glob*"`-like option. > or even just > > ':!(...': cannot mix short and long form pathspec magic > > it may be sufficiently clear where the problem is. I slightly disagree, and prefer the `extra_lookahead_chars`. Just 3 characters [`:!(`] is a bit too little, and the total message sits below the "you can disable this message" hint. > The seemingly-stray '; or' at the end aside, I am not sure what this > is trying to say. See the testcase: > hint: If '(glob)*...' is a valid path, explicitly terminate magic parsing with ':'; or > hint: Disable this message with "git config advice.mixedShortLongMagicPathspec false" I am segway-ing from the "explicitly stop parsing" to the "disable this message" sentence. > 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. I am siding with the "user entered this situation by mistake", and not with the "user is explicitly trying to match a file named `:(global,icase)foo`" side. Offering a more complete message, will become more complex. I disagree with that. I can settle by offering examples (mine and yours) in the documentation. > 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 don't see any explicit code in parsing the shortform magics, except: /* Special case alias for '!' */ if (ch == '^') { *magic |= PATHSPEC_EXCLUDE; continue; } and therefore I would like to avoid such task (although I love well-written DWIMs-or-close-to-them). > > +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. I hate macros, only because I cannot expand or modify them during gdb. (Suggestions welcome! :-D) > > - 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. True, but the message was explicitly referred to with i18n code specifically targeted for such initialization. I like code doing the same job, sitting together. I'd prefer to either move both inside (since no one else will ever refer to this message either), or keep them as-is. > - 7 is way too long for warning against something like ":!(glob)", > no? GRRRRR C :-p (I'll push the changes on the next iteration; including the `like glob` removed, and whatever comes from our discussion.) > 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. Lookahead is simply to inform user what git will do with the current state of affairs, i.e.: git log --oneline --format=%s -- ':!(glob)**/file' will filter with NOT '(glob)**/file' path (truncated for brevity)
> > - 7 is way too long for warning against something like ":!(glob)", > > no? > > GRRRRR C :-p > (I'll push the changes on the next iteration; including the `like glob` > removed, and whatever comes from our discussion.) Actually ... C does not have a problem with it: root@94ae60990b85:/usr/src/git/t# ../git log -- ':!(glob)' hint: ':!(glob)...': cannot mix shortform magic with longform [e.g. like :(glob)]. hint: If '(glob)...' is a valid path, explicitly terminate magic parsing with ':'; or hint: Disable this message with "git config advice.mixedShortLongMagicPathspec false" commit 14b8580dde326a0bbf9abacac7e09dbe4c38c25e (HEAD -> fix/mixed-short-long-magic) True, it does not look nice; theoretically, any number (even 1!) could be too much to contain ellipsis after that text. Given the size of the change, I would want to say that it is acceptable to leave this as-is. Unless you'd like me to write a generic truncate_at_chars(char *text, int width, char *ellipsis_char_or_default) function.
Stavros Ntentos <stdedos@gmail.com> writes: >> 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 don't see any explicit code in parsing the shortform magics, except: > /* Special case alias for '!' */ > if (ch == '^') { > *magic |= PATHSPEC_EXCLUDE; > continue; > } > and therefore I would like to avoid such task (although I love well-written > DWIMs-or-close-to-them). I am not yet interested in doing DWIM, but a good advice is always welcome. Along the lines of the attached illustration patch, perhaps. $ git show --oneline --stat -- ':/!(glob)**/*.txt' hint: ':/!(...': cannot mix short- and longform pathspec magic hint: instead, spell the shortform magic '/!' as 'top,exclude' inside :(... Documentation/config/advice.txt | 3 +++ advice.c | 3 +++ advice.h | 2 ++ pathspec.c | 29 +++++++++++++++++++++++++++++ 4 files changed, 37 insertions(+) diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt index acbd0c09aa..05a3cbc164 100644 --- c/Documentation/config/advice.txt +++ w/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 c/advice.c w/advice.c index 164742305f..9426bc5295 100644 --- c/advice.c +++ w/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 } @@ -113,6 +115,7 @@ static struct { [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated", 1 }, [ADVICE_IGNORED_HOOK] = { "ignoredHook", 1 }, [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity", 1 }, + [ADVICE_MIXED_PATHSPEC_MAGIC] = { "mixedPathspecMagic", 1 }, [ADVICE_NESTED_TAG] = { "nestedTag", 1 }, [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning", 1 }, [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists", 1 }, diff --git c/advice.h w/advice.h index bc2432980a..d56c1896a0 100644 --- c/advice.h +++ w/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: @@ -51,6 +52,7 @@ extern int advice_add_empty_pathspec; ADVICE_GRAFT_FILE_DEPRECATED, ADVICE_IGNORED_HOOK, ADVICE_IMPLICIT_IDENTITY, + ADVICE_MIXED_PATHSPEC_MAGIC, ADVICE_NESTED_TAG, ADVICE_OBJECT_NAME_WARNING, ADVICE_PUSH_ALREADY_EXISTS, diff --git c/pathspec.c w/pathspec.c index 18b3be362a..ce9d1738a8 100644 --- c/pathspec.c +++ w/pathspec.c @@ -336,6 +336,32 @@ 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 + */ +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(_("'%.*s(...': cannot mix short- and longform pathspec magic"), + (int)(pos - elem), elem); + advise(_("instead, spell the shortform magic '%.*s' as '%s' inside :(..."), + (int)(pos - (elem + 1)), elem + 1, + longform.buf); +} + + /* * Parse the pathspec element looking for short magic * @@ -356,6 +382,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/Documentation/config/advice.txt b/Documentation/config/advice.txt index acbd0c09aa..f7ccd05c3a 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -119,4 +119,9 @@ advice.*:: addEmptyPathspec:: Advice shown if a user runs the add command without providing the pathspec parameter. + mixedShortLongMagic:: + Advice shown if a user provides a pathspec that could be + interpreted as a mixed short and long magic modifier(s) + (i.e. contains an open parenthesis `(` without explictly + terminating pathspec magic parsing with `:`). -- diff --git a/advice.c b/advice.c index 164742305f..6630c57678 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_short_long_magic_pathspec = 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 }, + { "mixedShortLongMagicPathspec", &advice_mixed_short_long_magic_pathspec }, /* 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_SHORT_LONG_MAGIC_PATHSPEC] = { "mixedShortLongMagicPathspec", 1 }, }; static const char turn_off_instructions[] = diff --git a/advice.h b/advice.h index bc2432980a..050f058274 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_short_long_magic_pathspec; /* * 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_SHORT_LONG_MAGIC_PATHSPEC, }; int git_default_advice_config(const char *var, const char *value); diff --git a/pathspec.c b/pathspec.c index 18b3be362a..085a624ad9 100644 --- a/pathspec.c +++ b/pathspec.c @@ -336,6 +336,11 @@ static const char *parse_long_magic(unsigned *magic, int *prefix_len, return pos; } +static const char mixed_pathspec_magic[] = N_( + "'%.*s...': cannot mix shortform magic with longform [e.g. like :(glob)].\n" + "If '%.*s...' is a valid path, explicitly terminate magic parsing with ':'; or"); +static int extra_lookahead_chars = 7; + /* * Parse the pathspec element looking for short magic * @@ -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; diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh index 30328b87f0..b5d51c1429 100755 --- a/t/t6132-pathspec-exclude.sh +++ b/t/t6132-pathspec-exclude.sh @@ -244,4 +244,41 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' ' test_cmp expect-grep actual-grep ' +cat > expected_warn <<"EOF" +hint: ':!(glob)*...': cannot mix shortform magic with longform [e.g. like :(glob)]. +hint: If '(glob)*...' is a valid path, explicitly terminate magic parsing with ':'; or +hint: Disable this message with "git config advice.mixedShortLongMagicPathspec 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 suggestion (in https://lore.kernel.org/git/xmqqa6qoqw9n.fsf@gitster.g/). Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com> --- Documentation/config/advice.txt | 5 +++++ advice.c | 3 +++ advice.h | 2 ++ pathspec.c | 16 ++++++++++++++ t/t6132-pathspec-exclude.sh | 37 +++++++++++++++++++++++++++++++++ 5 files changed, 63 insertions(+)