login
Search
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 --
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, ...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 = ...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? --
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 --
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 --
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. --
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
--
I think that is the more painful approach Jeff mentioned, and my comment was to show that it is not the only way. --
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 --
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 --
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. --
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 --
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. --
Hi, Sorry, I misunderstood. At least you clarified it for me now. Thanks, Dscho --
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
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. --
