[PATCH] xdiff: trim common tail with -U0 after diff

[PATCH] xdiff: trim common tail with -U0 after diff

[Daniel Hahler]

When -U0 is used, trim_common_tail should be called after `xdl_diff`, so
that `--indent-heuristic` (and other diff processing) works as expected.

It also removes the check for `!(xecfg->flags & XDL_EMIT_FUNCCONTEXT)`
added in e0876bca4, which does not appear to be necessary anymore after
moving the trimming down.

This only adds a test to t4061-diff-indent.sh, but should also have one for
normal (i.e. non-experimental) diff mode probably?!

Ref: https://github.com/tomtom/quickfixsigns_vim/issues/74#issue-237900460
---
 t/t4061-diff-indent.sh | 15 +++++++++++++++
 xdiff-interface.c      |  7 ++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 2affd7a10..df3151393 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -116,6 +116,16 @@ test_expect_success 'prepare' '
 	 4
 	EOF
 
+	cat <<-\EOF >spaces-compacted-U0-expect &&
+	diff --git a/spaces.txt b/spaces.txt
+	--- a/spaces.txt
+	+++ b/spaces.txt
+	@@ -4,0 +5,3 @@ a
+	+b
+	+a
+	+
+	EOF
+
 	tr "_" " " <<-\EOF >functions-expect &&
 	diff --git a/functions.c b/functions.c
 	--- a/functions.c
@@ -184,6 +194,11 @@ test_expect_success 'diff: --indent-heuristic with --histogram' '
 	compare_diff spaces-compacted-expect out-compacted4
 '
 
+test_expect_success 'diff: --indent-heuristic with -U0' '
+	git diff -U0 --indent-heuristic old new -- spaces.txt >out-compacted5 &&
+	compare_diff spaces-compacted-U0-expect out-compacted5
+'
+
 test_expect_success 'diff: ugly functions' '
 	git diff --no-indent-heuristic old new -- functions.c >out &&
 	compare_diff functions-expect out
diff --git a/xdiff-interface.c b/xdiff-interface.c
index d3f78ca2a..a7e0e3583 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -125,16 +125,17 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)
 
 int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *xecb)
 {
+	int ret;
 	mmfile_t a = *mf1;
 	mmfile_t b = *mf2;
 
 	if (mf1->size > MAX_XDIFF_SIZE || mf2->size > MAX_XDIFF_SIZE)
 		return -1;
 
-	if (!xecfg->ctxlen && !(xecfg->flags & XDL_EMIT_FUNCCONTEXT))
+	ret = xdl_diff(&a, &b, xpp, xecfg, xecb);
+	if (ret && !xecfg->ctxlen)
 		trim_common_tail(&a, &b);
-
-	return xdl_diff(&a, &b, xpp, xecfg, xecb);
+	return ret;
 }
 
 int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,
-- 
2.13.1

Re: [PATCH] xdiff: trim common tail with -U0 after diff

[Junio C Hamano]

Daniel Hahler <git@thequod.de> writes:

> When -U0 is used, trim_common_tail should be called after `xdl_diff`, so
> that `--indent-heuristic` (and other diff processing) works as expected.

I thought common-tail trimming was a hack/optimization to avoid
having to pass the whole thing to have xdl_diff() work on it?  What
would we be gaining by unconditionally passing the whole thing first
and then still trimming?

> It also removes the check for `!(xecfg->flags & XDL_EMIT_FUNCCONTEXT)`
> added in e0876bca4, which does not appear to be necessary anymore after
> moving the trimming down.

The code was conditionally disabling the hack/optimization; with it
unconditionally disabled, of course it wouldn't be needed, no?

I could understand if the motivation and the proposed change were
"tail-trimming outlived its usefulness, so remove it altogether", or
"trail-trimming negatively affects the output by robbing useful
information that could be used by indent-heuristic, so disable it
when the heuristic is in use".

But neither is what this patch does, so I am sort of at loss.


