I guess I should have searched the list _before_ creating these patches
since I just now stumbled upon some of the questions about how this
should be done for example:

http://kerneltrap.org/mailarchive/git/2008/3/1/1035344

From my testing this seems to work fine, but I may have missed a use
case.  I actually created these patches because I was annoyed that:

git shortlog --author=bohrer -s HEAD

didn't work, and this also fixes that issue.

--
Shawn
--

[ message continues ]

This adds the PARSE_OPT_NO_ERROR_ON_UNKNOWN flag which prevents
parse_options() from erroring out when it finds an unknown option,
and leaves the original command and unknown options in argv.

This option is useful if the option parsing needs to be done in
multiple stages for example if the remaining options will be passed
to additional git commands.

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 parse-options.c |   25 ++++++++++++++++++++-----
 parse-options.h |    5 +++--
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 8071711..2635e18 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -131,7 +131,8 @@ static int get_value(struct optparse_t *p,
 	}
 }
 
-static int parse_short_opt(struct optparse_t *p, const struct option *options)
+static int parse_short_opt(struct optparse_t *p, const struct option *options,
+			   int flags)
 {
 	for (; options->type != OPTION_END; options++) {
 		if (options->short_name == *p->opt) {
@@ -139,11 +140,16 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)
 			return get_value(p, options, OPT_SHORT);
 		}
 	}
+
+	if (flags & PARSE_OPT_NO_ERROR_ON_UNKNOWN) {
+		p->out[p->cpidx++] = p->argv[0];
+		return 0;
+	}
 	return error("unknown switch `%c'", *p->opt);
 }
 
 static int parse_long_opt(struct optparse_t *p, const char *arg,
-                          const struct option *options)
+                          const struct option *options, int flags)
 {
 	const char *arg_end = strchr(arg, '=');
 	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
@@ -224,6 +230,11 @@ is_abbreviated:
 			abbrev_option->long_name);
 	if (abbrev_option)
 		return get_value(p, abbrev_option, abbrev_flags);
+
+	if (flags & PARSE_OPT_NO_ERROR_ON_UNKNOWN) {
+		p->out[p->cpidx++] = p->argv[0];
+		return 0;
+	}
 	return error("unknown option `%s'", arg);
 }
 
@@ -254,6 +265,8 @@ int parse_options(int argc, const char **argv, ...
[ message continues ]

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 builtin-shortlog.c |   54 +++++++++++++++++++++++++++------------------------
 1 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index e6a2865..b1087b5 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] [-w] [<commit-id>... ]";
+static const char *const shortlog_usage[] = {
+	"git-shortlog [-n] [-s] [-e] [-w] [<commit-id>... ]",
+	NULL
+};
 
 static int compare_by_number(const void *a1, const void *a2)
 {
@@ -189,8 +192,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);
@@ -230,35 +231,38 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 	struct shortlog log;
 	struct rev_info rev;
 	int nongit;
+	const char * wrap_options = NULL;
+	struct option options[] = {
+		OPT_BOOLEAN('n', "numbered", &log.sort_by_number,
+			    "sort by number"),
+		OPT_BOOLEAN('s', "summary", &log.summary,
+			    "only provide commit count summary"),
+		OPT_BOOLEAN('e', "email", &log.email,
+			    "show email address of author"),
+		{ OPTION_STRING, 'w', NULL, &wrap_options,
+		  "[<width>[,<indent1>[,<indent2>]]]", "linewrap the output",
+		  PARSE_OPT_OPTARG, NULL, (intptr_t)"-w" },
+		OPT_END()
+	};
 
 	prefix = setup_git_directory_gently(&nongit);
 	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 = ...
[ message continues ]

I have to say that this conceptually is broken.  How would you tell
without knowing what "--flag" is if the thing in argv[] after that is a
parameter to that option or the end of the options?
--

[ message continues ]

Agreed. I was just about to write the same thing. As it happens, I think
in the case of git-shortlog that there is not likely to be such a
parameter. The only three I see looking over setup_revisions are "-n"
(which is masked by shortlog anyway), "--default", and "-U" (which one
would never need with shortlog).

