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
