From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 22 Jan 2026 20:10:27 GMT
Subject: Re: [PATCH v4 6/6] fetch: delay user information post committing of transaction
Message-ID: <xmqqldhpmmrw.fsf@gitster.g>
In-Reply-To: <20260122-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v4-6-2ddba0832440@gmail.com>

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.

> @@ -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.