On Wed, Feb 27, 2019 at 3:10 AM Brandon <brandon1024.br@xxxxxxxxx> wrote: > > From: Brandon Richardson <brandon1024.br@xxxxxxxxx> > > Rather than parse options manually, which is both difficult to > read and error prone, parse options supplied to commit-tree > using the parse-options api. > > It was discovered that the --no-gpg-sign option was documented > but not implemented in 55ca3f99, and the existing implementation Most people refer to a commit with this format 55ca3f99ae (commit-tree: add and document --no-gpg-sign - 2013-12-13) It gives the reader some context without actually looking at the commit in question. And in the event that 55ca3f99 is ambiguous, it's easier to find the correct one. > would attempt to translate the option as a tree oid.It was also > suggested in 55ca3f99 that commit-tree should be migrated to > utilize the parse-options api, which could help prevent mistakes > like this in the future. Hence this change. > > Signed-off-by: Brandon Richardson <brandon1024.br@xxxxxxxxx> > --- > > Notes: > GitHub Pull Request: https://github.com/brandon1024/git/pull/1 > Travis CI Results: https://travis-ci.com/brandon1024/git/builds/102337393 > > builtin/commit-tree.c | 162 ++++++++++++++++++++++++------------------ > 1 file changed, 92 insertions(+), 70 deletions(-) > > diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c > index 12cc403bd7..310f38d000 100644 > --- a/builtin/commit-tree.c > +++ b/builtin/commit-tree.c > @@ -12,8 +12,14 @@ > #include "builtin.h" > #include "utf8.h" > #include "gpg-interface.h" > +#include "parse-options.h" > +#include "string-list.h" > > -static const char commit_tree_usage[] = "git commit-tree [(-p <sha1>)...] [-S[<keyid>]] [-m <message>] [-F <file>] <sha1>"; > +static const char * const builtin_commit_tree_usage[] = { > + N_("git commit-tree [(-p <parent>)...] [-S[<keyid>]] [(-m <message>)...] " > + "[(-F <file>)...] <tree>"), > + NULL > +}; > > static const char *sign_commit; > > @@ -39,87 +45,103 @@ static int commit_tree_config(const char *var, const char *value, void *cb) > return git_default_config(var, value, cb); > } > > +static int parse_parent_arg_callback(const struct option *opt, > + const char *arg, int unset) > +{ > + struct object_id oid; > + struct commit_list **parents = opt->value; > + > + BUG_ON_OPT_NEG(unset); > + > + if (!arg) > + return 1; This "return 1;" surprises me because I think we often just return 0 or -1. I know !arg cannot happen here, so maybe just drop it. Or if you want t play absolutely safe, maybe add a new macro like BUG_ON_NO_ARG(arg); which conveys the intention much better. > + if (get_oid_commit(arg, &oid)) > + die("Not a valid object name %s", arg); I'm asking extra so feel free to ignore. But maybe you could mark this string for translation as well while we're here? Also these die() messages should start with a lowercase because when printed, it is prefixed with "fatal: " so "Not" is not at the beginning of the sentence anymore. So... die(_("not a valid object name %s", arg); The same comment for other error strings. > + > + assert_oid_type(&oid, OBJ_COMMIT); > + new_parent(lookup_commit(the_repository, &oid), parents); > + return 0; > +} > + > +static int parse_message_arg_callback(const struct option *opt, > + const char *arg, int unset) In general we should try to avoid custom callbacks (more code, harder to understand...). Could we just use OPT_STRING_LIST() for handling -m? If you do, then you'll collect all -m values in a string list and can do the \n completion after parse_options(). > +{ > + struct strbuf *buf = opt->value; > + > + BUG_ON_OPT_NEG(unset); > + > + if (!arg) > + return 1; > + if (buf->len) > + strbuf_addch(buf, '\n'); > + strbuf_addstr(buf, arg); > + strbuf_complete_line(buf); > + > + return 0; > +} > + > +static int parse_file_arg_callback(const struct option *opt, > + const char *arg, int unset) I would suggest you do the same for -F, i.e. collect a string list of paths then do the heavy lifting afterwards _IF_ we don't support mixing -m and -F. If we do, then we have to handle both in callbacks to make sure we compose the message correctly. > +{ > + int fd; > + struct strbuf *buf = opt->value; > + > + BUG_ON_OPT_NEG(unset); > + > + if (!arg) > + return 1; > + if (buf->len) > + strbuf_addch(buf, '\n'); > + if (!strcmp(arg, "-")) > + fd = 0; > + else { > + fd = open(arg, O_RDONLY); > + if (fd < 0) > + die_errno("git commit-tree: failed to open '%s'", arg); > + } > + if (strbuf_read(buf, fd, 0) < 0) > + die_errno("git commit-tree: failed to read '%s'", arg); > + if (fd && close(fd)) > + die_errno("git commit-tree: failed to close '%s'", arg); > + > + return 0; > +} > + > int cmd_commit_tree(int argc, const char **argv, const char *prefix) > { > - int i, got_tree = 0; > + static struct strbuf buffer = STRBUF_INIT; > struct commit_list *parents = NULL; > struct object_id tree_oid; > struct object_id commit_oid; > - struct strbuf buffer = STRBUF_INIT; > + > + struct option builtin_commit_tree_options[] = { It's a local variable. I think we can just go with a shorter name like "options". Less to type later. Shorter lines. > + { OPTION_CALLBACK, 'p', NULL, &parents, "parent", Wrap N_() around "parent" so it can be translated. > + N_("id of a parent commit object"), PARSE_OPT_NONEG, > + parse_parent_arg_callback }, > + { OPTION_CALLBACK, 'm', NULL, &buffer, N_("message"), > + N_("commit message"), PARSE_OPT_NONEG, > + parse_message_arg_callback }, > + { OPTION_CALLBACK, 'F', NULL, &buffer, N_("file"), > + N_("read commit log message from file"), PARSE_OPT_NONEG, > + parse_file_arg_callback }, > + { OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"), > + N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, Avoid raw struct declaration if possible. Will OPT_STRING() macro work? > + OPT_END() > + }; I think you're using spaces here to indent instead of TABs. > > git_config(commit_tree_config, NULL); > > if (argc < 2 || !strcmp(argv[1], "-h")) > - usage(commit_tree_usage); > - > - for (i = 1; i < argc; i++) { > - const char *arg = argv[i]; > - if (!strcmp(arg, "-p")) { > - struct object_id oid; > - if (argc <= ++i) > - usage(commit_tree_usage); > - if (get_oid_commit(argv[i], &oid)) > - die("Not a valid object name %s", argv[i]); > - assert_oid_type(&oid, OBJ_COMMIT); > - new_parent(lookup_commit(the_repository, &oid), > - &parents); > - continue; > - } > + usage_with_options(builtin_commit_tree_usage, builtin_commit_tree_options); > > - if (!strcmp(arg, "--gpg-sign")) { > - sign_commit = ""; > - continue; > - } > - > - if (skip_prefix(arg, "-S", &sign_commit) || > - skip_prefix(arg, "--gpg-sign=", &sign_commit)) > - continue; > - > - if (!strcmp(arg, "--no-gpg-sign")) { > - sign_commit = NULL; > - continue; > - } > + argc = parse_options(argc, argv, prefix, builtin_commit_tree_options, > + builtin_commit_tree_usage, 0); > > - if (!strcmp(arg, "-m")) { > - if (argc <= ++i) > - usage(commit_tree_usage); > - if (buffer.len) > - strbuf_addch(&buffer, '\n'); > - strbuf_addstr(&buffer, argv[i]); > - strbuf_complete_line(&buffer); > - continue; > - } > - > - if (!strcmp(arg, "-F")) { > - int fd; > - > - if (argc <= ++i) > - usage(commit_tree_usage); > - if (buffer.len) > - strbuf_addch(&buffer, '\n'); > - if (!strcmp(argv[i], "-")) > - fd = 0; > - else { > - fd = open(argv[i], O_RDONLY); > - if (fd < 0) > - die_errno("git commit-tree: failed to open '%s'", > - argv[i]); > - } > - if (strbuf_read(&buffer, fd, 0) < 0) > - die_errno("git commit-tree: failed to read '%s'", > - argv[i]); > - if (fd && close(fd)) > - die_errno("git commit-tree: failed to close '%s'", > - argv[i]); > - continue; > - } > + if (argc != 1) > + die("Must give exactly one tree"); > > - if (get_oid_tree(arg, &tree_oid)) > - die("Not a valid object name %s", arg); > - if (got_tree) > - die("Cannot give more than one trees"); > - got_tree = 1; > - } > + if (get_oid_tree(argv[0], &tree_oid)) > + die("Not a valid object name %s", argv[0]); > > if (!buffer.len) { > if (strbuf_read(&buffer, 0, 0) < 0) > -- > 2.21.0 > -- Duy
