[PATCH v1 0/2] add additional config settings for merge

[PATCH v1 0/2] add additional config settings for merge

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

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.

Helped-by: Kevin Willford <kewillf@microsoft.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>

Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/a3d157f5be
Checkout: git fetch https://github.com/benpeart/git merge-options-v1 && git checkout a3d157f5be

Ben Peart (2):
  merge: Add merge.renames config setting
  merge: Add merge.aggressive config setting

 Documentation/merge-config.txt | 9 +++++++++
 merge-recursive.c              | 2 ++
 2 files changed, 11 insertions(+)


base-commit: 0b0cc9f86731f894cff8dd25299a9b38c254569e
-- 
2.17.0.windows.1

[PATCH v1 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 <johannes.schindelin@gmx.de>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 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

[PATCH v1 2/2] merge: Add merge.aggressive config setting

[Ben Peart]

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

Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Documentation/merge-config.txt | 4 ++++
 merge-recursive.c              | 1 +
 2 files changed, 5 insertions(+)

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 656f909eb3..5a9ab969db 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -1,3 +1,7 @@
+merge.aggressive::
+	Passes "aggressive" to read-tree which makes the command resolve
+	a few more cases internally. See "--aggressive" in linkgit:git-read-tree[1].
+
 merge.conflictStyle::
 	Specify the style in which conflicted hunks are written out to
 	working tree files upon merge.  The default is "merge", which
diff --git a/merge-recursive.c b/merge-recursive.c
index cd5367e890..0ca84e4b82 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -355,6 +355,7 @@ static int git_merge_trees(struct merge_options *o,
 	o->unpack_opts.fn = threeway_merge;
 	o->unpack_opts.src_index = &the_index;
 	o->unpack_opts.dst_index = &the_index;
+	git_config_get_bool("merge.aggressive", (int *)&o->unpack_opts.aggressive);
 	setup_unpack_trees_porcelain(&o->unpack_opts, "merge");
 
 	init_tree_desc_from_tree(t+0, common);
-- 
2.17.0.windows.1

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

[Elijah Newren]

On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart <Ben.Peart@microsoft.com> wrote:
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -37,6 +37,11 @@ merge.renameLimit::
>         during a merge; if not specified, defaults to the value of
>         diff.renameLimit.
>
> +merge.renames::
> +       Whether and how Git detects renames.  If set to "false",
> +       rename detection is disabled. If set to "true", basic rename
> +       detection is enabled. This is the default.

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

Re: [PATCH v1 2/2] merge: Add merge.aggressive config setting

[Elijah Newren]

On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart <Ben.Peart@microsoft.com> wrote:
> 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.  That said, it makes sense to me to do
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).

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

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

[Elijah Newren]

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

[Elijah Newren]

On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart <Ben.Peart@microsoft.com> wrote:
> 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.

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/alpine.DEB.2.00.1711211303290.20686@ds9.cixit.se/

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

[Ben Peart]

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

Yes, but that requires people to know they need to do that and then 
remember to pass it on the command line every time.  We've found that 
doesn't typically happen, we just get someone complaining about slow 
merges. :)

That is why we added them as config options which change the default. 
That way we can then set them on the repo and the default behavior gives 
them better performance.  They can still always override the config 
setting with the command line options.

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

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

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

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

> Also, if someone sets merge.renameLimit (to anything) and sets
> merge.renames to false, then they've got a contradictory setup.  Does
> it make sense to check and warn about that anywhere?
> 

I don't think we need to.  The merge.renameLimit is only used if 
detect_rename it turned on no matter how that gets turned on (default, 
config setting, command line option) so there isn't really a change in 
behavior here.

Re: [PATCH v1 0/2] add additional config settings for merge

[Ben Peart]

On 4/20/2018 1:34 PM, Elijah Newren wrote:
> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart <Ben.Peart@microsoft.com> wrote:
>> 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.
> 
> 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?
> 

Unfortunately, it isn't quite that simple.  My repo is _really_ big 
(3.2M files and ~100K commits per week) and requires me to use a custom 
fork of git that works with our GVFS solution for it to work at all.

I've been watching your work in this area and am hoping it pays off for 
us if/when we have users that want to do rename detection and override 
our defaults.

> 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/alpine.DEB.2.00.1711211303290.20686@ds9.cixit.se/
> 

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

[Elijah Newren]

Hi Ben,

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

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

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

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

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

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

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

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

> Yes, command line options override the config settings.

Good.  :-)

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

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

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

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

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> 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

[Eckhard Maaß]

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

[Ben Peart]

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

[Ben Peart]

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

[Ben Peart]

On 4/20/2018 2:34 PM, Elijah Newren wrote:
> Hi Ben,
> 
> On Fri, Apr 20, 2018 at 10:59 AM, Ben Peart <peartben@gmail.com> wrote:
>>
>> On 4/20/2018 1:02 PM, Elijah Newren wrote:
>>>
>>> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart <Ben.Peart@microsoft.com>
>>> wrote:
>>>>
>>>> --- a/Documentation/merge-config.txt
>>>> +++ b/Documentation/merge-config.txt
>>>> @@ -37,6 +37,11 @@ merge.renameLimit::
>>>>           during a merge; if not specified, defaults to the value of
>>>>           diff.renameLimit.
>>>>
>>>> +merge.renames::
>>>> +       Whether and how Git detects renames.  If set to "false",
>>>> +       rename detection is disabled. If set to "true", basic rename
>>>> +       detection is enabled. This is the default.
>>>
>>>
>>> One can already control o->detect_rename via the -Xno-renames and
>>> -Xfind-renames options.
> 
> This statement wasn't meant to be independent of the sentence that
> followed it...
> 
>> Yes, but that requires people to know they need to do that and then remember
>> to pass it on the command line every time.  We've found that doesn't
>> typically happen, we just get someone complaining about slow merges. :)
>>
>> That is why we added them as config options which change the default. That
>> way we can then set them on the repo and the default behavior gives them
>> better performance.  They can still always override the config setting with
>> the command line options.
> 
> Sorry, I think I wasn't being clear.  The documentation for the config
> options for e.g. diff.renameLimit, fetch.prune, log.abbrevCommit, and
> merge.ff all mention the equivalent command line parameters.  Your
> patch doesn't do that for merge.renames, but I think it would be
> helpful if it did.
> 
> Also, a link in the documentation the other way, from
> Documentation/merge-strategies.txt under the entries for -Xno-renames
> and -Xfind-renames should probably mention this new merge.renames
> config setting (much like the -Xno-renormalize flag mentions the
> merge.renomralize config option).
> 

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

[Ben Peart]

On 4/21/2018 12:23 AM, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> 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[=<n>]'.

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

[Eckhard Maaß]

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?= <eckhard.s.maass@gmail.com>
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ß <eckhard.s.maass@gmail.com>
---
 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

[Junio C Hamano]

Ben Peart <peartben@gmail.com> 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

[Johannes Schindelin]

Hi Junio,

On Tue, 24 Apr 2018, Junio C Hamano wrote:

> Ben Peart <peartben@gmail.com> 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 2/2] merge: Add merge.aggressive config setting

[Ben Peart]

On 4/20/2018 1:22 PM, Elijah Newren wrote:
> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart <Ben.Peart@microsoft.com> wrote:
>> 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
> 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).
> > 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.

> Thanks,
> Elijah
> 

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

[Ben Peart]

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?= <eckhard.s.maass@gmail.com>
> 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ß <eckhard.s.maass@gmail.com>
> ---
>   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);
> 

[PATCH v2 0/2] add additional config settings for merge

[Ben Peart]

Updated in response to feedback.  Mostly documentation changes but the diffstat
at the end of the merge (if on) now honors the new merge.rename setting as well.

Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/653bfe6e01
Checkout: git fetch https://github.com/benpeart/git merge-options-v2 && git checkout 653bfe6e01


### Interdiff (v1..v2):

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 5a9ab969db..38492bcb98 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -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.
 
 merge.renames::
 	Whether and how Git detects renames.  If set to "false",
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[=<n>];;
 	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=<n>;;
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", &opts.detect_rename);
 		diff_setup_done(&opts);
 		diff_tree_oid(head, new_head, "", &opts);
 		diffcore_std(&opts);


### Patches

Ben Peart (2):
  merge: Add merge.renames config setting
  merge: Add merge.aggressive config setting

 Documentation/diff-config.txt      |  3 ++-
 Documentation/merge-config.txt     | 12 +++++++++++-
 Documentation/merge-strategies.txt |  6 ++++--
 builtin/merge.c                    |  1 +
 merge-recursive.c                  |  2 ++
 5 files changed, 20 insertions(+), 4 deletions(-)


base-commit: 0b0cc9f86731f894cff8dd25299a9b38c254569e
-- 
2.17.0.windows.1

[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 <johannes.schindelin@gmx.de>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 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[=<n>];;
 	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=<n>;;
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", &opts.detect_rename);
 		diff_setup_done(&opts);
 		diff_tree_oid(head, new_head, "", &opts);
 		diffcore_std(&opts);
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

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

[Ben Peart]

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

Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Documentation/merge-config.txt | 4 ++++
 merge-recursive.c              | 1 +
 2 files changed, 5 insertions(+)

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 0540c44e23..38492bcb98 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -1,3 +1,7 @@
+merge.aggressive::
+	Passes "aggressive" to read-tree which makes the command resolve
+	a few more cases internally. See "--aggressive" in linkgit:git-read-tree[1].
+
 merge.conflictStyle::
 	Specify the style in which conflicted hunks are written out to
 	working tree files upon merge.  The default is "merge", which
diff --git a/merge-recursive.c b/merge-recursive.c
index cd5367e890..0ca84e4b82 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -355,6 +355,7 @@ static int git_merge_trees(struct merge_options *o,
 	o->unpack_opts.fn = threeway_merge;
 	o->unpack_opts.src_index = &the_index;
 	o->unpack_opts.dst_index = &the_index;
+	git_config_get_bool("merge.aggressive", (int *)&o->unpack_opts.aggressive);
 	setup_unpack_trees_porcelain(&o->unpack_opts, "merge");
 
 	init_tree_desc_from_tree(t+0, common);
-- 
2.17.0.windows.1

Re: [PATCH v1 2/2] merge: Add merge.aggressive config setting

[Elijah Newren]

Hi Ben,

On Tue, Apr 24, 2018 at 9:45 AM, Ben Peart <peartben@gmail.com> wrote:
> On 4/20/2018 1:22 PM, Elijah Newren wrote:
>> On Fri, Apr 20, 2018 at 6:36 AM, Ben Peart <Ben.Peart@microsoft.com>
>> wrote:
>>>
>>> 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

Um...color me perplexed here.  aggressive exists just to do some
resolutions that higher-level strategies can and totally ought to be
able to handle easily (the rules are almost trivially
straight-forward), but deferring allows the higher level strategies
(either merge-recursive or resolve's git-merge-one-file) to handle
slightly differently (e.g. by detecting renames).  merge-recursive
should be able to resolve anything that the unpack_trees aggressive
setting handles.  If it can't, it sounds like there's a horrible bug
somewhere.

Perhaps fixing that bug is the real problem?

Is there any chance you can dig out more details about any of these
conflicts or come up with a simple testcase where running 'git merge
-X no-renames' gives a merge conflict but running with this option
would run to completion?

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

I totally buy that you need at least one option to avoid waiting for
(current) rename detection in some fashion, and that you don't want
lots of spurious conflicts.  But I don't understand why you believe
that we need to keep the option to turn on the aggressive flag
independently.  What's the usecase?  It wasn't possible before in the
code, no one else has asked for it, and even you say you don't need it
as a separate option.  Is it a concern that turning on aggressive
whenever rename-detection is turned off will break something?  The
only reason I can see to keep the aggressive codepath in unpack_trees
behind a branch instead of it always running unconditionally for every
single caller throughout the codebase is because of renames.  So the
fact that you're turning renames off, to me, suggests that aggressive
flag should automatically be turned on.  I'd even call pre-existing
code (e.g. the -X no-renames option in merge-recursive) that doesn't
turn on the aggressive flag buggy (even if the only result is
suboptimal-performance).

I don't see how an option to turn on the aggressive flag independently
is possibly useful to anyone.  Further, we have strong reason to
believe it will soon be actively harmful.  So...why?  It's totally
possible I'm just missing something.  If there's a good reason for it,
providing some kind of benefit that the user could weigh in a
tradeoff, then I can get on board with providing it as an option, but
right now I just don't see it.

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

[Elijah Newren]

Hi Dscho,

On Tue, Apr 24, 2018 at 4:58 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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 v2 1/2] merge: Add merge.renames config setting

[Elijah Newren]

On Tue, Apr 24, 2018 at 10:11 AM, Ben Peart <Ben.Peart@microsoft.com> 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.

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 <Ben.Peart@microsoft.com> 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", &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?

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 <Ben.Peart@microsoft.com> 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", &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.

Re: [PATCH v1 2/2] merge: Add merge.aggressive config setting

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> That said, it makes sense to me to do
>> 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.

Re: [PATCH v2 0/2] add additional config settings for merge

[Junio C Hamano]

Ben Peart <Ben.Peart@microsoft.com> writes:

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

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.

>  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 5a9ab969db..38492bcb98 100644
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -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.

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

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

[Johannes Schindelin]

Hi Elijah,

On Tue, 24 Apr 2018, Elijah Newren wrote:

> On Tue, Apr 24, 2018 at 4:58 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> 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 2/2] merge: Add merge.aggressive config setting

[Ben Peart]

On 4/24/2018 7:57 PM, Junio C Hamano wrote:
> Ben Peart <peartben@gmail.com> writes:
> 
>> That said, it makes sense to me to do
>>> 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.
> 

Thank you. I understand this description and it make sense to me.  I'm 
unfamiliar with the merge code so have to rely on you who are the 
experts for a sanity check.

I will remove the separate merge.aggressive config setting and instead, 
set the aggressive flag when the renames is turned off (whether by the 
new merge.renames setting or by the -Xno-renames flag) as Elijah suggests.

Re: [PATCH v2 0/2] add additional config settings for merge

[Ben Peart]

On 4/24/2018 8:13 PM, Junio C Hamano wrote:
> Ben Peart <Ben.Peart@microsoft.com> writes:
> 
>>   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.
> 
> 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.
> 

Yes, this is referring to turned off via the config setting 
"diff.renames" but it could also be turned off by passing "--no-renames" 
on the command line.

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.  If it isn't helping, I can remove it.

>>   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 5a9ab969db..38492bcb98 100644
>> --- a/Documentation/merge-config.txt
>> +++ b/Documentation/merge-config.txt
>> @@ -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.
> 
>>   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.
> 

It looks like I'm in the minority on whether the merge settings should 
inherit from the corresponding diff settings so I will submit a new 
patch series that does it the same way as the {merge,diff}.renameLimit 
pair works.

I'll leave it as an exercise for someone else [1] to change any other 
merge settings that should behave that way.

[1] 
https://public-inbox.org/git/20180420133632.17580-1-benpeart@microsoft.com/T/#m52a3dbd0945360bfb873fd3b553472558ef3b796

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

[Elijah Newren]

On Tue, Apr 24, 2018 at 1:31 PM, Ben Peart <peartben@gmail.com> wrote:
> On 4/24/2018 2:59 PM, Elijah Newren wrote:
>> On Tue, Apr 24, 2018 at 10:11 AM, Ben Peart <Ben.Peart@microsoft.com>
>> 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",
>>> &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.

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 0/2] add additional config settings for merge

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

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

Yeah, I know, and I do think the new text is a good first step in
the right direction; I merely was trying to help in the clarifying
effort ;-)

[PATCH v3 0/3] add merge.renames config setting

[Ben Peart]

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 <kewillf@microsoft.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>

Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/6a8372d517
Checkout: git fetch https://github.com/benpeart/git merge-options-v3 && git checkout 6a8372d517

### Patches

Ben Peart (3):
  merge: update documentation for {merge,diff}.renameLimit
  merge: Add merge.renames config setting
  merge: pass aggressive when rename detection is turned off

 Documentation/diff-config.txt             |  3 ++-
 Documentation/merge-config.txt            |  9 +++++++-
 Documentation/merge-strategies.txt        |  6 +++--
 diff.c                                    |  2 +-
 diff.h                                    |  2 ++
 merge-recursive.c                         | 27 +++++++++++++++++------
 merge-recursive.h                         |  8 ++++++-
 t/t3034-merge-recursive-rename-options.sh | 18 +++++++++++++++
 8 files changed, 62 insertions(+), 13 deletions(-)


base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d
-- 
2.17.0.windows.1

[PATCH v3 1/3] merge: update documentation for {merge,diff}.renameLimit

[Ben Peart]

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 <benpeart@microsoft.com>
---
 Documentation/diff-config.txt  | 3 ++-
 Documentation/merge-config.txt | 3 ++-
 2 files changed, 4 insertions(+), 2 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..48ee3bce77 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -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 v3 2/3] merge: Add merge.renames config setting

[Ben Peart]

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 <johannes.schindelin@gmx.de>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Documentation/merge-config.txt            |  6 ++++++
 Documentation/merge-strategies.txt        |  6 ++++--
 diff.c                                    |  2 +-
 diff.h                                    |  2 ++
 merge-recursive.c                         | 23 +++++++++++++++++------
 merge-recursive.h                         |  8 +++++++-
 t/t3034-merge-recursive-rename-options.sh | 18 ++++++++++++++++++
 7 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 48ee3bce77..59848e5634 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -38,6 +38,12 @@ merge.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.  If set to "copies" or "copy", Git will
