[regression?] trailing slash required in .gitattributes

12 messages, 2 authors, 2016-06-15 · open the first message on its own page

[regression?] trailing slash required in .gitattributes

From: Jeff King <hidden>
Date: 2016-06-15 22:56:26

Prior to v1.8.1.1, if I did this:

  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 -

my archive would contain only "foo" and ".gitattributes", not subdir. As
of v1.8.1.1, the attribute on subdir is ignored unless it is written
with a trailing slash, like:

  subdir/ export-ignore

The issue bisects to 94bc671 (Add directory pattern matching to
attributes, 2012-12-08). That commit actually tests not only that
"subdir/" matches, but also that just "subdir" does not match.

The commit message there is vague about the reasoning, but my
understanding is that it was meant to harmonize gitignore and
gitattributes, the former of which can take "dir/". I don't have a
problem with offering "dir/" to match only directories, but what is the
point in disallowing just "dir" to match a directory?

It seems like a pointless regression to me, but I'm not clear whether it
was intentional or not (and if it was intentional, I think we would need
to handle it with a proper transition period, not in a maint release).

-Peff

Re: [regression?] trailing slash required in .gitattributes

From: Jeff King <hidden>
Date: 2016-06-15 22:56:27

On Tue, Mar 19, 2013 at 01:57:56PM -0400, Jeff King wrote:

Prior to v1.8.1.1, if I did this:

  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 -

my archive would contain only "foo" and ".gitattributes", not subdir. As
of v1.8.1.1, the attribute on subdir is ignored unless it is written
with a trailing slash, like:

  subdir/ export-ignore

The issue bisects to 94bc671 (Add directory pattern matching to
attributes, 2012-12-08). That commit actually tests not only that
"subdir/" matches, but also that just "subdir" does not match.

Sorry, I mis-read the tests. They are not testing that "subdir" does not
work, only that "subdir/" will match only a directory, not a regular
file. Which does make sense.

So I think the regression is accidental. And we would want tests like
this on top (which currently fail):

diff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh
index 0c847fb..3be809c 100755
--- a/t/t5002-archive-attr-pattern.sh
+++ b/t/t5002-archive-attr-pattern.sh

@@ -27,6 +27,10 @@ test_expect_success 'setup' '
 	echo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&
 	git add ignored-only-if-dir &&
 
+	mkdir -p ignored-without-slash &&
+	echo ignored without slash >ignored-without-slash/foo &&
+	git add ignored-without-slash/foo &&
+	echo ignored-without-slash 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 &&

@@ -49,6 +53,8 @@ test_expect_missing	archive/ignored-ony-if-dir/ignored-by-ignored-dir
 test_expect_exists	archive/not-ignored-dir/
 test_expect_missing	archive/ignored-only-if-dir/
 test_expect_missing	archive/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_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

Re: [regression?] trailing slash required in .gitattributes

From: Jeff King <hidden>
Date: 2016-06-15 22:56:29

On Tue, Mar 19, 2013 at 02:10:42PM -0400, Jeff King wrote:

quoted

The issue bisects to 94bc671 (Add directory pattern matching to
attributes, 2012-12-08). That commit actually tests not only that
"subdir/" matches, but also that just "subdir" does not match.

[...]
So I think the regression is accidental. And we would want tests like
this on top (which currently fail):
[...]

I'm having trouble figuring out the right solution for this.

The problem is in path_matches, which used to receive just the unadorned
pathname, and now receives "path/" for directories. It now looks like
this:

static int path_matches(const char *pathname, int pathlen,
			const char *basename,
			const struct pattern *pat,
			const char *base, int baselen)
{
	const char *pattern = pat->pattern;
	int prefix = pat->nowildcardlen;

	if ((pat->flags & EXC_FLAG_MUSTBEDIR) &&
	    ((!pathlen) || (pathname[pathlen-1] != '/')))
		return 0;

This first stanza checks that a pattern like "foo/" must be matched by a
real directory. Which is fine; that's the point of adding the "/" to the
pattern.

	if (pat->flags & EXC_FLAG_NODIR) {
		return match_basename(basename,
				      pathlen - (basename - pathname),
				      pattern, prefix,
				      pat->patternlen, pat->flags);
	}
	return match_pathname(pathname, pathlen,
			      base, baselen,
			      pattern, prefix, pat->patternlen, pat->flags);
}