> --- a/t/t4061-diff-indent.sh
> +++ b/t/t4061-diff-indent.sh
> @@ -116,6 +116,16 @@ test_expect_success 'prepare' '
>  	 4
>  	EOF
>  
> +	cat <<-\EOF >spaces-compacted-U0-expect &&
> +	diff --git a/spaces.txt b/spaces.txt
> +	--- a/spaces.txt
> +	+++ b/spaces.txt
> +	@@ -4,0 +5,3 @@ a
> +	+b
> +	+a
> +	+
> +	EOF
> +
>  	tr "_" " " <<-\EOF >functions-expect &&
>  	diff --git a/functions.c b/functions.c
>  	--- a/functions.c
> @@ -184,6 +194,11 @@ test_expect_success 'diff: --indent-heuristic with --histogram' '
>  	compare_diff spaces-compacted-expect out-compacted4
>  '
>  
> +test_expect_success 'diff: --indent-heuristic with -U0' '
> +	git diff -U0 --indent-heuristic old new -- spaces.txt >out-compacted5 &&
> +	compare_diff spaces-compacted-U0-expect out-compacted5
> +'
> +
>  test_expect_success 'diff: ugly functions' '
>  	git diff --no-indent-heuristic old new -- functions.c >out &&
>  	compare_diff functions-expect out
> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index d3f78ca2a..a7e0e3583 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -125,16 +125,17 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)
>  
>  int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *xecb)
>  {
> +	int ret;
>  	mmfile_t a = *mf1;
>  	mmfile_t b = *mf2;
>  
>  	if (mf1->size > MAX_XDIFF_SIZE || mf2->size > MAX_XDIFF_SIZE)
>  		return -1;
>  
> -	if (!xecfg->ctxlen && !(xecfg->flags & XDL_EMIT_FUNCCONTEXT))
> +	ret = xdl_diff(&a, &b, xpp, xecfg, xecb);
> +	if (ret && !xecfg->ctxlen)
>  		trim_common_tail(&a, &b);
> -
> -	return xdl_diff(&a, &b, xpp, xecfg, xecb);
> +	return ret;
>  }
>  
>  int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,

Re: [PATCH] xdiff: trim common tail with -U0 after diff

[Stefan Beller]

On Fri, Jun 23, 2017 at 12:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Daniel Hahler <git@thequod.de> writes:
>
>> When -U0 is used, trim_common_tail should be called after `xdl_diff`, so
>> that `--indent-heuristic` (and other diff processing) works as expected.
>
> I thought common-tail trimming was a hack/optimization to avoid
> having to pass the whole thing to have xdl_diff() work on it?  What
> would we be gaining by unconditionally passing the whole thing first
> and then still trimming?

So I tried finding out more about this hack,
and found the patch that introduced the common tail trimming at
  https://public-inbox.org/git/7vmysez0oa.fsf@gitster.siamese.dyndns.org/
  913b45f51b (xdi_diff: trim common trailing lines, 2007-12-13)

In the early days there were much larger email threads apparently.
I haven't seen such a big one in a while. I did not find the email that
the commit message referenced to unfortunately.

AFAICT we have 2 things going on:
* swapping the order of xdl_diff and the tail trimming
* change the condition under which the tail is trimmed
  (as a logical follow up/ cleanup because of the first point)

I do not understand the first one, yet.

Re: [PATCH] xdiff: trim common tail with -U0 after diff

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So I tried finding out more about this hack,
> and found the patch that introduced the common tail trimming at
>   https://public-inbox.org/git/7vmysez0oa.fsf@gitster.siamese.dyndns.org/
>   913b45f51b (xdi_diff: trim common trailing lines, 2007-12-13)

A relevant one that makes me hesitate to take this kind of change is
this:

https://public-inbox.org/git/alpine.LFD.0.9999.0712202009290.21557@woody.linux-foundation.org/#t

that resulted in this change:

