Patchwork [4,of,4] revlog: reuse 'descendant' implemention in 'isancestor'

login
register
mail settings
Submitter Paul Morelle
Date July 1, 2018, 6:38 a.m.
Message ID <6bfe8fc36b4e20fcdf6c.1530427122@belenos.localdomain>
Download mbox | patch
Permalink /patch/32539/
State Accepted
Headers show

Comments

Paul Morelle - July 1, 2018, 6:38 a.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1529622442 -3600
#      Fri Jun 22 00:07:22 2018 +0100
# Node ID 6bfe8fc36b4e20fcdf6cc49fe9ddb6e79bcf213f
# Parent  5ea9c5d20ecc1aac2aecdd4c0902b3cd470b04d5
# EXP-Topic descendant
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 6bfe8fc36b4e
revlog: reuse 'descendant' implemention in 'isancestor'

The two functions do the same thing, but one takes nodes while the other takes
revs. Using one to implement the other make sense.

We should probably cleanup the API at some point to avoid having so many similar
functions. However, we focus on an efficient implementation for now.
Yuya Nishihara - July 2, 2018, 12:03 p.m.
On Sun, 01 Jul 2018 08:38:42 +0200, Paul Morelle wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1529622442 -3600
> #      Fri Jun 22 00:07:22 2018 +0100
> # Node ID 6bfe8fc36b4e20fcdf6cc49fe9ddb6e79bcf213f
> # Parent  5ea9c5d20ecc1aac2aecdd4c0902b3cd470b04d5
> # EXP-Topic descendant
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 6bfe8fc36b4e
> revlog: reuse 'descendant' implemention in 'isancestor'

Queued the series, thanks.

> We should probably cleanup the API at some point to avoid having so many similar
> functions. However, we focus on an efficient implementation for now.

Yeah.

