Bug v1.9.3: Spurious whitespace warnings when using git apply with the --exclude option

Bug v1.9.3: Spurious whitespace warnings when using git apply with the --exclude option

[Peyton Randolph]

Bug v1.9.3: Spurious whitespace warnings when using git apply with the --exclude option

REPRO STEPS:
1. Create a patch PATCH that modifies two files, A and B. Make sure the modifications to file A add whitespace errors and the modifications to file B do not.
2. Apply that patch to a repo using git apply PATCH --exclude A

ACTUAL RESULT:
git apply outputs a "warning: n lines add whitespace errors.” where n is the number of lines with whitespace errors in A. 

EXPECTED RESULT:
No warning about adding whitespace errors because no whitespace errors are added to the tree since A is excluded and B has no whitespace errors.

Re: Bug v1.9.3: Spurious whitespace warnings when using git apply with the --exclude option

[Junio C Hamano]

Peyton Randolph <prandolph@apple.com> writes:

> Bug v1.9.3: Spurious whitespace warnings when using git apply with the --exclude option
> ...
> EXPECTED RESULT:
> No warning about adding whitespace errors because no whitespace
> errors are added to the tree since A is excluded and B has no
> whitespace errors.

By reading the way the code is structured, I would expect (I didn't
check) that all versions of apply that supports whitespace error
detection with include/exclude option to share this symptom, and
this bug is not specific to 1.9.3.

In order to decide if a hunk with a new whitespace error should be
reported, the program needs to parse the patch input to at least
find out what path the part being parsed is to be applied, and the
whitespace error detection and reporting is done when the patch
input is parsed.  After the part that applies to one path is fully
parsed, it then checks to see if the path is included/excluded and
omits it from the --stat output and also actual application.

That part of the code needs to be restructured so that at least
error reporting (and preferrably error detection) is not done when
the path is known to be excluded.  Let me see if this can be easily
done.

Thanks.

[PATCH 0/3] Two fixes to "git apply"

[Junio C Hamano]

This started as an attempt to fix a long-time bug, in which a
partial "git apply" with --exclude/--include still complained about
whitespace breakage in a part that was excluded from patch
application.  The final patch fixes it.

Restructuring of the code necessary to fix it revealing another
long-standing bug that is not related X-<, which turned out to be a
larger fix (patch 1).

Junio C Hamano (3):
  apply: use the right attribute for paths in non-Git patches
  apply: hoist use_patch() helper for path exclusion up
  apply: omit ws check for excluded paths

 builtin/apply.c          | 131 ++++++++++++++++++++++++-----------------------
 t/t4119-apply-config.sh  |  17 ++++++
 t/t4124-apply-ws-rule.sh |  11 ++++
 3 files changed, 96 insertions(+), 63 deletions(-)

-- 
2.1.0-rc1-209-g4e1b551

[PATCH 1/3] apply: use the right attribute for paths in non-Git patches

[Junio C Hamano]

We parse each patchfile and find to what path it applies to and then
use that name to consult the attribute system to find what
whitespace rules to be applied, and also which target file (either
in the working tree or in the index) to replay the changes the patch
represents.

A non-Git patch is taken as relative to the current working
directory, and the prefix_patches() helper function introduced in
56185f49 (git-apply: require -p<n> when working in a subdirectory.,
2007-02-19) is used to prepend the prefix to these names found in
the patch input.

However, this prepending of the prefix to the pathnames is done
after the patch is fully parsed and affects only what target files
are patched.  Because the attributes are checked against the names
found in the patch during the parsing, not against the final path,
whitespace checks that are done during parsing end up using
attributes for a wrong path.

Because a Git-generated patch records the full path to the target
with -p$n prefix (e.g. a/ and b/) and we apply it relative to the
top of the working tree, prefix_patches() is a no-op and this
problem does not manifest for them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c         | 41 +++++++++++++++++++----------------------
 t/t4119-apply-config.sh | 17 +++++++++++++++++
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 6013e19..4270cde 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1920,6 +1920,23 @@ static int parse_binary(char *buffer, unsigned long size, struct patch *patch)
 	return used;
 }
 