However I am still opposed to the concept, since its presence as a
parseopt flag implies that it isn't fundamentally broken.

I think the only right way to accomplish this is to convert the revision
and diff parameters into a parseopt-understandable format.

-Peff
--

[ message continues ]

BTW, looking in my personal repo, I have the start of the exact same
patch (except I called it PARSE_OPT_STOP_AT_UNKNOWN). I think I
abandoned it when I realized the fundamental flaw with the approach, but
I guess I never got it to the point of sharing with the list.

-Peff
--

[ message continues ]

Not necessarily.  You could structure individual option parsers like how
diff option parsers are done.  You iterate over argv[], feed diff option
parser the current index into argv[] and ask if it is an option diff
understands, have diff eat the option (and possibly its parameter) to
advance the index, or allow diff option to say "I do not understand this",
and then handle it yourself or hand it to other parsers.


--

[ message continues ]

Hi,


AFAIR Pierre tried a few ways, and settled with a macro to introduce the 
diff options into a caller's options.

IOW it would look something like this:

static struct option builtin_what_options[] = {
	[... options specific to this command ...]
	DIFF__OPT(&diff_options)
};

Ciao,
Dscho

--

[ message continues ]

I think that is the more painful approach Jeff mentioned, and my comment
was to show that it is not the only way.



--

[ message continues ]

It seems to me that you could implement Jeff's
PARSE_OPT_STOP_AT_UNKNOWN, and then if multiple option parsers are
needed you would simply loop over parse_options for each of the
commands, waiting for argc to stop changing.  Of course Jeff's flag
would also need to stop parse_options from eating the first argument.
Is this sort of what you are suggesting Junio?

--
Shawn
--

[ message continues ]

Hi,


I believe not.  I think that Junio prefers some callback that can handle a 
whole bunch of options (as opposed to the callback we can have now, to 
handle arguments for a specific option).

However, I am not sure what that would buy us over the approach Pierre 
settled.  Junio, maybe you thought that the option parsing macro would 
live in parse-options.h?  It was supposed to live in diff.h and 
revision.h, respectively.

Ciao,
Dscho

--

[ message continues ]

Sorry, no.  I do not want callbacks.  I've been saying that parser
cascading is easier if you use an incremental interface like diff option
parser does.

--

[ message continues ]

Now I'm confused: my understanding is that the diff option parser just
leaves unrecognized stuff in argv. But isn't that what a
PARSE_OPTIONS_IGNORE_UNKNOWN flag would do, and isn't that wrong?

-Peff
--

[ message continues ]

I was thinking more about the way how the lower level diff_opt_parse()
works by letting the caller to handle things that it itself does not know
how.

But I say this because I am not interested in "-a -b -c <=> -abc" and
haven't thought about how you would go about parsing something like that
sanely with partial knowledge.
--

[ message continues ]

Hi,


Sorry, I misunderstood.  At least you clarified it for me now.

Thanks,
Dscho

--

[ message continues ]

If you do that, you need to relocate pars option structures, and we
decided some time ago that it wasn't a good idea. Note that "recursing"
is not really trivial, because with flags aggregation and stuff like
that, things that look like an option can also be a value in the context
of an other option parser.

  That's why we settled for the other way Dscho pointed. But for that, I
need to work on it more than what I really have time to nowadays, and
moreover, it needs some things (the setup_revisions split and the log
traversal bits change) to be merged.

--=20
=C2=B7O=C2=B7  Pierre Habouzit
=C2=B7=C2=B7O                                                madcoder@debia=
n.org
OOO                                                http://www.madism.org
[ message continues ]

Note that I was just saying "not necessarily" in response to "the only
right way" to point out it is not the _only_ way.

Parse-options has been done in a tablish way and it would involve cost to
modify it in a way I outlined (even if such a rewrite would make chaining
different set of option parsers easier, as each parser needs to handle
only what it knows about and handling aggregation and stuff would become
trivial).  I do not know if it is worth the cost, and I am not married to
the option parser structure that diff and revision part uses.

--

[ message continues ]