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

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 <redacted>
---
 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 [off-list ref] 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.

On Sun, Feb 24, 2013 at 10:15 AM, Junio C Hamano [off-list ref] wrote:

Antoine Pelisse [off-list ref] writes:

quoted

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"

quoted

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.

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

On Sun, Feb 24, 2013 at 10:15 AM, Junio C Hamano [off-list ref] wrote:

quoted

Antoine Pelisse [off-list ref] writes:

quoted

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"

quoted

quoted

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)

quoted

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

quoted

quoted

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.

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

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

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