{"thread":{"id":"22801","subject":"[PATCH v2 2/4] apply: Allow blank context lines to match beyond EOF","startedAt":"2010-02-24T19:24:20Z","lastAt":"2010-02-24T23:02:52Z","messageCount":3,"participants":["Björn Gustavsson","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"135601","messageId":"4B857CE4.4000201@gmail.com","threadId":"22801","inReplyTo":null,"subject":"[PATCH v2 2/4] apply: Allow blank context lines to match beyond EOF","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2010-02-24T19:24:20Z","receivedAt":"2010-02-24T19:24:20Z","isPatch":true,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"\"git apply --whitespace=fix\" will not always succeed when used\non a series of patches in the following circumstances:\n\n* One patch adds a blank line at the end of a file. (Since\n  --whitespace=fix is used, the blank line will *not* be added.)\n\n* The next patch adds non-blanks lines after the blank line\n  introduced in the first patch. That patch will not apply\n  because the blank line that is expected to be found at end\n  of the file is no longer there.\n\nA patch series that starts by deleting lines at the end\nwill fail in a similar way.\n\nFix this problem by allowing a blank context line at the beginning\nof a hunk to match if parts of it falls beyond end of the file.\nWe still require that at least one non-blank context line match\nbefore the end of the file.\n\nSigned-off-by: Björn Gustavsson <bgustavsson@gmail.com>\n---\n builtin-apply.c |  135 +++++++++++++++++++++++++++++++++++++++++++++---------\n 1 files changed, 112 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex e1f849d..b8b89f7 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1854,33 +1854,81 @@ static int match_fragment(struct image *img,\n {\n \tint i;\n \tchar *fixed_buf, *buf, *orig, *target;\n+\tint limit;\n+\tint preimage_limit;\n \n-\tif (preimage->nr + try_lno > img->nr)\n+\tif (preimage->nr + try_lno <= img->nr) {\n+\t\t/*\n+\t\t * The hunk falls within the boundaries of img.\n+\t\t */\n+\t\tlimit = img->nr;\n+\t\tpreimage_limit = preimage->nr;\n+\t} else if (ws_error_action == correct_ws_error &&\n+\t\t   ws_rule & WS_BLANK_AT_EOF && match_end) {\n+\t\t/*\n+\t\t * This hunk that matches at the end, extends beyond\n+\t\t * the end of img and we are removing blanks line at\n+\t\t * the end of the file. Set up the limits so that\n+\t\t * tests below will pass and the quick hash test\n+\t\t * will only test the lines up to the last line\n+\t\t * in img.\n+\t\t */\n+\t\tlimit = try_lno + preimage->nr;\n+\t\tpreimage_limit = img->nr - try_lno;\n+\t} else {\n+\t\t/*\n+\t\t * The hunk extends beyond the end of the img and\n+\t\t * we are not removing blanks at the end, so we\n+\t\t * should reject the hunk at this position.\n+\t\t */\n \t\treturn 0;\n+\t}\n \n \tif (match_beginning && try_lno)\n \t\treturn 0;\n \n-\tif (match_end && preimage->nr + try_lno != img->nr)\n+\tif (match_end && preimage->nr + try_lno != limit)\n \t\treturn 0;\n \n \t/* Quick hash check */\n-\tfor (i = 0; i < preimage->nr; i++)\n+\tfor (i = 0; i < preimage_limit; i++)\n \t\tif (preimage->line[i].hash != img->line[try_lno + i].hash)\n \t\t\treturn 0;\n \n-\t/*\n-\t * Do we have an exact match?  If we were told to match\n-\t * at the end, size must be exactly at try+fragsize,\n-\t * otherwise try+fragsize must be still within the preimage,\n-\t * and either case, the old piece should match the preimage\n-\t * exactly.\n-\t */\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\treturn 1;\n+\tif (preimage_limit == preimage->nr) {\n+\t\t/*\n+\t\t * Do we have an exact match?  If we were told to match\n+\t\t * at the end, size must be exactly at try+fragsize,\n+\t\t * otherwise try+fragsize must be still within the preimage,\n+\t\t * and either case, the old piece should match the preimage\n+\t\t * exactly.\n+\t\t */\n+\t\tif ((match_end\n+\t\t     ? (try + preimage->len == img->len)\n+\t\t     : (try + preimage->len <= img->len)) &&\n+\t\t    !memcmp(img->buf + try, preimage->buf, preimage->len))\n+\t\t\treturn 1;\n+\t} else {\n+\t\t/*\n+\t\t * The preimage extends beyond the end of img, so\n+\t\t * there cannot be an exact match.\n+\t\t *\n+\t\t * There must be one non-blank context line that match\n+\t\t * a line before the of img.\n+\t\t */\n+\t\tchar *buf_end;\n+\n+\t\tbuf = preimage->buf; \n+\t\tbuf_end = buf;\n+\t\tfor (i = 0; i < preimage_limit; i++)\n+\t\t\tbuf_end += preimage->line[i].len;\n+\n+\t\tfor ( ; buf < buf_end; buf++)\n+\t\t\tif (!isspace(*buf))\n+\t\t\t\tbreak;\n+\t\tif (buf == buf_end)\n+\t\t\treturn 0;\n+\t}\n \n \t/*\n \t * No exact match. If we are ignoring whitespace, run a line-by-line\n@@ -1932,12 +1980,16 @@ static int match_fragment(struct image *img,\n \t * it might with whitespace fuzz. We haven't been asked to\n \t * ignore whitespace, we were asked to correct whitespace\n \t * errors, so let's try matching after whitespace correction.\n+\t *\n+\t * The preimage may extend beyond the end of the file,\n+\t * but in this loop we will only handle the part of the\n+\t * preimage that falls within the file.\n \t */\n \tfixed_buf = xmalloc(preimage->len + 1);\n \tbuf = fixed_buf;\n \torig = preimage->buf;\n \ttarget = img->buf + try;\n-\tfor (i = 0; i < preimage->nr; i++) {\n+\tfor (i = 0; i < preimage_limit; i++) {\n \t\tsize_t fixlen; /* length after fixing the preimage */\n \t\tsize_t oldlen = preimage->line[i].len;\n \t\tsize_t tgtlen = img->line[try_lno + i].len;\n@@ -1977,6 +2029,29 @@ static int match_fragment(struct image *img,\n \t\ttarget += tgtlen;\n \t}\n \n+\n+\t/*\n+\t * Now handle the lines in the preimage that falls beyond the\n+\t * end of the file (if any). They will only match if they are\n+\t * empty or only contain whitespace (if WS_BLANK_AT_EOL is\n+\t * false).\n+\t */\n+\tfor ( ; i < preimage->nr; i++) {\n+\t\tsize_t fixlen; /* length after fixing the preimage */\n+\t\tsize_t oldlen = preimage->line[i].len;\n+\t\tint j;\n+\n+\t\t/* Try fixing the line in the preimage */\n+\t\tfixlen = ws_fix_copy(buf, orig, oldlen, ws_rule, NULL);\n+\n+\t\tfor (j = 0; j < fixlen; j++)\n+\t\t\tif (!isspace(buf[j]))\n+\t\t\t\tgoto unmatch_exit;\n+\n+\t\torig += oldlen;\n+\t\tbuf += fixlen;\n+\t}\n+\n \t/*\n \t * Yes, the preimage is based on an older version that still\n \t * has whitespace breakages unfixed, and fixing them makes the\n@@ -2092,12 +2167,26 @@ static void update_image(struct image *img,\n \tint i, nr;\n \tsize_t remove_count, insert_count, applied_at = 0;\n \tchar *result;\n+\tint preimage_limit;\n+\n+\t/*\n+\t * If we are removing blank lines at the end of img,\n+\t * the preimage may extend beyond the end.\n+\t * If that is the case, we must be careful only to\n+\t * remove the part of the preimage that falls within\n+\t * the boundaries of img. Initialize preimage_limit\n+\t * to the number of lines in the preimage that falls\n+\t * within the boundaries.\n+\t */\n+\tpreimage_limit = preimage->nr;\n+\tif (preimage_limit > img->nr - applied_pos)\n+\t\tpreimage_limit = img->nr - applied_pos;\n \n \tfor (i = 0; i < applied_pos; i++)\n \t\tapplied_at += img->line[i].len;\n \n \tremove_count = 0;\n-\tfor (i = 0; i < preimage->nr; i++)\n+\tfor (i = 0; i < preimage_limit; i++)\n \t\tremove_count += img->line[applied_pos + i].len;\n \tinsert_count = postimage->len;\n \n@@ -2114,8 +2203,8 @@ static void update_image(struct image *img,\n \tresult[img->len] = '\\0';\n \n \t/* Adjust the line table */\n-\tnr = img->nr + postimage->nr - preimage->nr;\n-\tif (preimage->nr < postimage->nr) {\n+\tnr = img->nr + postimage->nr - preimage_limit;\n+\tif (preimage_limit < postimage->nr) {\n \t\t/*\n \t\t * NOTE: this knows that we never call remove_first_line()\n \t\t * on anything other than pre/post image.\n@@ -2123,10 +2212,10 @@ static void update_image(struct image *img,\n \t\timg->line = xrealloc(img->line, nr * sizeof(*img->line));\n \t\timg->line_allocated = img->line;\n \t}\n-\tif (preimage->nr != postimage->nr)\n+\tif (preimage_limit != postimage->nr)\n \t\tmemmove(img->line + applied_pos + postimage->nr,\n-\t\t\timg->line + applied_pos + preimage->nr,\n-\t\t\t(img->nr - (applied_pos + preimage->nr)) *\n+\t\t\timg->line + applied_pos + preimage_limit,\n+\t\t\t(img->nr - (applied_pos + preimage_limit)) *\n \t\t\tsizeof(*img->line));\n \tmemcpy(img->line + applied_pos,\n \t       postimage->line,\n@@ -2322,7 +2411,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \n \tif (applied_pos >= 0) {\n \t\tif (new_blank_lines_at_end &&\n-\t\t    preimage.nr + applied_pos == img->nr &&\n+\t\t    preimage.nr + applied_pos >= img->nr &&\n \t\t    (ws_rule & WS_BLANK_AT_EOF) &&\n \t\t    ws_error_action != nowarn_ws_error) {\n \t\t\trecord_ws_error(WS_BLANK_AT_EOF, \"+\", 1, frag->linenr);\n-- \n1.7.0\n"},{"id":"135614","messageId":"7vaauyfj3k.fsf@alter.siamese.dyndns.org","threadId":"22801","inReplyTo":"4B857CE4.4000201@gmail.com","subject":"Re: [PATCH v2 2/4] apply: Allow blank context lines to match beyond EOF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-24T20:56:47Z","receivedAt":"2010-02-24T20:56:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Björn Gustavsson <bgustavsson@gmail.com> writes:\n\n> \"git apply --whitespace=fix\" will not always succeed when used\n> on a series of patches in the following circumstances:\n\nVery nicely done.\n\nI wanted to explain to myself what \"limit\" and \"preimage_limit\" variables\nmean.  preimage-limit has a very clear definition: this many lines from\nthe beginning of preimage must match img, and the remainder of the\npreimage must be all blank (the remainder can exist only when we are\ntrying to match at the end).\n\nOn the other hand \"limit\" does not have such a good definition, other than\nas a work around to bypass line-number check when we are trying to match\nat the end.  It might be cleaner to read if we move the problematic \"line\nnumbers must match\" logic and eliminate this variable, like the attached\npatch does on top of this one.\n\nI couldn't figure out how this would interact with the ignore_ws_change\ncodepath, though.  That one shows a clear sign of being bolted on as an\nafterthought (once you fall into that \"if()\" statement you will not come\nback).\n\n builtin-apply.c |   15 +++++----------\n 1 files changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 4374d33..ae4452c 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1854,26 +1854,24 @@ static int match_fragment(struct image *img,\n {\n \tint i;\n \tchar *fixed_buf, *buf, *orig, *target;\n-\tint limit;\n \tint preimage_limit;\n \n \tif (preimage->nr + try_lno <= img->nr) {\n \t\t/*\n \t\t * The hunk falls within the boundaries of img.\n \t\t */\n-\t\tlimit = img->nr;\n \t\tpreimage_limit = preimage->nr;\n+\t\tif (match_end && (preimage->nr + try_lno != img->nr))\n+\t\t\treturn 0;\n \t} else if (ws_error_action == correct_ws_error &&\n \t\t   (ws_rule & WS_BLANK_AT_EOF) && match_end) {\n \t\t/*\n \t\t * This hunk that matches at the end extends beyond\n \t\t * the end of img, and we are removing blank lines\n-\t\t * at the end of the file. Set up the limits so that\n-\t\t * tests below will pass and the quick hash test\n-\t\t * will only test the lines up to the last line\n-\t\t * in img.\n+\t\t * at the end of the file.  This many lines from the\n+\t\t * beginning of the preimage must match with img, and\n+\t\t * the remainder of the preimage must be blank.\n \t\t */\n-\t\tlimit = try_lno + preimage->nr;\n \t\tpreimage_limit = img->nr - try_lno;\n \t} else {\n \t\t/*\n@@ -1887,9 +1885,6 @@ static int match_fragment(struct image *img,\n \tif (match_beginning && try_lno)\n \t\treturn 0;\n \n-\tif (match_end && preimage->nr + try_lno != limit)\n-\t\treturn 0;\n-\n \t/* Quick hash check */\n \tfor (i = 0; i < preimage_limit; i++)\n \t\tif (preimage->line[i].hash != img->line[try_lno + i].hash)\n"},{"id":"135624","messageId":"6672d0161002241502h2f80b511j445465fdc2fd16ab@mail.gmail.com","threadId":"22801","inReplyTo":"7vaauyfj3k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/4] apply: Allow blank context lines to match beyond EOF","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2010-02-24T23:02:52Z","receivedAt":"2010-02-24T23:02:52Z","isPatch":true,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"2010/2/24 Junio C Hamano <gitster@pobox.com>:\n> Very nicely done.\n\nThanks! :)\n\n> On the other hand \"limit\" does not have such a good definition, other than\n> as a work around to bypass line-number check when we are trying to match\n> at the end.  It might be cleaner to read if we move the problematic \"line\n> numbers must match\" logic and eliminate this variable, like the attached\n> patch does on top of this one.\n\nYes, your version is better. Having a \"limit\" variable no longer makes\nsense (my original patch used \"limit\" in two places). Feel free to\nsqueeze it in.\n\n> I couldn't figure out how this would interact with the ignore_ws_change\n> codepath, though.  That one shows a clear sign of being bolted on as an\n> afterthought (once you fall into that \"if()\" statement you will not come\n> back).\n\nYes, it does seem bolted on.\n\nI haven't looked much at that if() statement, because I\nsort of assumed that because of the return it couldn't do any\nharm.\n\nIt is too late in my time zone for me to think clearly, but it does\nseem that I was wrong and that I'll need to do some changes in\nthat \"if()\" statement, and also write some more tests for\nthe combination of --whitespace=fix and --ignore-space-change.\n\nI'll be back another day.\n\nThanks for the review.\n\n-- \nBjörn Gustavsson, Erlang/OTP, Ericsson AB\n"}]}