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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 5, 2015, 19:57 UTC
Message-ID: <xmqqbnj2e4ah.fsf@gitster.dls.corp.google.com>
In-Reply-To: <5520E9D2.4010603@gmail.com>

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

Show 16 quoted lines

> 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;
 }