Subject: Re: [RFC PATCH 1/4] sha1_file: teach packed_object_info about typename

From: Junio C Hamano <[email-masked]>
Date: Mon, 12 Jun 2017 13:55:18 -0700

Jonathan Tan <[email-masked]> writes:

> In commit 46f0344 ("sha1_file: support reading from a loose object of
> unknown type", 2015-05-06), "struct object_info" gained a "typename"
> field that could represent a type name from a loose object file, whether
> valid or invalid, as opposed to the existing "typep" which could only
> represent valid types. Some relatively complex manipulations were added
> to avoid breaking packed_object_info() without modifying it, but it is
> much easier to just teach packed_object_info() about the new field.
> Therefore, teach packed_object_info() as described above.
>
> Signed-off-by: Jonathan Tan <[email-masked]>
> ---
>  sha1_file.c | 29 ++++++++++++-----------------
>  1 file changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 59a4ed2ed..a52b27541 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2277,9 +2277,18 @@ int packed_object_info(struct packed_git *p, off_t obj_offset,
>  		*oi->disk_sizep = revidx[1].offset - obj_offset;
>  	}
>  
> -	if (oi->typep) {
> -		*oi->typep = packed_to_object_type(p, obj_offset, type, &w_curs, curpos);
> -		if (*oi->typep < 0) {
> +	if (oi->typep || oi->typename) {
> +		enum object_type ptot;
> +		ptot = packed_to_object_type(p, obj_offset, type, &w_curs,
> +					     curpos);
> +		if (oi->typep)
> +			*oi->typep = ptot;
> +		if (oi->typename) {
> +			const char *tn = typename(ptot);
> +			if (tn)
> +				strbuf_addstr(oi->typename, tn);
> +		}
> +		if (ptot < 0) {
>  			type = OBJ_BAD;
>  			goto out;
>  		}

OK.  When the caller wants to learn typename, we need to do this
type-to-string conversion somewhere anyway, and I agree that it is
better to do it here, instead of in the caller.

> @@ -2960,7 +2969,6 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
>  	struct cached_object *co;
>  	struct pack_entry e;
>  	int rtype;
> -	enum object_type real_type;
>  	const unsigned char *real = lookup_replace_object_extended(sha1, flags);
>  
>  	co = find_cached_object(real);
> @@ -2992,18 +3000,9 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
>  			return -1;
>  	}
>  
> -	/*
> -	 * packed_object_info() does not follow the delta chain to
> -	 * find out the real type, unless it is given oi->typep.
> -	 */
> -	if (oi->typename && !oi->typep)
> -		oi->typep = &real_type;
> -
>  	rtype = packed_object_info(e.p, e.offset, oi);
>  	if (rtype < 0) {
>  		mark_bad_packed_object(e.p, real);
> -		if (oi->typep == &real_type)
> -			oi->typep = NULL;
>  		return sha1_object_info_extended(real, oi, 0);
>  	} else if (in_delta_base_cache(e.p, e.offset)) {
>  		oi->whence = OI_DBCACHED;
> @@ -3014,10 +3013,6 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
>  		oi->u.packed.is_delta = (rtype == OBJ_REF_DELTA ||
>  					 rtype == OBJ_OFS_DELTA);
>  	}
> -	if (oi->typename)
> -		strbuf_addstr(oi->typename, typename(*oi->typep));
> -	if (oi->typep == &real_type)
> -		oi->typep = NULL;
>  
>  	return 0;
>  }