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

[Junio C Hamano <gitster@pobox.com>]

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