{"thread":{"id":"19344","subject":"Proposed patch for mktree [0/3]","startedAt":"2009-05-14T05:06:30Z","lastAt":"2009-05-15T16:57:35Z","messageCount":16,"participants":["Josh Micich","Junio C Hamano","Sverre Rabbelier","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"113874","messageId":"loom.20090514T050424-673@post.gmane.org","threadId":"19344","inReplyTo":null,"subject":"Proposed patch for mktree [0/3]","fromName":"Josh Micich","fromEmail":"josh.micich@gmail.com","sentAt":"2009-05-14T05:06:30Z","receivedAt":"2009-05-14T05:06:30Z","isPatch":false,"sender":{"key":"josh.micich@gmail.com","avatar":null},"body":"Here is a series of 3 patches to git-mktree in addition to the recent work done \non pu.\n\nThe first is just small fixes after the work on the '--missing' option.\nThe second patch adds a '--batch' option to mktree (similar to '--batch' in \ncat-file) that allows multiple tree objects to be created interactively by a \nsingle process.\nFinally there are some improvements to the validation of the type of object \nidentified by the tree entry sha1 (previously the sha1 was only used to check \nfor existence, not type).\n\n-josh\n\n\nJosh Micich (3):\n --missing option for mktree: re-added strbuf_release(&p_uq), Updated man page\n added --batch option to mk-tree\n improved validation of entry type in mktree\n\n Documentation/git-mktree.txt | 16 ++++++++--\n builtin-mktree.c | 63 +++++++++++++++++++++++++++++++----------\n 2 files changed, 60 insertions(+), 19 deletions(-)\n"},{"id":"113879","messageId":"loom.20090514T050742-791@post.gmane.org","threadId":"19344","inReplyTo":"loom.20090514T050424-673@post.gmane.org","subject":"[PATCH 1/3] '--missing' option for mktree: re-added strbuf_release(&p_uq), Updated man page","fromName":"Josh Micich","fromEmail":"josh.micich@gmail.com","sentAt":"2009-05-14T05:09:51Z","receivedAt":"2009-05-14T05:09:51Z","isPatch":true,"sender":{"key":"josh.micich@gmail.com","avatar":null},"body":"Re-added call to strbuf_release(&p_uq) that got lost in earlier changes.\nUpdated mktree_usage msg.\n\nUpdated man page to explain new '--missing' option.\nAlso clarified sorting behaviour.\n\nSigned-off-by: Josh Micich \n---\n Documentation/git-mktree.txt | 11 ++++++++---\n builtin-mktree.c | 5 ++++-\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-mktree.txt b/Documentation/git-mktree.txt\nindex af19f06..0461062 100644\n--- a/Documentation/git-mktree.txt\n+++ b/Documentation/git-mktree.txt\n@@ -8,18 +8,23 @@ git-mktree - Build a tree-object from ls-tree formatted text\n \n SYNOPSIS\n --------\n-'git mktree' [-z]\n+'git mktree' [-z] [--missing]\n \n DESCRIPTION\n -----------\n-Reads standard input in non-recursive `ls-tree` output format,\n-and creates a tree object. The object name of the tree object\n+Reads standard input in non-recursive `ls-tree` output format, and creates\n+a tree object. The order of the tree entries is normalised by mktree so\n+pre-sorting the input is not required. The object name of the tree object\n built is written to the standard output.\n \n OPTIONS\n -------\n -z::\n \tRead the NUL-terminated `ls-tree -z` output instead.\n+--missing::\n+\tAllow missing objects. The default behaviour (without this option)\n+\tis to verify that each tree entry's sha1 identifies an existing\n+\tobject.\n \n Author\n ------\ndiff --git a/builtin-mktree.c b/builtin-mktree.c\nindex e1c9a27..db647ce 100644\n--- a/builtin-mktree.c\n+++ b/builtin-mktree.c\n@@ -63,7 +63,7 @@ static void write_tree(unsigned char *sha1)\n }\n \n static const char *mktree_usage[] = {\n-\t\"git mktree [-z]\",\n+\t\"git mktree [-z] [--missing]\",\n \tNULL\n };\n \n@@ -112,6 +112,9 @@ static void mktree_line(char *buf, size_t len, int \nline_termination, int allow_m\n \t\tif (unquote_c_style(&p_uq, path, NULL))\n \t\t\tdie(\"invalid quoting\");\n \t\tpath = strbuf_detach(&p_uq, NULL);\n+\t\tappend_to_tree(mode, sha1, path);\n+\t\tstrbuf_release(&p_uq);\n+\t\treturn;\n \t}\n \tappend_to_tree(mode, sha1, path);\n }\n-- \n1.6.3.165.g2cce5.dirty\n"},{"id":"113877","messageId":"loom.20090514T051004-665@post.gmane.org","threadId":"19344","inReplyTo":"loom.20090514T050424-673@post.gmane.org","subject":"[PATCH 2/3] added --batch option to mktree","fromName":"Josh Micich","fromEmail":"josh.micich@gmail.com","sentAt":"2009-05-14T05:10:38Z","receivedAt":"2009-05-14T05:10:38Z","isPatch":true,"sender":{"key":"josh.micich@gmail.com","avatar":null},"body":"This option enables creation of many tree objects with a single process\n\nSigned-off-by: Josh Micich \n---\n Documentation/git-mktree.txt | 7 ++++++-\n builtin-mktree.c | 36 +++++++++++++++++++++++++++++-------\n 2 files changed, 35 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-mktree.txt b/Documentation/git-mktree.txt\nindex 0461062..3d2237a 100644\n--- a/Documentation/git-mktree.txt\n+++ b/Documentation/git-mktree.txt\n@@ -8,7 +8,7 @@ git-mktree - Build a tree-object from ls-tree formatted text\n \n SYNOPSIS\n --------\n-'git mktree' [-z] [--missing]\n+'git mktree' [-z] [--missing] [--batch]\n \n DESCRIPTION\n -----------\n@@ -25,6 +25,11 @@ OPTIONS\n \tAllow missing objects. The default behaviour (without this option)\n \tis to verify that each tree entry's sha1 identifies an existing\n \tobject.\n+--batch::\n+\tAllow building of more than one tree object before exiting. Each\n+\ttree is separated by as single blank line. The final new-line is\n+\toptional. Note - if the '-z' option is used, lines are terminated\n+\twith NUL.\n \n Author\n ------\ndiff --git a/builtin-mktree.c b/builtin-mktree.c\nindex db647ce..a56c917 100644\n--- a/builtin-mktree.c\n+++ b/builtin-mktree.c\n@@ -63,7 +63,7 @@ static void write_tree(unsigned char *sha1)\n }\n \n static const char *mktree_usage[] = {\n-\t\"git mktree [-z] [--missing]\",\n+\t\"git mktree [-z] [--missing] [--batch]\",\n \tNULL\n };\n \n@@ -125,20 +125,42 @@ int cmd_mktree(int ac, const char **av, const char \n*prefix)\n \tunsigned char sha1[20];\n \tint line_termination = '\\n';\n \tint allow_missing = 0;\n+\tint is_batch_mode = 0;\n+\n \tconst struct option option[] = {\n \t\tOPT_SET_INT('z', NULL, &line_termination, \"input is NUL \nterminated\", '\\0'),\n \t\tOPT_SET_INT( 0 , \"missing\", &allow_missing, \"allow missing \nobjects\", 1),\n+\t\tOPT_SET_INT( 0 , \"batch\", &is_batch_mode, \"interactively create \nmore than one tree\", 1),\n \t\tOPT_END()\n \t};\n \n \tac = parse_options(ac, av, option, mktree_usage, 0);\n \n-\twhile (strbuf_getline(&sb, stdin, line_termination) != EOF)\n-\t\tmktree_line(sb.buf, sb.len, line_termination, allow_missing);\n-\n+\tint got_eof = 0;\n+\twhile (!got_eof) {\n+\t\twhile (1) {\n+\t\t\tif (strbuf_getline(&sb, stdin, line_termination) == \nEOF) {\n+\t\t\t\tgot_eof = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (sb.buf[0] == '\\0') {\n+\t\t\t\t// empty lines denote tree boundaries in batch \nmode\n+\t\t\t\tif (is_batch_mode) {\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\tdie(\"input format error: (blank line only valid \nin batch mode)\");\n+\t\t\t}\n+\t\t\tmktree_line(sb.buf, sb.len, line_termination, \nallow_missing);\n+\t\t}\n+\t\tif (is_batch_mode && got_eof && used < 1) {\n+\t\t\t// allow input to finish with a new-line (or not)\n+\t\t} else {\n+\t\t\twrite_tree(sha1);\n+\t\t\tputs(sha1_to_hex(sha1));\n+\t\t\tfflush(stdout);\n+\t\t}\n+\t\tused=0; // reset tree entry buffer for re-use in batch mode\n+\t}\n \tstrbuf_release(&sb);\n-\n-\twrite_tree(sha1);\n-\tputs(sha1_to_hex(sha1));\n \texit(0);\n }\n-- \n1.6.3.165.g2cce5.dirty\n"},{"id":"113876","messageId":"loom.20090514T051052-920@post.gmane.org","threadId":"19344","inReplyTo":"loom.20090514T050424-673@post.gmane.org","subject":"[PATCH 3/3] improved validation of entry type in mktree","fromName":"Josh Micich","fromEmail":"josh.micich@gmail.com","sentAt":"2009-05-14T05:11:20Z","receivedAt":"2009-05-14T05:11:20Z","isPatch":true,"sender":{"key":"josh.micich@gmail.com","avatar":null},"body":"Previously mktree would accept tree entries which had a mismatch between the \ndeclared type and the actual type of object identified by the sha.\n\nSigned-off-by: Josh Micich \n---\n builtin-mktree.c | 34 +++++++++++++++++++++-------------\n 1 files changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-mktree.c b/builtin-mktree.c\nindex a56c917..a37954a 100644\n--- a/builtin-mktree.c\n+++ b/builtin-mktree.c\n@@ -71,7 +71,7 @@ static void mktree_line(char *buf, size_t len, int \nline_termination, int allow_m\n {\n \tchar *ptr, *ntr;\n \tunsigned mode;\n-\tenum object_type type;\n+\tenum object_type mode_type;\n \tchar *path;\n \tunsigned char sha1[20];\n \n@@ -94,29 +94,37 @@ static void mktree_line(char *buf, size_t len, int \nline_termination, int allow_m\n \tif (S_ISGITLINK(mode))\n \t\tallow_missing = 1;\n \n-\tif (!allow_missing)\n-\t\ttype = sha1_object_info(sha1, NULL);\n-\telse\n-\t\ttype = object_type(mode);\n-\n-\tif (type < 0)\n-\t\tdie(\"object %s unavailable\", sha1_to_hex(sha1));\n \n \t*ntr++ = 0; /* now at the beginning of SHA1 */\n-\tif (type != type_from_string(ptr))\n-\t\tdie(\"object type %s mismatch (%s)\", ptr, typename(type));\n \n \tpath = ntr + 41; /* at the beginning of name */\n+\tstruct strbuf p_uq = STRBUF_INIT;\n \tif (line_termination && path[0] == '\"') {\n-\t\tstruct strbuf p_uq = STRBUF_INIT;\n \t\tif (unquote_c_style(&p_uq, path, NULL))\n \t\t\tdie(\"invalid quoting\");\n \t\tpath = strbuf_detach(&p_uq, NULL);\n \t\tappend_to_tree(mode, sha1, path);\n-\t\tstrbuf_release(&p_uq);\n-\t\treturn;\n \t}\n+\n+\tmode_type = object_type(mode);\n+\tif (mode_type != type_from_string(ptr)) {\n+\t\tdie(\"entry '%s' object type (%s) doesn't match mode type (%s)\", \npath, ptr, typename(mode_type));\n+\t}\n+\n+\tenum object_type obj_type = sha1_object_info(sha1, NULL);\n+\tif (obj_type < 0) {\n+\t\tif (!allow_missing) {\n+\t\t\tdie(\"entry '%s' object %s is unavailable\", path, \nsha1_to_hex(sha1));\n+\t\t}\n+\t} else {\n+\t\tif (obj_type != mode_type) {\n+\t\t\tdie(\"entry '%s' object %s is a %s but specified type \nwas (%s)\",\n+\t\t\t\tpath, sha1_to_hex(sha1), typename(obj_type), \ntypename(mode_type));\n+\t\t}\n+\t}\n+\t\n \tappend_to_tree(mode, sha1, path);\n+\tstrbuf_release(&p_uq); // safe to call on unused empty buffer\n }\n \n int cmd_mktree(int ac, const char **av, const char *prefix)\n-- \n1.6.3.165.g2cce5.dirty\n"},{"id":"113883","messageId":"7vprecxlrw.fsf@alter.siamese.dyndns.org","threadId":"19344","inReplyTo":"loom.20090514T050742-791@post.gmane.org","subject":"Re: [PATCH 1/3] '--missing' option for mktree: re-added strbuf_release(&p_uq), Updated man page","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-14T06:04:51Z","receivedAt":"2009-05-14T06:04:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Micich writes:\n\n> DESCRIPTION\n> -----------\n> -Reads standard input in non-recursive `ls-tree` output format,\n> -and creates a tree object. The object name of the tree object\n> +Reads standard input in non-recursive `ls-tree` output format, and creates\n> +a tree object. The order of the tree entries is normalised by mktree so\n> +pre-sorting the input is not required. The object name of the tree object\n> built is written to the standard output.\n\nThanks ;-)\n\n> OPTIONS\n> -------\n> -z::\n> \tRead the NUL-terminated `ls-tree -z` output instead.\n> +--missing::\n> +\tAllow missing objects. The default behaviour (without this option)\n> +\tis to verify that each tree entry's sha1 identifies an existing\n> +\tobject.\n\nI'd suggest adding \"... except for gitlink entries (aka \"submodules\") that\nare always allowed to be missing\" here.\n\n> @@ -112,6 +112,9 @@ static void mktree_line(char *buf, size_t len, int \n> line_termination, int allow_m\n> \t\tif (unquote_c_style(&p_uq, path, NULL))\n> \t\t\tdie(\"invalid quoting\");\n> \t\tpath = strbuf_detach(&p_uq, NULL);\n> +\t\tappend_to_tree(mode, sha1, path);\n> +\t\tstrbuf_release(&p_uq);\n> +\t\treturn;\n> \t}\n> \tappend_to_tree(mode, sha1, path);\n> }\n\nEhh, why? detach already detaches the allocated buffer from strbuf and\nthere is nothing to clean up by strbuf_release().\n"},{"id":"113884","messageId":"7vhbzoxl5k.fsf@alter.siamese.dyndns.org","threadId":"19344","inReplyTo":"loom.20090514T051004-665@post.gmane.org","subject":"Re: [PATCH 2/3] added --batch option to mktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-14T06:18:15Z","receivedAt":"2009-05-14T06:18:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Micich writes:\n\n> This option enables creation of many tree objects with a single process\n\n... which is desirable in what way? how does this justify the cost of\nmaintenance? what is it used for?\n\n> DESCRIPTION\n> -----------\n> @@ -25,6 +25,11 @@ OPTIONS\n> \tAllow missing objects. The default behaviour (without this option)\n> \tis to verify that each tree entry's sha1 identifies an existing\n> \tobject.\n> +--batch::\n> +\tAllow building of more than one tree object before exiting. Each\n> +\ttree is separated by as single blank line. The final new-line is\n> +\toptional. Note - if the '-z' option is used, lines are terminated\n> +\twith NUL.\n\nI think you want an blank line before this.\n\n> Author\n> ------\n> diff --git a/builtin-mktree.c b/builtin-mktree.c\n> index db647ce..a56c917 100644\n> --- a/builtin-mktree.c\n> +++ b/builtin-mktree.c\n> @@ -63,7 +63,7 @@ static void write_tree(unsigned char *sha1)\n> }\n> \n> static const char *mktree_usage[] = {\n> -\t\"git mktree [-z] [--missing]\",\n> +\t\"git mktree [-z] [--missing] [--batch]\",\n> \tNULL\n> };\n> \n> @@ -125,20 +125,42 @@ int cmd_mktree(int ac, const char **av, const char \n> *prefix)\n\nLinewrapped and would not apply.\n\n> \tunsigned char sha1[20];\n> \tint line_termination = '\\n';\n> \tint allow_missing = 0;\n> +\tint is_batch_mode = 0;\n> +\n> \tconst struct option option[] = {\n> \t\tOPT_SET_INT('z', NULL, &line_termination, \"input is NUL \n> terminated\", '\\0'),\n> \t\tOPT_SET_INT( 0 , \"missing\", &allow_missing, \"allow missing \n> objects\", 1),\n> +\t\tOPT_SET_INT( 0 , \"batch\", &is_batch_mode, \"interactively create \n> more than one tree\", 1),\n\nWhat do you mean by \"interactively\"? Does anybody type from the standard\ninput?\n\n> \tac = parse_options(ac, av, option, mktree_usage, 0);\n> \n> -\twhile (strbuf_getline(&sb, stdin, line_termination) != EOF)\n> -\t\tmktree_line(sb.buf, sb.len, line_termination, allow_missing);\n> -\n> +\tint got_eof = 0;\n\nDecl-after-statement. \n\n> +\twhile (!got_eof) {\n> +\t\twhile (1) {\n> +\t\t\tif (strbuf_getline(&sb, stdin, line_termination) == \n> EOF) {\n> +\t\t\t\tgot_eof = 1;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t\tif (sb.buf[0] == '\\0') {\n> +\t\t\t\t// empty lines denote tree boundaries in batch \n> mode\n\nNo C++ comment please;\n\n> +\t\t\t\tif (is_batch_mode) {\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n\nLose excess {} pair;\n\n> +\t\tif (is_batch_mode && got_eof && used < 1) {\n> +\t\t\t// allow input to finish with a new-line (or not)\n\nStyle: have an explicit \";\" for an empty statement.\n\nBut more importantly, what does this comment mean? Why do you want to be\nloose in input format validation?\n\n> +\t\t} else {\n> +\t\t\twrite_tree(sha1);\n> +\t\t\tputs(sha1_to_hex(sha1));\n> +\t\t\tfflush(stdout);\n> +\t\t}\n> +\t\tused=0; // reset tree entry buffer for re-use in batch mode\n\n\tused = 0; /* .... */\n"},{"id":"113885","messageId":"7vd4acxkug.fsf@alter.siamese.dyndns.org","threadId":"19344","inReplyTo":"loom.20090514T051052-920@post.gmane.org","subject":"Re: [PATCH 3/3] improved validation of entry type in mktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-14T06:24:55Z","receivedAt":"2009-05-14T06:24:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Micich writes:\n\n> diff --git a/builtin-mktree.c b/builtin-mktree.c\n> index a56c917..a37954a 100644\n> --- a/builtin-mktree.c\n> +++ b/builtin-mktree.c\n> @@ -71,7 +71,7 @@ static void mktree_line(char *buf, size_t len, int \n> line_termination, int allow_m\n> {\n> \tchar *ptr, *ntr;\n> \tunsigned mode;\n> -\tenum object_type type;\n> +\tenum object_type mode_type;\n\nWhy?\n\n> @@ -94,29 +94,37 @@ static void mktree_line(char *buf, size_t len, int \n> line_termination, int allow_m\n> \tif (S_ISGITLINK(mode))\n> \t\tallow_missing = 1;\n> \n> -\tif (!allow_missing)\n> -\t\ttype = sha1_object_info(sha1, NULL);\n> -\telse\n> -\t\ttype = object_type(mode);\n> -\n> -\tif (type < 0)\n> -\t\tdie(\"object %s unavailable\", sha1_to_hex(sha1));\n> \n> \t*ntr++ = 0; /* now at the beginning of SHA1 */\n> -\tif (type != type_from_string(ptr))\n> -\t\tdie(\"object type %s mismatch (%s)\", ptr, typename(type));\n> \n> \tpath = ntr + 41; /* at the beginning of name */\n> +\tstruct strbuf p_uq = STRBUF_INIT;\n> \tif (line_termination && path[0] == '\"') {\n> -\t\tstruct strbuf p_uq = STRBUF_INIT;\n\nWhy make its lifetime longer even though you do not use it outside of this\nblock?\n\n> ...\n> \t}\n\n> +\n> +\tmode_type = object_type(mode);\n> +\tif (mode_type != type_from_string(ptr)) {\n> +\t\tdie(\"entry '%s' object type (%s) doesn't match mode type (%s)\", \n> path, ptr, typename(mode_type));\n> +\t}\n> +\n> +\tenum object_type obj_type = sha1_object_info(sha1, NULL);\n> +\tif (obj_type < 0) {\n> +\t\tif (!allow_missing) {\n\nThis is the other way around; when allow_missing is given you shouldn't\neven consult the object database to read it.\n"},{"id":"113912","messageId":"a644352c0905140224g41f645c1l734bee791656ea1d@mail.gmail.com","threadId":"19344","inReplyTo":"7vhbzoxl5k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] added --batch option to mktree","fromName":"Josh Micich","fromEmail":"josh.micich@gmail.com","sentAt":"2009-05-14T09:24:20Z","receivedAt":"2009-05-14T09:24:20Z","isPatch":true,"sender":{"key":"josh.micich@gmail.com","avatar":null},"body":"On Wed, May 13, 2009 at 11:18 PM, Junio C Hamano wrote:\n>> This option enables creation of many tree objects with a single process\n>\n>... which is desirable in what way? how does this justify the cost of\n>maintenance? what is it used for?\n\nI have been writing a few of my own utilities on top of basic git\ncommands and got mild performance gains from using the existing\n'--batch' option with \"git cat-file\" instead of executing new \"git\ncat-file -p \" processes for each object I wish to read. Later I\nwas writing a utility for mirroring another SCM (creates many trees),\nand I faced a similar performance issue with \"git mktree\". After\nadding the '--batch' option, I experienced about a 30% speed increase.\n\nIf you find this logic persuasive, I can fix all the other issues and\nre-submit this patch.\n\n>> +--batch::\n>I think you want an blank line before this.\nright\n\n\n>> @@ -125,20 +125,42 @@ int cmd_mktree(int ac, const char **av, const char\n>> *prefix)\n>\n>Linewrapped and would not apply.\nsorry - gmane web interface did that. BTW - does git have a coding\nstandard for maximum line length?\n\n\n>What do you mean by \"interactively\"? Does anybody type from the standard\n>input?\nI guess I was trying to stress the point that the created tree object\nids are output immediately as the complete text for each tree is\nreceived. Other (bad) applications I have seen wait for EOF on stdin\nbefore sending anything to stdout, and this is exactly what I had\navoided in the impl. Perhaps you could suggest better phrasing.\n\n\n>Decl-after-statement.\n>No C++ comment please;\n>Lose excess {} pair;\n>Style: have an explicit \";\" for an empty statement.\n>used = 0; /* .... */\nsorry - bad habits. gotta work out how to get my compiler to warn me about these\n\n\n>> + if (is_batch_mode && got_eof && used < 1) {\n>> + // allow input to finish with a new-line (or not)\n>\n>Style: have an explicit \";\" for an empty statement.\n>\n>But more importantly, what does this comment mean? Why do you want to be\n>loose in input format validation?\n\nI agree with your implied suggestion that tight input validation is\nbetter. I was actually trying to keep consistent with the way I\nbelieve mktree works today. The final \\n is optional as far as I can\ntell.\n"},{"id":"113917","messageId":"fabb9a1e0905140325g2f376cf1lcaf206278a942c47@mail.gmail.com","threadId":"19344","inReplyTo":"a644352c0905140224g41f645c1l734bee791656ea1d@mail.gmail.com","subject":"Re: [PATCH 2/3] added --batch option to mktree","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-05-14T10:25:41Z","receivedAt":"2009-05-14T10:25:41Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Thu, May 14, 2009 at 11:24, Josh Micich wrote:\n> If you find this logic persuasive, I can fix all the other issues and\n> re-submit this patch.\n\nI suspect Junio's point was more in the gist of \"you should explain\nthis in your commit message\". So if you do resend your patch, include\nthe above explanation (or at least the mention of the 30% speedup) in\nyour commit message.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"113953","messageId":"a644352c0905141244v4c8bdef6t69318152dc02c6a@mail.gmail.com","threadId":"19344","inReplyTo":"7vprecxlrw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] '--missing' option for mktree: re-added strbuf_release(&p_uq), Updated man page","fromName":"Josh Micich","fromEmail":"josh.micich@gmail.com","sentAt":"2009-05-14T19:44:10Z","receivedAt":"2009-05-14T19:44:10Z","isPatch":true,"sender":{"key":"josh.micich@gmail.com","avatar":null},"body":"On Wed, May 13, 2009 at 11:04 PM, Junio C Hamano wrote:\n>>               path = strbuf_detach(&p_uq, NULL);\n>> +             append_to_tree(mode, sha1, path);\n>> +             strbuf_release(&p_uq);\n>> +             return;\n>>       }\n>>       append_to_tree(mode, sha1, path);\n>>  }\n>\n> Ehh, why?  detach already detaches the allocated buffer from strbuf and\n> there is nothing to clean up by strbuf_release().\n>\nMy bad. While re-basing my work to fe0bb5, the strbuf_release() call\nwent missing, but I didn't notice you had also changed:\n- path = p_uq.buf;\n+ path = strbuf_detach(&p_uq, NULL);\nThat is much nicer\n"},{"id":"113955","messageId":"a644352c0905141251n168508eexa8f6ebf4d84c65bb@mail.gmail.com","threadId":"19344","inReplyTo":"fabb9a1e0905140325g2f376cf1lcaf206278a942c47@mail.gmail.com","subject":"Re: [PATCH 2/3] added --batch option to mktree","fromName":"Josh Micich","fromEmail":"josh.micich@gmail.com","sentAt":"2009-05-14T19:51:15Z","receivedAt":"2009-05-14T19:51:15Z","isPatch":true,"sender":{"key":"josh.micich@gmail.com","avatar":null},"body":"This option works in a similar way to the '--batch' option of 'git cat-file'.\nIt enables creation of many tree objects with a single process.\n\nThe change was motivated by performance considerations in applications that\nneed to create many tree objects. A non-rigorous test showed tree creation\ntimes improved from (roughly) 200ms to 50ms.\n\nSigned-off-by: Josh Micich \n---\n Documentation/git-mktree.txt | 8 +++++++-\n builtin-mktree.c | 40 +++++++++++++++++++++++++++++++++-------\n 2 files changed, 40 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-mktree.txt b/Documentation/git-mktree.txt\nindex 7336f48..81e3326 100644\n--- a/Documentation/git-mktree.txt\n+++ b/Documentation/git-mktree.txt\n@@ -8,7 +8,7 @@ git-mktree - Build a tree-object from ls-tree formatted text\n\n SYNOPSIS\n --------\n-'git mktree' [-z] [--missing]\n+'git mktree' [-z] [--missing] [--batch]\n\n DESCRIPTION\n -----------\n@@ -28,6 +28,12 @@ OPTIONS\n \tobject. This option has no effect on the treatment of gitlink entries\n \t(aka \"submodules\") which are always allowed to be missing.\n\n+--batch::\n+\tAllow building of more than one tree object before exiting. Each\n+\ttree is separated by as single blank line. The final new-line is\n+\toptional. Note - if the '-z' option is used, lines are terminated\n+\twith NUL.\n+\n Author\n ------\n Written by Junio C Hamano \ndiff --git a/builtin-mktree.c b/builtin-mktree.c\nindex 5ff0475..73b0abb 100644\n--- a/builtin-mktree.c\n+++ b/builtin-mktree.c\n@@ -63,7 +63,7 @@ static void write_tree(unsigned char *sha1)\n }\n\n static const char *mktree_usage[] = {\n-\t\"git mktree [-z] [--missing]\",\n+\t\"git mktree [-z] [--missing] [--batch]\",\n \tNULL\n };\n\n@@ -122,20 +122,46 @@ int cmd_mktree(int ac, const char **av, const\nchar *prefix)\n \tunsigned char sha1[20];\n \tint line_termination = '\\n';\n \tint allow_missing = 0;\n+\tint is_batch_mode = 0;\n+\tint got_eof = 0;\n+\n \tconst struct option option[] = {\n \t\tOPT_SET_INT('z', NULL, &line_termination, \"input is NUL terminated\", '\\0'),\n \t\tOPT_SET_INT( 0 , \"missing\", &allow_missing, \"allow missing objects\", 1),\n+\t\tOPT_SET_INT( 0 , \"batch\", &is_batch_mode, \"allow creation of more\nthan one tree\", 1),\n \t\tOPT_END()\n \t};\n\n \tac = parse_options(ac, av, option, mktree_usage, 0);\n\n-\twhile (strbuf_getline(&sb, stdin, line_termination) != EOF)\n-\t\tmktree_line(sb.buf, sb.len, line_termination, allow_missing);\n-\n+\twhile (!got_eof) {\n+\t\twhile (1) {\n+\t\t\tif (strbuf_getline(&sb, stdin, line_termination) == EOF) {\n+\t\t\t\tgot_eof = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (sb.buf[0] == '\\0') {\n+\t\t\t\t/* empty lines denote tree boundaries in batch mode */\n+\t\t\t\tif (is_batch_mode)\n+\t\t\t\t\tbreak;\n+\t\t\t\tdie(\"input format error: (blank line only valid in batch mode)\");\n+\t\t\t}\n+\t\t\tmktree_line(sb.buf, sb.len, line_termination, allow_missing);\n+\t\t}\n+\t\tif (is_batch_mode && got_eof && used < 1) {\n+\t\t\t/*\n+\t\t\t * Execution gets here if the last tree entry is terminated with a\n+\t\t\t * new-line. The final new-line has been made optional to be\n+\t\t\t * consistent with the original non-batch behaviour of mktree.\n+\t\t\t */\n+\t\t\t; /* skip creating an empty tree */\n+\t\t} else {\n+\t\t\twrite_tree(sha1);\n+\t\t\tputs(sha1_to_hex(sha1));\n+\t\t\tfflush(stdout);\n+\t\t}\n+\t\tused=0; /* reset tree entry buffer for re-use in batch mode */\n+\t}\n \tstrbuf_release(&sb);\n-\n-\twrite_tree(sha1);\n-\tputs(sha1_to_hex(sha1));\n \texit(0);\n }\n-- \n1.6.3.1.181.gfc9b3\n"},{"id":"113972","messageId":"a644352c0905141546w4193d45aq4940e72bbb4bda1c@mail.gmail.com","threadId":"19344","inReplyTo":"7vd4acxkug.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] improved validation of entry type in mktree","fromName":"Josh Micich","fromEmail":"josh.micich@gmail.com","sentAt":"2009-05-14T22:46:04Z","receivedAt":"2009-05-14T22:46:04Z","isPatch":true,"sender":{"key":"josh.micich@gmail.com","avatar":null},"body":"On Wed, May 13, 2009 at 11:24 PM, Junio C Hamano wrote:\n>> +     struct strbuf p_uq = STRBUF_INIT;\n>>       if (line_termination && path[0] == '\"') {\n>> -             struct strbuf p_uq = STRBUF_INIT;\n>\n> Why make its lifetime longer even though you do not use it outside of this\n> block?\n\nSorry, no need. Similar confusion cleared up in patch 1.\n\n>> +\tenum object_type obj_type = sha1_object_info(sha1, NULL);\n>> +     if (obj_type < 0) {\n>> +             if (!allow_missing) {\n>\n> This is the other way around; when allow_missing is given you shouldn't\n> even consult the object database to read it.\n\nI think mktree should verify the object types for objects that are available.\n\nFor example (given that 4b825d is empty tree and e69de2 is empty\nblob), mktree should reject the following entries:\n\n100644 blob 4b825dc642cb6eb9a060e54bf8d69288fbee4904\tfoo\n040000 tree e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\tbar\n\nI don't think the '--missing' option should cause mktree to accept a\ntree which is clearly invalid.\n\nFurthermore even with '--missing', a tree entry like this should be rejected:\n160000 commit e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 foo\n"},{"id":"113974","messageId":"a644352c0905141549q495973dfn664bdad80629ac17@mail.gmail.com","threadId":"19344","inReplyTo":"a644352c0905141546w4193d45aq4940e72bbb4bda1c@mail.gmail.com","subject":"Re: [PATCH 3/3] improved validation of entry type in mktree","fromName":"Josh Micich","fromEmail":"josh.micich@gmail.com","sentAt":"2009-05-14T22:49:10Z","receivedAt":"2009-05-14T22:49:10Z","isPatch":true,"sender":{"key":"josh.micich@gmail.com","avatar":null},"body":"Previously mktree would accept tree entries which had a mismatch\nbetween the declared type and the actual type of object identified by\nthe sha.\n\nSigned-off-by: Josh Micich \n---\n builtin-mktree.c | 43 +++++++++++++++++++++++++++++++++----------\n 1 files changed, 33 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-mktree.c b/builtin-mktree.c\nindex 73b0abb..dc4f1a7 100644\n--- a/builtin-mktree.c\n+++ b/builtin-mktree.c\n@@ -71,7 +71,8 @@ static void mktree_line(char *buf, size_t len, int\nline_termination, int allow_m\n {\n \tchar *ptr, *ntr;\n \tunsigned mode;\n-\tenum object_type type;\n+\tenum object_type mode_type; /* object type derived from mode */\n+\tenum object_type obj_type; /* object type derived from sha */\n \tchar *path;\n \tunsigned char sha1[20];\n\n@@ -94,17 +95,8 @@ static void mktree_line(char *buf, size_t len, int\nline_termination, int allow_m\n \tif (S_ISGITLINK(mode))\n \t\tallow_missing = 1;\n\n-\tif (!allow_missing)\n-\t\ttype = sha1_object_info(sha1, NULL);\n-\telse\n-\t\ttype = object_type(mode);\n-\n-\tif (type < 0)\n-\t\tdie(\"object %s unavailable\", sha1_to_hex(sha1));\n\n \t*ntr++ = 0; /* now at the beginning of SHA1 */\n-\tif (type != type_from_string(ptr))\n-\t\tdie(\"object type %s mismatch (%s)\", ptr, typename(type));\n\n \tpath = ntr + 41; /* at the beginning of name */\n \tif (line_termination && path[0] == '\"') {\n@@ -113,6 +105,37 @@ static void mktree_line(char *buf, size_t len,\nint line_termination, int allow_m\n \t\t\tdie(\"invalid quoting\");\n \t\tpath = strbuf_detach(&p_uq, NULL);\n \t}\n+\n+\t/*\n+\t * Object type is redundantly derivable three ways.\n+\t * These should all agree.\n+\t */\n+\tmode_type = object_type(mode);\n+\tif (mode_type != type_from_string(ptr)) {\n+\t\tdie(\"entry '%s' object type (%s) doesn't match mode type (%s)\",\n+\t\t\tpath, ptr, typename(mode_type));\n+\t}\n+\n+\t/* Check the type of object identified by sha1 */\n+\tobj_type = sha1_object_info(sha1, NULL);\n+\tif (obj_type < 0) {\n+\t\tif (allow_missing) {\n+\t\t\t; /* no problem - missing objects are presumed to be of the right type */\n+\t\t} else {\n+\t\t\tdie(\"entry '%s' object %s is unavailable\", path, sha1_to_hex(sha1));\n+\t\t}\n+\t} else {\n+\t\tif (obj_type != mode_type) {\n+\t\t\t/*\n+\t\t\t * The object exists but is of the wrong type.\n+\t\t\t * This is a problem regardless of allow_missing\n+\t\t\t * because the new tree entry will never be correct.\n+\t\t\t */\n+\t\t\tdie(\"entry '%s' object %s is a %s but specified type was (%s)\",\n+\t\t\t\tpath, sha1_to_hex(sha1), typename(obj_type), typename(mode_type));\n+\t\t}\n+\t}\n+\n \tappend_to_tree(mode, sha1, path);\n }\n\n-- \n1.6.3.1.181.gfc9b3.dirty\n"},{"id":"113985","messageId":"guj1pi$tbk$1@ger.gmane.org","threadId":"19344","inReplyTo":"a644352c0905141546w4193d45aq4940e72bbb4bda1c@mail.gmail.com","subject":"Re: [PATCH 3/3] improved validation of entry type in mktree","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-05-15T06:23:46Z","receivedAt":"2009-05-15T06:23:46Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Josh Micich wrote:\n\n> Furthermore even with '--missing', a tree entry like this should be rejected:\n> 160000 commit e69de29bb2d1d6434b8b29ae775ad8c2e48c5391  foo\n\nBut with submodules you might not _have_ e69de29b in object database\nto check its type!\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"113990","messageId":"7vskj6vkb9.fsf@alter.siamese.dyndns.org","threadId":"19344","inReplyTo":"a644352c0905140217h382a4d18h988b229c12577de3@mail.gmail.com","subject":"Re: [PATCH 2/3] added --batch option to mktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-15T08:31:38Z","receivedAt":"2009-05-15T08:31:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Micich writes:\n\n> On Wed, May 13, 2009 at 11:18 PM, Junio C Hamano wrote:\n>>> This option enables creation of many tree objects with a single process\n>>\n>>... which is desirable in what way? how does this justify the cost of\n>>maintenance? what is it used for?\n>\n> I have been writing a few of my own utilities on top of basic git commands\n> and got mild performance gains from using the existing '--batch' option with\n> \"git cat-file\" instead of executing new \"git cat-file -p \" processes\n> for each object I wish to read. Later I was writing a utility for mirroring\n> another SCM (creates many trees), and I faced a similar performance issue\n> with \"git mktree\". After adding the '--batch' option, I experienced about a\n> 30% speed increase.\n\n\"An operation that some people may want to do but the current toolset is\nnot very good at is _this_, and this patch rectifies that. _Here_ is a\nnumber to back up that claim.\" is a perfect way to justify your new\nfeature. Why not have that in the proposed commit log message ;-)?\n\n>>> + if (is_batch_mode && got_eof && used < 1) {\n>>> + // allow input to finish with a new-line (or not)\n>>\n>>Style: have an explicit \";\" for an empty statement.\n>>\n>>But more importantly, what does this comment mean? Why do you want to be\n>>loose in input format validation?\n>\n> I agree with your implied suggestion that tight input validation is better.\n> I was actually trying to keep consistent with the way I believe mktree\n> works today. The final \\n is optional as far as I can tell.\n\nOk.\n"},{"id":"114003","messageId":"loom.20090515T163644-832@post.gmane.org","threadId":"19344","inReplyTo":"guj1pi$tbk$1@ger.gmane.org","subject":"Re: [PATCH 3/3] improved validation of entry type in mktree","fromName":"Josh Micich","fromEmail":"josh.micich@gmail.com","sentAt":"2009-05-15T16:57:35Z","receivedAt":"2009-05-15T16:57:35Z","isPatch":true,"sender":{"key":"josh.micich@gmail.com","avatar":null},"body":"Jakub Narebski gmail.com> writes:\n\n> \n> Josh Micich wrote:\n> \n> > Furthermore even with '--missing', a tree entry like this should be \nrejected:\n> > 160000 commit e69de29bb2d1d6434b8b29ae775ad8c2e48c5391  foo\n> \n> But with submodules you might not _have_ e69de29b in object database\n> to check its type!\n\nThe code already accounted for that case. If the tree entry is a GIT_LINK \n(\"160000 commit ...\"), it is not mandatory for the referenced object to exist \nlocally. But if the object _does_ exist, the type should match. I guess I \nshould have prefixed that example with \"assuming object e69de29b (empty tree) \nis present in the local database\".\n"}]}