Convert 'git blame' to parse_options()

Convert 'git blame' to parse_options()

[Linus Torvalds]

Why? I did some more size statistics, and 'cmd_blame()' was the biggest 
function in all of git. Admittedly my script for finding sizes is probably 
broken, but it does look very messy.

Just cleaning up the option parsing actually cuts down the size a bit, and 
while parse_options() isn't really perfect for what cmd_blame() wants, 
it's manageable.

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.

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

That builtin-blame option parsing is really ugly, trying to then move 
unrecognized options to be parsed by a secondary

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

which parses the rest. This is _really_ not what parse_options() was 
written with in mind, and to give strictly the same behaviour as before 
you really do need some kind of PARSE_OPT_IGNORE_UNRECOGNIZED flag, but I 
think this is already a good enough halfway point.

Hmm?

Btw, making the "struct options" array be 'static' avoids having to build 
it up at run-time on the stack, which is why it's usually a good idea. Of 
course, it means that the option variables themselves have to be static, 
which has its own set of downsides, so it's a balancing act.

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

Re: Convert 'git blame' to parse_options()

[Junio C Hamano]

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

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

Yeah, but wasn't it because it needed to be compatible with both annotate
syntax and rev-list style "range" notation at the same time?

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

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

[Pierre Habouzit]

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

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.

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.

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

Where the callback is supposed to work that way:
we pass to it either a shortopt ('\0' else) or a longopt (NULL else) but
never both, and what the parser could find as a possible value in value
(NULL if no value found). Then the parser does what it has to, and
returns one of the previous enum values. ERROR would be a fatal error
(-1 chosen so that one can return error(...)), NOT_FOR_ME if it didn't
want the flag after all, IS_FOR_ME if it took only the flag, without the
value, the last one being if it consumed both the option flag _and_ the
value.

Of course for things like --long-opt=value if the callback doesn't eat
the value, then parse_options would have to barf loudly.


The major drawback of this method is that parse_options won't generate
the nice usage help for the things that recurse in such a function. But
at least it eases migration to parseoptions, because I believe rev_parse
and diff_opt_parse to be _way_ easier to migrate to such a callback
_and_ with the old API together (I really believe that the old big
consuming loops could use such a callback directly e.g.) so that
commands using diff and revision options can be migrated to parse-opt
*slowly* rather than all at a time (which is a no-go and is such a big
amount of work that I avoided it in despair).

If people believe it's a sane approach I can do it. I absolutely don't
know why no one thought of that before, but I don't see any major
drawback (except for the "help" bits, but like I said, it's a "code
upgrade path" and not meant to be the final state).



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

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

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

[Johannes Schindelin]

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:

-- snip --
[PATCH] parse-options: make it possible to have option "subtables"

Some programs share an awful lot of options, such as for the diff
machinery.  Allow an option table to recurse into another option
table with the new OPTION_OPTIONS() directive.

The subtable is expected to store the values into a struct whose address
is passed as 2nd parameter to OPTION_OPTIONS().

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

Re: Convert 'git blame' to parse_options()

[Johannes Schindelin]

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

IMHO this would not change anything in the way of making it harder; it is 
just a matter of adding the pairs to a container.  The more tricky thing 
is how to handle a bunch of intervals efficiently, without introducing 
too much ugliness.

Ciao,
Dscho

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

[Pierre Habouzit]

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

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

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

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

[Linus Torvalds]

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.

That said, anybody who doesn't use parse_options() right now won't be 
accepting something like "-abc" *anyway*, and people currently need to use 
"-a -b -c".

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

This doesn't really change anything. It just makes it harder to write a 
simple partial parser. Just passing in "IGNORE_UNKNOWN" (and probably 
"STOP_AT_UNKNOWN") is the simplest way to have multiple passes.

		Linus

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

[Linus Torvalds]

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?

Could you handle that with callbacks? Of course. "You can solve any 
problem in computer science with an added level of indirection". But would 
it be simpler to convert existing users? Hell no. 

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

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

[Johannes Schindelin]

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.

IIRC your solution was a bit involved, while I think that mine is at least 
small enough to be simple.

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

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

[Jeff King]

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

?

-Peff

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

[Johannes Schindelin]

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.

To keep things _simple_ a callback is not good.  Sure, you can work around 
the limitations of callbacks for aggregation, but the code change looks 
horrible.  And the same holds true for the help message.

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.

That is a totally independent issue from the one I discussed, namely sane 
handling of the diff (and rev) options.

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

We do have PARSE_OPT_STOP_AT_NON_OPTION since a0ec9d25(parseopt: add flag 
to stop on first non option), which tries to solve a _similar_ problem, 
and it should be not hard at all to get PARSE_OPT_STOP_AT_UNKNOWN without 
changing the loop as you suggested.

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.

And here I disagree.  You might not need a nice "--help" output, but most 
mortals do.  And this is not easy with your approach.

In contrast, by using my approach of having an option_table for a bundle 
of common options, which just set variables in a certain struct, you can 
have a relatively painless migration, and you get all the benefits of 
parse-options.

But I guess the approach of whoever has more time to work on it will 
win... ;-)

