* [PATCH] commit-tree: utilize parse-options api @ 2019-02-26 20:09 Brandon 2019-02-26 22:38 ` Andrei Rybak 2019-02-27 11:07 ` Duy Nguyen 0 siblings, 2 replies; 14+ messages in thread From: Brandon @ 2019-02-26 20:09 UTC (permalink / raw) To: git; +Cc: Brandon Richardson From: Brandon Richardson <brandon1024.br@gmail.com> 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 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@gmail.com> --- 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; + if (get_oid_commit(arg, &oid)) + die("Not a valid object name %s", arg); + + 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) +{ + 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) +{ + 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[] = { + { OPTION_CALLBACK, 'p', NULL, &parents, "parent", + 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) "" }, + OPT_END() + }; 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-26 20:09 [PATCH] commit-tree: utilize parse-options api Brandon @ 2019-02-26 22:38 ` Andrei Rybak 2019-02-26 23:42 ` Brandon Richardson 2019-02-27 11:07 ` Duy Nguyen 1 sibling, 1 reply; 14+ messages in thread From: Andrei Rybak @ 2019-02-26 22:38 UTC (permalink / raw) To: Brandon, git A couple of code style issues: On 2/26/19 9:09 PM, Brandon wrote: > From: Brandon Richardson <brandon1024.br@gmail.com> > > 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 > would attempt to translate the option as a tree oid.It was also Missing space after period. [snip] > + > 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[] = { Style: tab should be used instead of four spaces. > + { OPTION_CALLBACK, 'p', NULL, &parents, "parent", > + N_("id of a parent commit object"), PARSE_OPT_NONEG, Comparing to other similar places, a single tab should be used to align "N_" instead of two spaces. > + 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) "" }, > + OPT_END() > + }; [snip] > - > - 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); here "builtin_commit_tree_usage" should be aligned with "argc" in previous line. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-26 22:38 ` Andrei Rybak @ 2019-02-26 23:42 ` Brandon Richardson 2019-02-27 11:13 ` Duy Nguyen 0 siblings, 1 reply; 14+ messages in thread From: Brandon Richardson @ 2019-02-26 23:42 UTC (permalink / raw) To: Andrei Rybak; +Cc: Git Mailing List Hi Andrei, > > would attempt to translate the option as a tree oid.It was also > > Missing space after period. Oops, thanks for pointing that out. > > > + { OPTION_CALLBACK, 'p', NULL, &parents, "parent", > > + N_("id of a parent commit object"), PARSE_OPT_NONEG, > > Comparing to other similar places, a single tab should be used to > align "N_" instead of two spaces. I've seen a mix of both conventions scattered around, and wasn't sure which to stick to. I'll switch to that. Thanks for your comments :-) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-26 23:42 ` Brandon Richardson @ 2019-02-27 11:13 ` Duy Nguyen 0 siblings, 0 replies; 14+ messages in thread From: Duy Nguyen @ 2019-02-27 11:13 UTC (permalink / raw) To: Brandon Richardson; +Cc: Andrei Rybak, Git Mailing List On Wed, Feb 27, 2019 at 6:43 AM Brandon Richardson <brandon1024.br@gmail.com> wrote: > > Hi Andrei, > > > > would attempt to translate the option as a tree oid.It was also > > > > Missing space after period. > > Oops, thanks for pointing that out. > > > > > > + { OPTION_CALLBACK, 'p', NULL, &parents, "parent", > > > + N_("id of a parent commit object"), PARSE_OPT_NONEG, > > > > Comparing to other similar places, a single tab should be used to > > align "N_" instead of two spaces. > > I've seen a mix of both conventions scattered around, and wasn't sure which > to stick to. I'll switch to that. I think we sometimes use spaces for fine alignment (search "tabs and spaces" in CodingGuidelines). It's really up to you and Andrei which style is preferred ;-) -- Duy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-26 20:09 [PATCH] commit-tree: utilize parse-options api Brandon 2019-02-26 22:38 ` Andrei Rybak @ 2019-02-27 11:07 ` Duy Nguyen 2019-02-27 11:37 ` SZEDER Gábor ` (2 more replies) 1 sibling, 3 replies; 14+ messages in thread From: Duy Nguyen @ 2019-02-27 11:07 UTC (permalink / raw) To: Brandon; +Cc: Git Mailing List On Wed, Feb 27, 2019 at 3:10 AM Brandon <brandon1024.br@gmail.com> wrote: > > From: Brandon Richardson <brandon1024.br@gmail.com> > > 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@gmail.com> > --- > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-27 11:07 ` Duy Nguyen @ 2019-02-27 11:37 ` SZEDER Gábor 2019-02-27 11:49 ` Duy Nguyen 2019-02-27 15:24 ` Brandon Richardson 2019-02-27 16:35 ` Jeff King 2 siblings, 1 reply; 14+ messages in thread From: SZEDER Gábor @ 2019-02-27 11:37 UTC (permalink / raw) To: Duy Nguyen; +Cc: Brandon, Git Mailing List On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote: > > 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) No, most often we use 55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13) i.e. with a comma instead of a dash between subject and short date; and without quotes around the subject. Truly sorry for nitpicking :) Gábor ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-27 11:37 ` SZEDER Gábor @ 2019-02-27 11:49 ` Duy Nguyen 2019-02-27 12:36 ` SZEDER Gábor 0 siblings, 1 reply; 14+ messages in thread From: Duy Nguyen @ 2019-02-27 11:49 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Brandon, Git Mailing List On Wed, Feb 27, 2019 at 6:37 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote: > > > 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) > > No, most often we use > > 55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13) > > i.e. with a comma instead of a dash between subject and short date; > and without quotes around the subject. > > Truly sorry for nitpicking :) Naah it's about time I update my ~/.gitconfig to be "conformant" :D I think we both failed to mention where to find the command for Brandon though: search commit-reference in SubmittingPatches. -- Duy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-27 11:49 ` Duy Nguyen @ 2019-02-27 12:36 ` SZEDER Gábor 2019-02-28 7:21 ` Duy Nguyen 0 siblings, 1 reply; 14+ messages in thread From: SZEDER Gábor @ 2019-02-27 12:36 UTC (permalink / raw) To: Duy Nguyen; +Cc: Brandon, Git Mailing List On Wed, Feb 27, 2019 at 06:49:53PM +0700, Duy Nguyen wrote: > On Wed, Feb 27, 2019 at 6:37 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote: > > > > 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) > > > > No, most often we use > > > > 55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13) > > > > i.e. with a comma instead of a dash between subject and short date; > > and without quotes around the subject. > > > > Truly sorry for nitpicking :) > > Naah it's about time I update my ~/.gitconfig to be "conformant" :D I > think we both failed to mention where to find the command for Brandon > though: search commit-reference in SubmittingPatches. Well, yes... but I didn't mention that on purpose: SubmittingPatches advocates for quotes around the subject, which is still the less often used format of the two, and there is no good reason for those quotes (that 'deadbeef (' before and ', 2019-12-34)' after the subject provide plenty of separation and indicate quite clearly what's going on). However, looking at the length of the suggested command in SubmittingPatches made me remember that I've been using a couple of patches implementing 'git log --format=reference' for a couple of years now... I wonder whether it would be worth having something like that in git.git, and thus making it conveniently available for other projects as well. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-27 12:36 ` SZEDER Gábor @ 2019-02-28 7:21 ` Duy Nguyen 0 siblings, 0 replies; 14+ messages in thread From: Duy Nguyen @ 2019-02-28 7:21 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Brandon, Git Mailing List On Wed, Feb 27, 2019 at 7:36 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Wed, Feb 27, 2019 at 06:49:53PM +0700, Duy Nguyen wrote: > > On Wed, Feb 27, 2019 at 6:37 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > > > On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote: > > > > > 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) > > > > > > No, most often we use > > > > > > 55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13) > > > > > > i.e. with a comma instead of a dash between subject and short date; > > > and without quotes around the subject. > > > > > > Truly sorry for nitpicking :) > > > > Naah it's about time I update my ~/.gitconfig to be "conformant" :D I > > think we both failed to mention where to find the command for Brandon > > though: search commit-reference in SubmittingPatches. > > Well, yes... but I didn't mention that on purpose: SubmittingPatches > advocates for quotes around the subject, which is still the less often > used format of the two, and there is no good reason for those quotes > (that 'deadbeef (' before and ', 2019-12-34)' after the subject > provide plenty of separation and indicate quite clearly what's going > on). Perhaps a patch to strip those quotes from the command in SubmittingPatches? > However, looking at the length of the suggested command in > SubmittingPatches made me remember that I've been using a couple of > patches implementing 'git log --format=reference' for a couple of > years now... I wonder whether it would be worth having something like > that in git.git, and thus making it conveniently available for other > projects as well. It does sound nice to have something like this built in. But I'm not sure if "git log" would be the right place. For handling single revisions (most often the case), git-show or git-rev-parse might be the better interface. Even for referencing multiple hashes at the same time (e.g. you prepare a text with bare hashes first, then run some program to insert the "(..)" part) then name-rev might be a better candidate. A softer route to avoid any of that is simply adding default config "pretty.reference", then let the user define their own alias that uses --pretty=reference. Or perhaps just put it in EXAMPLES section of git-log. -- Duy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-27 11:07 ` Duy Nguyen 2019-02-27 11:37 ` SZEDER Gábor @ 2019-02-27 15:24 ` Brandon Richardson 2019-02-28 7:26 ` Duy Nguyen 2019-02-27 16:35 ` Jeff King 2 siblings, 1 reply; 14+ messages in thread From: Brandon Richardson @ 2019-02-27 15:24 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, szeder.dev Thank you all for the helpful comments :-) On Wed, 27 Feb 2019 at 07:08, Duy Nguyen <pclouds@gmail.com> wrote: > > 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. I didn't know this, thank you for the tip. I'll start doing this from now on. I will also reread through the SubmittingPatches doc. > > +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. I like the BUG_ON_NO_ARG approach. I will go that route. > > +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. I opted to use callbacks here to allow mixing -m and -F so that messages are composed correctly, as you mentioned. I did so in an attempt to match the existing functionality of commit-tree. > > > + OPT_END() > > + }; > > I think you're using spaces here to indent instead of TABs. Good eye on the whitespace issue. I'm still dialling in my environment, so please forgive me. I will address all comments in a v2. Thanks again. Brandon ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-27 15:24 ` Brandon Richardson @ 2019-02-28 7:26 ` Duy Nguyen 0 siblings, 0 replies; 14+ messages in thread From: Duy Nguyen @ 2019-02-28 7:26 UTC (permalink / raw) To: Brandon Richardson; +Cc: Git Mailing List, SZEDER Gábor On Wed, Feb 27, 2019 at 10:24 PM Brandon Richardson <brandon1024.br@gmail.com> wrote: > > > +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. > > I opted to use callbacks here to allow mixing -m and -F so that messages > are composed correctly, as you mentioned. I did so in an attempt to match > the existing functionality of commit-tree. Fair enough. Probably safest to do that anyway. If you feel like doing some improvements, maybe mention this behavior in git-commit-tree.txt too. It does say -m can be used multiple times, but nothing explicit about -F (and I wonder if -F also does the "becomes its own paragraph" like -m). Also mixing -m and -F technically could be inferred from the synopsis line, but it's just easier to read an plain English sentence. -- Duy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-27 11:07 ` Duy Nguyen 2019-02-27 11:37 ` SZEDER Gábor 2019-02-27 15:24 ` Brandon Richardson @ 2019-02-27 16:35 ` Jeff King 2019-02-28 2:46 ` Brandon Richardson 2 siblings, 1 reply; 14+ messages in thread From: Jeff King @ 2019-02-27 16:35 UTC (permalink / raw) To: Duy Nguyen; +Cc: Brandon, Git Mailing List On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote: > > +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. I think it should be spelled BUG_ON_OPT_NOARG() to match the other ones. One of the reasons I did not bother with that condition when I added the OPT_NEG() and OPT_ARG() variants is that you can only get an unexpected NULL argument if you explicitly give the NOARG or OPTARG flags. So it's very easy to _forget_ to give such a flag, because you simply aren't thinking about that case, and your callback is buggy by default. But it's rare to actually think to give one of those flags, but then forget to handle it in your callback. So I'm not entirely opposed, but it does feel weird to add such a macro without then using it in the 99% of callbacks which expect arg to be non-NULL. Actually, there is one subtlety, which is that it can be NULL if "unset" is true. But then callbacks should already be looking at "unset" or using BUG_ON_OPT_NEG(). But that just makes things worse. Take parse_opt_patchformat(), for example. It _does_ check "unset", so should not use BUG_ON_OPT_NEG(). But if "!unset", it expects "arg" to be non-NULL. So adding an assertion there turns our nice cascade of conditionals: if (unset) ...handle unset... else if (!strcmp(arg, "foo")) ...handle "foo"... ...and so on... into: if (unset) ...handle unset... else { BUG_ON_OPT_NOARG(arg); if (!strcmp, "foo")) .... ... and so on... } If we are going to go this route, I think you might actually want macros that take both "unset" and "args" and make sure that we're not in a situation the callback doesn't expect (e.g., "!unset && !arg"). That lets us continue to declare those at the top of the callback. But as you can see, it gets complicated quickly. I'm not really sure it's worth the trouble for a maintenance problem that's relatively unlikely. -Peff ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-27 16:35 ` Jeff King @ 2019-02-28 2:46 ` Brandon Richardson 2019-02-28 20:56 ` Jeff King 0 siblings, 1 reply; 14+ messages in thread From: Brandon Richardson @ 2019-02-28 2:46 UTC (permalink / raw) To: Jeff King; +Cc: Duy Nguyen, Git Mailing List Hi Jeff, > One of the reasons I did not bother with that condition when I added the > OPT_NEG() and OPT_ARG() variants is that you can only get an unexpected > NULL argument if you explicitly give the NOARG or OPTARG flags. So it's > very easy to _forget_ to give such a flag, because you simply aren't > thinking about that case, and your callback is buggy by default. > > But it's rare to actually think to give one of those flags, but then > forget to handle it in your callback. > > So I'm not entirely opposed, but it does feel weird to add such a macro > without then using it in the 99% of callbacks which expect arg to be > non-NULL. I'd like to agree with you here, especially given that commit-tree is a rather small part of project source. Experimenting with it a bit, I found using BUG_ON_OPT_NOARG() to be a big clunky. Like you said, we could end up with some less-than-ideal usage. If I were to use this in commit-tree, it would look something like this, which isn't very appealing: static int callback(const struct option *opt, const char *arg, int unset) { ... BUG_ON_OPT_NEG(unset); BUG_ON_OPT_NO_ARG(arg); ... However, I do still see a use case for a new macro for options that cannot be unset and arguments that must not be NULL. > If we are going to go this route, I think you might actually want macros > that take both "unset" and "args" and make sure that we're not in a > situation the callback doesn't expect (e.g., "!unset && !arg"). That > lets us continue to declare those at the top of the callback. In doing a quick search, I found a fair number instances of this: ... BUG_ON_OPT_NEG(unset); if (!arg) return -1; ... So a macro like this could be useful. I've also found a few instances of this: BUG_ON_OPT_NEG(unset); BUG_ON_OPT_ARG(arg); Perhaps two new macros BUG_ON_OPT_NEG_NO_ARG() ("!unset || !arg") and BUG_ON_OPT_NEG_ARG() ("!unset || arg")? I'm not a big fan of those names though. Brandon ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] commit-tree: utilize parse-options api 2019-02-28 2:46 ` Brandon Richardson @ 2019-02-28 20:56 ` Jeff King 0 siblings, 0 replies; 14+ messages in thread From: Jeff King @ 2019-02-28 20:56 UTC (permalink / raw) To: Brandon Richardson; +Cc: Duy Nguyen, Git Mailing List On Wed, Feb 27, 2019 at 10:46:49PM -0400, Brandon Richardson wrote: > > If we are going to go this route, I think you might actually want macros > > that take both "unset" and "args" and make sure that we're not in a > > situation the callback doesn't expect (e.g., "!unset && !arg"). That > > lets us continue to declare those at the top of the callback. > > In doing a quick search, I found a fair number instances of this: > ... > BUG_ON_OPT_NEG(unset); > > if (!arg) > return -1; > ... Those are probably my fault. The originals guarded against an unexpected "unset" by checking "!arg" and returning an error. But it made the compiler's -Wunused-parameter complain, so I added the BUG_ON_OPT_NEG() calls as an assertion. At that point the "if (!arg)" could never trigger, and could have been removed. > So a macro like this could be useful. I've also found a few instances of this: > > BUG_ON_OPT_NEG(unset); > BUG_ON_OPT_ARG(arg); These ones are different. The second one is checking that "arg" _is_ NULL (i.e., we expect that the options struct provided the right flag to disallow an argument). And that's orthogonal to the unset flag, so it would not be right to conflate the two in a single macro. -Peff ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-02-28 20:56 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-26 20:09 [PATCH] commit-tree: utilize parse-options api Brandon 2019-02-26 22:38 ` Andrei Rybak 2019-02-26 23:42 ` Brandon Richardson 2019-02-27 11:13 ` Duy Nguyen 2019-02-27 11:07 ` Duy Nguyen 2019-02-27 11:37 ` SZEDER Gábor 2019-02-27 11:49 ` Duy Nguyen 2019-02-27 12:36 ` SZEDER Gábor 2019-02-28 7:21 ` Duy Nguyen 2019-02-27 15:24 ` Brandon Richardson 2019-02-28 7:26 ` Duy Nguyen 2019-02-27 16:35 ` Jeff King 2019-02-28 2:46 ` Brandon Richardson 2019-02-28 20:56 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).