threads / patch / 32986

patchdiff: Fix rename pretty-print when suffix and prefix overlap

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

## tl;dr

16 messages between Feb 23, 2013 and Mar 6, 2013. Diffs are folded; open one to read it.

replies: 15people: 4as markdown or json

Antoine Pelisse· Feb 23, 2013, 16:48 UTC · lore

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(-)

Show changes to diff.c +10 −2

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

Junio C Hamano· Feb 24, 2013, 09:15 UTC · re: Antoine Pelisse · lore

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

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.

Show 33 quoted lines

>
> 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

Antoine Pelisse· Feb 25, 2013, 19:50 UTC · re: Junio C Hamano · lore

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

On Sun, Feb 24, 2013 at 10:15 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> 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"

Show 8 quoted lines

>> 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.

Show 5 quoted lines

> 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 Oakley· Feb 25, 2013, 22:36 UTC · re: Antoine Pelisse · lore

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

On 25/02/13 19:50, Antoine Pelisse wrote:

Show 28 quoted lines

> 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)

Show 9 quoted lines

>
>> 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

Antoine Pelisse· Feb 25, 2013, 22:41 UTC · re: Philip Oakley · lore

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

Show 11 quoted lines

>>> 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.

Antoine Pelisse· Feb 28, 2013, 21:55 UTC · re: Antoine Pelisse · lore

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"

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

Show changes to 2 files +64 −2

diff.c, 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

Thomas Rast· Feb 28, 2013, 22:14 UTC · re: Antoine Pelisse · lore

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

Antoine Pelisse <apelisse@gmail.com> writes:

Show 16 quoted lines

> 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

Antoine Pelisse· Feb 28, 2013, 22:22 UTC · re: Thomas Rast · lore

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

On Thu, Feb 28, 2013 at 11:14 PM, Thomas Rast <trast@student.ethz.ch> wrote:

Show 21 quoted lines

> 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.

Show 5 quoted lines

> 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

Junio C Hamano· Feb 28, 2013, 22:29 UTC · re: Antoine Pelisse · lore

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

Antoine Pelisse <apelisse@gmail.com> writes:

Show 17 quoted lines

> 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'?

Antoine Pelisse· Mar 2, 2013, 14:38 UTC · re: Junio C Hamano · lore

[PATCH] tests: make sure rename pretty print works

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

Show changes to t/t4056-rename-pretty.sh +54 −0

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

Junio C Hamano· Mar 3, 2013, 06:50 UTC · re: Antoine Pelisse · lore

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

Antoine Pelisse <apelisse@gmail.com> writes:

Show 8 quoted lines

> 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.

Show 11 quoted lines

> 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...

Show 5 quoted lines

> +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.

Antoine Pelisse· Mar 6, 2013, 21:36 UTC · re: Antoine Pelisse · lore

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

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(+)

Show changes to t/t4001-diff-rename.sh +54 −1

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

Junio C Hamano· Mar 6, 2013, 22:03 UTC · re: Antoine Pelisse · lore

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

Antoine Pelisse <apelisse@gmail.com> writes:

Show 14 quoted lines

> 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.

Show 57 quoted lines

> +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

Thomas Rast· Feb 26, 2013, 20:47 UTC · re: Antoine Pelisse · lore

[PATCH] diff: prevent pprint_rename from underrunning input

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(-)

Show changes to diff.c +10 −7

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

Junio C Hamano· Feb 26, 2013, 21:44 UTC · re: Thomas Rast · lore

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

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

Show 5 quoted lines

> 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.

Antoine Pelisse· Feb 27, 2013, 13:27 UTC · re: Junio C Hamano · lore

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

Show 5 quoted lines

>> 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