+static void prefix_one(char **name)
+{
+	char *old_name = *name;
+	if (!old_name)
+		return;
+	*name = xstrdup(prefix_filename(prefix, prefix_length, *name));
+	free(old_name);
+}
+
+static void prefix_patch(struct patch *p)
+{
+	if (!prefix || p->is_toplevel_relative)
+		return;
+	prefix_one(&p->new_name);
+	prefix_one(&p->old_name);
+}
+
 /*
  * Read the patch text in "buffer" that extends for "size" bytes; stop
  * reading after seeing a single patch (i.e. changes to a single file).
@@ -1935,6 +1952,8 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)
 	if (offset < 0)
 		return offset;
 
+	prefix_patch(patch);
+
 	patch->ws_rule = whitespace_rule(patch->new_name
 					 ? patch->new_name
 					 : patch->old_name);
@@ -4164,26 +4183,6 @@ static int use_patch(struct patch *p)
 	return !has_include;
 }
 
-
-static void prefix_one(char **name)
-{
-	char *old_name = *name;
-	if (!old_name)
-		return;
-	*name = xstrdup(prefix_filename(prefix, prefix_length, *name));
-	free(old_name);
-}
-
-static void prefix_patches(struct patch *p)
-{
-	if (!prefix || p->is_toplevel_relative)
-		return;
-	for ( ; p; p = p->next) {
-		prefix_one(&p->new_name);
-		prefix_one(&p->old_name);
-	}
-}
-
 #define INACCURATE_EOF	(1<<0)
 #define RECOUNT		(1<<1)
 
@@ -4209,8 +4208,6 @@ static int apply_patch(int fd, const char *filename, int options)
 			break;
 		if (apply_in_reverse)
 			reverse_patches(patch);
-		if (prefix)
-			prefix_patches(patch);
 		if (use_patch(patch)) {
 			patch_stats(patch);
 			*listp = patch;
diff --git a/t/t4119-apply-config.sh b/t/t4119-apply-config.sh
index 3d0384d..be325fa 100755
--- a/t/t4119-apply-config.sh
+++ b/t/t4119-apply-config.sh
@@ -159,4 +159,21 @@ test_expect_success 'same but with traditional patch input of depth 2' '
 	check_result sub/file1
 '
 
+test_expect_success 'in subdir with traditional patch input' '
+	cd "$D" &&
+	git config apply.whitespace strip &&
+	cat >.gitattributes <<-EOF &&
+	/* whitespace=blank-at-eol
+	sub/* whitespace=-blank-at-eol
+	EOF
+	rm -f sub/file1 &&
+	cp saved sub/file1 &&
+	git update-index --refresh &&
+
+	cd sub &&
+	git apply ../gpatch.file &&
+	echo "B " >expect &&
+	test_cmp expect file1
+'
+
 test_done
-- 
2.1.0-rc1-209-g4e1b551

[PATCH 2/3] apply: hoist use_patch() helper for path exclusion up

[Junio C Hamano]

We will be adding a caller to the function a bit earlier in this
file in a later patch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c | 81 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 43 insertions(+), 38 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 4270cde..bf075cc 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1938,6 +1938,49 @@ static void prefix_patch(struct patch *p)
 }
 
 /*
+ * include/exclude
+ */
+
+static struct string_list limit_by_name;
+static int has_include;
+static void add_name_limit(const char *name, int exclude)
+{
+	struct string_list_item *it;
+
+	it = string_list_append(&limit_by_name, name);
+	it->util = exclude ? NULL : (void *) 1;
+}
+
+static int use_patch(struct patch *p)
+{
+	const char *pathname = p->new_name ? p->new_name : p->old_name;
+	int i;
+
+	/* Paths outside are not touched regardless of "--include" */
+	if (0 < prefix_length) {
+		int pathlen = strlen(pathname);
+		if (pathlen <= prefix_length ||
+		    memcmp(prefix, pathname, prefix_length))
+			return 0;
+	}
+
+	/* See if it matches any of exclude/include rule */
+	for (i = 0; i < limit_by_name.nr; i++) {
+		struct string_list_item *it = &limit_by_name.items[i];
+		if (!fnmatch(it->string, pathname, 0))
+			return (it->util != NULL);
+	}
+
+	/*
+	 * If we had any include, a path that does not match any rule is
+	 * not used.  Otherwise, we saw bunch of exclude rules (or none)
+	 * and such a path is used.
+	 */
+	return !has_include;
+}
+
+
+/*
  * Read the patch text in "buffer" that extends for "size" bytes; stop
  * reading after seeing a single patch (i.e. changes to a single file).
  * Create fragments (i.e. patch hunks) and hang them to the given patch.
@@ -4145,44 +4188,6 @@ static int write_out_results(struct patch *list)
 
 static struct lock_file lock_file;
 
-static struct string_list limit_by_name;
-static int has_include;
-static void add_name_limit(const char *name, int exclude)
-{
-	struct string_list_item *it;
-
-	it = string_list_append(&limit_by_name, name);
-	it->util = exclude ? NULL : (void *) 1;
-}
-
-static int use_patch(struct patch *p)
-{
-	const char *pathname = p->new_name ? p->new_name : p->old_name;
-	int i;
-
-	/* Paths outside are not touched regardless of "--include" */
-	if (0 < prefix_length) {
-		int pathlen = strlen(pathname);
-		if (pathlen <= prefix_length ||
-		    memcmp(prefix, pathname, prefix_length))
-			return 0;
-	}
-
-	/* See if it matches any of exclude/include rule */
-	for (i = 0; i < limit_by_name.nr; i++) {
-		struct string_list_item *it = &limit_by_name.items[i];
-		if (!fnmatch(it->string, pathname, 0))
-			return (it->util != NULL);
-	}
-
-	/*
-	 * If we had any include, a path that does not match any rule is
-	 * not used.  Otherwise, we saw bunch of exclude rules (or none)
-	 * and such a path is used.
-	 */
-	return !has_include;
-}
-
 #define INACCURATE_EOF	(1<<0)
 #define RECOUNT		(1<<1)
 
