[PATCH v2 1/4] apply: Remove the quick rejection test

[PATCH v2 1/4] apply: Remove the quick rejection test

[Björn Gustavsson]

In the next commit, we will make it possible for blank context
lines to match beyond of the file. That means that a hunk with
a preimage that has more lines than present in the file may
be possible to successfully apply. Therefore, we must remove
the quick rejection test in find_pos().

find_pos() will already work correctly without the quick
rejection test, but that might not be obvious. Therefore,
comment the test for handling out-of-range line numbers in
find_pos() and cast the "line" variable to the same (unsigned)
type as img->nr.

What are performance implications of removing the quick
rejection test?

It can only help "git apply" to reject a patch faster. For example,
if I have a file with one million lines and a patch that removes
slightly more than 50 percent of the lines and try to apply that
patch twice, the second attempt will fail slightly faster
with the test than without (based on actual measurements).

However, there is the pathological case of a patch with many
more context lines than the default three, and applying that patch
using "git apply -C1". Without the rejection test, the running
time will be roughly proportional to the number of context lines
times the size of the file. That could be handled by writing
a more complicated rejection test (it would have to count the
number of blanks at the end of the preimage), but I don't find
that worth doing until there is a real-world use case that
would benfit from it.

It would be possible to keep the quick rejection test if
--whitespace=fix is not given, but I don't like that from
a testing point of view.

Signed-off-by: Björn Gustavsson <bgustavsson@gmail.com>
---
 builtin-apply.c           |   11 ++++++-----
 t/t4104-apply-boundary.sh |    9 +++++++++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 2a1004d..e1f849d 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2002,11 +2002,8 @@ 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;
-
 	/*
-	 * If match_begining or match_end is specified, there is no
+	 * If match_beginning or match_end is specified, there is no
 	 * point starting from a wrong line that will never match and
 	 * wander around and wait for a match at the specified end.
 	 */
@@ -2015,7 +2012,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;
diff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh
index 0e3ce36..1a74248 100755
--- a/t/t4104-apply-boundary.sh
+++ b/t/t4104-apply-boundary.sh
@@ -134,4 +134,13 @@ test_expect_success 'two lines' '
 
 '
 
+test_expect_success 'apply patch with 3 context lines matching at end' '
+	{ echo a; echo b; echo c; echo d; } >file &&
+	git add file &&
+	echo e >>file &&
+	git diff >patch &&
+	>file &&	
+	test_must_fail git apply patch
+'
+
 test_done
-- 
1.7.0