threads / patch / 38907

v5, 2 partscat-file: add a '--literally' option

Subject: [PATCH v5 0/2] cat-file: add a '--literally' option

## tl;dr

10 messages between Mar 25, 2015 and Mar 25, 2015. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

karthik nayak· Mar 25, 2015, 07:19 UTC · lore

Based on Junios and Erics suggestion I have made various changes over the previous iteration of the patch[1].

Changes are :
* polishing of code and commit messages.
Thanks to Junio and Eric for their suggestions and guidance.

Reference: [1]: http://thread.gmane.org/gmane.comp.version-control.git/265604

Karthik Nayak· Mar 25, 2015, 07:21 UTC · re: karthik nayak · lore

[PATCH v5 1/2] sha1_file.c: support reading from a loose object of unknown type

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>
---
 cache.h     |   2 ++
 sha1_file.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 89 insertions(+), 23 deletions(-)
Show changes to 2 files +89 −23

cache.h, sha1_file.c

diff --git a/cache.h b/cache.h
index 4d02efc..949ef4c 100644
--- a/cache.h
+++ b/cache.h
@@ -830,6 +830,7 @@ extern int is_ntfs_dotgit(const char *name);
 
 /* object replacement */
 #define LOOKUP_REPLACE_OBJECT 1
+#define LOOKUP_LITERALLY 2
 extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);
 static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
 {
@@ -1296,6 +1297,7 @@ struct object_info {
 	unsigned long *sizep;
 	unsigned long *disk_sizep;
 	unsigned char *delta_base_sha1;
+	struct strbuf *typename;
 
 	/* Response */
 	enum {
diff --git a/sha1_file.c b/sha1_file.c
index 69a60ec..39041a6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1564,6 +1564,34 @@ 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,
+					struct strbuf *header)
+{
+	unsigned char buffer[32], *cp;
+	unsigned long bufsiz = sizeof(buffer);
+	int status;
+
+	status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
+
+	if (status) {
+		strbuf_add(header, buffer, stream->next_out - buffer);
+		return status;
+	}
+
+	do {
+		status = git_inflate(stream, 0);
+		strbuf_add(header, buffer, stream->next_out - buffer);
+		for (cp = buffer; cp < stream->next_out; cp++)
+			if (!*cp)
+				/* Found the NUL at the end of the header */
+				return 0;
+		stream->next_out = buffer;
+		stream->avail_out = bufsiz;
+	} while (status == Z_OK);
+	return -1;
+}
+
 static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1)
 {
 	int bytes = strlen(buffer) + 1;
@@ -1614,27 +1642,24 @@ 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;
 
 	/*
 	 * The length must follow immediately, and be in canonical
@@ -1652,12 +1677,39 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)
 			size = size * 10 + c;
 		}
 	}
-	*sizep = size;
 
+	type = type_from_string_gently(typename.buf, typename.len, 1);
+	if (oi->sizep)
+		*oi->sizep = size;
+	if (oi->typename)
+		strbuf_addbuf(oi->typename, &typename);
+	strbuf_release(&typename);
+
+	/*
+	 * Set type to 0 if its an unknown object and
+	 * we're obtaining the type using '--literally'
+	 * option.
+	 */
+	if ((flags & LOOKUP_LITERALLY) && (type == -1))
+		type = 0;
+	else if (type == -1)
+		die("invalid object type");
+	if (oi->typep)
+		*oi->typep = type;
 	/*
 	 * The length must be followed by a zero byte
 	 */
-	return *hdr ? -1 : type_from_string(type);
+	return *hdr ? -1 : type;
+}
+
+int parse_sha1_header(const char *hdr, unsigned long *sizep)
+{
+	struct object_info oi;
+
+	oi.sizep = sizep;
+	oi.typename = NULL;
+	oi.typep = NULL;
+	return parse_sha1_header_extended(hdr, &oi, LOOKUP_REPLACE_OBJECT);
 }
 
 static void *unpack_sha1_file(void *map, unsigned long mapsize, enum object_type *type, unsigned long *size, const unsigned char *sha1)
@@ -2524,13 +2576,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);
@@ -2557,17 +2611,23 @@ 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", 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)
-		*oi->typep = status;
+	if (hdrbuf.buf)
+		strbuf_release(&hdrbuf);
 	return 0;
 }
 
