From: karthik nayak <karthik.188@gmail.com>
Date: Tue, 07 Apr 2015 10:34:15 GMT
Subject: Re: [PATCH v7 1/4] sha1_file.c: support reading from a loose object of unknown type
Message-ID: <5523B2A7.6060604@gmail.com>
In-Reply-To: <xmqqbnj2e4ah.fsf@gitster.dls.corp.google.com>


On 04/06/2015 01:27 AM, Junio C Hamano wrote:
> 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;
>   }
>
>
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.