Re: [PATCH 01/10] rebase: use "cleanup" pattern in do_interactive_rebase()

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Dec 31, 2022, 14:50 UTC
Message-ID: <5a562613-eae7-599c-c548-540faa60e3aa@dunelm.org.uk>
In-Reply-To: <patch-01.10-f3a4ed79c7d-20221230T071741Z-avarab@gmail.com>

Hi Ævar

On 30/12/2022 07:28, Ævar Arnfjörð Bjarmason wrote:

> 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),

I read this as meaning that commit added some code to this function, but it fact it just copied the function unchanged from another file.

Show 28 quoted lines

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

This sort of change potentially problematic as we're free()ing things that were not previously free()d but revisions and shortrevisions are initialized to NULL before passing them to get_revision_ranges() so it is safe.

Looks good

Phillip

Show 21 quoted lines

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