[PATCH v3 2/7] diff: simplify the line-range filter by classifying removals immediately

["Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com>]

From: Michael Montalbo <mmontalbo@gmail.com>

Currently, the diff line-range filter buffers preimage removal lines
until a postimage line arrives. That line's number confirms whether the
preimage line falls in a relevant range. However, storing preimage
lines in a separate buffer is unnecessary. Worse, the logic has a bug:
a preimage line outside the target range is included when it
immediately follows an in-range postimage line.

Preimage lines will always precede their postimage counterpart both in
content line number and emission order from xdiff's line callback
function. So preimage lines can share the postimage buffer. The filter
flushes them based on whether the postimage lines fall within the
target range.

Remove logic related to storing preimage lines in a separate "removal"
buffer and prepending them to the accumulating_hunk's line buffer.
Instead, store those lines in the accumulating_hunk's line_buffer
immediately and flush everything as appropriate based on postimage
line numbers that arrive. This resolves the bug by construction.

Also, calculate the old and new line counts for the diff hunk header
when flushing rather than storing counters in line_range_filter to
simplify state management further.

Add a test to t/t4211-line-log.sh that verifies the preimage line
emission bug is fixed.

Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
 diff.c              | 121 +++++++++++++++++---------------------------
 t/t4211-line-log.sh |  31 ++++++++++++
 2 files changed, 78 insertions(+), 74 deletions(-)

diff --git a/diff.c b/diff.c
index 679a0e27d4..c94ddbebe5 100644
--- a/diff.c
+++ b/diff.c
@@ -621,19 +621,14 @@ struct line_range_filter {
 	struct {
 		char func_name[80];
 		long func_name_len;
-		long old_begin, old_count;
-		long new_begin, new_count;
+		long old_begin;
+		long new_begin;
 		long lno_in_preimage;
 		long lno_in_postimage;
 		struct strbuf lines;
 		int active;
-		int has_changes;
 	} accumulating_hunk;
 
-	struct strbuf pending_rm;
-	int pending_rm_count;
-	long pending_rm_pre_begin;
-
 	int ret;
 };
 
@@ -2522,40 +2517,56 @@ static int quick_consume(void *priv, char *line UNUSED, unsigned long len UNUSED
 	return 1;
 }
 
-static void discard_pending_rm(struct line_range_filter *filter)
+static void begin_range_hunk(struct line_range_filter *filter)
 {
-	strbuf_reset(&filter->pending_rm);
-	filter->pending_rm_count = 0;
+	filter->accumulating_hunk.active = 1;
+	filter->accumulating_hunk.new_begin = filter->accumulating_hunk.lno_in_postimage;
+	filter->accumulating_hunk.old_begin = filter->accumulating_hunk.lno_in_preimage;
+	strbuf_reset(&filter->accumulating_hunk.lines);
 }
 
 static void flush_range_hunk(struct line_range_filter *filter)
 {
 	struct strbuf hdr = STRBUF_INIT;
 	const char *line_buf, *line_buf_end;
+	long old_count = 0, new_count = 0;
+	int has_changes = 0;
 
 	if (!filter->accumulating_hunk.active || filter->ret)
 		return;
 
-	if (filter->pending_rm_count) {
-		strbuf_addbuf(&filter->accumulating_hunk.lines, &filter->pending_rm);
-		filter->accumulating_hunk.old_count += filter->pending_rm_count;
-		filter->accumulating_hunk.has_changes = 1;
-		discard_pending_rm(filter);
+	line_buf = filter->accumulating_hunk.lines.buf;
+	line_buf_end = line_buf + filter->accumulating_hunk.lines.len;
+	while (line_buf < line_buf_end) {
+		const char *eol = memchr(line_buf, '\n', line_buf_end - line_buf);
+		if (*line_buf == ' ') {
+			old_count++;
+			new_count++;
+		}
+		else if (*line_buf == '-') {
+			old_count++;
+			has_changes = 1;
+		}
+		else if (*line_buf == '+') {
+			new_count++;
+			has_changes = 1;
+		}
+		line_buf = eol ? eol + 1 : line_buf_end;
 	}
 
-	if (!filter->accumulating_hunk.has_changes) {
+	if (!has_changes) {
 		filter->accumulating_hunk.active = 0;
 		strbuf_reset(&filter->accumulating_hunk.lines);
 		return;
 	}
 
 	strbuf_addf(&hdr, "@@ -%ld,%ld +%ld,%ld @@",
-		    filter->accumulating_hunk.old_begin, filter->accumulating_hunk.old_count,
-		    filter->accumulating_hunk.new_begin, filter->accumulating_hunk.new_count);
+		    filter->accumulating_hunk.old_begin, old_count,
+		    filter->accumulating_hunk.new_begin, new_count);
 	if (filter->accumulating_hunk.func_name_len > 0) {
 		strbuf_addch(&hdr, ' ');
 		strbuf_add(&hdr, filter->accumulating_hunk.func_name,
-	     filter->accumulating_hunk.func_name_len);
+			   filter->accumulating_hunk.func_name_len);
 	}
 	strbuf_addch(&hdr, '\n');
 
@@ -2598,84 +2609,48 @@ static void line_range_hunk_fn(void *data,
 static int line_range_line_fn(void *priv, char *line, unsigned long len)
 {
 	struct line_range_filter *filter = priv;
-	const struct range *cur;
-	long idx_in_postimage, cur_pre;
+	long idx_in_postimage;
+	int in_range;
 
 	if (filter->ret)
 		return filter->ret;
 
-	if (line[0] == '-') {
-		if (!filter->pending_rm_count)
-			filter->pending_rm_pre_begin =
-				filter->accumulating_hunk.lno_in_preimage;
-		filter->accumulating_hunk.lno_in_preimage++;
-		strbuf_add(&filter->pending_rm, line, len);
-		filter->pending_rm_count++;
-		return filter->ret;
-	}
-
 	if (line[0] == '\\') {
-		if (filter->pending_rm_count)
-			strbuf_add(&filter->pending_rm, line, len);
-		else if (filter->accumulating_hunk.active)
+		if (filter->accumulating_hunk.active)
 			strbuf_add(&filter->accumulating_hunk.lines, line, len);
 		return filter->ret;
 	}
 
-	if (line[0] != '+' && line[0] != ' ')
+	if (line[0] != '+' && line[0] != ' ' && line[0] != '-')
 		BUG("unexpected diff line type '%c'", line[0]);
 
 	idx_in_postimage = filter->accumulating_hunk.lno_in_postimage - 1;
-	cur_pre = filter->accumulating_hunk.lno_in_preimage;
-	filter->accumulating_hunk.lno_in_postimage++;
-	if (line[0] == ' ')
-		filter->accumulating_hunk.lno_in_preimage++;
 
 	while (filter->range_set_idx < filter->range_sets_to_filter_by->nr &&
 	       idx_in_postimage >=
 		filter->range_sets_to_filter_by->ranges[filter->range_set_idx].end) {
 		if (filter->accumulating_hunk.active)
 			flush_range_hunk(filter);
-		discard_pending_rm(filter);
 		filter->range_set_idx++;
 	}
 
-	if (filter->range_set_idx >= filter->range_sets_to_filter_by->nr) {
-		discard_pending_rm(filter);
-		return filter->ret;
-	}
-
-	cur = &filter->range_sets_to_filter_by->ranges[filter->range_set_idx];
-
-	if (idx_in_postimage < cur->start) {
-		discard_pending_rm(filter);
-		return filter->ret;
-	}
+	in_range = filter->range_set_idx < filter->range_sets_to_filter_by->nr &&
+		   idx_in_postimage >=
+		filter->range_sets_to_filter_by->ranges[filter->range_set_idx].start &&
+		   idx_in_postimage <
+		filter->range_sets_to_filter_by->ranges[filter->range_set_idx].end;
 
-	if (!filter->accumulating_hunk.active) {
-		filter->accumulating_hunk.active = 1;
-		filter->accumulating_hunk.has_changes = 0;
-		filter->accumulating_hunk.new_begin = idx_in_postimage + 1;
-		filter->accumulating_hunk.old_begin = filter->pending_rm_count
-			? filter->pending_rm_pre_begin : cur_pre;
-		filter->accumulating_hunk.old_count = 0;
-		filter->accumulating_hunk.new_count = 0;
-		strbuf_reset(&filter->accumulating_hunk.lines);
-	}
+	if (in_range) {
+		if (!filter->accumulating_hunk.active)
+			begin_range_hunk(filter);
 
-	if (filter->pending_rm_count) {
-		strbuf_addbuf(&filter->accumulating_hunk.lines, &filter->pending_rm);
-		filter->accumulating_hunk.old_count += filter->pending_rm_count;
-		filter->accumulating_hunk.has_changes = 1;
-		discard_pending_rm(filter);
+		strbuf_add(&filter->accumulating_hunk.lines, line, len);
 	}
 
-	strbuf_add(&filter->accumulating_hunk.lines, line, len);
-	filter->accumulating_hunk.new_count++;
-	if (line[0] == '+')
-		filter->accumulating_hunk.has_changes = 1;
-	else
-		filter->accumulating_hunk.old_count++;
+	if (line[0] == ' ' || line[0] == '+')
+		filter->accumulating_hunk.lno_in_postimage++;
+	if (line[0] == ' ' || line[0] == '-')
+		filter->accumulating_hunk.lno_in_preimage++;
 
 	return filter->ret;
 }
@@ -4033,7 +4008,6 @@ static void builtin_diff(const char *name_a,
 			lr_state.orig_cb_data = &ecbdata;
 			lr_state.range_sets_to_filter_by = line_ranges;
 			strbuf_init(&lr_state.accumulating_hunk.lines, 0);
-			strbuf_init(&lr_state.pending_rm, 0);
 
 			/*
 			 * Inflate ctxlen so that all changes within
@@ -4068,7 +4042,6 @@ static void builtin_diff(const char *name_a,
 				die("unable to generate diff for %s",
 				    one->path);
 			strbuf_release(&lr_state.accumulating_hunk.lines);
-			strbuf_release(&lr_state.pending_rm);
 		} else if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume,
 					 &ecbdata, &xpp, &xecfg))
 			die("unable to generate diff for %s", one->path);
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index d0a834ed8f..233dc232e3 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -738,6 +738,37 @@ test_expect_success '-L with -G filters to diff-text matches' '
 	test_grep "F2 + 2" actual
 '
 
+test_expect_success 'setup for trailing deletion test' '
+	git checkout --orphan trailing-del &&
+	git reset --hard &&
+	cat >file.c <<-\EOF &&
+	void tracked()
+	{
+	    return 1;
+	}
+	// trailing comment outside tracked range
+	EOF
+	git add file.c &&
+	test_tick &&
+	git commit -m "add file with trailing comment" &&
+	# Remove the trailing comment AND modify tracked() so there
+	# is a modification to the line range we track and a
+	# modification to the following line, which we do not track.
+	cat >file.c <<-\EOF &&
+	void tracked()
+	{
+	    return 2;
+	}
+	EOF
+	git commit -a -m "modify tracked and delete trailing comment"
+'
+
+test_expect_success '-L does not include deletions past end of tracked range' '
+	git log -L:tracked:file.c --format= -1 -p >actual &&
+	test_grep "return 2" actual &&
+	test_grep ! "trailing comment" actual
+'
+
 test_expect_success '-L with --diff-filter=M excludes root commit' '
 	git checkout parent-oids &&
 	git log -L:func2:file.c --diff-filter=M --format=%s --no-patch >actual &&
-- 
gitgitgadget