From: karthik nayak <karthik.188@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v3 2/3] sha1_file: implement changes for "cat-file --literally -t"
Date: Mon, 09 Mar 2015 18:38:23 +0530 [thread overview]
Message-ID: <54FD9B47.7080008@gmail.com> (raw)
In-Reply-To: <xmqq61abs3ur.fsf@gitster.dls.corp.google.com>
On 03/09/2015 12:39 AM, Junio C Hamano wrote:
> 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.
>
>> 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.
>
Hey Junio!
You are indeed right! I was only thinking about "--literally -t" and
totally ignored the possibility of a future implementation of
"--literally -s" and maybe a "--literally -e" and a "--literally -p".
Now I understand why you were saying i need to change
parse_sha1_header().
I will have to look into this, will get back to you when I come up with
something tangible.
Thanks for the detailed explanation and possible ways of coming across
this problem.
Regards
-Karthik
next prev parent reply other threads:[~2015-03-09 13:08 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-05 18:16 [PATCH v3 0/3] cat-file: add "--literally" option karthik nayak
2015-03-05 18:18 ` [PATCH v3 1/3] cache: modify for "cat-file --literally -t" Karthik Nayak
2015-03-08 22:25 ` Eric Sunshine
2015-03-09 12:56 ` karthik nayak
2015-03-05 18:19 ` [PATCH v3 2/3] sha1_file: implement changes " Karthik Nayak
2015-03-05 23:45 ` Junio C Hamano
2015-03-06 17:41 ` karthik nayak
2015-03-06 19:28 ` Junio C Hamano
2015-03-07 10:04 ` karthik nayak
2015-03-08 8:09 ` Junio C Hamano
2015-03-08 8:48 ` karthik nayak
2015-03-08 9:03 ` Junio C Hamano
2015-03-08 10:49 ` karthik nayak
2015-03-08 19:09 ` Junio C Hamano
2015-03-09 13:08 ` karthik nayak [this message]
2015-03-05 18:19 ` [PATCH v3 3/3] cat-file: add "--literally" option Karthik Nayak
2015-03-08 22:50 ` Eric Sunshine
2015-03-09 12:53 ` karthik nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54FD9B47.7080008@gmail.com \
--to=karthik.188@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).