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

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

[Brandon]

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
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;
+	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(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)
+{
+	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[] = {
+		{ OPTION_CALLBACK, 'p', NULL, &parents, "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(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

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

[Andrei Rybak]

A couple of code style issues:

On 2/26/19 9:09 PM, Brandon wrote:
> 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
> would attempt to translate the option as a tree oid.It was also

Missing space after period.

[snip]

> +
>  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[] = {

Style: tab should be used instead of four spaces.

> +		{ OPTION_CALLBACK, 'p', NULL, &parents, "parent",
> +		  N_("id of a parent commit object"), PARSE_OPT_NONEG,

Comparing to other similar places, a single tab should be used to
align "N_" instead of two spaces.

> +		  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()
> +    };

[snip]

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

here "builtin_commit_tree_usage" should be aligned with "argc" in
previous line.

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

[Brandon Richardson]

Hi Andrei,

> > would attempt to translate the option as a tree oid.It was also
>
> Missing space after period.

Oops, thanks for pointing that out.

>
> > +             { OPTION_CALLBACK, 'p', NULL, &parents, "parent",
> > +               N_("id of a parent commit object"), PARSE_OPT_NONEG,
>
> Comparing to other similar places, a single tab should be used to
> align "N_" instead of two spaces.

I've seen a mix of both conventions scattered around, and wasn't sure which
to stick to. I'll switch to that.

Thanks for your comments :-)

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

[Duy Nguyen]

On Wed, Feb 27, 2019 at 3:10 AM Brandon <brandon1024.br@gmail.com> wrote:
>
> 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.


> 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.

> +
> +       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

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

[Duy Nguyen]

On Wed, Feb 27, 2019 at 6:43 AM Brandon Richardson
<brandon1024.br@gmail.com> wrote:
>
> Hi Andrei,
>
> > > would attempt to translate the option as a tree oid.It was also
> >
> > Missing space after period.
>
> Oops, thanks for pointing that out.
>
> >
> > > +             { OPTION_CALLBACK, 'p', NULL, &parents, "parent",
> > > +               N_("id of a parent commit object"), PARSE_OPT_NONEG,
> >
> > Comparing to other similar places, a single tab should be used to
> > align "N_" instead of two spaces.
>
> I've seen a mix of both conventions scattered around, and wasn't sure which
> to stick to. I'll switch to that.

I think we sometimes use spaces for fine alignment (search "tabs and
spaces" in CodingGuidelines). It's really up to you and Andrei which
style is preferred ;-)
-- 
Duy

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

[SZEDER Gábor]

On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:
> > 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)

No, most often we use

  55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13)

i.e. with a comma instead of a dash between subject and short date;
and without quotes around the subject.

Truly sorry for nitpicking :)

Gábor

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

[Duy Nguyen]

On Wed, Feb 27, 2019 at 6:37 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:
> > > 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)
>
> No, most often we use
>
>   55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13)
>
> i.e. with a comma instead of a dash between subject and short date;
> and without quotes around the subject.
>
> Truly sorry for nitpicking :)

Naah it's about time I update my ~/.gitconfig to be "conformant" :D I
think we both failed to mention where to find the command for Brandon
though: search commit-reference in SubmittingPatches.
-- 
Duy

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

[SZEDER Gábor]

On Wed, Feb 27, 2019 at 06:49:53PM +0700, Duy Nguyen wrote:
> On Wed, Feb 27, 2019 at 6:37 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
> > On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:
> > > > 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)
> >
> > No, most often we use
> >
> >   55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13)
> >
> > i.e. with a comma instead of a dash between subject and short date;
> > and without quotes around the subject.
> >
> > Truly sorry for nitpicking :)
> 
> Naah it's about time I update my ~/.gitconfig to be "conformant" :D I
> think we both failed to mention where to find the command for Brandon
> though: search commit-reference in SubmittingPatches.

Well, yes...  but I didn't mention that on purpose: SubmittingPatches
advocates for quotes around the subject, which is still the less often
used format of the two, and there is no good reason for those quotes
(that 'deadbeef (' before and ', 2019-12-34)' after the subject
provide plenty of separation and indicate quite clearly what's going
on).

However, looking at the length of the suggested command in
SubmittingPatches made me remember that I've been using a couple of
patches implementing 'git log --format=reference' for a couple of
years now...  I wonder whether it would be worth having something like
that in git.git, and thus making it conveniently available for other
projects as well.

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

