v8, 4 partscat-file: teach cat-file a '--literally' option

> Update sha1_loose_object_info() to optionally allow it to read
> from a loose object file of unknown/bogus type; as the function
> usually returns the type of the object it read in the form of enum
> for known types, add an optional "typename" field to receive the
> name of the type in textual form and a flag to indicate the reading
> of a loose object file of unknown/bogus type.
>
> Add parse_sha1_header_extended() which acts as a wrapper around
> parse_sha1_header() allowing more information to be obtained.
>
> Add unpack_sha1_header_to_strbuf() to unpack sha1 headers of
> unknown/corrupt objects which have a unknown sha1 header size to
> a strbuf structure. This was written by Junio C Hamano but tested
> by me.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---

> @@ -1614,27 +1642,40 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s
>   * too permissive for what we want to check. So do an anal
>   * object header parse by hand.
>   */
> -int parse_sha1_header(const char *hdr, unsigned long *sizep)
> +int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
> +			       unsigned int flags)
>  {
> -	char type[10];
> -	int i;
> +	struct strbuf typename = STRBUF_INIT;
>  	unsigned long size;
> +	int type;
>  
>  	/*
>  	 * The type can be at most ten bytes (including the
>  	 * terminating '\0' that we add), and is followed by
>  	 * a space.
>  	 */
> -	i = 0;
>  	for (;;) {
>  		char c = *hdr++;
>  		if (c == ' ')
>  			break;
> -		type[i++] = c;
> -		if (i >= sizeof(type))
> -			return -1;
> +		strbuf_addch(&typename, c);
>  	}
> -	type[i] = 0;

> @@ -2541,7 +2596,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,
>  	 * return value implicitly indicates whether the
>  	 * object even exists.
>  	 */
> -	if (!oi->typep && !oi->sizep) {
> +	if (!oi->typep && !oi->typename && !oi->sizep) {

> > -		type[i++] = c;
> > -		if (i >= sizeof(type))
> > -			return -1;
> > +		strbuf_addch(&typename, c);
> >  	}
> > -	type[i] = 0;
> 
> This _might_ have some performance impact in that strbuf_addch()
> involves strbuf_grow(*, 1), which does "does it overflow to
> increment sb->len by one?"; I would say it should be unmeasurable
> because the function is expected to be used only on loose objects
> and you shouldn't have very many of them without packing in your
> repository in the first place.
> 
> I guess Peff's c1822d4f (strbuf: add an optimized 1-character
> strbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his
> new strbuf_grow_ch(), and once that happens the performance worry
> would disappear without this code to be changed at all.

> > This _might_ have some performance impact in that strbuf_addch()
> > involves strbuf_grow(*, 1), which does "does it overflow to
> > increment sb->len by one?"; I would say it should be unmeasurable
> > because the function is expected to be used only on loose objects
> > and you shouldn't have very many of them without packing in your
> > repository in the first place.
> > 
> > I guess Peff's c1822d4f (strbuf: add an optimized 1-character
> > strbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his
> > new strbuf_grow_ch(), and once that happens the performance worry
> > would disappear without this code to be changed at all.
> 
> I haven't re-rolled that series yet, but the discussion there showed
> that strbuf_grow_ch() is unnecessary; call-sites can just check:
> 
>   if (!strbuf_avail(sb))
> 	strbuf_grow(sb, 1);
> 
> to get the fast inline check. Since we go to the trouble to inline
> strbuf_addch, we should probably teach it the same trick. It would be
> nice to identify a place with a tight strbuf_addch() loop that could
> demonstrate the speed increase, though.

>
> But now we use type_from_string_gently, which can accept a length[1]. So
> we could just count the bytes to the first space and pass the original
> buffer along with that length, no?
>
> -Peff
>
> [1] Not related to your patch, but it looks like type_from_string_gently
>     is overly lax. It does a strncmp() with the length of the candidate
>     name, but does not check that we consumed all of the matching name.
>     So "tr" would match "tree", "comm" would match "commit", and so
>     forth.

> Jeff King <peff@peff.net> writes:
> 
> > If there _is_ a performance implication to worry about here, I think it
> > would be that we are doing an extra malloc/free.
> 
> Thanks for reminding me; yes, that also worried me.

> On Fri, Apr 17, 2015 at 09:21:31AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > If there _is_ a performance implication to worry about here, I think it
>> > would be that we are doing an extra malloc/free.
>> 
>> Thanks for reminding me; yes, that also worried me.
>
> As an aside, I worried about the extra allocation for reading the header
> in the first place. But it looks like we only do this on the --literally
> code path (and otherwise use the normal unpack_sha1_header).  Still, I
> wonder if we could make this work automagically.  That is, speculatively
> unpack the first N bytes, assuming we hit the end-of-header. If not,
> then go to a strbuf as the slow path. Then it would be fine to cover all
> cases; the normal ones would be fast, and only ridiculous things would
> incur the extra allocation.

> Jeff King <peff@peff.net> writes:
>
>> On Fri, Apr 17, 2015 at 09:21:31AM -0700, Junio C Hamano wrote:
>>
>>> Jeff King <peff@peff.net> writes:
>>>
>>>> If there _is_ a performance implication to worry about here, I think it
>>>> would be that we are doing an extra malloc/free.
>>>
>>> Thanks for reminding me; yes, that also worried me.
>>
>> As an aside, I worried about the extra allocation for reading the header
>> in the first place. But it looks like we only do this on the --literally
>> code path (and otherwise use the normal unpack_sha1_header).  Still, I
>> wonder if we could make this work automagically.  That is, speculatively
>> unpack the first N bytes, assuming we hit the end-of-header. If not,
>> then go to a strbuf as the slow path. Then it would be fine to cover all
>> cases; the normal ones would be fast, and only ridiculous things would
>> incur the extra allocation.
>
> Yes, that was what I was hoping to see eventually ;-)
>

> +static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char
> *map,
> +                                       unsigned long mapsize, void *buffer,
> +                                       unsigned long bufsiz, struct strbuf
> *header)
> +{
> +       unsigned char *cp;
> +       int status;
> +       int i = 0;
> +
> +       status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);

