Re: [PATCH v2 1/2] merge: Add merge.renames config setting

[Elijah Newren]

On Tue, Apr 24, 2018 at 1:31 PM, Ben Peart  wrote:
> On 4/24/2018 2:59 PM, Elijah Newren wrote:
>> On Tue, Apr 24, 2018 at 10:11 AM, Ben Peart 
>> wrote:
>>>
>>> diff --git a/builtin/merge.c b/builtin/merge.c
>>> index 8746c5e3e8..3be52cd316 100644
>>> --- a/builtin/merge.c
>>> +++ b/builtin/merge.c
>>> @@ -424,6 +424,7 @@ static void finish(struct commit *head_commit,
>>>  opts.output_format |=
>>>  DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
>>>  opts.detect_rename = DIFF_DETECT_RENAME;
>>> +   git_config_get_bool("merge.renames",
>>> _rename);
>>>  diff_setup_done();
>>>  diff_tree_oid(head, new_head, "", );
>>>  diffcore_std();
>>
>>
>> Shouldn't this also be turned off if either (a) merge.renames is unset
>> and diff.renames is false, or (b) the user specifies -Xno-renames?
>>
>
> This makes me think that I should probably remove the line that overrides
> the detect_rename setting with the merge config setting.  As I look at the
> code, none of the other merge options are reflected in the diffstat;
> instead, all the settings are pretty much hard coded.  Perhaps I shouldn't
> rock that boat.

Actually, stat_graph_width respects the diff.statGraphWidth config
option, even though it's slightly hidden due to the magic value of -1,
and being handled from diff.c.

However, trying to get this suggestion of mine hooked up, particularly
with -Xno-renames and -Xfind-renames (the latter because it might need
to override a merge.renames or diff.renames config setting), might be
slightly tricky because the -X options are only passed down to a
single merge strategy but this code is outside of the merge
strategies.  So making it a separate patch, or even a separate patch
series may make sense.  I'm still interested in this change if you
aren't, but I'm fine with it not being part of your series if you
don't want to tackle it.

Re: [PATCH v2 1/2] merge: Add merge.renames config setting

[Ben Peart]

On 4/24/2018 2:59 PM, Elijah Newren wrote:

Sorry, I noticed something else I missed on my last reading...

On Tue, Apr 24, 2018 at 10:11 AM, Ben Peart  wrote:

diff --git a/builtin/merge.c b/builtin/merge.c
index 8746c5e3e8..3be52cd316 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -424,6 +424,7 @@ static void finish(struct commit *head_commit,
 opts.output_format |=
 DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
 opts.detect_rename = DIFF_DETECT_RENAME;
+   git_config_get_bool("merge.renames", _rename);
 diff_setup_done();
 diff_tree_oid(head, new_head, "", );
 diffcore_std();


Shouldn't this also be turned off if either (a) merge.renames is unset
and diff.renames is false, or (b) the user specifies -Xno-renames?



This makes me think that I should probably remove the line that 
overrides the detect_rename setting with the merge config setting.  As I 
look at the code, none of the other merge options are reflected in the 
diffstat; instead, all the settings are pretty much hard coded.  Perhaps 
I shouldn't rock that boat.

Re: [PATCH v2 1/2] merge: Add merge.renames config setting

[Elijah Newren]

Sorry, I noticed something else I missed on my last reading...

On Tue, Apr 24, 2018 at 10:11 AM, Ben Peart  wrote:
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 8746c5e3e8..3be52cd316 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -424,6 +424,7 @@ static void finish(struct commit *head_commit,
> opts.output_format |=
> DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> opts.detect_rename = DIFF_DETECT_RENAME;
> +   git_config_get_bool("merge.renames", _rename);
> diff_setup_done();
> diff_tree_oid(head, new_head, "", );
> diffcore_std();

Shouldn't this also be turned off if either (a) merge.renames is unset
and diff.renames is false, or (b) the user specifies -Xno-renames?

Re: [PATCH v2 1/2] merge: Add merge.renames config setting

[Elijah Newren]

On Tue, Apr 24, 2018 at 10:11 AM, Ben Peart  wrote:
> Add the ability to control rename detection for merge via a config setting.

Sweet, thanks for including the documentation updates.

I lean towards the side of the argument that says that since
merge.renameLimit inherits from diff.renameLimit, merge.renames should
inherit default value from diff.renames (allow people to not have to
repeat themselves as much if they want to use the same rename settings
for all cases).  Sounds like you and Johannes disagree.  I don't feel
super strongly about this item, but it'd probably be good to get some
other git folks' opinions on this particular point.

Other than that unresolved question, and the separate one about
whether to go with a different option instead (e.g.
merge.defaultStrategy), as being discussed elsewhere in this thread,
the patch looks good to me.

[PATCH v2 1/2] merge: Add merge.renames config setting

[Ben Peart]

Add the ability to control rename detection for merge via a config setting.

Reviewed-by: Johannes Schindelin 
Signed-off-by: Ben Peart 
---
 Documentation/diff-config.txt  | 3 ++-
 Documentation/merge-config.txt | 8 +++-
 Documentation/merge-strategies.txt | 6 --
 builtin/merge.c| 1 +
 merge-recursive.c  | 1 +
 5 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 5ca942ab5e..77caa66c2f 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -112,7 +112,8 @@ diff.orderFile::
 
 diff.renameLimit::
The number of files to consider when performing the copy/rename
-   detection; equivalent to the 'git diff' option `-l`.
+   detection; equivalent to the 'git diff' option `-l`. This setting
+   has no effect if rename detection is turned off.
 
 diff.renames::
Whether and how Git detects renames.  If set to "false",
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 12b6bbf591..0540c44e23 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -35,7 +35,13 @@ include::fmt-merge-msg-config.txt[]
 merge.renameLimit::
The number of files to consider when performing rename detection
during a merge; if not specified, defaults to the value of
-   diff.renameLimit.
+   diff.renameLimit. This setting has no effect if rename detection
+   is turned off.
+
+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.
 
 merge.renormalize::
Tell Git that canonical representation of files in the
diff --git a/Documentation/merge-strategies.txt 
b/Documentation/merge-strategies.txt
index 4a58aad4b8..1e0728aa12 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -84,12 +84,14 @@ no-renormalize;;
`merge.renormalize` configuration variable.
 
 no-renames;;
-   Turn off rename detection.
+   Turn off rename detection. This overrides the `merge.renames`
+   configuration variable.
See also linkgit:git-diff[1] `--no-renames`.
 
 find-renames[=];;
Turn on rename detection, optionally setting the similarity
-   threshold.  This is the default.
+   threshold.  This is the default. This overrides the
+   'merge.renames' configuration variable.
See also linkgit:git-diff[1] `--find-renames`.
 
 rename-threshold=;;
diff --git a/builtin/merge.c b/builtin/merge.c
index 8746c5e3e8..3be52cd316 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -424,6 +424,7 @@ static void finish(struct commit *head_commit,
opts.output_format |=
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
opts.detect_rename = DIFF_DETECT_RENAME;
+   git_config_get_bool("merge.renames", _rename);
diff_setup_done();
diff_tree_oid(head, new_head, "", );
diffcore_std();
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", >verbosity);
git_config_get_int("diff.renamelimit", >diff_rename_limit);
git_config_get_int("merge.renamelimit", >merge_rename_limit);
+   git_config_get_bool("merge.renames", >detect_rename);
git_config(git_xmerge_config, NULL);
 }
 
-- 
2.17.0.windows.1