git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Duy Nguyen <pclouds@gmail.com>
Date
Feb 27, 2019, 11:07 UTC
Message-ID
<CACsJy8Bgz6FiTqnq8pnebuyOr55Bqh67iRhr6J+WvzgxPSBLhw@mail.gmail.com>
In-Reply-To
<20190226200952.33950-1-brandon1024.br@gmail.com>
On Wed, Feb 27, 2019 at 3:10 AM Brandon <brandon1024.br@gmail.com> wrote:
Show 9 quoted lines
>
> 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.

Show 49 quoted lines
> 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.
Show 8 quoted lines
> +
> +       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().

Show 17 quoted lines
> +{
> +       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.

Show 35 quoted lines
> +{
> +       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.
Show 10 quoted lines
> +                 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.
Show 87 quoted lines
>
>         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
Previous: Duy NguyenNext: SZEDER Gábor
Message 5 of 14 in “commit-tree: utilize parse-options api”
  1. commit-tree: utilize parse-options apiBrandon, Feb 26, 2019
  2. Andrei RybakFeb 26, 2019
  3. Brandon RichardsonFeb 26, 2019
  4. Duy NguyenFeb 27, 2019
  5. Duy NguyenFeb 27, 2019
  6. SZEDER GáborFeb 27, 2019
  7. Duy NguyenFeb 27, 2019
  8. SZEDER GáborFeb 27, 2019
  9. Duy NguyenFeb 28, 2019
  10. Brandon RichardsonFeb 27, 2019
  11. Duy NguyenFeb 28, 2019
  12. Jeff KingFeb 27, 2019
  13. Brandon RichardsonFeb 28, 2019
  14. Jeff KingFeb 28, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.