add additional config settings for merge

Documentation/merge-config.txt +0 -0

 merge.renormalize::
        Tell Git that canonical representation of files in the
        repository has changed over time (e.g. earlier commits record

merge-recursive.c +0 -0

index 9c05eb7f70..cd5367e890 100644

Quoted 6 lines · Ben Peart

Documentation/merge-config.txt +0 -0

@@ -37,6 +37,11 @@ merge.renameLimit::
        during a merge; if not specified, defaults to the value of
        diff.renameLimit.
 

Documentation/merge-config.txt merge.renameLimit:: · line 43

Quoted 4 lines

Documentation/merge-config.txt +4 -0

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

Quoted 7 lines · Elijah Newren

Quoted 6 lines · Ben Peart

Documentation/merge-config.txt +0 -0

@@ -37,6 +37,11 @@ merge.renameLimit::
         during a merge; if not specified, defaults to the value of
         diff.renameLimit.
 

Quoted 13 lines

Documentation/merge-config.txt merge.renameLimit:: · line 43

Quoted 4 lines

Documentation/merge-config.txt +4 -0

+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. 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 just realized another issue, though it also affects -Xno-renames. Even if rename detection is turned off for the merge, it is unconditionally turned on for the diffstat. In builtin/merge.c, function finish(), there is the code:

     if (new_head && show_diffstat) {
         ...
         opts.detect_rename = DIFF_DETECT_RENAME;

It seems that this option should affect that line as well. (Do you have diffstat turned off by chance? If not, you may be able to improve your performance even more...)

Quoted 6 lines · Ben Peart

Documentation/merge-config.txt +0 -0

@@ -37,6 +37,11 @@ merge.renameLimit::
         during a merge; if not specified, defaults to the value of
         diff.renameLimit.
 

Documentation/merge-config.txt merge.renameLimit:: · line 43

Quoted 4 lines

Documentation/merge-config.txt +4 -0

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

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

Quoted 10 lines

Documentation/merge-config.txt +0 -0

  merge.renormalize::
         Tell Git that canonical representation of files in the
         repository has changed over time (e.g. earlier commits record

merge-recursive.c +0 -0

index 9c05eb7f70..cd5367e890 100644
@@ -3256,6 +3256,7 @@ static void merge_recursive_config(struct merge_options *o)
         git_config_get_int("merge.verbosity", &o->verbosity);

merge-recursive.c static void merge_recursive_config(struct merge_options *o) · line 3261

Quoted 5 lines

merge-recursive.c +1 -0

         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?

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?

Quoted 10 lines · Elijah Newren

Quoted 7 lines · Ben Peart

Documentation/merge-config.txt +0 -0

@@ -37,6 +37,11 @@ merge.renameLimit::
         during a merge; if not specified, defaults to the value of
         diff.renameLimit.
 

Quoted 8 lines

Documentation/merge-config.txt merge.renameLimit:: · line 43

Quoted 4 lines

Documentation/merge-config.txt +4 -0

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

Yes, command line options override the config settings.

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Documentation/merge-config.txt +4 -0

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

Quoted 2 lines

Quoted 2 lines

One can already control o->detect_rename via the -Xno-renames and -Xfind-renames 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.

Quoted 6 lines · Elijah Newren

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Documentation/merge-config.txt +4 -0

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

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

One can already control o->detect_rename via the -Xno-renames and -Xfind-renames 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.

Yes, and if we are adding a new configuration, we should do so in such a way that we do not have to come back and extend it when we know what the command line option does and the configuration being proposed is less capable already.

single configuration whose value can be "never" to pretend as if "--Xno-renames" were given, and some similarity score like "50" to pretend as if "--Xfind-renames=50" were given.

That is, merge.renames does not have to a simple "yes-no to control the --Xno-renames option". And it would of course be better to document it.

I also had to wonder how "merge -s resolve" faired, if the project is not interested in renamed paths at all.

Thanks.

Quoted 3 lines

I also had to wonder how "merge -s resolve" faired, if the project is not interested in renamed paths at all.

To be clear, it isn't that we're not interested in detecting renamed files and paths. We're just opposed to it taking an hour to figure that out!

Quoted 7 lines · Ben Peart

Quoted 3 lines

I also had to wonder how "merge -s resolve" faired, if the project is not interested in renamed paths at all.

To be clear, it isn't that we're not interested in detecting renamed files and paths. We're just opposed to it taking an hour to figure that out!

Yeah, but as opposed to passing "oh, let's see if we can get a reasonable result without rename detection just this time" from the command line, configuring merge.renames=false in would mean exactly
that: "we don't need rename detection, just want to skip the cycles spent for it". That is why I wondered how well the resolve strategy would have fit your needs.

Hi Junio,

Quoted 6 lines · Junio C Hamano

Yeah, but as opposed to passing "oh, let's see if we can get a reasonable result without rename detection just this time" from the command line, configuring merge.renames=false in would mean exactly
that: "we don't need rename detection, just want to skip the cycles spent for it". That is why I wondered how well the resolve strategy would have fit your needs.

Please do not forget that the context is GVFS, where you would cause a lot of pain and suffering by letting users forget to specify that command-line option all the time, resulting in several gigabytes of objects having to be downloaded just for the sake of rename detection.

So there is a pretty good point in doing this as a config option.

Quoted 10 lines · Johannes Schindelin

Quoted 6 lines · Junio C Hamano

Yeah, but as opposed to passing "oh, let's see if we can get a reasonable result without rename detection just this time" from the command line, configuring merge.renames=false in would mean exactly
that: "we don't need rename detection, just want to skip the cycles spent for it". That is why I wondered how well the resolve strategy would have fit your needs.

Quoted 6 lines

Please do not forget that the context is GVFS, where you would cause a lot of pain and suffering by letting users forget to specify that command-line option all the time, resulting in several gigabytes of objects having to be downloaded just for the sake of rename detection.

So there is a pretty good point in doing this as a config option.

I agree you need a config option, but I think Junio has a good point that it's worth at least checking out the possibility of a different one. In particular, you could add a merge.defaultStrategy (or maybe merge.twohead to be similar to pull.twohead??) that is set to 'resolve', and use that to avoid rename detection.

Perhaps performance considerations rule out the resolve strategy and favor recursive, or maybe you need the 'recursive' part of the recursive strategy (rather than the rename part), or perhaps there's some other special reason you need to go this route, but since you are avoiding renames right now it's at least worth considering the resolve strategy.

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.

Quoted 5 lines · Elijah Newren

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.

I wonder here what the relation to the diff.* options should be in this regard anyway. There is already diff.renames. Naively, I would assume that these options are in sync, that is you control the behavior of both the normal diff family like git show and git merge. The reasoning, at least for me, is to keep consistency between the outcome of rename detection while merging and a later simple "git show MERGE_BASE..HEAD". I would expect those to give me the same style of rename detection.

Hence, I would like to use diff.renames and maybe enhance this option to also carry the score in backward compatible way (or introduce a second configuration option?). Is this idea going in a good direction? If yes, I will try to submit a patch for this.

Ah, by the way: for people that have not touched diff.renames there will be no visible change in how Git behaves - the default for diff.renames is a rename with 50% score with is the same for merge. So it will only change if one has tweaked diff.renames already. But I wonder if one does that and expect the merge to use a different rename detection anyway.

Greetings,
Eckhard

In commit 2a2ac926547 when merge.renamelimit was added, it was decided to have separate settings for merge and diff to give users the ability to control that behavior. In this particular case, it will default to the value of diff.renamelimit when it isn't set. That isn't consistent with the other merge settings.

Quoted 5 lines · Ben Peart

In commit 2a2ac926547 when merge.renamelimit was added, it was decided to have separate settings for merge and diff to give users the ability to control that behavior. In this particular case, it will default to the value of diff.renamelimit when it isn't set. That isn't consistent with the other merge settings.

However, it seems like a desirable way to do it.

Maybe let me throw in some code for discussion (test and documentation is missing, mainly to form an idea what the change in options should be). I admit the patch below is concerned only with diff.renames, but whatever we come up with for merge should be reflected there, too, doesn't it >
Greetings,
Eckhard

-- >8 --

From e8a88111f2aaf338a4c19e83251c7178f7152129 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Eckhard=20S=2E=20Maa=C3=9F?= <[email protected]>
Date: Sun, 22 Apr 2018 23:29:08 +0200
Subject: [PATCH] diff: enhance diff.renames to be able to set rename score
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by Eckhard S. Maaß <[email protected]>

---
  diff.c | 35 ++++++++++++++++++++++++++++-------
  1 file changed, 28 insertions(+), 7 deletions(-)

diff.c +28 -7

index 1289df4b1f..a3cedad5cf 100644
@@ -30,6 +30,7 @@
  #endif
 
  static int diff_detect_rename_default;
+static int diff_rename_score_default;
  static int diff_indent_heuristic = 1;
  static int diff_rename_limit_default = 400;
  static int diff_suppress_blank_empty;

@@ -177,13 +178,33 @@ static int parse_submodule_params(struct diff_options *options, const char *valu
  	return 0;
  }
 
+int parse_rename_score(const char **cp_p);
+
+static int git_config_rename_score(const char *value)
+{
+	int parsed_rename_score = parse_rename_score(&value);
+	if (parsed_rename_score == -1)
+		return error("invalid argument to diff.renamescore: %s", value);
+	diff_rename_score_default = parsed_rename_score;
+	return 0;
+}
+
  static int git_config_rename(const char *var, const char *value)
  {
-	if (!value)
-		return DIFF_DETECT_RENAME;
-	if (!strcasecmp(value, "copies") || !strcasecmp(value, "copy"))
-		return  DIFF_DETECT_COPY;
-	return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
+	if (!value) {
+		diff_detect_rename_default = DIFF_DETECT_RENAME;
+		return 0;
+	}
+	if (skip_to_optional_arg(value, "copies", &value) || skip_to_optional_arg(value, "copy", &value)) {
+		diff_detect_rename_default = DIFF_DETECT_COPY;
+		return git_config_rename_score(value);
+	}
+	if (skip_to_optional_arg(value, "renames", &value) || skip_to_optional_arg(value, "rename", &value)) {
+		diff_detect_rename_default = DIFF_DETECT_RENAME;
+		return git_config_rename_score(value);
+	}
+	diff_detect_rename_default = git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
+	return 0;
  }
 
  long parse_algorithm_value(const char *value)

@@ -307,8 +328,7 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
  		return 0;
  	}
  	if (!strcmp(var, "diff.renames")) {
-		diff_detect_rename_default = git_config_rename(var, value);
-		return 0;
+		return git_config_rename(var, value);
  	}
  	if (!strcmp(var, "diff.autorefreshindex")) {
  		diff_auto_refresh_index = git_config_bool(var, value);

@@ -4116,6 +4136,7 @@ void diff_setup(struct diff_options *options)
  	options->add_remove = diff_addremove;
  	options->use_color = diff_use_color_default;
  	options->detect_rename = diff_detect_rename_default;
+	options->rename_score = diff_rename_score_default;
  	options->xdl_opts |= diff_algorithm;
  	if (diff_indent_heuristic)
  		DIFF_XDL_SET(options, INDENT_HEURISTIC);

Hi Ben,

Quoted 12 lines · Ben Peart

Quoted 10 lines · Elijah Newren

Quoted 7 lines · Ben Peart

Documentation/merge-config.txt +0 -0

@@ -37,6 +37,11 @@ merge.renameLimit::
          during a merge; if not specified, defaults to the value of
          diff.renameLimit.
 

Quoted 8 lines

Quoted 8 lines

Documentation/merge-config.txt merge.renameLimit:: · line 43

Quoted 4 lines

Documentation/merge-config.txt +4 -0

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

Quoted 8 lines

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

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.

Add the ability to control the aggressive flag passed to read-tree via a config setting.

Quoted 1 line · Ben Peart

Add the ability to control the aggressive flag passed to read-tree via a config setting.

This feels like a workaround to the performance problems with index updates in merge-recursive.c.

this when rename detection is turned off. In fact, I think you'd automatically want to set aggressive to true whenever rename detection is turned off (whether by your merge.renames option or the -Xno-renames flag).

Quoted 1 line

I can't think of any reason this setting would be useful separate from

turning rename detection off, and it'd actively harm rename detection performance improvements I have in the pipeline. I'd really prefer to not add this option, and instead combine the setting of aggressive with the other flag. Do you have an independent reason for wanting this?

Thanks,
Elijah

Quoted 8 lines · Elijah Newren

Quoted 3 lines · Ben Peart

Add the ability to control the aggressive flag passed to read-tree via a config setting.

This feels like a workaround to the performance problems with index updates in merge-recursive.c.

This change wasn't done to solve performance problems. We turned it on because it reduced the number of unmerged entries (from 40K to 1) in the particular merge we were looking at. The additional 3 scenarios that --aggressive resolves made that much difference.

That said, it makes sense to me to do

Quoted 11 lines

this when rename detection is turned off. In fact, I think you'd automatically want to set aggressive to true whenever rename detection is turned off (whether by your merge.renames option or the -Xno-renames flag).

Quoted 1 line

I can't think of any reason this setting would be useful separate from

turning rename detection off, and it'd actively harm rename detection performance improvements I have in the pipeline. I'd really prefer to not add this option, and instead combine the setting of aggressive with the other flag. Do you have an independent reason for wanting this?

While combining them would work for our specific use scenario (since we turn both on already along with turning off merge.stat), I really hesitate to tie these two different flags and code paths together with a single config setting.

While I don't want to needlessly complicate your optimizations in this area (they are already complex enough!) I believe we need to keep the option to turn on --aggressive without turning off rename detection as a viable option. Perhaps if that is the case, your optimizations have less impact or don't apply but the user should be able to make that choice for their specific situation.

That said, it makes sense to me to do

Quoted 5 lines

this when rename detection is turned off. In fact, I think you'd automatically want to set aggressive to true whenever rename detection is turned off (whether by your merge.renames option or the -Xno-renames flag).
...

While combining them would work for our specific use scenario (since we turn both on already along with turning off merge.stat), I really hesitate to tie these two different flags and code paths together with a single config setting.

Quoted 11 lines · Ben Peart

That said, it makes sense to me to do

Quoted 5 lines

this when rename detection is turned off. In fact, I think you'd automatically want to set aggressive to true whenever rename detection is turned off (whether by your merge.renames option or the -Xno-renames flag).
...

While combining them would work for our specific use scenario (since we turn both on already along with turning off merge.stat), I really hesitate to tie these two different flags and code paths together with a single config setting.

The cases that non-agressive variant leaves unmerged are not auto-resolved only because marking them as merged will rob the chance from the rename detection logic to notice which ones are "new" paths that could be matched with "deleted" ones to turn into renames. If rename deteciton is not done, there is no reason to leave it non aggressive, as "#1 = missing, #2 = something and #3 = missing" entry (just one example that is not auto-resolved by non-agressive, but the principle is the same) left unmerged in the index will get resolved to keep the current entry by the post processing logic anyway.

In fact, checking git-merge-resolve would tell us that we already use "aggresive" variant there unconditionally.

So, I think Elijah is correct---there is no reason not to enable this setting when the other one to refuse rename detection is in effect.

This enables the user to set a couple of additional options for merge.

1. merge.aggressive - this is to try to resolve a few more trivial merge cases. It is documented in read-tree and is not something you can pass into merge itself.

2. merge.renames - this is to save git from having to go through the entire 3 trees to see if there were any renames that happened.

For the work item repro that I have been using this drops the merge time from ~1 hour to ~5 minutes and the unmerged entries goes down from ~40,000 to 1.

Quoted 9 lines · Ben Peart

This enables the user to set a couple of additional options for merge.

1. merge.aggressive - this is to try to resolve a few more trivial merge cases. It is documented in read-tree and is not something you can pass into merge itself.

2. merge.renames - this is to save git from having to go through the entire 3 trees to see if there were any renames that happened.

Quoted 3 lines

For the work item repro that I have been using this drops the merge time from ~1 hour to ~5 minutes and the unmerged entries goes down from ~40,000 to 1.

Ooh, this is *very* interesting. Is there any chance I could also get you to test performing the same merge with the version of git at
https://github.com/newren/git/tree/big-repo-small-cherry-pick and report on your timings?

The 'big-repo-small-cherry-pick' name could be improved, but that branch has a number of performance fixes for really poor rename detection performance during merges. From your description, I'm pretty sure it'll apply to your case. For my specific testcase, I got a speedup factor of 30. Someone else on the list saw a factor of 24[1]. Results are highly dependent on the specific repo, but it's certainly possible that it gets much of your factor of 12 speedup that you saw with these new config settings you added.

However, what makes this case even more interesting to me is that my branch may not be quite as effective as your workarounds. There are other other performance issues in merge that I am aware of, but for which I haven't had the time to write the patches yet (I've been waiting for the directory rename detection stuff to land and settle down before working more on the performance aspects). I do not know how big a factor those other performance issues are, but your workarounds (namely the aggressive setting) may get around some of those other issues as well, so I'm very interested to see how my current branch compares to the speedups you got with these settings.

Thanks,
Elijah

[1] https://public-inbox.org/git/[email protected]/

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

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

builtin/merge.c static void finish(struct commit *head_commit, · line 430

Quoted 12 lines · Ben Peart

builtin/merge.c +1 -0

index 8746c5e3e8..3be52cd316 100644
@@ -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", &opts.detect_rename);
                 diff_setup_done(&opts);
                 diff_tree_oid(head, new_head, "", &opts);
                 diffcore_std(&opts);

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?

Quoted 12 lines · Elijah Newren

Quoted 10 lines · Ben Peart

builtin/merge.c +1 -0

index 8746c5e3e8..3be52cd316 100644
@@ -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",

Quoted 9 lines

Quoted 4 lines

builtin/merge.c +0 -0

 &opts.detect_rename);
                 diff_setup_done(&opts);
                 diff_tree_oid(head, new_head, "", &opts);

                 diffcore_std(&opts);

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.

Documentation/diff-config.txt diff.orderFile:: · line 116

Quoted 5 lines · Ben Peart

diff.renameLimit::
The number of files to consider when performing the copy/rename

Documentation/diff-config.txt +2 -1

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

You mean "turned off via diff.renames"?

This is not meant as a suggestion to rewrite this paragraph further---but if the answer is "no", then that might be an indication that the sentence is inviting a misunderstanding.

Documentation/merge-config.txt include::fmt-merge-msg-config.txt[] · line 43

Quoted 13 lines

diff.renames::
Whether and how Git detects renames. If set to "false",

Documentation/merge-config.txt +2 -1

index 5a9ab969db..38492bcb98 100644
@@ -39,7 +39,8 @@ 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.

Ditto. If your design is to make the merge machinery completely ignore diff.renames and only pay attention to merge.renames [*1*], then it probably is a good idea to be more specific here, by saying "... is turned off via ...", though.

Documentation/merge-config.txt include::fmt-merge-msg-config.txt[] · line 46

Quoted 2 lines

Documentation/merge-config.txt +0 -0

  merge.renames::
  	Whether and how Git detects renames.  If set to "false",

[Footnote]

*1* ...which I do not think is such a good idea, by the way. I'd personally expect merge.renames to allow overriding and falling back to diff.renames, just like the {merge,diff}.renameLimit pair does.

To be clear, this documentation change isn't trying to document any changes to the code or behavior - it is just an attempt to clarify what the existing behavior is.

Update the documentation to better indicate that the renameLimit setting is ignored if rename detection is turned off via command line options or config settings.

Signed-off-by Ben Peart <[email protected]>

---
 Documentation/diff-config.txt  | 3 ++-
 Documentation/merge-config.txt | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

Documentation/diff-config.txt +2 -1

index 5ca942ab5e..77caa66c2f 100644
@@ -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",

Documentation/merge-config.txt +2 -2

index 12b6bbf591..48ee3bce77 100644
@@ -35,7 +35,8 @@ 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.renormalize::
        Tell Git that canonical representation of files in the
--

2.17.0.windows.1

Patch looks fine, but it's hard for me not to notice a separate issue in this area independent of your series: I'm curious if we should document that the value of 0 is special here (as per Jonathan Tan's commit 89973554b52c ("diffcore-rename: make diff-tree -l0 mean -l<large>", 2017-11-29)), and doesn't actually drop the limit to 0. cc'ing Jonathan Tan for his thoughts.

merge-recursive.h +2 -1

index 80d69d1401..0c5f7eff98 100644
@@ -17,7 +17,8 @@ struct merge_options {
        unsigned renormalize : 1;
        long xdl_opts;
        int verbosity;
-       int detect_rename;
+       int diff_detect_rename;
+       int merge_detect_rename;
        int diff_rename_limit;
        int merge_rename_limit;
        int rename_score;

Documentation/merge-config.txt merge.renameLimit:: · line 46

Quoted 6 lines · Ben Peart

Documentation/merge-config.txt +6 -0

+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.  If set to "copies" or "copy", Git will
+       detect copies, as well.  Defaults to the value of diff.renames.
+

We shouldn't allow users to force copy detection on for merges The diff side of the code will detect them correctly but the code in merge-recursive will mishandle the copy pairs. I think fixing it is somewhere between big can of worms and it's-a-configuration-that-doesn't-even-make-sense, but it's been a while since I thought about it.

Quoted 16 lines

merge-recursive.h +2 -1

index 80d69d1401..0c5f7eff98 100644
@@ -17,7 +17,8 @@ struct merge_options {
         unsigned renormalize : 1;
         long xdl_opts;
         int verbosity;
-       int detect_rename;
+       int diff_detect_rename;
+       int merge_detect_rename;
         int diff_rename_limit;
         int merge_rename_limit;
         int rename_score;

merge-recursive.h struct merge_options { · line 36

Quoted 8 lines

merge-recursive.h +5 -0

@@ -28,6 +29,11 @@ struct merge_options {
         struct hashmap current_file_dir_set;
         struct string_list df_conflict_file_set;
  };
+inline int merge_detect_rename(struct merge_options *o)
+{
+       return o->merge_detect_rename >= 0 ? o->merge_detect_rename :
+               o->diff_detect_rename >= 0 ? o->diff_detect_rename : 1;
+}

Why did you split o->detect_rename into two fields? You then recombine them in merge_detect_rename(), and after initial setup only ever access them through that function. Having two fields worries me that people will accidentally introduce bugs by using one of them instead of the merge_detect_rename() function. Is there a reason you decided against having the initial setup just set a single value and then use it directly?

Color me puzzled. :) The consensus was that the default value for merge.renames come from diff.renames. diff.renames supports copy detection which means that merge.renames will inherit that value. My assumption was that is what was intended so when I reimplemented it, I fully implemented it that way.

Are you now requesting to only use diff.renames as the default if the value is true or false but not if it is copy? What should happen if diff.renames is actually set to copy? Should merge silently change that to true, display a warning, error out, or something else? Do you have some other behavior for how to handle copy being inherited from diff.renames you'd like to see?

Can you write the documentation that clearly explains the exact behavior you want? That would kill two birds with one stone... :)

Quoted 13 lines · Ben Peart

Color me puzzled. :) The consensus was that the default value for merge.renames come from diff.renames. diff.renames supports copy detection which means that merge.renames will inherit that value. My assumption was that is what was intended so when I reimplemented it, I fully implemented it that way.

Are you now requesting to only use diff.renames as the default if the value is true or false but not if it is copy? What should happen if diff.renames is actually set to copy? Should merge silently change that to true, display a warning, error out, or something else? Do you have some other behavior for how to handle copy being inherited from diff.renames you'd like to see?

Quoted 2 lines

Can you write the documentation that clearly explains the exact behavior you want? That would kill two birds with one stone... :)

I think demoting from copy to rename-only is a good idea, at least for now, because I do not believe we have figured out what we want to happen when we detect copied files are involved in a merge.

But I am not sure if we even want to fail merge.renames=copy as an invalid configuration. So my gut feeling of the best solution to the above is to do something like:

- whether the configuration comes from diff.renames or merge.renames, turn *.renames=copy to true inside the merge recursive machinery.

- document the fact in "git merge-recursive" documentation (or "git merge" documentation) to say "_currently_ asking for rename detection to find copies and renames will do the same thing---copies are ignored", impliying "this might change in the future", in the BUGS section.

Quoted 30 lines · Junio C Hamano

Quoted 13 lines · Ben Peart

Color me puzzled. :) The consensus was that the default value for merge.renames come from diff.renames. diff.renames supports copy detection which means that merge.renames will inherit that value. My assumption was that is what was intended so when I reimplemented it, I fully implemented it that way.

Are you now requesting to only use diff.renames as the default if the value is true or false but not if it is copy? What should happen if diff.renames is actually set to copy? Should merge silently change that to true, display a warning, error out, or something else? Do you have some other behavior for how to handle copy being inherited from diff.renames you'd like to see?

Quoted 2 lines

Can you write the documentation that clearly explains the exact behavior you want? That would kill two birds with one stone... :)