> +       for (cp = buffer; cp < stream->next_out; cp++)
> +               if (!*cp) {
> +                       /* Found the NUL at the end of the header */
> +                       return 0;
> +               }

> On Tue, Apr 21, 2015 at 12:13:30AM +0530, karthik nayak wrote:
>
>> +static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char
>> *map,
>> +                                       unsigned long mapsize, void *buffer,
>> +                                       unsigned long bufsiz, struct strbuf
>> *header)
>> +{
>> +       unsigned char *cp;
>> +       int status;
>> +       int i = 0;
>> +
>> +       status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
>
> I wonder if we would feel comfortable just running this NUL-check as
> part of unpack_sha1_header (i.e., in all code paths). It _shouldn't_
> trigger in normal use, but I wonder if there would be any downsides
> (e.g., maliciously crafted objects getting us to allocate memory or
> something; I think it is fairly easy to convince git to allocate memory,
> though).
>

>> +       for (cp = buffer; cp < stream->next_out; cp++)
>> +               if (!*cp) {
>> +                       /* Found the NUL at the end of the header */
>> +                       return 0;
>> +               }
>
> I think we can spell this as:
>
>    if (memchr(buffer, '\0', stream->next_out - buffer))
> 	return 0;
>
> which is shorter and possibly more efficient.

>
> In theory we could also just start trying to parse the type/size header,
> and notice there when we don't find the NUL. That's probably not worth
> doing, though. The parsing is separated from the unpacking here, so it
> would require combining those two operations in a single function. And
> the extra NUL search here is likely not very expensive.
>

> >>+       status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
> >
> >I wonder if we would feel comfortable just running this NUL-check as
> >part of unpack_sha1_header (i.e., in all code paths). It _shouldn't_
> >trigger in normal use, but I wonder if there would be any downsides
> >(e.g., maliciously crafted objects getting us to allocate memory or
> >something; I think it is fairly easy to convince git to allocate memory,
> >though).
> >
> But why would we want it to be a part of unpack_sha1_header?

> On Wed, Apr 15, 2015 at 06:18:24PM -0400, Jeff King wrote:
>
> >> This _might_ have some performance impact in that strbuf_addch()
> >> involves strbuf_grow(*, 1), which does "does it overflow to
> >> increment sb->len by one?"; I would say it should be unmeasurable
> >> because the function is expected to be used only on loose objects
> >> and you shouldn't have very many of them without packing in your
> >> repository in the first place.
> >>
> >> I guess Peff's c1822d4f (strbuf: add an optimized 1-character
> >> strbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his
> >> new strbuf_grow_ch(), and once that happens the performance worry
> >> would disappear without this code to be changed at all.
> >
> > I haven't re-rolled that series yet, but the discussion there showed
> > that strbuf_grow_ch() is unnecessary; call-sites can just check:
> >
> >    if (!strbuf_avail(sb))
> >     strbuf_grow(sb, 1);
> >
> > to get the fast inline check. Since we go to the trouble to inline
> > strbuf_addch, we should probably teach it the same trick. It would be
> > nice to identify a place with a tight strbuf_addch() loop that could
> > demonstrate the speed increase, though.
>
> Just for reference, I did teach strbuf_addch this trick. And the config
> code is the tight loop to test it with. Results are here:
>
>    http://article.gmane.org/gmane.comp.version-control.git/267266
>
> In this code path, we are typically only seeing short strings (the
> original code used a 10-byte static buffer). So I doubt it makes all
> that much difference.
>
> If there _is_ a performance implication to worry about here, I think it
> would be that we are doing an extra malloc/free. I'm not sure I
> understand why we are copying it at all. The original code copied from
> the hdr into type[10] so that we could NUL-terminate it, which was
> required for type_from_string().
>
> But now we use type_from_string_gently, which can accept a length[1]. So
> we could just count the bytes to the first space and pass the original
> buffer along with that length, no?

> >But now we use type_from_string_gently, which can accept a length[1]. So
> >we could just count the bytes to the first space and pass the original
> >buffer along with that length, no?
> 
> Yes, we could, that would eliminate  "struct strbuf typename =
> STRBUF_INIT".
> 
> Something like this perhaps :

>   {
> -       char type[10];
> -       int i;
> +       const char *buf = hdr;
>          unsigned long size;
> +       int type, len = 0;

> On Sat, Apr 18, 2015 at 12:15:28AM +0530, karthik nayak wrote:
>
>>> But now we use type_from_string_gently, which can accept a length[1]. So
>>> we could just count the bytes to the first space and pass the original
>>> buffer along with that length, no?
>>
>> Yes, we could, that would eliminate  "struct strbuf typename =
>> STRBUF_INIT".
>>
>> Something like this perhaps :
>
> Yeah, this is exactly what I had in mind.
>
>>    {
>> -       char type[10];
>> -       int i;
>> +       const char *buf = hdr;
>>           unsigned long size;
>> +       int type, len = 0;
>
> Maybe switch the names of "buf" and "len" to "type_buf" and "type_len"
> to make their purpose more clear?
>
> -Peff
>

> karthik nayak <karthik.188@gmail.com> writes:
>
>> +       type = type_from_string_gently(buf, len, 1);
>> +       if (oi->typename) {
>> +               strbuf_add(oi->typename, buf, len);
>> +               strbuf_addch(oi->typename, '\0');
>
> add() has setlen() at the end so you do not have to NUL terminate it
> yourself.  Doing addch() is actively wrong, as oi->typename->len now
> counts the terminating NUL as part of the string, no?
>

> Update sha1_loose_object_info() to optionally allow it to read
> from a loose object file of unknown/bogus type; as the function
> usually returns the type of the object it read in the form of enum
> for known types, add an optional "typename" field to receive the
> name of the type in textual form and a flag to indicate the reading
> of a loose object file of unknown/bogus type.
> [...]
> ---
> diff --git a/sha1_file.c b/sha1_file.c
> index 980ce6b..267399d 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2522,13 +2575,15 @@ struct packed_git *find_sha1_pack(const unsigned char *sha1,
>  }
>
>  static int sha1_loose_object_info(const unsigned char *sha1,
> -                                 struct object_info *oi)
> +                                 struct object_info *oi,
> +                                 int flags)
>  {
> -       int status;
> -       unsigned long mapsize, size;
> +       int status = 0;
> +       unsigned long mapsize;
>         void *map;
>         git_zstream stream;
>         char hdr[32];
> +       struct strbuf hdrbuf = STRBUF_INIT;
>
>         if (oi->delta_base_sha1)
>                 hashclr(oi->delta_base_sha1);
> @@ -2555,17 +2610,26 @@ static int sha1_loose_object_info(const unsigned char *sha1,
>                 return -1;
>         if (oi->disk_sizep)
>                 *oi->disk_sizep = mapsize;
> -       if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
> -               status = error("unable to unpack %s header",
> -                              sha1_to_hex(sha1));
> -       else if ((status = parse_sha1_header(hdr, &size)) < 0)
> -               status = error("unable to parse %s header", sha1_to_hex(sha1));
> -       else if (oi->sizep)
> -               *oi->sizep = size;
> +       if ((flags & LOOKUP_LITERALLY)) {
> +               if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, &hdrbuf) < 0)
> +                       status = error("unable to unpack %s header with --literally",
> +                                      sha1_to_hex(sha1));
> +               else if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)
> +                       status = error("unable to parse %s header with --literally",
> +                                      sha1_to_hex(sha1));
> +       } else {
> +               if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
> +                       status = error("unable to unpack %s header",
> +                                      sha1_to_hex(sha1));
> +               else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)
> +                       status = error("unable to parse %s header", sha1_to_hex(sha1));
> +       }
>         git_inflate_end(&stream);
>         munmap(map, mapsize);
> -       if (oi->typep)
> +       if (status && oi->typep)
>                 *oi->typep = status;
> +       if (hdrbuf.buf)
> +               strbuf_release(&hdrbuf);

> On Wed, Apr 15, 2015 at 12:59 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Update sha1_loose_object_info() to optionally allow it to read
>> from a loose object file of unknown/bogus type; as the function
>> usually returns the type of the object it read in the form of enum
>> for known types, add an optional "typename" field to receive the
>> name of the type in textual form and a flag to indicate the reading
>> of a loose object file of unknown/bogus type.
>> [...]
>> ---
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 980ce6b..267399d 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -2522,13 +2575,15 @@ struct packed_git *find_sha1_pack(const unsigned char *sha1,
>>   }
>>
>>   static int sha1_loose_object_info(const unsigned char *sha1,
>> -                                 struct object_info *oi)
>> +                                 struct object_info *oi,
>> +                                 int flags)
>>   {
>> -       int status;
>> -       unsigned long mapsize, size;
>> +       int status = 0;
>> +       unsigned long mapsize;
>>          void *map;
>>          git_zstream stream;
>>          char hdr[32];
>> +       struct strbuf hdrbuf = STRBUF_INIT;
>>
>>          if (oi->delta_base_sha1)
>>                  hashclr(oi->delta_base_sha1);
>> @@ -2555,17 +2610,26 @@ static int sha1_loose_object_info(const unsigned char *sha1,
>>                  return -1;
>>          if (oi->disk_sizep)
>>                  *oi->disk_sizep = mapsize;
>> -       if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
>> -               status = error("unable to unpack %s header",
>> -                              sha1_to_hex(sha1));
>> -       else if ((status = parse_sha1_header(hdr, &size)) < 0)
>> -               status = error("unable to parse %s header", sha1_to_hex(sha1));
>> -       else if (oi->sizep)
>> -               *oi->sizep = size;
>> +       if ((flags & LOOKUP_LITERALLY)) {
>> +               if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, &hdrbuf) < 0)
>> +                       status = error("unable to unpack %s header with --literally",
>> +                                      sha1_to_hex(sha1));
>> +               else if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)
>> +                       status = error("unable to parse %s header with --literally",
>> +                                      sha1_to_hex(sha1));
>> +       } else {
>> +               if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
>> +                       status = error("unable to unpack %s header",
>> +                                      sha1_to_hex(sha1));
>> +               else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)
>> +                       status = error("unable to parse %s header", sha1_to_hex(sha1));
>> +       }
>>          git_inflate_end(&stream);
>>          munmap(map, mapsize);
>> -       if (oi->typep)
>> +       if (status && oi->typep)
>>                  *oi->typep = status;
>> +       if (hdrbuf.buf)
>> +               strbuf_release(&hdrbuf);
>
> Why is strbuf_release() protected by a conditional rather than being
> called unconditionally? Am I missing something obvious?

