threads / patch / 34573

patch, 11 partst8001/t8002: blame: decompose overly-large test

Subject: [PATCH 01/11] t8001/t8002: blame: decompose overly-large test

## tl;dr

17 messages between Jul 31, 2013 and Aug 5, 2013. Diffs are folded; open one to read it.

replies: 16people: 3as markdown or json

Eric Sunshine· Jul 31, 2013, 08:15 UTC · lore

[PATCH 00/11] blame/log -L: additional tests and bug fixes

While working on multiple -L support for git-blame, I encountered more issues with the existing -L facility in git-blame and git-log. This series fixes these problems and adds a slew of new tests.

Patch 6/11 (t4211: retire soon-to-be unimplementable tests) may be controversial. Removal of these tests was effectively a decision made in isolation since my request for input [1] regarding the issue generated only a single response (from j6t).

This series complements (does not replace) earlier -L-related fixes [2], [3], [4], [5].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/231035/focus=231126 [2]: http://thread.gmane.org/gmane.comp.version-control.git/229917 [3]: http://thread.gmane.org/gmane.comp.version-control.git/230532 [4]: http://git.661346.n2.nabble.com/PATCH-0-6-fix-blame-L-regression-add-tests-tp7592174.html [5]: http://thread.gmane.org/gmane.comp.version-control.git/231035

Eric Sunshine (11):
  t8001/t8002: blame: decompose overly-large test
  t8001/t8002: blame: demonstrate -L bounds checking bug
  t8001/t8002: blame: add empty file & partial-line tests
  blame: fix -L bounds checking bug
  t4211: log: demonstrate -L bounds checking bug
  t4211: retire soon-to-be unimplementable tests
  log: fix -L bounds checking bug
  t8001/t8002: blame: demonstrate acceptance of bogus -LX,+0 and -LX,-0
  blame: reject empty ranges -LX,+0 and -LX,-0
  t8001/t8002: blame: demonstrate acceptance of bogus -L,+0 and -L,-0
  blame: reject empty ranges -L,+0 and -L,-0
 builtin/blame.c     |   4 +-
 line-log.c          |   4 +-
 line-range.c        |   4 +-
 t/annotate-tests.sh | 142 +++++++++++++++++++++++++++++++++++++++++++++++++---
 t/t4211-line-log.sh |  31 +++++++++---
 5 files changed, 166 insertions(+), 19 deletions(-)
-- 
1.8.3.4.1120.gc240c48
Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

Checking all bogus -L syntax forms in a single test makes it difficult to identify the offender when one case fails. Decompose this conglomerate test in order to check each bad syntax case separately.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/annotate-tests.sh | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)
Show changes to t/annotate-tests.sh +25 −7
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 0bfee00..3524eaf 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -275,12 +275,30 @@ test_expect_success 'blame -L :nomatch' '
 	test_must_fail $PROG -L:nomatch hello.c
 '
 
-test_expect_success 'blame -L bogus' '
-	test_must_fail $PROG -L file &&
-	test_must_fail $PROG -L1,+ file &&
-	test_must_fail $PROG -L1,- file &&
-	test_must_fail $PROG -LX file &&
-	test_must_fail $PROG -L1,X file &&
-	test_must_fail $PROG -L1,+N file &&
+test_expect_success 'blame -L' '
+	test_must_fail $PROG -L file
+'
+
+test_expect_success 'blame -L X,+' '
+	test_must_fail $PROG -L1,+ file
+'
+
+test_expect_success 'blame -L X,-' '
+	test_must_fail $PROG -L1,- file
+'
+
+test_expect_success 'blame -L X (non-numeric X)' '
+	test_must_fail $PROG -LX file
+'
+
+test_expect_success 'blame -L X,Y (non-numeric Y)' '
+	test_must_fail $PROG -L1,Y file
+'
+
+test_expect_success 'blame -L X,+N (non-numeric N)' '
+	test_must_fail $PROG -L1,+N file
+'
+
+test_expect_success 'blame -L X,-N (non-numeric N)' '
 	test_must_fail $PROG -L1,-N file
 '

