From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 04 Jun 2024 13:58:17 GMT
Subject: Re: [PATCH 20/29] sequencer: fix leaking string buffer in `commit_staged_changes()`
Message-ID: <396e6893-80d4-48f5-b43f-a30684a64bac@gmail.com>
In-Reply-To: <Zl6_6Z6Bu-zgOmPs@framework>

Hi Patrick

On 04/06/2024 08:19, Patrick Steinhardt wrote:
> On Tue, Jun 04, 2024 at 08:45:11AM +0200, Patrick Steinhardt wrote:
>> On Mon, Jun 03, 2024 at 02:14:20PM +0100, Phillip Wood wrote:
>>> Hi Patrick
>>>
>>> On 03/06/2024 10:48, Patrick Steinhardt wrote:
>>>> @@ -5259,12 +5277,13 @@ static int commit_staged_changes(struct repository *r,
>>>>    				}
>>>>    			unuse_commit_buffer:
>>>>    				repo_unuse_commit_buffer(r, commit, p);
>>>> -				if (res)
>>>> -					return res;
>>>> +				if (res) {
>>>> +					ret = res;
>>>> +					goto out;
>>>> +				}
>>>
>>> Having 'ret' and 'res' in this block is a bit confusing - we could delete
>>> the declaration for 'res' and  either replace its use with 'ret', or rename
>>> 'ret' to 'res' in this patch.
>>
>> Yeah, let's just drop the local `res` variable here and use `ret`
>> instead.
> 
> For the record, below is the diff to address this. I'll refrain from
> sending out this whole patch series again for now to only address this
> one issue, but will include it if there are more things that need to be
> handled.
> 
> Patrick

This and all the other sequencer changes in this series look good to me

Thanks

Phillip

> diff --git a/sequencer.c b/sequencer.c
> index 9e90084692..cc57a30883 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5253,7 +5253,6 @@ static int commit_staged_changes(struct repository *r,
>   				 * We need to update the squash message to skip
>   				 * the latest commit message.
>   				 */
> -				int res = 0;
>   				struct commit *commit;
>   				const char *msg;
>   				const char *path = rebase_path_squash_msg();
> @@ -5266,22 +5265,22 @@ static int commit_staged_changes(struct repository *r,
>   
>   				p = repo_logmsg_reencode(r, commit, NULL, encoding);
>   				if (!p)  {
> -					res = error(_("could not parse commit %s"),
> +					ret = error(_("could not parse commit %s"),
>   						    oid_to_hex(&commit->object.oid));
>   					goto unuse_commit_buffer;
>   				}
>   				find_commit_subject(p, &msg);
>   				if (write_message(msg, strlen(msg), path, 0)) {
> -					res = error(_("could not write file: "
> +					ret = error(_("could not write file: "
>   						       "'%s'"), path);
>   					goto unuse_commit_buffer;
>   				}
> +
> +				ret = 0;
>   			unuse_commit_buffer:
>   				repo_unuse_commit_buffer(r, commit, p);
> -				if (res) {
> -					ret = res;
> +				if (ret)
>   					goto out;
> -				}
>   			}
>   		}
>