cat-file: teach cat-file a '--allow-unkown-type' 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.

sha1_file.c +5 -3

@@ -2522,13 +2572,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;

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

Quoted 2 lines

Add parse_sha1_header_extended() which acts as a wrapper around parse_sha1_header() allowing more information to be obtained.

Thanks. This mostly looks good modulo a nit.

sha1_file.c int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma · line 1569

Quoted 12 lines

sha1_file.c +3 -0

index 980ce6b..9a15634 100644
@@ -1564,6 +1564,33 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma
  	return git_inflate(stream, 0);
  }
 
+static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
+					unsigned long mapsize, void *buffer,
+					unsigned long bufsiz, struct strbuf *header)

This function in this round looks somewhat tricky.

sha1_file.c int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma · line 1574

Quoted 5 lines

sha1_file.c +5 -0

+{
+	unsigned char *cp;
+	int status;
+
+	status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);

Here, we unpack into the caller-supplied buffer, without touching the caller-supplied header strbuf.

sha1_file.c int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma · line 1580

Quoted 5 lines

sha1_file.c +5 -0

+	/*
+	 * Check if entire header is unpacked in the first iteration.
+	 */
+	if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
+		return 0;

And we return the object header in the buffer without ever touching the header strbuf. We expect that the caller would know that the buffer it gave us would contain the full object header line.

sha1_file.c int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma · line 1587

Quoted 6 lines

sha1_file.c +6 -0

+	strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);
+	do {
+		status = git_inflate(stream, 0);
+		strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);
+		if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
+			return 0;

However, here, we return the object header in the caller-supplied header strbuf, while using the caller-supplied buffer as a scratch area. We expect that the caller would know that the header strbuf is what it has to use to find the object header.

Which is good in the sense that there is no unnecessary copies, but the caller needs to be careful to do something like:

	if (!unpack_to_strbuf(... buffer, sizeof(buffer), &header)) {
     		if (header.len)
                 	object_header = header.buf;
		else
                 	object_header = buffer;
	} else {
         	error("cannot unpack the object header");
	}

It is a good trade-off between complexity and efficiency. The complexity is isolated as the function is file-scope-static and it is perfectly fine to force the callers to be extra careful.

But this suggests that the patch to add tests should try at least two, preferably three, kinds of test input. A bogus type that needs a header longer than the caller's fixed buffer, a bogus type whose header would fit within the fixed buffer, and optionally a correct type whose header should always fit within the fixed buffer.

sha1_file.c static void *unpack_sha1_rest(git_zstream *stream, void · line 1673

Quoted 12 lines

sha1_file.c +9 -0

@@ -1614,27 +1641,38 @@ static void *unpack_sha1_rest(git_zstream *stream, void
...
+	/*
+	 * Set type to 0 if its an unknown object and
+	 * we're obtaining the type using '--allow-unkown-type'
+	 * option.
+	 */
+	if ((flags & LOOKUP_UNKNOWN_OBJECT) && (type == -1))
+		type = 0;
+	else if (type == -1)
+		die("invalid object type");

Would "type == -2" or any other negative value, if existed, mean something different? I do not think so, and hence I would prefer these two checks be done with "type < 0" instead.

Quoted 17 lines

sha1_file.c +5 -3

@@ -2522,13 +2572,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;

sha1_file.c struct packed_git *find_sha1_pack(const unsigned char *sha1,

Quoted 10 lines

sha1_file.c +6 -1

...
+	if ((flags & LOOKUP_UNKNOWN_OBJECT)) {
+		if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, hdr, sizeof(hdr), &hdrbuf) < 0)
+			status = error("unable to unpack %s header with --allow-unknown-type",
+				       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(hdr, &size)) < 0)
+	if (hdrbuf.len) {

And here the caller is being careful, but the way it does is a bit subtle. Even if it has a codepath that entirely bypasses a call to unpack_to_strbuf(), it knows that the other codepath does not touch the hdrbuf strbuf, which means that this block can be reached only when it called unpack_to_strbuf(), the callee found a long object header, and the unpacking succeeded. Otherwise, it will make a call to parse_sha1_header_extended() using the fixed hdr[] in the "else if" clause that corresponds to this "if".

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

Quoted 5 lines

sha1_file.c +4 -0

+		if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)
+			status = error("unable to parse %s header with --allow-unknown-type",
+				       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));