-- 
1.8.3.4.1120.gc240c48
Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

[PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug

A bounds checking bug allows the X in -LX to extend one line past the end of file. For example, given a file with 5 lines, -L6 is accepted as valid. Demonstrate this problem.

While here, also add tests to check that the remaining cases of X and Y in -LX,Y are handled correctly at and in the vicinity of end-of-file.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/annotate-tests.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
Show changes to t/annotate-tests.sh +22 −0
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 3524eaf..02fbbf1 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' '
 	check_count -L/99/,-3 B 1 B2 1 D 1
 '
 
+# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than
+# git-blame sees, hence the last line is actually $(wc...)+1.
+test_expect_success 'blame -L X (X == nlines)' '
+	n=$(expr $(wc -l <file) + 1) &&
+	check_count -L$n C 1
+'
+
+test_expect_failure 'blame -L X (X == nlines + 1)' '
+	n=$(expr $(wc -l <file) + 2) &&
+	test_must_fail $PROG -L$n file
+'
+
 test_expect_success 'blame -L X (X > nlines)' '
 	test_must_fail $PROG -L12345 file
 '
 
+test_expect_success 'blame -L ,Y (Y == nlines)' '
+	n=$(expr $(wc -l <file) + 1) &&
+	check_count -L,$n A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1 E 1
+'
+
+test_expect_success 'blame -L ,Y (Y == nlines + 1)' '
+	n=$(expr $(wc -l <file) + 2) &&
+	test_must_fail $PROG -L,$n file
+'
+
 test_expect_success 'blame -L ,Y (Y > nlines)' '
 	test_must_fail $PROG -L,12345 file
 '

-- 
1.8.3.4.1120.gc240c48
Junio C Hamano· Aug 5, 2013, 19:29 UTC · re: Eric Sunshine · lore

Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 45 quoted lines
> A bounds checking bug allows the X in -LX to extend one line past the
> end of file. For example, given a file with 5 lines, -L6 is accepted as
> valid. Demonstrate this problem.
>
> While here, also add tests to check that the remaining cases of X and Y
> in -LX,Y are handled correctly at and in the vicinity of end-of-file.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/annotate-tests.sh | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
> index 3524eaf..02fbbf1 100644
> --- a/t/annotate-tests.sh
> +++ b/t/annotate-tests.sh
> @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' '
>  	check_count -L/99/,-3 B 1 B2 1 D 1
>  '
>  
> +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than
> +# git-blame sees, hence the last line is actually $(wc...)+1.
> +test_expect_success 'blame -L X (X == nlines)' '
> +	n=$(expr $(wc -l <file) + 1) &&
> +	check_count -L$n C 1
> +'
> +
> +test_expect_failure 'blame -L X (X == nlines + 1)' '
> +	n=$(expr $(wc -l <file) + 2) &&
> +	test_must_fail $PROG -L$n file
> +'
> +
>  test_expect_success 'blame -L X (X > nlines)' '
>  	test_must_fail $PROG -L12345 file
>  '
> +test_expect_success 'blame -L ,Y (Y == nlines)' '
> +	n=$(expr $(wc -l <file) + 1) &&
> +	check_count -L,$n A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1 E 1
> +'
> +
> +test_expect_success 'blame -L ,Y (Y == nlines + 1)' '
> +	n=$(expr $(wc -l <file) + 2) &&
> +	test_must_fail $PROG -L,$n file
> +'
> +
This is somewhat curious.

Does the problem triggers only on a file that ends with an incomplete line, or the test was inserted at this location only because it was convenient and the problem exists with or without the incomplete final line?

I am guessing that it is the latter.
Thanks.
>  test_expect_success 'blame -L ,Y (Y > nlines)' '
>  	test_must_fail $PROG -L,12345 file
>  '
Eric Sunshine· Aug 5, 2013, 19:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug

On Mon, Aug 5, 2013 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 56 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> A bounds checking bug allows the X in -LX to extend one line past the
>> end of file. For example, given a file with 5 lines, -L6 is accepted as
>> valid. Demonstrate this problem.
>>
>> While here, also add tests to check that the remaining cases of X and Y
>> in -LX,Y are handled correctly at and in the vicinity of end-of-file.
>>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>> ---
>>  t/annotate-tests.sh | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
>> index 3524eaf..02fbbf1 100644
>> --- a/t/annotate-tests.sh
>> +++ b/t/annotate-tests.sh
>> @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' '
>>       check_count -L/99/,-3 B 1 B2 1 D 1
>>  '
>>
>> +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than
>> +# git-blame sees, hence the last line is actually $(wc...)+1.
>> +test_expect_success 'blame -L X (X == nlines)' '
>> +     n=$(expr $(wc -l <file) + 1) &&
>> +     check_count -L$n C 1
>> +'
>> +
>> +test_expect_failure 'blame -L X (X == nlines + 1)' '
>> +     n=$(expr $(wc -l <file) + 2) &&
>> +     test_must_fail $PROG -L$n file
>> +'
>> +
>>  test_expect_success 'blame -L X (X > nlines)' '
>>       test_must_fail $PROG -L12345 file
>>  '
>> +test_expect_success 'blame -L ,Y (Y == nlines)' '
>> +     n=$(expr $(wc -l <file) + 1) &&
>> +     check_count -L,$n A 1 B 1 B1 1 B2 1 "A U Thor" 1 C 1 D 1 E 1
>> +'
>> +
>> +test_expect_success 'blame -L ,Y (Y == nlines + 1)' '
>> +     n=$(expr $(wc -l <file) + 2) &&
>> +     test_must_fail $PROG -L,$n file
>> +'
>> +
>
> This is somewhat curious.
>
> Does the problem triggers only on a file that ends with an
> incomplete line, or the test was inserted at this location only
> because it was convenient and the problem exists with or without the
> incomplete final line?
>
> I am guessing that it is the latter.

The problem exists with and without the incomplete line. The comment mentioning "incomplete line" and "wc" was inserted to explain why it was necessary to add one to the result of wc, which might not otherwise be obvious.

The tests were inserted at this location because they are semantically related to the "blame -L ,Y (Y > nlines)" test which already exists in the file (quote just below this response).

>>  test_expect_success 'blame -L ,Y (Y > nlines)' '
>>       test_must_fail $PROG -L,12345 file
>>  '
Eric Sunshine· Aug 5, 2013, 19:45 UTC · re: Eric Sunshine · lore

Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug

On Mon, Aug 5, 2013 at 3:35 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 42 quoted lines
> On Mon, Aug 5, 2013 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> A bounds checking bug allows the X in -LX to extend one line past the
>>> end of file. For example, given a file with 5 lines, -L6 is accepted as
>>> valid. Demonstrate this problem.
>>>
>>> While here, also add tests to check that the remaining cases of X and Y
>>> in -LX,Y are handled correctly at and in the vicinity of end-of-file.
>>>
>>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>>> ---
>>>  t/annotate-tests.sh | 22 ++++++++++++++++++++++
>>>  1 file changed, 22 insertions(+)
>>>
>>> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
>>> index 3524eaf..02fbbf1 100644
>>> --- a/t/annotate-tests.sh
>>> +++ b/t/annotate-tests.sh
>>> @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' '
>>>       check_count -L/99/,-3 B 1 B2 1 D 1
>>>  '
>>>
>>> +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than
>>> +# git-blame sees, hence the last line is actually $(wc...)+1.
>>> +test_expect_success 'blame -L X (X == nlines)' '
>>> +     n=$(expr $(wc -l <file) + 1) &&
>>> +     check_count -L$n C 1
>>
>> This is somewhat curious.
>>
>> Does the problem triggers only on a file that ends with an
>> incomplete line, or the test was inserted at this location only
>> because it was convenient and the problem exists with or without the
>> incomplete final line?
>>
>> I am guessing that it is the latter.
>
> The problem exists with and without the incomplete line. The comment
> mentioning "incomplete line" and "wc" was inserted to explain why it
> was necessary to add one to the result of wc, which might not
> otherwise be obvious.
Would the comment be clearer if phrased like this?
  # We want to test -LX where X is the last line of the file, so we need
  # to compute the number of lines in the file, which normally would be
  # done via 'wc -l'.  In this case, however, the last line of 'file' is
  # incomplete, so 'wc' reports one fewer than the actual line count. To
  # adjust for this anomaly, we must add one to the result of 'wc'.
Junio C Hamano· Aug 5, 2013, 23:30 UTC · re: Eric Sunshine · lore

Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 12 quoted lines
>> The problem exists with and without the incomplete line. The comment
>> mentioning "incomplete line" and "wc" was inserted to explain why it
>> was necessary to add one to the result of wc, which might not
>> otherwise be obvious.
>
> Would the comment be clearer if phrased like this?
>
>   # We want to test -LX where X is the last line of the file, so we need
>   # to compute the number of lines in the file, which normally would be
>   # done via 'wc -l'.  In this case, however, the last line of 'file' is
>   # incomplete, so 'wc' reports one fewer than the actual line count. To
>   # adjust for this anomaly, we must add one to the result of 'wc'.

If the patch picked a place where the test vector does not have to involve a file that ends with an incomplete line, the test would not have had to do anything tricky that required such a comment to explain why it is doing something mysterious.

Such a change would be much cleaner and may be worth the effort, but I do not think just rewording the comment is worth the bother.

I didn't see if there is such a place in the existing test sequence, though.

Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

[PATCH 03/11] t8001/t8002: blame: add empty file & partial-line tests

Add boundary case tests, with and without -L, for empty file; file with one partial line; file with one full line.

The empty file test without -L is of particular interest. Historically, this case has been supported (empty blame output) and this test protects against regression by a subsequent patch fixing an off-by-one bug which incorrectly accepts -LX where X is one past end-of-file.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/annotate-tests.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
Show changes to t/annotate-tests.sh +72 −0
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 02fbbf1..f67332c 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -297,6 +297,78 @@ test_expect_success 'blame -L :nomatch' '
 	test_must_fail $PROG -L:nomatch hello.c
 '
 
+test_expect_success 'setup incremental' '
+	(
+	GIT_AUTHOR_NAME=I &&
+	export GIT_AUTHOR_NAME &&
+	GIT_AUTHOR_EMAIL=I@test.git &&
+	export GIT_AUTHOR_EMAIL &&
+	>incremental &&
+	git add incremental &&
+	git commit -m "step 0" &&
+	printf "partial" >>incremental &&
+	git commit -a -m "step 0.5" &&
+	echo >>incremental &&
+	git commit -a -m "step 1"
+	)
+'
+
+test_expect_success 'blame empty' '
+	check_count -h HEAD^^ -f incremental
+'
+
+test_expect_success 'blame -L 0 empty (undocumented)' '
+	check_count -h HEAD^^ -f incremental -L0
+'
+
+test_expect_failure 'blame -L 1 empty' '
+	test_must_fail $PROG -L1 incremental HEAD^^
+'
+
+test_expect_success 'blame -L 2 empty' '
+	test_must_fail $PROG -L2 incremental HEAD^^
+'
+
+test_expect_success 'blame half' '
+	check_count -h HEAD^ -f incremental I 1
+'
+
+test_expect_success 'blame -L 0 half (undocumented)' '
+	check_count -h HEAD^ -f incremental -L0 I 1
+'
+
+test_expect_success 'blame -L 1 half' '
+	check_count -h HEAD^ -f incremental -L1 I 1
+'
+
+test_expect_failure 'blame -L 2 half' '
+	test_must_fail $PROG -L2 incremental HEAD^
+'
+
+test_expect_success 'blame -L 3 half' '
+	test_must_fail $PROG -L3 incremental HEAD^
+'
+
+test_expect_success 'blame full' '
+	check_count -f incremental I 1
+'
+
+test_expect_success 'blame -L 0 full (undocumented)' '
+	check_count -f incremental -L0 I 1
+'
+
+test_expect_success 'blame -L 1 full' '
+	check_count -f incremental -L1 I 1
+'
+
+test_expect_failure 'blame -L 2 full' '
+	test_must_fail $PROG -L2 incremental
+'
+
+test_expect_success 'blame -L 3 full' '
+	test_must_fail $PROG -L3 incremental
+'
+
 test_expect_success 'blame -L' '
 	test_must_fail $PROG -L file
 '

-- 
1.8.3.4.1120.gc240c48
Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

[PATCH 04/11] blame: fix -L bounds checking bug

Since inception, -LX,Y has correctly reported an out-of-range error when Y is beyond end of file, however, X was not checked, and an out-of-range X would cause a crash. 92f9e273 (blame: prevent a segv when -L given start > EOF; 2010-02-08) attempted to rectify this shortcoming but has its own off-by-one error which allows X to extend one line past end of file. For example, given a file with 5 lines:

  git blame -L5 foo  # OK, blames line 5
  git blame -L6 foo  # accepted, no error, no output, huh?
  git blame -L7 foo  # error "fatal: file foo has only 5 lines"
Fix this bug.

In order to avoid regressing "blame foo" when foo is an empty file, the fix is slightly more complicated than changing '<' to '<='.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/blame.c     | 4 ++--
 t/annotate-tests.sh | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)
