diff mbox series

[v2] commit-tree: utilize parse-options api

Message ID 20190301171304.2267-1-brandon1024.br@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] commit-tree: utilize parse-options api | expand

Commit Message

Brandon Richardson March 1, 2019, 5:13 p.m. UTC
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.

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

Notes:
    GitHub Pull Request: https://github.com/brandon1024/git/pull/2
    Travis CI Results: https://travis-ci.com/brandon1024/git/builds/102755598

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

Comments

Jeff King March 1, 2019, 7:09 p.m. UTC | #1
On Fri, Mar 01, 2019 at 01:13:04PM -0400, Brandon Richardson wrote:

> +/*
> + * Use this assertion for callbacks that expect to be called with NONEG,
> + * and require an argument be supplied.
> + */
> +#define BUG_ON_OPT_NEG_NOARG(unset, arg) do { \

I think this general concept is fine. It's a variant of
BUG_ON_OPT_NEG(), so you'd use one or the other.

However, the implementation:

> +	if((!unset) && (!arg)) \
> +		BUG("option callback does not expect negation and requires an argument"); \

does not really make sense. If "!unset" is true, then we know that
"!arg" will always be true as well. So this collapse down to "!unset",
which is the same as BUG_ON_OPT_NEG().

I think you want an "OR". Or even separate conditions, since really this
is just implying OPT_NEG(). In fact, you could implement and explain it
like this:

diff --git a/parse-options.h b/parse-options.h
index 14fe32428e..d46f89305c 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -202,6 +202,18 @@ 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.
+ * I.e., that we expect the NOARG and OPTARG flags _not_ to be set. Since
+ * negation is the other common cause of a NULL arg, this also implies
+ * BUG_ON_OPT_NEG(), letting you declare both assertions in a single line.
+ */
+#define BUG_ON_OPT_NOARG(unset, arg) do { \
+	BUG_ON_OPT_NEG(unset); \
+	if (!(arg)) \
+		BUG("option callback require an argument"); \
+} while (0)
+
 /*----- incremental advanced APIs -----*/
 
 enum {

-Peff
Eric Sunshine March 1, 2019, 8:53 p.m. UTC | #2
On Fri, Mar 1, 2019 at 2:10 PM Jeff King <peff@peff.net> wrote:
> On Fri, Mar 01, 2019 at 01:13:04PM -0400, Brandon Richardson wrote:
> > +     if((!unset) && (!arg)) \
> > +             BUG("option callback does not expect negation and requires an argument"); \

Peff didn't highlight this, but compare your use of macro arguments
against his...

> +/*
> + * Similar to the assertions above, but checks that "arg" is always non-NULL.
> + * I.e., that we expect the NOARG and OPTARG flags _not_ to be set. Since
> + * negation is the other common cause of a NULL arg, this also implies
> + * BUG_ON_OPT_NEG(), letting you declare both assertions in a single line.
> + */
> +#define BUG_ON_OPT_NOARG(unset, arg) do { \
> +       BUG_ON_OPT_NEG(unset); \
> +       if (!(arg)) \
> +               BUG("option callback require an argument"); \
> +} while (0)

Note, in particular how Peff used !(arg) rather than (!arg) in your
patch. This distinction is subtle but important enough to warrant
being called out. The reason that Peff did it this way (the _correct_
way) is that, as a macro argument, 'arg' may be a complex expression
rather than a simple boolean. for instance, a caller could conceivably
invoke the macro as:

    BUG_ON_OPT_NOARG(unset, foo || bar)

Let's say that 'foo' and 'bar' are both true. With Peff's version,
when the macro is expanded, that expression becomes:

    !(true || true)

which evaluates to false as expected and intended. With your version,
it expands to:

    (!true || true)

which evaluates to true (since ! has higher precedence than ||), which
is a very different and very unexpected (and likely wrong) result.
Brandon Richardson March 2, 2019, 3:29 a.m. UTC | #3
On Fri, Mar 1, 2019 at 2:09 PM Jeff King <peff@peff.net> wrote:
> I think you want an "OR". Or even separate conditions, since really this
> is just implying OPT_NEG(). In fact, you could implement and explain it
> like this:
>
> diff --git a/parse-options.h b/parse-options.h
> index 14fe32428e..d46f89305c 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -202,6 +202,18 @@ 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.
> + * I.e., that we expect the NOARG and OPTARG flags _not_ to be set. Since
> + * negation is the other common cause of a NULL arg, this also implies
> + * BUG_ON_OPT_NEG(), letting you declare both assertions in a single line.
> + */
> +#define BUG_ON_OPT_NOARG(unset, arg) do { \
> +       BUG_ON_OPT_NEG(unset); \
> +       if (!(arg)) \
> +               BUG("option callback require an argument"); \
> +} while (0)
> +
>  /*----- incremental advanced APIs -----*/

Ahh yes. I had originally used ((!unset) || (!arg)), and second guessed myself
before I submitted v2. However, I much prefer your solution which reuses
BUG_ON_OPT_NEG(). I'll switch to that :-)

Brandon
Brandon Richardson March 2, 2019, 3:35 a.m. UTC | #4
Hi Eric,

On Fri, Mar 1, 2019 at 3:53 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> Note, in particular how Peff used !(arg) rather than (!arg) in your
> patch. This distinction is subtle but important enough to warrant
> being called out. The reason that Peff did it this way (the _correct_
> way) is that, as a macro argument, 'arg' may be a complex expression
> rather than a simple boolean. for instance, a caller could conceivably
> invoke the macro as:
>
>     BUG_ON_OPT_NOARG(unset, foo || bar)

Thanks for pointing this out. I caught this shortly after I submitted
v2. I hadn't
considered that the argument could be an expression. Will fix in v3.
diff mbox series

Patch

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..9a80e83f96 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 commit_tree_usage[] = {
+	N_("git commit-tree [(-p <parent>)...] [-S[<keyid>]] [(-m <message>)...] "
+		"[(-F <file>)...] <tree>"),
+	NULL
+};
 
 static const char *sign_commit;
 
@@ -23,7 +29,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 +45,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(unset);
+
+	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..a6ab338be3 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -202,6 +202,15 @@  const char *optname(const struct option *opt, int flags);
 		BUG("option callback does not expect an argument"); \
 } while (0)
 
+/*
+ * Use this assertion for callbacks that expect to be called with NONEG,
+ * and require an argument be supplied.
+ */
+#define BUG_ON_OPT_NEG_NOARG(unset, arg) do { \
+	if((!unset) && (!arg)) \
+		BUG("option callback does not expect negation and requires an argument"); \
+} while(0)
+
 /*----- incremental advanced APIs -----*/
 
 enum {