* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Jeff King @ 2008-06-23 19:53 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806231137070.2926@woody.linux-foundation.org>
On Mon, Jun 23, 2008 at 11:47:49AM -0700, Linus Torvalds wrote:
> > $ git blame --default HEAD git.c
> > fatal: cannot stat path HEAD: No such file or directory
> >
> > Oops.
>
> Oops. And then, how would you fix this most easily?
>
> Be honest now.
Your statement is obviously loaded. I haven't seen _anything_ that fixes
it except what I have already mentioned. But I'm sure you are going to
complain that it isn't easy enough.
> Example: many arguments cause multiple option variables to change.
> parse_options() simply can't handle that well - you can do it with a
> callback, but then you need to make the option variables global or make
> them a structure or something. All of which just makes it nasty to do
> partial conversions for the simple cases.
I'm not so sure. I assumed that most of the callbacks would simply take
a "struct rev_list". So you would end up in builtin-blame.c with:
...
OPT__REVISION(&my_rev_list),
...
in your options table. And if setup_revisions takes options that affect
things that _aren't_ in that struct, then they probably ought to be.
> And I guarantee that just adding PARSE_OPT_{CONTINUE|STOP}_ON_UNKNOWN is
> going to be the smallest patch, and make for the easiest usage case. It
> may not be "pretty", but I can whip up a patch in five minutes.
I don't have a problem with STOP_ON_UNKNOWN, as I think it is a building
block upon which sane things can be done (like linearly going through
each parser and saying "did you want this?"). I think IGNORE/CONTINUE
has a fundamental flaw.
> Or are we going to sit around discussing this for another five months?
Please! :)
Pierre was working on the approach I mentioned, but I think he is short
on time. I will take a look at the conversion, but I have a few other
fixes on my plate first.
In the meantime, I don't think your patch makes anything _worse_, since
we already have these sorts of bugs in the current parsing code.
-Peff
^ permalink raw reply
* Git clone behaviour change in 1.5.6 (vs 1.5.5.1)
From: Kalle Vahlman @ 2008-06-23 19:51 UTC (permalink / raw)
To: git
Hi!
Switching to the 1.5.6 release from 1.5.5.1, I found out that the
rewritten git-clone command changed its behaviour wrt cloning to a
non-existing destination directory structure. In the shell version the
destination (work tree) is created with 'mkdir -p' but in the C
version with just the mkdir() call which doesn't create the parent
directories.
I can't find any indication that this would be intended in the repo
history nor in the mailing list, so I'm left thinking that this is an
unwanted regression. Could someone confirm this?
--
Kalle Vahlman, zuh@iki.fi
Powered by http://movial.fi
Interesting stuff at http://syslog.movial.fi
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Pierre Habouzit @ 2008-06-23 19:24 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Johannes Schindelin, Git Mailing List, Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806230912230.2926@woody.linux-foundation.org>
[-- Attachment #1: Type: text/plain, Size: 2940 bytes --]
On Mon, Jun 23, 2008 at 04:25:10PM +0000, Linus Torvalds wrote:
>
>
> On Mon, 23 Jun 2008, Johannes Schindelin wrote:
> >
> > Thinking about the recursive approach again, I came up with this POC:
>
> "recursive" is pointless.
>
> The problem with the current "parse_options()" is not that it's recursive
> (although that has been claimed multiple times!.
>
> The problem with parse_options() is that it's currently impossible to
> write something that handles _partial_ cases.
>
> Let me explain.
>
> Look at cmd_apply() in builtin-apply.c. Notice how it currently absolutely
> CANNOT sanely be turned into using "parse_options()", not because it needs
> any "recursive" handling, but simply because it wants to do *incremental*
> handling.
>
> It should be perfectly possible to change that argument loop from
>
> for (i = 1; i < argc; i++) {
> const char *arg = argv[i];
> if (strcmp(arg, "-")) {
> .. handle <stdin> ..
> continue;
> }
> ...
>
> to doing something like this:
>
> for (;;) {
> const char *arg;
> argc = parse_options(argc, argv,
> options, usage, PARSE_OPT_STOP_AT_UNKNOWN);
> if (!argc)
> break;
> arg = argv[1];
> argv++;
> argc--;
> if (strcmp(arg, "-")) {
> .. handle <stdin> ..
> continue;
> }
> ...
>
> or whatever. See?
Indeed, I read the thread many times, and I like the incremental
approach very much, it really makes sense, and we can easily migrate
code this way indeed. In fact that should have been the way
parse_options was designed from the beginning.
There is a bit of work to do from this "handwaved" solution, because
people care about the filtered argv, so one should rememeber some kind
of "writing" position. SOmething like:
parse_opt_ctx_t ctx;
/* will basically copy argc/argv */
parse_options_start(&ctx, argc, argv);
for (;;) {
const char *arg;
int res = parse_options_step(&ctx, options, usage, 0));
if (res == PARSE_OPT_HELP) {
/* generate help and exit */
}
if (res == PARSE_OPT_DONE)
break;
arg = ctx->argv[ctx->pos++];
if (strcmp(arg, "-")) {
... handle <stdin>....
continue;
}
}
argc = parse_options_end(&ctx);
/* at this point (argc,argv) is almost what parse_options would have
left us */
THis way, parse_options can be written:
parse_opt_ctx_t ctx;
parse_options_start(&ctx, argc, argv);
switch (parse_options_step(&ctx, options, usage, 0)) {
case PARSE_OPT_HELP: exit....
case PARSE_OPT_DONE: break;
default: exit(error("unknown option ....");
}
return parse_options_end(&ctx);
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Linus Torvalds @ 2008-06-23 19:16 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806231137070.2926@woody.linux-foundation.org>
On Mon, 23 Jun 2008, Linus Torvalds wrote:
>
> Or are we going to sit around discussing this for another five months?
So the gauntlet is thrown.
This is the trivial version that (on top of my original patch in this
thread) makes
git blame --since=April -b Makefile
work.
Does it handle all cases? No. In particular, because it still makes
builtin-blame.c use PARSE_OPT_STOP_AT_NON_OPTION, and because cmd_blame.c
simply doesn't do the sane thing (it took me more than five minutes just
because I tried to make it do the same thing and not do any argument
parsing at all, but I just gave up), you still cannot pass it things like
--default HEAD
because it will stop at HEAD, and then do the exact wrong thing.
Could it be improved? I'm sure. And I didn't test it much at all. I also
didn't change any existing users that could possibly use the new
PARSE_OPT_STOP_AT_UNKNOWN thing. So this patch is an example patch only,
to show what I'm talking about when I say "simple".
Linus
---
builtin-blame.c | 2 +-
parse-options.c | 26 ++++++++++++++++++++++----
parse-options.h | 2 ++
3 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/builtin-blame.c b/builtin-blame.c
index f04bd93..059062c 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2195,7 +2195,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
cmd_is_annotate = !strcmp(argv[0], "annotate");
argc = parse_options(argc, argv, options, blame_opt_usage,
- PARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH);
+ PARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN);
/*
* parse_options() offsets things by one - undo for now to make
diff --git a/parse-options.c b/parse-options.c
index acf3fe3..5b4653f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -7,7 +7,7 @@
struct optparse_t {
const char **argv;
const char **out;
- int argc, cpidx;
+ int argc, cpidx, flags;
const char *opt;
};
@@ -139,6 +139,8 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)
return get_value(p, options, OPT_SHORT);
}
}
+ if (p->flags & (PARSE_OPT_STOP_AT_UNKNOWN | PARSE_OPT_KEEP_UNKNOWN))
+ return -1;
return error("unknown switch `%c'", *p->opt);
}
@@ -224,6 +226,8 @@ is_abbreviated:
abbrev_option->long_name);
if (abbrev_option)
return get_value(p, abbrev_option, abbrev_flags);
+ if (p->flags & (PARSE_OPT_STOP_AT_UNKNOWN | PARSE_OPT_KEEP_UNKNOWN))
+ return -1;
return error("unknown option `%s'", arg);
}
@@ -253,7 +257,7 @@ static NORETURN void usage_with_options_internal(const char * const *,
int parse_options(int argc, const char **argv, const struct option *options,
const char * const usagestr[], int flags)
{
- struct optparse_t args = { argv + 1, argv, argc - 1, 0, NULL };
+ struct optparse_t args = { argv + 1, argv, argc - 1, 0, flags, NULL };
for (; args.argc; args.argc--, args.argv++) {
const char *arg = args.argv[0];
@@ -269,8 +273,15 @@ int parse_options(int argc, const char **argv, const struct option *options,
args.opt = arg + 1;
if (*args.opt == 'h')
usage_with_options(usagestr, options);
- if (parse_short_opt(&args, options) < 0)
+ if (parse_short_opt(&args, options) < 0) {
+ if (flags & PARSE_OPT_STOP_AT_UNKNOWN)
+ break;
+ if (flags & PARSE_OPT_KEEP_UNKNOWN) {
+ args.out[args.cpidx++] = args.argv[0];
+ continue;
+ }
usage_with_options(usagestr, options);
+ }
if (args.opt)
check_typos(arg + 1, options);
while (args.opt) {
@@ -294,8 +305,15 @@ int parse_options(int argc, const char **argv, const struct option *options,
usage_with_options_internal(usagestr, options, 1);
if (!strcmp(arg + 2, "help"))
usage_with_options(usagestr, options);
- if (parse_long_opt(&args, arg + 2, options))
+ if (parse_long_opt(&args, arg + 2, options)) {
+ if (flags & PARSE_OPT_STOP_AT_UNKNOWN)
+ break;
+ if (flags & PARSE_OPT_KEEP_UNKNOWN) {
+ args.out[args.cpidx++] = args.argv[0];
+ continue;
+ }
usage_with_options(usagestr, options);
+ }
}
memmove(args.out + args.cpidx, args.argv, args.argc * sizeof(*args.out));
diff --git a/parse-options.h b/parse-options.h
index 4ee443d..a98f0ec 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -20,6 +20,8 @@ enum parse_opt_type {
enum parse_opt_flags {
PARSE_OPT_KEEP_DASHDASH = 1,
PARSE_OPT_STOP_AT_NON_OPTION = 2,
+ PARSE_OPT_KEEP_UNKNOWN = 4,
+ PARSE_OPT_STOP_AT_UNKNOWN = 8,
};
enum parse_opt_option_flags {
^ permalink raw reply related
* Re: [PATCH v4] git-add--interactive: manual hunk editing mode
From: Johannes Schindelin @ 2008-06-23 18:54 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Rast, git
In-Reply-To: <20080623183840.GA28887@sigill.intra.peff.net>
Hi,
On Mon, 23 Jun 2008, Jeff King wrote:
> The big question is what is happening with the recount work. Johannes,
> are you planning on re-submitting those patches?
Oh, so much to do. I have 55 patches in my personal fork, on top of
'next'. And a few of them need resubmitting, such as the initHook work,
and, yes, add --edit.
I am not sure when I will have time for that (particularly given that I
got sidetracked with the OPTION_OPTIONS patch, when I should have worked
on something completely different).
In the meantime, feel free to submit in my name.
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Linus Torvalds @ 2008-06-23 18:47 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <20080623183358.GA28941@sigill.intra.peff.net>
On Mon, 23 Jun 2008, Jeff King wrote:
>
> It's worse than that. We assume by default that the option has no
> argument, so the argument becomes a non-option parameter to the original
> command. Try (with current git-blame, but I think your patch doesn't
> change this):
>
> $ git blame -n 1 git.c
> fatal: bad revision '1'
>
> $ git blame --default HEAD git.c
> fatal: cannot stat path HEAD: No such file or directory
>
> Oops.
Oops. And then, how would you fix this most easily?
Be honest now.
Hint: it _still_ involves PARSE_OPT_CONTINUE_ON_UNKNOWN. It would fix both
cases.
IOW, the whole builtin-blame.c option parsing *should* look like this:
argc = parse_options(argc, argv, options, usage, PARSE_OPT_CONTINUE_ON_UNKNOWN);
init_revisions(&revs, NULL);
setup_revisions(argc, argv, &revs, NULL);
and it should just work.
But we should *also* have some way to do things like the code in
builtin-ls-files.c which have a few options that don't work well with the
current parse_options(). Yes, you can make all of them work with
callbacks, but that often ends up requiring moving arguments around.
There's no way to make a trivial conversion for 90% of the cases, and then
leaving the 10% that need other changes.
Example: many arguments cause multiple option variables to change.
parse_options() simply can't handle that well - you can do it with a
callback, but then you need to make the option variables global or make
them a structure or something. All of which just makes it nasty to do
partial conversions for the simple cases.
And I guarantee that just adding PARSE_OPT_{CONTINUE|STOP}_ON_UNKNOWN is
going to be the smallest patch, and make for the easiest usage case. It
may not be "pretty", but I can whip up a patch in five minutes.
Or are we going to sit around discussing this for another five months?
Linus
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Johannes Schindelin @ 2008-06-23 18:41 UTC (permalink / raw)
To: Jeff King
Cc: Linus Torvalds, Pierre Habouzit, Git Mailing List, Junio C Hamano
In-Reply-To: <20080623172612.GD27265@sigill.intra.peff.net>
Hi,
On Mon, 23 Jun 2008, Jeff King wrote:
> On Mon, Jun 23, 2008 at 06:04:48PM +0100, Johannes Schindelin wrote:
>
> > > > Thinking about the recursive approach again, I came up with this
> > > > POC:
> > >
> > > "recursive" is pointless.
> >
> > Nope, it is not.
>
> AIUI, one difference between your approach and Pierre's is that he is
> suggesting (and I have suggested this in the past, too) a big
> DIFF_OPTIONS macro that you stick in the options table for each command.
> Whereas you are allowing for subtables accessible via pointers.
>
> Your approach should yield a much leaner text size
Yes, this is the only difference between Pierre's current approach and my
POC.
> > Heck, we could just as easily introduce PARSE_OPT_IGNORE_UNKNOWN.
>
> We could even send it to the list with message-id
>
> http://mid.gmane.org/1213758236-979-2-git-send-email-shawn.bohrer@gmail.com
>
> and then Junio and I could complain that the concept is broken.
Heh. I thought I saw it, but a quick search did not reveal it, and I
really don't have time to work on this, unfortunately.
Thanks,
Dscho
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Johannes Schindelin @ 2008-06-23 18:39 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Pierre Habouzit, Git Mailing List, Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806231019170.2926@woody.linux-foundation.org>
Hi,
On Mon, 23 Jun 2008, Linus Torvalds wrote:
> (And yes, I can handle even the --help cases. It's actually not that
> hard to just remember all the different option structs you've seen, and
> handle all of that totally independently internally in parse_options().
> You don't need recursion, you just need a trivial list of options.)
So what you actually do is just linearizing the recursion. Well then,
yes, your solution is good.
However, I thought that recursion (as I implemented it) would make the
revlist option parsing (which includes diff parsing) quite nice.
Whatever,
Dscho
^ permalink raw reply
* Re: [PATCH v4] git-add--interactive: manual hunk editing mode
From: Jeff King @ 2008-06-23 18:38 UTC (permalink / raw)
To: Thomas Rast; +Cc: Johannes Schindelin, git
In-Reply-To: <200806131748.44867.trast@student.ethz.ch>
On Fri, Jun 13, 2008 at 05:48:43PM +0200, Thomas Rast wrote:
> Adds a new option 'e' to the 'add -p' command loop that lets you edit
> the current hunk in your favourite editor.
> [...]
> Applying the changed hunk(s) relies on Johannes Schindelin's new
> --recount option for git-apply.
This version looks pretty good to me (though I do have a few comments
below), and now that we are in the new release cycle, I think it is time
to re-submit "for real".
The big question is what is happening with the recount work. Johannes,
are you planning on re-submitting those patches?
> > Right, but you may get false positives if a later conflicting hunk is
> > not staged. Though as you say, I think it is unlikely in either case.
> I'd rather reject early and offer to re-edit, than notice a problem
> much later, so I left it the way it was.
Yeah, thinking about it more, that is the sanest choice.
> I was tempted to reintroduce globbed unlinking of the temporary file
> as in v3 (that is, removing TMP* instead of just TMP). In the absence
> of it, backup files made by the user's editor will remain in .git/.
> Eventually I didn't because it seems vi doesn't make backups anyway,
> and while emacs does, enabling that for GIT_EDITOR seems a user error.
> Besides, how do we know the backups match that pattern anyway. Not
> sure though.
I would leave it; it seems like the editor's responsibility to handle
this. We don't know what patterns are used, or when it is safe to remove
such backups. And this is far from the only place where we invoke the
editor on a temporary file, so any solution should probably be applied
globally.
> + my @newtext = grep { !/^#/ } <$fh>;
> [...]
> + # Abort if nothing remains
> + if (@newtext == 0) {
> + return undef;
> + }
Reading over this again, I wonder if it should be:
# Abort if nothing remains
if (!grep { /\S/ } @newtext) {
return undef;
}
That is, we can't eliminate blank lines on input, since they might be
meaningful to the diff. But if the user removes every non-comment line
_except_ a blank line or a line with only whitespace, we probably want
to abort, too.
-Peff
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Linus Torvalds @ 2008-06-23 18:36 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <20080623181517.GA28527@sigill.intra.peff.net>
On Mon, 23 Jun 2008, Jeff King wrote:
>
> There is already a discussion underway about the proper solution.
No there isn't, not any I have seen.
The recursion discussion has been going on for ages. Nothing happened,
because people claimed it was too complex. And it doesn't solve all the
_other_ issues where the current behaviour is just painful.
That said, I don't care how this gets fixed. If you want to fix it some
other way, fine. Just _do_ it, dammit. And make the interface easier to
use for incremental changes of existing code.
Because no, I don't care about builtin-blame.c in particular either. But I
*do* care about the fact that currently it's sometimes a total *bitch* to
convert the trivial ad-hoc argument parsing to use parse_options(). It
takes effort and thinking.
And I seriously doubt that recursive or anything else will help that. But
I _guarantee_ that an incremental mode will make it much much easier to do
partial conversions of all the easy cases. And _that_ is what I care
about.
Linus
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Jeff King @ 2008-06-23 18:33 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806231114180.2926@woody.linux-foundation.org>
On Mon, Jun 23, 2008 at 11:20:46AM -0700, Linus Torvalds wrote:
> Actually, I guess "--default" does, but if you try to mix that up with (a)
> a default head that starts with a dash and (b) git-blame, you're doing
> something pretty odd.
Yes, and I think that is why "in practice" this works with git-blame.
> And yes, "-n" does too, but if you pass it negative numbers you get what
> you deserve.
It's worse than that. We assume by default that the option has no
argument, so the argument becomes a non-option parameter to the original
command. Try (with current git-blame, but I think your patch doesn't
change this):
$ git blame -n 1 git.c
fatal: bad revision '1'
$ git blame --default HEAD git.c
fatal: cannot stat path HEAD: No such file or directory
Oops. Now again, as it happens, git-blame seems to ignore "-n" entirely
(though I would have expected it to limit the depth of the blame
traversal), so maybe people shouldn't be using it.
> The point being, we really _do_ have a real-life existing case for
> PARSE_OPT_CONTINUE_ON_UNKNOWN, which is hard to handle any other way.
> Currently you can literally do
>
> git blame --since=April -b Makefile
>
> and while it's a totally made-up example, it's one I've picked to show
> exactly what does *not* work with my patch that started this whole thread.
>
> And guess what you need to fix it?
>
> If you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize.
I guessed "to convert the revision and diff options to a format that
parse_options understands, so we can add them as appropriate to the
options tables of the various commands." Do I win anything?
-Peff
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Linus Torvalds @ 2008-06-23 18:20 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806231027210.2926@woody.linux-foundation.org>
On Mon, 23 Jun 2008, Linus Torvalds wrote:
>
> Umm. Helloo, reality.. There are actually very few options that take a
> flag for their arguments. In particular, the option parsing we really
> _care_ about (revision parsing - see builtin-blame.c which is exactly
> where I wanted to convert things) very much DOES NOT.
Actually, I guess "--default" does, but if you try to mix that up with (a)
a default head that starts with a dash and (b) git-blame, you're doing
something pretty odd.
And yes, "-n" does too, but if you pass it negative numbers you get what
you deserve.
The point being, we really _do_ have a real-life existing case for
PARSE_OPT_CONTINUE_ON_UNKNOWN, which is hard to handle any other way.
Currently you can literally do
git blame --since=April -b Makefile
and while it's a totally made-up example, it's one I've picked to show
exactly what does *not* work with my patch that started this whole thread.
And guess what you need to fix it?
If you guessed PARSE_OPT_CONTINUE_ON_UNKNOWN, you win a prize.
Linus
^ permalink raw reply
* Code Formatting vs Trailing Whitespaces
From: Florian Köberle @ 2008-06-23 18:15 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce, Robin Rosenberg, Marek Zawirski
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi
I noticed that Robin removed trailing whitespaces from my patch(thanks).
It appears that if you enable code formatting and the removement of
trailing whitespaces in the save actions option that the formatting
option will win.
It will add a space at the second line of a javadoc comment:
/**
~ * <----
I think this is a bug in eclipse, did any of you create a bug report for
that? Or a feature request for an "no whitespace after javadoc" option
in the code formatter page?
I want to have both options active, because I don't want to commit wrong
formatted patches.
Best regards,
Florian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFIX+hW59ca4mzhfxMRApU4AKCbYzwRFqVcBCr11zc9lk6jqick2ACeMBhW
nXOoN4nIsTjzWWhQzDOxiz4=
=bm+i
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Jeff King @ 2008-06-23 18:15 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806231027210.2926@woody.linux-foundation.org>
On Mon, Jun 23, 2008 at 10:32:21AM -0700, Linus Torvalds wrote:
> > How can that be correct, if you don't know whether "-b" takes an
> > argument?
>
> Did you read my post or not?
>
> If you have that case, then use STOP_ON_UNKNOWN.
How do you know that is the case, unless you know what the other option
parsers are going to do? Or are you suggesting that I check every other
downstream option parser to make sure that it's OK in this particular
instance, use IGNORE_UNKNOWN, and then laugh maniacally when somebody
adds such an option to the diff option parser later?
> Umm. Helloo, reality.. There are actually very few options that take a
> flag for their arguments. In particular, the option parsing we really
> _care_ about (revision parsing - see builtin-blame.c which is exactly
> where I wanted to convert things) very much DOES NOT.
Reality: revision.c, lines 1008-1012. "-n" takes an argument.
Reality: revision.c, lines 1075-1080. "--default" takes an argument.
> Try just looking at the code!
I did. Or maybe you missed the thread where this exact feature was
mentioned, and I already looked at the code and mentioned those two
spots. It's right here:
http://thread.gmane.org/gmane.comp.version-control.git/85354/focus=85355
> So I'm really not interested in arguing about "theoretical issues", when
> we have a real-life *practical* issue to solve.
>
> Solve builtin-blame.c for me. I sent out a patch yesterday, but in the
> description of that patch I also described exactly why I want
> CONTINUE_ON_UNKNOWN.
There is already a discussion underway about the proper solution. This
isn't just a git-blame issue, but rather an issue with all commands that
have their own options and take revision parameters. So I think rather
than doing a halfway fix that happens to work with git-blame, it is more
useful to focus on a solution that works everywhere and fix _all_ of the
problems.
-Peff
^ permalink raw reply
* Re: [PATCH v6] gitweb: add test suite with Test::WWW::Mechanize::CGI
From: Lea Wiemann @ 2008-06-23 17:57 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <200806231531.13082.jnareb@gmail.com>
[Open issue about PERL_PATH at the bottom!]
Jakub Narebski wrote:
> Lea Wiemann wrote:
>
>> +# We don't count properly when skipping, so no_plan is necessary.
>> +use Test::More qw(no_plan);
>
> I'd rather either skip this comment all together, or change it [...]
I'll just remove it. Anybody trying to add a test count will give up
within 30 seconds. ;-)
>> +our $long_tests = $ENV{GIT_TEST_LONG};
>
> Here also it could be "my $long_tests";
This one is to make "local $long_tests = 0" work -- it wouldn't work
with a 'my' declaration.
>> +chomp(my @heads = map { (split('/', $_))[2] } `git-for-each-ref --sort=-committerdate refs/heads`);
>
> Wouldn't be easier to use '--format=%(refname)' in git-for-each-ref
*checks* No, since I want to strip the leading refs/heads/ anyway (in
order to test the page text, which mentions heads and tags without the
directory prefix) -- hence the 'split' wouldn't go away with
--format=%(refname).
>> +chomp(my @files = map { (split("\t", $_))[1] } `git-ls-tree HEAD`);
>
> Wouldn't it be easier to use "git ls-tree --names-only HEAD"?
Yup ('--name-only' though). Wonder why I didn't use it -- fixed.
>> +# Thus subroutine was copied (and modified to work with blanks in the
>> +# application path) from WWW::Mechanize::CGI 0.3, which is licensed
>> +# 'under the same terms as perl itself' and thus GPL compatible.
That's a tyop: s/Thus/This/ (fixed). It refers to the following "my
$cgi = sub". Does the comment make sense now?
>> +my $cgi = sub {
>> + # Use exec, not the shell, to support blanks in the path.
>
> blanks = embedded whitespace? path = path to gitweb?
Yup. Improved comment, and simplified system call:
- # Use exec, not the shell, to support blanks in the path.
- my $status = system { $ENV{GITPERL} } $ENV{GITPERL}, $gitweb;
+ # Use exec, not the shell, to support embedded whitespace in
+ # the path to gitweb.cgi.
+ my $status = system $ENV{GITPERL}, $gitweb;
>> + # WebService::Validator::Feed::W3C would be nice to
>> + # use, but it doesn't support direct input (as opposed
>> + # to URIs) long enough for our feeds.
>
> Could you tell us here what are the limitations? Are those limits
> of W3C SOAP interface for web validation, or the CPAN package mentioned?
No idea. I tried it and it told me that the URL was too long -- I
suspect it's the W3C server that rejected it. I wouldn't spend more
effort trying to get online validation to work; it's probably easier to
just occasionally validate manually. ;) XML well-formedness tests
should catch most occasional breakages just fine.
>> + # Broken link in Atom/RSS view -- cannot spider:
>> + # http://mid.gmane.org/485EB333.5070108@gmail.com
>> + local $long_tests = 0;
>> + test_page('?p=.git;a=atom', 'Atom feed');
>
> This I think should be written using Test::More equivalent of
> test_expect_failure in t/test-lib.sh, namely TODO block,
Right -- here's my new version (which still fails if the feeds die or
are not well-formed XML -- I'll want that when I change gitweb):
# RSS/Atom/OPML view
# Simply retrieve and verify well-formedness, but don't spider.
$mech->get_ok('?p=.git;a=atom', 'Atom feed') and _verify_page;
$mech->get_ok('?p=.git;a=rss', 'RSS feed') and _verify_page;
TODO: {
# Now spider -- but there are broken links.
# http://mid.gmane.org/485EB333.5070108@gmail.com
local $TODO = "fix broken links in Atom/RSS feeds";
test_page('?p=.git;a=atom', 'Atom feed');
test_page('?p=.git;a=rss', 'RSS feed');
}
test_page('?a=opml', 'OPML outline');
>> [in t/test-lib.sh:]
>> +GITPERL=${GITPERL:-perl}
>> +export GITPERL
>> [The idea is to easily run the test suite with different perl versions.]
>
> How it is different from PERL_PATH?
Right, I didn't think of that. PERL_PATH isn't available in the tests
though, it's only used internally by the Makefile to generate (among
other things) gitweb.cgi. This means that while we can control under
which Perl version gitweb.cgi runs, we cannot control under which Perl
version the test suite runs (at least without $PATH trickery). Does
this bother us?
If yes, I'd suggest we keep GITPERL but rename it to GIT_TEST_PERL,
because that's what it's about. If not, I'll rip it out and simply call
'perl' in the test shell script, whatever version it may be.
-- Lea
^ permalink raw reply
* [[JGIT PATCH]] Implementation of a copy constructor for FileNameMatcher.
From: Florian Köberle @ 2008-06-23 17:43 UTC (permalink / raw)
To: git; +Cc: robin.rosenberg, spearce, Florian Köberle
In-Reply-To: <485FDE42.1060106@web.de>
Signed-off-by: Florian Köberle <florianskarten@web.de>
---
.../spearce/jgit/fnmatch/FileNameMatcherTest.java | 31 ++++++++++++++++++++
.../org/spearce/jgit/fnmatch/FileNameMatcher.java | 30 +++++++++++++++++--
2 files changed, 58 insertions(+), 3 deletions(-)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/fnmatch/FileNameMatcherTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/fnmatch/FileNameMatcherTest.java
index ad72ac8..0c9501b 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/fnmatch/FileNameMatcherTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/fnmatch/FileNameMatcherTest.java
@@ -723,4 +723,35 @@ public class FileNameMatcherTest extends TestCase {
assertEquals(true, childMatcher.isMatch());
assertEquals(false, childMatcher.canAppendMatch());
}
+
+ public void testCopyConstructor() throws Exception {
+ final String pattern = "helloworld";
+ final FileNameMatcher matcher = new FileNameMatcher(pattern, null);
+ matcher.append("hello");
+ final FileNameMatcher copy = new FileNameMatcher(matcher);
+ assertEquals(false, matcher.isMatch());
+ assertEquals(true, matcher.canAppendMatch());
+ assertEquals(false, copy.isMatch());
+ assertEquals(true, copy.canAppendMatch());
+ matcher.append("world");
+ assertEquals(true, matcher.isMatch());
+ assertEquals(false, matcher.canAppendMatch());
+ assertEquals(false, copy.isMatch());
+ assertEquals(true, copy.canAppendMatch());
+ copy.append("world");
+ assertEquals(true, matcher.isMatch());
+ assertEquals(false, matcher.canAppendMatch());
+ assertEquals(true, copy.isMatch());
+ assertEquals(false, copy.canAppendMatch());
+ copy.reset();
+ assertEquals(true, matcher.isMatch());
+ assertEquals(false, matcher.canAppendMatch());
+ assertEquals(false, copy.isMatch());
+ assertEquals(true, copy.canAppendMatch());
+ copy.append("helloworld");
+ assertEquals(true, matcher.isMatch());
+ assertEquals(false, matcher.canAppendMatch());
+ assertEquals(true, copy.isMatch());
+ assertEquals(false, copy.canAppendMatch());
+ }
}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/fnmatch/FileNameMatcher.java b/org.spearce.jgit/src/org/spearce/jgit/fnmatch/FileNameMatcher.java
index 9ac1875..702f7b3 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/fnmatch/FileNameMatcher.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/fnmatch/FileNameMatcher.java
@@ -99,10 +99,23 @@ public class FileNameMatcher {
* must be a list which will never be modified.
*/
private FileNameMatcher(final List<Head> headsStartValue) {
+ this(headsStartValue, headsStartValue);
+ }
+
+ /**
+ *
+ * @param headsStartValue
+ * must be a list which will never be modified.
+ * @param heads
+ * a list which will be cloned and then used as current head
+ * list.
+ */
+ private FileNameMatcher(final List<Head> headsStartValue,
+ final List<Head> heads) {
this.headsStartValue = headsStartValue;
- this.heads = new ArrayList<Head>(headsStartValue.size());
- this.heads.addAll(this.headsStartValue);
- this.listForLocalUseage = new ArrayList<Head>(headsStartValue.size());
+ this.heads = new ArrayList<Head>(heads.size());
+ this.heads.addAll(heads);
+ this.listForLocalUseage = new ArrayList<Head>(heads.size());
}
/**
@@ -120,6 +133,17 @@ public class FileNameMatcher {
this(createHeadsStartValues(patternString, invalidWildgetCharacter));
}
+ /**
+ * A Copy Constructor which creates a new {@link FileNameMatcher} with the
+ * same state and reset point like <code>other</code>.
+ *
+ * @param other
+ * another {@link FileNameMatcher} instance.
+ */
+ public FileNameMatcher(FileNameMatcher other) {
+ this(other.headsStartValue, other.heads);
+ }
+
private static List<Head> createHeadsStartValues(
final String patternString, final Character invalidWildgetCharacter)
throws InvalidPatternException {
--
1.5.4.3
^ permalink raw reply related
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Linus Torvalds @ 2008-06-23 17:32 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <20080623171505.GB27265@sigill.intra.peff.net>
On Mon, 23 Jun 2008, Jeff King wrote:
>
> How can that be correct, if you don't know whether "-b" takes an
> argument?
Did you read my post or not?
If you have that case, then use STOP_ON_UNKNOWN.
> That is the only thing that makes sense to me, since the command line
> has to be parsed left-to-right (because the syntactic function of an
> element relies on the semantics of the element to its left).
Umm. Helloo, reality.. There are actually very few options that take a
flag for their arguments. In particular, the option parsing we really
_care_ about (revision parsing - see builtin-blame.c which is exactly
where I wanted to convert things) very much DOES NOT.
And that CONTINUE_ON_UNKNOWN behaviour is not some kind of theoretical
flag. It's the flag that builtin-blame.c *wants*. It's the flag that
describes hat builtin-blame.c does right now in the current git master
branch.
Try just looking at the code!
So I'm really not interested in arguing about "theoretical issues", when
we have a real-life *practical* issue to solve.
Solve builtin-blame.c for me. I sent out a patch yesterday, but in the
description of that patch I also described exactly why I want
CONTINUE_ON_UNKNOWN.
So can we please stop the clusterfuck now?
Linus
^ permalink raw reply
* Re: [PATCH 1/2] Create a fnmatch-style pattern TreeFilter
From: Florian Köberle @ 2008-06-23 17:32 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Shawn O. Pearce, Marek Zawirski
In-Reply-To: <1214177145-18963-1-git-send-email-robin.rosenberg@dewire.com>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Robin,
thank you for accepting my first patch :D.
| +/**
| + * This class implements a TreeeFilter that uses the wildcard style
pattern
| + * matching like of Posix fnmatch function.
| + */
Typo: One 'e' to much in TreeeFilter.
It would be more efficient to
| + @Override
| + public TreeFilter clone() {
| + return new WildCardTreeFilter(pattern);
| + }
One way to create a clone of the FileNameMatcher is to call:
originalMatcher.reset()
FileNameMatcher clone = originalMatcher.createMatcherForSuffix()
I will send a patch which implements a copy constructor for FileNameMatcher.
First I wanted to implement a clone() method, but found this page and
decided then to implement a copy constructor:
http://www.javapractices.com/topic/TopicAction.do?Id=71
A Implementor of a super class could imply that clone() of object gets
called, as stated in the javadoc of clone():
quote (javadoc ob Object#clone()):
- -----------
By convention, the returned object should be obtained by calling super.clone
- -----------
I think this is a bad convention, as one should not rely on
Object#clone() to do the copy job for one. If you really need a clone
method then I would do it the same way you did, by calling a constructor
which does the job.
Best regards,
Florian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFIX94k59ca4mzhfxMRAgDjAJ9S76L8I5Lqed4lKfgTf+2cp2IQ9gCfQNVh
z72+NGvmIy3H0gwveKRfn+w=
=wnpy
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Jeff King @ 2008-06-23 17:26 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Linus Torvalds, Pierre Habouzit, Git Mailing List, Junio C Hamano
In-Reply-To: <alpine.DEB.1.00.0806231756340.6440@racer>
On Mon, Jun 23, 2008 at 06:04:48PM +0100, Johannes Schindelin wrote:
> > > Thinking about the recursive approach again, I came up with this POC:
> >
> > "recursive" is pointless.
>
> Nope, it is not.
AIUI, one difference between your approach and Pierre's is that he is
suggesting (and I have suggested this in the past, too) a big
DIFF_OPTIONS macro that you stick in the options table for each command.
Whereas you are allowing for subtables accessible via pointers.
Your approach should yield a much leaner text size (which was what
started this whole thing) since we don't end up with the same repeated
subsets of options tables, and in particular the one-liner help text
(yes, compilers can sometimes point share the string literal components,
but most of our options tables are declared as static, so unless the
linker is very smart, I think we will end up with duplicates).
> Heck, we could just as easily introduce PARSE_OPT_IGNORE_UNKNOWN.
We could even send it to the list with message-id
http://mid.gmane.org/1213758236-979-2-git-send-email-shawn.bohrer@gmail.com
and then Junio and I could complain that the concept is broken.
-Peff
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Linus Torvalds @ 2008-06-23 17:21 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Pierre Habouzit, Git Mailing List, Junio C Hamano
In-Reply-To: <alpine.DEB.1.00.0806231756340.6440@racer>
On Mon, 23 Jun 2008, Johannes Schindelin wrote:
>
> > Look at cmd_apply() in builtin-apply.c. Notice how it currently
> > absolutely CANNOT sanely be turned into using "parse_options()", not
> > because it needs any "recursive" handling, but simply because it wants
> > to do *incremental* handling.
>
> That is a totally independent issue from the one I discussed, namely sane
> handling of the diff (and rev) options.
No it is not.
YOU THINK it is "independent" just because YOUR SOLUTION can do only one
case.
And I say: my solution handles both cases, so it's not "independent" any
more.
See?
(And yes, I can handle even the --help cases. It's actually not that hard
to just remember all the different option structs you've seen, and handle
all of that totally independently internally in parse_options(). You don't
need recursion, you just need a trivial list of options.)
Linus
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Jeff King @ 2008-06-23 17:15 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806230953550.2926@woody.linux-foundation.org>
On Mon, Jun 23, 2008 at 10:06:52AM -0700, Linus Torvalds wrote:
> You'd start off with argv[] looking like [ "foo" "-b" "-a" ] and then
> after calling parse_options with that, depending on whether it has
> PARSE_OPT_CONTINUE_ON_UNKNOWN or not, you'd either end up with the "-a"
> handled (and argv[] now being just [ "foo" "-b" ]), or if you have
How can that be correct, if you don't know whether "-b" takes an
argument?
> PARSE_OPT_STOP_ON_UNKNOWN then parse_options() would return without having
> done anything, and expecting you to handle the unknown option first and
> then restarting the argument parsing.
That is the only thing that makes sense to me, since the command line
has to be parsed left-to-right (because the syntactic function of an
element relies on the semantics of the element to its left).
-Peff
^ permalink raw reply
* Re: Incorrect default for git stash?
From: David Kågedal @ 2008-06-23 17:14 UTC (permalink / raw)
To: git
In-Reply-To: <911589C97062424796D53B625CEC0025E46170@USCOBRMFA-SE-70.northamerica.cexp.com>
<Patrick.Higgins@cexp.com> writes:
> Eric Raible wrote:
>
>> git branch -> list branches
>> git tag -> list tags
>> git stash -> create a stash
>
> git commit -> list commits?
> git checkout -> list checkouts?
> git prune -> list prunes?
> git pull -> list pullables?
>
> Why would you expect stash to behave like branch and tag?
Because is kindof does.
I would prefer to have two simple commands
git stash
git unstash
and not bother with named stashes etc (for that I use stgit or normal
branches). You'd need a way to clear the stash as well.
--
David Kågedal
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Linus Torvalds @ 2008-06-23 17:06 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <20080623164917.GA25474@sigill.intra.peff.net>
On Mon, 23 Jun 2008, Jeff King wrote:
> On Mon, Jun 23, 2008 at 09:25:10AM -0700, Linus Torvalds wrote:
>
> > Could you handle the "recursive" use of parse_options() in builtin-blame.c
> > by teaching it about recursion? Yes. But again, it's just _simpler_ to
> > just teach parse_options() to parse the things it knows about, and leave
> > the other things in place.
>
> If I know that I have option "-a", what is the correct partial parsing
> of:
>
> git foo -b -a
>
> ?
You'd start off with argv[] looking like [ "foo" "-b" "-a" ] and then
after calling parse_options with that, depending on whether it has
PARSE_OPT_CONTINUE_ON_UNKNOWN or not, you'd either end up with the "-a"
handled (and argv[] now being just [ "foo" "-b" ]), or if you have
PARSE_OPT_STOP_ON_UNKNOWN then parse_options() would return without having
done anything, and expecting you to handle the unknown option first and
then restarting the argument parsing.
The problem with parse_options() right now is:
- it cannot do this at all (it can stop or ignore *non*arguments, but
not things that start with "-" and will always error out)
- it actually puts the result somewhere different than the source, which
makes it very annoying to work with together with anything else that
also wants to look at, or change, argv/argc.
IOW, right now it takes it's arguments from argv[1...], but then puts
back the remainder into argv[0...]. Similarly, it takes a "count plus
one" value of argc, but then _returns_ a "count plus zero".
That second thing is an annoyance, and could be handled either with a new
flag (PARSE_OPT_PUT_BACK_IDENTICALLY), or by just changing the calling
convention. The latter is the "right thing", but it needs trivial changes
in all existing callers.
I actually suspect that the best fix for the second issue is to yes,
change the calling convention so that it puts things back where it found
them, and returns a "count plus one" count, *but* have a special case
where "if all arguments are used, return zero".
[ IOW, it would never ever return "1". If there is one argument left in
argv[1], it returns 2 because it counts "argv[0]" even if it doesn't
_use_ it. That's exactly the same way argv/argc works normally. But
then, if it actually used up everything, it wouldn't return 1, but
return 0 to make it
(a) easier and more obvious to test for "done" in general
(b) easier to convert existing users that basically expect a zero
return value to mean "I ate all the arguments". Many existing users
of "parse_options()" don't care about anything else, and wouldn't
need any changes at all.
but I haven't actually looked too closely yet to be able to say whether
this would avoid the bulk of the problems with changing the existing
practice of 'parse_option()' callers. ]
Hmm?
Linus
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Johannes Schindelin @ 2008-06-23 17:04 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Pierre Habouzit, Git Mailing List, Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806230912230.2926@woody.linux-foundation.org>
Hi,
On Mon, 23 Jun 2008, Linus Torvalds wrote:
> On Mon, 23 Jun 2008, Johannes Schindelin wrote:
> >
> > Thinking about the recursive approach again, I came up with this POC:
>
> "recursive" is pointless.
Nope, it is not.
To keep things _simple_ a callback is not good. Sure, you can work around
the limitations of callbacks for aggregation, but the code change looks
horrible. And the same holds true for the help message.
Just compare that to the recursive approach.
Okay, now for the "granted, your approach has merits" part.
> Look at cmd_apply() in builtin-apply.c. Notice how it currently
> absolutely CANNOT sanely be turned into using "parse_options()", not
> because it needs any "recursive" handling, but simply because it wants
> to do *incremental* handling.
That is a totally independent issue from the one I discussed, namely sane
handling of the diff (and rev) options.
> for (;;) {
> const char *arg;
> argc = parse_options(argc, argv,
> options, usage, PARSE_OPT_STOP_AT_UNKNOWN);
We do have PARSE_OPT_STOP_AT_NON_OPTION since a0ec9d25(parseopt: add flag
to stop on first non option), which tries to solve a _similar_ problem,
and it should be not hard at all to get PARSE_OPT_STOP_AT_UNKNOWN without
changing the loop as you suggested.
Heck, we could just as easily introduce PARSE_OPT_IGNORE_UNKNOWN.
> Could you handle the "recursive" use of parse_options() in builtin-blame.c
> by teaching it about recursion? Yes. But again, it's just _simpler_ to
> just teach parse_options() to parse the things it knows about, and leave
> the other things in place.
And here I disagree. You might not need a nice "--help" output, but most
mortals do. And this is not easy with your approach.
In contrast, by using my approach of having an option_table for a bundle
of common options, which just set variables in a certain struct, you can
have a relatively painless migration, and you get all the benefits of
parse-options.
But I guess the approach of whoever has more time to work on it will
win... ;-)
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC] Re: Convert 'git blame' to parse_options()
From: Jeff King @ 2008-06-23 16:49 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Pierre Habouzit, Git Mailing List,
Junio C Hamano
In-Reply-To: <alpine.LFD.1.10.0806230912230.2926@woody.linux-foundation.org>
On Mon, Jun 23, 2008 at 09:25:10AM -0700, Linus Torvalds wrote:
> Could you handle the "recursive" use of parse_options() in builtin-blame.c
> by teaching it about recursion? Yes. But again, it's just _simpler_ to
> just teach parse_options() to parse the things it knows about, and leave
> the other things in place.
If I know that I have option "-a", what is the correct partial parsing
of:
git foo -b -a
?
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox