git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ben Peart <peartben@gmail.com>
To: Elijah Newren <newren@gmail.com>, Ben Peart <Ben.Peart@microsoft.com>
Cc: "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 13:59:31 -0400	[thread overview]
Message-ID: <cd49481c-9665-124a-5f94-791f1a16657d@gmail.com> (raw)
In-Reply-To: <CABPp-BFANBs=tOhS5BFfTMkdQsNYbUDExWK8QB0V=qD9YwZyWw@mail.gmail.com>



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.  

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.

I think the documentation should mention that
> "false" is the same as passing -Xno-renames, and "true" is the same as
> passing -Xfind-renames.  However, find-renames does take similarity
> threshold as a parameter, so there's a question whether this option
> should provide some way to do the same.  I'm not sure the answer to
> that; it may be that we'd want a separate config option for that, and
> we can wait to add it until someone actually wants it.

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.

> 
>>   merge.renormalize::
>>          Tell Git that canonical representation of files in the
>>          repository has changed over time (e.g. earlier commits record
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index 9c05eb7f70..cd5367e890 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -3256,6 +3256,7 @@ static void merge_recursive_config(struct merge_options *o)
>>          git_config_get_int("merge.verbosity", &o->verbosity);
>>          git_config_get_int("diff.renamelimit", &o->diff_rename_limit);
>>          git_config_get_int("merge.renamelimit", &o->merge_rename_limit);
>> +       git_config_get_bool("merge.renames", &o->detect_rename);
>>          git_config(git_xmerge_config, NULL);
>>   }
> 
> I would expect an explicitly passed -Xno-renames or -Xfind-renames to
> override the config setting.  Could you check if that's the case?
> 

Yes, command line options override the config settings.  You can see 
that in the code where the call to init_merge_options() which loads the 
config settings is followed by parse_merge_opt() which loads the command 
line options.  I've also verified the behavior in the debugger (it's on 
by default in the code, the config setting turns it off, then the 
command line option turns it back on).

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

  parent reply	other threads:[~2018-04-20 17:59 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 [this message]
2018-04-20 18:34       ` Elijah Newren
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=cd49481c-9665-124a-5f94-791f1a16657d@gmail.com \
    --to=peartben@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=newren@gmail.com \
    --cc=pclouds@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).