Re: [RFC/PATCH 1/3] apply: Allow blank context lines to match beyond EOF

["Björn Gustavsson" <bgustavsson@gmail.com>]

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

>> @@ -2002,11 +2071,17 @@ static int find_pos(struct image *img,
>>       unsigned long backwards, forwards, try;
>>       int backwards_lno, forwards_lno, try_lno;
>>
>> -     if (preimage->nr > img->nr)
>> -             return -1;
>> +     /*
>> +      * There used to be a quick reject here in case preimage
>> +      * had more lines than img. We must let match_fragment()
>> +      * handle that case because a hunk is now allowed to
>> +      * extend beyond the end of img when --whitespace=fix
>> +      * has been given (and core.whitespace.blanks-at-eof is
>> +      * enabled).
>> +      */
>
> Is it worth to keep the quick-reject if we are not running under
> blank-at-eof mode?

Good point.

As far as I can understand, the quick reject could only make
a difference if there is a huge preimage applied to a big file
and it will only make "git apply" reject the patch faster.

So I created a text file containing one million lines. I deleted
about 60% of the lines and generated a diff.

Applying that diff on the file where the lines had already
been deleted (which would be the same as trying to
apply the patch twice on the original file), "git apply"
without my branch (standard 1.7.0) needed 0.076s
to reject the patch. With my branch (i.e. without the quick
reject), "git apply" rejected the patch in 0.087s.

So unless there is some other real-world use case I haven't
thought of, it does not seem worthwhile to keep
the quick rejection test for performance reasons.

I think I'll factor out the removal of the quick reject
into a separate commit in my next revision of the patch
series, including information from this email in the
commit message.

Another thing is whether the rejection test is actually
needed for correctness reasons. As far I understand
it, is is not not. There is one test that should be changed,
though, to make it clearer why it works. I intend to include
one of the following changes in my next revision of the patch
series.

Either this one:

diff --git a/builtin-apply.c b/builtin-apply.c
index 75c04f0..d58c1ea 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2090,7 +2090,11 @@ static int find_pos(struct image *img,
        else if (match_end)
                line = img->nr - preimage->nr;

-       if (line > img->nr)
+       /*
+        * Because the comparison is unsigned, the following test
+        * will also take care of a negative line number.
+        */
+       if ((size_t) line > img->nr)
                line = img->nr;

        try = 0;

Or this one:

diff --git a/builtin-apply.c b/builtin-apply.c
index 75c04f0..8ca0e32 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2090,7 +2090,9 @@ static int find_pos(struct image *img,
        else if (match_end)
                line = img->nr - preimage->nr;

-       if (line > img->nr)
+       if (line < 0)
+               line = 0;
+       else if (line > img->nr)
                line = img->nr;

        try = 0;

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