+	detect copies, as well.  Defaults to the value of diff.renames.
+
 merge.renormalize::
 	Tell Git that canonical representation of files in the
 	repository has changed over time (e.g. earlier commits record
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[=<n>];;
 	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=<n>;;
diff --git a/diff.c b/diff.c
index 1289df4b1f..5dfc24aa6d 100644
--- a/diff.c
+++ b/diff.c
@@ -177,7 +177,7 @@ static int parse_submodule_params(struct diff_options *options, const char *valu
 	return 0;
 }
 
-static int git_config_rename(const char *var, const char *value)
+int git_config_rename(const char *var, const char *value)
 {
 	if (!value)
 		return DIFF_DETECT_RENAME;
diff --git a/diff.h b/diff.h
index d29560f822..806faee2b3 100644
--- a/diff.h
+++ b/diff.h
@@ -324,6 +324,8 @@ extern int git_diff_ui_config(const char *var, const char *value, void *cb);
 extern void diff_setup(struct diff_options *);
 extern int diff_opt_parse(struct diff_options *, const char **, int, const char *);
 extern void diff_setup_done(struct diff_options *);
+extern int git_config_rename(const char *var, const char *value);
+
 
 #define DIFF_DETECT_RENAME	1
 #define DIFF_DETECT_COPY	2
diff --git a/merge-recursive.c b/merge-recursive.c
index 0c0d48624d..2637d34d87 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -555,13 +555,13 @@ static struct string_list *get_renames(struct merge_options *o,
 	struct diff_options opts;
 
 	renames = xcalloc(1, sizeof(struct string_list));
-	if (!o->detect_rename)
+	if (!merge_detect_rename(o))
 		return renames;
 
 	diff_setup(&opts);
 	opts.flags.recursive = 1;
 	opts.flags.rename_empty = 0;
-	opts.detect_rename = DIFF_DETECT_RENAME;
+	opts.detect_rename = merge_detect_rename(o);
 	opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
 			    o->diff_rename_limit >= 0 ? o->diff_rename_limit :
 			    1000;
@@ -2232,9 +2232,18 @@ int merge_recursive_generic(struct merge_options *o,
 
 static void merge_recursive_config(struct merge_options *o)
 {
+	char *value = NULL;
 	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);
+	if (!git_config_get_string("diff.renames", &value)) {
+		o->diff_detect_rename = git_config_rename("diff.renames", value);
+		free(value);
+	}
+	if (!git_config_get_string("merge.renames", &value)) {
+		o->merge_detect_rename = git_config_rename("merge.renames", value);
+		free(value);
+	}
 	git_config(git_xmerge_config, NULL);
 }
 
@@ -2244,10 +2253,11 @@ void init_merge_options(struct merge_options *o)
 	memset(o, 0, sizeof(struct merge_options));
 	o->verbosity = 2;
 	o->buffer_output = 1;
+	o->diff_detect_rename = -1;
+	o->merge_detect_rename = -1;
 	o->diff_rename_limit = -1;
 	o->merge_rename_limit = -1;
 	o->renormalize = 0;
-	o->detect_rename = 1;
 	merge_recursive_config(o);
 	merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
 	if (merge_verbosity)
@@ -2298,18 +2308,19 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 	else if (!strcmp(s, "no-renormalize"))
 		o->renormalize = 0;
 	else if (!strcmp(s, "no-renames"))
-		o->detect_rename = 0;
+		o->merge_detect_rename = 0;
 	else if (!strcmp(s, "find-renames")) {
-		o->detect_rename = 1;
+		o->merge_detect_rename = 1;
 		o->rename_score = 0;
 	}
 	else if (skip_prefix(s, "find-renames=", &arg) ||
 		 skip_prefix(s, "rename-threshold=", &arg)) {
 		if ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)
 			return -1;
-		o->detect_rename = 1;
+		o->merge_detect_rename = 1;
 	}
 	else
 		return -1;
+
 	return 0;
 }
diff --git a/merge-recursive.h b/merge-recursive.h
index 80d69d1401..0c5f7eff98 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -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;
@@ -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;
+}
 
 /* merge_trees() but with recursive ancestor consolidation */
 int merge_recursive(struct merge_options *o,
diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
index b9c4028496..3d9fae68c4 100755
--- a/t/t3034-merge-recursive-rename-options.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -309,4 +309,22 @@ test_expect_success 'last wins in --find-renames=<m> --rename-threshold=<n>' '
 	check_threshold_0
 '
 
+test_expect_success 'merge.renames disables rename detection' '
+	git read-tree --reset -u HEAD &&
+	git -c merge.renames=false merge-recursive $tail &&
+	check_no_renames
+'
+
+test_expect_success 'merge.renames defaults to diff.renames' '
+	git read-tree --reset -u HEAD &&
+	git -c diff.renames=false merge-recursive $tail &&
+	check_no_renames
+'
+
+test_expect_success 'merge.renames overrides diff.renames' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git -c diff.renames=false -c merge.renames=true merge-recursive $tail &&
+	$check_50
+'
+
 test_done
-- 
2.17.0.windows.1

[PATCH v3 3/3] merge: pass aggressive when rename detection is turned off

[Ben Peart]

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 <johannes.schindelin@gmx.de>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 merge-recursive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 2637d34d87..6cc4404144 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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

Re: [PATCH v3 0/3] add merge.renames config setting

[Elijah Newren]

Hi Ben,

On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart <Ben.Peart@microsoft.com> wrote:
> This is a complete rewrite based on the feedback from earlier patches.

Thanks for pushing forward on this.

> 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 <kewillf@microsoft.com>
> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
>
> Base Ref: master

We may need to figure out how to coordinate amongst a few topics.
Looking over your patches, there are going to be a few conflicts with
en/rename-directory-detection-reboot, so this won't apply to pu.
Martin's series to introduce clear_unpack_trees_porcelain()[1], which
he was waiting to submit until mine went through, will also conflict
with this, if he uses the changes I suggested for the handling in
merge-recursive[2].  These aren't major conflicts, but I'm just
flagging it.

[1] https://public-inbox.org/git/cover.1524545557.git.martin.agren@gmail.com/
[2] https://public-inbox.org/git/20180424162939.20956-1-newren@gmail.com/

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Elijah Newren]

On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart <Ben.Peart@microsoft.com> wrote:

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

> diff --git a/merge-recursive.h b/merge-recursive.h
> index 80d69d1401..0c5f7eff98 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -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;
> @@ -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?

Re: [PATCH v3 3/3] merge: pass aggressive when rename detection is turned off

[Elijah Newren]

On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart <Ben.Peart@microsoft.com> wrote:
> 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 <johannes.schindelin@gmx.de>
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
>  merge-recursive.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 2637d34d87..6cc4404144 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -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

Patch looks fine but as a heads up -- since merge_options is a
parameter in git_merge_trees after the
en/rename-directory-detection-reboot lands, we'll be able to switch
this patch to set opts.aggressive directly instead of needing to pass
it in as a parameter.

Re: [PATCH v3 1/3] merge: update documentation for {merge,diff}.renameLimit

[Elijah Newren]

On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart <Ben.Peart@microsoft.com> wrote:
> 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 <benpeart@microsoft.com>
> ---
>  Documentation/diff-config.txt  | 3 ++-
>  Documentation/merge-config.txt | 3 ++-
>  2 files changed, 4 insertions(+), 2 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..48ee3bce77 100644
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -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.

Re: [PATCH v3 1/3] merge: update documentation for {merge,diff}.renameLimit

[Jonathan Tan]

On Thu, 26 Apr 2018 16:11:50 -0700
Elijah Newren <newren@gmail.com> wrote:

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

Documenting that the value of 0 is special does make sense to me. I
think this patch can go in as-is, though - it is already an improvement.

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Ben Peart]

On 4/26/2018 6:52 PM, Elijah Newren wrote:
> On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart <Ben.Peart@microsoft.com> wrote:
> 
>> +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.

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

> 
>> diff --git a/merge-recursive.h b/merge-recursive.h
>> index 80d69d1401..0c5f7eff98 100644
>> --- a/merge-recursive.h
>> +++ b/merge-recursive.h
>> @@ -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;
>> @@ -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?
> 

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

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

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

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.

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Elijah Newren]

On Thu, Apr 26, 2018 at 7:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ben Peart <peartben@gmail.com> writes:
>
>> 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... :)
>
> 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.

Yes, I agree.  One more thing:

  - It may be best to avoid advertising "copies" as a vaild option for
merge.renames since it doesn't have any current practical use
anywhere.  (Remove the sentence 'If set to "copies" or "copy", Git
will detect copies, as well.' from the documentation)

My rationale for translating "copy" to "true" is a little different
than Junio's, though:

1) The reason we have configuration options around renames and copies
is primarily because they are expensive to compute.  So we let some
users specify that they don't want them, other users are willing to
pay for rename detection, and others are willing to pay for both
rename and copy detection.
2) If rename/copy detection were cheap, every part of git would just
compute whatever level of detection was relevant and use it.
3) The resolve and octopus merge strategies ignores diff.renames and
merge.renames, because they don't have logic to use any rename
information.  diff and log can use both renames and copies.  And the
recursive merge machinery is code which can use renames but not
copies.
4) Therefore, translating from "copy" to "true" inside the merge
recursive machinery is fine and not an error because we are using as
much detection information as is relevant to the algorithm and which
the user is willing to pay for.

To throw one more wrinkle in here, merge.renames could actually be set
to "copy" and make sense, because we compute diffs multiple times.
Twice within the recursive merge machinery (for which we'd want to
translate "copy" to "true"), and once for the diffstat at the end
(which comes from builtin/merge.c, and for which it could make sense
to detect copies).

(Kind of curious whether Junio agrees with my rationale or thinks I'm
out in left field with it...)

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Elijah Newren]

On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart <peartben@gmail.com> wrote:
> On 4/26/2018 6:52 PM, Elijah Newren wrote:
>> On Thu, Apr 26, 2018 at 1:52 PM, Ben Peart <Ben.Peart@microsoft.com>
>> wrote:
>>
>>> diff --git a/merge-recursive.h b/merge-recursive.h
>>> index 80d69d1401..0c5f7eff98 100644
>>> --- a/merge-recursive.h
>>> +++ b/merge-recursive.h
>>> @@ -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;
>>> @@ -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?
>>
> 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().

Ah, you're following the precedent set by
diff_rename_limit/merge_rename_limit; that makes sense.  Thanks for
the explanation.  I believe another possibility here is that for both
the {merge,diff}_rename_limit pair of variables and the
{diff,merge}_renames pair of variables, since the code parses all
inputs before ever using the result, we could calculate the result
once and store it rather than storing the constituent pieces of the
calculation.  That would also prevent people from trying to use one of
the pieces of the calculation instead of treating it as a coherent
whole.  However, while I would have preferred that the rename_limit
pair of variables also went away in favor of just one field which is
updated as it parses each input option, what you have is fine for this
series.

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Johannes Schindelin]

Hi,

On Thu, 26 Apr 2018, Elijah Newren wrote:

> On Thu, Apr 26, 2018 at 7:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Ben Peart <peartben@gmail.com> writes:
> >
> >> 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... :)
> >
> > 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.
> 
> Yes, I agree.

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.

Ciao,
Dscho

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Elijah Newren]

Hi Dsco,

On Fri, Apr 27, 2018 at 12:23 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
<snip>
>
> 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.

I apologize for any frustration; I've probably caused a good deal of
it by repeatedly catching or noticing things after one review and then
bringing them up later.  I just didn't catch it all at first.

Your restatement of the basis of the argument doesn't match my
rationale, though.  My reasoning was that (1) the configuration
options exist to allow the user to specify how much of a performance
cost they're willing to pay, (2) the two options are separate because
some users might want to configure one more loosely or tightly than
the other, but (3) many users will want to just specify once how much
they are willing to pay without being forced to repeat themselves for
different parts of git (diff, merge, possible future commands).

I'll add to my former basis by stating that I think the diffstat at
the end of the merge should be controlled by these variables, even if
that's not implemented as part of Ben's series.  And both of those
configuration options clearly feed into whether that diffstat should
be doing rename or copy or no detection.  While they could be kept
separate options, forcing us to somehow decide which one overrides
which, I think it's much more natural if we already have merge.renames
inheriting from and overriding diff.renames.

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

From my view, resolve and octopus merges auto-demote diff.renames and
merge.renames to false.  In fact, they optimize the work involved in
that demotion by not even checking the value.  I think demoting "copy"
to "true" in the recursive merge machinery is no more surprising than
that is.  You can find more details to this argument in the portion of
my email that you responded to but snipped out.

But to add to that argument, if auto-demotion is a violation of the
Law of Least Surprise, as you claim, then naming this option
merge.renames is also a violation of the Law of Least Surprise.  You
would instead need to rename it to something like
merge.recursive.renames.  (And then when a new merge strategy comes
along that also does renames, we'll need to add a merge.ort.renames.)

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

It was only half thought through, yes, at least by me.  But the more I
think through it, the more I like the inheritance personally.  I see
no problem with the demotion and think the inheritance has the
advantage of being easier to understand, because I see your proposal
as causing questions like:
  - "Why does merge.renameLimit inherit from diff.renameLimit but
merge.renames doesn't from diff.renames?"
  - "Why can't I just specify in one place that I {am, am not} willing
to pay for {full, both copy and} rename detection wherever it makes
sense?"
  - "How do I control the detection for the diffstat at the end of the merge?"


Hope that helps,
Elijah

(no subject)

[Elijah Newren]

From: Elijah Newren <newren@gmail.com>

On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart <peartben@gmail.com> wrote:

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

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 59848e5634..662c2713ca 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -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
diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 1e0728aa12..aa66cbe41e 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -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:
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 6cc4404144..b618f134d2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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);
+	/*
+	 * We do not have logic to handle the detection of copies.  In
+	 * fact, it may not even make sense to add such logic: would we
+	 * really want a change to a base file to be propagated through
+	 * multiple other files by a merge?
+	 */
+	if (opts.detect_rename > DIFF_DETECT_RENAME)
+		opts.detect_rename = DIFF_DETECT_RENAME;
 	opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
 			    o->diff_rename_limit >= 0 ? o->diff_rename_limit :
 			    1000;
-- 
2.17.0.294.g8e3b83c0e3

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Eckhard Maaß]

On Fri, Apr 27, 2018 at 11:23:56AM +0900, Junio C Hamano wrote:
> 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.

Greetings,
Eckhard

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Elijah Newren]

On Fri, Apr 27, 2018 at 11:37 AM, Eckhard Maaß
<eckhard.s.maass@googlemail.com> wrote:
> On Fri, Apr 27, 2018 at 11:23:56AM +0900, Junio C Hamano wrote:
>> 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)).

  - Directory rename detection added more (rename/rename/rename(1to3),
    rename/rename(Nto1), add/add/add, directory/file/file, n-fold
    transitive rename, etc.), -- and forced us to specify various
    rules to limit the possibility space so that conflicts could be
    representable in the index and understandable by the user.

  - I suspect adding copies would add new types of conflicts we
    haven't thought of yet.

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...and it'd open up a big can of edge
and corner case problems as well.

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Eckhard Maaß]

On Fri, Apr 27, 2018 at 01:23:20PM -0700, Elijah Newren wrote:
> 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.


> [...] 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 one just assumes the most likely outcome is that something from z wad
factored out to y, it might just be sufficient to see whether the
modifications of the two branches apply cleanly - if A touched the parts
of B that have been factored out there would be a normal merge conflict
(where one could be nice and give a hint that some content was copied to
y on the B branch), if A did not touched the parts touched (or moved) by
B, then there is no problem. If A exactly deleted the content moved by
B, there will be no conflict - but this is seems to be strange anyway.

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.

To sum up:
- fail as before for conflicting merges, but give a hint that one has
  copied to quicken up resolution.


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

With the above suggestion, if there are conflicts, you fail and give a
hint.

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

A copy on branch to x and one another to y seems strange even if z
merges cleanly. Did both sides try to factor the same thing out to
different files? Or did they try to make something independent, but
managed to make it to different files? For this I would be inclined to
just suggest fail with a copy/copy(somewhere else). But this is a real
corner case after all. Has anyone seen just thing in practice?

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

We do have the modified/deleted conflict where we could hint that
content also has been copied and then not try to do more.

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

For renaming one side and coping the other, I would think doing the same
as above is sensible enough: if there are conflicts one can give an
additional hint of the one part having been copied, but not change the
kind of conflicts much.

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

Hm, it didn't sound like that. Would you think that users would be
surprised by my suggestions? Or are they all too corner casey to be
worth implementing anyway?

Greetings,
Eckhard

Re:

[Ben Peart]

On 4/27/2018 2:19 PM, Elijah Newren wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart <peartben@gmail.com> wrote:
> 
>> 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(-)
> 
> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
> index 59848e5634..662c2713ca 100644
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -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
> diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
> index 1e0728aa12..aa66cbe41e 100644
> --- a/Documentation/merge-strategies.txt
> +++ b/Documentation/merge-strategies.txt
> @@ -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:
>   
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 6cc4404144..b618f134d2 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -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);
> +	/*
> +	 * We do not have logic to handle the detection of copies.  In
> +	 * fact, it may not even make sense to add such logic: would we
> +	 * really want a change to a base file to be propagated through
> +	 * multiple other files by a merge?
> +	 */
> +	if (opts.detect_rename > DIFF_DETECT_RENAME)
> +		opts.detect_rename = DIFF_DETECT_RENAME;
>   	opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
>   			    o->diff_rename_limit >= 0 ? o->diff_rename_limit :
>   			    1000;
> 

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?

