* git blame not respecting --find-copies-harder ?
@ 2008-07-30 9:39 Stephen R. van den Berg
2008-07-30 15:01 ` Björn Steinbrink
0 siblings, 1 reply; 17+ messages in thread
From: Stephen R. van den Berg @ 2008-07-30 9:39 UTC (permalink / raw)
To: Git Mailinglist
git clone git://git.cuci.nl/pike
Both of this "git blame" commands return the same (erroneous) results
at the end of the files (the last lines are older, and are from the old
README file next to it).
git blame README-CVS
git blame --find-copies-harder README-CVS
However, when using git show with and without --find-copies-harder, we
see that git indeed finds the copy with a 76% similarity index.
git show 9eb55816
git show --find-copies-harder 9eb55816
Shouldn't it be expected that blame should also give the same difference
in result with and without that option?
On a related note, it doesn't seem possible to make --find-copies-harder
a default; is that intentional?
--
Sincerely,
Stephen R. van den Berg.
How many weeks are there in a lightyear?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-30 9:39 git blame not respecting --find-copies-harder ? Stephen R. van den Berg
@ 2008-07-30 15:01 ` Björn Steinbrink
2008-07-30 15:43 ` Sverre Rabbelier
0 siblings, 1 reply; 17+ messages in thread
From: Björn Steinbrink @ 2008-07-30 15:01 UTC (permalink / raw)
To: Stephen R. van den Berg; +Cc: Git Mailinglist
On 2008.07.30 11:39:03 +0200, Stephen R. van den Berg wrote:
> git clone git://git.cuci.nl/pike
>
> Both of this "git blame" commands return the same (erroneous) results
> at the end of the files (the last lines are older, and are from the old
> README file next to it).
>
> git blame README-CVS
> git blame --find-copies-harder README-CVS
git blame doesn't know --find-copies-harder, it's -C -C for blame.
Björn
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-30 15:01 ` Björn Steinbrink
@ 2008-07-30 15:43 ` Sverre Rabbelier
2008-07-31 6:48 ` Jeff King
0 siblings, 1 reply; 17+ messages in thread
From: Sverre Rabbelier @ 2008-07-30 15:43 UTC (permalink / raw)
To: Björn Steinbrink; +Cc: Stephen R. van den Berg, Git Mailinglist
On Wed, Jul 30, 2008 at 17:01, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:
> git blame doesn't know --find-copies-harder, it's -C -C for blame.
Shouldn't it have died with "don't know option --find-copies-harder" then?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-30 15:43 ` Sverre Rabbelier
@ 2008-07-31 6:48 ` Jeff King
2008-07-31 7:05 ` Junio C Hamano
0 siblings, 1 reply; 17+ messages in thread
From: Jeff King @ 2008-07-31 6:48 UTC (permalink / raw)
To: sverre; +Cc: Björn Steinbrink, Stephen R. van den Berg, Git Mailinglist
On Wed, Jul 30, 2008 at 05:43:52PM +0200, Sverre Rabbelier wrote:
> On Wed, Jul 30, 2008 at 17:01, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:
> > git blame doesn't know --find-copies-harder, it's -C -C for blame.
>
> Shouldn't it have died with "don't know option --find-copies-harder" then?
Unfortunately, it _does_ know --find-copies-harder, because unknown
options get sent to the revision option parser, which chains to the diff
option parser. So it recognizes --find-copies-harder, but just sets a
flag that doesn't do what we expect.
I'm not sure if there is a simple fix. Does blame actually need the diff
option parsing? If not, then we might be able to pass a flag to
parse_revision_opt that says "don't do diff options, too".
-Peff
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 6:48 ` Jeff King
@ 2008-07-31 7:05 ` Junio C Hamano
2008-07-31 7:21 ` Jeff King
0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2008-07-31 7:05 UTC (permalink / raw)
To: Jeff King
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
Jeff King <peff@peff.net> writes:
> On Wed, Jul 30, 2008 at 05:43:52PM +0200, Sverre Rabbelier wrote:
>
>> On Wed, Jul 30, 2008 at 17:01, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:
>> > git blame doesn't know --find-copies-harder, it's -C -C for blame.
>>
>> Shouldn't it have died with "don't know option --find-copies-harder" then?
>
> Unfortunately, it _does_ know --find-copies-harder, because unknown
> options get sent to the revision option parser, which chains to the diff
> option parser. So it recognizes --find-copies-harder, but just sets a
> flag that doesn't do what we expect.
>
> I'm not sure if there is a simple fix. Does blame actually need the diff
> option parsing? If not, then we might be able to pass a flag to
> parse_revision_opt that says "don't do diff options, too".
Sigh...
We can probably pick up the result revision parser parsed out of
revs.diffopt, and then tweak "opt" with it, perhaps like this.
builtin-blame.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/builtin-blame.c b/builtin-blame.c
index 8b6b09b..4ea3431 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2346,6 +2346,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
parse_done:
argc = parse_options_end(&ctx);
+ if (DIFF_OPT_TST(&revs.diffopt, FIND_COPIES_HARDER))
+ opt |= (PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE |
+ PICKAXE_BLAME_COPY_HARDER);
+
if (!blame_move_score)
blame_move_score = BLAME_DEFAULT_MOVE_SCORE;
if (!blame_copy_score)
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 7:05 ` Junio C Hamano
@ 2008-07-31 7:21 ` Jeff King
2008-07-31 7:36 ` Junio C Hamano
0 siblings, 1 reply; 17+ messages in thread
From: Jeff King @ 2008-07-31 7:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
On Thu, Jul 31, 2008 at 12:05:22AM -0700, Junio C Hamano wrote:
> We can probably pick up the result revision parser parsed out of
> revs.diffopt, and then tweak "opt" with it, perhaps like this.
That is a sensible solution for this option, but I have to wonder: how
many other such ineffective options are hiding? How many of them
actually have a matching meaning in git-blame? E.g., what does "git
blame --name-only" mean?
Perhaps we should simply not worry about those ones, as people are
unlikely to try using them, and it is easy to say "has no impact,
because it doesn't make sense with blame." The truly confusing ones are
ones you _expect_ to do something, but don't (like
--find-copies-harder).
I took a look at implementing a "don't parse the diff options" flag, but
it is much larger than that. The revision parser understands a lot of
options that don't really make sense for blame (or shortlog), like
"--full-diff". So perhaps it is best to just fix this one (which we have
actually had a bug report about) and not worry about the rest.
-Peff
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 7:21 ` Jeff King
@ 2008-07-31 7:36 ` Junio C Hamano
2008-07-31 8:25 ` Jeff King
0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2008-07-31 7:36 UTC (permalink / raw)
To: Jeff King
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
Jeff King <peff@peff.net> writes:
> I took a look at implementing a "don't parse the diff options" flag, but
> it is much larger than that. The revision parser understands a lot of
> options that don't really make sense for blame (or shortlog), like
> "--full-diff". So perhaps it is best to just fix this one (which we have
> actually had a bug report about) and not worry about the rest.
That reminds me of an issue with shortlog.
I often wish to do this:
git shortlog --since=3.day --all | sort | uniq -c
This is to catch a stupid mistake of (1) applying a few patches to
'master', (2) forking a new topic from 'master' and applying a few patches
there, (3) realizing a few commits on 'master' that haven't been pushed
out was faulty and rewrite the 'master' history. Such a new topic made in
step (2) must be rebased on the updated 'master' built in step (3);
otherwise merging the topic to 'next' will contaminate it with the old
version of patches that have been rewritten on 'master'.
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 7:36 ` Junio C Hamano
@ 2008-07-31 8:25 ` Jeff King
2008-07-31 8:35 ` Junio C Hamano
0 siblings, 1 reply; 17+ messages in thread
From: Jeff King @ 2008-07-31 8:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
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.
-Peff
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 8:25 ` Jeff King
@ 2008-07-31 8:35 ` Junio C Hamano
2008-07-31 9:01 ` Pierre Habouzit
2008-07-31 9:03 ` Jeff King
0 siblings, 2 replies; 17+ messages in thread
From: Junio C Hamano @ 2008-07-31 8:35 UTC (permalink / raw)
To: Jeff King
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
Jeff King <peff@peff.net> 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.
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?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 8:35 ` Junio C Hamano
@ 2008-07-31 9:01 ` Pierre Habouzit
2008-07-31 9:05 ` Jeff King
2008-07-31 9:06 ` Pierre Habouzit
2008-07-31 9:03 ` Jeff King
1 sibling, 2 replies; 17+ messages in thread
From: Pierre Habouzit @ 2008-07-31 9:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
[-- Attachment #1: Type: text/plain, Size: 3052 bytes --]
On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote:
> Jeff King <peff@peff.net> 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@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 8:35 ` Junio C Hamano
2008-07-31 9:01 ` Pierre Habouzit
@ 2008-07-31 9:03 ` Jeff King
2008-07-31 9:15 ` Pierre Habouzit
1 sibling, 1 reply; 17+ messages in thread
From: Jeff King @ 2008-07-31 9:03 UTC (permalink / raw)
To: Junio C Hamano
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
On Thu, Jul 31, 2008 at 01:35:33AM -0700, Junio C Hamano wrote:
> > @@ -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.
>
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 9:01 ` Pierre Habouzit
@ 2008-07-31 9:05 ` Jeff King
2008-07-31 9:06 ` Pierre Habouzit
1 sibling, 0 replies; 17+ messages in thread
From: Jeff King @ 2008-07-31 9:05 UTC (permalink / raw)
To: Pierre Habouzit, Junio C Hamano, sverre, Björn Steinbrink,
Stephen R. van den Berg
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 9:01 ` Pierre Habouzit
2008-07-31 9:05 ` Jeff King
@ 2008-07-31 9:06 ` Pierre Habouzit
1 sibling, 0 replies; 17+ messages in thread
From: Pierre Habouzit @ 2008-07-31 9:06 UTC (permalink / raw)
To: Junio C Hamano, Jeff King, sverre, Björn Steinbrink,
Stephen R. van den Berg
[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]
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:
> > Jeff King <peff@peff.net> 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.
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
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 9:03 ` Jeff King
@ 2008-07-31 9:15 ` Pierre Habouzit
2008-07-31 9:34 ` Jeff King
0 siblings, 1 reply; 17+ messages in thread
From: Pierre Habouzit @ 2008-07-31 9:15 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, sverre, Björn Steinbrink,
Stephen R. van den Berg, Git Mailinglist
[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 9:15 ` Pierre Habouzit
@ 2008-07-31 9:34 ` Jeff King
2008-07-31 10:22 ` Pierre Habouzit
0 siblings, 1 reply; 17+ messages in thread
From: Jeff King @ 2008-07-31 9:34 UTC (permalink / raw)
To: Pierre Habouzit
Cc: Junio C Hamano, sverre, Björn Steinbrink,
Stephen R. van den Berg, Git Mailinglist
[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 <peff@peff.net>
---
revision.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
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=")) {
--
1.6.0.rc1.197.gc57ac6
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 9:34 ` Jeff King
@ 2008-07-31 10:22 ` Pierre Habouzit
2008-07-31 10:33 ` Jeff King
0 siblings, 1 reply; 17+ messages in thread
From: Pierre Habouzit @ 2008-07-31 10:22 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, sverre, Björn Steinbrink,
Stephen R. van den Berg, Git Mailinglist
[-- Attachment #1: Type: text/plain, Size: 3074 bytes --]
> [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:
> > --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 <peff@peff.net>
Acked-By: Pierre Habouzit <madcoder@debian.org>
---
revision.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
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=")) {
--
1.6.0.rc1.197.gc57ac6
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: git blame not respecting --find-copies-harder ?
2008-07-31 10:22 ` Pierre Habouzit
@ 2008-07-31 10:33 ` Jeff King
0 siblings, 0 replies; 17+ messages in thread
From: Jeff King @ 2008-07-31 10:33 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, Git Mailinglist
On Thu, Jul 31, 2008 at 12:22:23PM +0200, Pierre Habouzit wrote:
> > [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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2008-07-31 10:35 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-07-30 9:39 git blame not respecting --find-copies-harder ? Stephen R. van den Berg
2008-07-30 15:01 ` Björn Steinbrink
2008-07-30 15:43 ` Sverre Rabbelier
2008-07-31 6:48 ` Jeff King
2008-07-31 7:05 ` Junio C Hamano
2008-07-31 7:21 ` Jeff King
2008-07-31 7:36 ` Junio C Hamano
2008-07-31 8:25 ` Jeff King
2008-07-31 8:35 ` Junio C Hamano
2008-07-31 9:01 ` Pierre Habouzit
2008-07-31 9:05 ` Jeff King
2008-07-31 9:06 ` Pierre Habouzit
2008-07-31 9:03 ` Jeff King
2008-07-31 9:15 ` Pierre Habouzit
2008-07-31 9:34 ` Jeff King
2008-07-31 10:22 ` Pierre Habouzit
2008-07-31 10:33 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).