Brandon Richardson <brandon1024.br@xxxxxxxxx> writes:
> @@ -23,6 +23,9 @@ Creates a new commit object based on the provided tree object and
> emits the new commit object id on stdout. The log message is read
> from the standard input, unless `-m` or `-F` options are given.
>
> +When mixing `-m` and `-F` options, the commit log message will be
> +composed in the order in which the options are given.
It may be just me, but this new paragraph made me think that we can
give at most one -m and one -F option at the same time in any order,
and multiple -m or -F options are not supported. That, obviously,
is not the impression we want to give to the readers.
Even when you are not mixing -m and -F, but using -m more than once,
the log message will be composed in the order in which options are
given. So probably the word "mixing" is the primary culprit of
making the sentence easier to be misunderstood.
When using more than one `-m` or `-F` options, ...
perhaps.
> @@ -41,7 +44,7 @@ state was.
> OPTIONS
> -------
> <tree>::
> - An existing tree object
> + An existing tree object.
Good.
> @@ -52,7 +55,8 @@ OPTIONS
>
> -F <file>::
> Read the commit log message from the given file. Use `-` to read
> - from the standard input.
> + from the standard input. This can be given more than once and the
> + content of each file becomes its own paragraph.
OK, this matches what -m says about giving it multiple times.
> -static const char commit_tree_usage[] = "git commit-tree [(-p <sha1>)...] [-S[<keyid>]] [-m <message>] [-F <file>] <sha1>";
> +static const char * const commit_tree_usage[] = {
> + N_("git commit-tree [(-p <parent>)...] [-S[<keyid>]] [(-m <message>)...] "
> + "[(-F <file>)...] <tree>"),
> + NULL
> +};
Replacing a few placeholder tokens with more meaningful names---very
good attention to the detail.
> +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_NOARG(unset, arg);
> +
> + 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;
OK, this looks like a quite faithful conversion. We do not allow
tags that point at commit, for example. Good.
> +}
> +
> +static int parse_message_arg_callback(const struct option *opt,
> + const char *arg, int unset)
> +{
> + struct strbuf *buf = opt->value;
> +
> + BUG_ON_OPT_NEG_NOARG(unset, arg);
> +
> + if (buf->len)
> + strbuf_addch(buf, '\n');
> + strbuf_addstr(buf, arg);
> + strbuf_complete_line(buf);
> +
> + return 0;
> +}
Likewise. We make ourselves a new paragraph (if there is already
some message), add the message and complete the line. Good.
> +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_NOARG(unset, arg);
> +
> + 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;
> +}
Again, likewise. And it is far easier to see and read what is going
on, compared to the original that has 2 extra levels of indentation.
> int cmd_commit_tree(int argc, const char **argv, const char *prefix)
> {
The change to this main function looks quite straight-forward. I am
kind of surprised that a very low hanging fruit like this had survived
without getting hit by parseopt a lot earlier ;-)
