patch, 10 partssequencer API & users: fix widespread leaks

> and sets
> us up for a subsequent commit which'll make further use of the
> "cleanup" label.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 1481c5b6a5b..7141fd5e0c1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -256,7 +256,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)
>   
>   static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   {
> -	int ret;
> +	int ret = -1;
>   	char *revisions = NULL, *shortrevisions = NULL;
>   	struct strvec make_script_args = STRVEC_INIT;
>   	struct todo_list todo_list = TODO_LIST_INIT;
> @@ -265,16 +265,12 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   
>   	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
>   				&revisions, &shortrevisions))
> -		return -1;
> +		goto cleanup;

>   	if (init_basic_state(&replay,
>   			     opts->head_name ? opts->head_name : "detached HEAD",
> -			     opts->onto, &opts->orig_head->object.oid)) {
> -		free(revisions);
> -		free(shortrevisions);
> -
> -		return -1;
> -	}
> +			     opts->onto, &opts->orig_head->object.oid))
> +		goto cleanup;
>   
>   	if (!opts->upstream && opts->squash_onto)
>   		write_file(path_squash_onto(), "%s\n",
> @@ -304,6 +300,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   			opts->autosquash, opts->update_refs, &todo_list);
>   	}
>   
> +cleanup:
>   	string_list_clear(&commands, 0);
>   	free(revisions);
>   	free(shortrevisions);

> Split off the free()-ing in sequencer_remove_state() into a utility
> function, which will be adjusted and called independent of the other
> code in sequencer_remove_state() in a subsequent commit.
>
> The only functional changes here are:
>
>  * Changing the "int" to a "size_t", which is the correct type, as
>    "xopts_nr" is a "size_t".

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  sequencer.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index bcb662e23be..655ae9f1a72 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -351,10 +351,24 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>  	return buf.buf;
>  }
>
> +static void replay_opts_release(struct replay_opts *opts)
> +{
> +	free(opts->gpg_sign);
> +	free(opts->reflog_action);
> +	free(opts->default_strategy);
> +	free(opts->strategy);
> +	for (size_t i = 0; i < opts->xopts_nr; i++)
> +		free(opts->xopts[i]);
> +	free(opts->xopts);
> +	strbuf_release(&opts->current_fixups);
> +}
> +
>  int sequencer_remove_state(struct replay_opts *opts)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> -	int i, ret = 0;
> +	int ret = 0;
> +
> +	replay_opts_release(opts);
>
>  	if (is_rebase_i(opts) &&
>  	    strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
> @@ -373,15 +387,6 @@ int sequencer_remove_state(struct replay_opts *opts)
>  		}
>  	}
>
> -	free(opts->gpg_sign);
> -	free(opts->reflog_action);
> -	free(opts->default_strategy);
> -	free(opts->strategy);
> -	for (i = 0; i < opts->xopts_nr; i++)
> -		free(opts->xopts[i]);
> -	free(opts->xopts);
> -	strbuf_release(&opts->current_fixups);
> -
>  	strbuf_reset(&buf);
>  	strbuf_addstr(&buf, get_dir(opts));
>  	if (remove_dir_recursively(&buf, 0))

> Am 30.12.22 um 08:28 schrieb Ævar Arnfjörð Bjarmason:
>> Split off the free()-ing in sequencer_remove_state() into a utility
>> function, which will be adjusted and called independent of the other
>> code in sequencer_remove_state() in a subsequent commit.
>>
>> The only functional changes here are:
>>
>>   * Changing the "int" to a "size_t", which is the correct type, as
>>     "xopts_nr" is a "size_t".
> 
> Good, and you declare it in the for statement, which we can do now!
> 
>>
>>   * Calling the free() before the "if (is_rebase_i(opts) && ...)",
>>     which is OK, and makes a subsequent change smaller.
> 
> It's true that is_rebase_i() can be called after all that free()ing;
> here is its definition:
> 
> 	static inline int is_rebase_i(const struct replay_opts *opts)
> 	{
> 		return opts->action == REPLAY_INTERACTIVE_REBASE;
> 	}
> 
> But why?  Making a subsequent change smaller is just a trivial fact if
> you do a part if it earlier, but that in itself is not a valid reason
> for the reordering.

> And I can't find that patch -- sequencer_remove_state() is not touched
> again in this series.
> 
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   sequencer.c | 25 +++++++++++++++----------
>>   1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index bcb662e23be..655ae9f1a72 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -351,10 +351,24 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>>   	return buf.buf;
>>   }
>>
>> +static void replay_opts_release(struct replay_opts *opts)
>> +{
>> +	free(opts->gpg_sign);
>> +	free(opts->reflog_action);
>> +	free(opts->default_strategy);
>> +	free(opts->strategy);
>> +	for (size_t i = 0; i < opts->xopts_nr; i++)
>> +		free(opts->xopts[i]);
>> +	free(opts->xopts);
>> +	strbuf_release(&opts->current_fixups);
>> +}
>> +
>>   int sequencer_remove_state(struct replay_opts *opts)
>>   {
>>   	struct strbuf buf = STRBUF_INIT;
>> -	int i, ret = 0;
>> +	int ret = 0;
>> +
>> +	replay_opts_release(opts);
>>
>>   	if (is_rebase_i(opts) &&
>>   	    strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
>> @@ -373,15 +387,6 @@ int sequencer_remove_state(struct replay_opts *opts)
>>   		}
>>   	}
>>
>> -	free(opts->gpg_sign);
>> -	free(opts->reflog_action);
>> -	free(opts->default_strategy);
>> -	free(opts->strategy);
>> -	for (i = 0; i < opts->xopts_nr; i++)
>> -		free(opts->xopts[i]);
>> -	free(opts->xopts);
>> -	strbuf_release(&opts->current_fixups);
>> -
>>   	strbuf_reset(&buf);
>>   	strbuf_addstr(&buf, get_dir(opts));
>>   	if (remove_dir_recursively(&buf, 0))

