{"thread":{"id":"38650","subject":"[PATCH 0/2] cat-file --literally","startedAt":"2015-02-25T11:06:49Z","lastAt":"2015-02-26T15:07:38Z","messageCount":11,"participants":["karthik nayak","Karthik Nayak","David Turner","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"256630","messageId":"54EDACC9.5080204@gmail.com","threadId":"38650","inReplyTo":null,"subject":"[PATCH 0/2] cat-file --literally","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-02-25T11:06:49Z","receivedAt":"2015-02-25T11:06:49Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"These changes introduce a \"--literally\" option to \"cat-file\" to\nallow us to read the type of any loose object, mostly the ones\ncreated using the \"--literally\" option of \"hash-object\".\n\nFor now it works with the \"-t\" option as :\n\n     git cat-object -t --literally <object_sha>\n\nCan be made to work with \"-e\" and \"-s\" option in the future.\n\nThe first patch adds \"sha1_object_type_literally()\" which is\ncalled by \"cat-object\" to print the type.\nThe second patch adds the \"--literally\" option to \"cat-file\".\n\nThese patches are in response for the patches made here :\nhttp://thread.gmane.org/gmane.comp.version-control.git/243560/focus=243628\n"},{"id":"256631","messageId":"1424862460-13514-1-git-send-email-karthik.188@gmail.com","threadId":"38650","inReplyTo":"54EDACC9.5080204@gmail.com","subject":"[PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-02-25T11:07:40Z","receivedAt":"2015-02-25T11:07:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"sha1_object_type_literally takes a sha value and\ngives the type of the given loose object, used by\ngit cat-file -t --literally.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n cache.h     |  1 +\n sha1_file.c | 27 +++++++++++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 4d02efc..48086b9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -872,6 +872,7 @@ extern int git_open_noatime(const char *name);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\n extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\n+extern int sha1_object_type_literally(const unsigned char *sha1, char *type);\n \n /* global flag to enable extra checks when accessing packed objects */\n extern int do_check_packed_object_crc;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 69a60ec..34b4810 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2635,6 +2635,33 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n \treturn type;\n }\n \n+int sha1_object_type_literally(const unsigned char *sha1, char *type)\n+{\n+\tint status = 0;\n+\tunsigned long mapsize;\n+\tvoid *map;\n+\tgit_zstream stream;\n+\tchar hdr[32];\n+\tint i;\n+\n+\tmap = map_sha1_file(sha1, &mapsize);\n+\tif (!map)\n+\t\treturn -1;\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+\n+\tfor (i = 0; i < 32; i++) {\n+\t\tif (hdr[i] == ' ') {\n+\t\t\ttype[i] = '\\0';\n+\t\t\tbreak;\n+\t\t}\n+\t\ttype[i] = hdr[i];\n+\t}\n+\n+\treturn status;\n+}\n+\n static void *read_packed_sha1(const unsigned char *sha1,\n \t\t\t      enum object_type *type, unsigned long *size)\n {\n-- \n2.3.1.129.g11acff1.dirty\n"},{"id":"256632","messageId":"1424862485-13576-1-git-send-email-karthik.188@gmail.com","threadId":"38650","inReplyTo":"54EDACC9.5080204@gmail.com","subject":"[PATCH 2/2] cat-file: add --literally option.","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-02-25T11:08:05Z","receivedAt":"2015-02-25T11:08:05Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Add --literally option which when used with -t option\ngives the type of the object given, irrelevant of the type\nand its contents.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/cat-file.c | 35 +++++++++++++++++++++++++++++++++--\n 1 file changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex df99df4..1db94fe 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -107,6 +107,29 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \treturn 0;\n }\n \n+static int cat_one_file_literally(int opt, const char *exp_type, const char *obj_name)\n+{\n+\tunsigned char sha1[20];\n+\tunsigned char type[32];\n+\tstruct object_context obj_context;\n+\tstruct object_info oi = {NULL};\n+\tint retval = 0;\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+\n+\tretval = has_sha1_file(sha1);\n+\tif (!retval)\n+\t\treturn retval;\n+\n+\tif(sha1_object_type_literally(sha1, type))\n+\t\tdie(\"git cat-file -t --literally %s: invalid object\", obj_name);\n+\n+\tprintf(\"%s\\n\", type);\n+\n+\treturn retval;\n+}\n+\n struct expand_data {\n \tunsigned char sha1[20];\n \tenum object_type type;\n@@ -324,7 +347,7 @@ static int batch_objects(struct batch_options *opt)\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|-s|-e|-p|<type>|--textconv|-t --literally) <object>\"),\n \tNULL\n };\n \n@@ -359,6 +382,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 +393,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_(\"show the type of the gicen loose object, use for debugging\")),\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 +406,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 != 3 && argc != 2 && (!literally && 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 +431,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+\tif (literally && opt == 't')\n+\t\treturn cat_one_file_literally(opt, exp_type, obj_name);\n+\telse if (literally)\n+\t\tusage_with_options(cat_file_usage, options);\n+\n \treturn cat_one_file(opt, exp_type, obj_name);\n }\n-- \n2.3.1.129.g11acff1.dirty\n"},{"id":"256644","messageId":"1424888526.2968.3.camel@leckie","threadId":"38650","inReplyTo":"1424862460-13514-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-02-25T18:22:06Z","receivedAt":"2015-02-25T18:22:06Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-02-25 at 16:37 +0530, Karthik Nayak wrote:\n> +\tunsigned long mapsize;\n> ...\n> +\tmap = map_sha1_file(sha1, &mapsize);\n\nI know this is a pre-existing issue, but I'm not sure \"unsigned long\" is\nthe right type here.  Shouldn't it be a size_t?  \n\n> +\tif (!map)\n> +\t\treturn -1;\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> +\n> +\tfor (i = 0; i < 32; i++) {\n\nThis number should probably be a constant.\n\n> +\t\tif (hdr[i] == ' ') {\n> +\t\t\ttype[i] = '\\0';\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\ttype[i] = hdr[i];\n> +\t}\n\ntype might end up without a trailing \\0 here in the case where hdr has\nno space in it.  Is this possible?\n"},{"id":"256655","messageId":"54EE29AD.4060802@gmail.com","threadId":"38650","inReplyTo":"1424888526.2968.3.camel@leckie","subject":"Re: [PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-02-25T19:59:41Z","receivedAt":"2015-02-25T19:59:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 02/25/2015 11:52 PM, David Turner wrote:\n> On Wed, 2015-02-25 at 16:37 +0530, Karthik Nayak wrote:\n>> +\tunsigned long mapsize;\n>> ...\n>> +\tmap = map_sha1_file(sha1, &mapsize);\n>\n> I know this is a pre-existing issue, but I'm not sure \"unsigned long\" is\n> the right type here.  Shouldn't it be a size_t?\nI got the type from the function definition of map_sha1_file(), which is \n\"void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\"\n>\n>> +\tif (!map)\n>> +\t\treturn -1;\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>> +\n>> +\tfor (i = 0; i < 32; i++) {\n>\n> This number should probably be a constant.\nDo you want me to define it as a preprocessor directive?\n>\n>> +\t\tif (hdr[i] == ' ') {\n>> +\t\t\ttype[i] = '\\0';\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t\ttype[i] = hdr[i];\n>> +\t}\n>\n> type might end up without a trailing \\0 here in the case where hdr has\n> no space in it.  Is this possible?\nWhat's possible is when the object type name is greater than 32bytes \n\"hdr\" will not be able to hold the whole type, its a tradeoff, I guess I \nshould put a null terminator at the end of \"hdr\". What do you suggest?\n>\n>\n"},{"id":"256657","messageId":"1424895352.2968.8.camel@leckie","threadId":"38650","inReplyTo":"54EE29AD.4060802@gmail.com","subject":"Re: [PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-02-25T20:15:52Z","receivedAt":"2015-02-25T20:15:52Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Thu, 2015-02-26 at 01:29 +0530, karthik nayak wrote:\n> \n> On 02/25/2015 11:52 PM, David Turner wrote:\n> > On Wed, 2015-02-25 at 16:37 +0530, Karthik Nayak wrote:\n> >> +\tunsigned long mapsize;\n> >> ...\n> >> +\tmap = map_sha1_file(sha1, &mapsize);\n> >\n> > I know this is a pre-existing issue, but I'm not sure \"unsigned long\" is\n> > the right type here.  Shouldn't it be a size_t?\n> I got the type from the function definition of map_sha1_file(), which is \n> \"void *map_sha1_file(const unsigned char *sha1, unsigned long *size)\"\n\nYep. That's why I describe it as a pre-existing issue - it's not your\nfault that the type is funny.  But it might be worth cleaning up while\nyou're in here.  Or it might not -- maybe I'm totally off base and the\ntype is correct.\n\n> >\n> >> +\tif (!map)\n> >> +\t\treturn -1;\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> >> +\n> >> +\tfor (i = 0; i < 32; i++) {\n> >\n> > This number should probably be a constant.\n> Do you want me to define it as a preprocessor directive?\n\nSounds good to me.\n\n> >> +\t\tif (hdr[i] == ' ') {\n> >> +\t\t\ttype[i] = '\\0';\n> >> +\t\t\tbreak;\n> >> +\t\t}\n> >> +\t\ttype[i] = hdr[i];\n> >> +\t}\n> >\n> > type might end up without a trailing \\0 here in the case where hdr has\n> > no space in it.  Is this possible?\n> What's possible is when the object type name is greater than 32bytes \n> \"hdr\" will not be able to hold the whole type, its a tradeoff, I guess I \n> should put a null terminator at the end of \"hdr\". What do you suggest?\n\nIs it an error for the object type name to be > 32 bytes?  If there is\nno max length, then maybe you should use the strbuf API for this.\nSilently truncating is probably the wrong thing to do, and leaving it\nunterminated is dangerous unless callers know how to handle that case.\nIf it is an error, then you should return an error code.\n"},{"id":"256661","messageId":"xmqq61apoewr.fsf@gitster.dls.corp.google.com","threadId":"38650","inReplyTo":"1424862460-13514-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-25T21:32:52Z","receivedAt":"2015-02-25T21:32:52Z","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> +int sha1_object_type_literally(const unsigned char *sha1, char *type)\n> +{\n> +\tint status = 0;\n> +\tunsigned long mapsize;\n> +\tvoid *map;\n> +\tgit_zstream stream;\n> +\tchar hdr[32];\n> +\tint i;\n> +\n> +\tmap = map_sha1_file(sha1, &mapsize);\n> +\tif (!map)\n> +\t\treturn -1;\n\nI am not sure if it is a good idea to introduce a function with the\nabove signature and the semantics in the first place.  It is OK to\nassume that objects with funny typenames only appear as loose\nobjects, but it is not OK for \"cat-file --literally\" to fail to work\non objects that are of kosher types.\n\nWhat should \"git cat-file --literally -t HEAD~2\" do?  I think it\nshould say \"commit\" as long as my current history is at least 3\ncommits deep, even when my repository is fully packed.  But I\nsuspect the above code does not do that.  \n\nLooking at how we collect information on normal objects, it may make\nmore sense to model this after sha1_loose_object_info(), with a\ntweak to struct object_info datatype, and integrate it into\nsha1_object_info_extended() may make more sense, perhaps along the\nlines of the attached patch.\n\nThe new helper would mimick what sha1_loose_object_info() is doing,\nin that it may be used to learn on-disk size, object size, typename\nstring (returned in oi->typename strbuf that is optional).  There is\nno sensible value to stuff in oi->typep if the incoming object name\nrefers to the experimental invalid object, so perhaps you will store\nOBJ_NONE or something there and the \"cat-file --literally\" would use\nthe oi->typename to learn the name of the \"type\".\n\n\n cache.h     | 1 +\n sha1_file.c | 9 +++++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 4d02efc..72b7cfa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1296,6 +1296,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 {\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 69a60ec..2c0e83a 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2568,6 +2568,8 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \tmunmap(map, mapsize);\n \tif (oi->typep)\n \t\t*oi->typep = status;\n+\tif (oi->typename && 0 < status && typename(status))\n+\t\tstrbuf_add(oi->typename, typename(status));\n \treturn 0;\n }\n \n@@ -2593,6 +2595,13 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \t}\n \n \tif (!find_pack_entry(real, &e)) {\n+\t\t/* Have we been asked to check possibly invalid ones? */\n+\t\tif ((flags & LOOKUP_LITERALLY) &&\n+\t\t    !sha1_object_info_literally(real, oi)) {\n+\t\t\toi->whence = OI_LOOSE;\n+\t\t\treturn 0;\n+\t\t}\n+\n \t\t/* Most likely it's a loose object. */\n \t\tif (!sha1_loose_object_info(real, oi)) {\n \t\t\toi->whence = OI_LOOSE;\n"},{"id":"256664","messageId":"CAPig+cQDLU4CBQtE8vAKLyz4Xv=2DsDDMz787DVjrFwW2tiKXg@mail.gmail.com","threadId":"38650","inReplyTo":"1424862460-13514-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-02-25T21:55:35Z","receivedAt":"2015-02-25T21:55:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"I had written a longer review but was interrupted for a several hours,\nand upon returning found that David and Junio covered many of the same\nissues or overrode comments I was making, so the below review is pared\ndown quite a bit. Junio's proposed approach negates all of the below\nreview comments, but they may still be meaningful if kept in mind for\nfuture submissions.\n\nOn Wed, Feb 25, 2015 at 6:07 AM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> sha1_file: Add sha1_object_type_literally and export it.\n\nStyle: downcase \"Add\"; drop terminating period.\n\n> sha1_object_type_literally takes a sha value and\n> gives the type of the given loose object, used by\n> git cat-file -t --literally.\n>\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2635,6 +2635,33 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n>         return type;\n>  }\n>\n> +int sha1_object_type_literally(const unsigned char *sha1, char *type)\n\nThis functionality is very specific to the --literally option you're\nadding to cat-file, so it would make more sense to make it private to\nbuiltin/cat-file.c rather than publishing it globally.\n\nAlso, this is an unsafe contract. The caller does not know how many\nbytes to allocate for 'type', and this new function may write past the\nend of the buffer. It is more common to also pass in the size of the\n'type' buffer and ensure that you do not write beyond that. Or, if\nthis is intended for wider consumption, pass in a strbuf instead.\n\n> +{\n> +       int status = 0;\n> +       unsigned long mapsize;\n> +       void *map;\n> +       git_zstream stream;\n> +       char hdr[32];\n> +       int i;\n> +\n> +       map = map_sha1_file(sha1, &mapsize);\n> +       if (!map)\n> +               return -1;\n> +       if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n> +               status = error(\"unable to unpack %s header\",\n> +                              sha1_to_hex(sha1));\n\nSince 'hdr' unpacking failed, shouldn't you be returning at this point\nrather than continuing to the 'hdr' processing loop?\n\n> +       for (i = 0; i < 32; i++) {\n> +               if (hdr[i] == ' ') {\n> +                       type[i] = '\\0';\n> +                       break;\n> +               }\n> +               type[i] = hdr[i];\n> +       }\n\nDavid already mentioned that this loop is suspect. Perhaps take a look\nat, sha1_file.c:parse_sha1_header() for an example of cleaner logic.\n\n> +\n> +       return status;\n> +}\n> +\n>  static void *read_packed_sha1(const unsigned char *sha1,\n>                               enum object_type *type, unsigned long *size)\n>  {\n> --\n> 2.3.1.129.g11acff1.dirty\n"},{"id":"256665","messageId":"CAPig+cQeHxH=Bc2EyQXOfX=m33XFfUViS2VDi4K+yQoweBo1Nw@mail.gmail.com","threadId":"38650","inReplyTo":"1424862485-13576-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH 2/2] cat-file: add --literally option.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-02-25T22:14:15Z","receivedAt":"2015-02-25T22:14:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 25, 2015 at 6:08 AM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> cat-file: add --literally option.\n\nStyle: drop terminating period.\n\n> Add --literally option which when used with -t option\n> gives the type of the object given, irrelevant of the type\n> and its contents.\n\nConfusing: \"gives the type [...] irrelevant of the type\"?\n\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index df99df4..1db94fe 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -107,6 +107,29 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n>         return 0;\n>  }\n>\n> +static int cat_one_file_literally(int opt, const char *exp_type, const char *obj_name)\n\nUnused arguments: 'opt' and 'exp_type'\n\nThis function duplicates enough functionality of cat_one_file() that\nit almost seems that it could be folded into the 't' case in that\nfunction, but for the sake of review, let's assume that you'll keep it\nseparate.\n\n> +{\n> +       unsigned char sha1[20];\n> +       unsigned char type[32];\n> +       struct object_context obj_context;\n> +       struct object_info oi = {NULL};\n\nUnused variable: 'oi'\n\n> +       int retval = 0;\n> +\n> +       if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))\n\nYou never take advantage of the extra context in 'obj_context', so\nperhaps get_sha1() would be preferable?\n\n> +               die(\"Not a valid object name %s\", obj_name);\n> +\n> +       retval = has_sha1_file(sha1);\n> +       if (!retval)\n> +               return retval;\n\nShouldn't this case be emitting some sort of diagnostic rather than\nexiting silently?\n\n> +       if(sha1_object_type_literally(sha1, type))\n\nStyle: space after 'if'\n\n> +               die(\"git cat-file -t --literally %s: invalid object\", obj_name);\n\nInconsistent error message style with the die() just above this one.\n(cat_one_file() has its own inconsistencies, but new code need not\nfollow suit.)\n\n> +\n> +       printf(\"%s\\n\", type);\n> +\n> +       return retval;\n> +}\n> +\n>  struct expand_data {\n>         unsigned char sha1[20];\n>         enum object_type type;\n> @@ -324,7 +347,7 @@ static int batch_objects(struct batch_options *opt)\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|-s|-e|-p|<type>|--textconv|-t --literally) <object>\"),\n\nThis is strange. According to this diff, you've removed the --batch\nand --batch-check options; and you're showing the -t, -s, -e, etc.\noptions twice.\n\n>         NULL\n>  };\n>\n> @@ -359,6 +382,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>         int opt = 0;\n>         const char *exp_type = NULL, *obj_name = NULL;\n>         struct batch_options batch = {0};\n> +       int literally = 0;\n\nMental note: 'literally' is initialized to false (0).\n\n>         const struct option options[] = {\n>                 OPT_GROUP(N_(\"<type> can be one of: blob, tree, commit, tag\")),\n> @@ -369,6 +393,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>                 OPT_SET_INT('p', NULL, &opt, N_(\"pretty-print object's content\"), 'p'),\n>                 OPT_SET_INT(0, \"textconv\", &opt,\n>                             N_(\"for blob objects, run textconv on object's content\"), 'c'),\n> +               OPT_BOOL( 0, \"literally\", &literally,\n> +                         N_(\"show the type of the gicen loose object, use for debugging\")),\n\ns/gicen/given/\n\n>                 { OPTION_CALLBACK, 0, \"batch\", &batch, \"format\",\n>                         N_(\"show info and content of objects fed from the standard input\"),\n>                         PARSE_OPT_OPTARG, batch_option_callback },\n> @@ -380,7 +406,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>\n>         git_config(git_cat_file_config, NULL);\n>\n> -       if (argc != 3 && argc != 2)\n> +       if (argc != 3 && argc != 2 && (!literally && argc != 4))\n\nThis can't be correct. At this point, 'literally' is unconditionally\nfalse (0). It's value won't (potentially) change until the options are\nprocessed, which doesn't happen until parse_options() is invoked\n_after_ this conditional.\n\n>                 usage_with_options(cat_file_usage, options);\n>\n>         argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);\n> @@ -405,5 +431,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>         if (batch.enabled)\n>                 return batch_objects(&batch);\n>\n> +       if (literally && opt == 't')\n> +               return cat_one_file_literally(opt, exp_type, obj_name);\n> +       else if (literally)\n> +               usage_with_options(cat_file_usage, options);\n> +\n>         return cat_one_file(opt, exp_type, obj_name);\n>  }\n> --\n> 2.3.1.129.g11acff1.dirty\n"},{"id":"256666","messageId":"xmqqtwy9mx16.fsf@gitster.dls.corp.google.com","threadId":"38650","inReplyTo":"xmqq61apoewr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-25T22:44:21Z","receivedAt":"2015-02-25T22:44:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Looking at how we collect information on normal objects, it may make\n> more sense to model this after sha1_loose_object_info(), with a\n> tweak to struct object_info datatype, and integrate it into\n> sha1_object_info_extended() may make more sense, perhaps along the\n> lines of the attached patch.\n>\n> The new helper would mimick what sha1_loose_object_info() is doing,\n> in that it may be used to learn on-disk size, object size, typename\n> string (returned in oi->typename strbuf that is optional).  There is\n> no sensible value to stuff in oi->typep if the incoming object name\n> refers to the experimental invalid object, so perhaps you will store\n> OBJ_NONE or something there and the \"cat-file --literally\" would use\n> the oi->typename to learn the name of the \"type\".\n\nYou may be able to even reuse most of the sha1_loose_object_info()\nby doing something like this illustration (read: incomplete) patch:\n\n * add an optional typename pointer to object_info request structure\n   for the caller to ask sha1_object_info() to fill.\n\n * unpack_sha1_header() takes advantage of the fact that the object\n   header of a usual object of known type would fit within 32 bytes,\n   and that otherwise the object is invalid anyway.  A literal\n   reader cannot afford to rely on these assumptions, so introduce a\n   reader that can read into a strbuf, and use it instead from\n   sha1_loose_object_info() when the caller wants to deal with\n   invalid object with a possibly overlong header.\n\n * teach sha1_object_info_extended() pass the \"flags\" parameter from\n   the caller down the callchain to sha1_loose_object_info().\n\n\n cache.h     |  3 ++-\n sha1_file.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------\n 2 files changed, 62 insertions(+), 7 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4d02efc..34ede34 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -828,8 +828,8 @@ char *strip_path_suffix(const char *path, const char *suffix);\n int daemon_avoid_alias(const char *path);\n 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 +1296,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 {\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 69a60ec..0f6783e 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@@ -2524,13 +2554,16 @@ 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  unsigned flags)\n {\n \tint status;\n \tunsigned long mapsize, size;\n \tvoid *map;\n \tgit_zstream stream;\n \tchar hdr[32];\n+\tstruct strbuf hdrbuf = STRBUF_INIT;\n+\tchar *hdrp;\n \n \tif (oi->delta_base_sha1)\n \t\thashclr(oi->delta_base_sha1);\n@@ -2557,10 +2590,21 @@ 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+\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\",\n+\t\t\t\t       sha1_to_hex(sha1));\n+\t\thdrp = hdrbuf.buf;\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\thdrp = hdr;\n+\t}\n+\n+\tif (status)\n+\t\t; /* we already have error condition */\n+\telse if ((status = parse_sha1_header(hdrp, &size)) < 0)\n \t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n \telse if (oi->sizep)\n \t\t*oi->sizep = size;\n@@ -2568,6 +2612,16 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \tmunmap(map, mapsize);\n \tif (oi->typep)\n \t\t*oi->typep = status;\n+\tif (oi->typename) {\n+\t\tif (0 <= status && typename(status))\n+\t\t\tstrbuf_addstr(oi->typename, typename(status));\n+\t\telse if ((flags & LOOKUP_LITERALLY)) {\n+\t\t\tsize_t typelen = strcspn(hdrbuf.buf, \" \");\n+\t\t\tstrbuf_add(oi->typename, hdrbuf.buf, typelen);\n+\t\t}\n+\t}\n+\tif (hdrp == hdrbuf.buf)\n+\t\tstrbuf_release(&hdrbuf);\n \treturn 0;\n }\n \n@@ -2594,7 +2648,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\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"},{"id":"256693","messageId":"1424963258.13965.1.camel@gmail.com","threadId":"38650","inReplyTo":"CAPig+cQDLU4CBQtE8vAKLyz4Xv=2DsDDMz787DVjrFwW2tiKXg@mail.gmail.com","subject":"Re: [PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-02-26T15:07:38Z","receivedAt":"2015-02-26T15:07:38Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, 2015-02-25 at 16:55 -0500, Eric Sunshine wrote:\n> I had written a longer review but was interrupted for a several hours,\n> and upon returning found that David and Junio covered many of the same\n> issues or overrode comments I was making, so the below review is pared\n> down quite a bit. Junio's proposed approach negates all of the below\n> review comments, but they may still be meaningful if kept in mind for\n> future submissions.\n> \n> On Wed, Feb 25, 2015 at 6:07 AM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> > sha1_file: Add sha1_object_type_literally and export it.\n> \n> Style: downcase \"Add\"; drop terminating period.\n> \n> > sha1_object_type_literally takes a sha value and\n> > gives the type of the given loose object, used by\n> > git cat-file -t --literally.\n> >\n> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> > ---\n> > --- a/sha1_file.c\n> > +++ b/sha1_file.c\n> > @@ -2635,6 +2635,33 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n> >         return type;\n> >  }\n> >\n> > +int sha1_object_type_literally(const unsigned char *sha1, char *type)\n> \n> This functionality is very specific to the --literally option you're\n> adding to cat-file, so it would make more sense to make it private to\n> builtin/cat-file.c rather than publishing it globally.\n> \n> Also, this is an unsafe contract. The caller does not know how many\n> bytes to allocate for 'type', and this new function may write past the\n> end of the buffer. It is more common to also pass in the size of the\n> 'type' buffer and ensure that you do not write beyond that. Or, if\n> this is intended for wider consumption, pass in a strbuf instead.\n> \n> > +{\n> > +       int status = 0;\n> > +       unsigned long mapsize;\n> > +       void *map;\n> > +       git_zstream stream;\n> > +       char hdr[32];\n> > +       int i;\n> > +\n> > +       map = map_sha1_file(sha1, &mapsize);\n> > +       if (!map)\n> > +               return -1;\n> > +       if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n> > +               status = error(\"unable to unpack %s header\",\n> > +                              sha1_to_hex(sha1));\n> \n> Since 'hdr' unpacking failed, shouldn't you be returning at this point\n> rather than continuing to the 'hdr' processing loop?\n> \n> > +       for (i = 0; i < 32; i++) {\n> > +               if (hdr[i] == ' ') {\n> > +                       type[i] = '\\0';\n> > +                       break;\n> > +               }\n> > +               type[i] = hdr[i];\n> > +       }\n> \n> David already mentioned that this loop is suspect. Perhaps take a look\n> at, sha1_file.c:parse_sha1_header() for an example of cleaner logic.\n> \n> > +\n> > +       return status;\n> > +}\n> > +\n> >  static void *read_packed_sha1(const unsigned char *sha1,\n> >                               enum object_type *type, unsigned long *size)\n> >  {\n> > --\n> > 2.3.1.129.g11acff1.dirty\n\nThanks for all your inputs, I will work on the points you've mentioned\nconsidering what David and Junio also have mentioned.\n"}]}