cat-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 <[email protected]>

Helped-by Eric Sunshine <[email protected]>

Signed-off-by Karthik Nayak <[email protected]>

---

sha1_file.c +4 -4

@@ -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;

sha1_file.c static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s · line 1637

Quoted 6 lines

sha1_file.c +1 -4

-		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.

Quoted 11 lines

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.

Quoted 15 lines · Jeff King

Quoted 11 lines

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:

Quoted 7 lines

   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?

-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.

Quoted 3 lines

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 :

Quoted 8 lines · karthik nayak

Quoted 3 lines

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.

sha1_file.c static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s

Quoted 6 lines

   {

sha1_file.c +2 -2

-       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

Review

Quoted 4 lines · karthik nayak

(quoted diff) +4 -0

+       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?

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?

-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.

Quoted 2 lines · Jeff King

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.

Quoted 6 lines · Junio C Hamano

Quoted 2 lines · Jeff King

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.

Quoted 9 lines · Jeff King

Quoted 6 lines · Junio C Hamano

Quoted 2 lines · Jeff King

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.

Quoted 8 lines

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 ;-)

Quoted 10 lines · karthik nayak

+static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,

(quoted diff) +2 -0

+                                       unsigned long mapsize, void *buffer,
+                                       unsigned long bufsiz, struct strbuf

*header)

(quoted diff) +5 -0

+{
+       unsigned char *cp;
+       int status;
+       int i = 0;
+

Quoted 1 line

+ 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).

Review

Quoted 5 lines

(quoted diff) +5 -0

+       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.

-Peff

Quoted 9 lines

Quoted 1 line

+ 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?

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.
[...]
---

sha1_file.c +16 -10

index 980ce6b..267399d 100644
@@ -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));

sha1_file.c +0 -0

        return 0;
 }
 

Quoted 58 lines · Karthik Nayak

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.
[...]
---

sha1_file.c +19 -10

index 980ce6b..267399d 100644
@@ -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);

sha1_file.c static int sha1_loose_object_info(const unsigned char *sha1, · line 2632

Quoted 5 lines

sha1_file.c +3 -1

-       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?

Quoted 4 lines

sha1_file.c +0 -0

         return 0;
  }
 

builtin/cat-file.c +7 -1

+			die("git cat-file: could not get object info");
+		printf("%lu\n", size);
+		return 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 },

Karthik Nayak <[email protected]> writes:
...

Review

Quoted 3 lines

(quoted diff) +1 -1

-	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.

Quoted 25 lines · Junio C Hamano

Karthik Nayak <[email protected]> writes:
...

Review

Quoted 3 lines

(quoted diff) +1 -1

-	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.

Quoted 5 lines

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(-)

builtin/cat-file.c +5 -5

index 1ec7190..534991d 100644
@@ -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")),

Quoted 7 lines · Junio C Hamano

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.

... 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...

Quoted 10 lines · karthik nayak

Quoted 7 lines · Junio C Hamano

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.

Quoted 2 lines

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.

Quoted 4 lines

... 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.

Quoted 7 lines · Junio C Hamano

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?

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.

Quoted 4 lines · Charles Bailey

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.

Quoted 4 lines · Junio C Hamano

Quoted 1 line · Charles Bailey

The option isn't a true opposite of hash-object's --literally

because

Quoted 1 line

Quoted 1 line

that also allows the creation of known types with invalid contents

(e.g.

Quoted 1 line

Quoted 1 line

corrupt trees) whereas cat-file is quite happy to show the

_contents_ of

Quoted 3 lines

Quoted 1 line

such corrupt objects even without --literally.

Not really. If you create an object with corrupt type string (e.g.

"BLOB"

Quoted 1 line

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.

Sorry, but I didn't get you, broken objects created using hash-object --literally do not work with cat-file without the --literally option.

Quoted 1 line · Karthik Nayak

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.

Charles.

Quoted 5 lines · Charles Bailey

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.

Quoted 10 lines · karthik nayak

Quoted 5 lines · Charles Bailey

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.

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.

--allow-bogus-type

Quoted 3 lines · Eric Sunshine

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.

Quoted 1 line

--allow-bogus-type

or with s/bogus/unknown/, best describes what it currently does.

Thanks.

Quoted 7 lines

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.

Quoted 3 lines

Quoted 1 line

--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.

Quoted 7 lines · karthik nayak

Quoted 7 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

--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.

Quoted 12 lines · Junio C Hamano

Quoted 2 lines · karthik nayak

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.

Quoted 6 lines

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

Quoted 19 lines · karthik nayak

Quoted 12 lines · Junio C Hamano

Quoted 2 lines · karthik nayak

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.

Quoted 6 lines

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.

Quoted 7 lines

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.

t/t1006-cat-file.sh +4 -1

+       git cat-file -s --literally $bogus_sha1 >actual &&
+       test_cmp expect actual
+'
+
 test_done
--

2.4.0.rc1.249.gb598846

t/t1006-cat-file.sh test_expect_success '%(deltabase) reports packed delta bases' ' · line 313

Quoted 13 lines · Karthik Nayak

Signed-off-by Karthik Nayak <[email protected]>

---

t/t1006-cat-file.sh +3 -0

index ab36b1e..61fab78 100755
@@ -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")

t/t1006-cat-file.sh test_expect_success '%(deltabase) reports packed delta bases' ' · line 314

Quoted 1 line

t/t1006-cat-file.sh +1 -0

+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/

t/t1006-cat-file.sh test_expect_success '%(deltabase) reports packed delta bases' ' · line 323

Quoted 8 lines

t/t1006-cat-file.sh +8 -0

+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.

Quoted 7 lines

t/t1006-cat-file.sh +4 -1

+       git cat-file -s --literally $bogus_sha1 >actual &&
+       test_cmp expect actual
+'
+
  test_done
--

2.4.0.rc1.249.gb598846