However, I wonder what happens when "status" is already set after the call to unpack functions above. hdrbuf.len may be zero so this "else if" part would be taken, and it then still try to feed the hdr[] to parse_sha1_header_extended(), no?

In other words, shouldn't the flow be more like this?

	if (lookup-unknown) {
         	if (unpack-to-strbuf)
			status = error();
	} else if (unpack-to-hdr)
		status = error();

(quoted diff) +3 -1

+	if (status < 0)
+        	; /* do not do anything */
+	else if (hdrbuf.len)
-	if (hdrbuf.len)

		parse(hdrbuf.buf);
	else
         	parse(hdr);

Quoted 9 lines · Junio C Hamano

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

Quoted 4 lines

Quoted 2 lines

Add parse_sha1_header_extended() which acts as a wrapper around parse_sha1_header() allowing more information to be obtained.

Thanks. This mostly looks good modulo a nit.

Sorry didn't get what you meant by "modulo a nit.".

Quoted 9 lines

It is a good trade-off between complexity and efficiency. The complexity is isolated as the function is file-scope-static and it is perfectly fine to force the callers to be extra careful.

But this suggests that the patch to add tests should try at least two, preferably three, kinds of test input. A bogus type that needs a header longer than the caller's fixed buffer, a bogus type whose header would fit within the fixed buffer, and optionally a correct type whose header should always fit within the fixed buffer.

Yes it is a tradeoff, and it is complex as in the user has to check the strbuf provided to see if its been used. But this like you said I guess its a good tradeoff.
About the three tests, My patch checks "a bogus type whose header would fit within the fixed buffer" and "correct type whose header should always fit within the fixed buffer" but will write a test to check "A bogus type that needs a header longer than the caller's fixed buffer"

Quoted 13 lines · karthik nayak

Quoted 12 lines · Junio C Hamano

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

Add parse_sha1_header_extended() which acts as a wrapper around parse_sha1_header() allowing more information to be obtained.

Quoted 3 lines

Quoted 1 line

Thanks. This mostly looks good modulo a nit.

Sorry didn't get what you meant by "modulo a nit.".

"nit" as in "Nit-pick"; a small imperfection that may need to be corrected (such as the "what if we saw failure earlier and 'status' already had a value?" issue).

Quoted 11 lines

Quoted 10 lines

It is a good trade-off between complexity and efficiency. The complexity is isolated as the function is file-scope-static and it is perfectly fine to force the callers to be extra careful.

But this suggests that the patch to add tests should try at least two, preferably three, kinds of test input. A bogus type that needs a header longer than the caller's fixed buffer, a bogus type whose header would fit within the fixed buffer, and optionally a correct type whose header should always fit within the fixed buffer.

Quoted 8 lines

Yes it is a tradeoff, and it is complex as in the user has to check the strbuf provided to see if its been used. But this like you said I guess its a good tradeoff.
About the three tests, My patch checks "a bogus type whose header would fit within the fixed buffer" and "correct type whose header should always fit within the fixed buffer" but will write a test to check "A bogus type that needs a header longer than the caller's fixed buffer"

Yup. Please do so; that would make the test coverage more complete.

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 '--allow-unkown-type' option where it prints the type or size of a broken/corrupt object without throwing an error.

Quoted 8 lines · Karthik Nayak

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 '--allow-unkown-type' option where it prints the type or size of a broken/corrupt object without throwing an error.

In this entire series, replace all 'unkown' with 'unknown' in both the commit messages and the code ("unknown" is misspelled most of the time). I notice the switch name itself is misspelled, but also variable names such as 'unkown_type' in this patch.

Respectfully, because I know English is a challenging beast sometimes, and spelling is difficult even for many native speakers...

Quoted 3 lines