I think demoting from copy to rename-only is a good idea, at least for now, because I do not believe we have figured out what we want to happen when we detect copied files are involved in a merge.

But I am not sure if we even want to fail merge.renames=copy as an invalid configuration. So my gut feeling of the best solution to the above is to do something like:

- whether the configuration comes from diff.renames or merge.renames, turn *.renames=copy to true inside the merge recursive machinery.

Quoted 5 lines

- document the fact in "git merge-recursive" documentation (or "git merge" documentation) to say "_currently_ asking for rename detection to find copies and renames will do the same thing---copies are ignored", impliying "this might change in the future", in the BUGS section.

Yes, I agree.

Hi,

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.

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.

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.

I think demoting from copy to rename-only is a good idea, at least for now, because I do not believe we have figured out what we want to happen when we detect copied files are involved in a merge.

Quoted 3 lines · Junio C Hamano

I think demoting from copy to rename-only is a good idea, at least for now, because I do not believe we have figured out what we want to happen when we detect copied files are involved in a merge.

Does anyone know some threads concerning that topic? I tried to search for it, but somehow "merge copy detection" did not find me useful threads so far. I would be interested in that topic anyway, so I would like to know what the ideas are that floated so far.

I doubt it has ever been discussed before this thread. But, if you're curious, I'll try to dump a few thoughts.

