diff: simplify the line-range filter by classifying removals immediately
The filter buffered '-' lines in a pending_rm strbuf, deferring their classification until a '+' or ' ' line revealed the post-image position. That buffering is unnecessary: a removal occupies no post-image line, so it does not advance lno_in_postimage, and xdiff emits removals before additions within a change. A '-' therefore arrives while lno_in_postimage already holds the index the following '+'/' ' will occupy, and can be classified against the ranges as it arrives. The buffering also hid a bug: flush_range_hunk() drained pending_rm into the range hunk whenever the hunk was active, even after lno_in_postimage had advanced past the tracked range, so a deletion just after the tracked function leaked into the patch. Classifying each line as it arrives removes the pending_rm buffer, the discard_pending_rm() helper, three struct fields, and makes that bug impossible by construction. With every line classified on arrival, the buffered lines are the hunk's single source of truth, so the old/new counts need not be kept alongside them: flush_range_hunk() derives the counts (and whether the hunk holds any change) from the buffer when it builds the header. Drop the per-line counting and the old_count, new_count, and has_changes fields; there is no longer a second tally that could fall out of sync with the buffer. Add begin_range_hunk() to open the accumulator at the first in-range line, seeding both begins from the live image cursors, as the counterpart to flush_range_hunk(). With the counting gone too, line_range_line_fn() now only appends an in-range line. Document the coordinate model: a block comment on struct line_range_filter states it (the pre/post-image cursors, the 0-based idx_in_postimage, removals classified by the following line) with a worked example. Add tests for the leaked trailing deletion this fixes, the symmetric leading-deletion case, and the filter's range boundaries (a change at the first and last line of a range, and a pure in-range deletion). Signed-off-by: Michael Montalbo <mmontalbo@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Michael Montalbo committed
Jun 27, 2026 at 17:28 UTC
5a508c13ac159a6c8e938ba6ce544a33f234e58b
2 files changed
+243
-97
diff.c
+118
-97
@@ -610,18 +610,58 @@ struct emit_callback {
610
};
611
612
/*
613
- * State for the line-range callback wrappers that sit between
614
- * xdi_diff_outf() and fn_out_consume(). xdiff produces a normal,
615
- * unfiltered diff; the wrappers intercept each hunk header and line,
616
- * track post-image position, and forward only lines that fall within
617
- * the requested ranges. Contiguous in-range lines are collected into
618
- * range hunks and flushed with a synthetic @@ header so that
619
- * fn_out_consume() sees well-formed unified-diff fragments.
613
+ * Line-range filter: scopes "git log -L" output to the tracked ranges.
614
*
621
- * Removal lines ('-') cannot be classified by post-image position, so
622
- * they are buffered in pending_rm until the next '+' or ' ' line
623
- * reveals whether they precede an in-range line (flush into range hunk) or
624
- * an out-of-range line (discard).
615
+ * It sits between xdi_diff_outf() and an output callback (fn_out_consume,
616
+ * diffstat_consume, checkdiff_consume). xdiff produces a normal diff; the
617
+ * filter forwards only the lines inside the requested ranges, collecting
618
+ * contiguous in-range lines into a "range hunk" emitted with a synthetic
619
+ * @@ header so the callback sees well-formed unified-diff fragments.
620
+ *
621
+ * A diff describes the change from a pre-image to a post-image. Each
622
+ * line is context (' ', in both), a removal ('-', pre-image only), or
623
+ * an addition ('+', post-image only). -L tracks ranges in the
624
+ * post-image, so a line is in range by its post-image position.
625
+ *
626
+ * Two 1-based cursors track the next line in each image, named as in
627
+ * struct emit_callback and seeded from the xdiff hunk header:
628
+ *
629
+ * lno_in_postimage advances on '+' and ' ' (lines in the post-image)
630
+ * lno_in_preimage advances on '-' and ' ' (lines in the pre-image)
631
+ *
632
+ * Ranges are 0-based half-open [start, end), so a line is tested at the
633
+ * 0-based index idx_in_postimage = lno_in_postimage - 1.
634
+ *
635
+ * A '-' is not present in the post-image, so it has no post-image line
636
+ * number of its own. Since it does not advance lno_in_postimage, it is
637
+ * classified at the idx_in_postimage that the following '+'/' ' will
638
+ * occupy. xdiff emits a change's removals before its additions, so that
639
+ * index is already known when the '-' arrives.
640
+ *
641
+ * The synthetic "@@ -<old> +<new> @@" header has two sides, old (the
642
+ * pre-image) and new (the post-image), matching the xdiff_emit_hunk_fn
643
+ * callback; the hunk.old_begin / hunk.new_begin fields below hold those
644
+ * begins, and flush_range_hunk() derives the counts from the buffered
645
+ * lines.
646
+ *
647
+ * Example, tracking post-image line 2 (range [1, 2)) of:
648
+ *
649
+ * pre-image post-image
650
+ * 1 a 1 a
651
+ * 2 b 2 X (b -> X)
652
+ * 3 c 3 c
653
+ *
654
+ * classify each line by idx_in_postimage. The pre and post columns
655
+ * are each cursor's value while that line is classified, i.e. before
656
+ * the line advances them (pre = lno_in_preimage,
657
+ * post = lno_in_postimage, idx = idx_in_postimage):
658
+ * ' a' pre 1 post 1 idx 0 -> before start, skip
659
+ * '-b' pre 2 post 2 idx 1 -> keep (removal)
660
+ * '+X' pre 3 post 2 idx 1 -> keep (addition)
661
+ * ' c' pre 3 post 3 idx 2 -> past end, flush
662
+ *
663
+ * -b and +X share idx = 1 because -b did not advance lno_in_postimage;
664
+ * both land in the range hunk, flushed when ' c' crosses the range end.
665
*/
666
struct line_range_filter {
667
xdiff_emit_line_fn orig_line_fn;
@@ -640,20 +680,18 @@ struct line_range_filter {
680
char func[80];
681
long funclen;
682
643
- /* The range hunk being accumulated for the current range. */
683
+ /*
684
+ * The range hunk being accumulated. At most one is live at a time:
685
+ * it is flushed and reset as the cursor leaves each range (and once
686
+ * more at end of diff), then reused for the next range.
687
+ */
688
struct {
689
struct strbuf lines; /* buffered in-range diff lines */
646
- long old_begin, old_count;
647
- long new_begin, new_count;
690
+ long old_begin;
691
+ long new_begin;
692
int active;
649
- int has_changes; /* any '+' or '-' line? */
693
} hunk;
694
652
- /* Removal lines not yet known to be in-range */
653
- struct strbuf pending_rm;
654
- int pending_rm_count;
655
- long pending_rm_pre_begin; /* pre-image line of first pending */
656
-
695
int ret; /* latched error from orig_line_fn */
696
};
697
@@ -2542,26 +2580,48 @@ static int quick_consume(void *priv, char *line UNUSED, unsigned long len UNUSED
2580
return 1;
2581
}
2582
2545
-static void discard_pending_rm(struct line_range_filter *filter)
2583
+/*
2584
+ * Begin a range hunk at the first in-range line. Its position fixes the
2585
+ * hunk's begins, taken from the two image cursors before they advance:
2586
+ * new_begin from the post-image, old_begin from the pre-image. The line
2587
+ * counts are not tracked here; flush_range_hunk() derives them from the
2588
+ * buffered lines.
2589
+ */
2590
+static void begin_range_hunk(struct line_range_filter *filter)
2591
{
2547
- strbuf_reset(&filter->pending_rm);
2548
- filter->pending_rm_count = 0;
2592
+ filter->hunk.active = 1;
2593
+ filter->hunk.new_begin = filter->lno_in_postimage;
2594
+ filter->hunk.old_begin = filter->lno_in_preimage;
2595
+ strbuf_reset(&filter->hunk.lines);
2596
}
2597
2598
static void flush_range_hunk(struct line_range_filter *filter)
2599
{
2600
struct strbuf hdr = STRBUF_INIT;
2601
const char *p, *end;
2602
+ long old_count = 0, new_count = 0;
2603
+ int has_changes = 0;
2604
2605
if (!filter->hunk.active || filter->ret)
2606
return;
2607
2559
- /* Drain any pending removal lines into the range hunk */
2560
- if (filter->pending_rm_count) {
2561
- strbuf_addbuf(&filter->hunk.lines, &filter->pending_rm);
2562
- filter->hunk.old_count += filter->pending_rm_count;
2563
- filter->hunk.has_changes = 1;
2564
- discard_pending_rm(filter);
2608
+ /*
2609
+ * Derive the hunk's geometry from the buffered lines: a ' '
2610
+ * counts on both sides, a '-' on the old side, a '+' on the new.
2611
+ * A '-' or '+' marks a real change; the "\ No newline at end of
2612
+ * file" marker (line[0] == '\\') counts on neither side.
2613
+ */
2614
+ p = filter->hunk.lines.buf;
2615
+ end = p + filter->hunk.lines.len;
2616
+ while (p < end) {
2617
+ const char *eol = memchr(p, '\n', end - p);
2618
+ if (*p == ' ' || *p == '-')
2619
+ old_count++;
2620
+ if (*p == ' ' || *p == '+')
2621
+ new_count++;
2622
+ if (*p == '-' || *p == '+')
2623
+ has_changes = 1;
2624
+ p = eol ? eol + 1 : end;
2625
}
2626
2627
/*
@@ -2570,15 +2630,15 @@ static void flush_range_hunk(struct line_range_filter *filter)
2630
* ctxlen causes xdiff to emit context covering a range that
2631
* has no changes in this commit.
2632
*/
2573
- if (!filter->hunk.has_changes) {
2633
+ if (!has_changes) {
2634
filter->hunk.active = 0;
2635
strbuf_reset(&filter->hunk.lines);
2636
return;
2637
}
2638
2639
strbuf_addf(&hdr, "@@ -%ld,%ld +%ld,%ld @@",
2580
- filter->hunk.old_begin, filter->hunk.old_count,
2581
- filter->hunk.new_begin, filter->hunk.new_count);
2640
+ filter->hunk.old_begin, old_count,
2641
+ filter->hunk.new_begin, new_count);
2642
if (filter->funclen > 0) {
2643
strbuf_addch(&hdr, ' ');
2644
strbuf_add(&hdr, filter->func, filter->funclen);
@@ -2618,11 +2678,6 @@ static void line_range_hunk_fn(void *data,
2678
* When count > 0, begin is 1-based. When count == 0, begin is
2679
* adjusted down by 1 by xdl_emit_hunk_hdr(), but no lines of
2680
* that type will arrive, so the value is unused.
2621
- *
2622
- * Any pending removal lines from the previous xdiff hunk are
2623
- * intentionally left in pending_rm: the line callback will
2624
- * flush or discard them when the next content line reveals
2625
- * whether the removals precede in-range content.
2681
*/
2682
filter->lno_in_postimage = new_begin;
2683
filter->lno_in_preimage = old_begin;
@@ -2638,88 +2693,56 @@ static void line_range_hunk_fn(void *data,
2693
static int line_range_line_fn(void *priv, char *line, unsigned long len)
2694
{
2695
struct line_range_filter *filter = priv;
2641
- const struct range *cur;
2642
- long idx_in_postimage, cur_pre;
2696
+ long idx_in_postimage;
2697
+ int in_range;
2698
2699
if (filter->ret)
2700
return filter->ret;
2701
2647
- if (line[0] == '-') {
2648
- if (!filter->pending_rm_count)
2649
- filter->pending_rm_pre_begin = filter->lno_in_preimage;
2650
- filter->lno_in_preimage++;
2651
- strbuf_add(&filter->pending_rm, line, len);
2652
- filter->pending_rm_count++;
2653
- return filter->ret;
2654
- }
2655
-
2702
if (line[0] == '\\') {
2657
- if (filter->pending_rm_count)
2658
- strbuf_add(&filter->pending_rm, line, len);
2659
- else if (filter->hunk.active)
2703
+ if (filter->hunk.active)
2704
strbuf_add(&filter->hunk.lines, line, len);
2661
- /* otherwise outside tracked range; drop silently */
2705
return filter->ret;
2706
}
2707
2665
- if (line[0] != '+' && line[0] != ' ')
2708
+ if (line[0] != '+' && line[0] != ' ' && line[0] != '-')
2709
BUG("unexpected diff line type '%c'", line[0]);
2710
2711
+ /*
2712
+ * idx_in_postimage is this line's 0-based post-image index (see the model on
2713
+ * struct line_range_filter). The cursors are advanced only after
2714
+ * the line is classified, so a '-' is tested at the same idx_in_postimage as
2715
+ * the '+'/' ' that follows it.
2716
+ */
2717
idx_in_postimage = filter->lno_in_postimage - 1;
2669
- cur_pre = filter->lno_in_preimage; /* save before advancing for context lines */
2670
- filter->lno_in_postimage++;
2671
- if (line[0] == ' ')
2672
- filter->lno_in_preimage++;
2718
2674
- /* Advance past ranges we've passed */
2719
+ /* Retire ranges we have passed, flushing the one we leave. */
2720
while (filter->cur_range < filter->ranges->nr &&
2721
idx_in_postimage >= filter->ranges->ranges[filter->cur_range].end) {
2722
if (filter->hunk.active)
2723
flush_range_hunk(filter);
2679
- discard_pending_rm(filter);
2724
filter->cur_range++;
2725
}
2726
2683
- /* Past all ranges */
2684
- if (filter->cur_range >= filter->ranges->nr) {
2685
- discard_pending_rm(filter);
2686
- return filter->ret;
2687
- }
2727
+ in_range = filter->cur_range < filter->ranges->nr &&
2728
+ idx_in_postimage >= filter->ranges->ranges[filter->cur_range].start &&
2729
+ idx_in_postimage < filter->ranges->ranges[filter->cur_range].end;
2730
2689
- cur = &filter->ranges->ranges[filter->cur_range];
2731
+ if (in_range) {
2732
+ if (!filter->hunk.active)
2733
+ begin_range_hunk(filter);
2734
2691
- /* Before current range */
2692
- if (idx_in_postimage < cur->start) {
2693
- discard_pending_rm(filter);
2694
- return filter->ret;
2735
+ strbuf_add(&filter->hunk.lines, line, len);
2736
}
2737
2697
- /* In range so start a new range hunk if needed */
2698
- if (!filter->hunk.active) {
2699
- filter->hunk.active = 1;
2700
- filter->hunk.has_changes = 0;
2701
- filter->hunk.new_begin = idx_in_postimage + 1;
2702
- filter->hunk.old_begin = filter->pending_rm_count
2703
- ? filter->pending_rm_pre_begin : cur_pre;
2704
- filter->hunk.old_count = 0;
2705
- filter->hunk.new_count = 0;
2706
- strbuf_reset(&filter->hunk.lines);
2707
- }
2708
-
2709
- /* Flush pending removals into range hunk */
2710
- if (filter->pending_rm_count) {
2711
- strbuf_addbuf(&filter->hunk.lines, &filter->pending_rm);
2712
- filter->hunk.old_count += filter->pending_rm_count;
2713
- filter->hunk.has_changes = 1;
2714
- discard_pending_rm(filter);
2715
- }
2716
-
2717
- strbuf_add(&filter->hunk.lines, line, len);
2718
- filter->hunk.new_count++;
2719
- if (line[0] == '+')
2720
- filter->hunk.has_changes = 1;
2721
- else
2722
- filter->hunk.old_count++;
2738
+ /*
2739
+ * Advance each image's cursor: a line present in that image (see
2740
+ * the model) consumes one of its line numbers.
2741
+ */
2742
+ if (line[0] != '-')
2743
+ filter->lno_in_postimage++;
2744
+ if (line[0] != '+')
2745
+ filter->lno_in_preimage++;
2746
2747
return filter->ret;
2748
}
@@ -4097,7 +4120,6 @@ static void builtin_diff(const char *name_a,
4120
lr_state.orig_cb_data = &ecbdata;
4121
lr_state.ranges = line_ranges;
4122
strbuf_init(&lr_state.hunk.lines, 0);
4100
- strbuf_init(&lr_state.pending_rm, 0);
4123
4124
/*
4125
* Inflate ctxlen so that all changes within
@@ -4132,7 +4154,6 @@ static void builtin_diff(const char *name_a,
4154
die("unable to generate diff for %s",
4155
one->path);
4156
strbuf_release(&lr_state.hunk.lines);
4135
- strbuf_release(&lr_state.pending_rm);
4157
} else if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume,
4158
&ecbdata, &xpp, &xecfg))
4159
die("unable to generate diff for %s", one->path);
t/t4211-line-log.sh
+125
@@ -738,6 +738,131 @@ test_expect_success '-L with -G filters to diff-text matches' '
738
grep "F2 + 2" actual
739
'
740
741
+test_expect_success 'setup for trailing deletion test' '
742
+ git checkout --orphan trailing-del &&
743
+ git reset --hard &&
744
+ cat >file.c <<-\EOF &&
745
+ void tracked()
746
+ {
747
+ return 1;
748
+ }
749
+ // trailing comment
750
+ EOF
751
+ git add file.c &&
752
+ test_tick &&
753
+ git commit -m "add file with trailing comment" &&
754
+ # Modify tracked() AND delete the trailing comment in
755
+ # one commit, so the commit touches the tracked range
756
+ # and is not filtered out by the revision walker.
757
+ cat >file.c <<-\EOF &&
758
+ void tracked()
759
+ {
760
+ return 2;
761
+ }
762
+ EOF
763
+ git commit -a -m "modify tracked and delete trailing comment"
764
+'
765
+
766
+test_expect_success '-L does not include deletions past end of tracked range' '
767
+ git log -L:tracked:file.c --format= -1 -p >actual &&
768
+ # The trailing comment deletion is outside the tracked
769
+ # range and should not appear in the patch output.
770
+ test_grep "return 2" actual &&
771
+ test_grep ! "trailing comment" actual
772
+'
773
+
774
+test_expect_success '-L includes leading deletions resolved by in-range line' '
775
+ git checkout --orphan leading-del &&
776
+ git reset --hard &&
777
+ cat >file.c <<-\EOF &&
778
+ // leading comment
779
+ void tracked()
780
+ {
781
+ return 1;
782
+ }
783
+ EOF
784
+ git add file.c &&
785
+ test_tick &&
786
+ git commit -m "add file with leading comment" &&
787
+ cat >file.c <<-\EOF &&
788
+ void tracked()
789
+ {
790
+ return 2;
791
+ }
792
+ EOF
793
+ git commit -a -m "modify tracked and delete leading comment" &&
794
+ git log -L:tracked:file.c --format= -1 -p >actual &&
795
+ # The leading comment deletion is resolved by the next
796
+ # non-removal line (void tracked), which is in range: a
797
+ # removal is classified by the position of the following
798
+ # line, so it joins the range that line falls in.
799
+ test_grep "return 2" actual &&
800
+ test_grep "leading comment" actual
801
+'
802
+
803
+test_expect_success 'setup for line-range filter edge cases' '
804
+ git checkout --orphan filter-edge &&
805
+ git reset --hard &&
806
+ cat >file.c <<-\EOF &&
807
+ void before()
808
+ {
809
+ return 0;
810
+ }
811
+
812
+ void tracked()
813
+ {
814
+ int a = 1;
815
+ int b = 2;
816
+ int c = 3;
817
+ return a + b + c;
818
+ }
819
+
820
+ void after()
821
+ {
822
+ return 9;
823
+ }
824
+ EOF
825
+ git add file.c &&
826
+ test_tick &&
827
+ git commit -m "initial"
828
+'
829
+
830
+test_expect_success '-L change at exact first line of range' '
831
+ git checkout filter-edge &&
832
+ # Change the function signature (first line of range)
833
+ sed "s/void tracked/int tracked/" file.c >tmp &&
834
+ mv tmp file.c &&
835
+ git commit -a -m "change first line" &&
836
+ git log -L:tracked:file.c -p --format=%s -1 >actual &&
837
+ test_grep "change first line" actual &&
838
+ test_grep "+int tracked" actual &&
839
+ test_grep "\\-void tracked" actual
840
+'
841
+
842
+test_expect_success '-L change at exact last line of range' '
843
+ git checkout filter-edge &&
844
+ git reset --hard HEAD~1 &&
845
+ # Change the closing brace line (last line of range)
846
+ sed "s/^}$/} \/\/ end tracked/" file.c >tmp &&
847
+ mv tmp file.c &&
848
+ git commit -a -m "change last line" &&
849
+ git log -L:tracked:file.c -p --format=%s -1 >actual &&
850
+ test_grep "change last line" actual &&
851
+ test_grep "end tracked" actual
852
+'
853
+
854
+test_expect_success '-L pure deletion in range (no additions)' '
855
+ git checkout filter-edge &&
856
+ git reset --hard HEAD~1 &&
857
+ # Delete a line inside tracked() without adding anything
858
+ sed "/int c/d" file.c >tmp &&
859
+ mv tmp file.c &&
860
+ git commit -a -m "pure deletion" &&
861
+ git log -L:tracked:file.c -p --format=%s -1 >actual &&
862
+ test_grep "pure deletion" actual &&
863
+ test_grep "\\-.*int c" actual
864
+'
865
+
866
test_expect_success '-L with --diff-filter=M excludes root commit' '
867
git checkout parent-oids &&
868
git log -L:func2:file.c --diff-filter=M --format=%s --no-patch >actual &&