From: Karthik Nayak <karthik.188@gmail.com>
Date: Thu, 10 Sep 2026 16:32:52 GMT
Subject: Re: [PATCH v9 4/4] hook: introduce the receive-report hook
Message-ID: <CAOLa=ZROrWmr=2O+NrkNsJU8Zyz5rGbH30SRazjv0kzDF1RtTA@mail.gmail.com>
In-Reply-To: <xmqqjyotokyg.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static void override_cmds_error(struct command *commands, const char *err)
>> +{
>> +	for (struct command *cmd = commands; cmd; cmd = cmd->next)
>> +		cmd->error_string = err;
>> +}
>
> Doesn't this leak existing cmd->error_string if it is owned?  In
> other words, something like
>
> 	for (struct command *cmd = commands; cmd; cmd = cmd->next) {
> 		if (cmd->error_string_owned)
> 			FREE_AND_NULL(cmd->error_string_owned);
>                 cmd->error_string = err;
> 	}
>
> is in order, perhaps?

You're right, I thought of writing a test for this, my idea was to
create a test where we override a pre-allocated string. But, unless we
always do `cmd->error_string = cmd->error_string_owned = <string>`,
`cmd->error_string_owned` can end up pointing to something allocated,
while `cmd->error_string` is replaced. Eventually we'll call
`free(cmd->error_string_owned)`. So the memory leak is never realized.

Either ways, I'll also add a test which triggers this path.