Patchwork D7465: filemerge: fix a missing attribute usage

login
register
mail settings
Submitter phabricator
Date Nov. 21, 2019, 3:27 a.m.
Message ID <differential-rev-PHID-DREV-sspupxaqnqx25yvqipwp-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43379/
State Superseded
Headers show

Comments

phabricator - Nov. 21, 2019, 3:27 a.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Flagged by both pytype and VSCode.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7465

AFFECTED FILES
  mercurial/filemerge.py

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 21, 2019, 8:13 a.m.
dlax added inline comments.

INLINE COMMENTS

> filemerge.py:122
>              fctx.isabsent()
> -            and fctx.ctx() == self.ctx()
> +            and fctx.ctx() == self._ctx
>              and fctx.path() == self.path()

What about `fctx`? It could also lack the `ctx()` method if an instance of `absentfilectx`.
I wonder if the intent was not `fctx.changectx() == self.changectx()`.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7465/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7465

To: mharbison72, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Nov. 21, 2019, 12:37 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> dlax wrote in filemerge.py:122
> What about `fctx`? It could also lack the `ctx()` method if an instance of `absentfilectx`.
> I wonder if the intent was not `fctx.changectx() == self.changectx()`.

I’m not sure how this works, but this was enough to make pytype happy.  Does it make any sense to compare an absent file to an absent file?

Maybe the right thing is to add a ctx attribute here?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7465/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7465

To: mharbison72, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Nov. 21, 2019, 12:56 p.m.
dlax added inline comments.

INLINE COMMENTS

> mharbison72 wrote in filemerge.py:122
> I’m not sure how this works, but this was enough to make pytype happy.  Does it make any sense to compare an absent file to an absent file?
> 
> Maybe the right thing is to add a ctx attribute here?

> Does it make any sense to compare an absent file to an absent file?

Not sure, but due to lazy evaluation `fctx.ctx() == self.ctx()` would only be evaluated if `fctx` is an absent file, hence also lacking the `ctx()` method.
I suggested to use `changectx()` because this is already implemented and also part of the basefilectx interface.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7465/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7465

To: mharbison72, #hg-reviewers
Cc: dlax, mercurial-devel

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -119,7 +119,7 @@ 
         """
         return not (
             fctx.isabsent()
-            and fctx.ctx() == self.ctx()
+            and fctx.ctx() == self._ctx
             and fctx.path() == self.path()
         )