On 04/06/2015 01:27 AM, Junio C Hamano wrote:
karthik nayak <karthik....@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; }
Haha, If there is anything I love about code revisions its how you are subjected to so many different ways of thinking. I didn't think of what you said. We could do this and support typename without typep being used. This could probably even eliminate the typep used by cat-file while getting the type of an object. Will go through this again. Thanks a lot. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
