{"thread":{"id":"20001","subject":"[PATCH 0/2] git apply: cope with whitespace differences","startedAt":"2009-07-02T12:52:15Z","lastAt":"2009-07-02T18:29:07Z","messageCount":6,"participants":["Giuseppe Bilotta","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"117360","messageId":"1246539137-24754-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"20001","inReplyTo":null,"subject":"[PATCH 0/2] git apply: cope with whitespace differences","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T12:52:15Z","receivedAt":"2009-07-02T12:52:15Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"A small patchset that allows git-apply (and thus git-am and git-rebase)\nto cope with whitespace differences that would otherwise generate a conflict.\n\nThe first patch is basically an update of Rober Fitzsimons' previous work\nhttp://permalink.gmane.org/gmane.comp.version-control.git/7876\nwhereas the second one is entirely mine and makes the context lines\nmatch the original instead of the patched whitespace.\n\nGiuseppe Bilotta (2):\n  git apply: option to ignore whitespace differences\n  git apply: preserve original whitespace with --ignore-whitespace\n\n builtin-apply.c                        |   83 ++++++++++++++++++++++++++++++--\n contrib/completion/git-completion.bash |    2 +\n git-am.sh                              |    3 +-\n git-rebase.sh                          |    3 +\n t/t4107-apply-ignore-whitespace.sh     |   74 ++++++++++++++++++++++++++++\n 5 files changed, 159 insertions(+), 6 deletions(-)\n create mode 100755 t/t4107-apply-ignore-whitespace.sh\n"},{"id":"117361","messageId":"1246539137-24754-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"20001","inReplyTo":"1246539137-24754-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 1/2] git apply: option to ignore whitespace differences","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T12:52:16Z","receivedAt":"2009-07-02T12:52:16Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Introduce --ignore-whitespace option to ignore whitespace differences\nwhile applying the patch. The patch is an adaptatin of Robert\nFitzsimons' previous work, available at\nhttp://permalink.gmane.org/gmane.comp.version-control.git/7876\nincluding his test case.\n\n'git am' and 'git rebase' are made aware of this option and pass it\nthrough to 'git apply', and so is the bash git completion.\n---\n builtin-apply.c                        |   53 +++++++++++++++++++++-\n contrib/completion/git-completion.bash |    2 +\n git-am.sh                              |    3 +-\n git-rebase.sh                          |    3 +\n t/t4107-apply-ignore-whitespace.sh     |   74 ++++++++++++++++++++++++++++++++\n 5 files changed, 131 insertions(+), 4 deletions(-)\n create mode 100755 t/t4107-apply-ignore-whitespace.sh\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex dc0ff5e..86860d6 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -39,6 +39,7 @@ static int diffstat;\n static int numstat;\n static int summary;\n static int check;\n+static int ignore_whitespace = 0;\n static int apply = 1;\n static int apply_in_reverse;\n static int apply_with_reject;\n@@ -214,6 +215,45 @@ static uint32_t hash_line(const char *cp, size_t len)\n \treturn h;\n }\n \n+/*\n+ * Compare two memory areas ignoring whitespace differences\n+ */\n+static int memcmp_ignore_whitespace(const char *s1, const char *s2, size_t n)\n+{\n+\tconst char *stop1 = s1 + n;\n+\tconst char *stop2 = s2 + n;\n+\tint result;\n+\n+\tif (!n)\n+\t\treturn 0;\n+\n+\tdo {\n+\t\tif (isspace(*s1) && isspace(*s2)) {\n+\t\t\twhile (isspace(*s1)) {\n+\t\t\t\ts1++;\n+\t\t\t}\n+\t\t\twhile (isspace(*s2))\n+\t\t\t\ts2++;\n+\t\t}\n+\t\t/* Check here instead of in the while because\n+\t\t   the whitespace discarding might have moved us\n+\t\t   past the end */\n+\t\tif ((s1 >= stop1) || (s2 >= stop2))\n+\t\t\tbreak;\n+\t\tresult = *s1++ - *s2++;\n+\t} while (!result);\n+\n+\treturn result;\n+}\n+\n+static int memcmp_switch(const char *s1, const char *s2, size_t n)\n+{\n+\tif (ignore_whitespace)\n+\t\treturn memcmp_ignore_whitespace(s1, s2, n);\n+\telse\n+\t\treturn memcmp(s1, s2, n);\n+}\n+\n static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)\n {\n \tALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);\n@@ -1690,10 +1730,10 @@ static int match_fragment(struct image *img,\n \tif ((match_end\n \t     ? (try + preimage->len == img->len)\n \t     : (try + preimage->len <= img->len)) &&\n-\t    !memcmp(img->buf + try, preimage->buf, preimage->len))\n+\t    !memcmp_switch(img->buf + try, preimage->buf, preimage->len))\n \t\treturn 1;\n \n-\tif (ws_error_action != correct_ws_error)\n+\tif (!ignore_whitespace && (ws_error_action != correct_ws_error))\n \t\treturn 0;\n \n \t/*\n@@ -1731,8 +1771,13 @@ static int match_fragment(struct image *img,\n \t\t * In either case, we are fixing the whitespace breakages\n \t\t * so we might as well take the fix together with their\n \t\t * real change.\n+\t\t * If we are ignoring whitespace differences, don't check\n+\t\t * for length equality.\n \t\t */\n-\t\tmatch = (tgtfixlen == fixlen && !memcmp(tgtfix, buf, fixlen));\n+\t\tif (ignore_whitespace)\n+\t\t\tmatch = !memcmp_ignore_whitespace(buf, tgtfix, fixlen);\n+\t\telse\n+\t\t\tmatch = (tgtfixlen == fixlen && !memcmp(tgtfix, buf, fixlen));\n \n \t\tif (tgtfix != tgtfixbuf)\n \t\t\tfree(tgtfix);\n@@ -3304,6 +3349,8 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)\n \t\t{ OPTION_CALLBACK, 0, \"whitespace\", &whitespace_option, \"action\",\n \t\t\t\"detect new or modified lines that have whitespace errors\",\n \t\t\t0, option_parse_whitespace },\n+\t\tOPT_BOOLEAN(0, \"ignore-whitespace\", &ignore_whitespace,\n+\t\t\t\"ignore whitespace differences when finding context\"),\n \t\tOPT_BOOLEAN('R', \"reverse\", &apply_in_reverse,\n \t\t\t\"apply the patch in reverse\"),\n \t\tOPT_BOOLEAN(0, \"unidiff-zero\", &unidiff_zero,\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex ddb71e2..d3415b5 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -663,6 +663,7 @@ _git_am ()\n \t--*)\n \t\t__gitcomp \"\n \t\t\t--3way --committer-date-is-author-date --ignore-date\n+\t\t\t--ignore-whitespace\n \t\t\t--interactive --keep --no-utf8 --signoff --utf8\n \t\t\t--whitespace=\n \t\t\t\"\n@@ -684,6 +685,7 @@ _git_apply ()\n \t\t\t--stat --numstat --summary --check --index\n \t\t\t--cached --index-info --reverse --reject --unidiff-zero\n \t\t\t--apply --no-add --exclude=\n+\t\t\t--ignore-whitespace\n \t\t\t--whitespace= --inaccurate-eof --verbose\n \t\t\t\"\n \t\treturn\ndiff --git a/git-am.sh b/git-am.sh\nindex d64d997..fe024b1 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -16,6 +16,7 @@ s,signoff       add a Signed-off-by line to the commit message\n u,utf8          recode into utf8 (default)\n k,keep          pass -k flag to git-mailinfo\n whitespace=     pass it through git-apply\n+ignore-whitespace pass it through git-apply\n directory=      pass it through git-apply\n C=              pass it through git-apply\n p=              pass it through git-apply\n@@ -303,7 +304,7 @@ do\n \t\tgit_apply_opt=\"$git_apply_opt $(sq \"$1$2\")\"; shift ;;\n \t--patch-format)\n \t\tshift ; patch_format=\"$1\" ;;\n-\t--reject)\n+\t--reject|--ignore-whitespace)\n \t\tgit_apply_opt=\"$git_apply_opt $1\" ;;\n \t--committer-date-is-author-date)\n \t\tcommitter_date_is_author_date=t ;;\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 18bc694..d741752 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -333,6 +333,9 @@ do\n \t\t\t;;\n \t\tesac\n \t\t;;\n+\t--ignore-whitespace)\n+\t\tgit_am_opt=\"$git_am_opt $1\"\n+\t\t;;\n \t--committer-date-is-author-date|--ignore-date)\n \t\tgit_am_opt=\"$git_am_opt $1\"\n \t\tforce_rebase=t\ndiff --git a/t/t4107-apply-ignore-whitespace.sh b/t/t4107-apply-ignore-whitespace.sh\nnew file mode 100755\nindex 0000000..d31e0f3\n--- /dev/null\n+++ b/t/t4107-apply-ignore-whitespace.sh\n@@ -0,0 +1,74 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2005 Junio C Hamano\n+# Copyright (c) 2005 Robert Fitzsimons\n+#\n+\n+test_description='git-apply --ignore-whitespace.\n+\n+'\n+. ./test-lib.sh\n+\n+# setup\n+\n+cat > patch1.patch <<\\EOF\n+diff --git a/main.c b/main.c\n+new file mode 100644\n+--- /dev/null\n++++ b/main.c\n+@@ -0,0 +1,23 @@\n++#include <stdio.h>\n++\n++void print_int(int num);\n++int func(int num);\n++\n++int main() {\n++       int i;\n++\n++       for (i = 0; i < 10; i++) {\n++               print_int(func(i));\n++       }\n++\n++       return 0;\n++}\n++\n++int func(int num) {\n++       return num * num;\n++}\n++\n++void print_int(int num) {\n++       printf(\"%d\", num);\n++}\n++\n+EOF\n+cat > patch2.patch <<\\EOF\n+diff --git a/main.c b/main.c\n+--- a/main.c\n++++ b/main.c\n+@@ -10,6 +10,8 @@\n+ \t\tprint_int(func(i));\n+ \t}\n+ \n++\tprintf(\"\\n\");\n++\n+ \treturn 0;\n+ }\n+\n+EOF\n+\n+test_expect_success \"S = patch1\" \\\n+    'git-apply patch1.patch'\n+\n+test_expect_failure \"F = patch2\" \\\n+    'git-apply patch2.patch'\n+\n+test_expect_success \"S = patch2 (--ignore-whitespace)\" \\\n+    'git-apply --ignore-whitespace patch2.patch'\n+\n+rm -f main.c\n+test_expect_success \"S = patch1 (--ignore-whitespace)\" \\\n+    'git-apply --ignore-whitespace patch1.patch'\n+\n+test_done\n+\n+\n-- \n1.6.3.3.512.g1f6a.dirty\n"},{"id":"117362","messageId":"1246539137-24754-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"20001","inReplyTo":"1246539137-24754-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 2/2] git apply: preserve original whitespace with --ignore-whitespace","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T12:52:17Z","receivedAt":"2009-07-02T12:52:17Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"---\n builtin-apply.c |   32 +++++++++++++++++++++++++++++---\n 1 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 86860d6..d213cce 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1640,6 +1640,10 @@ static void update_pre_post_images(struct image *preimage,\n \tint i, ctx;\n \tchar *new, *old, *fixed;\n \tstruct image fixed_preimage;\n+\t/* Do we need more space for the postimage? */\n+\tsize_t newlen = postimage->len;\n+\tif (len > preimage->len)\n+\t\tnewlen += len - preimage->len;\n \n \t/*\n \t * Update the preimage with whitespace fixes.  Note that we\n@@ -1654,11 +1658,16 @@ static void update_pre_post_images(struct image *preimage,\n \t*preimage = fixed_preimage;\n \n \t/*\n-\t * Adjust the common context lines in postimage, in place.\n+\t * Adjust the common context lines in postimage, in place\n+\t * if we are not ignoring whitespace differences.\n \t * This is possible because whitespace fixing does not make\n \t * the string grow.\n \t */\n-\tnew = old = postimage->buf;\n+\told = postimage->buf;\n+\tif (ignore_whitespace)\n+\t\tnew = postimage->buf = xmalloc(newlen);\n+\telse\n+\t\tnew = old;\n \tfixed = preimage->buf;\n \tfor (i = ctx = 0; i < postimage->nr; i++) {\n \t\tsize_t len = postimage->line[i].len;\n@@ -1730,8 +1739,25 @@ static int match_fragment(struct image *img,\n \tif ((match_end\n \t     ? (try + preimage->len == img->len)\n \t     : (try + preimage->len <= img->len)) &&\n-\t    !memcmp_switch(img->buf + try, preimage->buf, preimage->len))\n+\t    !memcmp_switch(img->buf + try, preimage->buf, preimage->len)) {\n+\t\tif (ignore_whitespace) {\n+\t\t\t/*\n+\t\t\t * Replace the preimage whitespace with the original one\n+\t\t\t */\n+\t\t\tsize_t newlen = 0;\n+\t\t\tfor (i = 0; i < preimage->nr; i++) {\n+\t\t\t\tnewlen += preimage->line[i].len =\n+\t\t\t\t          img->line[try_lno + i].len;\n+\t\t\t}\n+\t\t\tfixed_buf = xmalloc(newlen);\n+\t\t\tmemcpy(fixed_buf, img->buf + try, newlen);\n+\n+\t\t\tupdate_pre_post_images(\n+\t\t\t\tpreimage, postimage,\n+\t\t\t\tfixed_buf, newlen);\n+\t\t}\n \t\treturn 1;\n+\t}\n \n \tif (!ignore_whitespace && (ws_error_action != correct_ws_error))\n \t\treturn 0;\n-- \n1.6.3.3.512.g1f6a.dirty\n"},{"id":"117372","messageId":"7vws6r822y.fsf@alter.siamese.dyndns.org","threadId":"20001","inReplyTo":"1246539137-24754-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 1/2] git apply: option to ignore whitespace differences","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-02T16:46:45Z","receivedAt":"2009-07-02T16:46:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> diff --git a/builtin-apply.c b/builtin-apply.c\n> index dc0ff5e..86860d6 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -39,6 +39,7 @@ static int diffstat;\n>  static int numstat;\n>  static int summary;\n>  static int check;\n> +static int ignore_whitespace = 0;\n\ns/ = 0//;\n\n> +/*\n> + * Compare two memory areas ignoring whitespace differences\n> + */\n> +static int memcmp_ignore_whitespace(const char *s1, const char *s2, size_t n)\n> +{\n\nDon't you think this function signature is bogus?\n\nYou are going to use this function to compare a line that is in the target\nbuffer (i.e. the contents of the current blob) with a line read from the\npatch context when the user says they might match if you ignore whitespace\ndifferences.\n\nHow on earth can you do that with only a single length parameter \"n\"?\nLength of which side are you talking about?\n\nRemember that git-apply is designed to be able to handle a patch that has\nNUL in it (generated with \"diff --text\"), so strlen() is not an acceptable\nanswer.\n\n> +static int memcmp_switch(const char *s1, const char *s2, size_t n)\n> +{\n> +\tif (ignore_whitespace)\n> +\t\treturn memcmp_ignore_whitespace(s1, s2, n);\n> +\telse\n> +\t\treturn memcmp(s1, s2, n);\n> +}\n> +\n\nThis function signature shares the same bogosity with the previous one.\n\nIn addition, it's name and semantics is bogus.\n\nThe original implementation had encapsulated the notion that it is\ncomparing two lines entirely in match_fragment().  Its use of memcmp() was\nan implementation detail of a half of the open-coded logic to compare two\nlines (the other half being the comparison of lengths), and direct use of\nmemcmp() made perfect sense.  It checked the length matched.  Now it\nwanted to make sure they matched byte-for-byte.\n\nIf you are separating that \"compare two lines\" logic into a helper\nfunction, I would expect its name actually reflect its purpose, whose\nbehaviour may change depending on --ignore-whitespace.  The traditional\ncodepath would say \"do they have the same length and match byte-for-byte?\"\nwhile the new loosened codepath would say \"we do not care about the\nwhitespaces; do they match if we disregard ws differences?\"\n\nI also suspect that you might be able to optimize the existing \"allow\nwhitespace-fixed match\" a bit by \"fix\"ing the target buffer only once,\ninstead of doing so line-by-line for every offset that find_pos() checks\nby calling match_fragment().  It is an independent issue, but it appears\nto me that the change this patch wants to do to match_fragment() might\nbecome cleaner if we did that conversion first, as match_fragment() itself\nwon't have to have two cases (early return for exact match, and match with\nwhitespace-fixed target).\n"},{"id":"117376","messageId":"cb7bb73a0907021046x7fcb921bs86d55b69de540c9d@mail.gmail.com","threadId":"20001","inReplyTo":"7vws6r822y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] git apply: option to ignore whitespace differences","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T17:46:27Z","receivedAt":"2009-07-02T17:46:27Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Jul 2, 2009 at 6:46 PM, Junio C Hamano<gitster@pobox.com> wrote:\n>> diff --git a/builtin-apply.c b/builtin-apply.c\n>> index dc0ff5e..86860d6 100644\n>> --- a/builtin-apply.c\n>> +++ b/builtin-apply.c\n>> @@ -39,6 +39,7 @@ static int diffstat;\n>>  static int numstat;\n>>  static int summary;\n>>  static int check;\n>> +static int ignore_whitespace = 0;\n>\n> s/ = 0//;\n\nAh, I wondered about that. I assume this leaves no possibility for\nuninitialized values because of the way option parsing is done?\n\n>> +/*\n>> + * Compare two memory areas ignoring whitespace differences\n>> + */\n>> +static int memcmp_ignore_whitespace(const char *s1, const char *s2, size_t n)\n>> +{\n>\n> Don't you think this function signature is bogus?\n\nI did have a few doubts on it. I think I worked out a proper solution,\nper your suggestion.\n\n>> +static int memcmp_switch(const char *s1, const char *s2, size_t n)\n>\n> This function signature shares the same bogosity with the previous one.\n>\n> In addition, it's name and semantics is bogus.\n\n> If you are separating that \"compare two lines\" logic into a helper\n> function, I would expect its name actually reflect its purpose, whose\n> behaviour may change depending on --ignore-whitespace.  The traditional\n> codepath would say \"do they have the same length and match byte-for-byte?\"\n> while the new loosened codepath would say \"we do not care about the\n> whitespaces; do they match if we disregard ws differences?\"\n\nIndeed. See upcoming patch for a better name 8-)\n\n> I also suspect that you might be able to optimize the existing \"allow\n> whitespace-fixed match\" a bit by \"fix\"ing the target buffer only once,\n> instead of doing so line-by-line for every offset that find_pos() checks\n> by calling match_fragment().  It is an independent issue, but it appears\n> to me that the change this patch wants to do to match_fragment() might\n> become cleaner if we did that conversion first, as match_fragment() itself\n> won't have to have two cases (early return for exact match, and match with\n> whitespace-fixed target).\n\nBut OTOH that would 'waste time' fixing whitespace when it might not\nbe needed ...\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117383","messageId":"7vocs36irw.fsf@alter.siamese.dyndns.org","threadId":"20001","inReplyTo":"cb7bb73a0907021046x7fcb921bs86d55b69de540c9d@mail.gmail.com","subject":"Re: [PATCH 1/2] git apply: option to ignore whitespace differences","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-02T18:29:07Z","receivedAt":"2009-07-02T18:29:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n> On Thu, Jul 2, 2009 at 6:46 PM, Junio C Hamano<gitster@pobox.com> wrote:\n>>> diff --git a/builtin-apply.c b/builtin-apply.c\n>>> index dc0ff5e..86860d6 100644\n>>> --- a/builtin-apply.c\n>>> +++ b/builtin-apply.c\n>>> @@ -39,6 +39,7 @@ static int diffstat;\n>>>  static int numstat;\n>>>  static int summary;\n>>>  static int check;\n>>> +static int ignore_whitespace = 0;\n>>\n>> s/ = 0//;\n>\n> Ah, I wondered about that. I assume this leaves no possibility for\n> uninitialized values because of the way option parsing is done?\n\nIt is not \"uninitialized\" to begin with.\n\nC initializes static variables without explicit initial values to 0.\n"}]}