Convert 'git blame' to parse_options()

Linus Torvalds· Jun 23, 2008, 05:15 UTC · lore

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

	setup_revisions(unk, argv, &revs, NULL);

Hmm?

 builtin-blame.c |  177 +++++++++++++++++++++++++++----------------------------
 1 files changed, 86 insertions(+), 91 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index b451f6c..f04bd93 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -18,24 +18,11 @@
 #include "cache-tree.h"
 #include "path-list.h"
 #include "mailmap.h"
+#include "parse-options.h"
 
-static char blame_usage[] =
-"git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S <revs-file>] [-M] [-C] [-C] [--contents <filename>] [--incremental] [commit] [--] file\n"
-"  -c                  Use the same output mode as git-annotate (Default: off)\n"
-"  -b                  Show blank SHA-1 for boundary commits (Default: off)\n"
-"  -l                  Show long commit SHA1 (Default: off)\n"
-"  --root              Do not treat root commits as boundaries (Default: off)\n"
-"  -t                  Show raw timestamp (Default: off)\n"
-"  -f, --show-name     Show original filename (Default: auto)\n"
-"  -n, --show-number   Show original linenumber (Default: off)\n"
-"  -s                  Suppress author name and timestamp (Default: off)\n"
-"  -p, --porcelain     Show in a format designed for machine consumption\n"
-"  -w                  Ignore whitespace differences\n"
-"  -L n,m              Process only line range n,m, counting from 1\n"
-"  -M, -C              Find line movements within and across files\n"
-"  --incremental       Show blame entries as we find them, incrementally\n"
-"  --contents file     Use <file>'s contents as the final image\n"
-"  -S revs-file        Use revisions from revs-file instead of calling git-rev-list\n";
+static char blame_usage[] = "git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S <revs-file>] [-M] [-C] [-C] [--contents <filename>] [--incremental] [commit] [--] file";
+
+static const char *blame_opt_usage[] = { blame_usage, NULL };
 
 static int longest_file;
 static int longest_author;
@@ -2122,6 +2109,50 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 	return commit;
 }
 
