[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, 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 apeli...@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 000..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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap
Antoine Pelisse apeli...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap
On Thu, Feb 28, 2013 at 11:14 PM, Thomas Rast tr...@student.ethz.ch wrote:
Antoine Pelisse apeli...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap
On Sun, Feb 24, 2013 at 10:15 AM, Junio C Hamano gits...@pobox.com wrote:
Antoine Pelisse apeli...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap
On 25/02/13 19:50, Antoine Pelisse wrote:
On Sun, Feb 24, 2013 at 10:15 AM, Junio C Hamano gits...@pobox.com wrote:
Antoine Pelisse apeli...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap
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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap
Antoine Pelisse apeli...@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 apeli...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[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 apeli...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
