From 18d472db6fe3909537ceb7e0c54cf01fa3466b8f Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Tue, 9 Jul 2013 01:55:04 -0400 Subject: [PATCH 1/7] t4211: fix broken test when one -L range is subset of another t4211 attempts to test multiple git-log -L ranges where one range is a superset of the other, and falsely succeeds because its "expected" output is incorrect. Overlapping -L ranges handed to git-log are coalesced by line-log.c:sort_and_merge_range_set() into a set of non-overlapping, disjoint ranges. When one range is a subset of another, sort_and_merge_range_set() should coalesce both ranges to the superset range, but instead the coalesced range often is incorrectly truncated to the end of the subset range. For example, ranges 2-8 and 3-4 are coalesced incorrectly to 2-4. One can observe this incorrect behavior with git-log -L using the test repository created by t4211. The superset/subset ranges t4211 employs are 4-$ and 8-12 (where $ represents end-of-file). The coalesced range should be 4-$. Manually invoking git-log with the same ranges the test employs, we see: % git log -L 4:a.c simple | awk '/^commit [0-9a-f]{40}/ { print substr($2,1,7) }' 4659538 100b61a 39b6eb2 a6eb826 f04fb20 de4c48a % git log -L 8,12:a.c simple | awk ... f04fb20 de4c48a % git log -L 4:a.c -L 8,12:a.c simple | awk ... a6eb826 f04fb20 de4c48a This last output is incorrect. 8-12 is a subset of 4-$, hence the output of the coalesced range should be the same as the 4-$ output shown first. In fact, the above incorrect output is the truncated bogus range 4-12: % git log -L 4,12:a.c simple | awk ... a6eb826 f04fb20 de4c48a Fix the test to correctly fail in the presence of the sort_and_merge_range_set() coalescing bug. Do so by changing the "expected" output to the commits mentioned in the 4-$ output above. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/t4211-line-log.sh | 4 +- t/t4211/expect.multiple-superset | 134 ++++++++++++++++++++++++++++++- 2 files changed, 133 insertions(+), 5 deletions(-) diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh index 7776f93e3d..549df9e054 100755 --- a/t/t4211-line-log.sh +++ b/t/t4211-line-log.sh @@ -50,8 +50,8 @@ canned_test "-M -L ':f:b.c' parallel-change" parallel-change-f-to-main canned_test "-L 4,12:a.c -L :main:a.c simple" multiple canned_test "-L 4,18:a.c -L :main:a.c simple" multiple-overlapping canned_test "-L :main:a.c -L 4,18:a.c simple" multiple-overlapping -canned_test "-L 4:a.c -L 8,12:a.c simple" multiple-superset -canned_test "-L 8,12:a.c -L 4:a.c simple" multiple-superset +canned_test_failure "-L 4:a.c -L 8,12:a.c simple" multiple-superset +canned_test_failure "-L 8,12:a.c -L 4:a.c simple" multiple-superset test_bad_opts "-L" "switch.*requires a value" test_bad_opts "-L b.c" "argument.*not of the form" diff --git a/t/t4211/expect.multiple-superset b/t/t4211/expect.multiple-superset index a1f5bc49c8..d930b6eec4 100644 --- a/t/t4211/expect.multiple-superset +++ b/t/t4211/expect.multiple-superset @@ -1,3 +1,100 @@ +commit 4659538844daa2849b1a9e7d6fadb96fcd26fc83 +Author: Thomas Rast +Date: Thu Feb 28 10:48:43 2013 +0100 + + change back to complete line + +diff --git a/a.c b/a.c +--- a/a.c ++++ b/a.c +@@ -4,19 +4,21 @@ + long f(long x) + { + int s = 0; + while (x) { + x >>= 1; + s++; + } + return s; + } + + /* + * This is only an example! + */ + + int main () + { + printf("%ld\n", f(15)); + return 0; +-} +\ No newline at end of file ++} ++ ++/* incomplete lines are bad! */ + +commit 100b61a6f2f720f812620a9d10afb3a960ccb73c +Author: Thomas Rast +Date: Thu Feb 28 10:48:10 2013 +0100 + + change to an incomplete line at end + +diff --git a/a.c b/a.c +--- a/a.c ++++ b/a.c +@@ -4,19 +4,19 @@ + long f(long x) + { + int s = 0; + while (x) { + x >>= 1; + s++; + } + return s; + } + + /* + * This is only an example! + */ + + int main () + { + printf("%ld\n", f(15)); + return 0; +-} ++} +\ No newline at end of file + +commit 39b6eb2d5b706d3322184a169f666f25ed3fbd00 +Author: Thomas Rast +Date: Thu Feb 28 10:45:41 2013 +0100 + + touch comment + +diff --git a/a.c b/a.c +--- a/a.c ++++ b/a.c +@@ -3,19 +3,19 @@ + long f(long x) + { + int s = 0; + while (x) { + x >>= 1; + s++; + } + return s; + } + + /* +- * A comment. ++ * This is only an example! + */ + + int main () + { + printf("%ld\n", f(15)); + return 0; + } + commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2 Author: Thomas Rast Date: Thu Feb 28 10:45:16 2013 +0100 @@ -7,7 +104,7 @@ Date: Thu Feb 28 10:45:16 2013 +0100 diff --git a/a.c b/a.c --- a/a.c +++ b/a.c -@@ -3,9 +3,9 @@ +@@ -3,19 +3,19 @@ -int f(int x) +long f(long x) { @@ -18,6 +115,17 @@ diff --git a/a.c b/a.c } return s; } + + /* + * A comment. + */ + + int main () + { +- printf("%d\n", f(15)); ++ printf("%ld\n", f(15)); + return 0; + } commit f04fb20f2c77850996cba739709acc6faecc58f7 Author: Thomas Rast @@ -28,7 +136,7 @@ Date: Thu Feb 28 10:44:55 2013 +0100 diff --git a/a.c b/a.c --- a/a.c +++ b/a.c -@@ -3,8 +3,9 @@ +@@ -3,18 +3,19 @@ int f(int x) { int s = 0; @@ -38,6 +146,16 @@ diff --git a/a.c b/a.c } + return s; } + + /* + * A comment. + */ + + int main () + { + printf("%d\n", f(15)); + return 0; + } commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a Author: Thomas Rast @@ -48,7 +166,7 @@ Date: Thu Feb 28 10:44:48 2013 +0100 diff --git a/a.c b/a.c --- /dev/null +++ b/a.c -@@ -0,0 +3,8 @@ +@@ -0,0 +3,18 @@ +int f(int x) +{ + int s = 0; @@ -57,3 +175,13 @@ diff --git a/a.c b/a.c + s++; + } +} ++ ++/* ++ * A comment. ++ */ ++ ++int main () ++{ ++ printf("%d\n", f(15)); ++ return 0; ++} From 3755b53af779ce75fa3ea4581a0e6525bc67278d Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Tue, 9 Jul 2013 01:55:05 -0400 Subject: [PATCH 2/7] range_set: fix coalescing bug when range is a subset of another When coalescing ranges, sort_and_merge_range_set() unconditionally assumes that the end of a range being folded into a preceding range should become the end of the coalesced range. This assumption, however, is invalid when one range is a subset of another. For example, given ranges 1-5 and 2-3 added via range_set_append_unsafe(), sort_and_merge_range_set() incorrectly coalesces them to range 1-3 rather than the correct union range 1-5. Fix this bug. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- line-log.c | 3 ++- t/t4211-line-log.sh | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/line-log.c b/line-log.c index 4bbb09be59..8cc29a0000 100644 --- a/line-log.c +++ b/line-log.c @@ -116,7 +116,8 @@ static void sort_and_merge_range_set(struct range_set *rs) for (i = 1; i < rs->nr; i++) { if (rs->ranges[i].start <= rs->ranges[o-1].end) { - rs->ranges[o-1].end = rs->ranges[i].end; + if (rs->ranges[o-1].end < rs->ranges[i].end) + rs->ranges[o-1].end = rs->ranges[i].end; } else { rs->ranges[o].start = rs->ranges[i].start; rs->ranges[o].end = rs->ranges[i].end; diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh index 549df9e054..7776f93e3d 100755 --- a/t/t4211-line-log.sh +++ b/t/t4211-line-log.sh @@ -50,8 +50,8 @@ canned_test "-M -L ':f:b.c' parallel-change" parallel-change-f-to-main canned_test "-L 4,12:a.c -L :main:a.c simple" multiple canned_test "-L 4,18:a.c -L :main:a.c simple" multiple-overlapping canned_test "-L :main:a.c -L 4,18:a.c simple" multiple-overlapping -canned_test_failure "-L 4:a.c -L 8,12:a.c simple" multiple-superset -canned_test_failure "-L 8,12:a.c -L 4:a.c simple" multiple-superset +canned_test "-L 4:a.c -L 8,12:a.c simple" multiple-superset +canned_test "-L 8,12:a.c -L 4:a.c simple" multiple-superset test_bad_opts "-L" "switch.*requires a value" test_bad_opts "-L b.c" "argument.*not of the form" From b6679e768f39f718b7f2983d1958f5fb1121f356 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Tue, 23 Jul 2013 10:28:04 -0400 Subject: [PATCH 3/7] range-set: fix sort_and_merge_range_set() corner case bug When handed an empty range_set (range_set.nr == 0), sort_and_merge_range_set() incorrectly sets range_set.nr to 1 at exit. Subsequent range_set functions then access the bogus range at element zero and crash or throw an assertion failure. Fix this bug. Signed-off-by: Eric Sunshine Acked-by: Thomas Rast Signed-off-by: Junio C Hamano --- line-log.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/line-log.c b/line-log.c index 8cc29a0000..52348796c1 100644 --- a/line-log.c +++ b/line-log.c @@ -110,12 +110,12 @@ static void range_set_check_invariants(struct range_set *rs) static void sort_and_merge_range_set(struct range_set *rs) { int i; - int o = 1; /* output cursor */ + int o = 0; /* output cursor */ qsort(rs->ranges, rs->nr, sizeof(struct range), range_cmp); - for (i = 1; i < rs->nr; i++) { - if (rs->ranges[i].start <= rs->ranges[o-1].end) { + for (i = 0; i < rs->nr; i++) { + if (o > 0 && rs->ranges[i].start <= rs->ranges[o-1].end) { if (rs->ranges[o-1].end < rs->ranges[i].end) rs->ranges[o-1].end = rs->ranges[i].end; } else { From 58960978462e66c3dbc53b274988e7f27e7c74e2 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Tue, 23 Jul 2013 10:28:05 -0400 Subject: [PATCH 4/7] t4211: demonstrate empty -L range crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Eric Sunshine Acked-by: Thomas Rast Helped-by: SZEDER Gábor Signed-off-by: Junio C Hamano --- t/t4211-line-log.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh index 7776f93e3d..9042178124 100755 --- a/t/t4211-line-log.sh +++ b/t/t4211-line-log.sh @@ -64,4 +64,12 @@ 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" +# 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_failure '-L {empty-range} (any -L)' ' + n=$(expr $(wc -l Date: Tue, 23 Jul 2013 10:28:07 -0400 Subject: [PATCH 5/7] t4211: demonstrate crash when first -L encountered is empty range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Eric Sunshine Acked-by: Thomas Rast Helped-by: SZEDER Gábor Signed-off-by: Junio C Hamano --- t/t4211-line-log.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh index 9042178124..d98efb3c4e 100755 --- a/t/t4211-line-log.sh +++ b/t/t4211-line-log.sh @@ -72,4 +72,9 @@ test_expect_failure '-L {empty-range} (any -L)' ' git log -L1,1:b.c -L$n:b.c ' +test_expect_failure '-L {empty-range} (first -L)' ' + n=$(expr $(wc -l Date: Tue, 23 Jul 2013 10:28:06 -0400 Subject: [PATCH 6/7] range-set: satisfy non-empty ranges invariant range-set invariants are: ranges must be (1) non-empty, (2) disjoint, (3) sorted in ascending order. During processing, various range-set utility functions break the invariants (for instance, by adding empty ranges), with the expectation that a finalizing sort_and_merge_range_set() will restore sanity. sort_and_merge_range_set(), however, neglects to fold out empty ranges, thus it fails to satisfy the non-empty constraint. Subsequent range-set functions crash or throw an assertion failure upon encountering such an anomaly. Rectify the situation by having sort_and_merge_range_set() fold out empty ranges. Signed-off-by: Eric Sunshine Acked-by: Thomas Rast Signed-off-by: Junio C Hamano --- line-log.c | 2 ++ t/t4211-line-log.sh | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/line-log.c b/line-log.c index 52348796c1..6f94d56063 100644 --- a/line-log.c +++ b/line-log.c @@ -115,6 +115,8 @@ static void sort_and_merge_range_set(struct range_set *rs) qsort(rs->ranges, rs->nr, sizeof(struct range), range_cmp); for (i = 0; i < rs->nr; i++) { + if (rs->ranges[i].start == rs->ranges[i].end) + continue; if (o > 0 && rs->ranges[i].start <= rs->ranges[o-1].end) { if (rs->ranges[o-1].end < rs->ranges[i].end) rs->ranges[o-1].end = rs->ranges[i].end; diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh index d98efb3c4e..e7a6e49965 100755 --- a/t/t4211-line-log.sh +++ b/t/t4211-line-log.sh @@ -67,7 +67,8 @@ test_bad_opts "-L :foo:b.c" "no match" # 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_failure '-L {empty-range} (any -L)' ' +test_expect_success '-L {empty-range} (any -L)' ' + n=$(expr $(cat b.c | wc -l) + 1) && n=$(expr $(wc -l Date: Tue, 23 Jul 2013 10:28:08 -0400 Subject: [PATCH 7/7] line-log: fix "log -LN" crash when N is last line of file range-set invariants are: ranges must be (1) non-empty, (2) disjoint, (3) sorted in ascending order. line_log_data_insert() breaks the non-empty invariant under the following conditions: the incoming range is empty and the pathname attached to the range has not yet been encountered. In this case, line_log_data_insert() assigns the empty range to a new line_log_data record without taking any action to ensure that the empty range is eventually folded out. Subsequent range-set functions crash or throw an assertion failure upon encountering such an anomaly. Fix this bug. Signed-off-by: Eric Sunshine Acked-by: Thomas Rast Signed-off-by: Junio C Hamano --- line-log.c | 1 + t/t4211-line-log.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/line-log.c b/line-log.c index 6f94d56063..c2d01dccc2 100644 --- a/line-log.c +++ b/line-log.c @@ -299,6 +299,7 @@ static void line_log_data_insert(struct line_log_data **list, p = xcalloc(1, sizeof(struct line_log_data)); p->path = path; range_set_append(&p->ranges, begin, end); + sort_and_merge_range_set(&p->ranges); if (ip) { p->next = ip->next; ip->next = p; diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh index e7a6e49965..00a850d611 100755 --- a/t/t4211-line-log.sh +++ b/t/t4211-line-log.sh @@ -73,7 +73,7 @@ test_expect_success '-L {empty-range} (any -L)' ' git log -L1,1:b.c -L$n:b.c ' -test_expect_failure '-L {empty-range} (first -L)' ' +test_expect_success '-L {empty-range} (first -L)' ' n=$(expr $(wc -l