{"thread":{"id":"50683","subject":"[PATCH v5] commit-tree: utilize parse-options api","startedAt":"2019-03-07T15:44:21Z","lastAt":"2019-03-07T15:44:21Z","messageCount":1,"participants":["Brandon Richardson"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"370913","messageId":"20190307154409.14808-1-brandon1024.br@gmail.com","threadId":"50683","inReplyTo":null,"subject":"[PATCH v5] commit-tree: utilize parse-options api","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-03-07T15:44:09Z","receivedAt":"2019-03-07T15:44:21Z","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/5\n    Travis CI Build: https://travis-ci.com/brandon1024/git/builds/103551319\n\n Documentation/git-commit-tree.txt |   9 +-\n builtin/commit-tree.c             | 158 ++++++++++++++++--------------\n parse-options.h                   |  11 +++\n 3 files changed, 104 insertions(+), 74 deletions(-)\n\ndiff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt\nindex 002dae625e..4b90b9c12a 100644\n--- a/Documentation/git-commit-tree.txt\n+++ b/Documentation/git-commit-tree.txt\n@@ -23,6 +23,10 @@ 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+The `-m` and `-F` options can be given any number of times, in any\n+order. The commit log message will be composed in the order in which\n+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 +45,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 +56,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"}]}