[RFC] CLI option parsing and usage generation for porcelains

[RFC] CLI option parsing and usage generation for porcelains

[Pierre Habouzit]

  Following Kristian momentum, I've reworked his parse_option module
quite a lot, and now have some quite interesting features. The series is
available from git://git.madism.org/git.git (branch ph/strbuf).

  The following series is open for comments, it's not 100% ready for
inclusion IMHO, as some details may need to be sorted out first, and
that I've not re-read the patches thoroughly yet. Though I uses the tip
of that branch as my everyday git for 2 weeks or so without any
noticeable issues.

  And as examples are always easier to grok:

$ git fetch -h
usage: git-fetch [options] [<repository> <refspec>...]

  -q, --quiet           be quiet
  -v, --verbose         be verbose

  -a, --append          append in .git/FETCH_HEAD
  -f, --force           force non fast-forwards updates
  --no-tags             don't follow tags at all
  -t, --tags            fetch all tags
  --depth <depth>       deepen history of a shallow clone

Advanced Options
  -k, --keep            keep downloaded pack
  -u, --update-head-ok  allow to update the head in the current branch
  --upload-pack <path>  path to git-upload-pack on the remote

$ git rm -rf xdiff # yeah -rf now works !
rm 'xdiff/xdiff.h'
rm 'xdiff/xdiffi.c'
rm 'xdiff/xdiffi.h'
rm 'xdiff/xemit.c'
rm 'xdiff/xemit.h'
rm 'xdiff/xinclude.h'
rm 'xdiff/xmacros.h'
rm 'xdiff/xmerge.c'
rm 'xdiff/xprepare.c'
rm 'xdiff/xprepare.h'
rm 'xdiff/xtypes.h'
rm 'xdiff/xutils.c'
rm 'xdiff/xutils.h'

Re: [PATCH] Add a simple option parser.

[Johannes Schindelin]

Hi,

On Sat, 13 Oct 2007, Pierre Habouzit wrote:

> Aggregation of single switches is allowed:
>   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).

I'd be more interested in "-rC 0" working...  Is that supported, too?

> diff --git a/parse-options.c b/parse-options.c
> new file mode 100644
> index 0000000..07abb50
> --- /dev/null
> +++ b/parse-options.c
> @@ -0,0 +1,227 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +#include "strbuf.h"
> +
> +#define OPT_SHORT 1
> +#define OPT_UNSET 2
> +
> +struct optparse_t {
> +	const char **argv;
> +	int argc;
> +	const char *opt;
> +};
> +
> +static inline const char *get_arg(struct optparse_t *p)
> +{
> +	if (p->opt) {
> +		const char *res = p->opt;
> +		p->opt = NULL;
> +		return res;
> +	}
> +	p->argc--;
> +	return *++p->argv;
> +}

This is only used once; I wonder if it is really that more readable having 
this as a function in its own right.

> +static inline const char *skippfx(const char *str, const char *prefix)

Personally, I do not like abbreviations like that.  They do not save that 
much screen estate (skip_prefix is only 4 characters longer, but much more 
readable).  Same goes for "cnt" later.