@@ -2588,13 +2648,15 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
 			*(oi->disk_sizep) = 0;
 		if (oi->delta_base_sha1)
 			hashclr(oi->delta_base_sha1);
+		if (oi->typename)
+			strbuf_addstr(oi->typename, typename(co->type));
 		oi->whence = OI_CACHED;
 		return 0;
 	}
 
 	if (!find_pack_entry(real, &e)) {
 		/* Most likely it's a loose object. */
-		if (!sha1_loose_object_info(real, oi)) {
+		if (!sha1_loose_object_info(real, oi, flags)) {
 			oi->whence = OI_LOOSE;
 			return 0;
 		}
@@ -2618,6 +2680,8 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
 		oi->u.packed.is_delta = (rtype == OBJ_REF_DELTA ||
 					 rtype == OBJ_OFS_DELTA);
 	}
+	if (oi->typename)
+		strbuf_addstr(oi->typename, typename(rtype));
 
 	return 0;
 }
-- 
2.3.1.170.g5319d60.dirty
Junio C Hamano· Mar 25, 2015, 19:13 UTC · re: Karthik Nayak · lore

Re: [PATCH v5 1/2] sha1_file.c: support reading from a loose object of unknown type

Karthik Nayak <karthik.188@gmail.com> writes:
Show 13 quoted lines
> +	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", 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));
> +	}

I wonder if you can further reduce an unnecessary duplication in the two "else if" clauses in the above, and if the result becomes easier to read and maintain. Perhaps

	if (((flags & LOOKUP_LITERALLY)
             ? unpack_sha1_header_to_strbuf(...)
             : unpack_sha1_header(...)) < 0)
		status = error(...);
	else if ((status = parse_sha1_header_extended(...)) < 0)
        	status = error(...);
or even
	status = 0;
	if (flags & LOOKUP_LITERALLY) {
		if (unpack_sha1_header_to_strbuf(...) < 0)
			status = error(...);
	} else {
		if (unpack_sha1_header(...) < 0)
			status = error(...);
	}
        if (!status) {
        	if (status = parse(...)) < 0)
	        	status = error(...);
	}
although I think the latter might be a bit harder to read.
karthik nayak· Mar 25, 2015, 20:20 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 1/2] sha1_file.c: support reading from a loose object of unknown type

On 03/26/2015 12:43 AM, Junio C Hamano wrote:
Show 44 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +	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", 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));
>> +	}
>
> I wonder if you can further reduce an unnecessary duplication in the
> two "else if" clauses in the above, and if the result becomes easier
> to read and maintain.  Perhaps
>
> 	if (((flags & LOOKUP_LITERALLY)
>               ? unpack_sha1_header_to_strbuf(...)
>               : unpack_sha1_header(...)) < 0)
> 		status = error(...);
> 	else if ((status = parse_sha1_header_extended(...)) < 0)
>          	status = error(...);
>
> or even
>
> 	status = 0;
> 	if (flags & LOOKUP_LITERALLY) {
> 		if (unpack_sha1_header_to_strbuf(...) < 0)
> 			status = error(...);
> 	} else {
> 		if (unpack_sha1_header(...) < 0)
> 			status = error(...);
> 	}
>          if (!status) {
>          	if (status = parse(...)) < 0)
> 	        	status = error(...);
> 	}
>
> although I think the latter might be a bit harder to read.
>

I hope you meant the former. The latter to me seems simpler as its a simple if else statement whereas the former has a ternary operator with function calls. I did think about this when writing the code, the problem is when flag == LOOKUP_LITERALLY, parse_sha1_header_extended() takes 'hdrbuf.buf' as first argument where as when flag != LOOKUP_LITERALLY, it takes 'hdr' as an argument. We could do this

