{"thread":{"id":"38828","subject":"[PATCH v4 0/2] cat-file: add a '--literally' option","startedAt":"2015-03-17T05:13:18Z","lastAt":"2015-03-18T19:35:08Z","messageCount":7,"participants":["karthik nayak","Karthik Nayak","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"257828","messageId":"5507B7EE.1070403@gmail.com","threadId":"38828","inReplyTo":null,"subject":"[PATCH v4 0/2] cat-file: add a '--literally' option","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-17T05:13:18Z","receivedAt":"2015-03-17T05:13:18Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Based on Junios and Erics suggestion I have made various\nchanges over the previous iteration of the patch[1].\n\nChanges in this version :\n* Add a object_info::typename to hold all the typenames.\n* Add a wrapper around parse_sha1_header() to get type and\n   size of broken/corrupt objects without throwing an error\n   whenever the type is unknown.\n* Also add an option for 'cat-file -s --literally'.\n\nThanks to Junio and Eric for their suggestions and guidance.\n\n[1]http://article.gmane.org/gmane.comp.version-control.git/264853\n"},{"id":"257829","messageId":"1426569400-8204-1-git-send-email-karthik.188@gmail.com","threadId":"38828","inReplyTo":"5507B7EE.1070403@gmail.com","subject":"[PATCH v4 1/2] cat-file: teach cat-file a '--literally' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-17T05:16:40Z","receivedAt":"2015-03-17T05:16:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Currently 'git cat-file' throws an error while trying to\nprint the type or size of a broken/corrupt object which is\ncreated using 'git hash-object --literally'. This is\nbecause these objects are usually of unknown types.\n\nTeach git cat-file a '--literally' option where it prints\nthe type or size of a broken/corrupt object without throwing\nan error.\n\nModify '-t' and '-s' options to call sha1_object_info_extended()\ndirectly to support the '--literally' option.\n\nAdd a 'LOOKUP_LITERALLY' flag to notify sha1_object_info_extended()\nwhenever the '--literally' is being used.\n\nAdd object_info::typename to support 'cat-file --literally' and\nstore the typename of objects.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/cat-file.c | 43 +++++++++++++++++++++++++++++++++----------\n cache.h            |  2 ++\n 2 files changed, 35 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex df99df4..1625246 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -9,13 +9,20 @@\n #include \"userdiff.h\"\n #include \"streaming.h\"\n \n-static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n+static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n+\t\t\tint literally)\n {\n \tunsigned char sha1[20];\n \tenum object_type type;\n \tchar *buf;\n \tunsigned long size;\n \tstruct object_context obj_context;\n+\tstruct object_info oi = {NULL};\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tunsigned flags = LOOKUP_REPLACE_OBJECT;\n+\n+\tif (literally)\n+\t\tflags |= LOOKUP_LITERALLY;\n \n \tif (get_sha1_with_context(obj_name, 0, sha1, &obj_context))\n \t\tdie(\"Not a valid object name %s\", obj_name);\n@@ -23,16 +30,24 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \tbuf = NULL;\n \tswitch (opt) {\n \tcase 't':\n-\t\ttype = sha1_object_info(sha1, NULL);\n-\t\tif (type > 0) {\n-\t\t\tprintf(\"%s\\n\", typename(type));\n+\t\toi.typep = &type;\n+\t\toi.typename = &sb;\n+\t\tif (sha1_object_info_extended(sha1, &oi, flags) < 0)\n+\t\t\tdie(\"git cat-file: could not get object info\");\n+\t\tif (sb.len) {\n+\t\t\tprintf(\"%s\\n\", sb.buf);\n+\t\t\tstrbuf_release(&sb);\n \t\t\treturn 0;\n \t\t}\n \t\tbreak;\n \n \tcase 's':\n-\t\ttype = sha1_object_info(sha1, &size);\n-\t\tif (type > 0) {\n+\t\toi.typep = &type;\n+\t\toi.typename = &sb;\n+\t\toi.sizep = &size;\n+\t\tif (sha1_object_info_extended(sha1, &oi, flags) < 0)\n+\t\t\tdie(\"git cat-file: could not get object info\");\n+\t\tif (sb.len) {\n \t\t\tprintf(\"%lu\\n\", size);\n \t\t\treturn 0;\n \t\t}\n@@ -323,8 +338,8 @@ static int batch_objects(struct batch_options *opt)\n }\n \n static const char * const cat_file_usage[] = {\n-\tN_(\"git cat-file (-t | -s | -e | -p | <type> | --textconv) <object>\"),\n-\tN_(\"git cat-file (--batch | --batch-check) < <list-of-objects>\"),\n+\tN_(\"git cat-file (-t [--literally]|-s [--literally]|-e|-p|<type>|--textconv) <object>\"),\n+\tN_(\"git cat-file (--batch | --batch-check) <list-of-objects>\"),\n \tNULL\n };\n \n@@ -359,6 +374,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \tint opt = 0;\n \tconst char *exp_type = NULL, *obj_name = NULL;\n \tstruct batch_options batch = {0};\n+\tint literally = 0;\n \n \tconst struct option options[] = {\n \t\tOPT_GROUP(N_(\"<type> can be one of: blob, tree, commit, tag\")),\n@@ -369,6 +385,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\tOPT_SET_INT('p', NULL, &opt, N_(\"pretty-print object's content\"), 'p'),\n \t\tOPT_SET_INT(0, \"textconv\", &opt,\n \t\t\t    N_(\"for blob objects, run textconv on object's content\"), 'c'),\n+\t\tOPT_BOOL( 0, \"literally\", &literally,\n+\t\t\t  N_(\"get information about corrupt objects for debugging Git\")),\n \t\t{ OPTION_CALLBACK, 0, \"batch\", &batch, \"format\",\n \t\t\tN_(\"show info and content of objects fed from the standard input\"),\n \t\t\tPARSE_OPT_OPTARG, batch_option_callback },\n@@ -380,7 +398,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_cat_file_config, NULL);\n \n-\tif (argc != 3 && argc != 2)\n+\tif (argc < 2 || argc > 4)\n \t\tusage_with_options(cat_file_usage, options);\n \n \targc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);\n@@ -405,5 +423,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \tif (batch.enabled)\n \t\treturn batch_objects(&batch);\n \n-\treturn cat_one_file(opt, exp_type, obj_name);\n+\tif (literally && (opt == 't' || opt == 's'))\n+\t\treturn cat_one_file(opt, exp_type, obj_name, literally);\n+\telse if (literally)\n+\t\tdie(\"git cat-file --literally: use with -s or -t\");\n+\n+\treturn cat_one_file(opt, exp_type, obj_name, literally);\n }\ndiff --git a/cache.h b/cache.h\nindex 761c570..7c151ee 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -830,6 +830,7 @@ extern int is_ntfs_dotgit(const char *name);\n \n /* object replacement */\n #define LOOKUP_REPLACE_OBJECT 1\n+#define LOOKUP_LITERALLY 2\n extern void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size, unsigned flag);\n static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)\n {\n@@ -1296,6 +1297,7 @@ struct object_info {\n \tunsigned long *sizep;\n \tunsigned long *disk_sizep;\n \tunsigned char *delta_base_sha1;\n+\tstruct strbuf *typename;\n \n \t/* Response */\n \tenum {\n-- \n2.3.1.307.gf3db8a5\n"},{"id":"257830","messageId":"1426569419-8266-1-git-send-email-karthik.188@gmail.com","threadId":"38828","inReplyTo":"5507B7EE.1070403@gmail.com","subject":"[PATCH v4 2/2] sha1_file: refactor sha1_file.c to support 'cat-file --literally'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-17T05:16:59Z","receivedAt":"2015-03-17T05:16:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Modify sha1_loose_object_info() to support 'cat-file --literally'\nby accepting flags and also make changes to copy the type to\nobject_info::typename.\n\nAdd parse_sha1_header_extended() which acts as a wrapper around\nparse_sha1_header() allowing for more information to be obtained\nbased on the given flags.\n\nAdd unpack_sha1_header_literally() to unpack sha1 headers of\nunknown/corrupt objects which have a unknown sha1 header size.\nThis was written by Junio C Hamano but tested by me.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n sha1_file.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 97 insertions(+), 24 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 69a60ec..e31e9e2 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1564,6 +1564,36 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n \treturn git_inflate(stream, 0);\n }\n \n+static int unpack_sha1_header_literally(git_zstream *stream, unsigned char *map,\n+\t\t\t\t\tunsigned long mapsize,\n+\t\t\t\t\tstruct strbuf *header)\n+{\n+\tunsigned char buffer[32], *cp;\n+\tunsigned long bufsiz = sizeof(buffer);\n+\tint status;\n+\n+\t/* Get the data stream */\n+\tmemset(stream, 0, sizeof(*stream));\n+\tstream->next_in = map;\n+\tstream->avail_in = mapsize;\n+\tstream->next_out = buffer;\n+\tstream->avail_out = bufsiz;\n+\n+\tgit_inflate_init(stream);\n+\n+\tdo {\n+\t\tstatus = git_inflate(stream, 0);\n+\t\tstrbuf_add(header, buffer, stream->next_out - buffer);\n+\t\tfor (cp = buffer; cp < stream->next_out; cp++)\n+\t\t\tif (!*cp)\n+\t\t\t\t/* Found the NUL at the end of the header */\n+\t\t\t\treturn 0;\n+\t\tstream->next_out = buffer;\n+\t\tstream->avail_out = bufsiz;\n+\t} while (status == Z_OK);\n+\treturn -1;\n+}\n+\n static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1)\n {\n \tint bytes = strlen(buffer) + 1;\n@@ -1609,32 +1639,24 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s\n \treturn NULL;\n }\n \n-/*\n- * We used to just use \"sscanf()\", but that's actually way\n- * too permissive for what we want to check. So do an anal\n- * object header parse by hand.\n- */\n-int parse_sha1_header(const char *hdr, unsigned long *sizep)\n+int parse_sha1_header_extended(const char *hdr, struct object_info *oi,\n+\t\t\t       int flags)\n {\n-\tchar type[10];\n-\tint i;\n+\tstruct strbuf typename = STRBUF_INIT;\n \tunsigned long size;\n+\tint type;\n \n \t/*\n \t * The type can be at most ten bytes (including the\n \t * terminating '\\0' that we add), and is followed by\n \t * a space.\n \t */\n-\ti = 0;\n \tfor (;;) {\n \t\tchar c = *hdr++;\n \t\tif (c == ' ')\n \t\t\tbreak;\n-\t\ttype[i++] = c;\n-\t\tif (i >= sizeof(type))\n-\t\t\treturn -1;\n+\t\tstrbuf_addch(&typename, c);\n \t}\n-\ttype[i] = 0;\n \n \t/*\n \t * The length must follow immediately, and be in canonical\n@@ -1652,12 +1674,45 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)\n \t\t\tsize = size * 10 + c;\n \t\t}\n \t}\n-\t*sizep = size;\n+\n+\ttype = type_from_string_gently(typename.buf, -1, 1);\n+\tif (oi->sizep)\n+\t\t*oi->sizep = size;\n+\tif (oi->typename)\n+\t\tstrbuf_addstr(oi->typename, typename.buf);\n+\tif (oi->typep)\n+\t\t*oi->typep = type;\n+\tstrbuf_release(&typename);\n+\n+\t/*\n+\t * Set type to 0 if its an unknown object and\n+\t * we're obtaining the type using '--literally'\n+\t * option.\n+\t */\n+\tif ((flags & LOOKUP_LITERALLY) && (type == -1))\n+\t\ttype = 0;\n+\telse if (type == -1)\n+\t\tdie(\"invalid object type\");\n \n \t/*\n \t * The length must be followed by a zero byte\n \t */\n-\treturn *hdr ? -1 : type_from_string(type);\n+\treturn *hdr ? -1 : type;\n+}\n+\n+/*\n+ * We used to just use \"sscanf()\", but that's actually way\n+ * too permissive for what we want to check. So do an anal\n+ * object header parse by hand. Calls the extended function.\n+ */\n+int parse_sha1_header(const char *hdr, unsigned long *sizep)\n+{\n+\tstruct object_info oi;\n+\n+\toi.sizep = sizep;\n+\toi.typename = NULL;\n+\toi.typep = NULL;\n+\treturn parse_sha1_header_extended(hdr, &oi, LOOKUP_REPLACE_OBJECT);\n }\n \n static void *unpack_sha1_file(void *map, unsigned long mapsize, enum object_type *type, unsigned long *size, const unsigned char *sha1)\n@@ -2524,13 +2579,15 @@ struct packed_git *find_sha1_pack(const unsigned char *sha1,\n }\n \n static int sha1_loose_object_info(const unsigned char *sha1,\n-\t\t\t\t  struct object_info *oi)\n+\t\t\t\t  struct object_info *oi,\n+\t\t\t\t  int flags)\n {\n-\tint status;\n+\tint status = 0;\n \tunsigned long mapsize, size;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[32];\n+\tstruct strbuf hdrbuf = STRBUF_INIT;\n \n \tif (oi->delta_base_sha1)\n \t\thashclr(oi->delta_base_sha1);\n@@ -2557,17 +2614,29 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \t\treturn -1;\n \tif (oi->disk_sizep)\n \t\t*oi->disk_sizep = mapsize;\n-\tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n-\t\tstatus = error(\"unable to unpack %s header\",\n-\t\t\t       sha1_to_hex(sha1));\n-\telse if ((status = parse_sha1_header(hdr, &size)) < 0)\n-\t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n-\telse if (oi->sizep)\n+\tif ((flags & LOOKUP_LITERALLY)) {\n+\t\tif (unpack_sha1_header_literally(&stream, map, mapsize, &hdrbuf) < 0)\n+\t\t\tstatus = error(\"unable to unpack %s header with --literally\",\n+\t\t\t\t       sha1_to_hex(sha1));\n+\t\telse if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)\n+\t\t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n+\t} else {\n+\t\tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n+\t\t\tstatus = error(\"unable to unpack %s header\",\n+\t\t\t\t       sha1_to_hex(sha1));\n+\t\telse if ((status = parse_sha1_header(hdr, &size)) < 0)\n+\t\t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n+\t}\n+\tif (oi->sizep)\n \t\t*oi->sizep = size;\n \tgit_inflate_end(&stream);\n \tmunmap(map, mapsize);\n \tif (oi->typep)\n \t\t*oi->typep = status;\n+\tif (oi->typename && !(oi->typename->len))\n+\t\tstrbuf_addstr(oi->typename, typename(status));\n+\tif (hdrbuf.buf)\n+\t\tstrbuf_release(&hdrbuf);\n \treturn 0;\n }\n \n@@ -2588,13 +2657,15 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \t\t\t*(oi->disk_sizep) = 0;\n \t\tif (oi->delta_base_sha1)\n \t\t\thashclr(oi->delta_base_sha1);\n+\t\tif (oi->typename)\n+\t\t\tstrbuf_addstr(oi->typename, typename(co->type));\n \t\toi->whence = OI_CACHED;\n \t\treturn 0;\n \t}\n \n \tif (!find_pack_entry(real, &e)) {\n \t\t/* Most likely it's a loose object. */\n-\t\tif (!sha1_loose_object_info(real, oi)) {\n+\t\tif (!sha1_loose_object_info(real, oi, flags)) {\n \t\t\toi->whence = OI_LOOSE;\n \t\t\treturn 0;\n \t\t}\n@@ -2618,6 +2689,8 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \t\toi->u.packed.is_delta = (rtype == OBJ_REF_DELTA ||\n \t\t\t\t\t rtype == OBJ_OFS_DELTA);\n \t}\n+\tif (oi->typename)\n+\t\tstrbuf_addstr(oi->typename, typename(rtype));\n \n \treturn 0;\n }\n-- \n2.3.1.307.gf3db8a5\n"},{"id":"257836","messageId":"CAPig+cSJVGy_L6mThFCU1Bw0ZrZEv=pZ8=W8gTZd=A5y2fcR7Q@mail.gmail.com","threadId":"38828","inReplyTo":"1426569400-8204-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v4 1/2] cat-file: teach cat-file a '--literally' option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-17T06:51:33Z","receivedAt":"2015-03-17T06:51:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 17, 2015 at 1:16 AM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index df99df4..1625246 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -323,8 +338,8 @@ static int batch_objects(struct batch_options *opt)\n>  }\n\nI don't presently have time for a proper read-through, however, this\npopped out while quickly running my eyes over the changes.\n\n>  static const char * const cat_file_usage[] = {\n> -       N_(\"git cat-file (-t | -s | -e | -p | <type> | --textconv) <object>\"),\n> -       N_(\"git cat-file (--batch | --batch-check) < <list-of-objects>\"),\n> +       N_(\"git cat-file (-t [--literally]|-s [--literally]|-e|-p|<type>|--textconv) <object>\"),\n> +       N_(\"git cat-file (--batch | --batch-check) <list-of-objects>\"),\n\nThe '<' in the second line before <list-of-objects> is intentional. It\nmeans that <list-of-objects> is provided via stdin. Therefore, it is\nincorrect to remove it.\n\n>         NULL\n>  };\n>\n"},{"id":"257853","messageId":"7BBE9C66-3654-4E6A-96FB-1EF16FC28E35@gmail.com","threadId":"38828","inReplyTo":"CAPig+cSJVGy_L6mThFCU1Bw0ZrZEv=pZ8=W8gTZd=A5y2fcR7Q@mail.gmail.com","subject":"Re: [PATCH v4 1/2] cat-file: teach cat-file a '--literally' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-17T13:59:40Z","receivedAt":"2015-03-17T13:59:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn March 17, 2015 12:21:33 PM GMT+05:30, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>On Tue, Mar 17, 2015 at 1:16 AM, Karthik Nayak <karthik.188@gmail.com>\n>wrote:\n>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n>> index df99df4..1625246 100644\n>> --- a/builtin/cat-file.c\n>> +++ b/builtin/cat-file.c\n>> @@ -323,8 +338,8 @@ static int batch_objects(struct batch_options\n>*opt)\n>>  }\n>\n>I don't presently have time for a proper read-through, however, this\n>popped out while quickly running my eyes over the changes.\n>\n>>  static const char * const cat_file_usage[] = {\n>> -       N_(\"git cat-file (-t | -s | -e | -p | <type> | --textconv)\n><object>\"),\n>> -       N_(\"git cat-file (--batch | --batch-check) <\n><list-of-objects>\"),\n>> +       N_(\"git cat-file (-t [--literally]|-s\n>[--literally]|-e|-p|<type>|--textconv) <object>\"),\n>> +       N_(\"git cat-file (--batch | --batch-check)\n><list-of-objects>\"),\n>\n>The '<' in the second line before <list-of-objects> is intentional. It\n>means that <list-of-objects> is provided via stdin. Therefore, it is\n>incorrect to remove it.\nI see. It seemed out of place. Makes sense, probably should have asked. \n\nThanks.\n>\n>>         NULL\n>>  };\n>>\n\n-- \nSent from my Android device with K-9 Mail. Please excuse my brevity.\n"},{"id":"257885","messageId":"xmqqbnjro05k.fsf@gitster.dls.corp.google.com","threadId":"38828","inReplyTo":"1426569419-8266-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v4 2/2] sha1_file: refactor sha1_file.c to support 'cat-file --literally'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T20:10:31Z","receivedAt":"2015-03-17T20:10:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Subject: Re: [PATCH v4 2/2] sha1_file: refactor sha1_file.c to support 'cat-file --literally'\n\n> Modify sha1_loose_object_info() to support 'cat-file --literally'\n> by accepting flags and also make changes to copy the type to\n> object_info::typename.\n\nIt would be more descriptive to mention the central point on the\ntitle regarding what it takes to \"support cat-file --literally\".\n\nFor example:\n\n  sha1_file.c: support reading from a loose object of a bogus type\n\n  Update sha1_loose_object_info() to optionally allow it to read\n  from a loose object file of unknown/bogus type; as the function\n  usually returns the type of the object it read in the form of enum\n  for known types, add an optional \"typename\" field to receive the\n  name of the type in textual form.\n\nBy the way, I think your 1/2 would not even compile unless you have\nthis change; the patches in these two patch series must be swapped,\nno?\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 69a60ec..e31e9e2 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1564,6 +1564,36 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n>  \treturn git_inflate(stream, 0);\n>  }\n>  \n> +static int unpack_sha1_header_literally(git_zstream *stream, unsigned char *map,\n> +\t\t\t\t\tunsigned long mapsize,\n> +\t\t\t\t\tstruct strbuf *header)\n> +{\n> +\tunsigned char buffer[32], *cp;\n> +\tunsigned long bufsiz = sizeof(buffer);\n> +\tint status;\n> +\n> +\t/* Get the data stream */\n> +\tmemset(stream, 0, sizeof(*stream));\n> +\tstream->next_in = map;\n> +\tstream->avail_in = mapsize;\n> +\tstream->next_out = buffer;\n> +\tstream->avail_out = bufsiz;\n> +\n> +\tgit_inflate_init(stream);\n> +\n> +\tdo {\n> +\t\tstatus = git_inflate(stream, 0);\n> +\t\tstrbuf_add(header, buffer, stream->next_out - buffer);\n> +\t\tfor (cp = buffer; cp < stream->next_out; cp++)\n> +\t\t\tif (!*cp)\n> +\t\t\t\t/* Found the NUL at the end of the header */\n> +\t\t\t\treturn 0;\n> +\t\tstream->next_out = buffer;\n> +\t\tstream->avail_out = bufsiz;\n> +\t} while (status == Z_OK);\n> +\treturn -1;\n> +}\n> +\n\nThere is nothing \"literal\" about this function.\n\nThe only difference between the original and this one is that this\nuses a strbuf, instead of a buffer of a fixed size allocated by the\ncaller, to return the early part of the inflated data.\n\nI wonder if it incurs too much overhead to the existing callers if\nyou reimplementated unpack_sha1_header() as a thin wrapper around\nthis function, something like\n\n\tint unpack_sha1_header(git_zstream *stream,\n\t\t\t\tunsigned char *map, unsigned long mapsize,\n                                void *buffer, unsigned long bufsiz)\n\t{\n                int status;\n\t\tstruct strbuf header = STRBUF_INIT;\n\n                status = unpack_sha1_header_to_strbuf(stream, map, mapsize, &header);\n\t\tif (bufsiz < header.len)\n                \tdie(\"object header too long\");\n\t\tmemcpy(buffer, header.buf, header.len);\n                return status;\n\t}\n    \t\t\t  \nNote that I am *not* suggesting to do this blindly.  If there is no\ndownside from performance point of view, however, the above would be\na way to avoid code duplication.\n\nAnother way to avoid code duplication may be to implement\nunpack_sha1_header_to_strbuf() in terms of unpack_sha1_header(),\nperhaps like this:\n\n\tstatic int unpack_sha1_header_to_strbuf(...)\n        {\n\t\tunsigned char buffer[32], *cp;\n                unsigned long bufsiz = sizeof(buffer);\n        \tint status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);\n\n\t\tif (status)\n                \treturn status;\n\t\tdo {\n\t\t\tstrbuf_add(header, buffer, stream->next_out - buffer);\n\t\t\tfor (cp = buffer; cp < stream->next_out; cp++)\n\t\t\t\tif (!*cp)\n\t\t\t\t\t/* Found the NUL at the end of the header */\n\t\t\t\t\treturn 0;\n\t\t\tstream->next_out = buffer;\n\t\t\tstream->avail_out = bufsiz;\n\t\t} while (status == Z_OK);\n\t\treturn -1;\n\t}\n\nwhich may be more in line with how we read data from loose objects.\n\n> +int parse_sha1_header_extended(const char *hdr, struct object_info *oi,\n> +\t\t\t       int flags)\n\nUnless we are taking advantage of the fact that MSB is special in\nsigned integral types, I would prefer to see us use an unsigned type\nto store these bits in a variable of an integral type.  That would\nlet the readers assume that they have fewer tricky things possibly\ngoing on in the code (also see the footnote to $gmane/263751).\n\n> @@ -1652,12 +1674,45 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)\n>  \t\t\tsize = size * 10 + c;\n>  \t\t}\n>  \t}\n> -\t*sizep = size;\n> +\n> +\ttype = type_from_string_gently(typename.buf, -1, 1);\n\nDoesn't the code know how long the typename is at this point?\n\n> +\tif (oi->sizep)\n> +\t\t*oi->sizep = size;\n> +\tif (oi->typename)\n> +\t\tstrbuf_addstr(oi->typename, typename.buf);\n\nLikewise.  Perhaps strbuf_addbuf()?\n\n> +\tif (oi->typep)\n> +\t\t*oi->typep = type;\n\nDo you want to store -1 to *oi->typep when the caller asked to do\nthe \"literally\" thing?  Shouldn't it match what is returned from\nthis function?\n\n> +\treturn *hdr ? -1 : type;\n> +}\n> +\n> +/*\n> + * We used to just use \"sscanf()\", but that's actually way\n> + * too permissive for what we want to check. So do an anal\n> + * object header parse by hand. Calls the extended function.\n> + */\n\nThe comment \"let's do better than sscanf() by parsing ourselves\"\napplies to the implementation of _extended() function, not this one,\nno?  It is clear to everybody that it \"Calls the extended function\",\nso why mention it?\n\n> +int parse_sha1_header(const char *hdr, unsigned long *sizep)\n> +{\n> +\tstruct object_info oi;\n> +\n> +\toi.sizep = sizep;\n> +\toi.typename = NULL;\n> +\toi.typep = NULL;\n> +\treturn parse_sha1_header_extended(hdr, &oi, LOOKUP_REPLACE_OBJECT);\n>  }\n\n> @@ -2524,13 +2579,15 @@ struct packed_git *find_sha1_pack(const unsigned char *sha1,\n>  }\n>  \n>  static int sha1_loose_object_info(const unsigned char *sha1,\n> -\t\t\t\t  struct object_info *oi)\n> +\t\t\t\t  struct object_info *oi,\n> +\t\t\t\t  int flags)\n>  {\n> -\tint status;\n> +\tint status = 0;\n>  \tunsigned long mapsize, size;\n>  \tvoid *map;\n>  \tgit_zstream stream;\n>  \tchar hdr[32];\n> +\tstruct strbuf hdrbuf = STRBUF_INIT;\n>  \n>  \tif (oi->delta_base_sha1)\n>  \t\thashclr(oi->delta_base_sha1);\n> @@ -2557,17 +2614,29 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n>  \t\treturn -1;\n>  \tif (oi->disk_sizep)\n>  \t\t*oi->disk_sizep = mapsize;\n> -\tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n> -\t\tstatus = error(\"unable to unpack %s header\",\n> -\t\t\t       sha1_to_hex(sha1));\n> -\telse if ((status = parse_sha1_header(hdr, &size)) < 0)\n> -\t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n> -\telse if (oi->sizep)\n> +\tif ((flags & LOOKUP_LITERALLY)) {\n> +\t\tif (unpack_sha1_header_literally(&stream, map, mapsize, &hdrbuf) < 0)\n> +\t\t\tstatus = error(\"unable to unpack %s header with --literally\",\n> +\t\t\t\t       sha1_to_hex(sha1));\n> +\t\telse if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)\n> +\t\t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n> +\t} else {\n> +\t\tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n> +\t\t\tstatus = error(\"unable to unpack %s header\",\n> +\t\t\t\t       sha1_to_hex(sha1));\n> +\t\telse if ((status = parse_sha1_header(hdr, &size)) < 0)\n> +\t\t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n> +\t}\n> +\tif (oi->sizep)\n>  \t\t*oi->sizep = size;\n\nHave you considered calling parse_sha1_header_extended() for both\nliterally and normal cases?  Then you wouldn't have to do any of the\n\"if (oi->blah) *oi->blah = value\", no?\n\n>  \tgit_inflate_end(&stream);\n>  \tmunmap(map, mapsize);\n>  \tif (oi->typep)\n>  \t\t*oi->typep = status;\n\nLikewise.\n\n> +\tif (oi->typename && !(oi->typename->len))\n> +\t\tstrbuf_addstr(oi->typename, typename(status));\n\nLikewise.\n"},{"id":"257975","messageId":"5509D36C.6000908@gmail.com","threadId":"38828","inReplyTo":"xmqqbnjro05k.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 2/2] sha1_file: refactor sha1_file.c to support 'cat-file --literally'","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-03-18T19:35:08Z","receivedAt":"2015-03-18T19:35:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 03/18/2015 01:40 AM, Junio C Hamano wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> Subject: Re: [PATCH v4 2/2] sha1_file: refactor sha1_file.c to support 'cat-file --literally'\n>\n>> Modify sha1_loose_object_info() to support 'cat-file --literally'\n>> by accepting flags and also make changes to copy the type to\n>> object_info::typename.\n>\n> It would be more descriptive to mention the central point on the\n> title regarding what it takes to \"support cat-file --literally\".\n>\n> For example:\n>\n>    sha1_file.c: support reading from a loose object of a bogus type\n>\n>    Update sha1_loose_object_info() to optionally allow it to read\n>    from a loose object file of unknown/bogus type; as the function\n>    usually returns the type of the object it read in the form of enum\n>    for known types, add an optional \"typename\" field to receive the\n>    name of the type in textual form.\n>\n> By the way, I think your 1/2 would not even compile unless you have\n> this change; the patches in these two patch series must be swapped,\n> no?\n>\nNoted. Yes it wouldn't. I thought both would be applied together and \ndidn't give it much thought, but yeah, I should pay more attention to that.\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 69a60ec..e31e9e2 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -1564,6 +1564,36 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma\n>>   \treturn git_inflate(stream, 0);\n>>   }\n>>\n>> +static int unpack_sha1_header_literally(git_zstream *stream, unsigned char *map,\n>> +\t\t\t\t\tunsigned long mapsize,\n>> +\t\t\t\t\tstruct strbuf *header)\n>> +{\n>> +\tunsigned char buffer[32], *cp;\n>> +\tunsigned long bufsiz = sizeof(buffer);\n>> +\tint status;\n>> +\n>> +\t/* Get the data stream */\n>> +\tmemset(stream, 0, sizeof(*stream));\n>> +\tstream->next_in = map;\n>> +\tstream->avail_in = mapsize;\n>> +\tstream->next_out = buffer;\n>> +\tstream->avail_out = bufsiz;\n>> +\n>> +\tgit_inflate_init(stream);\n>> +\n>> +\tdo {\n>> +\t\tstatus = git_inflate(stream, 0);\n>> +\t\tstrbuf_add(header, buffer, stream->next_out - buffer);\n>> +\t\tfor (cp = buffer; cp < stream->next_out; cp++)\n>> +\t\t\tif (!*cp)\n>> +\t\t\t\t/* Found the NUL at the end of the header */\n>> +\t\t\t\treturn 0;\n>> +\t\tstream->next_out = buffer;\n>> +\t\tstream->avail_out = bufsiz;\n>> +\t} while (status == Z_OK);\n>> +\treturn -1;\n>> +}\n>> +\n>\n> There is nothing \"literal\" about this function.\n>\n> The only difference between the original and this one is that this\n> uses a strbuf, instead of a buffer of a fixed size allocated by the\n> caller, to return the early part of the inflated data.\n>\n> I wonder if it incurs too much overhead to the existing callers if\n> you reimplementated unpack_sha1_header() as a thin wrapper around\n> this function, something like\n>\n> \tint unpack_sha1_header(git_zstream *stream,\n> \t\t\t\tunsigned char *map, unsigned long mapsize,\n>                                  void *buffer, unsigned long bufsiz)\n> \t{\n>                  int status;\n> \t\tstruct strbuf header = STRBUF_INIT;\n>\n>                  status = unpack_sha1_header_to_strbuf(stream, map, mapsize, &header);\n> \t\tif (bufsiz < header.len)\n>                  \tdie(\"object header too long\");\n> \t\tmemcpy(buffer, header.buf, header.len);\n>                  return status;\n> \t}\n>      \t\t\t\n> Note that I am *not* suggesting to do this blindly.  If there is no\n> downside from performance point of view, however, the above would be\n> a way to avoid code duplication.\n>\n> Another way to avoid code duplication may be to implement\n> unpack_sha1_header_to_strbuf() in terms of unpack_sha1_header(),\n> perhaps like this:\n>\n> \tstatic int unpack_sha1_header_to_strbuf(...)\n>          {\n> \t\tunsigned char buffer[32], *cp;\n>                  unsigned long bufsiz = sizeof(buffer);\n>          \tint status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);\n>\n> \t\tif (status)\n>                  \treturn status;\n> \t\tdo {\n> \t\t\tstrbuf_add(header, buffer, stream->next_out - buffer);\n> \t\t\tfor (cp = buffer; cp < stream->next_out; cp++)\n> \t\t\t\tif (!*cp)\n> \t\t\t\t\t/* Found the NUL at the end of the header */\n> \t\t\t\t\treturn 0;\n> \t\t\tstream->next_out = buffer;\n> \t\t\tstream->avail_out = bufsiz;\n> \t\t} while (status == Z_OK);\n> \t\treturn -1;\n> \t}\n>\n> which may be more in line with how we read data from loose objects.\n\nAgreed there is code duplication with unpack_sha1_header() and \nunpack_sha1_header_extended(). But I thought there would be a \nperformance trade off, Any way I could test this?\nAlso the second suggestion you have given seems to be more practical, As \nthere is no performance overhead, if called by existing code.\n>\n>> +int parse_sha1_header_extended(const char *hdr, struct object_info *oi,\n>> +\t\t\t       int flags)\n>\n> Unless we are taking advantage of the fact that MSB is special in\n> signed integral types, I would prefer to see us use an unsigned type\n> to store these bits in a variable of an integral type.  That would\n> let the readers assume that they have fewer tricky things possibly\n> going on in the code (also see the footnote to $gmane/263751).\n\nThanks for the link. Will change to unsigned.\n>\n>> @@ -1652,12 +1674,45 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)\n>>   \t\t\tsize = size * 10 + c;\n>>   \t\t}\n>>   \t}\n>> -\t*sizep = size;\n>> +\n>> +\ttype = type_from_string_gently(typename.buf, -1, 1);\n>\n> Doesn't the code know how long the typename is at this point?\nYes, will change.\n>\n>> +\tif (oi->sizep)\n>> +\t\t*oi->sizep = size;\n>> +\tif (oi->typename)\n>> +\t\tstrbuf_addstr(oi->typename, typename.buf);\n>\n> Likewise.  Perhaps strbuf_addbuf()?\nThat also calls strbuf_add(). But does improve readability\n>\n>> +\tif (oi->typep)\n>> +\t\t*oi->typep = type;\n>\n> Do you want to store -1 to *oi->typep when the caller asked to do\n> the \"literally\" thing?  Shouldn't it match what is returned from\n> this function?\nYes it should. Will make changes.\n>\n>> +\treturn *hdr ? -1 : type;\n>> +}\n>> +\n>> +/*\n>> + * We used to just use \"sscanf()\", but that's actually way\n>> + * too permissive for what we want to check. So do an anal\n>> + * object header parse by hand. Calls the extended function.\n>> + */\n>\n> The comment \"let's do better than sscanf() by parsing ourselves\"\n> applies to the implementation of _extended() function, not this one,\n> no?  It is clear to everybody that it \"Calls the extended function\",\n> so why mention it?\n>\n>> +int parse_sha1_header(const char *hdr, unsigned long *sizep)\n>> +{\n>> +\tstruct object_info oi;\n>> +\n>> +\toi.sizep = sizep;\n>> +\toi.typename = NULL;\n>> +\toi.typep = NULL;\n>> +\treturn parse_sha1_header_extended(hdr, &oi, LOOKUP_REPLACE_OBJECT);\n>>   }\n>\n>> @@ -2524,13 +2579,15 @@ struct packed_git *find_sha1_pack(const unsigned char *sha1,\n>>   }\n>>\n>>   static int sha1_loose_object_info(const unsigned char *sha1,\n>> -\t\t\t\t  struct object_info *oi)\n>> +\t\t\t\t  struct object_info *oi,\n>> +\t\t\t\t  int flags)\n>>   {\n>> -\tint status;\n>> +\tint status = 0;\n>>   \tunsigned long mapsize, size;\n>>   \tvoid *map;\n>>   \tgit_zstream stream;\n>>   \tchar hdr[32];\n>> +\tstruct strbuf hdrbuf = STRBUF_INIT;\n>>\n>>   \tif (oi->delta_base_sha1)\n>>   \t\thashclr(oi->delta_base_sha1);\n>> @@ -2557,17 +2614,29 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n>>   \t\treturn -1;\n>>   \tif (oi->disk_sizep)\n>>   \t\t*oi->disk_sizep = mapsize;\n>> -\tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n>> -\t\tstatus = error(\"unable to unpack %s header\",\n>> -\t\t\t       sha1_to_hex(sha1));\n>> -\telse if ((status = parse_sha1_header(hdr, &size)) < 0)\n>> -\t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n>> -\telse if (oi->sizep)\n>> +\tif ((flags & LOOKUP_LITERALLY)) {\n>> +\t\tif (unpack_sha1_header_literally(&stream, map, mapsize, &hdrbuf) < 0)\n>> +\t\t\tstatus = error(\"unable to unpack %s header with --literally\",\n>> +\t\t\t\t       sha1_to_hex(sha1));\n>> +\t\telse if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)\n>> +\t\t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n>> +\t} else {\n>> +\t\tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n>> +\t\t\tstatus = error(\"unable to unpack %s header\",\n>> +\t\t\t\t       sha1_to_hex(sha1));\n>> +\t\telse if ((status = parse_sha1_header(hdr, &size)) < 0)\n>> +\t\t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n>> +\t}\n>> +\tif (oi->sizep)\n>>   \t\t*oi->sizep = size;\n>\n> Have you considered calling parse_sha1_header_extended() for both\n> literally and normal cases?  Then you wouldn't have to do any of the\n> \"if (oi->blah) *oi->blah = value\", no?\n>\n>>   \tgit_inflate_end(&stream);\n>>   \tmunmap(map, mapsize);\n>>   \tif (oi->typep)\n>>   \t\t*oi->typep = status;\n>\n> Likewise.\n>\n>> +\tif (oi->typename && !(oi->typename->len))\n>> +\t\tstrbuf_addstr(oi->typename, typename(status));\n>\n> Likewise.\n>\n\nNo. I have not considered that. will look into this.\nThanks for the suggestions.\n\nRegards\n-Karthik\n"}]}