git blame not respecting --find-copies-harder ?

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?

On 2008.07.30 11:39:03 +0200, Stephen R. van den Berg wrote:

git blame doesn't know --find-copies-harder, it's -C -C for blame.

Björn

On Wed, Jul 30, 2008 at 17:01, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:

Shouldn't it have died with "don't know option --find-copies-harder" then?

On Wed, Jul 30, 2008 at 05:43:52PM +0200, Sverre Rabbelier wrote:

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

Jeff King <peff@peff.net> writes:

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

On Thu, Jul 31, 2008 at 12:05:22AM -0700, Junio C Hamano wrote:

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

Jeff King <peff@peff.net> writes:

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.

On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:

Hmm. Could it be as simple as:

Jeff King <peff@peff.net> writes:

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?

On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote:

  Ack this is a correct fix. I wonder how this even works with other
commands that use --all and stuff like that.

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

On Thu, Jul 31, 2008 at 11:01:46AM +0200, Pierre Habouzit wrote:

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

On Thu, Jul 31, 2008 at 09:01:46AM +0000, Pierre Habouzit wrote:

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.

On Thu, Jul 31, 2008 at 01:35:33AM -0700, Junio C Hamano 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.

-Peff

On Thu, Jul 31, 2008 at 09:03:49AM +0000, Jeff King 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.

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

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

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

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

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

On Thu, Jul 31, 2008 at 12:22:23PM +0200, Pierre Habouzit wrote:

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