[PATCH] convert shortlog to use parse_options

[PATCH] convert shortlog to use parse_options

[Jeff King]

The behavior should be identical, except that we now support
option bundling (e.g., "git shortlog -nse").

Signed-off-by: Jeff King <peff@peff.net>
---
Another of my pre-1.5.4 cleanups (I think we may enter 1.5.5 freeze
before I get through my backlog!). This was originally rejected because
it changed the semantics of "-w" to allow "-w X,Y,Z". However, Pierre's
c43a24834 forces short options to stick.

This is also, of course, respun on top of Daniel's shortlog changes.

 builtin-shortlog.c |   62 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 36 insertions(+), 26 deletions(-)

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index af31aba..bf6802e 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -7,9 +7,12 @@
 #include "utf8.h"
 #include "mailmap.h"
 #include "shortlog.h"
+#include "parse-options.h"
 
-static const char shortlog_usage[] =
-"git-shortlog [-n] [-s] [-e] [<commit-id>... ]";
+static const char * const shortlog_usage[] = {
+	"git-shortlog [-n] [-s] [-e] [<commit-id>... ]",
+	NULL
+};
 
 static int compare_by_number(const void *a1, const void *a2)
 {
@@ -188,8 +191,6 @@ static const char wrap_arg_usage[] = "-w[<width>[,<indent1>[,<indent2>]]]";
 
 static void parse_wrap_args(const char *arg, int *in1, int *in2, int *wrap)
 {
-	arg += 2; /* skip -w */
-
 	*wrap = parse_uint(&arg, ',');
 	if (*wrap < 0)
 		die(wrap_arg_usage);
@@ -212,6 +213,16 @@ static void parse_wrap_args(const char *arg, int *in1, int *in2, int *wrap)
 		die(wrap_arg_usage);
 }
 
+static int parse_wrap_callback(const struct option *opt, const char *arg,
+		int unset)
+{
+	struct shortlog *log = opt->value;
+	log->wrap_lines = 1;
+	if (arg)
+		parse_wrap_args(arg, &log->wrap, &log->in1, &log->in2);
+	return 0;
+}
+
 void shortlog_init(struct shortlog *log)
 {
 	memset(log, 0, sizeof(*log));
@@ -231,29 +242,28 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 
 	shortlog_init(&log);
 
-	/* since -n is a shadowed rev argument, parse our args first */
-	while (argc > 1) {
-		if (!strcmp(argv[1], "-n") || !strcmp(argv[1], "--numbered"))
-			log.sort_by_number = 1;
-		else if (!strcmp(argv[1], "-s") ||
-				!strcmp(argv[1], "--summary"))
-			log.summary = 1;
-		else if (!strcmp(argv[1], "-e") ||
-			 !strcmp(argv[1], "--email"))
-			log.email = 1;
-		else if (!prefixcmp(argv[1], "-w")) {
-			log.wrap_lines = 1;
-			parse_wrap_args(argv[1], &log.in1, &log.in2, &log.wrap);
-		}
-		else if (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help"))
-			usage(shortlog_usage);
-		else
-			break;
-		argv++;
-		argc--;
-	}
+	struct option options[] = {
+		OPT_BOOLEAN('n', "numbered", &log.sort_by_number,
+				"sort by number"),
+		OPT_BOOLEAN('s', "summary", &log.summary,
+				"show commit count summary"),
+		OPT_BOOLEAN('e', "email", &log.email, "show email addresses"),
+		{
+			OPTION_CALLBACK, 'w', NULL, &log, "wrap",
+			"line-wrap commit message", PARSE_OPT_OPTARG,
+			parse_wrap_callback
+		},
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, options, shortlog_usage, 0);
+
 	init_revisions(&rev, prefix);
-	argc = setup_revisions(argc, argv, &rev, NULL);
+	/* setup_revisions expects to have the program name in argv[0],
+	 * but parse_options has removed it. We have to do parse_options
+	 * before setup_revisions, though, to claim "-n". */
+	argc = setup_revisions(argc+1, argv-1, &rev, NULL);
+
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
 
-- 
1.5.4.3.341.g4e44

Re: [PATCH] convert shortlog to use parse_options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The behavior should be identical, except that we now support
> option bundling (e.g., "git shortlog -nse").

Sorry, but this breaks

	git shortlog -n -s -e --no-merges v1.5.4..

Re: [PATCH] convert shortlog to use parse_options

[Jeff King]

On Sat, Mar 01, 2008 at 01:37:26AM -0800, Junio C Hamano wrote:

> > The behavior should be identical, except that we now support
> > option bundling (e.g., "git shortlog -nse").
> 
> Sorry, but this breaks
> 
> 	git shortlog -n -s -e --no-merges v1.5.4..

I didn't test this very extensively (obviously!) since it was just a
respin of an old patch. Sorry to waste your time with something that is
so obviously broken.

I started to write a patch to give parse-options a "stop processing at
the first unknown option" flag, but it was very unsatisfactory.
Specifically:

  - the final parser has to know that it's the last, and complain about
    unrecognized options. In this case, setup_revisions would be the
    second and final parser, and it of course doesn't do this (though
    perhaps one could check the residual options from setup_revisions
    manually and barf on that)

  - the fact that we have two sets of parsed options can't be
    transparent to the user. We'd stop at "--no-merges", which means you
    can't say "git shortlog --no-merges -e", which is silly. But we
    can't possibly continue, since we don't know if "-e" is another
    option or an argument there.

And it doesn't work to do the setup_revisions first, since there is a
conflict over the "-n" option (and we can't even munge the result
afterwards, since one version takes an argument and one doesn't).

So I think using parse-options here should be put on hold until we have
the revision and diff parameters in a parseopt-understandable form. I
would think we could do something like:

#define OPT__REVISION(x) \
        OPT_BOOLEAN(0, "no-merges", &(x)->no_merges, "don't show merges"),
        OPT_BOOLEAN(0, "boundary", &(x)->boundary, "show boundary commits"),
        ...

and we could have unified options tables. I seem to recall some work
being done in this area early on in the parse-options history, but I
can't seem to find any mention of it in the list archive. Pierre, does
this ring a bell?

-Peff

Re: [PATCH] convert shortlog to use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

On Sun, Mar 02, 2008 at 06:15:42AM +0000, Jeff King wrote:
> #define OPT__REVISION(x) \
>         OPT_BOOLEAN(0, "no-merges", &(x)->no_merges, "don't show merges"),
>         OPT_BOOLEAN(0, "boundary", &(x)->boundary, "show boundary commits"),
>         ...
> 
> and we could have unified options tables. I seem to recall some work
> being done in this area early on in the parse-options history, but I
> can't seem to find any mention of it in the list archive. Pierre, does
> this ring a bell?

  Yes, I didn't had the time to finish that, I just started some ground
works in the diff options area, I hope I didn't lost that work, it's
probably somewhere on my public repository. Though revision parsing are
special because of --not, but I think the proper solution wrt --not and
--all in revision parsing is to ask parse-opt to "let" some options stay
as arguments, and do the final revision parsing with them kept.

  Like you may have noticed, I didn't have a lot of time for git
recently, and that's a shame :| But maybe your mail will beat me into
working on this again now that 1.5.4 is released, because I also feared
that reworking diff and revision parsing options will probably introduce
quite a few regressions, and it's rather better doing so at the
beginning of a release cycle :)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] convert shortlog to use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

On Sun, Mar 02, 2008 at 08:11:40AM +0000, Pierre Habouzit wrote:
> On Sun, Mar 02, 2008 at 06:15:42AM +0000, Jeff King wrote:
> > #define OPT__REVISION(x) \
> >         OPT_BOOLEAN(0, "no-merges", &(x)->no_merges, "don't show merges"),
> >         OPT_BOOLEAN(0, "boundary", &(x)->boundary, "show boundary commits"),
> >         ...
> > 
> > and we could have unified options tables. I seem to recall some work
> > being done in this area early on in the parse-options history, but I
> > can't seem to find any mention of it in the list archive. Pierre, does
> > this ring a bell?

  In fact everything is here:
http://git.madism.org/?p=~madcoder/git.git;a=shortlog;h=refs/heads/ph/parseopt

  I just resent the first patch that I sent (with a bug) not so long
time ago. The last one is the current state of work for diff opts.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]