Re: [PATCH] diff: add --ignore-blank-lines option

From: Antoine Pelisse <apelisse@gmail.com>
Date: Jun 12, 2013, 13:21 UTC
Message-ID: <CALWbr2zOkoX9NKuZGOWDHyG12xE8ASz+gCK8qwgM1Xqr+uqN7A@mail.gmail.com>
In-Reply-To: <7vsj0pej5s.fsf@alter.siamese.dyndns.org>

On Mon, Jun 10, 2013 at 11:43 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Antoine Pelisse <apelisse@gmail.com> writes:
>
>> On Sun, Jun 9, 2013 at 10:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> When any ignore blank option is used, there will be lines that
>>> actually has changes (hence should be shown with +/-) but we
>>> deliberately ignore their changes (hence, if they ever appear in the
>>> hunk, they do so as context lines prefixed with SP not +/-).  When
>>> we do so, we show the lines from the postimage in the context.
>>
>> Don't we actually use preimage (see below) ? I think using pre-image
>> allows the patch to be applicable to another tree (but ignoring the
>> space changes).

Answering to myself: OK, my package version of git is 1.7.9.5 while the post-image is used since 1.7.10 or something. That explains my confusion.

> But the result of such patch application is not usually what you
> want to use.  If we use postimage (which by the way was a deliberate
> design decision we made earlier), at least the review of the patch
> is easier because you would see the end result more clearly.

I've found the patch and discussion [1] about that switch from pre-image to post-image, so I can understand the motives (and see that you actually considered problems for applying such a patch). I always felt confident that running "git send-email -w" would send a patch (that can be applied) without the potential space errors/changes I would have added.

I think it's unfortunate that Git does generate patches with git-diff that can't be applied if any space option is used. I'm still not really convinced by the pre-image to post-image change, and maybe I would have made it a non-default option. What is done is done, but I'd rather like not do the same here, if possible.

Show 7 quoted lines

>> If we actually hide new blank lines that are in the context, it means
>> that we won't be able to apply a patch with 2 new blank lines in the 3
>> line context.
>
> Yes, but I do not think the point of --ignore-blank-lines is to
> produce a patch that can be applied in the first place.  It is to
> allow easier eyeballing.

I think it can not be applied because it's *hard* for a computer to actually find the correct location, and it may be equally hard for the reader to evaluate the change with removed/different context.

>> Anyway, I'm starting to think that "show blank lines changes near
>> other changes" makes sense more and more sense.
>
> Probably.

I'm glad to see how convinced you are ;)

I will send my patch and see what makes more sense.

[1]: $gmane/188305