{"thread":{"id":"36794","subject":"[puzzled and solved] \"shortlog\" not quite understanding all \"log\" options","startedAt":"2014-05-30T19:28:47Z","lastAt":"2014-06-18T08:08:48Z","messageCount":4,"participants":["Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"243040","messageId":"xmqqzjhz83rk.fsf@gitster.dls.corp.google.com","threadId":"36794","inReplyTo":null,"subject":"[puzzled and solved] \"shortlog\" not quite understanding all \"log\" options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-30T19:28:47Z","receivedAt":"2014-05-30T19:28:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This gives the list of all recent commits but not the ones that are\nonly reachable from the notes hierarchy:\n\n$ git log --exclude=refs/notes/\\* --all --no-merges --since=18.hours |\n git shortlog\n\nand we are supposed to be able to write the same with shortlog\nitself, i.e.\n\n$ git shortlog --exclude=refs/notes/\\* --all --no-merges --since=18.hours\n\nbut this does not understand the ref exclusion command-line option.\n\nA funny and puzzling thing is that this is understood:\n\n$ git shortlog --all --no-merges --since=18.hours\n\nWhat is strange is that --all, --no-merges, etc. and the ref\nexclusion option all are handled in revision.c and only one of them\nis being rejected.\n\n.... Aaaand, it turns out that the answer is in the big comment at\nthe beginning of handle_revision_pseudo_opt().\n\n-- >8 --\nSubject: shortlog: allow --exclude= to be passed\n\ne7b432c5 (revision: introduce --exclude= to tame wildcards,\n2013-08-30) taught a new option to the command-line parser of \"log\"\nand friends, but did not wire it fully so that it can also be used\nby \"shortlog\".\n\nSigned-off-by: Junio C Hamano \n---\n revision.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/revision.c b/revision.c\nindex 71e2337..3818b46 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1633,6 +1633,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t !strcmp(arg, \"--reflog\") || !strcmp(arg, \"--not\") ||\n \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\") ||\n \t !strcmp(arg, \"--bisect\") || starts_with(arg, \"--glob=\") ||\n+\t starts_with(arg, \"--exclude=\") ||\n \t starts_with(arg, \"--branches=\") || starts_with(arg, \"--tags=\") ||\n \t starts_with(arg, \"--remotes=\") || starts_with(arg, \"--no-walk=\"))\n \t{\n"},{"id":"243046","messageId":"20140530201652.GC5513@sigill.intra.peff.net","threadId":"36794","inReplyTo":"xmqqzjhz83rk.fsf@gitster.dls.corp.google.com","subject":"Re: [puzzled and solved] \"shortlog\" not quite understanding all \"log\" options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-30T20:16:53Z","receivedAt":"2014-05-30T20:16:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 30, 2014 at 12:28:47PM -0700, Junio C Hamano wrote:\n\n> .... Aaaand, it turns out that the answer is in the big comment at\n> the beginning of handle_revision_pseudo_opt().\n> \n> -- >8 --\n> Subject: shortlog: allow --exclude= to be passed\n> \n> e7b432c5 (revision: introduce --exclude= to tame wildcards,\n> 2013-08-30) taught a new option to the command-line parser of \"log\"\n> and friends, but did not wire it fully so that it can also be used\n> by \"shortlog\".\n\nFWIW, I think the discussion above the scissors adds a lot to the\ncontext. It might be nice to add it to the commit message.\n\nI am slightly puzzled why parse_revision_opt does not just call\nhandle_revision_pseudo_opt. According to f6aca0dc4, it is because\npseudo-options need to be acted on in-order, as they affect things like\nsubsequent \"--not\" options, etc. But if we are using parse_options_step,\nshouldn't we be handling the options in order?\n\nI am sure I am just missing something obvious, so do not trouble\nyourself if you do not know the answer offhand.\n\n-Peff\n"},{"id":"243059","messageId":"xmqqwqd36j9d.fsf@gitster.dls.corp.google.com","threadId":"36794","inReplyTo":"20140530201652.GC5513@sigill.intra.peff.net","subject":"Re: [puzzled and solved] \"shortlog\" not quite understanding all \"log\" options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-30T21:37:02Z","receivedAt":"2014-05-30T21:37:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, May 30, 2014 at 12:28:47PM -0700, Junio C Hamano wrote:\n>\n>> .... Aaaand, it turns out that the answer is in the big comment at\n>> the beginning of handle_revision_pseudo_opt().\n>> \n>> -- >8 --\n>> Subject: shortlog: allow --exclude= to be passed\n>> \n>> e7b432c5 (revision: introduce --exclude= to tame wildcards,\n>> 2013-08-30) taught a new option to the command-line parser of \"log\"\n>> and friends, but did not wire it fully so that it can also be used\n>> by \"shortlog\".\n>\n> FWIW, I think the discussion above the scissors adds a lot to the\n> context. It might be nice to add it to the commit message.\n\nOK. Try to remember when I reroll it.\n\n> I am slightly puzzled why parse_revision_opt does not just call\n> handle_revision_pseudo_opt. According to f6aca0dc4, it is because\n> pseudo-options need to be acted on in-order, as they affect things like\n> subsequent \"--not\" options, etc. But if we are using parse_options_step,\n> shouldn't we be handling the options in order?\n>\n> I am sure I am just missing something obvious, so do not trouble\n> yourself if you do not know the answer offhand.\n\nSorry, I don't know ;-)\n"},{"id":"244507","messageId":"20140618080848.GB11027@sigill.intra.peff.net","threadId":"36794","inReplyTo":"xmqqwqd36j9d.fsf@gitster.dls.corp.google.com","subject":"Re: [puzzled and solved] \"shortlog\" not quite understanding all \"log\" options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T08:08:48Z","receivedAt":"2014-06-18T08:08:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 30, 2014 at 02:37:02PM -0700, Junio C Hamano wrote:\n\n> > I am slightly puzzled why parse_revision_opt does not just call\n> > handle_revision_pseudo_opt. According to f6aca0dc4, it is because\n> > pseudo-options need to be acted on in-order, as they affect things like\n> > subsequent \"--not\" options, etc. But if we are using parse_options_step,\n> > shouldn't we be handling the options in order?\n> >\n> > I am sure I am just missing something obvious, so do not trouble\n> > yourself if you do not know the answer offhand.\n> \n> Sorry, I don't know ;-)\n\nHopefully I am not wasting your time by responding to an old thread, but\nI figured this out and wanted to post it for posterity.\n\nThe answer is that it is not about handling _options_ in order, but that\nwe need to handle pseudo-options in order with non-options, like:\n\n foo --not bar\n\nStepping through the options with parseopt will just cover dashed\noptions, but we handle non-option arguments later. So we have to handle\nthe pseudo-arguments like \"--not\" at the same later time.\n\nSo there's nothing interesting to clean up or fix here.\n\n-Peff\n"}]}