threads / discuss / 36794

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

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

## tl;dr

4 messages between May 30, 2014 and Jun 18, 2014.

replies: 3people: 2as markdown or json

Junio C Hamano· May 30, 2014, 19:28 UTC · lore

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="))
 	{

Jeff King· May 30, 2014, 20:16 UTC · re: Junio C Hamano · lore

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

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

Show 10 quoted lines

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

Junio C Hamano· May 30, 2014, 21:37 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 15 quoted lines

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

Show 8 quoted lines

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

Jeff King· Jun 18, 2014, 08:08 UTC · re: Junio C Hamano · lore

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

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

Show 10 quoted lines

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