-- 
2.1.0-rc1-209-g4e1b551

[PATCH 3/3] apply: omit ws check for excluded paths

[Junio C Hamano]

Whitespace breakages are checked while the patch is being parsed.
Disable them at the beginning of parse_chunk(), where each
individual patch is parsed, immediately after we learn what path the
patch applies to and before we start parsing the changes.

One may naively think we should be able to not just skip the
whitespace checks but simply fast-forward to the next patch without
doing anything once use_patch() tells us that this patch is not
going to be used.  But in reality we cannot really skip much of the
parsing, primarily because parsing "@@ -k,l +m,n @@" lines and
counting the input lines is how we determine the boundaries of
individual patches.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 builtin/apply.c          |  9 ++++++---
 t/t4124-apply-ws-rule.sh | 11 +++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index bf075cc..13319e8 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1997,9 +1997,12 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)
 
 	prefix_patch(patch);
 
-	patch->ws_rule = whitespace_rule(patch->new_name
-					 ? patch->new_name
-					 : patch->old_name);
+	if (!use_patch(patch))
+		patch->ws_rule = 0;
+	else
+		patch->ws_rule = whitespace_rule(patch->new_name
+						 ? patch->new_name
+						 : patch->old_name);
 
 	patchsize = parse_single_patch(buffer + offset + hdrsize,
 				       size - offset - hdrsize, patch);
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 5d0c598..c6474de 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -512,4 +512,15 @@ test_expect_success 'whitespace=fix to expand' '
 	git -c core.whitespace=tab-in-indent apply --whitespace=fix patch
 '
 
+test_expect_success 'whitespace check skipped for excluded paths' '
+	git config core.whitespace blank-at-eol &&
+	>used &&
+	>unused &&
+	git add used unused &&
+	echo "used" >used &&
+	echo "unused " >unused &&
+	git diff-files -p used unused >patch &&
+	git apply --include=used --stat --whitespace=error <patch
+'
+
 test_done
-- 
2.1.0-rc1-209-g4e1b551