Re: [PATCH 09/22] builtin/rebase: fix leaking `commit.gpgsign` value

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Patrick

On 06/08/2024 10:00, Patrick Steinhardt wrote:
> @@ -186,7 +186,15 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>   	replay.committer_date_is_author_date =
>   					opts->committer_date_is_author_date;
>   	replay.ignore_date = opts->ignore_date;
> +
> +	/*
> +	 * TODO: Is it really intentional that we unconditionally override
> +	 * `replay.gpg_sign` even if it has already been initialized via the
> +	 * configuration?
> +	 */
> +	free(replay.gpg_sign);
>   	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
> +

The code that handles "-S" could certainly be clearer. The value 
returned from the config is either "" or NULL, not a key name. In 
cmd_main() options.gpg_sign_opt is initialized by rebase_config(), we 
set gpg_sign to "" if options.gpg_sign_opt is non-NULL, free 
options.gpg_sign_opt and then copy gpg_sign back into 
options.gpg_sign_opt after parsing the command line so we're not losing 
anything by unconditionally copying it here. The code changes look good, 
though I'm not sure we need to add the blank lines. It's always nice to 
see more tests marked as leak-free especially a big file like t3404.

Best Wishes

Phillip