Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences

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

On Thu, Jul 2, 2009 at 9:45 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> Sorry for repying to myself here, but I'm not convinced again. Or to
>> be more specific: I think this kind of refactoring is totally out of
>> the scope of this patch. So although I agree with you in priciple, if
>> you don't mind I'll keep the first two patches simpler and less
>> invasive. I'll look into the refactoring as a third step.
>
> I am not interested in applying "this adds a broken ignore-whitespace
> option, but as long as you do not use it, it does not hurt the carefully
> crafted apply-with-context-whose-ws-breakage-was-fixed codepath."

Perfectly agreed. I'm not asking you to apply the patches as they are,
I sent them to the list for review, and CC'ed you since I couldn't
identify a git-apply maintainer.

> For
> example, I am not convinced at all that your patch does not break the
> update_pre_post_images() nor do I know what text the final pre/postimage
> will happen to end up with.  In other words, I do not see a clear logic in
> the change.

As you yourself pointed out in the previous email, when ignoring
whitespace the code wouldn't hit the whitespace fix path at all.
Indeed, I've updated the patch to this effect right after reading your
previous email.

> Also about the broken "only prefix matches" loop, I do not understand why
> you would want to consider this preimage from the patch
>
>        "this  has extra whitespace and other stuff\n"
>
> matches the target line
>
>        "this has extra whitespace\n"
>
> only because the prefix matches.

That's totally not what I meant. The other way around though, i.e. a preimage of
        "this has extra whitespace  \n"
should match against
        "this  has extra whitespace\nand other stuff\n"

And of course
        "this has extra whitespace  \n"
should fail against
        "this  has extra whitespace and other stuff\n"

Both cases behave correctly (I just extended the test to include this case, btw)

A more interesting question would be if a missing EOL at EOF should be
treated as whitespace difference or not.

> For that matter, I do not think I understand why the leading whitespaces
> of s1 and s2 are skipped only when they both begin with a whitespace,
> either.

Oh, thanks, when first adapting Robert's patch I hadn't considered it
missed this case. I've updated the patch accordingly and added a test
case for it.

> I do not want to be looking at this series until it gets into a bit more
> reviewable shape, which I would expect to take at least a week if you are
> not working on this full-time (and I presume nobody on the git list is).

I do wonder what makes the patch 'unreviewable', since I assume that
doesn't mean just 'does not include the refactoring I requested'.

> Please do not Cc me in the meantime, but please do ask for help from other
> people interested in this topic on the list.

I thought it appropriate to cc you for this specific reply since I was
addressing the doubts you raised, but I will not include you in the
next resend of the series, as per your request. Thanks for your
patience anyway.

-- 
Giuseppe "Oblomov" Bilotta