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:
signature.asc
Description: PGP signature
