From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 08 Aug 2024 10:08:23 GMT
Subject: Re: [PATCH 15/22] sequencer: release todo list on error paths
Message-ID: <e0685657-369f-4211-8518-cc41aa28dcb9@gmail.com>
In-Reply-To: <df4c21b49f86d6e1e9d2b28375ab6465ffa4339a.1722933642.git.ps@pks.im>

Hi Patrick

On 06/08/2024 10:00, Patrick Steinhardt wrote:
> We're not releasing the `todo_list` in `sequencer_pick_revisions()` when
> hitting an error path. Restructure the function to have a common exit
> path such that we can easily clean up the list and thus plug this memory
> leak.

This looks good, I've left a couple of small formatting comments below 
if you do end up re-rolling.

> @@ -5506,11 +5508,14 @@ int sequencer_pick_revisions(struct repository *r,
>   				enum object_type type = oid_object_info(r,
>   									&oid,
>   									NULL);
> -				return error(_("%s: can't cherry-pick a %s"),
> +				res = error(_("%s: can't cherry-pick a %s"),
>   					name, type_name(type));

This line needs re-indenting to match the changes above.

> +				goto out;
>   			}
> -		} else
> -			return error(_("%s: bad revision"), name);
> +		} else {
> +			res = error(_("%s: bad revision"), name);
> +			goto out;
> +		}
>   	}
>   
>   	/*
> @@ -5525,14 +5530,23 @@ int sequencer_pick_revisions(struct repository *r,
>   	    opts->revs->no_walk &&
>   	    !opts->revs->cmdline.rev->flags) {
>   		struct commit *cmit;
> -		if (prepare_revision_walk(opts->revs))
> -			return error(_("revision walk setup failed"));
> +

This whitespace change is good as it means we now have an empty line 
between the variable declarations and the code, the others I'm not 
fussed about either way.

Best Wishes

Phillip

> +		if (prepare_revision_walk(opts->revs)) {
> +			res = error(_("revision walk setup failed"));
> +			goto out;
> +		}
> +
>   		cmit = get_revision(opts->revs);
> -		if (!cmit)
> -			return error(_("empty commit set passed"));
> +		if (!cmit) {
> +			res = error(_("empty commit set passed"));
> +			goto out;
> +		}
> +
>   		if (get_revision(opts->revs))
>   			BUG("unexpected extra commit from walk");
> -		return single_pick(r, cmit, opts);
> +
> +		res = single_pick(r, cmit, opts);
> +		goto out;
>   	}
>   
>   	/*
> @@ -5542,16 +5556,30 @@ int sequencer_pick_revisions(struct repository *r,
>   	 */
>   
>   	if (walk_revs_populate_todo(&todo_list, opts) ||
> -			create_seq_dir(r) < 0)
> -		return -1;
> -	if (repo_get_oid(r, "HEAD", &oid) && (opts->action == REPLAY_REVERT))
> -		return error(_("can't revert as initial commit"));
> -	if (save_head(oid_to_hex(&oid)))
> -		return -1;
> -	if (save_opts(opts))
> -		return -1;
> +			create_seq_dir(r) < 0) {
> +		res = -1;
> +		goto out;
> +	}
> +
> +	if (repo_get_oid(r, "HEAD", &oid) && (opts->action == REPLAY_REVERT)) {
> +		res = error(_("can't revert as initial commit"));
> +		goto out;
> +	}
> +
> +	if (save_head(oid_to_hex(&oid))) {
> +		res = -1;
> +		goto out;
> +	}
> +
> +	if (save_opts(opts)) {
> +		res = -1;
> +		goto out;
> +	}
> +
>   	update_abort_safety_file();
>   	res = pick_commits(r, &todo_list, opts);
> +
> +out:
>   	todo_list_release(&todo_list);
>   	return res;
>   }
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 7eb52b12ed..93c725bac3 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -12,6 +12,7 @@ test_description='Test cherry-pick continuation features
>   
>   '
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   # Repeat first match 10 times