Re: [PATCH v2 2/4] apply: Allow blank context lines to match beyond EOF

From: Björn Gustavsson <bgustavsson@gmail.com>
Date: Feb 24, 2010, 23:02 UTC
Message-ID: <6672d0161002241502h2f80b511j445465fdc2fd16ab@mail.gmail.com>
In-Reply-To: <7vaauyfj3k.fsf@alter.siamese.dyndns.org>

2010/2/24 Junio C Hamano <gitster@pobox.com>:

> Very nicely done.

Thanks! :)

Show 5 quoted lines

> On the other hand "limit" does not have such a good definition, other than
> as a work around to bypass line-number check when we are trying to match
> at the end.  It might be cleaner to read if we move the problematic "line
> numbers must match" logic and eliminate this variable, like the attached
> patch does on top of this one.

Yes, your version is better. Having a "limit" variable no longer makes sense (my original patch used "limit" in two places). Feel free to squeeze it in.

> I couldn't figure out how this would interact with the ignore_ws_change
> codepath, though.  That one shows a clear sign of being bolted on as an
> afterthought (once you fall into that "if()" statement you will not come
> back).

Yes, it does seem bolted on.

I haven't looked much at that if() statement, because I sort of assumed that because of the return it couldn't do any harm.

It is too late in my time zone for me to think clearly, but it does seem that I was wrong and that I'll need to do some changes in that "if()" statement, and also write some more tests for the combination of --whitespace=fix and --ignore-space-change.

I'll be back another day.

Thanks for the review.

-- 
Björn Gustavsson, Erlang/OTP, Ericsson AB