Patchwork filectx: replace use of _filerev with _filenode

login
register
mail settings
Submitter Durham Goode
Date Feb. 8, 2016, 10:48 p.m.
Message ID <b8f90918f80e13e468d0.1454971700@dev8486.prn1.facebook.com>
Download mbox | patch
Permalink /patch/13056/
State Accepted
Headers show

Comments

Durham Goode - Feb. 8, 2016, 10:48 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1454969831 28800
#      Mon Feb 08 14:17:11 2016 -0800
# Node ID b8f90918f80e13e468d0fa22ca2f678896223cbd
# Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
filectx: replace use of _filerev with _filenode

_filerev depends on the filelog implementation using revlogs and linkrevs.
Alternative implementations, like remotefilelog, do not have rev numbers, so
this call fails. Replacing it with _filenode means it doesn't rely on rev
numbers, and doesn't cost anything extra, since _filerev is using _filenode
under the hood anyway.
Sean Farley - Feb. 8, 2016, 10:55 p.m.
Durham Goode <durham@fb.com> writes:

> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1454969831 28800
> #      Mon Feb 08 14:17:11 2016 -0800
> # Node ID b8f90918f80e13e468d0fa22ca2f678896223cbd
> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
> filectx: replace use of _filerev with _filenode
>
> _filerev depends on the filelog implementation using revlogs and linkrevs.
> Alternative implementations, like remotefilelog, do not have rev numbers, so
> this call fails. Replacing it with _filenode means it doesn't rely on rev
> numbers, and doesn't cost anything extra, since _filerev is using _filenode
> under the hood anyway.

Seems fine to me.
Siddharth Agarwal - Feb. 8, 2016, 10:55 p.m.
On 2/8/16 14:48, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1454969831 28800
> #      Mon Feb 08 14:17:11 2016 -0800
> # Node ID b8f90918f80e13e468d0fa22ca2f678896223cbd
> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
> filectx: replace use of _filerev with _filenode
>
> _filerev depends on the filelog implementation using revlogs and linkrevs.
> Alternative implementations, like remotefilelog, do not have rev numbers, so
> this call fails. Replacing it with _filenode means it doesn't rely on rev
> numbers, and doesn't cost anything extra, since _filerev is using _filenode
> under the hood anyway.

Someone else can easily reintroduce a dependency on _filerev without 
realizing -- as a followup, can we deprecate it somehow?