[...] Let's say we have branches
A and B, and:
A: modifies file z
B: copies z to y

Should the modifications to z done in A propagate to both z and y? If not, what good is copy detection? If so, then there are several ramifications...

- If B not only copied z but also first modified it, then do we have potential conflicts with both z and y -- possibly the exact same conflicts, making the user resolve them repeatedly?

- What if A copied z to x? Do changes to z propagate to all three of z and x and y? Do changes to either x or y affect z? Do they affect each other?

- If A deleted z, does that give us a copy/delete conflict for y? Do we also have to worry about copy/add conflicts? copy/add/delete? rename/copy (multiple variants)? copy/copy?

- Extra degrees of freedom may mean new conflict types:

- The extra degrees of freedom from renames introduced multiple new conflict types (e.g. rename/add, rename/rename(1to2), rename/rename(2to1)).

The more I think about it, the more I think that attempting to detect copies in a merge algorithm just doesn't make sense. Anything I can think of that someone might attempt to use detected copies for would just surprise users in a bad way...

Quoted 2 lines · Elijah Newren

I doubt it has ever been discussed before this thread. But, if you're curious, I'll try to dump a few thoughts.

Thank you, I try to dump some of mine, too. Maybe let me first stress that for me copy detection without --find-copies-harder is much more a "find content extracted" (like methods being factored out). In a way this is nearer to a rename than to a real copy.

