[PATCH 1 of 2 V2] revset: use smartset minus operator

[Martin von Zweigbergk]

Could you elaborate on why? It results in less duplication, which is
usually good for maintainability.

The only thing I disliked about my patch was the name "minusset". I would
perhaps rename that to "difference". Otherwise it seemed simple and clean
to me.

On Tue, Feb 23, 2016, 12:54 Durham Goode <durham at fb.com> wrote:

> Your patch routes all minus revsets through the and-not operator
> optimizer, which seemed inelegant and prone to future maintenance mistakes.
>
> From: Martin von Zweigbergk <martinvonz at google.com>
> Date: Tuesday, February 23, 2016 at 12:45 PM
> To: Durham Goode <durham at fb.com>
> Cc: "mercurial-devel at mercurial-scm.org" <mercurial-devel at mercurial-scm.org
> >
> Subject: Re: [PATCH 1 of 2 V2] revset: use smartset minus operator
>
> I think my patch also took care of all the improvements these two patches
> address. Are there any cases where my patch would be worse?
>
> On Tue, Feb 23, 2016, 12:30 Durham Goode <durham at fb.com> wrote:
>
>>
>>
>>
>>
>>
>> On 2/19/16, 11:39 PM, "Martin von Zweigbergk" <martinvonz at google.com>
>> wrote:
>>
>> >On Thu, Feb 18, 2016 at 11:38 AM, Durham Goode <durham at fb.com> wrote:
>> >> # HG changeset patch
>> >> # User Durham Goode <durham at fb.com>
>> >> # Date 1455824115 28800
>> >> #      Thu Feb 18 11:35:15 2016 -0800
>> >> # Node ID dc8d72c3df7dfcd7b67fa71d679cc7c3ad90f25c
>> >> # Parent  a036e1ae1fbe88ab99cb861ebfc2e4da7a3912ca
>> >> revset: use smartset minus operator
>> >>
>> >> Previously, revsets like 'X - Y' were translated to be 'X and not Y'.
>> This can
>> >> be expensive, since if Y is a single commit then 'not Y' becomes a
>> huge set and
>> >> sometimes the query optimizer doesn't account for it well.
>> >>
>> >> This patch changes revsets to use the built in smartset minus
>> operator, which is
>> >> often smarter than 'X and not Y'.
>> >
>> >How does this patch compare to my patch in
>> >
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__thread.gmane.org_gmane.comp.version-2Dcontrol.mercurial.devel_89348_focus-3D89391-3F&d=CwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Ic3AKLl02HHMU_yM5NlmBLJc3oZ8MUkGiPj-TnPQQiI&s=NoBhM7NiNWrsbHqoCgMbe-t_MuPSdYkIqB5W2go53mY&e=
>> >I did not include the test case changes there, but it only affects the
>> >first one among the ones affected by this patch, so there is no need
>> >to revert that behavior in a second patch like here. It also makes
>> >"(20000::) and not (20000)" faster (which these two patches do not).
>> >What are the disadvantages?
>>
>> We could add your patch as well, but it seems like it would have to be a
>> separate patch anyway (one adds minus and avoids the minus->and-not
>> translation, then another patch takes advantage of it for and not).  I did
>> not add the optimization to 'and' because none of the benchmark tests
>> showed any perf improvement.  I suppose "(20000::) and not (20000)" might
>> show an improvement, but that probably doesn't stop these from going.
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160223/531c64e1/attachment.html>