Skip to contentSkip to conversation

Attribute and conversion patches

PATCH In review Henrik Grubbström (Grubba) 3 participants 14 messages Jun 15, 2016 Documentation

lore
Share & tools
lore
#0 lore raw

v4 cover Attribute and conversion patches

These are some patches that are needed to get the support for attributes and especially the 'ident' attribute to a useable state.

Since last time the 'ident'-specific patch "Inhibit contraction of foreign $Id$ during stats." is gone, and replaced with the generic "Filter files that have changed only due to conversion changes.".

Henrik Grubbström (Grubba) (8):
convert: Safer handling of $Id$ contraction.
convert: Keep foreign $Id$ on checkout.
status: Added missing calls to diff_unmodified_pair() in
format_callbacks.
diff: Filter files that have changed only due to conversion changes.
convert: Added core.refilteronadd feature.
attr: Fixed debug output for macro expansion.
attr: Allow multiple changes to an attribute on the same line.
attr: Expand macros immediately when encountered.

 Documentation/config.txt |   12 +++++++
 attr.c                   |   38 ++++++++++++++--------
 cache.h                  |    2 +
 config.c                 |   10 ++++++
 convert.c                |   28 ++++++++++++++++-
 diff.c                   |   42 +++++++++++++++++++++++++
 environment.c            |    2 +
 sha1_file.c              |   57 ++++++++++++++++++++++++++++++++++
 t/t0003-attributes.sh    |   15 +++++++++
 t/t0021-conversion.sh    |   76 ++++++++++++++++++++++++++++++++++++++++++----
 wt-status.c              |    4 ++
 11 files changed, 264 insertions(+), 22 deletions(-)
#1 lore raw

PATCH v4 1/8 convert: Safer handling of $Id$ contraction.

The code to contract $Id:xxxxx$ strings could eat an arbitrary amount of source text if the terminating $ was lost. It now refuses to contract $Id:xxxxx$ strings spanning multiple lines.

---
The behaviour implemented by the patch is in line with what other VCSes that implement $Id$ do.

 convert.c             |   12 ++++++++++++
 t/t0021-conversion.sh |   16 ++++++++++------
 2 files changed, 22 insertions(+), 6 deletions(-)
convert.c +12 -0
index 4f8fcb7..239fa0a 100644
@@ -425,6 +425,8 @@ static int count_ident(const char *cp, unsigned long size)
 				cnt++;
 				break;
 			}
+			if (ch == '\n')
+				break;
 		}
 	}
 	return cnt;
@@ -455,6 +457,11 @@ static int ident_to_git(const char *path, const char *src, size_t len,
 			dollar = memchr(src + 3, '$', len - 3);
 			if (!dollar)
 				break;
+			if (memchr(src + 3, '\n', dollar - src - 3)) {
+				/* Line break before the next dollar. */
+				continue;
+			}
+
 			memcpy(dst, "Id$", 3);
 			dst += 3;
 			len -= dollar + 1 - src;
@@ -514,6 +521,11 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
 				break;
 			}
 
+			if (memchr(src + 3, '\n', dollar - src - 3)) {
+				/* Line break before the next dollar. */
+				continue;
+			}
+
 			len -= dollar + 1 - src;
 			src  = dollar + 1;
 		} else {
t/t0021-conversion.sh +10 -6
index 6cb8d60..29438c5 100755
@@ -65,17 +65,21 @@ test_expect_success expanded_in_repo '
 		echo "\$Id:NoSpaceAtFront \$"
 		echo "\$Id:NoSpaceAtEitherEnd\$"
 		echo "\$Id: NoTerminatingSymbol"
+		echo "\$Id: Foreign Commit With Spaces \$"
+		echo "\$Id: NoTerminatingSymbolAtEOF"
 	} > expanded-keywords &&
 
 	{
 		echo "File with expanded keywords"
-		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
-		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
-		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
-		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
-		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
-		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
+		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
+		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
+		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
+		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
+		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
+		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
 		echo "\$Id: NoTerminatingSymbol"
+		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
+		echo "\$Id: NoTerminatingSymbolAtEOF"
 	} > expected-output &&
 
 	git add expanded-keywords &&
