CLI option parsing and usage generation for porcelains

Signed-off-by Pierre Habouzit <[email protected]>

---
 builtin-update-ref.c |   71 +++++++++++++++++++++-----------------------------
 1 files changed, 30 insertions(+), 41 deletions(-)

builtin-update-ref.c +0 -0

index fe1f74c..eafb642 100644
@@ -1,59 +1,48 @@
 #include "cache.h"
 #include "refs.h"
 #include "builtin.h"

Signed-off-by Jonas Fonseca <[email protected]>

---
 builtin-branch.c     |    1 -
 builtin-update-ref.c |    1 -
 parse-options.c      |    2 +-
 3 files changed, 1 insertions(+), 3 deletions(-)

Pierre Habouzit <[email protected]> wrote Sat, Oct 13, 2007:

Quoted 13 lines

Signed-off-by Pierre Habouzit <[email protected]>

---
 builtin-update-ref.c |   71 +++++++++++++++++++++-----------------------------
 1 files changed, 30 insertions(+), 41 deletions(-)

builtin-update-ref.c +0 -0

index fe1f74c..eafb642 100644
@@ -1,59 +1,48 @@
 #include "cache.h"
 #include "refs.h"
 #include "builtin.h"

builtin-update-ref.c line 11

Quoted 10 lines

builtin-update-ref.c +7 -2

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

Hi,

Review

Quoted 8 lines · Pierre Habouzit

(quoted diff) +8 -0

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

Quoted 3 lines

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.

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.

Pierre Habouzit, Sat, Oct 13, 2007 17:03:06 +0200:

Quoted 7 lines · Johannes Schindelin

Quoted 3 lines

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

Quoted 3 lines

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.

+static int opterror(struct option *opt, const char *reason, int flags)

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

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

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

(quoted diff) +2 -0

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

Pierre Habouzit, Sat, Oct 13, 2007 15:29:03 +0200:

Quoted 1 line

[...]

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

Review

Quoted 9 lines

(quoted diff) +8 -0

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

Quoted 2 lines

(quoted diff) +2 -0

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

....

Review

Quoted 1 line

(quoted diff) +1 -0

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

Quoted 6 lines

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.

FWIW you can rework the .c like this:

Pierre Habouzit, Sat, Oct 13, 2007 22:54:04 +0200:

Quoted 8 lines · Alex Riesen

Quoted 6 lines

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

Quoted 2 lines

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.

Quoted 1 line

FWIW you can rework the .c like this:

on top of yours:

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

parse-options.c new file +13 -0

new file mode 100644
index 0000000..07abb50
@@ -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;
+};
+

parse-options.c new file +31 -0

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

parse-options.c new file +7 -0

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

Hi,

Quoted 2 lines · Pierre Habouzit

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?

Quoted 5 lines

(quoted diff) +5 -0

+static inline const char *get_arg(struct optparse_t *p)
+{
+	if (p->opt) {
+		const char *res = p->opt;
+		p->opt = NULL;

Review

Quoted 5 lines

(quoted diff) +5 -0

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

Quoted 1 line

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

Quoted 7 lines

(quoted diff) +7 -0

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

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Review

Quoted 5 lines

(quoted diff) +5 -0

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

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

Review

Quoted 3 lines

(quoted diff) +3 -0

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

Review

Quoted 5 lines

(quoted diff) +5 -0

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

Review

Quoted 3 lines

(quoted diff) +3 -0

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

Review

Quoted 8 lines

(quoted diff) +7 -0

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

Review

Quoted 7 lines

(quoted diff) +7 -0

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

parse-options.h line 5

Quoted 12 lines

parse-options.h new file +5 -0

new file mode 100644
index 0000000..4b33d17
@@ -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.

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.

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 !

Quoted 24 lines · 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

Quoted 6 lines

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)

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

Hi,

Quoted 3 lines · 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".

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

parse-options.c +0 -0

index afc6c89..acabb98 100644
@@ -137,6 +137,11 @@ is_abbreviated:
 				abbrev_flags = flags;

Quoted 20 lines · Johannes Schindelin

Hi,

Quoted 3 lines · 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".

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

parse-options.c +2 -0

index afc6c89..acabb98 100644
@@ -137,6 +137,11 @@ is_abbreviated:
 				abbrev_flags = flags;
 				continue;
 			}
+			/* negated and abbreviated very much? */
+			if (!prefixcmp("no-", arg)) {

parse-options.c is_abbreviated: · line 147

Quoted 6 lines

parse-options.c +3 -0

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

Quoted 21 lines · Pierre Habouzit

Quoted 20 lines · Johannes Schindelin

Hi,

Quoted 3 lines · 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".

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

parse-options.c +2 -0

index afc6c89..acabb98 100644
@@ -137,6 +137,11 @@ is_abbreviated:
 				abbrev_flags = flags;
 				continue;
 			}
+			/* negated and abbreviated very much? */
+			if (!prefixcmp("no-", arg)) {

Quoted 8 lines

parse-options.c is_abbreviated: · line 147

Quoted 6 lines

parse-options.c +3 -0

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

Hi,

Quoted 31 lines · Eric Wong

Quoted 21 lines · Pierre Habouzit

Quoted 20 lines · Johannes Schindelin

Hi,

Quoted 3 lines · 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".

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

parse-options.c +2 -0

index afc6c89..acabb98 100644
@@ -137,6 +137,11 @@ is_abbreviated:
 				abbrev_flags = flags;
 				continue;
 			}
+			/* negated and abbreviated very much? */
+			if (!prefixcmp("no-", arg)) {

Quoted 8 lines

parse-options.c is_abbreviated: · line 147

Quoted 6 lines

parse-options.c +3 -0

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

Quoted 1 line

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?

Quoted 8 lines · Johannes Schindelin

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

Quoted 2 lines

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

Hi,

Quoted 13 lines · Eric Wong

Quoted 10 lines · Johannes Schindelin

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?

Quoted 4 lines

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!

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

Quoted 3 lines · Benoit SIGOURE

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

Quoted 16 lines · Karl Hasselström

Quoted 3 lines · Benoit SIGOURE

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

Quoted 3 lines

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.

Quoted 17 lines · Chris Shoemaker

Quoted 16 lines · Karl Hasselström

Quoted 3 lines · Benoit SIGOURE

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

Quoted 6 lines

Quoted 3 lines

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

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.

- 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

Quoted 1 line

1000 fnmatch *per file* in the repo, which leads to git-status taking

more than 4s on my Core2Duo 2Ghz 2G RAM)

Quoted 2 lines · Benoit SIGOURE

- 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

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)

Quoted 10 lines · Benoit SIGOURE

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.

Quoted 19 lines · David Kastrup

Quoted 10 lines · Benoit SIGOURE

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

Quoted 3 lines

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.

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

Quoted 9 lines · Benoit SIGOURE

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

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

Quoted 2 lines · Benoit SIGOURE

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.

Quoted 2 lines · Benoit SIGOURE

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.