git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 0/4] attribute regression fix for maint-1.8.1 and upward

From
Duy Nguyen <pclouds@gmail.com>
Date
Mar 27, 2013, 01:13 UTC
Message-ID
<20130327011309.GA10946@duynguyen-vnpc.dek-tpc.internal>
In-Reply-To
<1364323171-20299-1-git-send-email-gitster@pobox.com>
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< --
diff --git a/archive.c b/archive.c
index 93e00bb..ab811b3 100644
--- a/archive.c
+++ b/archive.c
@@ -112,7 +112,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
 	write_archive_entry_fn_t write_entry = c->write_entry;
 	struct git_attr_check check[2];
 	const char *path_without_prefix;
-	int err;
+	int err, dtype;
 
 	args->convert = 0;
 	strbuf_reset(&path);
@@ -120,18 +120,18 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
 	strbuf_add(&path, args->base, args->baselen);
 	strbuf_add(&path, base, baselen);
 	strbuf_addstr(&path, filename);
-	if (S_ISDIR(mode) || S_ISGITLINK(mode))
-		strbuf_addch(&path, '/');
+	dtype = S_ISDIR(mode) || S_ISGITLINK(mode) ? DT_DIR : DT_REG;
 	path_without_prefix = path.buf + args->baselen;
 
 	setup_archive_check(check);
-	if (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {
+	if (!git_check_attr(path_without_prefix, dtype, ARRAY_SIZE(check), check)) {
 		if (ATTR_TRUE(check[0].value))
 			return 0;
 		args->convert = ATTR_TRUE(check[1].value);
 	}
 
 	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
+		strbuf_addch(&path, '/');
 		if (args->verbose)
 			fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
 		err = write_entry(args, sha1, path.buf, path.len, mode);
diff --git a/attr.c b/attr.c
index e2f9377..ab6422c 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)
+			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."));
@@ -657,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) {
@@ -704,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 : "";
@@ -713,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);
 	}
@@ -748,20 +749,17 @@ static int macroexpand_one(int attr_nr, int rem)
  * Collect all attributes for path into the array pointed to by
  * check_all_attr.
  */