Signature · 1 line
1.7.0.3.316.g33b5e
#2 lore raw

PATCH v4 2/8 convert: Keep foreign $Id$ on checkout.

In reply to Henrik Grubbström (Grubba)

If there are foreign $Id$ keywords in the repository, they are most likely there for a reason. Let's keep them on checkout (which is also what the documentation indicates). Foreign $Id$ keywords are now recognized by there being multiple space separated fields in $Id:xxxxx$.

---
The typical use case is for repositories that have been converted from some other VCS, where it is desirable to keep the old identifiers around until there's some other reason to alter the file.

Note that the comment about expanded Ids in the repository is obsoleted by the core.refilterOnDiff patch.

 convert.c             |   16 ++++++++++++++--
 t/t0021-conversion.sh |    2 +-
 2 files changed, 15 insertions(+), 3 deletions(-)
convert.c +14 -2
index 239fa0a..5a0b7fb 100644
@@ -477,7 +477,7 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
                              struct strbuf *buf, int ident)
 {
 	unsigned char sha1[20];
-	char *to_free = NULL, *dollar;
+	char *to_free = NULL, *dollar, *spc;
 	int cnt;
 
 	if (!ident)
@@ -513,7 +513,10 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
 		} else if (src[2] == ':') {
 			/*
 			 * It's possible that an expanded Id has crept its way into the
-			 * repository, we cope with that by stripping the expansion out
+			 * repository, we cope with that by stripping the expansion out.
+			 * This is probably not a good idea, since it will cause changes
+			 * on checkout, which won't go away by stash, but let's keep it
+			 * for git-style ids.
 			 */
 			dollar = memchr(src + 3, '$', len - 3);
 			if (!dollar) {
@@ -526,6 +529,15 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
 				continue;
 			}
 
+			spc = memchr(src + 4, ' ', dollar - src - 4);
+			if (spc && spc < dollar-1) {
+				/* There are spaces in unexpected places.
+				 * This is probably an id from some other
+				 * versioning system. Keep it for now.
+				 */
+				continue;
+			}
+
 			len -= dollar + 1 - src;
 			src  = dollar + 1;
 		} else {
t/t0021-conversion.sh +1 -1
index 29438c5..828e35b 100755
@@ -78,7 +78,7 @@ test_expect_success expanded_in_repo '
 		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
 		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
 		echo "\$Id: NoTerminatingSymbol"
-		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
+		echo "\$Id: Foreign Commit With Spaces \$"
 		echo "\$Id: NoTerminatingSymbolAtEOF"
 	} > expected-output &&
 
Signature · 1 line
1.7.0.3.316.g33b5e
#3 lore raw

PATCH v4 3/8 status: Added missing calls to diff_unmodified_pair() in format_callbacks.

In reply to Henrik Grubbström (Grubba)

The diff_queue_struct provided by diff_flush() is raw, and needs to be filtered through diff_unmodified_pair() before being used. This is already done by most of the other functions operating on diff_queue_struct called by diff_flush().

---
For diff_modified_pair() to be able to do its job, it needs to be called...

 wt-status.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
wt-status.c +4 -0
index 8ca59a2..e661225 100644
@@ -229,6 +229,8 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 		struct wt_status_change_data *d;
 
 		p = q->queue[i];
