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

[Jeff King <peff@peff.net>]

On Fri, Mar 22, 2013 at 04:08:08PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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?

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.

---
diff --git a/attr.c b/attr.c
index e2f9377..bd00a78 100644
--- a/attr.c
+++ b/attr.c
@@ -663,20 +663,58 @@ static int path_matches(const char *pathname, int pathlen,
 {
 	const char *pattern = pat->pattern;
 	int prefix = pat->nowildcardlen;
+	char path_munge = 0;
+	char pattern_munge = 0;
+	int ret;
 
 	if ((pat->flags & EXC_FLAG_MUSTBEDIR) &&
 	    ((!pathlen) || (pathname[pathlen-1] != '/')))
 		return 0;
 
+	/*
+	 * Drop trailing slash from path, as we would want
+	 * an unadorned pattern like "foo" to match both the
+	 * file "foo" and the directory "foo/".
+	 */
+	if (pathlen && pathname[pathlen-1] == '/') {
+		pathlen--;
+
+		/*
+		 * The match_* functions, despite taking a string length, will
+		 * happily read all the way up to the NUL-terminating character.
+		 * So we must not only shrink pathlen, but munge the buffer
+		 * to NUL-terminate it.
+		 */
+		path_munge = pathname[pathlen];
+		((char *)pathname)[pathlen] = '\0';
+	}
+
+	/*
+	 * The pattern up to patternlen will not include a
+	 * trailing slash, but it may still be present in the string.
+	 * And since the match_* functions will read up to the NUL,
+	 * we need to terminate the buffer.
+	 */
+	pattern_munge = pattern[pat->patternlen];
+	((char *)pattern)[pat->patternlen] = '\0';
+
 	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);
+		ret = match_basename(basename,
+				     pathlen - (basename - pathname),
+				     pattern, prefix,
+				     pat->patternlen, pat->flags);
+	}
+	else {
+		ret = match_pathname(pathname, pathlen,
+				     base, baselen,
+				     pattern, prefix,
+				     pat->patternlen, pat->flags);
+	}
+
+	if (path_munge)
+		((char *)pathname)[pathlen] = path_munge;
+	((char *)pattern)[pat->patternlen] = pattern_munge;
+	return ret;
 }
 
 static int macroexpand_one(int attr_nr, int rem);
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