I admit that a "real" copy would get unnoticed that way. But the semantics of such a copy isn't too clear for me either - did I copy the other part to make it independent of the other or did I just employ a copy and paste tactic? The former does not want the changes, the later does. But I am happy catering to the former here.

Quoted 28 lines · Elijah Newren

Quoted 15 lines · Ben Peart

merge-recursive.h +2 -1

index 80d69d1401..0c5f7eff98 100644
@@ -17,7 +17,8 @@ struct merge_options {
         unsigned renormalize : 1;
         long xdl_opts;
         int verbosity;
-       int detect_rename;
+       int diff_detect_rename;
+       int merge_detect_rename;
         int diff_rename_limit;
         int merge_rename_limit;
         int rename_score;

merge-recursive.h struct merge_options { · line 36

Quoted 8 lines

merge-recursive.h +5 -0

@@ -28,6 +29,11 @@ struct merge_options {
         struct hashmap current_file_dir_set;
         struct string_list df_conflict_file_set;
  };
+inline int merge_detect_rename(struct merge_options *o)
+{
+       return o->merge_detect_rename >= 0 ? o->merge_detect_rename :
+               o->diff_detect_rename >= 0 ? o->diff_detect_rename : 1;
+}

Quoted 8 lines

Why did you split o->detect_rename into two fields? You then recombine them in merge_detect_rename(), and after initial setup only ever access them through that function. Having two fields worries me that people will accidentally introduce bugs by using one of them instead of the merge_detect_rename() function. Is there a reason you decided against having the initial setup just set a single value and then use it directly?

The setup of this value is split into 3 places that may or may not all get called. The initial values, the values that come from the config settings and then any values passed on the command line.

Because the merge value can now inherit from the diff value, you only know the final value after you have received all possible inputs. That makes it necessary to be a calculated value.

If you look at diff_rename_limit/merge_rename_limit, detect_rename follow the same pattern for the same reasons. It turns out detect_rename was a little more complex because it is used in 3 different locations (vs just one) which is why I wrapped the inheritance logic into the helper function merge_detect_rename().

Can you write the documentation that clearly explains the exact behavior you want? That would kill two birds with one stone... :)

From: Elijah Newren <[email protected]>

Quoted 2 lines · Ben Peart

Can you write the documentation that clearly explains the exact behavior you want? That would kill two birds with one stone... :)

