From: Junio C Hamano <hidden> Date: 2016-06-15 22:45:05
Jeff King [off-list ref] writes:
quoted hunk
On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:
quoted
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.
@@ -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;-return0;+return1;}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.
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?
From: Pierre Habouzit <hidden> Date: 2016-06-15 22:45:05
On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:
quoted
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.
@@ -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;-return0;+return1;}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@debian.org
OOO http://www.madism.org
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.
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?
Not necessarily. The logic there goes:
1. it's not a revision option, so pass to diff
2. it's not a diff opt, so it is unknown; we parsed no options
3. the caller sees we failed to parse, so it barfs
but the logic here is:
1. it _is_ a revision option. Our handling of it is just a little odd,
in that we need to parse it later, when we are in setup_revisions.
So put it aside for now as a "revision" that just happens to look
like an option.
2. caller sees we parsed, and doesn't complain
3. caller later passes the "revision" to setup_revisions, which knows
what to do
Now my patch doesn't just operate on "--all", but rather several other
options including --no-walk. And maybe that is not right, and --all
should be handled separately (though I think --remotes would follow the
same logic). I'm not really sure why --no-walk is separated like that.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:45:05
On Thu, Jul 31, 2008 at 11:01:46AM +0200, Pierre Habouzit wrote:
Ack this is a correct fix. I wonder how this even works with other
commands that use --all and stuff like that.
--all is parsed separately in setup_revisions _before_ we call
handle_revision_opt. So the only codepath hitting that is parse_options
users who call parse_revision_opt (i.e., blame and shortlog).
-Peff
From: Pierre Habouzit <hidden> Date: 2016-06-15 22:45:05
On Thu, Jul 31, 2008 at 09:01:46AM +0000, Pierre Habouzit wrote:
On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote:
quoted
Jeff King [off-list ref] writes:
quoted
On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:
quoted
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.
@@ -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;-return0;+return1;}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.
Oh actually I know: the parse_revision_opt machinery (that is buggy
because of this 0 result) is used in git-blame where --all is
meaningless anyways, and ... git-shortlog only. The legacy way to parse
revision options does not treats `0' answers differently from `1'
actually, I wonder why, because this is probably wrong.
This was a regression, I suppose prior to its parse-optification
git-shortlog accepted --all (and I see no valid reason for it not to).
Thanks a lot to Jeff for the good catch.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
From: Pierre Habouzit <hidden> Date: 2016-06-15 22:45:05
On Thu, Jul 31, 2008 at 09:03:49AM +0000, Jeff King wrote:
Not necessarily. The logic there goes:
1. it's not a revision option, so pass to diff
2. it's not a diff opt, so it is unknown; we parsed no options
3. the caller sees we failed to parse, so it barfs
but the logic here is:
1. it _is_ a revision option. Our handling of it is just a little odd,
in that we need to parse it later, when we are in setup_revisions.
So put it aside for now as a "revision" that just happens to look
like an option.
2. caller sees we parsed, and doesn't complain
3. caller later passes the "revision" to setup_revisions, which knows
what to do
Now my patch doesn't just operate on "--all", but rather several other
options including --no-walk. And maybe that is not right, and --all
should be handled separately (though I think --remotes would follow the
same logic). I'm not really sure why --no-walk is separated like that.
--no-walk alters how add_pending_object_with_mode works, which is called
by handle_revision_arg. IOW, <ref1> --no-walk <ref2> is not the same as
--no-walk <ref1> <ref2>. IOW reference arguments and --no-walk ordering
must be preserved, IOW --no-walk is a pseudo-argument like --all --tags
or --remote are.
It's okay for parse_revision_opt to take out any option that doesn't
alter handle_revision_arg (as for parse_option based parsers it's the
sole thing setup_revision will be doing: consuming revisions in a loop
with handle_revision_arg), but only those.
The full list (that I made when I wrote parse_revision_opt) is actually:
pseudo arguments: --all --branches --tags --remotes --reflog
combining operator: --not
behavior modifying: --no-walk --do-walk
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
From: Jeff King <hidden> Date: 2016-06-15 22:45:05
[Pierre, you have the bogus MFT header that people are often complaining
about. Either I have to do extra work to fix the headers, or more people
end up in the To: field. I don't personally care about the latter, but
it seems that others organize their mail based on To versus Cc]
On Thu, Jul 31, 2008 at 11:15:15AM +0200, Pierre Habouzit wrote:
--no-walk alters how add_pending_object_with_mode works, which is called
by handle_revision_arg. IOW, <ref1> --no-walk <ref2> is not the same as
--no-walk <ref1> <ref2>. IOW reference arguments and --no-walk ordering
must be preserved, IOW --no-walk is a pseudo-argument like --all --tags
or --remote are.
Ah, OK. Then that makes sense, then. And I definitely feel that the
patch I posted is the right thing to do, then. So here is a patch with
commit message. The commit message to patch ratio is ridiculous, but
obviously this fix took some thinking, so it seems best to document it.
-- >8 --
allow "non-option" revision options in parse_option-enabled commands
Commands which use parse_options but also call
setup_revisions must do their parsing in a two step process:
1. first, they parse all options. Anything unknown goes to
parse_revision_opt (which calls handle_revision_opt),
which may claim the option or say "I don't recognize
this"
2. the non-option remainder goes to setup_revisions to
actually get turned into revisions
Some revision options are "non-options" in that they must be
parsed in order with their revision counterparts in
setup_revisions. For example, "--all" functions as a
pseudo-option expanding to all refs, and "--no-walk" affects
refs after it on the command line, but not before. The
revision option parser in step 1 recognizes such options and
sets them aside for later parsing by setup_revisions.
However, the return value used from handle_revision_opt
indicated "I didn't recognize this", which was wrong. It
did, and it took appropriate action (even though that action
was just deferring it for later parsing). Thus it should
return "yes, I recognized this."
Previously, these pseudo-options generated an error when
used with parse_options parsers (currently just blame and
shortlog). With this patch, they should work fine, enabling
things like "git shortlog --all".
Signed-off-by: Jeff King <redacted>
---
revision.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
From: Pierre Habouzit <hidden> Date: 2016-06-15 22:45:05
[Pierre, you have the bogus MFT header that people are often complaining
about. Either I have to do extra work to fix the headers, or more people
end up in the To: field. I don't personally care about the latter, but
it seems that others organize their mail based on To versus Cc]
[ err if you do a list reply, nobody ends up in To:... but oh well I
can probably remove it, it'll just make my own mutt git folder
awkwardly colorized. ]
On Thu, Jul 31, 2008 at 09:34:10AM +0000, Jeff King wrote:
On Thu, Jul 31, 2008 at 11:15:15AM +0200, Pierre Habouzit wrote:
quoted
--no-walk alters how add_pending_object_with_mode works, which is called
by handle_revision_arg. IOW, <ref1> --no-walk <ref2> is not the same as
--no-walk <ref1> <ref2>. IOW reference arguments and --no-walk ordering
must be preserved, IOW --no-walk is a pseudo-argument like --all --tags
or --remote are.
Ah, OK. Then that makes sense, then. And I definitely feel that the
patch I posted is the right thing to do,
Yeah I was convinced the first time I saw it already :)
-- >8 --
allow "non-option" revision options in parse_option-enabled commands
Commands which use parse_options but also call
setup_revisions must do their parsing in a two step process:
1. first, they parse all options. Anything unknown goes to
parse_revision_opt (which calls handle_revision_opt),
which may claim the option or say "I don't recognize
this"
2. the non-option remainder goes to setup_revisions to
actually get turned into revisions
Some revision options are "non-options" in that they must be
parsed in order with their revision counterparts in
setup_revisions. For example, "--all" functions as a
pseudo-option expanding to all refs, and "--no-walk" affects
refs after it on the command line, but not before. The
revision option parser in step 1 recognizes such options and
sets them aside for later parsing by setup_revisions.
However, the return value used from handle_revision_opt
indicated "I didn't recognize this", which was wrong. It
did, and it took appropriate action (even though that action
was just deferring it for later parsing). Thus it should
return "yes, I recognized this."
Previously, these pseudo-options generated an error when
used with parse_options parsers (currently just blame and
shortlog). With this patch, they should work fine, enabling
things like "git shortlog --all".
Signed-off-by: Jeff King <redacted>
Acked-By: Pierre Habouzit <redacted>
---
revision.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:45:05
On Thu, Jul 31, 2008 at 12:22:23PM +0200, Pierre Habouzit wrote:
quoted
[Pierre, you have the bogus MFT header that people are often complaining
about. Either I have to do extra work to fix the headers, or more people
end up in the To: field. I don't personally care about the latter, but
it seems that others organize their mail based on To versus Cc]
[ err if you do a list reply, nobody ends up in To:... but oh well I
can probably remove it, it'll just make my own mutt git folder
awkwardly colorized. ]
I can't do a list reply, as mutt sees no list (and I didn't bother
setting up a 'subscribe' variable, since I didn't want to generate MFTs
in the first place). However, I just realized that mutt has an
honor_followup_to, which will do what I want (AFAICT, there is no point
in honoring MFT when using the conventions of this mailing list).
But I wonder if it is as simple in other clients.
-Peff