Ciao,
Dscho

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

[Linus Torvalds]

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

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.

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

That second thing is an annoyance, and could be handled either with a new 
flag (PARSE_OPT_PUT_BACK_IDENTICALLY), or by just changing the calling 
convention. The latter is the "right thing", but it needs trivial changes 
in all existing callers.

I actually suspect that the best fix for the second issue is to yes, 
change the calling convention so that it puts things back where it found 
them, and returns a "count plus one" count, *but* have a special case 
where "if all arguments are used, return 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

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

[Jeff King]

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 

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

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

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

-Peff

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

[Linus Torvalds]

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.

YOU THINK it is "independent" just because YOUR SOLUTION can do only one 
case.

And I say: my solution handles both cases, so it's not "independent" any 
more.

See? 

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

		Linus

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

[Jeff King]

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 (which was what
started this whole thing) since we don't end up with the same repeated
subsets of options tables, and in particular the one-liner help text
(yes, compilers can sometimes point share the string literal components,
but most of our options tables are declared as static, so unless the
linker is very smart, I think we will end up with duplicates).

> 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

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

[Linus Torvalds]

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

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.

And that CONTINUE_ON_UNKNOWN behaviour is not some kind of theoretical 
flag. It's the flag that builtin-blame.c *wants*. It's the flag that 
describes hat builtin-blame.c does right now in the current git master 
branch.

Try just looking at the code!

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.

So can we please stop the clusterfuck now?

		Linus

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

[Jeff King]

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.

How do you know that is the case, unless you know what the other option
parsers are going to do? Or are you suggesting that I check every other
downstream option parser to make sure that it's OK in this particular
instance, use IGNORE_UNKNOWN, and then laugh maniacally when somebody
adds such an option to the diff option parser later?

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

I did. Or maybe you missed the thread where this exact feature was
mentioned, and I already looked at the code and mentioned those two
spots. It's right here:

  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.

There is already a discussion underway about the proper solution. This
isn't just a git-blame issue, but rather an issue with all commands that
have their own options and take revision parameters. So I think rather
than doing a halfway fix that happens to work with git-blame, it is more
useful to focus on a solution that works everywhere and fix _all_ of the
problems.

-Peff

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

[Linus Torvalds]

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. 

		Linus

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

[Jeff King]

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.

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. Now again, as it happens, git-blame seems to ignore "-n" entirely
(though I would have expected it to limit the depth of the blame
traversal), so maybe people shouldn't be using it.

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

I guessed "to convert the revision and diff options to a format that
parse_options understands, so we can add them as appropriate to the
options tables of the various commands." Do I win anything?

-Peff

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

[Linus Torvalds]

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.

The recursion discussion has been going on for ages. Nothing happened, 
because people claimed it was too complex. And it doesn't solve all the 
_other_ issues where the current behaviour is just painful.

That said, I don't care how this gets fixed. If you want to fix it some 
other way, fine. Just _do_ it, dammit. And make the interface easier to 
use for incremental changes of existing code.

