git-diff bug?

Is the following a bug?

    $ printf "aaa\nbbb\nccc\n\n" > 1
    $ printf "aaa\nbbb\n\nccc\n" > 2
    $ git diff --ignore-blank-lines 1 2

This shows a weird output, as if `ccc` was removed and then re-added. Flipping the 1 & 2 names makes it show no difference at all. I tried a bunch of variants, including --minimal, and the four algorithms, and all show the same results. (Similar brokenness happens with an empty line at the beginning on one side and after the first line on the other.)

I'm really not sure that the following is a bug, because I see the same behavior from `diff` (which is what made me try git-diff, hoping that it would be more consistent). (But I can't think of any rational that would make it not a bug.)

Am 02.11.20 um 07:53 schrieb Eli Barzilay:

    $ printf "aaa\nbbb\nccc\n\n" > 1
    $ printf "aaa\nbbb\n\nccc\n" > 2

    $ diff --ignore-blank-lines -u 1 2
    --- 1	2020-11-02 18:11:04.618133008 +0100
    +++ 2	2020-11-02 18:11:04.618133008 +0100
    @@ -1,4 +1,4 @@
     aaa
     bbb
    -ccc

    +ccc
    $ diff --ignore-blank-lines -u 2 1

This matches your results. That the order makes a difference is a bit odd. Both are valid diffs of the inputs and neither one changes blank lines, though, so it doesn't look like a bug.

    $ git diff --ignore-blank-lines 1 2
    $ git diff --ignore-blank-lines 2 1
    $ git --version
    git version 2.29.2

This matches your expectation, but not your results. Which version do you use?

René

On Mon, Nov 2, 2020 at 12:45 PM René Scharfe <l.s.r@web.de> wrote:

Yes, this is what I'm getting, also without a -u.  (Also on 2.29.2)

How is it valid? Isn't the whole point of `--ignore-blank-lines` to do the same thing as comparing a version of the files that drops all empty lines?

$ git diff --ignore-blank-lines 1 2

Eli Barzilay <eli@barzilay.org> writes:

Just this part. Any patch output that correctly explains how the preimage text changed to the postimage text is a "valid" diff, and that is how René used the word. There are multiple "valid" diff to bring the preimage to the postimage:

    (preimage)          (postimage)
    aaa                 aaa
    bbb                 bbb
    ccc
                        ccc

In an extreme case, this diff is even valid.

    @@ -1,4 +1,4 @@
    -aaa
    -bbb
    -ccc
    -
    +aaa
    +bbb
    +
    +ccc

It's just that it is not as _useful_ as other valid diff that explains how the preimage changed to the postimage.

HTH.

On Mon, Nov 2, 2020 at 5:15 PM Junio C Hamano <gitster@pobox.com> wrote:

To be clear, the "valid" in my question is about the correctness of the expected behavior, which (as I disclaimed) is likely a problem of the text that explains these expectations. If validity is only about the correctness of the resulting transform, then it is obviously valid, as well as the other alternatives that you included (and therefore this is not the meaning that I used, otherwise I wouldn't have sent this).

In any case, I think that I now see the problem: the (sparse) explanation says "Ignore changes whose lines are all blank.". It would have been helpful to clarify with "(but blank likes that are *part of* a change are still shown)".

Thanks,

Eli Barzilay <eli@barzilay.org> writes:

I know that, and that is why I clarified that you two are using the same word differently.

Looks sensible ;-)

René Scharfe <l.s.r@web.de> writes:

Interesting. If "diff" happens to pick the line with "ccc" on it as the unchanging pair of lines between the preimage and the postimage, then another "valid diff of the inputs" would look like this:

     aaa
     bbb
    +
     ccc
    -

What such a patch would change consists only of blank lines. It is reasonable to expect "--ignore-blank-lines" would turn it into a no-op, provided if "diff" picks "ccc" as the matching line.

But if "diff" picks that the blank line at the end of the original file as unchanged line, then we'll see the diff quoted in the first part of this message. And that patch does not change any blank lines, so it is unreasonable to expect "--ignore-blank-lines" to turn it into a no-op.

So it all depends on which matching pair "diff" first picks, before the "are all the lines changed by the hunk blank ones?" kicks in.

One could argue that "diff" should work hard to enumerate all the possible combinations (we just saw two possible combinations above) to find one that allows "--ignore-blank-lines" to produce an empty patch, but I am not sure it is a sensible thing to do.