Sure, something like the following is what I envision, and I've tried to include the suggestion from Junio to document the copy behavior in the merge-recursive documentation.

-- 8< --
Subject: [PATCH] fixup! merge: Add merge.renames config setting

---
  Documentation/merge-config.txt     | 3 +--
  Documentation/merge-strategies.txt | 5 +++--
  merge-recursive.c                  | 8 ++++++++
  3 files changed, 12 insertions(+), 4 deletions(-)

Documentation/merge-config.txt +1 -2

index 59848e5634..662c2713ca 100644
@@ -41,8 +41,7 @@ merge.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.  If set to "copies" or "copy", Git will
-	detect copies, as well.  Defaults to the value of diff.renames.
+	detection is enabled.  Defaults to the value of diff.renames.
 
  merge.renormalize::
  	Tell Git that canonical representation of files in the

Documentation/merge-strategies.txt +3 -2

index 1e0728aa12..aa66cbe41e 100644
@@ -23,8 +23,9 @@ recursive::
  	causing mismerges by tests done on actual merge commits
  	taken from Linux 2.6 kernel development history.
  	Additionally this can detect and handle merges involving
-	renames.  This is the default merge strategy when
-	pulling or merging one branch.
+	renames, but currently cannot make use of detected
+	copies.  This is the default merge strategy when pulling
+	or merging one branch.
  +
  The 'recursive' strategy can take the following options:
 