Re:

[Elijah Newren]

On Mon, Apr 30, 2018 at 6:11 AM, Ben Peart <peartben@gmail.com> wrote:
> On 4/27/2018 2:19 PM, Elijah Newren wrote:
>>
>> From: Elijah Newren <newren@gmail.com>
>>
>> On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart <peartben@gmail.com> wrote:
>>
>>> 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>
>
> 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
xmqqwox19ohw.fsf@gitster-ct.c.googlers.com 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.

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Elijah Newren]

Hi Eckhard,

On Mon, Apr 30, 2018 at 1:03 AM, Eckhard Maaß
<eckhard.s.maass@googlemail.com> wrote:
> On Fri, Apr 27, 2018 at 01:23:20PM -0700, Elijah Newren wrote:
>> 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.

Ooh, if you wanted to detect movement of code between files (as blame
does, I think searches for PICKAXE_BLAME_MOVE would point you in the
right direction) and then try to use that during merge to allow
applied changes to move with the code, that would be awesome.
Expensive, and might be a lot of work to wire it all up, but it'd be
very interesting.  I was only discussing DIFF_DETECT_COPY in my
previous email, which was all about duplicating entire files; that's
something I don't see utility in right now for resolving merges.

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

Right, if you have to assume that the copy was made to make the code
independent, then there's no value for merge resolution to having
detected the copy in the first place.  That has the advantage of
side-stepping the possible new edge and corner cases I mentioned in
the rest of my email, but it means we shouldn't even spend time
detecting copies -- whether whole file (via DIFF_DETECT_COPY) or
individual lines (via PICKAXE_BLAME_COPY and variants).


Elijah

Re:

[Ben Peart]

On 4/30/2018 12:12 PM, Elijah Newren wrote:
> On Mon, Apr 30, 2018 at 6:11 AM, Ben Peart <peartben@gmail.com> wrote:
>> On 4/27/2018 2:19 PM, Elijah Newren wrote:
>>>
>>> From: Elijah Newren <newren@gmail.com>
>>>
>>> On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart <peartben@gmail.com> wrote:
>>>
>>>> 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>
>>
>> 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
> xmqqwox19ohw.fsf@gitster-ct.c.googlers.com 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.
> 

I think this patch series (including Elijah's fixup!) improves the 
situation from where we were and it provides the necessary functionality 
to solve the problem I started out to solve.  While there are other 
changes that could be made, I think they should be done in separate 
follow up patches.

I'm happy to reroll this incorporating the fixup! so that we can make 
progress.  Junio, would you prefer I reroll this based on 
en/rename-directory-detection-reboot or master?

[PATCH v4 0/3] add additional config settings for merge

[Ben Peart]

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

Base Ref: en/rename-directory-detection
Web-Diff: https://github.com/benpeart/git/commit/16b175c25f
Checkout: git fetch https://github.com/benpeart/git merge-renames-v4 && git checkout 16b175c25f

### Patches

Ben Peart (3):
  merge: update documentation for {merge,diff}.renameLimit
  merge: Add merge.renames config setting
  merge: pass aggressive when rename detection is turned off

 Documentation/diff-config.txt             |  3 ++-
 Documentation/merge-config.txt            |  8 +++++-
 Documentation/merge-strategies.txt        | 11 +++++---
 diff.c                                    |  2 +-
 diff.h                                    |  1 +
 merge-recursive.c                         | 31 ++++++++++++++++++-----
 merge-recursive.h                         |  8 +++++-
 t/t3034-merge-recursive-rename-options.sh | 18 +++++++++++++
 8 files changed, 68 insertions(+), 14 deletions(-)


base-commit: c5b761fb2711542073cf1906c0e86a34616b79ae
-- 
2.17.0.windows.1

[PATCH v4 1/3] merge: update documentation for {merge,diff}.renameLimit

[Ben Peart]

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 <benpeart@microsoft.com>
---
 Documentation/diff-config.txt  | 3 ++-
 Documentation/merge-config.txt | 3 ++-
 2 files changed, 4 insertions(+), 2 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..48ee3bce77 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -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 v4 2/3] merge: Add merge.renames config setting

[Ben Peart]

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 <johannes.schindelin@gmx.de>
Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Documentation/merge-config.txt            |  5 ++++
 Documentation/merge-strategies.txt        | 11 ++++++---
 diff.c                                    |  2 +-
 diff.h                                    |  1 +
 merge-recursive.c                         | 30 ++++++++++++++++++-----
 merge-recursive.h                         |  8 +++++-
 t/t3034-merge-recursive-rename-options.sh | 18 ++++++++++++++
 7 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 48ee3bce77..662c2713ca 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -38,6 +38,11 @@ merge.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.  Defaults to the value of diff.renames.