>  	case 's':
> -		type = sha1_object_info(sha1, &size);
> -		if (type > 0) {
> -			printf("%lu\n", size);
> -			return 0;
> -		}
> -		break;
> +		oi.sizep = &size;
> +		if ((type = sha1_object_info_extended(sha1, &oi, flags)) < 0)

> @@ -323,7 +332,7 @@ static int batch_objects(struct batch_options *opt)
>  }
>  
>  static const char * const cat_file_usage[] = {
> -	N_("git cat-file (-t | -s | -e | -p | <type> | --textconv) <object>"),
> +	N_("git cat-file (-t [--literally]|-s [--literally]|-e|-p|<type>|--textconv) <object>"),
>  	N_("git cat-file (--batch | --batch-check) < <list-of-objects>"),
>  	NULL
>  };
> @@ -359,6 +368,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>  	int opt = 0;
>  	const char *exp_type = NULL, *obj_name = NULL;
>  	struct batch_options batch = {0};
> +	int literally = 0;
>  
>  	const struct option options[] = {
>  		OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),
> @@ -369,6 +379,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>  		OPT_SET_INT('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
>  		OPT_SET_INT(0, "textconv", &opt,
>  			    N_("for blob objects, run textconv on object's content"), 'c'),
> +		OPT_BOOL( 0, "literally", &literally,
> +			  N_("allow -s and -t to work with broken/corrupt objects")),
>  		{ OPTION_CALLBACK, 0, "batch", &batch, "format",
>  			N_("show info and content of objects fed from the standard input"),
>  			PARSE_OPT_OPTARG, batch_option_callback },
> @@ -380,7 +392,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>  
>  	git_config(git_cat_file_config, NULL);
>  
> -	if (argc != 3 && argc != 2)
> +	if (argc < 2 || argc > 4)
>  		usage_with_options(cat_file_usage, options);

> Karthik Nayak <karthik.188@gmail.com> writes:
> ...
>> -	if (argc != 3 && argc != 2)
>> +	if (argc < 2 || argc > 4)
>>  		usage_with_options(cat_file_usage, options);
>
> Hmm, why this change?
>
> By allowing 4 args blindly like this, you will let something like
> these to pass:
>
>     $ git cat-file --textconv -t HEAD
>     commit
>     $ git cat-file -p -t HEAD
>     commit
>     $ git cat-file -s -t HEAD
>     commit
>     $ git cat-file -t -s HEAD
>     879
>
> It is fine if you are declaring that the last one wins when these
> mutually-exclusive options are given. "git cat-file -e -s -t HEAD"
> should also say "commit" if that is what you are doing, but instead
> the code with this patch complains, which is bad.
>
> It also is fine and is more in line with the spirit of the original
> code if you keep the rule tight and only one of these mutually
> exclusive options is allowed.
>
> In either case, this check needs to be rethought.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>> ...
>>> -	if (argc != 3 && argc != 2)
>>> +	if (argc < 2 || argc > 4)
>>>   		usage_with_options(cat_file_usage, options);
>>
>> Hmm, why this change?
>>
>> By allowing 4 args blindly like this, you will let something like
>> these to pass:
>>
>>      $ git cat-file --textconv -t HEAD
>>      commit
>>      $ git cat-file -p -t HEAD
>>      commit
>>      $ git cat-file -s -t HEAD
>>      commit
>>      $ git cat-file -t -s HEAD
>>      879
>>
>> It is fine if you are declaring that the last one wins when these
>> mutually-exclusive options are given. "git cat-file -e -s -t HEAD"
>> should also say "commit" if that is what you are doing, but instead
>> the code with this patch complains, which is bad.
>>
>> It also is fine and is more in line with the spirit of the original
>> code if you keep the rule tight and only one of these mutually
>> exclusive options is allowed.
>>
>> In either case, this check needs to be rethought.
>
> I wonder if we want to do something like this as a preliminary step
> before your [PATCH 2/4].  Everything after the parse_options() call
> seems to protect against leftover argc depending on what they need
> already, so the only thing existing "we take only 2 or 3 args" check
> is doing is to protect against giving more than one command mode
> options, I think.  And OPT_CMDMODE() should do a much better job at
> that that kind of thing, by giving a more informative error message
> e.g.
>
>      $ git cat-file -p -e HEAD
>      error: switch 'e': incompatible with -p
>
> This is a tangent, but while we are in the vicinity, we may want to
> rethink the help message we attach to the '-e' option.  Technically
> the current message is _not_ wrong per-se, but it misses the point.
> The primary thing the option does is to check the (e)xistence of the
> named object, and the fact that it does so silently is merely a
> detail of the operation.  The current help text omits the more
> important part of what the option is.

>
>   builtin/cat-file.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 1ec7190..534991d 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -372,12 +372,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>
>   	const struct option options[] = {
>   		OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),
> -		OPT_SET_INT('t', NULL, &opt, N_("show object type"), 't'),
> -		OPT_SET_INT('s', NULL, &opt, N_("show object size"), 's'),
> -		OPT_SET_INT('e', NULL, &opt,
> +		OPT_CMDMODE('t', NULL, &opt, N_("show object type"), 't'),
> +		OPT_CMDMODE('s', NULL, &opt, N_("show object size"), 's'),
> +		OPT_CMDMODE('e', NULL, &opt,
>   			    N_("exit with zero when there's no error"), 'e'),
> -		OPT_SET_INT('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
> -		OPT_SET_INT(0, "textconv", &opt,
> +		OPT_CMDMODE('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
> +		OPT_CMDMODE(0, "textconv", &opt,
>   			    N_("for blob objects, run textconv on object's content"), 'c'),
>   		OPT_BOOL( 0, "literally", &literally,
>   			  N_("allow -s and -t to work with broken/corrupt objects")),
> @@ -392,9 +392,6 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>
>   	git_config(git_cat_file_config, NULL);
>
> -	if (argc < 2 || argc > 4)
> -		usage_with_options(cat_file_usage, options);
> -
>   	argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
>
>   	if (opt) {
>

> On 04/16/2015 02:22 AM, Junio C Hamano wrote:
>
>> This is a tangent, but while we are in the vicinity, we may want to
>> rethink the help message we attach to the '-e' option.  Technically
>> the current message is _not_ wrong per-se, but it misses the point.
>> The primary thing the option does is to check the (e)xistence of the
>> named object, and the fact that it does so silently is merely a
>> detail of the operation.  The current help text omits the more
>> important part of what the option is.
>
> Would you rather check '-e' and go on to check '-p' or do you merely
> just want a different message.

>karthik nayak <karthik.188@gmail.com> writes:
>
>> On 04/16/2015 02:22 AM, Junio C Hamano wrote:
>>
>>> This is a tangent, but while we are in the vicinity, we may want to
>>> rethink the help message we attach to the '-e' option.  Technically
>>> the current message is _not_ wrong per-se, but it misses the point.
>>> The primary thing the option does is to check the (e)xistence of the
>>> named object, and the fact that it does so silently is merely a
>>> detail of the operation.  The current help text omits the more
>>> important part of what the option is.
>>
>> Would you rather check '-e' and go on to check '-p' or do you merely
>> just want a different message.
>
>I meant just a different message.  The point of -e is to see if the
>thing exists.  It is good to mention _how_ the result is reported
>back to the user (i.e. via the exit code, not via an output to the
>standard output "exists" vs "missing", for example), but that is
>secondary.  Telling how it reports is meaningless without telling
>what it reports in the first place.

>
>> ... when a user is giving the '-e' option he just expects a silent
>> output if the object exists, hence we rather have the option '-e'
>> behave as a mutually exclusive option...
>
>Yes, and that is in line with the switch to OPT_CMDMODE.

>
> On April 16, 2015 7:05:04 PM GMT+05:30, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>I meant just a different message.  The point of -e is to see if the
>>thing exists.  It is good to mention _how_ the result is reported
>>back to the user (i.e. via the exit code, not via an output to the
>>standard output "exists" vs "missing", for example), but that is
>>secondary.  Telling how it reports is meaningless without telling
>>what it reports in the first place.
>
> I see what you mean. But I think it's beyond the scope of this patch series.

> Currently 'git cat-file' throws an error while trying to
> print the type or size of a broken/corrupt object. This is
> because these objects are usually of unknown types.
> 
> Teach git cat-file a '--literally' option where it prints
> the type or size of a broken/corrupt object without throwing
> an error.

> I'm sorry to come in with such a fundamental question at such a late
> revision of this patch series, but am I the only person to wonder about
> the choice of option name?
>
> To me, cat-file already output objects "literally" (without -p) as
> opposed to show. From the description, it feels more like it should be
> "--unchecked" or perhaps something better that I haven't thought of?

> On 20 Apr 2015, at 06:30, Junio C Hamano <gitster@pobox.com> wrote:
> Charles Bailey <charles@hashpling.org> writes:
> 
>> The option isn't a true opposite of hash-object's --literally because
>> that also allows the creation of known types with invalid contents (e.g.
>> corrupt trees) whereas cat-file is quite happy to show the _contents_ of
>> such corrupt objects even without --literally.
> 
> Not really.  If you create an object with corrupt type string (e.g. "BLOB"
> instead of "blob"), cat-file would not be happy.

>> On 20 Apr 2015, at 06:30, Junio C Hamano <gitster@pobox.com> wrote:
>> Charles Bailey <charles@hashpling.org> writes:
>> 
>>> The option isn't a true opposite of hash-object's --literally
>because
>>> that also allows the creation of known types with invalid contents
>(e.g.
>>> corrupt trees) whereas cat-file is quite happy to show the
>_contents_ of
>>> such corrupt objects even without --literally.
>> 
>> Not really.  If you create an object with corrupt type string (e.g.
>"BLOB"
>> instead of "blob"), cat-file would not be happy.
>
>Sorry, the emphasis should have been on "complete" of "complete
>opposite".  There are some types of bad objects that can be created
>only
>with hash-object --literally (malformed tag or tree), for which
>cat-file
>works with fine and there are other types (pun unintended - BLOB,

>wobble, etc.) for which --literally/--unchecked is required with
>cat-file.
>
>So I meant that cat-file's --literally is only a partial "opposite" or
>analogue of hash-object's.
>
>--allow-invalid-types? --force (in the sense of "suppress some possible
>errors")? It's not a big thing but I'm aware that if we can find a
>better
>name then now would be the best moment. If not, then --literally it is.

> On Mon, Apr 20, 2015 at 02:27:44PM +0530, Karthik Nayak wrote:
> > Sorry, but I didn't get you, broken objects created using hash-object --literally do not work with cat-file without the --literally option.
>
> Perhaps an example would help:
>
> I cannot create a bad tree without --literally:
>
> $ echo total garbage | ./git hash-object -t tree --stdin -w
> fatal: corrupt tree file
> $ echo total garbage | ./git hash-object -t tree --stdin -w --literally
> fa2905d47028d00baec739f6d73540bb2a75c6f7
>
> but I can use cat-file without --literally to query the contents and
> information about the object as it stands.
>
> $ ./git cat-file tree fa2905d47028d00baec739f6d73540bb2a75c6f7
> total garbage
> $ ./git cat-file -t fa2905d47028d00baec739f6d73540bb2a75c6f7
> tree
> $ ./git cat-file -s fa2905d47028d00baec739f6d73540bb2a75c6f7
> 14
>
> As far as I could tell - and please correct me if I've misunderstood,
> cat-file's literally is about dealing with unrecognized types whereas
> hash-object's --literally is about both creating objects with bad types
> and invalid objects of "recognized" types. This latter scenario is where
> the option name "literally" makes the most sense.

> 
> 
> On 04/20/2015 02:49 PM, Charles Bailey wrote:
> >As far as I could tell - and please correct me if I've misunderstood,
> >cat-file's literally is about dealing with unrecognized types whereas
> >hash-object's --literally is about both creating objects with bad types
> >and invalid objects of "recognized" types. This latter scenario is where
> >the option name "literally" makes the most sense.
> Yes. What you're saying is correct, but it also makes sense as we're asking
> "cat-file" to give us information about the object irrespective of the type of the
> object, hence asking it to literally print the information. Also it stays as a compliment
> to "hash-object --literally", which is already existing.

> On Mon, Apr 20, 2015 at 09:22:47PM +0530, karthik nayak wrote:
>> On 04/20/2015 02:49 PM, Charles Bailey wrote:
>> >As far as I could tell - and please correct me if I've misunderstood,
>> >cat-file's literally is about dealing with unrecognized types whereas
>> >hash-object's --literally is about both creating objects with bad types
>> >and invalid objects of "recognized" types. This latter scenario is where
>> >the option name "literally" makes the most sense.
>> Yes. What you're saying is correct, but it also makes sense as we're asking
>> "cat-file" to give us information about the object irrespective of the type of the
>> object, hence asking it to literally print the information. Also it stays as a compliment
>> to "hash-object --literally", which is already existing.
>
> OK, I think you've hit the main point which I was trying to make.
>
> To me, "literally" means "without transformation" or "exactly as
> written/recorded/transmitted" (which -t/-s do anyway) and doesn't really
> encompass the "irrespective of type" meaning that it has been given here.
>
> In any case, I've made my point so I won't labour it any further. I
> think that --no-validation or --allow-any-type might be more accurate
> but if everyone else is happy enough with --literally then I'm happy to
> live with that too.

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > It's easy to be blinded into thinking that cat-file's new option
> > should be named --literally since it was inspired by the --literally
> > option of hash-object, but indeed it may not be the best choice.
>
> Yeah, I wouldn't even say "inspired".  It was envisioned as a
> counter-part debugging aid, nothing more.
>
> Is there any other way to make cat-file looser other than accepting
> an unknown type name from the future?  If so, then perhaps it may
> make sense to give it a generic name that implys that we would
> trigger such additional looseness in the future.  But as the
> inventor of it as a debugging aid, I would say a name that spells
> out the specific way this option is being loose, e.g.
>
> >      --allow-bogus-type
>
> or with s/bogus/unknown/, best describes what it currently does.

>
> Thanks.
>
>
>

>> Is there any other way to make cat-file looser other than accepting
>> an unknown type name from the future?  If not, then perhaps it may
>> make sense to give it a generic name that implies that we would
>> trigger such additional looseness in the future.  But as the
>> inventor of it as a debugging aid, I would say a name that spells
>> out the specific way this option is being loose, e.g.
>>
>> >      --allow-bogus-type
>>
>> or with s/bogus/unknown/, best describes what it currently does.
>
> Yes this gives the best description, but its large, while we could use something
> like --no-strict instead.

> karthik nayak <karthik.188@gmail.com> writes:
>
> >> Is there any other way to make cat-file looser other than accepting
> >> an unknown type name from the future?  If not, then perhaps it may
> >> make sense to give it a generic name that implies that we would
> >> trigger such additional looseness in the future.  But as the
> >> inventor of it as a debugging aid, I would say a name that spells
> >> out the specific way this option is being loose, e.g.
> >>
> >>>       --allow-bogus-type
> >>
> >> or with s/bogus/unknown/, best describes what it currently does.
> >
> > Yes this gives the best description, but its large, while we could use something
> > like --no-strict instead.
>
> We could, if you answered my first question with "no".
>
> By naming this "--no-strict", the first bug report you will receive
> may be that "cat-file --no-strict" should parse a zlib deflate that
> begins with "blob 1234\n\0" (notice that there are two SPs instead
> of the usual one, and length is followed by LF that should not be
> there before the NUL) but it does not.
>
> As your option name "--no-strict" signals that you will make the
> best effort to parse such nonsense, that would be a valid bug
> report, against which you would need to update the code to make it
> work.  But is it worth the effort to make such a thing work?  I
> dunno.
>

> On 04/25/2015 10:34 PM, Junio C Hamano wrote:
>> karthik nayak <karthik.188@gmail.com> writes:
>> > Yes this gives the best description, but its large, while we could use
>> > something like --no-strict instead.
>>
>> We could, if you answered my first question with "no".
>>
>> By naming this "--no-strict", the first bug report you will receive
>> may be that "cat-file --no-strict" should parse a zlib deflate that
>> begins with "blob 1234\n\0" (notice that there are two SPs instead
>> of the usual one, and length is followed by LF that should not be
>> there before the NUL) but it does not.
>>
>> As your option name "--no-strict" signals that you will make the
>> best effort to parse such nonsense, that would be a valid bug
>> report, against which you would need to update the code to make it
>> work.  But is it worth the effort to make such a thing work?  I
>> dunno.
>>
> Nice point, I don't see the need to parse such objects at the moment.
> That rules out "--no-strict" and everything similar.
>
> I still find "--allow-unkown-type" a bit too big, what about something like
>
> * --any-type
> * --arbitrary-type

> On Mon, Apr 27, 2015 at 7:57 AM, karthik nayak <karthik.188@gmail.com> wrote:
> > On 04/25/2015 10:34 PM, Junio C Hamano wrote:
> >> karthik nayak <karthik.188@gmail.com> writes:
> >>> Yes this gives the best description, but its large, while we could use
> >>> something like --no-strict instead.
> >>
> >> We could, if you answered my first question with "no".
> >>
> >> By naming this "--no-strict", the first bug report you will receive
> >> may be that "cat-file --no-strict" should parse a zlib deflate that
> >> begins with "blob 1234\n\0" (notice that there are two SPs instead
> >> of the usual one, and length is followed by LF that should not be
> >> there before the NUL) but it does not.
> >>
> >> As your option name "--no-strict" signals that you will make the
> >> best effort to parse such nonsense, that would be a valid bug
> >> report, against which you would need to update the code to make it
> >> work.  But is it worth the effort to make such a thing work?  I
> >> dunno.
> >>
> > Nice point, I don't see the need to parse such objects at the moment.
> > That rules out "--no-strict" and everything similar.
> >
> > I still find "--allow-unkown-type" a bit too big, what about something like
> >
> > * --any-type
> > * --arbitrary-type
>
> As a diagnostic aid when encountering a (hopefully rare) broken or
> corrupt object, this option is not likely to be used often. The
> "allow" in --allow-unknown-type conveys the intended purpose more
> accurately than either of the shorter names suggested above; and
> considering how infrequently it is likely to be used, the extra six
> characters should not be a significant burden.
>

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index ab36b1e..61fab78 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -296,4 +308,21 @@ test_expect_success '%(deltabase) reports packed delta bases' '
>         }
>  '
>
> +bogus_type="bogus"
> +bogus_content="bogus"
> +bogus_size=$(strlen $bogus_content)

> +test_expect_success "Type of broken object is correct" '
> +       echo $bogus_type >expect &&
> +       git cat-file -t --literally $bogus_sha1 >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success "Size of broken object is correct" '
> +    echo $bogus_size >expect &&

> +       git cat-file -s --literally $bogus_sha1 >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.4.0.rc1.249.gb598846

> On Wed, Apr 15, 2015 at 1:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> > ---
> > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> > index ab36b1e..61fab78 100755
> > --- a/t/t1006-cat-file.sh
> > +++ b/t/t1006-cat-file.sh
> > @@ -296,4 +308,21 @@ test_expect_success '%(deltabase) reports packed delta bases' '
> >          }
> >   '
> >
> > +bogus_type="bogus"
> > +bogus_content="bogus"
> > +bogus_size=$(strlen $bogus_content)
>
> If someone ever changes the value of 'bogus_content' so it contains
> whitespace, then the result of strlen() will be incorrect as you've
> invoked it. You should quote its argument (as other callers in this
> script do) to safeguard against such an issue.
>
>      bogus_size=$(strlen "$bogus_content")
>
> > +bogus_sha1=$(printf $bogus_content | git hash-object -t $bogus_type --literally -w --stdin)
>
> Ditto regarding quoting of 'bogus_content'.
>
> Also, if someone ever modifies 'bogus_content' so that it contains a
> literal '%' (such as "%s"), then your printf() invocation will
> misbehave. Either call it like this:
>
>      $(printf '%s' "$bogus_content" | ...)
>
> or, better yet, call echo_without_newline() as other similar code
> elsewhere in this script does, and as suggested earlier[1].
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/266972/
>
> > +test_expect_success "Type of broken object is correct" '
> > +       echo $bogus_type >expect &&
> > +       git cat-file -t --literally $bogus_sha1 >actual &&
> > +       test_cmp expect actual
> > +'
> > +
> > +test_expect_success "Size of broken object is correct" '
> > +    echo $bogus_size >expect &&
>
> Bad indentation. Use tab rather than spaces.
>
> > +       git cat-file -s --literally $bogus_sha1 >actual &&
> > +       test_cmp expect actual
> > +'
> > +
> >   test_done
> > --
> > 2.4.0.rc1.249.gb598846