merge-recursive.c +1 -0

index 6cc4404144..b618f134d2 100644
@@ -564,6 +564,14 @@ static struct string_list *get_renames(struct merge_options *o,
  	opts.flags.recursive = 1;
  	opts.flags.rename_empty = 0;
  	opts.detect_rename = merge_detect_rename(o);
+	/*

Quoted 14 lines · Elijah Newren

From: Elijah Newren <[email protected]>

Quoted 3 lines · Ben Peart

Can you write the documentation that clearly explains the exact behavior you
want? That would kill two birds with one stone... :)

Sure, something like the following is what I envision, and I've tried to include the suggestion from Junio to document the copy behavior in the merge-recursive documentation.

Thanks Elijah. I've applied this patch and reviewed and tested it. It works and addresses the concerns around the settings inheritance from diff.renames. I still _prefer_ the simpler model that doesn't do the partial inheritance but I can use this model as well.

I'm unsure on the protocol here. Should I incorporate this patch and submit a reroll or can it just be applied as is?

Quoted 15 lines · Ben Peart

Quoted 14 lines · Elijah Newren

From: Elijah Newren <[email protected]>

Quoted 3 lines · Ben Peart

Can you write the documentation that clearly explains the exact behavior you
want? That would kill two birds with one stone... :)

Sure, something like the following is what I envision, and I've tried to include the suggestion from Junio to document the copy behavior in the merge-recursive documentation.

<snip>

Quoted 8 lines

Thanks Elijah. I've applied this patch and reviewed and tested it. It works and addresses the concerns around the settings inheritance from diff.renames. I still _prefer_ the simpler model that doesn't do the partial inheritance but I can use this model as well.

I'm unsure on the protocol here. Should I incorporate this patch and submit a reroll or can it just be applied as is?

I suspect you'll want to re-roll anyway, to base your series on en/rename-directory-detection-reboot instead of on master. (Junio plans to merge it down to next, and your series has four different merge conflicts with it.)

There are two other loose ends with this series that Junio will need to weigh in on:

- I'm obviously a strong proponent of the inherited setting, but Junio may change his mind after reading Dscho's arguments against it (or after reading my arguments for it).

- I like the setting as-is, and think we could allow a "copy" setting for merge.renames to specify that the post-merge diffstat should detect copies (not part of your series, but a useful addition I'd like to tackle afterwards). However, Junio had comments in [email protected] about merge.renames handling the scoring as well, like -Xfind-renames. Those sound incompatible to me for a single setting, and I'm unsure if Junio would resolve them the way I do or still feels strongly about the scoring.

Set aggressive flag in git_merge_trees() when rename detection is turned off. This allows read_tree() to auto resolve more cases that would have otherwise been handled by the rename detection.

Reviewed-by Johannes Schindelin <[email protected]>

Signed-off-by Ben Peart <[email protected]>

---
 merge-recursive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

merge-recursive.c +3 -2

index 2637d34d87..6cc4404144 100644
@@ -276,6 +276,7 @@ static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
 }
 
 static int git_merge_trees(int index_only,
+                          int aggressive,
                           struct tree *common,
                           struct tree *head,
                           struct tree *merge)

@@ -294,6 +295,7 @@ static int git_merge_trees(int index_only,
        opts.fn = threeway_merge;
        opts.src_index = &the_index;
        opts.dst_index = &the_index;
+       opts.aggressive = aggressive;
        setup_unpack_trees_porcelain(&opts, "merge");
 
        init_tree_desc_from_tree(t+0, common);

@@ -1993,7 +1995,7 @@ int merge_trees(struct merge_options *o,
                return 1;
        }
 
-       code = git_merge_trees(o->call_depth, common, head, merge);
+       code = git_merge_trees(o->call_depth, !merge_detect_rename(o), common, head, merge);
 
        if (code != 0) {
                if (show(o, 4) || o->call_depth)
--

2.17.0.windows.1

This is a complete rewrite based on the feedback from earlier patches.

Update the documentation to better indicate command line options that override various config settings related to merge.

Add a new config merge.renames setting to to control the rename detection behavior of merge. This setting will default to the value of diff.renames.

Also adds logic so that when rename detection is turned off, the aggressive flag is passed to read_tree() so that it can auto resolve more cases that would have been handled by rename detection.

For the repro that I have been using this drops the merge time from ~1 hour to ~5 minutes and the unmerged entries goes down from ~40,000 to 1.

Helped-by Kevin Willford <[email protected]>

Reviewed-by Johannes Schindelin <[email protected]>

Signed-off-by Ben Peart <[email protected]>

Base Ref: master

Add the ability to control rename detection for merge via a config setting. This setting behaves the same and defaults to the value of diff.renames but only applies to merge.

Reviewed-by Johannes Schindelin <[email protected]>

Helped-by Elijah Newren <[email protected]>

Signed-off-by Ben Peart <[email protected]>

...

merge-recursive.h +0 -0

index d863cf8867..c1d9b5b3d9 100644

This version incorporates Elijah's fixup patch and is now based on en/rename-directory-detection in next.