This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7465

filemerge: fix a missing attribute usage
ClosedPublic

Authored by mharbison72 on Nov 20 2019, 10:27 PM.

Details

Reviewers
dlax
Group Reviewers
hg-reviewers
Commits
rHGd5ce99a6db52: filemerge: fix a missing attribute usage
Summary

Flagged by both pytype and VSCode.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mharbison72 created this revision.Nov 20 2019, 10:27 PM
dlax added a subscriber: dlax.Nov 21 2019, 3:13 AM
dlax added inline comments.
mercurial/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().

mharbison72 added inline comments.Nov 21 2019, 7:37 AM
mercurial/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?

dlax added inline comments.Nov 21 2019, 7:56 AM
mercurial/filemerge.py
122

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.

mharbison72 updated this revision to Diff 18415.Dec 1 2019, 4:58 PM
dlax accepted this revision.Dec 2 2019, 3:23 AM
mharbison72 added a commit: rHGd5ce99a6db52: filemerge: fix a missing attribute usage.Dec 11 2019, 1:30 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGd5ce99a6db52: filemerge: fix a missing attribute usage (authored by mharbison72). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/filemerge.py (2 lines)

Diff 18255

mercurial/filemerge.py

def cmp(self, fctx): def cmp(self, fctx):
"""compare with other file context """compare with other file context
returns True if different from fctx. returns True if different from fctx.
""" """
return not ( return not (
fctx.isabsent() fctx.isabsent()
and fctx.ctx() == self.ctx() and fctx.ctx() == self._ctx
dlaxUnsubmitted
Not Done

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

dlax: What about `fctx`? It could also lack the `ctx()` method if an instance of `absentfilectx`. I…
mharbison72AuthorUnsubmitted
Not Done

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?

mharbison72: I’m not sure how this works, but this was enough to make pytype happy. Does it make any sense…
dlaxUnsubmitted
Not Done

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.

dlax: > Does it make any sense to compare an absent file to an absent file? Not sure, but due to…
and fctx.path() == self.path() and fctx.path() == self.path()
) )
def flags(self): def flags(self):
return b'' return b''
def changectx(self): def changectx(self):
return self._ctx return self._ctx