But then here we'll end up feeding "foo/" to be compared with "foo",
which we don't want. For a pattern "foo", we want to match _either_
"foo/" or "foo". So you'd think something like:

  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.

-Peff

Re: [regression?] trailing slash required in .gitattributes

From: Duy Nguyen <hidden>
Date: 2016-06-15 22:56:29

On Fri, Mar 22, 2013 at 06:24:39PM -0400, Jeff King wrote:

I'm having trouble figuring out the right solution for this.

Thanks for looking into this. It was on my todo list, but you beat me
to it :)

But then here we'll end up feeding "foo/" to be compared with "foo",
which we don't want. For a pattern "foo", we want to match _either_
"foo/" or "foo". So you'd think something like:

  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.

You can use nwildmatch() from this patch. I tested it lightly with
t3070-wildmatch.sh, feeding the strings with no terminating NUL. It
seems to work ok.

-- 8< --
Subject: [PATCH] wildmatch: do not require "text" to be NUL-terminated

This may be helpful when we just want to match a part of "text".
nwildmatch can be used for this purpose.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 wildmatch.c | 25 +++++++++++++------------
 wildmatch.h | 11 +++++++++--
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/wildmatch.c b/wildmatch.c
index 7192bdc..f97ae2a 100644
--- a/wildmatch.c
+++ b/wildmatch.c

@@ -52,7 +52,8 @@ typedef unsigned char uchar;
 #define ISXDIGIT(c) (ISASCII(c) && isxdigit(c))
 
 /* Match pattern "p" against "text" */
-static int dowild(const uchar *p, const uchar *text, unsigned int flags)
+static int dowild(const uchar *p, const uchar *text,
+		  const uchar *textend, unsigned int flags)
 {
 	uchar p_ch;
 	const uchar *pattern = p;

@@ -60,8 +61,9 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 	for ( ; (p_ch = *p) != '\0'; text++, p++) {
 		int matched, match_slash, negated;
 		uchar t_ch, prev_ch;
-		if ((t_ch = *text) == '\0' && p_ch != '*')
+		if (text >= textend && p_ch != '*')
 			return WM_ABORT_ALL;
+		t_ch = *text;
 		if ((flags & WM_CASEFOLD) && ISUPPER(t_ch))
 			t_ch = tolower(t_ch);
 		if ((flags & WM_CASEFOLD) && ISUPPER(p_ch))

@@ -101,7 +103,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 					 * both foo/bar and foo/a/bar.
 					 */
 					if (p[0] == '/' &&
-					    dowild(p + 1, text, flags) == WM_MATCH)
+					    dowild(p + 1, text, textend, flags) == WM_MATCH)
 						return WM_MATCH;
 					match_slash = 1;
 				} else

@@ -130,9 +132,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 				/* the slash is consumed by the top-level for loop */
 				break;
 			}
