Web lists-archives.com

Re: [PATCH] commit-tree: utilize parse-options api




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