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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]

On Thu, Aug 08, 2024 at 11:07:53AM +0100, Phillip Wood wrote:
> 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.

Okay. In that case I'll just drop the comment. Thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]