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