Re: [PATCH v1 1/2] merge: Add merge.renames config setting
Hi Elijah, On Tue, 24 Apr 2018, Elijah Newren wrote: > On Tue, Apr 24, 2018 at 4:58 AM, Johannes Schindelin > wrote: > > > > On Tue, 24 Apr 2018, Junio C Hamano wrote: > > > >> 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. > > 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. I would really hesitate to go to a different merge strategy. The recursive strategy really has the best track record in general, and we have to use all kinds of branching models (including heavily criss-crossed ones) with GVFS Git. Ciao, Dscho
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
Hi Dscho, On Tue, Apr 24, 2018 at 4:58 AM, Johannes Schindelin wrote: > Hi Junio, > > On Tue, 24 Apr 2018, Junio C Hamano wrote: > >> 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. 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. Elijah
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
On 4/23/2018 5:32 PM, Eckhard Maaß wrote:
On Mon, Apr 23, 2018 at 09:15:09AM -0400, Ben Peart wrote:
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.
I'm just one opinion among many but I personally believe the cascading
settings are complicated enough just with the various config files and
command line options and which overwrite the others. I'd rather not
complicate them further by having settings inherited from one feature
(diff) to another (merge).
There are currently ~15 merge specific config settings and only
merge.renamelimit currently does this inheritance. That said, at least
one other person thought it was a good idea. :)
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?=
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ß
---
diff.c | 35 ---
1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/diff.c b/diff.c
index 1289df4b1f..a3cedad5cf 100644
--- a/diff.c
+++ b/diff.c
@@ -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);
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
Hi Junio, On Tue, 24 Apr 2018, Junio C Hamano wrote: > Ben Peart writes: > > >> 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. 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. Ciao, Dscho
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
Ben Peart writes: >> 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.
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
On Mon, Apr 23, 2018 at 09:15:09AM -0400, Ben Peart wrote:
> 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?=
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ß
---
diff.c | 35 ---
1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/diff.c b/diff.c
index 1289df4b1f..a3cedad5cf 100644
--- a/diff.c
+++ b/diff.c
@@ -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);
--
2.17.0.252.gfe0a9eaf31
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
On 4/21/2018 12:23 AM, Junio C Hamano wrote: Elijah Newren writes: +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. ... 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. Between all the different command line options, config settings, merge strategies and the interactions between the diff and merge versions, I was trying to keep things as simple and consistent as possible. To that end 'merge.renames' was modeled after the existing 'diff.renames.' I wonder if we can just add a 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. With the existing differences in how these options are passed on the command line, I'm hesitant to add yet another pattern in the config settings that combines 'renames' and '--find-renames[=]'. I _have_ wondered why this all isn't configured via find-renames with find-renames=0 meaning renames=false (instead of mapping 0 to 32K). I think that could have eliminated the need for splitting rename across two different settings (which is what I think you are proposing above). I'd then want the config setting and command line option to be the same syntax and behavior. Moving the existing settings to this model and updating the config and command line options to be consistent without breaking backwards compatibility is outside the intended scope of this patch. 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! Thanks.
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
On 4/20/2018 2:34 PM, Elijah Newren wrote: Hi Ben, On Fri, Apr 20, 2018 at 10:59 AM, Ben Peart wrote: On 4/20/2018 1:02 PM, Elijah Newren wrote: On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart 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). I'm all in favor of having more information in the documentation. I'm of the opinion that if someone has made the effort to actually _read_ the documentation, we should be as descriptive and complete as possible. I'll take a cut at adding the things you have pointed out would be helpful. 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. I can add this additional documentation as well. While some might think it is stating the obvious, I'm sure someone will benefit from it being explicitly called out. 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.
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
On 4/22/2018 8:07 AM, Eckhard Maaß wrote: On Fri, Apr 20, 2018 at 11:34:25AM -0700, Elijah Newren wrote: 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. It's a fair question. If you look at all the options in Documentation/merge-config.txt, you will see many merge specific settings. I think the ability to control these settings separately is pretty well established. 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. Changing that behavior across the rest of the merge settings is outside the scope of this patch. I don't have a strong opinion as to whether that is a good or bad thing. 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
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
On 4/20/2018 1:26 PM, Elijah Newren wrote:
On Fri, Apr 20, 2018 at 10:02 AM, Elijah Newren wrote:
On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart 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. 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...)
Seems reasonable to me. I'll update the patch to do that.
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
On Fri, Apr 20, 2018 at 11:34:25AM -0700, Elijah Newren wrote: > 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
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
Elijah Newren writes: +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. > ... > 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. I wonder if we can just add a 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.
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
Hi Ben, On Fri, Apr 20, 2018 at 10:59 AM, Ben Peart wrote: > > On 4/20/2018 1:02 PM, Elijah Newren wrote: >> >> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart >> 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.) > 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.
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
On 4/20/2018 1:02 PM, Elijah Newren wrote:
On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart 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.
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
On Fri, Apr 20, 2018 at 10:02 AM, Elijah Newren wrote:
> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart 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. 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...)
Re: [PATCH v1 1/2] merge: Add merge.renames config setting
On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart 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. 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.
> 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?
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?
[PATCH v1 1/2] merge: Add merge.renames config setting
Add the ability to control rename detection for merge via a config setting.
Reviewed-by: Johannes Schindelin
Signed-off-by: Ben Peart
---
Documentation/merge-config.txt | 5 +
merge-recursive.c | 1 +
2 files changed, 6 insertions(+)
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 12b6bbf591..656f909eb3 100644
--- 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.
+
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);
}
--
2.17.0.windows.1