Because no, I don't care about builtin-blame.c in particular either. But I 
*do* care about the fact that currently it's sometimes a total *bitch* to 
convert the trivial ad-hoc argument parsing to use parse_options(). It 
takes effort and thinking.

And I seriously doubt that recursive or anything else will help that. But 
I _guarantee_ that an incremental mode will make it much much easier to do 
partial conversions of all the easy cases. And _that_ is what I care 
about.

		Linus

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

[Johannes Schindelin]

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

So what you actually do is just linearizing the recursion.  Well then, 
yes, your solution is good.

However, I thought that recursion (as I implemented it) would make the 
revlist option parsing (which includes diff parsing) quite nice.

Whatever,
Dscho

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

[Johannes Schindelin]

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

Yes, this is the only difference between Pierre's current approach and my 
POC.

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

Heh.  I thought I saw it, but a quick search did not reveal it, and I 
really don't have time to work on this, unfortunately.

Thanks,
Dscho

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

[Linus Torvalds]

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.

But we should *also* have some way to do things like the code in 
builtin-ls-files.c which have a few options that don't work well with the 
current parse_options(). Yes, you can make all of them work with 
callbacks, but that often ends up requiring moving arguments around. 
There's no way to make a trivial conversion for 90% of the cases, and then 
leaving the 10% that need other changes.

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.

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?

			Linus

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

[Linus Torvalds]

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.

This is the trivial version that (on top of my original patch in this 
thread) makes

	git blame --since=April -b Makefile

work.

Does it handle all cases? No. In particular, because it still makes 
builtin-blame.c use PARSE_OPT_STOP_AT_NON_OPTION, and because cmd_blame.c 
simply doesn't do the sane thing (it took me more than five minutes just 
because I tried to make it do the same thing and not do any argument 
parsing at all, but I just gave up), you still cannot pass it things like

	--default HEAD

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

Could it be improved? I'm sure. And I didn't test it much at all. I also 
didn't change any existing users that could possibly use the new 
PARSE_OPT_STOP_AT_UNKNOWN thing. So this patch is an example patch only, 
to show what I'm talking about when I say "simple".

		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 {

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

[Pierre Habouzit]

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

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

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

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

[Jeff King]

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.

Your statement is obviously loaded. I haven't seen _anything_ that fixes
it except what I have already mentioned. But I'm sure you are going to
complain that it isn't easy enough.

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

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.

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

I don't have a problem with STOP_ON_UNKNOWN, as I think it is a building
block upon which sane things can be done (like linearly going through
each parser and saying "did you want this?"). I think IGNORE/CONTINUE
has a fundamental flaw.

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

-Peff

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

[Pierre Habouzit]

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

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

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

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

[Linus Torvalds]

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.

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? 

The thign is, parse_options() was introduced 8 months ago or so. Have you 
looked at the output of

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

lately? Yes, they probably all "ought to be" something or other, but as it 
is, the fact is that they are a damn pain to convert.

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. 

			Linus

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

[Pierre Habouzit]

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

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


It's slightly more involved than what Linus handwaved, but I believe
it's usable this way.

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

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

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

[Pierre Habouzit]

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

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

[Pierre Habouzit]

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

[PATCH] parse-opt: create parse_options_step.

[Pierre Habouzit]

For now it's unable to stop at unknown options, this commit merely
reorganize some code around.

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

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

[Pierre Habouzit]

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

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

[Pierre Habouzit]

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

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

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

[Pierre Habouzit]

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

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

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

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

[Junio C Hamano]

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

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

[Linus Torvalds]

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?

			Linus

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

[Pierre Habouzit]

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

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

Well it's still rough on the edges, totally untested (I didn't bother to
run the testsuite), but I wanted some feedback before I cook this to
something nicer.

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

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

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

[Linus Torvalds]

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!

Side note: of course you can always just queue them up yourself, and then 
pass those off to the later stage. But where do you queue them? Can you 
re-use argv[]? If so, what's the difference with the simpler patch that I 
already posted? (simpler both from a usage standpoint _and_ from a patch 
size standpoint).