Show changes to 2 files +6 −6

builtin/blame.c, t/annotate-tests.sh

diff --git a/builtin/blame.c b/builtin/blame.c
index 079dcd3..e70b089 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2495,13 +2495,13 @@ parse_done:
 	bottom = top = 0;
 	if (bottomtop)
 		prepare_blame_range(&sb, bottomtop, lno, &bottom, &top);
+	if (lno < top || ((lno || bottom) && lno < bottom))
+		die("file %s has only %lu lines", path, lno);
 	if (bottom < 1)
 		bottom = 1;
 	if (top < 1)
 		top = lno;
 	bottom--;
-	if (lno < top || lno < bottom)
-		die("file %s has only %lu lines", path, lno);
 
 	ent = xcalloc(1, sizeof(*ent));
 	ent->lno = bottom;
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index f67332c..f117ef0 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -232,7 +232,7 @@ test_expect_success 'blame -L X (X == nlines)' '
 	check_count -L$n C 1
 '
 
-test_expect_failure 'blame -L X (X == nlines + 1)' '
+test_expect_success 'blame -L X (X == nlines + 1)' '
 	n=$(expr $(wc -l <file) + 2) &&
 	test_must_fail $PROG -L$n file
 '
@@ -321,7 +321,7 @@ test_expect_success 'blame -L 0 empty (undocumented)' '
 	check_count -h HEAD^^ -f incremental -L0
 '
 
