{"thread":{"id":"39085","subject":"[PATCH v8 0/4] cat-file: teach cat-file a '--literally' option","startedAt":"2015-04-15T16:55:50Z","lastAt":"2015-04-28T12:03:27Z","messageCount":44,"participants":["karthik nayak","Karthik Nayak","Junio C Hamano","Jeff King","Eric Sunshine","Charles Bailey"],"isPatch":true,"patchVersion":8,"patchTotal":4},"messages":[{"id":"259420","messageId":"552E9816.6040502@gmail.com","threadId":"39085","inReplyTo":null,"subject":"[PATCH v8 0/4] cat-file: teach cat-file a '--literally' option","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-15T16:55:50Z","receivedAt":"2015-04-15T16:55:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Version 7 can be found here :\nhttp://thread.gmane.org/gmane.comp.version-control.git/266761/match=patch+v7+0+4\n\nChanges In this version :\n\nsha1_file.c\n* sha1_object_info_extended() can now accept object_info *oi with \noi->typename without needing a oi->typep.\n* Changes in parse_sha1_header_extended() to make the code more organized.\n\ncat-file.c\n* Rephrasing of the '--literally' option.\n* cat_one_file() only uses oi.typename for case 't' and oi.sizep for \ncase 's'.\n\nt1006\n* Changed the size of bogus type from 0 to strlen(bogus_content).\n\nDocumentation\n* Rephrasing of the explanation for '--literally' option.\n\nSuggestion's given by Eric Sunshine and Junio Hamano.\n"},{"id":"259421","messageId":"1429117143-4882-1-git-send-email-karthik.188@gmail.com","threadId":"39085","inReplyTo":"552E9816.6040502@gmail.com","subject":"[PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-15T16:59:03Z","receivedAt":"2015-04-15T16:59:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Update sha1_loose_object_info() to optionally allow it to read\nfrom a loose object file of unknown/bogus type; as the function\nusually returns the type of the object it read in the form of enum\nfor known types, add an optional \"typename\" field to receive the\nname of the type in textual form and a flag to indicate the reading\nof a loose object file of unknown/bogus type.\n\nAdd parse_sha1_header_extended() which acts as a wrapper around\nparse_sha1_header() allowing more information to be obtained.\n\nAdd unpack_sha1_header_to_strbuf() to unpack sha1 headers of\nunknown/corrupt objects which have a unknown sha1 header size to\na strbuf structure. This was written by Junio C Hamano but tested\nby me.\n\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n cache.h | 2 +\n sha1_file.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++-----------\n 2 files changed, 105 insertions(+), 23 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c47323e..ea6faf0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -881,6 +881,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@@ -1349,6 +1350,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 980ce6b..267399d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1564,6 +1564,34 @@ 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_to_strbuf(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+\tstatus = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);\n+\n+\tif (status) {\n+\t\tstrbuf_add(header, buffer, stream->next_out - buffer);\n+\t\treturn status;\n+\t}\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_STREAM_END);\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@@ -1614,27 +1642,40 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s\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 unsigned 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+\ttype = type_from_string_gently(typename.buf, typename.len, 1);\n+\tif (oi->typename)\n+\t\tstrbuf_addbuf(oi->typename, &typename);\n+\tstrbuf_release(&typename);\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+\tif (oi->typep)\n+\t\t*oi->typep = type;\n \n \t/*\n \t * The length must follow immediately, and be in canonical\n@@ -1652,12 +1693,24 @@ 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+\tif (oi->sizep)\n+\t\t*oi->sizep = size;\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+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@@ -2522,13 +2575,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-\tunsigned long mapsize, size;\n+\tint status = 0;\n+\tunsigned long mapsize;\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@@ -2541,7 +2596,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \t * return value implicitly indicates whether the\n \t * object even exists.\n \t */\n-\tif (!oi->typep && !oi->sizep) {\n+\tif (!oi->typep && !oi->typename && !oi->sizep) {\n \t\tstruct stat st;\n \t\tif (stat_sha1_file(sha1, &st) < 0)\n \t\t\treturn -1;\n@@ -2555,17 +2610,26 @@ 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-\t\t*oi->sizep = size;\n+\tif ((flags & LOOKUP_LITERALLY)) {\n+\t\tif (unpack_sha1_header_to_strbuf(&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 with --literally\",\n+\t\t\t\t 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_extended(hdr, oi, flags)) < 0)\n+\t\t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n+\t}\n \tgit_inflate_end(&stream);\n \tmunmap(map, mapsize);\n-\tif (oi->typep)\n+\tif (status && oi->typep)\n \t\t*oi->typep = status;\n+\tif (hdrbuf.buf)\n+\t\tstrbuf_release(&hdrbuf);\n \treturn 0;\n }\n \n@@ -2574,6 +2638,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \tstruct cached_object *co;\n \tstruct pack_entry e;\n \tint rtype;\n+\tenum object_type real_type;\n \tconst unsigned char *real = lookup_replace_object_extended(sha1, flags);\n \n \tco = find_cached_object(real);\n@@ -2586,13 +2651,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@@ -2603,9 +2670,18 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \t\t\treturn -1;\n \t}\n \n+\t/*\n+\t * packed_object_info() does not follow the delta chain to\n+\t * find out the real type, unless it is given oi->typep.\n+\t */\n+\tif (oi->typename && !oi->typep)\n+\t\toi->typep = &real_type;\n+\n \trtype = packed_object_info(e.p, e.offset, oi);\n \tif (rtype < 0) {\n \t\tmark_bad_packed_object(e.p, real);\n+\t\tif (oi->typep == &real_type)\n+\t\t\toi->typep = NULL;\n \t\treturn sha1_object_info_extended(real, oi, 0);\n \t} else if (in_delta_base_cache(e.p, e.offset)) {\n \t\toi->whence = OI_DBCACHED;\n@@ -2616,6 +2692,10 @@ 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(*oi->typep));\n+\tif (oi->typep == &real_type)\n+\t\toi->typep = NULL;\n \n \treturn 0;\n }\n-- \n2.4.0.rc1.249.gb598846\n"},{"id":"259422","messageId":"1429117174-4968-1-git-send-email-karthik.188@gmail.com","threadId":"39085","inReplyTo":"552E9816.6040502@gmail.com","subject":"[PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-15T16:59:34Z","receivedAt":"2015-04-15T16:59:34Z","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. 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\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Karthik Nayak \n---\n builtin/cat-file.c | 40 +++++++++++++++++++++++++++-------------\n 1 file changed, 27 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex df99df4..1ec7190 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,20 +30,22 @@ 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.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\t\tprintf(\"%lu\\n\", size);\n-\t\t\treturn 0;\n-\t\t}\n-\t\tbreak;\n+\t\toi.sizep = &size;\n+\t\tif ((type = sha1_object_info_extended(sha1, &oi, flags)) < 0)\n+\t\t\tdie(\"git cat-file: could not get object info\");\n+\t\tprintf(\"%lu\\n\", size);\n+\t\treturn 0;\n \n \tcase 'e':\n \t\treturn !has_sha1_file(sha1);\n@@ -323,7 +332,7 @@ 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 | | --textconv) \"),\n+\tN_(\"git cat-file (-t [--literally]|-s [--literally]|-e|-p||--textconv) \"),\n \tN_(\"git cat-file (--batch | --batch-check) < \"),\n \tNULL\n };\n@@ -359,6 +368,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_(\" can be one of: blob, tree, commit, tag\")),\n@@ -369,6 +379,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_(\"allow -s and -t to work with broken/corrupt objects\")),\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 +392,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 +417,7 @@ 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\tdie(\"git cat-file --literally: use with -s or -t\");\n+\treturn cat_one_file(opt, exp_type, obj_name, literally);\n }\n-- \n2.4.0.rc1.249.gb598846\n"},{"id":"259423","messageId":"1429117213-5040-1-git-send-email-karthik.188@gmail.com","threadId":"39085","inReplyTo":"552E9816.6040502@gmail.com","subject":"[PATCH v8 3/4] cat-file: add documentation for '--literally' option.","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-15T17:00:13Z","receivedAt":"2015-04-15T17:00:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Signed-off-by: Karthik Nayak \n---\n Documentation/git-cat-file.txt | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex f6a16f4..226a9c4 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -9,7 +9,7 @@ git-cat-file - Provide content or type and size information for repository objec\n SYNOPSIS\n --------\n [verse]\n-'git cat-file' (-t | -s | -e | -p | | --textconv ) \n+'git cat-file' (-t [--literally]| -s [--literally]| -e | -p | | --textconv ) \n 'git cat-file' (--batch | --batch-check) < \n \n DESCRIPTION\n@@ -69,6 +69,9 @@ OPTIONS\n \tnot be combined with any other options or arguments. See the\n \tsection `BATCH OUTPUT` below for details.\n \n+--literally::\n+\tAllow -s or -t to query broken/corrupt objects of unknown type.\n+\n OUTPUT\n ------\n If '-t' is specified, one of the .\n-- \n2.4.0.rc1.249.gb598846\n"},{"id":"259425","messageId":"1429117232-5102-1-git-send-email-karthik.188@gmail.com","threadId":"39085","inReplyTo":"552E9816.6040502@gmail.com","subject":"[PATCH v8 4/4] t1006: add tests for git cat-file --literally","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-15T17:00:32Z","receivedAt":"2015-04-15T17:00:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Signed-off-by: Karthik Nayak \n---\n t/t1006-cat-file.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex ab36b1e..61fab78 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -47,6 +47,18 @@ $content\"\n \ttest_cmp expect actual\n '\n \n+ test_expect_success \"Type of $type is correct using --literally\" '\n+\techo $type >expect &&\n+\tgit cat-file -t --literally $sha1 >actual &&\n+\ttest_cmp expect actual\n+ '\n+\n+ test_expect_success \"Size of $type is correct using --literally\" '\n+\techo $size >expect &&\n+\tgit cat-file -s --literally $sha1 >actual &&\n+\ttest_cmp expect actual\n+ '\n+\n test -z \"$content\" ||\n test_expect_success \"Content of $type is correct\" '\n \tmaybe_remove_timestamp \"$content\" $no_ts >expect &&\n@@ -296,4 +308,21 @@ test_expect_success '%(deltabase) reports packed delta bases' '\n \t}\n '\n \n+bogus_type=\"bogus\"\n+bogus_content=\"bogus\"\n+bogus_size=$(strlen $bogus_content)\n+bogus_sha1=$(printf $bogus_content | git hash-object -t $bogus_type --literally -w --stdin)\n+\n+test_expect_success \"Type of broken object is correct\" '\n+\techo $bogus_type >expect &&\n+\tgit cat-file -t --literally $bogus_sha1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success \"Size of broken object is correct\" '\n+ echo $bogus_size >expect &&\n+\tgit cat-file -s --literally $bogus_sha1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.4.0.rc1.249.gb598846\n"},{"id":"259442","messageId":"xmqqtwwhjg8b.fsf@gitster.dls.corp.google.com","threadId":"39085","inReplyTo":"1429117174-4968-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T20:20:36Z","receivedAt":"2015-04-15T20:20:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> \tcase 's':\n> -\t\ttype = sha1_object_info(sha1, &size);\n> -\t\tif (type > 0) {\n> -\t\t\tprintf(\"%lu\\n\", size);\n> -\t\t\treturn 0;\n> -\t\t}\n> -\t\tbreak;\n> +\t\toi.sizep = &size;\n> +\t\tif ((type = sha1_object_info_extended(sha1, &oi, flags)) < 0)\n\nDo you still need to assign to type here?\n\n> +\t\t\tdie(\"git cat-file: could not get object info\");\n> +\t\tprintf(\"%lu\\n\", size);\n> +\t\treturn 0;\n\n> @@ -323,7 +332,7 @@ 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 | | --textconv) \"),\n> +\tN_(\"git cat-file (-t [--literally]|-s [--literally]|-e|-p||--textconv) \"),\n> \tN_(\"git cat-file (--batch | --batch-check) < \"),\n> \tNULL\n> };\n> @@ -359,6 +368,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_(\" can be one of: blob, tree, commit, tag\")),\n> @@ -369,6 +379,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_(\"allow -s and -t to work with broken/corrupt objects\")),\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 +392,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\nHmm, why this change?\n\nBy allowing 4 args blindly like this, you will let something like\nthese to pass:\n\n $ git cat-file --textconv -t HEAD\n commit\n $ git cat-file -p -t HEAD\n commit\n $ git cat-file -s -t HEAD\n commit\n $ git cat-file -t -s HEAD\n 879\n\nIt is fine if you are declaring that the last one wins when these\nmutually-exclusive options are given. \"git cat-file -e -s -t HEAD\"\nshould also say \"commit\" if that is what you are doing, but instead\nthe code with this patch complains, which is bad.\n\nIt also is fine and is more in line with the spirit of the original\ncode if you keep the rule tight and only one of these mutually\nexclusive options is allowed.\n\nIn either case, this check needs to be rethought.\n"},{"id":"259443","messageId":"xmqqmw29jg78.fsf@gitster.dls.corp.google.com","threadId":"39085","inReplyTo":"1429117143-4882-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T20:21:15Z","receivedAt":"2015-04-15T20:21:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\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 and a flag to indicate the reading\n> of a loose object file of unknown/bogus type.\n>\n> Add parse_sha1_header_extended() which acts as a wrapper around\n> parse_sha1_header() allowing more information to be obtained.\n>\n> Add unpack_sha1_header_to_strbuf() to unpack sha1 headers of\n> unknown/corrupt objects which have a unknown sha1 header size to\n> a strbuf structure. This was written by Junio C Hamano but tested\n> by me.\n>\n> Helped-by: Junio C Hamano \n> Helped-by: Eric Sunshine \n> Signed-off-by: Karthik Nayak \n> ---\n\nI see that you made the type parsing to happen earlier than the\nprevious round (which used to do the size first and then type).\n\nNot a problem, though. Just something I noticed...\n\n> @@ -1614,27 +1642,40 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s\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 unsigned 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\nThis _might_ have some performance impact in that strbuf_addch()\ninvolves strbuf_grow(*, 1), which does \"does it overflow to\nincrement sb->len by one?\"; I would say it should be unmeasurable\nbecause the function is expected to be used only on loose objects\nand you shouldn't have very many of them without packing in your\nrepository in the first place.\n\nI guess Peff's c1822d4f (strbuf: add an optimized 1-character\nstrbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his\nnew strbuf_grow_ch(), and once that happens the performance worry\nwould disappear without this code to be changed at all.\n\n> @@ -2541,7 +2596,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n> \t * return value implicitly indicates whether the\n> \t * object even exists.\n> \t */\n> -\tif (!oi->typep && !oi->sizep) {\n> +\tif (!oi->typep && !oi->typename && !oi->sizep) {\n\nYou didn't have this check in the earlier round, and this new one is\ncorrect, I think. Good eyes to spot this potential problem.\n\nThanks.\n"},{"id":"259446","messageId":"xmqqiocxjeqn.fsf@gitster.dls.corp.google.com","threadId":"39085","inReplyTo":"xmqqtwwhjg8b.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T20:52:48Z","receivedAt":"2015-04-15T20:52:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n> ...\n>> -\tif (argc != 3 && argc != 2)\n>> +\tif (argc < 2 || argc > 4)\n>> \t\tusage_with_options(cat_file_usage, options);\n>\n> Hmm, why this change?\n>\n> By allowing 4 args blindly like this, you will let something like\n> these to pass:\n>\n> $ git cat-file --textconv -t HEAD\n> commit\n> $ git cat-file -p -t HEAD\n> commit\n> $ git cat-file -s -t HEAD\n> commit\n> $ git cat-file -t -s HEAD\n> 879\n>\n> It is fine if you are declaring that the last one wins when these\n> mutually-exclusive options are given. \"git cat-file -e -s -t HEAD\"\n> should also say \"commit\" if that is what you are doing, but instead\n> the code with this patch complains, which is bad.\n>\n> It also is fine and is more in line with the spirit of the original\n> code if you keep the rule tight and only one of these mutually\n> exclusive options is allowed.\n>\n> In either case, this check needs to be rethought.\n\nI wonder if we want to do something like this as a preliminary step\nbefore your [PATCH 2/4]. Everything after the parse_options() call\nseems to protect against leftover argc depending on what they need\nalready, so the only thing existing \"we take only 2 or 3 args\" check\nis doing is to protect against giving more than one command mode\noptions, I think. And OPT_CMDMODE() should do a much better job at\nthat that kind of thing, by giving a more informative error message\ne.g.\n\n $ git cat-file -p -e HEAD\n error: switch 'e': incompatible with -p\n\nThis is a tangent, but while we are in the vicinity, we may want to\nrethink the help message we attach to the '-e' option. Technically\nthe current message is _not_ wrong per-se, but it misses the point.\nThe primary thing the option does is to check the (e)xistence of the\nnamed object, and the fact that it does so silently is merely a\ndetail of the operation. The current help text omits the more\nimportant part of what the option is.\n\n builtin/cat-file.c | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 1ec7190..534991d 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -372,12 +372,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \n \tconst struct option options[] = {\n \t\tOPT_GROUP(N_(\" can be one of: blob, tree, commit, tag\")),\n-\t\tOPT_SET_INT('t', NULL, &opt, N_(\"show object type\"), 't'),\n-\t\tOPT_SET_INT('s', NULL, &opt, N_(\"show object size\"), 's'),\n-\t\tOPT_SET_INT('e', NULL, &opt,\n+\t\tOPT_CMDMODE('t', NULL, &opt, N_(\"show object type\"), 't'),\n+\t\tOPT_CMDMODE('s', NULL, &opt, N_(\"show object size\"), 's'),\n+\t\tOPT_CMDMODE('e', NULL, &opt,\n \t\t\t N_(\"exit with zero when there's no error\"), 'e'),\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\tOPT_CMDMODE('p', NULL, &opt, N_(\"pretty-print object's content\"), 'p'),\n+\t\tOPT_CMDMODE(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_(\"allow -s and -t to work with broken/corrupt objects\")),\n@@ -392,9 +392,6 @@ 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 < 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 \n \tif (opt) {\n"},{"id":"259450","messageId":"20150415221824.GB27566@peff.net","threadId":"39085","inReplyTo":"xmqqmw29jg78.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-15T22:18:24Z","receivedAt":"2015-04-15T22:18:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 15, 2015 at 01:21:15PM -0700, Junio C Hamano wrote:\n\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> This _might_ have some performance impact in that strbuf_addch()\n> involves strbuf_grow(*, 1), which does \"does it overflow to\n> increment sb->len by one?\"; I would say it should be unmeasurable\n> because the function is expected to be used only on loose objects\n> and you shouldn't have very many of them without packing in your\n> repository in the first place.\n> \n> I guess Peff's c1822d4f (strbuf: add an optimized 1-character\n> strbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his\n> new strbuf_grow_ch(), and once that happens the performance worry\n> would disappear without this code to be changed at all.\n\nI haven't re-rolled that series yet, but the discussion there showed\nthat strbuf_grow_ch() is unnecessary; call-sites can just check:\n\n if (!strbuf_avail(sb))\n\tstrbuf_grow(sb, 1);\n\nto get the fast inline check. Since we go to the trouble to inline\nstrbuf_addch, we should probably teach it the same trick. It would be\nnice to identify a place with a tight strbuf_addch() loop that could\ndemonstrate the speed increase, though.\n\n-Peff\n"},{"id":"259466","messageId":"552F6429.9050304@gmail.com","threadId":"39085","inReplyTo":"xmqqiocxjeqn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-16T07:26:33Z","receivedAt":"2015-04-16T07:26:33Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 04/16/2015 02:22 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Karthik Nayak writes:\n>> ...\n>>> -\tif (argc != 3 && argc != 2)\n>>> +\tif (argc < 2 || argc > 4)\n>>> \t\tusage_with_options(cat_file_usage, options);\n>>\n>> Hmm, why this change?\n>>\n>> By allowing 4 args blindly like this, you will let something like\n>> these to pass:\n>>\n>> $ git cat-file --textconv -t HEAD\n>> commit\n>> $ git cat-file -p -t HEAD\n>> commit\n>> $ git cat-file -s -t HEAD\n>> commit\n>> $ git cat-file -t -s HEAD\n>> 879\n>>\n>> It is fine if you are declaring that the last one wins when these\n>> mutually-exclusive options are given. \"git cat-file -e -s -t HEAD\"\n>> should also say \"commit\" if that is what you are doing, but instead\n>> the code with this patch complains, which is bad.\n>>\n>> It also is fine and is more in line with the spirit of the original\n>> code if you keep the rule tight and only one of these mutually\n>> exclusive options is allowed.\n>>\n>> In either case, this check needs to be rethought.\n>\n> I wonder if we want to do something like this as a preliminary step\n> before your [PATCH 2/4]. Everything after the parse_options() call\n> seems to protect against leftover argc depending on what they need\n> already, so the only thing existing \"we take only 2 or 3 args\" check\n> is doing is to protect against giving more than one command mode\n> options, I think. And OPT_CMDMODE() should do a much better job at\n> that that kind of thing, by giving a more informative error message\n> e.g.\n>\n> $ git cat-file -p -e HEAD\n> error: switch 'e': incompatible with -p\n>\n> This is a tangent, but while we are in the vicinity, we may want to\n> rethink the help message we attach to the '-e' option. Technically\n> the current message is _not_ wrong per-se, but it misses the point.\n> The primary thing the option does is to check the (e)xistence of the\n> named object, and the fact that it does so silently is merely a\n> detail of the operation. The current help text omits the more\n> important part of what the option is.\n\nWould you rather check '-e' and go on to check '-p' or do you merely \njust want a different message.\nAs far as I see whenever any option other than a '-e' is given it \nindirectly has to check for the existence of the object.\n\neg:\n$ git cat-file -t asdfghjkl\nfatal: Not a valid object name asdfghjkl\n\n$ git cat-file -e asdfghjkl\nfatal: Not a valid object name asdfghjkl\n\nSo when a user is giving the '-e' option he just expects a silent output \nif the object exists, hence we rather have the option '-e' behave as a \nmutually exclusive option and output the error message like.\n\n$ git cat-file -p -e HEAD\nerror: switch 'e': incompatible with -p\n\nwhat do you think?\n>\n> builtin/cat-file.c | 13 +++++--------\n> 1 file changed, 5 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 1ec7190..534991d 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -372,12 +372,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>\n> \tconst struct option options[] = {\n> \t\tOPT_GROUP(N_(\" can be one of: blob, tree, commit, tag\")),\n> -\t\tOPT_SET_INT('t', NULL, &opt, N_(\"show object type\"), 't'),\n> -\t\tOPT_SET_INT('s', NULL, &opt, N_(\"show object size\"), 's'),\n> -\t\tOPT_SET_INT('e', NULL, &opt,\n> +\t\tOPT_CMDMODE('t', NULL, &opt, N_(\"show object type\"), 't'),\n> +\t\tOPT_CMDMODE('s', NULL, &opt, N_(\"show object size\"), 's'),\n> +\t\tOPT_CMDMODE('e', NULL, &opt,\n> \t\t\t N_(\"exit with zero when there's no error\"), 'e'),\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\tOPT_CMDMODE('p', NULL, &opt, N_(\"pretty-print object's content\"), 'p'),\n> +\t\tOPT_CMDMODE(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_(\"allow -s and -t to work with broken/corrupt objects\")),\n> @@ -392,9 +392,6 @@ 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 < 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>\n> \tif (opt) {\n>\nThanks! Didn't know about the OPT_CMDMODE option.\n"},{"id":"259494","messageId":"xmqq618wjiwn.fsf@gitster.dls.corp.google.com","threadId":"39085","inReplyTo":"552F6429.9050304@gmail.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-16T13:35:04Z","receivedAt":"2015-04-16T13:35:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"karthik nayak writes:\n\n> On 04/16/2015 02:22 AM, Junio C Hamano wrote:\n>\n>> This is a tangent, but while we are in the vicinity, we may want to\n>> rethink the help message we attach to the '-e' option. Technically\n>> the current message is _not_ wrong per-se, but it misses the point.\n>> The primary thing the option does is to check the (e)xistence of the\n>> named object, and the fact that it does so silently is merely a\n>> detail of the operation. The current help text omits the more\n>> important part of what the option is.\n>\n> Would you rather check '-e' and go on to check '-p' or do you merely\n> just want a different message.\n\nI meant just a different message. The point of -e is to see if the\nthing exists. It is good to mention _how_ the result is reported\nback to the user (i.e. via the exit code, not via an output to the\nstandard output \"exists\" vs \"missing\", for example), but that is\nsecondary. Telling how it reports is meaningless without telling\nwhat it reports in the first place.\n\n> ... when a user is giving the '-e' option he just expects a silent\n> output if the object exists, hence we rather have the option '-e'\n> behave as a mutually exclusive option...\n\nYes, and that is in line with the switch to OPT_CMDMODE.\n"},{"id":"259553","messageId":"8B5A4F0C-B550-4875-AE4C-3C38CDB25CDA@gmail.com","threadId":"39085","inReplyTo":"xmqq618wjiwn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-17T02:10:27Z","receivedAt":"2015-04-17T02:10:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn April 16, 2015 7:05:04 PM GMT+05:30, Junio C Hamano wrote:\n>karthik nayak writes:\n>\n>> On 04/16/2015 02:22 AM, Junio C Hamano wrote:\n>>\n>>> This is a tangent, but while we are in the vicinity, we may want to\n>>> rethink the help message we attach to the '-e' option. Technically\n>>> the current message is _not_ wrong per-se, but it misses the point.\n>>> The primary thing the option does is to check the (e)xistence of the\n>>> named object, and the fact that it does so silently is merely a\n>>> detail of the operation. The current help text omits the more\n>>> important part of what the option is.\n>>\n>> Would you rather check '-e' and go on to check '-p' or do you merely\n>> just want a different message.\n>\n>I meant just a different message. The point of -e is to see if the\n>thing exists. It is good to mention _how_ the result is reported\n>back to the user (i.e. via the exit code, not via an output to the\n>standard output \"exists\" vs \"missing\", for example), but that is\n>secondary. Telling how it reports is meaningless without telling\n>what it reports in the first place.\n\nI see what you mean. But I think it's beyond the scope of this patch series. If no one does, I'll work on it\n\n>\n>> ... when a user is giving the '-e' option he just expects a silent\n>> output if the object exists, hence we rather have the option '-e'\n>> behave as a mutually exclusive option...\n>\n>Yes, and that is in line with the switch to OPT_CMDMODE.\n\n-- \nSent from my Android device with K-9 Mail. Please excuse my brevity.\n"},{"id":"259554","messageId":"CAPc5daUrKzdjdk7Fx4CmG4U9HFcATP8y79YRjOvzb1RBUqYTiQ@mail.gmail.com","threadId":"39085","inReplyTo":"8B5A4F0C-B550-4875-AE4C-3C38CDB25CDA@gmail.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-17T02:14:36Z","receivedAt":"2015-04-17T02:14:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Apr 16, 2015 at 7:10 PM, Karthik Nayak wrote:\n>\n> On April 16, 2015 7:05:04 PM GMT+05:30, Junio C Hamano wrote:\n>>\n>>I meant just a different message. The point of -e is to see if the\n>>thing exists. It is good to mention _how_ the result is reported\n>>back to the user (i.e. via the exit code, not via an output to the\n>>standard output \"exists\" vs \"missing\", for example), but that is\n>>secondary. Telling how it reports is meaningless without telling\n>>what it reports in the first place.\n>\n> I see what you mean. But I think it's beyond the scope of this patch series.\n\nIt is perfectly fine to leave it as-is; that is why I said \"This is a tangent\".\n\nThanks.\n"},{"id":"259563","messageId":"20150417142310.GA12479@peff.net","threadId":"39085","inReplyTo":"20150415221824.GB27566@peff.net","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-17T14:23:10Z","receivedAt":"2015-04-17T14:23:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 15, 2015 at 06:18:24PM -0400, Jeff King wrote:\n\n> > This _might_ have some performance impact in that strbuf_addch()\n> > involves strbuf_grow(*, 1), which does \"does it overflow to\n> > increment sb->len by one?\"; I would say it should be unmeasurable\n> > because the function is expected to be used only on loose objects\n> > and you shouldn't have very many of them without packing in your\n> > repository in the first place.\n> > \n> > I guess Peff's c1822d4f (strbuf: add an optimized 1-character\n> > strbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his\n> > new strbuf_grow_ch(), and once that happens the performance worry\n> > would disappear without this code to be changed at all.\n> \n> I haven't re-rolled that series yet, but the discussion there showed\n> that strbuf_grow_ch() is unnecessary; call-sites can just check:\n> \n> if (!strbuf_avail(sb))\n> \tstrbuf_grow(sb, 1);\n> \n> to get the fast inline check. Since we go to the trouble to inline\n> strbuf_addch, we should probably teach it the same trick. It would be\n> nice to identify a place with a tight strbuf_addch() loop that could\n> demonstrate the speed increase, though.\n\nJust for reference, I did teach strbuf_addch this trick. And the config\ncode is the tight loop to test it with. Results are here:\n\n http://article.gmane.org/gmane.comp.version-control.git/267266\n\nIn this code path, we are typically only seeing short strings (the\noriginal code used a 10-byte static buffer). So I doubt it makes all\nthat much difference.\n\nIf there _is_ a performance implication to worry about here, I think it\nwould be that we are doing an extra malloc/free. I'm not sure I\nunderstand why we are copying it at all. The original code copied from\nthe hdr into type[10] so that we could NUL-terminate it, which was\nrequired for type_from_string().\n\nBut now we use type_from_string_gently, which can accept a length[1]. So\nwe could just count the bytes to the first space and pass the original\nbuffer along with that length, no?\n\n-Peff\n\n[1] Not related to your patch, but it looks like type_from_string_gently\n is overly lax. It does a strncmp() with the length of the candidate\n name, but does not check that we consumed all of the matching name.\n So \"tr\" would match \"tree\", \"comm\" would match \"commit\", and so\n forth.\n"},{"id":"259593","messageId":"xmqqd232hgj8.fsf@gitster.dls.corp.google.com","threadId":"39085","inReplyTo":"20150417142310.GA12479@peff.net","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-17T16:21:31Z","receivedAt":"2015-04-17T16:21:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If there _is_ a performance implication to worry about here, I think it\n> would be that we are doing an extra malloc/free.\n\nThanks for reminding me; yes, that also worried me.\n\n> I'm not sure I\n> understand why we are copying it at all. The original code copied from\n> the hdr into type[10] so that we could NUL-terminate it, which was\n> required for type_from_string().\n\nSounds like a good plan.\n\n>\n> But now we use type_from_string_gently, which can accept a length[1]. So\n> we could just count the bytes to the first space and pass the original\n> buffer along with that length, no?\n>\n> -Peff\n>\n> [1] Not related to your patch, but it looks like type_from_string_gently\n> is overly lax. It does a strncmp() with the length of the candidate\n> name, but does not check that we consumed all of the matching name.\n> So \"tr\" would match \"tree\", \"comm\" would match \"commit\", and so\n> forth.\n"},{"id":"259582","messageId":"553154C8.5090001@gmail.com","threadId":"39085","inReplyTo":"20150417142310.GA12479@peff.net","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-17T18:45:28Z","receivedAt":"2015-04-17T18:45:28Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 04/17/2015 07:53 PM, Jeff King wrote:\n> On Wed, Apr 15, 2015 at 06:18:24PM -0400, Jeff King wrote:\n>\n> >> This _might_ have some performance impact in that strbuf_addch()\n> >> involves strbuf_grow(*, 1), which does \"does it overflow to\n> >> increment sb->len by one?\"; I would say it should be unmeasurable\n> >> because the function is expected to be used only on loose objects\n> >> and you shouldn't have very many of them without packing in your\n> >> repository in the first place.\n> >>\n> >> I guess Peff's c1822d4f (strbuf: add an optimized 1-character\n> >> strbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his\n> >> new strbuf_grow_ch(), and once that happens the performance worry\n> >> would disappear without this code to be changed at all.\n> >\n> > I haven't re-rolled that series yet, but the discussion there showed\n> > that strbuf_grow_ch() is unnecessary; call-sites can just check:\n> >\n> > if (!strbuf_avail(sb))\n> > strbuf_grow(sb, 1);\n> >\n> > to get the fast inline check. Since we go to the trouble to inline\n> > strbuf_addch, we should probably teach it the same trick. It would be\n> > nice to identify a place with a tight strbuf_addch() loop that could\n> > demonstrate the speed increase, though.\n>\n> Just for reference, I did teach strbuf_addch this trick. And the config\n> code is the tight loop to test it with. Results are here:\n>\n> http://article.gmane.org/gmane.comp.version-control.git/267266\n>\n> In this code path, we are typically only seeing short strings (the\n> original code used a 10-byte static buffer). So I doubt it makes all\n> that much difference.\n>\n> If there _is_ a performance implication to worry about here, I think it\n> would be that we are doing an extra malloc/free. I'm not sure I\n> understand why we are copying it at all. The original code copied from\n> the hdr into type[10] so that we could NUL-terminate it, which was\n> required for type_from_string().\n>\n> But now we use type_from_string_gently, which can accept a length[1]. So\n> we could just count the bytes to the first space and pass the original\n> buffer along with that length, no?\n\nYes, we could, that would eliminate \"struct strbuf typename =\nSTRBUF_INIT\".\n\nSomething like this perhaps :\n\n@@ -1614,27 +1642,40 @@ static void *unpack_sha1_rest(git_zstream\n*stream, void *buffer, unsigned long s\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+static int parse_sha1_header_extended(const char *hdr, struct\nobject_info *oi,\n+ unsigned int flags)\n {\n- char type[10];\n- int i;\n+ const char *buf = hdr;\n unsigned long size;\n+ int type, len = 0;\n\n /*\n- * The type can be at most ten bytes (including the\n- * terminating '\\0' that we add), and is followed by\n+ * The type can be of any size but is followed by\n * a space.\n */\n- i = 0;\n for (;;) {\n char c = *hdr++;\n if (c == ' ')\n break;\n- type[i++] = c;\n- if (i >= sizeof(type))\n- return -1;\n+ len++;\n }\n- type[i] = 0;\n+\n+ type = type_from_string_gently(buf, len, 1);\n+ if (oi->typename) {\n+ strbuf_add(oi->typename, buf, len);\n+ strbuf_addch(oi->typename, '\\0');\n+ }\n+ /*\n+ * Set type to 0 if its an unknown object and\n+ * we're obtaining the type using '--literally'\n+ * option.\n+ */\n+ if ((flags & LOOKUP_LITERALLY) && (type == -1))\n+ type = 0;\n+ else if (type == -1)\n+ die(\"invalid object type\");\n+ if (oi->typep)\n+ *oi->typep = type;\n\n /*\n * The length must follow immediately, and be in canonical\n\n>\n> -Peff\n>\n> [1] Not related to your patch, but it looks like type_from_string_gently\n> is overly lax. It does a strncmp() with the length of the candidate\n> name, but does not check that we consumed all of the matching name.\n> So \"tr\" would match \"tree\", \"comm\" would match \"commit\", and so\n> forth.\n>\nThanks for the patch re-roll on strbuf_addch() and the patch on\ntype_from_string_gently().\n"},{"id":"259584","messageId":"20150417184939.GA32578@peff.net","threadId":"39085","inReplyTo":"553154C8.5090001@gmail.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-17T18:49:40Z","receivedAt":"2015-04-17T18:49:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 18, 2015 at 12:15:28AM +0530, karthik nayak wrote:\n\n> >But now we use type_from_string_gently, which can accept a length[1]. So\n> >we could just count the bytes to the first space and pass the original\n> >buffer along with that length, no?\n> \n> Yes, we could, that would eliminate \"struct strbuf typename =\n> STRBUF_INIT\".\n> \n> Something like this perhaps :\n\nYeah, this is exactly what I had in mind.\n\n> {\n> - char type[10];\n> - int i;\n> + const char *buf = hdr;\n> unsigned long size;\n> + int type, len = 0;\n\nMaybe switch the names of \"buf\" and \"len\" to \"type_buf\" and \"type_len\"\nto make their purpose more clear?\n\n-Peff\n"},{"id":"259592","messageId":"xmqqlhhqpnj5.fsf@gitster.dls.corp.google.com","threadId":"39085","inReplyTo":"553154C8.5090001@gmail.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-17T19:23:10Z","receivedAt":"2015-04-17T19:23:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"karthik nayak writes:\n\n> + type = type_from_string_gently(buf, len, 1);\n> + if (oi->typename) {\n> + strbuf_add(oi->typename, buf, len);\n> + strbuf_addch(oi->typename, '\\0');\n\nadd() has setlen() at the end so you do not have to NUL terminate it\nyourself. Doing addch() is actively wrong, as oi->typename->len now\ncounts the terminating NUL as part of the string, no?\n"},{"id":"259596","messageId":"20150417205125.GA7067@peff.net","threadId":"39085","inReplyTo":"xmqqd232hgj8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-17T20:51:26Z","receivedAt":"2015-04-17T20:51:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 17, 2015 at 09:21:31AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > If there _is_ a performance implication to worry about here, I think it\n> > would be that we are doing an extra malloc/free.\n> \n> Thanks for reminding me; yes, that also worried me.\n\nAs an aside, I worried about the extra allocation for reading the header\nin the first place. But it looks like we only do this on the --literally\ncode path (and otherwise use the normal unpack_sha1_header). Still, I\nwonder if we could make this work automagically. That is, speculatively\nunpack the first N bytes, assuming we hit the end-of-header. If not,\nthen go to a strbuf as the slow path. Then it would be fine to cover all\ncases; the normal ones would be fast, and only ridiculous things would\nincur the extra allocation.\n\n-Peff\n"},{"id":"259600","messageId":"xmqq4moepijp.fsf@gitster.dls.corp.google.com","threadId":"39085","inReplyTo":"20150417205125.GA7067@peff.net","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-17T21:10:50Z","receivedAt":"2015-04-17T21:10:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Apr 17, 2015 at 09:21:31AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > If there _is_ a performance implication to worry about here, I think it\n>> > would be that we are doing an extra malloc/free.\n>> \n>> Thanks for reminding me; yes, that also worried me.\n>\n> As an aside, I worried about the extra allocation for reading the header\n> in the first place. But it looks like we only do this on the --literally\n> code path (and otherwise use the normal unpack_sha1_header). Still, I\n> wonder if we could make this work automagically. That is, speculatively\n> unpack the first N bytes, assuming we hit the end-of-header. If not,\n> then go to a strbuf as the slow path. Then it would be fine to cover all\n> cases; the normal ones would be fast, and only ridiculous things would\n> incur the extra allocation.\n\nYes, that was what I was hoping to see eventually ;-)\n"},{"id":"259608","messageId":"CAPig+cSiYsF6ioi+-P_BP=1bs-EAtNdAGjSqw6KmZyE39rKWBA@mail.gmail.com","threadId":"39085","inReplyTo":"1429117143-4882-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-17T23:31:07Z","receivedAt":"2015-04-17T23:31:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 15, 2015 at 12:59 PM, Karthik Nayak wrote:\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 and a flag to indicate the reading\n> of a loose object file of unknown/bogus type.\n> [...]\n> ---\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 980ce6b..267399d 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2522,13 +2575,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> - struct object_info *oi)\n> + struct object_info *oi,\n> + int flags)\n> {\n> - int status;\n> - unsigned long mapsize, size;\n> + int status = 0;\n> + unsigned long mapsize;\n> void *map;\n> git_zstream stream;\n> char hdr[32];\n> + struct strbuf hdrbuf = STRBUF_INIT;\n>\n> if (oi->delta_base_sha1)\n> hashclr(oi->delta_base_sha1);\n> @@ -2555,17 +2610,26 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n> return -1;\n> if (oi->disk_sizep)\n> *oi->disk_sizep = mapsize;\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> - else if ((status = parse_sha1_header(hdr, &size)) < 0)\n> - status = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n> - else if (oi->sizep)\n> - *oi->sizep = size;\n> + if ((flags & LOOKUP_LITERALLY)) {\n> + if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, &hdrbuf) < 0)\n> + status = error(\"unable to unpack %s header with --literally\",\n> + sha1_to_hex(sha1));\n> + else if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)\n> + status = error(\"unable to parse %s header with --literally\",\n> + sha1_to_hex(sha1));\n> + } else {\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> + else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)\n> + status = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n> + }\n> git_inflate_end(&stream);\n> munmap(map, mapsize);\n> - if (oi->typep)\n> + if (status && oi->typep)\n> *oi->typep = status;\n> + if (hdrbuf.buf)\n> + strbuf_release(&hdrbuf);\n\nWhy is strbuf_release() protected by a conditional rather than being\ncalled unconditionally? Am I missing something obvious?\n\n> return 0;\n> }\n>\n"},{"id":"259610","messageId":"CAPig+cRoY5OEvoZ4oysdiDTpMh+mYgL3dYAOYOhnRgH_=a1fzQ@mail.gmail.com","threadId":"39085","inReplyTo":"1429117232-5102-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v8 4/4] t1006: add tests for git cat-file --literally","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-18T00:00:10Z","receivedAt":"2015-04-18T00:00:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 15, 2015 at 1:00 PM, Karthik Nayak wrote:\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index ab36b1e..61fab78 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -296,4 +308,21 @@ test_expect_success '%(deltabase) reports packed delta bases' '\n> }\n> '\n>\n> +bogus_type=\"bogus\"\n> +bogus_content=\"bogus\"\n> +bogus_size=$(strlen $bogus_content)\n\nIf someone ever changes the value of 'bogus_content' so it contains\nwhitespace, then the result of strlen() will be incorrect as you've\ninvoked it. You should quote its argument (as other callers in this\nscript do) to safeguard against such an issue.\n\n bogus_size=$(strlen \"$bogus_content\")\n\n> +bogus_sha1=$(printf $bogus_content | git hash-object -t $bogus_type --literally -w --stdin)\n\nDitto regarding quoting of 'bogus_content'.\n\nAlso, if someone ever modifies 'bogus_content' so that it contains a\nliteral '%' (such as \"%s\"), then your printf() invocation will\nmisbehave. Either call it like this:\n\n $(printf '%s' \"$bogus_content\" | ...)\n\nor, better yet, call echo_without_newline() as other similar code\nelsewhere in this script does, and as suggested earlier[1].\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/266972/\n\n> +test_expect_success \"Type of broken object is correct\" '\n> + echo $bogus_type >expect &&\n> + git cat-file -t --literally $bogus_sha1 >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success \"Size of broken object is correct\" '\n> + echo $bogus_size >expect &&\n\nBad indentation. Use tab rather than spaces.\n\n> + git cat-file -s --literally $bogus_sha1 >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.4.0.rc1.249.gb598846\n"},{"id":"259613","messageId":"5531EA31.1010104@gmail.com","threadId":"39085","inReplyTo":"CAPig+cRoY5OEvoZ4oysdiDTpMh+mYgL3dYAOYOhnRgH_=a1fzQ@mail.gmail.com","subject":"Re: [PATCH v8 4/4] t1006: add tests for git cat-file --literally","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-18T05:22:57Z","receivedAt":"2015-04-18T05:22:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\nOn 04/18/2015 05:30 AM, Eric Sunshine wrote:\n> On Wed, Apr 15, 2015 at 1:00 PM, Karthik Nayak wrote:\n> > Signed-off-by: Karthik Nayak \n> > ---\n> > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> > index ab36b1e..61fab78 100755\n> > --- a/t/t1006-cat-file.sh\n> > +++ b/t/t1006-cat-file.sh\n> > @@ -296,4 +308,21 @@ test_expect_success '%(deltabase) reports packed delta bases' '\n> > }\n> > '\n> >\n> > +bogus_type=\"bogus\"\n> > +bogus_content=\"bogus\"\n> > +bogus_size=$(strlen $bogus_content)\n>\n> If someone ever changes the value of 'bogus_content' so it contains\n> whitespace, then the result of strlen() will be incorrect as you've\n> invoked it. You should quote its argument (as other callers in this\n> script do) to safeguard against such an issue.\n>\n> bogus_size=$(strlen \"$bogus_content\")\n>\n> > +bogus_sha1=$(printf $bogus_content | git hash-object -t $bogus_type --literally -w --stdin)\n>\n> Ditto regarding quoting of 'bogus_content'.\n>\n> Also, if someone ever modifies 'bogus_content' so that it contains a\n> literal '%' (such as \"%s\"), then your printf() invocation will\n> misbehave. Either call it like this:\n>\n> $(printf '%s' \"$bogus_content\" | ...)\n>\n> or, better yet, call echo_without_newline() as other similar code\n> elsewhere in this script does, and as suggested earlier[1].\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/266972/\n>\n> > +test_expect_success \"Type of broken object is correct\" '\n> > + echo $bogus_type >expect &&\n> > + git cat-file -t --literally $bogus_sha1 >actual &&\n> > + test_cmp expect actual\n> > +'\n> > +\n> > +test_expect_success \"Size of broken object is correct\" '\n> > + echo $bogus_size >expect &&\n>\n> Bad indentation. Use tab rather than spaces.\n>\n> > + git cat-file -s --literally $bogus_sha1 >actual &&\n> > + test_cmp expect actual\n> > +'\n> > +\n> > test_done\n> > --\n> > 2.4.0.rc1.249.gb598846\nThanks Eric for the changes. I did \"echo -n\" at the beginning but that\ngave me a warning and asked me to use printf instead. I'll use\necho_without_newline, Thanks.\n"},{"id":"259616","messageId":"5532167E.5000206@gmail.com","threadId":"39085","inReplyTo":"20150417184939.GA32578@peff.net","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-18T08:31:58Z","receivedAt":"2015-04-18T08:31:58Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 04/18/2015 12:19 AM, Jeff King wrote:\n> On Sat, Apr 18, 2015 at 12:15:28AM +0530, karthik nayak wrote:\n>\n>>> But now we use type_from_string_gently, which can accept a length[1]. So\n>>> we could just count the bytes to the first space and pass the original\n>>> buffer along with that length, no?\n>>\n>> Yes, we could, that would eliminate \"struct strbuf typename =\n>> STRBUF_INIT\".\n>>\n>> Something like this perhaps :\n>\n> Yeah, this is exactly what I had in mind.\n>\n>> {\n>> - char type[10];\n>> - int i;\n>> + const char *buf = hdr;\n>> unsigned long size;\n>> + int type, len = 0;\n>\n> Maybe switch the names of \"buf\" and \"len\" to \"type_buf\" and \"type_len\"\n> to make their purpose more clear?\n>\n> -Peff\n>\n\nYes, will change the names.\n"},{"id":"259617","messageId":"5532169F.4070709@gmail.com","threadId":"39085","inReplyTo":"xmqqlhhqpnj5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-18T08:32:31Z","receivedAt":"2015-04-18T08:32:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 04/18/2015 12:53 AM, Junio C Hamano wrote:\n> karthik nayak writes:\n>\n>> + type = type_from_string_gently(buf, len, 1);\n>> + if (oi->typename) {\n>> + strbuf_add(oi->typename, buf, len);\n>> + strbuf_addch(oi->typename, '\\0');\n>\n> add() has setlen() at the end so you do not have to NUL terminate it\n> yourself. Doing addch() is actively wrong, as oi->typename->len now\n> counts the terminating NUL as part of the string, no?\n>\n\nYes. was speculative of that. thanks for clearing it up.\n"},{"id":"259619","messageId":"55321DCA.8030801@gmail.com","threadId":"39085","inReplyTo":"CAPig+cSiYsF6ioi+-P_BP=1bs-EAtNdAGjSqw6KmZyE39rKWBA@mail.gmail.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-18T09:03:06Z","receivedAt":"2015-04-18T09:03:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 04/18/2015 05:01 AM, Eric Sunshine wrote:\n> On Wed, Apr 15, 2015 at 12:59 PM, Karthik Nayak wrote:\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 and a flag to indicate the reading\n>> of a loose object file of unknown/bogus type.\n>> [...]\n>> ---\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 980ce6b..267399d 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -2522,13 +2575,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>> - struct object_info *oi)\n>> + struct object_info *oi,\n>> + int flags)\n>> {\n>> - int status;\n>> - unsigned long mapsize, size;\n>> + int status = 0;\n>> + unsigned long mapsize;\n>> void *map;\n>> git_zstream stream;\n>> char hdr[32];\n>> + struct strbuf hdrbuf = STRBUF_INIT;\n>>\n>> if (oi->delta_base_sha1)\n>> hashclr(oi->delta_base_sha1);\n>> @@ -2555,17 +2610,26 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n>> return -1;\n>> if (oi->disk_sizep)\n>> *oi->disk_sizep = mapsize;\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>> - else if ((status = parse_sha1_header(hdr, &size)) < 0)\n>> - status = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n>> - else if (oi->sizep)\n>> - *oi->sizep = size;\n>> + if ((flags & LOOKUP_LITERALLY)) {\n>> + if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, &hdrbuf) < 0)\n>> + status = error(\"unable to unpack %s header with --literally\",\n>> + sha1_to_hex(sha1));\n>> + else if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)\n>> + status = error(\"unable to parse %s header with --literally\",\n>> + sha1_to_hex(sha1));\n>> + } else {\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>> + else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)\n>> + status = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n>> + }\n>> git_inflate_end(&stream);\n>> munmap(map, mapsize);\n>> - if (oi->typep)\n>> + if (status && oi->typep)\n>> *oi->typep = status;\n>> + if (hdrbuf.buf)\n>> + strbuf_release(&hdrbuf);\n>\n> Why is strbuf_release() protected by a conditional rather than being\n> called unconditionally? Am I missing something obvious?\nNo, you're right.\n>\n>> return 0;\n>> }\n>>\n"},{"id":"259636","messageId":"20150419002807.GA11634@hashpling.org","threadId":"39085","inReplyTo":"1429117174-4968-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-04-19T00:28:07Z","receivedAt":"2015-04-19T00:28:07Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Apr 15, 2015 at 10:29:34PM +0530, Karthik Nayak wrote:\n> Currently 'git cat-file' throws an error while trying to\n> print the type or size of a broken/corrupt object. This is\n> because these objects are usually of unknown types.\n> \n> Teach git cat-file a '--literally' option where it prints\n> the type or size of a broken/corrupt object without throwing\n> an error.\n\nI'm sorry to come in with such a fundamental question at such a late\nrevision of this patch series, but am I the only person to wonder about\nthe choice of option name?\n\nTo me, cat-file already output objects \"literally\" (without -p) as\nopposed to show. From the description, it feels more like it should be\n\"--unchecked\" or perhaps something better that I haven't thought of?\n\nThe option isn't a true opposite of hash-object's --literally because\nthat also allows the creation of known types with invalid contents (e.g.\ncorrupt trees) whereas cat-file is quite happy to show the _contents_ of\nsuch corrupt objects even without --literally.\n\nCharles.\n"},{"id":"259656","messageId":"xmqq7ft7nz8l.fsf@gitster.dls.corp.google.com","threadId":"39085","inReplyTo":"20150419002807.GA11634@hashpling.org","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-20T05:30:02Z","receivedAt":"2015-04-20T05:30:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey writes:\n\n> I'm sorry to come in with such a fundamental question at such a late\n> revision of this patch series, but am I the only person to wonder about\n> the choice of option name?\n>\n> To me, cat-file already output objects \"literally\" (without -p) as\n> opposed to show. From the description, it feels more like it should be\n> \"--unchecked\" or perhaps something better that I haven't thought of?\n\nYeah, it was conceived as a way to grok what hash-object --literally\nwould create, but the operation by \"cat-file --literally\" is not\nabout showing the contents literally without interpreting (the\ngeneral \"cat-file \" does the literal output\nalready). So it was my fault to suggest that name, but I do not\nthink of a better alternative.\n\n> The option isn't a true opposite of hash-object's --literally because\n> that also allows the creation of known types with invalid contents (e.g.\n> corrupt trees) whereas cat-file is quite happy to show the _contents_ of\n> such corrupt objects even without --literally.\n\nNot really. If you create an object with corrupt type string (e.g. \"BLOB\"\ninstead of \"blob\"), cat-file would not be happy.\n"},{"id":"259663","messageId":"20150420074433.GA30422@hashpling.org","threadId":"39085","inReplyTo":"xmqq7ft7nz8l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-04-20T07:44:34Z","receivedAt":"2015-04-20T07:44:34Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"> On 20 Apr 2015, at 06:30, Junio C Hamano wrote:\n> Charles Bailey writes:\n> \n>> The option isn't a true opposite of hash-object's --literally because\n>> that also allows the creation of known types with invalid contents (e.g.\n>> corrupt trees) whereas cat-file is quite happy to show the _contents_ of\n>> such corrupt objects even without --literally.\n> \n> Not really. If you create an object with corrupt type string (e.g. \"BLOB\"\n> instead of \"blob\"), cat-file would not be happy.\n\nSorry, the emphasis should have been on \"complete\" of \"complete\nopposite\". There are some types of bad objects that can be created only\nwith hash-object --literally (malformed tag or tree), for which cat-file\nworks with fine and there are other types (pun unintended - BLOB,\nwobble, etc.) for which --literally/--unchecked is required with\ncat-file.\n\nSo I meant that cat-file's --literally is only a partial \"opposite\" or\nanalogue of hash-object's.\n\n--allow-invalid-types? --force (in the sense of \"suppress some possible\nerrors\")? It's not a big thing but I'm aware that if we can find a better\nname then now would be the best moment. If not, then --literally it is.\n"},{"id":"259667","messageId":"8CBC4DEB-EC50-4DD7-A687-443AA93A96A8@gmail.com","threadId":"39085","inReplyTo":"20150420074433.GA30422@hashpling.org","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-20T08:57:44Z","receivedAt":"2015-04-20T08:57:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn April 20, 2015 1:14:34 PM GMT+05:30, Charles Bailey wrote:\n>> On 20 Apr 2015, at 06:30, Junio C Hamano wrote:\n>> Charles Bailey writes:\n>> \n>>> The option isn't a true opposite of hash-object's --literally\n>because\n>>> that also allows the creation of known types with invalid contents\n>(e.g.\n>>> corrupt trees) whereas cat-file is quite happy to show the\n>_contents_ of\n>>> such corrupt objects even without --literally.\n>> \n>> Not really. If you create an object with corrupt type string (e.g.\n>\"BLOB\"\n>> instead of \"blob\"), cat-file would not be happy.\n>\n>Sorry, the emphasis should have been on \"complete\" of \"complete\n>opposite\". There are some types of bad objects that can be created\n>only\n>with hash-object --literally (malformed tag or tree), for which\n>cat-file\n>works with fine and there are other types (pun unintended - BLOB,\nSorry, but I didn't get you, broken objects created using hash-object --literally do not work with cat-file without the --literally option.\n>wobble, etc.) for which --literally/--unchecked is required with\n>cat-file.\n>\n>So I meant that cat-file's --literally is only a partial \"opposite\" or\n>analogue of hash-object's.\n>\n>--allow-invalid-types? --force (in the sense of \"suppress some possible\n>errors\")? It's not a big thing but I'm aware that if we can find a\n>better\n>name then now would be the best moment. If not, then --literally it is.\n\n-- \nSent from my Android device with K-9 Mail. Please excuse my brevity.\n"},{"id":"259668","messageId":"20150420091920.GA31279@hashpling.org","threadId":"39085","inReplyTo":"8CBC4DEB-EC50-4DD7-A687-443AA93A96A8@gmail.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-04-20T09:19:21Z","receivedAt":"2015-04-20T09:19:21Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Mon, Apr 20, 2015 at 02:27:44PM +0530, Karthik Nayak wrote:\n> Sorry, but I didn't get you, broken objects created using hash-object --literally do not work with cat-file without the --literally option.\n\nPerhaps an example would help:\n\nI cannot create a bad tree without --literally:\n\n$ echo total garbage | ./git hash-object -t tree --stdin -w\nfatal: corrupt tree file\n$ echo total garbage | ./git hash-object -t tree --stdin -w --literally\nfa2905d47028d00baec739f6d73540bb2a75c6f7\n\nbut I can use cat-file without --literally to query the contents and\ninformation about the object as it stands.\n\n$ ./git cat-file tree fa2905d47028d00baec739f6d73540bb2a75c6f7\ntotal garbage\n$ ./git cat-file -t fa2905d47028d00baec739f6d73540bb2a75c6f7\ntree\n$ ./git cat-file -s fa2905d47028d00baec739f6d73540bb2a75c6f7\n14\n\nAs far as I could tell - and please correct me if I've misunderstood,\ncat-file's literally is about dealing with unrecognized types whereas\nhash-object's --literally is about both creating objects with bad types\nand invalid objects of \"recognized\" types. This latter scenario is where\nthe option name \"literally\" makes the most sense.\n\nCharles.\n"},{"id":"259680","messageId":"553520CF.6070304@gmail.com","threadId":"39085","inReplyTo":"20150420091920.GA31279@hashpling.org","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-20T15:52:47Z","receivedAt":"2015-04-20T15:52:47Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 04/20/2015 02:49 PM, Charles Bailey wrote:\n> On Mon, Apr 20, 2015 at 02:27:44PM +0530, Karthik Nayak wrote:\n> > Sorry, but I didn't get you, broken objects created using hash-object --literally do not work with cat-file without the --literally option.\n>\n> Perhaps an example would help:\n>\n> I cannot create a bad tree without --literally:\n>\n> $ echo total garbage | ./git hash-object -t tree --stdin -w\n> fatal: corrupt tree file\n> $ echo total garbage | ./git hash-object -t tree --stdin -w --literally\n> fa2905d47028d00baec739f6d73540bb2a75c6f7\n>\n> but I can use cat-file without --literally to query the contents and\n> information about the object as it stands.\n>\n> $ ./git cat-file tree fa2905d47028d00baec739f6d73540bb2a75c6f7\n> total garbage\n> $ ./git cat-file -t fa2905d47028d00baec739f6d73540bb2a75c6f7\n> tree\n> $ ./git cat-file -s fa2905d47028d00baec739f6d73540bb2a75c6f7\n> 14\n>\n> As far as I could tell - and please correct me if I've misunderstood,\n> cat-file's literally is about dealing with unrecognized types whereas\n> hash-object's --literally is about both creating objects with bad types\n> and invalid objects of \"recognized\" types. This latter scenario is where\n> the option name \"literally\" makes the most sense.\nYes. What you're saying is correct, but it also makes sense as we're asking\n\"cat-file\" to give us information about the object irrespective of the type of the\nobject, hence asking it to literally print the information. Also it stays as a compliment\nto \"hash-object --literally\", which is already existing.\n>\n> Charles.\n>\n"},{"id":"259691","messageId":"553548D2.7010904@gmail.com","threadId":"39085","inReplyTo":"xmqq4moepijp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-20T18:43:30Z","receivedAt":"2015-04-20T18:43:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 04/18/2015 02:40 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Fri, Apr 17, 2015 at 09:21:31AM -0700, Junio C Hamano wrote:\n>>\n>>> Jeff King writes:\n>>>\n>>>> If there _is_ a performance implication to worry about here, I think it\n>>>> would be that we are doing an extra malloc/free.\n>>>\n>>> Thanks for reminding me; yes, that also worried me.\n>>\n>> As an aside, I worried about the extra allocation for reading the header\n>> in the first place. But it looks like we only do this on the --literally\n>> code path (and otherwise use the normal unpack_sha1_header). Still, I\n>> wonder if we could make this work automagically. That is, speculatively\n>> unpack the first N bytes, assuming we hit the end-of-header. If not,\n>> then go to a strbuf as the slow path. Then it would be fine to cover all\n>> cases; the normal ones would be fast, and only ridiculous things would\n>> incur the extra allocation.\n>\n> Yes, that was what I was hoping to see eventually ;-)\n>\n\nSorry for the delay, So after reading what Jeff said I tried to \nimplement it, this might not be a final version of the change, more of a \nRFC version. What do you'll think?\n\n@@ -1564,6 +1564,36 @@ int unpack_sha1_header(git_zstream *stream, \nunsigned char *map, unsigned long ma\n return git_inflate(stream, 0);\n }\n\n+static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned \nchar *map,\n+ unsigned long mapsize, void *buffer,\n+ unsigned long bufsiz, struct \nstrbuf *header)\n+{\n+ unsigned char *cp;\n+ int status;\n+ int i = 0;\n+\n+ status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);\n+\n+ for (cp = buffer; cp < stream->next_out; cp++)\n+ if (!*cp) {\n+ /* Found the NUL at the end of the header */\n+ return 0;\n+ }\n+\n+ strbuf_add(header, buffer, stream->next_out - (unsigned char \n*)buffer);\n+ do {\n+ status = git_inflate(stream, 0);\n+ strbuf_add(header, buffer, stream->next_out - (unsigned \nchar *)buffer);\n+ for (cp = buffer; cp < stream->next_out; cp++)\n+ if (!*cp)\n+ /* Found the NUL at the end of the header */\n+ return 0;\n+ stream->next_out = buffer;\n+ stream->avail_out = bufsiz;\n+ } while (status != Z_STREAM_END);\n+ return -1;\n+}\n+\n\n\n@@ -2555,17 +2610,24 @@ static int sha1_loose_object_info(const unsigned \nchar *sha1,\n return -1;\n if (oi->disk_sizep)\n *oi->disk_sizep = mapsize;\n- if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n+ if ((flags & LOOKUP_LITERALLY)) {\n+ if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, \nhdr, sizeof(hdr), &hdrbuf) < 0)\n+ status = error(\"unable to unpack %s header with \n--literally\",\n+ sha1_to_hex(sha1));\n+ } else if (unpack_sha1_header(&stream, map, mapsize, hdr, \nsizeof(hdr)) < 0)\n status = error(\"unable to unpack %s header\",\n sha1_to_hex(sha1));\n- else if ((status = parse_sha1_header(hdr, &size)) < 0)\n+ if (hdrbuf.len) {\n+ if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, \nflags)) < 0)\n+ status = error(\"unable to parse %s header with \n--literally\",\n+ sha1_to_hex(sha1));\n+ } else if ((status = parse_sha1_header_extended(hdr, oi, flags)) \n< 0)\n status = error(\"unable to parse %s header\", \nsha1_to_hex(sha1));\n\n\nand\n"},{"id":"259693","messageId":"20150420185122.GA13718@peff.net","threadId":"39085","inReplyTo":"553548D2.7010904@gmail.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-20T18:51:22Z","receivedAt":"2015-04-20T18:51:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 21, 2015 at 12:13:30AM +0530, karthik nayak wrote:\n\n> +static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char\n> *map,\n> + unsigned long mapsize, void *buffer,\n> + unsigned long bufsiz, struct strbuf\n> *header)\n> +{\n> + unsigned char *cp;\n> + int status;\n> + int i = 0;\n> +\n> + status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);\n\nI wonder if we would feel comfortable just running this NUL-check as\npart of unpack_sha1_header (i.e., in all code paths). It _shouldn't_\ntrigger in normal use, but I wonder if there would be any downsides\n(e.g., maliciously crafted objects getting us to allocate memory or\nsomething; I think it is fairly easy to convince git to allocate memory,\nthough).\n\n> + for (cp = buffer; cp < stream->next_out; cp++)\n> + if (!*cp) {\n> + /* Found the NUL at the end of the header */\n> + return 0;\n> + }\n\nI think we can spell this as:\n\n if (memchr(buffer, '\\0', stream->next_out - buffer))\n\treturn 0;\n\nwhich is shorter and possibly more efficient.\n\nIn theory we could also just start trying to parse the type/size header,\nand notice there when we don't find the NUL. That's probably not worth\ndoing, though. The parsing is separated from the unpacking here, so it\nwould require combining those two operations in a single function. And\nthe extra NUL search here is likely not very expensive.\n\n-Peff\n"},{"id":"259729","messageId":"20150421101641.GA13202@hashpling.org","threadId":"39085","inReplyTo":"553520CF.6070304@gmail.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2015-04-21T10:16:42Z","receivedAt":"2015-04-21T10:16:42Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Mon, Apr 20, 2015 at 09:22:47PM +0530, karthik nayak wrote:\n> \n> \n> On 04/20/2015 02:49 PM, Charles Bailey wrote:\n> >As far as I could tell - and please correct me if I've misunderstood,\n> >cat-file's literally is about dealing with unrecognized types whereas\n> >hash-object's --literally is about both creating objects with bad types\n> >and invalid objects of \"recognized\" types. This latter scenario is where\n> >the option name \"literally\" makes the most sense.\n> Yes. What you're saying is correct, but it also makes sense as we're asking\n> \"cat-file\" to give us information about the object irrespective of the type of the\n> object, hence asking it to literally print the information. Also it stays as a compliment\n> to \"hash-object --literally\", which is already existing.\n\nOK, I think you've hit the main point which I was trying to make.\n\nTo me, \"literally\" means \"without transformation\" or \"exactly as\nwritten/recorded/transmitted\" (which -t/-s do anyway) and doesn't really\nencompass the \"irrespective of type\" meaning that it has been given here.\n\nIn any case, I've made my point so I won't labour it any further. I\nthink that --no-validation or --allow-any-type might be more accurate\nbut if everyone else is happy enough with --literally then I'm happy to\nlive with that too.\n"},{"id":"259731","messageId":"553633D0.9020804@gmail.com","threadId":"39085","inReplyTo":"20150420185122.GA13718@peff.net","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-21T11:26:08Z","receivedAt":"2015-04-21T11:26:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\n\nOn 04/21/2015 12:21 AM, Jeff King wrote:\n> On Tue, Apr 21, 2015 at 12:13:30AM +0530, karthik nayak wrote:\n>\n>> +static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char\n>> *map,\n>> + unsigned long mapsize, void *buffer,\n>> + unsigned long bufsiz, struct strbuf\n>> *header)\n>> +{\n>> + unsigned char *cp;\n>> + int status;\n>> + int i = 0;\n>> +\n>> + status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);\n>\n> I wonder if we would feel comfortable just running this NUL-check as\n> part of unpack_sha1_header (i.e., in all code paths). It _shouldn't_\n> trigger in normal use, but I wonder if there would be any downsides\n> (e.g., maliciously crafted objects getting us to allocate memory or\n> something; I think it is fairly easy to convince git to allocate memory,\n> though).\n>\nBut why would we want it to be a part of unpack_sha1_header?\n\n>> + for (cp = buffer; cp < stream->next_out; cp++)\n>> + if (!*cp) {\n>> + /* Found the NUL at the end of the header */\n>> + return 0;\n>> + }\n>\n> I think we can spell this as:\n>\n> if (memchr(buffer, '\\0', stream->next_out - buffer))\n> \treturn 0;\n>\n> which is shorter and possibly more efficient.\nNoted. Thanks :)\n>\n> In theory we could also just start trying to parse the type/size header,\n> and notice there when we don't find the NUL. That's probably not worth\n> doing, though. The parsing is separated from the unpacking here, so it\n> would require combining those two operations in a single function. And\n> the extra NUL search here is likely not very expensive.\n>\nYes, even I though about doing that, but wasn't keen on combining those \ntwo functions, they're meant to do two different things.\n> -Peff\n>\n"},{"id":"259737","messageId":"20150421142455.GA14521@peff.net","threadId":"39085","inReplyTo":"553633D0.9020804@gmail.com","subject":"Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-21T14:24:56Z","receivedAt":"2015-04-21T14:24:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 21, 2015 at 04:56:08PM +0530, karthik nayak wrote:\n\n> >>+ status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);\n> >\n> >I wonder if we would feel comfortable just running this NUL-check as\n> >part of unpack_sha1_header (i.e., in all code paths). It _shouldn't_\n> >trigger in normal use, but I wonder if there would be any downsides\n> >(e.g., maliciously crafted objects getting us to allocate memory or\n> >something; I think it is fairly easy to convince git to allocate memory,\n> >though).\n> >\n> But why would we want it to be a part of unpack_sha1_header?\n\nJust to reduce the number of functions and the complexity of the caller.\nBut I guess it doesn't help that much if the caller would then need to\nspeculatively pass in a strbuf. So it's probably not a good idea.\n\n-Peff\n"},{"id":"259757","messageId":"CAPig+cRmPv5u_OCtUPNoYOUcOoa9xn++Xw-rkD6EP6_eq0YvEQ@mail.gmail.com","threadId":"39085","inReplyTo":"20150421101641.GA13202@hashpling.org","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-21T19:40:35Z","receivedAt":"2015-04-21T19:40:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 21, 2015 at 6:16 AM, Charles Bailey wrote:\n> On Mon, Apr 20, 2015 at 09:22:47PM +0530, karthik nayak wrote:\n>> On 04/20/2015 02:49 PM, Charles Bailey wrote:\n>> >As far as I could tell - and please correct me if I've misunderstood,\n>> >cat-file's literally is about dealing with unrecognized types whereas\n>> >hash-object's --literally is about both creating objects with bad types\n>> >and invalid objects of \"recognized\" types. This latter scenario is where\n>> >the option name \"literally\" makes the most sense.\n>> Yes. What you're saying is correct, but it also makes sense as we're asking\n>> \"cat-file\" to give us information about the object irrespective of the type of the\n>> object, hence asking it to literally print the information. Also it stays as a compliment\n>> to \"hash-object --literally\", which is already existing.\n>\n> OK, I think you've hit the main point which I was trying to make.\n>\n> To me, \"literally\" means \"without transformation\" or \"exactly as\n> written/recorded/transmitted\" (which -t/-s do anyway) and doesn't really\n> encompass the \"irrespective of type\" meaning that it has been given here.\n>\n> In any case, I've made my point so I won't labour it any further. I\n> think that --no-validation or --allow-any-type might be more accurate\n> but if everyone else is happy enough with --literally then I'm happy to\n> live with that too.\n\nIt's easy to be blinded into thinking that cat-file's new option\nshould be named --literally since it was inspired by the --literally\noption of hash-object, but indeed it may not be the best choice.\n\nIn addition to your above suggestions (and --unchecked which you\nproposed earlier), if we take inspiration from existing Git options,\nperhaps one of the following (or something derived from them) would be\nbetter?\n\n --force\n --ignore-errors\n --no-check\n --unsafe\n --no-strict\n --aggressive\n\nOr, some pure made-up bike-shedding?\n\n --try-harder\n --allow-bogus-type\n --ignore-bogus-type\n --loose\n --gently\n"},{"id":"259759","messageId":"xmqqh9s9gqw9.fsf@gitster.dls.corp.google.com","threadId":"39085","inReplyTo":"CAPig+cRmPv5u_OCtUPNoYOUcOoa9xn++Xw-rkD6EP6_eq0YvEQ@mail.gmail.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-21T20:36:38Z","receivedAt":"2015-04-21T20:36:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> It's easy to be blinded into thinking that cat-file's new option\n> should be named --literally since it was inspired by the --literally\n> option of hash-object, but indeed it may not be the best choice.\n\nYeah, I wouldn't even say \"inspired\". It was envisioned as a\ncounter-part debugging aid, nothing more.\n\nIs there any other way to make cat-file looser other than accepting\nan unknown type name from the future? If so, then perhaps it may\nmake sense to give it a generic name that implys that we would\ntrigger such additional looseness in the future. But as the\ninventor of it as a debugging aid, I would say a name that spells\nout the specific way this option is being loose, e.g.\n\n> --allow-bogus-type\n\nor with s/bogus/unknown/, best describes what it currently does.\n\nThanks.\n"},{"id":"259993","messageId":"553B78E2.5070608@gmail.com","threadId":"39085","inReplyTo":"xmqqh9s9gqw9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-25T11:22:10Z","receivedAt":"2015-04-25T11:22:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\nOn 04/22/2015 02:06 AM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n> > It's easy to be blinded into thinking that cat-file's new option\n> > should be named --literally since it was inspired by the --literally\n> > option of hash-object, but indeed it may not be the best choice.\n>\n> Yeah, I wouldn't even say \"inspired\". It was envisioned as a\n> counter-part debugging aid, nothing more.\n>\n> Is there any other way to make cat-file looser other than accepting\n> an unknown type name from the future? If so, then perhaps it may\n> make sense to give it a generic name that implys that we would\n> trigger such additional looseness in the future. But as the\n> inventor of it as a debugging aid, I would say a name that spells\n> out the specific way this option is being loose, e.g.\n>\n> > --allow-bogus-type\n>\n> or with s/bogus/unknown/, best describes what it currently does.\nYes this gives the best description, but its large, while we could use something\nlike --no-strict instead. Is the size worth the trade-off for a better description?\n>\n> Thanks.\n>\n>\n>\n"},{"id":"260001","messageId":"xmqqsiboyw90.fsf@gitster.dls.corp.google.com","threadId":"39085","inReplyTo":"553B78E2.5070608@gmail.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-25T17:04:59Z","receivedAt":"2015-04-25T17:04:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"karthik nayak writes:\n\n>> Is there any other way to make cat-file looser other than accepting\n>> an unknown type name from the future? If not, then perhaps it may\n>> make sense to give it a generic name that implies that we would\n>> trigger such additional looseness in the future. But as the\n>> inventor of it as a debugging aid, I would say a name that spells\n>> out the specific way this option is being loose, e.g.\n>>\n>> > --allow-bogus-type\n>>\n>> or with s/bogus/unknown/, best describes what it currently does.\n>\n> Yes this gives the best description, but its large, while we could use something\n> like --no-strict instead.\n\nWe could, if you answered my first question with \"no\".\n\nBy naming this \"--no-strict\", the first bug report you will receive\nmay be that \"cat-file --no-strict\" should parse a zlib deflate that\nbegins with \"blob 1234\\n\\0\" (notice that there are two SPs instead\nof the usual one, and length is followed by LF that should not be\nthere before the NUL) but it does not.\n\nAs your option name \"--no-strict\" signals that you will make the\nbest effort to parse such nonsense, that would be a valid bug\nreport, against which you would need to update the code to make it\nwork. But is it worth the effort to make such a thing work? I\ndunno.\n"},{"id":"260054","messageId":"553E2427.4090700@gmail.com","threadId":"39085","inReplyTo":"xmqqsiboyw90.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-27T11:57:27Z","receivedAt":"2015-04-27T11:57:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\nOn 04/25/2015 10:34 PM, Junio C Hamano wrote:\n> karthik nayak writes:\n>\n> >> Is there any other way to make cat-file looser other than accepting\n> >> an unknown type name from the future? If not, then perhaps it may\n> >> make sense to give it a generic name that implies that we would\n> >> trigger such additional looseness in the future. But as the\n> >> inventor of it as a debugging aid, I would say a name that spells\n> >> out the specific way this option is being loose, e.g.\n> >>\n> >>> --allow-bogus-type\n> >>\n> >> or with s/bogus/unknown/, best describes what it currently does.\n> >\n> > Yes this gives the best description, but its large, while we could use something\n> > like --no-strict instead.\n>\n> We could, if you answered my first question with \"no\".\n>\n> By naming this \"--no-strict\", the first bug report you will receive\n> may be that \"cat-file --no-strict\" should parse a zlib deflate that\n> begins with \"blob 1234\\n\\0\" (notice that there are two SPs instead\n> of the usual one, and length is followed by LF that should not be\n> there before the NUL) but it does not.\n>\n> As your option name \"--no-strict\" signals that you will make the\n> best effort to parse such nonsense, that would be a valid bug\n> report, against which you would need to update the code to make it\n> work. But is it worth the effort to make such a thing work? I\n> dunno.\n>\nNice point, I don't see the need to parse such objects at the moment.\nThat rules out \"--no-strict\" and everything similar.\n\nI still find \"--allow-unkown-type\" a bit too big, what about something like\n\n* --any-type\n* --arbitrary-type\n"},{"id":"260069","messageId":"CAPig+cTcAq_p3QXqcG+o1saWZyvDHCW=_JWYn6s7B1L4X5X1cQ@mail.gmail.com","threadId":"39085","inReplyTo":"553E2427.4090700@gmail.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-27T18:38:15Z","receivedAt":"2015-04-27T18:38:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 27, 2015 at 7:57 AM, karthik nayak wrote:\n> On 04/25/2015 10:34 PM, Junio C Hamano wrote:\n>> karthik nayak writes:\n>> > Yes this gives the best description, but its large, while we could use\n>> > something like --no-strict instead.\n>>\n>> We could, if you answered my first question with \"no\".\n>>\n>> By naming this \"--no-strict\", the first bug report you will receive\n>> may be that \"cat-file --no-strict\" should parse a zlib deflate that\n>> begins with \"blob 1234\\n\\0\" (notice that there are two SPs instead\n>> of the usual one, and length is followed by LF that should not be\n>> there before the NUL) but it does not.\n>>\n>> As your option name \"--no-strict\" signals that you will make the\n>> best effort to parse such nonsense, that would be a valid bug\n>> report, against which you would need to update the code to make it\n>> work. But is it worth the effort to make such a thing work? I\n>> dunno.\n>>\n> Nice point, I don't see the need to parse such objects at the moment.\n> That rules out \"--no-strict\" and everything similar.\n>\n> I still find \"--allow-unkown-type\" a bit too big, what about something like\n>\n> * --any-type\n> * --arbitrary-type\n\nAs a diagnostic aid when encountering a (hopefully rare) broken or\ncorrupt object, this option is not likely to be used often. The\n\"allow\" in --allow-unknown-type conveys the intended purpose more\naccurately than either of the shorter names suggested above; and\nconsidering how infrequently it is likely to be used, the extra six\ncharacters should not be a significant burden.\n"},{"id":"260123","messageId":"553F770F.9030106@gmail.com","threadId":"39085","inReplyTo":"CAPig+cTcAq_p3QXqcG+o1saWZyvDHCW=_JWYn6s7B1L4X5X1cQ@mail.gmail.com","subject":"Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-04-28T12:03:27Z","receivedAt":"2015-04-28T12:03:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"\nOn 04/28/2015 12:08 AM, Eric Sunshine wrote:\n> On Mon, Apr 27, 2015 at 7:57 AM, karthik nayak wrote:\n> > On 04/25/2015 10:34 PM, Junio C Hamano wrote:\n> >> karthik nayak writes:\n> >>> Yes this gives the best description, but its large, while we could use\n> >>> something like --no-strict instead.\n> >>\n> >> We could, if you answered my first question with \"no\".\n> >>\n> >> By naming this \"--no-strict\", the first bug report you will receive\n> >> may be that \"cat-file --no-strict\" should parse a zlib deflate that\n> >> begins with \"blob 1234\\n\\0\" (notice that there are two SPs instead\n> >> of the usual one, and length is followed by LF that should not be\n> >> there before the NUL) but it does not.\n> >>\n> >> As your option name \"--no-strict\" signals that you will make the\n> >> best effort to parse such nonsense, that would be a valid bug\n> >> report, against which you would need to update the code to make it\n> >> work. But is it worth the effort to make such a thing work? I\n> >> dunno.\n> >>\n> > Nice point, I don't see the need to parse such objects at the moment.\n> > That rules out \"--no-strict\" and everything similar.\n> >\n> > I still find \"--allow-unkown-type\" a bit too big, what about something like\n> >\n> > * --any-type\n> > * --arbitrary-type\n>\n> As a diagnostic aid when encountering a (hopefully rare) broken or\n> corrupt object, this option is not likely to be used often. The\n> \"allow\" in --allow-unknown-type conveys the intended purpose more\n> accurately than either of the shorter names suggested above; and\n> considering how infrequently it is likely to be used, the extra six\n> characters should not be a significant burden.\n>\nYou do have a point, thanks for putting it out. Will stick to \"--allow-unkown-type\".\n"}]}