+static int blame_copy_callback(const struct option *option, const char *arg, int unset)
+{
+	int *opt = option->value;
+
+	/*
+	 * -C enables copy from removed files;
+	 * -C -C enables copy from existing files, but only
+	 *       when blaming a new file;
+	 * -C -C -C enables copy from existing files for
+	 *          everybody
+	 */
+	if (*opt & PICKAXE_BLAME_COPY_HARDER)
+		*opt |= PICKAXE_BLAME_COPY_HARDEST;
+	if (*opt & PICKAXE_BLAME_COPY)
+		*opt |= PICKAXE_BLAME_COPY_HARDER;
+	*opt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE;
+
+	if (arg)
+		blame_copy_score = parse_score(arg);
+	return 0;
+}
+
+static int blame_move_callback(const struct option *option, const char *arg, int unset)
+{
+	int *opt = option->value;
+
+	*opt |= PICKAXE_BLAME_MOVE;
+
+	if (arg)
+		blame_move_score = parse_score(arg);
+	return 0;
+}
+
+static int blame_bottomtop_callback(const struct option *option, const char *arg, int unset)
+{
+	const char **bottomtop = option->value;
+	if (!arg)
+		return -1;
+	if (*bottomtop)
+		die("More than one '-L n,m' option given");
+	*bottomtop = arg;
+	return 0;
+}
+
 int cmd_blame(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
@@ -2129,94 +2160,58 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct scoreboard sb;
 	struct origin *o;
 	struct blame_entry *ent;
-	int i, seen_dashdash, unk, opt;
+	int i, seen_dashdash, unk;
 	long bottom, top, lno;
-	int output_option = 0;
-	int show_stats = 0;
-	const char *revs_file = NULL;
 	const char *final_commit_name = NULL;
 	enum object_type type;
-	const char *bottomtop = NULL;
-	const char *contents_from = NULL;
+
+	static const char *bottomtop = NULL;
+	static int output_option = 0, opt = 0;
+	static int show_stats = 0;
+	static const char *revs_file = NULL;
+	static const char *contents_from = NULL;
+	static const struct option options[] = {
+		OPT_BOOLEAN(0, "incremental", &incremental, "Show blame entries as we find them, incrementally"),
+		OPT_BOOLEAN('b', NULL, &blank_boundary, "Show blank SHA-1 for boundary commits (Default: off)"),
+		OPT_BOOLEAN(0, "root", &show_root, "Do not treat root commits as boundaries (Default: off)"),
+		OPT_BOOLEAN(0, "show-stats", &show_stats, "Show work cost statistics"),
+		OPT_BIT(0, "score-debug", &output_option, "Show output score for blame entries", OUTPUT_SHOW_SCORE),
+		OPT_BIT('f', "show-name", &output_option, "Show original filename (Default: auto)", OUTPUT_SHOW_NAME),
+		OPT_BIT('n', "show-number", &output_option, "Show original linenumber (Default: off)", OUTPUT_SHOW_NUMBER),
+		OPT_BIT('p', "porcelain", &output_option, "Show in a format designed for machine consumption", OUTPUT_PORCELAIN),
+		OPT_BIT('c', NULL, &output_option, "Use the same output mode as git-annotate (Default: off)", OUTPUT_ANNOTATE_COMPAT),
+		OPT_BIT('t', NULL, &output_option, "Show raw timestamp (Default: off)", OUTPUT_RAW_TIMESTAMP),
+		OPT_BIT('l', NULL, &output_option, "Show long commit SHA1 (Default: off)", OUTPUT_LONG_OBJECT_NAME),
+		OPT_BIT('s', NULL, &output_option, "Suppress author name and timestamp (Default: off)", OUTPUT_NO_AUTHOR),
+		OPT_BIT('w', NULL, &xdl_opts, "Ignore whitespace differences", XDF_IGNORE_WHITESPACE),
+		OPT_STRING('S', NULL, &revs_file, "file", "Use revisions from <file> instead of calling git-rev-list"),
+		OPT_STRING(0, "contents", &contents_from, "file", "Use <file>'s contents as the final image"),
+		{ OPTION_CALLBACK, 'C', NULL, &opt, "score", "Find line copies within and across files", PARSE_OPT_OPTARG, blame_copy_callback },
+		{ OPTION_CALLBACK, 'M', NULL, &opt, "score", "Find line movements within and across files", PARSE_OPT_OPTARG, blame_move_callback },
+		OPT_CALLBACK('L', NULL, &bottomtop, "n,m", "Process only line range n,m, counting from 1", blame_bottomtop_callback),
+		OPT_END()
+	};
 
 	cmd_is_annotate = !strcmp(argv[0], "annotate");
 
+	argc = parse_options(argc, argv, options, blame_opt_usage,
+		PARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH);
+
+	/*
+	 * parse_options() offsets things by one - undo for now to make
+	 * old code happy.
+	 */
+	argc++;
+	argv--;
+
 	git_config(git_blame_config, NULL);
 	save_commit_buffer = 0;
 
-	opt = 0;
 	seen_dashdash = 0;
 	for (unk = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (*arg != '-')
 			break;
-		else if (!strcmp("-b", arg))
-			blank_boundary = 1;
-		else if (!strcmp("--root", arg))
-			show_root = 1;
-		else if (!strcmp(arg, "--show-stats"))
-			show_stats = 1;
-		else if (!strcmp("-c", arg))
-			output_option |= OUTPUT_ANNOTATE_COMPAT;
-		else if (!strcmp("-t", arg))
-			output_option |= OUTPUT_RAW_TIMESTAMP;
-		else if (!strcmp("-l", arg))
-			output_option |= OUTPUT_LONG_OBJECT_NAME;
-		else if (!strcmp("-s", arg))
-			output_option |= OUTPUT_NO_AUTHOR;
-		else if (!strcmp("-w", arg))
-			xdl_opts |= XDF_IGNORE_WHITESPACE;
-		else if (!strcmp("-S", arg) && ++i < argc)
-			revs_file = argv[i];
-		else if (!prefixcmp(arg, "-M")) {
-			opt |= PICKAXE_BLAME_MOVE;
-			blame_move_score = parse_score(arg+2);
-		}
-		else if (!prefixcmp(arg, "-C")) {
-			/*
-			 * -C enables copy from removed files;
-			 * -C -C enables copy from existing files, but only
-			 *       when blaming a new file;
-			 * -C -C -C enables copy from existing files for
-			 *          everybody
-			 */
-			if (opt & PICKAXE_BLAME_COPY_HARDER)
-				opt |= PICKAXE_BLAME_COPY_HARDEST;
-			if (opt & PICKAXE_BLAME_COPY)
-				opt |= PICKAXE_BLAME_COPY_HARDER;
-			opt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE;
-			blame_copy_score = parse_score(arg+2);
-		}
-		else if (!prefixcmp(arg, "-L")) {
-			if (!arg[2]) {
-				if (++i >= argc)
-					usage(blame_usage);
-				arg = argv[i];
-			}
-			else
-				arg += 2;
-			if (bottomtop)
-				die("More than one '-L n,m' option given");
-			bottomtop = arg;
-		}
-		else if (!strcmp("--contents", arg)) {
-			if (++i >= argc)
-				usage(blame_usage);
-			contents_from = argv[i];
-		}
-		else if (!strcmp("--incremental", arg))
-			incremental = 1;
-		else if (!strcmp("--score-debug", arg))
-			output_option |= OUTPUT_SHOW_SCORE;
-		else if (!strcmp("-f", arg) ||
-			 !strcmp("--show-name", arg))
-			output_option |= OUTPUT_SHOW_NAME;
-		else if (!strcmp("-n", arg) ||
-			 !strcmp("--show-number", arg))
-			output_option |= OUTPUT_SHOW_NUMBER;
-		else if (!strcmp("-p", arg) ||
-			 !strcmp("--porcelain", arg))
-			output_option |= OUTPUT_PORCELAIN;
 		else if (!strcmp("--", arg)) {
 			seen_dashdash = 1;
 			i++;

Junio C Hamano· Jun 23, 2008, 06:35 UTC · re: Linus Torvalds · lore

Re: Convert 'git blame' to parse_options()

Linus Torvalds <torvalds@linux-foundation.org> writes:

> That builtin-blame option parsing is really ugly,...

> +static int blame_bottomtop_callback(const struct option *option, const char *arg, int unset)
> +{
> +	const char **bottomtop = option->value;
> +	if (!arg)
> +		return -1;
> +	if (*bottomtop)
> +		die("More than one '-L n,m' option given");
> +	*bottomtop = arg;
> +	return 0;
> +}

Johannes Schindelin· Jun 23, 2008, 12:28 UTC · re: Junio C Hamano · lore

Re: Convert 'git blame' to parse_options()

Hi,

On Sun, 22 Jun 2008, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > +static int blame_bottomtop_callback(const struct option *option, const char *arg, int unset)
> > +{
> > +	const char **bottomtop = option->value;
> > +	if (!arg)
> > +		return -1;
> > +	if (*bottomtop)
> > +		die("More than one '-L n,m' option given");
> > +	*bottomtop = arg;
> > +	return 0;
> > +}
> 
> Hmmmm.  I actually wanted to eventually allow more than one -L so that we
> can blame two functions inside a file, for example.  Would this make it
> even harder, I have to wonder...

Pierre Habouzit· Jun 23, 2008, 08:22 UTC · re: Linus Torvalds · lore

[RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 05:15:41AM +0000, Linus Torvalds wrote:

> I think I'll want to add a PARSE_OPT_IGNORE_UNRECOGNIZED flag, and also 
> make it not write the resulting array into argv[0..] but back into 
> argv[1..] (so that you can use parse_options() as a _filter_ for options 
> parsing and make it easier to do partial conversions), but in the meantime 
> this mostly works.

enum {
    FLAG_ERROR = -1,
    FLAG_NOT_FOR_ME,
    FLAG_IS_FOR_ME,
    FLAG_AND_VALUE_ARE_FOR_ME,
}

int (*parse_opt_unknown_cb)(int shortopt, const char *longopt,
                            const char *value, void *priv);

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

Johannes Schindelin· Jun 23, 2008, 12:26 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Hi,

On Mon, 23 Jun 2008, Pierre Habouzit wrote:

> This "PARSE_OPT_IGNORE_UNRECOGNIZED" thing has been discussed many times 
> in the past, but it just doesn't fly.
> 
> Though to help migrations we can probably introduce a new parse option
> construct that would be a callback that is responsible for dealing with
> "things" the upper level parser doesn't know about, something where the
> callback could be:
> 
> enum {
>     FLAG_ERROR = -1,
>     FLAG_NOT_FOR_ME,
>     FLAG_IS_FOR_ME,
>     FLAG_AND_VALUE_ARE_FOR_ME,
> }
> 
> int (*parse_opt_unknown_cb)(int shortopt, const char *longopt,
>                             const char *value, void *priv);

However, I think it buys us more trouble than it saves us.

Thinking about the recursive approach again, I came up with this POC:

Subtables can recurse into subtables, too.

Example:

	#define null ((struct diff_options *)NULL)
	struct option diff__options[] = {
		OPT_INTEGER('l', NULL, &null->rename_limit,
			"set rename limit"),
		[...]
	};

	struct option diff_files__options[] = {
		[...]
		OPTION_OPTIONS(diff__options, &revopt.diff_options),
		[...]
	};

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 parse-options.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
 parse-options.h      |    5 +++++
 test-parse-options.c |   16 ++++++++++++++++
 3 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index b8bde2b..c13c503 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1,5 +1,6 @@
 #include "git-compat-util.h"
 #include "parse-options.h"
+#include "cache.h"
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
@@ -247,6 +248,41 @@ void check_typos(const char *arg, const struct option *options)
 	}
 }
 
+struct growable_options {
+	int nr, alloc;
+	struct option *options;
+};
+
+static void *add_base_var(void *value, void *base_var)
+{
+	return (void *)(((char *)base_var) +
+			(((char *)value) - ((char *)NULL)));
+}
+
+static void expand_option_tables(const struct option *options, void *base_var,
+		struct growable_options *result, int add_option_end)
+{
+	for (; options->type != OPTION_END; options++) {
+		if (options->type == OPTION_OPTIONS)
+			expand_option_tables(options->option_table,
+					add_base_var(options->value, base_var),
+					result, 0);
+		else {
+			ALLOC_GROW(result->options,
+					result->nr + 1, result->alloc);
+			result->options[result->nr] = *options;
+			if (base_var)
+				result->options[result->nr].value =
+					add_base_var(options->value, base_var);
+			result->nr++;
+		}
+	}
+	if (add_option_end) {
+		ALLOC_GROW(result->options, result->nr + 1, result->alloc);
+		result->options[result->nr++].type = OPTION_END;
+	}
+}
+
 static NORETURN void usage_with_options_internal(const char * const *,
                                                  const struct option *, int);
 
@@ -254,6 +290,10 @@ int parse_options(int argc, const char **argv, const struct option *options,
                   const char * const usagestr[], int flags)
 {
 	struct optparse_t args = { argv + 1, argv, argc - 1, 0, NULL };
+	struct growable_options growable_options = { 0, 0, NULL };
+
+	expand_option_tables(options, NULL, &growable_options, 1);
+	options = growable_options.options;
 
 	for (; args.argc; args.argc--, args.argv++) {
 		const char *arg = args.argv[0];
@@ -298,6 +338,8 @@ int parse_options(int argc, const char **argv, const struct option *options,
 			usage_with_options(usagestr, options);
 	}
 
+	free(growable_options.options);
+
 	memmove(args.out + args.cpidx, args.argv, args.argc * sizeof(*args.out));
 	args.out[args.cpidx + args.argc] = NULL;
 	return args.cpidx + args.argc;
diff --git a/parse-options.h b/parse-options.h
index 4ee443d..be6e858 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -15,6 +15,8 @@ enum parse_opt_type {
 	OPTION_STRING,
 	OPTION_INTEGER,
 	OPTION_CALLBACK,
+	/* recursing into another options table */
+	OPTION_OPTIONS,
 };
 
 enum parse_opt_flags {
@@ -83,6 +85,7 @@ struct option {
 	int flags;
 	parse_opt_cb *callback;
 	intptr_t defval;
+	struct option *option_table;
 };
 
 #define OPT_END()                   { OPTION_END }
@@ -99,6 +102,8 @@ struct option {
 	  parse_opt_approxidate_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) \
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }
+#define OPT_OPTIONS(table, base_var) \
+	{ OPTION_OPTIONS, 0, NULL, base_var, NULL, NULL, 0, NULL, 0, table }
 
 /* parse_options() will filter out the processed options and leave the
  * non-option argments in argv[].
diff --git a/test-parse-options.c b/test-parse-options.c
index 2a79e72..5032e71 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -18,8 +18,21 @@ int length_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+struct some_struct {
+	int an_int;
+	char *a_string;
+};
+#define some_struct_null ((struct some_struct *)NULL)
+struct option an_option_table[] = {
+	OPT_GROUP("An option table"),
+	OPT_INTEGER('Y', NULL, &some_struct_null->an_int, "an integer"),
+	OPT_STRING(0, "a-string", &some_struct_null->a_string,
+		"a-string", "get another string"),
+};
+
 int main(int argc, const char **argv)
 {
+	struct some_struct some = { 0, NULL };
 	const char *usage[] = {
 		"test-parse-options <options>",
 		NULL
@@ -42,6 +55,7 @@ int main(int argc, const char **argv)
 		OPT_STRING('o', NULL, &string, "str", "get another string"),
 		OPT_SET_PTR(0, "default-string", &string,
 			"set string to default", (unsigned long)"default"),
+		OPT_OPTIONS(an_option_table, (void *)&some),
 		OPT_GROUP("Magic arguments"),
 		OPT_ARGUMENT("quux", "means --quux"),
 		OPT_GROUP("Standard options"),
@@ -62,6 +76,8 @@ int main(int argc, const char **argv)
 	printf("verbose: %d\n", verbose);
 	printf("quiet: %s\n", quiet ? "yes" : "no");
 	printf("dry run: %s\n", dry_run ? "yes" : "no");
+	printf("an int: %d\n", some.an_int);
+	printf("a string: %s\n", some.a_string);
 
 	for (i = 0; i < argc; i++)
 		printf("arg %02d: %s\n", i, argv[i]);

-- 
1.5.6.127.g3fb9f
-- snap --


Maybe this is food for thought.

Ciao,
Dscho

Pierre Habouzit· Jun 23, 2008, 15:53 UTC · re: Johannes Schindelin · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 12:26:41PM +0000, Johannes Schindelin wrote:

> Hi,
> 
> On Mon, 23 Jun 2008, Pierre Habouzit wrote:
> 
> > This "PARSE_OPT_IGNORE_UNRECOGNIZED" thing has been discussed many times 
> > in the past, but it just doesn't fly.
> > 
> > Though to help migrations we can probably introduce a new parse option
> > construct that would be a callback that is responsible for dealing with
> > "things" the upper level parser doesn't know about, something where the
> > callback could be:
> > 
> > enum {
> >     FLAG_ERROR = -1,
> >     FLAG_NOT_FOR_ME,
> >     FLAG_IS_FOR_ME,
> >     FLAG_AND_VALUE_ARE_FOR_ME,
> > }
> > 
> > int (*parse_opt_unknown_cb)(int shortopt, const char *longopt,
> >                             const char *value, void *priv);
> 
> I believe that this is what Junio was talking about when he mentioned 
> callbacks.
> 
> However, I think it buys us more trouble than it saves us.
> 
> Thinking about the recursive approach again, I came up with this POC:

  Well I proposed something like that in the past, and we believed it to
be too cumbersome. I can live with it well fwiw, but it doesn't solve
the issue of migrating a very complex option parsing chain to
parse-options well IMHO. THe big problem with diff and rev opt parsing
is that one you want to migrate _any_ of the commands, you have to
migrate _all_ of them, which is huge.

  I believe the callback proposal as an _intermediate_ step allows a
better "upgrade" path. We can then converge to this proposal, or a big
fat macro, I don't care what we do in the end, but as one guy that
migrated quite a few commands, I care about the work being doable in an
incremental way.

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

Johannes Schindelin· Jun 23, 2008, 16:25 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Hi,

On Mon, 23 Jun 2008, Pierre Habouzit wrote:

> On Mon, Jun 23, 2008 at 12:26:41PM +0000, Johannes Schindelin wrote:
> 
> > On Mon, 23 Jun 2008, Pierre Habouzit wrote:
> > 
> > > This "PARSE_OPT_IGNORE_UNRECOGNIZED" thing has been discussed many 
> > > times in the past, but it just doesn't fly.
> > > 
> > > Though to help migrations we can probably introduce a new parse 
> > > option construct that would be a callback that is responsible for 
> > > dealing with "things" the upper level parser doesn't know about, 
> > > something where the callback could be:
> > > 
> > > enum {
> > >     FLAG_ERROR = -1,
> > >     FLAG_NOT_FOR_ME,
> > >     FLAG_IS_FOR_ME,
> > >     FLAG_AND_VALUE_ARE_FOR_ME,
> > > }
> > > 
> > > int (*parse_opt_unknown_cb)(int shortopt, const char *longopt,
> > >                             const char *value, void *priv);
> > 
> > I believe that this is what Junio was talking about when he mentioned 
> > callbacks.
> > 
> > However, I think it buys us more trouble than it saves us.
> > 
> > Thinking about the recursive approach again, I came up with this POC:
> 
>   Well I proposed something like that in the past, and we believed it to
> be too cumbersome.

Note: it is a bit wasteful, allocating space for a new option table 
in every case, even if there are no OPTION_OPTIONS, but I think that is 
okay.

> I can live with it well fwiw, but it doesn't solve the issue of 
> migrating a very complex option parsing chain to parse-options well 
> IMHO. THe big problem with diff and rev opt parsing is that one you want 
> to migrate _any_ of the commands, you have to migrate _all_ of them, 
> which is huge.

I don't think you have to migrate all of them in one go.  To the contrary, 
if we have both "diff_opt_parse()" as well as a "struct options 
*diff__options", it can be done one by one.  During that time, though, 
people would have to add new diff options to both places.
 
Ciao,
Dscho

Linus Torvalds· Jun 23, 2008, 16:25 UTC · re: Johannes Schindelin · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Johannes Schindelin wrote:

> 
> Thinking about the recursive approach again, I came up with this POC:

"recursive" is pointless.

Let me explain.

It should be perfectly possible to change that argument loop from

	for (i = 1; i < argc; i++) {
		const char *arg = argv[i];
		if (strcmp(arg, "-")) {
			.. handle <stdin> ..
			continue;
		}
		...

to doing something like this:

	for (;;) {
		const char *arg;
		argc = parse_options(argc, argv,
			options, usage, PARSE_OPT_STOP_AT_UNKNOWN);
		if (!argc)
			break;
		arg = argv[1];
		argv++;
		argc--;
		if (strcmp(arg, "-")) {
			.. handle <stdin> ..
			continue;
		}
		...	

or whatever. See?

			Linus

Jeff King· Jun 23, 2008, 16:49 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 09:25:10AM -0700, Linus Torvalds wrote:

> Could you handle the "recursive" use of parse_options() in builtin-blame.c 
> by teaching it about recursion? Yes. But again, it's just _simpler_ to 
> just teach parse_options() to parse the things it knows about, and leave 
> the other things in place.

  git foo -b -a

?

-Peff

Linus Torvalds· Jun 23, 2008, 17:06 UTC · re: Jeff King · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Jeff King wrote:

> On Mon, Jun 23, 2008 at 09:25:10AM -0700, Linus Torvalds wrote:
> 
> > Could you handle the "recursive" use of parse_options() in builtin-blame.c 
> > by teaching it about recursion? Yes. But again, it's just _simpler_ to 
> > just teach parse_options() to parse the things it knows about, and leave 
> > the other things in place.
> 
> If I know that I have option "-a", what is the correct partial parsing
> of:
> 
>   git foo -b -a
> 
> ?

The problem with parse_options() right now is:

 - it cannot do this at all (it can stop or ignore *non*arguments, but 
   not things that start with "-" and will always error out)

 - it actually puts the result somewhere different than the source, which 
   makes it very annoying to work with together with anything else that 
   also wants to look at, or change, argv/argc.

   IOW, right now it takes it's arguments from argv[1...], but then puts 
   back the remainder into argv[0...]. Similarly, it takes a "count plus 
   one" value of argc, but then _returns_ a "count plus zero".

[ IOW, it would never ever return "1". If there is one argument left in 
  argv[1], it returns 2 because it counts "argv[0]" even if it doesn't 
  _use_ it. That's exactly the same way argv/argc works normally. But 
  then, if it actually used up everything, it wouldn't return 1, but 
  return 0 to make it

   (a) easier and more obvious to test for "done" in general

   (b) easier to convert existing users that basically expect a zero 
       return value to mean "I ate all the arguments". Many existing users 
       of "parse_options()" don't care about anything else, and wouldn't 
       need any changes at all.

  but I haven't actually looked too closely yet to be able to say whether 
  this would avoid the bulk of the problems with changing the existing 
  practice of 'parse_option()' callers. ]

Hmm?

		Linus

Jeff King· Jun 23, 2008, 17:15 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 10:06:52AM -0700, Linus Torvalds wrote:

> You'd start off with argv[] looking like [ "foo" "-b" "-a" ] and then 
> after calling parse_options with that, depending on whether it has 
> PARSE_OPT_CONTINUE_ON_UNKNOWN or not, you'd either end up with the "-a" 
> handled (and argv[] now being just [ "foo" "-b" ]), or if you have 

> PARSE_OPT_STOP_ON_UNKNOWN then parse_options() would return without having 
> done anything, and expecting you to handle the unknown option first and 
> then restarting the argument parsing.

-Peff

Linus Torvalds· Jun 23, 2008, 17:32 UTC · re: Jeff King · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Jeff King wrote:

> 
> How can that be correct, if you don't know whether "-b" takes an
> argument?

Did you read my post or not?

If you have that case, then use STOP_ON_UNKNOWN.

> That is the only thing that makes sense to me, since the command line
> has to be parsed left-to-right (because the syntactic function of an
> element relies on the semantics of the element to its left).

Try just looking at the code!

So can we please stop the clusterfuck now?

		Linus

Jeff King· Jun 23, 2008, 18:15 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 10:32:21AM -0700, Linus Torvalds wrote:

> > How can that be correct, if you don't know whether "-b" takes an
> > argument?
> 
> Did you read my post or not?
> 
> If you have that case, then use STOP_ON_UNKNOWN.

> Umm. Helloo, reality.. There are actually very few options that take a 
> flag for their arguments. In particular, the option parsing we really 
> _care_ about (revision parsing - see builtin-blame.c which is exactly 
> where I wanted to convert things) very much DOES NOT.

Reality: revision.c, lines 1008-1012. "-n" takes an argument.
Reality: revision.c, lines 1075-1080. "--default" takes an argument.

> Try just looking at the code!

  http://thread.gmane.org/gmane.comp.version-control.git/85354/focus=85355

> So I'm really not interested in arguing about "theoretical issues", when 
> we have a real-life *practical* issue to solve.
> 
> Solve builtin-blame.c for me. I sent out a patch yesterday, but in the 
> description of that patch I also described exactly why I want 
> CONTINUE_ON_UNKNOWN.

-Peff

Linus Torvalds· Jun 23, 2008, 18:36 UTC · re: Jeff King · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Jeff King wrote:

> 
> There is already a discussion underway about the proper solution.

No there isn't, not any I have seen.

		Linus

Linus Torvalds· Jun 23, 2008, 18:20 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Linus Torvalds wrote:

> 
> Umm. Helloo, reality.. There are actually very few options that take a 
> flag for their arguments. In particular, the option parsing we really 
> _care_ about (revision parsing - see builtin-blame.c which is exactly 
> where I wanted to convert things) very much DOES NOT.

	git blame --since=April -b Makefile

And guess what you need to fix it?

If you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize. 

		Linus

Jeff King· Jun 23, 2008, 18:33 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 11:20:46AM -0700, Linus Torvalds wrote:

> Actually, I guess "--default" does, but if you try to mix that up with (a) 
> a default head that starts with a dash and (b) git-blame, you're doing 
> something pretty odd.

Yes, and I think that is why "in practice" this works with git-blame.

> And yes, "-n" does too, but if you pass it negative numbers you get what 
> you deserve.

  $ git blame -n 1 git.c
  fatal: bad revision '1'

  $ git blame --default HEAD git.c
  fatal: cannot stat path HEAD: No such file or directory

> The point being, we really _do_ have a real-life existing case for 
> PARSE_OPT_CONTINUE_ON_UNKNOWN, which is hard to handle any other way. 
> Currently you can literally do
> 
> 	git blame --since=April -b Makefile
> 
> and while it's a totally made-up example, it's one I've picked to show 
> exactly what does *not* work with my patch that started this whole thread.
> 
> And guess what you need to fix it?
> 
> If you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize. 

-Peff

Linus Torvalds· Jun 23, 2008, 18:47 UTC · re: Jeff King · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Jeff King wrote:

> 
> It's worse than that. We assume by default that the option has no
> argument, so the argument becomes a non-option parameter to the original
> command. Try (with current git-blame, but I think your patch doesn't
> change this):
> 
>   $ git blame -n 1 git.c
>   fatal: bad revision '1'
> 
>   $ git blame --default HEAD git.c
>   fatal: cannot stat path HEAD: No such file or directory
> 
> Oops.

Oops. And then, how would you fix this most easily?

Be honest now.

Hint: it _still_ involves PARSE_OPT_CONTINUE_ON_UNKNOWN. It would fix both 
cases.

IOW, the whole builtin-blame.c option parsing *should* look like this:

	argc = parse_options(argc, argv, options, usage, PARSE_OPT_CONTINUE_ON_UNKNOWN);
	init_revisions(&revs, NULL);
	setup_revisions(argc, argv, &revs, NULL);

and it should just work.

Example: many arguments cause multiple option variables to change. 
parse_options() simply can't handle that well - you can do it with a 
callback, but then you need to make the option variables global or make 
them a structure or something. All of which just makes it nasty to do 
partial conversions for the simple cases.

Or are we going to sit around discussing this for another five months?

			Linus

Linus Torvalds· Jun 23, 2008, 19:16 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Linus Torvalds wrote:

> 
> Or are we going to sit around discussing this for another five months?

So the gauntlet is thrown.

	git blame --since=April -b Makefile

work.

	--default HEAD

because it will stop at HEAD, and then do the exact wrong thing.

		Linus

---
 builtin-blame.c |    2 +-
 parse-options.c |   26 ++++++++++++++++++++++----
 parse-options.h |    2 ++
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index f04bd93..059062c 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2195,7 +2195,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	cmd_is_annotate = !strcmp(argv[0], "annotate");
 
 	argc = parse_options(argc, argv, options, blame_opt_usage,
-		PARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH);
+		PARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN);
 
 	/*
 	 * parse_options() offsets things by one - undo for now to make
diff --git a/parse-options.c b/parse-options.c
index acf3fe3..5b4653f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -7,7 +7,7 @@
 struct optparse_t {
 	const char **argv;
 	const char **out;
-	int argc, cpidx;
+	int argc, cpidx, flags;
 	const char *opt;
 };
 
@@ -139,6 +139,8 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)
 			return get_value(p, options, OPT_SHORT);
 		}
 	}
+	if (p->flags & (PARSE_OPT_STOP_AT_UNKNOWN | PARSE_OPT_KEEP_UNKNOWN))
+		return -1;
 	return error("unknown switch `%c'", *p->opt);
 }
 
@@ -224,6 +226,8 @@ is_abbreviated:
 			abbrev_option->long_name);
 	if (abbrev_option)
 		return get_value(p, abbrev_option, abbrev_flags);
+	if (p->flags & (PARSE_OPT_STOP_AT_UNKNOWN | PARSE_OPT_KEEP_UNKNOWN))
+		return -1;
 	return error("unknown option `%s'", arg);
 }
 
@@ -253,7 +257,7 @@ static NORETURN void usage_with_options_internal(const char * const *,
 int parse_options(int argc, const char **argv, const struct option *options,
                   const char * const usagestr[], int flags)
 {
-	struct optparse_t args = { argv + 1, argv, argc - 1, 0, NULL };
+	struct optparse_t args = { argv + 1, argv, argc - 1, 0, flags, NULL };
 
 	for (; args.argc; args.argc--, args.argv++) {
 		const char *arg = args.argv[0];
@@ -269,8 +273,15 @@ int parse_options(int argc, const char **argv, const struct option *options,
 			args.opt = arg + 1;
 			if (*args.opt == 'h')
 				usage_with_options(usagestr, options);
-			if (parse_short_opt(&args, options) < 0)
+			if (parse_short_opt(&args, options) < 0) {
+				if (flags & PARSE_OPT_STOP_AT_UNKNOWN) 
+					break;
+				if (flags & PARSE_OPT_KEEP_UNKNOWN) {
+					args.out[args.cpidx++] = args.argv[0];
+					continue;
+				}
 				usage_with_options(usagestr, options);
+			}
 			if (args.opt)
 				check_typos(arg + 1, options);
 			while (args.opt) {
@@ -294,8 +305,15 @@ int parse_options(int argc, const char **argv, const struct option *options,
 			usage_with_options_internal(usagestr, options, 1);
 		if (!strcmp(arg + 2, "help"))
 			usage_with_options(usagestr, options);
-		if (parse_long_opt(&args, arg + 2, options))
+		if (parse_long_opt(&args, arg + 2, options)) {
+			if (flags & PARSE_OPT_STOP_AT_UNKNOWN)  
+				break;
+			if (flags & PARSE_OPT_KEEP_UNKNOWN) {
+				args.out[args.cpidx++] = args.argv[0];
+				continue;
+			}
 			usage_with_options(usagestr, options);
+		}
 	}
 
 	memmove(args.out + args.cpidx, args.argv, args.argc * sizeof(*args.out));
diff --git a/parse-options.h b/parse-options.h
index 4ee443d..a98f0ec 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -20,6 +20,8 @@ enum parse_opt_type {
 enum parse_opt_flags {
 	PARSE_OPT_KEEP_DASHDASH = 1,
 	PARSE_OPT_STOP_AT_NON_OPTION = 2,
+	PARSE_OPT_KEEP_UNKNOWN = 4,
+	PARSE_OPT_STOP_AT_UNKNOWN = 8,
 };
 
 enum parse_opt_option_flags {

Pierre Habouzit· Jun 23, 2008, 21:09 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 07:16:17PM +0000, Linus Torvalds wrote:

> 
> 
> On Mon, 23 Jun 2008, Linus Torvalds wrote:
> > 
> > Or are we going to sit around discussing this for another five months?
> 
> So the gauntlet is thrown.

  Let's see if I can catch it elegantly.

  Following is a series split into incremental patches to migrate the
parse_options API to an incremental one. Only the last step provides a
functional incremental API.

  With that, you write parsers this way:

{
    struct parse_opt_ctx_t ctx;

    parse_options_start(&ctx, argc, argv, 0);

    for (;;) {
        const char *arg;

        switch (parse_options_step(&ctx, options, usagestr)) {
        case PARSE_OPT_HELP:
            /* dump your help here, the one for options/usagestr is already dumped */
            exit(129);
        case PARSE_OPT_DONE:
            goto done;
        }

        arg = *ctx->argv++;
        ctx->argc--;

        if (strcmp(arg, "-")) {
            /* you're on baby ! */
        } else if ....
        } else {
            error("unknown option %s", arg);
            parse_options_usage(options, usagestr);
            /* dump your help here */
            exit(129);
        }
    }

done:
    argc = parse_options_end(&ctx);
}

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

Pierre Habouzit· Jun 23, 2008, 21:11 UTC · re: Pierre Habouzit · lore

[PATCH] parse-opt: have parse_options_{start,end}.

