Hi Ævar On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
Make the recently added replay_opts_release() function non-static and use it for freeing the "struct replay_opts" constructed by the get_replay_opts() function in "builtin/rebase.c". See [1] for the initial addition of get_replay_opts(). To safely call our new replay_opts_release() we'll need to change all the free() to a FREE_AND_NULL(), and set "xopts_nr" to "0" after we loop over it and free() it (the free() in the loop doesn't need to be a FREE_AND_NULL()). This is because in e.g. do_interactive_rebase() we construct a "struct replay_opts" with "get_replay_opts()", and then call "complete_action()". If we get far enough in that function without encountering errors we'll call "pick_commits()" which (indirectly) calls sequencer_remove_state() at the end. But if we encounter errors anywhere along the way we'd punt out early, and not free() the memory we allocated. Remembering whether we previously called sequencer_remove_state() would be a hassle, so let's make it safe to re-invoke replay_opts_release() instead. I experimented with a change to be more paranoid instead, i.e. to exhaustively check our state via an enum. We could make sure that we: - Only allow calling "replay_opts_release()" after "sequencer_remove_state()", but not the other way around. - Forbid invoking either function twice in a row. But such paranoia isn't warranted here, let's instead take the easy way out and FREE_AND_NULL() this.
The changes below look good, but this message needs updating to reflect the re-roll.
@@ -359,6 +359,7 @@ static void replay_opts_release(struct replay_opts *opts) free(opts->strategy); for (size_t i = 0; i < opts->xopts_nr; i++) free(opts->xopts[i]); + opts->xopts_nr = 0;
I don't think we need this now we're only calling replay_opts_release() once.
Best Wishes Phillip
