From: Junio C Hamano <hidden> Date: 2021-02-26 23:28:03
Σταύρος Ντέντος [off-list ref] writes:
git archive HEAD -o p.zip --prefix=p/ -- ':!(glob)**/.gitignore'
Which lead to **both of the .gitignores** to be included. And then I
was baffled.
Now I know (I guess) that the pathspec magic was `:!`, and I tried to
match against a `(glob)**/.gitignore`, i.e. `(glob)*/.gitignore`,
which, of course, it doesn't exist in any form.
You correctly guessed.
The section on pathspec in "git help glossary" may need to be
clarified so that ":!(exclude,glob)" is the right way to spell what
you meant, I think.
And I wonder:
Why doesn't `git archive ... -- ` understand `:!.gitignore` as a
.gitignore file would do (minus the `:`)?
At this point, asking "why" is fruitless. They are spelled
differently, because pathspecs and .gitignore patterns are simply
different.
Is there some reason that `git archive ... -- ` doesn't understand
`:!(glob)**/.gitignore`? It doesn't sound awfully complicated, or
risking a lot of regression (a folder starting with `(`, and
containing valid pathspec long forms IMHO is rare),
It was probably OK back in 2006 when the number of users and
projects that are using Git were smaller, but these days, whatever
anybody would think "rare" are used by somebody and it would hurt
many people (in absolute terms---they may be a tiny minority of the
population) if you make a backward incompatible change that breaks
these "rare" cases.
Is there some reason that `git archive ... -- ` doesn't warn me that
`:!(glob)**/.gitignore` is invalid (and maybe I meant
`(exclude,glob)`)?
This one does have merit. Patches are very much welcomed.
Thanks.
From: Stavros Ntentos <redacted>
This is my first contribution to the git project.
It is the first of the second takes that came from the
https://lore.kernel.org/git/xmqqft1iquka.fsf@gitster.g/ e-mail
(hopefully I linked it correctly using the git-magic commands).
For the review, I pulled in the latest _meaningful_ contributer,
as well as one big contributer to the file.
hopefully, all is in order and correctly done.
Please be gentle with "protocol violations". :-D
Stavros Ntentos (1):
pathspec: warn for a no-glob entry that contains `**`
pathspec.c | 13 +++++++++++++
pathspec.h | 1 +
t/t6130-pathspec-noglob.sh | 13 +++++++++++++
3 files changed, 27 insertions(+)
--
2.31.0
From: Stavros Ntentos <redacted>
If a pathspec is given that contains `**`, chances are that someone is
naively expecting that it will do what the manual has told him that `**`
will match (i.e. 0-or-more directories).
However, without an explicit `:(glob)` magic, that will fall out the sky:
the two `**` will merge into one star, which surrounded by slashes, will
match any directory name.
These changes attempt to bring awareness to this issue.
Signed-off-by: Stavros Ntentos <redacted>
---
pathspec.c | 13 +++++++++++++
pathspec.h | 1 +
t/t6130-pathspec-noglob.sh | 13 +++++++++++++
3 files changed, 27 insertions(+)
@@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,return1;}++voidcheck_missing_glob(constchar*entry,intflags){+if(flags&(PATHSPEC_GLOB|PATHSPEC_LITERAL)){+return;+}++if(strstr(entry,"**")){+warning(_("Pathspec provided contains `**`, but no :(glob) magic.\n\tIt will not match 0 or more directories!"));+}+}
@@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' 'test_must_be_emptyactual'+cat>expected<<"EOF"+warning:Pathspecprovidedcontains`**`,butnoglobmagic.+EOF+test_expect_success'** without :(glob) warns of lacking glob magic''+test_might_failgitstash--"**/bar"2>warns&&+grep-Ffexpectedwarns+'++test_expect_success'** with :(literal) does not warn of lacking glob magic''+test_might_failgitstash--":(literal)**/bar"2>warns&&+!grep-Ffexpectedwarns+'+ test_done
From: Stavros Ntentos <redacted>
If a pathspec is given that contains `**`, chances are that someone is
naively expecting that it will do what the manual has told him that `**`
will match (i.e. 0-or-more directories).
However, without an explicit `:(glob)` magic, that will fall out the sky:
the two `**` will merge into one star, which surrounded by slashes, will
match any directory name.
These changes attempt to bring awareness to this issue.
Signed-off-by: Stavros Ntentos <redacted>
---
pathspec.c | 13 +++++++++++++
pathspec.h | 1 +
t/t6130-pathspec-noglob.sh | 13 +++++++++++++
3 files changed, 27 insertions(+)
@@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,return1;}++voidcheck_missing_glob(constchar*entry,intflags){+if(flags&(PATHSPEC_GLOB|PATHSPEC_LITERAL)){+return;+}++if(strstr(entry,"**")){+warning(_("Pathspec provided contains `**`, but no :(glob) magic.\n\tIt will not match 0 or more directories!"));+}
Why did you add an extra \t? I think it is unnecessary indentation.
@@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' 'test_must_be_emptyactual'+cat>expected<<"EOF"+warning:Pathspecprovidedcontains`**`,butnoglobmagic.+EOF+test_expect_success'** without :(glob) warns of lacking glob magic''+test_might_failgitstash--"**/bar"2>warns&&+grep-Ffexpectedwarns+'++test_expect_success'** with :(literal) does not warn of lacking glob magic''
From: Stavros Ntentos <redacted>
If a pathspec is given that contains `**`, chances are that someone is
naively expecting that it will do what the manual has told him that `**`
will match (i.e. 0-or-more directories).
However, without an explicit `:(glob)` magic, that will fall out the sky:
the two `**` will merge into one star, which surrounded by slashes, will
match any directory name.
These changes attempt to bring awareness to this issue.
Signed-off-by: Stavros Ntentos <redacted>
---
pathspec.c | 13 +++++++++++++
pathspec.h | 1 +
t/t6130-pathspec-noglob.sh | 13 +++++++++++++
3 files changed, 27 insertions(+)
@@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,return1;}++voidcheck_missing_glob(constchar*entry,intflags){+if(flags&(PATHSPEC_GLOB|PATHSPEC_LITERAL)){+return;+}++if(strstr(entry,"**")){+warning(_("Pathspec provided contains `**`, but no :(glob) magic.\n\tIt will not match 0 or more directories!"));+}
@@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' 'test_must_be_emptyactual'+cat>expected<<"EOF"+warning:Pathspecprovidedcontains`**`,butnoglobmagic.+EOF+test_expect_success'** without :(glob) warns of lacking glob magic''+test_might_failgitstash--"**/bar"2>warns&&+grep-Ffexpectedwarns+'++test_expect_success'** with :(literal) does not warn of lacking glob magic''
+ if (strstr(entry, "**")) {
+ warning(_("Pathspec provided contains `**`, but no :(glob) magic.\n\tIt will not match 0 or more directories!"));
+ }
Why an extra \t? Unnecessary indentation?
It brings out the warning label:
root@30f6bde171fe:/usr/src/git/t# ../git stash -- **/bar
warning: Pathspec provided contains `**`, but no :(glob) magic.
It will not match 0 or more directories!
error: pathspec ':(,prefix:2)t/**/bar' did not match any file(s) known to git
Did you forget to 'git add'?
and "makes the user feel" that these two lines are in reality one
(but this is way over any sensible line limit to present as such).
I would've padded exactly, but:
* `\t` expansion is terminal-specific (usually set at 8, but not guaranteed)
* ` `-only would've been too long of a padding to an already long line
Ofc, if someone really wanted to solve this, someone
could rework the `void vreportf` split and auto-pad
prefix at newline, but sounds like a project on its own ...
quoted
+test_expect_success '** with :(literal) does not warn of lacking glob magic' '
Padding with without test above?
Yes; it somewhat aligns individual test output:
root@30f6bde171fe:/usr/src/git/t# ./t6130-pathspec-noglob.sh
...
ok 22 - ** without :(glob) warns of lacking glob magic
ok 23 - ** with :(literal) does not warn of lacking glob magic
# passed all 23 test(s)
to emphasize they are a positive/negative test pair.
I don't know how well I feel about this anyway; I can undo it.
From: Stavros Ntentos <redacted>
If a pathspec given that contains `**`, chances are that someone is
naively expecting that it will do what the manual has told him
(i.e. that `**` will match 0-or-more directories).
However, without an explicit `:(glob)` magic, that will fall out the sky:
the two `**` will merge into one star, which surrounded by slashes, will
match any directory name.
These changes attempt to bring awareness to this issue.
Signed-off-by: Stavros Ntentos <redacted>
---
pathspec.c | 13 +++++++++++++
pathspec.h | 1 +
t/t6130-pathspec-noglob.sh | 13 +++++++++++++
3 files changed, 27 insertions(+)
@@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,return1;}++voidcheck_missing_glob(constchar*pathspec_entry,intflags){+if(flags&(PATHSPEC_GLOB|PATHSPEC_LITERAL)){+return;+}++if(strstr(pathspec_entry,"**")){+warning(_("Pathspec provided contains `**`, but no :(glob) magic.\nIt will not match 0 or more directories!"));+}+}
@@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' 'test_must_be_emptyactual'+cat>expected<<"EOF"+warning:Pathspecprovidedcontains`**`,butno:(glob)magic.+EOF+test_expect_success'** without :(glob) warns of lacking glob magic''+test_might_failgitstash--"**/bar"2>warns&&+grep-Ffexpectedwarns+'++test_expect_success'** with :(literal) does not warn of lacking glob magic''+test_might_failgitstash--":(literal)**/bar"2>warns&&+!grep-Ffexpectedwarns+'+ test_done
From: Stavros Ntentos <redacted>
The second of the two issues identified in this thread.
Both patches work as expected.
There is a short solution, which is mostly okay.
It does not compile with -Werror, but:
It avoids malloc/free, and keeps the logic/sloc low.
All of this could be my lack of C experience.
Stavros Ntentos (1):
pathspec: warn: long and short forms are incompatible
pathspec.c | 30 ++++++++++++++++++++++++++++++
pathspec.h | 1 +
t/t6132-pathspec-exclude.sh | 33 +++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+)
--
2.31.0
@@ -745,16 +746,20 @@ int match_pathspec_attrs(const struct index_state *istate,}voidcheck_mishandled_exclude(constchar*entry){+char*flags,*path;size_tentry_len=strlen(entry);-charflags[entry_len];-charpath[entry_len];-if(sscanf(entry,":!(%4096[^)])%4096s",&flags,&path)!=2){-return;-}-if(count_slashes(flags)>0){-return;+flags=xstrdup(entry);+memset(flags,'\0',entry_len);+path=xstrdup(entry);+memset(path,'\0',entry_len);++if(sscanf(entry,":!(%4096[^)])%4096s",flags,path)==2){+if(count_slashes(flags)==0){+warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));+}}-warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));+FREE_AND_NULL(flags);+FREE_AND_NULL(path);}
From: Stavros Ntentos <redacted>
Namely, `!` and any other long magic form (e.g. `glob`)
cannot be combined to one entry.
Issue a warning when such thing happens, and hint to the solution.
Signed-off-by: Stavros Ntentos <redacted>
---
pathspec.c | 19 +++++++++++++++++++
pathspec.h | 1 +
t/t6132-pathspec-exclude.sh | 33 +++++++++++++++++++++++++++++++++
3 files changed, 53 insertions(+)
@@ -739,3 +743,18 @@ int match_pathspec_attrs(const struct index_state *istate,return1;}++voidcheck_mishandled_exclude(constchar*entry){+size_tentry_len=strlen(entry);+charflags[entry_len];+charpath[entry_len];++if(sscanf(entry,":!(%4096[^)])%4096s",&flags,&path)!=2){+return;+}+if(count_slashes(flags)>0){+return;+}++warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));+}
@@ -244,4 +244,37 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' 'test_cmpexpect-grepactual-grep'+cat>expected_warn<<"EOF"+Pathspecprovidedmatches`:!(...)`+EOF+test_expect_success'warn pathspec :!(...) skips the parenthesized magics''+gitlog--oneline--format=%s--'"'"':!(glob)**/file'"'"'>actual2>warn&&+cat<<EOF>expect&&+sub2/file+sub/sub/sub/file+sub/file2+sub/sub/file+sub/file+file+EOF+catactual&&+catwarn&&+test_cmpexpectactual&&+grep-Ffexpected_warnwarn+'++test_expect_success'do not warn that pathspec :!(...) skips the parenthesized magics (if parenthesis would not be part of the magic)''+gitlog--oneline--format=%s--'"'"':!(gl/ob)/file'"'"'>actual2>warn&&+cat<<EOF>expect&&+sub2/file+sub/sub/sub/file+sub/file2+sub/sub/file+sub/file+file+EOF+test_cmpexpectactual&&+!grep-Ffexpected_warnwarn+'+ test_done
From: Stavros Ntentos <redacted>
The second of the two issues identified in this thread.
Both patches work as expected.
There is a short solution, which is mostly okay.
It does not compile with -Werror, but:
It avoids malloc/free, and keeps the logic/sloc low.
All of this could be my lack of C experience.
Stavros Ntentos (1):
pathspec: warn: long and short forms are incompatible
pathspec.c | 30 ++++++++++++++++++++++++++++++
pathspec.h | 1 +
t/t6132-pathspec-exclude.sh | 33 +++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+)
--
2.31.0
From: Stavros Ntentos <redacted>
Namely, `!` and any other long magic form (e.g. `glob`)
cannot be combined to one entry.
Issue a warning when such thing happens, and hint to the solution.
Signed-off-by: Stavros Ntentos <redacted>
---
pathspec.c | 24 ++++++++++++++++++++++++
pathspec.h | 1 +
t/t6132-pathspec-exclude.sh | 33 +++++++++++++++++++++++++++++++++
3 files changed, 58 insertions(+)
@@ -739,3 +744,22 @@ int match_pathspec_attrs(const struct index_state *istate,return1;}++voidcheck_mishandled_exclude(constchar*entry){+char*flags,*path;+size_tentry_len=strlen(entry);++flags=xstrdup(entry);+memset(flags,'\0',entry_len);+path=xstrdup(entry);+memset(path,'\0',entry_len);++if(sscanf(entry,":!(%4096[^)])%4096s",flags,path)==2){+if(count_slashes(flags)==0){+warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));+}+}++FREE_AND_NULL(flags);+FREE_AND_NULL(path);+}
@@ -244,4 +244,37 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' 'test_cmpexpect-grepactual-grep'+cat>expected_warn<<"EOF"+Pathspecprovidedmatches`:!(...)`+EOF+test_expect_success'warn pathspec :!(...) skips the parenthesized magics''+gitlog--oneline--format=%s--'"'"':!(glob)**/file'"'"'>actual2>warn&&+cat<<EOF>expect&&+sub2/file+sub/sub/sub/file+sub/file2+sub/sub/file+sub/file+file+EOF+catactual&&+catwarn&&+test_cmpexpectactual&&+grep-Ffexpected_warnwarn+'++test_expect_success'do not warn that pathspec :!(...) skips the parenthesized magics (if parenthesis would not be part of the magic)''+gitlog--oneline--format=%s--'"'"':!(gl/ob)/file'"'"'>actual2>warn&&+cat<<EOF>expect&&+sub2/file+sub/sub/sub/file+sub/file2+sub/sub/file+sub/file+file+EOF+test_cmpexpectactual&&+!grep-Ffexpected_warnwarn+'+ test_done
We avoid using variable-length arrays in our codebase. For one thing,
they were not historically supported by all platforms (we are slowly
using more C99 features, but we are introducing them slowly and
intentionally).
But two, they are limited in size and the failure mode is not graceful.
If "entry" is larger than the available stack, then we'll get a segfault
with no option to handle it better.
We also generally avoid using scanf, because it's error-prone. The
"4096" is scary here, but I don't _think_ it's a buffer overflow,
because "path" is already the same size as "entry" (not including the
NUL terminator, but that is negated by the fact that we'll have skipped
at least ":!").
Is this "%4096[^)]" actually valid? I don't think scanf understands
regular expressions.
We'd want to avoid making an extra copy of the string anyway. So you'd
probably want to just parse left-to-right in the original string, like:
const char *p = entry;
/* skip past stuff we know must be there */
if (!skip_prefix(p, ":!(", &p))
return;
/* this checks count_slashes() > 0 in the flags section, though I'm
* not sure I understand what that is looking for... */
for (; *p && *p != ')'; p++) {
if (*p == '/')
return;
}
if (*p++ != ')')
return;
/* now p is pointing at "path", though we don't seem to do anything
* with it... */
-Peff
@@ -745,16 +746,20 @@ int match_pathspec_attrs(const struct index_state *istate,}voidcheck_mishandled_exclude(constchar*entry){+char*flags,*path;size_tentry_len=strlen(entry);-charflags[entry_len];-charpath[entry_len];-if(sscanf(entry,":!(%4096[^)])%4096s",&flags,&path)!=2){-return;-}-if(count_slashes(flags)>0){-return;+flags=xstrdup(entry);+memset(flags,'\0',entry_len);+path=xstrdup(entry);+memset(path,'\0',entry_len);++if(sscanf(entry,":!(%4096[^)])%4096s",flags,path)==2){+if(count_slashes(flags)==0){+warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));+}}-warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));
Message looks ok.
+ FREE_AND_NULL(flags);
+ FREE_AND_NULL(path);
}
--
An old man doll... just what I always wanted! - Clara
If a pathspec given that contains `**`, chances are that someone is
naively expecting that it will do what the manual has told him
(i.e. that `**` will match 0-or-more directories).
When `**` appears in the pathspec, the user may be expecting that
it would be matched using the "wildmatch" semantics,
matching 0 or more directories.
That is not what happens without ":(glob)" magic.
Teach the pathspec parser to emit an advice message when a substring
`**` appears in a pathspec element that does not have a `:(glob)` magic.
Make sure we don't disturb users who use ":(literal)" magic
with such a substring, as it is clear they want to find these strings literally.
Signed-off-by: Stavros Ntentos <redacted>
---
pathspec.c | 13 +++++++++++++
pathspec.h | 1 +
t/t6130-pathspec-noglob.sh | 13 +++++++++++++
3 files changed, 27 insertions(+)
@@ -739,3 +742,13 @@ int match_pathspec_attrs(const struct index_state *istate,return1;}++voidcheck_missing_glob(constchar*pathspec_entry,intflags){+if(flags&(PATHSPEC_GLOB|PATHSPEC_LITERAL)){+return;+}++if(strstr(pathspec_entry,"**")){+warning(_("Pathspec provided contains `**`, but no :(glob) magic.\nIt will not match 0 or more directories!"));+}+}
@@ -157,4 +157,17 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' 'test_must_be_emptyactual'+cat>expected<<"EOF"+warning:Pathspecprovidedcontains`**`,butno:(glob)magic.+EOF+test_expect_success'** without :(glob) warns of lacking glob magic''+test_might_failgitstash--"**/bar"2>warns&&+grep-Ffexpectedwarns+'++test_expect_success'** with :(literal) does not warn of lacking glob magic''+test_might_failgitstash--":(literal)**/bar"2>warns&&+!grep-Ffexpectedwarns+'+ test_done
@@ -119,4 +119,8 @@ advice.*:: addEmptyPathspec:: Advice shown if a user runs the add command without providing the pathspec parameter.+ starStarNoGlobPathspec::+ Advice shown if a user provides a pathspec via the terminal+ that contains `**`, anticipating that the pattern will be+ matched using wildmatch (aka `:(glob)` magic). --
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;intadvice_submodule_alternate_error_strategy_die=1;intadvice_add_ignored_file=1;intadvice_add_empty_pathspec=1;+intadvice_star_star_no_glob_pathspec=1;staticintadvice_use_color=-1;staticcharadvice_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},+{"starStarNoGlobPathspec",&advice_star_star_no_glob_pathspec},/* make this an alias for backward compatibility */{"pushNonFastForward",&advice_push_update_rejected}
@@ -744,11 +744,13 @@ int match_pathspec_attrs(const struct index_state *istate,}voidcheck_missing_glob(constchar*pathspec_entry,intflags){+constchar*advice=NULL;if(flags&(PATHSPEC_GLOB|PATHSPEC_LITERAL)){return;}+advice=_("Pathspec provided contains `**`, but no :(glob) magic.\nIt will not match 0 or more directories!");if(strstr(pathspec_entry,"**")){-warning(_("Pathspec provided contains `**`, but no :(glob) magic.\nIt will not match 0 or more directories!"));+advise_if_enabled(ADVICE_STAR_STAR_NO_GLOB_PATHSPEC,advice);}}
@@ -158,7 +158,9 @@ test_expect_success '**/ does not work with :(literal) and --glob-pathspecs' '' cat>expected<<"EOF"-warning:Pathspecprovidedcontains`**`,butno:(glob)magic.+hint:Pathspecprovidedcontains`**`,butno:(glob)magic.+hint:Itwillnotmatch0ormoredirectories!+hint:Disablethismessagewith"git config advice.starStarNoGlobPathspec false" EOF test_expect_success'** without :(glob) warns of lacking glob magic''test_might_failgitstash--"**/bar"2>warns&&
We avoid using variable-length arrays in our codebase. ...
Hear hear, however, I wanted to avoid the "small mess"
that allocate/free would cause (one or more of ++sloc, labels, if-nesting); ...
But two, they are limited in size and the failure mode is not graceful. ...
... however, my main issue is that - I don't know what's a sane allocation size.
... The "4096" is scary here ...
While scary, it is "a safe" upper high.
The first time the string ends up in pathspec.c for processing it's here:
entry = argv[i];
which comes from here
parse_pathspec(pathspec, magic_mask, flags, prefix, parsed_file.v);
and I don't know what's the maximum size of `parsed_file.v[0]`
Is this "%4096[^)]" actually valid? I don't think scanf understands
regular expressions.
... though I'm not sure I understand what that is looking for ...
I think it will help you see what I am trying to achieve if you read at the warning message / testcase
https://lore.kernel.org/git/20210326024005.26962-2-stdedos+git@gmail.com/#iZ30t:t6132-pathspec-exclude.sh
And, to clean up the testcase:
git log --oneline --format=%s -- ':!(glob)**/file'
I guess it should be now obvious what am I targetting:
If someone naively mixes short and long pathspec magics (`!`, and `(glob)`),
short form takes precedence and ignores long magic / assumes long magic as part of path.
(If it's not obvious, all the more reason to include such warning)
From: Jeff King <hidden> Date: 2021-03-27 09:45:24
On Fri, Mar 26, 2021 at 06:16:26PM +0200, Stavros Ntentos wrote:
quoted
We avoid using variable-length arrays in our codebase. ...
Hear hear, however, I wanted to avoid the "small mess"
that allocate/free would cause (one or more of ++sloc, labels, if-nesting); ...
quoted
But two, they are limited in size and the failure mode is not graceful. ...
... however, my main issue is that - I don't know what's a sane allocation size.
I don't think a VLA gets you out of knowing the allocation size. Either
way, you are using strlen(entry).
But I do think avoiding allocating altogether is better (as I showed in
my previous response).
quoted
... The "4096" is scary here ...
While scary, it is "a safe" upper high.
The first time the string ends up in pathspec.c for processing it's here:
entry = argv[i];
which comes from here
parse_pathspec(pathspec, magic_mask, flags, prefix, parsed_file.v);
and I don't know what's the maximum size of `parsed_file.v[0]`
There is no reasonable maximum size you can assume. Using 4096 is most
definitely not a safe upper bound.
However, as I said, I don't think it is doing anything useful in the
first place. You have sized the destination buffers as large as the
original string, so they must be large enough to hold any subset of the
original. Dropping them would be equally correct, but less distracting
to a reader.
quoted
Is this "%4096[^)]" actually valid? I don't think scanf understands
regular expressions.
Thanks, this is a corner of scanf I haven't looked at (mostly because
again, we generally avoid scanf in our code base entirely).
I think it will help you see what I am trying to achieve if you read at the warning message / testcase
https://lore.kernel.org/git/20210326024005.26962-2-stdedos+git@gmail.com/#iZ30t:t6132-pathspec-exclude.sh
And, to clean up the testcase:
git log --oneline --format=%s -- ':!(glob)**/file'
I guess it should be now obvious what am I targetting:
If someone naively mixes short and long pathspec magics (`!`, and `(glob)`),
short form takes precedence and ignores long magic / assumes long magic as part of path.
(If it's not obvious, all the more reason to include such warning)
I understand the overall goal. I am not sure why slashes in the flags
section are a reliable indicator that this mixing is not happening and
we should not show the warning.
It also feels like any checks like this should be relying on the
existing pathspec-magic parser a bit more. I don't know the pathspec
code that well, but surely at some point it has a notion of which parts
are magic flags (e.g., after parse_element_magic in init_pathspec_item).
-Peff
Attempt to force parsing long magic values to detect if
there is actually long magic present or not.
Signed-off-by: Stavros Ntentos <redacted>
---
pathspec.c | 35 +++++++++++++++++++----------------
pathspec.h | 2 +-
t/t6132-pathspec-exclude.sh | 4 ++--
3 files changed, 22 insertions(+), 19 deletions(-)
@@ -745,21 +745,24 @@ int match_pathspec_attrs(const struct index_state *istate,return1;}-voidcheck_mishandled_exclude(constchar*entry){-char*flags,*path;-size_tentry_len=strlen(entry);+voidcheck_mixed_short_and_long_magic(constchar*entry){+constchar*parsed_magic;-flags=xstrdup(entry);-memset(flags,'\0',entry_len);-path=xstrdup(entry);-memset(path,'\0',entry_len);+/* skip past stuff we know must be there */+if(!skip_prefix(entry,":",&entry)){+return;+}++/* Throwaway allocations */+unsignedmagic=0;+intprefix_len=-1;+structpathspec_item*item;+item=xmallocz(sizeof(&item));-if(sscanf(entry,":!(%4096[^)])%4096s",flags,path)==2){-if(count_slashes(flags)==0){-warning(_("Pathspec provided matches `:!(...)`\n\tDid you mean `:(exclude,...)`?"));-}+parsed_magic=parse_long_magic(&magic,&prefix_len,item,entry);+if(entry!=parsed_magic){+warning(_("Pathspec provided matches both short and long forms.\nShort forms take presedence over long forms!"));}-FREE_AND_NULL(flags);-FREE_AND_NULL(path);+FREE_AND_NULL(item);}
@@ -263,7 +263,7 @@ EOFgrep-Ffexpected_warnwarn'-test_expect_success'do not warn that pathspec :!(...) skips the parenthesized magics (if parenthesis would not be part of the magic)''+test_expect_success'do not warn that pathspec :!(...) skips the parenthesized magics (if parenthesized text would not be magic)''gitlog--oneline--format=%s--'"'"':!(gl/ob)/file'"'"'>actual2>warn&&cat<<EOF>expect&& sub2/file
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 [off-list ref]"'s code suggestion
(in https://lore.kernel.org/git/xmqqa6qoqw9n.fsf@gitster.g/).
Signed-off-by: Stavros Ntentos <redacted>
---
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(+)
@@ -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 `:`). --
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;intadvice_submodule_alternate_error_strategy_die=1;intadvice_add_ignored_file=1;intadvice_add_empty_pathspec=1;+intadvice_mixed_short_long_magic_pathspec=1;staticintadvice_use_color=-1;staticcharadvice_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}
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 [off-list ref]"'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 <redacted>
---
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(+)
@@ -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. --
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;intadvice_submodule_alternate_error_strategy_die=1;intadvice_add_ignored_file=1;intadvice_add_empty_pathspec=1;+intadvice_mixed_pathspec_magic=1;staticintadvice_use_color=-1;staticcharadvice_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}
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 [off-list ref]"'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 <redacted>
---
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(+)
@@ -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. --
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;intadvice_submodule_alternate_error_strategy_die=1;intadvice_add_ignored_file=1;intadvice_add_empty_pathspec=1;+intadvice_mixed_pathspec_magic=1;staticintadvice_use_color=-1;staticcharadvice_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}
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 [off-list ref]"'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 <redacted>
---
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(+)
@@ -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. --
@@ -33,6 +33,7 @@ int advice_checkout_ambiguous_remote_branch_name = 1;intadvice_submodule_alternate_error_strategy_die=1;intadvice_add_ignored_file=1;intadvice_add_empty_pathspec=1;+intadvice_mixed_pathspec_magic=1;staticintadvice_use_color=-1;staticcharadvice_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}
From: Junio C Hamano <hidden> Date: 2021-04-04 07:20:01
Stavros Ntentos [off-list ref] 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.
It is sufficient to just write a single line
Helped-by: Junio C Hamano [off-list ref]
immediately before your sign-off, I would think.
Signed-off-by: Stavros Ntentos <redacted>
The sign-off name and address must match the name and address of the
patch author (i.e. "Stavros Ntentos [off-list ref]").
+ 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.
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:(...'
quoted hunk
+ (int)(pos - elem), elem,
+ (int)(pos - (elem + 1)), elem + 1,
+ longform.buf,
+ (int)(pos - elem), elem);
+}
/*
* Parse the pathspec element looking for short magic
*
The sign-off name and address must match the name and address of the
patch author (i.e. "Stavros Ntentos [off-list ref]").
The author *is* Stavros Ntentos [off-list ref];
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 [off-list ref] wrote:
Stavros Ntentos [off-list ref] writes:
quoted
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.
quoted
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.
It is sufficient to just write a single line
Helped-by: Junio C Hamano [off-list ref]
immediately before your sign-off, I would think.
quoted
Signed-off-by: Stavros Ntentos <redacted>
The sign-off name and address must match the name and address of the
patch author (i.e. "Stavros Ntentos [off-list ref]").
quoted
+ mixedPathspecMagic::
+ Advice shown if a user tries to mix short- and
+ longform pathspec magic.
Good. Here the phrase "pathspec magic" is used.
quoted
+/*
+ * 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.
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.
quoted
+ "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.
quoted
+ "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:(...'
quoted
+ (int)(pos - elem), elem,
+ (int)(pos - (elem + 1)), elem + 1,
+ longform.buf,
+ (int)(pos - elem), elem);
+}
/*
* Parse the pathspec element looking for short magic
*