+		if (diff_unmodified_pair(p))
+			continue;
 		it = string_list_insert(p->one->path, &s->change);
 		d = it->util;
 		if (!d) {
@@ -276,6 +278,8 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,
 		struct wt_status_change_data *d;
 
 		p = q->queue[i];
+		if (diff_unmodified_pair(p))
+			continue;
 		it = string_list_insert(p->two->path, &s->change);
 		d = it->util;
 		if (!d) {
Signature · 1 line
1.7.0.3.316.g33b5e
#4 lore raw

Re: [PATCH v4 3/8] status: Added missing calls to diff_unmodified_pair() in format_callbacks.

Quoted 4 lines · Henrik Grubbström (Grubba)

The diff_queue_struct provided by diff_flush() is raw, and needs to be filtered through diff_unmodified_pair() before being used. This is already done by most of the other functions operating on diff_queue_struct called by diff_flush().

That is true but only if you are letting the diff front-end to feed unmodified pairs to begin with, e.g. --find-copies-harder. I don't think the internal caller in wt-status does that.

I don't think the patch is wrong nor it would hurt, but I am puzzled why you needed this patch.

Quoted 2 lines
 wt-status.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
#5 lore raw

Re: [PATCH v4 3/8] status: Added missing calls to diff_unmodified_pair() in format_callbacks.

In reply to Henrik Grubbström (Grubba)
Quoted 4 lines · Henrik Grubbström (Grubba)

The diff_queue_struct provided by diff_flush() is raw, and needs to be filtered through diff_unmodified_pair() before being used. This is already done by most of the other functions operating on diff_queue_struct called by diff_flush().

That is true but only if you are letting the diff front-end to feed unmodified pairs to begin with, e.g. --find-copies-harder. I don't think the internal caller in wt-status does that.

I don't think the patch is wrong nor it would hurt, but I am puzzled why you needed this patch.

Quoted 2 lines
 wt-status.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
#6 lore raw

Re: [PATCH v4 3/8] status: Added missing calls to diff_unmodified_pair() in format_callbacks.

Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Henrik Grubbström (Grubba)

The diff_queue_struct provided by diff_flush() is raw, and needs to be filtered through diff_unmodified_pair() before being used. This is already done by most of the other functions operating on diff_queue_struct called by diff_flush().

Quoted 6 lines

That is true but only if you are letting the diff front-end to feed unmodified pairs to begin with, e.g. --find-copies-harder. I don't think the internal caller in wt-status does that.

I don't think the patch is wrong nor it would hurt, but I am puzzled why you needed this patch.

Well, it's a prerequisite for the diff: Filter files that have changed... patch, albeit apparently not sufficient (yet).

--
Henrik Grubbström [email protected]
Roxen Internet Software AB [email protected]

#7 lore raw

PATCH v4 4/8 diff: Filter files that have changed only due to conversion changes.

In reply to Henrik Grubbström (Grubba)

When the conversion filter for a file is changed, files may get listed as modified even though the user has not made any changes to them. This patch adds a configuration option 'core.refilterOnDiff', which performs an extra renormalization pass to filter out such files.

---
The typical reason to enable this option is when you have lots of files that have been affected by a configuration change (eg crlf convention or ident expansion), but don't want to recommit the otherwise unchanged files just to get them on canonic form in the repository.

 Documentation/config.txt |    6 ++++++
 cache.h                  |    1 +
 config.c                 |    5 +++++
 diff.c                   |   42 ++++++++++++++++++++++++++++++++++++++++++
 environment.c            |    1 +
 t/t0021-conversion.sh    |   25 +++++++++++++++++++++++++
 6 files changed, 80 insertions(+), 0 deletions(-)
Documentation/config.txt +6 -0
index 06b2f82..4eb3ab3 100644
@@ -535,6 +535,12 @@ core.sparseCheckout::
 	Enable "sparse checkout" feature. See section "Sparse checkout" in
 	linkgit:git-read-tree[1] for more information.
 
+core.refilterOnDiff::
+	Enable "refilter on diff" feature. This causes source files that
+	have only changed from the committed version as a side effect of
+	a conversion filter change to be filtered from the output of eg
+	linkgit:git-status[1] and linkgit:git-diff[1].
+
 add.ignore-errors::
 	Tells 'git add' to continue adding files when some files cannot be
 	added due to indexing errors. Equivalent to the '--ignore-errors'
cache.h +1 -0
index 6dcb100..cd2bca4 100644
@@ -552,6 +552,7 @@ extern int read_replace_refs;
 extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
+extern int core_refilter_on_diff;
 
 enum safe_crlf {
 	SAFE_CRLF_FALSE = 0,
config.c +5 -0
index 6963fbe..4954797 100644
@@ -523,6 +523,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if(!strcmp(var, "core.refilterondiff")) {
+		core_refilter_on_diff = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff.c +42 -0

54 lines not on this page · raw message

environment.c +1 -0
index 876c5e5..1b52bed 100644
@@ -52,6 +52,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_apply_sparse_checkout;
+int core_refilter_on_diff;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
t/t0021-conversion.sh +25 -0
index 828e35b..48ae8bb 100755
@@ -93,4 +93,29 @@ test_expect_success expanded_in_repo '
 	cmp expanded-keywords expected-output
 '
 
+# Check that files containing keywords with proper markup aren't marked
+# as modified on checkout when core.refilterOnDiff is set.
+test_expect_success keywords_not_modified '
+	{
+		echo "File with foreign keywords"
+		echo "\$Id\$"
+		echo "\$Id: NoTerminatingSymbol"
+		echo "\$Id: Foreign Commit With Spaces \$"
+		echo "\$Id: GitCommitId \$"
+		echo "\$Id: NoTerminatingSymbolAtEOF"
+	} > expanded-keywords2 &&
+
+	git add expanded-keywords2 &&
+	git commit -m "File with keywords expanded" &&
+
+	echo "expanded-keywords2 ident" >> .gitattributes &&
+
+	rm -f expanded-keywords2 &&
+	git checkout -- expanded-keywords2 &&
+	test "x`git status --porcelain -- expanded-keywords2`" = \
+             "x M expanded-keywords2" &&
+	git config --add core.refilterondiff true &&
+	test "x`git status --porcelain -- expanded-keywords2`" = x
+'
+
 test_done
Signature · 1 line
1.7.0.3.316.g33b5e
#8 lore raw

Re: [PATCH v4 4/8] diff: Filter files that have changed only due to conversion changes.

Quoted 6 lines · Henrik Grubbström (Grubba)

When the conversion filter for a file is changed, files may get listed as modified even though the user has not made any changes to them. This patch adds a configuration option 'core.refilterOnDiff', which performs an extra renormalization pass to filter out such files.

Does this really have to be done for every invocation of diff?

It is a problem worthy of a clean solution that changing the filtering options makes files that are not really different (from the end user's point of view).

But the problem feels very similar to the issue that touching the inode information would make the cached stat information in the index invalid and plumbing commands such as "diff-files" would report phantom changes.

And the way we solve the latter issue without undue overhead for all command invocations is with "update-index --refresh" (either run directly as a command inside Porcelain scripts that work with the plumbing, or internally by calling refresh_cache() API in the C implementations of Porcelain commands). Hence:

	$ cat Makefile >Makefile+
        $ mv Makefile+ Makefile
        $ git diff-files --name-only
        Makefile
        $ git update-index --refresh
        $ git diff-files --name-only

once we spend cycles to revalidate the cached information in the index, subsequent commands can trust the validity information without recomputing the phantom differences that do not exist over and over.

I wonder if we can solve this in a similar way. Especially, because changing filtering options like the core.crlf settings is a one-off event that is done even rarely than "touch Makefile", it doesn't feel right to add an extra configuration that makes people pay the penalty during everyday use just in case such a one-off event might have happened.

Quoted 4 lines

The typical reason to enable this option is when you have lots of files that have been affected by a configuration change (eg crlf convention or ident expansion), but don't want to recommit the otherwise unchanged files just to get them on canonic form in the repository.

Of course you do not want to re-commit. If however these files that are unchanged from the end-user's point of view can be re-checked out safely, then that would be similar to what "update-index --refresh" does for paths that are stat-dirty.

#9 lore raw

Re: [PATCH v4 4/8] diff: Filter files that have changed only due to conversion changes.

Quoted 7 lines · Junio C Hamano
Quoted 5 lines · Henrik Grubbström (Grubba)

When the conversion filter for a file is changed, files may get listed as modified even though the user has not made any changes to them. This patch adds a configuration option 'core.refilterOnDiff', which performs an extra renormalization pass to filter out such files.

Quoted 7 lines
Quoted 1 line

Does this really have to be done for every invocation of diff?

But the problem feels very similar to the issue that touching the inode information would make the cached stat information in the index invalid and plumbing commands such as "diff-files" would report phantom changes.

True, storing this information in the index is a much better approach.

Quoted 4 lines

Of course you do not want to re-commit. If however these files that are unchanged from the end-user's point of view can be re-checked out safely, then that would be similar to what "update-index --refresh" does for paths that are stat-dirty.

I now have a tentative set of patches implementing this.

--
Henrik Grubbström [email protected]
Roxen Internet Software AB [email protected]

#10 lore raw

PATCH v4 5/8 convert: Added core.refilteronadd feature.

In reply to Henrik Grubbström (Grubba)

When having $Id$ tags in other versioning systems, it is customary to recalculate the tags in the source on commit or equvivalent. This commit adds a configuration option to git that causes source files to pass through a conversion roundtrip when added to the index.

---
 Documentation/config.txt |    6 +++++
 cache.h                  |    1 +
 config.c                 |    5 ++++
 environment.c            |    1 +
 sha1_file.c              |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 t/t0021-conversion.sh    |   35 ++++++++++++++++++++++++++++
 6 files changed, 105 insertions(+), 0 deletions(-)
Documentation/config.txt +6 -0
index 4eb3ab3..5225047 100644
@@ -535,6 +535,12 @@ core.sparseCheckout::
 	Enable "sparse checkout" feature. See section "Sparse checkout" in
 	linkgit:git-read-tree[1] for more information.
 
+core.refilterOnAdd::
+	Enable "refilter on add" feature. This causes source files to be
+	behave as if they were checked out after a linkgit:git-add[1].
+	This is typically usefull if eg the `ident` attribute is active,
+	in which case the $Id$ tags will be updated.
+
 core.refilterOnDiff::
 	Enable "refilter on diff" feature. This causes source files that
 	have only changed from the committed version as a side effect of
cache.h +1 -0
index cd2bca4..1bd8484 100644
@@ -552,6 +552,7 @@ extern int read_replace_refs;
 extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
+extern int core_refilter_on_add;
 extern int core_refilter_on_diff;
 
 enum safe_crlf {
config.c +5 -0
index 4954797..d284897 100644
@@ -523,6 +523,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.refilteronadd")) {
+		core_refilter_on_add = git_config_bool(var, value);
+		return 0;
+	}
+
 	if(!strcmp(var, "core.refilterondiff")) {
 		core_refilter_on_diff = git_config_bool(var, value);
 		return 0;
environment.c +1 -0
index 1b52bed..4b4c966 100644
@@ -52,6 +52,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_apply_sparse_checkout;
+int core_refilter_on_add;
 int core_refilter_on_diff;
 
 /* Parallel index stat data preload? */
sha1_file.c +57 -0

69 lines not on this page · raw message

t/t0021-conversion.sh +35 -0
index 48ae8bb..9ddbde3 100755
@@ -118,4 +118,39 @@ test_expect_success keywords_not_modified '
 	test "x`git status --porcelain -- expanded-keywords2`" = x
 '
 
+# Check that keywords are expanded on add when refilter is enabled.
+test_expect_success expanded_on_add '
+	git config --add core.refilteronadd true
+
+	echo "expanded-keywords3 ident" >> .gitattributes &&
+
+	{
+		echo "File with keyword"
+		echo "\$Id\$"
+	} > expanded-keywords3 &&
+
+	{
+		echo "File with keyword"
+		echo "\$Id: 0661580d6f976fe7cc1e4512f8db3f2ddb8d93cc \$"
+	} > expected-output3 &&
+
+	git add expanded-keywords3 &&
+
+	cat expanded-keywords3 &&
+	cmp expanded-keywords3 expected-output3
+'
+
+# Check that keywords are expanded on commit when refilter is enabled.
+test_expect_success expanded_on_commit '
+	{
+		echo "File with keyword"
+		echo "\$Id\$"
+	} > expanded-keywords3 &&
+
+	git commit -m "File with keyword" expanded-keywords3 &&
+
+	cat expanded-keywords3 &&
+	cmp expanded-keywords3 expected-output3
+'
+
 test_done
Signature · 1 line
1.7.0.3.316.g33b5e
#11 lore raw

PATCH v4 6/8 attr: Fixed debug output for macro expansion.

In reply to Henrik Grubbström (Grubba)

When debug_set() was called during macro expansion, it received a pointer to a struct git_attr rather than a string.

---
 attr.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
attr.c +3 -1
index f5346ed..5c6464e 100644
@@ -605,7 +605,9 @@ static int fill_one(const char *what, struct match_attr *a, int rem)
 		const char *v = a->state[i].setto;
 
 		if (*n == ATTR__UNKNOWN) {
-			debug_set(what, a->u.pattern, attr, v);
+			debug_set(what,
+				  a->is_macro?a->u.attr->name:a->u.pattern,
+				  attr, v);
 			*n = v;
 			rem--;
 		}
Signature · 1 line
1.7.0.3.316.g33b5e
#12 lore raw

PATCH v4 7/8 attr: Allow multiple changes to an attribute on the same line.

In reply to Henrik Grubbström (Grubba)

When using macros it isn't inconceivable to have an attribute being set by a macro, and then being reset explicitly.

---
NB: Currently the tests in the testsuite patch will have the opposite meaning, which is probably not what the user expects, and is contrary to the documentation.

 attr.c                |    2 +-
 t/t0003-attributes.sh |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletions(-)
attr.c +1 -1
index 5c6464e..968fb8b 100644
@@ -599,7 +599,7 @@ static int fill_one(const char *what, struct match_attr *a, int rem)
 	struct git_attr_check *check = check_all_attr;
 	int i;
 
-	for (i = 0; 0 < rem && i < a->num_attr; i++) {
+	for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
 		struct git_attr *attr = a->state[i].attr;
 		const char **n = &(check[attr->attr_nr].value);
 		const char *v = a->state[i].setto;
t/t0003-attributes.sh +6 -0
index 1c77192..bd9c8de 100755
@@ -22,6 +22,8 @@ test_expect_success 'setup' '
 	(
 		echo "f	test=f"
 		echo "a/i test=a/i"
+		echo "onoff test -test"
+		echo "offon -test test"
 	) >.gitattributes &&
 	(
 		echo "g test=a/g" &&
@@ -44,6 +46,8 @@ test_expect_success 'attribute test' '
 	attr_check b/g unspecified &&
 	attr_check a/b/h a/b/h &&
 	attr_check a/b/d/g "a/b/d/*"
+	attr_check onoff unset
+	attr_check offon set
 
 '
 
@@ -58,6 +62,8 @@ a/b/g: test: a/b/g
 b/g: test: unspecified
 a/b/h: test: a/b/h
 a/b/d/g: test: a/b/d/*
+onoff: test: unset
+offon: test: set
 EOF
 
 	sed -e "s/:.*//" < expect | git check-attr --stdin test > actual &&
Signature · 1 line
1.7.0.3.316.g33b5e
#13 lore raw

PATCH v4 8/8 attr: Expand macros immediately when encountered.

In reply to Henrik Grubbström (Grubba)

When using macros it is otherwise hard to know whether an attribute set by the macro should override an already set attribute. Consider the following .gitattributes file:

[attr]mybinary binary -ident
* ident
foo.bin mybinary
bar.bin mybinary ident

Without this patch both foo.bin and bar.bin will have the ident attribute set, which is probably not what the user expects. With this patch foo.bin will have an unset ident attribute, while bar.bin will have it set.

---
FYI: The use case I attempted was:

*.c crlf ident
[attr]foreign_ident -ident block_commit=Remove-foreign_ident-attribute.
src/version.c foreign_ident

Which currently causes src/version.c to have the ident attribute set.

 attr.c                |   32 ++++++++++++++++++++------------
 t/t0003-attributes.sh |    9 +++++++++
 2 files changed, 29 insertions(+), 12 deletions(-)
attr.c +20 -12
index 968fb8b..f90bb8e 100644
@@ -594,6 +594,8 @@ static int path_matches(const char *pathname, int pathlen,
 	return fnmatch(pattern, pathname + baselen, FNM_PATHNAME) == 0;
 }
 
+static int macroexpand_one(int attr_nr, int rem);
+
 static int fill_one(const char *what, struct match_attr *a, int rem)
 {
 	struct git_attr_check *check = check_all_attr;
@@ -610,6 +612,7 @@ static int fill_one(const char *what, struct match_attr *a, int rem)
 				  attr, v);
 			*n = v;
 			rem--;
+			rem = macroexpand_one(attr->attr_nr, rem);
 		}
 	}
 	return rem;
@@ -631,19 +634,27 @@ static int fill(const char *path, int pathlen, struct attr_stack *stk, int rem)
 	return rem;
 }
 
-static int macroexpand(struct attr_stack *stk, int rem)
+static int macroexpand_one(int attr_nr, int rem)
 {
+	struct attr_stack *stk;
+	struct match_attr *a = NULL;
 	int i;
-	struct git_attr_check *check = check_all_attr;
 
-	for (i = stk->num_matches - 1; 0 < rem && 0 <= i; i--) {
-		struct match_attr *a = stk->attrs[i];
-		if (!a->is_macro)
-			continue;
-		if (check[a->u.attr->attr_nr].value != ATTR__TRUE)
-			continue;
+	if (check_all_attr[attr_nr].value != ATTR__TRUE)
+		return rem;
+
+	for (stk = attr_stack; !a && stk; stk = stk->prev)
+		for (i = stk->num_matches - 1; !a && 0 <= i; i--) {
+			struct match_attr *ma = stk->attrs[i];
+			if (!ma->is_macro)
+				continue;
+			if (ma->u.attr->attr_nr == attr_nr)
+				a = ma;
+		}
+
+	if (a)
 		rem = fill_one("expand", a, rem);
-	}
+
 	return rem;
 }
 
@@ -668,9 +679,6 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)
 	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
 		rem = fill(path, pathlen, stk, rem);
 
-	for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
-		rem = macroexpand(stk, rem);
-
 	for (i = 0; i < num; i++) {
 		const char *value = check_all_attr[check[i].attr->attr_nr].value;
 		if (value == ATTR__UNKNOWN)
t/t0003-attributes.sh +9 -0
index bd9c8de..53bd7fc 100755
@@ -20,10 +20,12 @@ test_expect_success 'setup' '
 
 	mkdir -p a/b/d a/c &&
 	(
+		echo "[attr]notest !test"
 		echo "f	test=f"
 		echo "a/i test=a/i"
 		echo "onoff test -test"
 		echo "offon -test test"
+		echo "no notest"
 	) >.gitattributes &&
 	(
 		echo "g test=a/g" &&
@@ -32,6 +34,7 @@ test_expect_success 'setup' '
 	(
 		echo "h test=a/b/h" &&
 		echo "d/* test=a/b/d/*"
+		echo "d/yes notest"
 	) >a/b/.gitattributes
 
 '
@@ -48,6 +51,9 @@ test_expect_success 'attribute test' '
 	attr_check a/b/d/g "a/b/d/*"
 	attr_check onoff unset
 	attr_check offon set
+	attr_check no unspecified
+	attr_check a/b/d/no "a/b/d/*"
+	attr_check a/b/d/yes unspecified
 
 '
 
@@ -64,6 +70,9 @@ a/b/h: test: a/b/h
 a/b/d/g: test: a/b/d/*
 onoff: test: unset
 offon: test: set
+no: test: unspecified
+a/b/d/no: test: a/b/d/*
+a/b/d/yes: test: unspecified
 EOF
 
 	sed -e "s/:.*//" < expect | git check-attr --stdin test > actual &&
Signature · 1 line
1.7.0.3.316.g33b5e