+
 merge.renormalize::
 	Tell Git that canonical representation of files in the
 	repository has changed over time (e.g. earlier commits record
diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index fd5d748d1b..30acc99232 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -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:
 
@@ -84,12 +85,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[=<n>];;
 	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=<n>;;
diff --git a/diff.c b/diff.c
index fb22b19f09..e744b35cdc 100644
--- a/diff.c
+++ b/diff.c
@@ -177,7 +177,7 @@ static int parse_submodule_params(struct diff_options *options, const char *valu
 	return 0;
 }
 
-static int git_config_rename(const char *var, const char *value)
+int git_config_rename(const char *var, const char *value)
 {
 	if (!value)
 		return DIFF_DETECT_RENAME;
diff --git a/diff.h b/diff.h
index 7cf276f077..966fc8fce6 100644
--- a/diff.h
+++ b/diff.h
@@ -321,6 +321,7 @@ extern int git_diff_ui_config(const char *var, const char *value, void *cb);
 extern void diff_setup(struct diff_options *);
 extern int diff_opt_parse(struct diff_options *, const char **, int, const char *);
 extern void diff_setup_done(struct diff_options *);
+extern int git_config_rename(const char *var, const char *value);
 
 #define DIFF_DETECT_RENAME	1
 #define DIFF_DETECT_COPY	2
diff --git a/merge-recursive.c b/merge-recursive.c
index 5f42c677d5..372ffbbacc 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1524,7 +1524,15 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *o,
 	diff_setup(&opts);
 	opts.flags.recursive = 1;
 	opts.flags.rename_empty = 0;
-	opts.detect_rename = DIFF_DETECT_RENAME;
+	opts.detect_rename = merge_detect_rename(o);
+	/*
+	 * We do not have logic to handle the detection of copies.  In
+	 * fact, it may not even make sense to add such logic: would we
+	 * really want a change to a base file to be propagated through
+	 * multiple other files by a merge?
+	 */
+	if (opts.detect_rename > DIFF_DETECT_RENAME)
+		opts.detect_rename = DIFF_DETECT_RENAME;
 	opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
 			    o->diff_rename_limit >= 0 ? o->diff_rename_limit :
 			    1000;
@@ -2564,7 +2572,7 @@ static int handle_renames(struct merge_options *o,
 	ri->head_renames = NULL;
 	ri->merge_renames = NULL;
 
-	if (!o->detect_rename)
+	if (!merge_detect_rename(o))
 		return 1;
 
 	head_pairs = get_diffpairs(o, common, head);
@@ -3241,9 +3249,18 @@ int merge_recursive_generic(struct merge_options *o,
 
 static void merge_recursive_config(struct merge_options *o)
 {
+	char *value = NULL;
 	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);
+	if (!git_config_get_string("diff.renames", &value)) {
+		o->diff_detect_rename = git_config_rename("diff.renames", value);
+		free(value);
+	}
+	if (!git_config_get_string("merge.renames", &value)) {
+		o->merge_detect_rename = git_config_rename("merge.renames", value);
+		free(value);
+	}
 	git_config(git_xmerge_config, NULL);
 }
 
@@ -3256,7 +3273,8 @@ void init_merge_options(struct merge_options *o)
 	o->diff_rename_limit = -1;
 	o->merge_rename_limit = -1;
 	o->renormalize = 0;
-	o->detect_rename = 1;
+	o->diff_detect_rename = -1;
+	o->merge_detect_rename = -1;
 	merge_recursive_config(o);
 	merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
 	if (merge_verbosity)
@@ -3307,16 +3325,16 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 	else if (!strcmp(s, "no-renormalize"))
 		o->renormalize = 0;
 	else if (!strcmp(s, "no-renames"))
-		o->detect_rename = 0;
+		o->merge_detect_rename = 0;
 	else if (!strcmp(s, "find-renames")) {
-		o->detect_rename = 1;
+		o->merge_detect_rename = 1;
 		o->rename_score = 0;
 	}
 	else if (skip_prefix(s, "find-renames=", &arg) ||
 		 skip_prefix(s, "rename-threshold=", &arg)) {
 		if ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0)
 			return -1;
-		o->detect_rename = 1;
+		o->merge_detect_rename = 1;
 	}
 	else
 		return -1;
diff --git a/merge-recursive.h b/merge-recursive.h
index d863cf8867..c1d9b5b3d9 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -18,7 +18,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;
@@ -55,6 +56,11 @@ struct collision_entry {
 	struct string_list source_files;
 	unsigned reported_already:1;
 };
+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;
+}
 
 /* merge_trees() but with recursive ancestor consolidation */
 int merge_recursive(struct merge_options *o,
diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
index b9c4028496..3d9fae68c4 100755
--- a/t/t3034-merge-recursive-rename-options.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -309,4 +309,22 @@ test_expect_success 'last wins in --find-renames=<m> --rename-threshold=<n>' '
 	check_threshold_0
 '
 
+test_expect_success 'merge.renames disables rename detection' '
+	git read-tree --reset -u HEAD &&
+	git -c merge.renames=false merge-recursive $tail &&
+	check_no_renames
+'
+
+test_expect_success 'merge.renames defaults to diff.renames' '
+	git read-tree --reset -u HEAD &&
+	git -c diff.renames=false merge-recursive $tail &&
+	check_no_renames
+'
+
+test_expect_success 'merge.renames overrides diff.renames' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git -c diff.renames=false -c merge.renames=true merge-recursive $tail &&
+	$check_50
+'
+
 test_done
-- 
2.17.0.windows.1

[PATCH v4 3/3] merge: pass aggressive when rename detection is turned off

[Ben Peart]

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 <johannes.schindelin@gmx.de>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 merge-recursive.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index 372ffbbacc..cea054cfd4 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -355,6 +355,7 @@ static int git_merge_trees(struct merge_options *o,
 	o->unpack_opts.fn = threeway_merge;
 	o->unpack_opts.src_index = &the_index;
 	o->unpack_opts.dst_index = &the_index;
+	o->unpack_opts.aggressive = !merge_detect_rename(o);
 	setup_unpack_trees_porcelain(&o->unpack_opts, "merge");
 
 	init_tree_desc_from_tree(t+0, common);
-- 
2.17.0.windows.1

Re: [PATCH v4 0/3] add additional config settings for merge

[Elijah Newren]

Hi Ben,

Thanks for your persistence.

On Wed, May 2, 2018 at 9:01 AM, Ben Peart <Ben.Peart@microsoft.com> wrote:
> This version incorporates Elijah's fixup patch and is now based on
> en/rename-directory-detection in next.

en/rename-directory-detection was in master but reverted.
en/rename-directory-detection-reboot is the new series, and isn't
quite in next yet; Junio just said in the last "What's cooking" that
he intends to merge it down.  However, the two series are obviously
similar, so your rebasing even onto the old one does cut down on the
conflicts considerably.  Rebasing this latest series onto
en/rename-directory-detection-reboot leaves just one minor conflict.

Anyway, I've looked over this latest re-roll and it looks good to me:
Reviewed-by: Elijah Newren <newren@gmail.com>

Re: [PATCH v4 2/3] merge: Add merge.renames config setting

[Junio C Hamano]

Ben Peart <Ben.Peart@microsoft.com> writes:

I'd downcase the verb on the subject.

> 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 <johannes.schindelin@gmx.de>
> Helped-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ...
> diff --git a/merge-recursive.h b/merge-recursive.h
> index d863cf8867..c1d9b5b3d9 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -55,6 +56,11 @@ struct collision_entry {
>  	struct string_list source_files;
>  	unsigned reported_already:1;
>  };
> +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;
> +}

I'll tweak the above to leave a blank before the function, and make
it "static inline", to ensure that the output from

    $ git grep -e '\<inline\>' --and --not -e 'static inline' -- \*.h

is empty.

Re:

[Luna Jernberg]

What is the question?

Den ons 9 aug. 2023 kl 03:31 skrev <5598162950@mms.cricketwireless.net>:

Re:

[Mr. JAMES]

Hello, 

I'm James, an Entrepreneur, Venture Capitalist & Private Lender. I represent a group of Ultra High Net Worth Donors worldwide. Kindly let me know if you can be trusted to distribute charitable items which include Cash, Food Items and Clothing in your region.

Thank you
James.

Re:

[Jaydeep Das]

Please ignore this patch. I think I made some mistake
when copy-pasting the In-reply-to code.

Sorry for the trouble. I have sent this same patch on
the appropriate thread.

Thanks,
Jaydeep.

Re:

[Jeff King]

On Sat, Aug 21, 2021 at 08:10:59PM +0530, TECOB270_Ganesh Pawar wrote:

> To reproduce:
> 1. Set the contents of .git/hooks/prepare-commit-msg to this:
> ```
> #!/bin/sh
> 
> COMMIT_MSG_FILE=$1
> 
> echo "Initial Commit." > "$COMMIT_MSG_FILE"
> echo "" >> "$COMMIT_MSG_FILE"
> echo "# Some random comment." >> "$COMMIT_MSG_FILE"
> ```
> Notice the comment being added to the file.
> 
> 2. Append a commit with the --no-edit flag.
> `git commit --amend --no-edit`
> 
> The comment ("Some random comment" in this case) is included in the
> final commit message, but it shouldn't right?
> 
> If I don't pass the flag and just save the commit without changing
> anything, the comment isn't included. Shouldn't this be the case with
> the --no-edit flag too?

No, the behavior you're seeing is expected. Try this:

  git commit --cleanup=strip --amend --no-edit

The default for "--cleanup" is "strip" when the editor is run, and
"whitespace" otherwise. I.e., if Git did not insert comments, then it
doesn't remove them either.

If you have a hook which is inserting comments which may need to be
stripped, you may want to set the commit.cleanup config to tell Git to
always remove them (but beware that invocations like "git commit -F"
will also start stripping comments).

See "--cleanup" in "git help commit" for the possible values.

-Peff

Re:

[Martin Ågren]

[Trying with another e-mail address for Martin Nicolay. Maybe the one
from the in-body From header works better. wsmn.osm-gmbh.de couldn't be
found.]

On Fri, 15 Nov 2019 at 17:29, Martin Ågren <martin.agren@gmail.com> wrote:
>
> Hi Martin
>
> On Fri, 15 Nov 2019 at 17:17, Martin Nicolay <martin@wsmn.osm-gmbh.de> wrote:
>
> > While working with complex scripts invoking git multiple times my
> > editor detects the changes and calls "git status". This leads to
> > aborts in "git-stash". With this patch and an appropriate value
> > core.fileslocktimeout this problem goes away.
>
> Are you able to patch your editor to call
>   git --no-optional-locks status
> instead? See the bottom of git-status(1) ("BACKGROUND REFRESH") for more
> on this.
>
> > +long get_files_lock_timeout_ms(void)
> > +{
> > +       static int configured = 0;
> > +
> > +       /* The default timeout is 100 ms: */
> > +       static int timeout_ms = 100;
> > +
> > +       if (!configured) {
> > +               git_config_get_int("core.fileslocktimeout", &timeout_ms);
> > +               configured = 1;
> > +       }
> > +
> > +       return timeout_ms;
> > +}
> > +
>
> > @@ -172,7 +174,7 @@ static inline int hold_lock_file_for_update(
> >                 struct lock_file *lk, const char *path,
> >                 int flags)
> >  {
> > -       return hold_lock_file_for_update_timeout(lk, path, flags, 0);
> > +       return hold_lock_file_for_update_timeout(lk, path, flags, get_files_lock_timeout_ms() );
> >  }
>
> This looks like it changes the default from 0 ("try exactly once") to
> 100ms. Maybe we should stick with 0 for those who don't jump onto this
> new config knob?
>
> Martin

Re:

[Martin Ågren]

Hi Martin

On Fri, 15 Nov 2019 at 17:17, Martin Nicolay <martin@wsmn.osm-gmbh.de> wrote:

> While working with complex scripts invoking git multiple times my
> editor detects the changes and calls "git status". This leads to
> aborts in "git-stash". With this patch and an appropriate value
> core.fileslocktimeout this problem goes away.

Are you able to patch your editor to call
  git --no-optional-locks status
instead? See the bottom of git-status(1) ("BACKGROUND REFRESH") for more
on this.

> +long get_files_lock_timeout_ms(void)
> +{
> +       static int configured = 0;
> +
> +       /* The default timeout is 100 ms: */
> +       static int timeout_ms = 100;
> +
> +       if (!configured) {
> +               git_config_get_int("core.fileslocktimeout", &timeout_ms);
> +               configured = 1;
> +       }
> +
> +       return timeout_ms;
> +}
> +

> @@ -172,7 +174,7 @@ static inline int hold_lock_file_for_update(
>                 struct lock_file *lk, const char *path,
>                 int flags)
>  {
> -       return hold_lock_file_for_update_timeout(lk, path, flags, 0);
> +       return hold_lock_file_for_update_timeout(lk, path, flags, get_files_lock_timeout_ms() );
>  }

This looks like it changes the default from 0 ("try exactly once") to
100ms. Maybe we should stick with 0 for those who don't jump onto this
new config knob?

Martin

RE:

[Yagnatinsky, Mark]

No, not like that.  See here:
https://git.wiki.kernel.org/index.php/GitCommunity
The email address you send the "subscribe" message to is NOT the mailing list itself.
What you just did is send the words "subscribe git" to everyone already on the mailing list :)

-----Original Message-----
From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of William Baker
Sent: Tuesday, August 20, 2019 1:23 PM
To: git@vger.kernel.org
Subject: 

subscribe git

----------------------------------------------------------------------
This message, and any attachments, is for the intended recipient(s) only, may contain information that is privileged, confidential and/or proprietary and subject to important terms and conditions available at http://www.bankofamerica.com/emaildisclaimer.   If you are not the intended recipient, please delete this message.

Re:

[Rohit Ashiwal]

Hey Eric

On Tue, 5 Mar 2019 09:57:40 -0500 Eric Sunshine <sunshine@sunshineco.com> wrote:
> This patch, due to its length and repetitive nature, falls under the
> category of being tedious to review, which makes it all the more
> likely that a reviewer will overlook a problem.

Yes, I clearly understand that this patch has become too big to review.
It will require time to carefully review and reviewers are doing their
best to maintain the utmost quality of code.

> And, it's not always obvious at a glance that a change is correct. For
> instance, taking a look at the final patch band:
>
>     - ! test -d submod &&
>     - ! test -d submod/subsubmod/.git &&
>     + test_path_is_missing submod &&
>     + test_path_is_missing submod/subsubmod/.git &&

Duy actually confirms that this transformation is correct in this[1] email.
(I know that, it was given as an example, but I'll leave the link anyway).

Thanks
Rohit

[1]: https://public-inbox.org/git/CACsJy8BYeLvB7BSM_Jt4vwfGsEBuhaCZfzGPOHe=B=7cvnRwrg@mail.gmail.com/

Re:

[Johannes Schindelin]

Hi,

don't send this to git@vger.kernel.org. Send it to
majordomo@vger.kernel.org instead.

Thanks,
Johannes


On Wed, 23 Jan 2019, Cody Kratzer wrote:

> I've sent this same email 3 times. I don't think it works. I'm
> researching this morning how to unsubscribe from this git group.
> 
> CODY KRATZER WEB DEVELOPMENT MANAGER
> 866-344-3875 x145
> CODY@LIGHTINGNEWYORK.COM
> M - F 9 - 5:30
> 
> 
> On Wed, Jan 23, 2019 at 5:51 AM Christopher Hagler
> <haglerchristopher@gmail.com> wrote:
> >
> > Unsubscribe git
> >
> > Sent from my iPhone
> 

RE:

[Randall S. Becker]

On January 23, 2019 11:00, Christopher Hagler wrote:
> Send the email to this address
> Majordomo@vger.kernel.org and it will work
> > On Jan 23, 2019, at 8:16 AM, Cody Kratzer <cody@lightingnewyork.com>
> > I've sent this same email 3 times. I don't think it works. I'm
> > researching this morning how to unsubscribe from this git group.
> >
> > CODY KRATZER WEB DEVELOPMENT MANAGER
> > 866-344-3875 x145
> > CODY@LIGHTINGNEWYORK.COM
> > M - F 9 - 5:30
> > On Wed, Jan 23, 2019 at 5:51 AM Christopher Hagler
> > <haglerchristopher@gmail.com> wrote:
> >>
> >> Unsubscribe git

Reference information to the mailing lists are available here:
http://vger.kernel.org/vger-lists.html#git

Re:

[Christopher Hagler]

Send the email to this address 
Majordomo@vger.kernel.org and it will work

Sent from my iPhone

> On Jan 23, 2019, at 8:16 AM, Cody Kratzer <cody@lightingnewyork.com> wrote:
> 
> I've sent this same email 3 times. I don't think it works. I'm
> researching this morning how to unsubscribe from this git group.
> 
> CODY KRATZER WEB DEVELOPMENT MANAGER
> 866-344-3875 x145
> CODY@LIGHTINGNEWYORK.COM
> M - F 9 - 5:30
> 
> 
> On Wed, Jan 23, 2019 at 5:51 AM Christopher Hagler
> <haglerchristopher@gmail.com> wrote:
>> 
>> Unsubscribe git
>> 
>> Sent from my iPhone

Re:

[Thomas Braun]

Am 23.01.2019 um 15:16 schrieb Cody Kratzer:
> I've sent this same email 3 times. I don't think it works. I'm
> researching this morning how to unsubscribe from this git group.

Hi Cody,

https://git-scm.com/community says to subscribe you should send an email

with body content

subscribe git

to

majordomo@vger.kernel.org

so maybe

sending

unsubscribe git

to *that* address does unsubscribe you.

Re:

[Cody Kratzer]

I've sent this same email 3 times. I don't think it works. I'm
researching this morning how to unsubscribe from this git group.

CODY KRATZER WEB DEVELOPMENT MANAGER
866-344-3875 x145
CODY@LIGHTINGNEWYORK.COM
M - F 9 - 5:30


On Wed, Jan 23, 2019 at 5:51 AM Christopher Hagler
<haglerchristopher@gmail.com> wrote:
>
> Unsubscribe git
>
> Sent from my iPhone

RE:

[Inderpreet Saini]

unsubscribe git

Re:

[René Scharfe]

Am 27.02.2018 um 02:18 schrieb Alan Gage:
> Hello, I recently noticed a bug involving GitBash and Python. I was
> running a function that would post the system time once every second
> using a while loop but the text was only sent after the while loop
> ended due to a timer I had set. Essesntially, instead of it being
> entered every second into the terminal, it was entered all at once,
> when the loop ended. I tried this with the Command Line, among other
> things, and it worked as intended, with the text being entered every
> second. This is on Windows 10 Pro with the Fall Creators Update and
> the most recent version of GitBash.

Python buffers its output by default.  On terminals it enables line
buffering, i.e. the accumulated output is flushed when a newline
character is reached.  Otherwise it uses a system-dependent buffer
size in the range of a few kilobytes.  You can check if your output
is a terminal e.g. with:

  python -c "import sys; print(sys.stdout.isatty())"

You can disable buffering by running your script with "python -u".
This discussion mentions more options:

  https://stackoverflow.com/questions/107705/disable-output-buffering

You can also start bash on the command line.  I do wonder why Git CMD
seems to be started in what passes as a terminal, while Git BASH is
not, though.

You may want to check out https://gitforwindows.org/ and report your
findings using their issue tracker.

(This mailing list here, git@vger.kernel.org, is mostly used for
discussing Git itself, not so much about extra tools like bash or
Python that are packaged with Git for Windows.)

René

Re:

[Stefan Beller]

did you mean majordomo@kernel.org instead?

On Mon, Nov 20, 2017 at 7:10 AM, Viet Nguyen <ntviet18@gmail.com> wrote:
> unsubscribe git

Re:

[Stefan Beller]

To subscribe to the git mailing list, send the email to
majordomo@vger.kernel.org, not the mailing list itself.

On Sat, Nov 11, 2017 at 6:21 PM,  <hsed@unimetic.com> wrote:
> subscribe git

Re:

[Eric Wong]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Tue, Jan 24, 2017 at 4:21 PM, Stefan Beller <sbeller@google.com> wrote:
> >
> > +Do not PGP sign your patch. Most likely, your maintainer or other
> > +people on the list would not have your PGP key and would not bother
> > +obtaining it anyway.
> 
> I think even that could be further simplified - by just removing all
> comments about pgp email
> 
> Because it's not that the PGP keys would be hard to get, it's that
> PGP-signed email is an abject failure, and nobody sane does it.
> 
> Google for "phil zimmerman doesn't use pgp email".
> 
> It's dead. So I'm not sure it's worth mentioning at all.

I disagree, we still see it, and Debian still advocates it.
In fact, we may also want to mention S/MIME in the same breath:

  https://public-inbox.org/git/20170110004031.57985-2-hansenr@google.com/

Richard's more recent mails seem to indicate he's reformed :)

Re:

[Linus Torvalds]

On Tue, Jan 24, 2017 at 4:21 PM, Stefan Beller <sbeller@google.com> wrote:
>
> +Do not PGP sign your patch. Most likely, your maintainer or other
> +people on the list would not have your PGP key and would not bother
> +obtaining it anyway.

I think even that could be further simplified - by just removing all
comments about pgp email

Because it's not that the PGP keys would be hard to get, it's that
PGP-signed email is an abject failure, and nobody sane does it.

Google for "phil zimmerman doesn't use pgp email".

It's dead. So I'm not sure it's worth mentioning at all.

You might as well talk about how you shouldn't use EBCDIC encoding for
your patches, or about why git assumes that an email address has an
'@' sign in it, instead of being an UUCP bang path address.

              Linus

Re:

[Stefan Beller]

On Tue, Jan 24, 2017 at 4:43 PM, Cornelius Weig
<cornelius.weig@tngtech.com> wrote:

> -(5) Sign your work
> +(5) Certify your work by signing off.

That sounds better than what I proposed.

Thanks,
Stefan

Re:

[Cornelius Weig]

On 01/25/2017 01:21 AM, Stefan Beller wrote:
>>
>>> Do not PGP sign your patch, at least *for now*. (...)
>>
> 
> And maybe these 2 small words are the bug in the documentation?
> Shall we drop the "at least for now" part, like so:
> 
> ---8<---
> From 2c4fe0e67451892186ff6257b20c53e088c9ec67 Mon Sep 17 00:00:00 2001
> From: Stefan Beller <sbeller@google.com>
> Date: Tue, 24 Jan 2017 16:19:13 -0800
> Subject: [PATCH] SubmittingPatches: drop temporal reference for PGP signing
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/SubmittingPatches | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 08352deaae..28da4ad2d4 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -216,12 +216,12 @@ that it will be postponed.
>  Exception:  If your mailer is mangling patches then someone may ask
>  you to re-send them using MIME, that is OK.
>  
> -Do not PGP sign your patch, at least for now.  Most likely, your
> -maintainer or other people on the list would not have your PGP
> -key and would not bother obtaining it anyway.  Your patch is not
> -judged by who you are; a good patch from an unknown origin has a
> -far better chance of being accepted than a patch from a known,
> -respected origin that is done poorly or does incorrect things.
> +Do not PGP sign your patch. Most likely, your maintainer or other
> +people on the list would not have your PGP key and would not bother
> +obtaining it anyway. Your patch is not judged by who you are; a good
> +patch from an unknown origin has a far better chance of being accepted
> +than a patch from a known, respected origin that is done poorly or
> +does incorrect things.
>  
>  If you really really really really want to do a PGP signed
>  patch, format it as "multipart/signed", not a text/plain message
> 

It definitely is an improvement. Though it would still leave me puzzled
when finding a section about signing just below.

Is changing heading (5) too big a change? Like so:

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 08352de..71898dc 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -246,7 +246,7 @@ patch.
      *2* The mailing list: git@vger.kernel.org
 
 
-(5) Sign your work
+(5) Certify your work by signing off.
 
 To improve tracking of who did what, we've borrowed the
 "sign-off" procedure from the Linux kernel project on patches

Re:

[Stefan Beller]

On Mon, Apr 11, 2016 at 12:04 PM,  <miwilliams@google.com> wrote:
> From 7201fe08ede76e502211a781250c9a0b702a78b2 Mon Sep 17 00:00:00 2001
> From: Mike Williams <miwilliams@google.com>
> Date: Mon, 11 Apr 2016 14:18:39 -0400
> Subject: [PATCH 1/1] wt-status: Remove '!!' from
> wt_status_collect_changed_cb
>
> The wt_status_collect_changed_cb function uses an extraneous double negation
> (!!)

How is an !! errornous?

It serves the purpose to map an integer value(-1,0,1,2,3,4)
to a boolean (0,1, or a real bit in a bit field).

> when determining whether or not a submodule has new commits.
>
> Signed-off-by: Mike Williams <miwilliams@google.com>
> ---
>  wt-status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index ef74864..b955179 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -431,7 +431,7 @@ static void wt_status_collect_changed_cb(struct
> diff_queue_struct *q,
>                         d->worktree_status = p->status;
>                 d->dirty_submodule = p->two->dirty_submodule;
>                 if (S_ISGITLINK(p->two->mode))
> -                       d->new_submodule_commits = !!hashcmp(p->one->sha1,
> p->two->sha1);
> +                       d->new_submodule_commits = hashcmp(p->one->sha1,
> p->two->sha1);
>         }
>  }
>
> --
> 2.8.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re:

[christain147]

Good day,hoping you read this email and respond to me in good time.I do not intend to solicit for funds but  your time and energy in using my own resources to assist the less privileged.I am medically confined at the moment hence I request your indulgence.
I will give you a comprehensive brief once I hear from you.

Please forward your response to my private email address:
gudworks104@yahoo.com

Thanks and reply.

Robert Grondahl

Re:

[christain147]

Good day,hoping you read this email and respond to me in good time.I do not intend to solicit for funds but  your time and energy in using my own resources to assist the less privileged.I am medically confined at the moment hence I request your indulgence.
I will give you a comprehensive brief once I hear from you.

Please forward your response to my private email address:
gudworks104@yahoo.com

Thanks and reply.

Robert Grondahl

Re:

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Aug 5, 2015 at 7:47 PM, Ivan Chernyavsky <camposer@yandex.ru> wrote:
>> Dear community,
>>
>> For some time I'm wondering why there's no "--grep" option to the
>> "git branch" command, which would request to print only branches
>> having specified string/regexp in their history.
>
> Probably because nobody is interested and steps up to do it. The lack
> of response to you mail is a sign. Maybe you can try make a patch? I
> imagine it would not be so different from current --contains code, but
> this time we need to look into commits, not just commit id.

That is a dangeous thought.  I'd understand if it were internally
two step process, i.e. (1) the first pass finds commits that hits
the --grep criteria and then (2) the second pass does "--contains"
for all the hits found in the first pass using existing code, but
still, this operation is bound to dig all the way through the root
of the history when asked to find something that does not exist.

>> So for example:
>>
>>     $ git branch -r --grep=BUG12345
>>
>> should be roughly equivalent to following expression I'm using now for the same task:
>>
>>     $ for r in `git rev-list --grep=BUG12345 --remotes=origin`; do git branch -r --list --contains=$r 'origin/*'; done | sort -u

You should at least feed all --contains to a single invocation of
"git branch".  They are designed to be OR'ed together.

Re:

[Duy Nguyen]

On Wed, Aug 5, 2015 at 7:47 PM, Ivan Chernyavsky <camposer@yandex.ru> wrote:
> Dear community,
>
> For some time I'm wondering why there's no "--grep" option to the "git branch" command, which would request to print only branches having specified string/regexp in their history.

Probably because nobody is interested and steps up to do it. The lack
of response to you mail is a sign. Maybe you can try make a patch? I
imagine it would not be so different from current --contains code, but
this time we need to look into commits, not just commit id.

> So for example:
>
>     $ git branch -r --grep=BUG12345
>
> should be roughly equivalent to following expression I'm using now for the same task:
>
>     $ for r in `git rev-list --grep=BUG12345 --remotes=origin`; do git branch -r --list --contains=$r 'origin/*'; done | sort -u
>
> Am I missing something, is there some smarter/simpler way to do this?
>
> Thanks a lot in advance!
>
> --
>   Ivan
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Duy

Re: Re:

[Konstantin Khomoutov]

On Wed, 08 Apr 2015 23:58:58 +0200
Thomas Braun <thomas.braun@virtuell-zuhause.de> wrote:

[...]
> > I am trying to run the latest git 1.9.5 installer on windows. When I
> > run strings on libneon-25.dll it shows this:
> > 
> > ./libneon-25.dll:            OpenSSL 1.0.1h 5 Jun 2014
> > 
> > But when I load this dll in dependency walker, it picks up
> > msys-openssl 1.0.1m and has no trace of openssl-1.0.1h. My questions
> > to you:
> > 
> > 1. Is libneon-25.dll statically linked with openssl-1.0.1h?
> > 2. If not, where is the reference to 1.0.1h coming from?
> 
> I would be suprised if we link openssl statically into libneon. I
> guess libneon just reports against which openssl version it was
> *built*.
> 
> > I am asked to rebuild git with libneon-25.dll linked against
> > openssl-1.0.1m. But I am having a feeling that this is not needed,
> > since libneon is already picking the latest openssl version. Can you
> > please confirm?
> 
> You can download the development enviroment for git for windows here
> [1]. After installation, checkout the msys branch and then you can try
> to recomplile libneon using /src/subversion/release.sh.
> 
> [1]:
> https://github.com/msysgit/msysgit/releases/download/Git-1.9.5-preview20150319/msysGit-netinstall-1.9.5-preview20150319.exe
[...]

