git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Ben Peart <peartben@gmail.com>
Cc: Ben Peart <Ben.Peart@microsoft.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>,
	"peff@peff.net" <peff@peff.net>,
	"gitster@pobox.com" <gitster@pobox.com>,
	"pclouds@gmail.com" <pclouds@gmail.com>,
	"vmiklos@frugalware.org" <vmiklos@frugalware.org>,
	Kevin Willford <kewillf@microsoft.com>,
	"Johannes.Schindelin@gmx.de" <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v1 1/2] merge: Add merge.renames config setting
Date: Fri, 20 Apr 2018 11:34:25 -0700	[thread overview]
Message-ID: <CABPp-BFqj2TFiHUDsysafq0NHC4MV-QYZVxOZe1TNRrXMOQfng@mail.gmail.com> (raw)
In-Reply-To: <cd49481c-9665-124a-5f94-791f1a16657d@gmail.com>

Hi Ben,

On Fri, Apr 20, 2018 at 10:59 AM, Ben Peart <peartben@gmail.com> wrote:
>
> On 4/20/2018 1:02 PM, Elijah Newren wrote:
>>
>> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart <Ben.Peart@microsoft.com>
>> wrote:
>>>
>>> --- a/Documentation/merge-config.txt
>>> +++ b/Documentation/merge-config.txt
>>> @@ -37,6 +37,11 @@ merge.renameLimit::
>>>          during a merge; if not specified, defaults to the value of
>>>          diff.renameLimit.
>>>
>>> +merge.renames::
>>> +       Whether and how Git detects renames.  If set to "false",
>>> +       rename detection is disabled. If set to "true", basic rename
>>> +       detection is enabled. This is the default.
>>
>>
>> One can already control o->detect_rename via the -Xno-renames and
>> -Xfind-renames options.

This statement wasn't meant to be independent of the sentence that
followed it...

> Yes, but that requires people to know they need to do that and then remember
> to pass it on the command line every time.  We've found that doesn't
> typically happen, we just get someone complaining about slow merges. :)
>
> That is why we added them as config options which change the default. That
> way we can then set them on the repo and the default behavior gives them
> better performance.  They can still always override the config setting with
> the command line options.

Sorry, I think I wasn't being clear.  The documentation for the config
options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and
merge.ff all mention the equivalent command line parameters.  Your
patch doesn't do that for merge.renames, but I think it would be
helpful if it did.

Also, a link in the documentation the other way, from
Documentation/merge-strategies.txt under the entries for -Xno-renames
and -Xfind-renames should probably mention this new merge.renames
config setting (much like the -Xno-renormalize flag mentions the
merge.renomralize config option).

(In general, I think having this as a configuration option makes
sense, though I hope my other performance patches would be enough to
make people consider switching back to the defaults and use rename
detection again.)

<snip>
> I'm of the opinion that we shouldn't bother adding features that we aren't
> sure someone will want/use.  If it comes up, we can certainly add it at a
> later date.

Works for me; I was mostly throwing it out there for thought.

> Yes, command line options override the config settings.

Good.  :-)

>> Also, if someone sets merge.renameLimit (to anything) and sets
>> merge.renames to false, then they've got a contradictory setup.  Does
>> it make sense to check and warn about that anywhere?
>
> I don't think we need to.  The merge.renameLimit is only used if
> detect_rename it turned on no matter how that gets turned on (default,
> config setting, command line option) so there isn't really a change in
> behavior here.

I agree that's the pre-existing behavior, but prior to this patch
turning off rename detection could only be done manually with every
invocation.  I'm slightly concerned that users might be confused if
merge.renames was set to false somewhere -- perhaps even in a global
/etc/gitconfig that they had no knowledge of or control over -- and in
an attempt to get rename detection to work they started passing larger
and larger values for renameLimit all to no avail.

The easy fix here may just be documenting the diff.renameLimit and
merge.renameLimit options that they have no effect if rename detection
is turned off.

