Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 26, 2014, 18:02 UTC
Message-ID: <xmqqzjkcj0s5.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAByyCQBX+xfDdMwFOE2bZg8W2S0jwj2nLV36JwH1N3D4Fn2BUw@mail.gmail.com>

George Papanikolaou <g3orge.app@gmail.com> writes:

Show 6 quoted lines

> On Tue, Mar 25, 2014 at 6:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> As a tangent, I have a suspicion that the current implementation may
>> be wrong at the beginning of the string.  Wouldn't it match " abc"
>> and "abc", even though these two strings shouldn't match?
>
> Wouldn't that be accomplished by just removing the leading whitespace check?

Yes. I was wondering *what* semantics we want in the first place; how to implement what I suggested is so trivial that it goes without saying for the intended audiences of that comment ;-).

> I'm somewhat confused about what the function should match. I haven't
> grasped it.

This function is used when attempting to resurrect a patch that is whitespace-damaged. The patch may want to change a line "a_bc" in the original into something else [*1*], and we may not find "a_bc" in the current source, but there may be "a__bc" (two spaces instead of one the whitespace-damaged patch claims to expect). By ignoring the amount of whitespaces, it forces "git apply" to consider that "a_bc" in the broken patch meant to refer to "a__bc" in reality.

I _think_ the original motivation of ignore_ws_change was to match the "--ignore-space-change" option of "diff", i.e. "ignore changes in the amount of white space". I just checked the source (xdiff/xutils.c) and made sure that "git diff" does not treat the beginning of line any differently hence "_a_bc" and "a_bc" are not considered a match under its --ignore-space-change option.

The current implementation of "apply --ignore-space-change" that ignores leading whitespaces (not "ignore changes in the amount of leading whitespaces") is likely to be a bug from this point of view.

But I wanted to hear opinions from other Git experts [*2*]. Hence my "As a tangent, I have a suspicion".

[Footnote]

*1* This mode is not enabled by default.  I am not even sure if
    anybody sane would (or should) use this option.  Sure, the fuzzy
    match may be able to find the original line that the patch
    author may meant to patch even when it is whiltespace-damaged
    because it does not fully trust what the original lines exactly
    say (i.e. context lines prefixed by " " and old lines prefixed
    by "-").  What makes it sane for us to trust what the
    replacement lines (i.e. new lines prefixed by "+") in such a
    mangled patch says?

*2* For example, somebody may be able to point out that "this is
    meant to match the option of the same name 'diff' has", which is
    my assumption that leads to the above discussion, may not be
    true.