-static void collect_all_attrs(const char *path)
+static void collect_all_attrs(const char *path, int dtype)
 {
 	struct attr_stack *stk;
 	int i, pathlen, rem, dirlen;
-	const char *basename, *cp, *last_slash = NULL;
+	const char *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;
@@ -773,14 +771,14 @@ 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)
+int git_check_attr(const char *path, int dtype, int num, struct git_attr_check *check)
 {
 	int i;
 
-	collect_all_attrs(path);
+	collect_all_attrs(path, dtype);
 
 	for (i = 0; i < num; i++) {
 		const char *value = check_all_attr[check[i].attr->attr_nr].value;
@@ -796,7 +794,7 @@ int git_all_attrs(const char *path, int *num, struct git_attr_check **check)
 {
 	int i, count, j;
 
-	collect_all_attrs(path);
+	collect_all_attrs(path, DT_REG);
 
 	/* Count the number of attributes that are set. */
 	count = 0;
diff --git a/attr.h b/attr.h
index 8b08d33..ce39c9c 100644
--- a/attr.h
+++ b/attr.h
@@ -36,7 +36,7 @@ struct git_attr_check {
  */
 char *git_attr_name(struct git_attr *);
 
-int git_check_attr(const char *path, int, struct git_attr_check *);
+int git_check_attr(const char *path, int, int, struct git_attr_check *);
 
 /*
  * Retrieve all attributes that apply to the specified path.  *num
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 075d01d..261e57d 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -49,7 +49,7 @@ static void check_attr(const char *prefix, int cnt,
 	char *full_path =
 		prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
 	if (check != NULL) {
-		if (git_check_attr(full_path, cnt, check))
+		if (git_check_attr(full_path, DT_REG, cnt, check))
 			die("git_check_attr died");
 		output_attr(cnt, check, file);
 	} else {
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f069462..7a77288 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -894,7 +894,7 @@ static int no_try_delta(const char *path)
 	struct git_attr_check check[1];
 
 	setup_delta_attr_check(check);
-	if (git_check_attr(path, ARRAY_SIZE(check), check))
+	if (git_check_attr(path, DT_REG, ARRAY_SIZE(check), check))
 		return 0;
 	if (ATTR_FALSE(check->value))
 		return 1;
diff --git a/convert.c b/convert.c
index 3520252..3f09cbb 100644
--- a/convert.c
+++ b/convert.c
@@ -755,7 +755,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
 		git_config(read_convert_config, NULL);
 	}
 
-	if (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {
+	if (!git_check_attr(path, DT_REG, NUM_CONV_ATTRS, ccheck)) {
 		ca->crlf_action = git_path_check_crlf(path, ccheck + 4);
 		if (ca->crlf_action == CRLF_GUESS)
 			ca->crlf_action = git_path_check_crlf(path, ccheck + 0);
diff --git a/ll-merge.c b/ll-merge.c
index fb61ea6..1944392 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -342,7 +342,7 @@ static int git_path_check_merge(const char *path, struct git_attr_check check[2]
 		check[0].attr = git_attr("merge");
 		check[1].attr = git_attr("conflict-marker-size");
 	}
-	return git_check_attr(path, 2, check);
+	return git_check_attr(path, DT_REG, 2, check);
 }
 
 static void normalize_file(mmfile_t *mm, const char *path)
@@ -399,7 +399,7 @@ int ll_merge_marker_size(const char *path)
 
 	if (!check.attr)
 		check.attr = git_attr("conflict-marker-size");
-	if (!git_check_attr(path, 1, &check) && check.value) {
+	if (!git_check_attr(path, DT_REG, 1, &check) && check.value) {
 		marker_size = atoi(check.value);
 		if (marker_size <= 0)
 			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
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
diff --git a/userdiff.c b/userdiff.c
index ea43a03..fd4f576 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -260,7 +260,7 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)
 
 	if (!path)
 		return NULL;
-	if (git_check_attr(path, 1, &check))
+	if (git_check_attr(path, DT_REG, 1, &check))
 		return NULL;
 
 	if (ATTR_TRUE(check.value))
diff --git a/ws.c b/ws.c
index b498d75..34c2145 100644
--- a/ws.c
+++ b/ws.c
@@ -88,7 +88,7 @@ unsigned whitespace_rule(const char *pathname)
 	struct git_attr_check attr_whitespace_rule;
 
 	setup_whitespace_attr_check(&attr_whitespace_rule);
-	if (!git_check_attr(pathname, 1, &attr_whitespace_rule)) {
+	if (!git_check_attr(pathname, DT_REG, 1, &attr_whitespace_rule)) {
 		const char *value;
 
 		value = attr_whitespace_rule.value;
-- 8< --
Previous: Jeff KingNext: Junio C Hamano
Message 26 of 66 in “[regression?] trailing slash required in .gitattributes”
  1. Jeff KingMar 19, 2013
  2. Junio C HamanoMar 19, 2013
  3. Jeff KingMar 19, 2013
  4. Jeff KingMar 22, 2013
  5. Junio C HamanoMar 22, 2013
  6. Jeff KingMar 23, 2013
  7. Junio C HamanoMar 24, 2013
  8. 0/4 attribute regression fix for maint-1.8.1 and upwardJunio C Hamano, Mar 26, 2013
  9. 1/4 attr.c::path_matches(): the basename is part of the pathnameJunio C Hamano, Mar 26, 2013
  10. Jeff KingMar 26, 2013
  11. Duy NguyenMar 27, 2013
  12. 2/4 dir.c::match_basename(): pay attention to the length of string parametersJunio C Hamano, Mar 26, 2013
  13. Jeff KingMar 26, 2013
  14. Jeff KingMar 26, 2013
  15. Junio C HamanoMar 26, 2013
  16. Jeff KingMar 26, 2013
  17. Junio C HamanoMar 26, 2013
  18. Jeff KingMar 27, 2013
  19. 3/4 attr.c::path_matches(): special case paths that end with a slashJunio C Hamano, Mar 26, 2013
  20. Jeff KingMar 26, 2013
  21. Jeff KingMar 26, 2013
  22. Duy NguyenMar 27, 2013
  23. Jeff KingMar 28, 2013
  24. 4/4 make sure a pattern without trailing slash matches a directoryJunio C Hamano, Mar 26, 2013
  25. Jeff KingMar 26, 2013
  26. Duy NguyenMar 27, 2013
  27. Junio C HamanoMar 27, 2013
  28. Duy NguyenMar 27, 2013
  29. 0/6 attribute regression fix for maint-1.8.1 and upwardJeff King, Mar 28, 2013
  30. 1/6 attr.c::path_matches(): the basename is part of the pathnameJeff King, Mar 28, 2013
  31. 2/6 dir.c::match_basename(): pay attention to the length of string parametersJeff King, Mar 28, 2013
  32. Jeff KingMar 28, 2013
  33. Jeff KingMar 28, 2013
  34. Junio C HamanoMar 28, 2013
  35. Duy NguyenMar 28, 2013
  36. Duy NguyenMar 29, 2013
  37. Jeff KingMar 29, 2013
  38. Junio C HamanoMar 29, 2013
  39. 3/6 dir.c::match_pathname(): adjust patternlen when shifting patternJeff King, Mar 28, 2013
  40. 4/6 dir.c::match_pathname(): pay attention to the length of string parametersJeff King, Mar 28, 2013
  41. Junio C HamanoMar 28, 2013
  42. Duy NguyenMar 29, 2013
  43. Duy NguyenMar 29, 2013
  44. Torsten BögershausenMar 29, 2013
  45. Duy NguyenMar 29, 2013
  46. Jeff KingMar 29, 2013
  47. Duy NguyenMar 29, 2013
  48. Junio C HamanoMar 29, 2013
  49. Jeff KingMar 29, 2013
  50. Junio C HamanoMar 29, 2013
  51. Jeff KingMar 29, 2013
  52. Duy NguyenMar 30, 2013
  53. 5/6 attr.c::path_matches(): special case paths that end with a slashJeff King, Mar 28, 2013
  54. 6/6 t: check that a pattern without trailing slash matches a directoryJeff King, Mar 28, 2013
  55. Eric SunshineMar 28, 2013
  56. Jeff KingMar 28, 2013
  57. Duy NguyenMar 23, 2013
  58. Duy NguyenMar 23, 2013
  59. 0/4 attr directory matching regressionNguyễn Thái Ngọc Duy, Mar 25, 2013
  60. 1/4 wildmatch: do not require "text" to be NUL-terminatedNguyễn Thái Ngọc Duy, Mar 25, 2013
  61. 2/4 attr.c: fix pattern{,len} inconsistency in struct match_attrNguyễn Thái Ngọc Duy, Mar 25, 2013
  62. 3/4 dir.c: make match_{base,path}name respect {basename,path}lenNguyễn Thái Ngọc Duy, Mar 25, 2013
  63. 4/4 attr.c: fix matching "subdir" without the trailing slashNguyễn Thái Ngọc Duy, Mar 25, 2013
  64. Duy NguyenMar 25, 2013
  65. Duy NguyenMar 25, 2013
  66. Junio C HamanoMar 26, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.