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

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

[Brandon Richardson]

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 commit 70ddbd7767 (commit-tree: add missing
--gpg-sign flag, 2019-01-19), and the existing implementation
would attempt to translate the option as a tree oid. It was also
suggested earlier in commit 55ca3f99ae (commit-tree: add and document
--no-gpg-sign, 2013-12-13) 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.

Also update the documentation to better describe that mixing
`-m` and `-F` options will correctly compose commit log messages in the
order in which the options are given.

In the process, mark various strings for translation.

Signed-off-by: Brandon Richardson <brandon1024.br@gmail.com>
---

Notes:
    GitHub Pull Request: https://github.com/brandon1024/git/pull/4
    Travis CI Build: https://travis-ci.com/brandon1024/git/builds/103055317

 Documentation/git-commit-tree.txt |   8 +-
 builtin/commit-tree.c             | 158 ++++++++++++++++--------------
 parse-options.h                   |  11 +++
 3 files changed, 103 insertions(+), 74 deletions(-)

diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt
index 002dae625e..f4e20b62a0 100644
--- a/Documentation/git-commit-tree.txt
+++ b/Documentation/git-commit-tree.txt
@@ -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.
+
 A commit object may have any number of parents. With exactly one
 parent, it is an ordinary commit. Having more than one parent makes
 the commit a merge between several lines of history. Initial (root)
@@ -41,7 +44,7 @@ state was.
 OPTIONS
 -------
 <tree>::
-	An existing tree object
+	An existing tree object.
 
 -p <parent>::
 	Each `-p` indicates the id of a parent commit object.
@@ -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.
 
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 12cc403bd7..b866d83951 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -12,8 +12,13 @@
 #include "builtin.h"
 #include "utf8.h"
 #include "gpg-interface.h"
+#include "parse-options.h"
 
-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
+};
 
 static const char *sign_commit;
 
@@ -23,7 +28,7 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)
 	struct commit_list *parents;
 	for (parents = *parents_p; parents; parents = parents->next) {
 		if (parents->item == parent) {
-			error("duplicate parent %s ignored", oid_to_hex(oid));
+			error(_("duplicate parent %s ignored"), oid_to_hex(oid));
 			return;
 		}
 		parents_p = &parents->next;
@@ -39,91 +44,100 @@ 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_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;
+}
+
+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;
+}
+
+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;
+}
+
 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 options[] = {
+		{ OPTION_CALLBACK, 'p', NULL, &parents, N_("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(commit_tree_usage, options);
 
-		if (!strcmp(arg, "--gpg-sign")) {
-		    sign_commit = "";
-		    continue;
-		}
+	argc = parse_options(argc, argv, prefix, options, commit_tree_usage, 0);
 
-		if (skip_prefix(arg, "-S", &sign_commit) ||
-			skip_prefix(arg, "--gpg-sign=", &sign_commit))
-			continue;
+	if (argc != 1)
+		die(_("must give exactly one tree"));
 
-		if (!strcmp(arg, "--no-gpg-sign")) {
-			sign_commit = NULL;
-			continue;
-		}
-
-		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 (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)
-			die_errno("git commit-tree: failed to read");
+			die_errno(_("git commit-tree: failed to read"));
 	}
 
 	if (commit_tree(buffer.buf, buffer.len, &tree_oid, parents, &commit_oid,
diff --git a/parse-options.h b/parse-options.h
index 14fe32428e..3a442eee26 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -202,6 +202,17 @@ const char *optname(const struct option *opt, int flags);
 		BUG("option callback does not expect an argument"); \
 } while (0)
 
+/*
+ * Similar to the assertions above, but checks that "arg" is always non-NULL.
+ * This assertion also implies BUG_ON_OPT_NEG(), letting you declare both
+ * assertions in a single line.
+ */
+#define BUG_ON_OPT_NEG_NOARG(unset, arg) do { \
+	BUG_ON_OPT_NEG(unset); \
+	if(!(arg)) \
+		BUG("option callback expects an argument"); \
+} while(0)
+
 /*----- incremental advanced APIs -----*/
 
 enum {
-- 
2.21.0

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

[Junio C Hamano]

Brandon Richardson <brandon1024.br@gmail.com> 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 ;-)

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

[Brandon Richardson]

On Wed, Mar 6, 2019 at 7:21 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > +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.

Good call, 'mixing' is not the right word here. Will fix.

> 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 ;-)

I was surprised too, commit-tree hasn't seen much love over the years.
There are certainly others that could benefit from parse-options.

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

[Duy Nguyen]

On Thu, Mar 7, 2019 at 6:21 AM Junio C Hamano <gitster@pobox.com> wrote:
> 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 ;-)

There are more (I guess we tag #leftovers nowadays?)

git grep 'strcmp.*\"--[a-z]' builtin/

(with some false positives)
-- 
Duy