{"thread":{"id":"14755","subject":"git blame not respecting --find-copies-harder ?","startedAt":"2008-07-30T09:39:03Z","lastAt":"2008-07-31T10:33:52Z","messageCount":17,"participants":["Stephen R. van den Berg","Björn Steinbrink","Sverre Rabbelier","Jeff King","Junio C Hamano","Pierre Habouzit"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"85594","messageId":"20080730093903.GA14330@cuci.nl","threadId":"14755","inReplyTo":null,"subject":"git blame not respecting --find-copies-harder ?","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-07-30T09:39:03Z","receivedAt":"2008-07-30T09:39:03Z","isPatch":false,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"git clone git://git.cuci.nl/pike\n\nBoth of this \"git blame\" commands return the same (erroneous) results\nat the end of the files (the last lines are older, and are from the old\nREADME file next to it).\n\ngit blame README-CVS\ngit blame --find-copies-harder README-CVS\n\nHowever, when using git show with and without --find-copies-harder, we\nsee that git indeed finds the copy with a 76% similarity index.\n\ngit show 9eb55816\ngit show --find-copies-harder 9eb55816\n\nShouldn't it be expected that blame should also give the same difference\nin result with and without that option?\n\nOn a related note, it doesn't seem possible to make --find-copies-harder\na default; is that intentional?\n-- \nSincerely,\n Stephen R. van den Berg.\n\nHow many weeks are there in a lightyear?\n"},{"id":"85633","messageId":"20080730150123.GB9758@atjola.homenet","threadId":"14755","inReplyTo":"20080730093903.GA14330@cuci.nl","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2008-07-30T15:01:23Z","receivedAt":"2008-07-30T15:01:23Z","isPatch":false,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2008.07.30 11:39:03 +0200, Stephen R. van den Berg wrote:\n> git clone git://git.cuci.nl/pike\n> \n> Both of this \"git blame\" commands return the same (erroneous) results\n> at the end of the files (the last lines are older, and are from the old\n> README file next to it).\n> \n> git blame README-CVS\n> git blame --find-copies-harder README-CVS\n\ngit blame doesn't know --find-copies-harder, it's -C -C for blame.\n\nBjörn\n"},{"id":"85641","messageId":"bd6139dc0807300843l1d42d6fep95f6c99fe6e0ea0@mail.gmail.com","threadId":"14755","inReplyTo":"20080730150123.GB9758@atjola.homenet","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Sverre Rabbelier","fromEmail":"alturin@gmail.com","sentAt":"2008-07-30T15:43:52Z","receivedAt":"2008-07-30T15:43:52Z","isPatch":false,"sender":{"key":"alturin@gmail.com","avatar":null},"body":"On Wed, Jul 30, 2008 at 17:01, Björn Steinbrink wrote:\n> git blame doesn't know --find-copies-harder, it's -C -C for blame.\n\nShouldn't it have died with \"don't know option --find-copies-harder\" then?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"85732","messageId":"20080731064814.GA32431@sigill.intra.peff.net","threadId":"14755","inReplyTo":"bd6139dc0807300843l1d42d6fep95f6c99fe6e0ea0@mail.gmail.com","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T06:48:15Z","receivedAt":"2008-07-31T06:48:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 30, 2008 at 05:43:52PM +0200, Sverre Rabbelier wrote:\n\n> On Wed, Jul 30, 2008 at 17:01, Björn Steinbrink wrote:\n> > git blame doesn't know --find-copies-harder, it's -C -C for blame.\n> \n> Shouldn't it have died with \"don't know option --find-copies-harder\" then?\n\nUnfortunately, it _does_ know --find-copies-harder, because unknown\noptions get sent to the revision option parser, which chains to the diff\noption parser. So it recognizes --find-copies-harder, but just sets a\nflag that doesn't do what we expect.\n\nI'm not sure if there is a simple fix. Does blame actually need the diff\noption parsing? If not, then we might be able to pass a flag to\nparse_revision_opt that says \"don't do diff options, too\".\n\n-Peff\n"},{"id":"85734","messageId":"7vfxpq3559.fsf@gitster.siamese.dyndns.org","threadId":"14755","inReplyTo":"20080731064814.GA32431@sigill.intra.peff.net","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-31T07:05:22Z","receivedAt":"2008-07-31T07:05:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jul 30, 2008 at 05:43:52PM +0200, Sverre Rabbelier wrote:\n>\n>> On Wed, Jul 30, 2008 at 17:01, Björn Steinbrink wrote:\n>> > git blame doesn't know --find-copies-harder, it's -C -C for blame.\n>> \n>> Shouldn't it have died with \"don't know option --find-copies-harder\" then?\n>\n> Unfortunately, it _does_ know --find-copies-harder, because unknown\n> options get sent to the revision option parser, which chains to the diff\n> option parser. So it recognizes --find-copies-harder, but just sets a\n> flag that doesn't do what we expect.\n>\n> I'm not sure if there is a simple fix. Does blame actually need the diff\n> option parsing? If not, then we might be able to pass a flag to\n> parse_revision_opt that says \"don't do diff options, too\".\n\nSigh...\n\nWe can probably pick up the result revision parser parsed out of\nrevs.diffopt, and then tweak \"opt\" with it, perhaps like this.\n\n builtin-blame.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 8b6b09b..4ea3431 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2346,6 +2346,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n parse_done:\n \targc = parse_options_end(&ctx);\n \n+\tif (DIFF_OPT_TST(&revs.diffopt, FIND_COPIES_HARDER))\n+\t\topt |= (PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE |\n+\t\t\tPICKAXE_BLAME_COPY_HARDER);\n+\n \tif (!blame_move_score)\n \t\tblame_move_score = BLAME_DEFAULT_MOVE_SCORE;\n \tif (!blame_copy_score)\n"},{"id":"85737","messageId":"20080731072149.GA2304@sigill.intra.peff.net","threadId":"14755","inReplyTo":"7vfxpq3559.fsf@gitster.siamese.dyndns.org","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T07:21:49Z","receivedAt":"2008-07-31T07:21:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2008 at 12:05:22AM -0700, Junio C Hamano wrote:\n\n> We can probably pick up the result revision parser parsed out of\n> revs.diffopt, and then tweak \"opt\" with it, perhaps like this.\n\nThat is a sensible solution for this option, but I have to wonder: how\nmany other such ineffective options are hiding? How many of them\nactually have a matching meaning in git-blame? E.g., what does \"git\nblame --name-only\" mean?\n\nPerhaps we should simply not worry about those ones, as people are\nunlikely to try using them, and it is easy to say \"has no impact,\nbecause it doesn't make sense with blame.\" The truly confusing ones are\nones you _expect_ to do something, but don't (like\n--find-copies-harder).\n\nI took a look at implementing a \"don't parse the diff options\" flag, but\nit is much larger than that. The revision parser understands a lot of\noptions that don't really make sense for blame (or shortlog), like\n\"--full-diff\". So perhaps it is best to just fix this one (which we have\nactually had a bug report about) and not worry about the rest.\n\n-Peff\n"},{"id":"85741","messageId":"7v8wvi33ok.fsf@gitster.siamese.dyndns.org","threadId":"14755","inReplyTo":"20080731072149.GA2304@sigill.intra.peff.net","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-31T07:36:59Z","receivedAt":"2008-07-31T07:36:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I took a look at implementing a \"don't parse the diff options\" flag, but\n> it is much larger than that. The revision parser understands a lot of\n> options that don't really make sense for blame (or shortlog), like\n> \"--full-diff\". So perhaps it is best to just fix this one (which we have\n> actually had a bug report about) and not worry about the rest.\n\nThat reminds me of an issue with shortlog.\n\nI often wish to do this:\n\n\tgit shortlog --since=3.day --all | sort | uniq -c\n\nThis is to catch a stupid mistake of (1) applying a few patches to\n'master', (2) forking a new topic from 'master' and applying a few patches\nthere, (3) realizing a few commits on 'master' that haven't been pushed\nout was faulty and rewrite the 'master' history. Such a new topic made in\nstep (2) must be rebased on the updated 'master' built in step (3);\notherwise merging the topic to 'next' will contaminate it with the old\nversion of patches that have been rewritten on 'master'.\n\nAlas, shortlog does not take --all. Yes, I know\n\n\tgit log --since=3.day --all | git shortlog | sort | uniq -c\n\nis an obvious workaround, but it is mildly irritating.\n"},{"id":"85744","messageId":"20080731082553.GA19522@sigill.intra.peff.net","threadId":"14755","inReplyTo":"7v8wvi33ok.fsf@gitster.siamese.dyndns.org","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T08:25:54Z","receivedAt":"2008-07-31T08:25:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:\n\n> Alas, shortlog does not take --all. Yes, I know\n> \n> \tgit log --since=3.day --all | git shortlog | sort | uniq -c\n> \n> is an obvious workaround, but it is mildly irritating.\n\nHmm. Could it be as simple as:\n\ndiff --git a/revision.c b/revision.c\nindex a843c42..eaa5572 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\"))\n \t{\n \t\tunkv[(*unkc)++] = arg;\n-\t\treturn 0;\n+\t\treturn 1;\n \t}\n \n \tif (!prefixcmp(arg, \"--max-count=\")) {\n\nThat is, handle_revision_opt says \"yes we parsed this, and it should be\ngone\" even though it still gets stuck in the \"unknown\" section to be\nparsed by setup_revisions later.\n\n-Peff\n"},{"id":"85745","messageId":"7v8wvizc16.fsf@gitster.siamese.dyndns.org","threadId":"14755","inReplyTo":"20080731082553.GA19522@sigill.intra.peff.net","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-31T08:35:33Z","receivedAt":"2008-07-31T08:35:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:\n>\n>> Alas, shortlog does not take --all. Yes, I know\n>> \n>> \tgit log --since=3.day --all | git shortlog | sort | uniq -c\n>> \n>> is an obvious workaround, but it is mildly irritating.\n>\n> Hmm. Could it be as simple as:\n>\n> diff --git a/revision.c b/revision.c\n> index a843c42..eaa5572 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\"))\n> \t{\n> \t\tunkv[(*unkc)++] = arg;\n> -\t\treturn 0;\n> +\t\treturn 1;\n> \t}\n> \n> \tif (!prefixcmp(arg, \"--max-count=\")) {\n>\n> That is, handle_revision_opt says \"yes we parsed this, and it should be\n> gone\" even though it still gets stuck in the \"unknown\" section to be\n> parsed by setup_revisions later.\n\nHmm, wouldn't that suggest it needs to return 1 when an option candidate\ngiven to diff_opt_parse() turns out not to be a diff option and stuffed\nback to unkv[] at the end of this function?\n"},{"id":"85746","messageId":"20080731090146.GA12867@artemis.madism.org","threadId":"14755","inReplyTo":"7v8wvizc16.fsf@gitster.siamese.dyndns.org","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-07-31T09:01:46Z","receivedAt":"2008-07-31T09:01:46Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:\n> >\n> >> Alas, shortlog does not take --all. Yes, I know\n> >> \n> >> \tgit log --since=3.day --all | git shortlog | sort | uniq -c\n> >> \n> >> is an obvious workaround, but it is mildly irritating.\n> >\n> > Hmm. Could it be as simple as:\n> >\n> > diff --git a/revision.c b/revision.c\n> > index a843c42..eaa5572 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> > \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\"))\n> > \t{\n> > \t\tunkv[(*unkc)++] = arg;\n> > -\t\treturn 0;\n> > +\t\treturn 1;\n> > \t}\n> > \n> > \tif (!prefixcmp(arg, \"--max-count=\")) {\n> >\n> > That is, handle_revision_opt says \"yes we parsed this, and it should be\n> > gone\" even though it still gets stuck in the \"unknown\" section to be\n> > parsed by setup_revisions later.\n\n Ack this is a correct fix. I wonder how this even works with other\ncommands that use --all and stuff like that.\n\n> Hmm, wouldn't that suggest it needs to return 1 when an option candidate\n> given to diff_opt_parse() turns out not to be a diff option and stuffed\n> back to unkv[] at the end of this function?\n\n No, because diff-opts are our last chance, and those are _really_\nunknown options. \"--all\" is different because revision machinery\nacknowledge \"yeah it's really for us, we recognize it ...\" hence the 1\nresult \"... but we have to parse it later so keep it in place\".\n\n There is no such thing with unknown option for the revision _and_ diff\nmachinery. They _must_ return 0 to say \"no we really didn't know what\nthis is\".\n\n IOW, semantics is:\n\n < 0 : error\n 0 : option was not directed at us, unknown (this allow to chain\n option parsers.\n > 0 : yeah this was for us, and we consumed this amount of arguments\n in the process.\n\n Here \"--all\" is clearly something that we _recognized_ hence fit in\nthe third case, even if we decide to still keep it in the \"unk\" array\n(which is ill named, I kept its name, but it's not unknown options, it's\nactually non option arguments that are to be deal with in the last\nstage, setup_revisions here, or the command as a general rule).\n\n Unknown options to both the revision _and_ diff option machineries are\nclearly of the 2nd kind and must return 0. They put the option in 'unk'\nas well (and here this is properly named) because this is how it\nhistorically worked for many commands, and is here as a legacy\ncompatibility thing. parse-opt based parser (and git-shortlog has one)\ndoes not uses that (and you can see it in parse_revision_opt that is\nwhat such parser use: a result of `0' triggers a usage).\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"85747","messageId":"20080731090348.GB20691@sigill.intra.peff.net","threadId":"14755","inReplyTo":"7v8wvizc16.fsf@gitster.siamese.dyndns.org","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T09:03:49Z","receivedAt":"2008-07-31T09:03:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2008 at 01:35:33AM -0700, Junio C Hamano wrote:\n\n> > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> > \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\"))\n> > \t{\n> > \t\tunkv[(*unkc)++] = arg;\n> > -\t\treturn 0;\n> > +\t\treturn 1;\n> > \t}\n> > \n> > \tif (!prefixcmp(arg, \"--max-count=\")) {\n> >\n> > That is, handle_revision_opt says \"yes we parsed this, and it should be\n> > gone\" even though it still gets stuck in the \"unknown\" section to be\n> > parsed by setup_revisions later.\n> \n> Hmm, wouldn't that suggest it needs to return 1 when an option candidate\n> given to diff_opt_parse() turns out not to be a diff option and stuffed\n> back to unkv[] at the end of this function?\n\nNot necessarily. The logic there goes:\n\n 1. it's not a revision option, so pass to diff\n 2. it's not a diff opt, so it is unknown; we parsed no options\n 3. the caller sees we failed to parse, so it barfs\n\nbut the logic here is:\n\n 1. it _is_ a revision option. Our handling of it is just a little odd,\n in that we need to parse it later, when we are in setup_revisions.\n So put it aside for now as a \"revision\" that just happens to look\n like an option.\n 2. caller sees we parsed, and doesn't complain\n 3. caller later passes the \"revision\" to setup_revisions, which knows\n what to do\n\nNow my patch doesn't just operate on \"--all\", but rather several other\noptions including --no-walk. And maybe that is not right, and --all\nshould be handled separately (though I think --remotes would follow the\nsame logic). I'm not really sure why --no-walk is separated like that.\n\n-Peff\n"},{"id":"85748","messageId":"20080731090528.GC20691@sigill.intra.peff.net","threadId":"14755","inReplyTo":"20080731090146.GA12867@artemis.madism.org","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T09:05:28Z","receivedAt":"2008-07-31T09:05:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2008 at 11:01:46AM +0200, Pierre Habouzit wrote:\n\n> Ack this is a correct fix. I wonder how this even works with other\n> commands that use --all and stuff like that.\n\n--all is parsed separately in setup_revisions _before_ we call\nhandle_revision_opt. So the only codepath hitting that is parse_options\nusers who call parse_revision_opt (i.e., blame and shortlog).\n\n-Peff\n"},{"id":"85749","messageId":"20080731090646.GB12867@artemis.madism.org","threadId":"14755","inReplyTo":"20080731090146.GA12867@artemis.madism.org","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-07-31T09:06:46Z","receivedAt":"2008-07-31T09:06:46Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Jul 31, 2008 at 09:01:46AM +0000, Pierre Habouzit wrote:\n> On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote:\n> > Jeff King writes:\n> > \n> > > On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:\n> > >\n> > >> Alas, shortlog does not take --all. Yes, I know\n> > >> \n> > >> \tgit log --since=3.day --all | git shortlog | sort | uniq -c\n> > >> \n> > >> is an obvious workaround, but it is mildly irritating.\n> > >\n> > > Hmm. Could it be as simple as:\n> > >\n> > > diff --git a/revision.c b/revision.c\n> > > index a843c42..eaa5572 100644\n> > > --- a/revision.c\n> > > +++ b/revision.c\n> > > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> > > \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\"))\n> > > \t{\n> > > \t\tunkv[(*unkc)++] = arg;\n> > > -\t\treturn 0;\n> > > +\t\treturn 1;\n> > > \t}\n> > > \n> > > \tif (!prefixcmp(arg, \"--max-count=\")) {\n> > >\n> > > That is, handle_revision_opt says \"yes we parsed this, and it should be\n> > > gone\" even though it still gets stuck in the \"unknown\" section to be\n> > > parsed by setup_revisions later.\n> \n> Ack this is a correct fix. I wonder how this even works with other\n> commands that use --all and stuff like that.\n\nOh actually I know: the parse_revision_opt machinery (that is buggy\nbecause of this 0 result) is used in git-blame where --all is\nmeaningless anyways, and ... git-shortlog only. The legacy way to parse\nrevision options does not treats `0' answers differently from `1'\nactually, I wonder why, because this is probably wrong.\n\nThis was a regression, I suppose prior to its parse-optification\ngit-shortlog accepted --all (and I see no valid reason for it not to).\n\nThanks a lot to Jeff for the good catch.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"85750","messageId":"20080731091515.GC12867@artemis.madism.org","threadId":"14755","inReplyTo":"20080731090348.GB20691@sigill.intra.peff.net","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-07-31T09:15:15Z","receivedAt":"2008-07-31T09:15:15Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Jul 31, 2008 at 09:03:49AM +0000, Jeff King wrote:\n> Not necessarily. The logic there goes:\n> \n> 1. it's not a revision option, so pass to diff\n> 2. it's not a diff opt, so it is unknown; we parsed no options\n> 3. the caller sees we failed to parse, so it barfs\n> \n> but the logic here is:\n> \n> 1. it _is_ a revision option. Our handling of it is just a little odd,\n> in that we need to parse it later, when we are in setup_revisions.\n> So put it aside for now as a \"revision\" that just happens to look\n> like an option.\n> 2. caller sees we parsed, and doesn't complain\n> 3. caller later passes the \"revision\" to setup_revisions, which knows\n> what to do\n> \n> Now my patch doesn't just operate on \"--all\", but rather several other\n> options including --no-walk. And maybe that is not right, and --all\n> should be handled separately (though I think --remotes would follow the\n> same logic). I'm not really sure why --no-walk is separated like that.\n\n--no-walk alters how add_pending_object_with_mode works, which is called\nby handle_revision_arg. IOW, --no-walk is not the same as\n--no-walk . IOW reference arguments and --no-walk ordering\nmust be preserved, IOW --no-walk is a pseudo-argument like --all --tags\nor --remote are.\n\nIt's okay for parse_revision_opt to take out any option that doesn't\nalter handle_revision_arg (as for parse_option based parsers it's the\nsole thing setup_revision will be doing: consuming revisions in a loop\nwith handle_revision_arg), but only those.\n\nThe full list (that I made when I wrote parse_revision_opt) is actually:\n\n pseudo arguments: --all --branches --tags --remotes --reflog\n combining operator: --not\n behavior modifying: --no-walk --do-walk\n\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"85751","messageId":"20080731093410.GA21396@sigill.intra.peff.net","threadId":"14755","inReplyTo":"20080731091515.GC12867@artemis.madism.org","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T09:34:10Z","receivedAt":"2008-07-31T09:34:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[Pierre, you have the bogus MFT header that people are often complaining\nabout. Either I have to do extra work to fix the headers, or more people\nend up in the To: field. I don't personally care about the latter, but\nit seems that others organize their mail based on To versus Cc]\n\nOn Thu, Jul 31, 2008 at 11:15:15AM +0200, Pierre Habouzit wrote:\n\n> --no-walk alters how add_pending_object_with_mode works, which is called\n> by handle_revision_arg. IOW, --no-walk is not the same as\n> --no-walk . IOW reference arguments and --no-walk ordering\n> must be preserved, IOW --no-walk is a pseudo-argument like --all --tags\n> or --remote are.\n\nAh, OK. Then that makes sense, then. And I definitely feel that the\npatch I posted is the right thing to do, then. So here is a patch with\ncommit message. The commit message to patch ratio is ridiculous, but\nobviously this fix took some thinking, so it seems best to document it.\n\n-- >8 --\nallow \"non-option\" revision options in parse_option-enabled commands\n\nCommands which use parse_options but also call\nsetup_revisions must do their parsing in a two step process:\n\n 1. first, they parse all options. Anything unknown goes to\n parse_revision_opt (which calls handle_revision_opt),\n which may claim the option or say \"I don't recognize\n this\"\n\n 2. the non-option remainder goes to setup_revisions to\n actually get turned into revisions\n\nSome revision options are \"non-options\" in that they must be\nparsed in order with their revision counterparts in\nsetup_revisions. For example, \"--all\" functions as a\npseudo-option expanding to all refs, and \"--no-walk\" affects\nrefs after it on the command line, but not before. The\nrevision option parser in step 1 recognizes such options and\nsets them aside for later parsing by setup_revisions.\n\nHowever, the return value used from handle_revision_opt\nindicated \"I didn't recognize this\", which was wrong. It\ndid, and it took appropriate action (even though that action\nwas just deferring it for later parsing). Thus it should\nreturn \"yes, I recognized this.\"\n\nPreviously, these pseudo-options generated an error when\nused with parse_options parsers (currently just blame and\nshortlog). With this patch, they should work fine, enabling\nthings like \"git shortlog --all\".\n\nSigned-off-by: Jeff King \n---\n revision.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex a843c42..eaa5572 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\"))\n \t{\n \t\tunkv[(*unkc)++] = arg;\n-\t\treturn 0;\n+\t\treturn 1;\n \t}\n \n \tif (!prefixcmp(arg, \"--max-count=\")) {\n-- \n1.6.0.rc1.197.gc57ac6\n"},{"id":"85753","messageId":"20080731102223.GD12867@artemis.madism.org","threadId":"14755","inReplyTo":"20080731093410.GA21396@sigill.intra.peff.net","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-07-31T10:22:23Z","receivedAt":"2008-07-31T10:22:23Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"> [Pierre, you have the bogus MFT header that people are often complaining\n> about. Either I have to do extra work to fix the headers, or more people\n> end up in the To: field. I don't personally care about the latter, but\n> it seems that others organize their mail based on To versus Cc]\n\n [ err if you do a list reply, nobody ends up in To:... but oh well I\n can probably remove it, it'll just make my own mutt git folder\n awkwardly colorized. ]\n\nOn Thu, Jul 31, 2008 at 09:34:10AM +0000, Jeff King wrote:\n> On Thu, Jul 31, 2008 at 11:15:15AM +0200, Pierre Habouzit wrote:\n> > --no-walk alters how add_pending_object_with_mode works, which is called\n> > by handle_revision_arg. IOW, --no-walk is not the same as\n> > --no-walk . IOW reference arguments and --no-walk ordering\n> > must be preserved, IOW --no-walk is a pseudo-argument like --all --tags\n> > or --remote are.\n> \n> Ah, OK. Then that makes sense, then. And I definitely feel that the\n> patch I posted is the right thing to do,\n\n Yeah I was convinced the first time I saw it already :)\n\n-- >8 --\nallow \"non-option\" revision options in parse_option-enabled commands\n\nCommands which use parse_options but also call\nsetup_revisions must do their parsing in a two step process:\n\n 1. first, they parse all options. Anything unknown goes to\n parse_revision_opt (which calls handle_revision_opt),\n which may claim the option or say \"I don't recognize\n this\"\n\n 2. the non-option remainder goes to setup_revisions to\n actually get turned into revisions\n\nSome revision options are \"non-options\" in that they must be\nparsed in order with their revision counterparts in\nsetup_revisions. For example, \"--all\" functions as a\npseudo-option expanding to all refs, and \"--no-walk\" affects\nrefs after it on the command line, but not before. The\nrevision option parser in step 1 recognizes such options and\nsets them aside for later parsing by setup_revisions.\n\nHowever, the return value used from handle_revision_opt\nindicated \"I didn't recognize this\", which was wrong. It\ndid, and it took appropriate action (even though that action\nwas just deferring it for later parsing). Thus it should\nreturn \"yes, I recognized this.\"\n\nPreviously, these pseudo-options generated an error when\nused with parse_options parsers (currently just blame and\nshortlog). With this patch, they should work fine, enabling\nthings like \"git shortlog --all\".\n\nSigned-off-by: Jeff King \nAcked-By: Pierre Habouzit \n---\n revision.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex a843c42..eaa5572 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\"))\n \t{\n \t\tunkv[(*unkc)++] = arg;\n-\t\treturn 0;\n+\t\treturn 1;\n \t}\n \n \tif (!prefixcmp(arg, \"--max-count=\")) {\n-- \n1.6.0.rc1.197.gc57ac6\n"},{"id":"85754","messageId":"20080731103351.GA22895@sigill.intra.peff.net","threadId":"14755","inReplyTo":"20080731102223.GD12867@artemis.madism.org","subject":"Re: git blame not respecting --find-copies-harder ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-31T10:33:52Z","receivedAt":"2008-07-31T10:33:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2008 at 12:22:23PM +0200, Pierre Habouzit wrote:\n\n> > [Pierre, you have the bogus MFT header that people are often complaining\n> > about. Either I have to do extra work to fix the headers, or more people\n> > end up in the To: field. I don't personally care about the latter, but\n> > it seems that others organize their mail based on To versus Cc]\n> \n> [ err if you do a list reply, nobody ends up in To:... but oh well I\n> can probably remove it, it'll just make my own mutt git folder\n> awkwardly colorized. ]\n\nI can't do a list reply, as mutt sees no list (and I didn't bother\nsetting up a 'subscribe' variable, since I didn't want to generate MFTs\nin the first place). However, I just realized that mutt has an\nhonor_followup_to, which will do what I want (AFAICT, there is no point\nin honoring MFT when using the conventions of this mailing list).\n\nBut I wonder if it is as simple in other clients.\n\n-Peff\n"}]}