That said, I *really* don't care what the end result is, but I would 
strongly urge people to actually look at what existing code does. That 
includes at a minimum builtin-blame.c, but also all the existing users 
where _most_ of the arguments are just trivial. builtin-apply.c is a great 
example of something where it's almost all trivial, but a lot of the 
options set more than one value.

The guiding light here should not be how pretty or clean option-parse.c 
is, but how easy it is to convert the hundreds of existing options that 
haven't been converted yet. And a lot of them haven't been converted 
because it's currently very painful to do multi-valued things, even if 
most of those values really are pretty simple.

In that vein - maybe it would be a good idea to allow multiples of the 
same option, so that things like builtin-apply.c can handle the "-p" 
option (to give an example of this behaviour) as a set of TWO options that 
do

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

where "-p3" would then trigger on both, once setting "p_value" to 3, and 
once setting p_value_known to 1.

I dunno. But _this_ is the real problem with parse-options.c. It's hard to 
convert existing really simple parsers. Whether the problem is writing a 
sane set of rules for it without using callbacks (callbacks are neither 
easy nor clean), or whether the problem is that the parsing is done in 
multiple different places..

			Linus

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

[Pierre Habouzit]

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

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!

Well I really believe that with an incremental parse_revisions (I have
sent the patch to split {init,parse,setup}_revisions already) instead of
the 'do all at once' version, you can write the code in builtin-blame in
one pass exactly with the series I wrote.

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

I don't find it "hard to accept", I just don't like the idea because
it's broken wrt some parse-options mechanisms that I like a lot, and
would be sad to see broken, and that with a little work can be
overcomed. Note that builtin-blame is contains probably the most
involved option parser in git.git, and isn't really what one could call
representative of what awaits us on the path of migrating git.git to
parse-options fully.

I don't expect it to be easy to migrate at all. I do believe your code
works too, it's obvious to see it does actually. But it could be made
better, with real code that I will provide, just not tonight because I'm
exhausted.

Note that an incremental parse_revisions/parse_diff_opts/.. are a
precodition to many CLI parsers conversions anyways, so if the task
seems a bit exagerated for the sake of git-blame only, it actually helps
for dozens of other commands, where the conversion will just be trivial.

I fully understand you frustration wrt the poor handling of the command
line arguments in many git commands. Believe me I really do. I know I
haven't been active on this front for something like 6 months, but I'm
sadly not payed to work on git.git ;p Though things are moving right
now, so why don't we wait another week to see what other propose ?

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

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

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

[Junio C Hamano]

You'd need something like this on top.

Documentation/technical/api-parse-options says that non-zero return is
used to signal an error.



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

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

[Junio C Hamano]

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.

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

[Pierre Habouzit]

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

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

I'm not 100% convinced by the current usage generation yet. But I'll
sleep on it first.

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

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

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

[Pierre Habouzit]

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

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

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

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

[Pierre Habouzit]

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

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.

Actually one can do it this way without breaking "backward
compatibility" here:

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

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

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

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

[Junio C Hamano]

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.

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

[Pierre Habouzit]

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

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

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

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

[Linus Torvalds]

On Tue, 24 Jun 2008, Pierre Habouzit wrote:
> 
>   Are we sure argv[argc] is NULL when those are main() arguments ?

Yes. There's tons of code that ends up not caring about argc at all, so 
any system that doesn't NULL-terminate would break.

In fact, git itself already depends on it, because it turns argv directly 
into the pathspec name pattern array.

		Linus

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

[Junio C Hamano]

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.

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.

I am not saying that forcing these callers to change their loop invariants
in order to use parse_options() was bad.  I am saying that your "would
rather be a pain for them" is a bogus argument --- they needed to pay that
price when converting to parse_options() that munges argv[0] anyway.

But this argv[0] munging exactly is why parse_options() hurts users who
want to cascade option parsing.

Didn't one of the recent series have an option to tell parse_options() to
start parsing from argv[0] (persumably because it got an argument array
from somebody who munges it to drop argv[0])?  I think it was merge-in-C
from Miklos, but the approach feels very much backwards.  The caller
should be (at least) able to choose to say KEEP_ARGV0 like one of your
patch does.

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

[Junio C Hamano]

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

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

[Jeff King]

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

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

[Jeff King]

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.

Did you even read my mail? All I said "ought to be" is that revision
options "ought to be" affecting things in the struct. As in, when I
convert this to parse_options, if there are any cases which don't fall
into that pattern then I "ought to be" able to convert them as part of
the code cleanup. And looking over the revision parsing code, it looks
like everything falls into that case already.

So if you have something FACTUAL to say that contradicts that point, by
all means do it. You might save me some work if you find a showstopper
in my approach.

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

Well, given that I already said I would work on it, I guess that makes
me the sky wizard. Bow down before my wizardly might!

That being said, it looks like Pierre is working on a slightly different
approach, and I am happy to sit back and see how that pans out for now.
So perhaps he is the sky wizard.

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

Maybe we are fundamentally talking across each other. I'm not talking
about some specifics for converting a bunch of programs, or some
problems with parse_options. I'm suggesting that the main "two stage"
option parsing generally has the revision parsing as the "second stage".
Converting that one thing to parse_options will help with converting
many programs (e.g., a failed conversion of shortlog that others and I
worked on).

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

-Peff

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

[Pierre Habouzit]

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

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

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

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

[Pierre Habouzit]

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

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

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

Making parse-opt incremental, reworked series

[Pierre Habouzit]

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

The patch 7 is only a proof of concept, not meant to be merged at all, I
believe the rest is ready enough for now.

I'll try to do what I comment in patch 7/7 (reworking revisions option
parsing to be incremental as well) as soon as I have some time for it.

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

[Pierre Habouzit]

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

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

[Pierre Habouzit]

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

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

[Pierre Habouzit]

For now it's unable to stop at unknown options, this commit merely
reorganize some code around.

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

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

[Pierre Habouzit]

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

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

[Pierre Habouzit]

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

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

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

[Pierre Habouzit]

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 */
+	PARSE_OPT_KEEP_ARGV0 = 4,
 };
 
 enum parse_opt_option_flags {
-- 
1.5.6.110.g736c7.dirty

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

[Pierre Habouzit]

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

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

[Pierre Habouzit]

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

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

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

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

[Linus Torvalds]

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. 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 
parsing together in one place) couldn't handle it as one issue and one 
problem. Your solutions always looked at just some small part of it.

So no, I never introduced any other problems in the discussion at all. I 
had a single issue, and a single solution. You were the one who then 
argued against it and had *another* solution that fractured the problem 
up, and didn't actually solve _any_ of my original issues.

Do you see now?

So yes, we're arguing at cross purposes, but that's because you're 
constantly taking up a totally different and totally uninteresting 
position that has nothing what-so-ever to do with the original problem.

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!

			Linus

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

[Linus Torvalds]

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.

			Linus

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

[Johannes Schindelin]

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.

I never wanted anything else than having a way to convert the rest of the 
Git programs to parse options.

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

But I never wanted anything as complicated as to merit a 7-mail patch 
series that I have no time to even look at.

> Do you see now?

I do see now.

Sorry,
Dscho

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

[Linus Torvalds]

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

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

[Linus Torvalds]

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?

		Linus

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

[Jeff King]

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 

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.

But whether you want to call that a "single" problem or not, my point
was that I am not talking about most of those things.

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

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

That is the entirety of my point. I am glad you are trying to increase
parse_options uptake. There is obviously a problem with multi-stage
parsers. I talked about one way for them to be handled. I think there
are multiple ways of going about it. It looks like STOP_ON_UNKNOWN
is the way that Pierre is pursuing. I think this is good, because it
doesn't suffer from the corner cases that CONTINUE_ON_UNKNOWN does.

And now I will stop making these points, because I don't think I am
capable of saying them any more clearly than I already have, and because
Pierre seems to be moving in a sane direction.

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

Again, did you even read the mail you are responding to? The phrase
"ought to be" was totally incidental to the point I was making. I could
just as easily have said "and this is the method that I think will be
acceptable for dealing with this problem." But for some reason you
insist on harping on the phrase as if I have proposed magical fairies
should come work on the code, and totally ignoring the actual points
that were made.

