From: Karthik Nayak <karthik.188@gmail.com>
Date: Fri, 23 Jan 2026 14:49:46 GMT
Subject: Re: [PATCH v4 6/6] fetch: delay user information post committing of transaction
Message-ID: <CAOLa=ZSLPasvFrCgKzVOq7mDXiqX9SxoOf0MZdzBXOLn73okMQ@mail.gmail.com>
In-Reply-To: <xmqqldhpmmrw.fsf@gitster.g>

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

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +struct ref_update_display_info {
>> +	bool failed;
>> +	char success_code;
>> +	char fail_code;
>> +	const char *summary;
>> +	const char *fail_detail;
>> +	const char *success_detail;
>> +	const char *ref;
>> +	const char *remote;
>> +	struct object_id old_oid;
>> +	struct object_id new_oid;
>> +};
>> +
>> +struct ref_update_display_info_array {
>> +	struct ref_update_display_info *info;
>> +	size_t alloc, nr;
>> +};
>
> OK.  The ref_update_display_info structure is full of pointers.
> They are of "const char *" type, hinting that they are borrowed
> pieces of memory, and there is nothing to clean inside, other than
> the .info member itself?
>
>> +static struct ref_update_display_info *ref_update_display_info_append(
>> +					   struct ref_update_display_info_array *array,
>> +					   char success_code,
>> +					   char fail_code,
>> +					   const char *summary,
>> +					   const char *success_detail,
>> +					   const char *fail_detail,
>> +					   const char *ref,
>> +					   const char *remote,
>> +					   const struct object_id *old_oid,
>> +					   const struct object_id *new_oid)
>> +{
>
> This helper that consumes the structure is used throughout the
> patch, and relative to the previous round it got easier to read.
>
>> +static void ref_update_display_info_free(struct ref_update_display_info *info)
>> +{
>> +	free((char *)info->summary);
>> +	free((char *)info->success_detail);
>> +	free((char *)info->fail_detail);
>> +	free((char *)info->remote);
>> +	free((char *)info->ref);
>> +}
>
> This answers "no" to my previous question.  These are not borrowed,
> but are owned by this structure.
>

Yup, cannot be borrowed, since those go out of scope much earlier.

>> @@ -1965,7 +2090,17 @@ static int do_fetch(struct transport *transport,
>>  	 */
>>  	if (retcode && !atomic_fetch && transaction)
>>  		commit_ref_transaction(&transaction, false,
>> -				       transport->remote->name, &err);
>> +				       transport->remote->name,
>> +				       &rejected_refs, &err);
>> +
>> +	for (size_t i = 0; i < display_array.nr; i++) {
>> +		struct ref_update_display_info *info = &display_array.info[i];
>> +
>> +		if (!info->failed && strmap_contains(&rejected_refs, info->ref))
>> +			ref_update_display_info_set_failed(info);
>> +		ref_update_display_info_display(info, &display_state, summary_width);
>> +		ref_update_display_info_free(info);
>> +	}
>
> And after a fetch finishes and we consume the display_info, we call
> _free() to release the resource held there, plus ...
>
>>  	if (retcode) {
>>  		if (err.len) {
>> @@ -1980,6 +2115,9 @@ static int do_fetch(struct transport *transport,
>>
>>  	if (transaction)
>>  		ref_transaction_free(transaction);
>> +
>> +	free(display_array.info);
>
> ... of course the array itself, which makes sense.

Yeah, the CI also didn't show any leaks, so we should be good.