{"thread":{"id":"50621","subject":"[PATCH v2] commit-tree: utilize parse-options api","startedAt":"2019-03-01T17:14:57Z","lastAt":"2019-03-02T03:35:52Z","messageCount":5,"participants":["Brandon Richardson","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"370422","messageId":"20190301171304.2267-1-brandon1024.br@gmail.com","threadId":"50621","inReplyTo":null,"subject":"[PATCH v2] commit-tree: utilize parse-options api","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-03-01T17:13:04Z","receivedAt":"2019-03-01T17:14:57Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"Rather 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 commit 70ddbd7767 (commit-tree: add missing\n--gpg-sign flag, 2019-01-19), and the existing implementation\nwould attempt to translate the option as a tree oid. It was also\nsuggested earlier in commit 55ca3f99ae (commit-tree: add and document\n--no-gpg-sign, 2013-12-13) 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 <brandon1024.br@gmail.com>\n---\n\nNotes:\n    GitHub Pull Request: https://github.com/brandon1024/git/pull/2\n    Travis CI Results: https://travis-ci.com/brandon1024/git/builds/102755598\n\n Documentation/git-commit-tree.txt |   8 +-\n builtin/commit-tree.c             | 159 ++++++++++++++++--------------\n parse-options.h                   |   9 ++\n 3 files changed, 102 insertions(+), 74 deletions(-)\n\ndiff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt\nindex 002dae625e..f4e20b62a0 100644\n--- a/Documentation/git-commit-tree.txt\n+++ b/Documentation/git-commit-tree.txt\n@@ -23,6 +23,9 @@ Creates a new commit object based on the provided tree object and\n emits the new commit object id on stdout. The log message is read\n from the standard input, unless `-m` or `-F` options are given.\n \n+When mixing `-m` and `-F` options, the commit log message will be\n+composed in the order in which the options are given.\n+\n A commit object may have any number of parents. With exactly one\n parent, it is an ordinary commit. Having more than one parent makes\n the commit a merge between several lines of history. Initial (root)\n@@ -41,7 +44,7 @@ state was.\n OPTIONS\n -------\n <tree>::\n-\tAn existing tree object\n+\tAn existing tree object.\n \n -p <parent>::\n \tEach `-p` indicates the id of a parent commit object.\n@@ -52,7 +55,8 @@ OPTIONS\n \n -F <file>::\n \tRead the commit log message from the given file. Use `-` to read\n-\tfrom the standard input.\n+\tfrom the standard input. This can be given more than once and the\n+\tcontent of each file becomes its own paragraph.\n \n -S[<keyid>]::\n --gpg-sign[=<keyid>]::\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 12cc403bd7..9a80e83f96 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 <sha1>)...] [-S[<keyid>]] [-m <message>] [-F <file>] <sha1>\";\n+static const char * const commit_tree_usage[] = {\n+\tN_(\"git commit-tree [(-p <parent>)...] [-S[<keyid>]] [(-m <message>)...] \"\n+\t\t\"[(-F <file>)...] <tree>\"),\n+\tNULL\n+};\n \n static const char *sign_commit;\n \n@@ -23,7 +29,7 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)\n \tstruct commit_list *parents;\n \tfor (parents = *parents_p; parents; parents = parents->next) {\n \t\tif (parents->item == parent) {\n-\t\t\terror(\"duplicate parent %s ignored\", oid_to_hex(oid));\n+\t\t\terror(_(\"duplicate parent %s ignored\"), oid_to_hex(oid));\n \t\t\treturn;\n \t\t}\n \t\tparents_p = &parents->next;\n@@ -39,91 +45,100 @@ 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_NOARG(unset, arg);\n+\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_NOARG(unset, arg);\n+\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 (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+\tstruct option options[] = {\n+\t\t{ OPTION_CALLBACK, 'p', NULL, &parents, N_(\"parent\"),\n+\t\t\tN_(\"id of a parent commit object\"), PARSE_OPT_NONEG,\n+\t\t\tparse_parent_arg_callback },\n+\t\t{ OPTION_CALLBACK, 'm', NULL, &buffer, N_(\"message\"),\n+\t\t\tN_(\"commit message\"), PARSE_OPT_NONEG,\n+\t\t\tparse_message_arg_callback },\n+\t\t{ OPTION_CALLBACK, 'F', NULL, &buffer, N_(\"file\"),\n+\t\t\tN_(\"read commit log message from file\"), PARSE_OPT_NONEG,\n+\t\t\tparse_file_arg_callback },\n+\t\t{ OPTION_STRING, 'S', \"gpg-sign\", &sign_commit, N_(\"key-id\"),\n+\t\t\tN_(\"GPG sign commit\"), PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n+\t\tOPT_END()\n+\t};\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(commit_tree_usage, options);\n \n-\t\tif (!strcmp(arg, \"--gpg-sign\")) {\n-\t\t    sign_commit = \"\";\n-\t\t    continue;\n-\t\t}\n+\targc = parse_options(argc, argv, prefix, options, commit_tree_usage, 0);\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+\tif (argc != 1)\n+\t\tdie(_(\"must give exactly one tree\"));\n \n-\t\tif (!strcmp(arg, \"--no-gpg-sign\")) {\n-\t\t\tsign_commit = NULL;\n-\t\t\tcontinue;\n-\t\t}\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-\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-\t\t\tdie_errno(\"git commit-tree: failed to read\");\n+\t\t\tdie_errno(_(\"git commit-tree: failed to read\"));\n \t}\n \n \tif (commit_tree(buffer.buf, buffer.len, &tree_oid, parents, &commit_oid,\ndiff --git a/parse-options.h b/parse-options.h\nindex 14fe32428e..a6ab338be3 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -202,6 +202,15 @@ const char *optname(const struct option *opt, int flags);\n \t\tBUG(\"option callback does not expect an argument\"); \\\n } while (0)\n \n+/*\n+ * Use this assertion for callbacks that expect to be called with NONEG,\n+ * and require an argument be supplied.\n+ */\n+#define BUG_ON_OPT_NEG_NOARG(unset, arg) do { \\\n+\tif((!unset) && (!arg)) \\\n+\t\tBUG(\"option callback does not expect negation and requires an argument\"); \\\n+} while(0)\n+\n /*----- incremental advanced APIs -----*/\n \n enum {\n-- \n2.21.0\n\n"},{"id":"370443","messageId":"20190301190954.GG30847@sigill.intra.peff.net","threadId":"50621","inReplyTo":"20190301171304.2267-1-brandon1024.br@gmail.com","subject":"Re: [PATCH v2] commit-tree: utilize parse-options api","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-01T19:09:55Z","receivedAt":"2019-03-01T19:09:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2019 at 01:13:04PM -0400, Brandon Richardson wrote:\n\n> +/*\n> + * Use this assertion for callbacks that expect to be called with NONEG,\n> + * and require an argument be supplied.\n> + */\n> +#define BUG_ON_OPT_NEG_NOARG(unset, arg) do { \\\n\nI think this general concept is fine. It's a variant of\nBUG_ON_OPT_NEG(), so you'd use one or the other.\n\nHowever, the implementation:\n\n> +\tif((!unset) && (!arg)) \\\n> +\t\tBUG(\"option callback does not expect negation and requires an argument\"); \\\n\ndoes not really make sense. If \"!unset\" is true, then we know that\n\"!arg\" will always be true as well. So this collapse down to \"!unset\",\nwhich is the same as BUG_ON_OPT_NEG().\n\nI think you want an \"OR\". Or even separate conditions, since really this\nis just implying OPT_NEG(). In fact, you could implement and explain it\nlike this:\n\ndiff --git a/parse-options.h b/parse-options.h\nindex 14fe32428e..d46f89305c 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -202,6 +202,18 @@ const char *optname(const struct option *opt, int flags);\n \t\tBUG(\"option callback does not expect an argument\"); \\\n } while (0)\n \n+/*\n+ * Similar to the assertions above, but checks that \"arg\" is always non-NULL.\n+ * I.e., that we expect the NOARG and OPTARG flags _not_ to be set. Since\n+ * negation is the other common cause of a NULL arg, this also implies\n+ * BUG_ON_OPT_NEG(), letting you declare both assertions in a single line.\n+ */\n+#define BUG_ON_OPT_NOARG(unset, arg) do { \\\n+\tBUG_ON_OPT_NEG(unset); \\\n+\tif (!(arg)) \\\n+\t\tBUG(\"option callback require an argument\"); \\\n+} while (0)\n+\n /*----- incremental advanced APIs -----*/\n \n enum {\n\n-Peff\n"},{"id":"370449","messageId":"CAPig+cQoZQCTAzaDiaAdAvSqHBHSoapDoVLjPtpKjCEVSBL57g@mail.gmail.com","threadId":"50621","inReplyTo":"20190301190954.GG30847@sigill.intra.peff.net","subject":"Re: [PATCH v2] commit-tree: utilize parse-options api","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-01T20:53:43Z","receivedAt":"2019-03-01T20:53:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 1, 2019 at 2:10 PM Jeff King <peff@peff.net> wrote:\n> On Fri, Mar 01, 2019 at 01:13:04PM -0400, Brandon Richardson wrote:\n> > +     if((!unset) && (!arg)) \\\n> > +             BUG(\"option callback does not expect negation and requires an argument\"); \\\n\nPeff didn't highlight this, but compare your use of macro arguments\nagainst his...\n\n> +/*\n> + * Similar to the assertions above, but checks that \"arg\" is always non-NULL.\n> + * I.e., that we expect the NOARG and OPTARG flags _not_ to be set. Since\n> + * negation is the other common cause of a NULL arg, this also implies\n> + * BUG_ON_OPT_NEG(), letting you declare both assertions in a single line.\n> + */\n> +#define BUG_ON_OPT_NOARG(unset, arg) do { \\\n> +       BUG_ON_OPT_NEG(unset); \\\n> +       if (!(arg)) \\\n> +               BUG(\"option callback require an argument\"); \\\n> +} while (0)\n\nNote, in particular how Peff used !(arg) rather than (!arg) in your\npatch. This distinction is subtle but important enough to warrant\nbeing called out. The reason that Peff did it this way (the _correct_\nway) is that, as a macro argument, 'arg' may be a complex expression\nrather than a simple boolean. for instance, a caller could conceivably\ninvoke the macro as:\n\n    BUG_ON_OPT_NOARG(unset, foo || bar)\n\nLet's say that 'foo' and 'bar' are both true. With Peff's version,\nwhen the macro is expanded, that expression becomes:\n\n    !(true || true)\n\nwhich evaluates to false as expected and intended. With your version,\nit expands to:\n\n    (!true || true)\n\nwhich evaluates to true (since ! has higher precedence than ||), which\nis a very different and very unexpected (and likely wrong) result.\n"},{"id":"370460","messageId":"CAETBDP6oX=NtiOux=5_5t3cj8Vb2QFyfTPmj1h4dOW2oa-hurA@mail.gmail.com","threadId":"50621","inReplyTo":"20190301190954.GG30847@sigill.intra.peff.net","subject":"Re: [PATCH v2] commit-tree: utilize parse-options api","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-03-02T03:29:38Z","receivedAt":"2019-03-02T03:30:08Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"On Fri, Mar 1, 2019 at 2:09 PM Jeff King <peff@peff.net> wrote:\n> I think you want an \"OR\". Or even separate conditions, since really this\n> is just implying OPT_NEG(). In fact, you could implement and explain it\n> like this:\n>\n> diff --git a/parse-options.h b/parse-options.h\n> index 14fe32428e..d46f89305c 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -202,6 +202,18 @@ const char *optname(const struct option *opt, int flags);\n>                 BUG(\"option callback does not expect an argument\"); \\\n>  } while (0)\n>\n> +/*\n> + * Similar to the assertions above, but checks that \"arg\" is always non-NULL.\n> + * I.e., that we expect the NOARG and OPTARG flags _not_ to be set. Since\n> + * negation is the other common cause of a NULL arg, this also implies\n> + * BUG_ON_OPT_NEG(), letting you declare both assertions in a single line.\n> + */\n> +#define BUG_ON_OPT_NOARG(unset, arg) do { \\\n> +       BUG_ON_OPT_NEG(unset); \\\n> +       if (!(arg)) \\\n> +               BUG(\"option callback require an argument\"); \\\n> +} while (0)\n> +\n>  /*----- incremental advanced APIs -----*/\n\nAhh yes. I had originally used ((!unset) || (!arg)), and second guessed myself\nbefore I submitted v2. However, I much prefer your solution which reuses\nBUG_ON_OPT_NEG(). I'll switch to that :-)\n\nBrandon\n"},{"id":"370461","messageId":"CAETBDP5vU7DAAT+qgr8bBukYy2YxABPXO0WTrr+gEfdw1_4mEg@mail.gmail.com","threadId":"50621","inReplyTo":"CAPig+cQoZQCTAzaDiaAdAvSqHBHSoapDoVLjPtpKjCEVSBL57g@mail.gmail.com","subject":"Re: [PATCH v2] commit-tree: utilize parse-options api","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-03-02T03:35:23Z","receivedAt":"2019-03-02T03:35:52Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"Hi Eric,\n\nOn Fri, Mar 1, 2019 at 3:53 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> Note, in particular how Peff used !(arg) rather than (!arg) in your\n> patch. This distinction is subtle but important enough to warrant\n> being called out. The reason that Peff did it this way (the _correct_\n> way) is that, as a macro argument, 'arg' may be a complex expression\n> rather than a simple boolean. for instance, a caller could conceivably\n> invoke the macro as:\n>\n>     BUG_ON_OPT_NOARG(unset, foo || bar)\n\nThanks for pointing this out. I caught this shortly after I submitted\nv2. I hadn't\nconsidered that the argument could be an expression. Will fix in v3.\n"}]}