>
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -789,7 +789,7 @@ class basefilectx(object):
>           if fctx._customcmp:
>               return fctx.cmp(self)
>   
> -        if (fctx._filerev is None
> +        if (fctx._filenode is None
>               and (self._repo._encodefilterpats
>                    # if file data starts with '\1\n', empty metadata block is
>                    # prepended, which adds 4 bytes to filelog.size().
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - Feb. 8, 2016, 10:58 p.m.
On Mon, Feb 8, 2016 at 2:55 PM, Siddharth Agarwal <sid@less-broken.com> wrote:
> On 2/8/16 14:48, Durham Goode wrote:
>>
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1454969831 28800
>> #      Mon Feb 08 14:17:11 2016 -0800
>> # Node ID b8f90918f80e13e468d0fa22ca2f678896223cbd
>> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
>> filectx: replace use of _filerev with _filenode
>>
>> _filerev depends on the filelog implementation using revlogs and linkrevs.
>> Alternative implementations, like remotefilelog, do not have rev numbers,
>> so
>> this call fails. Replacing it with _filenode means it doesn't rely on rev
>> numbers, and doesn't cost anything extra, since _filerev is using
>> _filenode
>> under the hood anyway.
>
>
> Someone else can easily reintroduce a dependency on _filerev without
> realizing -- as a followup, can we deprecate it somehow?

Can we simply delete it?
Martin von Zweigbergk - Feb. 8, 2016, 11 p.m.
On Mon, Feb 8, 2016 at 2:58 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Mon, Feb 8, 2016 at 2:55 PM, Siddharth Agarwal <sid@less-broken.com> wrote:
>> On 2/8/16 14:48, Durham Goode wrote:
>>>
>>> # HG changeset patch
>>> # User Durham Goode <durham@fb.com>
>>> # Date 1454969831 28800
>>> #      Mon Feb 08 14:17:11 2016 -0800
>>> # Node ID b8f90918f80e13e468d0fa22ca2f678896223cbd
>>> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
>>> filectx: replace use of _filerev with _filenode
>>>
>>> _filerev depends on the filelog implementation using revlogs and linkrevs.
>>> Alternative implementations, like remotefilelog, do not have rev numbers,
>>> so
>>> this call fails. Replacing it with _filenode means it doesn't rely on rev
>>> numbers, and doesn't cost anything extra, since _filerev is using
>>> _filenode
>>> under the hood anyway.
>>
>>
>> Someone else can easily reintroduce a dependency on _filerev without
>> realizing -- as a followup, can we deprecate it somehow?
>
> Can we simply delete it?

But it still seems used elsewhere. Maybe that's why you suggested
deprecating it?
Siddharth Agarwal - Feb. 8, 2016, 11:01 p.m.
On 2/8/16 15:00, Martin von Zweigbergk wrote:
> On Mon, Feb 8, 2016 at 2:58 PM, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>> On Mon, Feb 8, 2016 at 2:55 PM, Siddharth Agarwal <sid@less-broken.com> wrote:
>>> On 2/8/16 14:48, Durham Goode wrote:
>>>> # HG changeset patch
>>>> # User Durham Goode <durham@fb.com>
>>>> # Date 1454969831 28800
>>>> #      Mon Feb 08 14:17:11 2016 -0800
>>>> # Node ID b8f90918f80e13e468d0fa22ca2f678896223cbd
>>>> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
>>>> filectx: replace use of _filerev with _filenode
>>>>
>>>> _filerev depends on the filelog implementation using revlogs and linkrevs.
>>>> Alternative implementations, like remotefilelog, do not have rev numbers,
>>>> so
>>>> this call fails. Replacing it with _filenode means it doesn't rely on rev
>>>> numbers, and doesn't cost anything extra, since _filerev is using
>>>> _filenode
>>>> under the hood anyway.
>>>
>>> Someone else can easily reintroduce a dependency on _filerev without
>>> realizing -- as a followup, can we deprecate it somehow?
>> Can we simply delete it?
> But it still seems used elsewhere. Maybe that's why you suggested
> deprecating it?

Yes :)
Durham Goode - Feb. 8, 2016, 11:01 p.m.
On 2/8/16, 2:58 PM, "Martin von Zweigbergk" <martinvonz@google.com> wrote:

>On Mon, Feb 8, 2016 at 2:55 PM, Siddharth Agarwal <sid@less-broken.com> wrote:

>> On 2/8/16 14:48, Durham Goode wrote:

>>>

>>> # HG changeset patch

>>> # User Durham Goode <durham@fb.com>

>>> # Date 1454969831 28800

>>> #      Mon Feb 08 14:17:11 2016 -0800

>>> # Node ID b8f90918f80e13e468d0fa22ca2f678896223cbd

>>> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90

>>> filectx: replace use of _filerev with _filenode

>>>

>>> _filerev depends on the filelog implementation using revlogs and linkrevs.

>>> Alternative implementations, like remotefilelog, do not have rev numbers,

>>> so

>>> this call fails. Replacing it with _filenode means it doesn't rely on rev

>>> numbers, and doesn't cost anything extra, since _filerev is using

>>> _filenode

>>> under the hood anyway.

>>

>>

>> Someone else can easily reintroduce a dependency on _filerev without

>> realizing -- as a followup, can we deprecate it somehow?

>

>Can we simply delete it?


Probably.  From a quick grepping, it only looks used in hgweb.  But it seems kind of anti-mercurialy to hide one of the core pieces of revlogs (the revision number).  I'd rather come back to this area in the future when we start to get rid of rev numbers as a whole and do a real fix then.
Pierre-Yves David - Feb. 13, 2016, 4:40 p.m.
On 02/08/2016 10:48 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1454969831 28800
> #      Mon Feb 08 14:17:11 2016 -0800
> # Node ID b8f90918f80e13e468d0fa22ca2f678896223cbd
> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
> filectx: replace use of _filerev with _filenode

Pushed to the clowncopter. Thanks

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -789,7 +789,7 @@  class basefilectx(object):
         if fctx._customcmp:
             return fctx.cmp(self)
 
-        if (fctx._filerev is None
+        if (fctx._filenode is None
             and (self._repo._encodefilterpats
                  # if file data starts with '\1\n', empty metadata block is
                  # prepended, which adds 4 bytes to filelog.size().