D5409: remotefilelog: accepting a None node to cmp

[rdamazio (Rodrigo Damazio Bovendorp)]

rdamazio added a comment.


  In https://phab.mercurial-scm.org/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).

REPOSITORY
  rHG Mercurial

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

To: rdamazio, #hg-reviewers
Cc: yuja, mercurial-devel