[Brandon Richardson]

Thank you all for the helpful comments :-)

On Wed, 27 Feb 2019 at 07:08, Duy Nguyen <pclouds@gmail.com> wrote:
> > 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.

I didn't know this, thank you for the tip. I'll start doing this from now on.
I will also reread through the SubmittingPatches doc.

> > +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.

I like the BUG_ON_NO_ARG approach. I will go that route.

> > +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.

I opted to use callbacks here to allow mixing -m and -F so that messages
are composed correctly, as you mentioned. I did so in an attempt to match
the existing functionality of commit-tree.

>
> > +               OPT_END()
> > +    };
>
> I think you're using spaces here to indent instead of TABs.

Good eye on the whitespace issue. I'm still dialling in my environment,
so please forgive me.

I will address all comments in a v2. Thanks again.

Brandon

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

[Jeff King]

On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:

> > +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.

I think it should be spelled BUG_ON_OPT_NOARG() to match the other ones.

One of the reasons I did not bother with that condition when I added the
OPT_NEG() and OPT_ARG() variants is that you can only get an unexpected
NULL argument if you explicitly give the NOARG or OPTARG flags. So it's
very easy to _forget_ to give such a flag, because you simply aren't
thinking about that case, and your callback is buggy by default.

But it's rare to actually think to give one of those flags, but then
forget to handle it in your callback.

So I'm not entirely opposed, but it does feel weird to add such a macro
without then using it in the 99% of callbacks which expect arg to be
non-NULL.

Actually, there is one subtlety, which is that it can be NULL if "unset"
is true. But then callbacks should already be looking at "unset" or
using BUG_ON_OPT_NEG(). But that just makes things worse. Take
parse_opt_patchformat(), for example. It _does_ check "unset", so should
not use BUG_ON_OPT_NEG(). But if "!unset", it expects "arg" to be
non-NULL. So adding an assertion there turns our nice cascade of
conditionals:

  if (unset)
	...handle unset...
  else if (!strcmp(arg, "foo"))
	...handle "foo"...
  ...and so on...

into:

  if (unset)
	...handle unset...
  else {
	BUG_ON_OPT_NOARG(arg);
	if (!strcmp, "foo"))
		....
	... and so on...
  }

If we are going to go this route, I think you might actually want macros
that take both "unset" and "args" and make sure that we're not in a
situation the callback doesn't expect (e.g., "!unset && !arg"). That
lets us continue to declare those at the top of the callback.

But as you can see, it gets complicated quickly. I'm not really sure
it's worth the trouble for a maintenance problem that's relatively
unlikely.

-Peff

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

[Brandon Richardson]

Hi Jeff,

> One of the reasons I did not bother with that condition when I added the
> OPT_NEG() and OPT_ARG() variants is that you can only get an unexpected
> NULL argument if you explicitly give the NOARG or OPTARG flags. So it's
> very easy to _forget_ to give such a flag, because you simply aren't
> thinking about that case, and your callback is buggy by default.
>
> But it's rare to actually think to give one of those flags, but then
> forget to handle it in your callback.
>
> So I'm not entirely opposed, but it does feel weird to add such a macro
> without then using it in the 99% of callbacks which expect arg to be
> non-NULL.

I'd like to agree with you here, especially given that commit-tree is a rather
small part of project source. Experimenting with it a bit, I found using
BUG_ON_OPT_NOARG() to be a big clunky. Like you said, we could
end up with some less-than-ideal usage. If I were to use this in commit-tree,
it would look something like this, which isn't very appealing:

