{"thread":{"id":"37300","subject":"Bug v1.9.3: Spurious whitespace warnings when using git apply with the --exclude option","startedAt":"2014-08-06T17:13:34Z","lastAt":"2014-08-06T22:58:05Z","messageCount":6,"participants":["Peyton Randolph","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"247343","messageId":"332C033B-7809-45A9-9F91-CC3A92090B86@apple.com","threadId":"37300","inReplyTo":null,"subject":"Bug v1.9.3: Spurious whitespace warnings when using git apply with the --exclude option","fromName":"Peyton Randolph","fromEmail":"prandolph@apple.com","sentAt":"2014-08-06T17:13:34Z","receivedAt":"2014-08-06T17:13:34Z","isPatch":false,"sender":{"key":"prandolph@apple.com","avatar":null},"body":"Bug v1.9.3: Spurious whitespace warnings when using git apply with the --exclude option\n\nREPRO STEPS:\n1. 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.\n2. Apply that patch to a repo using git apply PATCH --exclude A\n\nACTUAL RESULT:\ngit apply outputs a \"warning: n lines add whitespace errors.” where n is the number of lines with whitespace errors in A. \n\nEXPECTED RESULT:\nNo warning about adding whitespace errors because no whitespace errors are added to the tree since A is excluded and B has no whitespace errors."},{"id":"247361","messageId":"xmqq8un1ml7e.fsf@gitster.dls.corp.google.com","threadId":"37300","inReplyTo":"332C033B-7809-45A9-9F91-CC3A92090B86@apple.com","subject":"Re: Bug v1.9.3: Spurious whitespace warnings when using git apply with the --exclude option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-06T20:12:21Z","receivedAt":"2014-08-06T20:12:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peyton Randolph writes:\n\n> Bug v1.9.3: Spurious whitespace warnings when using git apply with the --exclude option\n> ...\n> EXPECTED RESULT:\n> No warning about adding whitespace errors because no whitespace\n> errors are added to the tree since A is excluded and B has no\n> whitespace errors.\n\nBy reading the way the code is structured, I would expect (I didn't\ncheck) that all versions of apply that supports whitespace error\ndetection with include/exclude option to share this symptom, and\nthis bug is not specific to 1.9.3.\n\nIn order to decide if a hunk with a new whitespace error should be\nreported, the program needs to parse the patch input to at least\nfind out what path the part being parsed is to be applied, and the\nwhitespace error detection and reporting is done when the patch\ninput is parsed. After the part that applies to one path is fully\nparsed, it then checks to see if the path is included/excluded and\nomits it from the --stat output and also actual application.\n\nThat part of the code needs to be restructured so that at least\nerror reporting (and preferrably error detection) is not done when\nthe path is known to be excluded. Let me see if this can be easily\ndone.\n\nThanks.\n"},{"id":"247372","messageId":"1407365885-1628-1-git-send-email-gitster@pobox.com","threadId":"37300","inReplyTo":"xmqq8un1ml7e.fsf@gitster.dls.corp.google.com","subject":"[PATCH 0/3] Two fixes to \"git apply\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-06T22:58:02Z","receivedAt":"2014-08-06T22:58:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This started as an attempt to fix a long-time bug, in which a\npartial \"git apply\" with --exclude/--include still complained about\nwhitespace breakage in a part that was excluded from patch\napplication. The final patch fixes it.\n\nRestructuring of the code necessary to fix it revealing another\nlong-standing bug that is not related X-<, which turned out to be a\nlarger fix (patch 1).\n\nJunio C Hamano (3):\n apply: use the right attribute for paths in non-Git patches\n apply: hoist use_patch() helper for path exclusion up\n apply: omit ws check for excluded paths\n\n builtin/apply.c | 131 ++++++++++++++++++++++++-----------------------\n t/t4119-apply-config.sh | 17 ++++++\n t/t4124-apply-ws-rule.sh | 11 ++++\n 3 files changed, 96 insertions(+), 63 deletions(-)\n\n-- \n2.1.0-rc1-209-g4e1b551\n"},{"id":"247373","messageId":"1407365885-1628-2-git-send-email-gitster@pobox.com","threadId":"37300","inReplyTo":"1407365885-1628-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/3] apply: use the right attribute for paths in non-Git patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-06T22:58:03Z","receivedAt":"2014-08-06T22:58:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We parse each patchfile and find to what path it applies to and then\nuse that name to consult the attribute system to find what\nwhitespace rules to be applied, and also which target file (either\nin the working tree or in the index) to replay the changes the patch\nrepresents.\n\nA non-Git patch is taken as relative to the current working\ndirectory, and the prefix_patches() helper function introduced in\n56185f49 (git-apply: require -p when working in a subdirectory.,\n2007-02-19) is used to prepend the prefix to these names found in\nthe patch input.\n\nHowever, this prepending of the prefix to the pathnames is done\nafter the patch is fully parsed and affects only what target files\nare patched. Because the attributes are checked against the names\nfound in the patch during the parsing, not against the final path,\nwhitespace checks that are done during parsing end up using\nattributes for a wrong path.\n\nBecause a Git-generated patch records the full path to the target\nwith -p$n prefix (e.g. a/ and b/) and we apply it relative to the\ntop of the working tree, prefix_patches() is a no-op and this\nproblem does not manifest for them.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 41 +++++++++++++++++++----------------------\n t/t4119-apply-config.sh | 17 +++++++++++++++++\n 2 files changed, 36 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 6013e19..4270cde 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1920,6 +1920,23 @@ static int parse_binary(char *buffer, unsigned long size, struct patch *patch)\n \treturn used;\n }\n \n+static void prefix_one(char **name)\n+{\n+\tchar *old_name = *name;\n+\tif (!old_name)\n+\t\treturn;\n+\t*name = xstrdup(prefix_filename(prefix, prefix_length, *name));\n+\tfree(old_name);\n+}\n+\n+static void prefix_patch(struct patch *p)\n+{\n+\tif (!prefix || p->is_toplevel_relative)\n+\t\treturn;\n+\tprefix_one(&p->new_name);\n+\tprefix_one(&p->old_name);\n+}\n+\n /*\n * Read the patch text in \"buffer\" that extends for \"size\" bytes; stop\n * reading after seeing a single patch (i.e. changes to a single file).\n@@ -1935,6 +1952,8 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)\n \tif (offset < 0)\n \t\treturn offset;\n \n+\tprefix_patch(patch);\n+\n \tpatch->ws_rule = whitespace_rule(patch->new_name\n \t\t\t\t\t ? patch->new_name\n \t\t\t\t\t : patch->old_name);\n@@ -4164,26 +4183,6 @@ static int use_patch(struct patch *p)\n \treturn !has_include;\n }\n \n-\n-static void prefix_one(char **name)\n-{\n-\tchar *old_name = *name;\n-\tif (!old_name)\n-\t\treturn;\n-\t*name = xstrdup(prefix_filename(prefix, prefix_length, *name));\n-\tfree(old_name);\n-}\n-\n-static void prefix_patches(struct patch *p)\n-{\n-\tif (!prefix || p->is_toplevel_relative)\n-\t\treturn;\n-\tfor ( ; p; p = p->next) {\n-\t\tprefix_one(&p->new_name);\n-\t\tprefix_one(&p->old_name);\n-\t}\n-}\n-\n #define INACCURATE_EOF\t(1<<0)\n #define RECOUNT\t\t(1<<1)\n \n@@ -4209,8 +4208,6 @@ static int apply_patch(int fd, const char *filename, int options)\n \t\t\tbreak;\n \t\tif (apply_in_reverse)\n \t\t\treverse_patches(patch);\n-\t\tif (prefix)\n-\t\t\tprefix_patches(patch);\n \t\tif (use_patch(patch)) {\n \t\t\tpatch_stats(patch);\n \t\t\t*listp = patch;\ndiff --git a/t/t4119-apply-config.sh b/t/t4119-apply-config.sh\nindex 3d0384d..be325fa 100755\n--- a/t/t4119-apply-config.sh\n+++ b/t/t4119-apply-config.sh\n@@ -159,4 +159,21 @@ test_expect_success 'same but with traditional patch input of depth 2' '\n \tcheck_result sub/file1\n '\n \n+test_expect_success 'in subdir with traditional patch input' '\n+\tcd \"$D\" &&\n+\tgit config apply.whitespace strip &&\n+\tcat >.gitattributes <<-EOF &&\n+\t/* whitespace=blank-at-eol\n+\tsub/* whitespace=-blank-at-eol\n+\tEOF\n+\trm -f sub/file1 &&\n+\tcp saved sub/file1 &&\n+\tgit update-index --refresh &&\n+\n+\tcd sub &&\n+\tgit apply ../gpatch.file &&\n+\techo \"B \" >expect &&\n+\ttest_cmp expect file1\n+'\n+\n test_done\n-- \n2.1.0-rc1-209-g4e1b551\n"},{"id":"247374","messageId":"1407365885-1628-3-git-send-email-gitster@pobox.com","threadId":"37300","inReplyTo":"1407365885-1628-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] apply: hoist use_patch() helper for path exclusion up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-06T22:58:04Z","receivedAt":"2014-08-06T22:58:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We will be adding a caller to the function a bit earlier in this\nfile in a later patch.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 81 ++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 43 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 4270cde..bf075cc 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1938,6 +1938,49 @@ static void prefix_patch(struct patch *p)\n }\n \n /*\n+ * include/exclude\n+ */\n+\n+static struct string_list limit_by_name;\n+static int has_include;\n+static void add_name_limit(const char *name, int exclude)\n+{\n+\tstruct string_list_item *it;\n+\n+\tit = string_list_append(&limit_by_name, name);\n+\tit->util = exclude ? NULL : (void *) 1;\n+}\n+\n+static int use_patch(struct patch *p)\n+{\n+\tconst char *pathname = p->new_name ? p->new_name : p->old_name;\n+\tint i;\n+\n+\t/* Paths outside are not touched regardless of \"--include\" */\n+\tif (0 < prefix_length) {\n+\t\tint pathlen = strlen(pathname);\n+\t\tif (pathlen <= prefix_length ||\n+\t\t memcmp(prefix, pathname, prefix_length))\n+\t\t\treturn 0;\n+\t}\n+\n+\t/* See if it matches any of exclude/include rule */\n+\tfor (i = 0; i < limit_by_name.nr; i++) {\n+\t\tstruct string_list_item *it = &limit_by_name.items[i];\n+\t\tif (!fnmatch(it->string, pathname, 0))\n+\t\t\treturn (it->util != NULL);\n+\t}\n+\n+\t/*\n+\t * If we had any include, a path that does not match any rule is\n+\t * not used. Otherwise, we saw bunch of exclude rules (or none)\n+\t * and such a path is used.\n+\t */\n+\treturn !has_include;\n+}\n+\n+\n+/*\n * Read the patch text in \"buffer\" that extends for \"size\" bytes; stop\n * reading after seeing a single patch (i.e. changes to a single file).\n * Create fragments (i.e. patch hunks) and hang them to the given patch.\n@@ -4145,44 +4188,6 @@ static int write_out_results(struct patch *list)\n \n static struct lock_file lock_file;\n \n-static struct string_list limit_by_name;\n-static int has_include;\n-static void add_name_limit(const char *name, int exclude)\n-{\n-\tstruct string_list_item *it;\n-\n-\tit = string_list_append(&limit_by_name, name);\n-\tit->util = exclude ? NULL : (void *) 1;\n-}\n-\n-static int use_patch(struct patch *p)\n-{\n-\tconst char *pathname = p->new_name ? p->new_name : p->old_name;\n-\tint i;\n-\n-\t/* Paths outside are not touched regardless of \"--include\" */\n-\tif (0 < prefix_length) {\n-\t\tint pathlen = strlen(pathname);\n-\t\tif (pathlen <= prefix_length ||\n-\t\t memcmp(prefix, pathname, prefix_length))\n-\t\t\treturn 0;\n-\t}\n-\n-\t/* See if it matches any of exclude/include rule */\n-\tfor (i = 0; i < limit_by_name.nr; i++) {\n-\t\tstruct string_list_item *it = &limit_by_name.items[i];\n-\t\tif (!fnmatch(it->string, pathname, 0))\n-\t\t\treturn (it->util != NULL);\n-\t}\n-\n-\t/*\n-\t * If we had any include, a path that does not match any rule is\n-\t * not used. Otherwise, we saw bunch of exclude rules (or none)\n-\t * and such a path is used.\n-\t */\n-\treturn !has_include;\n-}\n-\n #define INACCURATE_EOF\t(1<<0)\n #define RECOUNT\t\t(1<<1)\n \n-- \n2.1.0-rc1-209-g4e1b551\n"},{"id":"247375","messageId":"1407365885-1628-4-git-send-email-gitster@pobox.com","threadId":"37300","inReplyTo":"1407365885-1628-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] apply: omit ws check for excluded paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-06T22:58:05Z","receivedAt":"2014-08-06T22:58:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Whitespace breakages are checked while the patch is being parsed.\nDisable them at the beginning of parse_chunk(), where each\nindividual patch is parsed, immediately after we learn what path the\npatch applies to and before we start parsing the changes.\n\nOne may naively think we should be able to not just skip the\nwhitespace checks but simply fast-forward to the next patch without\ndoing anything once use_patch() tells us that this patch is not\ngoing to be used. But in reality we cannot really skip much of the\nparsing, primarily because parsing \"@@ -k,l +m,n @@\" lines and\ncounting the input lines is how we determine the boundaries of\nindividual patches.\n\nSigned-off-by: Junio C Hamano \n---\n\n builtin/apply.c | 9 ++++++---\n t/t4124-apply-ws-rule.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex bf075cc..13319e8 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1997,9 +1997,12 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)\n \n \tprefix_patch(patch);\n \n-\tpatch->ws_rule = whitespace_rule(patch->new_name\n-\t\t\t\t\t ? patch->new_name\n-\t\t\t\t\t : patch->old_name);\n+\tif (!use_patch(patch))\n+\t\tpatch->ws_rule = 0;\n+\telse\n+\t\tpatch->ws_rule = whitespace_rule(patch->new_name\n+\t\t\t\t\t\t ? patch->new_name\n+\t\t\t\t\t\t : patch->old_name);\n \n \tpatchsize = parse_single_patch(buffer + offset + hdrsize,\n \t\t\t\t size - offset - hdrsize, patch);\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex 5d0c598..c6474de 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -512,4 +512,15 @@ test_expect_success 'whitespace=fix to expand' '\n \tgit -c core.whitespace=tab-in-indent apply --whitespace=fix patch\n '\n \n+test_expect_success 'whitespace check skipped for excluded paths' '\n+\tgit config core.whitespace blank-at-eol &&\n+\t>used &&\n+\t>unused &&\n+\tgit add used unused &&\n+\techo \"used\" >used &&\n+\techo \"unused \" >unused &&\n+\tgit diff-files -p used unused >patch &&\n+\tgit apply --include=used --stat --whitespace=error