-test_expect_failure 'blame -L 1 empty' '
+test_expect_success 'blame -L 1 empty' '
 	test_must_fail $PROG -L1 incremental HEAD^^
 '
 
@@ -341,7 +341,7 @@ test_expect_success 'blame -L 1 half' '
 	check_count -h HEAD^ -f incremental -L1 I 1
 '
 
-test_expect_failure 'blame -L 2 half' '
+test_expect_success 'blame -L 2 half' '
 	test_must_fail $PROG -L2 incremental HEAD^
 '
 
@@ -361,7 +361,7 @@ test_expect_success 'blame -L 1 full' '
 	check_count -f incremental -L1 I 1
 '
 
-test_expect_failure 'blame -L 2 full' '
+test_expect_success 'blame -L 2 full' '
 	test_must_fail $PROG -L2 incremental
 '
 

-- 
1.8.3.4.1120.gc240c48
Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

[PATCH 05/11] t4211: log: demonstrate -L bounds checking bug

A bounds checking bug allows the X in -LX to extend one line past the end of file. For example, given a file with 5 lines, -L6 is accepted as valid. Demonstrate this problem.

While here, also add tests to check that the remaining cases of X and Y in -LX,Y are handled correctly at and in the vicinity of end-of-file.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4211-line-log.sh | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
Show changes to t/t4211-line-log.sh +30 −0
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 7665d67..f98275c 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -64,6 +64,36 @@ test_bad_opts "-L 1,1000:b.c" "has only.*lines"
 test_bad_opts "-L :b.c" "argument.*not of the form"
 test_bad_opts "-L :foo:b.c" "no match"
 
