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

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

[Eric Sunshine]

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

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

[Eric Sunshine]

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(-)

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

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

[Eric Sunshine]

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
+'
+
 test_expect_success 'blame -L ,Y (Y > nlines)' '
 	test_must_fail $PROG -L,12345 file
 '
-- 
1.8.3.4.1120.gc240c48

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

[Eric Sunshine]

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(+)

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

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

[Eric Sunshine]

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(-)

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

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

[Eric Sunshine]

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(+)

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

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

[Eric Sunshine]

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(-)

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

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

[Eric Sunshine]

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(-)

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

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

[Eric Sunshine]

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(+)

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

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

[Eric Sunshine]

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(-)

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

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

[Eric Sunshine]

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(+)

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

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

[Eric Sunshine]

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(-)

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

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

[Thomas Rast]

Eric Sunshine <sunshine@sunshineco.com> writes:

> 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

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

[Junio C Hamano]

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.

Thanks.

>  test_expect_success 'blame -L ,Y (Y > nlines)' '
>  	test_must_fail $PROG -L,12345 file
>  '

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

[Eric Sunshine]

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
>> +'
>> +
>> +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
>>  '

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

[Eric Sunshine]

On Mon, Aug 5, 2013 at 3:35 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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'.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> 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.