JFTR, the discussion about the same issue has been brought up on
git-users as well [2].

(People should really somehow use the basics of netiquette and mention
in their posts where they cross-post things.)

2. https://groups.google.com/d/topic/git-users/WXyWE5_JfNc/discussion

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re:

[Thomas Braun]

Am 08.04.2015 um 22:44 schrieb Mamta Upadhyay:
> Hi git team,

(CC'ing msysgit as this is the git for windows list)

Hi Mamta,

> I tried to research everywhere on a issue I am facing and emailing you
> as the last resource. This is critical for me and I needed your help.
> 
> I am trying to run the latest git 1.9.5 installer on windows. When I
> run strings on libneon-25.dll it shows this:
> 
> ./libneon-25.dll:            OpenSSL 1.0.1h 5 Jun 2014
> 
> But when I load this dll in dependency walker, it picks up
> msys-openssl 1.0.1m and has no trace of openssl-1.0.1h. My questions
> to you:
> 
> 1. Is libneon-25.dll statically linked with openssl-1.0.1h?
> 2. If not, where is the reference to 1.0.1h coming from?

I would be suprised if we link openssl statically into libneon. I guess
libneon just reports against which openssl version it was *built*.

> I am asked to rebuild git with libneon-25.dll linked against
> openssl-1.0.1m. But I am having a feeling that this is not needed,
> since libneon is already picking the latest openssl version. Can you
> please confirm?

You can download the development enviroment for git for windows here
[1]. After installation, checkout the msys branch and then you can try
to recomplile libneon using /src/subversion/release.sh.

[1]:
https://github.com/msysgit/msysgit/releases/download/Git-1.9.5-preview20150319/msysGit-netinstall-1.9.5-preview20150319.exe

Hope that helps
Thomas

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re:

[Stefan Beller]

On Wed, Mar 18, 2015 at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> What does the Icon^M try to catch, exactly? Is it a file? Is it a directory?
> Is it "anything that begins with Icon^M"?

It seems to be a special hidden file on Macs for UI convenience.

> On Apr 25, 2005, at 6:21 AM, Peter N. Lundblad wrote:
>
> The Icon^M file in a directory gives that directory a custom icon in
> the Finder. They are a holdover from MacOS 9 but there are still a lot
> of them out there. The "new" OS X format for icons are .icns files but
> I'm not sure if you can do custom file directory icons with them (you
> probably can, I just haven't found the docs yet).
>

Re:

[Junio C Hamano]

On Wed, Mar 18, 2015 at 2:28 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Mar 18, 2015 at 05:17:16PM -0400, Jeff King wrote:
>
>> [1] The double-CR fix works because we strip a single CR from the end of
>>     the line (as a convenience for CRLF systems), and then the remaining
>>     CR is syntactically significant. But I am surprised that quoting
>>     like:
>>
>>       printf '"Icon\r"' >.gitignore
>>
>>     does not seem to work.
>
> Answering myself: we don't do quoting like this in .gitignore. We allow
> backslashing to escape particular characters, like trailing whitespace.
> So in theory:
>
>   Icon\\r
>
> (where "\r" is a literal CR) would work. But it doesn't, because the
> CRLF chomping happens separately, and CR is therefore a special case. I
> suspect you could not .gitignore a file with a literal LF in it at all
> (and I equally suspect that nobody cares in practice).

What does the Icon^M try to catch, exactly? Is it a file? Is it a directory?
Is it "anything that begins with Icon^M"?

I am wondering if we need an opposite of '/' prefix in the .gitignore file
to say "the pattern does not match a directory, only a file".

Re:

[Jeff King]

On Wed, Mar 18, 2015 at 05:17:16PM -0400, Jeff King wrote:

> [1] The double-CR fix works because we strip a single CR from the end of
>     the line (as a convenience for CRLF systems), and then the remaining
>     CR is syntactically significant. But I am surprised that quoting
>     like:
> 
>       printf '"Icon\r"' >.gitignore
> 
>     does not seem to work.

Answering myself: we don't do quoting like this in .gitignore. We allow
backslashing to escape particular characters, like trailing whitespace.
So in theory:

  Icon\\r

(where "\r" is a literal CR) would work. But it doesn't, because the
CRLF chomping happens separately, and CR is therefore a special case. I
suspect you could not .gitignore a file with a literal LF in it at all
(and I equally suspect that nobody cares in practice).

-Peff

Re:

[Jeff King]

On Wed, Mar 18, 2015 at 02:06:22PM -0700, Stefan Beller wrote:

> > Where did you get that file from? We need to find whoever is
> > responsible and notify them so that these users who are having
> > the issue will be helped.
> 
> Given that this is part of https://github.com/github/gitignore
> which is the official collection of .gitignore files from Github,
> the company, we could ask Jeff or Michael if it is urgent.
> The actual fix being merged 3 years ago makes me belief
> it is not urgent though.

It looks like the fix they have in that repo does the right thing[1],
but for reference, you are much more likely to get results by creating
an issue or PR on that repository, rather than asking me.

-Peff

[1] The double-CR fix works because we strip a single CR from the end of
    the line (as a convenience for CRLF systems), and then the remaining
    CR is syntactically significant. But I am surprised that quoting
    like:

      printf '"Icon\r"' >.gitignore

    does not seem to work.

Re:

[Stefan Beller]

On Wed, Mar 18, 2015 at 1:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> On Wed, Mar 18, 2015 at 1:33 PM, Alessandro Zanardi
> <pensierinmusica@gmail.com> wrote:
>> Here are other sources describing the issue:
>>
>> http://stackoverflow.com/questions/21109672/git-ignoring-icon-files-because-of-icon-rule
>>
>> http://blog.bitfluent.com/post/173740409/ignoring-icon-in-gitignore
>>
>> Sorry to bother the Git development team with such a minor issue, I just
>> wanted to know if it's already been fixed.
>
> I do not ship your ~/.gitignore_global file as part of my software,
> so the problem is not mine to fix in the first place ;-)

Maybe this can be understood as a critique on the .gitignore format specifier
for paths. (Maybe not, I dunno)

So the `gitignore` script/executable which would generate your .gitignore file
for you introduced a bug to also ignore files in "Icons/...." when all you
wanted to have is ignoring the file "Icon\r\r"
(Mind that \r is an escape character to explain the meaning,
gitignore cannot understand it. Sometimes it also shows up as ^M^M
depending on operating system/editor used.)

But as you can see, there have been several attempts at fixing it right and
https://github.com/github/gitignore/pull/334 eventually got the right fix.
(it was merged 2012, which has been a while now), maybe
you'd want to use a new version of this gitignore script to
regenerate your gitignore?

>
> Where did you get that file from? We need to find whoever is
> responsible and notify them so that these users who are having
> the issue will be helped.

Given that this is part of https://github.com/github/gitignore
which is the official collection of .gitignore files from Github,
the company, we could ask Jeff or Michael if it is urgent.
The actual fix being merged 3 years ago makes me belief
it is not urgent though.

Thanks,
Stefan

Re:

[Junio C Hamano]

On Wed, Mar 18, 2015 at 1:33 PM, Alessandro Zanardi
<pensierinmusica@gmail.com> wrote:
> Here are other sources describing the issue:
>
> http://stackoverflow.com/questions/21109672/git-ignoring-icon-files-because-of-icon-rule
>
> http://blog.bitfluent.com/post/173740409/ignoring-icon-in-gitignore
>
> Sorry to bother the Git development team with such a minor issue, I just
> wanted to know if it's already been fixed.

I do not ship your ~/.gitignore_global file as part of my software,
so the problem is not mine to fix in the first place ;-)

Where did you get that file from? We need to find whoever is
responsible and notify them so that these users who are having
the issue will be helped.

Thanks.

Re:

[Duy Nguyen]

On Fri, Mar 13, 2015 at 8:34 AM,  <cody.taylor@maternityneighborhood.com> wrote:
> From 3e4e22e93bf07355b40ba0abcb3a15c4941cfee7 Mon Sep 17 00:00:00 2001
> From: Cody A Taylor <codemister99@yahoo.com>
> Date: Thu, 12 Mar 2015 20:36:44 -0400
> Subject: [PATCH] git prompt: Use toplevel to find untracked files.
>
> The __git_ps1() prompt function would not show an untracked
> state when the current working directory was not a parent of
> the untracked file.
>
> Signed-off-by: Cody A Taylor <codemister99@yahoo.com>
> ---
>  contrib/completion/git-prompt.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 214e859f99e7..f0d8a2669236 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -487,7 +487,8 @@ __git_ps1 ()
>
>                 if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ] &&
>                    [ "$(git config --bool bash.showUntrackedFiles)" != "false" ] &&
> -                  git ls-files --others --exclude-standard --error-unmatch -- '*' >/dev/null 2>/dev/null
> +                  git ls-files --others --exclude-standard --error-unmatch -- \
> +                    "$(git rev-parse --show-toplevel)/*" >/dev/null 2>/dev/null

Or make it a bit simpler, just replace '*' with ':/*'.
-- 
Duy

Re:

[R. Klomp]

I couldn't find information about whether the -solo feature is
available in all Beyond Compare versions.
At the least I can say that it is available in version 3 for Windows,
since that is the version that we're using.

This issue does not occur when using the normal difftool (command: git
difftool), which is odd and indicates that something must be wrong in
either Git or Beyond Compare.

On Wed, Sep 10, 2014 at 2:00 AM, David Aguilar <davvid@gmail.com> wrote:
> On Mon, Sep 08, 2014 at 04:36:49PM +0200, R. Klomp wrote:
>> Ok great! That indeed fixed the issue.
>> Although I still don't understand why it didn't work without -solo..
>> since it didn't work when no instance of Beyond Compare was running as
>> well.
>>
>> There must be something not quite right in either Git or Beyond Compare.
>>
>> On Mon, Sep 8, 2014 at 3:37 PM, Jim Naslund <jnaslund@gmail.com> wrote:
>> >
>> > On Sep 8, 2014 7:39 AM, "R. Klomp" <r.klomp@students.uu.nl> wrote:
>> >>
>> >> It seems like there's a bug involving git difftool's -d flag and Beyond
>> >> Compare.
>> >>
>> >> When using the difftool Beyond Compare, git difftool <..> <..> -d
>> >> immidiatly shuts down once the diff tree has been created. Beyond
>> >> Compare successfully shows the files that differ.
>> >> However, since git difftool doesn't wait for Beyond Compare to shut
>> >> down, all temporary files are gone. Due to this it's impossible to
>> >> view changes made inside files using the -d flag.
>> >>
>> >> I haven't tested if this issue also happens with other difftools.
>> >>
>> >> I'm using the latest versions of both Beyond Compare 3 (3.3.12, Pro
>> >> Edition for Windows) and Git (1.9.4 for Windows).
>> >>
>> >>
>> >> Thanks in advance for your help!
>> >
>> > I see the same behavior. For me it had something to do with the diff opening
>> > in a new tab in an existing window. Adding -solo to difftool.cmd will make
>> > beyond compare use a new window which fixes the issue for me.
>
> Interesting. Would it be worth changing difftool to use -solo by default, or
> are there any downsides to doing so?
>
> Is -solo a new feature that only exists in new versions of beyond compare?
> I would be okay saying that the user should use a fairly new version.
>
> Can we rely on -solo being available on all platforms?
> If so, I'd be okay with changing the default if there are no other downsides.
>
> The --dir-diff feature is not the only one that needs this blocking behavior.
> Does this issue also happen in the normal difftool mode without -d?
> --
> David

Re:

[David Aguilar]

On Mon, Sep 08, 2014 at 04:36:49PM +0200, R. Klomp wrote:
> Ok great! That indeed fixed the issue.
> Although I still don't understand why it didn't work without -solo..
> since it didn't work when no instance of Beyond Compare was running as
> well.
> 
> There must be something not quite right in either Git or Beyond Compare.
> 
> On Mon, Sep 8, 2014 at 3:37 PM, Jim Naslund <jnaslund@gmail.com> wrote:
> >
> > On Sep 8, 2014 7:39 AM, "R. Klomp" <r.klomp@students.uu.nl> wrote:
> >>
> >> It seems like there's a bug involving git difftool's -d flag and Beyond
> >> Compare.
> >>
> >> When using the difftool Beyond Compare, git difftool <..> <..> -d
> >> immidiatly shuts down once the diff tree has been created. Beyond
> >> Compare successfully shows the files that differ.
> >> However, since git difftool doesn't wait for Beyond Compare to shut
> >> down, all temporary files are gone. Due to this it's impossible to
> >> view changes made inside files using the -d flag.
> >>
> >> I haven't tested if this issue also happens with other difftools.
> >>
> >> I'm using the latest versions of both Beyond Compare 3 (3.3.12, Pro
> >> Edition for Windows) and Git (1.9.4 for Windows).
> >>
> >>
> >> Thanks in advance for your help!
> >
> > I see the same behavior. For me it had something to do with the diff opening
> > in a new tab in an existing window. Adding -solo to difftool.cmd will make
> > beyond compare use a new window which fixes the issue for me.

Interesting. Would it be worth changing difftool to use -solo by default, or
are there any downsides to doing so?

Is -solo a new feature that only exists in new versions of beyond compare?
I would be okay saying that the user should use a fairly new version.

Can we rely on -solo being available on all platforms?
If so, I'd be okay with changing the default if there are no other downsides.

The --dir-diff feature is not the only one that needs this blocking behavior.
Does this issue also happen in the normal difftool mode without -d?
-- 
David

Re:

[R. Klomp]

Ok great! That indeed fixed the issue.
Although I still don't understand why it didn't work without -solo..
since it didn't work when no instance of Beyond Compare was running as
well.

There must be something not quite right in either Git or Beyond Compare.

On Mon, Sep 8, 2014 at 3:37 PM, Jim Naslund <jnaslund@gmail.com> wrote:
>
> On Sep 8, 2014 7:39 AM, "R. Klomp" <r.klomp@students.uu.nl> wrote:
>>
>> It seems like there's a bug involving git difftool's -d flag and Beyond
>> Compare.
>>
>> When using the difftool Beyond Compare, git difftool <..> <..> -d
>> immidiatly shuts down once the diff tree has been created. Beyond
>> Compare successfully shows the files that differ.
>> However, since git difftool doesn't wait for Beyond Compare to shut
>> down, all temporary files are gone. Due to this it's impossible to
>> view changes made inside files using the -d flag.
>>
>> I haven't tested if this issue also happens with other difftools.
>>
>> I'm using the latest versions of both Beyond Compare 3 (3.3.12, Pro
>> Edition for Windows) and Git (1.9.4 for Windows).
>>
>>
>> Thanks in advance for your help!
>
> I see the same behavior. For me it had something to do with the diff opening
> in a new tab in an existing window. Adding -solo to difftool.cmd will make
> beyond compare use a new window which fixes the issue for me.
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message tomajordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html

Re:

[Constantine Gorbunov]

Johannes Sixt <j.sixt <at> viscovery.net> writes:

> 
> Am 2/6/2014 12:54, schrieb konstunn <at> ngs.ru:
> > However I typed the checkout directory in file
> > ..git/info/sparse-checkout by using different formats with
> > and without the leading and the trailing slashes, with and
> > without asterisk after trailing slash, having tried all
> > the possible combinations, but, all the same,
> > nevertheless, the error occured.
> 
> Make sure that you do not use CRLF line terminators in the sparse-checkout
> file.
> 

This is it. Right you are. I've just tried to edit "manually" with notepad 
.git\info\sparse-checkout and found out that there really was a CRLF line 
terminator. After I removed it I managed to succeed in my sparse checkout.

Re:

[Erik Faye-Lund]

This message should probably go to the msysGit mailing list. Included in CC.

On Tue, Jun 12, 2012 at 11:12 PM, rohit sood <rohit.s@lycos.com> wrote:
>
> Hi,
>   When trying a remote install of the git client using winrm on a Windows 2003 box, I get the following error :
>
> 2012-06-12 14:59:05.476   Line 852: Creating symbolic link "E:\apps\prod\Git\libexec/git-core/git-whatchanged.exe" failed, will try a hard link.
> 2012-06-12 14:59:05.523   Line 852: Creating symbolic link "E:\apps\prod\Git\libexec/git-core/git-write-tree.exe" failed, will try a hard link.
> 2012-06-12 14:59:05.570   Line 852: Creating symbolic link "E:\apps\prod\Git\libexec/git-core/git.exe" failed, will try a hard link.
> 2012-06-12 14:59:05.679   Message box (OK):
>                          Unable to configure the line ending conversion: core.autocrlf true
>
> I use the Git-1.7.10-preview20120409.exe executable .
> I am attempting to script an unattended silent install of the executable with the following options using Opscode Chef :
>
> options "/DIR=\"#{node['GIT']['HOME']}\" /VERYSILENT /SUPPRESSMSGBOXES /LOG=\"#{ENV['TEMP']}\\GIT_INSTALL.LOG\""
>
> Please advise
>
> thanks,
> Rohit
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Re:

[Anna]

Piedalies pirmaja Latvijas BEZMAKSAS pokera TV show, vinne celojumu uz Las Vegasu kur galvena balva ir $8.000.000!
http://latvijastvshovs1.co.nr

Re:

[Don Slutz]

Nope.  It was a mistake.  The local try worked, but the send to the 
maillist did not.  Sorry about this.
   -Don