+test_expect_success '-L X (X == nlines)' '
+	n=$(wc -l <b.c) &&
+	git log -L $n:b.c
+'
+
+test_expect_failure '-L X (X == nlines + 1)' '
+	n=$(expr $(wc -l <b.c) + 1) &&
+	test_must_fail git log -L $n:b.c
+'
+
+test_expect_success '-L X (X == nlines + 2)' '
+	n=$(expr $(wc -l <b.c) + 2) &&
+	test_must_fail git log -L $n:b.c
+'
+
+test_expect_success '-L ,Y (Y == nlines)' '
+	n=$(printf "%d" $(wc -l <b.c)) &&
+	git log -L ,$n:b.c
+'
+
+test_expect_success '-L ,Y (Y == nlines + 1)' '
+	n=$(expr $(wc -l <b.c) + 1) &&
+	test_must_fail git log -L ,$n:b.c
+'
+
+test_expect_success '-L ,Y (Y == nlines + 2)' '
+	n=$(expr $(wc -l <b.c) + 2) &&
+	test_must_fail git log -L ,$n:b.c
+'
+
 # There is a separate bug when an empty -L range is the first -L encountered,
 # thus to demonstrate this particular bug, the empty -L range must follow a
 # non-empty -L range.

-- 
1.8.3.4.1120.gc240c48
Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

