Re: [PATCHv3] git apply: option to ignore whitespace differences

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

On Tue, Jul 28, 2009 at 11:29 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> @@ -205,6 +209,9 @@ running `git apply --directory=modules/git-gui`.
>>  Configuration
>>  -------------
>>
>> +apply.ignore-whitespace::
>> +     Set this boolean configuration flag if you want to ignore whitespace
>> +     differences to be ignored by default.
>
> That's a strange sentence.
>
>        When set to true, changes in amount of whitespace are ignored.

Indeed, my sentene was horrible.

> I am not happy with the name --ignore-whitespace.
>
> Perhaps --ignore-space-change, to be consistent with a "git diff" option,
> would be more appropriate.  Doing so has an added benefit of leaving the
> door open to add --ignore-all-space option to the patch application side
> later.

On the other hand, --ignore-whitespace matches the option name (and
behavior) of the 'patch' command (just like "git diff"'s matches the
'diff' option name and behavior). Principle of least surprise says
that someone coming to git from raw diff/patch setups would expect
--ignore-whitespace on the patch side.

A possible future expansion to allow ignoring all whitespace
completely would be to allow --ignore-whitespace=all

> If I am reading the patch correctly, use of this option always disables
> checks and corrections of whitespace errors.

Not exactly. Whitespace correction is not attempted to try matching
context lines, but the whitespace correction on the new lines is still
applied.

> I personally feel that
> somebody who is willing to accept a patch that has whitespace breakages
> that needs this option would not care

Indeed, my first patch fell through to the whitespace correction, I
changed it per your suggestion.

> but the documentation should warn
> about it at the minimum.

The code comments does mention that context line whitespace correction
that will be skipped. I'll add a note about it in the man page.

> Needless to say, a lot better option is not to disable the checks and
> corrections at all.  After all, this "ignore space change" option is about
> matching the common context lines and replaced/removed contents before the
> change, and checks and corrections are about added/replaced contents after
> the change.  They should be orthogonal.

They are, for new lines.

Actually, one thing that I've been thinking about doing is to adjust
the new lines to match the kind of whitespace change the context line
underwent. Example: if all the context lines had the change 4 spaces
-> tab, it would be nice to have the new lines undergo the same
change. However, this is going to be rather hard to implement.

-- 
Giuseppe "Oblomov" Bilotta