-------- Original Message --------
Subject: Re:
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Don Slutz <slutz@krl.com>
CC: git@vger.kernel.org
Date: 5/11/2009 4:48 PM
> Hi,
>
> is this the new fashion, to send mails without a subject, all of a sudden 
> being okay only because Linus responded to one?
>
> Ciao,
> Dscho
>
>
>   



__________________________________________________________________________________________________________________
DISCLAIMER:"The information contained in this message and the attachments (if any) may be privileged and confidential and protected from disclosure. You are hereby notified that any unauthorized use, dissemination, distribution or copying of this communication, review, retransmission, or taking of any action based upon this information, by persons or entities other than the intended recipient, is strictly prohibited. If you are not the intended recipient or an employee or agent responsible for delivering this message, and have received this communication in error, please notify us immediately by replying to the message and kindly delete the original message, attachments, if any, and all its copies from your computer system. Thank you for your cooperation." 
________________________________________________________________________________________________________________

Re:

[Johannes Schindelin]

Hi,

is this the new fashion, to send mails without a subject, all of a sudden 
being okay only because Linus responded to one?

Ciao,
Dscho

Re:

[Linus Torvalds]

On Fri, 8 May 2009, Brandon Casey wrote:
> 
> btw, I've since done some more testing on some centos5.3 boxes we have.
> I get similar results (less ancient kernel 2.6.18).

Yes, 2.6.18 is still much too old to matter from a locking standpoint. 

When people initially worried about scalability, the issues were more 
about server side stuff and the cached cases. NFS (as a client) is 
certainly used on the server side too, but it tends to be a somewhat 
secondary worry where only specific parts really matter. So people worked 
a lot more on the core kernel, and on local high-performance filesystem 
scaling.

Only lately have we been pretty aggressive about finally really getting 
rid of the old "single big lock" (BKL) model entirely, or moving outwards 
from the core.

And while we removed the BKL from the normal NFS read/write paths long 
long ago, all the name lookup and directory handling code still had it 
until a year ago.

That, btw, is directly explained by perceived scalability issues: NFS is 
fairly often used as the backing store for a database and scaling thus 
matters there. But databases tend to keep their few big files open and use 
pread/pwrite - so pathname lookup is not nearly as significant for server 
ops as plain read/write.

(Pathname lookup is important for things like web servers etc, but they 
rely heavily on caching for that, and the cached case scales fine).

> I've also scanned through the errata announcements that RedHat has 
> released for their kernel updates.  A few of them involve NFS.  
> Possibly, whatever RedHat modified in the 5.X kernel was also backported 
> to the 4.X kernel.

That is very possibly the case. Expanding the BKL usage in some case could 
easily trigger the lock getting contention - and the way lock contention 
works, once you get a just even a small _hint_ of contention, things often 
fall off a cliff. The contention slows locking down, which in turn causes 
more CPU usage, which in turn causes _more_ contention.

So even a small amount of extra locking - or even just slowing down some 
code that was inside the lock - can have catastrophic behavioural changes 
when the lock is close to being a problem. You do not get a nice gradual 
slowdown at all - you just hit a hard wall.

I guess I should really try to set up some fileserver here at home to 
improve my test coverage. And to do better backups (or the little private 
data I have that I can't just mirror out to the world by turning it into 
an open-source project ;^)

				Linus

Re:

[Brandon Casey]

Linus Torvalds wrote:
> 
> On Fri, 8 May 2009, Brandon Casey wrote:
>> Before (cold cache):
>> % time     seconds  usecs/call     calls    errors syscall
>> ------ ----------- ----------- --------- --------- ----------------
>>  98.60    6.365501         111     57432           lstat64
>>
>> After (cold cache, no lstat fix, just cache_preload):
>> % time     seconds  usecs/call     calls    errors syscall
>> ------ ----------- ----------- --------- --------- ----------------
>>  90.90   23.717981         413     57432           lstat64
> 
> Yes, interesting. I really smells like it's all fixed performance and 
> there is a single lock around it. That 111us -> 413us increase is very 
> consistent with four cores all serializing on the same lock. So it 
> parallelizes to all four cores, but then will take exactly as long in 
> total.

Makes sense to me.

> Quite frankly, 2.6.9 is so old that I have absolutely _no_ memory of what 
> we used to do back then. Not that I follow NFS all that much even now - I 
> did some of the original page cache and dentry work on the Linux NFS 
> client way back when, but that was when I actually used NFS (and we were 
> converting everything to the page cache).
> 
> I've long since forgotten everything I knew, and I'm just as happy about 
> that. But clearly something is bad, and equally clearly it worked much 
> better for you a couple of months ago. Which does imply that there's 
> probably some centos issues.

In case you're not aware CentOS is just repacked RHEL.  I'm not sure if
centos has the resources for investigating problems.  We also have RHEL
licenses, so hopefully I'll be able to come up with something to submit
to them.

> Can you ask your MIS people if it would be possible to at least _test_ a 
> new kernel? In 2.6.9, I'm quite frankly inclined to just say "it will 
> likely never get fixed unless centos knows what it is", but if you test a 
> more modern kernel and see similar issues, then I'll be intrigued.

I think it's possible.  Just not on this specific machine.  Not sure what
we have lying around multi-processor wise.  Also, it won't happen until
next week since it's late Friday afternoon here.

btw, I've since done some more testing on some centos5.3 boxes we have.
I get similar results (less ancient kernel 2.6.18).  I've also scanned
through the errata announcements that RedHat has released for their
kernel updates.  A few of them involve NFS.  Possibly, whatever RedHat
modified in the 5.X kernel was also backported to the 4.X kernel.

> It's kind of sad, but at the same time, NFS was using the BKL up into 
> 2.6.26 or something like that (about a year ago). And your kernel is 
> based on something _much_ older.
> 
> That said, even with the BKL, NFS should allow all the actual IO to be 
> done in parallel (since the BKL is dropped on scheduling). But it's really 
> wasting a _lot_ of CPU time, and that hurts you enormously, even though 
> the cold-cache case still seems to win, judging by your other email:
>
>> Best without patch: 6.02 (systime 1.57)
>>
>>   0.43user 1.57system 0:06.02elapsed 33%CPU (0avgtext+0avgdata 0maxresident)k
>>   5336inputs+0outputs (12major+15472minor)pagefaults 0swaps
>>
>> Best with patch (preload_cache,lstat reduction): 2.69 (systime 10.47)
>>
>>   0.45user 10.47system 0:02.69elapsed 405%CPU (0avgtext+0avgdata 0maxresident)k
>>   5336inputs+0outputs (12major+13985minor)pagefaults 0swaps
> 
> so there's a _huge_ increase in system time (again), but the change from 
> 33% CPU -> 405% CPU makes up for it and you get lower elapsed times.
> 
> But that 7x increase in system time really is sad. I do suspect it's 
> likely due to spinning on the BKL. And if so, then a modern kernel should 
> fix it.

Thanks, I'll try to test next week.

-brandon

Re:

[Linus Torvalds]

On Fri, 8 May 2009, Brandon Casey wrote:
> 
> Before (cold cache):
> % time     seconds  usecs/call     calls    errors syscall
> ------ ----------- ----------- --------- --------- ----------------
>  98.60    6.365501         111     57432           lstat64
> 
> After (cold cache, no lstat fix, just cache_preload):
> % time     seconds  usecs/call     calls    errors syscall
> ------ ----------- ----------- --------- --------- ----------------
>  90.90   23.717981         413     57432           lstat64

Yes, interesting. I really smells like it's all fixed performance and 
there is a single lock around it. That 111us -> 413us increase is very 
consistent with four cores all serializing on the same lock. So it 
parallelizes to all four cores, but then will take exactly as long in 
total.

Quite frankly, 2.6.9 is so old that I have absolutely _no_ memory of what 
we used to do back then. Not that I follow NFS all that much even now - I 
did some of the original page cache and dentry work on the Linux NFS 
client way back when, but that was when I actually used NFS (and we were 
converting everything to the page cache).

I've long since forgotten everything I knew, and I'm just as happy about 
that. But clearly something is bad, and equally clearly it worked much 
better for you a couple of months ago. Which does imply that there's 
probably some centos issues.

Can you ask your MIS people if it would be possible to at least _test_ a 
new kernel? In 2.6.9, I'm quite frankly inclined to just say "it will 
likely never get fixed unless centos knows what it is", but if you test a 
more modern kernel and see similar issues, then I'll be intrigued.

It's kind of sad, but at the same time, NFS was using the BKL up into 
2.6.26 or something like that (about a year ago). And your kernel is 
based on something _much_ older.

That said, even with the BKL, NFS should allow all the actual IO to be 
done in parallel (since the BKL is dropped on scheduling). But it's really 
wasting a _lot_ of CPU time, and that hurts you enormously, even though 
the cold-cache case still seems to win, judging by your other email:

> Best without patch: 6.02 (systime 1.57)
> 
>   0.43user 1.57system 0:06.02elapsed 33%CPU (0avgtext+0avgdata 0maxresident)k
>   5336inputs+0outputs (12major+15472minor)pagefaults 0swaps
> 
> Best with patch (preload_cache,lstat reduction): 2.69 (systime 10.47)
> 
>   0.45user 10.47system 0:02.69elapsed 405%CPU (0avgtext+0avgdata 0maxresident)k
>   5336inputs+0outputs (12major+13985minor)pagefaults 0swaps

so there's a _huge_ increase in system time (again), but the change from 
33% CPU -> 405% CPU makes up for it and you get lower elapsed times.

But that 7x increase in system time really is sad. I do suspect it's 
likely due to spinning on the BKL. And if so, then a modern kernel should 
fix it.

			Linus

Re:

[Linus Torvalds]

On Fri, 8 May 2009, Brandon Casey wrote:
> 
> Something is definitely up.
> 
> I provided timing results for your original preload_cache implementation
> which affected status and diff, which was part of the justification for
> merging it in.
> 
>    http://article.gmane.org/gmane.comp.version-control.git/100998
> 
> You can see that cold cache system time for 'git status' went from 0.36 to
> 0.52 seconds.  Fine.  I just ran it again, and now I'm getting system time
> of 10 seconds!  This is the same machine.

Grr.

> OS: Centos4.7
> 
> $ cat /proc/version
> Linux version 2.6.9-78.0.17.ELsmp (mockbuild@builder16.centos.org) (gcc version 3.4.6 20060404 (Red Hat 3.4.6-9)) #1 SMP Thu Mar 12 20:05:15 EDT 2009

Ok, if that's really the true kernel version (2.6.9), then that's some 
ancient kernel there. At the same time it's obviously been recompiled 
recently, so it got updated. At a guess, something got screwed up. But I 
have absolutely _no_ way to even guess what kernel patches centos puts in 
their ancient kernel builds.

Perhaps a centos bugzilla entry might be appropriate? Somebody there might 
know what changed.

Of course, it _could_ be an external change too, where the NFS server or 
timing changed just enough to trigger a pre-existing issue. But that would 
be pretty unlikely.

			Linus

Re:

[Brandon Casey]

Brandon Casey wrote:
> Linus Torvalds wrote:
>> And 
>> the preloading sounds like it hits serialization overhead in the kernel, 
>> which I'm not at all surprised at, but not being surprised doesn't mean 
>> that I'm not interested to hear where it is.
>>
>> The Linux VFS dcache itself should scale better than that (but who knows - 
>> cacheline ping-pong due to lock contention can easily cause a 10x slowdown 
>> even without being _totally_ contended all the time). So I would _suspect_ 
>> that it's some NFS lock that you're seeing, but I'd love to know more.
>>
>> Btw, those system times are pretty high to begin with, so I'd love to know 
>> kernel version and see a profile even without the parallel case and 
>> presumably lock contention.

Here's an strace of 'git checkout':

Before (cold cache):
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 98.60    6.365501         111     57432           lstat64
  0.50    0.031984         359        89         2 close
  0.25    0.015818         115       137        77 open
  0.12    0.007670          23       339           write
  0.09    0.005631         110        51           munmap
  0.08    0.004873          49        99        69 stat64
  0.07    0.004771         140        34        15 access
  0.05    0.003083         280        11         5 waitpid
  0.05    0.002973          10       284           brk
  0.04    0.002816         469         6           execve
<snip>

After (cold cache, no lstat fix, just cache_preload):
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 90.90   23.717981         413     57432           lstat64
  8.72    2.273917      162423        14         2 futex
  0.12    0.032241         948        34           close
  0.04    0.011507         202        57           munmap
  0.04    0.009648         132        73           mmap2
  0.03    0.008508         149        57        20 open
  0.03    0.007771         311        25           mprotect
  0.03    0.007758         388        20           clone
  0.03    0.007548          23       334           write
  0.02    0.005247         262        20        10 access

-brandon

Re:

[Brandon Casey]

Linus Torvalds wrote:
> 
> On Fri, 8 May 2009, Brandon Casey wrote:
>> plain 'git checkout' on linux kernel over NFS.
> 
> Thanks.
> 
>> Best time without patch: 1.20 seconds
>>
>>   0.45user 0.71system 0:01.20elapsed 96%CPU (0avgtext+0avgdata 0maxresident)k
>>   0inputs+0outputs (0major+15467minor)pagefaults 0swaps
>>
>> Best time with patch (core.preloadindex = true): 1.10 seconds
>>
>>   0.43user 4.00system 0:01.10elapsed 402%CPU (0avgtext+0avgdata 0maxresident)k
>>   0inputs+0outputs (0major+13999minor)pagefaults 0swaps
>>
>> Best time with patch (core.preloadindex = false): 0.84 seconds
>>
>>   0.42user 0.39system 0:00.84elapsed 96%CPU (0avgtext+0avgdata 0maxresident)k
>>   0inputs+0outputs (0major+13965minor)pagefaults 0swaps
> 
> Ok, that is _disgusting_. The parallelism clearly works (402%CPU), but the 
> system time overhead is horrible. Going from 0.39s system time to 4s of 
> system time is really quite nasty.
> 
> Is there any possibility you could oprofile this (run it in a loop to get 
> better profiles)? It very much sounds like some serious lock contention, 
> and I'd love to hear more about exactly which lock it's hitting.

Possibly, I'll see if our sysadmin has time to "play".

> Also, you're already almost totally CPU-bound, with 96% CPU for the 
> single-threaded csase. So you may be running over NFS, but your NFS server 
> is likely pretty good and/or the client just captures everything in the 
> caches anyway.
> 
> I don't recall what the Linux NFS stat cache timeout is, but it's less 
> than a minute. I suspect that you ran things in a tight loop, which is why 
> you then got effectively the local caching behavior for the best times. 

Yeah, that's what I did.

> Can you do a "best time" check but with a 60-second pause between runs 
> (and before), to see what happens when the client doesn't do caching?

No problem.

>> Best time with read_cache_preload patch only: 1.38 seconds
>>
>>   0.45user 4.42system 0:01.38elapsed 352%CPU (0avgtext+0avgdata 0maxresident)k
>>   0inputs+0outputs (0major+13990minor)pagefaults 0swaps
> 
> Yeah, here you're not getting any advantage of fewer lstats, and you 
> show the same "almost entirely CPU-bound on four cores" behavior, and the 
> same (probable) lock contention that has pushed the system time way up.
> 
>> The read_cache_preload() changes actually slow things down for me for this
>> case.
>>
>> Reduction in lstat's gives a nice 30% improvement.
> 
> Yes, I think the one-liner lstat avoidance is a real fix regardless. And 
> the preloading sounds like it hits serialization overhead in the kernel, 
> which I'm not at all surprised at, but not being surprised doesn't mean 
> that I'm not interested to hear where it is.
> 
> The Linux VFS dcache itself should scale better than that (but who knows - 
> cacheline ping-pong due to lock contention can easily cause a 10x slowdown 
> even without being _totally_ contended all the time). So I would _suspect_ 
> that it's some NFS lock that you're seeing, but I'd love to know more.
> 
> Btw, those system times are pretty high to begin with, so I'd love to know 
> kernel version and see a profile even without the parallel case and 
> presumably lock contention. Because while I probably have a faster 
> machine anyway, what I see iis:
> 
> 	[torvalds@nehalem linux]$ /usr/bin/time git checkout
> 	0.13user 0.05system 0:00.19elapsed 98%CPU (0avgtext+0avgdata 0maxresident)k
> 	0inputs+0outputs (0major+13334minor)pagefaults 0swaps
> 
> ie my "system" time is _much_ lower than yours (and lower than your system 
> time). This is the 'without patch' time, btw, so this has extra lstat's. 
> And my system time is still lower than my user time, so I wonder where all 
> _your_ system time comes from. Your system time is much more comparable to 
> user time even in the good case, and I wonder why?
> 
> Could be just that kernel code tends to have more cache misses, and my 8MB 
> cache captures it all, and yours doesn't. Regardless, a profile would be 
> very interesting.

Something is definitely up.

I provided timing results for your original preload_cache implementation
which affected status and diff, which was part of the justification for
merging it in.

   http://article.gmane.org/gmane.comp.version-control.git/100998

You can see that cold cache system time for 'git status' went from 0.36 to
0.52 seconds.  Fine.  I just ran it again, and now I'm getting system time
of 10 seconds!  This is the same machine.

Similarly for the cold cache 'git checkout' reruns:

