Patchwork [STABLE] keyword: use fctx.filenode() instead of internal var (issue5364)

login
register
mail settings
Submitter Christian Ebert
Date Oct. 17, 2016, 11:59 a.m.
Message ID <20161017115910.GA11021@krille.blacktrash.org>
Download mbox | patch
Permalink /patch/17152/
State Not Applicable
Headers show

Comments

Christian Ebert - Oct. 17, 2016, 11:59 a.m.
* Yuya Nishihara on Friday, September 09, 2016 at 21:50:31 +0900
> On Thu, 08 Sep 2016 17:12:34 +0100, Christian Ebert wrote:
>> # HG changeset patch
>> # User Christian Ebert <blacktrash@gmx.net>
>> # Date 1473350741 -3600
>> #      Thu Sep 08 17:05:41 2016 +0100
>> # Branch stable
>> # Node ID d4f2b7b5b2e382dcc34409ddcb89b9280262c2d0
>> # Parent  e7766022a61a66a7c4218526b647f96bd442a4ce
>> keyword: use fctx.filenode() instead of internal var (issue5364)
>> 
>> I caved in to the proposal to use internal _filenode variable:
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2010-October/025237.html
> 
> Ah, I got the point why internal _filenode was preferred. I think Nicolas
> was right. Since kwfilectx_cmp() heavily depends on the internal of the filectx,
> using _filenode would catch more bugs.

As mentioned in the bug tracker, the current version uses
_filenode since 4a65c9c6cd3f, and I actually cannot repro the
issue with it.

>> --- a/hgext/keyword.py
>> +++ b/hgext/keyword.py
>> @@ -739,7 +739,7 @@ def reposetup(ui, repo):
>>     def kwfilectx_cmp(orig, self, fctx):
> 
> Maybe we'll have to handle _customcmp instead?
> 
> https://selenic.com/repo/hg/log?rev=bd19561b98d9%3A%3A7b038ec6c5fd

To be honest, I can't wrap my head around that. But do you simply mean
something like:
Yuya Nishihara - Oct. 17, 2016, 1:56 p.m.
On Mon, 17 Oct 2016 13:59:10 +0200, Christian Ebert wrote:
> * Yuya Nishihara on Friday, September 09, 2016 at 21:50:31 +0900
> > On Thu, 08 Sep 2016 17:12:34 +0100, Christian Ebert wrote:
> >> @@ -739,7 +739,7 @@ def reposetup(ui, repo):
> >>     def kwfilectx_cmp(orig, self, fctx):
> > 
> > Maybe we'll have to handle _customcmp instead?
> > 
> > https://selenic.com/repo/hg/log?rev=bd19561b98d9%3A%3A7b038ec6c5fd
> 
> To be honest, I can't wrap my head around that. But do you simply mean
> something like:
> 
> diff --git a/hgext/keyword.py b/hgext/keyword.py
> --- a/hgext/keyword.py
> +++ b/hgext/keyword.py
> @@ -737,6 +737,8 @@ def reposetup(ui, repo):
>              return ret
> 
>      def kwfilectx_cmp(orig, self, fctx):
> +        if fctx._customcmp:
> +            return fctx.comp(self)

Yeah, something like that. Per the backtrace in issue5364, fctx was an
absentfilectx. In which case, basefilectx.cmp() is delegated to
absentfilectx.cmp(). It makes sense to do the same here.
Christian Ebert - Oct. 17, 2016, 3:10 p.m.
* Yuya Nishihara on Monday, October 17, 2016 at 22:56:02 +0900
> On Mon, 17 Oct 2016 13:59:10 +0200, Christian Ebert wrote:
>> * Yuya Nishihara on Friday, September 09, 2016 at 21:50:31 +0900
>>> Maybe we'll have to handle _customcmp instead?
>>> 
>>> https://selenic.com/repo/hg/log?rev=bd19561b98d9%3A%3A7b038ec6c5fd
>> 
>> To be honest, I can't wrap my head around that. But do you simply mean
>> something like:
>> 
>> diff --git a/hgext/keyword.py b/hgext/keyword.py
>> --- a/hgext/keyword.py
>> +++ b/hgext/keyword.py
>> @@ -737,6 +737,8 @@ def reposetup(ui, repo):
>>             return ret
>> 
>>     def kwfilectx_cmp(orig, self, fctx):
>> +        if fctx._customcmp:
>> +            return fctx.comp(self)
> 
> Yeah, something like that. Per the backtrace in issue5364, fctx was an
> absentfilectx. In which case, basefilectx.cmp() is delegated to
> absentfilectx.cmp(). It makes sense to do the same here.

OK, I'll submit a patch. Feels a bit weird because the extension
has no way of knowing what a specific _customcmp does. Should be
OK for absentfilectx.cmp(), but there may be other _customcmp's
... I guess, that's a way to find out.

Patch

diff --git a/hgext/keyword.py b/hgext/keyword.py
--- a/hgext/keyword.py
+++ b/hgext/keyword.py
@@ -737,6 +737,8 @@  def reposetup(ui, repo):
             return ret

     def kwfilectx_cmp(orig, self, fctx):
+        if fctx._customcmp:
+            return fctx.comp(self)
         # keyword affects data size, comparing wdir and filelog size does
         # not make sense
         if (fctx.filenode() is None and