Or maybe I'm just worrying too much, but we (folks at $dayjob) were
bit pretty hard by renameLimit silently being capped at a value less
than the user specified and in a way that wasn't documented anywhere.

  reply	other threads:[~2018-04-20 18:34 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-20 13:36 [PATCH v1 0/2] add additional config settings for merge Ben Peart
2018-04-20 13:36 ` [PATCH v1 1/2] merge: Add merge.renames config setting Ben Peart
2018-04-20 17:02   ` Elijah Newren
2018-04-20 17:26     ` Elijah Newren
2018-04-23 12:57       ` Ben Peart
2018-04-20 17:59     ` Ben Peart
2018-04-20 18:34       ` Elijah Newren [this message]
2018-04-21  4:23         ` Junio C Hamano
2018-04-23 16:00           ` Ben Peart
2018-04-23 23:23             ` Junio C Hamano
2018-04-24 11:58               ` Johannes Schindelin
2018-04-24 17:47                 ` Elijah Newren
2018-04-25  8:20                   ` Johannes Schindelin
2018-04-22 12:07         ` Eckhard Maaß
2018-04-23 13:15           ` Ben Peart
2018-04-23 21:32             ` Eckhard Maaß
2018-04-24 16:53               ` Ben Peart
2018-04-23 13:22         ` Ben Peart
2018-04-20 13:36 ` [PATCH v1 2/2] merge: Add merge.aggressive " Ben Peart
2018-04-20 17:22   ` Elijah Newren
2018-04-24 16:45     ` Ben Peart
2018-04-24 17:36       ` Elijah Newren
2018-04-24 23:57       ` Junio C Hamano
2018-04-25 14:47         ` Ben Peart
2018-04-20 17:34 ` [PATCH v1 0/2] add additional config settings for merge Elijah Newren
2018-04-20 18:19   ` Ben Peart
2018-04-24 17:11 ` [PATCH v2 " Ben Peart
2018-04-24 17:11   ` [PATCH v2 1/2] merge: Add merge.renames config setting Ben Peart
2018-04-24 18:11     ` Elijah Newren
2018-04-24 18:59     ` Elijah Newren
2018-04-24 20:31       ` Ben Peart
2018-04-25 16:01         ` Elijah Newren
2018-04-24 17:11   ` [PATCH v2 2/2] merge: Add merge.aggressive " Ben Peart
2018-04-25  0:13   ` [PATCH v2 0/2] add additional config settings for merge Junio C Hamano
2018-04-25 15:22     ` Ben Peart
2018-04-26  1:48       ` Junio C Hamano
2018-04-26 20:52 ` [PATCH v3 0/3] add merge.renames config setting Ben Peart
2018-04-26 20:52   ` [PATCH v3 1/3] merge: update documentation for {merge,diff}.renameLimit Ben Peart
2018-04-26 23:11     ` Elijah Newren
2018-04-26 23:23       ` Jonathan Tan
2018-04-26 20:52   ` [PATCH v3 2/3] merge: Add merge.renames config setting Ben Peart
2018-04-26 22:52     ` Elijah Newren
2018-04-27  0:54       ` Ben Peart
2018-04-27  2:23         ` Junio C Hamano
2018-04-27  3:28           ` Elijah Newren
2018-04-27  7:23             ` Johannes Schindelin
2018-04-27 14:32               ` Elijah Newren
2018-04-27 18:37           ` Eckhard Maaß
2018-04-27 20:23             ` Elijah Newren
2018-04-30  8:03               ` Eckhard Maaß
2018-04-30 16:54                 ` Elijah Newren
2018-04-27  4:17         ` Elijah Newren
2018-04-27 18:19         ` Elijah Newren
2018-04-30 13:11           ` Ben Peart
2018-04-30 16:12             ` Re: Elijah Newren
2018-05-02 14:33               ` Re: Ben Peart
2018-04-26 20:52   ` [PATCH v3 3/3] merge: pass aggressive when rename detection is turned off Ben Peart
2018-04-26 23:00     ` Elijah Newren
2018-04-26 22:08   ` [PATCH v3 0/3] add merge.renames config setting Elijah Newren
2018-05-02 16:01 ` [PATCH v4 0/3] add additional config settings for merge Ben Peart
2018-05-02 16:01   ` [PATCH v4 1/3] merge: update documentation for {merge,diff}.renameLimit Ben Peart
2018-05-02 16:01   ` [PATCH v4 2/3] merge: Add merge.renames config setting Ben Peart
2018-05-04  3:07     ` Junio C Hamano
2018-05-02 16:01   ` [PATCH v4 3/3] merge: pass aggressive when rename detection is turned off Ben Peart
2018-05-02 17:20   ` [PATCH v4 0/3] add additional config settings for merge Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CABPp-BFqj2TFiHUDsysafq0NHC4MV-QYZVxOZe1TNRrXMOQfng@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=Ben.Peart@microsoft.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kewillf@microsoft.com \
    --cc=pclouds@gmail.com \
    --cc=peartben@gmail.com \
    --cc=peff@peff.net \
    --cc=vmiklos@frugalware.org \
    --subject='Re: [PATCH v1 1/2] merge: Add merge.renames config setting' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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