From: Junio C Hamano <hidden> Date: 2016-06-15 22:56:29
Jeff King [off-list ref] writes:
if (pathlen && pathname[pathlen-1] == '/')
pathlen--;
would work. But it seems that match_basename, despite taking the length
of all of the strings we pass it, will happily use NUL-terminated
functions like strcmp or fnmatch. Converting the former to check lengths
should be pretty straightforward. But there is no version of fnmatch
that does what we want. I wonder if we using wildmatch can get around
this limitation.
Or save away pathname[pathlen], temporarily NUL terminate and call
these functions?
From: Jeff King <hidden> Date: 2016-06-15 22:56:29
On Fri, Mar 22, 2013 at 04:08:08PM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
if (pathlen && pathname[pathlen-1] == '/')
pathlen--;
would work. But it seems that match_basename, despite taking the length
of all of the strings we pass it, will happily use NUL-terminated
functions like strcmp or fnmatch. Converting the former to check lengths
should be pretty straightforward. But there is no version of fnmatch
that does what we want. I wonder if we using wildmatch can get around
this limitation.
Or save away pathname[pathlen], temporarily NUL terminate and call
these functions?
Yeah, that is a possibility, though it involves casting away some
constness. Patch is below, which seems to work.
It still feels really ugly to me, and like match_basename is misdesigned
and should respect the lengths we pass it. Also, if it does respect the
lengths, it should be able to go much faster (e.g., in the common case,
we can drop a ton of strcmp_icase calls if we just check the lengths
beforehand). I feel like Duy was working on something like this
recently, but I don't see anything in pu.
---
From: Junio C Hamano <hidden> Date: 2016-06-15 22:56:31
So here is an attempt to fix the unintended regression, on top of
9db9eecfe5c2 (attr: avoid calling find_basename() twice per path,
2013-01-16). It consists of four patches.
The first patch is not essential to the fix, but I think it
clarifies what is going on in this codepath.
The second patch addresses the issue Jeff noticed; it appears as if
match_basename() takes counted strings, but one of the strings was
not a counted string at all. Its length was given to the function
because the caller already had one, so that we do not have to do
strlen() ourselves. And the other one was meant to be a counted
string, but the callee was not using it as such. The patch makes
them both counted strings and treat them as such.
The third patch is the main fix. As I said in the log message, I
didn't look at it very carefully, so extra sets of eyeballs are very
much appreciated.
The last one is a test stolen from Jeff to seal the series. It
needs sign-off from Jeff.
Jeff King (1):
make sure a pattern without trailing slash matches a directory
Junio C Hamano (3):
attr.c::path_matches(): the basename is part of the pathname
dir.c::match_basename(): pay attention to the length of string
parameters
attr.c::path_matches(): special case paths that end with a slash
attr.c | 23 ++++++++++++-----------
dir.c | 31 +++++++++++++++++++++++++++----
t/t5002-archive-attr-pattern.sh | 6 ++++++
3 files changed, 45 insertions(+), 15 deletions(-)
--
1.8.2-350-g3df87a1
From: Junio C Hamano <hidden> Date: 2016-06-15 22:56:31
The function takes two strings (pathname and basename) as if they
are independent strings, but in reality, the latter is always
pointing into a substring in the former.
Clarify this relationship by expressing the latter as an offset into
the former.
Signed-off-by: Junio C Hamano <redacted>
---
attr.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:56:31
The function is given a string that ends with a slash to signal that
the path is a directory to make sure that a pattern that ends with a
slash (i.e. MUSTBEDIR) can tell directories and non-directories
apart. However, the pattern itself (pat->pattern and
pat->patternlen) that came from such a MUSTBEDIR pattern is
represented as a string that ends with a slash, but patternlen does
not count that trailing slash. A MUSTBEDIR pattern "element/" is
represented as a counted string <"element/", 7> and this must match
match pathname "element/".
Because match_basename() wants to see pathname "element" to match
against the pattern <"element/", 7>, reduce the length of the path
to exclude the trailing slash when calling match_basename().
A similar adjustment for match_pathname() might be needed, but I
didn't look into it.
Signed-off-by: Junio C Hamano <redacted>
---
attr.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:56:31
The function takes two counted strings (<basename, basenamelen> and
<pattern, patternlen>) as parameters, together with prefix (the
length of the prefix in pattern that is to be matched literally
without globbing against the basename) and EXC_* flags that tells it
how to match the pattern against the basename.
However, it did not pay attention to the length of these counted
strings. Update them to do the following:
* When the entire pattern is to be matched literally, the pattern
matches the basename only when the lengths of them are the same,
and they match up to that length.
* When the pattern is "*" followed by a string to be matched
literally, make sure that the basenamelen is equal or longer than
the "literal" part of the pattern, and the tail of the basename
string matches that literal part.
* Otherwise, make sure we use only the counted part of the strings
when calling fnmatch_icase(). Because these counted strings are
full strings most of the time, avoid unnecessary allocation.
Signed-off-by: Junio C Hamano <redacted>
---
dir.c | 31 +++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
@@ -537,15 +537,38 @@ int match_basename(const char *basename, int basenamelen,intflags){if(prefix==patternlen){-if(!strcmp_icase(pattern,basename))+if(patternlen==basenamelen&&+!strncmp_icase(pattern,basename,basenamelen))return1;}elseif(flags&EXC_FLAG_ENDSWITH){+/* "*literal" matching against "fooliteral" */if(patternlen-1<=basenamelen&&-!strcmp_icase(pattern+1,-basename+basenamelen-patternlen+1))+!strncmp_icase(pattern+1,+basename+basenamelen-(patternlen-1),+patternlen-1))return1;}else{-if(fnmatch_icase(pattern,basename,0)==0)+intmatch_status;+structstrbufpat=STRBUF_INIT;+structstrbufbase=STRBUF_INIT;+constchar*use_pat=pattern;+constchar*use_base=basename;++if(pattern[patternlen]){+strbuf_add(&pat,pattern,patternlen);+use_pat=pat.buf;+}+if(basename[basenamelen]){+strbuf_add(&base,basename,basenamelen);+use_base=base.buf;+}+match_status=fnmatch_icase(use_pat,use_base,0);+if(use_pat)+strbuf_release(&pat);+if(use_base)+strbuf_release(&base);++if(match_status==0)return1;}return0;
From: Junio C Hamano <hidden> Date: 2016-06-15 22:56:31
From: Jeff King <redacted>
Prior to v1.8.1.1, with:
git init
echo content >foo &&
mkdir subdir &&
echo content >subdir/bar &&
echo "subdir export-ignore" >.gitattributes
git add . &&
git commit -m one &&
git archive HEAD | tar tf -
the resulting archive would contain only "foo" and ".gitattributes",
not subdir. This was broken with a recent change that intended to
allow "subdir/ export-ignore" to also exclude the directory, but
instead ended up _requiring_ the trailing slash by mistake.
A pattern "subdir" should match any path "subdir", whether it is a
directory or a non-diretory. A pattern "subdir/" insists that a
path "subdir" must be a directory for it to match.
Signed-off-by: Junio C Hamano <redacted>
---
t/t5002-archive-attr-pattern.sh | 6 ++++++
1 file changed, 6 insertions(+)
From: Jeff King <hidden> Date: 2016-06-15 22:56:31
On Tue, Mar 26, 2013 at 11:39:28AM -0700, Junio C Hamano wrote:
The function takes two strings (pathname and basename) as if they
are independent strings, but in reality, the latter is always
pointing into a substring in the former.
Clarify this relationship by expressing the latter as an offset into
the former.
Signed-off-by: Junio C Hamano <redacted>
This is a huge improvement in maintainability. My initial fix attempt
was to just xstrdup() the strings (knowing that the performance would be
horrible, but I was still investigating correctness issues at that
point). And of course I ran into this same issue as I tried to make a
copy of pathname.
So even without the rest of the fix, this is definitely a good idea. :)
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:56:31
On Tue, Mar 26, 2013 at 11:39:29AM -0700, Junio C Hamano wrote:
The function takes two counted strings (<basename, basenamelen> and
<pattern, patternlen>) as parameters, together with prefix (the
length of the prefix in pattern that is to be matched literally
without globbing against the basename) and EXC_* flags that tells it
how to match the pattern against the basename.
However, it did not pay attention to the length of these counted
strings. Update them to do the following:
* When the entire pattern is to be matched literally, the pattern
matches the basename only when the lengths of them are the same,
and they match up to that length.
* When the pattern is "*" followed by a string to be matched
literally, make sure that the basenamelen is equal or longer than
the "literal" part of the pattern, and the tail of the basename
string matches that literal part.
* Otherwise, make sure we use only the counted part of the strings
when calling fnmatch_icase(). Because these counted strings are
full strings most of the time, avoid unnecessary allocation.
I think this is OK, with the intention that we would eventually drop the
allocations from your third bullet point in favor of using a
byte-counted version of fnmatch (i.e., nwildmatch). But until then we're
going to see a performance drop.
The pattern is usually going to be NUL-terminated at the length counter,
but every time we feed a directory, it's going to run into this
allocation. And we do it once for _every_ directory against _every_
wildcard gitignore pattern. So I think it is probably going to be
measurable. I guess we can try measuring it on something like WebKit,
which has plenty of both directories and gitattributes.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:56:31
On Tue, Mar 26, 2013 at 11:39:30AM -0700, Junio C Hamano wrote:
A similar adjustment for match_pathname() might be needed, but I
didn't look into it.
I notice that match_pathname takes _two_ lengths for the pattern: the
nowildcardlen (called "prefix", and the full patternlen). But the first
thing it does is:
if (*pattern == '/') {
pattern++;
prefix--;
}
which seems obviously wrong, as patternlen should be dropped, too. But
we do not seem to look at patternlen at all! I think we can drop the
parameter totally.
We do seem to use strncmp_icase through the rest of the function,
though, which should be OK. The one exception is that we call fnmatch at
the end. Should the allocation hack from the previous patch make its way
into an "fnmatch_icase_bytes()" function, so we can use it here, too?
And then when we have a more efficient solution, we can just plug it in
there.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:56:31
On Tue, Mar 26, 2013 at 11:39:31AM -0700, Junio C Hamano wrote:
From: Jeff King <redacted>
Prior to v1.8.1.1, with:
git init
echo content >foo &&
mkdir subdir &&
echo content >subdir/bar &&
echo "subdir export-ignore" >.gitattributes
git add . &&
git commit -m one &&
git archive HEAD | tar tf -
the resulting archive would contain only "foo" and ".gitattributes",
not subdir. This was broken with a recent change that intended to
allow "subdir/ export-ignore" to also exclude the directory, but
instead ended up _requiring_ the trailing slash by mistake.
Yeah, I think that is fine. I'd just squash this test and description
into the previous patch, though (I do not care about dropping my commit
count by 1).
And of course,
Signed-off-by: Jeff King <redacted>
A pattern "subdir" should match any path "subdir", whether it is a
directory or a non-diretory. A pattern "subdir/" insists that a
path "subdir" must be a directory for it to match.
From: Jeff King <hidden> Date: 2016-06-15 22:56:32
On Tue, Mar 26, 2013 at 02:55:59PM -0400, Jeff King wrote:
quoted
* Otherwise, make sure we use only the counted part of the strings
when calling fnmatch_icase(). Because these counted strings are
full strings most of the time, avoid unnecessary allocation.
I think this is OK, with the intention that we would eventually drop the
allocations from your third bullet point in favor of using a
byte-counted version of fnmatch (i.e., nwildmatch). But until then we're
going to see a performance drop.
The pattern is usually going to be NUL-terminated at the length counter,
but every time we feed a directory, it's going to run into this
allocation. And we do it once for _every_ directory against _every_
wildcard gitignore pattern. So I think it is probably going to be
measurable. I guess we can try measuring it on something like WebKit,
which has plenty of both directories and gitattributes.
I timed this doing "git archive HEAD" on webkit.git before and after. It
actually ended up not mattering much (I think because it is only the
directories which are affected, not each individually path, so it's a
much smaller number than you'd think). The best-of-five timing was
slightly slower, but was within the noise.
So I do still think it would make sense to go to a byte-limited version
of fnmatch eventually, just for code cleanliness and predictability of
performance, but this is really not a bad solution in the interim.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:56:32
On Tue, Mar 26, 2013 at 03:05:58PM -0400, Jeff King wrote:
On Tue, Mar 26, 2013 at 11:39:30AM -0700, Junio C Hamano wrote:
quoted
A similar adjustment for match_pathname() might be needed, but I
didn't look into it.
I notice that match_pathname takes _two_ lengths for the pattern: the
nowildcardlen (called "prefix", and the full patternlen). But the first
thing it does is:
if (*pattern == '/') {
pattern++;
prefix--;
}
which seems obviously wrong, as patternlen should be dropped, too. But
we do not seem to look at patternlen at all! I think we can drop the
parameter totally.
We do seem to use strncmp_icase through the rest of the function,
though, which should be OK. The one exception is that we call fnmatch at
the end. Should the allocation hack from the previous patch make its way
into an "fnmatch_icase_bytes()" function, so we can use it here, too?
And then when we have a more efficient solution, we can just plug it in
there.
Hmm. match_pathname does have this:
/*
* baselen does not count the trailing slash. base[] may or
* may not end with a trailing slash though.
*/
if (pathlen < baselen + 1 ||
(baselen && pathname[baselen] != '/') ||
strncmp_icase(pathname, base, baselen))
return 0;
which seems to imply that the trailing slash is important here, and that
we should not drop it when passing the path to match_pathname. I'm
still trying to figure out exactly what it is that the extra slash check
is for, and whether it might not have the same problem.
-Peff
qOn Tue, Mar 26, 2013 at 11:39:27AM -0700, Junio C Hamano wrote:
So here is an attempt to fix the unintended regression, on top of
9db9eecfe5c2 (attr: avoid calling find_basename() twice per path,
2013-01-16). It consists of four patches.
Not that I disagree with this. Just wanted to see how far the "dtype"
idea went. How about this? git_check_attr() now takes dtype as an
argument and the caller must not add the trailing slash. This could be
split into two patches, one for git_check_attr prototype change, and
the other the real meat.
-- 8< --
@@ -704,7 +705,7 @@ static int fill_one(const char *what, struct match_attr *a, int rem)}staticintfill(constchar*path,intpathlen,constchar*basename,-structattr_stack*stk,intrem)+intdtype,structattr_stack*stk,intrem){inti;constchar*base=stk->origin?stk->origin:"";
@@ -713,7 +714,7 @@ static int fill(const char *path, int pathlen, const char *basename,structmatch_attr*a=stk->attrs[i];if(a->is_macro)continue;-if(path_matches(path,pathlen,basename,+if(path_matches(path,pathlen,basename,dtype,&a->u.pat,base,stk->originlen))rem=fill_one("fill",a,rem);}
@@ -748,20 +749,17 @@ static int macroexpand_one(int attr_nr, int rem)*Collectallattributesforpathintothearraypointedtoby*check_all_attr.*/-staticvoidcollect_all_attrs(constchar*path)+staticvoidcollect_all_attrs(constchar*path,intdtype){structattr_stack*stk;inti,pathlen,rem,dirlen;-constchar*basename,*cp,*last_slash=NULL;+constchar*basename,*cp;-for(cp=path;*cp;cp++){-if(*cp=='/'&&cp[1])-last_slash=cp;-}-pathlen=cp-path;-if(last_slash){-basename=last_slash+1;-dirlen=last_slash-path;+cp=strrchr(path,'/');+pathlen=strlen(path);+if(cp){+basename=cp+1;+dirlen=cp-path;}else{basename=path;dirlen=0;
@@ -796,7 +794,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check){inti,count,j;-collect_all_attrs(path);+collect_all_attrs(path,DT_REG);/* Count the number of attributes that are set. */count=0;
On Wed, Mar 27, 2013 at 4:33 AM, Jeff King [off-list ref] wrote:
Hmm. match_pathname does have this:
/*
* baselen does not count the trailing slash. base[] may or
* may not end with a trailing slash though.
*/
if (pathlen < baselen + 1 ||
(baselen && pathname[baselen] != '/') ||
strncmp_icase(pathname, base, baselen))
return 0;
which seems to imply that the trailing slash is important here, and that
we should not drop it when passing the path to match_pathname. I'm
still trying to figure out exactly what it is that the extra slash check
is for, and whether it might not have the same problem.
The "may not end with a trailing slash" can only happen when baselen
== 0. And that rule is documented in code, at this line in
last_exclude_matching_from_list:
assert(x->baselen == 0 || x->base[x->baselen - 1] == '/');
--
Duy
On Wed, Mar 27, 2013 at 1:49 AM, Jeff King [off-list ref] wrote:
On Tue, Mar 26, 2013 at 11:39:28AM -0700, Junio C Hamano wrote:
quoted
The function takes two strings (pathname and basename) as if they
are independent strings, but in reality, the latter is always
pointing into a substring in the former.
Clarify this relationship by expressing the latter as an offset into
the former.
Signed-off-by: Junio C Hamano <redacted>
This is a huge improvement in maintainability. My initial fix attempt
was to just xstrdup() the strings (knowing that the performance would be
horrible, but I was still investigating correctness issues at that
point). And of course I ran into this same issue as I tried to make a
copy of pathname.
So even without the rest of the fix, this is definitely a good idea. :)
match_{base,path}name and their exclude callers do the same thing. I
guess I'm used to it and did not see the maintainability issue. Maybe
we should do the same there too.
--
Duy
From: Jeff King <hidden> Date: 2016-06-15 22:56:33
On Tue, Mar 26, 2013 at 03:05:58PM -0400, Jeff King wrote:
On Tue, Mar 26, 2013 at 11:39:30AM -0700, Junio C Hamano wrote:
quoted
A similar adjustment for match_pathname() might be needed, but I
didn't look into it.
[...]
We do seem to use strncmp_icase through the rest of the function,
though, which should be OK. The one exception is that we call fnmatch at
the end. Should the allocation hack from the previous patch make its way
into an "fnmatch_icase_bytes()" function, so we can use it here, too?
And then when we have a more efficient solution, we can just plug it in
there.
Hmm, yeah, there is more going on here than just that. If I add the
tests below, the first one (a wildcard) passes, because you fixed the
fnmatch code path. But the deep/ ones do not, as they should be going
through match_pathname. I expected the deep/with/wildcard one to fail
(because of the fnmatch problem I mentioned above), but not the
deep/and/slashless one, which should be using strncmp. I'll see if I can
track down the cause.
-Peff
---
@@ -32,6 +32,21 @@ test_expect_success 'setup' 'gitaddignored-without-slash/foo&&echoignored-without-slashexport-ignore>>.git/info/attributes&&+mkdir-pwildcard-without-slash&&+echo"ignored without slash">wildcard-without-slash/foo&&+gitaddwildcard-without-slash/foo&&+echo"wild*-without-slash export-ignore">>.git/info/attributes&&++mkdir-pdeep/and/slashless&&+echo"ignored without slash">deep/and/slashless/foo&&+gitadddeep/and/slashless/foo&&+echodeep/and/slashlessexport-ignore>>.git/info/attributes&&++mkdir-pdeep/with/wildcard&&+echo"ignored without slash">deep/with/wildcard/foo&&+gitadddeep/with/wildcard/foo&&+echo"deep/*t*/wildcard export-ignore">>.git/info/attributes&&+mkdir-pone-level-lower/two-levels-lower/ignored-only-if-dir&&echoignoredbyignoreddir>one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir&&gitaddone-level-lower&&
From: Jeff King <hidden> Date: 2016-06-15 22:56:33
On Tue, Mar 26, 2013 at 11:39:27AM -0700, Junio C Hamano wrote:
So here is an attempt to fix the unintended regression, on top of
9db9eecfe5c2 (attr: avoid calling find_basename() twice per path,
2013-01-16). It consists of four patches.
Here's my update to the series. I think this should fix all of the
issues. And it should be very easy to drop in Duy's nwildmatch later on;
it can just replace the fnmatch_icase_mem function added in patch 2
below.
The main fix in this iteration is that match_pathname receives the same
treatment as match_basename, which is done in patches 3 and 4 (the
issues were subtly different enough that I didn't want to squash it all
together; plus, gotta keep that commit count up).
[1/6]: attr.c::path_matches(): the basename is part of the pathname
[2/6]: dir.c::match_basename(): pay attention to the length of string parameters
[3/6]: dir.c::match_pathname(): adjust patternlen when shifting pattern
[4/6]: dir.c::match_pathname(): pay attention to the length of string parameters
[5/6]: attr.c::path_matches(): special case paths that end with a slash
[6/6]: t: check that a pattern without trailing slash matches a directory
-Peff
PS I followed your subject-naming convention since I was adding into
your series, but it seems quite long to me. I would have just said:
"match_basename: pay attention...".
From: Jeff King <hidden> Date: 2016-06-15 22:56:33
From: Junio C Hamano <redacted>
The function takes two strings (pathname and basename) as if they
are independent strings, but in reality, the latter is always
pointing into a substring in the former.
Clarify this relationship by expressing the latter as an offset into
the former.
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jeff King <redacted>
---
This is identical to the original 1/4.
attr.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:56:33
From: Junio C Hamano <redacted>
The function takes two counted strings (<basename, basenamelen> and
<pattern, patternlen>) as parameters, together with prefix (the
length of the prefix in pattern that is to be matched literally
without globbing against the basename) and EXC_* flags that tells it
how to match the pattern against the basename.
However, it did not pay attention to the length of these counted
strings. Update them to do the following:
* When the entire pattern is to be matched literally, the pattern
matches the basename only when the lengths of them are the same,
and they match up to that length.
* When the pattern is "*" followed by a string to be matched
literally, make sure that the basenamelen is equal or longer than
the "literal" part of the pattern, and the tail of the basename
string matches that literal part.
* Otherwise, use the new fnmatch_icase_mem helper to make
sure we only lookmake sure we use only look at the
counted part of the strings. Because these counted strings are
full strings most of the time, we check for termination
to avoid unnecessary allocation.
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jeff King <redacted>
---
Compared to v1:
- This factors the fnmatch bits into a helper function so we can reuse it
later. As a result, the variable names are changed a bit.
- The original did:
if (use_pat)
strbuf_release(&pat);
but AFAICT that was a useless conditional; use_pat always points to
either the incoming buffer or the strbuf. But strbuf_release will
handle both cases for us.
dir.c | 40 ++++++++++++++++++++++++++++++++++++----
1 file changed, 36 insertions(+), 4 deletions(-)
@@ -34,6 +34,33 @@ int fnmatch_icase(const char *pattern, const char *string, int flags)returnfnmatch(pattern,string,flags|(ignore_case?FNM_CASEFOLD:0));}+staticintfnmatch_icase_mem(constchar*pattern,intpatternlen,+constchar*string,intstringlen,+intflags)+{+intmatch_status;+structstrbufpat_buf=STRBUF_INIT;+structstrbufstr_buf=STRBUF_INIT;+constchar*use_pat=pattern;+constchar*use_str=string;++if(pattern[patternlen]){+strbuf_add(&pat_buf,pattern,patternlen);+use_pat=pat_buf.buf;+}+if(string[stringlen]){+strbuf_add(&str_buf,string,stringlen);+use_str=str_buf.buf;+}++match_status=fnmatch_icase(use_pat,use_str,0);++strbuf_release(&pat_buf);+strbuf_release(&str_buf);++returnmatch_status;+}+staticsize_tcommon_prefix_len(constchar**pathspec){constchar*n,*first;
@@ -537,15 +564,20 @@ int match_basename(const char *basename, int basenamelen,intflags){if(prefix==patternlen){-if(!strcmp_icase(pattern,basename))+if(patternlen==basenamelen&&+!strncmp_icase(pattern,basename,basenamelen))return1;}elseif(flags&EXC_FLAG_ENDSWITH){+/* "*literal" matching against "fooliteral" */if(patternlen-1<=basenamelen&&-!strcmp_icase(pattern+1,-basename+basenamelen-patternlen+1))+!strncmp_icase(pattern+1,+basename+basenamelen-(patternlen-1),+patternlen-1))return1;}else{-if(fnmatch_icase(pattern,basename,0)==0)+if(fnmatch_icase_mem(pattern,patternlen,+basename,basenamelen,+0)==0)return1;}return0;
From: Jeff King <hidden> Date: 2016-06-15 22:56:33
If we receive a pattern that starts with "/", we shift it
forward to avoid looking at the "/" part. Since the prefix
and patternlen parameters are counts of what is in the
pattern, we must decrement them as we increment the pointer.
We remembered to handle prefix, but not patternlen. This
didn't cause any bugs, though, because the patternlen
parameter is not actually used. Since it will be used in
future patches, let's correct this oversight.
Signed-off-by: Jeff King <redacted>
---
New in this iteration.
dir.c | 1 +
1 file changed, 1 insertion(+)
From: Jeff King <hidden> Date: 2016-06-15 22:56:33
This function takes two counted strings: a <pattern, patternlen> pair
and a <pathname, pathlen> pair. But we end up feeding the result to
fnmatch, which expects NUL-terminated strings.
We can fix this by calling the fnmatch_icase_mem function, which
handles re-allocating into a NUL-terminated string if necessary.
While we're at it, we can avoid even calling fnmatch in some cases. In
addition to patternlen, we get "prefix", the size of the pattern that
contains no wildcard characters. We do a straight match of the prefix
part first, and then use fnmatch to cover the rest. But if there are
no wildcards in the pattern at all, we do not even need to call
fnmatch; we would simply be comparing two empty strings.
Signed-off-by: Jeff King <redacted>
---
New in this iteration.
dir.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
@@ -624,11 +624,22 @@ int match_pathname(const char *pathname, int pathlen,if(strncmp_icase(pattern,name,prefix))return0;pattern+=prefix;+patternlen-=prefix;name+=prefix;namelen-=prefix;++/*+*Ifthewholepatterndidnothaveawildcard,+*thenourprefixmatchisallweneed;we+*donotneedtocallfnmatchatall.+*/+if(!patternlen&&!namelen)+return1;}-returnfnmatch_icase(pattern,name,FNM_PATHNAME)==0;+returnfnmatch_icase_mem(pattern,patternlen,+name,namelen,+FNM_PATHNAME)==0;}/* Scan the list and let the last match determine the fate.
From: Jeff King <hidden> Date: 2016-06-15 22:56:33
From: Junio C Hamano <redacted>
The function is given a string that ends with a slash to signal that
the path is a directory to make sure that a pattern that ends with a
slash (i.e. MUSTBEDIR) can tell directories and non-directories
apart. However, the pattern itself (pat->pattern and
pat->patternlen) that came from such a MUSTBEDIR pattern is
represented as a string that ends with a slash, but patternlen does
not count that trailing slash. A MUSTBEDIR pattern "element/" is
represented as a counted string <"element/", 7> and this must match
match pathname "element/".
Because match_basename() and match_pathname() want to see pathname
"element" to match against the pattern <"element/", 7>, reduce the
length of the path to exclude the trailing slash when calling
these functions.
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jeff King <redacted>
---
Tweaked since v1 to also drop the trailing slash when we pass the path
to match_pathname.
attr.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:56:33
Prior to v1.8.1.1, with:
git init
echo content >foo &&
mkdir subdir &&
echo content >subdir/bar &&
echo "subdir export-ignore" >.gitattributes
git add . &&
git commit -m one &&
git archive HEAD | tar tf -
the resulting archive would contain only "foo" and
".gitattributes", not subdir. This was broken with a recent
change that intended to allow "subdir/ export-ignore" to
also exclude the directory, but instead ended up _requiring_
the trailing slash by mistake.
A pattern "subdir" should match any path "subdir", whether it is a
directory or a non-diretory. A pattern "subdir/" insists that a
path "subdir" must be a directory for it to match.
This patch adds test not just for this simple case, but also
for deeper cross-directory cases, as well as cases with
wildcards.
Signed-off-by: Jeff King <redacted>
---
Added new tests since v1 that handle the match_pathname code path.
t/t5002-archive-attr-pattern.sh | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
@@ -27,6 +27,25 @@ test_expect_success 'setup' 'echoignored-only-if-dir/export-ignore>>.git/info/attributes&&gitaddignored-only-if-dir&&+mkdir-pignored-without-slash&&+echo"ignored without slash">ignored-without-slash/foo&&+gitaddignored-without-slash/foo&&+echo"ignored-without-slash export-ignore">>.git/info/attributes&&++mkdir-pwildcard-without-slash&&+echo"ignored without slash">wildcard-without-slash/foo&&+gitaddwildcard-without-slash/foo&&+echo"wild*-without-slash export-ignore">>.git/info/attributes&&++mkdir-pdeep/and/slashless&&+echo"ignored without slash">deep/and/slashless/foo&&+gitadddeep/and/slashless/foo&&+echo"deep/and/slashless export-ignore">>.git/info/attributes&&++mkdir-pdeep/with/wildcard&&+echo"ignored without slash">deep/with/wildcard/foo&&+gitadddeep/with/wildcard/foo&&+echo"deep/*t*/wildcard export-ignore">>.git/info/attributes&&mkdir-pone-level-lower/two-levels-lower/ignored-only-if-dir&&echoignoredbyignoreddir>one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir&&
From: Jeff King <hidden> Date: 2016-06-15 22:56:34
On Thu, Mar 28, 2013 at 05:47:28PM -0400, Jeff King wrote:
From: Junio C Hamano <redacted>
The function takes two counted strings (<basename, basenamelen> and
<pattern, patternlen>) as parameters, together with prefix (the
length of the prefix in pattern that is to be matched literally
without globbing against the basename) and EXC_* flags that tells it
how to match the pattern against the basename.
However, it did not pay attention to the length of these counted
strings. Update them to do the following:
* When the entire pattern is to be matched literally, the pattern
matches the basename only when the lengths of them are the same,
and they match up to that length.
Hrm. Though the tip of this series passes all tests, this one actually
breaks bisectability. What happens is that the existing code passes:
path=foo/
pathlen=4
pattern=foo/
patternlen=3
match_basename is happy to compare "foo/" to "foo/" and realize they're
equal. With this change, we compare "foo" to "foo/" and do not match. It
isn't until the later patch where you start passing pathlen=3 that it
works again.
I wonder if it is worth reordering the series to put the path_matches
fix first.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:56:34
On Thu, Mar 28, 2013 at 06:40:27PM -0400, Jeff King wrote:
On Thu, Mar 28, 2013 at 05:47:28PM -0400, Jeff King wrote:
quoted
From: Junio C Hamano <redacted>
The function takes two counted strings (<basename, basenamelen> and
<pattern, patternlen>) as parameters, together with prefix (the
length of the prefix in pattern that is to be matched literally
without globbing against the basename) and EXC_* flags that tells it
how to match the pattern against the basename.
However, it did not pay attention to the length of these counted
strings. Update them to do the following:
* When the entire pattern is to be matched literally, the pattern
matches the basename only when the lengths of them are the same,
and they match up to that length.
Hrm. Though the tip of this series passes all tests, this one actually
breaks bisectability. What happens is that the existing code passes:
Ugh. That is a problem, but this series does _not_ pass all tests. I
think I failed to run the complete test suite on the correct tip.
My match_pathspec fix breaks at least t1011.
-Peff
Eek, yeah, that's obviously wrong. Thanks for catching it. Fixing that
clears up all of the test failures outside of t5002.
And if you move patch 5 ("special case paths that end with a slash")
into position 2, it cleans up the mid-series failures of t5002, making
the series clean for later bisecting.
Thanks for looking it over.
-Peff
I think you (or Junio) should rebase this on maint. Since c41244e
(included in maint), this call is turned to wildmatch(WM_PATHNAME) and
WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like
"foo/**/bar". A diff between next and pu shows me that WM_PATHNAME is
incorrectly converted to FNM_PATHNAME. I hope that is the cause of all
breakages Junio found out on pu.
--
Duy
I think you (or Junio) should rebase this on maint. Since c41244e
(included in maint), this call is turned to wildmatch(WM_PATHNAME) and
WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like
"foo/**/bar". A diff between next and pu shows me that WM_PATHNAME is
incorrectly converted to FNM_PATHNAME. I hope that is the cause of all
breakages Junio found out on pu.
Just tested. t0003 and t3001 on 'pu' work for me because I have
USE_WILDMATCH on (which turns FNM_PATHNAME to WM_PATHNAME). Both break
without USE_WILDMATCH.
--
Duy
I think you (or Junio) should rebase this on maint. Since c41244e
(included in maint), this call is turned to wildmatch(WM_PATHNAME) and
WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like
"foo/**/bar". A diff between next and pu shows me that WM_PATHNAME is
incorrectly converted to FNM_PATHNAME. I hope that is the cause of all
breakages Junio found out on pu.
Just tested. t0003 and t3001 on 'pu' work for me because I have
USE_WILDMATCH on (which turns FNM_PATHNAME to WM_PATHNAME). Both break
without USE_WILDMATCH.
@@ -564,7 +564,7 @@ int match_pathname(const char *pathname, int pathlen,returnfnmatch_icase_mem(pattern,patternlen,name,namelen,-FNM_PATHNAME)==0;+WM_PATHNAME)==0;}
Gives only one breakage, so we are coming closer.
*** t3001-ls-files-others-exclude.sh ***
[snip]
not ok 17 - ls-files with "**" patterns
On Fri, Mar 29, 2013 at 6:32 PM, Torsten Bögershausen [off-list ref] wrote:
quoted
Just tested. t0003 and t3001 on 'pu' work for me because I have
USE_WILDMATCH on (which turns FNM_PATHNAME to WM_PATHNAME). Both break
without USE_WILDMATCH.
Hm, tested what?
Tested t0003 and t3001 with and without USE_WILDMATCH, which is pretty
much like you patch, except that wildmatch is used instead of fnmatch.
@@ -564,7 +564,7 @@ int match_pathname(const char *pathname, int pathlen,returnfnmatch_icase_mem(pattern,patternlen,name,namelen,-FNM_PATHNAME)==0;+WM_PATHNAME)==0;}
Gives only one breakage, so we are coming closer.
*** t3001-ls-files-others-exclude.sh ***
[snip]
not ok 17 - ls-files with "**" patterns
I think you (or Junio) should rebase this on maint. Since c41244e
(included in maint), this call is turned to wildmatch(WM_PATHNAME) and
WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like
"foo/**/bar". A diff between next and pu shows me that WM_PATHNAME is
incorrectly converted to FNM_PATHNAME. I hope that is the cause of all
breakages Junio found out on pu.
I don't think we want to rebase; the regression is in the v1.8.1 series,
and I suspected that Junio was planning to ship a v1.8.1.6 with the fix.
The wildmatch code comes in v1.8.2.
So we would want to do any adjustment to the fix when we merge up to
maint.
-Peff
I think you (or Junio) should rebase this on maint. Since c41244e
(included in maint), this call is turned to wildmatch(WM_PATHNAME) and
WM_PATHNAME is _not_ the same as FNM_PATHNAME for patterns like
"foo/**/bar". A diff between next and pu shows me that WM_PATHNAME is
incorrectly converted to FNM_PATHNAME. I hope that is the cause of all
breakages Junio found out on pu.
I don't think we want to rebase; the regression is in the v1.8.1 series,
and I suspected that Junio was planning to ship a v1.8.1.6 with the fix.
The wildmatch code comes in v1.8.2.
So we would want to do any adjustment to the fix when we merge up to
maint.
OK. Then Junio, you may need to resolve the conflict with something
like this. Originally match_basename uses fnmatch, not wildmatch. But
using wildmatch there too should be fine, now that both
match_{base,path}name share fnmatch_icase_mem().
-- 8< --
@@ -81,7 +81,9 @@ static int fnmatch_icase_mem(const char *pattern, int patternlen,use_str=str_buf.buf;}-match_status=fnmatch_icase(use_pat,use_str,flags);+if(ignore_case)+flags|=WM_CASEFOLD;+match_status=wildmatch(use_pat,use_str,flags,NULL);strbuf_release(&pat_buf);strbuf_release(&str_buf);
@@ -564,7 +566,7 @@ int match_pathname(const char *pathname, int pathlen,returnfnmatch_icase_mem(pattern,patternlen,name,namelen,-FNM_PATHNAME)==0;+WM_PATHNAME)==0;}/*--8<--