{"thread":{"id":"50658","subject":"[PATCH v4] commit-tree: utilize parse-options api","startedAt":"2019-03-05T15:50:14Z","lastAt":"2019-03-07T07:48:04Z","messageCount":4,"participants":["Brandon Richardson","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"370723","messageId":"20190305154951.4407-1-brandon1024.br@gmail.com","threadId":"50658","inReplyTo":null,"subject":"[PATCH v4] commit-tree: utilize parse-options api","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-03-05T15:49:51Z","receivedAt":"2019-03-05T15:50:14Z","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\nAlso update the documentation to better describe that mixing\n`-m` and `-F` options will correctly compose commit log messages in the\norder in which the options are given.\n\nIn the process, mark various strings for translation.\n\nSigned-off-by: Brandon Richardson <brandon1024.br@gmail.com>\n---\n\nNotes:\n    GitHub Pull Request: https://github.com/brandon1024/git/pull/4\n    Travis CI Build: https://travis-ci.com/brandon1024/git/builds/103055317\n\n Documentation/git-commit-tree.txt |   8 +-\n builtin/commit-tree.c             | 158 ++++++++++++++++--------------\n parse-options.h                   |  11 +++\n 3 files changed, 103 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..b866d83951 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -12,8 +12,13 @@\n #include \"builtin.h\"\n #include \"utf8.h\"\n #include \"gpg-interface.h\"\n+#include \"parse-options.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 +28,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 +44,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_NOARG(unset, arg);\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..3a442eee26 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -202,6 +202,17 @@ 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+ * This assertion also implies BUG_ON_OPT_NEG(), letting you declare both\n+ * assertions in a single line.\n+ */\n+#define BUG_ON_OPT_NEG_NOARG(unset, arg) do { \\\n+\tBUG_ON_OPT_NEG(unset); \\\n+\tif(!(arg)) \\\n+\t\tBUG(\"option callback expects an argument\"); \\\n+} while(0)\n+\n /*----- incremental advanced APIs -----*/\n \n enum {\n-- \n2.21.0\n\n"},{"id":"370837","messageId":"xmqqy35rpp13.fsf@gitster-ct.c.googlers.com","threadId":"50658","inReplyTo":"20190305154951.4407-1-brandon1024.br@gmail.com","subject":"Re: [PATCH v4] commit-tree: utilize parse-options api","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-06T23:21:44Z","receivedAt":"2019-03-06T23:21:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Richardson <brandon1024.br@gmail.com> writes:\n\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\nIt may be just me, but this new paragraph made me think that we can\ngive at most one -m and one -F option at the same time in any order,\nand multiple -m or -F options are not supported.  That, obviously,\nis not the impression we want to give to the readers.\n\nEven when you are not mixing -m and -F, but using -m more than once,\nthe log message will be composed in the order in which options are\ngiven.  So probably the word \"mixing\" is the primary culprit of\nmaking the sentence easier to be misunderstood.\n\n\tWhen using more than one `-m` or `-F` options, ...\n\nperhaps.\n\n> @@ -41,7 +44,7 @@ state was.\n>  OPTIONS\n>  -------\n>  <tree>::\n> -\tAn existing tree object\n> +\tAn existing tree object.\n\nGood.\n\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\nOK, this matches what -m says about giving it multiple times.\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\nReplacing a few placeholder tokens with more meaningful names---very\ngood attention to the detail.\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\nOK, this looks like a quite faithful conversion.  We do not allow\ntags that point at commit, for example.  Good.\n\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\nLikewise.  We make ourselves a new paragraph (if there is already\nsome message), add the message and complete the line.  Good.\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_NOARG(unset, arg);\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\nAgain, likewise.  And it is far easier to see and read what is going\non, compared to the original that has 2 extra levels of indentation.\n\n>  int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n>  {\n\nThe change to this main function looks quite straight-forward.  I am\nkind of surprised that a very low hanging fruit like this had survived\nwithout getting hit by parseopt a lot earlier ;-)\n"},{"id":"370856","messageId":"CAETBDP4MUN6pV2-xC=qsxnVynHuexOkU-nYbQ1OWeNGwBt3-Ng@mail.gmail.com","threadId":"50658","inReplyTo":"xmqqy35rpp13.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4] commit-tree: utilize parse-options api","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-03-07T01:52:55Z","receivedAt":"2019-03-07T01:53:25Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"On Wed, Mar 6, 2019 at 7:21 PM Junio C Hamano <gitster@pobox.com> wrote:\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> It may be just me, but this new paragraph made me think that we can\n> give at most one -m and one -F option at the same time in any order,\n> and multiple -m or -F options are not supported.  That, obviously,\n> is not the impression we want to give to the readers.\n>\n> Even when you are not mixing -m and -F, but using -m more than once,\n> the log message will be composed in the order in which options are\n> given.  So probably the word \"mixing\" is the primary culprit of\n> making the sentence easier to be misunderstood.\n>\n>         When using more than one `-m` or `-F` options, ...\n>\n> perhaps.\n\nGood call, 'mixing' is not the right word here. Will fix.\n\n> The change to this main function looks quite straight-forward.  I am\n> kind of surprised that a very low hanging fruit like this had survived\n> without getting hit by parseopt a lot earlier ;-)\n\nI was surprised too, commit-tree hasn't seen much love over the years.\nThere are certainly others that could benefit from parse-options.\n"},{"id":"370874","messageId":"CACsJy8DUGxUOko5fk5LLWKo_pmxAF=4NJxobNvGqWjTFWFNFiA@mail.gmail.com","threadId":"50658","inReplyTo":"xmqqy35rpp13.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4] commit-tree: utilize parse-options api","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-07T07:47:34Z","receivedAt":"2019-03-07T07:48:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 7, 2019 at 6:21 AM Junio C Hamano <gitster@pobox.com> wrote:\n> The change to this main function looks quite straight-forward.  I am\n> kind of surprised that a very low hanging fruit like this had survived\n> without getting hit by parseopt a lot earlier ;-)\n\nThere are more (I guess we tag #leftovers nowadays?)\n\ngit grep 'strcmp.*\\\"--[a-z]' builtin/\n\n(with some false positives)\n-- \nDuy\n"}]}