Best without patch: 6.02 (systime 1.57)

  0.43user 1.57system 0:06.02elapsed 33%CPU (0avgtext+0avgdata 0maxresident)k
  5336inputs+0outputs (12major+15472minor)pagefaults 0swaps

Best with patch (preload_cache,lstat reduction): 2.69 (systime 10.47)

  0.45user 10.47system 0:02.69elapsed 405%CPU (0avgtext+0avgdata 0maxresident)k
  5336inputs+0outputs (12major+13985minor)pagefaults 0swaps


OS: Centos4.7

$ cat /proc/version
Linux version 2.6.9-78.0.17.ELsmp (mockbuild@builder16.centos.org) (gcc version 3.4.6 20060404 (Red Hat 3.4.6-9)) #1 SMP Thu Mar 12 20:05:15 EDT 2009

-brandon

Re:

[Linus Torvalds]

On Fri, 8 May 2009, Brandon Casey wrote:
> 
> plain 'git checkout' on linux kernel over NFS.

Thanks.

> Best time without patch: 1.20 seconds
> 
>   0.45user 0.71system 0:01.20elapsed 96%CPU (0avgtext+0avgdata 0maxresident)k
>   0inputs+0outputs (0major+15467minor)pagefaults 0swaps
> 
> Best time with patch (core.preloadindex = true): 1.10 seconds
> 
>   0.43user 4.00system 0:01.10elapsed 402%CPU (0avgtext+0avgdata 0maxresident)k
>   0inputs+0outputs (0major+13999minor)pagefaults 0swaps
> 
> Best time with patch (core.preloadindex = false): 0.84 seconds
> 
>   0.42user 0.39system 0:00.84elapsed 96%CPU (0avgtext+0avgdata 0maxresident)k
>   0inputs+0outputs (0major+13965minor)pagefaults 0swaps

Ok, that is _disgusting_. The parallelism clearly works (402%CPU), but the 
system time overhead is horrible. Going from 0.39s system time to 4s of 
system time is really quite nasty.

Is there any possibility you could oprofile this (run it in a loop to get 
better profiles)? It very much sounds like some serious lock contention, 
and I'd love to hear more about exactly which lock it's hitting.

Also, you're already almost totally CPU-bound, with 96% CPU for the 
single-threaded csase. So you may be running over NFS, but your NFS server 
is likely pretty good and/or the client just captures everything in the 
caches anyway.

I don't recall what the Linux NFS stat cache timeout is, but it's less 
than a minute. I suspect that you ran things in a tight loop, which is why 
you then got effectively the local caching behavior for the best times. 

Can you do a "best time" check but with a 60-second pause between runs 
(and before), to see what happens when the client doesn't do caching?

> Best time with read_cache_preload patch only: 1.38 seconds
> 
>   0.45user 4.42system 0:01.38elapsed 352%CPU (0avgtext+0avgdata 0maxresident)k
>   0inputs+0outputs (0major+13990minor)pagefaults 0swaps

Yeah, here you're not getting any advantage of fewer lstats, and you 
show the same "almost entirely CPU-bound on four cores" behavior, and the 
same (probable) lock contention that has pushed the system time way up.

> The read_cache_preload() changes actually slow things down for me for this
> case.
> 
> Reduction in lstat's gives a nice 30% improvement.

Yes, I think the one-liner lstat avoidance is a real fix regardless. And 
the preloading sounds like it hits serialization overhead in the kernel, 
which I'm not at all surprised at, but not being surprised doesn't mean 
that I'm not interested to hear where it is.

The Linux VFS dcache itself should scale better than that (but who knows - 
cacheline ping-pong due to lock contention can easily cause a 10x slowdown 
even without being _totally_ contended all the time). So I would _suspect_ 
that it's some NFS lock that you're seeing, but I'd love to know more.

Btw, those system times are pretty high to begin with, so I'd love to know 
kernel version and see a profile even without the parallel case and 
presumably lock contention. Because while I probably have a faster 
machine anyway, what I see iis:

	[torvalds@nehalem linux]$ /usr/bin/time git checkout
	0.13user 0.05system 0:00.19elapsed 98%CPU (0avgtext+0avgdata 0maxresident)k
	0inputs+0outputs (0major+13334minor)pagefaults 0swaps

ie my "system" time is _much_ lower than yours (and lower than your system 
time). This is the 'without patch' time, btw, so this has extra lstat's. 
And my system time is still lower than my user time, so I wonder where all 
_your_ system time comes from. Your system time is much more comparable to 
user time even in the good case, and I wonder why?

Could be just that kernel code tends to have more cache misses, and my 8MB 
cache captures it all, and yours doesn't. Regardless, a profile would be 
very interesting.

			Linus

RE:

[Bevan Watkiss]

