{"thread":{"id":"32986","subject":"[PATCH] diff: Fix rename pretty-print when suffix and prefix overlap","startedAt":"2013-02-23T16:48:45Z","lastAt":"2013-03-06T22:03:41Z","messageCount":16,"participants":["Antoine Pelisse","Junio C Hamano","Philip Oakley","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"210093","messageId":"1361638125-11245-1-git-send-email-apelisse@gmail.com","threadId":"32986","inReplyTo":null,"subject":"[PATCH] diff: Fix rename pretty-print when suffix and prefix overlap","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-02-23T16:48:45Z","receivedAt":"2013-02-23T16:48:45Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"When considering a rename for two files that have a suffix and a prefix\nthat can overlap, a confusing line is shown. As an example, renaming\n\"a/b/b/c\" to \"a/b/c\" shows \"a/b/{ => }/b/c\".\n\nCurrently, what we do is calculate the common prefix (\"a/b/\"), and the\ncommon suffix (\"/b/c\"), but the same \"/b/\" is actually counted both in\nprefix and suffix. Then when calculating the size of the non-common part,\nwe end-up with a negative value which is reset to 0, thus the \"{ => }\".\n\nDo not allow the common suffix to overlap the common prefix and stop\nwhen reaching a \"/\" that would be in both.\n\nSigned-off-by: Antoine Pelisse \n---\n diff.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 156fec4..80f4752 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1290,7 +1290,16 @@ static char *pprint_rename(const char *a, const char *b)\n \told = a + len_a;\n \tnew = b + len_b;\n \tsfx_length = 0;\n-\twhile (a <= old && b <= new && *old == *new) {\n+\t/*\n+\t * Note:\n+\t * if pfx_length is 0, old/new will never reach a - 1 because it\n+\t * would mean the whole string is common suffix. But then, the\n+\t * whole string would also be a common prefix, and we would not\n+\t * have pfx_length equals 0.\n+\t */\n+\twhile (a + pfx_length - 1 <= old &&\n+\t b + pfx_length - 1 <= new &&\n+\t *old == *new) {\n \t\tif (*old == '/')\n \t\t\tsfx_length = len_a - (old - a);\n \t\told--;\n--\n1.7.9.5\n"},{"id":"210177","messageId":"7vzjyu3we1.fsf@alter.siamese.dyndns.org","threadId":"32986","inReplyTo":"1361638125-11245-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-24T09:15:50Z","receivedAt":"2013-02-24T09:15:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> When considering a rename for two files that have a suffix and a prefix\n> that can overlap, a confusing line is shown. As an example, renaming\n> \"a/b/b/c\" to \"a/b/c\" shows \"a/b/{ => }/b/c\".\n\nThis would be vastly more readable if it had \"It should show XXX\ninstead\" somewhere in the description, perhaps at the end of this\nsentence. It can also be after \"thus the { => }\" below, but I think\ngiving the expected output earlier would be more appropriate.\n\n> Currently, what we do is calculate the common prefix (\"a/b/\"), and the\n> common suffix (\"/b/c\"), but the same \"/b/\" is actually counted both in\n> prefix and suffix. Then when calculating the size of the non-common part,\n> we end-up with a negative value which is reset to 0, thus the \"{ => }\".\n\nIn this example, the common prefix would be \"a/b/\" and the common\nsuffix that does not overlap with the prefix part would be \"/c\", so\nI am imagining that \"a/b/{ => b}/c\" would be the desired output?\n\nThis is a really old thinko (dating back to June 2005). I'll queue\nthe patch on maint-1.7.6 (because 1.7.6.6 is slightly more than one\nyear old while 1.7.5.4 is a lot older) to allow distros that issue\nincremental fixes on top of ancient versions of Git to pick up the\nfix if they wanted to. Perhaps we would want to add a few tests?\n\nThanks.\n\n>\n> Do not allow the common suffix to overlap the common prefix and stop\n> when reaching a \"/\" that would be in both.\n>\n> Signed-off-by: Antoine Pelisse \n> ---\n> diff.c | 11 ++++++++++-\n> 1 file changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 156fec4..80f4752 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1290,7 +1290,16 @@ static char *pprint_rename(const char *a, const char *b)\n> \told = a + len_a;\n> \tnew = b + len_b;\n> \tsfx_length = 0;\n> -\twhile (a <= old && b <= new && *old == *new) {\n> +\t/*\n> +\t * Note:\n> +\t * if pfx_length is 0, old/new will never reach a - 1 because it\n> +\t * would mean the whole string is common suffix. But then, the\n> +\t * whole string would also be a common prefix, and we would not\n> +\t * have pfx_length equals 0.\n> +\t */\n> +\twhile (a + pfx_length - 1 <= old &&\n> +\t b + pfx_length - 1 <= new &&\n> +\t *old == *new) {\n> \t\tif (*old == '/')\n> \t\t\tsfx_length = len_a - (old - a);\n> \t\told--;\n> --\n> 1.7.9.5\n"},{"id":"210286","messageId":"CALWbr2yviqF68zF7mBbhaXW7oFar0YRqROBWXwqjo7UNgZNVBQ@mail.gmail.com","threadId":"32986","inReplyTo":"7vzjyu3we1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-02-25T19:50:24Z","receivedAt":"2013-02-25T19:50:24Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Sun, Feb 24, 2013 at 10:15 AM, Junio C Hamano wrote:\n> Antoine Pelisse writes:\n>\n>> When considering a rename for two files that have a suffix and a prefix\n>> that can overlap, a confusing line is shown. As an example, renaming\n>> \"a/b/b/c\" to \"a/b/c\" shows \"a/b/{ => }/b/c\".\n>\n> This would be vastly more readable if it had \"It should show XXX\n> instead\" somewhere in the description, perhaps at the end of this\n> sentence. It can also be after \"thus the { => }\" below, but I think\n> giving the expected output earlier would be more appropriate.\n\nGood catch, this would probably be better:\n\n When considering a rename for two files that have a suffix and a prefix\n that can overlap, a confusing line is shown. As an example, renaming\n \"a/b/b/c\" to \"a/b/c\" shows \"a/b/{ => }/b/c\", instead of \"a/b/{ => b}/c\"\n\n>> Currently, what we do is calculate the common prefix (\"a/b/\"), and the\n>> common suffix (\"/b/c\"), but the same \"/b/\" is actually counted both in\n>> prefix and suffix. Then when calculating the size of the non-common part,\n>> we end-up with a negative value which is reset to 0, thus the \"{ => }\".\n>\n> In this example, the common prefix would be \"a/b/\" and the common\n> suffix that does not overlap with the prefix part would be \"/c\", so\n> I am imagining that \"a/b/{ => b}/c\" would be the desired output?\n\nYes, at least that's what I expected.\n\n> This is a really old thinko (dating back to June 2005). I'll queue\n> the patch on maint-1.7.6 (because 1.7.6.6 is slightly more than one\n> year old while 1.7.5.4 is a lot older) to allow distros that issue\n> incremental fixes on top of ancient versions of Git to pick up the\n> fix if they wanted to. Perhaps we would want to add a few tests?\n\nI can easily understand why that was missed.\nI will try to resubmit with tests very soon.\n"},{"id":"210303","messageId":"512BE787.1090901@iee.org","threadId":"32986","inReplyTo":"CALWbr2yviqF68zF7mBbhaXW7oFar0YRqROBWXwqjo7UNgZNVBQ@mail.gmail.com","subject":"Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-02-25T22:36:55Z","receivedAt":"2013-02-25T22:36:55Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 25/02/13 19:50, Antoine Pelisse wrote:\n> On Sun, Feb 24, 2013 at 10:15 AM, Junio C Hamano wrote:\n>> Antoine Pelisse writes:\n>>\n>>> When considering a rename for two files that have a suffix and a prefix\n>>> that can overlap, a confusing line is shown. As an example, renaming\n>>> \"a/b/b/c\" to \"a/b/c\" shows \"a/b/{ => }/b/c\".\n>>\n>> This would be vastly more readable if it had \"It should show XXX\n>> instead\" somewhere in the description, perhaps at the end of this\n>> sentence. It can also be after \"thus the { => }\" below, but I think\n>> giving the expected output earlier would be more appropriate.\n>\n> Good catch, this would probably be better:\n>\n> When considering a rename for two files that have a suffix and a prefix\n> that can overlap, a confusing line is shown. As an example, renaming\n> \"a/b/b/c\" to \"a/b/c\" shows \"a/b/{ => }/b/c\", instead of \"a/b/{ => b}/c\"\n>\n>>> Currently, what we do is calculate the common prefix (\"a/b/\"), and the\n>>> common suffix (\"/b/c\"), but the same \"/b/\" is actually counted both in\n>>> prefix and suffix. Then when calculating the size of the non-common part,\n>>> we end-up with a negative value which is reset to 0, thus the \"{ => }\".\n>>\n>> In this example, the common prefix would be \"a/b/\" and the common\n>> suffix that does not overlap with the prefix part would be \"/c\", so\n>> I am imagining that \"a/b/{ => b}/c\" would be the desired output?\n>\n> Yes, at least that's what I expected.\n\nSurely it would be \"a/b/{b => }/c\", that is, we have reduced the number \nof b's by one. Or am I misunderstanding something?\n(I'm guessing it was an all too obvious typo that was misread)\n\n>\n>> This is a really old thinko (dating back to June 2005). I'll queue\n>> the patch on maint-1.7.6 (because 1.7.6.6 is slightly more than one\n>> year old while 1.7.5.4 is a lot older) to allow distros that issue\n>> incremental fixes on top of ancient versions of Git to pick up the\n>> fix if they wanted to. Perhaps we would want to add a few tests?\n>\n> I can easily understand why that was missed.\n> I will try to resubmit with tests very soon.\nPhilip\n"},{"id":"210304","messageId":"CALWbr2w47K0jqrKaQs3eTUtQ18_L5TrLR=BttspKACT7vCmWEA@mail.gmail.com","threadId":"32986","inReplyTo":"512BE787.1090901@iee.org","subject":"Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-02-25T22:41:26Z","receivedAt":"2013-02-25T22:41:26Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":">>> In this example, the common prefix would be \"a/b/\" and the common\n>>> suffix that does not overlap with the prefix part would be \"/c\", so\n>>> I am imagining that \"a/b/{ => b}/c\" would be the desired output?\n>>\n>>\n>> Yes, at least that's what I expected.\n>\n>\n> Surely it would be \"a/b/{b => }/c\", that is, we have reduced the number of\n> b's by one. Or am I misunderstanding something?\n> (I'm guessing it was an all too obvious typo that was misread)\n\nIndeed, read to fast and reproduced in suggested new message.\na/b/b/c => a/b/c is equivalent to a/b/{b => }/c\n\nThank you for proof-reading.\n"},{"id":"210368","messageId":"d74fcfa693e5feb7249023feda8d3ad1f22ab16d.1361911196.git.trast@student.ethz.ch","threadId":"32986","inReplyTo":"1361638125-11245-1-git-send-email-apelisse@gmail.com","subject":"[PATCH] diff: prevent pprint_rename from underrunning input","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2013-02-26T20:47:01Z","receivedAt":"2013-02-26T20:47:01Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The logic described in d020e27 (diff: Fix rename pretty-print when\nsuffix and prefix overlap, 2013-02-23) is wrong: The proof in the\ncomment is valid only if both strings are the same length. *One* of\nold/new can reach a-1 (b-1, resp.) if 'a' is a suffix of 'b' (or vice\nversa).\n\nSince the intent was to let the loop run down to the '/' at the end of\nthe common prefix, fix it by making that distinction explicit: if\nthere is no prefix, allow no underrun.\n\nSigned-off-by: Thomas Rast \n---\n\nHi Antoine,\n\nUnfortunately there's this bug in your patch. Luckily it was found by\nvalgrind on t4016 and others.\n\nCheers\nThomas\n\n diff.c | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 7d49cab..8396527 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1264,6 +1264,7 @@ static char *pprint_rename(const char *a, const char *b)\n \tconst char *new = b;\n \tstruct strbuf name = STRBUF_INIT;\n \tint pfx_length, sfx_length;\n+\tint pfx_adjust_for_slash;\n \tint len_a = strlen(a);\n \tint len_b = strlen(b);\n \tint a_midlen, b_midlen;\n@@ -1291,14 +1292,16 @@ static char *pprint_rename(const char *a, const char *b)\n \tnew = b + len_b;\n \tsfx_length = 0;\n \t/*\n-\t * Note:\n-\t * if pfx_length is 0, old/new will never reach a - 1 because it\n-\t * would mean the whole string is common suffix. But then, the\n-\t * whole string would also be a common prefix, and we would not\n-\t * have pfx_length equals 0.\n+\t * If there is a common prefix, it must end in a slash. In\n+\t * that case we let this loop run 1 into the prefix to see the\n+\t * same slash.\n+\t *\n+\t * If there is no common prefix, we cannot do this as it would\n+\t * underrun the input strings.\n \t */\n-\twhile (a + pfx_length - 1 <= old &&\n-\t b + pfx_length - 1 <= new &&\n+\tpfx_adjust_for_slash = (pfx_length ? 1 : 0);\n+\twhile (a + pfx_length - pfx_adjust_for_slash <= old &&\n+\t b + pfx_length - pfx_adjust_for_slash <= new &&\n \t *old == *new) {\n \t\tif (*old == '/')\n \t\t\tsfx_length = len_a - (old - a);\n-- \n1.8.2.rc1.307.ge0d2dea\n"},{"id":"210372","messageId":"7vsj4ilphz.fsf@alter.siamese.dyndns.org","threadId":"32986","inReplyTo":"d74fcfa693e5feb7249023feda8d3ad1f22ab16d.1361911196.git.trast@student.ethz.ch","subject":"Re: [PATCH] diff: prevent pprint_rename from underrunning input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-26T21:44:08Z","receivedAt":"2013-02-26T21:44:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> The logic described in d020e27 (diff: Fix rename pretty-print when\n> suffix and prefix overlap, 2013-02-23) is wrong: The proof in the\n> comment is valid only if both strings are the same length. *One* of\n> old/new can reach a-1 (b-1, resp.) if 'a' is a suffix of 'b' (or vice\n> versa).\n\nThanks. I was also having hard time convincing myself why that -1\ndoes not under-run yesterday.\n\nWill queue.\n"},{"id":"210391","messageId":"CALWbr2zq9WzQiry0U+G0mpQOqd838r8+GYJ91601MBMp=D94xA@mail.gmail.com","threadId":"32986","inReplyTo":"7vsj4ilphz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff: prevent pprint_rename from underrunning input","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-02-27T13:27:15Z","receivedAt":"2013-02-27T13:27:15Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":">> The logic described in d020e27 (diff: Fix rename pretty-print when\n>> suffix and prefix overlap, 2013-02-23) is wrong: The proof in the\n>> comment is valid only if both strings are the same length. *One* of\n>> old/new can reach a-1 (b-1, resp.) if 'a' is a suffix of 'b' (or vice\n>> versa).\n\nIndeed, sorry about that. I guess I was too focused on my specific\nissue and didn't broaden my thoughts.\n\n> Thanks. I was also having hard time convincing myself why that -1\n> does not under-run yesterday.\n>\n> Will queue.\n\nThanks\n"},{"id":"210443","messageId":"1362088540-14564-1-git-send-email-apelisse@gmail.com","threadId":"32986","inReplyTo":"CALWbr2yviqF68zF7mBbhaXW7oFar0YRqROBWXwqjo7UNgZNVBQ@mail.gmail.com","subject":"[PATCH] diff: Fix rename pretty-print when suffix and prefix overlap","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-02-28T21:55:40Z","receivedAt":"2013-02-28T21:55:40Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"When considering a rename for two files that have a suffix and a prefix\nthat can overlap, a confusing line is shown. As an example, renaming\n\"a/b/b/c\" to \"a/b/c\" shows \"a/b/{ => }/b/c\", instead of \"a/b/{b => }/c\"\n\nCurrently, what we do is calculate the common prefix (\"a/b/\"), and the\ncommon suffix (\"/b/c\"), but the same \"/b/\" is actually counted both in\nprefix and suffix. Then when calculating the size of the non-common part,\nwe end-up with a negative value which is reset to 0, thus the \"{ => }\".\n\nDo not allow the common suffix to overlap the common prefix and stop\nwhen reaching a \"/\" that would be in both.\n\nAlso add some test file to place corner-cases we could met (and this one)\nwith rename pretty print.\n\nSigned-off-by: Antoine Pelisse \n---\n diff.c | 11 +++++++++-\n t/t4056-rename-pretty.sh | 54 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 64 insertions(+), 1 deletion(-)\n create mode 100755 t/t4056-rename-pretty.sh\n\ndiff --git a/diff.c b/diff.c\nindex 9038f19..e1d82c9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1177,7 +1177,16 @@ static char *pprint_rename(const char *a, const char *b)\n \told = a + len_a;\n \tnew = b + len_b;\n \tsfx_length = 0;\n-\twhile (a <= old && b <= new && *old == *new) {\n+\t/*\n+\t * Note:\n+\t * if pfx_length is 0, old/new will never reach a - 1 because it\n+\t * would mean the whole string is common suffix. But then, the\n+\t * whole string would also be a common prefix, and we would not\n+\t * have pfx_length equals 0.\n+\t */\n+\twhile (a + pfx_length - 1 <= old &&\n+\t b + pfx_length - 1 <= new &&\n+\t *old == *new) {\n \t\tif (*old == '/')\n \t\t\tsfx_length = len_a - (old - a);\n \t\told--;\ndiff --git a/t/t4056-rename-pretty.sh b/t/t4056-rename-pretty.sh\nnew file mode 100755\nindex 0000000..806046f\n--- /dev/null\n+++ b/t/t4056-rename-pretty.sh\n@@ -0,0 +1,54 @@\n+#!/bin/sh\n+\n+test_description='Rename pretty print\n+\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success nothing_common '\n+\tmkdir -p a/b/ &&\n+\t: >a/b/c &&\n+\tgit add a/b/c &&\n+\tgit commit -m. &&\n+\tmkdir -p c/b/ &&\n+\tgit mv a/b/c c/b/a &&\n+\tgit commit -m. &&\n+\tgit show -M --summary >output &&\n+\ttest_i18ngrep \"a/b/c => c/b/a\" output\n+'\n+\n+test_expect_success common_prefix '\n+\tmkdir -p c/d &&\n+\tgit mv c/b/a c/d/e &&\n+\tgit commit -m. &&\n+\tgit show -M --summary >output &&\n+\ttest_i18ngrep \"c/{b/a => d/e}\" output\n+'\n+\n+test_expect_success common_suffix '\n+\tmkdir d &&\n+\tgit mv c/d/e d/e &&\n+\tgit commit -m. &&\n+\tgit show -M --summary >output &&\n+\ttest_i18ngrep \"{c/d => d}/e\" output\n+'\n+\n+test_expect_success common_suffix_prefix '\n+\tmkdir d/f &&\n+\tgit mv d/e d/f/e &&\n+\tgit commit -m. &&\n+\tgit show -M --summary >output &&\n+\ttest_i18ngrep \"d/{ => f}/e\" output\n+'\n+\n+test_expect_success common_overlap '\n+\tmkdir d/f/f &&\n+\tgit mv d/f/e d/f/f/e &&\n+\tgit commit -m. &&\n+\tgit show -M --summary >output &&\n+\ttest_i18ngrep \"d/f/{ => f}/e\" output\n+'\n+\n+\n+test_done\n--\n1.7.9.5\n"},{"id":"210444","messageId":"874ngw2ii0.fsf@pctrast.inf.ethz.ch","threadId":"32986","inReplyTo":"1362088540-14564-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2013-02-28T22:14:47Z","receivedAt":"2013-02-28T22:14:47Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Antoine Pelisse writes:\n\n> diff --git a/diff.c b/diff.c\n> index 9038f19..e1d82c9 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1177,7 +1177,16 @@ static char *pprint_rename(const char *a, const char *b)\n> -\twhile (a <= old && b <= new && *old == *new) {\n> +\t/*\n> +\t * Note:\n> +\t * if pfx_length is 0, old/new will never reach a - 1 because it\n> +\t * would mean the whole string is common suffix. But then, the\n> +\t * whole string would also be a common prefix, and we would not\n> +\t * have pfx_length equals 0.\n> +\t */\n> +\twhile (a + pfx_length - 1 <= old &&\n> +\t b + pfx_length - 1 <= new &&\n> +\t *old == *new) {\n\nUmm, you still have the broken version here, and the previous patch is\nalready in next. I think you should decide for one thing ;-)\n\nEither: consider this a reroll; Junio would have to revert the version\nalready in next (which isn't _so_ bad, because next will eventually be\nrebuilt) and apply this new version. But if you do that, you should\nsquash my change that deals with the underrun issue (I'd be fine with\nthat).\n\nOr: consider it an incremental improvement on the series, in which case\nyou should send only the tests with a new commit message.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"210445","messageId":"CALWbr2y7d5kGQpF=gBCkVx2ZgZR-9mmdhj17f2QcZZTaMPM_4w@mail.gmail.com","threadId":"32986","inReplyTo":"874ngw2ii0.fsf@pctrast.inf.ethz.ch","subject":"Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-02-28T22:22:05Z","receivedAt":"2013-02-28T22:22:05Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Thu, Feb 28, 2013 at 11:14 PM, Thomas Rast wrote:\n> Antoine Pelisse writes:\n>\n>> diff --git a/diff.c b/diff.c\n>> index 9038f19..e1d82c9 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -1177,7 +1177,16 @@ static char *pprint_rename(const char *a, const char *b)\n>> - while (a <= old && b <= new && *old == *new) {\n>> + /*\n>> + * Note:\n>> + * if pfx_length is 0, old/new will never reach a - 1 because it\n>> + * would mean the whole string is common suffix. But then, the\n>> + * whole string would also be a common prefix, and we would not\n>> + * have pfx_length equals 0.\n>> + */\n>> + while (a + pfx_length - 1 <= old &&\n>> + b + pfx_length - 1 <= new &&\n>> + *old == *new) {\n>\n> Umm, you still have the broken version here, and the previous patch is\n> already in next. I think you should decide for one thing ;-)\n\nThanks ! I had not paid enough attention to that.\n\n> Either: consider this a reroll; Junio would have to revert the version\n> already in next (which isn't _so_ bad, because next will eventually be\n> rebuilt) and apply this new version. But if you do that, you should\n> squash my change that deals with the underrun issue (I'd be fine with\n> that).\n\nI would not have done that without your consent (that's why I kept the\nbuggy version)\n\n> Or: consider it an incremental improvement on the series, in which case\n> you should send only the tests with a new commit message.\n\nThat seems like the best solution to me. I will resend later with just\nthe tests and a new commit message.\n\nCheers,\nAntoine\n"},{"id":"210447","messageId":"7vip5cgjhp.fsf@alter.siamese.dyndns.org","threadId":"32986","inReplyTo":"1362088540-14564-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-28T22:29:38Z","receivedAt":"2013-02-28T22:29:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> When considering a rename for two files that have a suffix and a prefix\n> that can overlap, a confusing line is shown. As an example, renaming\n> \"a/b/b/c\" to \"a/b/c\" shows \"a/b/{ => }/b/c\", instead of \"a/b/{b => }/c\"\n>\n> Currently, what we do is calculate the common prefix (\"a/b/\"), and the\n> common suffix (\"/b/c\"), but the same \"/b/\" is actually counted both in\n> prefix and suffix. Then when calculating the size of the non-common part,\n> we end-up with a negative value which is reset to 0, thus the \"{ => }\".\n>\n> Do not allow the common suffix to overlap the common prefix and stop\n> when reaching a \"/\" that would be in both.\n>\n> Also add some test file to place corner-cases we could met (and this one)\n> with rename pretty print.\n>\n> Signed-off-by: Antoine Pelisse \n> ---\n\nHmm, haven't we already applied this with a fix from Thomas Rast and\nqueued the result to 'next'?\n"},{"id":"210510","messageId":"1362235092-16914-1-git-send-email-apelisse@gmail.com","threadId":"32986","inReplyTo":"7vip5cgjhp.fsf@alter.siamese.dyndns.org","subject":"[PATCH] tests: make sure rename pretty print works","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-03-02T14:38:12Z","receivedAt":"2013-03-02T14:38:12Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"Add basic use cases and corner cases tests for\n\"git diff -M --summary/stat\".\n\nSigned-off-by: Antoine Pelisse \n---\n t/t4056-rename-pretty.sh | 54 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 54 insertions(+)\n create mode 100755 t/t4056-rename-pretty.sh\n\ndiff --git a/t/t4056-rename-pretty.sh b/t/t4056-rename-pretty.sh\nnew file mode 100755\nindex 0000000..806046f\n--- /dev/null\n+++ b/t/t4056-rename-pretty.sh\n@@ -0,0 +1,54 @@\n+#!/bin/sh\n+\n+test_description='Rename pretty print\n+\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success nothing_common '\n+\tmkdir -p a/b/ &&\n+\t: >a/b/c &&\n+\tgit add a/b/c &&\n+\tgit commit -m. &&\n+\tmkdir -p c/b/ &&\n+\tgit mv a/b/c c/b/a &&\n+\tgit commit -m. &&\n+\tgit show -M --summary >output &&\n+\ttest_i18ngrep \"a/b/c => c/b/a\" output\n+'\n+\n+test_expect_success common_prefix '\n+\tmkdir -p c/d &&\n+\tgit mv c/b/a c/d/e &&\n+\tgit commit -m. &&\n+\tgit show -M --summary >output &&\n+\ttest_i18ngrep \"c/{b/a => d/e}\" output\n+'\n+\n+test_expect_success common_suffix '\n+\tmkdir d &&\n+\tgit mv c/d/e d/e &&\n+\tgit commit -m. &&\n+\tgit show -M --summary >output &&\n+\ttest_i18ngrep \"{c/d => d}/e\" output\n+'\n+\n+test_expect_success common_suffix_prefix '\n+\tmkdir d/f &&\n+\tgit mv d/e d/f/e &&\n+\tgit commit -m. &&\n+\tgit show -M --summary >output &&\n+\ttest_i18ngrep \"d/{ => f}/e\" output\n+'\n+\n+test_expect_success common_overlap '\n+\tmkdir d/f/f &&\n+\tgit mv d/f/e d/f/f/e &&\n+\tgit commit -m. &&\n+\tgit show -M --summary >output &&\n+\ttest_i18ngrep \"d/f/{ => f}/e\" output\n+'\n+\n+\n+test_done\n-- \n1.7.9.5\n"},{"id":"210520","messageId":"7vehfxc6yt.fsf@alter.siamese.dyndns.org","threadId":"32986","inReplyTo":"1362235092-16914-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] tests: make sure rename pretty print works","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-03T06:50:34Z","receivedAt":"2013-03-03T06:50:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> Add basic use cases and corner cases tests for\n> \"git diff -M --summary/stat\".\n>\n> Signed-off-by: Antoine Pelisse \n> ---\n> t/t4056-rename-pretty.sh | 54 ++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 54 insertions(+)\n> create mode 100755 t/t4056-rename-pretty.sh\n\nI wonder if this needs a new test script of its own.\n\nIf we anticipate future additions, it would make sense but otherwise\nit may be better if we can find an existing one these tests can be\nfolded into.\n\n> diff --git a/t/t4056-rename-pretty.sh b/t/t4056-rename-pretty.sh\n> new file mode 100755\n> index 0000000..806046f\n> --- /dev/null\n> +++ b/t/t4056-rename-pretty.sh\n> @@ -0,0 +1,54 @@\n> +#!/bin/sh\n> +\n> +test_description='Rename pretty print\n> +\n> +'\n\nA single line would be sufficient...\n\n> +test_expect_success common_prefix '\n> +\tmkdir -p c/d &&\n> +\tgit mv c/b/a c/d/e &&\n> +\tgit commit -m. &&\n> +\tgit show -M --summary >output &&\n\nI guess the unsightly \"commit -m.\" is an attempt to prevent the\nlater grep from matching log message randomly, but if you test the\noutput from \"git diff -M --stat/summary HEAD^ HEAD\" you do not have\nto worry about it, no?\n\nAlso I wonder if we can verify the filename part in --stat output.\n\n> +\ttest_i18ngrep \"c/{b/a => d/e}\" output\n\nWe would want to make sure that we do not have random cruft around\nthe paths, and the byte before that 'c' and after that '}' may want\nto be verified.\n"},{"id":"210723","messageId":"1362605772-14639-1-git-send-email-apelisse@gmail.com","threadId":"32986","inReplyTo":"1362235092-16914-1-git-send-email-apelisse@gmail.com","subject":"[PATCH v2] tests: make sure rename pretty print works","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-03-06T21:36:12Z","receivedAt":"2013-03-06T21:36:12Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"Add basic use cases and corner cases tests for\n\"git diff -M --summary/stat\".\n\nSigned-off-by: Antoine Pelisse \n---\nlist of fixes:\n - Test using diff instead of show\n (that is more consistent with commit message).\n - add extra spaces around paths\n - Use better commit messages\n - Moved to existing t4001\n\n t/t4001-diff-rename.sh | 54 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 54 insertions(+)\n\ndiff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\nindex 844277c..2f327b7 100755\n--- a/t/t4001-diff-rename.sh\n+++ b/t/t4001-diff-rename.sh\n@@ -102,4 +102,58 @@ test_expect_success 'setup for many rename source candidates' '\n \tgrep warning actual.err\n '\n\n+test_expect_success 'rename pretty print with nothing in common' '\n+\tmkdir -p a/b/ &&\n+\t: >a/b/c &&\n+\tgit add a/b/c &&\n+\tgit commit -m \"create a/b/c\" &&\n+\tmkdir -p c/b/ &&\n+\tgit mv a/b/c c/b/a &&\n+\tgit commit -m \"a/b/c -> c/b/a\" &&\n+\tgit diff -M --summary HEAD^ HEAD >output &&\n+\ttest_i18ngrep \" a/b/c => c/b/a \" output &&\n+\tgit diff -M --stat HEAD^ HEAD >output &&\n+\ttest_i18ngrep \" a/b/c => c/b/a \" output\n+'\n+\n+test_expect_success 'rename pretty print with common prefix' '\n+\tmkdir -p c/d &&\n+\tgit mv c/b/a c/d/e &&\n+\tgit commit -m \"c/b/a -> c/d/e\" &&\n+\tgit diff -M --summary HEAD^ HEAD >output &&\n+\ttest_i18ngrep \" c/{b/a => d/e} \" output &&\n+\tgit diff -M --stat HEAD^ HEAD >output &&\n+\ttest_i18ngrep \" c/{b/a => d/e} \" output\n+'\n+\n+test_expect_success 'rename pretty print with common suffix' '\n+\tmkdir d &&\n+\tgit mv c/d/e d/e &&\n+\tgit commit -m \"c/d/e -> d/e\" &&\n+\tgit diff -M --summary HEAD^ HEAD >output &&\n+\ttest_i18ngrep \" {c/d => d}/e \" output &&\n+\tgit diff -M --stat HEAD^ HEAD >output &&\n+\ttest_i18ngrep \" {c/d => d}/e \" output\n+'\n+\n+test_expect_success 'rename pretty print with common prefix and suffix' '\n+\tmkdir d/f &&\n+\tgit mv d/e d/f/e &&\n+\tgit commit -m \"d/e -> d/f/e\" &&\n+\tgit diff -M --summary HEAD^ HEAD >output &&\n+\ttest_i18ngrep \" d/{ => f}/e \" output &&\n+\tgit diff -M --stat HEAD^ HEAD >output &&\n+\ttest_i18ngrep \" d/{ => f}/e \" output\n+'\n+\n+test_expect_success 'rename pretty print common prefix and suffix overlap' '\n+\tmkdir d/f/f &&\n+\tgit mv d/f/e d/f/f/e &&\n+\tgit commit -m \"d/f/e d/f/f/e\" &&\n+\tgit diff -M --summary HEAD^ HEAD >output &&\n+\ttest_i18ngrep \" d/f/{ => f}/e \" output &&\n+\tgit diff -M --stat HEAD^ HEAD >output &&\n+\ttest_i18ngrep \" d/f/{ => f}/e \" output\n+'\n+\n test_done\n--\n1.7.9.5\n"},{"id":"210728","messageId":"7v7glkp4n6.fsf@alter.siamese.dyndns.org","threadId":"32986","inReplyTo":"1362605772-14639-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH v2] tests: make sure rename pretty print works","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-06T22:03:41Z","receivedAt":"2013-03-06T22:03:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> Add basic use cases and corner cases tests for\n> \"git diff -M --summary/stat\".\n>\n> Signed-off-by: Antoine Pelisse \n> ---\n> list of fixes:\n> - Test using diff instead of show\n> (that is more consistent with commit message).\n> - add extra spaces around paths\n> - Use better commit messages\n> - Moved to existing t4001\n>\n> t/t4001-diff-rename.sh | 54 ++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 54 insertions(+)\n\nNice.\n\nWill queue, but we may want to make these tests more independent\nfrom each other later. For example, if the first test fails before\nits \"git mv a/b/c c/b/a\", the set-up of the second test to further\nmove it to c/d/e will fail, which is probably not very desirable.\n\nThanks.\n\n> +test_expect_success 'rename pretty print with nothing in common' '\n> +\tmkdir -p a/b/ &&\n> +\t: >a/b/c &&\n> +\tgit add a/b/c &&\n> +\tgit commit -m \"create a/b/c\" &&\n> +\tmkdir -p c/b/ &&\n> +\tgit mv a/b/c c/b/a &&\n> +\tgit commit -m \"a/b/c -> c/b/a\" &&\n> +\tgit diff -M --summary HEAD^ HEAD >output &&\n> +\ttest_i18ngrep \" a/b/c => c/b/a \" output &&\n> +\tgit diff -M --stat HEAD^ HEAD >output &&\n> +\ttest_i18ngrep \" a/b/c => c/b/a \" output\n> +'\n> +\n> +test_expect_success 'rename pretty print with common prefix' '\n> +\tmkdir -p c/d &&\n> +\tgit mv c/b/a c/d/e &&\n> +\tgit commit -m \"c/b/a -> c/d/e\" &&\n> +\tgit diff -M --summary HEAD^ HEAD >output &&\n> +\ttest_i18ngrep \" c/{b/a => d/e} \" output &&\n> +\tgit diff -M --stat HEAD^ HEAD >output &&\n> +\ttest_i18ngrep \" c/{b/a => d/e} \" output\n> +'\n> +\n> +test_expect_success 'rename pretty print with common suffix' '\n> +\tmkdir d &&\n> +\tgit mv c/d/e d/e &&\n> +\tgit commit -m \"c/d/e -> d/e\" &&\n> +\tgit diff -M --summary HEAD^ HEAD >output &&\n> +\ttest_i18ngrep \" {c/d => d}/e \" output &&\n> +\tgit diff -M --stat HEAD^ HEAD >output &&\n> +\ttest_i18ngrep \" {c/d => d}/e \" output\n> +'\n> +\n> +test_expect_success 'rename pretty print with common prefix and suffix' '\n> +\tmkdir d/f &&\n> +\tgit mv d/e d/f/e &&\n> +\tgit commit -m \"d/e -> d/f/e\" &&\n> +\tgit diff -M --summary HEAD^ HEAD >output &&\n> +\ttest_i18ngrep \" d/{ => f}/e \" output &&\n> +\tgit diff -M --stat HEAD^ HEAD >output &&\n> +\ttest_i18ngrep \" d/{ => f}/e \" output\n> +'\n> +\n> +test_expect_success 'rename pretty print common prefix and suffix overlap' '\n> +\tmkdir d/f/f &&\n> +\tgit mv d/f/e d/f/f/e &&\n> +\tgit commit -m \"d/f/e d/f/f/e\" &&\n> +\tgit diff -M --summary HEAD^ HEAD >output &&\n> +\ttest_i18ngrep \" d/f/{ => f}/e \" output &&\n> +\tgit diff -M --stat HEAD^ HEAD >output &&\n> +\ttest_i18ngrep \" d/f/{ => f}/e \" output\n> +'\n> +\n> test_done\n> --\n> 1.7.9.5\n"}]}