Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash
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: 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 --- diff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh index 3be809c..234a615 100755 --- a/t/t5002-archive-attr-pattern.sh +++ b/t/t5002-archive-attr-pattern.sh @@ -32,6 +32,21 @@ test_expect_success 'setup' ' git add ignored-without-slash/foo echo ignored-without-slash export-ignore .git/info/attributes + mkdir -p wildcard-without-slash + echo ignored without slash wildcard-without-slash/foo + git add wildcard-without-slash/foo + echo wild*-without-slash export-ignore .git/info/attributes + + mkdir -p deep/and/slashless + echo ignored without slash deep/and/slashless/foo + git add deep/and/slashless/foo + echo deep/and/slashless export-ignore .git/info/attributes + + mkdir -p deep/with/wildcard + echo ignored without slash deep/with/wildcard/foo + git add deep/with/wildcard/foo + echo deep/*t*/wildcard export-ignore .git/info/attributes + mkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir echo ignored by ignored dir one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir git add one-level-lower @@ -55,6 +70,12 @@ test_expect_missing archive/ignored-without-slash/foo test_expect_missingarchive/ignored-ony-if-dir/ignored-by-ignored-dir test_expect_missing archive/ignored-without-slash/ test_expect_missing archive/ignored-without-slash/foo +test_expect_missing archive/wildcard-without-slash/ +test_expect_missing archive/wildcard-without-slash/foo +test_expect_missing archive/deep/and/slashless/ +test_expect_missing archive/deep/and/slashless/foo +test_expect_missing archive/deep/with/wildcard/ +test_expect_missing archive/deep/with/wildcard/foo test_expect_exists archive/one-level-lower/ test_expect_missing archive/one-level-lower/two-levels-lower/ignored-only-if-dir/ test_expect_missing archive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash
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:
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash
On Wed, Mar 27, 2013 at 4:33 AM, Jeff King p...@peff.net 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
