Re: [FEATURE] Proposal: git format-patch with `--with-line-numbers` flag

From: Seyi Kuforiji <kuforiji98@gmail.com>
Date: Aug 21, 2025, 12:04 UTC
Message-ID: <CAGedMtf2CW_L8uSc1KRqmAoJ=2Sw4t5AL2AC0uKQJb5keX63ZA@mail.gmail.com>
In-Reply-To: <xmqqfrdok1g6.fsf@gitster.g>

Hi Junio,

On Mon, 18 Aug 2025 at 18:02, Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

>
> Seyi Kuforiji <kuforiji98@gmail.com> writes:
>
> > While working on converting unit tests and sending patches, I ran into a
> > pain point during review. The reviews by Junio, Patrick, and others pointed out
> > issues in my patches, but without line numbers in the emailed code
> > context, it was sometimes hard to know exactly which line was being
> > referenced. I had to manually count through the diff hunks, which slowed
> > things down.
>
> Count through?  I do not usually see a review that talks line
> numbers (e.g. "your change to line 772 is wrong and should look like
> this"), so I am not sure which review comment against which patch
> you had trouble with.  Can you give us an example or two?  URL into
> the lore archive would be good.
>
> One things I try in my reviews is, even though I trim my quotes
> heavily and leave only the part I comment on, I try to leave the
> filename part (i.e. "diff --git" line) and the hunk header (i.e. "@@
> -L,K +M,N @@" line) in.  See
>
>     https://lore.kernel.org/git/xmqqikla86id.fsf@gitster.g/
>
> for an example.
>

Ah, thanks for the clarification; that makes sense now. Up until this point, I didn't know the hunk headers "(@@ -L,K +M,N @@ lines)" provided enough context in terms of the lines the changes were made. I just never read them and usually just jump to the reviews on the code changes, and I try to locate the changes locally :(. I agree this already provides sufficient context, and I've definitely learned something new here :). I am wondering if a description of this is covered in our documentation. If not, maybe I could add it, since I imagine others might have the same question.

Show 42 quoted lines

> > To address this, I’d like to propose adding an option to `git
> > format-patch` (e.g., `--with-line-numbers`) that would include line numbers
> > numbers alongside context lines in the generated patch. This would not
> > affect patch application (`git am` / `git apply`), but would be a visual
> > aid for mailing list readers.
>
> "This would not affect" how?  If you show something like below, it
> would break it so badly that the patch would not apply at all, so
> you may have something else in mind, but I do not know what it would
> be.
>
> diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
> index 980130be78..e12e18f97f 100755
> --- a/t/t0450-txt-doc-vs-help.sh
> +++ b/t/t0450-txt-doc-vs-help.sh
> @@ -112,16 +112,19 @@ do
> 112     adoc="$(builtin_to_adoc "$builtin")" &&
> 113     preq="$(echo BUILTIN_ADOC_$builtin | tr '[:lower:]-' '[:upper:]_')" &&
> 114
> 115-    # if and only if *.adoc is missing, builtin shall be listed in t0450/adoc-missing
> 116-    result=success
> 117+    # If and only if *.adoc is missing, builtin shall be listed in t0450/adoc-missing.
> 118     if grep -q "^$builtin$" "$TEST_DIRECTORY"/t0450/adoc-missing
> 119     then
> 120+            test_expect_success "$builtin appropriately marked as not having .adoc" '
> 121+                    ! test -f "$adoc"
> 122+            '
> 123+    else
> 124             test_set_prereq "$preq"
> 125-            result=failure
> 126-    fi &&
> 127-    test_expect_$result "$builtin appropriately marked as having missing .adoc" '
> 128-            test -f "$adoc"
> 129-    '
> 130+
> 131+            test_expect_success "$builtin appropriately marked as having .adoc" '
> 132+                    test -f "$adoc"
> 133+            '
> 134+    fi
> 135
> 136     # *.adoc output assertions
> 137     test_expect_success "$preq" "$builtin *.adoc SYNOPSIS has dashed labels" '

There isn't a need anymore for my proposed changes to the way `git format-patch` operates.

Best regards, Seyi Kuforiji