From f350cf9ea5058ca0ad9e02033616d574c59ee064 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 31 Jul 2013 04:15:35 -0400 Subject: t8001/t8002: blame: decompose overly-large test 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 Signed-off-by: Junio C Hamano diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index d4e7f47..ffc5697 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 ' -- cgit v0.10.2-6-g49f6 From 580b4f3acf076f5f3346206d3c54912ffc663d86 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 31 Jul 2013 04:15:36 -0400 Subject: 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 Signed-off-by: Junio C Hamano diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index ffc5697..47b5007 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 nlines)' ' test_must_fail $PROG -L12345 file ' +test_expect_success 'blame -L ,Y (Y == nlines)' ' + n=$(expr $(wc -l nlines)' ' test_must_fail $PROG -L,12345 file ' -- cgit v0.10.2-6-g49f6 From a8fa8eca3f38ab89ad116575d0a5c47cdd03a7e9 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 31 Jul 2013 04:15:37 -0400 Subject: 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 Signed-off-by: Junio C Hamano diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index 47b5007..22db31e 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 ' -- cgit v0.10.2-6-g49f6 From 164a9cf4303d4a8e5c969e057adf9d700468ddc5 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 31 Jul 2013 04:15:38 -0400 Subject: 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 Signed-off-by: Junio C Hamano 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 22db31e..015acab 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 Date: Wed, 31 Jul 2013 04:15:39 -0400 Subject: 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 Signed-off-by: Junio C Hamano 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 Date: Wed, 31 Jul 2013 04:15:40 -0400 Subject: 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 Signed-off-by: Junio C Hamano 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 Date: Wed, 31 Jul 2013 04:15:41 -0400 Subject: 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 Signed-off-by: Junio C Hamano 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 Date: Wed, 31 Jul 2013 04:15:42 -0400 Subject: 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 Signed-off-by: Junio C Hamano diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index 015acab..bbf3ee6 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 ' -- cgit v0.10.2-6-g49f6 From abba35395f859600cca1c04f1eab633963a8646e Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 31 Jul 2013 04:15:43 -0400 Subject: 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 Signed-off-by: Junio C Hamano 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 bbf3ee6..cd9222b 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 ' -- cgit v0.10.2-6-g49f6 From 82cd7e5d3e56c173769bc5fba91e0c6415e208f0 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 31 Jul 2013 04:15:44 -0400 Subject: 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 Signed-off-by: Junio C Hamano diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh index cd9222b..aca87e8 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 ' -- cgit v0.10.2-6-g49f6 From 5d57cac6ae7c661d430dcc7dd2e44c994bf797be Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Wed, 31 Jul 2013 04:15:45 -0400 Subject: 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 Signed-off-by: Junio C Hamano 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 ,-5" for 5 lines ending at * . */ - 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 aca87e8..ce5b8ed 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 ' -- cgit v0.10.2-6-g49f6