[puzzled and solved] "shortlog" not quite understanding all "log" options

[puzzled and solved] "shortlog" not quite understanding all "log" options

[Junio C Hamano]

This gives the list of all recent commits but not the ones that are
only reachable from the notes hierarchy:

$ git log --exclude=refs/notes/\* --all --no-merges --since=18.hours |
  git shortlog

and we are supposed to be able to write the same with shortlog
itself, i.e.

$ git shortlog --exclude=refs/notes/\* --all --no-merges --since=18.hours

but this does not understand the ref exclusion command-line option.

A funny and puzzling thing is that this is understood:

$ git shortlog --all --no-merges --since=18.hours

What is strange is that --all, --no-merges, etc. and the ref
exclusion option all are handled in revision.c and only one of them
is being rejected.

.... Aaaand, it turns out that the answer is in the big comment at
the beginning of handle_revision_pseudo_opt().

-- >8 --
Subject: shortlog: allow --exclude=<glob> to be passed

e7b432c5 (revision: introduce --exclude=<glob> to tame wildcards,
2013-08-30) taught a new option to the command-line parser of "log"
and friends, but did not wire it fully so that it can also be used
by "shortlog".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/revision.c b/revision.c
index 71e2337..3818b46 100644
--- a/revision.c
+++ b/revision.c
@@ -1633,6 +1633,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	    !strcmp(arg, "--reflog") || !strcmp(arg, "--not") ||
 	    !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk") ||
 	    !strcmp(arg, "--bisect") || starts_with(arg, "--glob=") ||
+	    starts_with(arg, "--exclude=") ||
 	    starts_with(arg, "--branches=") || starts_with(arg, "--tags=") ||
 	    starts_with(arg, "--remotes=") || starts_with(arg, "--no-walk="))
 	{

Re: [puzzled and solved] "shortlog" not quite understanding all "log" options

[Jeff King]

On Fri, May 30, 2014 at 12:28:47PM -0700, Junio C Hamano wrote:

> .... Aaaand, it turns out that the answer is in the big comment at
> the beginning of handle_revision_pseudo_opt().
> 
> -- >8 --
> Subject: shortlog: allow --exclude=<glob> to be passed
> 
> e7b432c5 (revision: introduce --exclude=<glob> to tame wildcards,
> 2013-08-30) taught a new option to the command-line parser of "log"
> and friends, but did not wire it fully so that it can also be used
> by "shortlog".

FWIW, I think the discussion above the scissors adds a lot to the
context. It might be nice to add it to the commit message.

I am slightly puzzled why parse_revision_opt does not just call
handle_revision_pseudo_opt. According to f6aca0dc4, it is because
pseudo-options need to be acted on in-order, as they affect things like
subsequent "--not" options, etc. But if we are using parse_options_step,
shouldn't we be handling the options in order?

I am sure I am just missing something obvious, so do not trouble
yourself if you do not know the answer offhand.

-Peff

Re: [puzzled and solved] "shortlog" not quite understanding all "log" options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, May 30, 2014 at 12:28:47PM -0700, Junio C Hamano wrote:
>
>> .... Aaaand, it turns out that the answer is in the big comment at
>> the beginning of handle_revision_pseudo_opt().
>> 
>> -- >8 --
>> Subject: shortlog: allow --exclude=<glob> to be passed
>> 
>> e7b432c5 (revision: introduce --exclude=<glob> to tame wildcards,
>> 2013-08-30) taught a new option to the command-line parser of "log"
>> and friends, but did not wire it fully so that it can also be used
>> by "shortlog".
>
> FWIW, I think the discussion above the scissors adds a lot to the
> context. It might be nice to add it to the commit message.

OK.  Try to remember when I reroll it.

> I am slightly puzzled why parse_revision_opt does not just call
> handle_revision_pseudo_opt. According to f6aca0dc4, it is because
> pseudo-options need to be acted on in-order, as they affect things like
> subsequent "--not" options, etc. But if we are using parse_options_step,
> shouldn't we be handling the options in order?
>
> I am sure I am just missing something obvious, so do not trouble
> yourself if you do not know the answer offhand.

Sorry, I don't know ;-)

Re: [puzzled and solved] "shortlog" not quite understanding all "log" options

[Jeff King]

On Fri, May 30, 2014 at 02:37:02PM -0700, Junio C Hamano wrote:

> > I am slightly puzzled why parse_revision_opt does not just call
> > handle_revision_pseudo_opt. According to f6aca0dc4, it is because
> > pseudo-options need to be acted on in-order, as they affect things like
> > subsequent "--not" options, etc. But if we are using parse_options_step,
> > shouldn't we be handling the options in order?
> >
> > I am sure I am just missing something obvious, so do not trouble
> > yourself if you do not know the answer offhand.
> 
> Sorry, I don't know ;-)

Hopefully I am not wasting your time by responding to an old thread, but
I figured this out and wanted to post it for posterity.

The answer is that it is not about handling _options_ in order, but that
we need to handle pseudo-options in order with non-options, like:

  foo --not bar

Stepping through the options with parseopt will just cover dashed
options, but we handle non-option arguments later. So we have to handle
the pseudo-arguments like "--not" at the same later time.

So there's nothing interesting to clean up or fix here.

-Peff