D5409: remotefilelog: accepting a None node to cmp
yuja added a comment. I'm dropping this since I found a bug in memfilectx. That's probably why absorb crashed. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5409 To: rdamazio, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: D5409: remotefilelog: accepting a None node to cmp
I'm dropping this since I found a bug in memfilectx. That's probably why absorb crashed. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5409: remotefilelog: accepting a None node to cmp
yuja added a comment. > 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5409 To: rdamazio, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: D5409: remotefilelog: accepting a None node to cmp
> 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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5409: remotefilelog: accepting a None node to cmp
rdamazio added a comment. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5409 To: rdamazio, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5409: remotefilelog: accepting a None node to cmp
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5409: remotefilelog: accepting a None node to cmp
yuja added a comment. > 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? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5409 To: rdamazio, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: D5409: remotefilelog: accepting a None node to cmp
> 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? ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5409: remotefilelog: accepting a None node to cmp
This revision was automatically updated to reflect the committed changes. Closed by commit rHG4e17679c336b: remotefilelog: accepting a None node to cmp (authored by rdamazio, committed by ). REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D5409?vs=12820=12821 REVISION DETAIL https://phab.mercurial-scm.org/D5409 AFFECTED FILES hgext/remotefilelog/remotefilelog.py CHANGE DETAILS diff --git a/hgext/remotefilelog/remotefilelog.py b/hgext/remotefilelog/remotefilelog.py --- a/hgext/remotefilelog/remotefilelog.py +++ b/hgext/remotefilelog/remotefilelog.py @@ -187,7 +187,7 @@ returns True if text is different than what is stored. """ -if node == nullid: +if not node or node == nullid: return True nodetext = self.read(node) To: rdamazio, #hg-reviewers Cc: mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5409: remotefilelog: accepting a None node to cmp
rdamazio created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY In context.py, basefilectx.cmp explicitly calls it with None, so it has to be supported. Specifically, this breaks "hg absorb -i" currently. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5409 AFFECTED FILES hgext/remotefilelog/remotefilelog.py CHANGE DETAILS diff --git a/hgext/remotefilelog/remotefilelog.py b/hgext/remotefilelog/remotefilelog.py --- a/hgext/remotefilelog/remotefilelog.py +++ b/hgext/remotefilelog/remotefilelog.py @@ -187,7 +187,7 @@ returns True if text is different than what is stored. """ -if node == nullid: +if not node or node == nullid: return True nodetext = self.read(node) To: rdamazio, #hg-reviewers Cc: mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