status = 0;
char * hdrp;
if (flags & LOOKUP_LITERALLY) {
	if (unpack_sha1_header_to_strbuf(...) < 0)
		status = error(...);
	hdrp = hdrbuf.buf;
} else {
	if (unpack_sha1_header(...) < 0)
		status = error(...);
	hdrp = hdr;
}
if (!status)
	if (status = parse(hdrp, ...)) < 0)
		status = error(...);
}

But I think it just introduces another variable to keep track of, which I rather not have.

Junio C Hamano· Mar 25, 2015, 20:32 UTC · re: karthik nayak · lore

Re: [PATCH v5 1/2] sha1_file.c: support reading from a loose object of unknown type

karthik nayak <karthik.188@gmail.com> writes:
> I hope you meant the former.

I did mean the latter, which spends more lines without much information (i.e. only closing braces), only to reduce the duplication of two simple lines.

But you are right, I did miss the distinction between hdrbuf.buf and hdr, so they are not identically duplicated.

Junio C Hamano· Mar 25, 2015, 19:27 UTC · re: Karthik Nayak · lore

Re: [PATCH v5 1/2] sha1_file.c: support reading from a loose object of unknown type

Karthik Nayak <karthik.188@gmail.com> writes:
Show 26 quoted lines
> +static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
> +					unsigned long mapsize,
> +					struct strbuf *header)
> +{
> +	unsigned char buffer[32], *cp;
> +	unsigned long bufsiz = sizeof(buffer);
> +	int status;
> +
> +	status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
> +
> +	if (status) {
> +		strbuf_add(header, buffer, stream->next_out - buffer);
> +		return status;
> +	}
> +
> +	do {
> +		status = git_inflate(stream, 0);
> +		strbuf_add(header, buffer, stream->next_out - buffer);
> +		for (cp = buffer; cp < stream->next_out; cp++)
> +			if (!*cp)
> +				/* Found the NUL at the end of the header */
> +				return 0;
> +		stream->next_out = buffer;
> +		stream->avail_out = bufsiz;
> +	} while (status == Z_OK);
> +	return -1;

A caller of unpack_sha1_header() gets the return from git_inflate(); I wonder if we should be returning "status" here for consistency, perhaps?

karthik nayak· Mar 25, 2015, 20:22 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 1/2] sha1_file.c: support reading from a loose object of unknown type

On 03/26/2015 12:57 AM, Junio C Hamano wrote:
Show 33 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
>> +					unsigned long mapsize,
>> +					struct strbuf *header)
>> +{
>> +	unsigned char buffer[32], *cp;
>> +	unsigned long bufsiz = sizeof(buffer);
>> +	int status;
>> +
>> +	status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
>> +
>> +	if (status) {
>> +		strbuf_add(header, buffer, stream->next_out - buffer);
>> +		return status;
>> +	}
>> +
>> +	do {
>> +		status = git_inflate(stream, 0);
>> +		strbuf_add(header, buffer, stream->next_out - buffer);
>> +		for (cp = buffer; cp < stream->next_out; cp++)
>> +			if (!*cp)
>> +				/* Found the NUL at the end of the header */
>> +				return 0;
>> +		stream->next_out = buffer;
>> +		stream->avail_out = bufsiz;
>> +	} while (status == Z_OK);
>> +	return -1;
>
> A caller of unpack_sha1_header() gets the return from git_inflate();
> I wonder if we should be returning "status" here for consistency,
> perhaps?
>

Although the caller only checks for negative values for failure. For consistency it would be better to return status as unpack_sha1_header() does.

Karthik Nayak· Mar 25, 2015, 07:22 UTC · re: karthik nayak · lore

[PATCH v5 2/2] cat-file: teach cat-file a '--literally' option

Currently 'git cat-file' throws an error while trying to print the type or size of a broken/corrupt object which is created using 'git hash-object --literally'. 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.

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

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/cat-file.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)
Show changes to builtin/cat-file.c +32 −9
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index df99df4..6fee461 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -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 literally)
 {
 	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 (literally)
+		flags |= LOOKUP_LITERALLY;
 
 	if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
 		die("Not a valid object name %s", obj_name);
@@ -23,16 +30,24 @@ 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.typep = &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) {
+		oi.typep = &type;
+		oi.typename = &sb;
+		oi.sizep = &size;
+		if (sha1_object_info_extended(sha1, &oi, flags) < 0)
+			die("git cat-file: could not get object info");
+		if (sb.len) {
 			printf("%lu\n", size);
 			return 0;
 		}
@@ -323,7 +338,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 +374,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 +385,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_("get information about corrupt objects for debugging Git")),
 		{ 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 +398,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);
 
 	argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
