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

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