> diff -r 5ea9c5d20ecc -r 6bfe8fc36b4e mercurial/revlog.py
> --- a/mercurial/revlog.py	Fri Jun 22 00:05:20 2018 +0100
> +++ b/mercurial/revlog.py	Fri Jun 22 00:07:22 2018 +0100
> @@ -1404,7 +1404,8 @@
>  
>          The implementation of this is trivial but the use of
>          commonancestorsheads is not."""
> -        return a in self.commonancestorsheads(a, b)
> +        a, b = self.rev(a), self.rev(b)
> +        return self.descendant(a, b)

Strictly speaking, this is an API change since isancestor(null, whatever)
now returns True. But that seems more correct, and no core caller appears
to rely on the old behavior.
via Mercurial-devel - July 2, 2018, 4:37 p.m.
On Mon, Jul 2, 2018 at 5:05 AM Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 01 Jul 2018 08:38:42 +0200, Paul Morelle wrote:
> > # HG changeset patch
> > # User Boris Feld <boris.feld@octobus.net>
> > # Date 1529622442 -3600
> > #      Fri Jun 22 00:07:22 2018 +0100
> > # Node ID 6bfe8fc36b4e20fcdf6cc49fe9ddb6e79bcf213f
> > # Parent  5ea9c5d20ecc1aac2aecdd4c0902b3cd470b04d5
> > # EXP-Topic descendant
> > # Available At https://bitbucket.org/octobus/mercurial-devel/
> > #              hg pull https://bitbucket.org/octobus/mercurial-devel/
> -r 6bfe8fc36b4e
> > revlog: reuse 'descendant' implemention in 'isancestor'
>
> Queued the series, thanks.
>
> > We should probably cleanup the API at some point to avoid having so many
> similar
> > functions. However, we focus on an efficient implementation for now.
>
> Yeah.
>

Perhaps we should rename "descendant" to "isdescendant" to match
"isancestor" then.
Boris Feld - July 3, 2018, 10:05 a.m.
On 02/07/2018 18:37, Martin von Zweigbergk via Mercurial-devel wrote:
>
>
> On Mon, Jul 2, 2018 at 5:05 AM Yuya Nishihara <yuya@tcha.org 
> <mailto:yuya@tcha.org>> wrote:
>
>     On Sun, 01 Jul 2018 08:38:42 +0200, Paul Morelle wrote:
>     > # HG changeset patch
>     > # User Boris Feld <boris.feld@octobus.net
>     <mailto:boris.feld@octobus.net>>
>     > # Date 1529622442 -3600
>     > #      Fri Jun 22 00:07:22 2018 +0100
>     > # Node ID 6bfe8fc36b4e20fcdf6cc49fe9ddb6e79bcf213f
>     > # Parent  5ea9c5d20ecc1aac2aecdd4c0902b3cd470b04d5
>     > # EXP-Topic descendant
>     > # Available At https://bitbucket.org/octobus/mercurial-devel/
>     > #              hg pull
>     https://bitbucket.org/octobus/mercurial-devel/ -r 6bfe8fc36b4e
>     > revlog: reuse 'descendant' implemention in 'isancestor'
>
>     Queued the series, thanks.
>
>     > We should probably cleanup the API at some point to avoid having
>     so many similar
>     > functions. However, we focus on an efficient implementation for now.
>
>     Yeah.
>
>
> Perhaps we should rename "descendant" to "isdescendant" to match 
> "isancestor" then.

Having `isdescendant` and `isancestor` sharing the same signature and 
behavior except for their arguments types (one accepting revs and the 
other accepting nodes) seems confusing.

We could maybe use the `rev(s)` prefix, giving for example 
`isancestorrev` and `commonancestorheadrevs`. This prefix is already 
used in the code base:

* revlog.parentrevs
* revlog.findcommonmissing vs revlog.findmissingrevs
* revlog.heads vs revlog.headrevs
* revlog.incrementalmissingrevs

>
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - July 3, 2018, 1:21 p.m.
On Tue, Jul 3, 2018 at 3:05 AM Boris FELD <boris.feld@octobus.net> wrote:

> On 02/07/2018 18:37, Martin von Zweigbergk via Mercurial-devel wrote:
>
>
>
> On Mon, Jul 2, 2018 at 5:05 AM Yuya Nishihara <yuya@tcha.org> wrote:
>
>> On Sun, 01 Jul 2018 08:38:42 +0200, Paul Morelle wrote:
>> > # HG changeset patch
>> > # User Boris Feld <boris.feld@octobus.net>
>> > # Date 1529622442 -3600
>> > #      Fri Jun 22 00:07:22 2018 +0100
>> > # Node ID 6bfe8fc36b4e20fcdf6cc49fe9ddb6e79bcf213f
>> > # Parent  5ea9c5d20ecc1aac2aecdd4c0902b3cd470b04d5
>> > # EXP-Topic descendant
>> > # Available At https://bitbucket.org/octobus/mercurial-devel/
>> > #              hg pull https://bitbucket.org/octobus/mercurial-devel/
>> -r 6bfe8fc36b4e
>> > revlog: reuse 'descendant' implemention in 'isancestor'
>>
>> Queued the series, thanks.
>>
>> > We should probably cleanup the API at some point to avoid having so
>> many similar
>> > functions. However, we focus on an efficient implementation for now.
>>
>> Yeah.
>>
>
> Perhaps we should rename "descendant" to "isdescendant" to match
> "isancestor" then.
>
>
> Having `isdescendant` and `isancestor` sharing the same signature and
> behavior except for their arguments types (one accepting revs and the other
> accepting nodes) seems confusing.
>

Of course (but I'm not sure it's much more confusing than having descendant
and isancestor work on different types). I didn't mean to suggest that they
should be different, I had just not checked. I actually had felt like the
new isancestor should have been called isancestorrev or isrevancestor or
revisancestor, but I didn't bother suggesting it.


> We could maybe use the `rev(s)` prefix, giving for example `isancestorrev`
> and `commonancestorheadrevs`. This prefix is already used in the code base:
>
> * revlog.parentrevs
> * revlog.findcommonmissing vs revlog.findmissingrevs
> * revlog.heads vs revlog.headrevs
> * revlog.incrementalmissingrevs
>
>
>
>
> _______________________________________________
> Mercurial-devel mailing listMercurial-devel@mercurial-scm.orghttps://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
>

Patch

diff -r 5ea9c5d20ecc -r 6bfe8fc36b4e mercurial/revlog.py
--- a/mercurial/revlog.py	Fri Jun 22 00:05:20 2018 +0100
+++ b/mercurial/revlog.py	Fri Jun 22 00:07:22 2018 +0100
@@ -1404,7 +1404,8 @@ 
 
         The implementation of this is trivial but the use of
         commonancestorsheads is not."""
-        return a in self.commonancestorsheads(a, b)
+        a, b = self.rev(a), self.rev(b)
+        return self.descendant(a, b)
 
     def ancestor(self, a, b):
         """calculate the "best" common ancestor of nodes a and b"""