git-diff bug?

git-diff bug?

[Eli Barzilay]

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.)

-- 
                   ((x=>x(x))(x=>x(x)))                  Eli Barzilay:
                   http://barzilay.org/                  Maze is Life!

Re: git-diff bug?

[René Scharfe]

Am 02.11.20 um 07:53 schrieb Eli Barzilay:
> 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.)

    $ 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é

Re: git-diff bug?

[Eli Barzilay]

On Mon, Nov 2, 2020 at 12:45 PM René Scharfe <l.s.r@web.de> wrote:
>
>     $ 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

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


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

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
>     $ 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?

$ git diff --ignore-blank-lines 1 2
diff --git a/1 b/2
index fc13a35..bd05737 100644
--- a/1
+++ b/2
@@ -1,4 +1,4 @@
 aaa
 bbb
-ccc

+ccc
$ git --version
git version 2.29.2


-- 
                   ((x=>x(x))(x=>x(x)))                  Eli Barzilay:
                   http://barzilay.org/                  Maze is Life!

Re: git-diff bug?

[Junio C Hamano]

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

>     $ 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.

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.


>     $ 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é

Re: git-diff bug?

[Junio C Hamano]

Eli Barzilay <eli@barzilay.org> writes:

>> 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.
>
> How is it valid?

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.

Re: git-diff bug?

[Eli Barzilay]

On Mon, Nov 2, 2020 at 5:15 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Eli Barzilay <eli@barzilay.org> writes:
>
> > How is it valid?
>
> 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.

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,
-- 
                   ((x=>x(x))(x=>x(x)))                  Eli Barzilay:
                   http://barzilay.org/                  Maze is Life!

Re: git-diff bug?

[Junio C Hamano]

Eli Barzilay <eli@barzilay.org> writes:

> On Mon, Nov 2, 2020 at 5:15 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Eli Barzilay <eli@barzilay.org> writes:
>>
>> > How is it valid?
>>
>> 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.
>
> To be clear, the "valid" in my question is about the correctness of
> the expected behavior,...

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

> 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)".

Looks sensible ;-)