> 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.
> 
> See [2] for the initial implementation of "sequencer_remove_state()",
> which assumed that it should be removing the full (including on-disk)
> rebase state as a one-off.
> 
> 1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
>     2019-04-17)
> 2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
>     2011-08-04)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> Re the "I experimented with...", the earlier version can be seen at
> https://github.com/avar/git/commit/7a150d1b7e2; It's the commit I
> linked to from
> https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/
> (which is mentioned in the CL).
> 
>   builtin/rebase.c                       |  2 ++
>   sequencer.c                            | 13 +++++++------
>   sequencer.h                            |  1 +
>   t/t3405-rebase-malformed.sh            |  1 +
>   t/t3412-rebase-root.sh                 |  1 +
>   t/t3423-rebase-reword.sh               |  1 +
>   t/t3437-rebase-fixup-options.sh        |  1 +
>   t/t3438-rebase-broken-files.sh         |  2 ++
>   t/t7402-submodule-rebase.sh            |  1 +
>   t/t9106-git-svn-commit-diff-clobber.sh |  1 -
>   t/t9164-git-svn-dcommit-concurrent.sh  |  1 -
>   11 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 7141fd5e0c1..91bf55be6e6 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -301,6 +301,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   	}
>   
>   cleanup:
> +	replay_opts_release(&replay);
>   	string_list_clear(&commands, 0);
>   	free(revisions);
>   	free(shortrevisions);
> @@ -343,6 +344,7 @@ static int run_sequencer_rebase(struct rebase_options *opts)
>   		struct replay_opts replay_opts = get_replay_opts(opts);
>   
>   		ret = sequencer_continue(the_repository, &replay_opts);
> +		replay_opts_release(&replay_opts);
>   		break;
>   	}
>   	case ACTION_EDIT_TODO:
> diff --git a/sequencer.c b/sequencer.c
> index 655ae9f1a72..e29a97b6caa 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -351,15 +351,16 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>   	return buf.buf;
>   }
>   
> -static void replay_opts_release(struct replay_opts *opts)
> +void replay_opts_release(struct replay_opts *opts)
>   {
> -	free(opts->gpg_sign);
> -	free(opts->reflog_action);
> -	free(opts->default_strategy);
> -	free(opts->strategy);
> +	FREE_AND_NULL(opts->gpg_sign);
> +	FREE_AND_NULL(opts->reflog_action);
> +	FREE_AND_NULL(opts->default_strategy);
> +	FREE_AND_NULL(opts->strategy);
>   	for (size_t i = 0; i < opts->xopts_nr; i++)
>   		free(opts->xopts[i]);
> -	free(opts->xopts);
> +	opts->xopts_nr = 0;
> +	FREE_AND_NULL(opts->xopts);
>   	strbuf_release(&opts->current_fixups);
>   }
>   
> diff --git a/sequencer.h b/sequencer.h
> index 888c18aad71..3bcdfa1b586 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -158,6 +158,7 @@ int sequencer_pick_revisions(struct repository *repo,
>   int sequencer_continue(struct repository *repo, struct replay_opts *opts);
>   int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
>   int sequencer_skip(struct repository *repo, struct replay_opts *opts);
> +void replay_opts_release(struct replay_opts *opts);
>   int sequencer_remove_state(struct replay_opts *opts);
>   
>   #define TODO_LIST_KEEP_EMPTY (1U << 0)
> diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
> index 25243318618..8979bc34073 100755
> --- a/t/t3405-rebase-malformed.sh
> +++ b/t/t3405-rebase-malformed.sh
> @@ -5,6 +5,7 @@ test_description='rebase should handle arbitrary git message'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY"/lib-rebase.sh
>   
> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
> index 58371d8a547..e75b3d0e07c 100755
> --- a/t/t3412-rebase-root.sh
> +++ b/t/t3412-rebase-root.sh
> @@ -7,6 +7,7 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   log_with_names () {
> diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
> index 4859bb8f722..2fab703d615 100755
> --- a/t/t3423-rebase-reword.sh
> +++ b/t/t3423-rebase-reword.sh
> @@ -2,6 +2,7 @@
>   
>   test_description='git rebase interactive with rewording'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   . "$TEST_DIRECTORY"/lib-rebase.sh
> diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
> index c023fefd681..274699dadb8 100755
> --- a/t/t3437-rebase-fixup-options.sh
> +++ b/t/t3437-rebase-fixup-options.sh
> @@ -14,6 +14,7 @@ to the "fixup" command that works with "fixup!", "fixup -C" works with
>   "amend!" upon --autosquash.
>   '
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   . "$TEST_DIRECTORY"/lib-rebase.sh
> diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh
> index b92a3ce46b8..c614c4f2e4b 100755
> --- a/t/t3438-rebase-broken-files.sh
> +++ b/t/t3438-rebase-broken-files.sh
> @@ -1,6 +1,8 @@
>   #!/bin/sh
>   
>   test_description='rebase behavior when on-disk files are broken'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success 'set up conflicting branches' '
> diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
> index ebeca12a711..b19792b3269 100755
> --- a/t/t7402-submodule-rebase.sh
> +++ b/t/t7402-submodule-rebase.sh
> @@ -5,6 +5,7 @@
>   
>   test_description='Test rebasing, stashing, etc. with submodules'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success setup '
> diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
> index 3cab0b9720a..bca496c40e0 100755
> --- a/t/t9106-git-svn-commit-diff-clobber.sh
> +++ b/t/t9106-git-svn-commit-diff-clobber.sh
> @@ -3,7 +3,6 @@
>   # Copyright (c) 2006 Eric Wong
>   test_description='git svn commit-diff clobber'
>   
> -TEST_FAILS_SANITIZE_LEAK=true
>   . ./lib-git-svn.sh
>   
>   test_expect_success 'initialize repo' '
> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> index 1465156072e..c8e6c0733f4 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -5,7 +5,6 @@
>   
>   test_description='concurrent git svn dcommit'
>   
> -TEST_FAILS_SANITIZE_LEAK=true
>   . ./lib-git-svn.sh
>   
>   

> Refactor the return pattern in run_sequencer() to make it easier to
> insert a "replay_opts_release()" call between the "fn(...)" invocation
> and the eventual return.
>
> Usually we'd name the "cbfun" here "fn", but by using this name we'll
> nicely align all the "cmd == ?" comparisons.

> Use the replay_opts_release() function introduced in a preceding
> commit to fix a memory leak in run_sequencer().
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/revert.c                  | 1 +
>  t/t3419-rebase-patch-id.sh        | 1 +
>  t/t3425-rebase-topology-merges.sh | 2 ++
>  t/t3501-revert-cherry-pick.sh     | 1 +
>  t/t3502-cherry-pick-merge.sh      | 1 +
>  t/t3503-cherry-pick-root.sh       | 1 +
>  t/t3506-cherry-pick-ff.sh         | 1 +
>  t/t3511-cherry-pick-x.sh          | 1 +
>  8 files changed, 9 insertions(+)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index e956d125a2b..2f656b25619 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -240,6 +240,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>  		cmd == 's' ? sequencer_skip :
>  		sequencer_pick_revisions;
>  	ret = cbfun(the_repository, opts);
> +	replay_opts_release(opts);

>  	return ret;
>  }
>
> diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
> index 7181f176b81..6c61f240cf9 100755
> --- a/t/t3419-rebase-patch-id.sh
> +++ b/t/t3419-rebase-patch-id.sh
> @@ -5,6 +5,7 @@ test_description='git rebase - test patch id computation'
>  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  scramble () {
> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> index 63acc1ea4da..a16428bdf54 100755
> --- a/t/t3425-rebase-topology-merges.sh
> +++ b/t/t3425-rebase-topology-merges.sh
> @@ -1,6 +1,8 @@
>  #!/bin/sh
>
>  test_description='rebase topology tests with merges'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-rebase.sh
>
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 1f4cfc37449..2f3e3e24169 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -13,6 +13,7 @@ test_description='test cherry-pick and revert with renames
>  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  test_expect_success setup '
> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
> index 5495eacfec1..1b2c0d6aca6 100755
> --- a/t/t3502-cherry-pick-merge.sh
> +++ b/t/t3502-cherry-pick-merge.sh
> @@ -11,6 +11,7 @@ test_description='cherry picking and reverting a merge
>  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  test_expect_success setup '
> diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
> index 95fe4feaeee..76d393dc8a3 100755
> --- a/t/t3503-cherry-pick-root.sh
> +++ b/t/t3503-cherry-pick-root.sh
> @@ -5,6 +5,7 @@ test_description='test cherry-picking (and reverting) a root commit'
>  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  test_expect_success setup '
> diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
> index 7e11bd4a4c5..b71bad17b85 100755
> --- a/t/t3506-cherry-pick-ff.sh
> +++ b/t/t3506-cherry-pick-ff.sh
> @@ -5,6 +5,7 @@ test_description='test cherry-picking with --ff option'
>  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  test_expect_success setup '
> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
> index 84a587daf3a..dd5d92ef302 100755
> --- a/t/t3511-cherry-pick-x.sh
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -2,6 +2,7 @@
>
>  test_description='Test cherry-pick -x and -s'
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  pristine_detach () {

> Am 30.12.22 um 08:28 schrieb Ævar Arnfjörð Bjarmason:
>> Use the replay_opts_release() function introduced in a preceding
>> commit to fix a memory leak in run_sequencer().
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   builtin/revert.c                  | 1 +
>>   t/t3419-rebase-patch-id.sh        | 1 +
>>   t/t3425-rebase-topology-merges.sh | 2 ++
>>   t/t3501-revert-cherry-pick.sh     | 1 +
>>   t/t3502-cherry-pick-merge.sh      | 1 +
>>   t/t3503-cherry-pick-root.sh       | 1 +
>>   t/t3506-cherry-pick-ff.sh         | 1 +
>>   t/t3511-cherry-pick-x.sh          | 1 +
>>   8 files changed, 9 insertions(+)
>>
>> diff --git a/builtin/revert.c b/builtin/revert.c
>> index e956d125a2b..2f656b25619 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -240,6 +240,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>>   		cmd == 's' ? sequencer_skip :
>>   		sequencer_pick_revisions;
>>   	ret = cbfun(the_repository, opts);
>> +	replay_opts_release(opts);
> 
> Is this the right place for this call?  opts is passed in by this function's
> two callers.  They should clean it up instead, no?  That would add one line,
> but avoid the need for patch 4.  Would make patch 6 easier to read as well.

>>   	return ret;
>>   }
>>
>> diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
>> index 7181f176b81..6c61f240cf9 100755
>> --- a/t/t3419-rebase-patch-id.sh
>> +++ b/t/t3419-rebase-patch-id.sh
>> @@ -5,6 +5,7 @@ test_description='git rebase - test patch id computation'
>>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>
>> +TEST_PASSES_SANITIZE_LEAK=true
>>   . ./test-lib.sh
>>
>>   scramble () {
>> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
>> index 63acc1ea4da..a16428bdf54 100755
>> --- a/t/t3425-rebase-topology-merges.sh
>> +++ b/t/t3425-rebase-topology-merges.sh
>> @@ -1,6 +1,8 @@
>>   #!/bin/sh
>>
>>   test_description='rebase topology tests with merges'
>> +
>> +TEST_PASSES_SANITIZE_LEAK=true
>>   . ./test-lib.sh
>>   . "$TEST_DIRECTORY"/lib-rebase.sh
>>
>> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
>> index 1f4cfc37449..2f3e3e24169 100755
>> --- a/t/t3501-revert-cherry-pick.sh
>> +++ b/t/t3501-revert-cherry-pick.sh
>> @@ -13,6 +13,7 @@ test_description='test cherry-pick and revert with renames
>>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>
>> +TEST_PASSES_SANITIZE_LEAK=true
>>   . ./test-lib.sh
>>
>>   test_expect_success setup '
>> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
>> index 5495eacfec1..1b2c0d6aca6 100755
>> --- a/t/t3502-cherry-pick-merge.sh
>> +++ b/t/t3502-cherry-pick-merge.sh
>> @@ -11,6 +11,7 @@ test_description='cherry picking and reverting a merge
>>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>
>> +TEST_PASSES_SANITIZE_LEAK=true
>>   . ./test-lib.sh
>>
>>   test_expect_success setup '
>> diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
>> index 95fe4feaeee..76d393dc8a3 100755
>> --- a/t/t3503-cherry-pick-root.sh
>> +++ b/t/t3503-cherry-pick-root.sh
>> @@ -5,6 +5,7 @@ test_description='test cherry-picking (and reverting) a root commit'
>>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>
>> +TEST_PASSES_SANITIZE_LEAK=true
>>   . ./test-lib.sh
>>
>>   test_expect_success setup '
>> diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
>> index 7e11bd4a4c5..b71bad17b85 100755
>> --- a/t/t3506-cherry-pick-ff.sh
>> +++ b/t/t3506-cherry-pick-ff.sh
>> @@ -5,6 +5,7 @@ test_description='test cherry-picking with --ff option'
>>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>
>> +TEST_PASSES_SANITIZE_LEAK=true
>>   . ./test-lib.sh
>>
>>   test_expect_success setup '
>> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
>> index 84a587daf3a..dd5d92ef302 100755
>> --- a/t/t3511-cherry-pick-x.sh
>> +++ b/t/t3511-cherry-pick-x.sh
>> @@ -2,6 +2,7 @@
>>
>>   test_description='Test cherry-pick -x and -s'
>>
>> +TEST_PASSES_SANITIZE_LEAK=true
>>   . ./test-lib.sh
>>
>>   pristine_detach () {
> 

> In [1] we started saving away the earlier xstrdup()'d
> "options.onto_name" assignment to free() it, but when [2] added this
> "keep_base" branch it didn't free() the already assigned
> "squash_onto_name" before re-assigning to "options.onto_name". Let's
> do that, and fix the memory leak.

> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
> 2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c                 | 3 ++-
>   t/t3416-rebase-onto-threedots.sh | 1 +
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 91bf55be6e6..f7fd20d71c0 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1658,7 +1658,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		strbuf_addstr(&buf, options.upstream_name);
>   		strbuf_addstr(&buf, "...");
>   		strbuf_addstr(&buf, branch_name);
> -		options.onto_name = xstrdup(buf.buf);
> +		free(squash_onto_name);
> +		options.onto_name = squash_onto_name = xstrdup(buf.buf);
>   	} else if (!options.onto_name)
>   		options.onto_name = options.upstream_name;
>   	if (strstr(options.onto_name, "...")) {
> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index ea501f2b42b..f8c4ed78c9e 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY/lib-rebase.sh"
>   

> In [1] the strbuf_release(&msgbuf) was moved into this
> do_pick_commit(), but didn't take into account the case of [2], where
> we'd return before the strbuf_release(&msgbuf).
> 
> Then when the "fixup" support was added in [3] this leak got worse, as
> we added another place where we'd "return" before reaching the
> strbuf_release().

> Let's move it to a "cleanup" label, and use an appropriate "goto". It
> may or may not be safe to combine the existing "leave" and "cleanup"
> labels, but this change doesn't attempt to answer that question. Let's
> instead avoid calling update_abort_safety_file() in these cases, as we
> didn't do so before.

> 1. 452202c74b8 (sequencer: stop releasing the strbuf in
>     write_message(), 2016-10-21)
> 2. f241ff0d0a9 (prepare the builtins for a libified merge_recursive(),
>     2016-07-26)
> 3. 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
>     'squash' commands, 2017-01-02)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   sequencer.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 47367e66842..db8d789fa76 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2280,8 +2280,10 @@ static int do_pick_commit(struct repository *r,
>   		reword = 1;
>   	else if (is_fixup(command)) {
>   		if (update_squash_messages(r, command, commit,
> -					   opts, item->flags))
> -			return -1;
> +					   opts, item->flags)) {
> +			res = -1;
> +			goto cleanup;
> +		}
>   		flags |= AMEND_MSG;
>   		if (!final_fixup)
>   			msg_file = rebase_path_squash_msg();
> @@ -2291,9 +2293,11 @@ static int do_pick_commit(struct repository *r,
>   		} else {
>   			const char *dest = git_path_squash_msg(r);
>   			unlink(dest);
> -			if (copy_file(dest, rebase_path_squash_msg(), 0666))
> -				return error(_("could not rename '%s' to '%s'"),
> -					     rebase_path_squash_msg(), dest);
> +			if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
> +				res = error(_("could not rename '%s' to '%s'"),
> +					    rebase_path_squash_msg(), dest);
> +				goto cleanup;
> +			}
>   			unlink(git_path_merge_msg(r));
>   			msg_file = dest;
>   			flags |= EDIT_MSG;
> @@ -2331,7 +2335,6 @@ static int do_pick_commit(struct repository *r,
>   		free_commit_list(common);
>   		free_commit_list(remotes);
>   	}
> -	strbuf_release(&msgbuf);
>   
>   	/*
>   	 * If the merge was clean or if it failed due to conflict, we write
> @@ -2403,9 +2406,11 @@ static int do_pick_commit(struct repository *r,
>   	}
>   
>   leave:
> +	update_abort_safety_file();
> +cleanup:
>   	free_message(commit, &msg);
>   	free(author);
> -	update_abort_safety_file();
> +	strbuf_release(&msgbuf);
>   
>   	return res;
>   }

> This series fixes various widespread leaks in the sequencer and its
> users (rebase, revert, cherry-pick). As a result 18 tests become
> leak-free in their entirety.
> 
> The main change is the 3/10 here, where we introduce a
> replay_opts_release() to free the "struct replay_opts". The rest is
> then either refactorings to be able to call that destructor
> (e.g. "return" to "goto cleanup"), or other miscellanious adjacent
> leaks.
> 
> This is a follow-up to the discussion ending at [1], as noted there
> the recent ff84d031a9d (Merge branch 'pw/rebase-no-reflog-action',
> 2022-11-23) ended up introducing a leak because of the disfunctional
> lack of a destructor (or rather, the current logic being tied up in
> sequencer_remove_state().

> This can be queued and graduated independently of the other concurrent
> leak series I've submitted[2]. When the two are combined we'll end up
> passing more tests, i.e. both topics combined get us over the finish
> line for some of them, but neither one is enough in isolation.
> 
> But that's OK, we just won't opt them into the "linux-leaks"
> testing. I'll submit a follow-up similar to [3] at some later date to
> mark them as passing. I think that's a better trade-off than making
> these two depend on one another.
> 
> 1. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/
> 2. https://lore.kernel.org/git/cover-v2-00.20-00000000000-20221230T020341Z-avarab@gmail.com/
> 3. https://lore.kernel.org/git/patch-v2-01.20-3de29c6d75f-20221230T020341Z-avarab@gmail.com/
> 
> Ævar Arnfjörð Bjarmason (10):
>    rebase: use "cleanup" pattern in do_interactive_rebase()
>    sequencer.c: split up sequencer_remove_state()
>    rebase & sequencer API: fix get_replay_opts() leak in "rebase"
>    builtin/revert.c: refactor run_sequencer() return pattern
>    builtin/revert.c: fix common leak by using replay_opts_release()
>    builtin/revert.c: move free-ing of "revs" to replay_opts_release()
>    builtin/rebase.c: fix "options.onto_name" leak
>    sequencer.c: always free() the "msgbuf" in do_pick_commit()
>    builtin/rebase.c: free() "options.strategy_opts"
>    commit.c: free() revs.commit in get_fork_point()
> 
>   builtin/rebase.c                       | 19 +++++-----
>   builtin/revert.c                       | 40 +++++++++++----------
>   commit.c                               |  1 +
>   sequencer.c                            | 48 +++++++++++++++++---------
>   sequencer.h                            |  1 +
>   t/t3405-rebase-malformed.sh            |  1 +
>   t/t3412-rebase-root.sh                 |  1 +
>   t/t3416-rebase-onto-threedots.sh       |  1 +
>   t/t3419-rebase-patch-id.sh             |  1 +
>   t/t3423-rebase-reword.sh               |  1 +
>   t/t3425-rebase-topology-merges.sh      |  2 ++
>   t/t3431-rebase-fork-point.sh           |  1 +
>   t/t3432-rebase-fast-forward.sh         |  1 +
>   t/t3437-rebase-fixup-options.sh        |  1 +
>   t/t3438-rebase-broken-files.sh         |  2 ++
>   t/t3501-revert-cherry-pick.sh          |  1 +
>   t/t3502-cherry-pick-merge.sh           |  1 +
>   t/t3503-cherry-pick-root.sh            |  1 +
>   t/t3506-cherry-pick-ff.sh              |  1 +
>   t/t3511-cherry-pick-x.sh               |  1 +
>   t/t7402-submodule-rebase.sh            |  1 +
>   t/t9106-git-svn-commit-diff-clobber.sh |  1 -
>   t/t9164-git-svn-dcommit-concurrent.sh  |  1 -
>   23 files changed, 82 insertions(+), 47 deletions(-)
> 

> now that we've got a "replay_opts_release()" for
> the "struct replay_opts" it should know how to free these "revs",
> rather than having these users reach into the struct to free its
> individual members.
> 
> As explained in earlier change we should be using FREE_AND_NULL() in
> replay_opts_release() rather than free().

> 1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
>     2022-11-08)
> 2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/revert.c | 6 ------
>   sequencer.c      | 3 +++
>   2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 1cab16bf3ed..77d2035616e 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -248,9 +248,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>   	res = run_sequencer(argc, argv, &opts);
>   	if (res < 0)
>   		die(_("revert failed"));
> -	if (opts.revs)
> -		release_revisions(opts.revs);
> -	free(opts.revs);
>   	replay_opts_release(&opts);
>   	return res;
>   }
> @@ -263,9 +260,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>   	opts.action = REPLAY_PICK;
>   	sequencer_init_config(&opts);
>   	res = run_sequencer(argc, argv, &opts);
> -	if (opts.revs)
> -		release_revisions(opts.revs);
> -	free(opts.revs);
>   	if (res < 0)
>   		die(_("cherry-pick failed"));
>   	replay_opts_release(&opts);
> diff --git a/sequencer.c b/sequencer.c
> index 5d8c68912a1..c729ce77260 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -362,6 +362,9 @@ void replay_opts_release(struct replay_opts *opts)
>   	opts->xopts_nr = 0;
>   	free(opts->xopts);
>   	strbuf_release(&opts->current_fixups);
> +	if (opts->revs)
> +		release_revisions(opts->revs);
> +	free(opts->revs);
>   }
>   
>   int sequencer_remove_state(struct replay_opts *opts)

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

> @@ -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;

> In [1] we started saving away the earlier xstrdup()'d
> "options.onto_name" assignment to free() it, but when [2] added this
> "keep_base" branch it didn't free() the already assigned value before
> re-assigning to "options.onto_name". Let's do that, and fix the memory
> leak.

> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
> 2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c                 | 3 ++-
>   t/t3416-rebase-onto-threedots.sh | 1 +
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0d8c050f6b3..b4857b89f19 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1660,7 +1660,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		strbuf_addstr(&buf, options.upstream_name);
>   		strbuf_addstr(&buf, "...");
>   		strbuf_addstr(&buf, branch_name);
> -		options.onto_name = xstrdup(buf.buf);
> +		free(to_free);
> +		options.onto_name = to_free = xstrdup(buf.buf);
>   	} else if (!options.onto_name)
>   		options.onto_name = options.upstream_name;
>   	if (strstr(options.onto_name, "...")) {
> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index ea501f2b42b..f8c4ed78c9e 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY/lib-rebase.sh"
>   

> The real use of the "squash_onto_name" added in [1] is to keep track
> of a value for later free()-ing, we don't subsequently use it for
> anything else.
> 
> Let's rename it in preparation for re-using it for free()-ing before
> another assignment to "options.onto_name", which is an outstanding
> leak that'll be fixed in a subsequent commit.

> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5859a5387d8..0d8c050f6b3 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1036,7 +1036,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	const char *rebase_merges = NULL;
>   	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
>   	struct object_id squash_onto;
> -	char *squash_onto_name = NULL;
> +	char *to_free = NULL;
>   	int reschedule_failed_exec = -1;
>   	int allow_preemptive_ff = 1;
>   	int preserve_merges_selected = 0;
> @@ -1589,7 +1589,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   					&squash_onto, NULL, NULL) < 0)
>   				die(_("Could not create new root commit"));
>   			options.squash_onto = &squash_onto;
> -			options.onto_name = squash_onto_name =
> +			options.onto_name = to_free =
>   				xstrdup(oid_to_hex(&squash_onto));
>   		} else
>   			options.root_with_onto = 1;
> @@ -1835,7 +1835,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	free(options.cmd);
>   	free(options.strategy);
>   	strbuf_release(&options.git_format_patch_opt);
> -	free(squash_onto_name);
> +	free(to_free);
>   	string_list_clear(&exec, 0);
>   	string_list_clear(&strategy_options, 0);
>   	return !!ret;

> This series fixes various widespread leaks in the sequencer and its
> users (rebase, revert, cherry-pick). As a result 18 tests become
> leak-free in their entirety.
> 
> See the v1 for a longer general summary:
> https://lore.kernel.org/git/cover-00.10-00000000000-20221230T071741Z-avarab@gmail.com/
> 
> Changes since v1:
> 
>   * I think this addresses all the outstanding feedback, thanks all.
>   * Most significantly, the replay_opts_release() is now moved out of
>     sequencer_remove_state() as suggested.
>   * There's a prep change for renaming "squash_onto_name", per the
>     discussion in v1.
>   * Just do "goto leave" rather than being paranoid and introdungi
>     "goto cleanup", thanks Phillip!
>   * Various other small changes, see the range-diff.
> 
> A branch & passing CI for this are at:
> https://github.com/avar/git/tree/avar/leak-fixes-sequencer-rebase-2
> 
> Ævar Arnfjörð Bjarmason (9):
>    rebase: use "cleanup" pattern in do_interactive_rebase()
>    sequencer.c: split up sequencer_remove_state()
>    rebase & sequencer API: fix get_replay_opts() leak in "rebase"
>    builtin/revert.c: move free-ing of "revs" to replay_opts_release()
>    builtin/rebase.c: rename "squash_onto_name" to "to_free"
>    builtin/rebase.c: fix "options.onto_name" leak
>    sequencer.c: always free() the "msgbuf" in do_pick_commit()
>    builtin/rebase.c: free() "options.strategy_opts"
>    commit.c: free() revs.commit in get_fork_point()
> 
>   builtin/rebase.c                       | 27 +++++++++-------
>   builtin/revert.c                       |  8 ++---
>   commit.c                               |  1 +
>   sequencer.c                            | 43 ++++++++++++++++----------
>   sequencer.h                            |  1 +
>   t/t3405-rebase-malformed.sh            |  1 +
>   t/t3412-rebase-root.sh                 |  1 +
>   t/t3416-rebase-onto-threedots.sh       |  1 +
>   t/t3419-rebase-patch-id.sh             |  1 +
>   t/t3423-rebase-reword.sh               |  1 +
>   t/t3425-rebase-topology-merges.sh      |  2 ++
>   t/t3431-rebase-fork-point.sh           |  1 +
>   t/t3432-rebase-fast-forward.sh         |  1 +
>   t/t3437-rebase-fixup-options.sh        |  1 +
>   t/t3438-rebase-broken-files.sh         |  2 ++
>   t/t3501-revert-cherry-pick.sh          |  1 +
>   t/t3502-cherry-pick-merge.sh           |  1 +
>   t/t3503-cherry-pick-root.sh            |  1 +
>   t/t3506-cherry-pick-ff.sh              |  1 +
>   t/t3511-cherry-pick-x.sh               |  1 +
>   t/t7402-submodule-rebase.sh            |  1 +
>   t/t9106-git-svn-commit-diff-clobber.sh |  1 -
>   t/t9164-git-svn-dcommit-concurrent.sh  |  1 -
>   23 files changed, 64 insertions(+), 36 deletions(-)
> 
> Range-diff against v1:
>   1:  f3a4ed79c7d !  1:  d0a0524f3d4 rebase: use "cleanup" pattern in do_interactive_rebase()
>      @@ Commit message
>           rebase: use "cleanup" pattern in do_interactive_rebase()
>       
>           Use a "goto cleanup" pattern in do_interactive_rebase(). This
>      -    eliminates some duplicated free() code added in 0609b741a43 (rebase
>      -    -i: combine rebase--interactive.c with rebase.c, 2019-04-17), and sets
>      -    us up for a subsequent commit which'll make further use of the
>      -    "cleanup" label.
>      +    eliminates some duplicated free() code added in 53bbcfbde7c (rebase
>      +    -i: implement the main part of interactive rebase as a builtin,
>      +    2018-09-27), and sets us up for a subsequent commit which'll make
>      +    further use of the "cleanup" label.
>       
>           Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>   2:  4994940a0a9 !  2:  c4eaa8dfef4 sequencer.c: split up sequencer_remove_state()
>      @@ Commit message
>           function, which will be adjusted and called independent of the other
>           code in sequencer_remove_state() in a subsequent commit.
>       
>      -    The only functional changes here are:
>      -
>      -     * Changing the "int" to a "size_t", which is the correct type, as
>      -       "xopts_nr" is a "size_t".
>      -
>      -     * Calling the free() before the "if (is_rebase_i(opts) && ...)",
>      -       which is OK, and makes a subsequent change smaller.
>      +    The only functional change here is changing the "int" to a "size_t",
>      +    which is the correct type, as "xopts_nr" is a "size_t".
>       
>           Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>      @@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>        	struct strbuf buf = STRBUF_INIT;
>       -	int i, ret = 0;
>       +	int ret = 0;
>      -+
>      -+	replay_opts_release(opts);
>        
>        	if (is_rebase_i(opts) &&
>        	    strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
>      @@ sequencer.c: int sequencer_remove_state(struct replay_opts *opts)
>       -		free(opts->xopts[i]);
>       -	free(opts->xopts);
>       -	strbuf_release(&opts->current_fixups);
>      --
>      ++	replay_opts_release(opts);
>      +
>        	strbuf_reset(&buf);
>        	strbuf_addstr(&buf, get_dir(opts));
>      - 	if (remove_dir_recursively(&buf, 0))
>   3:  3e9c4df61fe !  3:  f06f565ceaf rebase & sequencer API: fix get_replay_opts() leak in "rebase"
>      @@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts)
>        		break;
>        	}
>        	case ACTION_EDIT_TODO:
>      +@@ builtin/rebase.c: static int finish_rebase(struct rebase_options *opts)
>      +
>      + 		replay.action = REPLAY_INTERACTIVE_REBASE;
>      + 		ret = sequencer_remove_state(&replay);
>      ++		replay_opts_release(&replay);
>      + 	} else {
>      + 		strbuf_addstr(&dir, opts->state_dir);
>      + 		if (remove_dir_recursively(&dir, 0))
>      +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>      +
>      + 			replay.action = REPLAY_INTERACTIVE_REBASE;
>      + 			ret = sequencer_remove_state(&replay);
>      ++			replay_opts_release(&replay);
>      + 		} else {
>      + 			strbuf_reset(&buf);
>      + 			strbuf_addstr(&buf, options.state_dir);
>      +
>      + ## builtin/revert.c ##
>      +@@ builtin/revert.c: int cmd_revert(int argc, const char **argv, const char *prefix)
>      + 	if (opts.revs)
>      + 		release_revisions(opts.revs);
>      + 	free(opts.revs);
>      ++	replay_opts_release(&opts);
>      + 	return res;
>      + }
>      +
>      +@@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>      + 	free(opts.revs);
>      + 	if (res < 0)
>      + 		die(_("cherry-pick failed"));
>      ++	replay_opts_release(&opts);
>      + 	return res;
>      + }
>       
>        ## sequencer.c ##
>       @@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>      @@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>       -static void replay_opts_release(struct replay_opts *opts)
>       +void replay_opts_release(struct replay_opts *opts)
>        {
>      --	free(opts->gpg_sign);
>      --	free(opts->reflog_action);
>      --	free(opts->default_strategy);
>      --	free(opts->strategy);
>      -+	FREE_AND_NULL(opts->gpg_sign);
>      -+	FREE_AND_NULL(opts->reflog_action);
>      -+	FREE_AND_NULL(opts->default_strategy);
>      -+	FREE_AND_NULL(opts->strategy);
>      + 	free(opts->gpg_sign);
>      + 	free(opts->reflog_action);
>      +@@ sequencer.c: 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]);
>      --	free(opts->xopts);
>       +	opts->xopts_nr = 0;
>      -+	FREE_AND_NULL(opts->xopts);
>      + 	free(opts->xopts);
>        	strbuf_release(&opts->current_fixups);
>        }
>      +@@ sequencer.c: int sequencer_remove_state(struct replay_opts *opts)
>      + 		}
>      + 	}
>        
>      +-	replay_opts_release(opts);
>      +-
>      + 	strbuf_reset(&buf);
>      + 	strbuf_addstr(&buf, get_dir(opts));
>      + 	if (remove_dir_recursively(&buf, 0))
>       
>        ## sequencer.h ##
>       @@ sequencer.h: int sequencer_pick_revisions(struct repository *repo,
>      @@ t/t3412-rebase-root.sh: Tests if git rebase --root --onto <newparent> can rebase
>        
>        log_with_names () {
>       
>      + ## t/t3419-rebase-patch-id.sh ##
>      +@@ t/t3419-rebase-patch-id.sh: test_description='git rebase - test patch id computation'
>      + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>      + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>      +
>      ++TEST_PASSES_SANITIZE_LEAK=true
>      + . ./test-lib.sh
>      +
>      + scramble () {
>      +
>        ## t/t3423-rebase-reword.sh ##
>       @@
>        
>      @@ t/t3423-rebase-reword.sh
>        
>        . "$TEST_DIRECTORY"/lib-rebase.sh
>       
>      + ## t/t3425-rebase-topology-merges.sh ##
>      +@@
>      + #!/bin/sh
>      +
>      + test_description='rebase topology tests with merges'
>      ++
>      ++TEST_PASSES_SANITIZE_LEAK=true
>      + . ./test-lib.sh
>      + . "$TEST_DIRECTORY"/lib-rebase.sh
>      +
>      +
>        ## t/t3437-rebase-fixup-options.sh ##
>       @@ t/t3437-rebase-fixup-options.sh: to the "fixup" command that works with "fixup!", "fixup -C" works with
>        "amend!" upon --autosquash.
>      @@ t/t3438-rebase-broken-files.sh
>        
>        test_expect_success 'set up conflicting branches' '
>       
>      + ## t/t3501-revert-cherry-pick.sh ##
>      +@@ t/t3501-revert-cherry-pick.sh: test_description='test cherry-pick and revert with renames
>      + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>      + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>      +
>      ++TEST_PASSES_SANITIZE_LEAK=true
>      + . ./test-lib.sh
>      +
>      + test_expect_success setup '
>      +
>      + ## t/t3502-cherry-pick-merge.sh ##
>      +@@ t/t3502-cherry-pick-merge.sh: test_description='cherry picking and reverting a merge
>      + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>      + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>      +
>      ++TEST_PASSES_SANITIZE_LEAK=true
>      + . ./test-lib.sh
>      +
>      + test_expect_success setup '
>      +
>      + ## t/t3503-cherry-pick-root.sh ##
>      +@@ t/t3503-cherry-pick-root.sh: test_description='test cherry-picking (and reverting) a root commit'
>      + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>      + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>      +
>      ++TEST_PASSES_SANITIZE_LEAK=true
>      + . ./test-lib.sh
>      +
>      + test_expect_success setup '
>      +
>      + ## t/t3506-cherry-pick-ff.sh ##
>      +@@ t/t3506-cherry-pick-ff.sh: test_description='test cherry-picking with --ff option'
>      + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>      + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>      +
>      ++TEST_PASSES_SANITIZE_LEAK=true
>      + . ./test-lib.sh
>      +
>      + test_expect_success setup '
>      +
>      + ## t/t3511-cherry-pick-x.sh ##
>      +@@
>      +
>      + test_description='Test cherry-pick -x and -s'
>      +
>      ++TEST_PASSES_SANITIZE_LEAK=true
>      + . ./test-lib.sh
>      +
>      + pristine_detach () {
>      +
>        ## t/t7402-submodule-rebase.sh ##
>       @@
>        
>   4:  1e4e504c533 <  -:  ----------- builtin/revert.c: refactor run_sequencer() return pattern
>   5:  e2895bb9795 <  -:  ----------- builtin/revert.c: fix common leak by using replay_opts_release()
>   6:  21eea8eb802 !  4:  e83bdfab046 builtin/revert.c: move free-ing of "revs" to replay_opts_release()
>      @@ builtin/revert.c: int cmd_revert(int argc, const char **argv, const char *prefix
>       -	if (opts.revs)
>       -		release_revisions(opts.revs);
>       -	free(opts.revs);
>      -+	replay_opts_release(&opts);
>      + 	replay_opts_release(&opts);
>        	return res;
>        }
>      -
>       @@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>        	opts.action = REPLAY_PICK;
>        	sequencer_init_config(&opts);
>      @@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *p
>       -	if (opts.revs)
>       -		release_revisions(opts.revs);
>       -	free(opts.revs);
>      -+	replay_opts_release(&opts);
>        	if (res < 0)
>        		die(_("cherry-pick failed"));
>      - 	return res;
>      + 	replay_opts_release(&opts);
>       
>        ## sequencer.c ##
>       @@ sequencer.c: void replay_opts_release(struct replay_opts *opts)
>        	opts->xopts_nr = 0;
>      - 	FREE_AND_NULL(opts->xopts);
>      + 	free(opts->xopts);
>        	strbuf_release(&opts->current_fixups);
>       +	if (opts->revs)
>       +		release_revisions(opts->revs);
>      -+	FREE_AND_NULL(opts->revs);
>      ++	free(opts->revs);
>        }
>        
>        int sequencer_remove_state(struct replay_opts *opts)
>   -:  ----------- >  5:  4fea2b77c6d builtin/rebase.c: rename "squash_onto_name" to "to_free"
>   7:  484ebbfd6d1 !  6:  898bb7698fc builtin/rebase.c: fix "options.onto_name" leak
>      @@ Commit message
>       
>           In [1] we started saving away the earlier xstrdup()'d
>           "options.onto_name" assignment to free() it, but when [2] added this
>      -    "keep_base" branch it didn't free() the already assigned
>      -    "squash_onto_name" before re-assigning to "options.onto_name". Let's
>      -    do that, and fix the memory leak.
>      +    "keep_base" branch it didn't free() the already assigned value before
>      +    re-assigning to "options.onto_name". Let's do that, and fix the memory
>      +    leak.
>       
>           1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
>           2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>        		strbuf_addstr(&buf, "...");
>        		strbuf_addstr(&buf, branch_name);
>       -		options.onto_name = xstrdup(buf.buf);
>      -+		free(squash_onto_name);
>      -+		options.onto_name = squash_onto_name = xstrdup(buf.buf);
>      ++		free(to_free);
>      ++		options.onto_name = to_free = xstrdup(buf.buf);
>        	} else if (!options.onto_name)
>        		options.onto_name = options.upstream_name;
>        	if (strstr(options.onto_name, "...")) {
>   8:  d607dbac38e !  7:  fb38dc873f9 sequencer.c: always free() the "msgbuf" in do_pick_commit()
>      @@ Commit message
>           we'd return before the strbuf_release(&msgbuf).
>       
>           Then when the "fixup" support was added in [3] this leak got worse, as
>      -    we added another place where we'd "return" before reaching the
>      -    strbuf_release().
>      +    in this error case we added another place where we'd "return" before
>      +    reaching the strbuf_release().
>       
>      -    Let's move it to a "cleanup" label, and use an appropriate "goto". It
>      -    may or may not be safe to combine the existing "leave" and "cleanup"
>      -    labels, but this change doesn't attempt to answer that question. Let's
>      -    instead avoid calling update_abort_safety_file() in these cases, as we
>      -    didn't do so before.
>      +    This changes the behavior so that we'll call
>      +    update_abort_safety_file() in these cases where we'd previously
>      +    "return", but as noted in [4] "update_abort_safety_file() is a no-op
>      +    when rebasing and you're changing code that is only run when
>      +    rebasing.". Here "no-op" refers to the early return in
>      +    update_abort_safety_file() if git_path_seq_dir() doesn't exist.
>       
>           1. 452202c74b8 (sequencer: stop releasing the strbuf in
>              write_message(), 2016-10-21)
>      @@ Commit message
>              2016-07-26)
>           3. 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
>              'squash' commands, 2017-01-02)
>      +    4. https://lore.kernel.org/git/bcace50b-a4c3-c468-94a3-4fe0c62b3671@dunelm.org.uk/
>       
>           Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>      @@ sequencer.c: static int do_pick_commit(struct repository *r,
>       -			return -1;
>       +					   opts, item->flags)) {
>       +			res = -1;
>      -+			goto cleanup;
>      ++			goto leave;
>       +		}
>        		flags |= AMEND_MSG;
>        		if (!final_fixup)
>      @@ sequencer.c: static int do_pick_commit(struct repository *r,
>       +			if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
>       +				res = error(_("could not rename '%s' to '%s'"),
>       +					    rebase_path_squash_msg(), dest);
>      -+				goto cleanup;
>      ++				goto leave;
>       +			}
>        			unlink(git_path_merge_msg(r));
>        			msg_file = dest;
>      @@ sequencer.c: static int do_pick_commit(struct repository *r,
>        	/*
>        	 * If the merge was clean or if it failed due to conflict, we write
>       @@ sequencer.c: static int do_pick_commit(struct repository *r,
>      - 	}
>      -
>        leave:
>      -+	update_abort_safety_file();
>      -+cleanup:
>        	free_message(commit, &msg);
>        	free(author);
>      --	update_abort_safety_file();
>       +	strbuf_release(&msgbuf);
>      + 	update_abort_safety_file();
>        
>        	return res;
>      - }
>   9:  cd0489a2384 !  8:  d4b0e2a5c83 builtin/rebase.c: free() "options.strategy_opts"
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>        	free(options.strategy);
>       +	free(options.strategy_opts);
>        	strbuf_release(&options.git_format_patch_opt);
>      - 	free(squash_onto_name);
>      + 	free(to_free);
>        	string_list_clear(&exec, 0);
> 10:  eb3678b4667 =  9:  fd9c7a5547c commit.c: free() revs.commit in get_fork_point()

> Hi Ævar
>
> The code changes in this version look good modulo a couple of minor
> comments, however some of the commit messages need to be updated to
> reflect the (very welcome) changes you've made in v2.
>
> Thanks again for working on this

> 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.
> 
> Using a FREE_AND_NULL() pattern would also work, as it would be safe
> replay_opts_release() repeatedly, but let's fix this properly instead,
> by having the owner of the data free() it.
> > See [2] for the initial implementation of "sequencer_remove_state()",
> which assumed that it should be removing the full (including on-disk)
> rebase state as a one-off.

> 1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
>     2019-04-17)
> 2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
>     2011-08-04)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c                       | 4 ++++
>   builtin/revert.c                       | 2 ++
>   sequencer.c                            | 4 +---
>   sequencer.h                            | 1 +
>   t/t3405-rebase-malformed.sh            | 1 +
>   t/t3412-rebase-root.sh                 | 1 +
>   t/t3419-rebase-patch-id.sh             | 1 +
>   t/t3423-rebase-reword.sh               | 1 +
>   t/t3425-rebase-topology-merges.sh      | 2 ++
>   t/t3437-rebase-fixup-options.sh        | 1 +
>   t/t3438-rebase-broken-files.sh         | 2 ++
>   t/t3501-revert-cherry-pick.sh          | 1 +
>   t/t3502-cherry-pick-merge.sh           | 1 +
>   t/t3503-cherry-pick-root.sh            | 1 +
>   t/t3506-cherry-pick-ff.sh              | 1 +
>   t/t3511-cherry-pick-x.sh               | 1 +
>   t/t7402-submodule-rebase.sh            | 1 +
>   t/t9106-git-svn-commit-diff-clobber.sh | 1 -
>   t/t9164-git-svn-dcommit-concurrent.sh  | 1 -
>   19 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 7141fd5e0c1..5859a5387d8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -301,6 +301,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   	}
>   
>   cleanup:
> +	replay_opts_release(&replay);
>   	string_list_clear(&commands, 0);
>   	free(revisions);
>   	free(shortrevisions);
> @@ -343,6 +344,7 @@ static int run_sequencer_rebase(struct rebase_options *opts)
>   		struct replay_opts replay_opts = get_replay_opts(opts);
>   
>   		ret = sequencer_continue(the_repository, &replay_opts);
> +		replay_opts_release(&replay_opts);
>   		break;
>   	}
>   	case ACTION_EDIT_TODO:
> @@ -558,6 +560,7 @@ static int finish_rebase(struct rebase_options *opts)
>   
>   		replay.action = REPLAY_INTERACTIVE_REBASE;
>   		ret = sequencer_remove_state(&replay);
> +		replay_opts_release(&replay);
>   	} else {
>   		strbuf_addstr(&dir, opts->state_dir);
>   		if (remove_dir_recursively(&dir, 0))
> @@ -1331,6 +1334,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   
>   			replay.action = REPLAY_INTERACTIVE_REBASE;
>   			ret = sequencer_remove_state(&replay);
> +			replay_opts_release(&replay);
>   		} else {
>   			strbuf_reset(&buf);
>   			strbuf_addstr(&buf, options.state_dir);
> diff --git a/builtin/revert.c b/builtin/revert.c
> index f2d86d2a8f9..1cab16bf3ed 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -251,6 +251,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>   	if (opts.revs)
>   		release_revisions(opts.revs);
>   	free(opts.revs);
> +	replay_opts_release(&opts);
>   	return res;
>   }
>   
> @@ -267,5 +268,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>   	free(opts.revs);
>   	if (res < 0)
>   		die(_("cherry-pick failed"));
> +	replay_opts_release(&opts);
>   	return res;
>   }
> diff --git a/sequencer.c b/sequencer.c
> index d385bea2bed..8ff29262c1e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -351,7 +351,7 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>   	return buf.buf;
>   }
>   
> -static void replay_opts_release(struct replay_opts *opts)
> +void replay_opts_release(struct replay_opts *opts)
>   {
>   	free(opts->gpg_sign);
>   	free(opts->reflog_action);
> @@ -385,8 +385,6 @@ int sequencer_remove_state(struct replay_opts *opts)
>   		}
>   	}
>   
> -	replay_opts_release(opts);
> -
>   	strbuf_reset(&buf);
>   	strbuf_addstr(&buf, get_dir(opts));
>   	if (remove_dir_recursively(&buf, 0))
> diff --git a/sequencer.h b/sequencer.h
> index 888c18aad71..3bcdfa1b586 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -158,6 +158,7 @@ int sequencer_pick_revisions(struct repository *repo,
>   int sequencer_continue(struct repository *repo, struct replay_opts *opts);
>   int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
>   int sequencer_skip(struct repository *repo, struct replay_opts *opts);
> +void replay_opts_release(struct replay_opts *opts);
>   int sequencer_remove_state(struct replay_opts *opts);
>   
>   #define TODO_LIST_KEEP_EMPTY (1U << 0)
> diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
> index 25243318618..8979bc34073 100755
> --- a/t/t3405-rebase-malformed.sh
> +++ b/t/t3405-rebase-malformed.sh
> @@ -5,6 +5,7 @@ test_description='rebase should handle arbitrary git message'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY"/lib-rebase.sh
>   
> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
> index 58371d8a547..e75b3d0e07c 100755
> --- a/t/t3412-rebase-root.sh
> +++ b/t/t3412-rebase-root.sh
> @@ -7,6 +7,7 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   log_with_names () {
> diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
> index 7181f176b81..6c61f240cf9 100755
> --- a/t/t3419-rebase-patch-id.sh
> +++ b/t/t3419-rebase-patch-id.sh
> @@ -5,6 +5,7 @@ test_description='git rebase - test patch id computation'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   scramble () {
> diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
> index 4859bb8f722..2fab703d615 100755
> --- a/t/t3423-rebase-reword.sh
> +++ b/t/t3423-rebase-reword.sh
> @@ -2,6 +2,7 @@
>   
>   test_description='git rebase interactive with rewording'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   . "$TEST_DIRECTORY"/lib-rebase.sh
> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> index 63acc1ea4da..a16428bdf54 100755
> --- a/t/t3425-rebase-topology-merges.sh
> +++ b/t/t3425-rebase-topology-merges.sh
> @@ -1,6 +1,8 @@
>   #!/bin/sh
>   
>   test_description='rebase topology tests with merges'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY"/lib-rebase.sh
>   
> diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
> index c023fefd681..274699dadb8 100755
> --- a/t/t3437-rebase-fixup-options.sh
> +++ b/t/t3437-rebase-fixup-options.sh
> @@ -14,6 +14,7 @@ to the "fixup" command that works with "fixup!", "fixup -C" works with
>   "amend!" upon --autosquash.
>   '
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   . "$TEST_DIRECTORY"/lib-rebase.sh
> diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh
> index b92a3ce46b8..c614c4f2e4b 100755
> --- a/t/t3438-rebase-broken-files.sh
> +++ b/t/t3438-rebase-broken-files.sh
> @@ -1,6 +1,8 @@
>   #!/bin/sh
>   
>   test_description='rebase behavior when on-disk files are broken'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success 'set up conflicting branches' '
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 1f4cfc37449..2f3e3e24169 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -13,6 +13,7 @@ test_description='test cherry-pick and revert with renames
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success setup '
> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
> index 5495eacfec1..1b2c0d6aca6 100755
> --- a/t/t3502-cherry-pick-merge.sh
> +++ b/t/t3502-cherry-pick-merge.sh
> @@ -11,6 +11,7 @@ test_description='cherry picking and reverting a merge
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success setup '
> diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
> index 95fe4feaeee..76d393dc8a3 100755
> --- a/t/t3503-cherry-pick-root.sh
> +++ b/t/t3503-cherry-pick-root.sh
> @@ -5,6 +5,7 @@ test_description='test cherry-picking (and reverting) a root commit'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success setup '
> diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
> index 7e11bd4a4c5..b71bad17b85 100755
> --- a/t/t3506-cherry-pick-ff.sh
> +++ b/t/t3506-cherry-pick-ff.sh
> @@ -5,6 +5,7 @@ test_description='test cherry-picking with --ff option'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success setup '
> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
> index 84a587daf3a..dd5d92ef302 100755
> --- a/t/t3511-cherry-pick-x.sh
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -2,6 +2,7 @@
>   
>   test_description='Test cherry-pick -x and -s'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   pristine_detach () {
> diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
> index ebeca12a711..b19792b3269 100755
> --- a/t/t7402-submodule-rebase.sh
> +++ b/t/t7402-submodule-rebase.sh
> @@ -5,6 +5,7 @@
>   
>   test_description='Test rebasing, stashing, etc. with submodules'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success setup '
> diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
> index 3cab0b9720a..bca496c40e0 100755
> --- a/t/t9106-git-svn-commit-diff-clobber.sh
> +++ b/t/t9106-git-svn-commit-diff-clobber.sh
> @@ -3,7 +3,6 @@
>   # Copyright (c) 2006 Eric Wong
>   test_description='git svn commit-diff clobber'
>   
> -TEST_FAILS_SANITIZE_LEAK=true
>   . ./lib-git-svn.sh
>   
>   test_expect_success 'initialize repo' '
> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> index 1465156072e..c8e6c0733f4 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -5,7 +5,6 @@
>   
>   test_description='concurrent git svn dcommit'
>   
> -TEST_FAILS_SANITIZE_LEAK=true
>   . ./lib-git-svn.sh
>   
>   

> This series fixes various widespread leaks in the sequencer and its
> users (rebase, revert, cherry-pick). As a result 18 tests become
> leak-free in their entirety.
> 
> See the v1 for a longer general summary:
> https://lore.kernel.org/git/cover-00.10-00000000000-20221230T071741Z-avarab@gmail.com/
> 
> Changes since v3:
> 
> * Rebased for newer "master", there were some conflicts due to
>    adjacent changes.
> * Addressed Phillip's commit message comments (hopefully).

> Branch & CI for this at:
> https://github.com/avar/git/tree/avar/leak-fixes-sequencer-rebase-4
> 
> Ævar Arnfjörð Bjarmason (8):
>    rebase: use "cleanup" pattern in do_interactive_rebase()
>    sequencer.c: split up sequencer_remove_state()
>    sequencer API users: fix get_replay_opts() leaks
>    builtin/revert.c: move free-ing of "revs" to replay_opts_release()
>    builtin/rebase.c: fix "options.onto_name" leak
>    sequencer.c: always free() the "msgbuf" in do_pick_commit()
>    builtin/rebase.c: free() "options.strategy_opts"
>    commit.c: free() revs.commit in get_fork_point()
> 
>   builtin/rebase.c                       | 22 ++++++++------
>   builtin/revert.c                       |  8 ++---
>   commit.c                               |  1 +
>   sequencer.c                            | 42 ++++++++++++++++----------
>   sequencer.h                            |  1 +
>   t/t3405-rebase-malformed.sh            |  1 +
>   t/t3412-rebase-root.sh                 |  1 +
>   t/t3416-rebase-onto-threedots.sh       |  1 +
>   t/t3419-rebase-patch-id.sh             |  1 +
>   t/t3423-rebase-reword.sh               |  1 +
>   t/t3425-rebase-topology-merges.sh      |  2 ++
>   t/t3431-rebase-fork-point.sh           |  1 +
>   t/t3432-rebase-fast-forward.sh         |  1 +
>   t/t3437-rebase-fixup-options.sh        |  1 +
>   t/t3438-rebase-broken-files.sh         |  2 ++
>   t/t3501-revert-cherry-pick.sh          |  1 +
>   t/t3502-cherry-pick-merge.sh           |  1 +
>   t/t3503-cherry-pick-root.sh            |  1 +
>   t/t3506-cherry-pick-ff.sh              |  1 +
>   t/t3511-cherry-pick-x.sh               |  1 +
>   t/t7402-submodule-rebase.sh            |  1 +
>   t/t9106-git-svn-commit-diff-clobber.sh |  1 -
>   t/t9164-git-svn-dcommit-concurrent.sh  |  1 -
>   23 files changed, 61 insertions(+), 33 deletions(-)
> 
> Range-diff against v3:
> 1:  b223429df33 ! 1:  029fc5f4b8c rebase: use "cleanup" pattern in do_interactive_rebase()
>      @@ Commit message
>           Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>        ## builtin/rebase.c ##
>      -@@ builtin/rebase.c: static void split_exec_commands(const char *cmd, struct string_list *commands)
>      +@@ builtin/rebase.c: static int init_basic_state(struct replay_opts *opts, const char *head_name,
>        
>        static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>        {
>      @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts,
>        	}
>        
>       +cleanup:
>      - 	string_list_clear(&commands, 0);
>        	free(revisions);
>        	free(shortrevisions);
>      + 	todo_list_release(&todo_list);
> 2:  00c7f04363f = 2:  b0c9da95ca1 sequencer.c: split up sequencer_remove_state()
> 3:  e4a96898a68 ! 3:  dbac0501424 rebase & sequencer API: fix get_replay_opts() leak in "rebase"
>      @@ Metadata
>       Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>        ## Commit message ##
>      -    rebase & sequencer API: fix get_replay_opts() leak in "rebase"
>      +    sequencer API users: fix get_replay_opts() leaks
>       
>      -    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().
>      +    Make the replay_opts_release() function added in the preceding commit
>      +    non-static, and use it for freeing the "struct replay_opts"
>      +    constructed for "rebase" and "revert".
>       
>           To safely call our new replay_opts_release() we'll need to stop
>           calling it in sequencer_remove_state(), and instead call it where we
>      @@ Commit message
>           previously called sequencer_remove_state() would be a hassle.
>       
>           Using a FREE_AND_NULL() pattern would also work, as it would be safe
>      -    replay_opts_release() repeatedly, but let's fix this properly instead,
>      -    by having the owner of the data free() it.
>      -
>      -    See [2] for the initial implementation of "sequencer_remove_state()",
>      -    which assumed that it should be removing the full (including on-disk)
>      -    rebase state as a one-off.
>      -
>      -    1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
>      -       2019-04-17)
>      -    2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
>      -       2011-08-04)
>      +    to call replay_opts_release() repeatedly. But let's fix this properly
>      +    instead, by having the owner of the data free() it.
>       
>           Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>      @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts,
>        
>        cleanup:
>       +	replay_opts_release(&replay);
>      - 	string_list_clear(&commands, 0);
>        	free(revisions);
>        	free(shortrevisions);
>      + 	todo_list_release(&todo_list);
>       @@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts)
>        		struct replay_opts replay_opts = get_replay_opts(opts);
>        
> 4:  9f72cc6e46b = 4:  6b29d7d00c2 builtin/revert.c: move free-ing of "revs" to replay_opts_release()
> 5:  3d5c3152f69 ! 5:  f9c4d17fe70 builtin/rebase.c: fix "options.onto_name" leak
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>        	strbuf_release(&options.git_format_patch_opt);
>        	free(squash_onto_name);
>       +	free(keep_base_onto_name);
>      - 	string_list_clear(&exec, 0);
>        	string_list_clear(&strategy_options, 0);
>        	return !!ret;
>      + }
>       
>        ## t/t3416-rebase-onto-threedots.sh ##
>       @@ t/t3416-rebase-onto-threedots.sh: test_description='git rebase --onto A...B'
> 6:  c07dc006c6d = 6:  5c2870ed2e6 sequencer.c: always free() the "msgbuf" in do_pick_commit()
> 7:  ee8262ab22a ! 7:  07ab875c3e2 builtin/rebase.c: free() "options.strategy_opts"
>      @@ Commit message
>        ## builtin/rebase.c ##
>       @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>        	free(options.gpg_sign_opt);
>      - 	free(options.cmd);
>      + 	string_list_clear(&options.exec, 0);
>        	free(options.strategy);
>       +	free(options.strategy_opts);
>        	strbuf_release(&options.git_format_patch_opt);
> 8:  84343ea6bf6 = 8:  6ab2edcc135 commit.c: free() revs.commit in get_fork_point()

> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
> 2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c                 | 4 +++-
>   t/t3416-rebase-onto-threedots.sh | 1 +
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5859a5387d8..5c474fb6edd 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1037,6 +1037,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
>   	struct object_id squash_onto;
>   	char *squash_onto_name = NULL;
> +	char *keep_base_onto_name = NULL;
>   	int reschedule_failed_exec = -1;
>   	int allow_preemptive_ff = 1;
>   	int preserve_merges_selected = 0;
> @@ -1660,7 +1661,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		strbuf_addstr(&buf, options.upstream_name);
>   		strbuf_addstr(&buf, "...");
>   		strbuf_addstr(&buf, branch_name);
> -		options.onto_name = xstrdup(buf.buf);
> +		options.onto_name = keep_base_onto_name = xstrdup(buf.buf);
>   	} else if (!options.onto_name)
>   		options.onto_name = options.upstream_name;
>   	if (strstr(options.onto_name, "...")) {
> @@ -1836,6 +1837,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	free(options.strategy);
>   	strbuf_release(&options.git_format_patch_opt);
>   	free(squash_onto_name);
> +	free(keep_base_onto_name);
>   	string_list_clear(&exec, 0);
>   	string_list_clear(&strategy_options, 0);
>   	return !!ret;
> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index ea501f2b42b..f8c4ed78c9e 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY/lib-rebase.sh"
>   

> This series fixes various widespread leaks in the sequencer and its
> users (rebase, revert, cherry-pick). As a result 18 tests become
> leak-free in their entirety.
> 
> See the v1 for a longer general summary:
> https://lore.kernel.org/git/cover-00.10-00000000000-20221230T071741Z-avarab@gmail.com/
> 
> Changes since v2:
> 
>   * Reword/amend commit messages for some of the functional changes in
>     v1..v2.
>   * Remove leftover "opts->xopts_nr = 0" now that we don't
>     FREE_AND_NULL() anymore.
>   * Drop the "squash_onto_name" refactoring to a "to_free"
>   * Instead add a new "keep_base_onto_name" in the next commit. I'd
>     missed that if both options were provided we'd die(), so that
>     free() wasn't needed before re-assignment, as Phillip pointed
>     out...

> CI & branch for this available at:
> https://github.com/avar/git/tree/avar/leak-fixes-sequencer-rebase-3
> 
> Ævar Arnfjörð Bjarmason (8):
>    rebase: use "cleanup" pattern in do_interactive_rebase()
>    sequencer.c: split up sequencer_remove_state()
>    rebase & sequencer API: fix get_replay_opts() leak in "rebase"
>    builtin/revert.c: move free-ing of "revs" to replay_opts_release()
>    builtin/rebase.c: fix "options.onto_name" leak
>    sequencer.c: always free() the "msgbuf" in do_pick_commit()
>    builtin/rebase.c: free() "options.strategy_opts"
>    commit.c: free() revs.commit in get_fork_point()
> 
>   builtin/rebase.c                       | 22 ++++++++------
>   builtin/revert.c                       |  8 ++---
>   commit.c                               |  1 +
>   sequencer.c                            | 42 ++++++++++++++++----------
>   sequencer.h                            |  1 +
>   t/t3405-rebase-malformed.sh            |  1 +
>   t/t3412-rebase-root.sh                 |  1 +
>   t/t3416-rebase-onto-threedots.sh       |  1 +
>   t/t3419-rebase-patch-id.sh             |  1 +
>   t/t3423-rebase-reword.sh               |  1 +
>   t/t3425-rebase-topology-merges.sh      |  2 ++
>   t/t3431-rebase-fork-point.sh           |  1 +
>   t/t3432-rebase-fast-forward.sh         |  1 +
>   t/t3437-rebase-fixup-options.sh        |  1 +
>   t/t3438-rebase-broken-files.sh         |  2 ++
>   t/t3501-revert-cherry-pick.sh          |  1 +
>   t/t3502-cherry-pick-merge.sh           |  1 +
>   t/t3503-cherry-pick-root.sh            |  1 +
>   t/t3506-cherry-pick-ff.sh              |  1 +
>   t/t3511-cherry-pick-x.sh               |  1 +
>   t/t7402-submodule-rebase.sh            |  1 +
>   t/t9106-git-svn-commit-diff-clobber.sh |  1 -
>   t/t9164-git-svn-dcommit-concurrent.sh  |  1 -
>   23 files changed, 61 insertions(+), 33 deletions(-)
> 
> Range-diff against v2:
>   1:  d0a0524f3d4 =  1:  b223429df33 rebase: use "cleanup" pattern in do_interactive_rebase()
>   2:  c4eaa8dfef4 =  2:  00c7f04363f sequencer.c: split up sequencer_remove_state()
>   3:  f06f565ceaf !  3:  e4a96898a68 rebase & sequencer API: fix get_replay_opts() leak in "rebase"
>      @@ Commit message
>           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()).
>      +    To safely call our new replay_opts_release() we'll need to stop
>      +    calling it in sequencer_remove_state(), and instead call it where we
>      +    allocate the "struct replay_opts" itself.
>       
>           This is because in e.g. do_interactive_rebase() we construct a "struct
>           replay_opts" with "get_replay_opts()", and then call
>      @@ Commit message
>       
>           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.
>      +    previously called sequencer_remove_state() would be a hassle.
>       
>      -    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.
>      +    Using a FREE_AND_NULL() pattern would also work, as it would be safe
>      +    replay_opts_release() repeatedly, but let's fix this properly instead,
>      +    by having the owner of the data free() it.
>       
>           See [2] for the initial implementation of "sequencer_remove_state()",
>           which assumed that it should be removing the full (including on-disk)
>      @@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>        {
>        	free(opts->gpg_sign);
>        	free(opts->reflog_action);
>      -@@ sequencer.c: 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;
>      - 	free(opts->xopts);
>      - 	strbuf_release(&opts->current_fixups);
>      - }
>       @@ sequencer.c: int sequencer_remove_state(struct replay_opts *opts)
>        		}
>        	}
>   4:  e83bdfab046 !  4:  9f72cc6e46b builtin/revert.c: move free-ing of "revs" to replay_opts_release()
>      @@ Commit message
>           rather than having these users reach into the struct to free its
>           individual members.
>       
>      -    As explained in earlier change we should be using FREE_AND_NULL() in
>      -    replay_opts_release() rather than free().
>      -
>           1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
>              2022-11-08)
>           2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)
>      @@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *p
>       
>        ## sequencer.c ##
>       @@ sequencer.c: void replay_opts_release(struct replay_opts *opts)
>      - 	opts->xopts_nr = 0;
>      + 		free(opts->xopts[i]);
>        	free(opts->xopts);
>        	strbuf_release(&opts->current_fixups);
>       +	if (opts->revs)
>   5:  4fea2b77c6d <  -:  ----------- builtin/rebase.c: rename "squash_onto_name" to "to_free"
>   6:  898bb7698fc !  5:  3d5c3152f69 builtin/rebase.c: fix "options.onto_name" leak
>      @@ Metadata
>        ## Commit message ##
>           builtin/rebase.c: fix "options.onto_name" leak
>       
>      -    In [1] we started saving away the earlier xstrdup()'d
>      -    "options.onto_name" assignment to free() it, but when [2] added this
>      -    "keep_base" branch it didn't free() the already assigned value before
>      -    re-assigning to "options.onto_name". Let's do that, and fix the memory
>      -    leak.
>      +    Similar to the existing "squash_onto_name" added in [1] we need to
>      +    free() the xstrdup()'d "options.onto.name" added for "--keep-base" in
>      +    [2]..
>       
>           1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
>           2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
>      @@ Commit message
>           Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>        ## builtin/rebase.c ##
>      +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>      + 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
>      + 	struct object_id squash_onto;
>      + 	char *squash_onto_name = NULL;
>      ++	char *keep_base_onto_name = NULL;
>      + 	int reschedule_failed_exec = -1;
>      + 	int allow_preemptive_ff = 1;
>      + 	int preserve_merges_selected = 0;
>       @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>        		strbuf_addstr(&buf, options.upstream_name);
>        		strbuf_addstr(&buf, "...");
>        		strbuf_addstr(&buf, branch_name);
>       -		options.onto_name = xstrdup(buf.buf);
>      -+		free(to_free);
>      -+		options.onto_name = to_free = xstrdup(buf.buf);
>      ++		options.onto_name = keep_base_onto_name = xstrdup(buf.buf);
>        	} else if (!options.onto_name)
>        		options.onto_name = options.upstream_name;
>        	if (strstr(options.onto_name, "...")) {
>      +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>      + 	free(options.strategy);
>      + 	strbuf_release(&options.git_format_patch_opt);
>      + 	free(squash_onto_name);
>      ++	free(keep_base_onto_name);
>      + 	string_list_clear(&exec, 0);
>      + 	string_list_clear(&strategy_options, 0);
>      + 	return !!ret;
>       
>        ## t/t3416-rebase-onto-threedots.sh ##
>       @@ t/t3416-rebase-onto-threedots.sh: test_description='git rebase --onto A...B'
>   7:  fb38dc873f9 =  6:  c07dc006c6d sequencer.c: always free() the "msgbuf" in do_pick_commit()
>   8:  d4b0e2a5c83 !  7:  ee8262ab22a builtin/rebase.c: free() "options.strategy_opts"
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>        	free(options.strategy);
>       +	free(options.strategy_opts);
>        	strbuf_release(&options.git_format_patch_opt);
>      - 	free(to_free);
>      - 	string_list_clear(&exec, 0);
>      + 	free(squash_onto_name);
>      + 	free(keep_base_onto_name);
>   9:  fd9c7a5547c =  8:  84343ea6bf6 commit.c: free() revs.commit in get_fork_point()