Re: [PATCH v4 5/8] sha1_file: refactor read_object

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

Jonathan Tan <jonathantanmy@google.com> writes:

> @@ -2914,6 +2912,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,
>  	git_zstream stream;
>  	char hdr[32];
>  	struct strbuf hdrbuf = STRBUF_INIT;
> +	unsigned long size_scratch;
>  
>  	if (oi->delta_base_sha1)
>  		hashclr(oi->delta_base_sha1);
> @@ -2939,6 +2938,10 @@ static int sha1_loose_object_info(const unsigned char *sha1,
>  	map = map_sha1_file(sha1, &mapsize);
>  	if (!map)
>  		return -1;
> +
> +	if (!oi->sizep)
> +		oi->sizep = &size_scratch;
> +
>  	if (oi->disk_sizep)
>  		*oi->disk_sizep = mapsize;
>  	if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) {
>  	if (status && oi->typep)
>  		*oi->typep = status;
> +	if (oi->sizep == &size_scratch)
> +		oi->sizep = NULL;

This looked somewhat unusual but nevertheless is correct.  Because
of the way parse_sha1_header_extended() interacts with its callers,
the usual fn(oi->sizep ? oi->sizep : &dummy) pattern does not apply
to this codepath.

> @@ -3077,28 +3090,15 @@ int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,
>  static void *read_object(const unsigned char *sha1, enum object_type *type,
>  			 unsigned long *size)
>  {
> -	unsigned long mapsize;
> -	void *map, *buf;
> -	struct cached_object *co;
> -
> -	co = find_cached_object(sha1);
> -	if (co) {
> -		*type = co->type;
> -		*size = co->size;
> -		return xmemdupz(co->buf, co->size);
> -	}
> +	struct object_info oi = OBJECT_INFO_INIT;
> +	void *content;
> +	oi.typep = type;
> +	oi.sizep = size;
> +	oi.contentp = &content;
>  
> -	buf = read_packed_sha1(sha1, type, size);
> -	if (buf)
> -		return buf;
> -	map = map_sha1_file(sha1, &mapsize);
> -	if (map) {
> -		buf = unpack_sha1_file(map, mapsize, type, size, sha1);
> -		munmap(map, mapsize);
> -		return buf;
> -	}
> -	reprepare_packed_git();
> -	return read_packed_sha1(sha1, type, size);
> +	if (sha1_object_info_extended(sha1, &oi, 0))
> +		return NULL;
> +	return content;
>  }

Nice code reduction; it is somewhat funny to think that a function
meant to gather 'object info' does so much, but we can always say
the contents is part of the information about the object ;-).

Same comment as the other one applies here; the definition of how an
error is reported by sha1_object_info_extended() should be kept
consistent with existing callers.

Thanks.