Re: [PATCH GSoC RFC v12 09/12] transport: add client support for object-info

[Junio C Hamano <gitster@pobox.com>]

Pablo Sabater <pabloosabaterr@gmail.com> writes:

[jc: removed recipients from Cc: list whose addresses bounce]

> From: Calvin Wan <calvinwan@google.com>
>
> Sometimes, it is beneficial to retrieve information about an object
> without downloading it entirely. The server-side logic for this
> functionality was implemented in commit "a2ba162cda (object-info:
> ...
> diff --git a/fetch-object-info.c b/fetch-object-info.c
> ...
> +int fetch_object_info(const enum protocol_version version, struct object_info_args *args,
> +		      struct packet_reader *reader, struct object_info *object_info_data,
> +		      const int stateless_rpc, const int fd_out)
> +{
> ...
> +	for (size_t i = 0; packet_reader_read(reader) == PACKET_READ_NORMAL && i < args->oids->nr; i++) {
> +		struct string_list object_info_values = STRING_LIST_INIT_DUP;
> +
> +		string_list_split(&object_info_values, reader->line, " ", -1);
> +		if (0 <= size_index) {
> +			if (!strcmp(object_info_values.items[1 + size_index].string, ""))
> +				die("object-info: server does not recognize object %s",
> +				    object_info_values.items[0].string);
> +
> +			if (strtoul_ul(object_info_values.items[1 + size_index].string, 10, object_info_data[i].sizep))


Overly long lines need to be fixed, by using a shorter and crisper
variable name in such a short scope, and line wrapping if needed.

More importantly, on this line (wrapped):

			if (strtoul_ul(object_info_values.items[1 + size_index].string,
				       10, object_info_data[i].sizep))

we notice object_info_data[i] is of type "struct object_info", which
is 

    struct object_info {
            /* Request */
            enum object_type *typep;
            size_t *sizep;
            off_t *disk_sizep;
	    ...

but the last parameter strtoul_ul() takes is unsurprisingly a
pointer to "unsigned long", not a pointer to "size_t".

Which will break on 32-bit boxes where size_t is "unsigned int"
that is 32-bit and different from "unsigned long".

Perhaps something along this line?


diff --git a/fetch-object-info.c b/fetch-object-info.c
index 425929a269..5210e7d954 100644
--- a/fetch-object-info.c
+++ b/fetch-object-info.c
@@ -75,14 +75,17 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
 
 		string_list_split(&object_info_values, reader->line, " ", -1);
 		if (0 <= size_index) {
+			unsigned long sz;
 			if (!strcmp(object_info_values.items[1 + size_index].string, ""))
 				die("object-info: server does not recognize object %s",
 				    object_info_values.items[0].string);
 
-			if (strtoul_ul(object_info_values.items[1 + size_index].string, 10, object_info_data[i].sizep))
+			if (strtoul_ul(object_info_values.items[1 + size_index].string,
+				       10, &sz))
 				die("object-info: ref %s has invalid size %s",
 				    object_info_values.items[0].string,
 				    object_info_values.items[1 + size_index].string);
+			*object_info_data[i].sizep = sz;
 		}
 
 		string_list_clear(&object_info_values, 0);