# [RFC] CLI option parsing and usage generation for porcelains 42 messages from 2007-10-13 to 2007-10-16. Participants: Pierre Habouzit, Johannes Schindelin, Wincent Colaiuta, Alex Riesen, Eric Wong, Jonas Fonseca, Benoit SIGOURE, Andreas Ericsson, David Kastrup, Karl Hasselström, Chris Shoemaker, Linus Torvalds. Thread: https://gitlist.dev/t/10264 ## Pierre Habouzit, 2007-10-13 13:29 Subject: [RFC] CLI option parsing and usage generation for porcelains Message-ID: <1192282153-26684-1-git-send-email-madcoder@debian.org> URL: https://gitlist.dev/e/1192282153-26684-1-git-send-email-madcoder%40debian.org ``` 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] [ ...] -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 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 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' ``` ## Johannes Schindelin, 2007-10-13 14:39 Subject: Re: [PATCH] Add a simple option parser. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710131519510.25221%40racer.site In-Reply-To: <1192282153-26684-2-git-send-email-madcoder@debian.org> ``` 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 ``` ## Johannes Schindelin, 2007-10-13 14:47 Subject: Re: [PATCH] Port builtin-add.c to use the new option parser. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710131544030.25221%40racer.site In-Reply-To: <1192282153-26684-3-git-send-email-madcoder@debian.org> ``` 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 ``` ## Wincent Colaiuta, 2007-10-13 14:53 Subject: Re: [RFC] CLI option parsing and usage generation for porcelains Message-ID: <14AB1C41-E8D6-41E7-AE09-A85589A3FB92@wincent.com> URL: https://gitlist.dev/e/14AB1C41-E8D6-41E7-AE09-A85589A3FB92%40wincent.com In-Reply-To: <1192282153-26684-1-git-send-email-madcoder@debian.org> ``` 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 ``` ## Pierre Habouzit, 2007-10-13 14:58 Subject: Re: [PATCH] Add a simple option parser. Message-ID: <20071013145809.GG7110@artemis.corp> URL: https://gitlist.dev/e/20071013145809.GG7110%40artemis.corp In-Reply-To: ``` 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 ``` ## Pierre Habouzit, 2007-10-13 15:03 Subject: Re: [PATCH] Port builtin-add.c to use the new option parser. Message-ID: <20071013150306.GH7110@artemis.corp> URL: https://gitlist.dev/e/20071013150306.GH7110%40artemis.corp In-Reply-To: ``` 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 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 ``` ## Alex Riesen, 2007-10-13 19:16 Subject: Re: [PATCH] Add a simple option parser. Message-ID: <20071013191655.GA2875@steel.home> URL: https://gitlist.dev/e/20071013191655.GA2875%40steel.home In-Reply-To: <1192282153-26684-2-git-send-email-madcoder@debian.org> ``` 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 :) ``` ## Alex Riesen, 2007-10-13 19:22 Subject: Re: [PATCH] Port builtin-add.c to use the new option parser. Message-ID: <20071013192213.GB2875@steel.home> URL: https://gitlist.dev/e/20071013192213.GB2875%40steel.home In-Reply-To: <20071013150306.GH7110@artemis.corp> ``` 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. ``` ## Pierre Habouzit, 2007-10-13 20:27 Subject: Re: [PATCH] Port builtin-add.c to use the new option parser. Message-ID: <20071013202706.GJ7110@artemis.corp> URL: https://gitlist.dev/e/20071013202706.GJ7110%40artemis.corp In-Reply-To: <20071013192213.GB2875@steel.home> ``` 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 ``` ## Pierre Habouzit, 2007-10-13 20:54 Subject: Re: [PATCH] Add a simple option parser. Message-ID: <20071013205404.GK7110@artemis.corp> URL: https://gitlist.dev/e/20071013205404.GK7110%40artemis.corp In-Reply-To: <20071013191655.GA2875@steel.home> ``` 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 ``` ## Alex Riesen, 2007-10-13 22:14 Subject: Re: [PATCH] Add a simple option parser. Message-ID: <20071013221450.GC2875@steel.home> URL: https://gitlist.dev/e/20071013221450.GC2875%40steel.home In-Reply-To: <20071013205404.GK7110@artemis.corp> ``` 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 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 --- 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, " "); + fputs(" ", 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 ``` ## Pierre Habouzit, 2007-10-14 07:02 Subject: Re: [PATCH] Add a simple option parser. Message-ID: <20071014070219.GA1198@artemis.corp> URL: https://gitlist.dev/e/20071014070219.GA1198%40artemis.corp In-Reply-To: <20071013221450.GC2875@steel.home> ``` 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 ``` ## Eric Wong, 2007-10-14 09:18 Subject: Re: [RFC] CLI option parsing and usage generation for porcelains Message-ID: <20071014091855.GA17397@soma> URL: https://gitlist.dev/e/20071014091855.GA17397%40soma In-Reply-To: <1192282153-26684-1-git-send-email-madcoder@debian.org> ``` Pierre Habouzit 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] [ ...] > > -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 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 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 ``` ## Pierre Habouzit, 2007-10-14 09:57 Subject: Re: [RFC] CLI option parsing and usage generation for porcelains Message-ID: <20071014095755.GF1198@artemis.corp> URL: https://gitlist.dev/e/20071014095755.GF1198%40artemis.corp In-Reply-To: <20071014091855.GA17397@soma> ``` On Sun, Oct 14, 2007 at 09:18:55AM +0000, Eric Wong wrote: > Pierre Habouzit 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] [ ...] > > > > -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 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 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 ``` ## Jonas Fonseca, 2007-10-14 14:01 Subject: [PATCH] Simplify usage string printing Message-ID: <20071014140116.GA20970@diku.dk> URL: https://gitlist.dev/e/20071014140116.GA20970%40diku.dk In-Reply-To: <1192282153-26684-10-git-send-email-madcoder@debian.org> ``` Signed-off-by: Jonas Fonseca --- builtin-branch.c | 1 - builtin-update-ref.c | 1 - parse-options.c | 2 +- 3 files changed, 1 insertions(+), 3 deletions(-) Pierre Habouzit wrote Sat, Oct 13, 2007: > Signed-off-by: Pierre Habouzit > --- > 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 ] (-d | [--no-deref] [])"; > +static const char * const git_update_ref_usage[] = { > + "", > + "git-update-ref [options] -d ", > + "git-update-ref [options] []", > + 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 or: git-update-ref [options] [] -m reason of the update -d deletes the reference --no-deref update 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] []", "git-branch [options] [-r] (-d | -D) ", 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 ", "git-update-ref [options] []", 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 ``` ## Jonas Fonseca, 2007-10-14 14:10 Subject: [PATCH] Update manpages to reflect new short and long option aliases Message-ID: <20071014141042.GA21197@diku.dk> URL: https://gitlist.dev/e/20071014141042.GA21197%40diku.dk In-Reply-To: <1192282153-26684-2-git-send-email-madcoder@debian.org> ``` Signed-off-by: Jonas Fonseca --- 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=:: 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 is not a symbolic ref but a detached HEAD; instead exit with non-zero status silently. -- 1.5.3.4.1166.gf076 -- Jonas Fonseca ``` ## Pierre Habouzit, 2007-10-14 16:26 Subject: Re: [PATCH] Simplify usage string printing Message-ID: <20071014162628.GG1198@artemis.corp> URL: https://gitlist.dev/e/20071014162628.GG1198%40artemis.corp In-Reply-To: <20071014140116.GA20970@diku.dk> ``` On Sun, Oct 14, 2007 at 02:01:16PM +0000, Jonas Fonseca wrote: > Signed-off-by: Jonas Fonseca > --- > builtin-branch.c | 1 - > builtin-update-ref.c | 1 - > parse-options.c | 2 +- > 3 files changed, 1 insertions(+), 3 deletions(-) > > Pierre Habouzit wrote Sat, Oct 13, 2007: > > Signed-off-by: Pierre Habouzit > > --- > > 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 ] (-d | [--no-deref] [])"; > > +static const char * const git_update_ref_usage[] = { > > + "", > > + "git-update-ref [options] -d ", > > + "git-update-ref [options] []", > > + 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 > or: git-update-ref [options] [] 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 of the update -d deletes the reference --no-deref update not the one it points to -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Pierre Habouzit, 2007-10-14 16:26 Subject: Re: [PATCH] Update manpages to reflect new short and long option aliases Message-ID: <20071014162653.GH1198@artemis.corp> URL: https://gitlist.dev/e/20071014162653.GH1198%40artemis.corp In-Reply-To: <20071014141042.GA21197@diku.dk> ``` added and pushed. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Johannes Schindelin, 2007-10-14 16:54 Subject: [PATCH] parse-options: Allow abbreviated options when unambiguous Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710141751530.25221%40racer.site In-Reply-To: <20071014095755.GF1198@artemis.corp> ``` 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 --- 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 ``` ## Johannes Schindelin, 2007-10-14 18:02 Subject: Re: [PATCH] parse-options: Allow abbreviated options when unambiguous Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710141901450.25221%40racer.site In-Reply-To: ``` 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; ``` ## Pierre Habouzit, 2007-10-14 18:08 Subject: Re: [PATCH] parse-options: Allow abbreviated options when unambiguous Message-ID: <20071014180815.GK1198@artemis.corp> URL: https://gitlist.dev/e/20071014180815.GK1198%40artemis.corp In-Reply-To: ``` 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 ``` ## Eric Wong, 2007-10-14 21:01 Subject: Re: [PATCH] parse-options: Allow abbreviated options when unambiguous Message-ID: <20071014210130.GA17675@soma> URL: https://gitlist.dev/e/20071014210130.GA17675%40soma In-Reply-To: <20071014180815.GK1198@artemis.corp> ``` Pierre Habouzit 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 ``` ## Johannes Schindelin, 2007-10-14 22:12 Subject: Re: [PATCH] parse-options: Allow abbreviated options when unambiguous Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710142309010.25221%40racer.site In-Reply-To: <20071014210130.GA17675@soma> ``` Hi, On Sun, 14 Oct 2007, Eric Wong wrote: > Pierre Habouzit 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 ``` ## Eric Wong, 2007-10-14 22:49 Subject: Re: [PATCH] parse-options: Allow abbreviated options when unambiguous Message-ID: <20071014224959.GA17828@untitled> URL: https://gitlist.dev/e/20071014224959.GA17828%40untitled In-Reply-To: ``` Johannes Schindelin wrote: > Hi, > > On Sun, 14 Oct 2007, Eric Wong wrote: > > > Pierre Habouzit 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 ``` ## Johannes Schindelin, 2007-10-14 22:59 Subject: git-svn and submodules, was Re: [PATCH] parse-options: Allow abbreviated options when unambiguous Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710142359020.25221%40racer.site In-Reply-To: <20071014224959.GA17828@untitled> ``` Hi, On Sun, 14 Oct 2007, Eric Wong wrote: > Johannes Schindelin 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 ``` ## Benoit SIGOURE, 2007-10-15 07:07 Subject: Re: git-svn and submodules Message-ID: <05CAB148-56ED-4FF1-8AAB-4BA2A0B70C2C@lrde.epita.fr> URL: https://gitlist.dev/e/05CAB148-56ED-4FF1-8AAB-4BA2A0B70C2C%40lrde.epita.fr In-Reply-To: ``` On Oct 15, 2007, at 12:59 AM, Johannes Schindelin wrote: > Hi, > > On Sun, 14 Oct 2007, Eric Wong wrote: > >> Johannes Schindelin 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 ``` ## Andreas Ericsson, 2007-10-15 10:00 Subject: Re: git-svn and submodules Message-ID: <47133A40.20303@op5.se> URL: https://gitlist.dev/e/47133A40.20303%40op5.se In-Reply-To: <05CAB148-56ED-4FF1-8AAB-4BA2A0B70C2C@lrde.epita.fr> ``` 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 ``` ## David Kastrup, 2007-10-15 10:14 Subject: Re: git-svn and submodules Message-ID: <86lka4ofb8.fsf@lola.quinscape.zz> URL: https://gitlist.dev/e/86lka4ofb8.fsf%40lola.quinscape.zz In-Reply-To: <05CAB148-56ED-4FF1-8AAB-4BA2A0B70C2C@lrde.epita.fr> ``` Benoit SIGOURE 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 ``` ## Benoit SIGOURE, 2007-10-15 10:51 Subject: Re: git-svn and submodules Message-ID: <83CC7B21-DF40-4725-9550-A09AAFF88673@lrde.epita.fr> URL: https://gitlist.dev/e/83CC7B21-DF40-4725-9550-A09AAFF88673%40lrde.epita.fr In-Reply-To: <47133A40.20303@op5.se> ``` 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 ``` ## Benoit SIGOURE, 2007-10-15 10:53 Subject: Re: git-svn and submodules Message-ID: URL: https://gitlist.dev/e/AA453A15-BBF1-4EA6-B1AC-1C4E00E89FB2%40lrde.epita.fr In-Reply-To: <86lka4ofb8.fsf@lola.quinscape.zz> ``` On Oct 15, 2007, at 12:14 PM, David Kastrup wrote: > Benoit SIGOURE 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 ``` ## Karl Hasselström, 2007-10-15 14:45 Subject: Re: git-svn and submodules Message-ID: <20071015144513.GB7351@diana.vm.bytemark.co.uk> URL: https://gitlist.dev/e/20071015144513.GB7351%40diana.vm.bytemark.co.uk In-Reply-To: <05CAB148-56ED-4FF1-8AAB-4BA2A0B70C2C@lrde.epita.fr> ``` 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 :: 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 ``` ## Chris Shoemaker, 2007-10-15 15:14 Subject: .gitignore and svn:ignore [WAS: git-svn and submodules] Message-ID: <20071015151405.GA1655@pe.Belkin> URL: https://gitlist.dev/e/20071015151405.GA1655%40pe.Belkin In-Reply-To: <20071015144513.GB7351@diana.vm.bytemark.co.uk> ``` 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 :: > 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 ``` ## Linus Torvalds, 2007-10-15 15:53 Subject: Re: git-svn and submodules Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.999.0710150848380.6887%40woody.linux-foundation.org In-Reply-To: <05CAB148-56ED-4FF1-8AAB-4BA2A0B70C2C@lrde.epita.fr> ``` 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. ``` ## Benoit SIGOURE, 2007-10-15 16:17 Subject: Performance issue with excludes (was: Re: git-svn and submodules) Message-ID: URL: https://gitlist.dev/e/C7EA8AD7-BACA-4116-9C6B-90BA23F0005C%40lrde.epita.fr In-Reply-To: ``` 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 ``` ## Andreas Ericsson, 2007-10-15 16:27 Subject: Re: git-svn and submodules Message-ID: <471394D5.3070509@op5.se> URL: https://gitlist.dev/e/471394D5.3070509%40op5.se In-Reply-To: ``` Benoit SIGOURE wrote: > On Oct 15, 2007, at 12:14 PM, David Kastrup wrote: > >> Benoit SIGOURE 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 ``` ## Linus Torvalds, 2007-10-15 16:34 Subject: Re: Performance issue with excludes (was: Re: git-svn and submodules) Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.999.0710150928450.6887%40woody.linux-foundation.org In-Reply-To: ``` 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 ``` ## Benoit SIGOURE, 2007-10-15 16:51 Subject: Re: Performance issue with excludes (was: Re: git-svn and submodules) Message-ID: <45410184-8D7D-47ED-AB10-1A4E52D0ADB0@lrde.epita.fr> URL: https://gitlist.dev/e/45410184-8D7D-47ED-AB10-1A4E52D0ADB0%40lrde.epita.fr In-Reply-To: ``` 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 ``` ## Linus Torvalds, 2007-10-15 17:10 Subject: Re: Performance issue with excludes (was: Re: git-svn and submodules) Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.999.0710151009460.6887%40woody.linux-foundation.org In-Reply-To: <45410184-8D7D-47ED-AB10-1A4E52D0ADB0@lrde.epita.fr> ``` 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 ``` ## Benoit SIGOURE, 2007-10-15 17:38 Subject: Re: Performance issue with excludes (was: Re: git-svn and submodules) Message-ID: URL: https://gitlist.dev/e/DF6FA0BD-C227-4F62-82D8-F4873CC52B5A%40lrde.epita.fr In-Reply-To: ``` 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 ``` ## Eric Wong, 2007-10-16 07:58 Subject: Re: .gitignore and svn:ignore [WAS: git-svn and submodules] Message-ID: <20071016075827.GB32348@soma> URL: https://gitlist.dev/e/20071016075827.GB32348%40soma In-Reply-To: <20071015151405.GA1655@pe.Belkin> ``` Chris Shoemaker 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 :: > > 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 ``` ## Karl Hasselström, 2007-10-16 09:43 Subject: Re: .gitignore and svn:ignore [WAS: git-svn and submodules] Message-ID: <20071016094330.GA5945@diana.vm.bytemark.co.uk> URL: https://gitlist.dev/e/20071016094330.GA5945%40diana.vm.bytemark.co.uk In-Reply-To: <20071016075827.GB32348@soma> ``` 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 ``` ## Chris Shoemaker, 2007-10-16 13:05 Subject: Re: .gitignore and svn:ignore [WAS: git-svn and submodules] Message-ID: <20071016130526.GA14263@pe.Belkin> URL: https://gitlist.dev/e/20071016130526.GA14263%40pe.Belkin In-Reply-To: <20071016075827.GB32348@soma> ``` On Tue, Oct 16, 2007 at 12:58:27AM -0700, Eric Wong wrote: > Chris Shoemaker 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 :: > > > 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 ```