{"thread":{"id":"20007","subject":"[PATCHv1bis 0/2] git apply: cope with whitespace differences","startedAt":"2009-07-02T17:48:39Z","lastAt":"2009-07-03T06:40:01Z","messageCount":12,"participants":["Giuseppe Bilotta","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"117377","messageId":"1246556921-5819-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"20007","inReplyTo":null,"subject":"[PATCHv1bis 0/2] git apply: cope with whitespace differences","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T17:48:39Z","receivedAt":"2009-07-02T17:48:39Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Sometimes I do some very stupid things, as was the case with the first\nversion of this sequence. This new version still doesn't include Junio's\nsuggestion to move whitespace fixing to before the check, because I'm\nnot convinced it should be the case.\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                        |   90 +++++++++++++++++++++++++++++--\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, 165 insertions(+), 7 deletions(-)\n create mode 100755 t/t4107-apply-ignore-whitespace.sh\n"},{"id":"117378","messageId":"1246556921-5819-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"20007","inReplyTo":"1246556921-5819-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv1bis 1/2] git apply: option to ignore whitespace differences","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T17:48:40Z","receivedAt":"2009-07-02T17:48:40Z","isPatch":false,"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 based on Robert Fitzsimons' work\nhttp://permalink.gmane.org/gmane.comp.version-control.git/7876\nand includes 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                        |   60 ++++++++++++++++++++++++--\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, 137 insertions(+), 5 deletions(-)\n create mode 100755 t/t4107-apply-ignore-whitespace.sh\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex dc0ff5e..70cc985 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;\n static int apply = 1;\n static int apply_in_reverse;\n static int apply_with_reject;\n@@ -214,6 +215,49 @@ 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, size_t n1, const char *s2, size_t n2)\n+{\n+\tconst char *stop1 = s1 + n1;\n+\tconst char *stop2 = s2 + n2;\n+\tint result;\n+\n+\tif (!(n1 | n2))\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+/*\n+ * Returns true if the given lines (buffer + len) match\n+ * according to the ignore_whitespace setting\n+ */\n+static int lines_match(const char *s1, size_t n1, const char *s2, size_t n2)\n+{\n+\tif (ignore_whitespace)\n+\t\treturn !memcmp_ignore_whitespace(s1, n1, s2, n2);\n+\telse\n+\t\treturn (n1 == n2) && !memcmp(s1, s2, n1);\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@@ -1665,6 +1709,7 @@ static int match_fragment(struct image *img,\n {\n \tint i;\n \tchar *fixed_buf, *buf, *orig, *target;\n+\tsize_t img_len = 0;\n \n \tif (preimage->nr + try_lno > img->nr)\n \t\treturn 0;\n@@ -1676,9 +1721,11 @@ static int match_fragment(struct image *img,\n \t\treturn 0;\n \n \t/* Quick hash check */\n-\tfor (i = 0; i < preimage->nr; i++)\n+\tfor (i = 0; i < preimage->nr; i++) {\n+\t\timg_len += img->line[try_lno + i].len;\n \t\tif (preimage->line[i].hash != img->line[try_lno + i].hash)\n \t\t\treturn 0;\n+\t}\n \n \t/*\n \t * Do we have an exact match?  If we were told to match\n@@ -1690,10 +1737,11 @@ 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    lines_match(img->buf + try, img_len,\n+\t                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 +1779,10 @@ 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\tmatch = lines_match(buf, fixlen, tgtfix, tgtfixlen);\n \n \t\tif (tgtfix != tgtfixbuf)\n \t\t\tfree(tgtfix);\n@@ -3304,6 +3354,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.511.g0ded0.dirty\n"},{"id":"117379","messageId":"1246556921-5819-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"20007","inReplyTo":"1246556921-5819-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv1bis 2/2] git apply: preserve original whitespace with --ignore-whitespace","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T17:48:41Z","receivedAt":"2009-07-02T17:48:41Z","isPatch":false,"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 70cc985..24ab286 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1644,6 +1644,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@@ -1658,11 +1662,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@@ -1738,8 +1747,25 @@ static int match_fragment(struct image *img,\n \t     ? (try + preimage->len == img->len)\n \t     : (try + preimage->len <= img->len)) &&\n \t    lines_match(img->buf + try, img_len,\n-\t                preimage->buf, preimage->len))\n+\t                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.511.g0ded0.dirty\n"},{"id":"117382","messageId":"7vvdmb6ium.fsf@alter.siamese.dyndns.org","threadId":"20007","inReplyTo":"1246556921-5819-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-02T18:27:29Z","receivedAt":"2009-07-02T18:27:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n> +static int memcmp_ignore_whitespace(const char *s1, size_t n1, const char *s2, size_t n2)\n> +{\n> +\tconst char *stop1 = s1 + n1;\n> +\tconst char *stop2 = s2 + n2;\n> +\tint result;\n> +\n> +\tif (!(n1 | n2))\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\nIf s1 is longer than s2 (or vice versa) but one is a prefix of the other,\nyou will return \"they match\", because previous round compared *s1 and *s2\nand found them the same?\n\n> +/*\n> + * Returns true if the given lines (buffer + len) match\n> + * according to the ignore_whitespace setting\n> + */\n> +static int lines_match(const char *s1, size_t n1, const char *s2, size_t n2)\n> +{\n> +\tif (ignore_whitespace)\n> +\t\treturn !memcmp_ignore_whitespace(s1, n1, s2, n2);\n> +\telse\n> +\t\treturn (n1 == n2) && !memcmp(s1, s2, n1);\n> +}\n> +\n\nI think this still is an abstraction at the wrong level.  For one thing,\nif ignore-whitespace is set, you do not even need nor want to do the \"fix\nonly the ws breakages we are going to fix anyway according to the ws_rule\"\ntransformation applied to the preimage.\n\nI think the handling of correct_ws_error in the caller should also be\nmoved here.  IOW, shouldn't this function look like this?\n\n\tlines_match()\n        {\n        \t/* if the user wants fuzz, so be it */\n        \tif (ignore_whitespace)\n                \treturn compare_lines_wo_ws();\n\t\t/* most common case: matches without fuzzing */\n\t\tif (length matches && !memcmp())\n                \treturn 1;\n\t\t/* we are done, if we are not correcting */\n\t\tif (ws_error_action != correct_ws_error)\n                \treturn 0;\n\t\t... apply configured ws fix to preimage ...;\n                /* does it apply with ws breakage in its context fixed? */\n                if (length now matches && !memcmp(fixed preimage, postimage)\n\t\t\treturn 1;\n\t\t/* noop, this won't apply */\n\t\treturn 0;\n\t}\n\nwhich would make the large-ish loop near the end of match_fragment() to\nsomething like:\n\n\tfor (i = 0; i < preimage->nr; i++) {\n        \t... set up arguments to lines_match ...;\n\t\tmatch = lines_match();\n\t\tif (!match)\n                \tgoto unmatch_exit;\n\t\t... adjust for next iteration ...;\n        }\n"},{"id":"117384","messageId":"cb7bb73a0907021202ra322425pc64b54953f4f544d@mail.gmail.com","threadId":"20007","inReplyTo":"7vvdmb6ium.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T19:02:02Z","receivedAt":"2009-07-02T19:02:02Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Jul 2, 2009 at 8:27 PM, Junio C Hamano<gitster@pobox.com> wrote:\n> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>\n>> +static int memcmp_ignore_whitespace(const char *s1, size_t n1, const char *s2, size_t n2)\n>> +{\n>> +     const char *stop1 = s1 + n1;\n>> +     const char *stop2 = s2 + n2;\n>> +     int result;\n>> +\n>> +     if (!(n1 | n2))\n>> +             return 0;\n>> +\n>> +     do {\n>> +             if (isspace(*s1) && isspace(*s2)) {\n>> +                     while (isspace(*s1)) {\n>> +                             s1++;\n>> +                     }\n>> +                     while (isspace(*s2))\n>> +                             s2++;\n>> +             }\n>> +             /* Check here instead of in the while because\n>> +                the whitespace discarding might have moved us\n>> +                past the end */\n>> +             if ((s1 >= stop1) || (s2 >= stop2))\n>> +                     break;\n>\n> If s1 is longer than s2 (or vice versa) but one is a prefix of the other,\n> you will return \"they match\", because previous round compared *s1 and *s2\n> and found them the same?\n\nYes. Actually, more specifically, now that I think more about this,\nthat's the reason why my previous code was fine: when we compare the\nlines in the preimage against the image, we only look for a match\nwhich is as long as the preimage, we don't care what ELSE is there in\nthe image. So it makes sense to pass a single length parameter, the\npreimage length.\n\nHowever, my first version should be fixed so that the order of the\nparameter becomes significant, and the early bailout is only done if\nthe preimage length has been totally consumed.\n\n>> +/*\n>> + * Returns true if the given lines (buffer + len) match\n>> + * according to the ignore_whitespace setting\n>> + */\n>> +static int lines_match(const char *s1, size_t n1, const char *s2, size_t n2)\n>> +{\n>> +     if (ignore_whitespace)\n>> +             return !memcmp_ignore_whitespace(s1, n1, s2, n2);\n>> +     else\n>> +             return (n1 == n2) && !memcmp(s1, s2, n1);\n>> +}\n>> +\n>\n> I think this still is an abstraction at the wrong level.  For one thing,\n> if ignore-whitespace is set, you do not even need nor want to do the \"fix\n> only the ws breakages we are going to fix anyway according to the ws_rule\"\n> transformation applied to the preimage.\n\nI've thought some more about this, and you are right. We still want to\nws fix the postimage, but that's done elsewhere.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117385","messageId":"cb7bb73a0907021228q7e9d2791vafead8e0c5b06b79@mail.gmail.com","threadId":"20007","inReplyTo":"cb7bb73a0907021202ra322425pc64b54953f4f544d@mail.gmail.com","subject":"Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T19:28:47Z","receivedAt":"2009-07-02T19:28:47Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Jul 2, 2009 at 9:02 PM, Giuseppe\nBilotta<giuseppe.bilotta@gmail.com> wrote:\n> On Thu, Jul 2, 2009 at 8:27 PM, Junio C Hamano<gitster@pobox.com> wrote:\n>>> +/*\n>>> + * Returns true if the given lines (buffer + len) match\n>>> + * according to the ignore_whitespace setting\n>>> + */\n>>> +static int lines_match(const char *s1, size_t n1, const char *s2, size_t n2)\n>>> +{\n>>> +     if (ignore_whitespace)\n>>> +             return !memcmp_ignore_whitespace(s1, n1, s2, n2);\n>>> +     else\n>>> +             return (n1 == n2) && !memcmp(s1, s2, n1);\n>>> +}\n>>> +\n>>\n>> I think this still is an abstraction at the wrong level.  For one thing,\n>> if ignore-whitespace is set, you do not even need nor want to do the \"fix\n>> only the ws breakages we are going to fix anyway according to the ws_rule\"\n>> transformation applied to the preimage.\n>\n> I've thought some more about this, and you are right. We still want to\n> ws fix the postimage, but that's done elsewhere.\n\nSorry for repying to myself here, but I'm not convinced again. Or to\nbe more specific: I think this kind of refactoring is totally out of\nthe scope of this patch. So although I agree with you in priciple, if\nyou don't mind I'll keep the first two patches simpler and less\ninvasive. I'll look into the refactoring as a third step.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117386","messageId":"7vhbxu6f87.fsf@alter.siamese.dyndns.org","threadId":"20007","inReplyTo":"cb7bb73a0907021228q7e9d2791vafead8e0c5b06b79@mail.gmail.com","subject":"Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-02T19:45:44Z","receivedAt":"2009-07-02T19:45:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n> Sorry for repying to myself here, but I'm not convinced again. Or to\n> be more specific: I think this kind of refactoring is totally out of\n> the scope of this patch. So although I agree with you in priciple, if\n> you don't mind I'll keep the first two patches simpler and less\n> invasive. I'll look into the refactoring as a third step.\n\nI am not interested in applying \"this adds a broken ignore-whitespace\noption, but as long as you do not use it, it does not hurt the carefully\ncrafted apply-with-context-whose-ws-breakage-was-fixed codepath.\"  For\nexample, I am not convinced at all that your patch does not break the \nupdate_pre_post_images() nor do I know what text the final pre/postimage\nwill happen to end up with.  In other words, I do not see a clear logic in\nthe change.\n\nAlso about the broken \"only prefix matches\" loop, I do not understand why\nyou would want to consider this preimage from the patch\n\n\t\"this  has extra whitespace and other stuff\\n\"\n\nmatches the target line\n\n\t\"this has extra whitespace\\n\"\n\nonly because the prefix matches.\n\nFor that matter, I do not think I understand why the leading whitespaces\nof s1 and s2 are skipped only when they both begin with a whitespace,\neither.\n\nI do not want to be looking at this series until it gets into a bit more\nreviewable shape, which I would expect to take at least a week if you are\nnot working on this full-time (and I presume nobody on the git list is).\n\nPlease do not Cc me in the meantime, but please do ask for help from other\npeople interested in this topic on the list.\n"},{"id":"117388","messageId":"cb7bb73a0907021333t6f377d61v1c1479c15b72c436@mail.gmail.com","threadId":"20007","inReplyTo":"7vhbxu6f87.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T20:33:10Z","receivedAt":"2009-07-02T20:33:10Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Jul 2, 2009 at 9:45 PM, Junio C Hamano<gitster@pobox.com> wrote:\n> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>\n>> Sorry for repying to myself here, but I'm not convinced again. Or to\n>> be more specific: I think this kind of refactoring is totally out of\n>> the scope of this patch. So although I agree with you in priciple, if\n>> you don't mind I'll keep the first two patches simpler and less\n>> invasive. I'll look into the refactoring as a third step.\n>\n> I am not interested in applying \"this adds a broken ignore-whitespace\n> option, but as long as you do not use it, it does not hurt the carefully\n> crafted apply-with-context-whose-ws-breakage-was-fixed codepath.\"\n\nPerfectly agreed. I'm not asking you to apply the patches as they are,\nI sent them to the list for review, and CC'ed you since I couldn't\nidentify a git-apply maintainer.\n\n> For\n> example, I am not convinced at all that your patch does not break the\n> update_pre_post_images() nor do I know what text the final pre/postimage\n> will happen to end up with.  In other words, I do not see a clear logic in\n> the change.\n\nAs you yourself pointed out in the previous email, when ignoring\nwhitespace the code wouldn't hit the whitespace fix path at all.\nIndeed, I've updated the patch to this effect right after reading your\nprevious email.\n\n> Also about the broken \"only prefix matches\" loop, I do not understand why\n> you would want to consider this preimage from the patch\n>\n>        \"this  has extra whitespace and other stuff\\n\"\n>\n> matches the target line\n>\n>        \"this has extra whitespace\\n\"\n>\n> only because the prefix matches.\n\nThat's totally not what I meant. The other way around though, i.e. a preimage of\n        \"this has extra whitespace  \\n\"\nshould match against\n        \"this  has extra whitespace\\nand other stuff\\n\"\n\nAnd of course\n        \"this has extra whitespace  \\n\"\nshould fail against\n        \"this  has extra whitespace and other stuff\\n\"\n\nBoth cases behave correctly (I just extended the test to include this case, btw)\n\nA more interesting question would be if a missing EOL at EOF should be\ntreated as whitespace difference or not.\n\n> For that matter, I do not think I understand why the leading whitespaces\n> of s1 and s2 are skipped only when they both begin with a whitespace,\n> either.\n\nOh, thanks, when first adapting Robert's patch I hadn't considered it\nmissed this case. I've updated the patch accordingly and added a test\ncase for it.\n\n> I do not want to be looking at this series until it gets into a bit more\n> reviewable shape, which I would expect to take at least a week if you are\n> not working on this full-time (and I presume nobody on the git list is).\n\nI do wonder what makes the patch 'unreviewable', since I assume that\ndoesn't mean just 'does not include the refactoring I requested'.\n\n> Please do not Cc me in the meantime, but please do ask for help from other\n> people interested in this topic on the list.\n\nI thought it appropriate to cc you for this specific reply since I was\naddressing the doubts you raised, but I will not include you in the\nnext resend of the series, as per your request. Thanks for your\npatience anyway.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117390","messageId":"7vws6q3in0.fsf@alter.siamese.dyndns.org","threadId":"20007","inReplyTo":"cb7bb73a0907021333t6f377d61v1c1479c15b72c436@mail.gmail.com","subject":"Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-02T21:00:19Z","receivedAt":"2009-07-02T21:00:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n\n> I do wonder what makes the patch 'unreviewable', since I assume that\n> doesn't mean just 'does not include the refactoring I requested'.\n\nOk, unreviewable may not be the word, and \"it is apparent that the patch\nhas not been well thought through\" is the word I would want.\n\nIOW, it really should have been marked as \"[RFC/PATCH]\".\n"},{"id":"117392","messageId":"cb7bb73a0907021405r15639624y7fdbb2a0aed85b27@mail.gmail.com","threadId":"20007","inReplyTo":"7vws6q3in0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-02T21:05:40Z","receivedAt":"2009-07-02T21:05:40Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Jul 2, 2009 at 11:00 PM, Junio C Hamano<gitster@pobox.com> wrote:\n> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:\n>\n>> I do wonder what makes the patch 'unreviewable', since I assume that\n>> doesn't mean just 'does not include the refactoring I requested'.\n>\n> Ok, unreviewable may not be the word, and \"it is apparent that the patch\n> has not been well thought through\" is the word I would want.\n>\n> IOW, it really should have been marked as \"[RFC/PATCH]\".\n\nAh yes, I keep forgetting about that. I remembered it on the very\nfirst try (actually, I wrote PATCH/RFC), but then I forgot about it in\nthe next rounds.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117394","messageId":"7vhbxuzlkk.fsf@alter.siamese.dyndns.org","threadId":"20007","inReplyTo":"cb7bb73a0907021333t6f377d61v1c1479c15b72c436@mail.gmail.com","subject":"Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-02T23:55:55Z","receivedAt":"2009-07-02T23:55:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"By the way, I think we need to make sure your understanding of how the\ncurrent code works matches mine before you go any further.\n\nAre the words \"preimage\", \"postimage\" and \"target\" used consistently\nbetween us?  By these words, I mean:\n\n  preimage = the lines prefixed with '-' and ' ' in the patch\n\n  postimage = the lines prefixed with ' ' and '+' in the patch\n\n  target = lines in the file being patched that corresponds to the preimage\n\nThe point of patch application is to find a block of lines in the target\nthat matches preimage, and replace that block with postimage.  When the\npatch applies cleanly (which is the case we should optimize for), the\npreimage match the target byte-for-byte.  The hunk starting at line 1690\ndoes a memcmp of the whole thing, without ws fuzz, for this reason.  You\ndo not want to touch that part with your patch (and that is why I am\nwriting this message to make sure you understand what you are doing).\n\nAfter that, as a fallback, we compare line-by-line, while fixing the\nwhitespace breakage in the preimage (what the patch author based on) and\nthe target (what we currently have).  The reason for the loop is because\nwe are interested in two cases:\n\n (1) The patch was made against an old code without recent whitespace fix\n     we already have.\n\n (2) The patch was made against a code with whitespace fix we do not have\n     yet.\n\nIn either case, preimage and target won't match byte-for-byte, but by\napplying the whitespace breakage on each of the preimage line and the\ncorresponding target line, they will match in either of the above cases.\nWhile doing this \"convert-and-match\", we prepare a version of preimage\nwith whitespace breakage fixed to give to update_pre_post_images() at the\nend of the function in fixed_buf.\n\nThe contents of fixed_buf is used to update the preimage and the postimage\nby calling update_pre_post_images().  This is to avoid reverting the\nwhitespace fix we already had in the target when we are in situation (1).\nThe postimage is what replaces the block of lines in the image that\nmatched the preimage, so this step is essential.\n\nThis is another point I am worried about your patch.  Suppose you have this\ntarget:\n\n    a a a\n    b b b\n    c c\n    d\n    e e\n\nAnd we have a broken patch that needs --ignore-whitespace to apply:\n\n    diff --git a/file b/file\n    index xxxxxx..yyyyyy 100644\n    @@ -1,4, +1,5 @@\n     a  a  a\n     b b  b\n    +q\n     c  c\n       d\n\nYour preimage is \"a  a  a\\nb b  b\\nc  c\\n  d\\n\",\ntarget is        \"a a a\\nb b b\\nc c\\nd\\ne e\\n\",\nand postimage is \"a  a  a\\nb b  b\\nq\\nc  c\\n  d\\n\".\n\nWouldn't you want to have this as the result of patch application?\n\n    a a a\n    b b b\n    q\n    c c\n    d\n    e e\n\nWith whitespace squashed, the preimage would match the target (perhaps\nafter fixing line_matches()), but wsfix_copy() called while we fix each\npreimage line won't have changed anything in the fixed_buf that is to\nbecome the new preimage, and update_pre_post_images() while copying the\nfixed preimage to the postimage won't have corrected \"a a a\" back to \"a a\na\" that was in the target as the result.\n\nSo I suspect that you would instead end up with:\n\n    a  a  a\n    b b  b\n    c  c\n      d\n    e e\n\nI think the intent of --ignore-whitespace is \"don't worry about ws\ndifferences in the context when locating where to make the change\", and it\nis not \"I do not care about getting whitespace mangled anywhere in the\nfile the patch touches.\"  correct_ws_error is special in that we can\nafford to take the fixed pre/postimage, \"because we are fixing the ws\nbreakage anyway\", but arguably it _might_ be nicer to limit the change to\nthe lines marked with '-' and '+' in the patch even in that case.\n"},{"id":"117405","messageId":"cb7bb73a0907022340l88a226egad74a275055fb972@mail.gmail.com","threadId":"20007","inReplyTo":"7vhbxuzlkk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-03T06:40:01Z","receivedAt":"2009-07-03T06:40:01Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Jul 3, 2009 at 1:55 AM, Junio C Hamano<gitster@pobox.com> wrote:\n> By the way, I think we need to make sure your understanding of how the\n> current code works matches mine before you go any further.\n\nSouns reasonable.\n\n> Are the words \"preimage\", \"postimage\" and \"target\" used consistently\n> between us?  By these words, I mean:\n>\n>  preimage = the lines prefixed with '-' and ' ' in the patch\n>\n>  postimage = the lines prefixed with ' ' and '+' in the patch\n>\n>  target = lines in the file being patched that corresponds to the preimage\n\nIt _did_ take me a little to understand the names when I started\nworking on the feature, but I got on track pretty soon (at the first\nsegfault ;-)).\n\n> The point of patch application is to find a block of lines in the target\n> that matches preimage, and replace that block with postimage.  When the\n> patch applies cleanly (which is the case we should optimize for), the\n> preimage match the target byte-for-byte.  The hunk starting at line 1690\n> does a memcmp of the whole thing, without ws fuzz, for this reason.  You\n> do not want to touch that part with your patch (and that is why I am\n> writing this message to make sure you understand what you are doing).\n\nOf course.\n\n> After that, as a fallback, we compare line-by-line, while fixing the\n> whitespace breakage in the preimage (what the patch author based on) and\n> the target (what we currently have).\n\n> [...] preimage and target won't match byte-for-byte, but by\n> applying the whitespace breakage on each of the preimage line and the\n> corresponding target line, they will match in either of the above cases.\n> While doing this \"convert-and-match\", we prepare a version of preimage\n> with whitespace breakage fixed to give to update_pre_post_images() at the\n> end of the function in fixed_buf.\n\nIndeed. This is why in my 2/2 patch I do a similar operation to bring\nthe preimage whitespace to match the target whitespace if matching was\ndone ignoring whitespace (but we never got to that part, for obvious\nreasons).\n\n> This is another point I am worried about your patch.  Suppose you have this\n> target:\n>\n>    a a a\n>    b b b\n>    c c\n>    d\n>    e e\n>\n> And we have a broken patch that needs --ignore-whitespace to apply:\n>\n>    diff --git a/file b/file\n>    index xxxxxx..yyyyyy 100644\n>    @@ -1,4, +1,5 @@\n>     a  a  a\n>     b b  b\n>    +q\n>     c  c\n>       d\n>\n> Your preimage is \"a  a  a\\nb b  b\\nc  c\\n  d\\n\",\n> target is        \"a a a\\nb b b\\nc c\\nd\\ne e\\n\",\n> and postimage is \"a  a  a\\nb b  b\\nq\\nc  c\\n  d\\n\".\n>\n> Wouldn't you want to have this as the result of patch application?\n>\n>    a a a\n>    b b b\n>    q\n>    c c\n>    d\n>    e e\n>\n> With whitespace squashed, the preimage would match the target (perhaps\n> after fixing line_matches()), but wsfix_copy() called while we fix each\n> preimage line won't have changed anything in the fixed_buf that is to\n> become the new preimage, and update_pre_post_images() while copying the\n> fixed preimage to the postimage won't have corrected \"a a a\" back to \"a a\n> a\" that was in the target as the result.\n>\n> So I suspect that you would instead end up with:\n>\n>    a  a  a\n>    b b  b\n>    c  c\n>      d\n>    e e\n\nThis is indeed the case with my 1/2 patch: no whitespace adjustment is\ndone on the pre- and postimage when the preimage and target match with\nwhitespace fuzz and ignore_whitespace is active. In the first RFC I\nsent I expressely mentioned that this was not what I liked about my\npatch. When I first sent a _series_, it was made of two patches, the\nsecond of which served the purpose of realigning the whitespaces of\nthe patch (pre and postimage) to the whitespaces of the target (at\nleast for the common lines).\n\n> I think the intent of --ignore-whitespace is \"don't worry about ws\n> differences in the context when locating where to make the change\", and it\n> is not \"I do not care about getting whitespace mangled anywhere in the\n> file the patch touches.\"\n\nI totally agree. This is important because it also means that when\nre-diffing the applied patch you still get changes ONLY in the lines\nwhere you SHOULD get changes, and not in the nearby context that only\nhad different whitespace.\n\nI sent the thing in two patches to make it easier to review. If you\nthink it's more appropriate to squash them, I can do that no problem.\n\n> correct_ws_error is special in that we can\n> afford to take the fixed pre/postimage, \"because we are fixing the ws\n> breakage anyway\", but arguably it _might_ be nicer to limit the change to\n> the lines marked with '-' and '+' in the patch even in that case.\n\nBut that's a path we're not going to hit in match_fragment when\nignoring whitespace. Instead, one thing we could consider in this case\n(ignore_whitespace) is to adjust the leading space in the + lines to\nmatch the ws transformations done in the context lines, but that might\nbe making whitespace fixing a little too far. Or we should rename it\nto whitespace=adjust...\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}