commit d2f82950a9226ae1102a7a97f03440a4bf8c6c09
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu Dec 20 20:22:46 2007 -0800

    Re(-re)*fix trim_common_tail()
    
    The tar-ball and the git archive itself is fine, but yes, the diff from
    2.6.23 to 2.6.24-rc6 is bad. It's the "trim_common_tail()" optimization
    that has caused way too much pain.
    
    Very interesting breakage. The patch was actually "correct" in a (rather
    limited) technical sense, but the context at the end was missing because
    while the trim_common_tail() code made sure to keep enough common context
    to allow a valid diff to be generated, the diff machinery itself could
    decide that it could generate the diff differently than the "obvious"
    solution.
    
    Thee sad fact is that the git optimization (which is very important for
    "git blame", which needs no context), is only really valid for that one
    case where we really don't need any context.

Re: [PATCH] xdiff: trim common tail with -U0 after diff

[René Scharfe]

Am 23.06.2017 um 12:36 schrieb Daniel Hahler:
> When -U0 is used, trim_common_tail should be called after `xdl_diff`, so
> that `--indent-heuristic` (and other diff processing) works as expected.
> 
> It also removes the check for `!(xecfg->flags & XDL_EMIT_FUNCCONTEXT)`
> added in e0876bca4, which does not appear to be necessary anymore after
> moving the trimming down.
> 
> This only adds a test to t4061-diff-indent.sh, but should also have one for
> normal (i.e. non-experimental) diff mode probably?!
> 
> Ref: https://github.com/tomtom/quickfixsigns_vim/issues/74#issue-237900460
> ---
>   t/t4061-diff-indent.sh | 15 +++++++++++++++
>   xdiff-interface.c      |  7 ++++---
>   2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
> index 2affd7a10..df3151393 100755
> --- a/t/t4061-diff-indent.sh
> +++ b/t/t4061-diff-indent.sh
> @@ -116,6 +116,16 @@ test_expect_success 'prepare' '
>   	 4
>   	EOF
>   
> +	cat <<-\EOF >spaces-compacted-U0-expect &&
> +	diff --git a/spaces.txt b/spaces.txt
> +	--- a/spaces.txt
> +	+++ b/spaces.txt
> +	@@ -4,0 +5,3 @@ a
> +	+b
> +	+a
> +	+
> +	EOF
> +
>   	tr "_" " " <<-\EOF >functions-expect &&
>   	diff --git a/functions.c b/functions.c
>   	--- a/functions.c
> @@ -184,6 +194,11 @@ test_expect_success 'diff: --indent-heuristic with --histogram' '
>   	compare_diff spaces-compacted-expect out-compacted4
>   '
>   
> +test_expect_success 'diff: --indent-heuristic with -U0' '
> +	git diff -U0 --indent-heuristic old new -- spaces.txt >out-compacted5 &&
> +	compare_diff spaces-compacted-U0-expect out-compacted5
> +'
> +
>   test_expect_success 'diff: ugly functions' '
>   	git diff --no-indent-heuristic old new -- functions.c >out &&
>   	compare_diff functions-expect out

The changed test script passes just fine for me even without your change
to xdiff-interface.c, which is odd.  Do you have another way to
demonstrate the unexpected behavior?  And can someone replicate the
failure?

René

Re: [PATCH] xdiff: trim common tail with -U0 after diff

[Daniel Hahler]

[-- Attachment #1.1: Type: text/plain, Size: 897 bytes --]

On 23.06.2017 22:39, René Scharfe wrote:

> The changed test script passes just fine for me even without your change
> to xdiff-interface.c, which is odd.

Sorry, I've apparently messed this up - it seems to be the case for me, too.

I would assume that using the functions.c context/diff in this test file might prove it, but when I wrote this test I was already unsure to base it on an experimental feature, that might just get removed again (with its tests).

> Do you have another way to demonstrate the unexpected behavior?

It was clearly reproducible with a local test case, based on "copying an existing test case, and modifying the header for it only".

Given the other replies, it seems like it is more a question now if the trimming should get moved down, or removed completely it seems.
So I will not update the patch/test for now.


-- 
http://daniel.hahler.de/


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]