Re: [PATCH v3 2/3] merge: Add merge.renames config setting

From: Elijah Newren <newren@gmail.com>
Date: Apr 27, 2018, 14:32 UTC
Message-ID: <CABPp-BEoY3bLQVNzfcxKqfXktdpfq0m7yaExM0f3-Ad+Mi0GBQ@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1804270918430.72@tvgsbejvaqbjf.bet>

Hi Dsco,

On Fri, Apr 27, 2018 at 12:23 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> Hi,

<snip>

>
> Guys, you argued long and hard that one config setting (diff.renames)
> should magically imply another one (merge.renames), on the basis that they
> essentially do the same.

I apologize for any frustration; I've probably caused a good deal of it by repeatedly catching or noticing things after one review and then bringing them up later. I just didn't catch it all at first.

Your restatement of the basis of the argument doesn't match my rationale, though. My reasoning was that (1) the configuration options exist to allow the user to specify how much of a performance cost they're willing to pay, (2) the two options are separate because some users might want to configure one more loosely or tightly than the other, but (3) many users will want to just specify once how much they are willing to pay without being forced to repeat themselves for different parts of git (diff, merge, possible future commands).

I'll add to my former basis by stating that I think the diffstat at the end of the merge should be controlled by these variables, even if that's not implemented as part of Ben's series. And both of those configuration options clearly feed into whether that diffstat should be doing rename or copy or no detection. While they could be kept separate options, forcing us to somehow decide which one overrides which, I think it's much more natural if we already have merge.renames inheriting from and overriding diff.renames.

Show 6 quoted lines

> And now you are suggesting that they *cannot* be essentially the same? Are
> you agreeing on such a violation of the Law of Least Surprise?
>
> Please, make up your mind. Inheriting merge.renames from diff.renames is
> "magic" enough to be puzzling. Introducing that auto-demoting is just
> simply creating a bad user experience. Please don't.

From my view, resolve and octopus merges auto-demote diff.renames and merge.renames to false. In fact, they optimize the work involved in that demotion by not even checking the value. I think demoting "copy" to "true" in the recursive merge machinery is no more surprising than that is. You can find more details to this argument in the portion of my email that you responded to but snipped out.

But to add to that argument, if auto-demotion is a violation of the Law of Least Surprise, as you claim, then naming this option merge.renames is also a violation of the Law of Least Surprise. You would instead need to rename it to something like merge.recursive.renames. (And then when a new merge strategy comes along that also does renames, we'll need to add a merge.ort.renames.)

Show 8 quoted lines

> It is fine, of course, to admit at this stage that the whole magic idea of
> letting merge.renames inherit from diff.renames was only half thought
> through (we're in reviewing stage right now for the purpose of weighing
> alternative approaches and trying to come up with the best solution after
> all) and that we should go with Ben's original approach, which has the
> rather huge and dramatic advantage of being very, very clear and easy to
> understand to the user. We could even document that (and why) the 'copy'
> value in merge.renames is not allowed for the time being.

It was only half thought through, yes, at least by me.  But the more I
think through it, the more I like the inheritance personally.  I see
no problem with the demotion and think the inheritance has the
advantage of being easier to understand, because I see your proposal
as causing questions like:
  - "Why does merge.renameLimit inherit from diff.renameLimit but
merge.renames doesn't from diff.renames?"
  - "Why can't I just specify in one place that I {am, am not} willing
to pay for {full, both copy and} rename detection wherever it makes
sense?"
  - "How do I control the detection for the diffstat at the end of the merge?"

Hope that helps, Elijah