-			while (1) {
-				if (t_ch == '\0')
-					break;
+			while (text < textend) {
 				/*
 				 * Try to advance faster when an asterisk is
 				 * followed by a literal. We know in this case

@@ -145,18 +145,18 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 					p_ch = *p;
 					if ((flags & WM_CASEFOLD) && ISUPPER(p_ch))
 						p_ch = tolower(p_ch);
-					while ((t_ch = *text) != '\0' &&
+					while (text < textend &&
 					       (match_slash || t_ch != '/')) {
 						if ((flags & WM_CASEFOLD) && ISUPPER(t_ch))
 							t_ch = tolower(t_ch);
 						if (t_ch == p_ch)
 							break;
-						text++;
+						t_ch = *++text;
 					}
 					if (t_ch != p_ch)
 						return WM_NOMATCH;
 				}
-				if ((matched = dowild(p, text, flags)) != WM_NOMATCH) {
+				if ((matched = dowild(p, text, textend, flags)) != WM_NOMATCH) {
 					if (!match_slash || matched != WM_ABORT_TO_STARSTAR)
 						return matched;
 				} else if (!match_slash && t_ch == '/')

@@ -261,12 +261,13 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 		}
 	}
 
-	return *text ? WM_NOMATCH : WM_MATCH;
+	return text < textend ? WM_NOMATCH : WM_MATCH;
 }
 
 /* Match the "pattern" against the "text" string. */
-int wildmatch(const char *pattern, const char *text,
+int nwildmatch(const char *pattern, const char *text, int textlen,
 	      unsigned int flags, struct wildopts *wo)
 {
-	return dowild((const uchar*)pattern, (const uchar*)text, flags);
+	return dowild((const uchar*)pattern, (const uchar*)text,
+		      (const uchar*)text + textlen, flags);
 }

diff --git a/wildmatch.h b/wildmatch.h
index 4090c8f..cdd7544 100644
--- a/wildmatch.h
+++ b/wildmatch.h

@@ -12,7 +12,14 @@
 
 struct wildopts;
 
-int wildmatch(const char *pattern, const char *text,
-	      unsigned int flags,
+int nwildmatch(const char *pattern, const char *text,
+	      int len, unsigned int flags,
 	      struct wildopts *wo);
+
+/* Match the "pattern" against the "text" string. */
+static inline int wildmatch(const char *pattern, const char *text,
+	      unsigned int flags, struct wildopts *wo)
+{
+	return nwildmatch(pattern, text, strlen(text), flags, wo);
+}
 #endif

-- 
1.8.2.83.gc99314b

-- 8< --

Re: [regression?] trailing slash required in .gitattributes

From: Duy Nguyen <hidden>
Date: 2016-06-15 22:56:29

On Sat, Mar 23, 2013 at 11:18:24AM +0700, Duy Nguyen wrote:

You can use nwildmatch() from this patch. I tested it lightly with
t3070-wildmatch.sh, feeding the strings with no terminating NUL. It
seems to work ok.

And valgrind spotted my faults, especially for using strchr. You would
need this on top:

-- 8< --

diff --git a/wildmatch.c b/wildmatch.c
index f97ae2a..939bac8 100644
--- a/wildmatch.c
+++ b/wildmatch.c

@@ -61,9 +61,13 @@ static int dowild(const uchar *p, const uchar *text,
 	for ( ; (p_ch = *p) != '\0'; text++, p++) {
 		int matched, match_slash, negated;
 		uchar t_ch, prev_ch;
-		if (text >= textend && p_ch != '*')
-			return WM_ABORT_ALL;
-		t_ch = *text;
+		if (text >= textend) {
+			if (p_ch != '*')
+				return WM_ABORT_ALL;
+			else
+				t_ch = '\0';
+		} else
+			t_ch = *text;
 		if ((flags & WM_CASEFOLD) && ISUPPER(t_ch))
 			t_ch = tolower(t_ch);
 		if ((flags & WM_CASEFOLD) && ISUPPER(p_ch))

@@ -115,8 +119,9 @@ static int dowild(const uchar *p, const uchar *text,
 				/* Trailing "**" matches everything.  Trailing "*" matches
 				 * only if there are no more slash characters. */
 				if (!match_slash) {
-					if (strchr((char*)text, '/') != NULL)
-						return WM_NOMATCH;
+					for (;text < textend; text++)
+						if (*text == '/')
+							return WM_NOMATCH;
 				}
 				return WM_MATCH;
 			} else if (!match_slash && *p == '/') {

@@ -125,10 +130,11 @@ static int dowild(const uchar *p, const uchar *text,
 				 * with WM_PATHNAME matches the next
 				 * directory
 				 */
-				const char *slash = strchr((char*)text, '/');
-				if (!slash)
+				for (;text < textend; text++)
+					if (*text == '/')
+						break;
+				if (text == textend)
 					return WM_NOMATCH;
-				text = (const uchar*)slash;
 				/* the slash is consumed by the top-level for loop */
 				break;
 			}

@@ -151,7 +157,7 @@ static int dowild(const uchar *p, const uchar *text,
 							t_ch = tolower(t_ch);
 						if (t_ch == p_ch)
 							break;
-						t_ch = *++text;
+						t_ch = ++text < textend ? *text : '\0';
 					}
 					if (t_ch != p_ch)
 						return WM_NOMATCH;
-- 8< --

[PATCH 0/4] attr directory matching regression

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:56:30

I think the fix is something like this. There is still one thing I'd
like to do: make this code not rely on NUL for terminating the
patterns. That should remove the ugly "p[len] = '\0'" in
prepare_attr_stack() 4/4 and and the reallocation in add_exclude() (in
current code). But let's deal with the regression first.

Nguyễn Thái Ngọc Duy (4):
  wildmatch: do not require "text" to be NUL-terminated
  attr.c: fix pattern{,len} inconsistency in struct match_attr
  dir.c: make match_{base,path}name respect {basename,path}len
  attr.c: fix matching "subdir" without the trailing slash

 attr.c                          | 11 ++++++++++-
 dir.c                           | 13 ++++++++-----
 dir.h                           |  2 +-
 t/t5002-archive-attr-pattern.sh |  6 ++++++
 wildmatch.c                     | 43 ++++++++++++++++++++++++-----------------
 wildmatch.h                     | 11 +++++++++--
 6 files changed, 59 insertions(+), 27 deletions(-)

-- 
1.8.2.82.gc24b958

[PATCH 1/4] wildmatch: do not require "text" to be NUL-terminated

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:56:30

This may be helpful when we just want to match a part of "text".
nwildmatch can be used for this purpose.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 wildmatch.c | 43 +++++++++++++++++++++++++------------------
 wildmatch.h | 11 +++++++++--
 2 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/wildmatch.c b/wildmatch.c
index 7192bdc..939bac8 100644
--- a/wildmatch.c
+++ b/wildmatch.c

@@ -52,7 +52,8 @@ typedef unsigned char uchar;
 #define ISXDIGIT(c) (ISASCII(c) && isxdigit(c))
 
 /* Match pattern "p" against "text" */
-static int dowild(const uchar *p, const uchar *text, unsigned int flags)
+static int dowild(const uchar *p, const uchar *text,
+		  const uchar *textend, unsigned int flags)
 {
 	uchar p_ch;
 	const uchar *pattern = p;

@@ -60,8 +61,13 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 	for ( ; (p_ch = *p) != '\0'; text++, p++) {
 		int matched, match_slash, negated;
 		uchar t_ch, prev_ch;
-		if ((t_ch = *text) == '\0' && p_ch != '*')
-			return WM_ABORT_ALL;
+		if (text >= textend) {
+			if (p_ch != '*')
+				return WM_ABORT_ALL;
+			else
+				t_ch = '\0';
+		} else
+			t_ch = *text;
 		if ((flags & WM_CASEFOLD) && ISUPPER(t_ch))
 			t_ch = tolower(t_ch);
 		if ((flags & WM_CASEFOLD) && ISUPPER(p_ch))

@@ -101,7 +107,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 					 * both foo/bar and foo/a/bar.
 					 */
 					if (p[0] == '/' &&
-					    dowild(p + 1, text, flags) == WM_MATCH)
+					    dowild(p + 1, text, textend, flags) == WM_MATCH)
 						return WM_MATCH;
 					match_slash = 1;
 				} else

@@ -113,8 +119,9 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 				/* Trailing "**" matches everything.  Trailing "*" matches
 				 * only if there are no more slash characters. */
 				if (!match_slash) {
-					if (strchr((char*)text, '/') != NULL)
-						return WM_NOMATCH;
+					for (;text < textend; text++)
+						if (*text == '/')
+							return WM_NOMATCH;
 				}
 				return WM_MATCH;
 			} else if (!match_slash && *p == '/') {

@@ -123,16 +130,15 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 				 * with WM_PATHNAME matches the next
 				 * directory
 				 */
-				const char *slash = strchr((char*)text, '/');
-				if (!slash)
+				for (;text < textend; text++)
+					if (*text == '/')
+						break;
+				if (text == textend)
 					return WM_NOMATCH;
-				text = (const uchar*)slash;
 				/* the slash is consumed by the top-level for loop */
 				break;
 			}
-			while (1) {
-				if (t_ch == '\0')
-					break;
+			while (text < textend) {
 				/*
 				 * Try to advance faster when an asterisk is
 				 * followed by a literal. We know in this case

@@ -145,18 +151,18 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 					p_ch = *p;
 					if ((flags & WM_CASEFOLD) && ISUPPER(p_ch))
 						p_ch = tolower(p_ch);
-					while ((t_ch = *text) != '\0' &&
+					while (text < textend &&
 					       (match_slash || t_ch != '/')) {
 						if ((flags & WM_CASEFOLD) && ISUPPER(t_ch))
 							t_ch = tolower(t_ch);
 						if (t_ch == p_ch)
 							break;
-						text++;
+						t_ch = ++text < textend ? *text : '\0';
 					}
 					if (t_ch != p_ch)
 						return WM_NOMATCH;
 				}
-				if ((matched = dowild(p, text, flags)) != WM_NOMATCH) {
+				if ((matched = dowild(p, text, textend, flags)) != WM_NOMATCH) {
 					if (!match_slash || matched != WM_ABORT_TO_STARSTAR)
 						return matched;
 				} else if (!match_slash && t_ch == '/')

@@ -261,12 +267,13 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 		}
 	}
 
-	return *text ? WM_NOMATCH : WM_MATCH;
+	return text < textend ? WM_NOMATCH : WM_MATCH;
 }
 
 /* Match the "pattern" against the "text" string. */
-int wildmatch(const char *pattern, const char *text,
+int nwildmatch(const char *pattern, const char *text, int textlen,
 	      unsigned int flags, struct wildopts *wo)
 {
-	return dowild((const uchar*)pattern, (const uchar*)text, flags);
+	return dowild((const uchar*)pattern, (const uchar*)text,
+		      (const uchar*)text + textlen, flags);
 }

diff --git a/wildmatch.h b/wildmatch.h
index 4090c8f..cdd7544 100644
--- a/wildmatch.h
+++ b/wildmatch.h

@@ -12,7 +12,14 @@
 
 struct wildopts;
 
-int wildmatch(const char *pattern, const char *text,
-	      unsigned int flags,
+int nwildmatch(const char *pattern, const char *text,
+	      int len, unsigned int flags,
 	      struct wildopts *wo);
+
+/* Match the "pattern" against the "text" string. */
+static inline int wildmatch(const char *pattern, const char *text,
+	      unsigned int flags, struct wildopts *wo)
+{
+	return nwildmatch(pattern, text, strlen(text), flags, wo);
+}
 #endif

-- 
1.8.2.82.gc24b958

[PATCH 2/4] attr.c: fix pattern{,len} inconsistency in struct match_attr

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:56:30

When parse_exclude_pattern detects EXC_FLAG_MUSTBEDIR, it sets
patternlen _not_ to include the trailing slash and expects the caller
to trim it. Of the two callers, add_exclude() does, parse_attr_line()
does not.

Because of that, after parse_attr_line() returns, we may have pattern
"foo/" but its length is reported 3. Some functions do not care about
patternlen and will see the pattern as "foo/" while others may see it
as "foo". This patch makes patternlen reflect the true length of
pattern.

This is a bandage patch that's required for the next patch to pass the
test suite as that patch will rely on patternlen's correctness. The
true fix comes in the patch after the next one.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 attr.c | 2 ++
 dir.h  | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/attr.c b/attr.c
index e2f9377..1818ba5 100644
--- a/attr.c
+++ b/attr.c

@@ -255,6 +255,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 				      &res->u.pat.patternlen,
 				      &res->u.pat.flags,
 				      &res->u.pat.nowildcardlen);
+		if (res->u.pat.flags & EXC_FLAG_MUSTBEDIR)
+			res->u.pat.patternlen++;
 		if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {
 			warning(_("Negative patterns are ignored in git attributes\n"
 				  "Use '\\!' for literal leading exclamation."));

diff --git a/dir.h b/dir.h
index c3eb4b5..dc63fc8 100644
--- a/dir.h
+++ b/dir.h

@@ -40,7 +40,7 @@ struct exclude_list {
 		struct exclude_list *el;
 
 		const char *pattern;
-		int patternlen;
+		int patternlen;	/* must equal strlen(pattern) */
 		int nowildcardlen;
 		const char *base;
 		int baselen;

-- 
1.8.2.82.gc24b958

[PATCH 3/4] dir.c: make match_{base,path}name respect {basename,path}len

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:56:30

When match_basename was split out of excluded_from_list in 593cb88
(exclude: split basename matching code into a separate function -
2012-10-15), it took basenamelen only as a hint. basename was required
to be NUL-terminated at the given length.

This was fine until match_basename had a new caller (from attr.c) and
therefore was no longer excluded_from_list's internal business. Make
match_basename stop relying on the NUL assumption above.

Do the same for match_pathname. From now on, only pattern is required
to be NUL-terminated at the specified patternlen for both
match_{base,path}name.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 dir.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index 57394e4..30f5241 100644
--- a/dir.c
+++ b/dir.c

@@ -626,15 +626,18 @@ int match_basename(const char *basename, int basenamelen,
 		   int flags)
 {
 	if (prefix == patternlen) {
-		if (!strcmp_icase(pattern, basename))
+		if (patternlen == basenamelen &&
+		    !strncmp_icase(pattern, basename, basenamelen))
 			return 1;
 	} else if (flags & EXC_FLAG_ENDSWITH) {
 		if (patternlen - 1 <= basenamelen &&
-		    !strcmp_icase(pattern + 1,
-				  basename + basenamelen - patternlen + 1))
+		    !strncmp_icase(pattern + 1,
+				   basename + basenamelen - patternlen + 1,
+				   patternlen - 1))
 			return 1;
 	} else {
-		if (fnmatch_icase(pattern, basename, 0) == 0)
+		if (nwildmatch(pattern, basename, basenamelen,
+			       ignore_case ? WM_CASEFOLD : 0, NULL) == 0)
 			return 1;
 	}
 	return 0;

@@ -684,7 +687,7 @@ int match_pathname(const char *pathname, int pathlen,
 		namelen -= prefix;
 	}
 
-	return wildmatch(pattern, name,
+	return nwildmatch(pattern, name, namelen,
 			 WM_PATHNAME | (ignore_case ? WM_CASEFOLD : 0),
 			 NULL) == 0;
 }

-- 
1.8.2.82.gc24b958

[PATCH 4/4] attr.c: fix matching "subdir" without the trailing slash

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:56:30

The story goes back to 94bc671 (Add directory pattern matching to
attributes - 2012-12-08). Before this commit, directories are passed
to path_matches without the trailing slash. This is fine for matching
pattern "subdir" with "foo/subdir".

Patterns like "subdir/" (i.e. match _directory_ subdir) won't work
though. So paths are now passed to path_matches with the trailing
slash (i.e. "subdir/"). The trailing slash is used as the directory
indicator (similar to dtype in exclude case). This makes pattern
"subdir/" match directory "subdir/". Pattern "subdir" no longer match
subdir, which is now "subdir/".

As the trailing slash in pathname is the directory indicator, we do
not need to keep it in the pathname for matching. The trailing slash
should be turned to dtype "DT_DIR" and stripped out of pathname. This
keeps the code pattern similar to exclude.

The same applies for the pattern "subdir/". The trailing slash is
converted to flag EXC_FLAG_MUSTBEDIR and should not remain in the
pattern, as noted in parse_exclude_pattern(). prepare_attr_stack()
breaks this and keeps the trailing slash anyway.

To sum up, both patterns and pathnames should never have the trailing
slash when it comes to match_basename.

Reported-and-analyzed-by: Jeff King [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 attr.c                          | 11 +++++++++--
 t/t5002-archive-attr-pattern.sh |  6 ++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/attr.c b/attr.c
index 1818ba5..a95c837 100644
--- a/attr.c
+++ b/attr.c

@@ -256,7 +256,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 				      &res->u.pat.flags,
 				      &res->u.pat.nowildcardlen);
 		if (res->u.pat.flags & EXC_FLAG_MUSTBEDIR)
-			res->u.pat.patternlen++;
+			p[res->u.pat.patternlen] = '\0';
 		if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {
 			warning(_("Negative patterns are ignored in git attributes\n"
 				  "Use '\\!' for literal leading exclamation."));

@@ -665,9 +665,16 @@ static int path_matches(const char *pathname, int pathlen,
 {
 	const char *pattern = pat->pattern;
 	int prefix = pat->nowildcardlen;
+	int dtype;
+
+	if (pathlen && pathname[pathlen-1] == '/') {
+		dtype = DT_DIR;
+		pathlen--;
+	} else
+		dtype = DT_REG;
 
 	if ((pat->flags & EXC_FLAG_MUSTBEDIR) &&
-	    ((!pathlen) || (pathname[pathlen-1] != '/')))
+	    dtype != DT_DIR)
 		return 0;
 
 	if (pat->flags & EXC_FLAG_NODIR) {

diff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh
index 0c847fb..98ccc3c 100755
--- a/t/t5002-archive-attr-pattern.sh
+++ b/t/t5002-archive-attr-pattern.sh

@@ -27,6 +27,10 @@ test_expect_success 'setup' '
 	echo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&
 	git add ignored-only-if-dir &&
 
+	mkdir -p ignored-without-slash &&
+	echo ignored without slash >ignored-without-slash/foo &&
+	git add ignored-without-slash/foo &&
+	echo ignored-without-slash 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 &&

@@ -49,6 +53,8 @@ test_expect_exists	archive/not-ignored-dir/ignored-only-if-dir
 test_expect_exists	archive/not-ignored-dir/
 test_expect_missing	archive/ignored-only-if-dir/
 test_expect_missing	archive/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_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

-- 
1.8.2.82.gc24b958

Re: [PATCH 4/4] attr.c: fix matching "subdir" without the trailing slash

From: Duy Nguyen <hidden>
Date: 2016-06-15 22:56:30

On Mon, Mar 25, 2013 at 1:05 PM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:

The story goes back to 94bc671 (Add directory pattern matching to
attributes - 2012-12-08). Before this commit, directories are passed
to path_matches without the trailing slash. This is fine for matching
pattern "subdir" with "foo/subdir".

Patterns like "subdir/" (i.e. match _directory_ subdir) won't work
though. So paths are now passed to path_matches with the trailing
slash (i.e. "subdir/"). The trailing slash is used as the directory
indicator (similar to dtype in exclude case). This makes pattern
"subdir/" match directory "subdir/". Pattern "subdir" no longer match
subdir, which is now "subdir/".

As the trailing slash in pathname is the directory indicator, we do
not need to keep it in the pathname for matching. The trailing slash
should be turned to dtype "DT_DIR" and stripped out of pathname. This
keeps the code pattern similar to exclude.

On second thought, maybe we should not pass path "subdir/" at all.
Instead we create a fake dtype based on the trailing slash and pass it
down to attr.c:fill() -> path_matches(), just like how
last_exclude_matching_from_list() is called.
-- 
Duy

Re: [PATCH 4/4] attr.c: fix matching "subdir" without the trailing slash

From: Duy Nguyen <hidden>
Date: 2016-06-15 22:56:30

On Mon, Mar 25, 2013 at 02:20:31PM +0700, Duy Nguyen wrote:

On second thought, maybe we should not pass path "subdir/" at all.
Instead we create a fake dtype based on the trailing slash and pass it
down to attr.c:fill() -> path_matches(), just like how
last_exclude_matching_from_list() is called.

I was hoping to make a small patch, but as it turns out,
collect_all_attrs() takes a const path that contains the trailing
slash, we still need to ignore it in match_{base,path}name so the
whole series is still required. The only difference is in the final
patch, which is a bit longer:

-- 8< --

diff --git a/attr.c b/attr.c
index 1818ba5..0b2b716 100644
--- a/attr.c
+++ b/attr.c

@@ -256,7 +256,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 				      &res->u.pat.flags,
 				      &res->u.pat.nowildcardlen);
 		if (res->u.pat.flags & EXC_FLAG_MUSTBEDIR)
-			res->u.pat.patternlen++;
+			p[res->u.pat.patternlen] = '\0';
 		if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {
 			warning(_("Negative patterns are ignored in git attributes\n"
 				  "Use '\\!' for literal leading exclamation."));

@@ -659,15 +659,14 @@ static void prepare_attr_stack(const char *path, int dirlen)
 }
 
 static int path_matches(const char *pathname, int pathlen,
-			const char *basename,
+			const char *basename, int dtype,
 			const struct pattern *pat,
 			const char *base, int baselen)
 {
 	const char *pattern = pat->pattern;
 	int prefix = pat->nowildcardlen;
 
-	if ((pat->flags & EXC_FLAG_MUSTBEDIR) &&
-	    ((!pathlen) || (pathname[pathlen-1] != '/')))
+	if ((pat->flags & EXC_FLAG_MUSTBEDIR) && dtype != DT_DIR)
 		return 0;
 
 	if (pat->flags & EXC_FLAG_NODIR) {

@@ -706,7 +705,7 @@ static int fill_one(const char *what, struct match_attr *a, int rem)
 }
 
 static int fill(const char *path, int pathlen, const char *basename,
-		struct attr_stack *stk, int rem)
+		int dtype, struct attr_stack *stk, int rem)
 {
 	int i;
 	const char *base = stk->origin ? stk->origin : "";

@@ -715,7 +714,7 @@ static int fill(const char *path, int pathlen, const char *basename,
 		struct match_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);
 	}

@@ -755,11 +754,17 @@ static void collect_all_attrs(const char *path)
 	struct attr_stack *stk;
 	int i, pathlen, rem, dirlen;
 	const char *basename, *cp, *last_slash = NULL;
+	int dtype;
 
 	for (cp = path; *cp; cp++) {
 		if (*cp == '/' && cp[1])
 			last_slash = cp;
 	}
+	if (cp > path && cp[-1] == '/') {
+		dtype = DT_DIR;
+		cp--;
+	} else
+		dtype = DT_REG;
 	pathlen = cp - path;
 	if (last_slash) {
 		basename = last_slash + 1;

@@ -775,7 +780,7 @@ static void collect_all_attrs(const char *path)
 
 	rem = attr_nr;
 	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
-		rem = fill(path, pathlen, basename, stk, rem);
+		rem = fill(path, pathlen, basename, dtype, stk, rem);
 }
 
 int git_check_attr(const char *path, int num, struct git_attr_check *check)

diff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh
index 0c847fb..98ccc3c 100755
--- a/t/t5002-archive-attr-pattern.sh
+++ b/t/t5002-archive-attr-pattern.sh

@@ -27,6 +27,10 @@ test_expect_success 'setup' '
 	echo ignored-only-if-dir/ export-ignore >>.git/info/attributes &&
 	git add ignored-only-if-dir &&
 
+	mkdir -p ignored-without-slash &&
+	echo ignored without slash >ignored-without-slash/foo &&
+	git add ignored-without-slash/foo &&
+	echo ignored-without-slash 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 &&

@@ -49,6 +53,8 @@ test_expect_exists	archive/not-ignored-dir/ignored-only-if-dir
 test_expect_exists	archive/not-ignored-dir/
 test_expect_missing	archive/ignored-only-if-dir/
 test_expect_missing	archive/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_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
-- 8<--

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help