On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:
> >
> >> Alas, shortlog does not take --all. Yes, I know
> >>
> >> git log --since=3.day --all | git shortlog | sort | uniq -c
> >>
> >> is an obvious workaround, but it is mildly irritating.
> >
> > Hmm. Could it be as simple as:
> >
> > diff --git a/revision.c b/revision.c
> > index a843c42..eaa5572 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> > !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk"))
> > {
> > unkv[(*unkc)++] = arg;
> > - return 0;
> > + return 1;
> > }
> >
> > if (!prefixcmp(arg, "--max-count=")) {
> >
> > That is, handle_revision_opt says "yes we parsed this, and it should be
> > gone" even though it still gets stuck in the "unknown" section to be
> > parsed by setup_revisions later.
Ack this is a correct fix. I wonder how this even works with other
commands that use --all and stuff like that.
> Hmm, wouldn't that suggest it needs to return 1 when an option candidate
> given to diff_opt_parse() turns out not to be a diff option and stuffed
> back to unkv[] at the end of this function?
No, because diff-opts are our last chance, and those are _really_
unknown options. "--all" is different because revision machinery
acknowledge "yeah it's really for us, we recognize it ..." hence the 1
result "... but we have to parse it later so keep it in place".
There is no such thing with unknown option for the revision _and_ diff
machinery. They _must_ return 0 to say "no we really didn't know what
this is".
IOW, semantics is:
< 0 : error
0 : option was not directed at us, unknown (this allow to chain
option parsers.
> 0 : yeah this was for us, and we consumed this amount of arguments
in the process.
Here "--all" is clearly something that we _recognized_ hence fit in
the third case, even if we decide to still keep it in the "unk" array
(which is ill named, I kept its name, but it's not unknown options, it's
actually non option arguments that are to be deal with in the last
stage, setup_revisions here, or the command as a general rule).
Unknown options to both the revision _and_ diff option machineries are
clearly of the 2nd kind and must return 0. They put the option in 'unk'
as well (and here this is properly named) because this is how it
historically worked for many commands, and is here as a legacy
compatibility thing. parse-opt based parser (and git-shortlog has one)
does not uses that (and you can see it in parse_revision_opt that is
what such parser use: a result of `0' triggers a usage).
--
·O· Pierre Habouzit
··O madcoder@xxxxxxxxxx
OOO http://www.madism.org
Attachment:
pgpNu7IJVYeJk.pgp
Description: PGP signature