-Peff

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

[Linus Torvalds]

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.

It was a single issue - the issue of being able to do incremental parsing, 
and mixing *different* parsers together (not just two different calls to 
"parse_options()", but also having hand-parsing still in the picture for 
things where it was hard to convert).

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.

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

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.

So screw it. I'm simply not interested in discussing this with you. You 
aren't apparently interested in looking at the problems we have today, 
because you're only interested in fixing some theoretical case.

		Linus

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

[Pierre Habouzit]

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

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

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

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

[Pierre Habouzit]

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

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

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

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

[Pierre Habouzit]

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

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

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

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

[Pierre Habouzit]

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

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

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

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

[Jeff King]

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.

OK, fair enough. You are working towards a single goal. But I think
there is a flaw with that one sub-part of your goal (specifically your
second bullet point). I don't care about the other parts, and in fact I
even said "this is a fine way of doing that".

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

How can you even say something so ridiculous? Were you sitting at my
computer all day yesterday, making sure I didn't read your patch? No?
Funnily enough, I was there. And guess what I saw? Yes, it was me
READING YOUR PATCH (in a mirror, of course).

But I don't expect you to have a camera installed over my shoulder. So I
guess you would have had to just content yourself with THE EMAILS WHERE I
MENTION YOUR PATCH AND ITS EFFECTS.

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

I care. Apparently Junio cared in the thread that I pointed out earlier.
Other commands that parse the options will care, if we are to ever port
them to parse_options. I understand that you don't care about those
things, and you only want git-blame to work.  I am merely trying to help
amortize work that goes into fixing git-blame into helping to fix other
commands. To do that, I pointed out a flaw that might prevent the same
solution being used again.

I understand that you are interested in incremental change, and that
doing something now in git-blame is better than doing nothing while we
wait for a longer, more complete solution to arrive. But did I say "No,
we shouldn't apply this patch from Linus because it's not the optimal
solution?" No. Instead, I said "In the meantime, your patch does not
make this particular problem any worse."

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

I KNOW AND I EVEN SAID THAT. But here it is in case you are hard of
hearing:

  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.

Again, how in the world can you say that I didn't look at how things do
work WHEN I CUT AND PASTED A SAMPLE TRANSCRIPT SHOWING HOW THEY DO WORK?

BTW, this is the third time you have mentioned the phrase "ought to
work" in a way that is totally disingenuous about what I actually said.
I can understand that perhaps I was not clear in the initial statement.
But I would have thought the other two times I explained it further
would have made it clear. So did you truly not understand my point, or
are you just being intentionally rude?

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

And it's not correct as it is now. And we have lived with it. Which is
why I didn't say "your proposal makes things worse." In fact, I said the
opposite.

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

I already volunteered to work on it. I'm sorry that the patch didn't
materialize immediately, but I was working on other parts of git, just
as I mentioned when I said I would work on it.

In the meantime, I think Pierre has proposed an alternate approach that
also has promise, so I think it makes sense to see how he progresses
with that rather than potentially duplicate work.

And I will shut up now, because this is obviously getting nowhere. I
just find your responses so rude and misinformed not about technical
matters, but about my statements and my intent that I feel the need to
publicly defend myself.

-Peff

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

[Pierre Habouzit]

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

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


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

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

[Pierre Habouzit]

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

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

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


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

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

[Johannes Sixt]

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

We use threads only for those fork()s that are not followed by an exec().
So this is not a reason to worry about memory leaks in the option parser.

-- Hannes

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

[Andreas Ericsson]

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

Why not just provide some api-functions to return a strbuf of the short
and long options each?

parse_opt_short_help(strbuf *sb, options...);
parse_opt_long_help(strbuf *sb_long, options...);

That way multi-parseopt programs can get their help-texts done right
with very little extra work.

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

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

[Pierre Habouzit]

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


  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

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

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

[Junio C Hamano]

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.  I've been swamped and I still have your "revisions:
refactor init_revisions and setup_revisions" sitting in my inbox.

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

[Pierre Habouzit]

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

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

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