{"thread":{"id":"65554","subject":"[PATCH] diff: simplify line-range filter by classifying removals immediately","startedAt":"2026-04-26T19:10:32Z","lastAt":"2026-04-26T19:10:32Z","messageCount":1,"participants":["Michael Montalbo via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"542334","messageId":"pull.2099.git.1777230630020.gitgitgadget@gmail.com","threadId":"65554","inReplyTo":null,"subject":"[PATCH] diff: simplify line-range filter by classifying removals immediately","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-26T19:10:29Z","receivedAt":"2026-04-26T19:10:32Z","isPatch":true,"body":"From: Michael Montalbo <mmontalbo@gmail.com>\n\nThe line-range filter buffered '-' (removal) lines in a pending_rm\nstrbuf, deferring their classification until a '+' or ' ' line\narrived to reveal the post-image position.\n\nThis buffering is unnecessary.  Removal lines are pre-image content\nthat occupies no post-image space, so they do not advance lno_post.\nWithin a hunk, xdiff emits removals before additions for each\nchange, so a '-' line always arrives while lno_post is at the same\nposition that the following '+' or ' ' line will occupy.  Each\nline can therefore be classified against the tracked ranges as it\narrives, without waiting for a non-removal line to confirm the\nposition.\n\nThe buffering also had a bug: flush_rhunk() unconditionally drained\nthe pending buffer when the range hunk was active, even if lno_post\nhad advanced past the tracked range.  This caused deletions\nimmediately after the tracked function to be incorrectly included\nin patch output.\n\nRemove the pending_rm buffer and classify '-' lines using the same\nin_range check applied to '+' and ' ' lines.  This simplifies the\nfilter, removes three struct fields and a helper function, and\nmakes the flush_rhunk() bug impossible by construction.\n\nSigned-off-by: Michael Montalbo <mmontalbo@gmail.com>\n---\n    diff: simplify line-range filter by classifying removals immediately\n    \n    The line-range filter buffered '-' (removal) lines in a\n    pending_rm strbuf, deferring their classification until a '+'\n    or ' ' line arrived to reveal the post-image position.\n    \n    This buffering is unnecessary. Removal lines are pre-image\n    content that occupies no post-image space, so they do not\n    advance lno_post. Within a hunk, xdiff emits removals before additions\n    for each change, so a '-' line always arrives while\n    lno_post is at the same position that the following '+' or ' ' line will\n    occupy. Each line can therefore be classified against\n    the tracked ranges as it arrives, without waiting for a\n    non-removal line to confirm the position.\n    \n    The buffering also had a bug: flush_rhunk() unconditionally\n    drained the pending buffer when the range hunk was active, even\n    if lno_post had advanced past the tracked range. This caused deletions\n    immediately after the tracked function to be\n    incorrectly included in patch output.\n    \n    Remove the pending_rm buffer and classify '-' lines using the\n    same in_range check applied to '+' and ' ' lines. This simplifies the\n    filter, removes three struct fields and a helper function, and makes the\n    flush_rhunk() bug impossible by\n    construction.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2099%2Fmmontalbo%2Fmm%2Fline-log-immediate-classify-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2099/mmontalbo/mm/line-log-immediate-classify-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2099\n\n diff.c              | 110 ++++++++++++---------------------------\n t/t4211-line-log.sh | 124 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 157 insertions(+), 77 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 397e38b41c..5662d080be 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -616,11 +616,6 @@ struct emit_callback {\n  * the requested ranges.  Contiguous in-range lines are collected into\n  * range hunks and flushed with a synthetic @@ header so that\n  * fn_out_consume() sees well-formed unified-diff fragments.\n- *\n- * Removal lines ('-') cannot be classified by post-image position, so\n- * they are buffered in pending_rm until the next '+' or ' ' line\n- * reveals whether they precede an in-range line (flush into range hunk) or\n- * an out-of-range line (discard).\n  */\n struct line_range_callback {\n \txdiff_emit_line_fn orig_line_fn;\n@@ -646,11 +641,6 @@ struct line_range_callback {\n \tint rhunk_active;\n \tint rhunk_has_changes;\t\t/* any '+' or '-' lines? */\n \n-\t/* Removal lines not yet known to be in-range */\n-\tstruct strbuf pending_rm;\n-\tint pending_rm_count;\n-\tlong pending_rm_pre_begin;\t/* pre-image line of first pending */\n-\n \tint ret;\t\t\t/* latched error from orig_line_fn */\n };\n \n@@ -2539,12 +2529,6 @@ static int quick_consume(void *priv, char *line UNUSED, unsigned long len UNUSED\n \treturn 1;\n }\n \n-static void discard_pending_rm(struct line_range_callback *s)\n-{\n-\tstrbuf_reset(&s->pending_rm);\n-\ts->pending_rm_count = 0;\n-}\n-\n static void flush_rhunk(struct line_range_callback *s)\n {\n \tstruct strbuf hdr = STRBUF_INIT;\n@@ -2553,14 +2537,6 @@ static void flush_rhunk(struct line_range_callback *s)\n \tif (!s->rhunk_active || s->ret)\n \t\treturn;\n \n-\t/* Drain any pending removal lines into the range hunk */\n-\tif (s->pending_rm_count) {\n-\t\tstrbuf_addbuf(&s->rhunk, &s->pending_rm);\n-\t\ts->rhunk_old_count += s->pending_rm_count;\n-\t\ts->rhunk_has_changes = 1;\n-\t\tdiscard_pending_rm(s);\n-\t}\n-\n \t/*\n \t * Suppress context-only hunks: they contain no actual changes\n \t * and would just be noise.  This can happen when the inflated\n@@ -2615,11 +2591,6 @@ static void line_range_hunk_fn(void *data,\n \t * When count > 0, begin is 1-based.  When count == 0, begin is\n \t * adjusted down by 1 by xdl_emit_hunk_hdr(), but no lines of\n \t * that type will arrive, so the value is unused.\n-\t *\n-\t * Any pending removal lines from the previous xdiff hunk are\n-\t * intentionally left in pending_rm: the line callback will\n-\t * flush or discard them when the next content line reveals\n-\t * whether the removals precede in-range content.\n \t */\n \ts->lno_post = new_begin;\n \ts->lno_pre = old_begin;\n@@ -2635,88 +2606,75 @@ static void line_range_hunk_fn(void *data,\n static int line_range_line_fn(void *priv, char *line, unsigned long len)\n {\n \tstruct line_range_callback *s = priv;\n-\tconst struct range *cur;\n-\tlong lno_0, cur_pre;\n+\tlong lno_0;\n+\tint in_range;\n \n \tif (s->ret)\n \t\treturn s->ret;\n \n-\tif (line[0] == '-') {\n-\t\tif (!s->pending_rm_count)\n-\t\t\ts->pending_rm_pre_begin = s->lno_pre;\n-\t\ts->lno_pre++;\n-\t\tstrbuf_add(&s->pending_rm, line, len);\n-\t\ts->pending_rm_count++;\n-\t\treturn s->ret;\n-\t}\n-\n \tif (line[0] == '\\\\') {\n-\t\tif (s->pending_rm_count)\n-\t\t\tstrbuf_add(&s->pending_rm, line, len);\n-\t\telse if (s->rhunk_active)\n+\t\tif (s->rhunk_active)\n \t\t\tstrbuf_add(&s->rhunk, line, len);\n-\t\t/* otherwise outside tracked range; drop silently */\n \t\treturn s->ret;\n \t}\n \n-\tif (line[0] != '+' && line[0] != ' ')\n+\tif (line[0] != '+' && line[0] != ' ' && line[0] != '-')\n \t\tBUG(\"unexpected diff line type '%c'\", line[0]);\n \n+\t/*\n+\t * Compute post-image position.  '+' and ' ' lines advance\n+\t * lno_post; '-' lines do not (they occupy no post-image space).\n+\t */\n \tlno_0 = s->lno_post - 1;\n-\tcur_pre = s->lno_pre;\t/* save before advancing for context lines */\n-\ts->lno_post++;\n-\tif (line[0] == ' ')\n-\t\ts->lno_pre++;\n+\tif (line[0] != '-')\n+\t\ts->lno_post++;\n \n-\t/* Advance past ranges we've passed */\n+\t/*\n+\t * Advance past any ranges we've moved beyond.  Emit the\n+\t * accumulated range hunk for the range we're leaving.\n+\t */\n \twhile (s->cur_range < s->ranges->nr &&\n \t       lno_0 >= s->ranges->ranges[s->cur_range].end) {\n \t\tif (s->rhunk_active)\n \t\t\tflush_rhunk(s);\n-\t\tdiscard_pending_rm(s);\n \t\ts->cur_range++;\n \t}\n \n-\t/* Past all ranges */\n-\tif (s->cur_range >= s->ranges->nr) {\n-\t\tdiscard_pending_rm(s);\n-\t\treturn s->ret;\n-\t}\n-\n-\tcur = &s->ranges->ranges[s->cur_range];\n+\tin_range = s->cur_range < s->ranges->nr &&\n+\t\t   lno_0 >= s->ranges->ranges[s->cur_range].start &&\n+\t\t   lno_0 < s->ranges->ranges[s->cur_range].end;\n \n-\t/* Before current range */\n-\tif (lno_0 < cur->start) {\n-\t\tdiscard_pending_rm(s);\n+\tif (!in_range) {\n+\t\tif (line[0] != '+')\n+\t\t\ts->lno_pre++;\n \t\treturn s->ret;\n \t}\n \n-\t/* In range so start a new range hunk if needed */\n+\t/* Start a new range hunk if this is the first in-range line */\n \tif (!s->rhunk_active) {\n \t\ts->rhunk_active = 1;\n \t\ts->rhunk_has_changes = 0;\n \t\ts->rhunk_new_begin = lno_0 + 1;\n-\t\ts->rhunk_old_begin = s->pending_rm_count\n-\t\t\t? s->pending_rm_pre_begin : cur_pre;\n+\t\ts->rhunk_old_begin = s->lno_pre;\n \t\ts->rhunk_old_count = 0;\n \t\ts->rhunk_new_count = 0;\n \t\tstrbuf_reset(&s->rhunk);\n \t}\n \n-\t/* Flush pending removals into range hunk */\n-\tif (s->pending_rm_count) {\n-\t\tstrbuf_addbuf(&s->rhunk, &s->pending_rm);\n-\t\ts->rhunk_old_count += s->pending_rm_count;\n-\t\ts->rhunk_has_changes = 1;\n-\t\tdiscard_pending_rm(s);\n-\t}\n-\n+\t/* Append line to the range hunk */\n \tstrbuf_add(&s->rhunk, line, len);\n-\ts->rhunk_new_count++;\n-\tif (line[0] == '+')\n+\tif (line[0] == '-') {\n+\t\ts->rhunk_old_count++;\n \t\ts->rhunk_has_changes = 1;\n-\telse\n+\t\ts->lno_pre++;\n+\t} else if (line[0] == '+') {\n+\t\ts->rhunk_new_count++;\n+\t\ts->rhunk_has_changes = 1;\n+\t} else {\n \t\ts->rhunk_old_count++;\n+\t\ts->rhunk_new_count++;\n+\t\ts->lno_pre++;\n+\t}\n \n \treturn s->ret;\n }\n@@ -4072,7 +4030,6 @@ static void builtin_diff(const char *name_a,\n \t\t\tlr_state.orig_cb_data = &ecbdata;\n \t\t\tlr_state.ranges = line_ranges;\n \t\t\tstrbuf_init(&lr_state.rhunk, 0);\n-\t\t\tstrbuf_init(&lr_state.pending_rm, 0);\n \n \t\t\t/*\n \t\t\t * Inflate ctxlen so that all changes within\n@@ -4107,7 +4064,6 @@ static void builtin_diff(const char *name_a,\n \t\t\t\tdie(\"unable to generate diff for %s\",\n \t\t\t\t    one->path);\n \t\t\tstrbuf_release(&lr_state.rhunk);\n-\t\t\tstrbuf_release(&lr_state.pending_rm);\n \t\t} else if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume,\n \t\t\t\t\t &ecbdata, &xpp, &xecfg))\n \t\t\tdie(\"unable to generate diff for %s\", one->path);\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex aaf197d2ed..2bff0e4c26 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -711,4 +711,128 @@ test_expect_success '-L with -G filters to diff-text matches' '\n \tgrep \"F2 + 2\" actual\n '\n \n+test_expect_success 'setup for trailing deletion test' '\n+\tgit checkout --orphan trailing-del &&\n+\tgit reset --hard &&\n+\tcat >file.c <<-\\EOF &&\n+\tvoid tracked()\n+\t{\n+\t    return 1;\n+\t}\n+\t// trailing comment\n+\tEOF\n+\tgit add file.c &&\n+\ttest_tick &&\n+\tgit commit -m \"add file with trailing comment\" &&\n+\t# Modify tracked() AND delete the trailing comment in\n+\t# one commit, so the commit touches the tracked range\n+\t# and is not filtered out by the revision walker.\n+\tcat >file.c <<-\\EOF &&\n+\tvoid tracked()\n+\t{\n+\t    return 2;\n+\t}\n+\tEOF\n+\tgit commit -a -m \"modify tracked and delete trailing comment\"\n+'\n+\n+test_expect_success '-L does not include deletions past end of tracked range' '\n+\tgit log -L:tracked:file.c --format= -1 -p >actual &&\n+\t# The trailing comment deletion is outside the tracked\n+\t# range and should not appear in the patch output.\n+\tgrep \"return 2\" actual &&\n+\t! grep \"trailing comment\" actual\n+'\n+\n+test_expect_success '-L includes leading deletions resolved by in-range line' '\n+\tgit checkout --orphan leading-del &&\n+\tgit reset --hard &&\n+\tcat >file.c <<-\\EOF &&\n+\t// leading comment\n+\tvoid tracked()\n+\t{\n+\t    return 1;\n+\t}\n+\tEOF\n+\tgit add file.c &&\n+\ttest_tick &&\n+\tgit commit -m \"add file with leading comment\" &&\n+\tcat >file.c <<-\\EOF &&\n+\tvoid tracked()\n+\t{\n+\t    return 2;\n+\t}\n+\tEOF\n+\tgit commit -a -m \"modify tracked and delete leading comment\" &&\n+\tgit log -L:tracked:file.c --format= -1 -p >actual &&\n+\t# The leading comment deletion is resolved by the next\n+\t# non-removal line (void tracked), which is in range.\n+\t# Pending removals are attributed to the range they precede.\n+\tgrep \"return 2\" actual &&\n+\tgrep \"leading comment\" actual\n+'\n+\n+test_expect_success 'setup for line-range filter edge cases' '\n+\tgit checkout --orphan filter-edge &&\n+\tgit reset --hard &&\n+\tcat >file.c <<-\\EOF &&\n+\tvoid before()\n+\t{\n+\t    return 0;\n+\t}\n+\n+\tvoid tracked()\n+\t{\n+\t    int a = 1;\n+\t    int b = 2;\n+\t    int c = 3;\n+\t    return a + b + c;\n+\t}\n+\n+\tvoid after()\n+\t{\n+\t    return 9;\n+\t}\n+\tEOF\n+\tgit add file.c &&\n+\ttest_tick &&\n+\tgit commit -m \"initial\"\n+'\n+\n+test_expect_success '-L change at exact first line of range' '\n+\tgit checkout filter-edge &&\n+\t# Change the function signature (first line of range)\n+\tsed \"s/void tracked/int tracked/\" file.c >tmp &&\n+\tmv tmp file.c &&\n+\tgit commit -a -m \"change first line\" &&\n+\tgit log -L:tracked:file.c -p --format=%s -1 >actual &&\n+\tgrep \"change first line\" actual &&\n+\tgrep \"+int tracked\" actual &&\n+\tgrep \"\\\\-void tracked\" actual\n+'\n+\n+test_expect_success '-L change at exact last line of range' '\n+\tgit checkout filter-edge &&\n+\tgit reset --hard HEAD~1 &&\n+\t# Change the closing brace line (last line of range)\n+\tsed \"s/^}$/} \\/\\/ end tracked/\" file.c >tmp &&\n+\tmv tmp file.c &&\n+\tgit commit -a -m \"change last line\" &&\n+\tgit log -L:tracked:file.c -p --format=%s -1 >actual &&\n+\tgrep \"change last line\" actual &&\n+\tgrep \"end tracked\" actual\n+'\n+\n+test_expect_success '-L pure deletion in range (no additions)' '\n+\tgit checkout filter-edge &&\n+\tgit reset --hard HEAD~1 &&\n+\t# Delete a line inside tracked() without adding anything\n+\tsed \"/int c/d\" file.c >tmp &&\n+\tmv tmp file.c &&\n+\tgit commit -a -m \"pure deletion\" &&\n+\tgit log -L:tracked:file.c -p --format=%s -1 >actual &&\n+\tgrep \"pure deletion\" actual &&\n+\tgrep \"\\\\-.*int c\" actual\n+'\n+\n test_done\n\nbase-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199\n-- \ngitgitgadget\n"}]}