[PATCH 06/11] t4211: retire soon-to-be unimplementable tests

58960978 and 99780b0a added tests which demonstrated bugs (crashes) in range-set and line-log when handed empty ranges specified via "log -LX:file" where X is one greater than the last line of the file. After these tests were added, it was realized that the ability to specify an empty range is a loophole due to a bug in -L bounds checking. That bug is slated to be fixed in a subsequent patch.

Unfortunately, the closure of this loophole makes it impossible to continue checking range-set and line-log behavior with regard to empty ranges since there is no other way to specify empty ranges via the command-line. APIs of both facilities are private (file static) so there likewise is no way to test their behaviors programmatically. Consequently, retire these two tests.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4211-line-log.sh | 13 -------------
 1 file changed, 13 deletions(-)
Show changes to t/t4211-line-log.sh +0 −13
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index f98275c..769ac68 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -94,17 +94,4 @@ test_expect_success '-L ,Y (Y == nlines + 2)' '
 	test_must_fail git log -L ,$n:b.c
 '
 
-# There is a separate bug when an empty -L range is the first -L encountered,
-# thus to demonstrate this particular bug, the empty -L range must follow a
-# non-empty -L range.
-test_expect_success '-L {empty-range} (any -L)' '
-	n=$(expr $(wc -l <b.c) + 1) &&
-	git log -L1,1:b.c -L$n:b.c
-'
-
-test_expect_success '-L {empty-range} (first -L)' '
-	n=$(expr $(wc -l <b.c) + 1) &&
-	git log -L$n:b.c
-'
-
 test_done

-- 
1.8.3.4.1120.gc240c48
Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

[PATCH 07/11] log: fix -L bounds checking bug

When 12da1d1f added -L support to git-log, a broken bounds check was copied from git-blame -L which incorrectly allows -LX to extend one line past end of file without reporting an error. Instead, it generates an empty range. Fix this bug.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 line-log.c          | 4 ++--
 t/t4211-line-log.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 2 files +3 −3

line-log.c, t/t4211-line-log.sh

diff --git a/line-log.c b/line-log.c
index c2d01dc..1c3ac8d 100644
--- a/line-log.c
+++ b/line-log.c
@@ -594,13 +594,13 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)
 				    lines, &begin, &end,
 				    full_name))
 			die("malformed -L argument '%s'", range_part);