static int callback(const struct option *opt, const char *arg, int unset)
{
     ...
     BUG_ON_OPT_NEG(unset);
     BUG_ON_OPT_NO_ARG(arg);
     ...

However, I do still see a use case for a new macro for options that cannot
be unset and arguments that must not be NULL.

> If we are going to go this route, I think you might actually want macros
> that take both "unset" and "args" and make sure that we're not in a
> situation the callback doesn't expect (e.g., "!unset && !arg"). That
> lets us continue to declare those at the top of the callback.

In doing a quick search, I found a fair number instances of this:
...
BUG_ON_OPT_NEG(unset);

if (!arg)
     return -1;
...

So a macro like this could be useful. I've also found a few instances of this:

BUG_ON_OPT_NEG(unset);
BUG_ON_OPT_ARG(arg);

Perhaps two new macros BUG_ON_OPT_NEG_NO_ARG() ("!unset || !arg")
and BUG_ON_OPT_NEG_ARG() ("!unset || arg")? I'm not a big fan of those
names though.

Brandon

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

[Duy Nguyen]

On Wed, Feb 27, 2019 at 7:36 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Wed, Feb 27, 2019 at 06:49:53PM +0700, Duy Nguyen wrote:
> > On Wed, Feb 27, 2019 at 6:37 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > >
> > > On Wed, Feb 27, 2019 at 06:07:42PM +0700, Duy Nguyen wrote:
> > > > > 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)
> > >
> > > No, most often we use
> > >
> > >   55ca3f99ae (commit-tree: add and document --no-gpg-sign, 2013-12-13)
> > >
> > > i.e. with a comma instead of a dash between subject and short date;
> > > and without quotes around the subject.
> > >
> > > Truly sorry for nitpicking :)
> >
> > Naah it's about time I update my ~/.gitconfig to be "conformant" :D I
> > think we both failed to mention where to find the command for Brandon
> > though: search commit-reference in SubmittingPatches.
>
> Well, yes...  but I didn't mention that on purpose: SubmittingPatches
> advocates for quotes around the subject, which is still the less often
> used format of the two, and there is no good reason for those quotes
> (that 'deadbeef (' before and ', 2019-12-34)' after the subject
> provide plenty of separation and indicate quite clearly what's going
> on).

Perhaps a patch to strip those quotes from the command in SubmittingPatches?

> However, looking at the length of the suggested command in
> SubmittingPatches made me remember that I've been using a couple of
> patches implementing 'git log --format=reference' for a couple of
> years now...  I wonder whether it would be worth having something like
> that in git.git, and thus making it conveniently available for other
> projects as well.

It does sound nice to have something like this built in. But I'm not
sure if "git log" would be the right place. For handling single
revisions (most often the case), git-show or git-rev-parse might be
the better interface. Even for referencing multiple hashes at the same
time (e.g. you prepare a text with bare hashes first, then run some
program to insert the "(..)" part) then name-rev might be a better
candidate.

A softer route to avoid any of that is simply adding default config
"pretty.reference", then let the user define their own alias that uses
--pretty=reference. Or perhaps just put it in EXAMPLES section of
git-log.
-- 
Duy

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

[Duy Nguyen]

On Wed, Feb 27, 2019 at 10:24 PM Brandon Richardson
<brandon1024.br@gmail.com> wrote:
> > > +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.
>
> I opted to use callbacks here to allow mixing -m and -F so that messages
> are composed correctly, as you mentioned. I did so in an attempt to match
> the existing functionality of commit-tree.

Fair enough. Probably safest to do that anyway.

If you feel like doing some improvements, maybe mention this behavior
in git-commit-tree.txt too. It does say -m can be used multiple times,
but nothing explicit about -F (and I wonder if -F also does the
"becomes its own paragraph" like -m). Also mixing -m and -F
technically could be inferred from the synopsis line, but it's just
easier to read an plain English sentence.
-- 
Duy

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

[Jeff King]

On Wed, Feb 27, 2019 at 10:46:49PM -0400, Brandon Richardson wrote:

> > If we are going to go this route, I think you might actually want macros
> > that take both "unset" and "args" and make sure that we're not in a
> > situation the callback doesn't expect (e.g., "!unset && !arg"). That
> > lets us continue to declare those at the top of the callback.
> 
> In doing a quick search, I found a fair number instances of this:
> ...
> BUG_ON_OPT_NEG(unset);
> 
> if (!arg)
>      return -1;
> ...

Those are probably my fault. The originals guarded against an unexpected
"unset" by checking "!arg" and returning an error. But it made the
compiler's -Wunused-parameter complain, so I added the BUG_ON_OPT_NEG()
calls as an assertion. At that point the "if (!arg)" could never
trigger, and could have been removed.

> So a macro like this could be useful. I've also found a few instances of this:
> 
> BUG_ON_OPT_NEG(unset);
> BUG_ON_OPT_ARG(arg);

These ones are different. The second one is checking that "arg" _is_
NULL (i.e., we expect that the options struct provided the right flag to
disallow an argument). And that's orthogonal to the unset flag, so it
would not be right to conflate the two in a single macro.

-Peff