Re: [PATCH] diff: make -M -C mean the same as -C -M

From: Mike Hommey <mh@glandium.org>
Date: Jan 23, 2015, 22:26 UTC
Message-ID: <20150123222659.GA22303@glandium.org>
In-Reply-To: <xmqqr3ulqr09.fsf@gitster.dls.corp.google.com>

On Fri, Jan 23, 2015 at 10:41:10AM -0800, Junio C Hamano wrote:

Show 20 quoted lines

> Mike Hommey <mh@glandium.org> writes:
> 
> > While -C implies -M, it is quite common to see both on example command lines
> > here and there. The unintuitive thing is that if -M appears after -C, then
> > copy detection is turned off because of how the command line arguments are
> > handled.
> 
> This is deliberate, see below.
> 
> > Change this so that when both -C and -M appear, whatever their order, copy
> > detection is on.
> >
> > Signed-off-by: Mike Hommey <mh@glandium.org>
> > ---
> >
> > Interestingly, I even found mentions of -C -M in this order for benchmarks,
> > on this very list (see 6555655.XSJ9EnW4BY@mako).
> 
> Aside from the general warning against taking everything you see on
> this list as true

The problem is not whether what is on the list is true or not, but that people will use what they see.

> I think you are looking at an orange and talking
> about an apple.  $gmane/244581 is about "git blame", and "-M/-C"
> over there mean completely different things.

I thought blame was using the diff option parser, and I was wrong.

(snip)

> In the context of "git blame", "-C" and "-M" control orthogonal
> concepts and it makes sense to use only one but not the other, or
> both.

In the context of blame both -C and -M |= a flags set, so one doesn't override the other. You can place them in any order, the result will be the same. Incidentally, -C includes the flag -M sets, so -C -M is actually redundant. What -C and -M can be used for is set different scores (-C9 -M8).

Show 15 quoted lines

> But "-M" given to "git diff" is not about such an orthogonal
> concept.
> 
> Even though its source candidates are narrower than that of "-C" in
> that "-M" chooses only from the set of files that disappeared while
> "-C" also chooses from files that remain after the change, they are
> both about "which file did the whole thing come from?", and it makes
> sense to have progression of "-M" < "-C" < "-C -C".  You can think
> of these as a short-hand for --rename-copy-level={0,1,2,3} option
> (where the level 0 -- do not do anything -- corresponds to giving no
> "-M/-C").  It can be argued both ways: either we take the maximum of
> the values given to --rename-copy-level options (which corresponds
> to what your patch attempts to do), or the last one wins.  We happen
> to have implemented the latter long time ago and that is how
> existing users expect things to work.

Are they? I, for one, wasn't. It is easy to understand that --foo=1 --foo=2 is going to take the last given --foo, but do people really expect the last of -C -M to be used? Reading the code further, I can see that it's even more confusing than that: -C -C wins over -M, wherever it happens. So -C -C -M == -C -C ; -C -M == -M ; -M -C == -C.

At the very least, this should be spelled out in the documentation.

Mike