Respectfully, because I know English is a challenging beast sometimes, and spelling is difficult even for many native speakers...

Thanks for that, Yes it does get a bit tricky with spellings, will find an editor with spellcheck and avoid nano :D

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 '--allow-unkown-type' option where it prints the type or size of a broken/corrupt object without throwing an error.

Modify '-t' and '-s' options to call sha1_object_info_extended() directly to support the '--allow-unkown-type' option.

Helped-by Junio C Hamano <[email protected]>

Helped-by Eric Sunshine <[email protected]>

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

---
 builtin/cat-file.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

builtin/cat-file.c +26 -12

index 53b5376..299e2e5 100644
@@ -9,13 +9,20 @@
 #include "userdiff.h"
 #include "streaming.h"
 
-static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
+static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
+			int unkown_type)
 {
 	unsigned char sha1[20];
 	enum object_type type;
 	char *buf;
 	unsigned long size;
 	struct object_context obj_context;
+	struct object_info oi = {NULL};
+	struct strbuf sb = STRBUF_INIT;
+	unsigned flags = LOOKUP_REPLACE_OBJECT;
+
+	if (unkown_type)
+		flags |= LOOKUP_UNKNOWN_OBJECT;
 
 	if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
 		die("Not a valid object name %s", obj_name);

@@ -23,20 +30,22 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 	buf = NULL;
 	switch (opt) {
 	case 't':
-		type = sha1_object_info(sha1, NULL);
-		if (type > 0) {
-			printf("%s\n", typename(type));
+		oi.typename = &sb;
+		if (sha1_object_info_extended(sha1, &oi, flags) < 0)
+			die("git cat-file: could not get object info");
+		if (sb.len) {
+			printf("%s\n", sb.buf);
+			strbuf_release(&sb);
 			return 0;
 		}
 		break;
 
 	case 's':
-		type = sha1_object_info(sha1, &size);
-		if (type > 0) {
-			printf("%lu\n", size);
-			return 0;
-		}
-		break;
+		oi.sizep = &size;
+		if (sha1_object_info_extended(sha1, &oi, flags) < 0)
+			die("git cat-file: could not get object info");
+		printf("%lu\n", size);
+		return 0;
 
 	case 'e':
 		return !has_sha1_file(sha1);

@@ -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 [--allow-unkown-type]|-s [--allow-unkown-type]|-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 unkown_type = 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_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, "allow-unkown-type", &unkown_type,
+			  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 },

@@ -402,5 +414,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	if (batch.enabled)
 		return batch_objects(&batch);
 
-	return cat_one_file(opt, exp_type, obj_name);
+	if (unkown_type && opt != 't' && opt != 's')
+		die("git cat-file --allow-unkown-type: use with -s or -t");
+	return cat_one_file(opt, exp_type, obj_name, unkown_type);
 }

Quoted 4 lines · Karthik Nayak

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.

Quoted 3 lines

Teach git cat-file a '--allow-unkown-type' option where it prints the type or size of a broken/corrupt object without throwing an error.

Drop "Currently" from the description of the problem you are solving. We know that the problem you have to solve in the code is current without being told. This comment applies to all patches.

Quoted 104 lines

Modify '-t' and '-s' options to call sha1_object_info_extended() directly to support the '--allow-unkown-type' option.

Helped-by Junio C Hamano <[email protected]>

Helped-by Eric Sunshine <[email protected]>

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

---
  builtin/cat-file.c | 38 ++++++++++++++++++++++++++------------
  1 file changed, 26 insertions(+), 12 deletions(-)

builtin/cat-file.c +26 -12

index 53b5376..299e2e5 100644
@@ -9,13 +9,20 @@
  #include "userdiff.h"
  #include "streaming.h"
 
-static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
+static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
+			int unkown_type)
  {
  	unsigned char sha1[20];
  	enum object_type type;
  	char *buf;
  	unsigned long size;
  	struct object_context obj_context;
+	struct object_info oi = {NULL};
+	struct strbuf sb = STRBUF_INIT;
+	unsigned flags = LOOKUP_REPLACE_OBJECT;
+
+	if (unkown_type)
+		flags |= LOOKUP_UNKNOWN_OBJECT;
 
  	if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
  		die("Not a valid object name %s", obj_name);

@@ -23,20 +30,22 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
  	buf = NULL;
  	switch (opt) {
  	case 't':
-		type = sha1_object_info(sha1, NULL);
-		if (type > 0) {
-			printf("%s\n", typename(type));
+		oi.typename = &sb;
+		if (sha1_object_info_extended(sha1, &oi, flags) < 0)
+			die("git cat-file: could not get object info");
+		if (sb.len) {
+			printf("%s\n", sb.buf);
+			strbuf_release(&sb);
  			return 0;
  		}
  		break;
 
  	case 's':
-		type = sha1_object_info(sha1, &size);
-		if (type > 0) {
-			printf("%lu\n", size);
-			return 0;
-		}
-		break;
+		oi.sizep = &size;
+		if (sha1_object_info_extended(sha1, &oi, flags) < 0)
+			die("git cat-file: could not get object info");
+		printf("%lu\n", size);
+		return 0;
 
  	case 'e':
  		return !has_sha1_file(sha1);

@@ -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 [--allow-unkown-type]|-s [--allow-unkown-type]|-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 unkown_type = 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_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, "allow-unkown-type", &unkown_type,
+			  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 },

@@ -402,5 +414,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
  	if (batch.enabled)
  		return batch_objects(&batch);
 
-	return cat_one_file(opt, exp_type, obj_name);
+	if (unkown_type && opt != 't' && opt != 's')
+		die("git cat-file --allow-unkown-type: use with -s or -t");
+	return cat_one_file(opt, exp_type, obj_name, unkown_type);
  }

cat-file: add documentation for '--allow-unkown-type' option.

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

---

Documentation/git-cat-file.txt +4 -2

index f6a16f4..f6f6064 100644
@@ -9,7 +9,7 @@ git-cat-file - Provide content or type and size information for repository objec
 SYNOPSIS
 --------
 [verse]
-'git cat-file' (-t | -s | -e | -p | <type> | --textconv ) <object>
+'git cat-file' (-t [--allow-unkown-type]| -s [--allow-unkown-type]| -e | -p | <type> | --textconv ) <object>
 'git cat-file' (--batch | --batch-check) < <list-of-objects>
 
 DESCRIPTION

@@ -69,6 +69,9 @@ OPTIONS
        not be combined with any other options or arguments.  See the
        section `BATCH OUTPUT` below for details.
 
+--allow-unkown-type::
+       Allow -s or -t to query broken/corrupt objects of unknown type.
+
 OUTPUT
 ------
 If '-t' is specified, one of the <type>.
--

2.4.0.rc1.250.g565e85b

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

+       test_cmp expect actual
+    '
+
+    test_expect_success "Size of $type is correct using --allow-unkown-type" '
+       echo $size >expect &&
+    git cat-file -s --allow-unkown-type $sha1 >actual &&
+       test_cmp expect actual
+    '
+
     test -z "$content" ||
     test_expect_success "Content of $type is correct" '
        maybe_remove_timestamp "$content" $no_ts >expect &&

@@ -296,4 +308,21 @@ test_expect_success '%(deltabase) reports packed delta bases' '
        }
 '
 
+bogus_type="bogus"
+bogus_content="bogus"
+bogus_size=$(strlen "$bogus_content")
+bogus_sha1=$(echo_without_newline "$bogus_content" | git hash-object -t $bogus_type --literally -w --stdin)
+
+test_expect_success "Type of broken object is correct" '
+       echo $bogus_type >expect &&
+    git cat-file -t --allow-unkown-type $bogus_sha1 >actual &&
+       test_cmp expect actual
+'
+
+test_expect_success "Size of broken object is correct" '
+       echo $bogus_size >expect &&
+    git cat-file -s --allow-unkown-type $bogus_sha1 >actual &&
+       test_cmp expect actual
+'
+
 test_done
--

2.4.0.rc1.250.g565e85b