Re: [PATCH v7 1/4] sha1_file.c: support reading from a loose object of unknown type

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

karthik nayak <karthik.188@gmail.com> writes:

> On 04/05/2015 01:16 PM, Junio C Hamano wrote:
>
>> If it semantically does not make sense to ask for the typename
>> without asking for the type code, then we can and should make that
>> as a new calling convention _all_ callers must follow.
>>
>> In other words, I think it is better to have
>>
>> 	if (oi->typename && !oi->typep)
>> 		die("BUG");
>>
>> somewhere near the beginning of the callchain that takes oi; that
>> will ensure all callers understand the rule.
>
> Yes! this is a better approach as it will enforce that typep must be
> set when typename is set.

Not so fast ;-)

The key phrase in what I wrote above is "If it does not make sense",
and I am not yet convinced if that is the case or not.  If we are to
change the calling convention for the callers, the reason why it
does not make sense to ask only for typename but not typep must be
explained in the log message.

And if it turns out that the answer to that question is "it is valid
to ask only for typename", then it would be wrong to force everybody
who wants to learn the stringified typename to supply typep.  And in
such a case it might be better to do something like this instead (on
top of your patch I am responding to):

 sha1_file.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index f4055dd..26fbb7c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2639,6 +2639,7 @@ 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);
@@ -2670,9 +2671,18 @@ 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;
@@ -2686,6 +2696,8 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
 	if (oi->typename)
 		strbuf_addstr(oi->typename, typename(*oi->typep));
 
+	if (oi->typep == &real_type)
+		oi->typep = NULL;
 	return 0;
 }