{"thread":{"id":"46959","subject":"Out of memory with diff.colormoved enabled","startedAt":"2017-10-12T19:53:30Z","lastAt":"2017-10-20T22:37:15Z","messageCount":34,"participants":["Orgad Shaneh","Jeff King","Stefan Beller","Junio C Hamano","Simon Ruderich"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"330299","messageId":"CAGHpTBLQ8vi00e3Tt3KTrCfaAWhZKQX6u6Ca17t2ySVQdoGc5g@mail.gmail.com","threadId":"46959","inReplyTo":null,"subject":"Out of memory with diff.colormoved enabled","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2017-10-12T19:53:23Z","receivedAt":"2017-10-12T19:53:30Z","isPatch":false,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"Hi,\n\ngit version 2.15.0.rc0 (from debian sid package)\n\nThere is an infinite loop when colormoved is used with --ignore-space-change:\n\ngit init\nseq 20 > test\ngit add test\nsed -i 's/9/42/' test\ngit -c diff.colormoved diff --ignore-space-change -- test\n\n- Orgad\n"},{"id":"330300","messageId":"20171012200536.m6oz4zrjcze3yw4i@sigill.intra.peff.net","threadId":"46959","inReplyTo":"CAGHpTBLQ8vi00e3Tt3KTrCfaAWhZKQX6u6Ca17t2ySVQdoGc5g@mail.gmail.com","subject":"Re: Out of memory with diff.colormoved enabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-12T20:05:36Z","receivedAt":"2017-10-12T20:05:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 12, 2017 at 10:53:23PM +0300, Orgad Shaneh wrote:\n\n> There is an infinite loop when colormoved is used with --ignore-space-change:\n> \n> git init\n> seq 20 > test\n> git add test\n> sed -i 's/9/42/' test\n> git -c diff.colormoved diff --ignore-space-change -- test\n\nThanks for an easy reproduction recipe.\n\nIt looks like the problem is that next_byte() doesn't make any forward\nprogress in the buffer with --ignore-space-change. We try to convert\nwhitespace into a single space (I'm not sure why, but I'm not very\nfamiliar with this part of the code). But if there's no space, then the\n\"cp\" pointer never gets advanced.\n\nThis fixes it, but I have no idea if it's doing the right thing:\n\ndiff --git a/diff.c b/diff.c\nindex 69f03570ad..e8dedc7357 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -713,13 +713,17 @@ static int next_byte(const char **cp, const char **endp,\n \t\treturn -1;\n \n \tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {\n-\t\twhile (*cp < *endp && isspace(**cp))\n+\t\tint saw_whitespace = 0;\n+\t\twhile (*cp < *endp && isspace(**cp)) {\n \t\t\t(*cp)++;\n+\t\t\tsaw_whitespace = 1;\n+\t\t}\n \t\t/*\n \t\t * After skipping a couple of whitespaces, we still have to\n \t\t * account for one space.\n \t\t */\n-\t\treturn (int)' ';\n+\t\tif (saw_whitespace)\n+\t\t\treturn (int)' ';\n \t}\n \n \tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {\n\nI guess it would be equally correct to not enter that if-block unless\nisspace(**cp).\n\n-Peff\n"},{"id":"330312","messageId":"CAGZ79kZ=v0GiBfNPK=iRp7Hi=zG=KUL6+jHuc3N5CRqtECt7fw@mail.gmail.com","threadId":"46959","inReplyTo":"20171012200536.m6oz4zrjcze3yw4i@sigill.intra.peff.net","subject":"Re: Out of memory with diff.colormoved enabled","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-12T22:39:03Z","receivedAt":"2017-10-12T22:39:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 12, 2017 at 1:05 PM, Jeff King wrote:\n> On Thu, Oct 12, 2017 at 10:53:23PM +0300, Orgad Shaneh wrote:\n>\n>> There is an infinite loop when colormoved is used with --ignore-space-change:\n>>\n>> git init\n>> seq 20 > test\n>> git add test\n>> sed -i 's/9/42/' test\n>> git -c diff.colormoved diff --ignore-space-change -- test\n>\n> Thanks for an easy reproduction recipe.\n\nThanks here as well!\n\n> It looks like the problem is that next_byte() doesn't make any forward\n> progress in the buffer with --ignore-space-change. We try to convert\n> whitespace into a single space\n\n> (I'm not sure why, but I'm not very\n> familiar with this part of the code).\n\n(on why you don't feel familiar)\nBecause it is quite new, and you weren't one of the main reviewers.\n2e2d5ac184 (diff.c: color moved lines differently, 2017-06-30) was also very\nlarge, such that it is easy to overlook. Though I remember Junio and me\ndiscussing the next_byte part quite vividly. :/\n\n(On why it is the way it is)\nConsider the three strings\n one SP word\n one TAB word\n oneword\n\nThe first two are equal, but the third is different with\n`--ignore-space-change` given. To achieve that goal,\nthe easiest thing to do (in my mind) was to replace\nany sequence of blank characters by \"a standard\nspace\" sequence. That will make all strings with different\nwhite space sequences compare equal, but the one\nwithout blanks will be different.\n\n> But if there's no space, then the\n> \"cp\" pointer never gets advanced.\n\nRight, because of the early return, skipping the increment of *cp\n\n> This fixes it, but I have no idea if it's doing the right thing:\n>\n> diff --git a/diff.c b/diff.c\n> index 69f03570ad..e8dedc7357 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -713,13 +713,17 @@ static int next_byte(const char **cp, const char **endp,\n> return -1;\n>\n> if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {\n> - while (*cp < *endp && isspace(**cp))\n> + int saw_whitespace = 0;\n> + while (*cp < *endp && isspace(**cp)) {\n> (*cp)++;\n> + saw_whitespace = 1;\n> + }\n> /*\n> * After skipping a couple of whitespaces, we still have to\n> * account for one space.\n> */\n> - return (int)' ';\n> + if (saw_whitespace)\n> + return (int)' ';\n\nThe \"else\" is implicit and it falls through to\nthe standard case at the end of the function,\nincrementing *cp, returning the character *cp\npointed at prior to being incremented.\n\nThat sounds correct.\n\n> }\n>\n> if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {\n>\n> I guess it would be equally correct to not enter that if-block unless\n> isspace(**cp).\n\nThis also sounds correct.\n\n>\n> -Peff\n"},{"id":"330316","messageId":"20171012233322.31203-1-sbeller@google.com","threadId":"46959","inReplyTo":"20171012200536.m6oz4zrjcze3yw4i@sigill.intra.peff.net","subject":"[PATCH] diff.c: increment buffer pointer in all code path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-12T23:33:22Z","receivedAt":"2017-10-12T23:33:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The added test would hang up Git due to an infinite loop. The function\n`next_byte()` doesn't make any forward progress in the buffer with\n`--ignore-space-change`.\n\nFix this by only returning early when there was actual white space\nto be covered, fall back to the default case at the end of the function\nwhen there is no white space.\n\nReported-by: Orgad Shaneh \nDebugged-by: Jeff King \nSigned-off-by: Stefan Beller \n---\n\nPeff, feel free to take ownership here. I merely made it to a patch.\n\nThanks,\nStefan\n\n diff.c | 12 ++++++++----\n t/t4015-diff-whitespace.sh | 8 ++++++++\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 69f03570ad..6fe84e6994 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -713,13 +713,17 @@ static int next_byte(const char **cp, const char **endp,\n \t\treturn -1;\n \n \tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {\n-\t\twhile (*cp < *endp && isspace(**cp))\n+\t\tint saw_whitespace = 0;\n+\t\twhile (*cp < *endp && isspace(**cp)) {\n \t\t\t(*cp)++;\n+\t\t\tsaw_whitespace = 1;\n+\t\t}\n \t\t/*\n-\t\t * After skipping a couple of whitespaces, we still have to\n-\t\t * account for one space.\n+\t\t * After skipping a couple of whitespaces,\n+\t\t * we still have to account for one space.\n \t\t */\n-\t\treturn (int)' ';\n+\t\tif (saw_whitespace)\n+\t\t\treturn (int)' ';\n \t}\n \n \tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex bd0f75d9f7..c088ae86af 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1530,4 +1530,12 @@ test_expect_success 'move detection with submodules' '\n \ttest_cmp expect decoded_actual\n '\n \n+test_expect_success 'move detection with whitespace changes' '\n+\ttest_seq 10 > test &&\n+\tgit add test &&\n+\tsed -i \"s/3/42/\" test &&\n+\tgit -c diff.colormoved diff --ignore-space-change -- test &&\n+\tgit reset --hard\n+'\n+\n test_done\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"330320","messageId":"20171013001837.43nx5paeqisbrflq@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171012233322.31203-1-sbeller@google.com","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-13T00:18:37Z","receivedAt":"2017-10-13T00:18:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 12, 2017 at 04:33:22PM -0700, Stefan Beller wrote:\n\n> Peff, feel free to take ownership here. I merely made it to a patch.\n\nI think compared to my original, it makes sense to actually wrap the\nwhole thing with a check for the whitespace. You can do it just in the\nIGNORE_WHITESPACE_CHANGE conditional, but I think it makes more sense to\ncover both. Like the patch below (view with \"-w\" to see the logic more\nclearly).\n\nI also tweaked the test to remove \"sed -i\", which isn't portable, and\nfixed up a few style nits.\n\n-- >8 --\nSubject: diff: fix infinite loop with --color-moved --ignore-space-change\n\nThe --color-moved code uses next_byte() to advance through\nthe blob contents. When the user has asked to ignore\nwhitespace changes, we try to collapse any whitespace change\ndown to a single space.\n\nHowever, we enter the conditional block whenever we see the\nIGNORE_WHITESPACE_CHANGE flag, even if the next byte isn't\nwhitespace.\n\nThis means that the combination of \"--color-moved and\n--ignore-space-change\" was completely broken. Worse, because\nwe return from next_byte() without having advanced our\npointer, the function makes no forward progress in the\nbuffer and loops infinitely.\n\nFix this by entering the conditional only when we actually\nsee whitespace. We can apply this also to the\nIGNORE_WHITESPACE change. That code path isn't buggy\n(because it falls through to returning the next\nnon-whitespace byte), but it makes the logic more clear if\nwe only bother to look at whitespace flags after seeing that\nthe next byte is whitespace.\n\nReported-by: Orgad Shaneh \nSigned-off-by: Jeff King \n---\n diff.c | 28 +++++++++++++++-------------\n t/t4015-diff-whitespace.sh | 9 +++++++++\n 2 files changed, 24 insertions(+), 13 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 69f03570ad..d76bb937c1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -712,20 +712,22 @@ static int next_byte(const char **cp, const char **endp,\n \tif (*cp > *endp)\n \t\treturn -1;\n \n-\tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {\n-\t\twhile (*cp < *endp && isspace(**cp))\n-\t\t\t(*cp)++;\n-\t\t/*\n-\t\t * After skipping a couple of whitespaces, we still have to\n-\t\t * account for one space.\n-\t\t */\n-\t\treturn (int)' ';\n-\t}\n+\tif (isspace(**cp)) {\n+\t\tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {\n+\t\t\twhile (*cp < *endp && isspace(**cp))\n+\t\t\t\t(*cp)++;\n+\t\t\t/*\n+\t\t\t * After skipping a couple of whitespaces,\n+\t\t\t * we still have to account for one space.\n+\t\t\t */\n+\t\t\treturn (int)' ';\n+\t\t}\n \n-\tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {\n-\t\twhile (*cp < *endp && isspace(**cp))\n-\t\t\t(*cp)++;\n-\t\t/* return the first non-ws character via the usual below */\n+\t\tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {\n+\t\t\twhile (*cp < *endp && isspace(**cp))\n+\t\t\t\t(*cp)++;\n+\t\t\t/* return the first non-ws character via the usual below */\n+\t\t}\n \t}\n \n \tretval = (unsigned char)(**cp);\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex bd0f75d9f7..87083f728f 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1530,4 +1530,13 @@ test_expect_success 'move detection with submodules' '\n \ttest_cmp expect decoded_actual\n '\n \n+test_expect_success 'move detection with whitespace changes' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\ttest_seq 10 >test &&\n+\tgit add test &&\n+\tsed s/3/42/ test.tmp &&\n+\tmv test.tmp test &&\n+\tgit -c diff.colormoved diff --ignore-space-change -- test\n+'\n+\n test_done\n-- \n2.15.0.rc1.381.g94fac273d4\n\n"},{"id":"330321","messageId":"20171013002057.froqi54olmhmah6b@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171013001837.43nx5paeqisbrflq@sigill.intra.peff.net","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-13T00:20:57Z","receivedAt":"2017-10-13T00:21:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 12, 2017 at 08:18:37PM -0400, Jeff King wrote:\n\n> Fix this by entering the conditional only when we actually\n> see whitespace. We can apply this also to the\n> IGNORE_WHITESPACE change. That code path isn't buggy\n> (because it falls through to returning the next\n> non-whitespace byte), but it makes the logic more clear if\n> we only bother to look at whitespace flags after seeing that\n> the next byte is whitespace.\n\nI think there actually _is_ a bug in that code path, but it's unrelated\nto this one. If you have whitespace at the end of the buffer, then we'd\nadvance *cp until it matches *endp, and then return whatever is at *endp\n(which is nonsense, or probably a NUL) rather than returning \"-1\".\n\nI'm out of time for tonight and not familiar enough with the color-moved\ncode to come up with a reasonable test case quickly, but maybe you can\nsee if that can trigger bad behavior?\n\n-Peff\n"},{"id":"330323","messageId":"CAGZ79kbBb=MOK5MsDNXeTwvrYPgnmVsfMEcGhMq_iWurkzETQA@mail.gmail.com","threadId":"46959","inReplyTo":"20171013002057.froqi54olmhmah6b@sigill.intra.peff.net","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-13T00:24:41Z","receivedAt":"2017-10-13T00:24:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 12, 2017 at 5:20 PM, Jeff King wrote:\n> On Thu, Oct 12, 2017 at 08:18:37PM -0400, Jeff King wrote:\n>\n>> Fix this by entering the conditional only when we actually\n>> see whitespace. We can apply this also to the\n>> IGNORE_WHITESPACE change. That code path isn't buggy\n>> (because it falls through to returning the next\n>> non-whitespace byte), but it makes the logic more clear if\n>> we only bother to look at whitespace flags after seeing that\n>> the next byte is whitespace.\n>\n> I think there actually _is_ a bug in that code path, but it's unrelated\n> to this one. If you have whitespace at the end of the buffer, then we'd\n> advance *cp until it matches *endp, and then return whatever is at *endp\n> (which is nonsense, or probably a NUL) rather than returning \"-1\".\n\nGood catch! This plays together interestingly with\nIGNORE_WHITESPACE_AT_EOL, too. If that is set the endp is just put\nfurther to the front, so we'd actually compare white spaces after endp.\n\nIf that flag is not set, we'd get NUL or nonsense.\n\n> I'm out of time for tonight and not familiar enough with the color-moved\n> code to come up with a reasonable test case quickly, but maybe you can\n> see if that can trigger bad behavior?\n>\n> -Peff\n"},{"id":"330621","messageId":"20171019050459.p2cx63yrxfwq4ta3@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171013002057.froqi54olmhmah6b@sigill.intra.peff.net","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T05:04:59Z","receivedAt":"2017-10-19T05:05:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 12, 2017 at 08:20:57PM -0400, Jeff King wrote:\n\n> On Thu, Oct 12, 2017 at 08:18:37PM -0400, Jeff King wrote:\n> \n> > Fix this by entering the conditional only when we actually\n> > see whitespace. We can apply this also to the\n> > IGNORE_WHITESPACE change. That code path isn't buggy\n> > (because it falls through to returning the next\n> > non-whitespace byte), but it makes the logic more clear if\n> > we only bother to look at whitespace flags after seeing that\n> > the next byte is whitespace.\n> \n> I think there actually _is_ a bug in that code path, but it's unrelated\n> to this one. If you have whitespace at the end of the buffer, then we'd\n> advance *cp until it matches *endp, and then return whatever is at *endp\n> (which is nonsense, or probably a NUL) rather than returning \"-1\".\n> \n> I'm out of time for tonight and not familiar enough with the color-moved\n> code to come up with a reasonable test case quickly, but maybe you can\n> see if that can trigger bad behavior?\n\nI found a few moments to follow up on this. I'm not sure if it's a bug\nor intentional behavior. It's definitely easy to trigger next_byte()\nreading *endp (even without ignoring whitespace, we always dereference\nit).\n\nThis function is always operating on the buffer from an\nemitted_diff_symbol, which is NUL-terminated. So *endp is always NUL. So\nwe'll return \"0\" at the end of the string.\n\nThere are two callers. The first is get_string_hash(), which does:\n\n while ((c = next_byte(&ap, &ae, o)) > 0)\n\nso we'll break out on the NUL byte. But it also sometimes shrinks the\nend-pointer \"ae\" to skip trailing whitespace. Or at least it tries to.\nIt does:\n\n const char *ap = es->line, *ae = es->line + es->len;\n ...\n while (ae > ap && isspace(*ae))\n ae--;\n\nbut AFAICT that loop will never trigger, since *ae will always point to\nNUL to begin with. But it points to the expectation that our end-pointer\ndoes not follow the usual one-past-the-end convention, but rather is\nmeant to point to the actual last character in the string.\n\nThe same problem is repeated in the other caller, moved_entry_cmp(),\nwhich tries to eat trailing whitespace for IGNORE_WHITESPACE_AT_EOL. But\nit uses the same loop that starts on NUL and will never trigger.\n\nIt then goes on to call next_byte(). The extra NUL there should not\ncause any problem, because we are just checking for equality between the\ntwo strings (so if they both return NUL at the same time, good; if not,\nthen we know they are different).\n\nSo. That leaves me with:\n\n - I'm unclear on whether next_byte() is meant to return that trailing\n NUL or not. I don't think it causes any bugs, but it certainly\n confused me for a function to take a cp/endp pair of pointers, and\n then dereference endp. It might be worth either fixing or clarifying\n with a comment.\n\n - Those loops to eat trailing whitespace are doing nothing. I'm not\n sure if that all works out because next_byte() eats whitespaces or\n not (I think not, because it doesn't eat whitespace for the\n IGNORE_WHITESPACE_AT_EOL case). But I'm not quite sure what a test\n would look like.\n\n-Peff\n"},{"id":"330623","messageId":"20171019052457.gqenoshgyjcw53tb@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171019050459.p2cx63yrxfwq4ta3@sigill.intra.peff.net","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T05:24:57Z","receivedAt":"2017-10-19T05:25:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 01:04:59AM -0400, Jeff King wrote:\n\n> So. That leaves me with:\n> \n> - I'm unclear on whether next_byte() is meant to return that trailing\n> NUL or not. I don't think it causes any bugs, but it certainly\n> confused me for a function to take a cp/endp pair of pointers, and\n> then dereference endp. It might be worth either fixing or clarifying\n> with a comment.\n> \n> - Those loops to eat trailing whitespace are doing nothing. I'm not\n> sure if that all works out because next_byte() eats whitespaces or\n> not (I think not, because it doesn't eat whitespace for the\n> IGNORE_WHITESPACE_AT_EOL case). But I'm not quite sure what a test\n> would look like.\n\nI had trouble constructing a test at first, but I think my test lines\njust weren't long enough to trigger the movement heuristics. If I switch\nto something besides seq, I can do:\n\n # any input that has reasonably sized lines\n look e | head -50 >file\n git add file\n \n perl -i -ne '\n # pick up lines 20-25 to move to line 40, and\n # add some trailing whitespace to them\n if ($. >= 20 && $. <= 25) {\n s/$/ /;\n $hold .= $_;\n } else {\n print $hold if ($. == 40);\n print;\n }\n ' file\n\n git diff --color-moved --ignore-space-at-eol\n\nI think that _should_ show the block as moved, but it doesn't. But if I\napply this patch:\n\ndiff --git a/diff.c b/diff.c\nindex 93dccd1817..375d9cf447 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -743,8 +743,8 @@ static int moved_entry_cmp(const struct diff_options *diffopt,\n \t\t\t const struct moved_entry *b,\n \t\t\t const void *keydata)\n {\n-\tconst char *ap = a->es->line, *ae = a->es->line + a->es->len;\n-\tconst char *bp = b->es->line, *be = b->es->line + b->es->len;\n+\tconst char *ap = a->es->line, *ae = a->es->line + a->es->len - 1;\n+\tconst char *bp = b->es->line, *be = b->es->line + b->es->len - 1;\n \n \tif (!(diffopt->xdl_opts & XDF_WHITESPACE_FLAGS))\n \t\treturn a->es->len != b->es->len || memcmp(ap, bp, a->es->len);\n@@ -771,7 +771,7 @@ static unsigned get_string_hash(struct emitted_diff_symbol *es, struct diff_opti\n {\n \tif (o->xdl_opts & XDF_WHITESPACE_FLAGS) {\n \t\tstatic struct strbuf sb = STRBUF_INIT;\n-\t\tconst char *ap = es->line, *ae = es->line + es->len;\n+\t\tconst char *ap = es->line, *ae = es->line + es->len - 1;\n \t\tint c;\n \n \t\tstrbuf_reset(&sb);\n\nit does. It just adjusts our \"end pointer\" to point to the last valid\ncharacter in the string (rather than one past), which seems to be the\nconvention that those loops (and next_byte) expect.\n\n-Peff\n"},{"id":"330624","messageId":"xmqqzi8niu1r.fsf@gitster.mtv.corp.google.com","threadId":"46959","inReplyTo":"20171019052457.gqenoshgyjcw53tb@sigill.intra.peff.net","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-19T05:30:08Z","receivedAt":"2017-10-19T05:30:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> it does. It just adjusts our \"end pointer\" to point to the last valid\n> character in the string (rather than one past), which seems to be the\n> convention that those loops (and next_byte) expect.\n\nYeah I am not sure if I like this comparison at the beginning of the\nfunction:\n\n static int next_byte(const char **cp, const char **endp,\n const struct diff_options *diffopt)\n {\n int retval;\n\n if (*cp > *endp)\n return -1;\n\nbut it says endp _is_ part of valid input, contrary to my intuition.\n\nAnd your change to the initialization of ae/be in moved_entry_cmp()\nmakes it consistent with it, I think.\n\nBut doesn't it mean ae computation in get_string_hash() also needs a\nmassaging?\n"},{"id":"330625","messageId":"xmqqvajbityj.fsf@gitster.mtv.corp.google.com","threadId":"46959","inReplyTo":"xmqqzi8niu1r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-19T05:32:04Z","receivedAt":"2017-10-19T05:32:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> it does. It just adjusts our \"end pointer\" to point to the last valid\n>> character in the string (rather than one past), which seems to be the\n>> convention that those loops (and next_byte) expect.\n>\n> Yeah I am not sure if I like this comparison at the beginning of the\n> function:\n>\n> static int next_byte(const char **cp, const char **endp,\n> const struct diff_options *diffopt)\n> {\n> int retval;\n>\n> if (*cp > *endp)\n> return -1;\n>\n> but it says endp _is_ part of valid input, contrary to my intuition.\n>\n> And your change to the initialization of ae/be in moved_entry_cmp()\n> makes it consistent with it, I think.\n>\n> But doesn't it mean ae computation in get_string_hash() also needs a\n> massaging?\n\nAh, forget the last two lines. You do do the massaging in your\npatch.\n\nMy preference actually is to fix next_byte to follow the usual \"when\nwe end, it points one past the valid region\", though.\n\nThanks for digging it through.\n\n\n\n"},{"id":"330626","messageId":"20171019053245.g4n32ebnyi4xmbwi@sigill.intra.peff.net","threadId":"46959","inReplyTo":"xmqqvajbityj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T05:32:45Z","receivedAt":"2017-10-19T05:32:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 02:32:04PM +0900, Junio C Hamano wrote:\n\n> > Yeah I am not sure if I like this comparison at the beginning of the\n> > function:\n> >\n> > static int next_byte(const char **cp, const char **endp,\n> > const struct diff_options *diffopt)\n> > {\n> > int retval;\n> >\n> > if (*cp > *endp)\n> > return -1;\n> >\n> > but it says endp _is_ part of valid input, contrary to my intuition.\n> >\n> > And your change to the initialization of ae/be in moved_entry_cmp()\n> > makes it consistent with it, I think.\n> >\n> > But doesn't it mean ae computation in get_string_hash() also needs a\n> > massaging?\n> \n> Ah, forget the last two lines. You do do the massaging in your\n> patch.\n\nI was just replying so. :)\n\n> My preference actually is to fix next_byte to follow the usual \"when\n> we end, it points one past the valid region\", though.\n\nYeah, I think that is my preference, too.\n\n-Peff\n"},{"id":"330628","messageId":"20171019054246.jii62lq2druohbpo@sigill.intra.peff.net","threadId":"46959","inReplyTo":"xmqqzi8niu1r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T05:42:46Z","receivedAt":"2017-10-19T05:42:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 02:30:08PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > it does. It just adjusts our \"end pointer\" to point to the last valid\n> > character in the string (rather than one past), which seems to be the\n> > convention that those loops (and next_byte) expect.\n> \n> Yeah I am not sure if I like this comparison at the beginning of the\n> function:\n> \n> static int next_byte(const char **cp, const char **endp,\n> const struct diff_options *diffopt)\n> {\n> int retval;\n> \n> if (*cp > *endp)\n> return -1;\n> \n> but it says endp _is_ part of valid input, contrary to my intuition.\n\nActually, I think even this function is confused about its convention.\nIn the line you quote, we clearly treat *endp as part of the input. But\nlater we do:\n\n while (*cp < *endp && isspace(**cp))\n\t (*cp)++;\n\nmeaning that we'd fail to soak up whitespace at *endp. That wouldn't be\nso bad if not for the other bug which fails to eat whitespace at endp in\nthe first place. :)\n\nSo I think the right fix is this:\n\ndiff --git a/diff.c b/diff.c\nindex 6fd288420b..09081a207c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -712,7 +712,7 @@ static int next_byte(const char **cp, const char **endp,\n {\n \tint retval;\n \n-\tif (*cp > *endp)\n+\tif (*cp >= *endp)\n \t\treturn -1;\n \n \tif (isspace(**cp)) {\n@@ -729,7 +729,12 @@ static int next_byte(const char **cp, const char **endp,\n \t\tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {\n \t\t\twhile (*cp < *endp && isspace(**cp))\n \t\t\t\t(*cp)++;\n-\t\t\t/* return the first non-ws character via the usual below */\n+\t\t\t/*\n+\t\t\t * return the first non-ws character via the usual\n+\t\t\t * below, unless we ate all of the bytes\n+\t\t\t */\n+\t\t\tif (*cp >= *endp)\n+\t\t\t\treturn -1;\n \t\t}\n \t}\n \n@@ -750,9 +755,9 @@ static int moved_entry_cmp(const struct diff_options *diffopt,\n \t\treturn a->es->len != b->es->len || memcmp(ap, bp, a->es->len);\n \n \tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_AT_EOL)) {\n-\t\twhile (ae > ap && isspace(*ae))\n+\t\twhile (ae > ap && isspace(ae[-1]))\n \t\t\tae--;\n-\t\twhile (be > bp && isspace(*be))\n+\t\twhile (be > bp && isspace(be[-1]))\n \t\t\tbe--;\n \t}\n \n@@ -775,7 +780,7 @@ static unsigned get_string_hash(struct emitted_diff_symbol *es, struct diff_opti\n \t\tint c;\n \n \t\tstrbuf_reset(&sb);\n-\t\twhile (ae > ap && isspace(*ae))\n+\t\twhile (ae > ap && isspace(ae[-1]))\n \t\t\tae--;\n \t\twhile ((c = next_byte(&ap, &ae, o)) > 0)\n \t\t\tstrbuf_addch(&sb, c);\n\nIt's late here, so I'll wait for comments from Stefan and then try to\nwrap it up with a commit message and test tomorrow.\n\n-Peff\n"},{"id":"330668","messageId":"CAGZ79kbRFVd=hbc4DCrdkOJ4aUE_g1_NhMbG-tfPGnOJikf1GA@mail.gmail.com","threadId":"46959","inReplyTo":"20171019052457.gqenoshgyjcw53tb@sigill.intra.peff.net","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T19:53:03Z","receivedAt":"2017-10-19T19:53:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 18, 2017 at 10:24 PM, Jeff King wrote:\n> On Thu, Oct 19, 2017 at 01:04:59AM -0400, Jeff King wrote:\n>\n>> So. That leaves me with:\n>>\n>> - I'm unclear on whether next_byte() is meant to return that trailing\n>> NUL or not. I don't think it causes any bugs, but it certainly\n>> confused me for a function to take a cp/endp pair of pointers, and\n>> then dereference endp. It might be worth either fixing or clarifying\n>> with a comment.\n>>\n>> - Those loops to eat trailing whitespace are doing nothing. I'm not\n>> sure if that all works out because next_byte() eats whitespaces or\n>> not (I think not, because it doesn't eat whitespace for the\n>> IGNORE_WHITESPACE_AT_EOL case). But I'm not quite sure what a test\n>> would look like.\n>\n> I had trouble constructing a test at first, but I think my test lines\n> just weren't long enough to trigger the movement heuristics. If I switch\n> to something besides seq, I can do:\n>\n> # any input that has reasonably sized lines\n> look e | head -50 >file\n> git add file\n>\n> perl -i -ne '\n> # pick up lines 20-25 to move to line 40, and\n> # add some trailing whitespace to them\n> if ($. >= 20 && $. <= 25) {\n> s/$/ /;\n> $hold .= $_;\n> } else {\n> print $hold if ($. == 40);\n> print;\n> }\n> ' file\n>\n> git diff --color-moved --ignore-space-at-eol\n>\n> I think that _should_ show the block as moved, but it doesn't. But if I\n> apply this patch:\n>\n> diff --git a/diff.c b/diff.c\n> index 93dccd1817..375d9cf447 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -743,8 +743,8 @@ static int moved_entry_cmp(const struct diff_options *diffopt,\n> const struct moved_entry *b,\n> const void *keydata)\n> {\n> - const char *ap = a->es->line, *ae = a->es->line + a->es->len;\n> - const char *bp = b->es->line, *be = b->es->line + b->es->len;\n> + const char *ap = a->es->line, *ae = a->es->line + a->es->len - 1;\n> + const char *bp = b->es->line, *be = b->es->line + b->es->len - 1;\n>\n> if (!(diffopt->xdl_opts & XDF_WHITESPACE_FLAGS))\n> return a->es->len != b->es->len || memcmp(ap, bp, a->es->len);\n> @@ -771,7 +771,7 @@ static unsigned get_string_hash(struct emitted_diff_symbol *es, struct diff_opti\n> {\n> if (o->xdl_opts & XDF_WHITESPACE_FLAGS) {\n> static struct strbuf sb = STRBUF_INIT;\n> - const char *ap = es->line, *ae = es->line + es->len;\n> + const char *ap = es->line, *ae = es->line + es->len - 1;\n> int c;\n>\n> strbuf_reset(&sb);\n>\n> it does. It just adjusts our \"end pointer\" to point to the last valid\n> character in the string (rather than one past),\n\nThanks for spotting. I can send a proper patch with tests if you'd like.\n\n\n> which seems to be the\n> convention that those loops (and next_byte) expect.\n\nI'll look at that again.\n\nThanks for poking!\nStefan\n\n>\n> -Peff\n"},{"id":"330670","messageId":"CAGZ79kb7pz6sWG_hMJLt3TmPNmDqWB1Jy1xNO73-P6iMPM-BYg@mail.gmail.com","threadId":"46959","inReplyTo":"20171019054246.jii62lq2druohbpo@sigill.intra.peff.net","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T19:55:49Z","receivedAt":"2017-10-19T19:55:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 18, 2017 at 10:42 PM, Jeff King wrote:\n\n>\n> So I think the right fix is this:\n>\n[...]\n>\n> It's late here, so I'll wait for comments from Stefan and then try to\n> wrap it up with a commit message and test tomorrow.\n>\n> -Peff\n\nI agree that this is better and looks correct.\nThanks for offering to send a patch. I'd be happy to review it.\n\nThanks,\nStefan\n"},{"id":"330671","messageId":"20171019195552.sg7su63a4qqwa4v5@sigill.intra.peff.net","threadId":"46959","inReplyTo":"CAGZ79kbRFVd=hbc4DCrdkOJ4aUE_g1_NhMbG-tfPGnOJikf1GA@mail.gmail.com","subject":"Re: [PATCH] diff.c: increment buffer pointer in all code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T19:55:52Z","receivedAt":"2017-10-19T19:56:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 12:53:03PM -0700, Stefan Beller wrote:\n\n> > @@ -771,7 +771,7 @@ static unsigned get_string_hash(struct emitted_diff_symbol *es, struct diff_opti\n> > {\n> > if (o->xdl_opts & XDF_WHITESPACE_FLAGS) {\n> > static struct strbuf sb = STRBUF_INIT;\n> > - const char *ap = es->line, *ae = es->line + es->len;\n> > + const char *ap = es->line, *ae = es->line + es->len - 1;\n> > int c;\n> >\n> > strbuf_reset(&sb);\n> >\n> > it does. It just adjusts our \"end pointer\" to point to the last valid\n> > character in the string (rather than one past),\n> \n> Thanks for spotting. I can send a proper patch with tests if you'd like.\n\nI'm putting the finishing touches on a series that should be out in a\nfew minutes. Hold off until then.\n\n-Peff\n"},{"id":"330672","messageId":"20171019202326.grovyfsragl2d7xx@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171019054246.jii62lq2druohbpo@sigill.intra.peff.net","subject":"[PATCH 0/5] fix \"diff --color-moved --ignore-space-at-eol\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T20:23:26Z","receivedAt":"2017-10-19T20:23:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 01:42:46AM -0400, Jeff King wrote:\n\n> It's late here, so I'll wait for comments from Stefan and then try to\n> wrap it up with a commit message and test tomorrow.\n\nHere it is.\n\nThe fix is in patch 4. The earlier ones are just beefing up the test\ncoverage, and the last one is a minor cleanup we can do on top.\n\n [1/5]: t4015: refactor --color-moved whitespace test\n [2/5]: t4015: check \"negative\" case for \"-w --color-moved\"\n [3/5]: t4015: test the output of \"diff --color-moved -b\"\n [4/5]: diff: fix whitespace-skipping with --color-moved\n [5/5]: diff: handle NULs in get_string_hash()\n\n diff.c | 17 ++--\n t/t4015-diff-whitespace.sh | 213 +++++++++++++++++++++++++++++++++++++--------\n 2 files changed, 188 insertions(+), 42 deletions(-)\n\n-Peff\n"},{"id":"330673","messageId":"20171019202403.7srcpos5xlsvmqrl@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171019202326.grovyfsragl2d7xx@sigill.intra.peff.net","subject":"[PATCH 1/5] t4015: refactor --color-moved whitespace test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T20:24:03Z","receivedAt":"2017-10-19T20:24:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In preparation for testing several different whitespace\noptions, let's split out the setup and cleanup steps of the\nwhitespace test.\n\nWhile we're here, let's also switch to using \"<<-\" to indent\nour here-documents properly, and use q_to_tab to more\nexplicitly mark where we expect whitespace to appear.\n\nSigned-off-by: Jeff King \n---\n t/t4015-diff-whitespace.sh | 49 +++++++++++++++++++++++++++-------------------\n 1 file changed, 29 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 87083f728f..164b502405 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1318,30 +1318,34 @@ test_expect_success 'no effect from --color-moved with --word-diff' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'move detection ignoring whitespace ' '\n+test_expect_success 'set up whitespace tests' '\n \tgit reset --hard &&\n-\tcat <<\\EOF >lines.txt &&\n-line 1\n-line 2\n-line 3\n-line 4\n-long line 5\n-long line 6\n-long line 7\n-EOF\n-\tgit add lines.txt &&\n-\tgit commit -m \"add poetry\" &&\n-\tcat <<\\EOF >lines.txt &&\n+\t# Note that these lines have no leading or trailing whitespace.\n+\tcat <<-\\EOF >lines.txt &&\n+\tline 1\n+\tline 2\n+\tline 3\n+\tline 4\n \tlong line 5\n \tlong line 6\n \tlong line 7\n-line 1\n-line 2\n-line 3\n-line 4\n-EOF\n-\ttest_config color.diff.oldMoved \"magenta\" &&\n-\ttest_config color.diff.newMoved \"cyan\" &&\n+\tEOF\n+\tgit add lines.txt &&\n+\tgit commit -m \"add poetry\" &&\n+\tgit config color.diff.oldMoved \"magenta\" &&\n+\tgit config color.diff.newMoved \"cyan\"\n+'\n+\n+test_expect_success 'move detection ignoring whitespace ' '\n+\tq_to_tab <<-\\EOF >lines.txt &&\n+\tQlong line 5\n+\tQlong line 6\n+\tQlong line 7\n+\tline 1\n+\tline 2\n+\tline 3\n+\tline 4\n+\tEOF\n \tgit diff HEAD --no-renames --color-moved --color |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n@@ -1385,6 +1389,11 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'clean up whitespace-test colors' '\n+\tgit config --unset color.diff.oldMoved &&\n+\tgit config --unset color.diff.newMoved\n+'\n+\n test_expect_success '--color-moved block at end of diff output respects MIN_ALNUM_COUNT' '\n \tgit reset --hard &&\n \t>bar &&\n-- \n2.15.0.rc1.560.g5f0609e481\n\n"},{"id":"330674","messageId":"20171019202556.hkz4r7hq4tlkjhvh@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171019202326.grovyfsragl2d7xx@sigill.intra.peff.net","subject":"[PATCH 2/5] t4015: check \"negative\" case for \"-w --color-moved\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T20:25:57Z","receivedAt":"2017-10-19T20:26:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We test that lines with whitespace changes are not found by\n\"--color-moved\" by default, but are found if \"-w\" is added.\nLet's add one more twist: a line that has non-whitespace\nchanges should not be marked as a pure move.\n\nThis is perhaps an obvious case for us to get right (and we\ndo), but as we add more whitespace tests, they will form a\npattern of \"make sure this case is a move and this other\ncase is not\".\n\nNote that we have to add a line to our moved block, since\nhaving a too-small block doesn't trigger the \"moved\"\nheuristics. And we also add a line of context to ensure\nthat there's more context lines than moved lines (so the\ndiff shows us moving the lines up, rather than moving the\ncontext down).\n\nSigned-off-by: Jeff King \n---\n t/t4015-diff-whitespace.sh | 26 ++++++++++++++++++--------\n 1 file changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 164b502405..503c9bc7f3 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1326,9 +1326,11 @@ test_expect_success 'set up whitespace tests' '\n \tline 2\n \tline 3\n \tline 4\n-\tlong line 5\n+\tline 5\n \tlong line 6\n \tlong line 7\n+\tlong line 8\n+\tlong line 9\n \tEOF\n \tgit add lines.txt &&\n \tgit commit -m \"add poetry\" &&\n@@ -1338,13 +1340,15 @@ test_expect_success 'set up whitespace tests' '\n \n test_expect_success 'move detection ignoring whitespace ' '\n \tq_to_tab <<-\\EOF >lines.txt &&\n-\tQlong line 5\n \tQlong line 6\n \tQlong line 7\n+\tQlong line 8\n+\tQchanged long line 9\n \tline 1\n \tline 2\n \tline 3\n \tline 4\n+\tline 5\n \tEOF\n \tgit diff HEAD --no-renames --color-moved --color |\n \t\tgrep -v \"index\" |\n@@ -1353,17 +1357,20 @@ test_expect_success 'move detection ignoring whitespace ' '\n \tdiff --git a/lines.txt b/lines.txt\n \t--- a/lines.txt\n \t+++ b/lines.txt\n-\t@@ -1,7 +1,7 @@\n-\t+\tlong line 5\n+\t@@ -1,9 +1,9 @@\n \t+\tlong line 6\n \t+\tlong line 7\n+\t+\tlong line 8\n+\t+\tchanged long line 9\n \t line 1\n \t line 2\n \t line 3\n \t line 4\n-\t-long line 5\n+\t line 5\n \t-long line 6\n \t-long line 7\n+\t-long line 8\n+\t-long line 9\n \tEOF\n \ttest_cmp expected actual &&\n \n@@ -1374,17 +1381,20 @@ test_expect_success 'move detection ignoring whitespace ' '\n \tdiff --git a/lines.txt b/lines.txt\n \t--- a/lines.txt\n \t+++ b/lines.txt\n-\t@@ -1,7 +1,7 @@\n-\t+\tlong line 5\n+\t@@ -1,9 +1,9 @@\n \t+\tlong line 6\n \t+\tlong line 7\n+\t+\tlong line 8\n+\t+\tchanged long line 9\n \t line 1\n \t line 2\n \t line 3\n \t line 4\n-\t-long line 5\n+\t line 5\n \t-long line 6\n \t-long line 7\n+\t-long line 8\n+\t-long line 9\n \tEOF\n \ttest_cmp expected actual\n '\n-- \n2.15.0.rc1.560.g5f0609e481\n\n"},{"id":"330675","messageId":"20171019202630.plnm6g4x3cuggxoj@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171019202326.grovyfsragl2d7xx@sigill.intra.peff.net","subject":"[PATCH 3/5] t4015: test the output of \"diff --color-moved -b\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T20:26:31Z","receivedAt":"2017-10-19T20:26:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit fa5ba2c1dd (diff: fix infinite loop with\n--color-moved --ignore-space-change, 2017-10-12) added a\ntest to make sure that \"--color-moved -b\" doesn't run\nforever, but the test in question doesn't actually have any\nmoved lines in it.\n\nLet's scrap that test and add a variant of the existing\n\"--color-moved -w\" test, but this time we'll check that we\nfind the move with whitespace changes, but not arbitrary\nwhitespace additions.\n\nSigned-off-by: Jeff King \n---\n t/t4015-diff-whitespace.sh | 73 ++++++++++++++++++++++++++++++++++++++++------\n 1 file changed, 64 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 503c9bc7f3..1f54816c6b 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1399,6 +1399,70 @@ test_expect_success 'move detection ignoring whitespace ' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'move detection ignoring whitespace changes' '\n+\tgit reset --hard &&\n+\t# Lines 6-8 have a space change, but 9 is new whitespace\n+\tq_to_tab <<-\\EOF >lines.txt &&\n+\tlongQline 6\n+\tlongQline 7\n+\tlongQline 8\n+\tlong liQne 9\n+\tline 1\n+\tline 2\n+\tline 3\n+\tline 4\n+\tline 5\n+\tEOF\n+\n+\tgit diff HEAD --no-renames --color-moved --color |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/lines.txt b/lines.txt\n+\t--- a/lines.txt\n+\t+++ b/lines.txt\n+\t@@ -1,9 +1,9 @@\n+\t+long\tline 6\n+\t+long\tline 7\n+\t+long\tline 8\n+\t+long li\tne 9\n+\t line 1\n+\t line 2\n+\t line 3\n+\t line 4\n+\t line 5\n+\t-long line 6\n+\t-long line 7\n+\t-long line 8\n+\t-long line 9\n+\tEOF\n+\ttest_cmp expected actual &&\n+\n+\tgit diff HEAD --no-renames -b --color-moved --color |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/lines.txt b/lines.txt\n+\t--- a/lines.txt\n+\t+++ b/lines.txt\n+\t@@ -1,9 +1,9 @@\n+\t+long\tline 6\n+\t+long\tline 7\n+\t+long\tline 8\n+\t+long li\tne 9\n+\t line 1\n+\t line 2\n+\t line 3\n+\t line 4\n+\t line 5\n+\t-long line 6\n+\t-long line 7\n+\t-long line 8\n+\t-long line 9\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'clean up whitespace-test colors' '\n \tgit config --unset color.diff.oldMoved &&\n \tgit config --unset color.diff.newMoved\n@@ -1549,13 +1613,4 @@ test_expect_success 'move detection with submodules' '\n \ttest_cmp expect decoded_actual\n '\n \n-test_expect_success 'move detection with whitespace changes' '\n-\ttest_when_finished \"git reset --hard\" &&\n-\ttest_seq 10 >test &&\n-\tgit add test &&\n-\tsed s/3/42/ test.tmp &&\n-\tmv test.tmp test &&\n-\tgit -c diff.colormoved diff --ignore-space-change -- test\n-'\n-\n test_done\n-- \n2.15.0.rc1.560.g5f0609e481\n\n"},{"id":"330676","messageId":"20171019202926.irldca42wqosmxrd@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171019202326.grovyfsragl2d7xx@sigill.intra.peff.net","subject":"[PATCH 4/5] diff: fix whitespace-skipping with --color-moved","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T20:29:26Z","receivedAt":"2017-10-19T20:29:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The code for handling whitespace with --color-moved\nrepresents partial strings as a pair of pointers. There are\ntwo possible conventions for the end pointer:\n\n 1. It points to the byte right after the end of the\n string.\n\n 2. It points to the final byte of the string.\n\nBut we seem to use both conventions in the code:\n\n a. we assign the initial pointers from the NUL-terminated\n string using (1)\n\n b. we eat trailing whitespace by checking the second\n pointer for isspace(), which needs (2)\n\n c. the next_byte() function checks for end-of-string with\n \"if (cp > endp)\", which is (2)\n\n d. in next_byte() we skip past internal whitespace with\n \"while (cp < end)\", which is (1)\n\nThis creates fewer bugs than you might think, because there\nare some subtle interactions. Because of (a) and (c), we\nalways return the NUL-terminator from next_byte(). But all\nof the callers of next_byte() happen to handle that\ngracefully.\n\nBecause of the mismatch between (d) and (c), next_byte()\ncould accidentally return a whitespace character right at\nendp. But because of the interaction of (a) and (b), we fail\nto actually chomp trailing whitespace, meaning our endp\n_always_ points to a NUL, canceling out the problem.\n\nBut that does leave (b) as a real bug: when ignoring\nwhitespace only at the end-of-line, we don't correctly trim\nit, and fail to match up lines.\n\nWe can fix the whole thing by moving consistently to one\nconvention. Since convention (1) is idiomatic in our code\nbase, we'll pick that one.\n\nThe existing \"-w\" and \"-b\" tests continue to pass, and a new\n\"--ignore-space-at-eol\" shows off the breakage we're fixing.\n\nSigned-off-by: Jeff King \n---\n diff.c | 15 +++++++----\n t/t4015-diff-whitespace.sh | 67 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 77 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6fd288420b..09081a207c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -712,7 +712,7 @@ static int next_byte(const char **cp, const char **endp,\n {\n \tint retval;\n \n-\tif (*cp > *endp)\n+\tif (*cp >= *endp)\n \t\treturn -1;\n \n \tif (isspace(**cp)) {\n@@ -729,7 +729,12 @@ static int next_byte(const char **cp, const char **endp,\n \t\tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE)) {\n \t\t\twhile (*cp < *endp && isspace(**cp))\n \t\t\t\t(*cp)++;\n-\t\t\t/* return the first non-ws character via the usual below */\n+\t\t\t/*\n+\t\t\t * return the first non-ws character via the usual\n+\t\t\t * below, unless we ate all of the bytes\n+\t\t\t */\n+\t\t\tif (*cp >= *endp)\n+\t\t\t\treturn -1;\n \t\t}\n \t}\n \n@@ -750,9 +755,9 @@ static int moved_entry_cmp(const struct diff_options *diffopt,\n \t\treturn a->es->len != b->es->len || memcmp(ap, bp, a->es->len);\n \n \tif (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_AT_EOL)) {\n-\t\twhile (ae > ap && isspace(*ae))\n+\t\twhile (ae > ap && isspace(ae[-1]))\n \t\t\tae--;\n-\t\twhile (be > bp && isspace(*be))\n+\t\twhile (be > bp && isspace(be[-1]))\n \t\t\tbe--;\n \t}\n \n@@ -775,7 +780,7 @@ static unsigned get_string_hash(struct emitted_diff_symbol *es, struct diff_opti\n \t\tint c;\n \n \t\tstrbuf_reset(&sb);\n-\t\twhile (ae > ap && isspace(*ae))\n+\t\twhile (ae > ap && isspace(ae[-1]))\n \t\t\tae--;\n \t\twhile ((c = next_byte(&ap, &ae, o)) > 0)\n \t\t\tstrbuf_addch(&sb, c);\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 1f54816c6b..eb9431da7d 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1463,6 +1463,73 @@ test_expect_success 'move detection ignoring whitespace changes' '\n \ttest_cmp expected actual\n '\n \n+test_expect_failure 'move detection ignoring whitespace at eol' '\n+\tgit reset --hard &&\n+\t# Lines 6-9 have new eol whitespace, but 9 also has it in the middle\n+\tq_to_tab <<-\\EOF >lines.txt &&\n+\tlong line 6Q\n+\tlong line 7Q\n+\tlong line 8Q\n+\tlongQline 9Q\n+\tline 1\n+\tline 2\n+\tline 3\n+\tline 4\n+\tline 5\n+\tEOF\n+\n+\t# avoid cluttering the output with complaints about our eol whitespace\n+\ttest_config core.whitespace -blank-at-eol &&\n+\n+\tgit diff HEAD --no-renames --color-moved --color |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/lines.txt b/lines.txt\n+\t--- a/lines.txt\n+\t+++ b/lines.txt\n+\t@@ -1,9 +1,9 @@\n+\t+long line 6\t\n+\t+long line 7\t\n+\t+long line 8\t\n+\t+long\tline 9\t\n+\t line 1\n+\t line 2\n+\t line 3\n+\t line 4\n+\t line 5\n+\t-long line 6\n+\t-long line 7\n+\t-long line 8\n+\t-long line 9\n+\tEOF\n+\ttest_cmp expected actual &&\n+\n+\tgit diff HEAD --no-renames --ignore-space-at-eol --color-moved --color |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/lines.txt b/lines.txt\n+\t--- a/lines.txt\n+\t+++ b/lines.txt\n+\t@@ -1,9 +1,9 @@\n+\t+long line 6\t\n+\t+long line 7\t\n+\t+long line 8\t\n+\t+long\tline 9\t\n+\t line 1\n+\t line 2\n+\t line 3\n+\t line 4\n+\t line 5\n+\t-long line 6\n+\t-long line 7\n+\t-long line 8\n+\t-long line 9\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'clean up whitespace-test colors' '\n \tgit config --unset color.diff.oldMoved &&\n \tgit config --unset color.diff.newMoved\n-- \n2.15.0.rc1.560.g5f0609e481\n\n"},{"id":"330677","messageId":"20171019203119.d2xbryov2y5t2aig@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171019202326.grovyfsragl2d7xx@sigill.intra.peff.net","subject":"[PATCH 5/5] diff: handle NULs in get_string_hash()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T20:31:20Z","receivedAt":"2017-10-19T20:31:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For computing moved lines, we feed the characters of each\nline into a hash. When we've been asked to ignore\nwhitespace, then we pick each character using next_byte(),\nwhich returns -1 on end-of-string, which it determines using\nthe start/end pointers we feed it.\n\nHowever our check of its return value treats \"0\" the same as\n\"-1\", meaning we'd quit if the string has an embedded NUL.\nThis is unlikely to ever come up in practice since our line\nboundaries generally come from calling strlen() in the first\nplace.\n\nBut it was a bit surprising to me as a reader of the\nnext_byte() code. And it's possible that we may one day feed\nthis function with more exotic input, which otherwise works\nwith arbitrary ptr/len pairs.\n\nSigned-off-by: Jeff King \n---\nI noticed that we make an extra copy of each line here, just to feed it\nto memihash! I guess \"-w\" is not a critical-performance code path, but\nthis could be fixed if we could do memhash() incrementally (e.g., by\nputting the FNV state into a struct and letting callers \"add\" to it\nincrementally). Maybe an interesting #leftoverbits, though I'd want to\nsee timing tests that show it's worth doing.\n\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 09081a207c..c4a669ffa8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -782,7 +782,7 @@ static unsigned get_string_hash(struct emitted_diff_symbol *es, struct diff_opti\n \t\tstrbuf_reset(&sb);\n \t\twhile (ae > ap && isspace(ae[-1]))\n \t\t\tae--;\n-\t\twhile ((c = next_byte(&ap, &ae, o)) > 0)\n+\t\twhile ((c = next_byte(&ap, &ae, o)) >= 0)\n \t\t\tstrbuf_addch(&sb, c);\n \n \t\treturn memhash(sb.buf, sb.len);\n-- \n2.15.0.rc1.560.g5f0609e481\n"},{"id":"330679","messageId":"CAGZ79kZyx5tezrTN2Y26G=0E4VaXyQSODFK6=19ziSHprfDLsQ@mail.gmail.com","threadId":"46959","inReplyTo":"20171019202556.hkz4r7hq4tlkjhvh@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] t4015: check \"negative\" case for \"-w --color-moved\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T20:54:15Z","receivedAt":"2017-10-19T20:54:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 1:25 PM, Jeff King wrote:\n> We test that lines with whitespace changes are not found by\n> \"--color-moved\" by default, but are found if \"-w\" is added.\n> Let's add one more twist: a line that has non-whitespace\n> changes should not be marked as a pure move.\n>\n> This is perhaps an obvious case for us to get right (and we\n> do), but as we add more whitespace tests, they will form a\n> pattern of \"make sure this case is a move and this other\n> case is not\".\n>\n> Note that we have to add a line to our moved block, since\n> having a too-small block doesn't trigger the \"moved\"\n> heuristics. And we also add a line of context to ensure\n> that there's more context lines than moved lines (so the\n> diff shows us moving the lines up, rather than moving the\n> context down).\n>\n> Signed-off-by: Jeff King \n\nThis patch is\nReviewed-by: Stefan Beller \n\nThanks!\n"},{"id":"330680","messageId":"CAGZ79kahYb5=BEvr0io-bSJ+JaOfViicyghH4CtRErqmtAXU-g@mail.gmail.com","threadId":"46959","inReplyTo":"20171019202403.7srcpos5xlsvmqrl@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] t4015: refactor --color-moved whitespace test","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T20:56:55Z","receivedAt":"2017-10-19T20:57:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 1:24 PM, Jeff King wrote:\n\n>\n> +test_expect_success 'clean up whitespace-test colors' '\n> + git config --unset color.diff.oldMoved &&\n> + git config --unset color.diff.newMoved\n> +'\n\nThis could be part of the previous test as\n\n test_config color.diff.oldMoved \"magenta\" &&\n test_config color.diff.newMoved \"cyan\" &&\n\nin the beginning. (That way we also do not pollute the setup,\nbut keeping it test local).\n\nWith or without this nit, this is\nReviewed-by: Stefan Beller \n\nThanks,\nStefan\n"},{"id":"330683","messageId":"CAGZ79kaszx26d8D0UrEj948Fam5b41hQWXcpC0V-5Rrom6M15w@mail.gmail.com","threadId":"46959","inReplyTo":"20171019202630.plnm6g4x3cuggxoj@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] t4015: test the output of \"diff --color-moved -b\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T21:03:54Z","receivedAt":"2017-10-19T21:04:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 1:26 PM, Jeff King wrote:\n> Commit fa5ba2c1dd (diff: fix infinite loop with\n> --color-moved --ignore-space-change, 2017-10-12) added a\n> test to make sure that \"--color-moved -b\" doesn't run\n> forever, but the test in question doesn't actually have any\n> moved lines in it.\n>\n> Let's scrap that test and add a variant of the existing\n> \"--color-moved -w\" test, but this time we'll check that we\n> find the move with whitespace changes, but not arbitrary\n> whitespace additions.\n>\n> Signed-off-by: Jeff King \n> ---\n\n> +\n> + git diff HEAD --no-renames --color-moved --color |\n> + grep -v \"index\" |\n> + test_decode_color >actual &&\n\nThe -v index grep makes it future proof (for a new hash)!\nI like that. What I do not like is the pipe at the end of\ngit-diff as we certainly want to inspect the return code\n(or for a segfault ;)\n\n> + git diff HEAD --no-renames -b --color-moved --color |\n> + grep -v \"index\" |\n> + test_decode_color >actual &&\n\nDo we need (or rather want) to give --color additionally,\ninstead of having --color-moved imply it? I guess this is\nextra carefulness from the \"color always\" series?\n\n\n> test_expect_success 'clean up whitespace-test colors' '\n> git config --unset color.diff.oldMoved &&\n> git config --unset color.diff.newMoved\n> @@ -1549,13 +1613,4 @@ test_expect_success 'move detection with submodules' '\n> test_cmp expect decoded_actual\n> '\n>\n> -test_expect_success 'move detection with whitespace changes' '\n> - test_when_finished \"git reset --hard\" &&\n> - test_seq 10 >test &&\n> - git add test &&\n> - sed s/3/42/ test.tmp &&\n> - mv test.tmp test &&\n> - git -c diff.colormoved diff --ignore-space-change -- test\n> -'\n\nOk, this is covered above in testing for the unchanged lines (1-5)\n"},{"id":"330686","messageId":"20171019211041.yfzlwcxhqb7uwqgh@sigill.intra.peff.net","threadId":"46959","inReplyTo":"CAGZ79kahYb5=BEvr0io-bSJ+JaOfViicyghH4CtRErqmtAXU-g@mail.gmail.com","subject":"Re: [PATCH 1/5] t4015: refactor --color-moved whitespace test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T21:10:41Z","receivedAt":"2017-10-19T21:10:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 01:56:55PM -0700, Stefan Beller wrote:\n\n> On Thu, Oct 19, 2017 at 1:24 PM, Jeff King wrote:\n> \n> >\n> > +test_expect_success 'clean up whitespace-test colors' '\n> > + git config --unset color.diff.oldMoved &&\n> > + git config --unset color.diff.newMoved\n> > +'\n> \n> This could be part of the previous test as\n> \n> test_config color.diff.oldMoved \"magenta\" &&\n> test_config color.diff.newMoved \"cyan\" &&\n> \n> in the beginning. (That way we also do not pollute the setup,\n> but keeping it test local).\n\nIt used to be in the previous test as test_config, but part of the setup\nrefactoring is to keep these common bits across several tests. Hence we\n\"git config\" in the setup step, and we must \"git config --unset\" here in\nthe cleanup. There's only the one test between setup/cleanup right now,\nbut the point is to add more.\n\nThe other alternative (besides repeating ourselves) would be to put all\nthose common bits into a function and call the function at the top of\neach test.\n\n-Peff\n"},{"id":"330687","messageId":"20171019211435.jatotd34knltutco@sigill.intra.peff.net","threadId":"46959","inReplyTo":"CAGZ79kaszx26d8D0UrEj948Fam5b41hQWXcpC0V-5Rrom6M15w@mail.gmail.com","subject":"Re: [PATCH 3/5] t4015: test the output of \"diff --color-moved -b\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T21:14:36Z","receivedAt":"2017-10-19T21:14:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 02:03:54PM -0700, Stefan Beller wrote:\n\n> > +\n> > + git diff HEAD --no-renames --color-moved --color |\n> > + grep -v \"index\" |\n> > + test_decode_color >actual &&\n> \n> The -v index grep makes it future proof (for a new hash)!\n> I like that. What I do not like is the pipe at the end of\n> git-diff as we certainly want to inspect the return code\n> (or for a segfault ;)\n\nI can claim neither credit nor failure here. These lines are copied\nstraight from your existing test. :)\n\n> > + git diff HEAD --no-renames -b --color-moved --color |\n> > + grep -v \"index\" |\n> > + test_decode_color >actual &&\n> \n> Do we need (or rather want) to give --color additionally,\n> instead of having --color-moved imply it? I guess this is\n> extra carefulness from the \"color always\" series?\n\nI do think --color-moved probably ought to imply --color (or at least\n--color=auto). But AFAIK it doesn't do so now, so we need --color (and\nif it implied \"auto\", we'd have to overcome that anyway).\n\nBut again, this comes straight from the existing test.\n\n> > -test_expect_success 'move detection with whitespace changes' '\n> > - test_when_finished \"git reset --hard\" &&\n> > - test_seq 10 >test &&\n> > - git add test &&\n> > - sed s/3/42/ test.tmp &&\n> > - mv test.tmp test &&\n> > - git -c diff.colormoved diff --ignore-space-change -- test\n> > -'\n> \n> Ok, this is covered above in testing for the unchanged lines (1-5)\n\nYes, though I think it's actually the _changed_ lines which do it (the\nproblem was in next_byte(), so any diff with \"--color-moved -b\", whether\nit had moves or not, would infinite loop as long as it had lines without\nwhitespace at the end).\n\n-Peff\n"},{"id":"330688","messageId":"CAGZ79kZhCoFe1P_gwigzrgzzcqRunrUWGOAgy9xvS2ugNeVLmA@mail.gmail.com","threadId":"46959","inReplyTo":"20171019202926.irldca42wqosmxrd@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] diff: fix whitespace-skipping with --color-moved","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T21:15:12Z","receivedAt":"2017-10-19T21:15:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 1:29 PM, Jeff King wrote:\n> The code for handling whitespace with --color-moved\n> represents partial strings as a pair of pointers. There are\n> two possible conventions for the end pointer:\n>\n> 1. It points to the byte right after the end of the\n> string.\n>\n> 2. It points to the final byte of the string.\n>\n> But we seem to use both conventions in the code:\n>\n> a. we assign the initial pointers from the NUL-terminated\n> string using (1)\n>\n> b. we eat trailing whitespace by checking the second\n> pointer for isspace(), which needs (2)\n>\n> c. the next_byte() function checks for end-of-string with\n> \"if (cp > endp)\", which is (2)\n>\n> d. in next_byte() we skip past internal whitespace with\n> \"while (cp < end)\", which is (1)\n>\n> This creates fewer bugs than you might think, because there\n> are some subtle interactions. Because of (a) and (c), we\n> always return the NUL-terminator from next_byte(). But all\n> of the callers of next_byte() happen to handle that\n> gracefully.\n>\n> Because of the mismatch between (d) and (c), next_byte()\n> could accidentally return a whitespace character right at\n> endp. But because of the interaction of (a) and (b), we fail\n> to actually chomp trailing whitespace, meaning our endp\n> _always_ points to a NUL, canceling out the problem.\n>\n> But that does leave (b) as a real bug: when ignoring\n> whitespace only at the end-of-line, we don't correctly trim\n> it, and fail to match up lines.\n>\n> We can fix the whole thing by moving consistently to one\n> convention. Since convention (1) is idiomatic in our code\n> base, we'll pick that one.\n>\n> The existing \"-w\" and \"-b\" tests continue to pass, and a new\n> \"--ignore-space-at-eol\" shows off the breakage we're fixing.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff.c | 15 +++++++----\n> t/t4015-diff-whitespace.sh | 67 ++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 77 insertions(+), 5 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 6fd288420b..09081a207c 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -712,7 +712,7 @@ static int next_byte(const char **cp, const char **endp,\n> {\n> int retval;\n>\n> - if (*cp > *endp)\n> + if (*cp >= *endp)\n> return -1;\n\nThis converts (c) from (2) to (1).\n\n> while (*cp < *endp && isspace(**cp))\n> (*cp)++;\n> - /* return the first non-ws character via the usual below */\n> + /*\n> + * return the first non-ws character via the usual\n> + * below, unless we ate all of the bytes\n> + */\n> + if (*cp >= *endp)\n> + return -1;\n\nThis fixes the mismatch between (d) and (c).\n\nWhen I wrote the code, I did not follow proper commenting style\nby capitalizing the sentence start and putting periods. :(\n(Well it was a single line comment, which I have seen to not\nfollow style occasionally unlike any multi line comment. Anyway\nno need to fix it here and now, just pointing out my bad code\nin the beginning.)\n\n> - while (ae > ap && isspace(*ae))\n> + while (ae > ap && isspace(ae[-1]))\n> ae--;\n> - while (be > bp && isspace(*be))\n> + while (be > bp && isspace(be[-1]))\n> be--;\n...\n> - while (ae > ap && isspace(*ae))\n> + while (ae > ap && isspace(ae[-1]))\n\nThese fixes convert (b) to (1), so we're all on (1).\nAs we check for strict endpointer > firstpointer\n(and not >=), the check of -1 is fine, too.\n\n\n> @@ -1463,6 +1463,73 @@ test_expect_success 'move detection ignoring whitespace changes' '\n> test_cmp expected actual\n> '\n>\n> +test_expect_failure 'move detection ignoring whitespace at eol' '\n> + git reset --hard &&\n> + # Lines 6-9 have new eol whitespace, but 9 also has it in the middle\n> + q_to_tab <<-\\EOF >lines.txt &&\n> + long line 6Q\n> + long line 7Q\n> + long line 8Q\n> + longQline 9Q\n> + line 1\n> + line 2\n> + line 3\n> + line 4\n> + line 5\n> + EOF\n> +\n> + # avoid cluttering the output with complaints about our eol whitespace\n> + test_config core.whitespace -blank-at-eol &&\n\nWe avoid the eol space change as we want to test the move detection\nwithout interference. Do we want to test it with that as well?\n\n> + git diff HEAD --no-renames --color-moved --color |\n> + grep -v \"index\" |\n> + test_decode_color >actual &&\n..\n> + git diff HEAD --no-renames --ignore-space-at-eol --color-moved --color |\n> + grep -v \"index\" |\n> + test_decode_color >actual &&\n..\n> + +long line 9 \n\nok, we also have no interference with space changes,\nwhich we assume is orthogonal.\n\nThe commit message really enlightened me,\nThanks!\n\nStefan\n"},{"id":"330689","messageId":"20171019211947.cif4ui2qwdzizazw@sigill.intra.peff.net","threadId":"46959","inReplyTo":"CAGZ79kZhCoFe1P_gwigzrgzzcqRunrUWGOAgy9xvS2ugNeVLmA@mail.gmail.com","subject":"Re: [PATCH 4/5] diff: fix whitespace-skipping with --color-moved","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T21:19:47Z","receivedAt":"2017-10-19T21:19:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 02:15:12PM -0700, Stefan Beller wrote:\n\n> > +test_expect_failure 'move detection ignoring whitespace at eol' '\n> > + git reset --hard &&\n> > + # Lines 6-9 have new eol whitespace, but 9 also has it in the middle\n> > + q_to_tab <<-\\EOF >lines.txt &&\n> > + long line 6Q\n> > + long line 7Q\n> > + long line 8Q\n> > + longQline 9Q\n> > + line 1\n> > + line 2\n> > + line 3\n> > + line 4\n> > + line 5\n> > + EOF\n> > +\n> > + # avoid cluttering the output with complaints about our eol whitespace\n> > + test_config core.whitespace -blank-at-eol &&\n> \n> We avoid the eol space change as we want to test the move detection\n> without interference. Do we want to test it with that as well?\n\nI don't think it creates interference. It's just that the expected\noutput becomes more like:\n\n +long line 6 \n\nand the extra coloring just made it harder for me to read and write the\ntest. :)\n\nI agree it might be worth checking the interaction between whitespace\ncoloring and --color-moved, but I think we'd want it to be more thorough\n(e.g., covering the beginning of line with indent-with-non-tab or\nsimilar).\n\n> The commit message really enlightened me,\n> Thanks!\n\nThanks for reviewing.\n\n-Peff\n"},{"id":"330690","messageId":"CAGZ79kY3jVgBRCKkswHV7xbjiiNgvsbVsgq_9PpVwa-zTt4q0Q@mail.gmail.com","threadId":"46959","inReplyTo":"20171019203119.d2xbryov2y5t2aig@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] diff: handle NULs in get_string_hash()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T21:31:20Z","receivedAt":"2017-10-19T21:31:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 1:31 PM, Jeff King wrote:\n> For computing moved lines, we feed the characters of each\n> line into a hash. When we've been asked to ignore\n> whitespace, then we pick each character using next_byte(),\n> which returns -1 on end-of-string, which it determines using\n> the start/end pointers we feed it.\n>\n> However our check of its return value treats \"0\" the same as\n> \"-1\", meaning we'd quit if the string has an embedded NUL.\n\nI agree. The code looks correct.\n\n> This is unlikely to ever come up in practice since our line\n> boundaries generally come from calling strlen() in the first\n> place.\n\nget_string_hash is called from\n prepare_entry, which in turn is called from\n add_lines_to_move_detection or mark_color_as_moved\n diff_flush_patch_all_file_pairs\n\nthat constructs the arguments in\ndiff_flush_patch\n run_diff\n run_diff_cmd\n builtin_diff (part \"/* Crazy xdl interfaces.. */\")\n xdi_diff_outf( fn_out_consume as arg!)\n xdi_diff\n xdl_diff\n xdl_call_hunk_func\n -> fn_out_consume(cb, line, len)\n\nxdl_call_hunk_func however uses pointer arithmetic instead\nof strlen. So I think this sentence is not a good idea to put in\nthe commit message.\n\nIt may not occur in practice, due to binary files detection using\nNUL as a signal, but conceptually our move-colored(!) diffs\nshould be compatible with NULs with this patch now.\n\n> But it was a bit surprising to me as a reader of the\n> next_byte() code. And it's possible that we may one day feed\n> this function with more exotic input, which otherwise works\n> with arbitrary ptr/len pairs.\n\nGood point.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> I noticed that we make an extra copy of each line here, just to feed it\n> to memihash! I guess \"-w\" is not a critical-performance code path, but\n> this could be fixed if we could do memhash() incrementally (e.g., by\n> putting the FNV state into a struct and letting callers \"add\" to it\n> incrementally). Maybe an interesting #leftoverbits, though I'd want to\n> see timing tests that show it's worth doing.\n>\n\nI agree.\n\nThanks,\nStefan\n"},{"id":"330691","messageId":"20171019213958.d6rp2upiga5tsvys@sigill.intra.peff.net","threadId":"46959","inReplyTo":"CAGZ79kY3jVgBRCKkswHV7xbjiiNgvsbVsgq_9PpVwa-zTt4q0Q@mail.gmail.com","subject":"Re: [PATCH 5/5] diff: handle NULs in get_string_hash()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-19T21:39:58Z","receivedAt":"2017-10-19T21:40:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 19, 2017 at 02:31:20PM -0700, Stefan Beller wrote:\n\n> > This is unlikely to ever come up in practice since our line\n> > boundaries generally come from calling strlen() in the first\n> > place.\n> \n> get_string_hash is called from\n> prepare_entry, which in turn is called from\n> add_lines_to_move_detection or mark_color_as_moved\n> diff_flush_patch_all_file_pairs\n> \n> that constructs the arguments in\n> diff_flush_patch\n> run_diff\n> run_diff_cmd\n> builtin_diff (part \"/* Crazy xdl interfaces.. */\")\n> xdi_diff_outf( fn_out_consume as arg!)\n> xdi_diff\n> xdl_diff\n> xdl_call_hunk_func\n> -> fn_out_consume(cb, line, len)\n> \n> xdl_call_hunk_func however uses pointer arithmetic instead\n> of strlen. So I think this sentence is not a good idea to put in\n> the commit message.\n> \n> It may not occur in practice, due to binary files detection using\n> NUL as a signal, but conceptually our move-colored(!) diffs\n> should be compatible with NULs with this patch now.\n\nGood catch. I just skimmed over all the diff_emit_* functions, which use\nstrlen(). But at the bottom is emit_add_line(), which has a real length\nmarker from xdiff.\n\nI agree we wouldn't see NULs in general, but this is maybe fixing \"diff\n--color-moved -a\". I dunno. It's probably not worth digging too much,\nsince it seems like the right thing to do regardless.\n\n-Peff\n"},{"id":"330693","messageId":"CAGZ79kahvE=S_SwZqzC_VrAPURVCkqjaa9sVN-bCecYLoA4yRw@mail.gmail.com","threadId":"46959","inReplyTo":"20171019213958.d6rp2upiga5tsvys@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] diff: handle NULs in get_string_hash()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T21:50:58Z","receivedAt":"2017-10-19T21:51:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 2:39 PM, Jeff King wrote:\n> On Thu, Oct 19, 2017 at 02:31:20PM -0700, Stefan Beller wrote:\n>\n>> > This is unlikely to ever come up in practice since our line\n>> > boundaries generally come from calling strlen() in the first\n>> > place.\n>>\n>> get_string_hash is called from\n>> prepare_entry, which in turn is called from\n>> add_lines_to_move_detection or mark_color_as_moved\n>> diff_flush_patch_all_file_pairs\n>>\n>> that constructs the arguments in\n>> diff_flush_patch\n>> run_diff\n>> run_diff_cmd\n>> builtin_diff (part \"/* Crazy xdl interfaces.. */\")\n>> xdi_diff_outf( fn_out_consume as arg!)\n>> xdi_diff\n>> xdl_diff\n>> xdl_call_hunk_func\n>> -> fn_out_consume(cb, line, len)\n>>\n>> xdl_call_hunk_func however uses pointer arithmetic instead\n>> of strlen. So I think this sentence is not a good idea to put in\n>> the commit message.\n>>\n>> It may not occur in practice, due to binary files detection using\n>> NUL as a signal, but conceptually our move-colored(!) diffs\n>> should be compatible with NULs with this patch now.\n>\n> Good catch. I just skimmed over all the diff_emit_* functions, which use\n> strlen(). But at the bottom is emit_add_line(), which has a real length\n> marker from xdiff.\n\nDoh!\nThere is diff_emit_submodule_* but I presume you meant\nemit_diff_* actually as there the strlen is legit, as we generate\nknown non-NUL data to print.\n\nThe submodule output may get mangled in diff_emit_submodule_{add,del}\nas the input is coming from user data.\n\n> I agree we wouldn't see NULs in general, but this is maybe fixing \"diff\n> --color-moved -a\". I dunno. It's probably not worth digging too much,\n> since it seems like the right thing to do regardless.\n\nI agree, that this digging seems not worth it; I was just agitated at\nthe seemingly incorrect commit message.\n\nThanks,\nStefan\n\n>\n> -Peff\n"},{"id":"330719","messageId":"20171020072354.seqloliweseotuos@ruderich.org","threadId":"46959","inReplyTo":"20171019202926.irldca42wqosmxrd@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] diff: fix whitespace-skipping with --color-moved","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-10-20T07:23:54Z","receivedAt":"2017-10-20T07:24:03Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Thu, Oct 19, 2017 at 04:29:26PM -0400, Jeff King wrote:\n> [snip]\n>\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -1463,6 +1463,73 @@ test_expect_success 'move detection ignoring whitespace changes' '\n> \ttest_cmp expected actual\n> '\n>\n> +test_expect_failure 'move detection ignoring whitespace at eol' '\n\nShouldn't this be test_expect_success? According to the commit\nmessage \"and a new \"--ignore-space-at-eol\" shows off the breakage\nwe're fixing.\". I didn't actually run the code so I don't know if\nthe test fails or not.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"330752","messageId":"20171020223709.tlixfgqtr3bzwicw@sigill.intra.peff.net","threadId":"46959","inReplyTo":"20171020072354.seqloliweseotuos@ruderich.org","subject":"Re: [PATCH 4/5] diff: fix whitespace-skipping with --color-moved","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-20T22:37:09Z","receivedAt":"2017-10-20T22:37:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 20, 2017 at 09:23:54AM +0200, Simon Ruderich wrote:\n\n> On Thu, Oct 19, 2017 at 04:29:26PM -0400, Jeff King wrote:\n> > [snip]\n> >\n> > --- a/t/t4015-diff-whitespace.sh\n> > +++ b/t/t4015-diff-whitespace.sh\n> > @@ -1463,6 +1463,73 @@ test_expect_success 'move detection ignoring whitespace changes' '\n> > \ttest_cmp expected actual\n> > '\n> >\n> > +test_expect_failure 'move detection ignoring whitespace at eol' '\n> \n> Shouldn't this be test_expect_success? According to the commit\n> message \"and a new \"--ignore-space-at-eol\" shows off the breakage\n> we're fixing.\". I didn't actually run the code so I don't know if\n> the test fails or not.\n\nThanks, good catch. I had originally added all the tests in a single\ncommit, with the intent of flipping this failure to success. But when I\nshifted it to just get added here, I accidentally lost that flip.\n\n-Peff\n"}]}