> +static int get_value(struct optparse_t *p, struct option *opt, int flags)
> +{
> +	if (p->opt && (flags & OPT_UNSET))
> +		return opterror(opt, "takes no value", flags);
> +
> +	switch (opt->type) {
> +	case OPTION_BOOLEAN:
> +		if (!(flags & OPT_SHORT) && p->opt)
> +			return opterror(opt, "takes no value", flags);
> +		if (flags & OPT_UNSET) {
> +			*(int *)opt->value = 0;
> +		} else {
> +			(*(int *)opt->value)++;
> +		}
> +		return 0;
> +
> +	case OPTION_STRING:
> +		if (flags & OPT_UNSET) {
> +			*(const char **)opt->value = (const char *)NULL;
> +		} else {
> +			if (!p->opt && p->argc < 1)
> +				return opterror(opt, "requires a value", flags);
> +			*(const char **)opt->value = get_arg(p);
> +		}
> +		return 0;
> +
> +	case OPTION_INTEGER:
> +		if (flags & OPT_UNSET) {
> +			*(int *)opt->value = 0;
> +		} else {
> +			const char *s;
> +			if (!p->opt && p->argc < 1)
> +				return opterror(opt, "requires a value", flags);
> +			*(int *)opt->value = strtol(*p->argv, (char **)&s, 10);
> +			if (*s)
> +				return opterror(opt, "expects a numerical value", flags);
> +		}
> +		return 0;
> +
> +	default:
> +		die("should not happen, someone must be hit on the forehead");

:-P

> +static int parse_long_opt(struct optparse_t *p, const char *arg,
> +                          struct option *options, int count)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		const char *rest;
> +		int flags = 0;
> +		
> +		if (!options[i].long_name)
> +			continue;
> +
> +		rest = skippfx(arg, options[i].long_name);
> +		if (!rest && !strncmp(arg, "no-", 3)) {
> +			rest = skippfx(arg + 3, options[i].long_name);
> +			flags |= OPT_SHORT;
> +		}

Would this not be more intuitive as

		if (!prefixcmp(arg, "no-")) {
			arg += 3;
			flags |= OPT_UNSET;
		}
		rest = skip_prefix(arg, options[i].long_name);

Hm?  (Note that I say UNSET, not SHORT... ;-)

> +		if (!rest)
> +			continue;
> +		if (*rest) {
> +			if (*rest != '=')
> +				continue;

Is this really no error?  For example, "git log 
--decorate-walls-and-roofs" would not fail...

> +int parse_options(int argc, const char **argv,
> +                  struct option *options, int count,
> +				  const char * const usagestr[], int flags)

Please indent by the same amount.

> +		if (arg[1] != '-') {
> +			optp.opt = arg + 1;
> +			do {
> +				if (*optp.opt == 'h')
> +					make_usage(usagestr, options, count);

How about calling this "usage_with_options()"?  With that name I expected 
make_usage() to return a strbuf.

> +		if (!arg[2]) { /* "--" */
> +			if (!(flags & OPT_COPY_DASHDASH))
> +				optp.argc--, optp.argv++;

I would prefer this as 

			if (!(flags & OPT_COPY_DASHDASH)) {
				optp.argc--;
				optp.argv++;
			}

While I'm at it: could you use "args" instead of "optp"?  It is misleading 
both in that it not only contains options (but other arguments, too) as in 
that it is not a pointer (the trailing "p" is used as an indicator of that 
very often, including git's source code).

In the same vein, OPT_COPY_DASHDASH should be named 
PARSE_OPT_KEEP_DASHDASH.

> +		if (opts->short_name) {
> +			strbuf_addf(&sb, "-%c", opts->short_name);
> +		}
> +		if (opts->long_name) {
> +			strbuf_addf(&sb, opts->short_name ? ", --%s" : "--%s",
> +						opts->long_name);
> +		}

Please lose the curly brackets.

> +		if (sb.len - pos <= USAGE_OPTS_WIDTH) {
> +			int pad = USAGE_OPTS_WIDTH - (sb.len - pos) + USAGE_GAP;
> +			strbuf_addf(&sb, "%*s%s\n", pad, "", opts->help);
> +		} else {
> +			strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> +						opts->help);
> +		}

Same here.  (And I'd also make sure that the lines are not that long.)

> diff --git a/parse-options.h b/parse-options.h
> new file mode 100644
> index 0000000..4b33d17
> --- /dev/null
> +++ b/parse-options.h
> @@ -0,0 +1,37 @@
> +#ifndef PARSE_OPTIONS_H
> +#define PARSE_OPTIONS_H
> +
> +enum option_type {
> +	OPTION_BOOLEAN,

I know that I proposed "BOOLEAN", but actually, you use it more like an 
"INCREMENTAL", right?

Other than that: I like it very much.

Ciao,
Dscho

Re: [PATCH] Port builtin-add.c to use the new option parser.

[Johannes Schindelin]

Hi,

On Sat, 13 Oct 2007, Pierre Habouzit wrote:

> +static struct option builtin_add_options[] = {
> +	OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
> +	OPT_BOOLEAN('n', NULL, &show_only, "dry-run"),
> +	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
> +	OPT_BOOLEAN('v', NULL, &verbose, "be verbose"),
> +	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files that git already knows about"),
> +	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh stat() informations in the index"),
> +};

I see you terminate the list by a ",".  How does this play with the option 
parser?

Thinking about this more, I am reverting my stance on the ARRAY_SIZE() 
issue.  I think if you introduce a "OPTION_NONE = 0" in the enum, then 
this single last comma should be enough.

In the same vein, you would not need the NULL in builtin_add_usage[], 
right?

Ciao,
Dscho

Re: [RFC] CLI option parsing and usage generation for porcelains

[Wincent Colaiuta]

El 13/10/2007, a las 15:29, Pierre Habouzit escribió:

>   The following series is open for comments, it's not 100% ready for
> inclusion IMHO, as some details may need to be sorted out first, and
> that I've not re-read the patches thoroughly yet. Though I uses the  
> tip
> of that branch as my everyday git for 2 weeks or so without any
> noticeable issues.

Great to see two things:

- the simplification in the commands switched over to use the options  
parser

- the improved readability and usefulness of the options help

Great work, Pierre! I'll take a closer look at this and trial it in  
my local Git install for a while to see if any issues come up.

Wincent

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

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

On Sat, Oct 13, 2007 at 02:39:10PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 13 Oct 2007, Pierre Habouzit wrote:
> 
> > Aggregation of single switches is allowed:
> >   -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
> 
> I'd be more interested in "-rC 0" working...  Is that supported, too?

  yes it is.

> > +static inline const char *get_arg(struct optparse_t *p)
> > +{
> > +	if (p->opt) {
> > +		const char *res = p->opt;
> > +		p->opt = NULL;
> > +		return res;
> > +	}
> > +	p->argc--;
> > +	return *++p->argv;
> > +}
> 
> This is only used once; I wonder if it is really that more readable having 
> this as a function in its own right.

  it's used twice, and it makes the code more readable I believe.

> > +static inline const char *skippfx(const char *str, const char *prefix)
> 
> Personally, I do not like abbreviations like that.  They do not save that 
> much screen estate (skip_prefix is only 4 characters longer, but much more 
> readable).  Same goes for "cnt" later.

  Ack I'll fix that.

> > +static int parse_long_opt(struct optparse_t *p, const char *arg,
> > +                          struct option *options, int count)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		const char *rest;
> > +		int flags = 0;
> > +		
> > +		if (!options[i].long_name)
> > +			continue;
> > +
> > +		rest = skippfx(arg, options[i].long_name);
> > +		if (!rest && !strncmp(arg, "no-", 3)) {
> > +			rest = skippfx(arg + 3, options[i].long_name);
> > +			flags |= OPT_SHORT;
> > +		}
> 
> Would this not be more intuitive as
> 
> 		if (!prefixcmp(arg, "no-")) {
> 			arg += 3;
> 			flags |= OPT_UNSET;
> 		}
> 		rest = skip_prefix(arg, options[i].long_name);

  Yes, that can be done indeed, but the point is, we have sometimes
option whose long-name is "no-foo" (because it's what makes sense) but I
can rework that.

> Hm?  (Note that I say UNSET, not SHORT... ;-)

  fsck, good catch.

> > +		if (!rest)
> > +			continue;
> > +		if (*rest) {
> > +			if (*rest != '=')
> > +				continue;
> 
> Is this really no error?  For example, "git log 
> --decorate-walls-and-roofs" would not fail...

  it would be an error, it will yield a "option not found".

> > +int parse_options(int argc, const char **argv,
> > +                  struct option *options, int count,
> > +				  const char * const usagestr[], int flags)
> 
> Please indent by the same amount.

  oops, stupid space vs. tab thing.

> > +		if (arg[1] != '-') {
> > +			optp.opt = arg + 1;
> > +			do {
> > +				if (*optp.opt == 'h')
> > +					make_usage(usagestr, options, count);
> 
> How about calling this "usage_with_options()"?  With that name I expected 
> make_usage() to return a strbuf.

  will do.

> > +		if (!arg[2]) { /* "--" */
> > +			if (!(flags & OPT_COPY_DASHDASH))
> > +				optp.argc--, optp.argv++;
> 
> I would prefer this as 
> 
> 			if (!(flags & OPT_COPY_DASHDASH)) {
> 				optp.argc--;
> 				optp.argv++;
> 			}
> 
> While I'm at it: could you use "args" instead of "optp"?  It is misleading 
> both in that it not only contains options (but other arguments, too) as in 
> that it is not a pointer (the trailing "p" is used as an indicator of that 
> very often, including git's source code).

  okay.

> In the same vein, OPT_COPY_DASHDASH should be named 
> PARSE_OPT_KEEP_DASHDASH.

  okay.

> 
> > +		if (opts->short_name) {
> > +			strbuf_addf(&sb, "-%c", opts->short_name);
> > +		}
> > +		if (opts->long_name) {
> > +			strbuf_addf(&sb, opts->short_name ? ", --%s" : "--%s",
> > +						opts->long_name);
> > +		}
> 
> Please lose the curly brackets.
> 
> > +		if (sb.len - pos <= USAGE_OPTS_WIDTH) {
> > +			int pad = USAGE_OPTS_WIDTH - (sb.len - pos) + USAGE_GAP;
> > +			strbuf_addf(&sb, "%*s%s\n", pad, "", opts->help);
> > +		} else {
> > +			strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> > +						opts->help);
> > +		}
> 
> Same here.  (And I'd also make sure that the lines are not that long.)

  okay.

> 
> > diff --git a/parse-options.h b/parse-options.h
> > new file mode 100644
> > index 0000000..4b33d17
> > --- /dev/null
> > +++ b/parse-options.h
> > @@ -0,0 +1,37 @@
> > +#ifndef PARSE_OPTIONS_H
> > +#define PARSE_OPTIONS_H
> > +
> > +enum option_type {
> > +	OPTION_BOOLEAN,
> 
> I know that I proposed "BOOLEAN", but actually, you use it more like an 
> "INCREMENTAL", right?

  yes, I don't like _BOOLEAN either, I would have prefered _FLAG or sth
like that. INCREMENTAL is just too long.

> Other than that: I like it very much.

:P

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

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

Re: [PATCH] Port builtin-add.c to use the new option parser.

[Pierre Habouzit]

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

On Sat, Oct 13, 2007 at 02:47:20PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 13 Oct 2007, Pierre Habouzit wrote:
> 
> > +static struct option builtin_add_options[] = {
> > +	OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
> > +	OPT_BOOLEAN('n', NULL, &show_only, "dry-run"),
> > +	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
> > +	OPT_BOOLEAN('v', NULL, &verbose, "be verbose"),
> > +	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files that git already knows about"),
> > +	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh stat() informations in the index"),
> > +};
> 
> I see you terminate the list by a ",".  How does this play with the option 
> parser?
> 
> Thinking about this more, I am reverting my stance on the ARRAY_SIZE() 
> issue.  I think if you introduce a "OPTION_NONE = 0" in the enum, then 
> this single last comma should be enough.

  adding a trailing comma does not add a NULL after that, it's ignored,
you're confused.

    ┌─(17:00)────
    └[artemis] cat a.c
    #include <stdio.h>

      int main(void) {
	const char * const arr[] = { "1", "2", };
	printf("%d\n", sizeof(arr) / sizeof(arr[0]));
	return 0;
    };
    ┌─(17:00)────
    └[artemis] ./a
    2

  Very few compilers do not grok trailing commas, I always put them
because it avoids spurious diffs for nothing, and that you can reorder
lines easily too.

  Note that I don't really like using ARRAY_SIZE either, I kept it that
way, but my taste would rather be to have an "empty" option, and
explicitely mark the end of the array.

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

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

Re: [PATCH] Add a simple option parser.

[Alex Riesen]

Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> +static int opterror(struct option *opt, const char *reason, int flags)

"const struct option *opt"? You never modify the struct option itself,
only the values under the pointers it contains. Using const here will
allow the compiler to reuse string constants (not that there will be
much of the opportunity, but anyway) in the option arrays.

> +static int get_value(struct optparse_t *p, struct option *opt, int flags)

"const struct option *opt"?

> +static int parse_short_opt(struct optparse_t *p, struct option *options, int count)

"const struct option *options"?

> +int parse_options(int argc, const char **argv,
> +                  struct option *options, int count,
> +				  const char * const usagestr[], int flags)

"const struct option *options"?

> +void make_usage(const char * const usagestr[], struct option *opts, int cnt)

"const struct option *opts"?

Why not "const char *const *usagestr"? Especially if you change
"usagestr" (the pointer itself) later. "[]" is sometimes a hint that
the pointer itself should not be changed, being an array.

And you want make opts const.

BTW, it does not "make" usage. It calls the usage() or prints a usage
description. "make" implies it creates the "usage", which according to
the prototype is later nowhere to be found.

> +{
> +	struct strbuf sb;
> +
> +	strbuf_init(&sb, 4096);
> +	do {
> +		strbuf_addstr(&sb, *usagestr++);
> +		strbuf_addch(&sb, '\n');
> +	} while (*usagestr);

This will crash for empty usagestr, like  "{ NULL }". Was it
deliberately? (I'd make it deliberately, if I were you. I'd even used
cnt of opts, to force people to document all options).

> +     strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> +		    opts->help);
...
> +	usage(sb.buf);

BTW, if you just printed the usage message out (it is about usage of a
program, isn't it?) and called exit() everyone would be just as happy.
And you wouldn't have to include strbuf (it is the only use of it),
less code, too. It'd make simplier to stea^Wcopy your implementation,
which I like :)

Re: [PATCH] Port builtin-add.c to use the new option parser.

[Alex Riesen]

Pierre Habouzit, Sat, Oct 13, 2007 17:03:06 +0200:
> On Sat, Oct 13, 2007 at 02:47:20PM +0000, Johannes Schindelin wrote:
> > Thinking about this more, I am reverting my stance on the ARRAY_SIZE() 
> > issue.  I think if you introduce a "OPTION_NONE = 0" in the enum, then 
> > this single last comma should be enough.
> 
>   adding a trailing comma does not add a NULL after that, it's ignored,
> you're confused.

Yep

>   Note that I don't really like using ARRAY_SIZE either, I kept it that
> way, but my taste would rather be to have an "empty" option, and
> explicitely mark the end of the array.

You can have both. Just stop at NULL-entry or when the 'size' elements
passed, whatever happens first.

Re: [PATCH] Port builtin-add.c to use the new option parser.

[Pierre Habouzit]

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

On Sat, Oct 13, 2007 at 07:22:13PM +0000, Alex Riesen wrote:
> Pierre Habouzit, Sat, Oct 13, 2007 17:03:06 +0200:
> > On Sat, Oct 13, 2007 at 02:47:20PM +0000, Johannes Schindelin wrote:
> > > Thinking about this more, I am reverting my stance on the ARRAY_SIZE() 
> > > issue.  I think if you introduce a "OPTION_NONE = 0" in the enum, then 
> > > this single last comma should be enough.
> > 
> >   adding a trailing comma does not add a NULL after that, it's ignored,
> > you're confused.
> 
> Yep
> 
> >   Note that I don't really like using ARRAY_SIZE either, I kept it that
> > way, but my taste would rather be to have an "empty" option, and
> > explicitely mark the end of the array.
> 
> You can have both. Just stop at NULL-entry or when the 'size' elements
> passed, whatever happens first.

  Well I dislike the "count" thing, and Dscho agreed that it somehow
sucked too. If you go see the current state of the ph/parseopt series
you'll see it's not here anymore.

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

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

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

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

On Sat, Oct 13, 2007 at 07:16:55PM +0000, Alex Riesen wrote:
> Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> > [...]
> 
> "const struct option *opts"?
> 
> Why not "const char *const *usagestr"? Especially if you change
> "usagestr" (the pointer itself) later. "[]" is sometimes a hint that
> the pointer itself should not be changed, being an array.
> 
> And you want make opts const.

  Ok.

> BTW, it does not "make" usage. It calls the usage() or prints a usage
> description. "make" implies it creates the "usage", which according to
> the prototype is later nowhere to be found.

  Yes this has been spotted and fixed already.

> 
> > +{
> > +	struct strbuf sb;
> > +
> > +	strbuf_init(&sb, 4096);
> > +	do {
> > +		strbuf_addstr(&sb, *usagestr++);
> > +		strbuf_addch(&sb, '\n');
> > +	} while (*usagestr);
> 
> This will crash for empty usagestr, like  "{ NULL }". Was it
> deliberately? (I'd make it deliberately, if I were you. I'd even used
> cnt of opts, to force people to document all options).

  Yes this is intentional, there should be at least on string in the
usagestr array.


> > +     strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> > +		    opts->help);
> ....
> > +	usage(sb.buf);
> 
> BTW, if you just printed the usage message out (it is about usage of a
> program, isn't it?) and called exit() everyone would be just as happy.
> And you wouldn't have to include strbuf (it is the only use of it),
> less code, too. It'd make simplier to stea^Wcopy your implementation,
> which I like :)

  the reason is that usage() is a wrapper around a callback, and I
suppose it's used by some GUI's or anything like that.

  FWIW you can rework the .c like this:

  pos = 0; /* and not pos = sb.len */

  replace the strbuf_add* by the equivalents:
  pos += printf("....");

  and tada, you're done.


  Note that in the most recent version, I also deal with a
OPTION_CALLBACK that passes the value to a callback.

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

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

Re: [PATCH] Add a simple option parser.

[Alex Riesen]

Pierre Habouzit, Sat, Oct 13, 2007 22:54:04 +0200:
> On Sat, Oct 13, 2007 at 07:16:55PM +0000, Alex Riesen wrote:
> > Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> > BTW, if you just printed the usage message out (it is about usage of a
> > program, isn't it?) and called exit() everyone would be just as happy.
> > And you wouldn't have to include strbuf (it is the only use of it),
> > less code, too. It'd make simplier to stea^Wcopy your implementation,
> > which I like :)
> 
>   the reason is that usage() is a wrapper around a callback, and I
> suppose it's used by some GUI's or anything like that.

It is not. Not yet. What could they use a usage text for?
Besides, you could just export the callback (call_usage_callback or
something) from usage.c and call it.

>   FWIW you can rework the .c like this:

on top of yours:

From: Alex Riesen <raa.lkml@gmail.com>
Date: Sun, 14 Oct 2007 00:10:51 +0200
Subject: [PATCH] Rework make_usage to print the usage message immediately

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 parse-options.c |   60 ++++++++++++++++++++++++------------------------------
 1 files changed, 27 insertions(+), 33 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 07abb50..1e3940f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1,6 +1,5 @@
 #include "git-compat-util.h"
 #include "parse-options.h"
-#include "strbuf.h"
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
@@ -171,57 +170,52 @@ int parse_options(int argc, const char **argv,
 
 void make_usage(const char * const usagestr[], struct option *opts, int cnt)
 {
-	struct strbuf sb;
-
-	strbuf_init(&sb, 4096);
-	do {
-		strbuf_addstr(&sb, *usagestr++);
-		strbuf_addch(&sb, '\n');
-	} while (*usagestr);
+	fprintf(stderr, "usage: ");
+	while (*usagestr)
+		fprintf(stderr, "%s\n", *usagestr++);
 
 	if (cnt && opts->type != OPTION_GROUP)
-		strbuf_addch(&sb, '\n');
+		fputc('\n', stderr);
 
 	for (; cnt-- > 0; opts++) {
 		size_t pos;
 
 		if (opts->type == OPTION_GROUP) {
-			strbuf_addch(&sb, '\n');
+			fputc('\n', stderr);
 			if (*opts->help)
-				strbuf_addf(&sb, "%s\n", opts->help);
+				fprintf(stderr, "%s\n", opts->help);
 			continue;
 		}
 
-		pos = sb.len;
-		strbuf_addstr(&sb, "    ");
-		if (opts->short_name) {
-			strbuf_addf(&sb, "-%c", opts->short_name);
-		}
-		if (opts->long_name) {
-			strbuf_addf(&sb, opts->short_name ? ", --%s" : "--%s",
-						opts->long_name);
-		}
+		pos = fprintf(stderr, "    ");
+		if (opts->short_name)
+			pos += fprintf(stderr, "-%c", opts->short_name);
+		if (opts->long_name)
+			pos += fprintf(stderr,
+				       opts->short_name ? ", --%s" : "--%s",
+				       opts->long_name);
 		switch (opts->type) {
 		case OPTION_INTEGER:
-			strbuf_addstr(&sb, " <n>");
+			fputs(" <n>", stderr);
+			pos += 4;
 			break;
 		case OPTION_STRING:
-			if (opts->argh) {
-				strbuf_addf(&sb, " <%s>", opts->argh);
-			} else {
-				strbuf_addstr(&sb, " ...");
+			if (opts->argh)
+				pos += fprintf(stderr, " <%s>", opts->argh);
+			else {
+				fputs(" ...", stderr);
+				pos += 4;
 			}
 			break;
 		default:
 			break;
 		}
-		if (sb.len - pos <= USAGE_OPTS_WIDTH) {
-			int pad = USAGE_OPTS_WIDTH - (sb.len - pos) + USAGE_GAP;
-			strbuf_addf(&sb, "%*s%s\n", pad, "", opts->help);
-		} else {
-			strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
-						opts->help);
-		}
+		if (pos <= USAGE_OPTS_WIDTH) {
+			int pad = USAGE_OPTS_WIDTH - pos + USAGE_GAP;
+			fprintf(stderr, "%*s%s\n", pad, "", opts->help);
+		} else
+			fprintf(stderr, "\n%*s%s\n",
+				USAGE_OPTS_WIDTH + USAGE_GAP, "", opts->help);
 	}
-	usage(sb.buf);
+	exit(129);
 }
-- 
1.5.3.4.232.ga843

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

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

On Sat, Oct 13, 2007 at 10:14:50PM +0000, Alex Riesen wrote:
> Pierre Habouzit, Sat, Oct 13, 2007 22:54:04 +0200:
> > On Sat, Oct 13, 2007 at 07:16:55PM +0000, Alex Riesen wrote:
> > > Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:
> > > BTW, if you just printed the usage message out (it is about usage of a
> > > program, isn't it?) and called exit() everyone would be just as happy.
> > > And you wouldn't have to include strbuf (it is the only use of it),
> > > less code, too. It'd make simplier to stea^Wcopy your implementation,
> > > which I like :)
> > 
> >   the reason is that usage() is a wrapper around a callback, and I
> > suppose it's used by some GUI's or anything like that.
> 
> It is not. Not yet. What could they use a usage text for?
> Besides, you could just export the callback (call_usage_callback or
> something) from usage.c and call it.

  Okay makes sense.

> >   FWIW you can rework the .c like this:
> 
> on top of yours:

  Added (reworked a bit for the current state of parse_options), and pushed.

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

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

Re: [RFC] CLI option parsing and usage generation for porcelains

[Eric Wong]

Pierre Habouzit <madcoder@debian.org> wrote:
>   Following Kristian momentum, I've reworked his parse_option module
> quite a lot, and now have some quite interesting features. The series is
> available from git://git.madism.org/git.git (branch ph/strbuf).
> 
>   The following series is open for comments, it's not 100% ready for
> inclusion IMHO, as some details may need to be sorted out first, and
> that I've not re-read the patches thoroughly yet. Though I uses the tip
> of that branch as my everyday git for 2 weeks or so without any
> noticeable issues.
> 
>   And as examples are always easier to grok:
> 
> $ git fetch -h
> usage: git-fetch [options] [<repository> <refspec>...]
> 
>   -q, --quiet           be quiet
>   -v, --verbose         be verbose
> 
>   -a, --append          append in .git/FETCH_HEAD
>   -f, --force           force non fast-forwards updates
>   --no-tags             don't follow tags at all
>   -t, --tags            fetch all tags
>   --depth <depth>       deepen history of a shallow clone
> 
> Advanced Options
>   -k, --keep            keep downloaded pack
>   -u, --update-head-ok  allow to update the head in the current branch
>   --upload-pack <path>  path to git-upload-pack on the remote
> 
> $ git rm -rf xdiff # yeah -rf now works !

Very nice.  I worked on gitopt around summer of 2006 but never had the
time to test it thoroughly.  It was a _lot_ more intrusive than yours
currently is (it touched the diff + revision family of commands).

One feature I really like is automatically handling of long option
abbreviations.  gitopt supported this at the expense of complexity
and the aforementioned intrusivenes.  This allows automatic handling
of the abbreviation style seen commonly in git shell scripts:

   --a|--am|--ame|--amen|--amend)  (from git-commit.sh)

-- 
Eric Wong

Re: [RFC] CLI option parsing and usage generation for porcelains

[Pierre Habouzit]

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

On Sun, Oct 14, 2007 at 09:18:55AM +0000, Eric Wong wrote:
> Pierre Habouzit <madcoder@debian.org> wrote:
> >   Following Kristian momentum, I've reworked his parse_option module
> > quite a lot, and now have some quite interesting features. The series is
> > available from git://git.madism.org/git.git (branch ph/strbuf).
> > 
> >   The following series is open for comments, it's not 100% ready for
> > inclusion IMHO, as some details may need to be sorted out first, and
> > that I've not re-read the patches thoroughly yet. Though I uses the tip
> > of that branch as my everyday git for 2 weeks or so without any
> > noticeable issues.
> > 
> >   And as examples are always easier to grok:
> > 
> > $ git fetch -h
> > usage: git-fetch [options] [<repository> <refspec>...]
> > 
> >   -q, --quiet           be quiet
> >   -v, --verbose         be verbose
> > 
> >   -a, --append          append in .git/FETCH_HEAD
> >   -f, --force           force non fast-forwards updates
> >   --no-tags             don't follow tags at all
> >   -t, --tags            fetch all tags
> >   --depth <depth>       deepen history of a shallow clone
> > 
> > Advanced Options
> >   -k, --keep            keep downloaded pack
> >   -u, --update-head-ok  allow to update the head in the current branch
> >   --upload-pack <path>  path to git-upload-pack on the remote
> > 
> > $ git rm -rf xdiff # yeah -rf now works !
> 
> Very nice.  I worked on gitopt around summer of 2006 but never had the
> time to test it thoroughly.  It was a _lot_ more intrusive than yours
> currently is (it touched the diff + revision family of commands).
> 
> One feature I really like is automatically handling of long option
> abbreviations.  gitopt supported this at the expense of complexity
> and the aforementioned intrusivenes.  This allows automatic handling
> of the abbreviation style seen commonly in git shell scripts:
> 
>    --a|--am|--ame|--amen|--amend)  (from git-commit.sh)

  Yes, but if you do that, you can't order options in the order you
want (because of first match issues), making the help dumps hopelessly
random. I prefer exact match, especially since your shell can help you
autocomplete the proper command.

  I intend to have some magic in the parse_options module to dump the
options in a machine parseable way, so that zsh/bash completion for the
parseopt aware commands is almost trivial. (this was requested from one
of the zsh upstream developpers, and it definitely make sense).

  Note that I didn't migrated all the commands yet especially not
diff.c, We'll need a new construct for that: embedding a struct options
array into another to inherit its flags, though I'm not sure it's
enough, as a struct options right now embeds pointers to the variables
it fills, which doesn't work with the "pure" `diff_opt_parse` approach
right now. But I'm sure I'll come up with something :)

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

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

[PATCH] Simplify usage string printing

[Jonas Fonseca]

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
 builtin-branch.c     |    1 -
 builtin-update-ref.c |    1 -
 parse-options.c      |    2 +-
 3 files changed, 1 insertions(+), 3 deletions(-)

 Pierre Habouzit <madcoder@debian.org> wrote Sat, Oct 13, 2007:
 > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
 > ---
 >  builtin-update-ref.c |   71 +++++++++++++++++++++-----------------------------
 >  1 files changed, 30 insertions(+), 41 deletions(-)
 > 
 > diff --git a/builtin-update-ref.c b/builtin-update-ref.c
 > index fe1f74c..eafb642 100644
 > --- a/builtin-update-ref.c
 > +++ b/builtin-update-ref.c
 > @@ -1,59 +1,48 @@
 >  #include "cache.h"
 >  #include "refs.h"
 >  #include "builtin.h"
 > +#include "parse-options.h"
 >  
 > -static const char git_update_ref_usage[] =
 > -"git-update-ref [-m <reason>] (-d <refname> <value> | [--no-deref] <refname> <value> [<oldval>])";
 > +static const char * const git_update_ref_usage[] = {
 > +	"",
 > +	"git-update-ref [options] -d <refname> <oldval>",
 > +	"git-update-ref [options]    <refname> <newval> [<oldval>]",
 > +	NULL
 > +};

 How about something like this to get rid of these empty strings
 that look strange?

	> ./git update-ref -h
	usage: git-update-ref [options] -d <refname> <oldval>
	   or: git-update-ref [options]    <refname> <newval> [<oldval>]

	    -m <reason>           reason of the update
	    -d                    deletes the reference
	    --no-deref            update <refname> not the one it points to

diff --git a/builtin-branch.c b/builtin-branch.c
index fbf983e..d7c4657 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -14,7 +14,6 @@
 #include "parse-options.h"
 
 static const char * const builtin_branch_usage[] = {
-	"",
 	"git-branch [options] [-r | -a]",
 	"git-branch [options] [-l] [-f] <branchname> [<start-point>]",
 	"git-branch [options] [-r] (-d | -D) <branchname>",
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index d66d9b5..0cd7817 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -4,7 +4,6 @@
 #include "parse-options.h"
 
 static const char * const git_update_ref_usage[] = {
-	"",
 	"git-update-ref [options] -d <refname> <oldval>",
 	"git-update-ref [options]    <refname> <newval> [<oldval>]",
 	NULL
diff --git a/parse-options.c b/parse-options.c
index c45bb9b..b1d9608 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -181,7 +181,7 @@ void usage_with_options(const char * const *usagestr,
 {
 	fprintf(stderr, "usage: %s\n", *usagestr);
 	while (*++usagestr)
-		fprintf(stderr, "    %s\n", *usagestr);
+		fprintf(stderr, "   or: %s\n", *usagestr);
 
 	if (opts->type != OPTION_GROUP)
 		fputc('\n', stderr);
-- 
1.5.3.4.1166.gf076

-- 
Jonas Fonseca

[PATCH] Update manpages to reflect new short and long option aliases

[Jonas Fonseca]

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
 Documentation/git-add.txt          |    4 ++--
 Documentation/git-branch.txt       |    2 +-
 Documentation/git-mv.txt           |    2 +-
 Documentation/git-rm.txt           |    4 ++--
 Documentation/git-symbolic-ref.txt |    2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

 Maybe this should wait, but it is just to document that this series (as
 of the version in your git tree) also adds new option aliases.
 
 BTW, I didn't bother to change the synopsis lines but maybe I should.

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 2fe7355..963e1ab 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -50,10 +50,10 @@ OPTIONS
 	and `dir/file2`) can be given to add all files in the
 	directory, recursively.
 
--n::
+-n, \--dry-run::
         Don't actually add the file(s), just show if they exist.
 
--v::
+-v, \--verbose::
         Be verbose.
 
 -f::
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index b7285bc..5e81aa4 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -85,7 +85,7 @@ OPTIONS
 -a::
 	List both remote-tracking branches and local branches.
 
--v::
+-v, --verbose::
 	Show sha1 and commit subject line for each head.
 
 --abbrev=<length>::
diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
index 2c9cf74..3b8ca76 100644
--- a/Documentation/git-mv.txt
+++ b/Documentation/git-mv.txt
@@ -34,7 +34,7 @@ OPTIONS
 	condition. An error happens when a source is neither existing nor
         controlled by GIT, or when it would overwrite an existing
         file unless '-f' is given.
--n::
+-n, \--dry-run::
 	Do nothing; only show what would happen
 
 
diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index be61a82..48c1d97 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -30,7 +30,7 @@ OPTIONS
 -f::
 	Override the up-to-date check.
 
--n::
+-n, \--dry-run::
         Don't actually remove the file(s), just show if they exist in
         the index.
 
@@ -51,7 +51,7 @@ OPTIONS
 \--ignore-unmatch::
 	Exit with a zero status even if no files matched.
 
-\--quiet::
+-q, \--quiet::
 	git-rm normally outputs one line (in the form of an "rm" command)
 	for each file removed. This option suppresses that output.
 
diff --git a/Documentation/git-symbolic-ref.txt b/Documentation/git-symbolic-ref.txt
index a88f722..694caba 100644
--- a/Documentation/git-symbolic-ref.txt
+++ b/Documentation/git-symbolic-ref.txt
@@ -26,7 +26,7 @@ a regular file whose contents is `ref: refs/heads/master`.
 OPTIONS
 -------
 
--q::
+-q, --quiet::
 	Do not issue an error message if the <name> is not a
 	symbolic ref but a detached HEAD; instead exit with
 	non-zero status silently.
-- 
1.5.3.4.1166.gf076

-- 
Jonas Fonseca

Re: [PATCH] Simplify usage string printing

[Pierre Habouzit]

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

On Sun, Oct 14, 2007 at 02:01:16PM +0000, Jonas Fonseca wrote:
> Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
> ---
>  builtin-branch.c     |    1 -
>  builtin-update-ref.c |    1 -
>  parse-options.c      |    2 +-
>  3 files changed, 1 insertions(+), 3 deletions(-)
> 
>  Pierre Habouzit <madcoder@debian.org> wrote Sat, Oct 13, 2007:
>  > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
>  > ---
>  >  builtin-update-ref.c |   71 +++++++++++++++++++++-----------------------------
>  >  1 files changed, 30 insertions(+), 41 deletions(-)
>  > 
>  > diff --git a/builtin-update-ref.c b/builtin-update-ref.c
>  > index fe1f74c..eafb642 100644
>  > --- a/builtin-update-ref.c
>  > +++ b/builtin-update-ref.c
>  > @@ -1,59 +1,48 @@
>  >  #include "cache.h"
>  >  #include "refs.h"
>  >  #include "builtin.h"
>  > +#include "parse-options.h"
>  >  
>  > -static const char git_update_ref_usage[] =
>  > -"git-update-ref [-m <reason>] (-d <refname> <value> | [--no-deref] <refname> <value> [<oldval>])";
>  > +static const char * const git_update_ref_usage[] = {
>  > +	"",
>  > +	"git-update-ref [options] -d <refname> <oldval>",
>  > +	"git-update-ref [options]    <refname> <newval> [<oldval>]",
>  > +	NULL
>  > +};
> 
>  How about something like this to get rid of these empty strings
>  that look strange?
> 
> 	> ./git update-ref -h
> 	usage: git-update-ref [options] -d <refname> <oldval>
> 	   or: git-update-ref [options]    <refname> <newval> [<oldval>]

  I like the idea, though we may want to have more text to explain some
things about the command, so I'll do something in between that uses or:
until an empty line is met, and just prefix the result with four spaces
else, this way we can have:

usage: git-foo ...
   or: git-foo ...

    Did you know that you can do bar with git-foo ?
    but beware that it cannot do quux.

    -m <reason>           reason of the update
    -d                    deletes the reference
    --no-deref            update <refname> not the one it points to


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

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

Re: [PATCH] Update manpages to reflect new short and long option aliases

[Pierre Habouzit]

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

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

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

[PATCH] parse-options: Allow abbreviated options when unambiguous

[Johannes Schindelin]

When there is an option "--amend", the option parser now recognizes
"--am" for that option, provided that there is no other option beginning
with "--am".

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sun, 14 Oct 2007, Pierre Habouzit wrote:

	> On Sun, Oct 14, 2007 at 09:18:55AM +0000, Eric Wong wrote:
	> > 
	> > One feature I really like is automatically handling of long 
	> > option abbreviations.  gitopt supported this at the expense of 
	> > complexity and the aforementioned intrusivenes.  This allows 
	> > automatic handling of the abbreviation style seen commonly in 
	> > git shell scripts:
	> > 
	> >    --a|--am|--ame|--amen|--amend)  (from git-commit.sh)
	> 
	> Yes, but if you do that, you can't order options in the order 
	> you want (because of first match issues), making the help dumps 
	> hopelessly random.

I think this patch proves that you do not need to order the options...

;-)

 parse-options.c          |   32 ++++++++++++++++++++++++++++++++
 t/t0040-parse-options.sh |   22 ++++++++++++++++++++++
 2 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 72656a8..afc6c89 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -102,6 +102,13 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)
 static int parse_long_opt(struct optparse_t *p, const char *arg,
                           const struct option *options)
 {
+	const char *arg_end = strchr(arg, '=');
+	const struct option *abbrev_option = NULL;
+	int abbrev_flags = 0;
+
+	if (!arg_end)
+		arg_end = arg + strlen(arg);
+
 	for (; options->type != OPTION_END; options++) {
 		const char *rest;
 		int flags = 0;
@@ -111,10 +118,33 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
 
 		rest = skip_prefix(arg, options->long_name);
 		if (!rest) {
+			/* abbreviated? */
+			if (!strncmp(options->long_name, arg, arg_end - arg)) {
+is_abbreviated:
+				if (abbrev_option)
+					die ("Ambiguous option: %s "
+						"(could be --%s%s or --%s%s)",
+						arg,
+						(flags & OPT_UNSET) ?
+							"no-" : "",
+						options->long_name,
+						(abbrev_flags & OPT_UNSET) ?
+							"no-" : "",
+						abbrev_option->long_name);
+				if (!(flags & OPT_UNSET) && *arg_end)
+					p->opt = arg_end + 1;
+				abbrev_option = options;
+				abbrev_flags = flags;
+				continue;
+			}
+			/* negated? */
 			if (strncmp(arg, "no-", 3))
 				continue;
 			flags |= OPT_UNSET;
 			rest = skip_prefix(arg + 3, options->long_name);
+			/* abbreviated and negated? */
+			if (!rest && !prefixcmp(options->long_name, arg + 3))
+				goto is_abbreviated;
 			if (!rest)
 				continue;
 		}
@@ -125,6 +155,8 @@ static int parse_long_opt(struct optparse_t *p, const char *arg,
 		}
 		return get_value(p, options, flags);
 	}
+	if (abbrev_option)
+		return get_value(p, abbrev_option, abbrev_flags);
 	return error("unknown option `%s'", arg);
 }
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 09b3230..e4dd86f 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -66,4 +66,26 @@ test_expect_success 'intermingled arguments' '
 	git diff expect output
 '
 
+cat > expect << EOF
+boolean: 0
+integer: 2
+string: (not set)
+EOF
+
+test_expect_success 'unambiguously abbreviated option' '
+	test-parse-options --int 2 --boolean --no-bo > output 2> output.err &&
+	test ! -s output.err &&
+	git diff expect output
+'
+
+test_expect_success 'unambiguously abbreviated option with "="' '
+	test-parse-options --int=2 > output 2> output.err &&
+	test ! -s output.err &&
+	git diff expect output
+'
+
+test_expect_failure 'ambiguously abbreviated option' '
+	test-parse-options --strin 123
+'
+
 test_done
-- 
1.5.3.4.1174.gcd0d6-dirty

Re: [PATCH] parse-options: Allow abbreviated options when unambiguous

[Johannes Schindelin]

Hi,

On Sun, 14 Oct 2007, Johannes Schindelin wrote:

> When there is an option "--amend", the option parser now recognizes 
> "--am" for that option, provided that there is no other option beginning 
> with "--am".

And an amend for ultra-abbreviated options (as you noticed on IRC):

diff --git a/parse-options.c b/parse-options.c
index afc6c89..acabb98 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -137,6 +137,11 @@ is_abbreviated:
 				abbrev_flags = flags;
 				continue;
 			}
+			/* negated and abbreviated very much? */
+			if (!prefixcmp("no-", arg)) {
+				flags |= OPT_UNSET;
+				goto is_abbreviated;
+			}
 			/* negated? */
 			if (strncmp(arg, "no-", 3))
 				continue;

Re: [PATCH] parse-options: Allow abbreviated options when unambiguous

[Pierre Habouzit]

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

On Sun, Oct 14, 2007 at 06:02:33PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 14 Oct 2007, Johannes Schindelin wrote:
> 
> > When there is an option "--amend", the option parser now recognizes 
> > "--am" for that option, provided that there is no other option beginning 
> > with "--am".
> 
> And an amend for ultra-abbreviated options (as you noticed on IRC):
> 
> diff --git a/parse-options.c b/parse-options.c
> index afc6c89..acabb98 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -137,6 +137,11 @@ is_abbreviated:
>  				abbrev_flags = flags;
>  				continue;
>  			}
> +			/* negated and abbreviated very much? */
> +			if (!prefixcmp("no-", arg)) {
> +				flags |= OPT_UNSET;
> +				goto is_abbreviated;
> +			}
>  			/* negated? */
>  			if (strncmp(arg, "no-", 3))
>  				continue;

  squashed on top on the previous, and pushed to my ph/parseopt branch.

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

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

Re: [PATCH] parse-options: Allow abbreviated options when unambiguous

[Eric Wong]

Pierre Habouzit <madcoder@debian.org> wrote:
> On Sun, Oct 14, 2007 at 06:02:33PM +0000, Johannes Schindelin wrote:
> > Hi,
> > 
> > On Sun, 14 Oct 2007, Johannes Schindelin wrote:
> > 
> > > When there is an option "--amend", the option parser now recognizes 
> > > "--am" for that option, provided that there is no other option beginning 
> > > with "--am".
> > 
> > And an amend for ultra-abbreviated options (as you noticed on IRC):
> > 
> > diff --git a/parse-options.c b/parse-options.c
> > index afc6c89..acabb98 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -137,6 +137,11 @@ is_abbreviated:
> >  				abbrev_flags = flags;
> >  				continue;
> >  			}
> > +			/* negated and abbreviated very much? */
> > +			if (!prefixcmp("no-", arg)) {
> > +				flags |= OPT_UNSET;
> > +				goto is_abbreviated;
> > +			}
> >  			/* negated? */
> >  			if (strncmp(arg, "no-", 3))
> >  				continue;
> 
>   squashed on top on the previous, and pushed to my ph/parseopt branch.

Awesome.  Thanks to both of you.

-- 
Eric Wong

Re: [PATCH] parse-options: Allow abbreviated options when unambiguous

[Johannes Schindelin]

Hi,

On Sun, 14 Oct 2007, Eric Wong wrote:

> Pierre Habouzit <madcoder@debian.org> wrote:
> > On Sun, Oct 14, 2007 at 06:02:33PM +0000, Johannes Schindelin wrote:
> > > Hi,
> > > 
> > > On Sun, 14 Oct 2007, Johannes Schindelin wrote:
> > > 
> > > > When there is an option "--amend", the option parser now recognizes 
> > > > "--am" for that option, provided that there is no other option beginning 
> > > > with "--am".
> > > 
> > > And an amend for ultra-abbreviated options (as you noticed on IRC):
> > > 
> > > diff --git a/parse-options.c b/parse-options.c
> > > index afc6c89..acabb98 100644
> > > --- a/parse-options.c
> > > +++ b/parse-options.c
> > > @@ -137,6 +137,11 @@ is_abbreviated:
> > >  				abbrev_flags = flags;
> > >  				continue;
> > >  			}
> > > +			/* negated and abbreviated very much? */
> > > +			if (!prefixcmp("no-", arg)) {
> > > +				flags |= OPT_UNSET;
> > > +				goto is_abbreviated;
> > > +			}
> > >  			/* negated? */
> > >  			if (strncmp(arg, "no-", 3))
> > >  				continue;
> > 
> >   squashed on top on the previous, and pushed to my ph/parseopt branch.
> 
> Awesome.  Thanks to both of you.

Hehe, you're welcome.  Pierre even realised that my patch was not complete 
(it did not catch overly short abbreviations "--n" and "--no"), and that 
has been fixed, too.

While I have your attention: last weekend, I spoke to a guy from the 
ffmpeg project, and he said that the only thing preventing them from 
switching to git was the lack of svn:external support...

(Of course I know that it is more difficult than that: ffmpeg itself is an 
svn:external of MPlayer, but maybe we can get both of them to switch ;-)

Do you have any idea when/if you're coming around to add that to git-svn?

Ciao,
Dscho

Re: [PATCH] parse-options: Allow abbreviated options when unambiguous

[Eric Wong]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
> 
> On Sun, 14 Oct 2007, Eric Wong wrote:
> 
> > Pierre Habouzit <madcoder@debian.org> wrote:
> > > On Sun, Oct 14, 2007 at 06:02:33PM +0000, Johannes Schindelin wrote:
> > > > Hi,
> > > > 
> > > > On Sun, 14 Oct 2007, Johannes Schindelin wrote:
> > > > 
> > > > > When there is an option "--amend", the option parser now recognizes 
> > > > > "--am" for that option, provided that there is no other option beginning 
> > > > > with "--am".
> > > > 
> > > > And an amend for ultra-abbreviated options (as you noticed on IRC):
> > > > 
> > > > diff --git a/parse-options.c b/parse-options.c
> > > > index afc6c89..acabb98 100644
> > > > --- a/parse-options.c
> > > > +++ b/parse-options.c
> > > > @@ -137,6 +137,11 @@ is_abbreviated:
> > > >  				abbrev_flags = flags;
> > > >  				continue;
> > > >  			}
> > > > +			/* negated and abbreviated very much? */
> > > > +			if (!prefixcmp("no-", arg)) {
> > > > +				flags |= OPT_UNSET;
> > > > +				goto is_abbreviated;
> > > > +			}
> > > >  			/* negated? */
> > > >  			if (strncmp(arg, "no-", 3))
> > > >  				continue;
> > > 
> > >   squashed on top on the previous, and pushed to my ph/parseopt branch.
> > 
> > Awesome.  Thanks to both of you.
> 
> Hehe, you're welcome.  Pierre even realised that my patch was not complete 
> (it did not catch overly short abbreviations "--n" and "--no"), and that 
> has been fixed, too.
 
> While I have your attention: last weekend, I spoke to a guy from the 
> ffmpeg project, and he said that the only thing preventing them from 
> switching to git was the lack of svn:external support...
> 
> (Of course I know that it is more difficult than that: ffmpeg itself is an 
> svn:external of MPlayer, but maybe we can get both of them to switch ;-)
> 
> Do you have any idea when/if you're coming around to add that to git-svn?

Soonish, possibly within a next week, even.  I have actually have
started a project (using git) that wants to use SVN-hosted repositories
directly submodules; so the fact that I'll actually need something like
it bodes well for getting it implemented :)

-- 
Eric Wong

git-svn and submodules, was Re: [PATCH] parse-options: Allow abbreviated options when unambiguous

[Johannes Schindelin]

Hi,

On Sun, 14 Oct 2007, Eric Wong wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> > While I have your attention: last weekend, I spoke to a guy from the 
> > ffmpeg project, and he said that the only thing preventing them from 
> > switching to git was the lack of svn:external support...
> > 
> > (Of course I know that it is more difficult than that: ffmpeg itself 
> > is an svn:external of MPlayer, but maybe we can get both of them to 
> > switch ;-)
> > 
> > Do you have any idea when/if you're coming around to add that to 
> > git-svn?
> 
> Soonish, possibly within a next week, even.  I have actually have 
> started a project (using git) that wants to use SVN-hosted repositories 
> directly submodules; so the fact that I'll actually need something like 
> it bodes well for getting it implemented :)

Hehe.  Thanks!

Ciao,
Dscho

Re: git-svn and submodules

[Benoit SIGOURE]

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

On Oct 15, 2007, at 12:59 AM, Johannes Schindelin wrote:

> Hi,
>
> On Sun, 14 Oct 2007, Eric Wong wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>>
>>> While I have your attention: last weekend, I spoke to a guy from the
>>> ffmpeg project, and he said that the only thing preventing them from
>>> switching to git was the lack of svn:external support...
>>>
>>> (Of course I know that it is more difficult than that: ffmpeg itself
>>> is an svn:external of MPlayer, but maybe we can get both of them to
>>> switch ;-)
>>>
>>> Do you have any idea when/if you're coming around to add that to
>>> git-svn?
>>
>> Soonish, possibly within a next week, even.  I have actually have
>> started a project (using git) that wants to use SVN-hosted  
>> repositories
>> directly submodules; so the fact that I'll actually need something  
>> like
>> it bodes well for getting it implemented :)
>
> Hehe.  Thanks!
>

Thanks for making this another thread because I didn't read the  
answers to that patch and I was going to try and implement this  
(svn:externals via submodules) sooner or later.  Hadn't I seen this,  
we'd probably end up duplicating effort.  Maybe I can help with the  
implementation?

This week I'm probably going to start to dive in git-svn by  
implementing simpler things first:
   - git svn create-ignore (to create one .gitignore per directory  
from the svn:ignore properties.  This has the disadvantage of  
committing the .gitignore during the next dcommit, but when you  
import a repo with tons of ignores (>1000), using git svn show-ignore  
to build .git/info/exclude is *not* a good idea, because things like  
git-status will end up doing >1000 fnmatch *per file* in the repo,  
which leads to git-status taking more than 4s on my Core2Duo 2Ghz 2G  
RAM)
   - git svn propget (to easily retrieve svn properties from withing  
git-svn).  git svn propset would be nice too, but I guess it's harder  
to implement.

Cheers,

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: git-svn and submodules

[Andreas Ericsson]

Benoit SIGOURE wrote:
>   - git svn create-ignore (to create one .gitignore per directory from 
> the svn:ignore properties.  This has the disadvantage of committing the 
> .gitignore during the next dcommit, but when you import a repo with tons 
> of ignores (>1000), using git svn show-ignore to build .git/info/exclude 
> is *not* a good idea, because things like git-status will end up doing 
>  >1000 fnmatch *per file* in the repo, which leads to git-status taking 
> more than 4s on my Core2Duo 2Ghz 2G RAM)

How spoiled we are. I just ran cvs status on a checkout of a repo located
on a server in the local network. It took 6 seconds to complete :P

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

Re: git-svn and submodules

[David Kastrup]

Benoit SIGOURE <tsuna@lrde.epita.fr> writes:

> This week I'm probably going to start to dive in git-svn by
> implementing simpler things first:
>   - git svn create-ignore (to create one .gitignore per directory
> from the svn:ignore properties.  This has the disadvantage of
> committing the .gitignore during the next dcommit, but when you
> import a repo with tons of ignores (>1000), using git svn show-ignore
> to build .git/info/exclude is *not* a good idea, because things like
> git-status will end up doing >1000 fnmatch *per file* in the repo,
> which leads to git-status taking more than 4s on my Core2Duo 2Ghz 2G
> RAM)

Well, then this should be fixed in git general, by sorting the ignores
(wildcards in the first place where they can match), and then just
moving those patterns that can actually match according to sort order
to the list of fnmatch candidates (and moving those files that can't
match anymore die to the sort order out again).

I don't think that the final "solution" for avoiding a lousy global
O(n^2) algorithm is to replace it with lousy local O(n^2) algorithms
and just hope for smaller values of n.

-- 
David Kastrup

Re: git-svn and submodules

[Benoit SIGOURE]

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

On Oct 15, 2007, at 12:00 PM, Andreas Ericsson wrote:

> Benoit SIGOURE wrote:
>>   - git svn create-ignore (to create one .gitignore per directory  
>> from the svn:ignore properties.  This has the disadvantage of  
>> committing the .gitignore during the next dcommit, but when you  
>> import a repo with tons of ignores (>1000), using git svn show- 
>> ignore to build .git/info/exclude is *not* a good idea, because  
>> things like git-status will end up doing  >1000 fnmatch *per file*  
>> in the repo, which leads to git-status taking more than 4s on my  
>> Core2Duo 2Ghz 2G RAM)
>
> How spoiled we are. I just ran cvs status on a checkout of a repo  
> located
> on a server in the local network. It took 6 seconds to complete :P

Hehe, true, once you get used to the taste of Git, you'll never want  
to switch back to these disgusting SCMs.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: git-svn and submodules

[Benoit SIGOURE]

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

On Oct 15, 2007, at 12:14 PM, David Kastrup wrote:

> Benoit SIGOURE <tsuna@lrde.epita.fr> writes:
>
>> This week I'm probably going to start to dive in git-svn by
>> implementing simpler things first:
>>   - git svn create-ignore (to create one .gitignore per directory
>> from the svn:ignore properties.  This has the disadvantage of
>> committing the .gitignore during the next dcommit, but when you
>> import a repo with tons of ignores (>1000), using git svn show-ignore
>> to build .git/info/exclude is *not* a good idea, because things like
>> git-status will end up doing >1000 fnmatch *per file* in the repo,
>> which leads to git-status taking more than 4s on my Core2Duo 2Ghz 2G
>> RAM)
>
> Well, then this should be fixed in git general, by sorting the ignores
> (wildcards in the first place where they can match), and then just
> moving those patterns that can actually match according to sort order
> to the list of fnmatch candidates (and moving those files that can't
> match anymore die to the sort order out again).
>
> I don't think that the final "solution" for avoiding a lousy global
> O(n^2) algorithm is to replace it with lousy local O(n^2) algorithms
> and just hope for smaller values of n.

That's entirely true, it's more of a workaround than a real  
solution.  Anyways, there could be other situations in which someone  
would like to generate the .gitignore instead of using .git/info/ 
exclude, so this feature could be useful anyways.

I can try to address this issue later, if I have enough free time in  
my hands to do so.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: git-svn and submodules

[Karl Hasselström]

On 2007-10-15 09:07:21 +0200, Benoit SIGOURE wrote:

>   - git svn create-ignore (to create one .gitignore per directory
> from the svn:ignore properties. This has the disadvantage of
> committing the .gitignore during the next dcommit,

I built ignore support for git-svnignore a long time ago. It converts
the per-directory svn:ignore to per-directory .gitignore at commit
import time, which is very handy:

-I <ignorefile_name>::
        Import the svn:ignore directory property to files with this
        name in each directory. (The Subversion and GIT ignore
        syntaxes are similar enough that using the Subversion patterns
        directly with "-I .gitignore" will almost always just work.)

The only downside with that is that svn ignore patterns are
non-recursive, while git ignore patterns are recursive. This could be
solved by prefixing them with a "/".

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

.gitignore and svn:ignore [WAS: git-svn and submodules]

[Chris Shoemaker]

On Mon, Oct 15, 2007 at 04:45:13PM +0200, Karl Hasselström wrote:
> On 2007-10-15 09:07:21 +0200, Benoit SIGOURE wrote:
> 
> >   - git svn create-ignore (to create one .gitignore per directory
> > from the svn:ignore properties. This has the disadvantage of
> > committing the .gitignore during the next dcommit,
> 
> I built ignore support for git-svnignore a long time ago. It converts
> the per-directory svn:ignore to per-directory .gitignore at commit
> import time, which is very handy:
> 
> -I <ignorefile_name>::
>         Import the svn:ignore directory property to files with this
>         name in each directory. (The Subversion and GIT ignore
>         syntaxes are similar enough that using the Subversion patterns
>         directly with "-I .gitignore" will almost always just work.)
> 
> The only downside with that is that svn ignore patterns are
> non-recursive, while git ignore patterns are recursive. This could be
> solved by prefixing them with a "/".

Has anyone put any thought into mapping the other direction? 
i.e. .gitignore  ->  svn:ignore

-chris

Re: git-svn and submodules

[Linus Torvalds]

On Mon, 15 Oct 2007, Benoit SIGOURE wrote:
>
>  - git svn create-ignore (to create one .gitignore per directory from the
> svn:ignore properties.  This has the disadvantage of committing the .gitignore
> during the next dcommit, but when you import a repo with tons of ignores
> (>1000), using git svn show-ignore to build .git/info/exclude is *not* a good
> idea, because things like git-status will end up doing >1000 fnmatch *per
> file* in the repo, which leads to git-status taking more than 4s on my
> Core2Duo 2Ghz 2G RAM)

Ouch.

That sounds largely unavoidable.. *But*.

Maybe we have a bug here. In particular, we generally shouldn't care about 
the exclude/.gitignore file for ay paths that we know about, which means 
that during an import, we really shouldn't ever even care about 
.gitignore, since all the files are files we are expected to know about.

So yes, in general, "git status" is going to be slow in a tree that has 
been built (since things like object files etc will have to be checked 
against the exclude list! (*)), but if it's a clean import with no 
generated files and only files we already know about, that should not be 
the case.

So maybe we have a totally unnecessary performance issue, and do all the 
fnmatch() on every path, whether we know about it or not?

		Linus

(*) It might be that we could also re-order the exclude list so that 
entries that trigger are moved to the head of the list, because it's 
likely that if you have tons of exclude entries, some of them trigger a 
lot more than others (ie "*.o"), and trying those first is likely a good 
idea.

Performance issue with excludes (was: Re: git-svn and submodules)

[Benoit SIGOURE]

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

On Oct 15, 2007, at 5:53 PM, Linus Torvalds wrote:

> On Mon, 15 Oct 2007, Benoit SIGOURE wrote:
>>
>>  - git svn create-ignore (to create one .gitignore per directory  
>> from the
>> svn:ignore properties.  This has the disadvantage of committing  
>> the .gitignore
>> during the next dcommit, but when you import a repo with tons of  
>> ignores
>> (>1000), using git svn show-ignore to build .git/info/exclude is  
>> *not* a good
>> idea, because things like git-status will end up doing >1000  
>> fnmatch *per
>> file* in the repo, which leads to git-status taking more than 4s  
>> on my
>> Core2Duo 2Ghz 2G RAM)
>
> Ouch.
>
> That sounds largely unavoidable.. *But*.
>
> Maybe we have a bug here. In particular, we generally shouldn't  
> care about
> the exclude/.gitignore file for ay paths that we know about, which  
> means
> that during an import, we really shouldn't ever even care about
> .gitignore, since all the files are files we are expected to know  
> about.
>
> So yes, in general, "git status" is going to be slow in a tree that  
> has
> been built (since things like object files etc will have to be checked
> against the exclude list! (*)), but if it's a clean import with no
> generated files and only files we already know about, that should  
> not be
> the case.

I re-used the test that was posted some time ago:

------------------------------------------------------------------------ 
---
#
# first create a tree of roughly 100k files
#
mkdir bummer
cd bummer
for ((i=0;i<100;i++)); do
mkdir $i && pushd $i;
for ((j=0;j<1000;j++)); do
echo "$j" >$j; done; popd;
done

#
# init and add this to git
#
time git init
git config user.email "no@thx"
git config user.name "nothx"
time git add .
time git commit -m 'buurrrrn' -a

for ((j=0;j<1000;j++)); do
   echo "/pattern$j" >.git/info/exclude
done

#
# git-status, tunes in at around ~8s for me
#
time git-status
time git-status
time git-status
------------------------------------------------------------------------ 
---

[...]
git commit -m 'buurrrrn' -a  5.62s user 16.84s system 87% cpu 25.634  
total
# On branch master
nothing to commit (working directory clean)
git-status  2.48s user 5.97s system 96% cpu 8.718 total
# On branch master
nothing to commit (working directory clean)
git-status  2.48s user 5.94s system 97% cpu 8.646 total
# On branch master
nothing to commit (working directory clean)
git-status  2.48s user 5.95s system 96% cpu 8.720 total

My machine is a Core2Duo 2Ghz 2G RAM.

>
> So maybe we have a totally unnecessary performance issue, and do  
> all the
> fnmatch() on every path, whether we know about it or not?
>
> 		Linus
>
> (*) It might be that we could also re-order the exclude list so that
> entries that trigger are moved to the head of the list, because it's
> likely that if you have tons of exclude entries, some of them  
> trigger a
> lot more than others (ie "*.o"), and trying those first is likely a  
> good
> idea.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: git-svn and submodules

[Andreas Ericsson]

Benoit SIGOURE wrote:
> On Oct 15, 2007, at 12:14 PM, David Kastrup wrote:
> 
>> Benoit SIGOURE <tsuna@lrde.epita.fr> writes:
>>
>>> This week I'm probably going to start to dive in git-svn by
>>> implementing simpler things first:
>>>   - git svn create-ignore (to create one .gitignore per directory
>>> from the svn:ignore properties.  This has the disadvantage of
>>> committing the .gitignore during the next dcommit, but when you
>>> import a repo with tons of ignores (>1000), using git svn show-ignore
>>> to build .git/info/exclude is *not* a good idea, because things like
>>> git-status will end up doing >1000 fnmatch *per file* in the repo,
>>> which leads to git-status taking more than 4s on my Core2Duo 2Ghz 2G
>>> RAM)
>>
>> Well, then this should be fixed in git general, by sorting the ignores
>> (wildcards in the first place where they can match), and then just
>> moving those patterns that can actually match according to sort order
>> to the list of fnmatch candidates (and moving those files that can't
>> match anymore die to the sort order out again).
>>
>> I don't think that the final "solution" for avoiding a lousy global
>> O(n^2) algorithm is to replace it with lousy local O(n^2) algorithms
>> and just hope for smaller values of n.
> 
> That's entirely true, it's more of a workaround than a real solution.  
> Anyways, there could be other situations in which someone would like to 
> generate the .gitignore instead of using .git/info/exclude, so this 
> feature could be useful anyways.
> 
> I can try to address this issue later, if I have enough free time in my 
> hands to do so.
> 

Ah, finally found the thread. I sent a core.ignorefile patch to the list
(Let users decide the name of the ignore file) a while ago, but didn't
find this mail to respond to. My apologies.

It's one way of solving it, which I'm currently using, although not so
fitting for when you're importing svn repos permanently.

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

Re: Performance issue with excludes (was: Re: git-svn and submodules)

[Linus Torvalds]

On Mon, 15 Oct 2007, Benoit SIGOURE wrote:
> 
> I re-used the test that was posted some time ago:

I think your test is scrogged. You should add the ".gitignore" file 
*before* you do the "git add .". That's when it's going to hurt (since 
that's when you have new files you don't yet know about).

But then it should hurt only for the "git add ." phase, not for anything 
else (unless we have the performance bug of doing the ignore matching even 
on files we know about). And more importantly, it should hurt only once 
(since afterwards, we'll know about the files and know not to ignore 
them).

		Linus

Re: Performance issue with excludes (was: Re: git-svn and submodules)

[Benoit SIGOURE]

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

On Oct 15, 2007, at 6:34 PM, Linus Torvalds wrote:

> On Mon, 15 Oct 2007, Benoit SIGOURE wrote:
>>
>> I re-used the test that was posted some time ago:
>
> I think your test is scrogged. You should add the ".gitignore" file
> *before* you do the "git add .". That's when it's going to hurt (since
> that's when you have new files you don't yet know about).
>
> But then it should hurt only for the "git add ." phase, not for  
> anything
> else (unless we have the performance bug of doing the ignore  
> matching even
> on files we know about). And more importantly, it should hurt only  
> once
> (since afterwards, we'll know about the files and know not to ignore
> them).

There is no .gitignore, only .git/info/exclude.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: Performance issue with excludes (was: Re: git-svn and submodules)

[Linus Torvalds]

On Mon, 15 Oct 2007, Benoit SIGOURE wrote:
> 
> There is no .gitignore, only .git/info/exclude.

They do exactly the same thing (apart from the nesting nature of 
.gitignore wrt subdirectories), so that doesn't change anything.

		Linus

Re: Performance issue with excludes (was: Re: git-svn and submodules)

[Benoit SIGOURE]

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

On Oct 15, 2007, at 7:10 PM, Linus Torvalds wrote:

> On Mon, 15 Oct 2007, Benoit SIGOURE wrote:
>>
>> There is no .gitignore, only .git/info/exclude.
>
> They do exactly the same thing (apart from the nesting nature of
> .gitignore wrt subdirectories), so that doesn't change anything.

I fail to see how the mechanism work then.  You said that I needed to  
add the .gitignore before adding all the other bummer stuff, fair  
enough.  AFAIK .git/info/exclude doesn't need to be added, it's just  
there.  But you can try to change the test, add the .git/info/exclude  
*first* and then make a commit and then add all the bummer stuff and  
then commit, and finally, do a git-status, for me it still takes 9s.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: .gitignore and svn:ignore [WAS: git-svn and submodules]

[Eric Wong]

Chris Shoemaker <c.shoemaker@cox.net> wrote:
> On Mon, Oct 15, 2007 at 04:45:13PM +0200, Karl Hasselström wrote:
> > On 2007-10-15 09:07:21 +0200, Benoit SIGOURE wrote:
> > 
> > >   - git svn create-ignore (to create one .gitignore per directory
> > > from the svn:ignore properties. This has the disadvantage of
> > > committing the .gitignore during the next dcommit,
> > 
> > I built ignore support for git-svnignore a long time ago. It converts
> > the per-directory svn:ignore to per-directory .gitignore at commit
> > import time, which is very handy:
> > 
> > -I <ignorefile_name>::
> >         Import the svn:ignore directory property to files with this
> >         name in each directory. (The Subversion and GIT ignore
> >         syntaxes are similar enough that using the Subversion patterns
> >         directly with "-I .gitignore" will almost always just work.)
> > 
> > The only downside with that is that svn ignore patterns are
> > non-recursive, while git ignore patterns are recursive. This could be
> > solved by prefixing them with a "/".
> 
> Has anyone put any thought into mapping the other direction? 
> i.e. .gitignore  ->  svn:ignore

If we support .gitignore <-> svn:ignore in git-svn; bidirectional,
transparent mapping is the only way I want to go.


This means that *all* .gitignore files will be translated to svn:ignore
files and vice versa; and the .gitignore files will be NOT be committed
to SVN itself, but present in the git-svn created mirrors.  Recursive
.gitignore definitions will be mapped to svn:ignore recursively on the
client side; and non-recursive ones will only map to one directory.

Sound good?

I may be sleepy at the moment, but the thought of implementing this is
sounding complicated now...


One goal of git-svn is that other users shouldn't be able to tell if a
user is using git-svn or plain svn; even.


But back to submodules, I plan on mapping svn:externals <=> .gitmodules
files in a similar fashion.  .gitmodule files will never be seen by SVN
users, period.

That being said, the first step to submodule/externals support in
git-svn will be to allow /any/ git repository to use a submodule that
points to SVN; and then git-submodule will invoke git-svn if it
sees such a submodule.

Yes, I have a plan, sort of...

Since externals/submodules don't operate recursively in either
system like .gitignore; supporting svn:externals <=> submodules
will be much easier and done first[1] :)


[1] - I've personally rarely bothered with putting svn:ignores in the
repository and have been very much spoiled by .git/info/exclude;
whereas externals support I have semi-immediate use for.

-- 
Eric Wong

Re: .gitignore and svn:ignore [WAS: git-svn and submodules]

[Karl Hasselström]

On 2007-10-16 00:58:27 -0700, Eric Wong wrote:

> If we support .gitignore <-> svn:ignore in git-svn; bidirectional,
> transparent mapping is the only way I want to go.

Fair enough.

> This means that *all* .gitignore files will be translated to
> svn:ignore files and vice versa; and the .gitignore files will be
> NOT be committed to SVN itself, but present in the git-svn created
> mirrors.

OK.

> Recursive .gitignore definitions will be mapped to svn:ignore
> recursively on the client side; and non-recursive ones will only map
> to one directory.
>
> Sound good?
>
> I may be sleepy at the moment, but the thought of implementing this
> is sounding complicated now...

I think this is a mistake. If a user adds *.foo to the top-level
.gitignore, this will add *.foo to svn:ignore of _every_ directory in
the whole tree. And coming up with semantics that are sane for e.g.
git -> svn -> git roundtrips seems difficult.

It would be better and far simpler to either

  1. Move the contents of svn:ignore and .gitignore back and forth
     untouched, disregarding the slight semantic mismatch.
     git-svnignore does this (albeit only in one direction), and it
     works surprisingly well in my experience.

  2. Do as in (1), but call the file .svnignore instead of .gitignore.
     And have a git-svn command that translates all the .svnignore
     files in the tree to corresponding .gitignore files.

> One goal of git-svn is that other users shouldn't be able to tell if
> a user is using git-svn or plain svn; even.

Agreed.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: .gitignore and svn:ignore [WAS: git-svn and submodules]

[Chris Shoemaker]

On Tue, Oct 16, 2007 at 12:58:27AM -0700, Eric Wong wrote:
> Chris Shoemaker <c.shoemaker@cox.net> wrote:
> > On Mon, Oct 15, 2007 at 04:45:13PM +0200, Karl Hasselström wrote:
> > > On 2007-10-15 09:07:21 +0200, Benoit SIGOURE wrote:
> > > 
> > > >   - git svn create-ignore (to create one .gitignore per directory
> > > > from the svn:ignore properties. This has the disadvantage of
> > > > committing the .gitignore during the next dcommit,
> > > 
> > > I built ignore support for git-svnignore a long time ago. It converts
> > > the per-directory svn:ignore to per-directory .gitignore at commit
> > > import time, which is very handy:
> > > 
> > > -I <ignorefile_name>::
> > >         Import the svn:ignore directory property to files with this
> > >         name in each directory. (The Subversion and GIT ignore
> > >         syntaxes are similar enough that using the Subversion patterns
> > >         directly with "-I .gitignore" will almost always just work.)
> > > 
> > > The only downside with that is that svn ignore patterns are
> > > non-recursive, while git ignore patterns are recursive. This could be
> > > solved by prefixing them with a "/".
> > 
> > Has anyone put any thought into mapping the other direction? 
> > i.e. .gitignore  ->  svn:ignore
> 
> If we support .gitignore <-> svn:ignore in git-svn; bidirectional,
> transparent mapping is the only way I want to go.
> 
> 
> This means that *all* .gitignore files will be translated to svn:ignore
> files and vice versa; and the .gitignore files will be NOT be committed
> to SVN itself, but present in the git-svn created mirrors.  Recursive
> .gitignore definitions will be mapped to svn:ignore recursively on the
> client side; and non-recursive ones will only map to one directory.
> 
> Sound good?
> 
> I may be sleepy at the moment, but the thought of implementing this is
> sounding complicated now...
> 

OTOH, a general propset solution would probably be good enough that I
wouldn't even miss any transparent .gitignore -> svn:ignore mapping.

I would just accept that I'd have to explicitly specify the
svn:ignores.

> Since externals/submodules don't operate recursively in either
> system like .gitignore; supporting svn:externals <=> submodules
> will be much easier and done first[1] :)
> 
> [1] - I've personally rarely bothered with putting svn:ignores in the
> repository and have been very much spoiled by .git/info/exclude;
> whereas externals support I have semi-immediate use for.

That's great.   I'm eager to see/test the svn:externals support.  Thanks.

-chris

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

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

> +/* Parse the given options against the list of known options.  The
> + * order of the option structs matters, in that ambiguous
> + * abbreviations (eg, --in could be short for --include or
> + * --interactive) are matched by the first option that share the
> + * prefix.

  Do we really want that ?

  I do believe that it's a very bad idea, as it silently breaks. Most of
the command line switches people need to use have a short form, or their
shell will complete it properly.

  A very interesting feature though, would be to finally be able to
parse aggregated switches (`git rm -rf` anyone ?).

  I also believe that it's a pity that parse_options isn't able to
generate the usage by itself. But we can add that later.

  I've though an alternate proposal, based on your work, for the first
patch.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

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

On Wed, Oct 03, 2007 at 09:45:01PM +0000, Kristian Høgsberg wrote:
> +static int parse_one(const char **argv,
> +		     struct option *options, int count,
> +		     const char *usage_string)
> +{
> +	const char *eq, *arg, *value;
> +	int i, processed;

  gcc complains processed could be returned without being initialized
first, so should be processed = 0; Even if it cannot occurs, it avoid
raising eyebrows.

> +	case OPTION_INTEGER:
> +		if (value == NULL) {
> +			error("option %s requires a value.", argv);

                                                             ^^^
                                           should probably be arg.

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

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

Re: [PATCH] Add a simple option parser.

[Johannes Schindelin]

Hi,

On Thu, 4 Oct 2007, Pierre Habouzit wrote:

>   Okay, popt seems to be quite complicated, and depends upon gettext

... which makes me vote against popt ...

> (which we may require as per survey results, but right now it seems a
> useless dependency).

Nope.  git-gui got a script doing the job of msgfmt, which was the only 
part of that beast known as gettext anyway.

So we will not require it for git-gui.

And I do not see core git being i18n'ised.  Ever.

Ciao,
Dscho

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

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

On jeu, oct 04, 2007 at 03:15:32 +0000, Pierre Habouzit wrote:
> On Thu, Oct 04, 2007 at 02:57:58PM +0000, Kristian Høgsberg wrote:
> > I'm not sure - we can go with the current proposal and add new options
> > types and probably the callback option type I suggested as we go.  I
> > don't want to block builtin-commit on figuring out what the perfect
> > option parser should look like and what I sent out earlier work for
> > commit.  I think the way you handled the strbuf rewrites worked pretty
> > well; extending and rewriting the API as you put it to use in more and
> > more places.  We can do the same thing with parse_options().
> 
>   Of course we can do that, or junio said that some people talked about
> popt some time ago. I understand that you don't want to block the
> git-commit work, but doing things right from the beginning is often a
> big win on the long term.
> 
>   I don't know popt, and I don't know if it has sufficient expressivity.
> For sure I don't like getopt_long APIs at all, so if popt is as
> cumbersome, rolling our own based on the current parse_options you
> propose is probably a good choice.

  Okay, popt seems to be quite complicated, and depends upon gettext
(which we may require as per survey results, but right now it seems a
useless dependency). Don't get me wrong, I'm sure it's very powerful,
but again, I believe we can have a 200 line ad-hoc module that fits what
git really needs, the less cumbersome way.

  So well, I'd be (I'm not in position to decide anything btw ;p) in
favor of pursuing the work into git-commit like you did, and ASAP it
gets merged into next, I'm definitely willing to pursue a refactoring to
use it (now that strbufs seems to have been used where needed).

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

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

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

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

On Thu, Oct 04, 2007 at 02:57:58PM +0000, Kristian Høgsberg wrote:
> 
> On Thu, 2007-10-04 at 01:11 +0200, Pierre Habouzit wrote:
> > On Wed, Oct 03, 2007 at 09:45:01PM +0000, Kristian Høgsberg wrote:
> > > The option parser takes argc, argv, an array of struct option
> > > and a usage string.  Each of the struct option elements in the array
> > > describes a valid option, its type and a pointer to the location where the
> > > value is written.  The entry point is parse_options(), which scans through
> > > the given argv, and matches each option there against the list of valid
> > > options.  During the scan, argv is rewritten to only contain the
> > > non-option command line arguments and the number of these is returned.
> > 
> >   if we are going in that direction (and I believe it's a good one), we
> > should be sure that the model fits with other commands as well. And as I
> > said on IRC, I believe the most "horrible" (as in complex) option parser
> > in git is the one from git-grep.
> > 
> >   A migration of git-grep on that API should be tried first. If this
> > works well enough, I believe that the rest of the git commands will be
> > migrated easily enough. (with maybe small addition to parse-option.[hc]
> > but the hardcore things should have been met with git-grep already I
> > think).
> 
> I'm not sure - we can go with the current proposal and add new options
> types and probably the callback option type I suggested as we go.  I
> don't want to block builtin-commit on figuring out what the perfect
> option parser should look like and what I sent out earlier work for
> commit.  I think the way you handled the strbuf rewrites worked pretty
> well; extending and rewriting the API as you put it to use in more and
> more places.  We can do the same thing with parse_options().

  Of course we can do that, or junio said that some people talked about
popt some time ago. I understand that you don't want to block the
git-commit work, but doing things right from the beginning is often a
big win on the long term.

  I don't know popt, and I don't know if it has sufficient expressivity.
For sure I don't like getopt_long APIs at all, so if popt is as
cumbersome, rolling our own based on the current parse_options you
propose is probably a good choice.

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

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

Re: [PATCH] Add a simple option parser.

[Kristian Høgsberg]

On Thu, 2007-10-04 at 01:11 +0200, Pierre Habouzit wrote:
> On Wed, Oct 03, 2007 at 09:45:01PM +0000, Kristian Høgsberg wrote:
> > The option parser takes argc, argv, an array of struct option
> > and a usage string.  Each of the struct option elements in the array
> > describes a valid option, its type and a pointer to the location where the
> > value is written.  The entry point is parse_options(), which scans through
> > the given argv, and matches each option there against the list of valid
> > options.  During the scan, argv is rewritten to only contain the
> > non-option command line arguments and the number of these is returned.
> 
>   if we are going in that direction (and I believe it's a good one), we
> should be sure that the model fits with other commands as well. And as I
> said on IRC, I believe the most "horrible" (as in complex) option parser
> in git is the one from git-grep.
> 
>   A migration of git-grep on that API should be tried first. If this
> works well enough, I believe that the rest of the git commands will be
> migrated easily enough. (with maybe small addition to parse-option.[hc]
> but the hardcore things should have been met with git-grep already I
> think).

I'm not sure - we can go with the current proposal and add new options
types and probably the callback option type I suggested as we go.  I
don't want to block builtin-commit on figuring out what the perfect
option parser should look like and what I sent out earlier work for
commit.  I think the way you handled the strbuf rewrites worked pretty
well; extending and rewriting the API as you put it to use in more and
more places.  We can do the same thing with parse_options().

cheers,
Kristian

Re: [PATCH] Add a simple option parser.

[Pierre Habouzit]

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

On Wed, Oct 03, 2007 at 09:45:01PM +0000, Kristian Høgsberg wrote:
> The option parser takes argc, argv, an array of struct option
> and a usage string.  Each of the struct option elements in the array
> describes a valid option, its type and a pointer to the location where the
> value is written.  The entry point is parse_options(), which scans through
> the given argv, and matches each option there against the list of valid
> options.  During the scan, argv is rewritten to only contain the
> non-option command line arguments and the number of these is returned.

  if we are going in that direction (and I believe it's a good one), we
should be sure that the model fits with other commands as well. And as I
said on IRC, I believe the most "horrible" (as in complex) option parser
in git is the one from git-grep.

  A migration of git-grep on that API should be tried first. If this
works well enough, I believe that the rest of the git commands will be
migrated easily enough. (with maybe small addition to parse-option.[hc]
but the hardcore things should have been met with git-grep already I
think).

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

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

[PATCH] Add a simple option parser.

[Kristian Høgsberg]

The option parser takes argc, argv, an array of struct option
and a usage string.  Each of the struct option elements in the array
describes a valid option, its type and a pointer to the location where the
value is written.  The entry point is parse_options(), which scans through
the given argv, and matches each option there against the list of valid
options.  During the scan, argv is rewritten to only contain the
non-option command line arguments and the number of these is returned.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 Makefile        |    2 +-
 parse-options.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 parse-options.h |   33 +++++++++++++++++
 3 files changed, 140 insertions(+), 1 deletions(-)
 create mode 100644 parse-options.c
 create mode 100644 parse-options.h

diff --git a/Makefile b/Makefile
index 62bdac6..d90e959 100644
--- a/Makefile
+++ b/Makefile
@@ -310,7 +310,7 @@ LIB_OBJS = \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
 	color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
 	convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
-	transport.o bundle.o
+	transport.o bundle.o parse-options.o
 
 BUILTIN_OBJS = \
 	builtin-add.o \
diff --git a/parse-options.c b/parse-options.c
new file mode 100644
index 0000000..130b609
--- /dev/null
+++ b/parse-options.c
@@ -0,0 +1,106 @@
+#include "git-compat-util.h"
+#include "parse-options.h"
+
+static int parse_one(const char **argv,
+		     struct option *options, int count,
+		     const char *usage_string)
+{
+	const char *eq, *arg, *value;
+	int i, processed;
+
+	arg = argv[0];
+	value = NULL;
+
+	if (arg[0] != '-')
+		return 0;
+
+	for (i = 0; i < count; i++) {
+		if (arg[1] == '-') {
+			if (!prefixcmp(options[i].long_name, arg + 2)) {
+				if (options[i].type != OPTION_BOOLEAN) {
+					value = argv[1];
+					processed = 2;
+				} else {
+					processed = 1;
+				}
+				break;
+			}
+
+			eq = strchr(arg + 2, '=');
+			if (eq && options[i].type != OPTION_BOOLEAN &&
+			    !strncmp(arg + 2,
+				     options[i].long_name, eq - arg - 2)) {
+				value = eq + 1;
+				processed = 1;
+				break;
+			}
+		}
+
+		if (arg[1] == options[i].short_name) {
+			if (arg[2] == '\0') {
+				if (options[i].type != OPTION_BOOLEAN) {
+					value = argv[1];
+					processed = 2;
+				} else {
+					processed = 1;
+				}
+				break;
+			}
+
+			if (options[i].type != OPTION_BOOLEAN) {
+				value = arg + 2;
+				processed = 1;
+				break;
+			}
+		}
+	}
+
+	if (i == count)
+		usage(usage_string);
+	else switch (options[i].type) {
+	case OPTION_BOOLEAN:
+		(*(int *)options[i].value)++;
+		break;
+	case OPTION_STRING:
+		if (value == NULL) {
+			error("option %s requires a value.", arg);
+			usage(usage_string);
+		}
+		*(const char **)options[i].value = value;
+		break;
+	case OPTION_INTEGER:
+		if (value == NULL) {
+			error("option %s requires a value.", argv);
+			usage(usage_string);
+		}
+		*(int *)options[i].value = atoi(value);
+		break;
+	default:
+		assert(0);
+	}
+
+	return processed;
+}
+
+int parse_options(int argc, const char **argv,
+		  struct option *options, int count,
+		  const char *usage_string)
+{
+	int i, j, processed;
+
+	for (i = 1, j = 0; i < argc; ) {
+		if (!strcmp(argv[i], "--"))
+			break;
+		processed = parse_one(argv + i, options, count, usage_string);
+		if (processed == 0)
+			argv[j++] = argv[i++];
+		else
+			i += processed;
+	}
+
+	while (i < argc)
+		argv[j++] = argv[i++];
+	argv[j] = NULL;
+
+	return j;
+}
diff --git a/parse-options.h b/parse-options.h
new file mode 100644
index 0000000..5be9c20
--- /dev/null
+++ b/parse-options.h
@@ -0,0 +1,33 @@
+#ifndef PARSE_OPTIONS_H
+#define PARSE_OPTIONS_H
+
+enum option_type {
+	OPTION_BOOLEAN,
+	OPTION_STRING,
+	OPTION_INTEGER,
+	OPTION_LAST,
+};
+
+struct option {
+	enum option_type type;
+	const char *long_name;
+	char short_name;
+	void *value;
+};
+
+/* Parse the given options against the list of known options.  The
+ * order of the option structs matters, in that ambiguous
+ * abbreviations (eg, --in could be short for --include or
+ * --interactive) are matched by the first option that share the
+ * prefix.
+ *
+ * parse_options() will filter out the processed options and leave the
+ * non-option argments in argv[].  The return value is the number of
+ * arguments left in argv[].
+ */
+
+extern int parse_options(int argc, const char **argv,
+			 struct option *options, int count,
+			 const char *usage_string);
+
+#endif
-- 
1.5.2.5