+		if (lines < end || ((lines || begin) && lines < begin))
+			die("file %s has only %lu lines", name_part, lines);
 		if (begin < 1)
 			begin = 1;
 		if (end < 1)
 			end = lines;
 		begin--;
-		if (lines < end || lines < begin)
-			die("file %s has only %ld lines", name_part, lines);
 		line_log_data_insert(&ranges, full_name, begin, end);
 
 		free_filespec(spec);
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 769ac68..b01b3dd 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -69,7 +69,7 @@ test_expect_success '-L X (X == nlines)' '
 	git log -L $n:b.c
 '
 
-test_expect_failure '-L X (X == nlines + 1)' '
+test_expect_success '-L X (X == nlines + 1)' '
 	n=$(expr $(wc -l <b.c) + 1) &&
 	test_must_fail git log -L $n:b.c
 '

-- 
1.8.3.4.1120.gc240c48
Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

[PATCH 08/11] t8001/t8002: blame: demonstrate acceptance of bogus -LX,+0 and -LX,-0

Empty ranges -LX,+0 and -LX,-0 are nonsensical in the context of blame yet they are accepted. They should be errors. Demonstrate this shortcoming.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/annotate-tests.sh | 8 ++++++++
 1 file changed, 8 insertions(+)
Show changes to t/annotate-tests.sh +8 −0
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index f117ef0..0f80cba 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -185,6 +185,10 @@ test_expect_success 'blame -L Y,X (undocumented)' '
 	check_count -L6,3 B 1 B1 1 B2 1 D 1
 '
 
+test_expect_failure 'blame -L X,+0' '
+	test_must_fail $PROG -L1,+0 file
+'
+
 test_expect_success 'blame -L X,+1' '
 	check_count -L3,+1 B2 1
 '
@@ -193,6 +197,10 @@ test_expect_success 'blame -L X,+N' '
 	check_count -L3,+4 B 1 B1 1 B2 1 D 1
 '
 
+test_expect_failure 'blame -L X,-0' '
+	test_must_fail $PROG -L1,-0 file
+'
+
 test_expect_success 'blame -L X,-1' '
 	check_count -L3,-1 B2 1
 '

-- 
1.8.3.4.1120.gc240c48
Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

[PATCH 09/11] blame: reject empty ranges -LX,+0 and -LX,-0

Empty ranges -LX,+0 and -LX,-0 are nonsensical in the context of blame yet they are accepted (in fact, both are interpreted as -LX,+2). Report them as invalid.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 line-range.c        | 2 ++
 t/annotate-tests.sh | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)
Show changes to 2 files +4 −2

line-range.c, t/annotate-tests.sh