> -----Original Message-----
> From: david@lang.hm [mailto:david@lang.hm]
> Sent: May 7, 2009 7:31 PM
> To: Linus Torvalds
> Cc: Bevan Watkiss; 'Alex Riesen'; Git Mailing List
> Subject: RE:
> 
> On Thu, 7 May 2009, Linus Torvalds wrote:
> 
> > On Thu, 7 May 2009, david@lang.hm wrote:
> >>
> >> what about a reset --hard? (is there any command that would force the
> files to
> >> be re-written, no matter what git thinks is already there)
> >
> > No, not "git reset --hard" either, I think. Git very much tries to avoid
> > rewriting files, and if you've told it that file contents are stable, it
> > will believe you.
> >
> > In fact, I think people used CE_VALID explicitly for the missing parts
> of
> > "partial checkouts", so if we'd suddenly start writing files despite
> them
> > being marked as ok in the tree, I think we'd have broken that part.
> >
> > (Although again - I'm not sure who would use CE_VALID and friends).
> >
> > If you want to force everything to be rewritten, you should just remove
> > the index (or remove the specific entries in it if you want to do it
> just
> > to a particular file) and then do a "git checkout" to re-read and
> > re-populate the tree.
> >
> > But I'm not really seeing why you want to do this. If you told git that
> it
> > shouldn't care about the working tree, why do you now want it do care?
> 
> to be able to manually recover from the case where someone did things that
> they weren't supposed to
> 
> removing the index and doing a checkout would be a reasonable thing to do
> (at least conceptually), I will admit that I don't remember ever seeing a
> command (or discussion of one) that would let me do that.

Added the patch and now the time is down to 4 1/2 minutes.  Still a little
slow for my needs though.  

Since I'm looking for a more instantaneous update I'll probably use
something more along the lines of
	git fetch origin/master
	git log --name-only ..HEAD
to get the list of files that have changed and copy them from a local
repository.  Nightly doing a real pull to confirm the files are correct and
up to date.

Bevan

Re:

[Brandon Casey]

Linus Torvalds wrote:
> 
> On Fri, 8 May 2009, Alex Riesen wrote:
>> I did (cygwin). My guess, the improvement is completely dwarfed by the
>> other overheads (like starting git and writing files).
> 
> Oh, I meant "git checkout" as in not even switching branches, or perhaps 
> switching branches but just changing a single file (among thousands).
> 
> If you actually end up re-writing all files, then yes, it will obviously 
> be totally dominated by other things.
> 
> For example, in the kernel, switching between two branches that only 
> differ in one file (Makefile) went from 0.18 seconds down to 0.14 seconds 
> for me just because of the fewer lstat() calls.
> 
> Noticeable? No. But it might be more noticeable on some other OS, or with 
> some networked filesystem.

plain 'git checkout' on linux kernel over NFS.

Best time without patch: 1.20 seconds

  0.45user 0.71system 0:01.20elapsed 96%CPU (0avgtext+0avgdata 0maxresident)k
  0inputs+0outputs (0major+15467minor)pagefaults 0swaps

Best time with patch (core.preloadindex = true): 1.10 seconds

  0.43user 4.00system 0:01.10elapsed 402%CPU (0avgtext+0avgdata 0maxresident)k
  0inputs+0outputs (0major+13999minor)pagefaults 0swaps

Best time with patch (core.preloadindex = false): 0.84 seconds

  0.42user 0.39system 0:00.84elapsed 96%CPU (0avgtext+0avgdata 0maxresident)k
  0inputs+0outputs (0major+13965minor)pagefaults 0swaps

Best time with read_cache_preload patch only: 1.38 seconds

  0.45user 4.42system 0:01.38elapsed 352%CPU (0avgtext+0avgdata 0maxresident)k
  0inputs+0outputs (0major+13990minor)pagefaults 0swaps

The read_cache_preload() changes actually slow things down for me for this
case.

Reduction in lstat's gives a nice 30% improvement.

-brandon

Re:

[Linus Torvalds]

On Fri, 8 May 2009, Alex Riesen wrote:
> 
> I did (cygwin). My guess, the improvement is completely dwarfed by the
> other overheads (like starting git and writing files).

Oh, I meant "git checkout" as in not even switching branches, or perhaps 
switching branches but just changing a single file (among thousands).

If you actually end up re-writing all files, then yes, it will obviously 
be totally dominated by other things.

For example, in the kernel, switching between two branches that only 
differ in one file (Makefile) went from 0.18 seconds down to 0.14 seconds 
for me just because of the fewer lstat() calls.

Noticeable? No. But it might be more noticeable on some other OS, or with 
some networked filesystem.

		Linus

Re:

[Alex Riesen]

2009/5/7 Linus Torvalds <torvalds@linux-foundation.org>:
>
> Somebody should check. It would be interesting to hear about whether this
> makes a performance impact, especially with slow filesystems and/or other
> operating systems that have a relatively higher cost for 'lstat()'.
>

I did (cygwin). My guess, the improvement is completely dwarfed by the
other overheads (like starting git and writing files).

# Without the patch
real    11m22.338s
user    0m54.629s
sys     8m33.638s

# With checkout index preload
real    11m14.361s
user    0m46.609s
sys     7m56.300s

The script:

#!/bin/sh

if [ "$1" = setup ]; then
    for i in 1 2 3 4
    do
        n=$(date)
        for f in `seq 1 10000`
        do
            echo "$n" >file$f
        done
        git add .
        printf "Commit $i:"
        git commit -m"$n"
    done
    exit
fi

export GIT_EXEC_PATH=/d/git-win
time for f in `seq 1 10`
do
    $GIT_EXEC_PATH/git checkout master~3 &&
    $GIT_EXEC_PATH/git checkout master~2 &&
    $GIT_EXEC_PATH/git checkout master~1 &&
    $GIT_EXEC_PATH/git checkout master
done
exit

Re:

[Johan Herland]

On Friday 08 May 2009, david@lang.hm wrote:
> removing the index and doing a checkout would be a reasonable thing to do
> (at least conceptually), I will admit that I don't remember ever seeing a
> command (or discussion of one) that would let me do that.

What about:

  rm .git/index
  git checkout -f

or maybe:

  git update-index --no-assume-unchanged --refresh
  git checkout -f

Hm?

....Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

RE:

[david]

On Thu, 7 May 2009, Linus Torvalds wrote:

> On Thu, 7 May 2009, david@lang.hm wrote:
>>
>> what about a reset --hard? (is there any command that would force the files to
>> be re-written, no matter what git thinks is already there)
>
> No, not "git reset --hard" either, I think. Git very much tries to avoid
> rewriting files, and if you've told it that file contents are stable, it
> will believe you.
>
> In fact, I think people used CE_VALID explicitly for the missing parts of
> "partial checkouts", so if we'd suddenly start writing files despite them
> being marked as ok in the tree, I think we'd have broken that part.
>
> (Although again - I'm not sure who would use CE_VALID and friends).
>
> If you want to force everything to be rewritten, you should just remove
> the index (or remove the specific entries in it if you want to do it just
> to a particular file) and then do a "git checkout" to re-read and
> re-populate the tree.
>
> But I'm not really seeing why you want to do this. If you told git that it
> shouldn't care about the working tree, why do you now want it do care?

to be able to manually recover from the case where someone did things that 
they weren't supposed to

removing the index and doing a checkout would be a reasonable thing to do 
(at least conceptually), I will admit that I don't remember ever seeing a 
command (or discussion of one) that would let me do that.

RE:

[Linus Torvalds]

On Thu, 7 May 2009, david@lang.hm wrote:
> 
> what about a reset --hard? (is there any command that would force the files to
> be re-written, no matter what git thinks is already there)

No, not "git reset --hard" either, I think. Git very much tries to avoid 
rewriting files, and if you've told it that file contents are stable, it 
will believe you.

In fact, I think people used CE_VALID explicitly for the missing parts of 
"partial checkouts", so if we'd suddenly start writing files despite them 
being marked as ok in the tree, I think we'd have broken that part.

(Although again - I'm not sure who would use CE_VALID and friends).

If you want to force everything to be rewritten, you should just remove 
the index (or remove the specific entries in it if you want to do it just 
to a particular file) and then do a "git checkout" to re-read and 
re-populate the tree.

But I'm not really seeing why you want to do this. If you told git that it 
shouldn't care about the working tree, why do you now want it do care?

			Linus

RE:

[david]

On Thu, 7 May 2009, Linus Torvalds wrote:

> On Thu, 7 May 2009, david@lang.hm wrote:
>>
>> even with this a git checkout -f should replace all files, correct?
>
> Hmm. I don't think so.
>
> As far as I recall, "-f" only overrides certain errors (like unmerged
> files or not up-to-date content), it doesn't change behavior wrt files
> that git thinks are already up-to-date.

what about a reset --hard? (is there any command that would force the 
files to be re-written, no matter what git thinks is already there)

David Lang

RE:

[Linus Torvalds]

On Thu, 7 May 2009, david@lang.hm wrote:
> 
> even with this a git checkout -f should replace all files, correct?

Hmm. I don't think so.

As far as I recall, "-f" only overrides certain errors (like unmerged 
files or not up-to-date content), it doesn't change behavior wrt files 
that git thinks are already up-to-date.

But I didn't check.

		Linus

RE:

[david]

On Thu, 7 May 2009, Linus Torvalds wrote:

> On Thu, 7 May 2009, david@lang.hm wrote:
>>
>> his use case (as I understand it) is that the working tree is never updated by
>> anything other than git. it never recieves patches or manual edits.
>
> Well, you can certainly just use the CE_VALID bit in the index too (and
> this time I really mean CE_VALID). But it won't help anybody else, so it
> wouldn't be nearly as interesting. And I wonder how badly that code has
> rotted, thanks to not getting used.
>
> But yes, one thing to do would be
>
> 	git update-index --assume-unchanged --refresh
>
> which should hopefully set the bit, and then after that setting
> 'core.ignoreStat' should hopefully keep it set.
>
> Of course, you had then better _never_ make any mistakes and touch the
> files with non-git commands.

even with this a git checkout -f should replace all files, correct?

David Lang

> And hope that the code still works ;)
>
> 		Linus
>

RE:

[Linus Torvalds]

On Thu, 7 May 2009, david@lang.hm wrote:
> 
> his use case (as I understand it) is that the working tree is never updated by
> anything other than git. it never recieves patches or manual edits.

Well, you can certainly just use the CE_VALID bit in the index too (and 
this time I really mean CE_VALID). But it won't help anybody else, so it 
wouldn't be nearly as interesting. And I wonder how badly that code has 
rotted, thanks to not getting used.

But yes, one thing to do would be

	git update-index --assume-unchanged --refresh

which should hopefully set the bit, and then after that setting 
'core.ignoreStat' should hopefully keep it set.

Of course, you had then better _never_ make any mistakes and touch the 
files with non-git commands.

And hope that the code still works ;)

		Linus

RE:

[david]

On Thu, 7 May 2009, Linus Torvalds wrote:

this patch is worthwhile in itself, but the use case that is presented 
here is slightly different, and I wonder if it's common enough to be worth 
having a config option for.

his use case (as I understand it) is that the working tree is never 
updated by anything other than git. it never recieves patches or manual 
edits.

as such _any_ lstats of the tree are a waste of time. if git knows what 
was checked out before and what is being checked out now, it can find what 
files need to be changed.

this situation is not common for most developers, but it would be 
reasonable for build farms, so it's not just a one-person issue.

David Lang


> On Thu, 7 May 2009, Linus Torvalds wrote:
>>
>> Hmm. The second pass comes from
>>
>> 	show_local_changes(&new->commit->object);
>>
>> (this is the "git checkout" without actual filenames), and is suppressed
>> if we ask for a quiet checkout. But it's sad how it re-loads the index. I
>> wonder where the CE_VALID bit got dropped.
>
> Ahh. It's not actually dropped, it's still there.
>
> It's just that 'get_stat_data()' doesn't check it, when asking for
> noncached data.
>
> The logic of 'get_stat_data()' is that it will return the stat data from
> the filesystem (unless we explicitly ask for just the cached case, in
> which case it will take it from the cache entry directly).
>
> However, the code doesn't realize that if ce_uptodate() is true, then we
> already know the stat data, so no need to do the lstat() again, and we
> can take it all from the cache entry regardless of whether we asked for
> filesystem data or cached data.
>
> So here's a better patch. It should cut down the 'lstat()' calls from "git
> checkout" a lot.
>
> It looks obvious enough, and it passes testing (and now "git checkout"
> only does about as many lstat's as there are files in the repository, and
> they seem to all be properly asynchronous if 'core.preloadindex' is set.
>
> Somebody should check. It would be interesting to hear about whether this
> makes a performance impact, especially with slow filesystems and/or other
> operating systems that have a relatively higher cost for 'lstat()'.
>
> 		Linus
>
> ---
> builtin-checkout.c |    4 ++--
> diff-lib.c         |    2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index 15f0c32..3100ccd 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -216,7 +216,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
> 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
>
> 	newfd = hold_locked_index(lock_file, 1);
> -	if (read_cache() < 0)
> +	if (read_cache_preload(pathspec) < 0)
> 		return error("corrupt index file");
>
> 	if (source_tree)
> @@ -367,7 +367,7 @@ static int merge_working_tree(struct checkout_opts *opts,
> 	int newfd = hold_locked_index(lock_file, 1);
> 	int reprime_cache_tree = 0;
>
> -	if (read_cache() < 0)
> +	if (read_cache_preload(NULL) < 0)
> 		return error("corrupt index file");
>
> 	cache_tree_free(&active_cache_tree);
> diff --git a/diff-lib.c b/diff-lib.c
> index a310fb2..0aba6cd 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -214,7 +214,7 @@ static int get_stat_data(struct cache_entry *ce,
> 	const unsigned char *sha1 = ce->sha1;
> 	unsigned int mode = ce->ce_mode;
>
> -	if (!cached) {
> +	if (!cached && !ce_uptodate(ce)) {
> 		int changed;
> 		struct stat st;
> 		changed = check_removed(ce, &st);
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

RE:

[Linus Torvalds]

On Thu, 7 May 2009, Linus Torvalds wrote:
> 
> Hmm. The second pass comes from 
> 
> 	show_local_changes(&new->commit->object);
> 
> (this is the "git checkout" without actual filenames), and is suppressed 
> if we ask for a quiet checkout. But it's sad how it re-loads the index. I 
> wonder where the CE_VALID bit got dropped.

Ahh. It's not actually dropped, it's still there.

It's just that 'get_stat_data()' doesn't check it, when asking for 
noncached data.

The logic of 'get_stat_data()' is that it will return the stat data from 
the filesystem (unless we explicitly ask for just the cached case, in 
which case it will take it from the cache entry directly).

However, the code doesn't realize that if ce_uptodate() is true, then we 
already know the stat data, so no need to do the lstat() again, and we 
can take it all from the cache entry regardless of whether we asked for 
filesystem data or cached data.

So here's a better patch. It should cut down the 'lstat()' calls from "git 
checkout" a lot.

It looks obvious enough, and it passes testing (and now "git checkout" 
only does about as many lstat's as there are files in the repository, and 
they seem to all be properly asynchronous if 'core.preloadindex' is set.

Somebody should check. It would be interesting to hear about whether this 
makes a performance impact, especially with slow filesystems and/or other 
operating systems that have a relatively higher cost for 'lstat()'.

		Linus

---
 builtin-checkout.c |    4 ++--
 diff-lib.c         |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 15f0c32..3100ccd 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -216,7 +216,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
 	newfd = hold_locked_index(lock_file, 1);
-	if (read_cache() < 0)
+	if (read_cache_preload(pathspec) < 0)
 		return error("corrupt index file");
 
 	if (source_tree)
@@ -367,7 +367,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 	int newfd = hold_locked_index(lock_file, 1);
 	int reprime_cache_tree = 0;
 
-	if (read_cache() < 0)
+	if (read_cache_preload(NULL) < 0)
 		return error("corrupt index file");
 
 	cache_tree_free(&active_cache_tree);
diff --git a/diff-lib.c b/diff-lib.c
index a310fb2..0aba6cd 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -214,7 +214,7 @@ static int get_stat_data(struct cache_entry *ce,
 	const unsigned char *sha1 = ce->sha1;
 	unsigned int mode = ce->ce_mode;
 
-	if (!cached) {
+	if (!cached && !ce_uptodate(ce)) {
 		int changed;
 		struct stat st;
 		changed = check_removed(ce, &st);

Re:

[Linus Torvalds]

On Thu, 7 May 2009, Junio C Hamano wrote:
>
> I do not think you mean CE_VALID; CE_UPTODATE isn't it?

Yes, sorry.

		Linus

Re:

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, 7 May 2009, Linus Torvalds wrote:
>>
>> The patch is TOTALLY UNTESTED. It also worries me that 'git checkout' 
>> seems to do _two_ 'lstat()' calls per file. I didn't look any more 
>> closely, but there may be other issues here.
>
> Hmm. The second pass comes from 
>
> 	show_local_changes(&new->commit->object);
>
> (this is the "git checkout" without actual filenames), and is suppressed 
> if we ask for a quiet checkout. But it's sad how it re-loads the index. I 
> wonder where the CE_VALID bit got dropped.

I do not think you mean CE_VALID; CE_UPTODATE isn't it?

RE:

[Linus Torvalds]

On Thu, 7 May 2009, Linus Torvalds wrote:
>
> The patch is TOTALLY UNTESTED. It also worries me that 'git checkout' 
> seems to do _two_ 'lstat()' calls per file. I didn't look any more 
> closely, but there may be other issues here.

Hmm. The second pass comes from 

	show_local_changes(&new->commit->object);

(this is the "git checkout" without actual filenames), and is suppressed 
if we ask for a quiet checkout. But it's sad how it re-loads the index. I 
wonder where the CE_VALID bit got dropped.

			Linus

RE:

[Linus Torvalds]

On Thu, 7 May 2009, Bevan Watkiss wrote:
>
> Looking at the trace it does appear that most of this is the lstat.  It's
> the problem of having many tiny files on a network drive, and trying to use
> git for something it's not meant.
> 
> The log has 265430 lines of lstat and 10887 other lines.  If you still want
> the log file I'll strip out the directory names and send it off.

Actually, if it's just the lstat's, then it's not all that interesting any 
more, it's a known problem with at least a known _partial_ solution.

However, I think it turns out that we've only enabled the index preloading 
with "git diff" and "git commit". Not on "git checkout".

So start off doing that

> 	[core]
> 		preloadindex = true

AND apply the following patch to git, and see how much (if any) that 
helps. It sounds like you have a pretty damn large repository, together 
with a slow filesystem. It really could be a big improvement.

The patch is TOTALLY UNTESTED. It also worries me that 'git checkout' 
seems to do _two_ 'lstat()' calls per file. I didn't look any more 
closely, but there may be other issues here.

		Linus

---
 builtin-checkout.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 15f0c32..3100ccd 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -216,7 +216,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
 	newfd = hold_locked_index(lock_file, 1);
-	if (read_cache() < 0)
+	if (read_cache_preload(pathspec) < 0)
 		return error("corrupt index file");
 
 	if (source_tree)
@@ -367,7 +367,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 	int newfd = hold_locked_index(lock_file, 1);
 	int reprime_cache_tree = 0;
 
-	if (read_cache() < 0)
+	if (read_cache_preload(NULL) < 0)
 		return error("corrupt index file");
 
 	cache_tree_free(&active_cache_tree);

Re:

[Björn Steinbrink]

[Please don't top-post...]

On 2009.05.07 14:48:20 -0400, Bevan Watkiss wrote:
> From: Alex Riesen [mailto:raa.lkml@gmail.com] 
> > 2009/5/7 Bevan Watkiss <bevan.watkiss@cloakware.com>:
> > > It's the looking for local changes I'm trying to avoid.  Doing a
> > > reset still goes over the tree, which isn't helpful.
> > 
> > The stat(2) is slow? Then try setting core.ignoreStat (see manpage
> > of git config) to true: git config core.ignorestat true and read
> > below.
>
> Still took 11 minutes.

IIRC, to see the effects of core.ignorestat, you need to have updated
all files once. So you might need, for example, "git checkout -f HEAD"
(not sure if a plain checkout is enough) once first, and then the future
"git checkout $something" should be faster.

Björn

RE:

[Bevan Watkiss]

Looking at the trace it does appear that most of this is the lstat.  It's
the problem of having many tiny files on a network drive, and trying to use
git for something it's not meant.

The log has 265430 lines of lstat and 10887 other lines.  If you still want
the log file I'll strip out the directory names and send it off.

It would be nice to have an option that you can pull only the files that
changed in the changesets you are updating and ignore the state of the other
files.

Bevan

-----Original Message-----
From: Linus Torvalds [mailto:torvalds@linux-foundation.org] 
Sent: May 7, 2009 2:56 PM
To: Bevan Watkiss
Cc: 'Alex Riesen'; git@vger.kernel.org
Subject: RE: 



On Thu, 7 May 2009, Bevan Watkiss wrote:
> 
> Basically I have a copy of my tree where only git can write to it, so I
know
> the files are right.  The NAS box I have the tree on is slow, so reading
the
> tree adds about 10 minutes to the process when I only want to update a few
> files.

Ouch.

You could try doing

	[core]
		preloadindex = true

and see if that helps some of your loads. It does limit even the parallel 
tree stat to 20 or so, but if most of your cost is in just doing the 
lstat() over the files to see that they haven't changed, you might be 
getting a factor-of-20 speedup for at least _some_ of what you do.

If you can, it might also be interesting to see system call trace patterns 
(with times!) to see if there is something obviously horribly bad going 
on. If you're running under Linux, and don't think the data contains 
anything very private, send me the output of "strace -f -T" of the most 
problematic operations, and maybe I can see if I can come up with anything 
interesting.

I have long refused to use networked filesystems because I used to find 
them -so- painful when working with CVS, so none of my performance work 
has ever really directly concentrated on long-latency filesystems. Even 
the index preload was all done "blind" with other people reporting issues 
(and happily I could see some of the effects with local filesystems and 
multiple CPU's ;).

			Linus
	

RE:

[Linus Torvalds]

On Thu, 7 May 2009, Bevan Watkiss wrote:
> 
> Basically I have a copy of my tree where only git can write to it, so I know
> the files are right.  The NAS box I have the tree on is slow, so reading the
> tree adds about 10 minutes to the process when I only want to update a few
> files.

Ouch.

You could try doing

	[core]
		preloadindex = true

and see if that helps some of your loads. It does limit even the parallel 
tree stat to 20 or so, but if most of your cost is in just doing the 
lstat() over the files to see that they haven't changed, you might be 
getting a factor-of-20 speedup for at least _some_ of what you do.

If you can, it might also be interesting to see system call trace patterns 
(with times!) to see if there is something obviously horribly bad going 
on. If you're running under Linux, and don't think the data contains 
anything very private, send me the output of "strace -f -T" of the most 
problematic operations, and maybe I can see if I can come up with anything 
interesting.

I have long refused to use networked filesystems because I used to find 
them -so- painful when working with CVS, so none of my performance work 
has ever really directly concentrated on long-latency filesystems. Even 
the index preload was all done "blind" with other people reporting issues 
(and happily I could see some of the effects with local filesystems and 
multiple CPU's ;).

			Linus
	

RE:

[Bevan Watkiss]

Still took 11 minutes.

The idea I've come up with today is something along the lines of
git fetch origin/master
git log --name-only ..<hash> | xargs git checkout -f --

This should work to quickly keep my files upto date, and I can then
periodically pull properly to move the HEAD.

Thanks for the info

Bevan

-----Original Message-----
From: Alex Riesen [mailto:raa.lkml@gmail.com] 
Sent: May 7, 2009 2:18 PM
To: Bevan Watkiss
Cc: git@vger.kernel.org
Subject: Re:

2009/5/7 Bevan Watkiss <bevan.watkiss@cloakware.com>:
> It's the looking for local changes I'm trying to avoid.  Doing a reset
still
> goes over the tree, which isn't helpful.

The stat(2) is slow? Then try setting core.ignoreStat (see manpage
of git config) to true: git config core.ignorestat true
and read below.

> Basically I have a copy of my tree where only git can write to it, so I
know
> the files are right.  The NAS box I have the tree on is slow, so reading
the
> tree adds about 10 minutes to the process when I only want to update a few
> files.

Try "git checkout origin/master". It uses index and shouldn't checkout files
which are uptodate with the index. And actually, git merge should
fast-forward,
in your case and will update just the changed files...

Of course, you can always compare HEAD and origin/master, and resolve
the changes yourself (see git diff -z --name-status), but it is unlikely to
be
any faster.

Re:

[Alex Riesen]

2009/5/7 Bevan Watkiss <bevan.watkiss@cloakware.com>:
> It's the looking for local changes I'm trying to avoid.  Doing a reset still
> goes over the tree, which isn't helpful.

The stat(2) is slow? Then try setting core.ignoreStat (see manpage
of git config) to true: git config core.ignorestat true
and read below.

> Basically I have a copy of my tree where only git can write to it, so I know
> the files are right.  The NAS box I have the tree on is slow, so reading the
> tree adds about 10 minutes to the process when I only want to update a few
> files.

Try "git checkout origin/master". It uses index and shouldn't checkout files
which are uptodate with the index. And actually, git merge should fast-forward,
in your case and will update just the changed files...

Of course, you can always compare HEAD and origin/master, and resolve
the changes yourself (see git diff -z --name-status), but it is unlikely to be
any faster.

RE:

[Bevan Watkiss]

It's the looking for local changes I'm trying to avoid.  Doing a reset still
goes over the tree, which isn't helpful.

Basically I have a copy of my tree where only git can write to it, so I know
the files are right.  The NAS box I have the tree on is slow, so reading the
tree adds about 10 minutes to the process when I only want to update a few
files.

-----Original Message-----
From: Alex Riesen [mailto:raa.lkml@gmail.com] 
Sent: May 7, 2009 1:14 PM
To: Bevan Watkiss
Cc: git@vger.kernel.org
Subject: Re:

2009/5/7 Bevan Watkiss <bevan.watkiss@cloakware.com>:
> I am trying to create a working tree for people to read from and have it
> update from a bare repository regularly.  Right now I am using git-pull to
> fetch the changes, but it’s running slow due to the size of my repo and
the
> speed of the hardware as it seems to be checking the working tree for any
> changes.
>
> Is there a way to make the pull ignore the local working tree and only
look
> at files that are changed in the change sets being pulled?

Assuming you didn't modify that directory you pull into,
git pull will do almost exactly what you described. Almost,
because the operation (the merge) will involve looking for local
changes (committed and not).

It should be faster to do something like this:

  git fetch && git reset --hard origin/master

Again, assuming the directory supposed to be read-only.
Otherwise, you have to merge (i.e. git pull).

Re:

[Alex Riesen]

2009/5/7 Bevan Watkiss <bevan.watkiss@cloakware.com>:
> I am trying to create a working tree for people to read from and have it
> update from a bare repository regularly.  Right now I am using git-pull to
> fetch the changes, but it’s running slow due to the size of my repo and the
> speed of the hardware as it seems to be checking the working tree for any
> changes.
>
> Is there a way to make the pull ignore the local working tree and only look
> at files that are changed in the change sets being pulled?

Assuming you didn't modify that directory you pull into,
git pull will do almost exactly what you described. Almost,
because the operation (the merge) will involve looking for local
changes (committed and not).

It should be faster to do something like this:

  git fetch && git reset --hard origin/master

Again, assuming the directory supposed to be read-only.
Otherwise, you have to merge (i.e. git pull).

Re:

[Junio C Hamano]

Markus Heidelberg <markus.heidelberg@web.de> writes:

> David Aguilar, 30.03.2009:
>> This is based on top of Junio's "pu" branch and is a
>> continuation of the recent difftool series.
>
> For everyone who wants to apply the patch series: Patch 5/8 depends on
> this:
>   [PATCH v2] difftool: add support for a difftool.prompt config variable
> sent about 8 minutes before this series.

Thanks for keeping an eye on this series, as a recent contributor to
mergetool.  I do not use mergetool myself, but this refactoring seems to
be a good idea in general, and help in reviewing the series is very much
appreciated.

Re:

[Markus Heidelberg]

David Aguilar, 30.03.2009:
> This is based on top of Junio's "pu" branch and is a
> continuation of the recent difftool series.

For everyone who wants to apply the patch series: Patch 5/8 depends on
this:
  [PATCH v2] difftool: add support for a difftool.prompt config variable
sent about 8 minutes before this series.

Markus

Re:

[Francesco Pretto]

2007/11/1, Francesco Pretto <ceztkoml@gmail.com>:
> subscribe git
>

Sorry! Wrong address trying to subscribe.

Re:

[H. Peter Anvin]

Jason Riedy wrote:
> I guess our home directories recently were changed from symlinks
> to autmounts.  Solaris 8's mkdir(2) returns ENOSYS when applied
> to these, breaking safe_create_leading_directories.  I don't
> know if ENOSYS is available everywhere, or if this odd behavior
> is appropriate everywhere.
> 
> This works for me, but should I wrap mkdir for bizarre behavior
> by adding a compat/gitmkdir.c?

Wow, Solaris really can be braindamaged sometimes...

	-hpa

Re:

[Martin Schlemmer]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On Fri, 2005-04-22 at 15:19 -0700, atani wrote:

<snip>

> Martin Schlemmer,  I ran "emerge sync" today and found git has been 
> added to portage, version 0.5.  Also note that there are now two "git" 
> entries within portage app-misc/git and dev-util/git.  app-misc/git is 
> GNU Interactive Tools 
>  

Yeah, I know - that is actually why I complained to r3pek, as most of
the guys interested in doing patches, etc will prob pull and build
themselfs, but the user that just want to get the latest kernel, will
rather want cogito (or git-pasky).  So basically the git I mentioned
that I wanted added (or maybe replace the current one in the tree
depending on what r3pek do), was Petr's stuff ...


Thanks,

-- 
Martin Schlemmer


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]