D674: filemerge: use arbitraryfilectx for backup files

[martinvonz (Martin von Zweigbergk)]

martinvonz added inline comments.

INLINE COMMENTS

> context.py:701-705
> +class abstractfilectx(object):
> +    def data(self):
> +        raise NotImplementedError
> +
> +class basefilectx(abstractfilectx):

abstractfilectx doesn't seem referenced anywhere else, so just revert this part? Maybe you meant to make arbitraryfilectx extend it? That would make sense. If we do, I feel like we should implement the smart cmp() in this abstract base class to avoid the ugly asymmetry in the implementation otherwise (a.cmp(b) might be faster or slower than b.cmp(a) and one might perhaps even crash?).

Perhaps we should even make it a top-level function so it will be easier to override by extensions that want to add their own subclasses? Something like:

  def filectxcmp(fctx1, fctx2):
      ...
  class abstractfilectx(object):
      def cmp(self, otherfctx): # don't override in subclasses, wrap filefctxcmp() instead
          return filectxcmp(fctx1, fctx2)

Maybe there's precedence for this kind of thing elsewhere in Mercurial? Surely at least elsewhere in Python.

Or maybe I'm just overthinking this and we're pretty sure all call sites will pass it as arbitraryfilectx.cmp(filectx) and not the other way around, so it won't be a problem in practice. I'm not even sure I got that right (and I don't know where overlayfilectx fits in), which seems like a sign that it's best to have a single cmp() method.

REPOSITORY
  rHG Mercurial

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

To: phillco, #hg-reviewers
Cc: sid0, martinvonz, mercurial-devel