Re: [PATCH GSoC v2 4/6] fetch-object-info: parse type from server response

From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Aug 1, 2026, 21:28 UTC
Message-ID: <DKDXCRLFABZK.2DP7XB6NXY9SA@gmail.com>
In-Reply-To: <xmqq7bmaa0sw.fsf@gitster.g>

On Sat Aug 1, 2026 at 7:04 AM CEST, Junio C Hamano wrote:

Show 25 quoted lines

> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
>> @@ -104,8 +105,13 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
>>  			for (size_t j = 0; j < args->oids->nr; j++)
>>  				object_info_data[j].sizep =
>>  					xcalloc(1, sizeof(*object_info_data[j].sizep));
>> +		} else if (!strcmp(reader->line, "type")) {
>> +			type_index = (int)i;
>> +			for (size_t j = 0; j < args->oids->nr; j++)
>> +				object_info_data[j].typep =
>> +					xcalloc(1, sizeof(*object_info_data[j].typep));
>
> Do object_info_data[j].typep and object_info_data[k].typep need to
> be independently freeable?  Separate allocations by calling calloc
> args->oids->nr times would allow that, but if there is no such need,
> nr contiguous allocation of them,
>
> 		enum object_type *types;
>
> 		*types = xcalloc(args->oids->nr, sizeof(*types));
> 		for (size_t j = 0; j < args->oids->nr; j++)
> 			object_info_data[j].typep = &types[j];
>
> would be simpler to manage and easier to get rid of once you are
> done.

Hmmmm, they don't need to be independently freeable but they are freed by free_object_info_contents() called at the end of parse_cmd_remote_object_info() at 'builtin/cat-file.c' in a loop:

	for (size_t i = 0; i < object_info_oids.nr; i++)
		free_object_info_contents(&remote_object_info[i]);

free_object_info_contents() is:

	void free_object_info_contents(struct object_info *object_info)
	{
		if (!object_info)
			return;
		free(object_info->typep);
		free(object_info->sizep);
		free(object_info->disk_sizep);
		free(object_info->delta_base_oid);
	}

This function was implemented by the series that introduced remote-object-info (the one that this series is based on) so parse_cmd_remote_object_info() is the only caller.

Thinking about it, your suggestion can be done easily. To free types it is enough to do free(remote_object_info[0].typep); (same for sizep).

I'll make it work as a prep patch for size and modify this one to do the same. free_object_info_contents() gets dropped in the prep patch because it would have no callers after it.

Show 18 quoted lines

>
>>  		} else {
>> -			BUG("only size is supported");
>> +			BUG("unexpected object-info option: %s", reader->line);
>>  		}
>>  	}
>>
>> @@ -151,6 +157,10 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
>>  			    object_info_values.items[0].string,
>>  			    object_info_values.items[size_index + 1].string);
>>
>> +		if (type_index >= 0)
>> +			*object_info_data[i].typep =
>> +				type_from_string(object_info_values.items[type_index + 1].string);
>> +
>>  		string_list_clear(&object_info_values, 0);
>>  	}
>>  	check_stateless_delimiter(stateless_rpc, reader, "stateless delimiter expected");

Thanks for the review, Pablo