Re: [PATCH v3 2/3] sha1_file: implement changes for "cat-file --literally -t"

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 8, 2015, 19:09 UTC
Message-ID: <xmqq61abs3ur.fsf@gitster.dls.corp.google.com>
In-Reply-To: <54FC292C.5060405@gmail.com>

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

> Sorry for the confusion, you did already say that in $gmane/264955 , I'm
> talking about how I tackled the issue in $gmane/264855.

Well, I am suggesting how to improve what you did in your $gmane/264855 and a part of that was to suggest that teaching parse_sha1_header() the "literally" mode may be necessary and why "do not have to call parse_sha1_header()" may not be a good solution.

Unless our goal is only to support "--literally -t" and go home, never intending to support other things like "--literally -s" and "--literally flob $OBJ", that is ;-)

Let's try again.

Show 13 quoted lines

> Like :
>
>     else if ((flags & LOOKUP_LITERALLY)) {
>         size_t typelen = strcspn(hdrbuf.buf, " ");
>         strbuf_add(oi->typename, hdrbuf.buf, typelen);
>     }
>     else if ((status = parse_sha1_header(hdrp, &size)) < 0)
>         status = error("unable to parse %s header", sha1_to_hex(sha1));
>     else if (oi->sizep)
>         *oi->sizep = size;
>
> This way, we don't have to modify parse_sha1_header() to worry if "literally"
> is set or not.

When you are dealing with a crafted object $OBJ of type "florb", how would your

    $ git cat-file --literally florb $OBJ
    $ git cat-file --literally -s $OBJ

be implemented, if parse_sha1_header() has not been enhanced to allow you to say "for this invocation, the type name in the object header may be something unknown to us, and it is OK"?

One possible implementation of "--literally florb $OBJ" would be to still call the same loose object reading codepath, which would eventually hit parse_sha1_header() to see what the type of the object is and how long the object contents is, and error out if the type is not "florb" or the length of the inflated contents does not match the expected size. Wouldn't you need a way for you to say "for this invocation, the type name in the object header may be something unknown to us, and it is OK"?

One possible implementation of "--literally -s $OBJ" would be to change the call to sha1_object_info() to instead to call the _extended() version, just like you did for "--literally -t", and then read the result from *(oi.sizep), but the quoted piece of code above would not let you use it that way, no?

Of course the above two are both "one possible implementation", and not how the implementation ought to be [*1*]. But knowing a bit of this part of the codepath, they look the most natural way to enhance these codepaths, at least to me.

[Footnote]

*1* You could for example sidestep the issue and introduce a parallel implementation of what parse_sha1_header() does, minus the validation to ensure the object type is one of the known ones, and use that function instead of the users of parse_sha1_header() in the codepaths that implement "-s" and "cat-file" itself.

But I do not think that is a good direction to go in to keep the codebase healthy in the longer term. A refactoring that is similar to what we did when we introduced sha1_object_info_extended(), which was done in 9a490590 (sha1_object_info_extended(): expose a bit more info, 2011-05-12) would be more desirable, so that we can avoid such a duplicated parallel implementations. That way, the existing callers that do not have to know about "--literally" can keep using parse_sha1_header(), but parse_sha1_header_extended() is called from the updated parse_sha1_header() behind the scene. And the extended version would let callers that need to care about "--literally" ask "the type name in the object header may be something unknown to us, and it is OK" by passing an extra flag.