{"thread":{"id":"50598","subject":"[PATCH] commit-tree: utilize parse-options api","startedAt":"2019-02-26T20:10:02Z","lastAt":"2019-02-28T20:56:36Z","messageCount":14,"participants":["Brandon","Andrei Rybak","Brandon Richardson","Duy Nguyen","SZEDER Gábor","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"370269","messageId":"20190226200952.33950-1-brandon1024.br@gmail.com","threadId":"50598","inReplyTo":null,"subject":"[PATCH] commit-tree: utilize parse-options api","fromName":"Brandon","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-02-26T20:09:52Z","receivedAt":"2019-02-26T20:10:02Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"From: Brandon Richardson \n\nRather than parse options manually, which is both difficult to\nread and error prone, parse options supplied to commit-tree\nusing the parse-options api.\n\nIt was discovered that the --no-gpg-sign option was documented\nbut not implemented in 55ca3f99, and the existing implementation\nwould attempt to translate the option as a tree oid.It was also\nsuggested in 55ca3f99 that commit-tree should be migrated to\nutilize the parse-options api, which could help prevent mistakes\nlike this in the future. Hence this change.\n\nSigned-off-by: Brandon Richardson \n---\n\nNotes:\n GitHub Pull Request: https://github.com/brandon1024/git/pull/1\n Travis CI Results: https://travis-ci.com/brandon1024/git/builds/102337393\n\n builtin/commit-tree.c | 162 ++++++++++++++++++++++++------------------\n 1 file changed, 92 insertions(+), 70 deletions(-)\n\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 12cc403bd7..310f38d000 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -12,8 +12,14 @@\n #include \"builtin.h\"\n #include \"utf8.h\"\n #include \"gpg-interface.h\"\n+#include \"parse-options.h\"\n+#include \"string-list.h\"\n \n-static const char commit_tree_usage[] = \"git commit-tree [(-p )...] [-S[]] [-m ] [-F ] \";\n+static const char * const builtin_commit_tree_usage[] = {\n+\tN_(\"git commit-tree [(-p )...] [-S[]] [(-m )...] \"\n+\t\t\"[(-F )...] \"),\n+\tNULL\n+};\n \n static const char *sign_commit;\n \n@@ -39,87 +45,103 @@ static int commit_tree_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n+static int parse_parent_arg_callback(const struct option *opt,\n+\t\tconst char *arg, int unset)\n+{\n+\tstruct object_id oid;\n+\tstruct commit_list **parents = opt->value;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\n+\tif (!arg)\n+\t\treturn 1;\n+\tif (get_oid_commit(arg, &oid))\n+\t\tdie(\"Not a valid object name %s\", arg);\n+\n+\tassert_oid_type(&oid, OBJ_COMMIT);\n+\tnew_parent(lookup_commit(the_repository, &oid), parents);\n+\treturn 0;\n+}\n+\n+static int parse_message_arg_callback(const struct option *opt,\n+\t\tconst char *arg, int unset)\n+{\n+\tstruct strbuf *buf = opt->value;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\n+\tif (!arg)\n+\t\treturn 1;\n+\tif (buf->len)\n+\t\tstrbuf_addch(buf, '\\n');\n+\tstrbuf_addstr(buf, arg);\n+\tstrbuf_complete_line(buf);\n+\n+\treturn 0;\n+}\n+\n+static int parse_file_arg_callback(const struct option *opt,\n+\t\tconst char *arg, int unset)\n+{\n+\tint fd;\n+\tstruct strbuf *buf = opt->value;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\n+\tif (!arg)\n+\t\treturn 1;\n+\tif (buf->len)\n+\t\tstrbuf_addch(buf, '\\n');\n+\tif (!strcmp(arg, \"-\"))\n+\t\tfd = 0;\n+\telse {\n+\t\tfd = open(arg, O_RDONLY);\n+\t\tif (fd < 0)\n+\t\t\tdie_errno(\"git commit-tree: failed to open '%s'\", arg);\n+\t}\n+\tif (strbuf_read(buf, fd, 0) < 0)\n+\t\tdie_errno(\"git commit-tree: failed to read '%s'\", arg);\n+\tif (fd && close(fd))\n+\t\tdie_errno(\"git commit-tree: failed to close '%s'\", arg);\n+\n+\treturn 0;\n+}\n+\n int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n {\n-\tint i, got_tree = 0;\n+\tstatic struct strbuf buffer = STRBUF_INIT;\n \tstruct commit_list *parents = NULL;\n \tstruct object_id tree_oid;\n \tstruct object_id commit_oid;\n-\tstruct strbuf buffer = STRBUF_INIT;\n+\n+ struct option builtin_commit_tree_options[] = {\n+\t\t{ OPTION_CALLBACK, 'p', NULL, &parents, \"parent\",\n+\t\t N_(\"id of a parent commit object\"), PARSE_OPT_NONEG,\n+\t\t parse_parent_arg_callback },\n+\t\t{ OPTION_CALLBACK, 'm', NULL, &buffer, N_(\"message\"),\n+\t\t N_(\"commit message\"), PARSE_OPT_NONEG,\n+\t\t parse_message_arg_callback },\n+\t\t{ OPTION_CALLBACK, 'F', NULL, &buffer, N_(\"file\"),\n+\t\t N_(\"read commit log message from file\"), PARSE_OPT_NONEG,\n+\t\t parse_file_arg_callback },\n+\t\t{ OPTION_STRING, 'S', \"gpg-sign\", &sign_commit, N_(\"key-id\"),\n+\t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n+\t\tOPT_END()\n+ };\n \n \tgit_config(commit_tree_config, NULL);\n \n \tif (argc < 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage(commit_tree_usage);\n-\n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n-\t\tif (!strcmp(arg, \"-p\")) {\n-\t\t\tstruct object_id oid;\n-\t\t\tif (argc <= ++i)\n-\t\t\t\tusage(commit_tree_usage);\n-\t\t\tif (get_oid_commit(argv[i], &oid))\n-\t\t\t\tdie(\"Not a valid object name %s\", argv[i]);\n-\t\t\tassert_oid_type(&oid, OBJ_COMMIT);\n-\t\t\tnew_parent(lookup_commit(the_repository, &oid),\n-\t\t\t\t\t\t &parents);\n-\t\t\tcontinue;\n-\t\t}\n+\t\tusage_with_options(builtin_commit_tree_usage, builtin_commit_tree_options);\n \n-\t\tif (!strcmp(arg, \"--gpg-sign\")) {\n-\t\t sign_commit = \"\";\n-\t\t continue;\n-\t\t}\n-\n-\t\tif (skip_prefix(arg, \"-S\", &sign_commit) ||\n-\t\t\tskip_prefix(arg, \"--gpg-sign=\", &sign_commit))\n-\t\t\tcontinue;\n-\n-\t\tif (!strcmp(arg, \"--no-gpg-sign\")) {\n-\t\t\tsign_commit = NULL;\n-\t\t\tcontinue;\n-\t\t}\n+\targc = parse_options(argc, argv, prefix, builtin_commit_tree_options,\n+\t\t\tbuiltin_commit_tree_usage, 0);\n \n-\t\tif (!strcmp(arg, \"-m\")) {\n-\t\t\tif (argc <= ++i)\n-\t\t\t\tusage(commit_tree_usage);\n-\t\t\tif (buffer.len)\n-\t\t\t\tstrbuf_addch(&buffer, '\\n');\n-\t\t\tstrbuf_addstr(&buffer, argv[i]);\n-\t\t\tstrbuf_complete_line(&buffer);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!strcmp(arg, \"-F\")) {\n-\t\t\tint fd;\n-\n-\t\t\tif (argc <= ++i)\n-\t\t\t\tusage(commit_tree_usage);\n-\t\t\tif (buffer.len)\n-\t\t\t\tstrbuf_addch(&buffer, '\\n');\n-\t\t\tif (!strcmp(argv[i], \"-\"))\n-\t\t\t\tfd = 0;\n-\t\t\telse {\n-\t\t\t\tfd = open(argv[i], O_RDONLY);\n-\t\t\t\tif (fd < 0)\n-\t\t\t\t\tdie_errno(\"git commit-tree: failed to open '%s'\",\n-\t\t\t\t\t\t argv[i]);\n-\t\t\t}\n-\t\t\tif (strbuf_read(&buffer, fd, 0) < 0)\n-\t\t\t\tdie_errno(\"git commit-tree: failed to read '%s'\",\n-\t\t\t\t\t argv[i]);\n-\t\t\tif (fd && close(fd))\n-\t\t\t\tdie_errno(\"git commit-tree: failed to close '%s'\",\n-\t\t\t\t\t argv[i]);\n-\t\t\tcontinue;\n-\t\t}\n+\tif (argc != 1)\n+\t\tdie(\"Must give exactly one tree\");\n \n-\t\tif (get_oid_tree(arg, &tree_oid))\n-\t\t\tdie(\"Not a valid object name %s\", arg);\n-\t\tif (got_tree)\n-\t\t\tdie(\"Cannot give more than one trees\");\n-\t\tgot_tree = 1;\n-\t}\n+\tif (get_oid_tree(argv[0], &tree_oid))\n+\t\tdie(\"Not a valid object name %s\", argv[0]);\n \n \tif (!buffer.len) {\n \t\tif (strbuf_read(&buffer, 0, 0) < 0)\n-- \n2.21.0\n\n"},{"id":"370281","messageId":"33efa988-ea80-d9b4-f4aa-3876331a1dfb@gmail.com","threadId":"50598","inReplyTo":"20190226200952.33950-1-brandon1024.br@gmail.com","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-02-26T22:38:00Z","receivedAt":"2019-02-26T22:38:07Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"A couple of code style issues:\n\nOn 2/26/19 9:09 PM, Brandon wrote:\n> From: Brandon Richardson \n> \n> Rather than parse options manually, which is both difficult to\n> read and error prone, parse options supplied to commit-tree\n> using the parse-options api.\n> \n> It was discovered that the --no-gpg-sign option was documented\n> but not implemented in 55ca3f99, and the existing implementation\n> would attempt to translate the option as a tree oid.It was also\n\nMissing space after period.\n\n[snip]\n\n> +\n> int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n> {\n> -\tint i, got_tree = 0;\n> +\tstatic struct strbuf buffer = STRBUF_INIT;\n> \tstruct commit_list *parents = NULL;\n> \tstruct object_id tree_oid;\n> \tstruct object_id commit_oid;\n> -\tstruct strbuf buffer = STRBUF_INIT;\n> +\n> + struct option builtin_commit_tree_options[] = {\n\nStyle: tab should be used instead of four spaces.\n\n> +\t\t{ OPTION_CALLBACK, 'p', NULL, &parents, \"parent\",\n> +\t\t N_(\"id of a parent commit object\"), PARSE_OPT_NONEG,\n\nComparing to other similar places, a single tab should be used to\nalign \"N_\" instead of two spaces.\n\n> +\t\t parse_parent_arg_callback },\n> +\t\t{ OPTION_CALLBACK, 'm', NULL, &buffer, N_(\"message\"),\n> +\t\t N_(\"commit message\"), PARSE_OPT_NONEG,\n> +\t\t parse_message_arg_callback },\n> +\t\t{ OPTION_CALLBACK, 'F', NULL, &buffer, N_(\"file\"),\n> +\t\t N_(\"read commit log message from file\"), PARSE_OPT_NONEG,\n> +\t\t parse_file_arg_callback },\n> +\t\t{ OPTION_STRING, 'S', \"gpg-sign\", &sign_commit, N_(\"key-id\"),\n> +\t\t N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n> +\t\tOPT_END()\n> + };\n\n[snip]\n\n> -\n> -\t\tif (!strcmp(arg, \"--no-gpg-sign\")) {\n> -\t\t\tsign_commit = NULL;\n> -\t\t\tcontinue;\n> -\t\t}\n> +\targc = parse_options(argc, argv, prefix, builtin_commit_tree_options,\n> +\t\t\tbuiltin_commit_tree_usage, 0);\n\nhere \"builtin_commit_tree_usage\" should be aligned with \"argc\" in\nprevious line.\n\n"},{"id":"370285","messageId":"CAETBDP7AyYKE2gQY-HbP+LBhYwvf1QXt0-JaEQwnVyr=PjrKMw@mail.gmail.com","threadId":"50598","inReplyTo":"33efa988-ea80-d9b4-f4aa-3876331a1dfb@gmail.com","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-02-26T23:42:04Z","receivedAt":"2019-02-26T23:42:22Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"Hi Andrei,\n\n> > would attempt to translate the option as a tree oid.It was also\n>\n> Missing space after period.\n\nOops, thanks for pointing that out.\n\n>\n> > + { OPTION_CALLBACK, 'p', NULL, &parents, \"parent\",\n> > + N_(\"id of a parent commit object\"), PARSE_OPT_NONEG,\n>\n> Comparing to other similar places, a single tab should be used to\n> align \"N_\" instead of two spaces.\n\nI've seen a mix of both conventions scattered around, and wasn't sure which\nto stick to. I'll switch to that.\n\nThanks for your comments :-)\n"},{"id":"370294","messageId":"CACsJy8Bgz6FiTqnq8pnebuyOr55Bqh67iRhr6J+WvzgxPSBLhw@mail.gmail.com","threadId":"50598","inReplyTo":"20190226200952.33950-1-brandon1024.br@gmail.com","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-27T11:07:42Z","receivedAt":"2019-02-27T11:08:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 27, 2019 at 3:10 AM Brandon wrote:\n>\n> From: Brandon Richardson \n>\n> Rather than parse options manually, which is both difficult to\n> read and error prone, parse options supplied to commit-tree\n> using the parse-options api.\n>\n> It was discovered that the --no-gpg-sign option was documented\n> but not implemented in 55ca3f99, and the existing implementation\n\nMost people refer to a commit with this format\n\n55ca3f99ae (commit-tree: add and document --no-gpg-sign - 2013-12-13)\n\nIt gives the reader some context without actually looking at the\ncommit in question. And in the event that 55ca3f99 is ambiguous, it's\neasier to find the correct one.\n\n\n> would attempt to translate the option as a tree oid.It was also\n> suggested in 55ca3f99 that commit-tree should be migrated to\n> utilize the parse-options api, which could help prevent mistakes\n> like this in the future. Hence this change.\n>\n> Signed-off-by: Brandon Richardson \n> ---\n>\n> Notes:\n> GitHub Pull Request: https://github.com/brandon1024/git/pull/1\n> Travis CI Results: https://travis-ci.com/brandon1024/git/builds/102337393\n>\n> builtin/commit-tree.c | 162 ++++++++++++++++++++++++------------------\n> 1 file changed, 92 insertions(+), 70 deletions(-)\n>\n> diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\n> index 12cc403bd7..310f38d000 100644\n> --- a/builtin/commit-tree.c\n> +++ b/builtin/commit-tree.c\n> @@ -12,8 +12,14 @@\n> #include \"builtin.h\"\n> #include \"utf8.h\"\n> #include \"gpg-interface.h\"\n> +#include \"parse-options.h\"\n> +#include \"string-list.h\"\n>\n> -static const char commit_tree_usage[] = \"git commit-tree [(-p )...] [-S[]] [-m ] [-F ] \";\n> +static const char * const builtin_commit_tree_usage[] = {\n> + N_(\"git commit-tree [(-p )...] [-S[]] [(-m )...] \"\n> + \"[(-F )...] \"),\n> + NULL\n> +};\n>\n> static const char *sign_commit;\n>\n> @@ -39,87 +45,103 @@ static int commit_tree_config(const char *var, const char *value, void *cb)\n> return git_default_config(var, value, cb);\n> }\n>\n> +static int parse_parent_arg_callback(const struct option *opt,\n> + const char *arg, int unset)\n> +{\n> + struct object_id oid;\n> + struct commit_list **parents = opt->value;\n> +\n> + BUG_ON_OPT_NEG(unset);\n> +\n> + if (!arg)\n> + return 1;\n\nThis \"return 1;\" surprises me because I think we often just return 0\nor -1. I know !arg cannot happen here, so maybe just drop it. Or if\nyou want t play absolutely safe, maybe add a new macro like\n\nBUG_ON_NO_ARG(arg);\n\nwhich conveys the intention much better.\n\n> + if (get_oid_commit(arg, &oid))\n> + die(\"Not a valid object name %s\", arg);\n\nI'm asking extra so feel free to ignore. But maybe you could mark this\nstring for translation as well while we're here? Also these die()\nmessages should start with a lowercase because when printed, it is\nprefixed with \"fatal: \" so \"Not\" is not at the beginning of the\nsentence anymore. So...\n\ndie(_(\"not a valid object name %s\", arg);\n\nThe same comment for other error strings.\n\n> +\n> + assert_oid_type(&oid, OBJ_COMMIT);\n> + new_parent(lookup_commit(the_repository, &oid), parents);\n> + return 0;\n> +}\n> +\n> +static int parse_message_arg_callback(const struct option *opt,\n> + const char *arg, int unset)\n\nIn general we should try to avoid custom callbacks (more code, harder\nto understand...). Could we just use OPT_STRING_LIST() for handling\n-m?\n\nIf you do, then you'll collect all -m values in a string list and can\ndo the \\n completion after parse_options().\n\n> +{\n> + struct strbuf *buf = opt->value;\n> +\n> + BUG_ON_OPT_NEG(unset);\n> +\n> + if (!arg)\n> + return 1;\n> + if (buf->len)\n> + strbuf_addch(buf, '\\n');\n> + strbuf_addstr(buf, arg);\n> + strbuf_complete_line(buf);\n> +\n> + return 0;\n> +}\n> +\n> +static int parse_file_arg_callback(const struct option *opt,\n> + const char *arg, int unset)\n\nI would suggest you do the same for -F, i.e. collect a string list of\npaths then do the heavy lifting afterwards _IF_ we don't support\nmixing -m and -F. If we do, then we have to handle both in callbacks\nto make sure we compose the message correctly.\n\n> +{\n> + int fd;\n> + struct strbuf *buf = opt->value;\n> +\n> + BUG_ON_OPT_NEG(unset);\n> +\n> + if (!arg)\n> + return 1;\n> + if (buf->len)\n> + strbuf_addch(buf, '\\n');\n> + if (!strcmp(arg, \"-\"))\n> + fd = 0;\n> + else {\n> + fd = open(arg, O_RDONLY);\n> + if (fd < 0)\n> + die_errno(\"git commit-tree: failed to open '%s'\", arg);\n> + }\n> + if (strbuf_read(buf, fd, 0) < 0)\n> + die_errno(\"git commit-tree: failed to read '%s'\", arg);\n> + if (fd && close(fd))\n> + die_errno(\"git commit-tree: failed to close '%s'\", arg);\n> +\n> + return 0;\n> +}\n> +\n> int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n> {\n> - int i, got_tree = 0;\n> + static struct strbuf buffer = STRBUF_INIT;\n> struct commit_list *parents = NULL;\n> struct object_id tree_oid;\n> struct object_id commit_oid;\n> - struct strbuf buffer = STRBUF_INIT;\n> +\n> + struct option builtin_commit_tree_options[] = {\n\nIt's a local variable. I think we can just go with a shorter name like\n\"options\". Less to type later. Shorter lines.\n\n> + { OPTION_CALLBACK, 'p', NULL, &parents, \"parent\",\n\nWrap N_() around \"parent\" so it can be translated.\n\n> + N_(\"id of a parent commit object\"), PARSE_OPT_NONEG,\n> + parse_parent_arg_callback },\n> + { OPTION_CALLBACK, 'm', NULL, &buffer, N_(\"message\"),\n> + N_(\"commit message\"), PARSE_OPT_NONEG,\n> + parse_message_arg_callback },\n> + { OPTION_CALLBACK, 'F', NULL, &buffer, N_(\"file\"),\n> + N_(\"read commit log message from file\"), PARSE_OPT_NONEG,\n> + parse_file_arg_callback },\n> + { OPTION_STRING, 'S', \"gpg-sign\", &sign_commit, N_(\"key-id\"),\n> + N_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n\nAvoid raw struct declaration if possible. Will OPT_STRING() macro work?\n\n> + OPT_END()\n> + };\n\nI think you're using spaces here to indent instead of TABs.\n\n>\n> git_config(commit_tree_config, NULL);\n>\n> if (argc < 2 || !strcmp(argv[1], \"-h\"))\n> - usage(commit_tree_usage);\n> -\n> - for (i = 1; i < argc; i++) {\n> - const char *arg = argv[i];\n> - if (!strcmp(arg, \"-p\")) {\n> - struct object_id oid;\n> - if (argc <= ++i)\n> - usage(commit_tree_usage);\n> - if (get_oid_commit(argv[i], &oid))\n> - die(\"Not a valid object name %s\", argv[i]);\n> - assert_oid_type(&oid, OBJ_COMMIT);\n> - new_parent(lookup_commit(the_repository, &oid),\n> - &parents);\n> - continue;\n> - }\n> + usage_with_options(builtin_commit_tree_usage, builtin_commit_tree_options);\n>\n> - if (!strcmp(arg, \"--gpg-sign\")) {\n> - sign_commit = \"\";\n> - continue;\n> - }\n> -\n> - if (skip_prefix(arg, \"-S\", &sign_commit) ||\n> - skip_prefix(arg, \"--gpg-sign=\", &sign_commit))\n> - continue;\n> -\n> - if (!strcmp(arg, \"--no-gpg-sign\")) {\n> - sign_commit = NULL;\n> - continue;\n> - }\n> + argc = parse_options(argc, argv, prefix, builtin_commit_tree_options,\n> + builtin_commit_tree_usage, 0);\n>\n> - if (!strcmp(arg, \"-m\")) {\n> - if (argc <= ++i)\n> - usage(commit_tree_usage);\n> - if (buffer.len)\n> - strbuf_addch(&buffer, '\\n');\n> - strbuf_addstr(&buffer, argv[i]);\n> - strbuf_complete_line(&buffer);\n> - continue;\n> - }\n> -\n> - if (!strcmp(arg, \"-F\")) {\n> - int fd;\n> -\n> - if (argc <= ++i)\n> - usage(commit_tree_usage);\n> - if (buffer.len)\n> - strbuf_addch(&buffer, '\\n');\n> - if (!strcmp(argv[i], \"-\"))\n> - fd = 0;\n> - else {\n> - fd = open(argv[i], O_RDONLY);\n> - if (fd < 0)\n> - die_errno(\"git commit-tree: failed to open '%s'\",\n> - argv[i]);\n> - }\n> - if (strbuf_read(&buffer, fd, 0) < 0)\n> - die_errno(\"git commit-tree: failed to read '%s'\",\n> - argv[i]);\n> - if (fd && close(fd))\n> - die_errno(\"git commit-tree: failed to close '%s'\",\n> - argv[i]);\n> - continue;\n> - }\n> + if (argc != 1)\n> + die(\"Must give exactly one tree\");\n>\n> - if (get_oid_tree(arg, &tree_oid))\n> - die(\"Not a valid object name %s\", arg);\n> - if (got_tree)\n> - die(\"Cannot give more than one trees\");\n> - got_tree = 1;\n> - }\n> + if (get_oid_tree(argv[0], &tree_oid))\n> + die(\"Not a valid object name %s\", argv[0]);\n>\n> if (!buffer.len) {\n> if (strbuf_read(&buffer, 0, 0) < 0)\n> --\n> 2.21.0\n>\n\n\n-- \nDuy\n"},{"id":"370295","messageId":"CACsJy8BhEWRcEKRKpcBiZwewxYYoK0jipMZo9PeWAPCgDt1aNg@mail.gmail.com","threadId":"50598","inReplyTo":"CAETBDP7AyYKE2gQY-HbP+LBhYwvf1QXt0-JaEQwnVyr=PjrKMw@mail.gmail.com","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-27T11:13:07Z","receivedAt":"2019-02-27T11:13:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 27, 2019 at 6:43 AM Brandon Richardson\n wrote:\n>\n> Hi Andrei,\n>\n> > > would attempt to translate the option as a tree oid.It was also\n> >\n> > Missing space after period.\n>\n> Oops, thanks for pointing that out.\n>\n> >\n> > > + { OPTION_CALLBACK, 'p', NULL, &parents, \"parent\",\n> > > + N_(\"id of a parent commit object\"), PARSE_OPT_NONEG,\n> >\n> > Comparing to other similar places, a single tab should be used to\n> > align \"N_\" instead of two spaces.\n>\n> I've seen a mix of both conventions scattered around, and wasn't sure which\n> to stick to. I'll switch to that.\n\nI think we sometimes use spaces for fine alignment (search \"tabs and\nspaces\" in CodingGuidelines). It's really up to you and Andrei which\nstyle is preferred ;-)\n-- \nDuy\n"},{"id":"370296","messageId":"20190227113711.GF19739@szeder.dev","threadId":"50598","inReplyTo":"CACsJy8Bgz6FiTqnq8pnebuyOr55Bqh67iRhr6J+WvzgxPSBLhw@mail.gmail.com","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-27T11:37:11Z","receivedAt":"2019-02-27T11:37:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:\n> > It was discovered that the --no-gpg-sign option was documented\n> > but not implemented in 55ca3f99, and the existing implementation\n> \n> Most people refer to a commit with this format\n> \n> 55ca3f99ae (commit-tree: add and document --no-gpg-sign - 2013-12-13)\n\nNo, most often we use\n\n 55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13)\n\ni.e. with a comma instead of a dash between subject and short date;\nand without quotes around the subject.\n\nTruly sorry for nitpicking :)\n\nGábor\n\n"},{"id":"370297","messageId":"CACsJy8AcrRBtEUFtFVDUbDZDodDDMAHxnwsf55zH+TzKCoyVMw@mail.gmail.com","threadId":"50598","inReplyTo":"20190227113711.GF19739@szeder.dev","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-27T11:49:53Z","receivedAt":"2019-02-27T11:50:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 27, 2019 at 6:37 PM SZEDER Gábor wrote:\n>\n> On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:\n> > > It was discovered that the --no-gpg-sign option was documented\n> > > but not implemented in 55ca3f99, and the existing implementation\n> >\n> > Most people refer to a commit with this format\n> >\n> > 55ca3f99ae (commit-tree: add and document --no-gpg-sign - 2013-12-13)\n>\n> No, most often we use\n>\n> 55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13)\n>\n> i.e. with a comma instead of a dash between subject and short date;\n> and without quotes around the subject.\n>\n> Truly sorry for nitpicking :)\n\nNaah it's about time I update my ~/.gitconfig to be \"conformant\" :D I\nthink we both failed to mention where to find the command for Brandon\nthough: search commit-reference in SubmittingPatches.\n-- \nDuy\n"},{"id":"370300","messageId":"20190227123650.GG19739@szeder.dev","threadId":"50598","inReplyTo":"CACsJy8AcrRBtEUFtFVDUbDZDodDDMAHxnwsf55zH+TzKCoyVMw@mail.gmail.com","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-27T12:36:50Z","receivedAt":"2019-02-27T12:36:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Feb 27, 2019 at 06:49:53PM +0700, Duy Nguyen wrote:\n> On Wed, Feb 27, 2019 at 6:37 PM SZEDER Gábor wrote:\n> >\n> > On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:\n> > > > It was discovered that the --no-gpg-sign option was documented\n> > > > but not implemented in 55ca3f99, and the existing implementation\n> > >\n> > > Most people refer to a commit with this format\n> > >\n> > > 55ca3f99ae (commit-tree: add and document --no-gpg-sign - 2013-12-13)\n> >\n> > No, most often we use\n> >\n> > 55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13)\n> >\n> > i.e. with a comma instead of a dash between subject and short date;\n> > and without quotes around the subject.\n> >\n> > Truly sorry for nitpicking :)\n> \n> Naah it's about time I update my ~/.gitconfig to be \"conformant\" :D I\n> think we both failed to mention where to find the command for Brandon\n> though: search commit-reference in SubmittingPatches.\n\nWell, yes... but I didn't mention that on purpose: SubmittingPatches\nadvocates for quotes around the subject, which is still the less often\nused format of the two, and there is no good reason for those quotes\n(that 'deadbeef (' before and ', 2019-12-34)' after the subject\nprovide plenty of separation and indicate quite clearly what's going\non).\n\nHowever, looking at the length of the suggested command in\nSubmittingPatches made me remember that I've been using a couple of\npatches implementing 'git log --format=reference' for a couple of\nyears now... I wonder whether it would be worth having something like\nthat in git.git, and thus making it conveniently available for other\nprojects as well.\n\n"},{"id":"370314","messageId":"CAETBDP5pfuNP4JQDaxN613sthRziJT7CZd=tjhWLpMSME9JjOQ@mail.gmail.com","threadId":"50598","inReplyTo":"CACsJy8Bgz6FiTqnq8pnebuyOr55Bqh67iRhr6J+WvzgxPSBLhw@mail.gmail.com","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-02-27T15:24:46Z","receivedAt":"2019-02-27T15:25:00Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"Thank you all for the helpful comments :-)\n\nOn Wed, 27 Feb 2019 at 07:08, Duy Nguyen wrote:\n> > It was discovered that the --no-gpg-sign option was documented\n> > but not implemented in 55ca3f99, and the existing implementation\n>\n> Most people refer to a commit with this format\n>\n> 55ca3f99ae (commit-tree: add and document --no-gpg-sign - 2013-12-13)\n>\n> It gives the reader some context without actually looking at the\n> commit in question. And in the event that 55ca3f99 is ambiguous, it's\n> easier to find the correct one.\n\nI didn't know this, thank you for the tip. I'll start doing this from now on.\nI will also reread through the SubmittingPatches doc.\n\n> > +static int parse_parent_arg_callback(const struct option *opt,\n> > + const char *arg, int unset)\n> > +{\n> > + struct object_id oid;\n> > + struct commit_list **parents = opt->value;\n> > +\n> > + BUG_ON_OPT_NEG(unset);\n> > +\n> > + if (!arg)\n> > + return 1;\n>\n> This \"return 1;\" surprises me because I think we often just return 0\n> or -1. I know !arg cannot happen here, so maybe just drop it. Or if\n> you want t play absolutely safe, maybe add a new macro like\n>\n> BUG_ON_NO_ARG(arg);\n>\n> which conveys the intention much better.\n\nI like the BUG_ON_NO_ARG approach. I will go that route.\n\n> > +static int parse_file_arg_callback(const struct option *opt,\n> > + const char *arg, int unset)\n>\n> I would suggest you do the same for -F, i.e. collect a string list of\n> paths then do the heavy lifting afterwards _IF_ we don't support\n> mixing -m and -F. If we do, then we have to handle both in callbacks\n> to make sure we compose the message correctly.\n\nI opted to use callbacks here to allow mixing -m and -F so that messages\nare composed correctly, as you mentioned. I did so in an attempt to match\nthe existing functionality of commit-tree.\n\n>\n> > + OPT_END()\n> > + };\n>\n> I think you're using spaces here to indent instead of TABs.\n\nGood eye on the whitespace issue. I'm still dialling in my environment,\nso please forgive me.\n\nI will address all comments in a v2. Thanks again.\n\nBrandon\n"},{"id":"370322","messageId":"20190227163522.GA25188@sigill.intra.peff.net","threadId":"50598","inReplyTo":"CACsJy8Bgz6FiTqnq8pnebuyOr55Bqh67iRhr6J+WvzgxPSBLhw@mail.gmail.com","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-27T16:35:22Z","receivedAt":"2019-02-27T16:35:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:\n\n> > +static int parse_parent_arg_callback(const struct option *opt,\n> > + const char *arg, int unset)\n> > +{\n> > + struct object_id oid;\n> > + struct commit_list **parents = opt->value;\n> > +\n> > + BUG_ON_OPT_NEG(unset);\n> > +\n> > + if (!arg)\n> > + return 1;\n> \n> This \"return 1;\" surprises me because I think we often just return 0\n> or -1. I know !arg cannot happen here, so maybe just drop it. Or if\n> you want t play absolutely safe, maybe add a new macro like\n> \n> BUG_ON_NO_ARG(arg);\n> \n> which conveys the intention much better.\n\nI think it should be spelled BUG_ON_OPT_NOARG() to match the other ones.\n\nOne of the reasons I did not bother with that condition when I added the\nOPT_NEG() and OPT_ARG() variants is that you can only get an unexpected\nNULL argument if you explicitly give the NOARG or OPTARG flags. So it's\nvery easy to _forget_ to give such a flag, because you simply aren't\nthinking about that case, and your callback is buggy by default.\n\nBut it's rare to actually think to give one of those flags, but then\nforget to handle it in your callback.\n\nSo I'm not entirely opposed, but it does feel weird to add such a macro\nwithout then using it in the 99% of callbacks which expect arg to be\nnon-NULL.\n\nActually, there is one subtlety, which is that it can be NULL if \"unset\"\nis true. But then callbacks should already be looking at \"unset\" or\nusing BUG_ON_OPT_NEG(). But that just makes things worse. Take\nparse_opt_patchformat(), for example. It _does_ check \"unset\", so should\nnot use BUG_ON_OPT_NEG(). But if \"!unset\", it expects \"arg\" to be\nnon-NULL. So adding an assertion there turns our nice cascade of\nconditionals:\n\n if (unset)\n\t...handle unset...\n else if (!strcmp(arg, \"foo\"))\n\t...handle \"foo\"...\n ...and so on...\n\ninto:\n\n if (unset)\n\t...handle unset...\n else {\n\tBUG_ON_OPT_NOARG(arg);\n\tif (!strcmp, \"foo\"))\n\t\t....\n\t... and so on...\n }\n\nIf we are going to go this route, I think you might actually want macros\nthat take both \"unset\" and \"args\" and make sure that we're not in a\nsituation the callback doesn't expect (e.g., \"!unset && !arg\"). That\nlets us continue to declare those at the top of the callback.\n\nBut as you can see, it gets complicated quickly. I'm not really sure\nit's worth the trouble for a maintenance problem that's relatively\nunlikely.\n\n-Peff\n"},{"id":"370348","messageId":"CAETBDP42djjmSXeLig6mcRJVR0YMPnDUfCJT4z8SU==Ei62N4w@mail.gmail.com","threadId":"50598","inReplyTo":"20190227163522.GA25188@sigill.intra.peff.net","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-02-28T02:46:49Z","receivedAt":"2019-02-28T02:47:04Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"Hi Jeff,\n\n> One of the reasons I did not bother with that condition when I added the\n> OPT_NEG() and OPT_ARG() variants is that you can only get an unexpected\n> NULL argument if you explicitly give the NOARG or OPTARG flags. So it's\n> very easy to _forget_ to give such a flag, because you simply aren't\n> thinking about that case, and your callback is buggy by default.\n>\n> But it's rare to actually think to give one of those flags, but then\n> forget to handle it in your callback.\n>\n> So I'm not entirely opposed, but it does feel weird to add such a macro\n> without then using it in the 99% of callbacks which expect arg to be\n> non-NULL.\n\nI'd like to agree with you here, especially given that commit-tree is a rather\nsmall part of project source. Experimenting with it a bit, I found using\nBUG_ON_OPT_NOARG() to be a big clunky. Like you said, we could\nend up with some less-than-ideal usage. If I were to use this in commit-tree,\nit would look something like this, which isn't very appealing:\n\nstatic int callback(const struct option *opt, const char *arg, int unset)\n{\n ...\n BUG_ON_OPT_NEG(unset);\n BUG_ON_OPT_NO_ARG(arg);\n ...\n\nHowever, I do still see a use case for a new macro for options that cannot\nbe unset and arguments that must not be NULL.\n\n> If we are going to go this route, I think you might actually want macros\n> that take both \"unset\" and \"args\" and make sure that we're not in a\n> situation the callback doesn't expect (e.g., \"!unset && !arg\"). That\n> lets us continue to declare those at the top of the callback.\n\nIn doing a quick search, I found a fair number instances of this:\n...\nBUG_ON_OPT_NEG(unset);\n\nif (!arg)\n return -1;\n...\n\nSo a macro like this could be useful. I've also found a few instances of this:\n\nBUG_ON_OPT_NEG(unset);\nBUG_ON_OPT_ARG(arg);\n\nPerhaps two new macros BUG_ON_OPT_NEG_NO_ARG() (\"!unset || !arg\")\nand BUG_ON_OPT_NEG_ARG() (\"!unset || arg\")? I'm not a big fan of those\nnames though.\n\nBrandon\n"},{"id":"370351","messageId":"CACsJy8DEqbZMW+vgZJ=FizP2agUmONrQBe9MrhRefc3qFsh0iw@mail.gmail.com","threadId":"50598","inReplyTo":"20190227123650.GG19739@szeder.dev","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-28T07:21:27Z","receivedAt":"2019-02-28T07:21:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 27, 2019 at 7:36 PM SZEDER Gábor wrote:\n>\n> On Wed, Feb 27, 2019 at 06:49:53PM +0700, Duy Nguyen wrote:\n> > On Wed, Feb 27, 2019 at 6:37 PM SZEDER Gábor wrote:\n> > >\n> > > On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:\n> > > > > It was discovered that the --no-gpg-sign option was documented\n> > > > > but not implemented in 55ca3f99, and the existing implementation\n> > > >\n> > > > Most people refer to a commit with this format\n> > > >\n> > > > 55ca3f99ae (commit-tree: add and document --no-gpg-sign - 2013-12-13)\n> > >\n> > > No, most often we use\n> > >\n> > > 55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13)\n> > >\n> > > i.e. with a comma instead of a dash between subject and short date;\n> > > and without quotes around the subject.\n> > >\n> > > Truly sorry for nitpicking :)\n> >\n> > Naah it's about time I update my ~/.gitconfig to be \"conformant\" :D I\n> > think we both failed to mention where to find the command for Brandon\n> > though: search commit-reference in SubmittingPatches.\n>\n> Well, yes... but I didn't mention that on purpose: SubmittingPatches\n> advocates for quotes around the subject, which is still the less often\n> used format of the two, and there is no good reason for those quotes\n> (that 'deadbeef (' before and ', 2019-12-34)' after the subject\n> provide plenty of separation and indicate quite clearly what's going\n> on).\n\nPerhaps a patch to strip those quotes from the command in SubmittingPatches?\n\n> However, looking at the length of the suggested command in\n> SubmittingPatches made me remember that I've been using a couple of\n> patches implementing 'git log --format=reference' for a couple of\n> years now... I wonder whether it would be worth having something like\n> that in git.git, and thus making it conveniently available for other\n> projects as well.\n\nIt does sound nice to have something like this built in. But I'm not\nsure if \"git log\" would be the right place. For handling single\nrevisions (most often the case), git-show or git-rev-parse might be\nthe better interface. Even for referencing multiple hashes at the same\ntime (e.g. you prepare a text with bare hashes first, then run some\nprogram to insert the \"(..)\" part) then name-rev might be a better\ncandidate.\n\nA softer route to avoid any of that is simply adding default config\n\"pretty.reference\", then let the user define their own alias that uses\n--pretty=reference. Or perhaps just put it in EXAMPLES section of\ngit-log.\n-- \nDuy\n"},{"id":"370352","messageId":"CACsJy8D99BYRaWR+95VzM1gyhENje4N=HBNLJ=AA-op+y4yu2A@mail.gmail.com","threadId":"50598","inReplyTo":"CAETBDP5pfuNP4JQDaxN613sthRziJT7CZd=tjhWLpMSME9JjOQ@mail.gmail.com","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-28T07:26:22Z","receivedAt":"2019-02-28T07:26:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 27, 2019 at 10:24 PM Brandon Richardson\n wrote:\n> > > +static int parse_file_arg_callback(const struct option *opt,\n> > > + const char *arg, int unset)\n> >\n> > I would suggest you do the same for -F, i.e. collect a string list of\n> > paths then do the heavy lifting afterwards _IF_ we don't support\n> > mixing -m and -F. If we do, then we have to handle both in callbacks\n> > to make sure we compose the message correctly.\n>\n> I opted to use callbacks here to allow mixing -m and -F so that messages\n> are composed correctly, as you mentioned. I did so in an attempt to match\n> the existing functionality of commit-tree.\n\nFair enough. Probably safest to do that anyway.\n\nIf you feel like doing some improvements, maybe mention this behavior\nin git-commit-tree.txt too. It does say -m can be used multiple times,\nbut nothing explicit about -F (and I wonder if -F also does the\n\"becomes its own paragraph\" like -m). Also mixing -m and -F\ntechnically could be inferred from the synopsis line, but it's just\neasier to read an plain English sentence.\n-- \nDuy\n"},{"id":"370376","messageId":"20190228205633.GA12199@sigill.intra.peff.net","threadId":"50598","inReplyTo":"CAETBDP42djjmSXeLig6mcRJVR0YMPnDUfCJT4z8SU==Ei62N4w@mail.gmail.com","subject":"Re: [PATCH] commit-tree: utilize parse-options api","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-28T20:56:33Z","receivedAt":"2019-02-28T20:56:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 27, 2019 at 10:46:49PM -0400, Brandon Richardson wrote:\n\n> > If we are going to go this route, I think you might actually want macros\n> > that take both \"unset\" and \"args\" and make sure that we're not in a\n> > situation the callback doesn't expect (e.g., \"!unset && !arg\"). That\n> > lets us continue to declare those at the top of the callback.\n> \n> In doing a quick search, I found a fair number instances of this:\n> ...\n> BUG_ON_OPT_NEG(unset);\n> \n> if (!arg)\n> return -1;\n> ...\n\nThose are probably my fault. The originals guarded against an unexpected\n\"unset\" by checking \"!arg\" and returning an error. But it made the\ncompiler's -Wunused-parameter complain, so I added the BUG_ON_OPT_NEG()\ncalls as an assertion. At that point the \"if (!arg)\" could never\ntrigger, and could have been removed.\n\n> So a macro like this could be useful. I've also found a few instances of this:\n> \n> BUG_ON_OPT_NEG(unset);\n> BUG_ON_OPT_ARG(arg);\n\nThese ones are different. The second one is checking that \"arg\" _is_\nNULL (i.e., we expect that the options struct provided the right flag to\ndisallow an argument). And that's orthogonal to the unset flag, so it\nwould not be right to conflate the two in a single macro.\n\n-Peff\n"}]}