{"thread":{"id":"34573","subject":"[PATCH 01/11] t8001/t8002: blame: decompose overly-large test","startedAt":"2013-07-31T08:15:34Z","lastAt":"2013-08-05T23:30:35Z","messageCount":17,"participants":["Eric Sunshine","Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"224344","messageId":"1375258545-42240-1-git-send-email-sunshine@sunshineco.com","threadId":"34573","inReplyTo":null,"subject":"[PATCH 00/11] blame/log -L: additional tests and bug fixes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-31T08:15:34Z","receivedAt":"2013-07-31T08:15:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"While working on multiple -L support for git-blame, I encountered more\nissues with the existing -L facility in git-blame and git-log. This\nseries fixes these problems and adds a slew of new tests.\n\nPatch 6/11 (t4211: retire soon-to-be unimplementable tests) may be\ncontroversial. Removal of these tests was effectively a decision made in\nisolation since my request for input [1] regarding the issue generated\nonly a single response (from j6t).\n\nThis series complements (does not replace) earlier -L-related fixes [2],\n[3], [4], [5].\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/231035/focus=231126\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/229917\n[3]: http://thread.gmane.org/gmane.comp.version-control.git/230532\n[4]: http://git.661346.n2.nabble.com/PATCH-0-6-fix-blame-L-regression-add-tests-tp7592174.html\n[5]: http://thread.gmane.org/gmane.comp.version-control.git/231035\n\nEric Sunshine (11):\n t8001/t8002: blame: decompose overly-large test\n t8001/t8002: blame: demonstrate -L bounds checking bug\n t8001/t8002: blame: add empty file & partial-line tests\n blame: fix -L bounds checking bug\n t4211: log: demonstrate -L bounds checking bug\n t4211: retire soon-to-be unimplementable tests\n log: fix -L bounds checking bug\n t8001/t8002: blame: demonstrate acceptance of bogus -LX,+0 and -LX,-0\n blame: reject empty ranges -LX,+0 and -LX,-0\n t8001/t8002: blame: demonstrate acceptance of bogus -L,+0 and -L,-0\n blame: reject empty ranges -L,+0 and -L,-0\n\n builtin/blame.c | 4 +-\n line-log.c | 4 +-\n line-range.c | 4 +-\n t/annotate-tests.sh | 142 +++++++++++++++++++++++++++++++++++++++++++++++++---\n t/t4211-line-log.sh | 31 +++++++++---\n 5 files changed, 166 insertions(+), 19 deletions(-)\n\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"224333","messageId":"1375258545-42240-2-git-send-email-sunshine@sunshineco.com","threadId":"34573","inReplyTo":"1375258545-42240-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 01/11] t8001/t8002: blame: decompose overly-large test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-31T08:15:35Z","receivedAt":"2013-07-31T08:15:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Checking all bogus -L syntax forms in a single test makes it difficult\nto identify the offender when one case fails. Decompose this\nconglomerate test in order to check each bad syntax case separately.\n\nSigned-off-by: Eric Sunshine \n---\n t/annotate-tests.sh | 32 +++++++++++++++++++++++++-------\n 1 file changed, 25 insertions(+), 7 deletions(-)\n\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex 0bfee00..3524eaf 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -275,12 +275,30 @@ test_expect_success 'blame -L :nomatch' '\n \ttest_must_fail $PROG -L:nomatch hello.c\n '\n \n-test_expect_success 'blame -L bogus' '\n-\ttest_must_fail $PROG -L file &&\n-\ttest_must_fail $PROG -L1,+ file &&\n-\ttest_must_fail $PROG -L1,- file &&\n-\ttest_must_fail $PROG -LX file &&\n-\ttest_must_fail $PROG -L1,X file &&\n-\ttest_must_fail $PROG -L1,+N file &&\n+test_expect_success 'blame -L' '\n+\ttest_must_fail $PROG -L file\n+'\n+\n+test_expect_success 'blame -L X,+' '\n+\ttest_must_fail $PROG -L1,+ file\n+'\n+\n+test_expect_success 'blame -L X,-' '\n+\ttest_must_fail $PROG -L1,- file\n+'\n+\n+test_expect_success 'blame -L X (non-numeric X)' '\n+\ttest_must_fail $PROG -LX file\n+'\n+\n+test_expect_success 'blame -L X,Y (non-numeric Y)' '\n+\ttest_must_fail $PROG -L1,Y file\n+'\n+\n+test_expect_success 'blame -L X,+N (non-numeric N)' '\n+\ttest_must_fail $PROG -L1,+N file\n+'\n+\n+test_expect_success 'blame -L X,-N (non-numeric N)' '\n \ttest_must_fail $PROG -L1,-N file\n '\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"224334","messageId":"1375258545-42240-3-git-send-email-sunshine@sunshineco.com","threadId":"34573","inReplyTo":"1375258545-42240-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-31T08:15:36Z","receivedAt":"2013-07-31T08:15:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"A bounds checking bug allows the X in -LX to extend one line past the\nend of file. For example, given a file with 5 lines, -L6 is accepted as\nvalid. Demonstrate this problem.\n\nWhile here, also add tests to check that the remaining cases of X and Y\nin -LX,Y are handled correctly at and in the vicinity of end-of-file.\n\nSigned-off-by: Eric Sunshine \n---\n t/annotate-tests.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex 3524eaf..02fbbf1 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' '\n \tcheck_count -L/99/,-3 B 1 B2 1 D 1\n '\n \n+# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than\n+# git-blame sees, hence the last line is actually $(wc...)+1.\n+test_expect_success 'blame -L X (X == nlines)' '\n+\tn=$(expr $(wc -l nlines)' '\n \ttest_must_fail $PROG -L12345 file\n '\n \n+test_expect_success 'blame -L ,Y (Y == nlines)' '\n+\tn=$(expr $(wc -l nlines)' '\n \ttest_must_fail $PROG -L,12345 file\n '\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"224336","messageId":"1375258545-42240-4-git-send-email-sunshine@sunshineco.com","threadId":"34573","inReplyTo":"1375258545-42240-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 03/11] t8001/t8002: blame: add empty file & partial-line tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-31T08:15:37Z","receivedAt":"2013-07-31T08:15:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Add boundary case tests, with and without -L, for empty file; file with\none partial line; file with one full line.\n\nThe empty file test without -L is of particular interest. Historically,\nthis case has been supported (empty blame output) and this test protects\nagainst regression by a subsequent patch fixing an off-by-one bug which\nincorrectly accepts -LX where X is one past end-of-file.\n\nSigned-off-by: Eric Sunshine \n---\n t/annotate-tests.sh | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 72 insertions(+)\n\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex 02fbbf1..f67332c 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -297,6 +297,78 @@ test_expect_success 'blame -L :nomatch' '\n \ttest_must_fail $PROG -L:nomatch hello.c\n '\n \n+test_expect_success 'setup incremental' '\n+\t(\n+\tGIT_AUTHOR_NAME=I &&\n+\texport GIT_AUTHOR_NAME &&\n+\tGIT_AUTHOR_EMAIL=I@test.git &&\n+\texport GIT_AUTHOR_EMAIL &&\n+\t>incremental &&\n+\tgit add incremental &&\n+\tgit commit -m \"step 0\" &&\n+\tprintf \"partial\" >>incremental &&\n+\tgit commit -a -m \"step 0.5\" &&\n+\techo >>incremental &&\n+\tgit commit -a -m \"step 1\"\n+\t)\n+'\n+\n+test_expect_success 'blame empty' '\n+\tcheck_count -h HEAD^^ -f incremental\n+'\n+\n+test_expect_success 'blame -L 0 empty (undocumented)' '\n+\tcheck_count -h HEAD^^ -f incremental -L0\n+'\n+\n+test_expect_failure 'blame -L 1 empty' '\n+\ttest_must_fail $PROG -L1 incremental HEAD^^\n+'\n+\n+test_expect_success 'blame -L 2 empty' '\n+\ttest_must_fail $PROG -L2 incremental HEAD^^\n+'\n+\n+test_expect_success 'blame half' '\n+\tcheck_count -h HEAD^ -f incremental I 1\n+'\n+\n+test_expect_success 'blame -L 0 half (undocumented)' '\n+\tcheck_count -h HEAD^ -f incremental -L0 I 1\n+'\n+\n+test_expect_success 'blame -L 1 half' '\n+\tcheck_count -h HEAD^ -f incremental -L1 I 1\n+'\n+\n+test_expect_failure 'blame -L 2 half' '\n+\ttest_must_fail $PROG -L2 incremental HEAD^\n+'\n+\n+test_expect_success 'blame -L 3 half' '\n+\ttest_must_fail $PROG -L3 incremental HEAD^\n+'\n+\n+test_expect_success 'blame full' '\n+\tcheck_count -f incremental I 1\n+'\n+\n+test_expect_success 'blame -L 0 full (undocumented)' '\n+\tcheck_count -f incremental -L0 I 1\n+'\n+\n+test_expect_success 'blame -L 1 full' '\n+\tcheck_count -f incremental -L1 I 1\n+'\n+\n+test_expect_failure 'blame -L 2 full' '\n+\ttest_must_fail $PROG -L2 incremental\n+'\n+\n+test_expect_success 'blame -L 3 full' '\n+\ttest_must_fail $PROG -L3 incremental\n+'\n+\n test_expect_success 'blame -L' '\n \ttest_must_fail $PROG -L file\n '\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"224335","messageId":"1375258545-42240-5-git-send-email-sunshine@sunshineco.com","threadId":"34573","inReplyTo":"1375258545-42240-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 04/11] blame: fix -L bounds checking bug","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-31T08:15:38Z","receivedAt":"2013-07-31T08:15:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Since inception, -LX,Y has correctly reported an out-of-range error when\nY is beyond end of file, however, X was not checked, and an out-of-range\nX would cause a crash. 92f9e273 (blame: prevent a segv when -L given\nstart > EOF; 2010-02-08) attempted to rectify this shortcoming but has\nits own off-by-one error which allows X to extend one line past end of\nfile. For example, given a file with 5 lines:\n\n git blame -L5 foo # OK, blames line 5\n git blame -L6 foo # accepted, no error, no output, huh?\n git blame -L7 foo # error \"fatal: file foo has only 5 lines\"\n\nFix this bug.\n\nIn order to avoid regressing \"blame foo\" when foo is an empty file, the\nfix is slightly more complicated than changing '<' to '<='.\n\nSigned-off-by: Eric Sunshine \n---\n builtin/blame.c | 4 ++--\n t/annotate-tests.sh | 8 ++++----\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 079dcd3..e70b089 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2495,13 +2495,13 @@ parse_done:\n \tbottom = top = 0;\n \tif (bottomtop)\n \t\tprepare_blame_range(&sb, bottomtop, lno, &bottom, &top);\n+\tif (lno < top || ((lno || bottom) && lno < bottom))\n+\t\tdie(\"file %s has only %lu lines\", path, lno);\n \tif (bottom < 1)\n \t\tbottom = 1;\n \tif (top < 1)\n \t\ttop = lno;\n \tbottom--;\n-\tif (lno < top || lno < bottom)\n-\t\tdie(\"file %s has only %lu lines\", path, lno);\n \n \tent = xcalloc(1, sizeof(*ent));\n \tent->lno = bottom;\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex f67332c..f117ef0 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -232,7 +232,7 @@ test_expect_success 'blame -L X (X == nlines)' '\n \tcheck_count -L$n C 1\n '\n \n-test_expect_failure 'blame -L X (X == nlines + 1)' '\n+test_expect_success 'blame -L X (X == nlines + 1)' '\n \tn=$(expr $(wc -l \n---\n t/t4211-line-log.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 7665d67..f98275c 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -64,6 +64,36 @@ test_bad_opts \"-L 1,1000:b.c\" \"has only.*lines\"\n test_bad_opts \"-L :b.c\" \"argument.*not of the form\"\n test_bad_opts \"-L :foo:b.c\" \"no match\"\n \n+test_expect_success '-L X (X == nlines)' '\n+\tn=$(wc -l \n---\n t/t4211-line-log.sh | 13 -------------\n 1 file changed, 13 deletions(-)\n\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex f98275c..769ac68 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -94,17 +94,4 @@ test_expect_success '-L ,Y (Y == nlines + 2)' '\n \ttest_must_fail git log -L ,$n:b.c\n '\n \n-# There is a separate bug when an empty -L range is the first -L encountered,\n-# thus to demonstrate this particular bug, the empty -L range must follow a\n-# non-empty -L range.\n-test_expect_success '-L {empty-range} (any -L)' '\n-\tn=$(expr $(wc -l \n---\n line-log.c | 4 ++--\n t/t4211-line-log.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex c2d01dc..1c3ac8d 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -594,13 +594,13 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)\n \t\t\t\t lines, &begin, &end,\n \t\t\t\t full_name))\n \t\t\tdie(\"malformed -L argument '%s'\", range_part);\n+\t\tif (lines < end || ((lines || begin) && lines < begin))\n+\t\t\tdie(\"file %s has only %lu lines\", name_part, lines);\n \t\tif (begin < 1)\n \t\t\tbegin = 1;\n \t\tif (end < 1)\n \t\t\tend = lines;\n \t\tbegin--;\n-\t\tif (lines < end || lines < begin)\n-\t\t\tdie(\"file %s has only %ld lines\", name_part, lines);\n \t\tline_log_data_insert(&ranges, full_name, begin, end);\n \n \t\tfree_filespec(spec);\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 769ac68..b01b3dd 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -69,7 +69,7 @@ test_expect_success '-L X (X == nlines)' '\n \tgit log -L $n:b.c\n '\n \n-test_expect_failure '-L X (X == nlines + 1)' '\n+test_expect_success '-L X (X == nlines + 1)' '\n \tn=$(expr $(wc -l \n---\n t/annotate-tests.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex f117ef0..0f80cba 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -185,6 +185,10 @@ test_expect_success 'blame -L Y,X (undocumented)' '\n \tcheck_count -L6,3 B 1 B1 1 B2 1 D 1\n '\n \n+test_expect_failure 'blame -L X,+0' '\n+\ttest_must_fail $PROG -L1,+0 file\n+'\n+\n test_expect_success 'blame -L X,+1' '\n \tcheck_count -L3,+1 B2 1\n '\n@@ -193,6 +197,10 @@ test_expect_success 'blame -L X,+N' '\n \tcheck_count -L3,+4 B 1 B1 1 B2 1 D 1\n '\n \n+test_expect_failure 'blame -L X,-0' '\n+\ttest_must_fail $PROG -L1,-0 file\n+'\n+\n test_expect_success 'blame -L X,-1' '\n \tcheck_count -L3,-1 B2 1\n '\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"224337","messageId":"1375258545-42240-10-git-send-email-sunshine@sunshineco.com","threadId":"34573","inReplyTo":"1375258545-42240-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 09/11] blame: reject empty ranges -LX,+0 and -LX,-0","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-31T08:15:43Z","receivedAt":"2013-07-31T08:15:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Empty ranges -LX,+0 and -LX,-0 are nonsensical in the context of blame\nyet they are accepted (in fact, both are interpreted as -LX,+2). Report\nthem as invalid.\n\nSigned-off-by: Eric Sunshine \n---\n line-range.c | 2 ++\n t/annotate-tests.sh | 4 ++--\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/line-range.c b/line-range.c\nindex 3942475..a816951 100644\n--- a/line-range.c\n+++ b/line-range.c\n@@ -26,6 +26,8 @@ static const char *parse_loc(const char *spec, nth_line_fn_t nth_line,\n \t\tif (term != spec + 1) {\n \t\t\tif (!ret)\n \t\t\t\treturn term;\n+\t\t\tif (num == 0)\n+\t\t\t\tdie(\"-L invalid empty range\");\n \t\t\tif (spec[0] == '-')\n \t\t\t\tnum = 0 - num;\n \t\t\tif (0 < num)\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex 0f80cba..01d50c5 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -185,7 +185,7 @@ test_expect_success 'blame -L Y,X (undocumented)' '\n \tcheck_count -L6,3 B 1 B1 1 B2 1 D 1\n '\n \n-test_expect_failure 'blame -L X,+0' '\n+test_expect_success 'blame -L X,+0' '\n \ttest_must_fail $PROG -L1,+0 file\n '\n \n@@ -197,7 +197,7 @@ test_expect_success 'blame -L X,+N' '\n \tcheck_count -L3,+4 B 1 B1 1 B2 1 D 1\n '\n \n-test_expect_failure 'blame -L X,-0' '\n+test_expect_success 'blame -L X,-0' '\n \ttest_must_fail $PROG -L1,-0 file\n '\n \n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"224339","messageId":"1375258545-42240-11-git-send-email-sunshine@sunshineco.com","threadId":"34573","inReplyTo":"1375258545-42240-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 10/11] t8001/t8002: blame: demonstrate acceptance of bogus -L,+0 and -L,-0","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-31T08:15:44Z","receivedAt":"2013-07-31T08:15:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Empty ranges -L,+0 and -L,-0 are nonsensical in the context of blame yet\nthey are accepted. They should be errors. Demonstrate this shortcoming.\n\nSigned-off-by: Eric Sunshine \n---\n t/annotate-tests.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex 01d50c5..d7807df 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -185,6 +185,10 @@ test_expect_success 'blame -L Y,X (undocumented)' '\n \tcheck_count -L6,3 B 1 B1 1 B2 1 D 1\n '\n \n+test_expect_failure 'blame -L ,+0' '\n+\ttest_must_fail $PROG -L,+0 file\n+'\n+\n test_expect_success 'blame -L X,+0' '\n \ttest_must_fail $PROG -L1,+0 file\n '\n@@ -197,6 +201,10 @@ test_expect_success 'blame -L X,+N' '\n \tcheck_count -L3,+4 B 1 B1 1 B2 1 D 1\n '\n \n+test_expect_failure 'blame -L ,-0' '\n+\ttest_must_fail $PROG -L,-0 file\n+'\n+\n test_expect_success 'blame -L X,-0' '\n \ttest_must_fail $PROG -L1,-0 file\n '\n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"224340","messageId":"1375258545-42240-12-git-send-email-sunshine@sunshineco.com","threadId":"34573","inReplyTo":"1375258545-42240-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 11/11] blame: reject empty ranges -L,+0 and -L,-0","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-31T08:15:45Z","receivedAt":"2013-07-31T08:15:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Empty ranges -L,+0 and -L,-0 are nonsensical in the context of blame yet\nthey are accepted (in fact, both are interpreted as -L1,Y where Y is\nend-of-file). Report them as invalid.\n\nSigned-off-by: Eric Sunshine \n---\n line-range.c | 2 +-\n t/annotate-tests.sh | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/line-range.c b/line-range.c\nindex a816951..69e8d6b 100644\n--- a/line-range.c\n+++ b/line-range.c\n@@ -21,7 +21,7 @@ static const char *parse_loc(const char *spec, nth_line_fn_t nth_line,\n \t * for 20 lines, or \"-L ,-5\" for 5 lines ending at\n \t * .\n \t */\n-\tif (1 < begin && (spec[0] == '+' || spec[0] == '-')) {\n+\tif (1 <= begin && (spec[0] == '+' || spec[0] == '-')) {\n \t\tnum = strtol(spec + 1, &term, 10);\n \t\tif (term != spec + 1) {\n \t\t\tif (!ret)\ndiff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\nindex d7807df..e422a9e 100644\n--- a/t/annotate-tests.sh\n+++ b/t/annotate-tests.sh\n@@ -185,7 +185,7 @@ test_expect_success 'blame -L Y,X (undocumented)' '\n \tcheck_count -L6,3 B 1 B1 1 B2 1 D 1\n '\n \n-test_expect_failure 'blame -L ,+0' '\n+test_expect_success 'blame -L ,+0' '\n \ttest_must_fail $PROG -L,+0 file\n '\n \n@@ -201,7 +201,7 @@ test_expect_success 'blame -L X,+N' '\n \tcheck_count -L3,+4 B 1 B1 1 B2 1 D 1\n '\n \n-test_expect_failure 'blame -L ,-0' '\n+test_expect_success 'blame -L ,-0' '\n \ttest_must_fail $PROG -L,-0 file\n '\n \n-- \n1.8.3.4.1120.gc240c48\n"},{"id":"224523","messageId":"87ob9e25ww.fsf@hexa.v.cablecom.net","threadId":"34573","inReplyTo":"1375258545-42240-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH 00/11] blame/log -L: additional tests and bug fixes","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-08-03T14:41:19Z","receivedAt":"2013-08-03T14:41:19Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Eric Sunshine writes:\n\n> While working on multiple -L support for git-blame, I encountered more\n> issues with the existing -L facility in git-blame and git-log. This\n> series fixes these problems and adds a slew of new tests.\n>\n> Patch 6/11 (t4211: retire soon-to-be unimplementable tests) may be\n> controversial. Removal of these tests was effectively a decision made in\n> isolation since my request for input [1] regarding the issue generated\n> only a single response (from j6t).\n\nI agree with that decision. It's better to not leave any user-facing\nquirks just for internal's sake.\n\nThe right thing would be to either expose enough of the range_set api\nthrough a test-range-set command, or just write all the tests in C.\nBoth seem a bit excessive since the API doesn't have any users outside\nof log -L, which probably approaches \"reasonable quality\" now that you\nshaked it down quite a bit.\n\nAs for the series, my tuits don't go further than a cursory read today,\nbut from that it seems good.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"224619","messageId":"7v4nb4j5qu.fsf@alter.siamese.dyndns.org","threadId":"34573","inReplyTo":"1375258545-42240-3-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-05T19:29:45Z","receivedAt":"2013-08-05T19:29:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> A bounds checking bug allows the X in -LX to extend one line past the\n> end of file. For example, given a file with 5 lines, -L6 is accepted as\n> valid. Demonstrate this problem.\n>\n> While here, also add tests to check that the remaining cases of X and Y\n> in -LX,Y are handled correctly at and in the vicinity of end-of-file.\n>\n> Signed-off-by: Eric Sunshine \n> ---\n> t/annotate-tests.sh | 22 ++++++++++++++++++++++\n> 1 file changed, 22 insertions(+)\n>\n> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\n> index 3524eaf..02fbbf1 100644\n> --- a/t/annotate-tests.sh\n> +++ b/t/annotate-tests.sh\n> @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' '\n> \tcheck_count -L/99/,-3 B 1 B2 1 D 1\n> '\n> \n> +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than\n> +# git-blame sees, hence the last line is actually $(wc...)+1.\n> +test_expect_success 'blame -L X (X == nlines)' '\n> +\tn=$(expr $(wc -l +\tcheck_count -L$n C 1\n> +'\n> +\n> +test_expect_failure 'blame -L X (X == nlines + 1)' '\n> +\tn=$(expr $(wc -l +\ttest_must_fail $PROG -L$n file\n> +'\n> +\n> test_expect_success 'blame -L X (X > nlines)' '\n> \ttest_must_fail $PROG -L12345 file\n> '\n> +test_expect_success 'blame -L ,Y (Y == nlines)' '\n> +\tn=$(expr $(wc -l +\tcheck_count -L,$n A 1 B 1 B1 1 B2 1 \"A U Thor\" 1 C 1 D 1 E 1\n> +'\n> +\n> +test_expect_success 'blame -L ,Y (Y == nlines + 1)' '\n> +\tn=$(expr $(wc -l +\ttest_must_fail $PROG -L,$n file\n> +'\n> +\n\nThis is somewhat curious.\n\nDoes the problem triggers only on a file that ends with an\nincomplete line, or the test was inserted at this location only\nbecause it was convenient and the problem exists with or without the\nincomplete final line?\n\nI am guessing that it is the latter.\n\nThanks.\n\n> test_expect_success 'blame -L ,Y (Y > nlines)' '\n> \ttest_must_fail $PROG -L,12345 file\n> '\n"},{"id":"224622","messageId":"CAPig+cTDrhruSHouGrKwFy3TqUe1_4YxJi-oWTUUe6KpZ7p4Qw@mail.gmail.com","threadId":"34573","inReplyTo":"7v4nb4j5qu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-05T19:35:37Z","receivedAt":"2013-08-05T19:35:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 5, 2013 at 3:29 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>> A bounds checking bug allows the X in -LX to extend one line past the\n>> end of file. For example, given a file with 5 lines, -L6 is accepted as\n>> valid. Demonstrate this problem.\n>>\n>> While here, also add tests to check that the remaining cases of X and Y\n>> in -LX,Y are handled correctly at and in the vicinity of end-of-file.\n>>\n>> Signed-off-by: Eric Sunshine \n>> ---\n>> t/annotate-tests.sh | 22 ++++++++++++++++++++++\n>> 1 file changed, 22 insertions(+)\n>>\n>> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\n>> index 3524eaf..02fbbf1 100644\n>> --- a/t/annotate-tests.sh\n>> +++ b/t/annotate-tests.sh\n>> @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' '\n>> check_count -L/99/,-3 B 1 B2 1 D 1\n>> '\n>>\n>> +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than\n>> +# git-blame sees, hence the last line is actually $(wc...)+1.\n>> +test_expect_success 'blame -L X (X == nlines)' '\n>> + n=$(expr $(wc -l > + check_count -L$n C 1\n>> +'\n>> +\n>> +test_expect_failure 'blame -L X (X == nlines + 1)' '\n>> + n=$(expr $(wc -l > + test_must_fail $PROG -L$n file\n>> +'\n>> +\n>> test_expect_success 'blame -L X (X > nlines)' '\n>> test_must_fail $PROG -L12345 file\n>> '\n>> +test_expect_success 'blame -L ,Y (Y == nlines)' '\n>> + n=$(expr $(wc -l > + check_count -L,$n A 1 B 1 B1 1 B2 1 \"A U Thor\" 1 C 1 D 1 E 1\n>> +'\n>> +\n>> +test_expect_success 'blame -L ,Y (Y == nlines + 1)' '\n>> + n=$(expr $(wc -l > + test_must_fail $PROG -L,$n file\n>> +'\n>> +\n>\n> This is somewhat curious.\n>\n> Does the problem triggers only on a file that ends with an\n> incomplete line, or the test was inserted at this location only\n> because it was convenient and the problem exists with or without the\n> incomplete final line?\n>\n> I am guessing that it is the latter.\n\nThe problem exists with and without the incomplete line. The comment\nmentioning \"incomplete line\" and \"wc\" was inserted to explain why it\nwas necessary to add one to the result of wc, which might not\notherwise be obvious.\n\nThe tests were inserted at this location because they are semantically\nrelated to the \"blame -L ,Y (Y > nlines)\" test which already exists in\nthe file (quote just below this response).\n\n>> test_expect_success 'blame -L ,Y (Y > nlines)' '\n>> test_must_fail $PROG -L,12345 file\n>> '\n"},{"id":"224623","messageId":"CAPig+cRYASsHyujx3uhHtRqSnfC8e1cAEMAa2thS3mgOyF2Ozw@mail.gmail.com","threadId":"34573","inReplyTo":"CAPig+cTDrhruSHouGrKwFy3TqUe1_4YxJi-oWTUUe6KpZ7p4Qw@mail.gmail.com","subject":"Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-08-05T19:45:03Z","receivedAt":"2013-08-05T19:45:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 5, 2013 at 3:35 PM, Eric Sunshine wrote:\n> On Mon, Aug 5, 2013 at 3:29 PM, Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>>\n>>> A bounds checking bug allows the X in -LX to extend one line past the\n>>> end of file. For example, given a file with 5 lines, -L6 is accepted as\n>>> valid. Demonstrate this problem.\n>>>\n>>> While here, also add tests to check that the remaining cases of X and Y\n>>> in -LX,Y are handled correctly at and in the vicinity of end-of-file.\n>>>\n>>> Signed-off-by: Eric Sunshine \n>>> ---\n>>> t/annotate-tests.sh | 22 ++++++++++++++++++++++\n>>> 1 file changed, 22 insertions(+)\n>>>\n>>> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh\n>>> index 3524eaf..02fbbf1 100644\n>>> --- a/t/annotate-tests.sh\n>>> +++ b/t/annotate-tests.sh\n>>> @@ -225,10 +225,32 @@ test_expect_success 'blame -L /RE/,-N' '\n>>> check_count -L/99/,-3 B 1 B2 1 D 1\n>>> '\n>>>\n>>> +# 'file' ends with an incomplete line, so 'wc' reports one fewer lines than\n>>> +# git-blame sees, hence the last line is actually $(wc...)+1.\n>>> +test_expect_success 'blame -L X (X == nlines)' '\n>>> + n=$(expr $(wc -l >> + check_count -L$n C 1\n>>\n>> This is somewhat curious.\n>>\n>> Does the problem triggers only on a file that ends with an\n>> incomplete line, or the test was inserted at this location only\n>> because it was convenient and the problem exists with or without the\n>> incomplete final line?\n>>\n>> I am guessing that it is the latter.\n>\n> The problem exists with and without the incomplete line. The comment\n> mentioning \"incomplete line\" and \"wc\" was inserted to explain why it\n> was necessary to add one to the result of wc, which might not\n> otherwise be obvious.\n\nWould the comment be clearer if phrased like this?\n\n # We want to test -LX where X is the last line of the file, so we need\n # to compute the number of lines in the file, which normally would be\n # done via 'wc -l'. In this case, however, the last line of 'file' is\n # incomplete, so 'wc' reports one fewer than the actual line count. To\n # adjust for this anomaly, we must add one to the result of 'wc'.\n"},{"id":"224632","messageId":"7vk3jziulg.fsf@alter.siamese.dyndns.org","threadId":"34573","inReplyTo":"CAPig+cRYASsHyujx3uhHtRqSnfC8e1cAEMAa2thS3mgOyF2Ozw@mail.gmail.com","subject":"Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-05T23:30:35Z","receivedAt":"2013-08-05T23:30:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> The problem exists with and without the incomplete line. The comment\n>> mentioning \"incomplete line\" and \"wc\" was inserted to explain why it\n>> was necessary to add one to the result of wc, which might not\n>> otherwise be obvious.\n>\n> Would the comment be clearer if phrased like this?\n>\n> # We want to test -LX where X is the last line of the file, so we need\n> # to compute the number of lines in the file, which normally would be\n> # done via 'wc -l'. In this case, however, the last line of 'file' is\n> # incomplete, so 'wc' reports one fewer than the actual line count. To\n> # adjust for this anomaly, we must add one to the result of 'wc'.\n\nIf the patch picked a place where the test vector does not have to\ninvolve a file that ends with an incomplete line, the test would not\nhave had to do anything tricky that required such a comment to\nexplain why it is doing something mysterious.\n\nSuch a change would be much cleaner and may be worth the effort, but\nI do not think just rewording the comment is worth the bother.\n\nI didn't see if there is such a place in the existing test sequence,\nthough.\n"}]}