Re: [PATCH v3 1/2] format-patch: simplify get_notes_arg parameters
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 2, 2026, 16:50 UTC
- Message-ID
- <xmqqtsn4xd17.fsf@gitster.g>
- In-Reply-To
- <V3_simplify_params.d3a@m5gid.xyz>
kristofferhaugsbakk@fastmail.com writes:
Show 8 quoted lines
> From: Kristoffer Haugsbakk <code@khaugsbakk.name> > > 85bd88a7 (revision: add rdiff_log_arg to rev_info, 2025-09-25) added > `rdiff_log_arg` to `struct rev_info`. I changed `get_notes_arg` by > simply replacing the first argument with an access on this struct > member. But the second argument was already `struct rev_info`. So I > should have just simplified to *only* passing that parameter. Let’s do > that now.
The readers do not necessarily want to read the "author's journey" narrative in log messages. Let's be more detached and objective, like
85bd88a7e8 (revision: add rdiff_log_arg to rev_info, 2025-09-25) updated get_notes_args() to push into rev->rdiff_log_arg instead of an explicit strvec, but left the rev argument as the second parameter and strvec *arg as the first. Simplify the signature of get_notes_args() to take only struct rev_info *rev, dropping the redundant strvec *arg parameter.
perhaps?
Show 8 quoted lines
> Now is also a good time to format this `for_each...` line since it’s > gotten quite long. > > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > --- > > Notes (testing): > just compile tested
The code change looks good. As long as this stays as a static helper function, this is not a loss of flexibility but a simplification of the calling convention.
Show 39 quoted lines
> builtin/log.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 350b35c5563..560af00e2fd 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1333,16 +1333,18 @@ static int get_notes_refs(struct string_list_item *item, void *arg)
> return 0;
> }
>
> -static void get_notes_args(struct strvec *arg, struct rev_info *rev)
> +static void get_notes_args(struct rev_info *rev)
> {
> if (!rev->show_notes) {
> - strvec_push(arg, "--no-notes");
> + strvec_push(&rev->rdiff_log_arg, "--no-notes");
> } else if (rev->notes_opt.use_default_notes > 0 ||
> (rev->notes_opt.use_default_notes == -1 &&
> !rev->notes_opt.extra_notes_refs.nr)) {
> - strvec_push(arg, "--notes");
> + strvec_push(&rev->rdiff_log_arg, "--notes");
> } else {
> - for_each_string_list(&rev->notes_opt.extra_notes_refs, get_notes_refs, arg);
> + for_each_string_list(&rev->notes_opt.extra_notes_refs,
> + get_notes_refs,
> + &rev->rdiff_log_arg);
> }
> }
>
> @@ -2404,7 +2406,7 @@ int cmd_format_patch(int argc,
> rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
> _("Range-diff:"),
> _("Range-diff against v%d:"));
> - get_notes_args(&(rev.rdiff_log_arg), &rev);
> + get_notes_args(&rev);
> }
>
> /*