diff --git a/line-range.c b/line-range.c
index 3942475..a816951 100644
--- a/line-range.c
+++ b/line-range.c
@@ -26,6 +26,8 @@ static const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
 		if (term != spec + 1) {
 			if (!ret)
 				return term;
+			if (num == 0)
+				die("-L invalid empty range");
 			if (spec[0] == '-')
 				num = 0 - num;
 			if (0 < num)
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 0f80cba..01d50c5 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -185,7 +185,7 @@ test_expect_success 'blame -L Y,X (undocumented)' '
 	check_count -L6,3 B 1 B1 1 B2 1 D 1
 '
 
-test_expect_failure 'blame -L X,+0' '
+test_expect_success 'blame -L X,+0' '
 	test_must_fail $PROG -L1,+0 file
 '
 
@@ -197,7 +197,7 @@ test_expect_success 'blame -L X,+N' '
 	check_count -L3,+4 B 1 B1 1 B2 1 D 1
 '
 
-test_expect_failure 'blame -L X,-0' '
+test_expect_success 'blame -L X,-0' '
 	test_must_fail $PROG -L1,-0 file
 '
 

-- 
1.8.3.4.1120.gc240c48
Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

[PATCH 10/11] t8001/t8002: blame: demonstrate acceptance of bogus -L,+0 and -L,-0

Empty ranges -L,+0 and -L,-0 are nonsensical in the context of blame yet they are accepted. They should be errors. Demonstrate this shortcoming.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/annotate-tests.sh | 8 ++++++++
 1 file changed, 8 insertions(+)
Show changes to t/annotate-tests.sh +8 −0
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 01d50c5..d7807df 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -185,6 +185,10 @@ test_expect_success 'blame -L Y,X (undocumented)' '
 	check_count -L6,3 B 1 B1 1 B2 1 D 1
 '
 
+test_expect_failure 'blame -L ,+0' '
+	test_must_fail $PROG -L,+0 file
+'
+
 test_expect_success 'blame -L X,+0' '
 	test_must_fail $PROG -L1,+0 file
 '
@@ -197,6 +201,10 @@ test_expect_success 'blame -L X,+N' '
 	check_count -L3,+4 B 1 B1 1 B2 1 D 1
 '
 
+test_expect_failure 'blame -L ,-0' '
+	test_must_fail $PROG -L,-0 file
+'
+
 test_expect_success 'blame -L X,-0' '
 	test_must_fail $PROG -L1,-0 file
 '

-- 
1.8.3.4.1120.gc240c48
Eric Sunshine· Jul 31, 2013, 08:15 UTC · re: Eric Sunshine · lore

[PATCH 11/11] blame: reject empty ranges -L,+0 and -L,-0

Empty ranges -L,+0 and -L,-0 are nonsensical in the context of blame yet they are accepted (in fact, both are interpreted as -L1,Y where Y is end-of-file). Report them as invalid.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 line-range.c        | 2 +-
 t/annotate-tests.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 2 files +3 −3

line-range.c, t/annotate-tests.sh

diff --git a/line-range.c b/line-range.c
index a816951..69e8d6b 100644
--- a/line-range.c
+++ b/line-range.c
@@ -21,7 +21,7 @@ static const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
 	 * for 20 lines, or "-L <something>,-5" for 5 lines ending at
 	 * <something>.
 	 */
-	if (1 < begin && (spec[0] == '+' || spec[0] == '-')) {
+	if (1 <= begin && (spec[0] == '+' || spec[0] == '-')) {
 		num = strtol(spec + 1, &term, 10);
 		if (term != spec + 1) {
 			if (!ret)
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index d7807df..e422a9e 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -185,7 +185,7 @@ test_expect_success 'blame -L Y,X (undocumented)' '
 	check_count -L6,3 B 1 B1 1 B2 1 D 1
 '
 
-test_expect_failure 'blame -L ,+0' '
+test_expect_success 'blame -L ,+0' '
 	test_must_fail $PROG -L,+0 file
 '
 
@@ -201,7 +201,7 @@ test_expect_success 'blame -L X,+N' '
 	check_count -L3,+4 B 1 B1 1 B2 1 D 1
 '
 
-test_expect_failure 'blame -L ,-0' '
+test_expect_success 'blame -L ,-0' '
 	test_must_fail $PROG -L,-0 file
 '
 

-- 
1.8.3.4.1120.gc240c48
Thomas Rast· Aug 3, 2013, 14:41 UTC · re: Eric Sunshine · lore

Re: [PATCH 00/11] blame/log -L: additional tests and bug fixes

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 8 quoted lines
> While working on multiple -L support for git-blame, I encountered more
> issues with the existing -L facility in git-blame and git-log. This
> series fixes these problems and adds a slew of new tests.
>
> Patch 6/11 (t4211: retire soon-to-be unimplementable tests) may be
> controversial. Removal of these tests was effectively a decision made in
> isolation since my request for input [1] regarding the issue generated
> only a single response (from j6t).

I agree with that decision. It's better to not leave any user-facing quirks just for internal's sake.

The right thing would be to either expose enough of the range_set api through a test-range-set command, or just write all the tests in C. Both seem a bit excessive since the API doesn't have any users outside of log -L, which probably approaches "reasonable quality" now that you shaked it down quite a bit.

As for the series, my tuits don't go further than a cursory read today, but from that it seems good.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

← back to recent threads