From cab7bb36eb85dbe38ad95ee02b083f11f0820e24 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 4 Aug 2018 22:33:59 +0200 Subject: [PATCH 1/4] line-log: demonstrate a bug with nearly-overlapping ranges Currently, this test case throws an assertion: Assertion failed! Program: git.exe File: line-log.c, Line 71 Signed-off-by: Johannes Schindelin --- t/t4211-line-log.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh index 436b13ad21d063..61ff374300208b 100755 --- a/t/t4211-line-log.sh +++ b/t/t4211-line-log.sh @@ -115,4 +115,21 @@ test_expect_success 'range_set_union' ' git log $(for x in $(test_seq 200); do echo -L $((2*x)),+1:c.c; done) ' +q_to_lf () { + tr Q '\012' +} + +test_expect_failure 'close to overlapping ranges' ' + test_seq 5 >a1.c && + git add a1.c && + git commit -m "5 lines" a1.c && + sed s/3/3QaQb/ tmp && + mv tmp a1.c && + git commit -m "2 more lines" a1.c && + sed s/4/cQ4/ tmp && + mv tmp a1.c && + git commit -m "1 more line" a1.c && + git --no-pager log -L 1,3:a1.c -L 5,8:a1.c +' + test_done From 7f92d92154143127734a638e41e064adce46a2e2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 4 Aug 2018 22:56:05 +0200 Subject: [PATCH 2/4] line-log: adjust start/end of ranges individually When traversing commits and adjusting the ranges, things can get really tricky. For example, when the line range of interest encloses several hunks of a commit, the line range can actually shrink. Currently, range_set_shift_diff() does not anticipate that scenario and blindly adjusts start and end by the same offset ("shift" the range). This can lead to a couple of surprising issues, such as assertions in range_set_append() (when the end of a given range is not adjusted properly, it can point after the start of the next range) or even segmentation faults (when t_end in the loop of dump_diff_hacky_one() points outside the valid line range). Let's fix this by adjusting the start and the end offsets individually. Signed-off-by: Johannes Schindelin --- line-log.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/line-log.c b/line-log.c index 72a5fed661ca0a..d8d09b5ee7ef72 100644 --- a/line-log.c +++ b/line-log.c @@ -427,7 +427,7 @@ static void range_set_shift_diff(struct range_set *out, struct diff_ranges *diff) { unsigned int i, j = 0; - long offset = 0; + long offset = 0, start_offset; struct range *src = rs->ranges; struct range *target = diff->target.ranges; struct range *parent = diff->parent.ranges; @@ -438,7 +438,13 @@ static void range_set_shift_diff(struct range_set *out, - (target[j].end-target[j].start); j++; } - range_set_append(out, src[i].start+offset, src[i].end+offset); + start_offset = offset; + while (j < diff->target.nr && src[i].end > target[j].end) { + offset += (parent[j].end-parent[j].start) + - (target[j].end-target[j].start); + j++; + } + range_set_append(out, src[i].start+start_offset, src[i].end+offset); } } From d5d9db3c1124d29e26864596a8c36f0dc4de8a7e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 4 Aug 2018 23:01:16 +0200 Subject: [PATCH 3/4] line-log: optimize ranges by joining them when possible Technically, it is okay to have line ranges that touch (i.e. the end of the first range ends just before the next range begins). However, it is inefficient, and when the user provides such touching ranges via multiple `-L` options, we already join them. When we traverse the history, though, we never join ranges, even they become "touchy-feely" due to added lines (which are "removed" from line-log's point of view because it traverses the commit history into the past). Let's optimize also this case. Signed-off-by: Johannes Schindelin --- line-log.c | 4 ++++ t/t4211-line-log.sh | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/line-log.c b/line-log.c index d8d09b5ee7ef72..bc7ef69d6c11c0 100644 --- a/line-log.c +++ b/line-log.c @@ -68,6 +68,10 @@ void range_set_append_unsafe(struct range_set *rs, long a, long b) void range_set_append(struct range_set *rs, long a, long b) { + if (rs->nr > 0 && rs->ranges[rs->nr-1].end + 1 == a) { + rs->ranges[rs->nr-1].end = b; + return; + } assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a); range_set_append_unsafe(rs, a, b); } diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh index 61ff374300208b..ebaf5ea8641b12 100755 --- a/t/t4211-line-log.sh +++ b/t/t4211-line-log.sh @@ -119,7 +119,7 @@ q_to_lf () { tr Q '\012' } -test_expect_failure 'close to overlapping ranges' ' +test_expect_success 'close to overlapping ranges' ' test_seq 5 >a1.c && git add a1.c && git commit -m "5 lines" a1.c && From faf35214f0f339b792a30a3bd013056217d9a2c1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 4 Aug 2018 23:06:34 +0200 Subject: [PATCH 4/4] line-log: convert an assertion to a full BUG() call The assertion in question really indicates a bug, when triggered, so we might just as well use the sanctioned method to report it. Signed-off-by: Johannes Schindelin --- line-log.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/line-log.c b/line-log.c index bc7ef69d6c11c0..0e09df9dba966d 100644 --- a/line-log.c +++ b/line-log.c @@ -72,7 +72,9 @@ void range_set_append(struct range_set *rs, long a, long b) rs->ranges[rs->nr-1].end = b; return; } - assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a); + if (rs->nr > 0 && rs->ranges[rs->nr-1].end > a) + BUG("append %ld-%ld, after %ld-%ld?!?", a, b, + rs->ranges[rs->nr-1].start, rs->ranges[rs->nr-1].end); range_set_append_unsafe(rs, a, b); }