This is an archive of the discontinued Mercurial Phabricator instance.

remotefilelog: accepting a None node to cmp
ClosedPublic

Authored by rdamazio on Dec 10 2018, 8:51 PM.

Details

Summary

In context.py, basefilectx.cmp explicitly calls it with None, so it has to be
supported. Specifically, this breaks "hg absorb -i" currently.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rdamazio created this revision.Dec 10 2018, 8:51 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Dec 11 2018, 7:42 AM
In context.py, basefilectx.cmp explicitly calls it with None, so it has to be
supported. Specifically, this breaks "hg absorb -i" currently.

IIUC, self._filenode should never be None if the given fctx._filenode
is None. If None were passed down to the filelog layer, exception would
be raised.

returns True if text is different than what is stored.
"""
  • if node == nullid:

+ if not node or node == nullid:

return True

Are we sure that the working-directory data is different from the given text?

In D5409#80203, @yuja wrote:
In context.py, basefilectx.cmp explicitly calls it with None, so it has to be
supported. Specifically, this breaks "hg absorb -i" currently.

IIUC, self._filenode should never be None if the given fctx._filenode
is None. If None were passed down to the filelog layer, exception would
be raised.

returns True if text is different than what is stored.
"""
  • if node == nullid:

+ if not node or node == nullid:

return True

Are we sure that the working-directory data is different from the given text?

TLDR: I don't know, to be honest, I don't understand RFL deeply enough to understand all the edge cases.

About it being None - notice the block that calls this:

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().
         or self.size() - 4 == fctx.size())
    or self.size() == fctx.size()):
    return self._filelog.cmp(self._filenode, fctx.data())

which is essentially:

if f == None:
  return filelog.cmp(f)

so I assumed it must be the intention to actually pass None, and with so much handling, that None was an ok value?

Returning True seemed like the safe approach, rather than claiming the file was unmodified, but I'm not confident that it covers 100% of the cases.

If someone has a better understanding of all the remotefilelog overriding/interaction here, I'm happy to propose or look at a different patch, but this one does fix the immediate issue (absorb -i crashing).

Oops, my oversight - what's passed to cmp is not the same that's compared to None in that if :) I have even less confidence on this now.

yuja added a comment.Dec 13 2018, 6:40 AM
Oops, my oversight - what's passed to cmp is not the same that's compared to None in that if :) I have even less confidence on this now.

And workingfilectx.cmp(self, fctx) flips self and fctx as it knows
self._filenode is None, and the other's wouldn't be None. I don't like it,
but that's how fctx.cmp() is working.

yuja added a comment.Dec 16 2018, 3:22 AM

I'm dropping this since I found a bug in memfilectx. That's probably why
absorb crashed.