From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 02 Oct 2026 16:50:12 GMT
Subject: Re: [PATCH v3 1/2] format-patch: simplify get_notes_arg parameters
Message-ID: <xmqqtsn4xd17.fsf@gitster.g>
In-Reply-To: <V3_simplify_params.d3a@m5gid.xyz>

kristofferhaugsbakk@fastmail.com writes:

> 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?

> 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.

>  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);
>  	}
>  
>  	/*