{"thread":{"id":"64183","subject":"[PATCH 0/2] format-patch: handle range-diff on notes correctly for single patches","startedAt":"2025-09-22T21:10:35Z","lastAt":"2025-09-25T17:09:11Z","messageCount":15,"participants":["kristofferhaugsbakk@fastmail.com","Junio C Hamano","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"526986","messageId":"cover.1758574974.git.code@khaugsbakk.name","threadId":"64183","inReplyTo":null,"subject":"[PATCH 0/2] format-patch: handle range-diff on notes correctly for single patches","fromName":"","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-22T21:10:21Z","receivedAt":"2025-09-22T21:10:35Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"From: Kristoffer Haugsbakk \n\ngit-format-patch(1) does not handle Git notes correctly in the\nrange-diff output for single-commit series. It reverts to the\ndefault behavior of git-range-diff(1), which is to act like git-log(1).\n\nFix that notes handling to always output the same notes (namespaces) in\nthe two positions:\n\n• beneath the commit message; and\n• in the range-diff.\n\nDo that by (patch by patch):\n\n1. Refactoring to use a new `ref_info` struct member\n2. Using that in `log-tree.c`\n\n§ Testing\n\nI have (for once) tried to check for leaks by running the test suite\nwith this `config.mak`:\n\n```\nDEVELOPER=1\nDEBUG=1\nCC = ccache gcc\nCFLAGS+=-O0\nCFLAGS+=-ggdb3\nUSE_ASCIIDOCTOR=true\nSANITIZE=leak,address\n```\n\nKristoffer Haugsbakk (2):\n revision: add rdiff_other_arg to rev_info\n format-patch: handle range-diff on notes correctly for single patches\n\n builtin/log.c | 7 +++----\n log-tree.c | 3 ++-\n revision.h | 1 +\n t/t3206-range-diff.sh | 16 +++++++++++++++-\n 4 files changed, 21 insertions(+), 6 deletions(-)\n\n\nbase-commit: ca2559c1d630eb4f04cdee2328aaf1c768907a9e\n-- \n2.51.0.270.gdb73cbc1bc1\n\n"},{"id":"526987","messageId":"bb065767336.1758574974.git.code@khaugsbakk.name","threadId":"64183","inReplyTo":"cover.1758574974.git.code@khaugsbakk.name","subject":"[PATCH 1/2] revision: add rdiff_other_arg to rev_info","fromName":"","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-22T21:10:22Z","receivedAt":"2025-09-22T21:10:53Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"From: Kristoffer Haugsbakk \n\ngit-format-patch(1) needs to pass `--[no-]notes` options on to the\nrange-diff subprocess in `range-diff.c`. This is handled in `builtin/\nlog.c` by the local variable `other_arg` in the case of multiple\ncommits, but not in the single commit case where there is no cover\nletter and the range-diff is on that single resulting patch; the\nrange-diff is then made in `log-tree.c`, whither `other_arg` has not\nbeen propagated.\n\ngit-format-patch(1) is supposed to treat Git notes the same between\nnotes output beneath the commit message and the notes output for the\nrange-diff. But this lack of notes handling in `log-tree.c` breaks\nthat expected behavior; range-diff notes handling for a single patch\nacts like a normal git-range-diff(1) invocation with regards to notes.\nYou can, for example, end up with a patch where the note beneath the\ncommit message has the correct notes namespace, but the range-diff has\nall the notes that are configured to be displayed by git-log(1).[1]\n\nWe need to fix this. But first lay the groundwork by converting\n`other_arg` to a struct member; next we can simply use that member\nin `log-tree.c` without having to thread it from `builtin/log.c`.\n\nNo functional changes.\n\n† 1: See the configuration variable `format.notes` for git-format-\n patch(1); c.f. `notes.displayRef` for git-log(1). These two\n have nothing to do with each other.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n There is also `other_arg` in `builtin/range-diff.c` but `rev_info` does\n not seem to be involved.\n\n builtin/log.c | 7 +++----\n revision.h | 1 +\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 5f552d14c0f..56dd9fbc057 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1400,13 +1400,12 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\t * can be added later if deemed desirable.\n \t\t */\n \t\tstruct diff_options opts;\n-\t\tstruct strvec other_arg = STRVEC_INIT;\n \t\tstruct range_diff_options range_diff_opts = {\n \t\t\t.creation_factor = rev->creation_factor,\n \t\t\t.dual_color = 1,\n \t\t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n \t\t\t.diffopt = &opts,\n-\t\t\t.other_arg = &other_arg\n+\t\t\t.other_arg = &rev->rdiff_other_arg\n \t\t};\n \n \t\trepo_diff_setup(the_repository, &opts);\n@@ -1414,9 +1413,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\topts.use_color = rev->diffopt.use_color;\n \t\tdiff_setup_done(&opts);\n \t\tfprintf_ln(rev->diffopt.file, \"%s\", rev->rdiff_title);\n-\t\tget_notes_args(&other_arg, rev);\n \t\tshow_range_diff(rev->rdiff1, rev->rdiff2, &range_diff_opts);\n-\t\tstrvec_clear(&other_arg);\n \t}\n }\n \n@@ -2328,6 +2325,7 @@ int cmd_format_patch(int argc,\n \t\trev.rdiff_title = diff_title(&rdiff_title, reroll_count,\n \t\t\t\t\t _(\"Range-diff:\"),\n \t\t\t\t\t _(\"Range-diff against v%d:\"));\n+\t\tget_notes_args(&(rev.rdiff_other_arg), &rev);\n \t}\n \n \t/*\n@@ -2487,6 +2485,7 @@ int cmd_format_patch(int argc,\n \trev.diffopt.no_free = 0;\n \trelease_revisions(&rev);\n \tformat_config_release(&cfg);\n+\tstrvec_clear(&rev.rdiff_other_arg);\n \treturn 0;\n }\n \ndiff --git a/revision.h b/revision.h\nindex 21e288c5baa..26c18a0934b 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -334,6 +334,7 @@ struct rev_info {\n \t/* range-diff */\n \tconst char *rdiff1;\n \tconst char *rdiff2;\n+\tstruct strvec rdiff_other_arg;\n \tint creation_factor;\n \tconst char *rdiff_title;\n \n-- \n2.51.0.270.gdb73cbc1bc1\n\n"},{"id":"526988","messageId":"7f2487af433.1758574974.git.code@khaugsbakk.name","threadId":"64183","inReplyTo":"cover.1758574974.git.code@khaugsbakk.name","subject":"[PATCH 2/2] format-patch: handle range-diff on notes correctly for single patches","fromName":"","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-22T21:10:23Z","receivedAt":"2025-09-22T21:11:12Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"From: Kristoffer Haugsbakk \n\nNo `--[no-]notes` options are sent to the range-diff subprocess in\n`range-diff.c` when making a single patch. This means that you can get\ndifferent Git notes below the commit message and in the range-diff\npart. (See the previous commit for elaboration.)\n\nUse the struct member that we introduced and populated in the\nprevious commit.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n I’ve tried to conform to 6caa96c2 (t3206: test_when_finished before\n dirtying operations, not after, 2024-08-06) in the test here.\n\n log-tree.c | 3 ++-\n t/t3206-range-diff.sh | 16 +++++++++++++++-\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 73d21f71764..831284288f9 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -718,7 +718,8 @@ static void show_diff_of_diff(struct rev_info *opt)\n \t\t\t.creation_factor = opt->creation_factor,\n \t\t\t.dual_color = 1,\n \t\t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n-\t\t\t.diffopt = &opts\n+\t\t\t.diffopt = &opts,\n+\t\t\t.other_arg = &opt->rdiff_other_arg\n \t\t};\n \n \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex e091df6d01d..1e812df806b 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -707,7 +707,7 @@ test_expect_success 'format-patch --range-diff does not compare notes by default\n \t! grep \"note\" 0000-*\n '\n \n-test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' '\n+test_expect_success 'format-patch --notes=custom --range-diff --cover-letter only compares custom notes' '\n \ttest_when_finished \"git notes remove topic unmodified || :\" &&\n \tgit notes add -m \"topic note\" topic &&\n \tgit notes add -m \"unmodified note\" unmodified &&\n@@ -721,6 +721,20 @@ test_expect_success 'format-patch --notes=custom --range-diff only compares cust\n \t! grep \"## Notes ##\" 0000-*\n '\n \n+# --range-diff on a single commit requires --no-cover-letter\n+test_expect_success 'format-patch --notes=custom --range-diff on single commit only compares custom notes' '\n+\ttest_when_finished \"git notes remove HEAD unmodified || :\" &&\n+\tgit notes add -m \"topic note\" HEAD &&\n+\ttest_when_finished \"git notes --ref=custom remove HEAD unmodified || :\" &&\n+\tgit notes add -m \"unmodified note\" unmodified &&\n+\tgit notes --ref=custom add -m \"topic note (custom)\" HEAD &&\n+\tgit notes --ref=custom add -m \"unmodified note (custom)\" unmodified &&\n+\tgit format-patch --notes=custom --range-diff=$prev \\\n+\t\t-1 --stdout >actual &&\n+\ttest_grep \"## Notes (custom) ##\" actual &&\n+\ttest_grep ! \"## Notes ##\" actual\n+'\n+\n test_expect_success 'format-patch --range-diff with --no-notes' '\n \ttest_when_finished \"git notes remove topic unmodified || :\" &&\n \tgit notes add -m \"topic note\" topic &&\n-- \n2.51.0.270.gdb73cbc1bc1\n\n"},{"id":"526996","messageId":"xmqqikharvyl.fsf@gitster.g","threadId":"64183","inReplyTo":"bb065767336.1758574974.git.code@khaugsbakk.name","subject":"Re: [PATCH 1/2] revision: add rdiff_other_arg to rev_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-22T21:58:10Z","receivedAt":"2025-09-22T21:58:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"kristofferhaugsbakk@fastmail.com writes:\n\n> git-format-patch(1) is supposed to treat Git notes the same between\n> notes output beneath the commit message and the notes output for the\n> range-diff.\n\nIs this an opinion, or are there things that existing pieces of code\nalready do to achieve such a behaviour already?\n\n> diff --git a/revision.h b/revision.h\n> index 21e288c5baa..26c18a0934b 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -334,6 +334,7 @@ struct rev_info {\n> \t/* range-diff */\n> \tconst char *rdiff1;\n> \tconst char *rdiff2;\n> +\tstruct strvec rdiff_other_arg;\n> \tint creation_factor;\n> \tconst char *rdiff_title;\n\nWhen embedding a struct A in a struct B, we should always make sure\nthat initialization macro/function for struct B is updated so that\nthe initialization for struct A is done correctly for the new member.\n\nWe do have REV_INFO_INIT for \"struct rev_info\"\n\n #define REV_INFO_INIT { \\\n .abbrev = DEFAULT_ABBREV, \\\n .simplify_history = 1, \\\n .pruning.flags.recursive = 1, \\\n ...\n .expand_tabs_in_log_default = 8, \\\n }\n\nthat does not allow any existing callers to leave it uninitialized\nor get away by zero-initializing, so all the users must be using it\nor the system before your patch is already buggy.\n\nAnd we do have STRVEC_INIT that we must use in that initializer.\n\n extern const char *empty_strvec[];\n\n struct strvec {\n const char **v;\n size_t nr;\n size_t alloc;\n };\n\n #define STRVEC_INIT { \\\n .v = empty_strvec, \\\n }\n\nSo this step forgets to update revision.h to teach STRVEC_INIT on\nthe new rdiff_other_arg member.\n\nBack when it was a random one-shot variable in range-diff, it might\nnot have mattered all that much, but now we have it as a proper\nmember of the struct, can we give it a name better than 'other_arg\"?\nOr is it the case that truly any random crap can be slurped into the\narray and thrown back at \"git log\" without range-diff machinery\nunderstanding what it is doing at all (which I would not be\nsurprised, as some parts of our code base is written in somewhat a\nsloppy way)?\n\nThanks.\n"},{"id":"526997","messageId":"xmqqecryrvt6.fsf@gitster.g","threadId":"64183","inReplyTo":"7f2487af433.1758574974.git.code@khaugsbakk.name","subject":"Re: [PATCH 2/2] format-patch: handle range-diff on notes correctly for single patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-22T22:01:25Z","receivedAt":"2025-09-22T22:01:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"kristofferhaugsbakk@fastmail.com writes:\n\n> From: Kristoffer Haugsbakk \n>\n> No `--[no-]notes` options are sent to the range-diff subprocess in\n> `range-diff.c` when making a single patch. This means that you can get\n> different Git notes below the commit message and in the range-diff\n> part. (See the previous commit for elaboration.)\n\nWould this also mean \"range-diff --no-notes\" would not have any\neffect in squelching the note output in such a mode? If so, perhaps\nwe should say not just \"can get different Git notes\" but \"can get\nnotes even when you asked not to\"?\n\n> @@ -718,7 +718,8 @@ static void show_diff_of_diff(struct rev_info *opt)\n> \t\t\t.creation_factor = opt->creation_factor,\n> \t\t\t.dual_color = 1,\n> \t\t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n> -\t\t\t.diffopt = &opts\n> +\t\t\t.diffopt = &opts,\n> +\t\t\t.other_arg = &opt->rdiff_other_arg\n> \t\t};\n> \n> \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index e091df6d01d..1e812df806b 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -707,7 +707,7 @@ test_expect_success 'format-patch --range-diff does not compare notes by default\n> \t! grep \"note\" 0000-*\n> '\n> \n> -test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' '\n> +test_expect_success 'format-patch --notes=custom --range-diff --cover-letter only compares custom notes' '\n> \ttest_when_finished \"git notes remove topic unmodified || :\" &&\n> \tgit notes add -m \"topic note\" topic &&\n> \tgit notes add -m \"unmodified note\" unmodified &&\n> @@ -721,6 +721,20 @@ test_expect_success 'format-patch --notes=custom --range-diff only compares cust\n> \t! grep \"## Notes ##\" 0000-*\n> '\n> \n> +# --range-diff on a single commit requires --no-cover-letter\n> +test_expect_success 'format-patch --notes=custom --range-diff on single commit only compares custom notes' '\n> +\ttest_when_finished \"git notes remove HEAD unmodified || :\" &&\n> +\tgit notes add -m \"topic note\" HEAD &&\n> +\ttest_when_finished \"git notes --ref=custom remove HEAD unmodified || :\" &&\n> +\tgit notes add -m \"unmodified note\" unmodified &&\n> +\tgit notes --ref=custom add -m \"topic note (custom)\" HEAD &&\n> +\tgit notes --ref=custom add -m \"unmodified note (custom)\" unmodified &&\n> +\tgit format-patch --notes=custom --range-diff=$prev \\\n> +\t\t-1 --stdout >actual &&\n> +\ttest_grep \"## Notes (custom) ##\" actual &&\n> +\ttest_grep ! \"## Notes ##\" actual\n> +'\n\nSounds sensible.\n\n> test_expect_success 'format-patch --range-diff with --no-notes' '\n> \ttest_when_finished \"git notes remove topic unmodified || :\" &&\n> \tgit notes add -m \"topic note\" topic &&\n"},{"id":"527096","messageId":"ba9b7fb2-c990-44fb-a506-0800d02854a9@app.fastmail.com","threadId":"64183","inReplyTo":"xmqqikharvyl.fsf@gitster.g","subject":"Re: [PATCH 1/2] revision: add rdiff_other_arg to rev_info","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-23T15:53:54Z","receivedAt":"2025-09-23T15:54:15Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Mon, Sep 22, 2025, at 23:58, Junio C Hamano wrote:\n> kristofferhaugsbakk@fastmail.com writes:\n>\n>> git-format-patch(1) is supposed to treat Git notes the same between\n>> notes output beneath the commit message and the notes output for the\n>> range-diff.\n>\n> Is this an opinion, or are there things that existing pieces of code\n> already do to achieve such a behaviour already?\n\nWhat I mean is that\n\n Notes (...)\n\nBeneath the commit message and\n\n ### Notes (...) ###\n\nIn the range-diff should be from the same namespaces. It shouldn’t be,\nfor example:\n\n Notes (presentation):\n\nBeneath the commit message while the range-diff has:\n\n ### Notes (testing) ###\n ...\n ### Notes (scratchpad) ###\n\nThat’s the point of passing `--notes` to range-diff.\n\nAm I missing something in the explanation? The commit message might\nneed a rewrite.\n\n>> diff --git a/revision.h b/revision.h\n>> index 21e288c5baa..26c18a0934b 100644\n>> --- a/revision.h\n>> +++ b/revision.h\n>> @@ -334,6 +334,7 @@ struct rev_info {\n>> \t/* range-diff */\n>> \tconst char *rdiff1;\n>> \tconst char *rdiff2;\n>> +\tstruct strvec rdiff_other_arg;\n>> \tint creation_factor;\n>> \tconst char *rdiff_title;\n>\n> When embedding a struct A in a struct B, we should always make sure\n> that initialization macro/function for struct B is updated so that\n> the initialization for struct A is done correctly for the new member.\n>\n> We do have REV_INFO_INIT for \"struct rev_info\"\n>\n> #define REV_INFO_INIT { \\\n> .abbrev = DEFAULT_ABBREV, \\\n> .simplify_history = 1, \\\n> .pruning.flags.recursive = 1, \\\n> ...\n> .expand_tabs_in_log_default = 8, \\\n> }\n>\n> that does not allow any existing callers to leave it uninitialized\n> or get away by zero-initializing, so all the users must be using it\n> or the system before your patch is already buggy.\n>\n> And we do have STRVEC_INIT that we must use in that initializer.\n>\n> extern const char *empty_strvec[];\n>\n> struct strvec {\n> const char **v;\n> size_t nr;\n> size_t alloc;\n> };\n>\n> #define STRVEC_INIT { \\\n> .v = empty_strvec, \\\n> }\n>\n> So this step forgets to update revision.h to teach STRVEC_INIT on\n> the new rdiff_other_arg member.\n\nThanks for the explanation. I’ve added `.rdiff_other_arg = STRVEC_INIT\n\\` to `REV_INFO_INIT`.\n\n> Back when it was a random one-shot variable in range-diff, it might\n> not have mattered all that much, but now we have it as a proper\n> member of the struct, can we give it a name better than 'other_arg\"?\n\nI’ve had that thought too. But then I forgot what a better name\nwould be.\n\nCould it be as simple as `log_arg` or `log_args`?\n\n(This could be added as a preliminary patch)\n\n`builtin/range-diff.c` uses `range_diff_options.other_arg` to pass\n`--notes` but also (and quite recently[1]) to pass `--merges`.\n\n /* If `--diff-merges` was specified, imply `--merges` */\n\n† 1: f8043236 (range-diff: optionally include merge commits' diffs in\n the analysis, 2024-12-16)\n\n> Or is it the case that truly any random crap can be slurped into the\n> array and thrown back at \"git log\" without range-diff machinery\n> understanding what it is doing at all (which I would not be\n> surprised, as some parts of our code base is written in somewhat a\n> sloppy way)?\n\nWell anything you throw into it will ultimately end up in the git-log(1)\nchild process in `range-diff.c:73`.[2] But this doesn’t seem like a\nproblem given that all the arguments are “programmed” as constant\nstrings?\n\n† 2: on ca2559c1 (The tenth batch, 2025-09-18)\n"},{"id":"527101","messageId":"269dd1ed-e8b0-448a-a85f-fdcafafbd0a0@app.fastmail.com","threadId":"64183","inReplyTo":"xmqqecryrvt6.fsf@gitster.g","subject":"Re: [PATCH 2/2] format-patch: handle range-diff on notes correctly for single patches","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-23T16:26:33Z","receivedAt":"2025-09-23T16:26:54Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Tue, Sep 23, 2025, at 00:01, Junio C Hamano wrote:\n> kristofferhaugsbakk@fastmail.com writes:\n>\n>> From: Kristoffer Haugsbakk \n>>\n>> No `--[no-]notes` options are sent to the range-diff subprocess in\n>> `range-diff.c` when making a single patch. This means that you can get\n>> different Git notes below the commit message and in the range-diff\n>> part. (See the previous commit for elaboration.)\n>\n> Would this also mean \"range-diff --no-notes\" would not have any\n> effect in squelching the note output in such a mode?\n\nDo you mean `git format-patch ... --range-diff --no-notes`? Yes,\n`--no-notes` has no effect. range-diff just does the default thing\nwhich is `--show-notes-by-default` (act like git-log(1), which shows the\ndefault notes namespace unless any `--[no-]notes` options are given (and\nthere are no such options in this case)).\n\n> If so, perhaps we should say not just \"can get different Git notes\"\n> but \"can get notes even when you asked not to\"?\n\nI think “can get different Git notes” covers all possibilities, both too\nmany and too few.\n\nBut like the previous commit this one could maybe use a rewrite.\n\n No `--[no-]--notes` options are sent to the range-diff subprocess in\n `range-diff.c` when making a single patch. This means that range-diff\n will handle Git notes like git-log(1).\n\n This is a problem when you ask to use certain notes, or none at all,\n since that set of notes will appear beneath the commit message but the\n range-diff will have whatever notes that git-log(1) would have given\n you.\n\nThat’s at least less dense.\n\n>> @@ -718,7 +718,8 @@ static void show_diff_of_diff(struct rev_info *opt)\n>> [snip]\n"},{"id":"527110","messageId":"xmqqa52loyvq.fsf@gitster.g","threadId":"64183","inReplyTo":"ba9b7fb2-c990-44fb-a506-0800d02854a9@app.fastmail.com","subject":"Re: [PATCH 1/2] revision: add rdiff_other_arg to rev_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-23T17:35:37Z","receivedAt":"2025-09-23T17:35:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Mon, Sep 22, 2025, at 23:58, Junio C Hamano wrote:\n>> kristofferhaugsbakk@fastmail.com writes:\n>>\n>>> git-format-patch(1) is supposed to treat Git notes the same between\n>>> notes output beneath the commit message and the notes output for the\n>>> range-diff.\n>>\n>> Is this an opinion, or are there things that existing pieces of code\n>> already do to achieve such a behaviour already?\n>\n> What I mean is that\n>\n> Notes (...)\n>\n> Beneath the commit message and\n>\n> ### Notes (...) ###\n>\n> In the range-diff should be from the same namespaces. It shouldn’t be,\n> for example:\n>\n> Notes (presentation):\n>\n> Beneath the commit message while the range-diff has:\n>\n> ### Notes (testing) ###\n> ...\n> ### Notes (scratchpad) ###\n>\n> That’s the point of passing `--notes` to range-diff.\n\nOK. So it is more like \"range-diff is supposed to show comparison\nof pairs of patches; if format-patch shows one set of notes after\nthree-dash lines in its output, range-diff invoked by format-patch\nto compare its patches with another set of patches should also be\ncomparing patches generated with the same set of notes\". That makes\nsense to me.\n\n> Thanks for the explanation. I’ve added `.rdiff_other_arg = STRVEC_INIT\n> \\` to `REV_INFO_INIT`.\n\nYup, I think I already have a fix-up patch mixed in your series in\nthe integration result I pushed out last night.\n\n> Could it be as simple as `log_arg` or `log_args`?\n\nYeah, that is much better than \"other\" (where it is unclear what are\nthe \"primary\" things that \"others\" are in contrast).\n\nThanks.\n"},{"id":"527117","messageId":"f1da0f06-e57c-4abd-b9ce-ef3fd5ce2037@app.fastmail.com","threadId":"64183","inReplyTo":"xmqqa52loyvq.fsf@gitster.g","subject":"Re: [PATCH 1/2] revision: add rdiff_other_arg to rev_info","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-23T17:47:52Z","receivedAt":"2025-09-23T17:48:14Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Tue, Sep 23, 2025, at 19:35, Junio C Hamano wrote:\n> \"Kristoffer Haugsbakk\" writes:\n>\n>> On Mon, Sep 22, 2025, at 23:58, Junio C Hamano wrote:\n>>> kristofferhaugsbakk@fastmail.com writes:\n>>>\n>>>> git-format-patch(1) is supposed to treat Git notes the same between\n>>>> notes output beneath the commit message and the notes output for the\n>>>> range-diff.\n>>>\n>>> Is this an opinion, or are there things that existing pieces of code\n>>> already do to achieve such a behaviour already?\n>>\n>> What I mean is that\n>> [snip]\n>> That’s the point of passing `--notes` to range-diff.\n>\n> OK. So it is more like \"range-diff is supposed to show comparison\n> of pairs of patches; if format-patch shows one set of notes after\n> three-dash lines in its output, range-diff invoked by format-patch\n> to compare its patches with another set of patches should also be\n> comparing patches generated with the same set of notes\". That makes\n> sense to me.\n\nExactly.\n\nI might eventually send some drafts of better commit messages.\n\n>> Thanks for the explanation. I’ve added `.rdiff_other_arg = STRVEC_INIT\n>> \\` to `REV_INFO_INIT`.\n>\n> Yup, I think I already have a fix-up patch mixed in your series in\n> the integration result I pushed out last night.\n\nOh yeah I saw that afterwards. Which made me spot a mistake in my own\nfixup... which was instructive. ;)\n\n>> Could it be as simple as `log_arg` or `log_args`?\n>\n> Yeah, that is much better than \"other\" (where it is unclear what are\n> the \"primary\" things that \"others\" are in contrast).\n\nI remember looking at the code and having that realization: Oh, *other*\nis *log*. Huh.\n\n... Maybe specifically `log_args` since it’s a `strvec`? `git grep\n'struct strvec'` seems to give me a lot of plural (style).\n"},{"id":"527140","messageId":"xmqqtt0sookw.fsf@gitster.g","threadId":"64183","inReplyTo":"f1da0f06-e57c-4abd-b9ce-ef3fd5ce2037@app.fastmail.com","subject":"Re: [PATCH 1/2] revision: add rdiff_other_arg to rev_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-23T21:18:07Z","receivedAt":"2025-09-23T21:18:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n>>> Could it be as simple as `log_arg` or `log_args`?\n>>\n>> Yeah, that is much better than \"other\" (where it is unclear what are\n>> the \"primary\" things that \"others\" are in contrast).\n>\n> I remember looking at the code and having that realization: Oh, *other*\n> is *log*. Huh.\n>\n> ... Maybe specifically `log_args` since it’s a `strvec`? `git grep\n> 'struct strvec'` seems to give me a lot of plural (style).\n\nIf you were asking me about singular vs plural, sorry, I didn't even\nrealize that was the quesiton being asked.\n\nMy personal preference is to name arrays singular so that you can\nname its 0th element by saying dog[0], not dogs[0]. \"dog[1] and\ndog[2] are friends\" not dogs[1] and dogs[2]. An exception is when\nmost of the time you use the array as a single unit as a collection,\npassing it around in the call chain, rarely addressing each individual\nelement. I am OK to see such an array called plural (but of course,\nsingular names are always fine)..\n\nAn instance of \"struct strvec\" may fall into the same category as\nthe latter. A single \"struct strvec args\" may be used as a\ncollection of arguments passed to the program .\n\nSo, log_args is fine by me.\n"},{"id":"527142","messageId":"xmqqplbgoogl.fsf@gitster.g","threadId":"64183","inReplyTo":"269dd1ed-e8b0-448a-a85f-fdcafafbd0a0@app.fastmail.com","subject":"Re: [PATCH 2/2] format-patch: handle range-diff on notes correctly for single patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-23T21:20:42Z","receivedAt":"2025-09-23T21:20:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Tue, Sep 23, 2025, at 00:01, Junio C Hamano wrote:\n>> kristofferhaugsbakk@fastmail.com writes:\n>>\n>>> From: Kristoffer Haugsbakk \n>>>\n>>> No `--[no-]notes` options are sent to the range-diff subprocess in\n>>> `range-diff.c` when making a single patch. This means that you can get\n>>> different Git notes below the commit message and in the range-diff\n>>> part. (See the previous commit for elaboration.)\n>>\n>> Would this also mean \"range-diff --no-notes\" would not have any\n>> effect in squelching the note output in such a mode?\n>\n> Do you mean `git format-patch ... --range-diff --no-notes`? Yes,\n> `--no-notes` has no effect. range-diff just does the default thing\n> which is `--show-notes-by-default` (act like git-log(1), which shows the\n> default notes namespace unless any `--[no-]notes` options are given (and\n> there are no such options in this case)).\n\nAnd this change will fix that too, which is nice.\n\n> But like the previous commit this one could maybe use a rewrite.\n>\n> No `--[no-]--notes` options are sent to the range-diff subprocess in\n\n\"--notes\" --> \"notes\", as a required single dash after negation is\nalready inside [] ;-)\n\n> `range-diff.c` when making a single patch. This means that range-diff\n> will handle Git notes like git-log(1).\n>\n> This is a problem when you ask to use certain notes, or none at all,\n> since that set of notes will appear beneath the commit message but the\n> range-diff will have whatever notes that git-log(1) would have given\n> you.\n>\n> That’s at least less dense.\n\nThanks.\n"},{"id":"527337","messageId":"v2-cover.1758819879.git.code@khaugsbakk.name","threadId":"64183","inReplyTo":"cover.1758574974.git.code@khaugsbakk.name","subject":"[PATCH v2 0/3] format-patch: handle range-diff on notes correctly for single patches","fromName":"","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-25T17:07:33Z","receivedAt":"2025-09-25T17:08:15Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"From: Kristoffer Haugsbakk \n\ngit-format-patch(1) does not handle Git notes correctly in the\nrange-diff output for single-commit series. It reverts to the\ndefault behavior of git-range-diff(1), which is to act like git-log(1).\n\nGit notes can be added to patches where they go beneath the commit\nmessage (similar to git-log(1)). You can of course use any set of notes\nref names. And the range-diff is supposed to use the same ref names.\nThis works for the case when you have a cover letter (for more than one\ncommit) but not when you have a single commit.\n\nSee patch 2/3 for a full explanation of the problem.\n\nFix that notes handling to always output the same notes (ref names) in\nthe two positions:\n\n• beneath the commit message; and\n• in the range diff.\n\nPatch-by-patch:\n\n1. Rename `other_arg` to `log_arg`\n2. Refactor to use a new `rev_info` struct member\n3. Use that in `log-tree.c` in order to solve the problem\n\n§ Changes in v2\n\nMostly better commit messages. Fix a mistake in the `rev_info` struct\nhandling.\n\nPatch-by-patch (see notes on them for details):\n\n1. (new) Better variable name\n2. Rewrite commit message and fix mistake\n3. Rewrite commit message by mostly stealing from patch 2/3\n\nAbout the commit messages: the first one just jumped into the problem\nwithout a proper introduction (see the note on patch 2/3). Not clear\nenough. But I didn’t act on this review feedback:[1]\n\n “ Would this also mean \"range-diff --no-notes\" would not have any\n effect in squelching the note output in such a mode? If so, perhaps\n we should say not just \"can get different Git notes\" but \"can get\n notes even when you asked not to\"?\n\n🔗 1: https://lore.kernel.org/git/xmqqecryrvt6.fsf@gitster.g/\n\nSince both commits in v1 already said `--[no-]notes`.\n\n§ CC\n\nDenton Liu wrote a commit mentioned in patch 1/3. It’s been more years\nthan the recommended cutoff, but he has been active recently.\n\n§ Link to the previous version\n\nhttps://lore.kernel.org/git/cover.1758574974.git.code@khaugsbakk.name/\n\n❦ ❦ ❦ ❦ ❦\n\n(this means you can skip to the diffs)\n\n§ How I Present Patch Series\n\nVersion: 1\n\nThe cover letter:\n\n• Problem statement and solution\n• (optional) Summary of what each commit/patch does\n• Changes compared to the previous version\n • Note that all versions are not included\n• (optional) “CC”/“Cc” which explains all or parts of the CC list\n • But some things are not noteworthy, like the standard practice of\n including previous-round respondents\n• Link to the previous version\n • This is new. I thought it was redundant but some people (like me!)\n use webapp email clients which are not that great for navigating\n among tree-like email threads. So I was inspired by this practice\n which many others already use.\n• I like including an interdiff in addition to the range diff if I can.\n They are just so convenient and complementary to the range diff.\n\nThen each commit/patch might have Git notes with a `series` namespace\n(ref name) These contain:\n\n• Comments/questions to reviewers about my approach or statements made\n in the commit message.\n• Lines starting with `v:` which introduce a changelog for that\n version. Lately (Sep 2025) these have been written in the “imperative\n mood”, like what is done for a commit message in this (Git) project.\n A bit strange, given that most others seem to use the more immediately\n natural past-tense. But on the other hand: how many new contributors\n to this project use the wrong tense/mood in their commit messages?\n The Git project rule is not “natural”. But I think it’s better\n nonetheless and worth the effort.\n\n (And it might be worth the effort in this context, too. We will see.)\n• ... But this kind of changelog might also be conducive to a bullet\n list of changes. So I might either skip the previous point, do only a\n bullet point, or do both: a presentation and then the bullet points\n summarizing the presentation.\n• Note that all `v:` are kept between versions, which is not\n consistent with how I only have the “Changes” part for the previous\n version.\n• `v:` are ordered newest-to-oldest.\n\nKristoffer Haugsbakk (3):\n range-diff: rename other_arg to log_arg\n revision: add rdiff_log_arg to rev_info\n format-patch: handle range-diff on notes correctly for single patches\n\n builtin/log.c | 7 +++----\n builtin/range-diff.c | 16 ++++++++--------\n log-tree.c | 3 ++-\n range-diff.c | 10 +++++-----\n range-diff.h | 2 +-\n revision.h | 2 ++\n t/t3206-range-diff.sh | 16 +++++++++++++++-\n 7 files changed, 36 insertions(+), 20 deletions(-)\n\nInterdiff against v1:\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 56dd9fbc057..9eff62ce111 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1405,7 +1405,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\t\t.dual_color = 1,\n \t\t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n \t\t\t.diffopt = &opts,\n-\t\t\t.other_arg = &rev->rdiff_other_arg\n+\t\t\t.log_arg = &rev->rdiff_log_arg\n \t\t};\n \n \t\trepo_diff_setup(the_repository, &opts);\n@@ -2325,7 +2325,7 @@ int cmd_format_patch(int argc,\n \t\trev.rdiff_title = diff_title(&rdiff_title, reroll_count,\n \t\t\t\t\t _(\"Range-diff:\"),\n \t\t\t\t\t _(\"Range-diff against v%d:\"));\n-\t\tget_notes_args(&(rev.rdiff_other_arg), &rev);\n+\t\tget_notes_args(&(rev.rdiff_log_arg), &rev);\n \t}\n \n \t/*\n@@ -2485,7 +2485,7 @@ int cmd_format_patch(int argc,\n \trev.diffopt.no_free = 0;\n \trelease_revisions(&rev);\n \tformat_config_release(&cfg);\n-\tstrvec_clear(&rev.rdiff_other_arg);\n+\tstrvec_clear(&rev.rdiff_log_arg);\n \treturn 0;\n }\n \ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex aafcc99b962..f88b40e3607 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -37,13 +37,13 @@ int cmd_range_diff(int argc,\n \t\t struct repository *repo UNUSED)\n {\n \tstruct diff_options diffopt = { NULL };\n-\tstruct strvec other_arg = STRVEC_INIT;\n+\tstruct strvec log_arg = STRVEC_INIT;\n \tstruct strvec diff_merges_arg = STRVEC_INIT;\n \tstruct range_diff_options range_diff_opts = {\n \t\t.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,\n \t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n \t\t.diffopt = &diffopt,\n-\t\t.other_arg = &other_arg\n+\t\t.log_arg = &log_arg\n \t};\n \tint simple_color = -1, left_only = 0, right_only = 0;\n \tstruct option range_diff_options[] = {\n@@ -52,7 +52,7 @@ int cmd_range_diff(int argc,\n \t\t\t N_(\"percentage by which creation is weighted\")),\n \t\tOPT_BOOL(0, \"no-dual-color\", &simple_color,\n \t\t\t N_(\"use simple diff colors\")),\n-\t\tOPT_PASSTHRU_ARGV(0, \"notes\", &other_arg,\n+\t\tOPT_PASSTHRU_ARGV(0, \"notes\", &log_arg,\n \t\t\t\t N_(\"notes\"), N_(\"passed to 'git log'\"),\n \t\t\t\t PARSE_OPT_OPTARG),\n \t\tOPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n@@ -92,7 +92,7 @@ int cmd_range_diff(int argc,\n \t/* If `--diff-merges` was specified, imply `--merges` */\n \tif (diff_merges_arg.nr) {\n \t\trange_diff_opts.include_merges = 1;\n-\t\tstrvec_pushv(&other_arg, diff_merges_arg.v);\n+\t\tstrvec_pushv(&log_arg, diff_merges_arg.v);\n \t}\n \n \tfor (i = 0; i < argc; i++)\n@@ -124,7 +124,7 @@ int cmd_range_diff(int argc,\n \t\tstrbuf_addf(&range1, \"%s..%s\", argv[0], argv[1]);\n \t\tstrbuf_addf(&range2, \"%s..%s\", argv[0], argv[2]);\n \n-\t\tstrvec_pushv(&other_arg, argv +\n+\t\tstrvec_pushv(&log_arg, argv +\n \t\t\t (dash_dash < 0 ? 3 : dash_dash));\n \t} else if (dash_dash == 2 ||\n \t\t (dash_dash < 0 && argc > 1 &&\n@@ -144,7 +144,7 @@ int cmd_range_diff(int argc,\n \t\tstrbuf_addstr(&range1, argv[0]);\n \t\tstrbuf_addstr(&range2, argv[1]);\n \n-\t\tstrvec_pushv(&other_arg, argv +\n+\t\tstrvec_pushv(&log_arg, argv +\n \t\t\t (dash_dash < 0 ? 2 : dash_dash));\n \t} else if (dash_dash == 1 ||\n \t\t (dash_dash < 0 && argc > 0 &&\n@@ -175,7 +175,7 @@ int cmd_range_diff(int argc,\n \t\tstrbuf_addf(&range1, \"%s..%.*s\", b, a_len, a);\n \t\tstrbuf_addf(&range2, \"%.*s..%s\", a_len, a, b);\n \n-\t\tstrvec_pushv(&other_arg, argv +\n+\t\tstrvec_pushv(&log_arg, argv +\n \t\t\t (dash_dash < 0 ? 1 : dash_dash));\n \t} else\n \t\tusage_msg_opt(_(\"need two commit ranges\"),\n@@ -187,7 +187,7 @@ int cmd_range_diff(int argc,\n \trange_diff_opts.right_only = right_only;\n \tres = show_range_diff(range1.buf, range2.buf, &range_diff_opts);\n \n-\tstrvec_clear(&other_arg);\n+\tstrvec_clear(&log_arg);\n \tstrvec_clear(&diff_merges_arg);\n \tstrbuf_release(&range1);\n \tstrbuf_release(&range2);\ndiff --git a/log-tree.c b/log-tree.c\nindex 831284288f9..3d38c748e45 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -719,7 +719,7 @@ static void show_diff_of_diff(struct rev_info *opt)\n \t\t\t.dual_color = 1,\n \t\t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n \t\t\t.diffopt = &opts,\n-\t\t\t.other_arg = &opt->rdiff_other_arg\n+\t\t\t.log_arg = &opt->rdiff_log_arg\n \t\t};\n \n \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\ndiff --git a/range-diff.c b/range-diff.c\nindex ca449a07693..57edff40a85 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -39,7 +39,7 @@ struct patch_util {\n * as struct object_id (will need to be free()d).\n */\n static int read_patches(const char *range, struct string_list *list,\n-\t\t\tconst struct strvec *other_arg,\n+\t\t\tconst struct strvec *log_arg,\n \t\t\tunsigned int include_merges)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -69,8 +69,8 @@ static int read_patches(const char *range, struct string_list *list,\n \tif (!include_merges)\n \t\tstrvec_push(&cp.args, \"--no-merges\");\n \tstrvec_push(&cp.args, range);\n-\tif (other_arg)\n-\t\tstrvec_pushv(&cp.args, other_arg->v);\n+\tif (log_arg)\n+\t\tstrvec_pushv(&cp.args, log_arg->v);\n \tcp.out = -1;\n \tcp.no_stdin = 1;\n \tcp.git_cmd = 1;\n@@ -594,9 +594,9 @@ int show_range_diff(const char *range1, const char *range2,\n \tif (range_diff_opts->left_only && range_diff_opts->right_only)\n \t\tres = error(_(\"options '%s' and '%s' cannot be used together\"), \"--left-only\", \"--right-only\");\n \n-\tif (!res && read_patches(range1, &branch1, range_diff_opts->other_arg, include_merges))\n+\tif (!res && read_patches(range1, &branch1, range_diff_opts->log_arg, include_merges))\n \t\tres = error(_(\"could not parse log for '%s'\"), range1);\n-\tif (!res && read_patches(range2, &branch2, range_diff_opts->other_arg, include_merges))\n+\tif (!res && read_patches(range2, &branch2, range_diff_opts->log_arg, include_merges))\n \t\tres = error(_(\"could not parse log for '%s'\"), range2);\n \n \tif (!res) {\ndiff --git a/range-diff.h b/range-diff.h\nindex 9d39818e349..9b70a80009e 100644\n--- a/range-diff.h\n+++ b/range-diff.h\n@@ -23,7 +23,7 @@ struct range_diff_options {\n \tunsigned include_merges:1;\n \tsize_t max_memory;\n \tconst struct diff_options *diffopt; /* may be NULL */\n-\tconst struct strvec *other_arg; /* may be NULL */\n+\tconst struct strvec *log_arg; /* may be NULL */\n };\n \n /*\ndiff --git a/revision.h b/revision.h\nindex 26c18a0934b..ce30570d86a 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -334,7 +334,7 @@ struct rev_info {\n \t/* range-diff */\n \tconst char *rdiff1;\n \tconst char *rdiff2;\n-\tstruct strvec rdiff_other_arg;\n+\tstruct strvec rdiff_log_arg;\n \tint creation_factor;\n \tconst char *rdiff_title;\n \n@@ -411,6 +411,7 @@ struct rev_info {\n \t.expand_tabs_in_log = -1, \\\n \t.commit_format = CMIT_FMT_DEFAULT, \\\n \t.expand_tabs_in_log_default = 8, \\\n+\t.rdiff_log_arg = STRVEC_INIT, \\\n }\n \n /**\nRange-diff against v1:\n-: ----------- > 1: bd037df14f5 range-diff: rename other_arg to log_arg\n1: bb065767336 ! 2: d9419743773 revision: add rdiff_other_arg to rev_info\n @@ Metadata\n Author: Kristoffer Haugsbakk \n \n ## Commit message ##\n - revision: add rdiff_other_arg to rev_info\n -\n - git-format-patch(1) needs to pass `--[no-]notes` options on to the\n - range-diff subprocess in `range-diff.c`. This is handled in `builtin/\n - log.c` by the local variable `other_arg` in the case of multiple\n - commits, but not in the single commit case where there is no cover\n - letter and the range-diff is on that single resulting patch; the\n - range-diff is then made in `log-tree.c`, whither `other_arg` has not\n - been propagated.\n -\n - git-format-patch(1) is supposed to treat Git notes the same between\n - notes output beneath the commit message and the notes output for the\n - range-diff. But this lack of notes handling in `log-tree.c` breaks\n - that expected behavior; range-diff notes handling for a single patch\n - acts like a normal git-range-diff(1) invocation with regards to notes.\n - You can, for example, end up with a patch where the note beneath the\n - commit message has the correct notes namespace, but the range-diff has\n - all the notes that are configured to be displayed by git-log(1).[1]\n -\n - We need to fix this. But first lay the groundwork by converting\n - `other_arg` to a struct member; next we can simply use that member\n + revision: add rdiff_log_arg to rev_info\n +\n + git-format-patch(1) supports Git notes by showing them beneath the\n + patch/commit message, similar to git-log(1). The command also supports\n + showing those same notes ref names in the range diff output.\n +\n + Note *the same* ref names; any Git notes options or configuration\n + variables need to be handed off to the range-diff machinery. This works\n + correctly in the case when the range diff is on the cover letter. But it\n + does not work correctly when the output is a single patch with an\n + embedded range diff.\n +\n + Concretely, git-format-patch(1) needs to pass `--[no-]notes` options\n + on to the range-diff subprocess in `range-diff.c`. This is handled in\n + `builtin/log.c` by the local variable `log_arg` in the case of mul-\n + tiple commits, but not in the single commit case where there is no\n + cover letter and the range diff is embedded in the patch output; the\n + range diff is then made in `log-tree.c`, whither `log_arg` has not\n + been propagated. This means that the range-diff subprocess reverts\n + to its default behavior, which is to act like git-log(1) w.r.t. notes.\n +\n + We need to fix this. But first lay the groundwork by converting\n + `log_arg` to a struct member; next we can simply use that member\n in `log-tree.c` without having to thread it from `builtin/log.c`.\n \n No functional changes.\n \n - † 1: See the configuration variable `format.notes` for git-format-\n - patch(1); c.f. `notes.displayRef` for git-log(1). These two\n - have nothing to do with each other.\n -\n + Helped-by: Junio C Hamano \n Signed-off-by: Kristoffer Haugsbakk \n \n \n ## Notes (series) ##\n + v2:\n +\n + Rewrite the commit message. The message jumps into the problem without\n + setting the stage. I think the problem should be presented for a future\n + reader with only some basic prerequisite knowledge of this area; they\n + might use git-format-patch(1), but they might not use Git notes *with*\n + format-patch.[1] For that reason, start with two paragraphs that pre-\n + sent how notes are handled. Then continue with the story in a format\n + similar to v1.\n +\n + Note that the different “range diff” and “range-diff” spellings are\n + intentional. “Range diff” here refers to the diff output while\n + “range-diff” refers to the machinery that creates that output.\n +\n + † 1: I don’t really see much Git notes use on patches on this mailing\n + list. And even less with non-default namespaces. But in any case:\n + the stage should be set properly even if regulars here *did* use\n + format-patch notes a lot.\n +\n + Also fix handling of struct-in-struct (helped by Junio).\n +\n + • Rewrite the commit message\n + • Fix struct-in-struct\n + • (And) Reset author date. I started this in June but the time\n + investment is mostly from these last days.\n +\n + v1:\n +\n There is also `other_arg` in `builtin/range-diff.c` but `rev_info` does\n not seem to be involved.\n \n @@ builtin/log.c: static void make_cover_letter(struct rev_info *rev, int use_separ\n \t\t * can be added later if deemed desirable.\n \t\t */\n \t\tstruct diff_options opts;\n --\t\tstruct strvec other_arg = STRVEC_INIT;\n +-\t\tstruct strvec log_arg = STRVEC_INIT;\n \t\tstruct range_diff_options range_diff_opts = {\n \t\t\t.creation_factor = rev->creation_factor,\n \t\t\t.dual_color = 1,\n \t\t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n \t\t\t.diffopt = &opts,\n --\t\t\t.other_arg = &other_arg\n -+\t\t\t.other_arg = &rev->rdiff_other_arg\n +-\t\t\t.log_arg = &log_arg\n ++\t\t\t.log_arg = &rev->rdiff_log_arg\n \t\t};\n \n \t\trepo_diff_setup(the_repository, &opts);\n @@ builtin/log.c: static void make_cover_letter(struct rev_info *rev, int use_separ\n \t\topts.use_color = rev->diffopt.use_color;\n \t\tdiff_setup_done(&opts);\n \t\tfprintf_ln(rev->diffopt.file, \"%s\", rev->rdiff_title);\n --\t\tget_notes_args(&other_arg, rev);\n +-\t\tget_notes_args(&log_arg, rev);\n \t\tshow_range_diff(rev->rdiff1, rev->rdiff2, &range_diff_opts);\n --\t\tstrvec_clear(&other_arg);\n +-\t\tstrvec_clear(&log_arg);\n \t}\n }\n \n @@ builtin/log.c: int cmd_format_patch(int argc,\n \t\trev.rdiff_title = diff_title(&rdiff_title, reroll_count,\n \t\t\t\t\t _(\"Range-diff:\"),\n \t\t\t\t\t _(\"Range-diff against v%d:\"));\n -+\t\tget_notes_args(&(rev.rdiff_other_arg), &rev);\n ++\t\tget_notes_args(&(rev.rdiff_log_arg), &rev);\n \t}\n \n \t/*\n @@ builtin/log.c: int cmd_format_patch(int argc,\n \trev.diffopt.no_free = 0;\n \trelease_revisions(&rev);\n \tformat_config_release(&cfg);\n -+\tstrvec_clear(&rev.rdiff_other_arg);\n ++\tstrvec_clear(&rev.rdiff_log_arg);\n \treturn 0;\n }\n \n @@ revision.h: struct rev_info {\n \t/* range-diff */\n \tconst char *rdiff1;\n \tconst char *rdiff2;\n -+\tstruct strvec rdiff_other_arg;\n ++\tstruct strvec rdiff_log_arg;\n \tint creation_factor;\n \tconst char *rdiff_title;\n \n +@@ revision.h: struct rev_info {\n + \t.expand_tabs_in_log = -1, \\\n + \t.commit_format = CMIT_FMT_DEFAULT, \\\n + \t.expand_tabs_in_log_default = 8, \\\n ++\t.rdiff_log_arg = STRVEC_INIT, \\\n + }\n + \n + /**\n2: 7f2487af433 ! 3: 2be637081d4 format-patch: handle range-diff on notes correctly for single patches\n @@ Metadata\n ## Commit message ##\n format-patch: handle range-diff on notes correctly for single patches\n \n - No `--[no-]notes` options are sent to the range-diff subprocess in\n - `range-diff.c` when making a single patch. This means that you can get\n - different Git notes below the commit message and in the range-diff\n - part. (See the previous commit for elaboration.)\n + (The two next paragraphs are taken from the previous commit.)\n \n - Use the struct member that we introduced and populated in the\n + git-format-patch(1) supports Git notes by showing them beneath the\n + patch/commit message, similar to git-log(1). The command also supports\n + showing those same notes ref names in the range diff output.\n +\n + Note *the same* ref names; any Git notes options or configuration\n + variables need to be handed off to the range-diff machinery. This works\n + correctly in the case when the range diff is on the cover letter. But it\n + does not work correctly when the output is a single patch with an\n + embedded range diff.\n +\n + Concretely, git-format-patch(1) needs to pass `--[no-]notes` options on\n + to the range-diff subprocess in `range-diff.c`. Range diffs for single-\n + commit series are handled in `log-tree.c`. But `log-tree.c` had no\n + access to any `log_arg` variable before we added it to `rev_info` in the\n previous commit.\n \n + Use that new struct member to fix this inconsistency.\n +\n Signed-off-by: Kristoffer Haugsbakk \n \n \n ## Notes (series) ##\n + v1:\n +\n I’ve tried to conform to 6caa96c2 (t3206: test_when_finished before\n dirtying operations, not after, 2024-08-06) in the test here.\n \n @@ log-tree.c: static void show_diff_of_diff(struct rev_info *opt)\n \t\t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n -\t\t\t.diffopt = &opts\n +\t\t\t.diffopt = &opts,\n -+\t\t\t.other_arg = &opt->rdiff_other_arg\n ++\t\t\t.log_arg = &opt->rdiff_log_arg\n \t\t};\n \n \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n\nbase-commit: ca2559c1d630eb4f04cdee2328aaf1c768907a9e\n-- \n2.51.0.311.g9b2318464ce\n\n"},{"id":"527338","messageId":"bd037df14f5.1758819879.git.code@khaugsbakk.name","threadId":"64183","inReplyTo":"v2-cover.1758819879.git.code@khaugsbakk.name","subject":"[PATCH v2 1/3] range-diff: rename other_arg to log_arg","fromName":"","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-25T17:07:34Z","receivedAt":"2025-09-25T17:08:34Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"From: Kristoffer Haugsbakk \n\nRename `other_arg` to `log_arg` in `range_diff_options` and\nrelated places.\n\n“Other argument” comes from bd361918 (range-diff: pass through --notes\nto `git log`, 2019-11-20) which introduced Git notes handling to\ngit-range-diff(1) by passing that option on to git-log(1). And that kind\nof name might be fine in a local context. However, it was initially\nspread among multiple files, and is now[1] part of the\n`range_diff_options` struct. It is, prima facie, difficult to guess what\n“other” means, especially when just looking at the struct.\n\nBut with a little reading we find out that it is used for `--[no-]notes`\nand `--diff-merges`, which are both passed on to git-log(1). We should\njust rename it to reflect this role; `log_arg` suggests, along with the\n`strvec` type, that it is used to pass extra arguments to git-log(1).\n\n† 1: since f1ce6c19 (range-diff: combine all options in a single data\n structure, 2021-02-05)\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v2 (new):\n \n Add a preliminary commit after discussing the `other_arg` name with\n Junio.\n \n “ Back when it was a random one-shot variable in range-diff, it might\n not have mattered all that much, but now we have it as a proper\n member of the struct, can we give it a name better than 'other_arg\"?\n \n Link: https://lore.kernel.org/git/xmqqikharvyl.fsf@gitster.g/\n \n Later:\n \n “ My personal preference is to name arrays singular so that you can\n name its 0th element by saying dog[0], not dogs[0]. \"dog[1] and\n dog[2] are friends\" not dogs[1] and dogs[2]. An exception is when\n most of the time you use the array as a single unit as a collection,\n passing it around in the call chain, rarely addressing each individual\n element. I am OK to see such an array called plural (but of course,\n singular names are always fine)..\n \n And I chose singular to personal preference in this context (the use\n etc. for this kind of variable).\n\n builtin/log.c | 8 ++++----\n builtin/range-diff.c | 16 ++++++++--------\n range-diff.c | 10 +++++-----\n range-diff.h | 2 +-\n 4 files changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 5f552d14c0f..131512ac1af 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1400,13 +1400,13 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\t * can be added later if deemed desirable.\n \t\t */\n \t\tstruct diff_options opts;\n-\t\tstruct strvec other_arg = STRVEC_INIT;\n+\t\tstruct strvec log_arg = STRVEC_INIT;\n \t\tstruct range_diff_options range_diff_opts = {\n \t\t\t.creation_factor = rev->creation_factor,\n \t\t\t.dual_color = 1,\n \t\t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n \t\t\t.diffopt = &opts,\n-\t\t\t.other_arg = &other_arg\n+\t\t\t.log_arg = &log_arg\n \t\t};\n \n \t\trepo_diff_setup(the_repository, &opts);\n@@ -1414,9 +1414,9 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\topts.use_color = rev->diffopt.use_color;\n \t\tdiff_setup_done(&opts);\n \t\tfprintf_ln(rev->diffopt.file, \"%s\", rev->rdiff_title);\n-\t\tget_notes_args(&other_arg, rev);\n+\t\tget_notes_args(&log_arg, rev);\n \t\tshow_range_diff(rev->rdiff1, rev->rdiff2, &range_diff_opts);\n-\t\tstrvec_clear(&other_arg);\n+\t\tstrvec_clear(&log_arg);\n \t}\n }\n \ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex aafcc99b962..f88b40e3607 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -37,13 +37,13 @@ int cmd_range_diff(int argc,\n \t\t struct repository *repo UNUSED)\n {\n \tstruct diff_options diffopt = { NULL };\n-\tstruct strvec other_arg = STRVEC_INIT;\n+\tstruct strvec log_arg = STRVEC_INIT;\n \tstruct strvec diff_merges_arg = STRVEC_INIT;\n \tstruct range_diff_options range_diff_opts = {\n \t\t.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,\n \t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n \t\t.diffopt = &diffopt,\n-\t\t.other_arg = &other_arg\n+\t\t.log_arg = &log_arg\n \t};\n \tint simple_color = -1, left_only = 0, right_only = 0;\n \tstruct option range_diff_options[] = {\n@@ -52,7 +52,7 @@ int cmd_range_diff(int argc,\n \t\t\t N_(\"percentage by which creation is weighted\")),\n \t\tOPT_BOOL(0, \"no-dual-color\", &simple_color,\n \t\t\t N_(\"use simple diff colors\")),\n-\t\tOPT_PASSTHRU_ARGV(0, \"notes\", &other_arg,\n+\t\tOPT_PASSTHRU_ARGV(0, \"notes\", &log_arg,\n \t\t\t\t N_(\"notes\"), N_(\"passed to 'git log'\"),\n \t\t\t\t PARSE_OPT_OPTARG),\n \t\tOPT_PASSTHRU_ARGV(0, \"diff-merges\", &diff_merges_arg,\n@@ -92,7 +92,7 @@ int cmd_range_diff(int argc,\n \t/* If `--diff-merges` was specified, imply `--merges` */\n \tif (diff_merges_arg.nr) {\n \t\trange_diff_opts.include_merges = 1;\n-\t\tstrvec_pushv(&other_arg, diff_merges_arg.v);\n+\t\tstrvec_pushv(&log_arg, diff_merges_arg.v);\n \t}\n \n \tfor (i = 0; i < argc; i++)\n@@ -124,7 +124,7 @@ int cmd_range_diff(int argc,\n \t\tstrbuf_addf(&range1, \"%s..%s\", argv[0], argv[1]);\n \t\tstrbuf_addf(&range2, \"%s..%s\", argv[0], argv[2]);\n \n-\t\tstrvec_pushv(&other_arg, argv +\n+\t\tstrvec_pushv(&log_arg, argv +\n \t\t\t (dash_dash < 0 ? 3 : dash_dash));\n \t} else if (dash_dash == 2 ||\n \t\t (dash_dash < 0 && argc > 1 &&\n@@ -144,7 +144,7 @@ int cmd_range_diff(int argc,\n \t\tstrbuf_addstr(&range1, argv[0]);\n \t\tstrbuf_addstr(&range2, argv[1]);\n \n-\t\tstrvec_pushv(&other_arg, argv +\n+\t\tstrvec_pushv(&log_arg, argv +\n \t\t\t (dash_dash < 0 ? 2 : dash_dash));\n \t} else if (dash_dash == 1 ||\n \t\t (dash_dash < 0 && argc > 0 &&\n@@ -175,7 +175,7 @@ int cmd_range_diff(int argc,\n \t\tstrbuf_addf(&range1, \"%s..%.*s\", b, a_len, a);\n \t\tstrbuf_addf(&range2, \"%.*s..%s\", a_len, a, b);\n \n-\t\tstrvec_pushv(&other_arg, argv +\n+\t\tstrvec_pushv(&log_arg, argv +\n \t\t\t (dash_dash < 0 ? 1 : dash_dash));\n \t} else\n \t\tusage_msg_opt(_(\"need two commit ranges\"),\n@@ -187,7 +187,7 @@ int cmd_range_diff(int argc,\n \trange_diff_opts.right_only = right_only;\n \tres = show_range_diff(range1.buf, range2.buf, &range_diff_opts);\n \n-\tstrvec_clear(&other_arg);\n+\tstrvec_clear(&log_arg);\n \tstrvec_clear(&diff_merges_arg);\n \tstrbuf_release(&range1);\n \tstrbuf_release(&range2);\ndiff --git a/range-diff.c b/range-diff.c\nindex ca449a07693..57edff40a85 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -39,7 +39,7 @@ struct patch_util {\n * as struct object_id (will need to be free()d).\n */\n static int read_patches(const char *range, struct string_list *list,\n-\t\t\tconst struct strvec *other_arg,\n+\t\t\tconst struct strvec *log_arg,\n \t\t\tunsigned int include_merges)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -69,8 +69,8 @@ static int read_patches(const char *range, struct string_list *list,\n \tif (!include_merges)\n \t\tstrvec_push(&cp.args, \"--no-merges\");\n \tstrvec_push(&cp.args, range);\n-\tif (other_arg)\n-\t\tstrvec_pushv(&cp.args, other_arg->v);\n+\tif (log_arg)\n+\t\tstrvec_pushv(&cp.args, log_arg->v);\n \tcp.out = -1;\n \tcp.no_stdin = 1;\n \tcp.git_cmd = 1;\n@@ -594,9 +594,9 @@ int show_range_diff(const char *range1, const char *range2,\n \tif (range_diff_opts->left_only && range_diff_opts->right_only)\n \t\tres = error(_(\"options '%s' and '%s' cannot be used together\"), \"--left-only\", \"--right-only\");\n \n-\tif (!res && read_patches(range1, &branch1, range_diff_opts->other_arg, include_merges))\n+\tif (!res && read_patches(range1, &branch1, range_diff_opts->log_arg, include_merges))\n \t\tres = error(_(\"could not parse log for '%s'\"), range1);\n-\tif (!res && read_patches(range2, &branch2, range_diff_opts->other_arg, include_merges))\n+\tif (!res && read_patches(range2, &branch2, range_diff_opts->log_arg, include_merges))\n \t\tres = error(_(\"could not parse log for '%s'\"), range2);\n \n \tif (!res) {\ndiff --git a/range-diff.h b/range-diff.h\nindex 9d39818e349..9b70a80009e 100644\n--- a/range-diff.h\n+++ b/range-diff.h\n@@ -23,7 +23,7 @@ struct range_diff_options {\n \tunsigned include_merges:1;\n \tsize_t max_memory;\n \tconst struct diff_options *diffopt; /* may be NULL */\n-\tconst struct strvec *other_arg; /* may be NULL */\n+\tconst struct strvec *log_arg; /* may be NULL */\n };\n \n /*\n-- \n2.51.0.311.g9b2318464ce\n\n"},{"id":"527339","messageId":"d9419743773.1758819879.git.code@khaugsbakk.name","threadId":"64183","inReplyTo":"v2-cover.1758819879.git.code@khaugsbakk.name","subject":"[PATCH v2 2/3] revision: add rdiff_log_arg to rev_info","fromName":"","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-25T17:07:35Z","receivedAt":"2025-09-25T17:08:52Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"From: Kristoffer Haugsbakk \n\ngit-format-patch(1) supports Git notes by showing them beneath the\npatch/commit message, similar to git-log(1). The command also supports\nshowing those same notes ref names in the range diff output.\n\nNote *the same* ref names; any Git notes options or configuration\nvariables need to be handed off to the range-diff machinery. This works\ncorrectly in the case when the range diff is on the cover letter. But it\ndoes not work correctly when the output is a single patch with an\nembedded range diff.\n\nConcretely, git-format-patch(1) needs to pass `--[no-]notes` options\non to the range-diff subprocess in `range-diff.c`. This is handled in\n`builtin/log.c` by the local variable `log_arg` in the case of mul-\ntiple commits, but not in the single commit case where there is no\ncover letter and the range diff is embedded in the patch output; the\nrange diff is then made in `log-tree.c`, whither `log_arg` has not\nbeen propagated. This means that the range-diff subprocess reverts\nto its default behavior, which is to act like git-log(1) w.r.t. notes.\n\nWe need to fix this. But first lay the groundwork by converting\n`log_arg` to a struct member; next we can simply use that member\nin `log-tree.c` without having to thread it from `builtin/log.c`.\n\nNo functional changes.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v2:\n \n Rewrite the commit message. The message jumps into the problem without\n setting the stage. I think the problem should be presented for a future\n reader with only some basic prerequisite knowledge of this area; they\n might use git-format-patch(1), but they might not use Git notes *with*\n format-patch.[1] For that reason, start with two paragraphs that pre-\n sent how notes are handled. Then continue with the story in a format\n similar to v1.\n \n Note that the different “range diff” and “range-diff” spellings are\n intentional. “Range diff” here refers to the diff output while\n “range-diff” refers to the machinery that creates that output.\n \n † 1: I don’t really see much Git notes use on patches on this mailing\n list. And even less with non-default namespaces. But in any case:\n the stage should be set properly even if regulars here *did* use\n format-patch notes a lot.\n \n Also fix handling of struct-in-struct (helped by Junio).\n \n • Rewrite the commit message\n • Fix struct-in-struct\n • (And) Reset author date. I started this in June but the time\n investment is mostly from these last days.\n \n v1:\n \n There is also `other_arg` in `builtin/range-diff.c` but `rev_info` does\n not seem to be involved.\n\n builtin/log.c | 7 +++----\n revision.h | 2 ++\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 131512ac1af..9eff62ce111 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1400,13 +1400,12 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\t * can be added later if deemed desirable.\n \t\t */\n \t\tstruct diff_options opts;\n-\t\tstruct strvec log_arg = STRVEC_INIT;\n \t\tstruct range_diff_options range_diff_opts = {\n \t\t\t.creation_factor = rev->creation_factor,\n \t\t\t.dual_color = 1,\n \t\t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n \t\t\t.diffopt = &opts,\n-\t\t\t.log_arg = &log_arg\n+\t\t\t.log_arg = &rev->rdiff_log_arg\n \t\t};\n \n \t\trepo_diff_setup(the_repository, &opts);\n@@ -1414,9 +1413,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\topts.use_color = rev->diffopt.use_color;\n \t\tdiff_setup_done(&opts);\n \t\tfprintf_ln(rev->diffopt.file, \"%s\", rev->rdiff_title);\n-\t\tget_notes_args(&log_arg, rev);\n \t\tshow_range_diff(rev->rdiff1, rev->rdiff2, &range_diff_opts);\n-\t\tstrvec_clear(&log_arg);\n \t}\n }\n \n@@ -2328,6 +2325,7 @@ int cmd_format_patch(int argc,\n \t\trev.rdiff_title = diff_title(&rdiff_title, reroll_count,\n \t\t\t\t\t _(\"Range-diff:\"),\n \t\t\t\t\t _(\"Range-diff against v%d:\"));\n+\t\tget_notes_args(&(rev.rdiff_log_arg), &rev);\n \t}\n \n \t/*\n@@ -2487,6 +2485,7 @@ int cmd_format_patch(int argc,\n \trev.diffopt.no_free = 0;\n \trelease_revisions(&rev);\n \tformat_config_release(&cfg);\n+\tstrvec_clear(&rev.rdiff_log_arg);\n \treturn 0;\n }\n \ndiff --git a/revision.h b/revision.h\nindex 21e288c5baa..ce30570d86a 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -334,6 +334,7 @@ struct rev_info {\n \t/* range-diff */\n \tconst char *rdiff1;\n \tconst char *rdiff2;\n+\tstruct strvec rdiff_log_arg;\n \tint creation_factor;\n \tconst char *rdiff_title;\n \n@@ -410,6 +411,7 @@ struct rev_info {\n \t.expand_tabs_in_log = -1, \\\n \t.commit_format = CMIT_FMT_DEFAULT, \\\n \t.expand_tabs_in_log_default = 8, \\\n+\t.rdiff_log_arg = STRVEC_INIT, \\\n }\n \n /**\n-- \n2.51.0.311.g9b2318464ce\n\n"},{"id":"527341","messageId":"2be637081d4.1758819879.git.code@khaugsbakk.name","threadId":"64183","inReplyTo":"v2-cover.1758819879.git.code@khaugsbakk.name","subject":"[PATCH v2 3/3] format-patch: handle range-diff on notes correctly for single patches","fromName":"","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-25T17:07:36Z","receivedAt":"2025-09-25T17:09:11Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"From: Kristoffer Haugsbakk \n\n(The two next paragraphs are taken from the previous commit.)\n\ngit-format-patch(1) supports Git notes by showing them beneath the\npatch/commit message, similar to git-log(1). The command also supports\nshowing those same notes ref names in the range diff output.\n\nNote *the same* ref names; any Git notes options or configuration\nvariables need to be handed off to the range-diff machinery. This works\ncorrectly in the case when the range diff is on the cover letter. But it\ndoes not work correctly when the output is a single patch with an\nembedded range diff.\n\nConcretely, git-format-patch(1) needs to pass `--[no-]notes` options on\nto the range-diff subprocess in `range-diff.c`. Range diffs for single-\ncommit series are handled in `log-tree.c`. But `log-tree.c` had no\naccess to any `log_arg` variable before we added it to `rev_info` in the\nprevious commit.\n\nUse that new struct member to fix this inconsistency.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v1:\n \n I’ve tried to conform to 6caa96c2 (t3206: test_when_finished before\n dirtying operations, not after, 2024-08-06) in the test here.\n\n log-tree.c | 3 ++-\n t/t3206-range-diff.sh | 16 +++++++++++++++-\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 73d21f71764..3d38c748e45 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -718,7 +718,8 @@ static void show_diff_of_diff(struct rev_info *opt)\n \t\t\t.creation_factor = opt->creation_factor,\n \t\t\t.dual_color = 1,\n \t\t\t.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,\n-\t\t\t.diffopt = &opts\n+\t\t\t.diffopt = &opts,\n+\t\t\t.log_arg = &opt->rdiff_log_arg\n \t\t};\n \n \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex e091df6d01d..1e812df806b 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -707,7 +707,7 @@ test_expect_success 'format-patch --range-diff does not compare notes by default\n \t! grep \"note\" 0000-*\n '\n \n-test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' '\n+test_expect_success 'format-patch --notes=custom --range-diff --cover-letter only compares custom notes' '\n \ttest_when_finished \"git notes remove topic unmodified || :\" &&\n \tgit notes add -m \"topic note\" topic &&\n \tgit notes add -m \"unmodified note\" unmodified &&\n@@ -721,6 +721,20 @@ test_expect_success 'format-patch --notes=custom --range-diff only compares cust\n \t! grep \"## Notes ##\" 0000-*\n '\n \n+# --range-diff on a single commit requires --no-cover-letter\n+test_expect_success 'format-patch --notes=custom --range-diff on single commit only compares custom notes' '\n+\ttest_when_finished \"git notes remove HEAD unmodified || :\" &&\n+\tgit notes add -m \"topic note\" HEAD &&\n+\ttest_when_finished \"git notes --ref=custom remove HEAD unmodified || :\" &&\n+\tgit notes add -m \"unmodified note\" unmodified &&\n+\tgit notes --ref=custom add -m \"topic note (custom)\" HEAD &&\n+\tgit notes --ref=custom add -m \"unmodified note (custom)\" unmodified &&\n+\tgit format-patch --notes=custom --range-diff=$prev \\\n+\t\t-1 --stdout >actual &&\n+\ttest_grep \"## Notes (custom) ##\" actual &&\n+\ttest_grep ! \"## Notes ##\" actual\n+'\n+\n test_expect_success 'format-patch --range-diff with --no-notes' '\n \ttest_when_finished \"git notes remove topic unmodified || :\" &&\n \tgit notes add -m \"topic note\" topic &&\n-- \n2.51.0.311.g9b2318464ce\n\n"}]}