[PATCH] diff: Fix rename pretty-print when suffix and prefix overlap

[PATCH] diff: Fix rename pretty-print when suffix and prefix overlap

[Antoine Pelisse]

When considering a rename for two files that have a suffix and a prefix
that can overlap, a confusing line is shown. As an example, renaming
"a/b/b/c" to "a/b/c" shows "a/b/{ => }/b/c".

Currently, what we do is calculate the common prefix ("a/b/"), and the
common suffix ("/b/c"), but the same "/b/" is actually counted both in
prefix and suffix. Then when calculating the size of the non-common part,
we end-up with a negative value which is reset to 0, thus the "{ => }".

Do not allow the common suffix to overlap the common prefix and stop
when reaching a "/" that would be in both.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 diff.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 156fec4..80f4752 100644
--- a/diff.c
+++ b/diff.c
@@ -1290,7 +1290,16 @@ static char *pprint_rename(const char *a, const char *b)
 	old = a + len_a;
 	new = b + len_b;
 	sfx_length = 0;
-	while (a <= old && b <= new && *old == *new) {
+	/*
+	 * Note:
+	 * if pfx_length is 0, old/new will never reach a - 1 because it
+	 * would mean the whole string is common suffix. But then, the
+	 * whole string would also be a common prefix, and we would not
+	 * have pfx_length equals 0.
+	 */
+	while (a + pfx_length - 1 <= old &&
+	       b + pfx_length - 1 <= new &&
+	       *old == *new) {
 		if (*old == '/')
 			sfx_length = len_a - (old - a);
 		old--;
--
1.7.9.5

Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> When considering a rename for two files that have a suffix and a prefix
> that can overlap, a confusing line is shown. As an example, renaming
> "a/b/b/c" to "a/b/c" shows "a/b/{ => }/b/c".

This would be vastly more readable if it had "It should show XXX
instead" somewhere in the description, perhaps at the end of this
sentence.  It can also be after "thus the { => }" below, but I think
giving the expected output earlier would be more appropriate.

> Currently, what we do is calculate the common prefix ("a/b/"), and the
> common suffix ("/b/c"), but the same "/b/" is actually counted both in
> prefix and suffix. Then when calculating the size of the non-common part,
> we end-up with a negative value which is reset to 0, thus the "{ => }".

In this example, the common prefix would be "a/b/" and the common
suffix that does not overlap with the prefix part would be "/c", so
I am imagining that "a/b/{ => b}/c" would be the desired output?

This is a really old thinko (dating back to June 2005).  I'll queue
the patch on maint-1.7.6 (because 1.7.6.6 is slightly more than one
year old while 1.7.5.4 is a lot older) to allow distros that issue
incremental fixes on top of ancient versions of Git to pick up the
fix if they wanted to.  Perhaps we would want to add a few tests?

Thanks.

>
> Do not allow the common suffix to overlap the common prefix and stop
> when reaching a "/" that would be in both.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
>  diff.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/diff.c b/diff.c
> index 156fec4..80f4752 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1290,7 +1290,16 @@ static char *pprint_rename(const char *a, const char *b)
>  	old = a + len_a;
>  	new = b + len_b;
>  	sfx_length = 0;
> -	while (a <= old && b <= new && *old == *new) {
> +	/*
> +	 * Note:
> +	 * if pfx_length is 0, old/new will never reach a - 1 because it
> +	 * would mean the whole string is common suffix. But then, the
> +	 * whole string would also be a common prefix, and we would not
> +	 * have pfx_length equals 0.
> +	 */
> +	while (a + pfx_length - 1 <= old &&
> +	       b + pfx_length - 1 <= new &&
> +	       *old == *new) {
>  		if (*old == '/')
>  			sfx_length = len_a - (old - a);
>  		old--;
> --
> 1.7.9.5

Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap

[Antoine Pelisse]

On Sun, Feb 24, 2013 at 10:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Antoine Pelisse <apelisse@gmail.com> writes:
>
>> When considering a rename for two files that have a suffix and a prefix
>> that can overlap, a confusing line is shown. As an example, renaming
>> "a/b/b/c" to "a/b/c" shows "a/b/{ => }/b/c".
>
> This would be vastly more readable if it had "It should show XXX
> instead" somewhere in the description, perhaps at the end of this
> sentence.  It can also be after "thus the { => }" below, but I think
> giving the expected output earlier would be more appropriate.

Good catch, this would probably be better:

    When considering a rename for two files that have a suffix and a prefix
    that can overlap, a confusing line is shown. As an example, renaming
    "a/b/b/c" to "a/b/c" shows "a/b/{ => }/b/c", instead of "a/b/{ => b}/c"

>> Currently, what we do is calculate the common prefix ("a/b/"), and the
>> common suffix ("/b/c"), but the same "/b/" is actually counted both in
>> prefix and suffix. Then when calculating the size of the non-common part,
>> we end-up with a negative value which is reset to 0, thus the "{ => }".
>
> In this example, the common prefix would be "a/b/" and the common
> suffix that does not overlap with the prefix part would be "/c", so
> I am imagining that "a/b/{ => b}/c" would be the desired output?

Yes, at least that's what I expected.

> This is a really old thinko (dating back to June 2005).  I'll queue
> the patch on maint-1.7.6 (because 1.7.6.6 is slightly more than one
> year old while 1.7.5.4 is a lot older) to allow distros that issue
> incremental fixes on top of ancient versions of Git to pick up the
> fix if they wanted to.  Perhaps we would want to add a few tests?

I can easily understand why that was missed.
I will try to resubmit with tests very soon.

Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap

[Philip Oakley]

On 25/02/13 19:50, Antoine Pelisse wrote:
> On Sun, Feb 24, 2013 at 10:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Antoine Pelisse <apelisse@gmail.com> writes:
>>
>>> When considering a rename for two files that have a suffix and a prefix
>>> that can overlap, a confusing line is shown. As an example, renaming
>>> "a/b/b/c" to "a/b/c" shows "a/b/{ => }/b/c".
>>
>> This would be vastly more readable if it had "It should show XXX
>> instead" somewhere in the description, perhaps at the end of this
>> sentence.  It can also be after "thus the { => }" below, but I think
>> giving the expected output earlier would be more appropriate.
>
> Good catch, this would probably be better:
>
>      When considering a rename for two files that have a suffix and a prefix
>      that can overlap, a confusing line is shown. As an example, renaming
>      "a/b/b/c" to "a/b/c" shows "a/b/{ => }/b/c", instead of "a/b/{ => b}/c"
>
>>> Currently, what we do is calculate the common prefix ("a/b/"), and the
>>> common suffix ("/b/c"), but the same "/b/" is actually counted both in
>>> prefix and suffix. Then when calculating the size of the non-common part,
>>> we end-up with a negative value which is reset to 0, thus the "{ => }".
>>
>> In this example, the common prefix would be "a/b/" and the common
>> suffix that does not overlap with the prefix part would be "/c", so
>> I am imagining that "a/b/{ => b}/c" would be the desired output?
>
> Yes, at least that's what I expected.

Surely it would be "a/b/{b => }/c", that is, we have reduced the number 
of b's by one. Or am I misunderstanding something?
(I'm guessing it was an all too obvious typo that was misread)

>
>> This is a really old thinko (dating back to June 2005).  I'll queue
>> the patch on maint-1.7.6 (because 1.7.6.6 is slightly more than one
>> year old while 1.7.5.4 is a lot older) to allow distros that issue
>> incremental fixes on top of ancient versions of Git to pick up the
>> fix if they wanted to.  Perhaps we would want to add a few tests?
>
> I can easily understand why that was missed.
> I will try to resubmit with tests very soon.
Philip

Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap

[Antoine Pelisse]

>>> In this example, the common prefix would be "a/b/" and the common
>>> suffix that does not overlap with the prefix part would be "/c", so
>>> I am imagining that "a/b/{ => b}/c" would be the desired output?
>>
>>
>> Yes, at least that's what I expected.
>
>
> Surely it would be "a/b/{b => }/c", that is, we have reduced the number of
> b's by one. Or am I misunderstanding something?
> (I'm guessing it was an all too obvious typo that was misread)

Indeed, read to fast and reproduced in suggested new message.
a/b/b/c => a/b/c is equivalent to a/b/{b => }/c

Thank you for proof-reading.

[PATCH] diff: Fix rename pretty-print when suffix and prefix overlap

[Antoine Pelisse]

When considering a rename for two files that have a suffix and a prefix
that can overlap, a confusing line is shown. As an example, renaming
"a/b/b/c" to "a/b/c" shows "a/b/{ => }/b/c", instead of "a/b/{b => }/c"

Currently, what we do is calculate the common prefix ("a/b/"), and the
common suffix ("/b/c"), but the same "/b/" is actually counted both in
prefix and suffix. Then when calculating the size of the non-common part,
we end-up with a negative value which is reset to 0, thus the "{ => }".

Do not allow the common suffix to overlap the common prefix and stop
when reaching a "/" that would be in both.

Also add some test file to place corner-cases we could met (and this one)
with rename pretty print.

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 diff.c                   |   11 +++++++++-
 t/t4056-rename-pretty.sh |   54 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100755 t/t4056-rename-pretty.sh

diff --git a/diff.c b/diff.c
index 9038f19..e1d82c9 100644
--- a/diff.c
+++ b/diff.c
@@ -1177,7 +1177,16 @@ static char *pprint_rename(const char *a, const char *b)
 	old = a + len_a;
 	new = b + len_b;
 	sfx_length = 0;
-	while (a <= old && b <= new && *old == *new) {
+	/*
+	 * Note:
+	 * if pfx_length is 0, old/new will never reach a - 1 because it
+	 * would mean the whole string is common suffix. But then, the
+	 * whole string would also be a common prefix, and we would not
+	 * have pfx_length equals 0.
+	 */
+	while (a + pfx_length - 1 <= old &&
+	       b + pfx_length - 1 <= new &&
+	       *old == *new) {
 		if (*old == '/')
 			sfx_length = len_a - (old - a);
 		old--;
diff --git a/t/t4056-rename-pretty.sh b/t/t4056-rename-pretty.sh
new file mode 100755
index 0000000..806046f
--- /dev/null
+++ b/t/t4056-rename-pretty.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+test_description='Rename pretty print
+
+'
+
+. ./test-lib.sh
+
+test_expect_success nothing_common '
+	mkdir -p a/b/ &&
+	: >a/b/c &&
+	git add a/b/c &&
+	git commit -m. &&
+	mkdir -p c/b/ &&
+	git mv a/b/c c/b/a &&
+	git commit -m. &&
+	git show -M --summary >output &&
+	test_i18ngrep "a/b/c => c/b/a" output
+'
+
+test_expect_success common_prefix '
+	mkdir -p c/d &&
+	git mv c/b/a c/d/e &&
+	git commit -m. &&
+	git show -M --summary >output &&
+	test_i18ngrep "c/{b/a => d/e}" output
+'
+
+test_expect_success common_suffix '
+	mkdir d &&
+	git mv c/d/e d/e &&
+	git commit -m. &&
+	git show -M --summary >output &&
+	test_i18ngrep "{c/d => d}/e" output
+'
+
+test_expect_success common_suffix_prefix '
+	mkdir d/f &&
+	git mv d/e d/f/e &&
+	git commit -m. &&
+	git show -M --summary >output &&
+	test_i18ngrep "d/{ => f}/e" output
+'
+
+test_expect_success common_overlap '
+	mkdir d/f/f &&
+	git mv d/f/e d/f/f/e &&
+	git commit -m. &&
+	git show -M --summary >output &&
+	test_i18ngrep "d/f/{ => f}/e" output
+'
+
+
+test_done
--
1.7.9.5

Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap

[Thomas Rast]

Antoine Pelisse <apelisse@gmail.com> writes:

> diff --git a/diff.c b/diff.c
> index 9038f19..e1d82c9 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1177,7 +1177,16 @@ static char *pprint_rename(const char *a, const char *b)
> -	while (a <= old && b <= new && *old == *new) {
> +	/*
> +	 * Note:
> +	 * if pfx_length is 0, old/new will never reach a - 1 because it
> +	 * would mean the whole string is common suffix. But then, the
> +	 * whole string would also be a common prefix, and we would not
> +	 * have pfx_length equals 0.
> +	 */
> +	while (a + pfx_length - 1 <= old &&
> +	       b + pfx_length - 1 <= new &&
> +	       *old == *new) {

Umm, you still have the broken version here, and the previous patch is
already in next.  I think you should decide for one thing ;-)

Either: consider this a reroll; Junio would have to revert the version
already in next (which isn't _so_ bad, because next will eventually be
rebuilt) and apply this new version.  But if you do that, you should
squash my change that deals with the underrun issue (I'd be fine with
that).

Or: consider it an incremental improvement on the series, in which case
you should send only the tests with a new commit message.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap

[Antoine Pelisse]

On Thu, Feb 28, 2013 at 11:14 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Antoine Pelisse <apelisse@gmail.com> writes:
>
>> diff --git a/diff.c b/diff.c
>> index 9038f19..e1d82c9 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -1177,7 +1177,16 @@ static char *pprint_rename(const char *a, const char *b)
>> -     while (a <= old && b <= new && *old == *new) {
>> +     /*
>> +      * Note:
>> +      * if pfx_length is 0, old/new will never reach a - 1 because it
>> +      * would mean the whole string is common suffix. But then, the
>> +      * whole string would also be a common prefix, and we would not
>> +      * have pfx_length equals 0.
>> +      */
>> +     while (a + pfx_length - 1 <= old &&
>> +            b + pfx_length - 1 <= new &&
>> +            *old == *new) {
>
> Umm, you still have the broken version here, and the previous patch is
> already in next.  I think you should decide for one thing ;-)

Thanks ! I had not paid enough attention to that.

> Either: consider this a reroll; Junio would have to revert the version
> already in next (which isn't _so_ bad, because next will eventually be
> rebuilt) and apply this new version.  But if you do that, you should
> squash my change that deals with the underrun issue (I'd be fine with
> that).

I would not have done that without your consent (that's why I kept the
buggy version)

> Or: consider it an incremental improvement on the series, in which case
> you should send only the tests with a new commit message.

That seems like the best solution to me. I will resend later with just
the tests and a new commit message.

Cheers,
Antoine

Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> When considering a rename for two files that have a suffix and a prefix
> that can overlap, a confusing line is shown. As an example, renaming
> "a/b/b/c" to "a/b/c" shows "a/b/{ => }/b/c", instead of "a/b/{b => }/c"
>
> Currently, what we do is calculate the common prefix ("a/b/"), and the
> common suffix ("/b/c"), but the same "/b/" is actually counted both in
> prefix and suffix. Then when calculating the size of the non-common part,
> we end-up with a negative value which is reset to 0, thus the "{ => }".
>
> Do not allow the common suffix to overlap the common prefix and stop
> when reaching a "/" that would be in both.
>
> Also add some test file to place corner-cases we could met (and this one)
> with rename pretty print.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---

Hmm, haven't we already applied this with a fix from Thomas Rast and
queued the result to 'next'?

[PATCH] tests: make sure rename pretty print works

[Antoine Pelisse]

Add basic use cases and corner cases tests for
"git diff -M --summary/stat".

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 t/t4056-rename-pretty.sh |   54 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100755 t/t4056-rename-pretty.sh

diff --git a/t/t4056-rename-pretty.sh b/t/t4056-rename-pretty.sh
new file mode 100755
index 0000000..806046f
--- /dev/null
+++ b/t/t4056-rename-pretty.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+test_description='Rename pretty print
+
+'
+
+. ./test-lib.sh
+
+test_expect_success nothing_common '
+	mkdir -p a/b/ &&
+	: >a/b/c &&
+	git add a/b/c &&
+	git commit -m. &&
+	mkdir -p c/b/ &&
+	git mv a/b/c c/b/a &&
+	git commit -m. &&
+	git show -M --summary >output &&
+	test_i18ngrep "a/b/c => c/b/a" output
+'
+
+test_expect_success common_prefix '
+	mkdir -p c/d &&
+	git mv c/b/a c/d/e &&
+	git commit -m. &&
+	git show -M --summary >output &&
+	test_i18ngrep "c/{b/a => d/e}" output
+'
+
+test_expect_success common_suffix '
+	mkdir d &&
+	git mv c/d/e d/e &&
+	git commit -m. &&
+	git show -M --summary >output &&
+	test_i18ngrep "{c/d => d}/e" output
+'
+
+test_expect_success common_suffix_prefix '
+	mkdir d/f &&
+	git mv d/e d/f/e &&
+	git commit -m. &&
+	git show -M --summary >output &&
+	test_i18ngrep "d/{ => f}/e" output
+'
+
+test_expect_success common_overlap '
+	mkdir d/f/f &&
+	git mv d/f/e d/f/f/e &&
+	git commit -m. &&
+	git show -M --summary >output &&
+	test_i18ngrep "d/f/{ => f}/e" output
+'
+
+
+test_done
-- 
1.7.9.5

Re: [PATCH] tests: make sure rename pretty print works

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> Add basic use cases and corner cases tests for
> "git diff -M --summary/stat".
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
>  t/t4056-rename-pretty.sh |   54 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100755 t/t4056-rename-pretty.sh

I wonder if this needs a new test script of its own.

If we anticipate future additions, it would make sense but otherwise
it may be better if we can find an existing one these tests can be
folded into.

> diff --git a/t/t4056-rename-pretty.sh b/t/t4056-rename-pretty.sh
> new file mode 100755
> index 0000000..806046f
> --- /dev/null
> +++ b/t/t4056-rename-pretty.sh
> @@ -0,0 +1,54 @@
> +#!/bin/sh
> +
> +test_description='Rename pretty print
> +
> +'

A single line would be sufficient...

> +test_expect_success common_prefix '
> +	mkdir -p c/d &&
> +	git mv c/b/a c/d/e &&
> +	git commit -m. &&
> +	git show -M --summary >output &&

I guess the unsightly "commit -m." is an attempt to prevent the
later grep from matching log message randomly, but if you test the
output from "git diff -M --stat/summary HEAD^ HEAD" you do not have
to worry about it, no?

Also I wonder if we can verify the filename part in --stat output.

> +	test_i18ngrep "c/{b/a => d/e}" output

We would want to make sure that we do not have random cruft around
the paths, and the byte before that 'c' and after that '}' may want
to be verified.

[PATCH v2] tests: make sure rename pretty print works

[Antoine Pelisse]

Add basic use cases and corner cases tests for
"git diff -M --summary/stat".

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
list of fixes:
 - Test using diff instead of show
 (that is more consistent with commit message).
 - add extra spaces around paths
 - Use better commit messages
 - Moved to existing t4001

 t/t4001-diff-rename.sh |   54 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 844277c..2f327b7 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -102,4 +102,58 @@ test_expect_success 'setup for many rename source candidates' '
 	grep warning actual.err
 '

+test_expect_success 'rename pretty print with nothing in common' '
+	mkdir -p a/b/ &&
+	: >a/b/c &&
+	git add a/b/c &&
+	git commit -m "create a/b/c" &&
+	mkdir -p c/b/ &&
+	git mv a/b/c c/b/a &&
+	git commit -m "a/b/c -> c/b/a" &&
+	git diff -M --summary HEAD^ HEAD >output &&
+	test_i18ngrep " a/b/c => c/b/a " output &&
+	git diff -M --stat HEAD^ HEAD >output &&
+	test_i18ngrep " a/b/c => c/b/a " output
+'
+
+test_expect_success 'rename pretty print with common prefix' '
+	mkdir -p c/d &&
+	git mv c/b/a c/d/e &&
+	git commit -m "c/b/a -> c/d/e" &&
+	git diff -M --summary HEAD^ HEAD >output &&
+	test_i18ngrep " c/{b/a => d/e} " output &&
+	git diff -M --stat HEAD^ HEAD >output &&
+	test_i18ngrep " c/{b/a => d/e} " output
+'
+
+test_expect_success 'rename pretty print with common suffix' '
+	mkdir d &&
+	git mv c/d/e d/e &&
+	git commit -m "c/d/e -> d/e" &&
+	git diff -M --summary HEAD^ HEAD >output &&
+	test_i18ngrep " {c/d => d}/e " output &&
+	git diff -M --stat HEAD^ HEAD >output &&
+	test_i18ngrep " {c/d => d}/e " output
+'
+
+test_expect_success 'rename pretty print with common prefix and suffix' '
+	mkdir d/f &&
+	git mv d/e d/f/e &&
+	git commit -m "d/e -> d/f/e" &&
+	git diff -M --summary HEAD^ HEAD >output &&
+	test_i18ngrep " d/{ => f}/e " output &&
+	git diff -M --stat HEAD^ HEAD >output &&
+	test_i18ngrep " d/{ => f}/e " output
+'
+
+test_expect_success 'rename pretty print common prefix and suffix overlap' '
+	mkdir d/f/f &&
+	git mv d/f/e d/f/f/e &&
+	git commit -m "d/f/e d/f/f/e" &&
+	git diff -M --summary HEAD^ HEAD >output &&
+	test_i18ngrep " d/f/{ => f}/e " output &&
+	git diff -M --stat HEAD^ HEAD >output &&
+	test_i18ngrep " d/f/{ => f}/e " output
+'
+
 test_done
--
1.7.9.5

Re: [PATCH v2] tests: make sure rename pretty print works

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> Add basic use cases and corner cases tests for
> "git diff -M --summary/stat".
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> list of fixes:
>  - Test using diff instead of show
>  (that is more consistent with commit message).
>  - add extra spaces around paths
>  - Use better commit messages
>  - Moved to existing t4001
>
>  t/t4001-diff-rename.sh |   54 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)

Nice.

Will queue, but we may want to make these tests more independent
from each other later.  For example, if the first test fails before
its "git mv a/b/c c/b/a", the set-up of the second test to further
move it to c/d/e will fail, which is probably not very desirable.

Thanks.

> +test_expect_success 'rename pretty print with nothing in common' '
> +	mkdir -p a/b/ &&
> +	: >a/b/c &&
> +	git add a/b/c &&
> +	git commit -m "create a/b/c" &&
> +	mkdir -p c/b/ &&
> +	git mv a/b/c c/b/a &&
> +	git commit -m "a/b/c -> c/b/a" &&
> +	git diff -M --summary HEAD^ HEAD >output &&
> +	test_i18ngrep " a/b/c => c/b/a " output &&
> +	git diff -M --stat HEAD^ HEAD >output &&
> +	test_i18ngrep " a/b/c => c/b/a " output
> +'
> +
> +test_expect_success 'rename pretty print with common prefix' '
> +	mkdir -p c/d &&
> +	git mv c/b/a c/d/e &&
> +	git commit -m "c/b/a -> c/d/e" &&
> +	git diff -M --summary HEAD^ HEAD >output &&
> +	test_i18ngrep " c/{b/a => d/e} " output &&
> +	git diff -M --stat HEAD^ HEAD >output &&
> +	test_i18ngrep " c/{b/a => d/e} " output
> +'
> +
> +test_expect_success 'rename pretty print with common suffix' '
> +	mkdir d &&
> +	git mv c/d/e d/e &&
> +	git commit -m "c/d/e -> d/e" &&
> +	git diff -M --summary HEAD^ HEAD >output &&
> +	test_i18ngrep " {c/d => d}/e " output &&
> +	git diff -M --stat HEAD^ HEAD >output &&
> +	test_i18ngrep " {c/d => d}/e " output
> +'
> +
> +test_expect_success 'rename pretty print with common prefix and suffix' '
> +	mkdir d/f &&
> +	git mv d/e d/f/e &&
> +	git commit -m "d/e -> d/f/e" &&
> +	git diff -M --summary HEAD^ HEAD >output &&
> +	test_i18ngrep " d/{ => f}/e " output &&
> +	git diff -M --stat HEAD^ HEAD >output &&
> +	test_i18ngrep " d/{ => f}/e " output
> +'
> +
> +test_expect_success 'rename pretty print common prefix and suffix overlap' '
> +	mkdir d/f/f &&
> +	git mv d/f/e d/f/f/e &&
> +	git commit -m "d/f/e d/f/f/e" &&
> +	git diff -M --summary HEAD^ HEAD >output &&
> +	test_i18ngrep " d/f/{ => f}/e " output &&
> +	git diff -M --stat HEAD^ HEAD >output &&
> +	test_i18ngrep " d/f/{ => f}/e " output
> +'
> +
>  test_done
> --
> 1.7.9.5

[PATCH] diff: prevent pprint_rename from underrunning input

[Thomas Rast]

The logic described in d020e27 (diff: Fix rename pretty-print when
suffix and prefix overlap, 2013-02-23) is wrong: The proof in the
comment is valid only if both strings are the same length.  *One* of
old/new can reach a-1 (b-1, resp.) if 'a' is a suffix of 'b' (or vice
versa).

Since the intent was to let the loop run down to the '/' at the end of
the common prefix, fix it by making that distinction explicit: if
there is no prefix, allow no underrun.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Hi Antoine,

Unfortunately there's this bug in your patch.  Luckily it was found by
valgrind on t4016 and others.

Cheers
Thomas

 diff.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index 7d49cab..8396527 100644
--- a/diff.c
+++ b/diff.c
@@ -1264,6 +1264,7 @@ static char *pprint_rename(const char *a, const char *b)
 	const char *new = b;
 	struct strbuf name = STRBUF_INIT;
 	int pfx_length, sfx_length;
+	int pfx_adjust_for_slash;
 	int len_a = strlen(a);
 	int len_b = strlen(b);
 	int a_midlen, b_midlen;
@@ -1291,14 +1292,16 @@ static char *pprint_rename(const char *a, const char *b)
 	new = b + len_b;
 	sfx_length = 0;
 	/*
-	 * Note:
-	 * if pfx_length is 0, old/new will never reach a - 1 because it
-	 * would mean the whole string is common suffix. But then, the
-	 * whole string would also be a common prefix, and we would not
-	 * have pfx_length equals 0.
+	 * If there is a common prefix, it must end in a slash.  In
+	 * that case we let this loop run 1 into the prefix to see the
+	 * same slash.
+	 *
+	 * If there is no common prefix, we cannot do this as it would
+	 * underrun the input strings.
 	 */
-	while (a + pfx_length - 1 <= old &&
-	       b + pfx_length - 1 <= new &&
+	pfx_adjust_for_slash = (pfx_length ? 1 : 0);
+	while (a + pfx_length - pfx_adjust_for_slash <= old &&
+	       b + pfx_length - pfx_adjust_for_slash <= new &&
 	       *old == *new) {
 		if (*old == '/')
 			sfx_length = len_a - (old - a);
-- 
1.8.2.rc1.307.ge0d2dea

Re: [PATCH] diff: prevent pprint_rename from underrunning input

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> The logic described in d020e27 (diff: Fix rename pretty-print when
> suffix and prefix overlap, 2013-02-23) is wrong: The proof in the
> comment is valid only if both strings are the same length.  *One* of
> old/new can reach a-1 (b-1, resp.) if 'a' is a suffix of 'b' (or vice
> versa).

Thanks.  I was also having hard time convincing myself why that -1
does not under-run yesterday.

Will queue.

Re: [PATCH] diff: prevent pprint_rename from underrunning input

[Antoine Pelisse]

>> The logic described in d020e27 (diff: Fix rename pretty-print when
>> suffix and prefix overlap, 2013-02-23) is wrong: The proof in the
>> comment is valid only if both strings are the same length.  *One* of
>> old/new can reach a-1 (b-1, resp.) if 'a' is a suffix of 'b' (or vice
>> versa).

Indeed, sorry about that. I guess I was too focused on my specific
issue and didn't broaden my thoughts.

> Thanks.  I was also having hard time convincing myself why that -1
> does not under-run yesterday.
>
> Will queue.

Thanks