Hi Junio, On Mon, 18 Aug 2025 at 18:02, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Seyi Kuforiji <kuforiji98@xxxxxxxxx> 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. > > 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