@@ -405,5 +423,10 @@ 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 (literally && (opt == 't' || opt == 's'))
+		return cat_one_file(opt, exp_type, obj_name, literally);
+	else if (literally)
+		die("git cat-file --literally: use with -s or -t");
+
+	return cat_one_file(opt, exp_type, obj_name, literally);
 }
-- 
2.3.1.170.g5319d60.dirty
Eric Sunshine· Mar 25, 2015, 07:42 UTC · re: Karthik Nayak · lore

Re: [PATCH v5 2/2] cat-file: teach cat-file a '--literally' option

On Wed, Mar 25, 2015 at 3:22 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 31 quoted lines
> Currently 'git cat-file' throws an error while trying to
> print the type or size of a broken/corrupt object which is
> created using 'git hash-object --literally'. 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.
>
> Modify '-t' and '-s' options to call sha1_object_info_extended()
> directly to support the '--literally' option.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index df99df4..6fee461 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -405,5 +423,10 @@ 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 (literally && (opt == 't' || opt == 's'))
> +               return cat_one_file(opt, exp_type, obj_name, literally);
> +       else if (literally)
> +               die("git cat-file --literally: use with -s or -t");
> +
> +       return cat_one_file(opt, exp_type, obj_name, literally);

Although I haven't read the patch closely yet, this unnecessarily complex logic caught my eye as I was scanning the changes. If you check for the illegal option combination first, then you can coalesce the two identical cat_one_file() invocations, and this entire hunk reduces to the more readable:

    if (literally && opt != 't' && opt != 's')
        die("git cat-file --literally: use with -s or -t");
    return cat_one_file(opt, exp_type, obj_name, literally);
>  }
> --
> 2.3.1.170.g5319d60.dirty
karthik nayak· Mar 25, 2015, 07:48 UTC · re: Eric Sunshine · lore

Re: [PATCH v5 2/2] cat-file: teach cat-file a '--literally' option

On 03/25/2015 01:12 PM, Eric Sunshine wrote:
Show 47 quoted lines
> On Wed, Mar 25, 2015 at 3:22 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> > Currently 'git cat-file' throws an error while trying to
> > print the type or size of a broken/corrupt object which is
> > created using 'git hash-object --literally'. 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.
> >
> > Modify '-t' and '-s' options to call sha1_object_info_extended()
> > directly to support the '--literally' option.
> >
> > Helped-by: Junio C Hamano <gitster@pobox.com>
> > Helped-by: Eric Sunshine <sunshine@sunshineco
> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> > ---
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index df99df4..6fee461 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -405,5 +423,10 @@ 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 (literally && (opt == 't' || opt == 's'))
> > +               return cat_one_file(opt, exp_type, obj_name, literally);
> > +       else if (literally)
> > +               die("git cat-file --literally: use with -s or -t");
> > +
> > +       return cat_one_file(opt, exp_type, obj_name, literally);
>
> Although I haven't read the patch closely yet, this unnecessarily
> complex logic caught my eye as I was scanning the changes. If you
> check for the illegal option combination first, then you can coalesce
> the two identical cat_one_file() invocations, and this entire hunk
> reduces to the more readable:
>
>      if (literally && opt != 't' && opt != 's')
>          die("git cat-file --literally: use with -s or -t");
>
>      return cat_one_file(opt, exp_type, obj_name, literally);
>
> >   }
> > --
> > 2.3.1.170.g5319d60.dirty

Hey Eric, Yes that could be done.

When small fixes like these need to be done, Is it sufficient to reply to this mail with the fixed patch, or should I resend the whole patch series?

Thanks, Karthik

← back to recent threads