Make the struct optparse_t public under the better name parse_opt_ctx_t.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   69 +++++++++++++++++++++++++++++++------------------------
 parse-options.h |   16 ++++++++++++
 2 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index b8bde2b..774e647 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -4,14 +4,7 @@
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
-struct optparse_t {
-	const char **argv;
-	const char **out;
-	int argc, cpidx;
-	const char *opt;
-};
-
-static inline const char *get_arg(struct optparse_t *p)
+static inline const char *get_arg(struct parse_opt_ctx_t *p)
 {
 	if (p->opt) {
 		const char *res = p->opt;
@@ -37,7 +30,7 @@ static int opterror(const struct option *opt, const char *reason, int flags)
 	return error("option `%s' %s", opt->long_name, reason);
 }
 
-static int get_value(struct optparse_t *p,
+static int get_value(struct parse_opt_ctx_t *p,
                      const struct option *opt, int flags)
 {
 	const char *s, *arg;
@@ -131,7 +124,7 @@ 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 parse_opt_ctx_t *p, const struct option *options)
 {
 	for (; options->type != OPTION_END; options++) {
 		if (options->short_name == *p->opt) {
@@ -142,7 +135,7 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)
 	return error("unknown switch `%c'", *p->opt);
 }
 
-static int parse_long_opt(struct optparse_t *p, const char *arg,
+static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
                           const struct option *options)
 {
 	const char *arg_end = strchr(arg, '=');
@@ -247,45 +240,63 @@ void check_typos(const char *arg, const struct option *options)
 	}
 }
 
+void parse_options_start(struct parse_opt_ctx_t *ctx,
+                         int argc, const char **argv, int flags)
+{
+	memset(ctx, 0, sizeof(*ctx));
+	ctx->argc = argc - 1;
+	ctx->argv = argv + 1;
+	ctx->out  = argv;
+	ctx->flags = flags;
+}
+
+int parse_options_end(struct parse_opt_ctx_t *ctx)
+{
+	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
+	ctx->out[ctx->cpidx + ctx->argc] = NULL;
+	return ctx->cpidx + ctx->argc;
+}
+
 static NORETURN void usage_with_options_internal(const char * const *,
                                                  const struct option *, int);
 
 int parse_options(int argc, const char **argv, const struct option *options,
                   const char * const usagestr[], int flags)
 {
-	struct optparse_t args = { argv + 1, argv, argc - 1, 0, NULL };
+	struct parse_opt_ctx_t ctx;
 
-	for (; args.argc; args.argc--, args.argv++) {
-		const char *arg = args.argv[0];
+	parse_options_start(&ctx, argc, argv, flags);
+	for (; ctx.argc; ctx.argc--, ctx.argv++) {
+		const char *arg = ctx.argv[0];
 
 		if (*arg != '-' || !arg[1]) {
-			if (flags & PARSE_OPT_STOP_AT_NON_OPTION)
+			if (ctx.flags & PARSE_OPT_STOP_AT_NON_OPTION)
 				break;
-			args.out[args.cpidx++] = args.argv[0];
+			ctx.out[ctx.cpidx++] = ctx.argv[0];
 			continue;
 		}
 
 		if (arg[1] != '-') {
-			args.opt = arg + 1;
-			if (*args.opt == 'h')
+			ctx.opt = arg + 1;
+			if (*ctx.opt == 'h')
 				usage_with_options(usagestr, options);
-			if (parse_short_opt(&args, options) < 0)
+			if (parse_short_opt(&ctx, options) < 0)
 				usage_with_options(usagestr, options);
-			if (args.opt)
+			if (ctx.opt)
 				check_typos(arg + 1, options);
-			while (args.opt) {
-				if (*args.opt == 'h')
+			while (ctx.opt) {
+				if (*ctx.opt == 'h')
 					usage_with_options(usagestr, options);
-				if (parse_short_opt(&args, options) < 0)
+				if (parse_short_opt(&ctx, options) < 0)
 					usage_with_options(usagestr, options);
 			}
 			continue;
 		}
 
 		if (!arg[2]) { /* "--" */
-			if (!(flags & PARSE_OPT_KEEP_DASHDASH)) {
-				args.argc--;
-				args.argv++;
+			if (!(ctx.flags & PARSE_OPT_KEEP_DASHDASH)) {
+				ctx.argc--;
+				ctx.argv++;
 			}
 			break;
 		}
@@ -294,13 +305,11 @@ int parse_options(int argc, const char **argv, const struct option *options,
 			usage_with_options_internal(usagestr, options, 1);
 		if (!strcmp(arg + 2, "help"))
 			usage_with_options(usagestr, options);
-		if (parse_long_opt(&args, arg + 2, options))
+		if (parse_long_opt(&ctx, arg + 2, options))
 			usage_with_options(usagestr, options);
 	}
 
-	memmove(args.out + args.cpidx, args.argv, args.argc * sizeof(*args.out));
-	args.out[args.cpidx + args.argc] = NULL;
-	return args.cpidx + args.argc;
+	return parse_options_end(&ctx);
 }
 
 #define USAGE_OPTS_WIDTH 24
diff --git a/parse-options.h b/parse-options.h
index 4ee443d..db6c986 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -111,6 +111,22 @@ extern int parse_options(int argc, const char **argv,
 extern NORETURN void usage_with_options(const char * const *usagestr,
                                         const struct option *options);
 
+/*----- incremantal advanced APIs -----*/
+
+struct parse_opt_ctx_t {
+	const char **argv;
+	const char **out;
+	int argc, cpidx;
+	const char *opt;
+	int flags;
+};
+
+extern void parse_options_start(struct parse_opt_ctx_t *ctx,
+                                int argc, const char **argv, int flags);
+
+extern int parse_options_end(struct parse_opt_ctx_t *ctx);
+
+
 /*----- some often used options -----*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
 extern int parse_opt_approxidate_cb(const struct option *, const char *, int);

-- 
1.5.6.117.g5fe2

Pierre Habouzit· Jun 23, 2008, 21:11 UTC · re: Pierre Habouzit · lore

[PATCH] parse-opt: Export a non NORETURN usage dumper.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   24 +++++++++++++++++-------
 parse-options.h |    9 +++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 774e647..4e9e675 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -257,8 +257,8 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
 	return ctx->cpidx + ctx->argc;
 }
 
-static NORETURN void usage_with_options_internal(const char * const *,
-                                                 const struct option *, int);
+static int usage_with_options_internal(const char * const *,
+                                       const struct option *, int, int);
 
 int parse_options(int argc, const char **argv, const struct option *options,
                   const char * const usagestr[], int flags)
@@ -302,7 +302,7 @@ int parse_options(int argc, const char **argv, const struct option *options,
 		}
 
 		if (!strcmp(arg + 2, "help-all"))
-			usage_with_options_internal(usagestr, options, 1);
+			usage_with_options_internal(usagestr, options, 1, 1);
 		if (!strcmp(arg + 2, "help"))
 			usage_with_options(usagestr, options);
 		if (parse_long_opt(&ctx, arg + 2, options))
@@ -315,8 +315,8 @@ int parse_options(int argc, const char **argv, const struct option *options,
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
-void usage_with_options_internal(const char * const *usagestr,
-                                 const struct option *opts, int full)
+int usage_with_options_internal(const char * const *usagestr,
+                                const struct option *opts, int full, int do_exit)
 {
 	fprintf(stderr, "usage: %s\n", *usagestr++);
 	while (*usagestr && **usagestr)
@@ -401,15 +401,25 @@ void usage_with_options_internal(const char * const *usagestr,
 	}
 	fputc('\n', stderr);
 
-	exit(129);
+	if (do_exit)
+		exit(129);
+	return PARSE_OPT_HELP;
 }
 
 void usage_with_options(const char * const *usagestr,
                         const struct option *opts)
 {
-	usage_with_options_internal(usagestr, opts, 0);
+	usage_with_options_internal(usagestr, opts, 0, 1);
+	exit(129); /* make gcc happy */
+}
+
+int parse_options_usage(const char * const *usagestr,
+                        const struct option *opts)
+{
+	return usage_with_options_internal(usagestr, opts, 0, 0);
 }
 
+
 /*----- some often used options -----*/
 #include "cache.h"
 
diff --git a/parse-options.h b/parse-options.h
index db6c986..424ea46 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -113,6 +113,12 @@ extern NORETURN void usage_with_options(const char * const *usagestr,
 
 /*----- incremantal advanced APIs -----*/
 
+enum {
+	PARSE_OPT_HELP = -1,
+	PARSE_OPT_DONE,
+	PARSE_OPT_UNKNOWN,
+};
+
 struct parse_opt_ctx_t {
 	const char **argv;
 	const char **out;
@@ -121,6 +127,9 @@ struct parse_opt_ctx_t {
 	int flags;
 };
 
+extern int parse_options_usage(const char * const *usagestr,
+                               const struct option *opts);
+
 extern void parse_options_start(struct parse_opt_ctx_t *ctx,
                                 int argc, const char **argv, int flags);
 

-- 
1.5.6.117.g5fe2

Pierre Habouzit· Jun 23, 2008, 21:11 UTC · re: Pierre Habouzit · lore

[PATCH] parse-opt: create parse_options_step.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   89 +++++++++++++++++++++++++++++++-----------------------
 parse-options.h |    4 ++
 2 files changed, 55 insertions(+), 38 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 4e9e675..71b3476 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -250,64 +250,79 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	ctx->flags = flags;
 }
 
-int parse_options_end(struct parse_opt_ctx_t *ctx)
-{
-	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
-	ctx->out[ctx->cpidx + ctx->argc] = NULL;
-	return ctx->cpidx + ctx->argc;
-}
-
 static int usage_with_options_internal(const char * const *,
-                                       const struct option *, int, int);
+                                       const struct option *, int);
 
-int parse_options(int argc, const char **argv, const struct option *options,
-                  const char * const usagestr[], int flags)
+int parse_options_step(struct parse_opt_ctx_t *ctx,
+                       const struct option *options,
+                       const char * const usagestr[])
 {
-	struct parse_opt_ctx_t ctx;
-
-	parse_options_start(&ctx, argc, argv, flags);
-	for (; ctx.argc; ctx.argc--, ctx.argv++) {
-		const char *arg = ctx.argv[0];
+	for (; ctx->argc; ctx->argc--, ctx->argv++) {
+		const char *arg = ctx->argv[0];
 
 		if (*arg != '-' || !arg[1]) {
-			if (ctx.flags & PARSE_OPT_STOP_AT_NON_OPTION)
+			if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
 				break;
-			ctx.out[ctx.cpidx++] = ctx.argv[0];
+			ctx->out[ctx->cpidx++] = ctx->argv[0];
 			continue;
 		}
 
 		if (arg[1] != '-') {
-			ctx.opt = arg + 1;
-			if (*ctx.opt == 'h')
-				usage_with_options(usagestr, options);
-			if (parse_short_opt(&ctx, options) < 0)
+			ctx->opt = arg + 1;
+			if (*ctx->opt == 'h')
+				return parse_options_usage(usagestr, options);
+			if (parse_short_opt(ctx, options) < 0)
 				usage_with_options(usagestr, options);
-			if (ctx.opt)
+			if (ctx->opt)
 				check_typos(arg + 1, options);
-			while (ctx.opt) {
-				if (*ctx.opt == 'h')
-					usage_with_options(usagestr, options);
-				if (parse_short_opt(&ctx, options) < 0)
+			while (ctx->opt) {
+				if (*ctx->opt == 'h')
+					return parse_options_usage(usagestr, options);
+				if (parse_short_opt(ctx, options) < 0)
 					usage_with_options(usagestr, options);
 			}
 			continue;
 		}
 
 		if (!arg[2]) { /* "--" */
-			if (!(ctx.flags & PARSE_OPT_KEEP_DASHDASH)) {
-				ctx.argc--;
-				ctx.argv++;
+			if (!(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {
+				ctx->argc--;
+				ctx->argv++;
 			}
 			break;
 		}
 
 		if (!strcmp(arg + 2, "help-all"))
-			usage_with_options_internal(usagestr, options, 1, 1);
+			return usage_with_options_internal(usagestr, options, 1);
 		if (!strcmp(arg + 2, "help"))
-			usage_with_options(usagestr, options);
-		if (parse_long_opt(&ctx, arg + 2, options))
+			return parse_options_usage(usagestr, options);
+		if (parse_long_opt(ctx, arg + 2, options))
 			usage_with_options(usagestr, options);
 	}
+	return PARSE_OPT_DONE;
+}
+
+int parse_options_end(struct parse_opt_ctx_t *ctx)
+{
+	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
+	ctx->out[ctx->cpidx + ctx->argc] = NULL;
+	return ctx->cpidx + ctx->argc;
+}
+
+int parse_options(int argc, const char **argv, const struct option *options,
+                  const char * const usagestr[], int flags)
+{
+	struct parse_opt_ctx_t ctx;
+
+	parse_options_start(&ctx, argc, argv, flags);
+	switch (parse_options_step(&ctx, options, usagestr)) {
+	case PARSE_OPT_HELP:
+		exit(129);
+	case PARSE_OPT_DONE:
+		break;
+	default: /* PARSE_OPT_UNKNOWN */
+		abort(); /* unreached yet */
+	}
 
 	return parse_options_end(&ctx);
 }
@@ -316,7 +331,7 @@ int parse_options(int argc, const char **argv, const struct option *options,
 #define USAGE_GAP         2
 
 int usage_with_options_internal(const char * const *usagestr,
-                                const struct option *opts, int full, int do_exit)
+                                const struct option *opts, int full)
 {
 	fprintf(stderr, "usage: %s\n", *usagestr++);
 	while (*usagestr && **usagestr)
@@ -401,22 +416,20 @@ int usage_with_options_internal(const char * const *usagestr,
 	}
 	fputc('\n', stderr);
 
-	if (do_exit)
-		exit(129);
 	return PARSE_OPT_HELP;
 }
 
 void usage_with_options(const char * const *usagestr,
                         const struct option *opts)
 {
-	usage_with_options_internal(usagestr, opts, 0, 1);
-	exit(129); /* make gcc happy */
+	usage_with_options_internal(usagestr, opts, 0);
+	exit(129);
 }
 
 int parse_options_usage(const char * const *usagestr,
                         const struct option *opts)
 {
-	return usage_with_options_internal(usagestr, opts, 0, 0);
+	return usage_with_options_internal(usagestr, opts, 0);
 }
 
 
diff --git a/parse-options.h b/parse-options.h
index 424ea46..9da5e8c 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -133,6 +133,10 @@ extern int parse_options_usage(const char * const *usagestr,
 extern void parse_options_start(struct parse_opt_ctx_t *ctx,
                                 int argc, const char **argv, int flags);
 
+extern int parse_options_step(struct parse_opt_ctx_t *ctx,
+                              const struct option *options,
+                              const char * const usagestr[]);
+
 extern int parse_options_end(struct parse_opt_ctx_t *ctx);
 
 

-- 
1.5.6.117.g5fe2

Pierre Habouzit· Jun 23, 2008, 21:11 UTC · re: Pierre Habouzit · lore

[PATCH] parse-opt: do not pring errors on unknown options, return -2 intead.

This way we can catch "unknown" options more easily.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 71b3476..1a1d4dc 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -132,7 +132,7 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio
 			return get_value(p, options, OPT_SHORT);
 		}
 	}
-	return error("unknown switch `%c'", *p->opt);
+	return -2;
 }
 
 static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
@@ -217,7 +217,7 @@ is_abbreviated:
 			abbrev_option->long_name);
 	if (abbrev_option)
 		return get_value(p, abbrev_option, abbrev_flags);
-	return error("unknown option `%s'", arg);
+	return -2;
 }
 
 void check_typos(const char *arg, const struct option *options)
@@ -271,15 +271,23 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			ctx->opt = arg + 1;
 			if (*ctx->opt == 'h')
 				return parse_options_usage(usagestr, options);
-			if (parse_short_opt(ctx, options) < 0)
-				usage_with_options(usagestr, options);
+			switch (parse_short_opt(ctx, options)) {
+			  case -1:
+				return parse_options_usage(usagestr, options);
+			  case -2:
+				return PARSE_OPT_UNKNOWN;
+			}
 			if (ctx->opt)
 				check_typos(arg + 1, options);
 			while (ctx->opt) {
 				if (*ctx->opt == 'h')
 					return parse_options_usage(usagestr, options);
-				if (parse_short_opt(ctx, options) < 0)
-					usage_with_options(usagestr, options);
+				switch (parse_short_opt(ctx, options)) {
+				  case -1:
+					return parse_options_usage(usagestr, options);
+				  case -2:
+					return PARSE_OPT_UNKNOWN;
+				}
 			}
 			continue;
 		}
@@ -296,8 +304,12 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			return usage_with_options_internal(usagestr, options, 1);
 		if (!strcmp(arg + 2, "help"))
 			return parse_options_usage(usagestr, options);
-		if (parse_long_opt(ctx, arg + 2, options))
-			usage_with_options(usagestr, options);
+		switch (parse_long_opt(ctx, arg + 2, options)) {
+		  case -1:
+			return parse_options_usage(usagestr, options);
+		  case -2:
+			return PARSE_OPT_UNKNOWN;
+		}
 	}
 	return PARSE_OPT_DONE;
 }
@@ -321,7 +333,12 @@ int parse_options(int argc, const char **argv, const struct option *options,
 	case PARSE_OPT_DONE:
 		break;
 	default: /* PARSE_OPT_UNKNOWN */
-		abort(); /* unreached yet */
+		if (ctx.argv[0][1] == '-') {
+			error("unknown option `%s'", ctx.argv[0] + 2);
+		} else {
+			error("unknown switch `%c'", *ctx.opt);
+		}
+		usage_with_options(usagestr, options);
 	}
 
 	return parse_options_end(&ctx);

-- 
1.5.6.117.g5fe2

Pierre Habouzit· Jun 23, 2008, 21:11 UTC · re: Pierre Habouzit · lore

[PATCH] parse-opt: fake short strings for callers to believe in.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   18 ++++++++++++++++--
 parse-options.h |   12 ++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 1a1d4dc..f3fd296 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -248,6 +248,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	ctx->argv = argv + 1;
 	ctx->out  = argv;
 	ctx->flags = flags;
+	strbuf_init(&ctx->buf, 0);
 }
 
 static int usage_with_options_internal(const char * const *,
@@ -257,6 +258,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
                        const struct option *options,
                        const char * const usagestr[])
 {
+	/* we must reset ->opt, unknown short option leave it dangling */
+	ctx->opt = NULL;
+
 	for (; ctx->argc; ctx->argc--, ctx->argv++) {
 		const char *arg = ctx->argv[0];
 
@@ -275,7 +279,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			  case -1:
 				return parse_options_usage(usagestr, options);
 			  case -2:
-				return PARSE_OPT_UNKNOWN;
+				goto unknown_fixup;
 			}
 			if (ctx->opt)
 				check_typos(arg + 1, options);
@@ -286,10 +290,19 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 				  case -1:
 					return parse_options_usage(usagestr, options);
 				  case -2:
-					return PARSE_OPT_UNKNOWN;
+					goto unknown_fixup;
 				}
 			}
 			continue;
+unknown_fixup:
+			/* fake a short option thing to hide the fact that we may have
+			 * started to parse aggregated stuff
+			 */
+			strbuf_reset(&ctx->buf);
+			strbuf_addch(&ctx->buf, '-');
+			strbuf_addstr(&ctx->buf, ctx->opt);
+			*ctx->argv = ctx->buf.buf;
+			return PARSE_OPT_UNKNOWN;
 		}
 
 		if (!arg[2]) { /* "--" */
@@ -318,6 +331,7 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
 {
 	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
 	ctx->out[ctx->cpidx + ctx->argc] = NULL;
+	strbuf_release(&ctx->buf);
 	return ctx->cpidx + ctx->argc;
 }
 
diff --git a/parse-options.h b/parse-options.h
index 9da5e8c..14447d5 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -1,6 +1,8 @@
 #ifndef PARSE_OPTIONS_H
 #define PARSE_OPTIONS_H
 
+#include "strbuf.h"
+
 enum parse_opt_type {
 	/* special types */
 	OPTION_END,
@@ -119,12 +121,18 @@ enum {
 	PARSE_OPT_UNKNOWN,
 };
 
+/*
+ * It's okay for the caller to consume argv/argc in the usual way.
+ * Other fields of that structure are private to parse-options and should not
+ * be modified in any way.
+ */
 struct parse_opt_ctx_t {
 	const char **argv;
 	const char **out;
 	int argc, cpidx;
 	const char *opt;
 	int flags;
+	struct strbuf buf;
 };
 
 extern int parse_options_usage(const char * const *usagestr,
@@ -133,6 +141,10 @@ extern int parse_options_usage(const char * const *usagestr,
 extern void parse_options_start(struct parse_opt_ctx_t *ctx,
                                 int argc, const char **argv, int flags);
 
+/* Warning: you cannot keep pointers to ctx->argv during the parse
+ *          because some "option strings" are faked. It's okay to use
+ *          ctx->argv after a parse_options_end obviously
+ */
 extern int parse_options_step(struct parse_opt_ctx_t *ctx,
                               const struct option *options,
                               const char * const usagestr[]);

-- 
1.5.6.117.g5fe2

Junio C Hamano· Jun 23, 2008, 22:08 UTC · re: Pierre Habouzit · lore

Re: [PATCH] parse-opt: do not pring errors on unknown options, return -2 intead.

You'd need something like this on top.

 parse-options.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 132d34c..71a8056 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -94,14 +94,14 @@ static int get_value(struct parse_opt_ctx_t *p,
 
 	case OPTION_CALLBACK:
 		if (unset)
-			return (*opt->callback)(opt, NULL, 1);
+			return (*opt->callback)(opt, NULL, 1) ? (-1) : 0;
 		if (opt->flags & PARSE_OPT_NOARG)
-			return (*opt->callback)(opt, NULL, 0);
+			return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
 		if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
-			return (*opt->callback)(opt, NULL, 0);
+			return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
 		if (!arg)
 			return opterror(opt, "requires a value", flags);
-		return (*opt->callback)(opt, get_arg(p), 0);
+		return (*opt->callback)(opt, get_arg(p), 0) ? (-1) : 0;
 
 	case OPTION_INTEGER:
 		if (unset) {
@@ -276,9 +276,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			if (*ctx->opt == 'h')
 				return parse_options_usage(usagestr, options);
 			switch (parse_short_opt(ctx, options)) {
-			  case -1:
+			case -1:
 				return parse_options_usage(usagestr, options);
-			  case -2:
+			case -2:
 				goto unknown_fixup;
 			}
 			if (ctx->opt)
@@ -318,9 +318,9 @@ unknown_fixup:
 		if (!strcmp(arg + 2, "help"))
 			return parse_options_usage(usagestr, options);
 		switch (parse_long_opt(ctx, arg + 2, options)) {
-		  case -1:
+		case -1:
 			return parse_options_usage(usagestr, options);
-		  case -2:
+		case -2:
 			return PARSE_OPT_UNKNOWN;
 		}
 	}

Pierre Habouzit· Jun 23, 2008, 22:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] parse-opt: do not pring errors on unknown options, return -2 intead.

On Mon, Jun 23, 2008 at 10:08:04PM +0000, Junio C Hamano wrote:

> You'd need something like this on top.
> 
> Documentation/technical/api-parse-options says that non-zero return is
> used to signal an error.

Oh right, like said, it's still rough on the edges :)

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

Pierre Habouzit· Jun 23, 2008, 21:23 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 09:09:35PM +0000, Pierre Habouzit wrote:

> {
>     struct parse_opt_ctx_t ctx;
> 
>     parse_options_start(&ctx, argc, argv, 0);
> 
>     for (;;) {
>         const char *arg;
> 
>         switch (parse_options_step(&ctx, options, usagestr)) {
>         case PARSE_OPT_HELP:
>             /* dump your help here, the one for options/usagestr is already dumped */

  At the expense of reworking how help is output, especially in the line
above, we also can have the nice feature that you can chain
parse_options_step with different options structure basically this way
(error handling removed):

  if (parse_options_step(&ctx, options1) == PARSE_OPT_UNKNOWN &&
      parse_options_step(&ctx, options2) == PARSE_OPT_UNKNOWN &&
      ...
      parse_options_step(&ctx, options_n) == PARSE_OPT_UNKNOWN)
  {
      /* you're on ! */
  }

  But with the proposed series, it's not possible (well in the sense
that for valid command lines it will work, but you'll end up with funny
usage dumps for invalid ones).

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

Junio C Hamano· Jun 23, 2008, 21:23 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Pierre Habouzit <madcoder@debian.org> writes:

>   With that, you write parsers this way:
>
> {
>     struct parse_opt_ctx_t ctx;
>
>     parse_options_start(&ctx, argc, argv, 0);
>
>     for (;;) {
>         const char *arg;
>
>         switch (parse_options_step(&ctx, options, usagestr)) {
>         case PARSE_OPT_HELP:
>             /* dump your help here, the one for options/usagestr is already dumped */
>             exit(129);
>         case PARSE_OPT_DONE:
>             goto done;
>         }
>
>         arg = *ctx->argv++;
>         ctx->argc--;
>
>         if (strcmp(arg, "-")) {
>             /* you're on baby ! */
>         } else if ....
>         } else {
>             error("unknown option %s", arg);
>             parse_options_usage(options, usagestr);
>             /* dump your help here */
>             exit(129);
>         }
>     }
>
> done:
>     argc = parse_options_end(&ctx);
> }

Pierre Habouzit· Jun 23, 2008, 21:28 UTC · re: Junio C Hamano · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 09:23:58PM +0000, Junio C Hamano wrote:

> Pierre Habouzit <madcoder@debian.org> writes:
> 
> >   With that, you write parsers this way:
> >
> > {
> >     struct parse_opt_ctx_t ctx;
> >
> >     parse_options_start(&ctx, argc, argv, 0);
> >
> >     for (;;) {
> >         const char *arg;
> >
> >         switch (parse_options_step(&ctx, options, usagestr)) {
> >         case PARSE_OPT_HELP:
> >             /* dump your help here, the one for options/usagestr is already dumped */
> >             exit(129);
> >         case PARSE_OPT_DONE:
> >             goto done;
> >         }
> >
> >         arg = *ctx->argv++;
> >         ctx->argc--;
> >
> >         if (strcmp(arg, "-")) {
> >             /* you're on baby ! */
> >         } else if ....
> >         } else {
> >             error("unknown option %s", arg);
> >             parse_options_usage(options, usagestr);
> >             /* dump your help here */
> >             exit(129);
> >         }
> >     }
> >
> > done:
> >     argc = parse_options_end(&ctx);
> > }
> 
> Nice.  I have started doing the same (insignificant details are different;
> e.g. I used "positive is unknown" convention instead ) and then the
> solution is sitting in my mbox ;-)

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

Linus Torvalds· Jun 23, 2008, 21:26 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Pierre Habouzit wrote:

> 
>   Let's see if I can catch it elegantly.

No.

Look at builtin-blame.c.

THEN get back to me.

			Linus

Linus Torvalds· Jun 23, 2008, 21:41 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Linus Torvalds wrote:

> 
> Trust me, you need what I wrote. Something that parses all the options in 
> one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT 
> function than the caller will call it!

	OPT_INTEGER('p', NULL, &p_value, "Patch depth"),
	OPT_BOOLEAN('p', NULL, &p_value_known),

			Linus

Pierre Habouzit· Jun 23, 2008, 21:47 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 09:26:39PM +0000, Linus Torvalds wrote:

> 
> 
> On Mon, 23 Jun 2008, Pierre Habouzit wrote:
> > 
> >   Let's see if I can catch it elegantly.
> 
> No.
> 
> Look at builtin-blame.c.
> 
> THEN get back to me.
> 
> Trust me, you need what I wrote. Something that parses all the options in 
> one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT 
> function than the caller will call it!

> Why is that so hard to accept? Especially since I already wrote the code 
> and sent it out?

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

Junio C Hamano· Jun 23, 2008, 22:11 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, 23 Jun 2008, Pierre Habouzit wrote:
>> 
>>   Let's see if I can catch it elegantly.
>
> No.
>
> Look at builtin-blame.c.
>
> THEN get back to me.
>
> Trust me, you need what I wrote. Something that parses all the options in 
> one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT 
> function than the caller will call it!

I do not think you two are saying vastly different things.

Pierre Habouzit· Jun 23, 2008, 22:24 UTC · re: Junio C Hamano · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 10:11:11PM +0000, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Mon, 23 Jun 2008, Pierre Habouzit wrote:
> >> 
> >>   Let's see if I can catch it elegantly.
> >
> > No.
> >
> > Look at builtin-blame.c.
> >
> > THEN get back to me.
> >
> > Trust me, you need what I wrote. Something that parses all the options in 
> > one go, and ignores the ones it cannot parse, because a TOTALLY DIFFERENT 
> > function than the caller will call it!
> 
> I do not think you two are saying vastly different things.
> 
> The series Pierre has just posted is prerequisite for teaching
> parse_options() to queue the unknown ones (besides allowing a finer
> grained stepwise parsing), so that the caller like cmd_blame() can call
> revision parser with the remainder.

Yeah actually one can use it this way, what I wrote can be used to do
the very same thing as your _KEEP_UNKNOWN flag, but also more.
 
Though I didn't fixed the fact that parse_options clobbers argv[0],
which can be easily fixed. The issue with that is that _some_ callers
use the fact that the filtered argv is NULL terminated. I'm unsure if
posix says that argv[argc] is readable and NULL at all, if it isn't,
then changing that would break git for some commands on some OSes other
than Linux and BSD where AFAICT argv[argc] == NULL holds.

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

Pierre Habouzit· Jun 23, 2008, 22:36 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 10:24:05PM +0000, Pierre Habouzit wrote:

> Yeah actually one can use it this way, what I wrote can be used to do
> the very same thing as your _KEEP_UNKNOWN flag, but also more.
>  
> Though I didn't fixed the fact that parse_options clobbers argv[0],
> which can be easily fixed. The issue with that is that _some_ callers
> use the fact that the filtered argv is NULL terminated. I'm unsure if
> posix says that argv[argc] is readable and NULL at all, if it isn't,
> then changing that would break git for some commands on some OSes other
> than Linux and BSD where AFAICT argv[argc] == NULL holds.

----8<---
From 8955adf557fded857eacc6c03c885a3d6334580f Mon Sep 17 00:00:00 2001
From: Pierre Habouzit <madcoder@debian.org>
Date: Tue, 24 Jun 2008 00:31:31 +0200
Subject: [PATCH] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |    8 +++++---
 parse-options.h |    2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 1ade2ac..c1c5a94 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -246,7 +246,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	memset(ctx, 0, sizeof(*ctx));
 	ctx->argc = argc - 1;
 	ctx->argv = argv + 1;
-	ctx->out  = argv;
+	ctx->out  = argv + ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
 	strbuf_init(&ctx->buf, 0);
 }
@@ -329,10 +329,12 @@ unknown_fixup:
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
 {
+	int res = ctx->cpidx + ctx->argc;
 	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
-	ctx->out[ctx->cpidx + ctx->argc] = NULL;
+	if (ctx->out + res < ctx->argv + ctx->argc)
+		ctx->out[res] = NULL;
 	strbuf_release(&ctx->buf);
-	return ctx->cpidx + ctx->argc;
+	return res;
 }
 
 int parse_options(int argc, const char **argv, const struct option *options,
diff --git a/parse-options.h b/parse-options.h
index 14447d5..6745c7d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -22,6 +22,8 @@ enum parse_opt_type {
 enum parse_opt_flags {
 	PARSE_OPT_KEEP_DASHDASH = 1,
 	PARSE_OPT_STOP_AT_NON_OPTION = 2,
+	/* using that option, the filtered argv may not be NULL terminated */
+	PARSE_OPT_KEEP_ARGV0 = 4,
 };
 
 enum parse_opt_option_flags {

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

Junio C Hamano· Jun 23, 2008, 22:38 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Pierre Habouzit <madcoder@debian.org> writes:

> Though I didn't fixed the fact that parse_options clobbers argv[0],
> which can be easily fixed. The issue with that is that _some_ callers
> use the fact that the filtered argv is NULL terminated.

Pierre Habouzit· Jun 23, 2008, 23:31 UTC · re: Junio C Hamano · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 10:38:11PM +0000, Junio C Hamano wrote:

> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > Though I didn't fixed the fact that parse_options clobbers argv[0],
> > which can be easily fixed. The issue with that is that _some_ callers
> > use the fact that the filtered argv is NULL terminated.
> 
> Isn't it just the matter of (1) leave argv[0] as is, (2) start copy dest
> at argv[1] not argv[0], and (3) terminate argv[nargc] = NULL where nargc
> is what you did not handle?
> 
> You have argc args in incoming argv[], you consume zero or more of them
> starting from argv[1] up to potentially argv[argc-1] (and argv[argc] is
> NULL).  So why is it an issue?  Sorry, I do not understand.

  Are we sure argv[argc] is NULL when those are main() arguments ? If
yes there is no issue, and I should read posix more carefully, but I was
under the impression that POSIX wasn't enforcing that.

  Unrelated but worth to note: many parse_options users just don't care
about argv[0] and having it kept each time would rather be a pain for
them (they would need to call argv++, argc-- themselves).

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

Linus Torvalds· Jun 23, 2008, 23:40 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Tue, 24 Jun 2008, Pierre Habouzit wrote:

> 
>   Are we sure argv[argc] is NULL when those are main() arguments ?

		Linus

Junio C Hamano· Jun 23, 2008, 23:51 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Pierre Habouzit <madcoder@debian.org> writes:

>   Unrelated but worth to note: many parse_options users just don't care
> about argv[0] and having it kept each time would rather be a pain for
> them (they would need to call argv++, argc-- themselves).

See for example these:

 * 5eee6b2 (Make builtin-reset.c use parse_options., 2008-03-04)
	The caller used to start counting from 1, after the conversion it
	has to count from 0.

 * 8320199 (Rewrite builtin-fetch option parsing to use parse_options()., 2007-12-04)
 * 3968658 (Make builtin-tag.c use parse_options., 2007-11-09)
	The caller used to say (i==argc) is "no param" case, now it has to
	say "!argc" is the equivalent condition.

Pierre Habouzit· Jun 24, 2008, 07:50 UTC · re: Junio C Hamano · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 11:51:52PM +0000, Junio C Hamano wrote:

> Pierre Habouzit <madcoder@debian.org> writes:
> 
> >   Unrelated but worth to note: many parse_options users just don't care
> > about argv[0] and having it kept each time would rather be a pain for
> > them (they would need to call argv++, argc-- themselves).
> 
> Not necessarily.  If they were parsing by hand, they were written to deal
> with the fact that argv[0] is not the program argument (iow, they start
> counting from one).  And before and after calling parse_options(), they
> need to change that assumption anyway, because parse_options() makes
> argv[0] disappear.

  Sure, but for _some_ commands it's easier not having to argv++,
argc--. The patch I sent just allow the caller to ask parse_options to
keep argv[0] in place or not, so that one can cascade parsers if wanted.
Note that with the series I sent, it's less useful to cascade option
parser, you rather mix more of them in your step by step parser. But oh
well, it's a useful feature anyways. We can even make it default, my
sole concern what that NULL-terminated thing, and now that I know we can
count on it, then, well, I absolutely don't care :)

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

Jeff King· Jun 24, 2008, 01:27 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Tue, Jun 24, 2008 at 01:31:46AM +0200, Pierre Habouzit wrote:

>   Are we sure argv[argc] is NULL when those are main() arguments ? If
> yes there is no issue, and I should read posix more carefully, but I was
> under the impression that POSIX wasn't enforcing that.

It's not POSIX; it's actually in the C standard. 5.1.2.2.1, paragraph 2:

  "argv[argc] shall be a null pointer"

-Peff

Jeff King· Jun 23, 2008, 19:53 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 11:47:49AM -0700, Linus Torvalds wrote:

> >   $ git blame --default HEAD git.c
> >   fatal: cannot stat path HEAD: No such file or directory
> > 
> > Oops.
> 
> Oops. And then, how would you fix this most easily?
> 
> Be honest now.

> Example: many arguments cause multiple option variables to change. 
> parse_options() simply can't handle that well - you can do it with a 
> callback, but then you need to make the option variables global or make 
> them a structure or something. All of which just makes it nasty to do 
> partial conversions for the simple cases.

  ...
  OPT__REVISION(&my_rev_list),
  ...

> And I guarantee that just adding PARSE_OPT_{CONTINUE|STOP}_ON_UNKNOWN is 
> going to be the smallest patch, and make for the easiest usage case. It 
> may not be "pretty", but I can whip up a patch in five minutes.

> Or are we going to sit around discussing this for another five months?

Please! :)

-Peff

Pierre Habouzit· Jun 23, 2008, 20:04 UTC · re: Jeff King · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 07:53:14PM +0000, Jeff King wrote:

> On Mon, Jun 23, 2008 at 11:47:49AM -0700, Linus Torvalds wrote:
> > Or are we going to sit around discussing this for another five months?
> 
> Please! :)
> 
> Pierre was working on the approach I mentioned, but I think he is short
> on time. I will take a look at the conversion, but I have a few other
> fixes on my plate first.
> 
> In the meantime, I don't think your patch makes anything _worse_, since
> we already have these sorts of bugs in the current parsing code.

  To be fair, I lack the time to do a complete parse option overhaul
that is like a few days of work doing that only (and my employer will
probably complain if I do so ;P). Though I'm trying to make
parse_options incremental right now, and I believe that it can work
quite well, and allow way more incremental conversions.

  One can overcome many limitations by exposing some stuff from the
parse_options logic, and just parse things in one pass. The _really_
nice thing with this approach is that you can trivially merge
parse_options descriptors by chaining parse_option_step (see my other
mail).

  I'll probably have a RFC series soon.

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

Linus Torvalds· Jun 23, 2008, 20:12 UTC · re: Jeff King · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Jeff King wrote:

> 
> I'm not so sure. I assumed that most of the callbacks would simply take
> a "struct rev_list". So you would end up in builtin-blame.c with:
> 
>   ...
>   OPT__REVISION(&my_rev_list),
>   ...
> 
> in your options table. And if setup_revisions takes options that affect
> things that _aren't_ in that struct, then they probably ought to be.

The world isn't like "it ought to be". It is like it is.

Then you say that it all "ought to be" in those options already.

Sure. Do we have some invisible sky wizard to make it so? 

	git grep 'str[n]*cmp.*"--'

			Linus

Jeff King· Jun 24, 2008, 05:35 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 01:12:28PM -0700, Linus Torvalds wrote:

> > in your options table. And if setup_revisions takes options that affect
> > things that _aren't_ in that struct, then they probably ought to be.
> 
> The world isn't like "it ought to be". It is like it is.
> 
> If you keep on dreaming about how things "ought to be", you'll never 
> confront the things as they are. The fact is, turning the existing very 
> simple argument parsers into using "parse_options()" is just _hard_.
> 
> Then you say that it all "ought to be" in those options already.

> Sure. Do we have some invisible sky wizard to make it so?

> I tried to do just _one_ program. Trust me, I'm not going to even bother 
> trying to do another unless parse_options() is made more palatable to do 
> in small pieces. 

-Peff

Linus Torvalds· Jun 24, 2008, 16:59 UTC · re: Jeff King · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Tue, 24 Jun 2008, Jeff King wrote:

> 
> You seem to have a bunch of _other_ problems with parse_options. And
> that is fine, but they have nothing whatsoever to do with anything I've
> said. So don't "sky wizard" _me_ about those problems. ;P

I also had a _single_ fix for it.

I never had "other" problems.

Do you see now?

			Linus

Johannes Schindelin· Jun 24, 2008, 17:13 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Hi,

On Tue, 24 Jun 2008, Linus Torvalds wrote:

> On Tue, 24 Jun 2008, Jeff King wrote:
> > 
> > You seem to have a bunch of _other_ problems with parse_options. And 
> > that is fine, but they have nothing whatsoever to do with anything 
> > I've said. So don't "sky wizard" _me_ about those problems. ;P
> 
> I have a _single_ problem I have with parse_options(), namely that it was 
> painful to convert in pieces.

Okay.

Of course, my approach was not meant incremental, yours was.

> Do you see now?

I do see now.

Jeff King· Jun 24, 2008, 17:34 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Tue, Jun 24, 2008 at 09:59:47AM -0700, Linus Torvalds wrote:

> I have a _single_ problem I have with parse_options(), namely that it was 
> painful to convert in pieces. It may well be that builtin-blame.c was one 
> of the more painful cases, but it really was a _single_ issue.
> 
> I also had a _single_ fix for it.
> 
> I never had "other" problems.
>
> What happened was that you and Dscho and others then tried to pick that 
> _single_ issue apart, because the solutions _you_ wanted (tying all the 

  - You proposed a CONTINUE_ON_UNKNOWN type of flag.

  - It is impossible for that mechanism to be correct in all cases, due
    to the syntactic rules for command lines. IOW, you cannot parse an
    element until you know the function of the element to the left.

  - I wanted to mention it specifically, because that exact mechanism
    had already been proposed in a patch last week, and Junio said "this
    conceptually is broken".

  - There has been discussion underway about what is the best mechanism
    to solve the same situation.

> And then you talk about how things "ought to be" in your world, to make 
> your solution relevant at all.
> 
> And I'm trying to tell you that "ought to be" has no relevance, because 
> you're not even looking at the problem!

-Peff

Linus Torvalds· Jun 24, 2008, 17:44 UTC · re: Jeff King · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Tue, 24 Jun 2008, Jeff King wrote:

> 
> Perhaps I was confused about the definition of "single", because
> throughout this thread you seem to be making multiple complaints about
> parse_options, including its lack of a "stop on unknown" flag, a
> "continue on unknown flag", and the movement of arguments within the
> argv array.

You think that is multiple problems, but it's not.

 - stop at unknown options (so that I could hand-parse them)

 - ignore unknown options (so that I could parse all the ones I knew 
   about, and then either hand-parse the rest, or just pass them on to 
   _another_ function that used some arbitrary model to parse the parts it 
   knew about)

See? Single issue.

> So I will say one last time, as clearly as I possibly can, what I was
> trying to bring to the discussion:

You constantly try to change the discussion to be about SOMETHIGN ELSE.

For example, you keep on bringing up this TOTAL RED HERRING:

>   - It is impossible for that mechanism to be correct in all cases, due
>     to the syntactic rules for command lines. IOW, you cannot parse an
>     element until you know the function of the element to the left.

NOBODY F*CKIGN CARES!

Because what builtin-blame.c *already* does is exactly that.

		Linus

Jeff King· Jun 24, 2008, 19:46 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Tue, Jun 24, 2008 at 10:44:24AM -0700, Linus Torvalds wrote:

> And to solve that _single_ problem, I wanted parse_options() to be able 
> to:
> 
>  - stop at unknown options (so that I could hand-parse them)
> 
>  - ignore unknown options (so that I could parse all the ones I knew 
>    about, and then either hand-parse the rest, or just pass them on to 
>    _another_ function that used some arbitrary model to parse the parts it 
>    knew about)
> 
> See? Single issue.

> And I even sent out a single patch for it. That single patch, btw, was 
> even rather small. 
> 
> Did you ever look at that patch? Did you ever look at the code I was 
> trying to have use parse_options()? No.

> You constantly try to change the discussion to be about SOMETHIGN ELSE.
> 
> For example, you keep on bringing up this TOTAL RED HERRING:
> 
> >   - It is impossible for that mechanism to be correct in all cases, due
> >     to the syntactic rules for command lines. IOW, you cannot parse an
> >     element until you know the function of the element to the left.
> 
> NOBODY F*CKIGN CARES!

> Because what builtin-blame.c *already* does is exactly that.

  From 20080623195314.GA29569@sigill.intra.peff.net:

    "In the meantime, I don't think your patch makes anything _worse_,
    since we already have these sorts of bugs in the current parsing
    code."

  From 20080623183358.GA28941@sigill.intra.peff.net:

    "It's worse than that...Try (with current git-blame...

     $ git blame -n 1 git.gc
     fatal: bad revision '1'

> This is what I'm complaining about with your totally IDIOTIC mails. You're 
> ignoring reality, and talking about how things "ought to work", and never 
> ever apparently looked at how things *do* work.

> The fact is, the one program I wanted to convert already does exactly what 
> you claim is "impossible to be correct in all cases".

> So either shut up, or send a patch to fix what you consider a bug. I'm 
> waiting.

-Peff

Junio C Hamano· Jun 24, 2008, 00:30 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, 23 Jun 2008, Linus Torvalds wrote:
>> 
>> Umm. Helloo, reality.. There are actually very few options that take a 
>> flag for their arguments. In particular, the option parsing we really 
>> _care_ about (revision parsing - see builtin-blame.c which is exactly 
>> where I wanted to convert things) very much DOES NOT.
>
> Actually, I guess "--default" does, but if you try to mix that up with (a) 
> a default head that starts with a dash and (b) git-blame, you're doing 
> something pretty odd.
>
> And yes, "-n" does too, but if you pass it negative numbers you get what 
> you deserve.
>
> The point being, we really _do_ have a real-life existing case for 
> PARSE_OPT_CONTINUE_ON_UNKNOWN, which is hard to handle any other way. 
> Currently you can literally do
>
> 	git blame --since=April -b Makefile
>
> and while it's a totally made-up example, it's one I've picked to show 
> exactly what does *not* work with my patch that started this whole thread.
>
> And guess what you need to fix it?
>
> If you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize. 

-- >8 --
Subject: [PATCH] parse-options: PARSE_OPT_SKIP_UNKNOWN

---
 parse-options.c |   14 ++++++++++----
 parse-options.h |    1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 71a8056..9f1eb65 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -341,20 +341,26 @@ int parse_options(int argc, const char **argv, const struct option *options,
 	struct parse_opt_ctx_t ctx;
 
 	parse_options_start(&ctx, argc, argv, flags);
+
+ again:
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 		exit(129);
 	case PARSE_OPT_DONE:
 		break;
 	default: /* PARSE_OPT_UNKNOWN */
-		if (ctx.argv[0][1] == '-') {
+		if (flags & PARSE_OPT_KEEP_UNKNOWN) {
+			ctx.out[ctx.cpidx++] = ctx.argv[0];
+			ctx.argc--;
+			ctx.argv++;
+			goto again;
+		}
+		if (ctx.argv[0][1] == '-')
 			error("unknown option `%s'", ctx.argv[0] + 2);
-		} else {
+		else
 			error("unknown switch `%c'", *ctx.opt);
-		}
 		usage_with_options(usagestr, options);
 	}
-
 	return parse_options_end(&ctx);
 }
 
diff --git a/parse-options.h b/parse-options.h
index 403794f..30fbf7e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -22,6 +22,7 @@ enum parse_opt_type {
 enum parse_opt_flags {
 	PARSE_OPT_KEEP_DASHDASH = 1,
 	PARSE_OPT_STOP_AT_NON_OPTION = 2,
+	PARSE_OPT_KEEP_UNKNOWN = 4,
 };
 
 enum parse_opt_option_flags {

-- 
1.5.6.49.g112db

Pierre Habouzit· Jun 24, 2008, 08:24 UTC · re: Junio C Hamano · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Tue, Jun 24, 2008 at 12:30:48AM +0000, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Mon, 23 Jun 2008, Linus Torvalds wrote:
> >> 
> >> Umm. Helloo, reality.. There are actually very few options that take a 
> >> flag for their arguments. In particular, the option parsing we really 
> >> _care_ about (revision parsing - see builtin-blame.c which is exactly 
> >> where I wanted to convert things) very much DOES NOT.
> >
> > Actually, I guess "--default" does, but if you try to mix that up with (a) 
> > a default head that starts with a dash and (b) git-blame, you're doing 
> > something pretty odd.
> >
> > And yes, "-n" does too, but if you pass it negative numbers you get what 
> > you deserve.
> >
> > The point being, we really _do_ have a real-life existing case for 
> > PARSE_OPT_CONTINUE_ON_UNKNOWN, which is hard to handle any other way. 
> > Currently you can literally do
> >
> > 	git blame --since=April -b Makefile
> >
> > and while it's a totally made-up example, it's one I've picked to show 
> > exactly what does *not* work with my patch that started this whole thread.
> >
> > And guess what you need to fix it?
> >
> > If you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize. 
> 
> With this on top of Pierre's series, and adding PARSE_OPT_SKIP_UNKNOWN to
> the obvious place in your patch, "blame --since=April -b Makefile" would
> work.
> 
> -- >8 --
> Subject: [PATCH] parse-options: PARSE_OPT_SKIP_UNKNOWN
> 
> ---
>  parse-options.c |   14 ++++++++++----
>  parse-options.h |    1 +
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index 71a8056..9f1eb65 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -341,20 +341,26 @@ int parse_options(int argc, const char **argv, const struct option *options,
>  	struct parse_opt_ctx_t ctx;
>  
>  	parse_options_start(&ctx, argc, argv, flags);
> +
> + again:
>  	switch (parse_options_step(&ctx, options, usagestr)) {
>  	case PARSE_OPT_HELP:
>  		exit(129);
>  	case PARSE_OPT_DONE:
>  		break;
>  	default: /* PARSE_OPT_UNKNOWN */
> -		if (ctx.argv[0][1] == '-') {
> +		if (flags & PARSE_OPT_KEEP_UNKNOWN) {
> +			ctx.out[ctx.cpidx++] = ctx.argv[0];

  Actually this doesn't work because it may point into the strbuf that
will be invalidated later. Our sole option here is to leak some memory I
fear.

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

Linus Torvalds· Jun 24, 2008, 17:05 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Tue, 24 Jun 2008, Pierre Habouzit wrote:

> 
>   Actually this doesn't work because it may point into the strbuf that
> will be invalidated later. Our sole option here is to leak some memory I
> fear.

			Linus

Pierre Habouzit· Jun 24, 2008, 19:30 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Tue, Jun 24, 2008 at 05:05:50PM +0000, Linus Torvalds wrote:

> 
> 
> On Tue, 24 Jun 2008, Pierre Habouzit wrote:
> > 
> >   Actually this doesn't work because it may point into the strbuf that
> > will be invalidated later. Our sole option here is to leak some memory I
> > fear.
> 
> I think leaking memory is ok (it's obviously going to be bounded by the 
> size of the arguments you pass into a program), but I also think you can 
> just change the option strings in place.
> 
> Yeah, I know - it's impolite, and we even marked things "const char", but 
> "const" in C is just a politeness thing, we can just choose to have a 
> function with a big comment that changes the string anyway. We'll have to 
> make sure that the few places that actually create argument strings by 
> hand (ie not from the ones supplied by a real "execve()") not do them so 
> that they need splitting (but no current ones would need to, obviously, 
> since splitting the argumens isn't even supported yet).
> 
> Or, if people hate that, just leak a few malloc'ed areas. That's arguably 
> the more straightforward way.

  The problem is that we sometimes feed the option parser with hand
crafted const char *[] where strings are indeed in rodata, and well,
changing things is not only impolite, it tends to SIG{BUS,SEGV} ;)

  But I think too that leaking is an option. git rev-parse --parseopt
already leaks in the same ways e.g.

  Though for the win32 port where fork is replaced with threads, well,
it may cause some issues, so I was reluctant wrt them. Of course it's
unlikely that it will cause problems, but one never knows ;)

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

Pierre Habouzit· Jun 24, 2008, 19:43 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Tue, Jun 24, 2008 at 07:30:28PM +0000, Pierre Habouzit wrote:

>   Though for the win32 port where fork is replaced with threads, well,
> it may cause some issues, so I was reluctant wrt them. Of course it's
> unlikely that it will cause problems, but one never knows ;)

  OTOH if it's really a problem, we could easily use a custom allocator
in parse-options that registers a pthread_cleanup_push (or whichever
atexit() like pthread use, I'm not really into threads) that would
cleanup this[0]. So maybe just leaking is the simplest way.

  As a consequence it makes the restriction about not keeping pointers
to argv during the parse go away, and I frankly don't like this
restriction, it's counterintuitive and very error prone, which arguably
means it's a really bad design.

  [0] In fact we may even want to have some kind of
      xmalloc_that_is_freed_at_exit that could be used where we
      purposely leak things, and cleanse that on atexit() for POSIX
      platforms and whatever win32 people like to use in their threads.
      It has the nice property to avoid lots of false positives when you
      are using valgrind and other memory checkers.

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

Johannes Sixt· Jun 25, 2008, 06:09 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Pierre Habouzit schrieb:

>   Though for the win32 port where fork is replaced with threads, well,
> it may cause some issues, so I was reluctant wrt them. Of course it's
> unlikely that it will cause problems, but one never knows ;)

-- Hannes

Johannes Schindelin· Jun 23, 2008, 17:04 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Hi,

On Mon, 23 Jun 2008, Linus Torvalds wrote:

> On Mon, 23 Jun 2008, Johannes Schindelin wrote:
> > 
> > Thinking about the recursive approach again, I came up with this POC:
> 
> "recursive" is pointless.

Nope, it is not.

Just compare that to the recursive approach.

Okay, now for the "granted, your approach has merits" part.

> Look at cmd_apply() in builtin-apply.c. Notice how it currently 
> absolutely CANNOT sanely be turned into using "parse_options()", not 
> because it needs any "recursive" handling, but simply because it wants 
> to do *incremental* handling.

> 	for (;;) {
> 		const char *arg;
> 		argc = parse_options(argc, argv,
> 			options, usage, PARSE_OPT_STOP_AT_UNKNOWN);

Heck, we could just as easily introduce PARSE_OPT_IGNORE_UNKNOWN.

> Could you handle the "recursive" use of parse_options() in builtin-blame.c 
> by teaching it about recursion? Yes. But again, it's just _simpler_ to 
> just teach parse_options() to parse the things it knows about, and leave 
> the other things in place.

Linus Torvalds· Jun 23, 2008, 17:21 UTC · re: Johannes Schindelin · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Johannes Schindelin wrote:

> 
> > Look at cmd_apply() in builtin-apply.c. Notice how it currently 
> > absolutely CANNOT sanely be turned into using "parse_options()", not 
> > because it needs any "recursive" handling, but simply because it wants 
> > to do *incremental* handling.
> 
> That is a totally independent issue from the one I discussed, namely sane 
> handling of the diff (and rev) options.

No it is not.

See? 

		Linus

Johannes Schindelin· Jun 23, 2008, 18:39 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Hi,

On Mon, 23 Jun 2008, Linus Torvalds wrote:

> (And yes, I can handle even the --help cases. It's actually not that 
> hard to just remember all the different option structs you've seen, and 
> handle all of that totally independently internally in parse_options(). 
> You don't need recursion, you just need a trivial list of options.)

Jeff King· Jun 23, 2008, 17:26 UTC · re: Johannes Schindelin · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 06:04:48PM +0100, Johannes Schindelin wrote:

> > > Thinking about the recursive approach again, I came up with this POC:
> > 
> > "recursive" is pointless.
> 
> Nope, it is not.

> Heck, we could just as easily introduce PARSE_OPT_IGNORE_UNKNOWN.

We could even send it to the list with message-id

  http://mid.gmane.org/1213758236-979-2-git-send-email-shawn.bohrer@gmail.com

and then Junio and I could complain that the concept is broken.

-Peff

Johannes Schindelin· Jun 23, 2008, 18:41 UTC · re: Jeff King · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

Hi,

On Mon, 23 Jun 2008, Jeff King wrote:

> On Mon, Jun 23, 2008 at 06:04:48PM +0100, Johannes Schindelin wrote:
> 
> > > > Thinking about the recursive approach again, I came up with this 
> > > > POC:
> > > 
> > > "recursive" is pointless.
> > 
> > Nope, it is not.
> 
> AIUI, one difference between your approach and Pierre's is that he is 
> suggesting (and I have suggested this in the past, too) a big 
> DIFF_OPTIONS macro that you stick in the options table for each command. 
> Whereas you are allowing for subtables accessible via pointers.
> 
> Your approach should yield a much leaner text size

> > Heck, we could just as easily introduce PARSE_OPT_IGNORE_UNKNOWN.
> 
> We could even send it to the list with message-id
> 
>   http://mid.gmane.org/1213758236-979-2-git-send-email-shawn.bohrer@gmail.com
> 
> and then Junio and I could complain that the concept is broken.

Pierre Habouzit· Jun 23, 2008, 19:24 UTC · re: Linus Torvalds · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, Jun 23, 2008 at 04:25:10PM +0000, Linus Torvalds wrote:

> 
> 
> On Mon, 23 Jun 2008, Johannes Schindelin wrote:
> > 
> > Thinking about the recursive approach again, I came up with this POC:
> 
> "recursive" is pointless.
> 
> The problem with the current "parse_options()" is not that it's recursive 
> (although that has been claimed multiple times!.
> 
> The problem with parse_options() is that it's currently impossible to 
> write something that handles _partial_ cases.
> 
> Let me explain.
> 
> Look at cmd_apply() in builtin-apply.c. Notice how it currently absolutely 
> CANNOT sanely be turned into using "parse_options()", not because it needs 
> any "recursive" handling, but simply because it wants to do *incremental* 
> handling.
> 
> It should be perfectly possible to change that argument loop from
> 
> 	for (i = 1; i < argc; i++) {
> 		const char *arg = argv[i];
> 		if (strcmp(arg, "-")) {
> 			.. handle <stdin> ..
> 			continue;
> 		}
> 		...
> 
> to doing something like this:
> 
> 	for (;;) {
> 		const char *arg;
> 		argc = parse_options(argc, argv,
> 			options, usage, PARSE_OPT_STOP_AT_UNKNOWN);
> 		if (!argc)
> 			break;
> 		arg = argv[1];
> 		argv++;
> 		argc--;
> 		if (strcmp(arg, "-")) {
> 			.. handle <stdin> ..
> 			continue;
> 		}
> 		...	
> 
> or whatever. See?

  Indeed, I read the thread many times, and I like the incremental
approach very much, it really makes sense, and we can easily migrate
code this way indeed. In fact that should have been the way
parse_options was designed from the beginning.

  There is a bit of work to do from this "handwaved" solution, because
people care about the filtered argv, so one should rememeber some kind
of "writing" position. SOmething like:

  parse_opt_ctx_t ctx;

  /* will basically copy argc/argv */
  parse_options_start(&ctx, argc, argv);

  for (;;) {
      const char *arg;
      int res = parse_options_step(&ctx, options, usage, 0));

      if (res == PARSE_OPT_HELP) {
          /* generate help and exit */
      }
      if (res == PARSE_OPT_DONE)
          break;

      arg = ctx->argv[ctx->pos++];
      if (strcmp(arg, "-")) {
          ... handle <stdin>....
          continue;
      }
  }

  argc = parse_options_end(&ctx);

  /* at this point (argc,argv) is almost what parse_options would have
     left us */

  THis way, parse_options can be written:

      parse_opt_ctx_t ctx;
      parse_options_start(&ctx, argc, argv);
      switch (parse_options_step(&ctx, options, usage, 0)) {
	  case PARSE_OPT_HELP: exit....
	  case PARSE_OPT_DONE: break;
	  default: exit(error("unknown option ....");
      }
      return parse_options_end(&ctx);

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

Linus Torvalds· Jun 23, 2008, 16:11 UTC · re: Pierre Habouzit · lore

Re: [RFC] Re: Convert 'git blame' to parse_options()

On Mon, 23 Jun 2008, Pierre Habouzit wrote:

> 
> You can't, mainly because of option aggregation: if the parser1 knows
> about -a and -b, parser2 about -c, then, this kind of things is
> problematic: -acb because you need to go to the parser '2' to know about
> -c, and you can't filter the arguments and keep -c and give -b to
> parser1 again, *BECAUSE* 'b' could also be -c argument.

Sure you can. You just rewrite the arguments themselves.

> int (*parse_opt_unknown_cb)(int shortopt, const char *longopt,
>                             const char *value, void *priv);

		Linus

Pierre Habouzit· Jun 24, 2008, 09:12 UTC · re: Linus Torvalds · lore

Making parse-opt incremental, reworked series

Here is a cleaned up series, with Junio's fixes, and some more.

Pierre Habouzit· Jun 24, 2008, 09:12 UTC · re: Pierre Habouzit · lore

[PATCH 1/7] parse-opt: have parse_options_{start,end}.

Make the struct optparse_t public under the better name parse_opt_ctx_t.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   69 +++++++++++++++++++++++++++++++------------------------
 parse-options.h |   16 ++++++++++++
 2 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index b8bde2b..774e647 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -4,14 +4,7 @@
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
-struct optparse_t {
-	const char **argv;
-	const char **out;
-	int argc, cpidx;
-	const char *opt;
-};
-
-static inline const char *get_arg(struct optparse_t *p)
+static inline const char *get_arg(struct parse_opt_ctx_t *p)
 {
 	if (p->opt) {
 		const char *res = p->opt;
@@ -37,7 +30,7 @@ static int opterror(const struct option *opt, const char *reason, int flags)
 	return error("option `%s' %s", opt->long_name, reason);
 }
 
-static int get_value(struct optparse_t *p,
+static int get_value(struct parse_opt_ctx_t *p,
                      const struct option *opt, int flags)
 {
 	const char *s, *arg;
@@ -131,7 +124,7 @@ 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 parse_opt_ctx_t *p, const struct option *options)
 {
 	for (; options->type != OPTION_END; options++) {
 		if (options->short_name == *p->opt) {
@@ -142,7 +135,7 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)
 	return error("unknown switch `%c'", *p->opt);
 }
 
-static int parse_long_opt(struct optparse_t *p, const char *arg,
+static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
                           const struct option *options)
 {
 	const char *arg_end = strchr(arg, '=');
@@ -247,45 +240,63 @@ void check_typos(const char *arg, const struct option *options)
 	}
 }
 
+void parse_options_start(struct parse_opt_ctx_t *ctx,
+                         int argc, const char **argv, int flags)
+{
+	memset(ctx, 0, sizeof(*ctx));
+	ctx->argc = argc - 1;
+	ctx->argv = argv + 1;
+	ctx->out  = argv;
+	ctx->flags = flags;
+}
+
+int parse_options_end(struct parse_opt_ctx_t *ctx)
+{
+	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
+	ctx->out[ctx->cpidx + ctx->argc] = NULL;
+	return ctx->cpidx + ctx->argc;
+}
+
 static NORETURN void usage_with_options_internal(const char * const *,
                                                  const struct option *, int);
 
 int parse_options(int argc, const char **argv, const struct option *options,
                   const char * const usagestr[], int flags)
 {
-	struct optparse_t args = { argv + 1, argv, argc - 1, 0, NULL };
+	struct parse_opt_ctx_t ctx;
 
-	for (; args.argc; args.argc--, args.argv++) {
-		const char *arg = args.argv[0];
+	parse_options_start(&ctx, argc, argv, flags);
+	for (; ctx.argc; ctx.argc--, ctx.argv++) {
+		const char *arg = ctx.argv[0];
 
 		if (*arg != '-' || !arg[1]) {
-			if (flags & PARSE_OPT_STOP_AT_NON_OPTION)
+			if (ctx.flags & PARSE_OPT_STOP_AT_NON_OPTION)
 				break;
-			args.out[args.cpidx++] = args.argv[0];
+			ctx.out[ctx.cpidx++] = ctx.argv[0];
 			continue;
 		}
 
 		if (arg[1] != '-') {
-			args.opt = arg + 1;
-			if (*args.opt == 'h')
+			ctx.opt = arg + 1;
+			if (*ctx.opt == 'h')
 				usage_with_options(usagestr, options);
-			if (parse_short_opt(&args, options) < 0)
+			if (parse_short_opt(&ctx, options) < 0)
 				usage_with_options(usagestr, options);
-			if (args.opt)
+			if (ctx.opt)
 				check_typos(arg + 1, options);
-			while (args.opt) {
-				if (*args.opt == 'h')
+			while (ctx.opt) {
+				if (*ctx.opt == 'h')
 					usage_with_options(usagestr, options);
-				if (parse_short_opt(&args, options) < 0)
+				if (parse_short_opt(&ctx, options) < 0)
 					usage_with_options(usagestr, options);
 			}
 			continue;
 		}
 
 		if (!arg[2]) { /* "--" */
-			if (!(flags & PARSE_OPT_KEEP_DASHDASH)) {
-				args.argc--;
-				args.argv++;
+			if (!(ctx.flags & PARSE_OPT_KEEP_DASHDASH)) {
+				ctx.argc--;
+				ctx.argv++;
 			}
 			break;
 		}
@@ -294,13 +305,11 @@ int parse_options(int argc, const char **argv, const struct option *options,
 			usage_with_options_internal(usagestr, options, 1);
 		if (!strcmp(arg + 2, "help"))
 			usage_with_options(usagestr, options);
-		if (parse_long_opt(&args, arg + 2, options))
+		if (parse_long_opt(&ctx, arg + 2, options))
 			usage_with_options(usagestr, options);
 	}
 
-	memmove(args.out + args.cpidx, args.argv, args.argc * sizeof(*args.out));
-	args.out[args.cpidx + args.argc] = NULL;
-	return args.cpidx + args.argc;
+	return parse_options_end(&ctx);
 }
 
 #define USAGE_OPTS_WIDTH 24
diff --git a/parse-options.h b/parse-options.h
index 4ee443d..db6c986 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -111,6 +111,22 @@ extern int parse_options(int argc, const char **argv,
 extern NORETURN void usage_with_options(const char * const *usagestr,
                                         const struct option *options);
 
+/*----- incremantal advanced APIs -----*/
+
+struct parse_opt_ctx_t {
+	const char **argv;
+	const char **out;
+	int argc, cpidx;
+	const char *opt;
+	int flags;
+};
+
+extern void parse_options_start(struct parse_opt_ctx_t *ctx,
+                                int argc, const char **argv, int flags);
+
+extern int parse_options_end(struct parse_opt_ctx_t *ctx);
+
+
 /*----- some often used options -----*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
 extern int parse_opt_approxidate_cb(const struct option *, const char *, int);

-- 
1.5.6.110.g736c7.dirty

Pierre Habouzit· Jun 24, 2008, 09:12 UTC · re: Pierre Habouzit · lore

[PATCH 2/7] parse-opt: Export a non NORETURN usage dumper.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   24 +++++++++++++++++-------
 parse-options.h |    9 +++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 774e647..4e9e675 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -257,8 +257,8 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
 	return ctx->cpidx + ctx->argc;
 }
 
-static NORETURN void usage_with_options_internal(const char * const *,
-                                                 const struct option *, int);
+static int usage_with_options_internal(const char * const *,
+                                       const struct option *, int, int);
 
 int parse_options(int argc, const char **argv, const struct option *options,
                   const char * const usagestr[], int flags)
@@ -302,7 +302,7 @@ int parse_options(int argc, const char **argv, const struct option *options,
 		}
 
 		if (!strcmp(arg + 2, "help-all"))
-			usage_with_options_internal(usagestr, options, 1);
+			usage_with_options_internal(usagestr, options, 1, 1);
 		if (!strcmp(arg + 2, "help"))
 			usage_with_options(usagestr, options);
 		if (parse_long_opt(&ctx, arg + 2, options))
@@ -315,8 +315,8 @@ int parse_options(int argc, const char **argv, const struct option *options,
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
-void usage_with_options_internal(const char * const *usagestr,
-                                 const struct option *opts, int full)
+int usage_with_options_internal(const char * const *usagestr,
+                                const struct option *opts, int full, int do_exit)
 {
 	fprintf(stderr, "usage: %s\n", *usagestr++);
 	while (*usagestr && **usagestr)
@@ -401,15 +401,25 @@ void usage_with_options_internal(const char * const *usagestr,
 	}
 	fputc('\n', stderr);
 
-	exit(129);
+	if (do_exit)
+		exit(129);
+	return PARSE_OPT_HELP;
 }
 
 void usage_with_options(const char * const *usagestr,
                         const struct option *opts)
 {
-	usage_with_options_internal(usagestr, opts, 0);
+	usage_with_options_internal(usagestr, opts, 0, 1);
+	exit(129); /* make gcc happy */
+}
+
+int parse_options_usage(const char * const *usagestr,
+                        const struct option *opts)
+{
+	return usage_with_options_internal(usagestr, opts, 0, 0);
 }
 
+
 /*----- some often used options -----*/
 #include "cache.h"
 
diff --git a/parse-options.h b/parse-options.h
index db6c986..424ea46 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -113,6 +113,12 @@ extern NORETURN void usage_with_options(const char * const *usagestr,
 
 /*----- incremantal advanced APIs -----*/
 
+enum {
+	PARSE_OPT_HELP = -1,
+	PARSE_OPT_DONE,
+	PARSE_OPT_UNKNOWN,
+};
+
 struct parse_opt_ctx_t {
 	const char **argv;
 	const char **out;
@@ -121,6 +127,9 @@ struct parse_opt_ctx_t {
 	int flags;
 };
 
+extern int parse_options_usage(const char * const *usagestr,
+                               const struct option *opts);
+
 extern void parse_options_start(struct parse_opt_ctx_t *ctx,
                                 int argc, const char **argv, int flags);
 

-- 
1.5.6.110.g736c7.dirty

Pierre Habouzit· Jun 24, 2008, 09:12 UTC · re: Pierre Habouzit · lore

[PATCH 3/7] parse-opt: create parse_options_step.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   89 +++++++++++++++++++++++++++++++-----------------------
 parse-options.h |    4 ++
 2 files changed, 55 insertions(+), 38 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 4e9e675..71b3476 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -250,64 +250,79 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	ctx->flags = flags;
 }
 
-int parse_options_end(struct parse_opt_ctx_t *ctx)
-{
-	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
-	ctx->out[ctx->cpidx + ctx->argc] = NULL;
-	return ctx->cpidx + ctx->argc;
-}
-
 static int usage_with_options_internal(const char * const *,
-                                       const struct option *, int, int);
+                                       const struct option *, int);
 
-int parse_options(int argc, const char **argv, const struct option *options,
-                  const char * const usagestr[], int flags)
+int parse_options_step(struct parse_opt_ctx_t *ctx,
+                       const struct option *options,
+                       const char * const usagestr[])
 {
-	struct parse_opt_ctx_t ctx;
-
-	parse_options_start(&ctx, argc, argv, flags);
-	for (; ctx.argc; ctx.argc--, ctx.argv++) {
-		const char *arg = ctx.argv[0];
+	for (; ctx->argc; ctx->argc--, ctx->argv++) {
+		const char *arg = ctx->argv[0];
 
 		if (*arg != '-' || !arg[1]) {
-			if (ctx.flags & PARSE_OPT_STOP_AT_NON_OPTION)
+			if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
 				break;
-			ctx.out[ctx.cpidx++] = ctx.argv[0];
+			ctx->out[ctx->cpidx++] = ctx->argv[0];
 			continue;
 		}
 
 		if (arg[1] != '-') {
-			ctx.opt = arg + 1;
-			if (*ctx.opt == 'h')
-				usage_with_options(usagestr, options);
-			if (parse_short_opt(&ctx, options) < 0)
+			ctx->opt = arg + 1;
+			if (*ctx->opt == 'h')
+				return parse_options_usage(usagestr, options);
+			if (parse_short_opt(ctx, options) < 0)
 				usage_with_options(usagestr, options);
-			if (ctx.opt)
+			if (ctx->opt)
 				check_typos(arg + 1, options);
-			while (ctx.opt) {
-				if (*ctx.opt == 'h')
-					usage_with_options(usagestr, options);
-				if (parse_short_opt(&ctx, options) < 0)
+			while (ctx->opt) {
+				if (*ctx->opt == 'h')
+					return parse_options_usage(usagestr, options);
+				if (parse_short_opt(ctx, options) < 0)
 					usage_with_options(usagestr, options);
 			}
 			continue;
 		}
 
 		if (!arg[2]) { /* "--" */
-			if (!(ctx.flags & PARSE_OPT_KEEP_DASHDASH)) {
-				ctx.argc--;
-				ctx.argv++;
+			if (!(ctx->flags & PARSE_OPT_KEEP_DASHDASH)) {
+				ctx->argc--;
+				ctx->argv++;
 			}
 			break;
 		}
 
 		if (!strcmp(arg + 2, "help-all"))
-			usage_with_options_internal(usagestr, options, 1, 1);
+			return usage_with_options_internal(usagestr, options, 1);
 		if (!strcmp(arg + 2, "help"))
-			usage_with_options(usagestr, options);
-		if (parse_long_opt(&ctx, arg + 2, options))
+			return parse_options_usage(usagestr, options);
+		if (parse_long_opt(ctx, arg + 2, options))
 			usage_with_options(usagestr, options);
 	}
+	return PARSE_OPT_DONE;
+}
+
+int parse_options_end(struct parse_opt_ctx_t *ctx)
+{
+	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
+	ctx->out[ctx->cpidx + ctx->argc] = NULL;
+	return ctx->cpidx + ctx->argc;
+}
+
+int parse_options(int argc, const char **argv, const struct option *options,
+                  const char * const usagestr[], int flags)
+{
+	struct parse_opt_ctx_t ctx;
+
+	parse_options_start(&ctx, argc, argv, flags);
+	switch (parse_options_step(&ctx, options, usagestr)) {
+	case PARSE_OPT_HELP:
+		exit(129);
+	case PARSE_OPT_DONE:
+		break;
+	default: /* PARSE_OPT_UNKNOWN */
+		abort(); /* unreached yet */
+	}
 
 	return parse_options_end(&ctx);
 }
@@ -316,7 +331,7 @@ int parse_options(int argc, const char **argv, const struct option *options,
 #define USAGE_GAP         2
 
 int usage_with_options_internal(const char * const *usagestr,
-                                const struct option *opts, int full, int do_exit)
+                                const struct option *opts, int full)
 {
 	fprintf(stderr, "usage: %s\n", *usagestr++);
 	while (*usagestr && **usagestr)
@@ -401,22 +416,20 @@ int usage_with_options_internal(const char * const *usagestr,
 	}
 	fputc('\n', stderr);
 
-	if (do_exit)
-		exit(129);
 	return PARSE_OPT_HELP;
 }
 
 void usage_with_options(const char * const *usagestr,
                         const struct option *opts)
 {
-	usage_with_options_internal(usagestr, opts, 0, 1);
-	exit(129); /* make gcc happy */
+	usage_with_options_internal(usagestr, opts, 0);
+	exit(129);
 }
 
 int parse_options_usage(const char * const *usagestr,
                         const struct option *opts)
 {
-	return usage_with_options_internal(usagestr, opts, 0, 0);
+	return usage_with_options_internal(usagestr, opts, 0);
 }
 
 
diff --git a/parse-options.h b/parse-options.h
index 424ea46..9da5e8c 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -133,6 +133,10 @@ extern int parse_options_usage(const char * const *usagestr,
 extern void parse_options_start(struct parse_opt_ctx_t *ctx,
                                 int argc, const char **argv, int flags);
 
+extern int parse_options_step(struct parse_opt_ctx_t *ctx,
+                              const struct option *options,
+                              const char * const usagestr[]);
+
 extern int parse_options_end(struct parse_opt_ctx_t *ctx);
 
 

-- 
1.5.6.110.g736c7.dirty

Pierre Habouzit· Jun 24, 2008, 09:12 UTC · re: Pierre Habouzit · lore

[PATCH 4/7] parse-opt: do not pring errors on unknown options, return -2 intead.

This way we can catch "unknown" options more easily.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   43 ++++++++++++++++++++++++++++++-------------
 1 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 71b3476..90935f3 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -94,14 +94,14 @@ static int get_value(struct parse_opt_ctx_t *p,
 
 	case OPTION_CALLBACK:
 		if (unset)
-			return (*opt->callback)(opt, NULL, 1);
+			return (*opt->callback)(opt, NULL, 1) ? (-1) : 0;
 		if (opt->flags & PARSE_OPT_NOARG)
-			return (*opt->callback)(opt, NULL, 0);
+			return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
 		if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
-			return (*opt->callback)(opt, NULL, 0);
+			return (*opt->callback)(opt, NULL, 0) ? (-1) : 0;
 		if (!arg)
 			return opterror(opt, "requires a value", flags);
-		return (*opt->callback)(opt, get_arg(p), 0);
+		return (*opt->callback)(opt, get_arg(p), 0) ? (-1) : 0;
 
 	case OPTION_INTEGER:
 		if (unset) {
@@ -132,7 +132,7 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio
 			return get_value(p, options, OPT_SHORT);
 		}
 	}
-	return error("unknown switch `%c'", *p->opt);
+	return -2;
 }
 
 static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
@@ -217,7 +217,7 @@ is_abbreviated:
 			abbrev_option->long_name);
 	if (abbrev_option)
 		return get_value(p, abbrev_option, abbrev_flags);
-	return error("unknown option `%s'", arg);
+	return -2;
 }
 
 void check_typos(const char *arg, const struct option *options)
@@ -271,15 +271,23 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			ctx->opt = arg + 1;
 			if (*ctx->opt == 'h')
 				return parse_options_usage(usagestr, options);
-			if (parse_short_opt(ctx, options) < 0)
-				usage_with_options(usagestr, options);
+			switch (parse_short_opt(ctx, options)) {
+			case -1:
+				return parse_options_usage(usagestr, options);
+			case -2:
+				return PARSE_OPT_UNKNOWN;
+			}
 			if (ctx->opt)
 				check_typos(arg + 1, options);
 			while (ctx->opt) {
 				if (*ctx->opt == 'h')
 					return parse_options_usage(usagestr, options);
-				if (parse_short_opt(ctx, options) < 0)
-					usage_with_options(usagestr, options);
+				switch (parse_short_opt(ctx, options)) {
+				case -1:
+					return parse_options_usage(usagestr, options);
+				case -2:
+					return PARSE_OPT_UNKNOWN;
+				}
 			}
 			continue;
 		}
@@ -296,8 +304,12 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			return usage_with_options_internal(usagestr, options, 1);
 		if (!strcmp(arg + 2, "help"))
 			return parse_options_usage(usagestr, options);
-		if (parse_long_opt(ctx, arg + 2, options))
-			usage_with_options(usagestr, options);
+		switch (parse_long_opt(ctx, arg + 2, options)) {
+		case -1:
+			return parse_options_usage(usagestr, options);
+		case -2:
+			return PARSE_OPT_UNKNOWN;
+		}
 	}
 	return PARSE_OPT_DONE;
 }
@@ -321,7 +333,12 @@ int parse_options(int argc, const char **argv, const struct option *options,
 	case PARSE_OPT_DONE:
 		break;
 	default: /* PARSE_OPT_UNKNOWN */
-		abort(); /* unreached yet */
+		if (ctx.argv[0][1] == '-') {
+			error("unknown option `%s'", ctx.argv[0] + 2);
+		} else {
+			error("unknown switch `%c'", *ctx.opt);
+		}
+		usage_with_options(usagestr, options);
 	}
 
 	return parse_options_end(&ctx);

-- 
1.5.6.110.g736c7.dirty

Pierre Habouzit· Jun 24, 2008, 09:12 UTC · re: Pierre Habouzit · lore

[PATCH 5/7] parse-opt: fake short strings for callers to believe in.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |   12 ++++++++++++
 parse-options.h |   12 ++++++++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 90935f3..ee82cf3 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -248,6 +248,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	ctx->argv = argv + 1;
 	ctx->out  = argv;
 	ctx->flags = flags;
+	strbuf_init(&ctx->buf, 0);
 }
 
 static int usage_with_options_internal(const char * const *,
@@ -257,6 +258,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
                        const struct option *options,
                        const char * const usagestr[])
 {
+	/* we must reset ->opt, unknown short option leave it dangling */
+	ctx->opt = NULL;
+
 	for (; ctx->argc; ctx->argc--, ctx->argv++) {
 		const char *arg = ctx->argv[0];
 
@@ -286,6 +290,13 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 				case -1:
 					return parse_options_usage(usagestr, options);
 				case -2:
+					/* fake a short option thing to hide the fact that we may have
+					 * started to parse aggregated stuff
+					 */
+					strbuf_reset(&ctx->buf);
+					strbuf_addch(&ctx->buf, '-');
+					strbuf_addstr(&ctx->buf, ctx->opt);
+					*ctx->argv = ctx->buf.buf;
 					return PARSE_OPT_UNKNOWN;
 				}
 			}
@@ -318,6 +329,7 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
 {
 	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
 	ctx->out[ctx->cpidx + ctx->argc] = NULL;
+	strbuf_release(&ctx->buf);
 	return ctx->cpidx + ctx->argc;
 }
 
diff --git a/parse-options.h b/parse-options.h
index 9da5e8c..14447d5 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -1,6 +1,8 @@
 #ifndef PARSE_OPTIONS_H
 #define PARSE_OPTIONS_H
 
+#include "strbuf.h"
+
 enum parse_opt_type {
 	/* special types */
 	OPTION_END,
@@ -119,12 +121,18 @@ enum {
 	PARSE_OPT_UNKNOWN,
 };
 
+/*
+ * It's okay for the caller to consume argv/argc in the usual way.
+ * Other fields of that structure are private to parse-options and should not
+ * be modified in any way.
+ */
 struct parse_opt_ctx_t {
 	const char **argv;
 	const char **out;
 	int argc, cpidx;
 	const char *opt;
 	int flags;
+	struct strbuf buf;
 };
 
 extern int parse_options_usage(const char * const *usagestr,
@@ -133,6 +141,10 @@ extern int parse_options_usage(const char * const *usagestr,
 extern void parse_options_start(struct parse_opt_ctx_t *ctx,
                                 int argc, const char **argv, int flags);
 
+/* Warning: you cannot keep pointers to ctx->argv during the parse
+ *          because some "option strings" are faked. It's okay to use
+ *          ctx->argv after a parse_options_end obviously
+ */
 extern int parse_options_step(struct parse_opt_ctx_t *ctx,
                               const struct option *options,
                               const char * const usagestr[]);

-- 
1.5.6.110.g736c7.dirty

Pierre Habouzit· Jun 24, 2008, 09:12 UTC · re: Pierre Habouzit · lore

[PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |    7 ++++---
 parse-options.h |    2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index ee82cf3..a6b5e04 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -246,7 +246,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	memset(ctx, 0, sizeof(*ctx));
 	ctx->argc = argc - 1;
 	ctx->argv = argv + 1;
-	ctx->out  = argv;
+	ctx->out  = argv + ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
 	strbuf_init(&ctx->buf, 0);
 }
@@ -327,10 +327,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
 {
+	int res = ctx->cpidx + ctx->argc;
 	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
-	ctx->out[ctx->cpidx + ctx->argc] = NULL;
+	ctx->out[res] = NULL;
 	strbuf_release(&ctx->buf);
-	return ctx->cpidx + ctx->argc;
+	return res + ((ctx->flags & PARSE_OPT_KEEP_ARGV0) != 0);
 }
 
 int parse_options(int argc, const char **argv, const struct option *options,
diff --git a/parse-options.h b/parse-options.h
index 14447d5..6745c7d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -22,6 +22,8 @@ enum parse_opt_type {
 enum parse_opt_flags {
 	PARSE_OPT_KEEP_DASHDASH = 1,
 	PARSE_OPT_STOP_AT_NON_OPTION = 2,
+	/* using that option, the filtered argv may not be NULL terminated */
+	PARSE_OPT_KEEP_ARGV0 = 4,
 };
 
 enum parse_opt_option_flags {

-- 
1.5.6.110.g736c7.dirty

Pierre Habouzit· Jun 24, 2008, 09:12 UTC · re: Pierre Habouzit · lore

[PATCH 7/7] Migrate git-blame to parse-option partially.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

  This patch is not really nice yet, but shows what we can do on top of the
  rest of the series. I shamelessly "stole" most of the code from Linus'
  patch.

  The nasty strdup can be removed safely using my setup_revisions split into
  setup_revisions and parse_revisions, by initializing revisions _before_
  actualy option parsing, making parse_revisions incremental (IOW eating
  options one at a time only) and do the whole thing in one single pass.

  IOW, one would see a inner loop like:

+   for (;;) {
+       int res;
+
+       switch (parse_options_step(&ctx, options, blame_opt_usage)) {
+       case PARSE_OPT_HELP:
+           exit(129);
+       case PARSE_OPT_DONE:
+           goto parse_done;
+       }
+
+       if (!strcmp(ctx.argv[0], "--reverse")) {
+           reverse = 1;
+           ctx.argv[0] = "--children";
+       }
+       res = parse_revisions(&revs, ctx.argc, ctx.argv);
+       if (res <= 0) {
+           if (res == 0)
+               error("unknown option %s", ctx.argv[0]);
+           usage_with_options(options, blame_opt_usage);
+       }
+       ctx.argv += j;
+       ctx.argc -= j;
+   }

  But despite being quite ugly (because of the cpidx fiddling and the strdup),
  this patch works as intended.

 builtin-blame.c |  195 ++++++++++++++++++++++++++++---------------------------
 1 files changed, 100 insertions(+), 95 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index cf41511..632a28d 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -18,24 +18,11 @@
 #include "cache-tree.h"
 #include "path-list.h"
 #include "mailmap.h"
+#include "parse-options.h"
 
-static char blame_usage[] =
-"git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S <revs-file>] [-M] [-C] [-C] [--contents <filename>] [--incremental] [commit] [--] file\n"
-"  -c                  Use the same output mode as git-annotate (Default: off)\n"
-"  -b                  Show blank SHA-1 for boundary commits (Default: off)\n"
-"  -l                  Show long commit SHA1 (Default: off)\n"
-"  --root              Do not treat root commits as boundaries (Default: off)\n"
-"  -t                  Show raw timestamp (Default: off)\n"
-"  -f, --show-name     Show original filename (Default: auto)\n"
-"  -n, --show-number   Show original linenumber (Default: off)\n"
-"  -s                  Suppress author name and timestamp (Default: off)\n"
-"  -p, --porcelain     Show in a format designed for machine consumption\n"
-"  -w                  Ignore whitespace differences\n"
-"  -L n,m              Process only line range n,m, counting from 1\n"
-"  -M, -C              Find line movements within and across files\n"
-"  --incremental       Show blame entries as we find them, incrementally\n"
-"  --contents file     Use <file>'s contents as the final image\n"
-"  -S revs-file        Use revisions from revs-file instead of calling git-rev-list\n";
+static char blame_usage[] = "git-blame [options] [--] file";
+
+static const char *blame_opt_usage[] = { blame_usage, NULL };
 
 static int longest_file;
 static int longest_author;
@@ -2219,6 +2206,50 @@ static const char *prepare_initial(struct scoreboard *sb)
    return final_commit_name;
 }
 
+static int blame_copy_callback(const struct option *option, const char *arg, int unset)
+{
+	int *opt = option->value;
+
+	/*
+	 * -C enables copy from removed files;
+	 * -C -C enables copy from existing files, but only
+	 *       when blaming a new file;
+	 * -C -C -C enables copy from existing files for
+	 *          everybody
+	 */
+	if (*opt & PICKAXE_BLAME_COPY_HARDER)
+		*opt |= PICKAXE_BLAME_COPY_HARDEST;
+	if (*opt & PICKAXE_BLAME_COPY)
+		*opt |= PICKAXE_BLAME_COPY_HARDER;
+	*opt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE;
+
+	if (arg)
+		blame_copy_score = parse_score(arg);
+	return 0;
+}
+
+static int blame_move_callback(const struct option *option, const char *arg, int unset)
+{
+	int *opt = option->value;
+
+	*opt |= PICKAXE_BLAME_MOVE;
+
+	if (arg)
+		blame_move_score = parse_score(arg);
+	return 0;
+}
+
+static int blame_bottomtop_callback(const struct option *option, const char *arg, int unset)
+{
+	const char **bottomtop = option->value;
+	if (!arg)
+		return -1;
+	if (*bottomtop)
+		die("More than one '-L n,m' option given");
+	*bottomtop = arg;
+	return 0;
+}
+
 int cmd_blame(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
@@ -2226,98 +2257,72 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct scoreboard sb;
 	struct origin *o;
 	struct blame_entry *ent;
-	int i, seen_dashdash, unk, opt;
+	int i, seen_dashdash, unk;
 	long bottom, top, lno;
-	int output_option = 0;
-	int show_stats = 0;
-	const char *revs_file = NULL;
 	const char *final_commit_name = NULL;
 	enum object_type type;
-	const char *bottomtop = NULL;
-	const char *contents_from = NULL;
+
+	static const char *bottomtop = NULL;
+	static int output_option = 0, opt = 0;
+	static int show_stats = 0;
+	static const char *revs_file = NULL;
+	static const char *contents_from = NULL;
+	static const struct option options[] = {
+		OPT_BOOLEAN(0, "incremental", &incremental, "Show blame entries as we find them, incrementally"),
+		OPT_BOOLEAN('b', NULL, &blank_boundary, "Show blank SHA-1 for boundary commits (Default: off)"),
+		OPT_BOOLEAN(0, "root", &show_root, "Do not treat root commits as boundaries (Default: off)"),
+		OPT_BOOLEAN(0, "show-stats", &show_stats, "Show work cost statistics"),
+		OPT_BIT(0, "score-debug", &output_option, "Show output score for blame entries", OUTPUT_SHOW_SCORE),
+		OPT_BIT('f', "show-name", &output_option, "Show original filename (Default: auto)", OUTPUT_SHOW_NAME),
+		OPT_BIT('n', "show-number", &output_option, "Show original linenumber (Default: off)", OUTPUT_SHOW_NUMBER),
+		OPT_BIT('p', "porcelain", &output_option, "Show in a format designed for machine consumption", OUTPUT_PORCELAIN),
+		OPT_BIT('c', NULL, &output_option, "Use the same output mode as git-annotate (Default: off)", OUTPUT_ANNOTATE_COMPAT),
+		OPT_BIT('t', NULL, &output_option, "Show raw timestamp (Default: off)", OUTPUT_RAW_TIMESTAMP),
+		OPT_BIT('l', NULL, &output_option, "Show long commit SHA1 (Default: off)", OUTPUT_LONG_OBJECT_NAME),
+		OPT_BIT('s', NULL, &output_option, "Suppress author name and timestamp (Default: off)", OUTPUT_NO_AUTHOR),
+		OPT_BIT('w', NULL, &xdl_opts, "Ignore whitespace differences", XDF_IGNORE_WHITESPACE),
+		OPT_STRING('S', NULL, &revs_file, "file", "Use revisions from <file> instead of calling git-rev-list"),
+		OPT_STRING(0, "contents", &contents_from, "file", "Use <file>'s contents as the final image"),
+		{ OPTION_CALLBACK, 'C', NULL, &opt, "score", "Find line copies within and across files", PARSE_OPT_OPTARG, blame_copy_callback },
+		{ OPTION_CALLBACK, 'M', NULL, &opt, "score", "Find line movements within and across files", PARSE_OPT_OPTARG, blame_move_callback },
+		OPT_CALLBACK('L', NULL, &bottomtop, "n,m", "Process only line range n,m, counting from 1", blame_bottomtop_callback),
+		OPT_END()
+	};
+
+	struct parse_opt_ctx_t ctx;
 
 	cmd_is_annotate = !strcmp(argv[0], "annotate");
 
 	git_config(git_blame_config, NULL);
 	save_commit_buffer = 0;
 
-	opt = 0;
+	parse_options_start(&ctx, argc, argv, PARSE_OPT_KEEP_DASHDASH |
+	                    PARSE_OPT_KEEP_ARGV0);
+	for (;;) {
+		switch (parse_options_step(&ctx, options, blame_opt_usage)) {
+		case PARSE_OPT_HELP:
+			exit(129);
+		case PARSE_OPT_DONE:
+			goto parse_done;
+		}
+
+		if (!strcmp(ctx.argv[0], "--reverse")) {
+			ctx.out[ctx.cpidx++] = "--children";
+			reverse = 1;
+		} else {
+			ctx.out[ctx.cpidx++] = strdup(ctx.argv[0]);
+		}
+		ctx.argv++;
+		ctx.argc--;
+	}
+parse_done:
+	argc = parse_options_end(&ctx);
+
 	seen_dashdash = 0;
 	for (unk = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (*arg != '-')
 			break;
-		else if (!strcmp("-b", arg))
-			blank_boundary = 1;
-		else if (!strcmp("--root", arg))
-			show_root = 1;
-		else if (!strcmp("--reverse", arg)) {
-			argv[unk++] = "--children";
-			reverse = 1;
-		}
-		else if (!strcmp(arg, "--show-stats"))
-			show_stats = 1;
-		else if (!strcmp("-c", arg))
-			output_option |= OUTPUT_ANNOTATE_COMPAT;
-		else if (!strcmp("-t", arg))
-			output_option |= OUTPUT_RAW_TIMESTAMP;
-		else if (!strcmp("-l", arg))
-			output_option |= OUTPUT_LONG_OBJECT_NAME;
-		else if (!strcmp("-s", arg))
-			output_option |= OUTPUT_NO_AUTHOR;
-		else if (!strcmp("-w", arg))
-			xdl_opts |= XDF_IGNORE_WHITESPACE;
-		else if (!strcmp("-S", arg) && ++i < argc)
-			revs_file = argv[i];
-		else if (!prefixcmp(arg, "-M")) {
-			opt |= PICKAXE_BLAME_MOVE;
-			blame_move_score = parse_score(arg+2);
-		}
-		else if (!prefixcmp(arg, "-C")) {
-			/*
-			 * -C enables copy from removed files;
-			 * -C -C enables copy from existing files, but only
-			 *       when blaming a new file;
-			 * -C -C -C enables copy from existing files for
-			 *          everybody
-			 */
-			if (opt & PICKAXE_BLAME_COPY_HARDER)
-				opt |= PICKAXE_BLAME_COPY_HARDEST;
-			if (opt & PICKAXE_BLAME_COPY)
-				opt |= PICKAXE_BLAME_COPY_HARDER;
-			opt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE;
-			blame_copy_score = parse_score(arg+2);
-		}
-		else if (!prefixcmp(arg, "-L")) {
-			if (!arg[2]) {
-				if (++i >= argc)
-					usage(blame_usage);
-				arg = argv[i];
-			}
-			else
-				arg += 2;
-			if (bottomtop)
-				die("More than one '-L n,m' option given");
-			bottomtop = arg;
-		}
-		else if (!strcmp("--contents", arg)) {
-			if (++i >= argc)
-				usage(blame_usage);
-			contents_from = argv[i];
-		}
-		else if (!strcmp("--incremental", arg))
-			incremental = 1;
-		else if (!strcmp("--score-debug", arg))
-			output_option |= OUTPUT_SHOW_SCORE;
-		else if (!strcmp("-f", arg) ||
-			 !strcmp("--show-name", arg))
-			output_option |= OUTPUT_SHOW_NAME;
-		else if (!strcmp("-n", arg) ||
-			 !strcmp("--show-number", arg))
-			output_option |= OUTPUT_SHOW_NUMBER;
-		else if (!strcmp("-p", arg) ||
-			 !strcmp("--porcelain", arg))
-			output_option |= OUTPUT_PORCELAIN;
 		else if (!strcmp("--", arg)) {
 			seen_dashdash = 1;
 			i++;

-- 
1.5.6.110.g736c7.dirty

Pierre Habouzit· Jun 24, 2008, 10:03 UTC · re: Pierre Habouzit · lore

Re: [PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.

On Tue, Jun 24, 2008 at 09:12:11AM +0000, Pierre Habouzit wrote:

> This way, argv[0] isn't clobbered, to the cost of maybe not having a
> resulting NULL terminated argv array.
> 
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>  parse-options.c |    7 ++++---
>  parse-options.h |    2 ++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index ee82cf3..a6b5e04 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -246,7 +246,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
>  	memset(ctx, 0, sizeof(*ctx));
>  	ctx->argc = argc - 1;
>  	ctx->argv = argv + 1;
> -	ctx->out  = argv;
> +	ctx->out  = argv + ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
>  	ctx->flags = flags;
>  	strbuf_init(&ctx->buf, 0);
>  }
> @@ -327,10 +327,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>  
>  int parse_options_end(struct parse_opt_ctx_t *ctx)
>  {
> +	int res = ctx->cpidx + ctx->argc;
>  	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
> -	ctx->out[ctx->cpidx + ctx->argc] = NULL;
> +	ctx->out[res] = NULL;
>  	strbuf_release(&ctx->buf);
> -	return ctx->cpidx + ctx->argc;
> +	return res + ((ctx->flags & PARSE_OPT_KEEP_ARGV0) != 0);
>  }
>  
>  int parse_options(int argc, const char **argv, const struct option *options,
> diff --git a/parse-options.h b/parse-options.h
> index 14447d5..6745c7d 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -22,6 +22,8 @@ enum parse_opt_type {
>  enum parse_opt_flags {
>  	PARSE_OPT_KEEP_DASHDASH = 1,
>  	PARSE_OPT_STOP_AT_NON_OPTION = 2,
> +	/* using that option, the filtered argv may not be NULL terminated */

  This comment is bogus and shall be stripped, I forgot to…

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

Linus Torvalds· Jun 24, 2008, 17:18 UTC · re: Pierre Habouzit · lore

Re: [PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.

On Tue, 24 Jun 2008, Pierre Habouzit wrote:

>
> This way, argv[0] isn't clobbered, to the cost of maybe not having a
> resulting NULL terminated argv array.

Umm. I think it's much easier to do by always having

	ctx->out  = argv;

and then just initializing cpix to 0 or 1:

	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);

because now parse_options_end() doesn't need to play games any more. It 
doesn't need to care about PARSE_OPT_KEEP_ARGV0, it can just do exactly 
what it always used to do, because "ctx->cpidx + ctx->argc" automatically 
does the right thing (it is both the return value _and_ the index that you 
should fill with NULL.
	
Hmm?

		Linus

Pierre Habouzit· Jun 24, 2008, 19:27 UTC · re: Linus Torvalds · lore

Re: [PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.

On Tue, Jun 24, 2008 at 05:18:56PM +0000, Linus Torvalds wrote:

> 
> 
> On Tue, 24 Jun 2008, Pierre Habouzit wrote:
> >
> > This way, argv[0] isn't clobbered, to the cost of maybe not having a
> > resulting NULL terminated argv array.
> 
> Umm. I think it's much easier to do by always having
> 
> 	ctx->out  = argv;
> 
> and then just initializing cpix to 0 or 1:
> 
> 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
> 
> because now parse_options_end() doesn't need to play games any more. It 
> doesn't need to care about PARSE_OPT_KEEP_ARGV0, it can just do exactly 
> what it always used to do, because "ctx->cpidx + ctx->argc" automatically 
> does the right thing (it is both the return value _and_ the index that you 
> should fill with NULL.

  Oh right, ack it's more elegant.

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

Pierre Habouzit· Jun 24, 2008, 20:55 UTC · re: Linus Torvalds · lore

Re: [PATCH 6/7] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.

On Tue, Jun 24, 2008 at 05:18:56PM +0000, Linus Torvalds wrote:

> 
> 
> On Tue, 24 Jun 2008, Pierre Habouzit wrote:
> >
> > This way, argv[0] isn't clobbered, to the cost of maybe not having a
> > resulting NULL terminated argv array.
> 
> Umm. I think it's much easier to do by always having
> 
> 	ctx->out  = argv;
> 
> and then just initializing cpix to 0 or 1:
> 
> 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
> 
> because now parse_options_end() doesn't need to play games any more. It 
> doesn't need to care about PARSE_OPT_KEEP_ARGV0, it can just do exactly 
> what it always used to do, because "ctx->cpidx + ctx->argc" automatically 
> does the right thing (it is both the return value _and_ the index that you 
> should fill with NULL.

It indeed is now a trivial patch:

-----8<-----

Subject: [PATCH] parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.

This way, argv[0] isn't clobbered.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 parse-options.c |    1 +
 parse-options.h |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 60a11e8..51a44e3 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -248,6 +248,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	ctx->argc = argc - 1;
 	ctx->argv = argv + 1;
 	ctx->out  = argv;
+	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
 }
 
diff --git a/parse-options.h b/parse-options.h
index b391bb6..6299632 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -20,6 +20,7 @@ enum parse_opt_type {
 enum parse_opt_flags {
 	PARSE_OPT_KEEP_DASHDASH = 1,
 	PARSE_OPT_STOP_AT_NON_OPTION = 2,
+	PARSE_OPT_KEEP_ARGV0 = 4,
 };
 
 enum parse_opt_option_flags {

-- 
1.5.6.120.g3adb8.dirty

Linus Torvalds· Jun 24, 2008, 17:20 UTC · re: Pierre Habouzit · lore

Re: [PATCH 5/7] parse-opt: fake short strings for callers to believe in.

On Tue, 24 Jun 2008, Pierre Habouzit wrote:

>
> If we begin to parse -abc and that the parser knew about -a and -b, it
> will fake a -c switch for the caller to deal with.
> 
> Of course in the case of -acb (supposing -c is not taking an argument) the
> caller will have to be especially clever to do the same thing. We could
> think about exposing an API to do so if it's really needed, but oh well...

		Linus

Pierre Habouzit· Jun 24, 2008, 19:26 UTC · re: Linus Torvalds · lore

Re: [PATCH 5/7] parse-opt: fake short strings for callers to believe in.

On Tue, Jun 24, 2008 at 05:20:28PM +0000, Linus Torvalds wrote:

> 
> 
> On Tue, 24 Jun 2008, Pierre Habouzit wrote:
> >
> > If we begin to parse -abc and that the parser knew about -a and -b, it
> > will fake a -c switch for the caller to deal with.
> > 
> > Of course in the case of -acb (supposing -c is not taking an argument) the
> > caller will have to be especially clever to do the same thing. We could
> > think about exposing an API to do so if it's really needed, but oh well...
> 
> Well, if the other parser is _also_ parse_options() (ie you just cascade 
> them incrementally in a loop), then the other parser should get it right 
> automatically. No?

  Exactly. There are minor glitches wrt the help generation to deal
with, but for pure parsing issues yes, it will work.

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

Andreas Ericsson· Jun 25, 2008, 15:07 UTC · re: Pierre Habouzit · lore

Re: [PATCH 5/7] parse-opt: fake short strings for callers to believe in.

Pierre Habouzit wrote:

> On Tue, Jun 24, 2008 at 05:20:28PM +0000, Linus Torvalds wrote:
>>
>> On Tue, 24 Jun 2008, Pierre Habouzit wrote:
>>> If we begin to parse -abc and that the parser knew about -a and -b, it
>>> will fake a -c switch for the caller to deal with.
>>>
>>> Of course in the case of -acb (supposing -c is not taking an argument) the
>>> caller will have to be especially clever to do the same thing. We could
>>> think about exposing an API to do so if it's really needed, but oh well...
>> Well, if the other parser is _also_ parse_options() (ie you just cascade 
>> them incrementally in a loop), then the other parser should get it right 
>> automatically. No?
> 
>   Exactly. There are minor glitches wrt the help generation to deal
> with, but for pure parsing issues yes, it will work.
> 

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Pierre Habouzit· Jun 24, 2008, 20:58 UTC · re: Pierre Habouzit · lore

[REPLACEMENT PATCH] parse-opt: fake short strings for callers to believe in.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

  On top of my leaky() mini series, one can on purpose leak some memory
  here, and avoid the strbuf and limitations at once. It means that in my
  current git-blame proof of concept one can drop the strdup(), which
  makes it slightly less disgusting.

 parse-options.c |    9 +++++++++
 parse-options.h |    5 +++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 90935f3..60a11e8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1,5 +1,6 @@
 #include "git-compat-util.h"
 #include "parse-options.h"
+#include "cache.h"
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
@@ -257,6 +258,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
                        const struct option *options,
                        const char * const usagestr[])
 {
+	/* we must reset ->opt, unknown short option leave it dangling */
+	ctx->opt = NULL;
+
 	for (; ctx->argc; ctx->argc--, ctx->argv++) {
 		const char *arg = ctx->argv[0];
 
@@ -286,6 +290,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 				case -1:
 					return parse_options_usage(usagestr, options);
 				case -2:
+					/* fake a short option thing to hide the fact that we may have
+					 * started to parse aggregated stuff
+					 */
+					ctx->argv[0] = leaky(xstrdup(ctx->opt - 1));
+					*(char *)ctx->argv[0] = '-';
 					return PARSE_OPT_UNKNOWN;
 				}
 			}
diff --git a/parse-options.h b/parse-options.h
index 9da5e8c..b391bb6 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -119,6 +119,11 @@ enum {
 	PARSE_OPT_UNKNOWN,
 };
 
+/*
+ * It's okay for the caller to consume argv/argc in the usual way.
+ * Other fields of that structure are private to parse-options and should not
+ * be modified in any way.
+ */
 struct parse_opt_ctx_t {
 	const char **argv;
 	const char **out;

-- 
1.5.6.120.g3adb8.dirty

Pierre Habouzit· Jun 26, 2008, 08:35 UTC · re: Pierre Habouzit · lore

Re: [REPLACEMENT PATCH] parse-opt: fake short strings for callers to believe in.

  Hi Junio,

  I saw that you merged this series into pu, for which I'm really glad,
though I believe that the version that works on top of the "leaky()"
series is better, since everyone agrees that the small leaks in the
option parser are better than the limitations the one you merged has.

  I won't flood the list again with the patches, but you can find them
in git://git.madism.org/~madcoder/git.git#ph/parseopt (based on current
next). This includes the two leaky() patches (with the remarks that have
been made to me incorporated), the current state of the parse-options
improvements, and the current builtin-blame.c proof of concept (without
the ugly strdup).

  I really don't like the strbuf thingy, and I'd be glad to never see it
merged in next, it will cause more harm than it helps.

TIA.

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

Junio C Hamano· Jun 26, 2008, 08:40 UTC · re: Pierre Habouzit · lore

Re: [REPLACEMENT PATCH] parse-opt: fake short strings for callers to believe in.

Pierre Habouzit <madcoder@debian.org> writes:

>   I saw that you merged this series into pu, for which I'm really glad,

Pierre Habouzit· Jun 26, 2008, 09:37 UTC · re: Junio C Hamano · lore

Re: [REPLACEMENT PATCH] parse-opt: fake short strings for callers to believe in.

On Thu, Jun 26, 2008 at 08:40:16AM +0000, Junio C Hamano wrote:

> Pierre Habouzit <madcoder@debian.org> writes:
> 
> >   I saw that you merged this series into pu, for which I'm really glad,
> 
> Heh, you shouldn't be glad to have "landed" on 'pu', as being in 'pu' is
> just a sign of "undecided" ;-)
> 
> And thanks for a reminder.  Every bit of help to make integration smoother
> really helps.

  You're welcome.

> I've been swamped and I still have your "revisions: refactor
> init_revisions and setup_revisions" sitting in my inbox.

  Well I'll resubmit it probably in some kind of improved way that mixes
better within the parse-options stuff. Trust me for not forgetting about
it. IOW the refactor will remain, but it will probably become some kind
of series with a second refactor where parse_revisions has a
'eat-one-option-at-a-time' behavior as well.

  Though I believe the current stuff I point you to, to be quite ready
(except for the builtin-blame proof of concept that is .. a PoC, not
really meant for next), and once it'll be ready for next, I'll try to
work on the options in git again, because it can be done in an
incremental way now :)

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