From: Elijah Newren <newren@gmail.com>
To: Ben Peart <peartben@gmail.com>
Cc: Ben Peart <Ben.Peart@microsoft.com>,
"git@vger.kernel.org" <git@vger.kernel.org>,
"peff@peff.net" <peff@peff.net>,
"gitster@pobox.com" <gitster@pobox.com>,
"pclouds@gmail.com" <pclouds@gmail.com>,
"vmiklos@frugalware.org" <vmiklos@frugalware.org>,
Kevin Willford <kewillf@microsoft.com>,
"Johannes.Schindelin@gmx.de" <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v1 2/2] merge: Add merge.aggressive config setting
Date: Tue, 24 Apr 2018 10:36:23 -0700 [thread overview]
Message-ID: <CABPp-BHYrxyg1W0+144M=Bstunuw36ZCtRJhvu5G_kCm1g7e4w@mail.gmail.com> (raw)
In-Reply-To: <a34144ff-b91e-6f00-93e8-b472ad5887d0@gmail.com>
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.
next prev parent reply other threads:[~2018-04-24 17:36 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-20 13:36 [PATCH v1 0/2] add additional config settings for merge Ben Peart
2018-04-20 13:36 ` [PATCH v1 1/2] merge: Add merge.renames config setting Ben Peart
2018-04-20 17:02 ` Elijah Newren
2018-04-20 17:26 ` Elijah Newren
2018-04-23 12:57 ` Ben Peart
2018-04-20 17:59 ` Ben Peart
2018-04-20 18:34 ` Elijah Newren
2018-04-21 4:23 ` Junio C Hamano
2018-04-23 16:00 ` Ben Peart
2018-04-23 23:23 ` Junio C Hamano
2018-04-24 11:58 ` Johannes Schindelin
2018-04-24 17:47 ` Elijah Newren
2018-04-25 8:20 ` Johannes Schindelin
2018-04-22 12:07 ` Eckhard Maaß
2018-04-23 13:15 ` Ben Peart
2018-04-23 21:32 ` Eckhard Maaß
2018-04-24 16:53 ` Ben Peart
2018-04-23 13:22 ` Ben Peart
2018-04-20 13:36 ` [PATCH v1 2/2] merge: Add merge.aggressive " Ben Peart
2018-04-20 17:22 ` Elijah Newren
2018-04-24 16:45 ` Ben Peart
2018-04-24 17:36 ` Elijah Newren [this message]
2018-04-24 23:57 ` Junio C Hamano
2018-04-25 14:47 ` Ben Peart
2018-04-20 17:34 ` [PATCH v1 0/2] add additional config settings for merge Elijah Newren
2018-04-20 18:19 ` Ben Peart
2018-04-24 17:11 ` [PATCH v2 " Ben Peart
2018-04-24 17:11 ` [PATCH v2 1/2] merge: Add merge.renames config setting Ben Peart
2018-04-24 18:11 ` Elijah Newren
2018-04-24 18:59 ` Elijah Newren
2018-04-24 20:31 ` Ben Peart
2018-04-25 16:01 ` Elijah Newren
2018-04-24 17:11 ` [PATCH v2 2/2] merge: Add merge.aggressive " Ben Peart
2018-04-25 0:13 ` [PATCH v2 0/2] add additional config settings for merge Junio C Hamano
2018-04-25 15:22 ` Ben Peart
2018-04-26 1:48 ` Junio C Hamano
2018-04-26 20:52 ` [PATCH v3 0/3] add merge.renames config setting Ben Peart
2018-04-26 20:52 ` [PATCH v3 1/3] merge: update documentation for {merge,diff}.renameLimit Ben Peart
2018-04-26 23:11 ` Elijah Newren
2018-04-26 23:23 ` Jonathan Tan
2018-04-26 20:52 ` [PATCH v3 2/3] merge: Add merge.renames config setting Ben Peart
2018-04-26 22:52 ` Elijah Newren
2018-04-27 0:54 ` Ben Peart
2018-04-27 2:23 ` Junio C Hamano
2018-04-27 3:28 ` Elijah Newren
2018-04-27 7:23 ` Johannes Schindelin
2018-04-27 14:32 ` Elijah Newren
2018-04-27 18:37 ` Eckhard Maaß
2018-04-27 20:23 ` Elijah Newren
2018-04-30 8:03 ` Eckhard Maaß
2018-04-30 16:54 ` Elijah Newren
2018-04-27 4:17 ` Elijah Newren
2018-04-27 18:19 ` Elijah Newren
2018-04-30 13:11 ` Ben Peart
2018-04-30 16:12 ` Re: Elijah Newren
2018-05-02 14:33 ` Re: Ben Peart
2018-04-26 20:52 ` [PATCH v3 3/3] merge: pass aggressive when rename detection is turned off Ben Peart
2018-04-26 23:00 ` Elijah Newren
2018-04-26 22:08 ` [PATCH v3 0/3] add merge.renames config setting Elijah Newren
2018-05-02 16:01 ` [PATCH v4 0/3] add additional config settings for merge Ben Peart
2018-05-02 16:01 ` [PATCH v4 1/3] merge: update documentation for {merge,diff}.renameLimit Ben Peart
2018-05-02 16:01 ` [PATCH v4 2/3] merge: Add merge.renames config setting Ben Peart
2018-05-04 3:07 ` Junio C Hamano
2018-05-02 16:01 ` [PATCH v4 3/3] merge: pass aggressive when rename detection is turned off Ben Peart
2018-05-02 17:20 ` [PATCH v4 0/3] add additional config settings for merge Elijah Newren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABPp-BHYrxyg1W0+144M=Bstunuw36ZCtRJhvu5G_kCm1g7e4w@mail.gmail.com' \
--to=newren@gmail.com \
--cc=Ben.Peart@microsoft.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=kewillf@microsoft.com \
--cc=pclouds@gmail.com \
--cc=peartben@gmail.com \
--cc=peff@peff.net \
--cc=vmiklos@frugalware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).