( )⚙ D674 filemerge: use arbitraryfilectx for backup files

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D674

filemerge: use arbitraryfilectx for backup files
AbandonedPublic

Authored by phillco on Sep 11 2017, 12:03 AM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Summary

With in-memory merge, backup files might be overlayworkingfilectxs stored
in memory. But they could also be real files if the user's backup directory is
outside the working dir.

Rather than have two code paths everywhere, let's use arbitraryfilectx so they
can be consistent.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

phillco created this revision.Sep 11 2017, 12:03 AM
phillco updated this revision to Diff 1724.Sep 11 2017, 4:05 PM
martinvonz added a subscriber: martinvonz.Sep 11 2017, 8:00 PM
martinvonz added inline comments.
mercurial/context.py
701–705

Feel like checking out the "abc" module and see if that's helpful here?

855

Looks like either _customcmp should be part of abstractfilectx or checked for here (util.safehasattr())

mercurial/filemerge.py
744

The documentation for filecmp.cmp() says

Unless shallow is given and is false, files with identical os.stat() signatures are taken to be equal.

Are we losing out on that optimization with this patch (when not doing in-memory merge). Do you have any sense of how relevant that optimization is?

tests/test-dirstate-race.t
85 ↗(On Diff #1724)

Why did this change?

phillco added a subscriber: sid0.Sep 13 2017, 1:42 AM
phillco added inline comments.
tests/test-dirstate-race.t
85 ↗(On Diff #1724)

It's caused by the addition of this block into abstractfilectx.

if isinstance(fctx, abstractfilectx):
    return self.data() != fctx.data()

When fctx is a workingfilectx, and has been replaced by a directory, calling data() on it raises an IOError instead of returning True like this function used to do.

Interestingly, the test behavior is caused by this error being caught by an existing block inside workingctx._checklookup():

except (IOError, OSError):
        # A file become inaccessible in between? Mark it as deleted,
        # matching dirstate behavior (issue5584).
        # The dirstate has more complex behavior around whether a
        # missing file matches a directory, etc, but we don't need to
        # bother with that: if f has made it to this point, we're sure
        # it's in the dirstate.
        deleted.append(f)

Which seems to somewhat predict the case of files being unavailable and raising raw IOErrors, although not this case specifically.

Also, @sid0's comment around the test case says:

XXX Note that this returns M for files that got replaced by directories. This is
definitely a bug, but the fix for that is hard and the next status run is fine
anyway.

Which is in fact the case for e. So maybe continuing to throw is actually the better behavior here, given that it would also throw if fctx was missing so the idea isn't unprecedented.

On the other hand, just catching the error and returning True is the most conservative path forward, so I might just do that here.

phillco added inline comments.Sep 13 2017, 1:49 AM
mercurial/filemerge.py
744

Yes, and it's a bigger impact than past changes of this nature, since each merged file, and its backup file, will be read again. So I think we need some way to reintroduce this fast-path for two disk-backed files inside cmp.

I'd propose adding something like this to filectx:

def ondisk():
   """Returns True iff this filectx is directly backed by a file in the filesystem and not some other abstraction. If so, callers can run system file functions on it for better performance.
   """

It'd be True only for workingfilectxs and abstractfilectxs.

Then, inside abstractfilectx.cmp(), check if both the caller and other are on-disk and use filecmp in that case.

It's a naive first take though, so improvements are appreciated.

martinvonz added inline comments.Sep 13 2017, 12:02 PM
mercurial/filemerge.py
744

Can we not simply make arbitraryfilectx.cmp() something like

def cmp(self, fctx):
    if isinstance(fctx, arbitraryfilectx):
        return filecmp.cmp(self.path(), fctx.path())
    return self.data() != otherfilectx.data()
phillco added inline comments.Sep 13 2017, 2:05 PM
mercurial/filemerge.py
744

fcd is a workingfilectx, so it'd need to need to be something like:

return filecmp.cmp(self.path(), self._repo.wjoin(fctx.path()))

and arbitraryfilectx doesn't have a _repo because we use it in contrib/simplemerge. Maybe we could make it an optional property and raise in this case if it's missing? contrib/simplemerge doesn't need it.

phillco updated this revision to Diff 1830.Sep 14 2017, 4:13 PM
phillco planned changes to this revision.Sep 15 2017, 3:38 PM

Putting back in my queue -- still have two comments of Martin's to respond to.

phillco updated this revision to Diff 1871.Sep 18 2017, 4:35 PM
phillco updated this revision to Diff 1874.Sep 18 2017, 4:40 PM
durin42 added a subscriber: durin42.Sep 20 2017, 12:05 PM

One extra comment: maybe include some "why" as well as "what" in your commit message. :)

durin42 removed a subscriber: durin42.Sep 20 2017, 12:05 PM
martinvonz added inline comments.Sep 21 2017, 12:45 AM
mercurial/context.py
701–706

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.

phillco updated this revision to Diff 1989.Sep 21 2017, 6:35 PM
martinvonz added inline comments.Sep 22 2017, 2:04 PM
mercurial/filemerge.py
611

Isn't repo.wjoin(fcd.path()) the same thing as _workingpath(repo, fcd) inlined? You call the same thing further down, so why not keep the "a" variable (possibly renamed to something better)?

616

Why care whether it's in the working directory when doing in-memory merge? Why not instead always (when doing in-memory merge) either redirect the backup to memory or put it outside of the working directory? Is it so if a there's a conflict, it will get flushed to the place the user expects?

phillco added a comment.Sep 26 2017, 9:34 AM

One extra comment: maybe include some "why" as well as "what" in your commit message. :)

Sorry I missed this, the comment is very valid. Will send a new version.

mercurial/filemerge.py
611

Yeah, not sure why I reverted to wjoin. I'll switch it back.

616

Is it so if a there's a conflict, it will get flushed to the place the user expects?

Yes, basically.

phillco edited the summary of this revision. (Show Details)Sep 26 2017, 9:38 AM
phillco updated this revision to Diff 2088.Sep 26 2017, 10:06 AM
martinvonz requested changes to this revision.Oct 3 2017, 5:08 PM
martinvonz added inline comments.
mercurial/filemerge.py
617–624

I mentioned on IRC the other day that this *could* be left for another patch and that I didn't expect it to be here after reading the commit message. I didn't insist that had to be done, but Phil liked the idea, so he said he'd do it. I just thought I'd point that out here too to get the status right on the dashboard.

This revision now requires changes to proceed.Oct 3 2017, 5:08 PM
phillco abandoned this revision.Oct 13 2017, 3:48 PM

I've split this into D1056, D1057, D1058, D1059, D1060, so abandoning this version.

@martinvonz

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (22 lines)
Mmercurial/filemerge.py (48 lines)

Diff 1871

mercurial/context.py

# context.py - changeset and file context objects for mercurial # context.py - changeset and file context objects for mercurial
# #
# Copyright 2006, 2007 Matt Mackall <mpm@selenic.com> # Copyright 2006, 2007 Matt Mackall <mpm@selenic.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import errno import errno
import filecmp
import os import os
import re import re
import stat import stat
from .i18n import _ from .i18n import _
from .node import ( from .node import (
addednodeid, addednodeid,
bin, bin,
match.bad(fn, _('no such file in rev %s') % self) match.bad(fn, _('no such file in rev %s') % self)
m = matchmod.badmatch(match, bad) m = matchmod.badmatch(match, bad)
return self._manifest.walk(m) return self._manifest.walk(m)
def matches(self, match): def matches(self, match):
return self.walk(match) return self.walk(match)
class basefilectx(object): class abstractfilectx(object):
def data(self):
raise error.ProgrammingError("Must be implemented by subclasses")
class basefilectx(abstractfilectx):
martinvonzUnsubmitted
Not Done

Feel like checking out the "abc" module and see if that's helpful here?

martinvonz: Feel like checking out the "abc" module and see if that's helpful here?
"""A filecontext object represents the common logic for its children: """A filecontext object represents the common logic for its children:
martinvonzUnsubmitted
Not Done

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.

martinvonz: abstractfilectx doesn't seem referenced anywhere else, so just revert this part? Maybe you…
filectx: read-only access to a filerevision that is already present filectx: read-only access to a filerevision that is already present
in the repo, in the repo,
workingfilectx: a filecontext that represents files from the working workingfilectx: a filecontext that represents files from the working
directory, directory,
memfilectx: a filecontext that represents files in-memory, memfilectx: a filecontext that represents files in-memory,
overlayfilectx: duplicate another filecontext with some fields overridden. overlayfilectx: duplicate another filecontext with some fields overridden.
""" """
@propertycache @propertycache
return False return False
_customcmp = False _customcmp = False
def cmp(self, fctx): def cmp(self, fctx):
"""compare with other file context """compare with other file context
returns True if different than fctx. returns True if different than fctx.
""" """
if fctx._customcmp: if fctx._customcmp:
martinvonzUnsubmitted
Not Done

Looks like either _customcmp should be part of abstractfilectx or checked for here (util.safehasattr())

martinvonz: Looks like either _customcmp should be part of abstractfilectx or checked for here (util.
return fctx.cmp(self) return fctx.cmp(self)
if (fctx._filenode is None if (fctx._filenode is None
and (self._repo._encodefilterpats and (self._repo._encodefilterpats
# if file data starts with '\1\n', empty metadata block is # if file data starts with '\1\n', empty metadata block is
# prepended, which adds 4 bytes to filelog.size(). # prepended, which adds 4 bytes to filelog.size().
or self.size() - 4 == fctx.size()) or self.size() - 4 == fctx.size())
or self.size() == fctx.size()): or self.size() == fctx.size()):
def __init__(self, repo, path, filelog=None, parent=None): def __init__(self, repo, path, filelog=None, parent=None):
super(overlayworkingfilectx, self).__init__(repo, path, filelog, super(overlayworkingfilectx, self).__init__(repo, path, filelog,
parent) parent)
self._repo = repo self._repo = repo
self._parent = parent self._parent = parent
self._path = path self._path = path
def cmp(self, fctx):
return self.data() != fctx.data()
def ctx(self): def ctx(self):
return self._parent return self._parent
def data(self): def data(self):
return self._parent.data(self._path) return self._parent.data(self._path)
def date(self): def date(self):
return self._parent.filedate(self._path) return self._parent.filedate(self._path)
removed.append(f) removed.append(f)
return scmutil.status(modified, added, removed, [], [], [], []) return scmutil.status(modified, added, removed, [], [], [], [])
class arbitraryfilectx(object): class arbitraryfilectx(object):
"""Allows you to use filectx-like functions on a file in an arbitrary """Allows you to use filectx-like functions on a file in an arbitrary
location on disk, possibly not in the working directory. location on disk, possibly not in the working directory.
""" """
def __init__(self, path): def __init__(self, path, repo=None):
# Repo is optional because contrib/simplemerge uses this class.
self._repo = repo
self._path = path self._path = path
def cmp(self, otherfilectx): def cmp(self, fctx):
return self.data() != otherfilectx.data() if isinstance(fctx, workingfilectx) and self._repo:
# Add a fast-path for merge if both sides are disk-backed.
# Note that filecmp uses the opposite return values as cmp.
return not filecmp.cmp(self.path(), self._repo.wjoin(fctx.path()))
return self.data() != fctx.data()
def path(self): def path(self):
return self._path return self._path
def flags(self): def flags(self):
return '' return ''
def data(self): def data(self):

mercurial/filemerge.py

# filemerge.py - file-level merge handling for Mercurial # filemerge.py - file-level merge handling for Mercurial
# #
# Copyright 2006, 2007, 2008 Matt Mackall <mpm@selenic.com> # Copyright 2006, 2007, 2008 Matt Mackall <mpm@selenic.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import filecmp
import os import os
import re import re
import tempfile import tempfile
from .i18n import _ from .i18n import _
from .node import nullid, short from .node import nullid, short
from . import ( from . import (
if '\r\n' in data: # Windows if '\r\n' in data: # Windows
return '\r\n' return '\r\n'
if '\r' in data: # Old Mac if '\r' in data: # Old Mac
return '\r' return '\r'
if '\n' in data: # UNIX if '\n' in data: # UNIX
return '\n' return '\n'
return None # unknown return None # unknown
def _matcheol(file, origfile): def _matcheol(file, back):
"Convert EOL markers in a file to match origfile" "Convert EOL markers in a file to match origfile"
tostyle = _eoltype(util.readfile(origfile)) tostyle = _eoltype(back.data()) # No repo.wread filters?
if tostyle: if tostyle:
data = util.readfile(file) data = util.readfile(file)
style = _eoltype(data) style = _eoltype(data)
if style: if style:
newdata = data.replace(style, tostyle) newdata = data.replace(style, tostyle)
if newdata != data: if newdata != data:
util.writefile(file, newdata) util.writefile(file, newdata)
same directory as ``a.txt``. same directory as ``a.txt``.
This implies permerge. Therefore, files aren't dumped, if premerge This implies permerge. Therefore, files aren't dumped, if premerge
runs successfully. Use :forcedump to forcibly write files out. runs successfully. Use :forcedump to forcibly write files out.
""" """
a = _workingpath(repo, fcd) a = _workingpath(repo, fcd)
fd = fcd.path() fd = fcd.path()
# Run ``flushall()`` to make any missing folders the following wwrite
# calls might be depending on.
from . import context
if isinstance(fcd, context.overlayworkingfilectx):
fcd.ctx().flushall()
util.writefile(a + ".local", fcd.decodeddata()) util.writefile(a + ".local", fcd.decodeddata())
repo.wwrite(fd + ".other", fco.data(), fco.flags()) repo.wwrite(fd + ".other", fco.data(), fco.flags())
repo.wwrite(fd + ".base", fca.data(), fca.flags()) repo.wwrite(fd + ".base", fca.data(), fca.flags())
return False, 1, False return False, 1, False
@internaltool('forcedump', mergeonly) @internaltool('forcedump', mergeonly)
def _forcedump(repo, mynode, orig, fcd, fco, fca, toolconf, files, def _forcedump(repo, mynode, orig, fcd, fco, fca, toolconf, files,
labels=None): labels=None):
'HG_MY_ISLINK': 'l' in fcd.flags(), 'HG_MY_ISLINK': 'l' in fcd.flags(),
'HG_OTHER_ISLINK': 'l' in fco.flags(), 'HG_OTHER_ISLINK': 'l' in fco.flags(),
'HG_BASE_ISLINK': 'l' in fca.flags(), 'HG_BASE_ISLINK': 'l' in fca.flags(),
} }
ui = repo.ui ui = repo.ui
args = _toolstr(ui, tool, "args", '$local $base $other') args = _toolstr(ui, tool, "args", '$local $base $other')
if "$output" in args: if "$output" in args:
out, a = a, back # read input from backup, write to original # read input from backup, write to original
out = a
a = repo.wvfs.join(back.path())
replace = {'local': a, 'base': b, 'other': c, 'output': out} replace = {'local': a, 'base': b, 'other': c, 'output': out}
args = util.interpolate(r'\$', replace, args, args = util.interpolate(r'\$', replace, args,
lambda s: util.shellquote(util.localpath(s))) lambda s: util.shellquote(util.localpath(s)))
cmd = toolpath + ' ' + args cmd = toolpath + ' ' + args
if _toolbool(ui, tool, "gui"): if _toolbool(ui, tool, "gui"):
repo.ui.status(_('running merge tool %s for file %s\n') % repo.ui.status(_('running merge tool %s for file %s\n') %
(tool, fcd.path())) (tool, fcd.path()))
repo.ui.debug('launching merge tool: %s\n' % cmd) repo.ui.debug('launching merge tool: %s\n' % cmd)
return { return {
"l": " [%s]" % labels[0], "l": " [%s]" % labels[0],
"o": " [%s]" % labels[1], "o": " [%s]" % labels[1],
} }
def _restorebackup(fcd, back): def _restorebackup(fcd, back):
# TODO: Add a workingfilectx.write(otherfilectx) path so we can use # TODO: Add a workingfilectx.write(otherfilectx) path so we can use
# util.copy here instead. # util.copy here instead.
fcd.write(util.readfile(back), fcd.flags()) fcd.write(back.data(), fcd.flags())
def _makebackup(repo, ui, fcd, premerge): def _makebackup(repo, ui, fcd, premerge):
"""Makes a backup of the local `fcd` file prior to merging. """Makes and returns a filectx-like object for ``fcd``'s backup file.
In addition to preserving the user's pre-existing modifications to `fcd` In addition to preserving the user's pre-existing modifications to `fcd`
(if any), the backup is used to undo certain premerges, confirm whether a (if any), the backup is used to undo certain premerges, confirm whether a
merge changed anything, and determine what line endings the new file should merge changed anything, and determine what line endings the new file should
have. have.
""" """
if fcd.isabsent(): if fcd.isabsent():
return None return None
from . import context
back = scmutil.origpath(ui, repo, repo.wjoin(fcd.path()))
martinvonzUnsubmitted
Not Done

Isn't repo.wjoin(fcd.path()) the same thing as _workingpath(repo, fcd) inlined? You call the same thing further down, so why not keep the "a" variable (possibly renamed to something better)?

martinvonz: Isn't repo.wjoin(fcd.path()) the same thing as _workingpath(repo, fcd) inlined? You call the…
phillcoAuthorUnsubmitted
Not Done

Yeah, not sure why I reverted to wjoin. I'll switch it back.

phillco: Yeah, not sure why I reverted to wjoin. I'll switch it back.
a = _workingpath(repo, fcd) inworkingdir = (back.startswith(repo.wvfs.base) and not
back = scmutil.origpath(ui, repo, a) back.startswith(repo.vfs.base))
if isinstance(fcd, context.overlayworkingfilectx) and inworkingdir:
martinvonzUnsubmitted
Not Done

Why care whether it's in the working directory when doing in-memory merge? Why not instead always (when doing in-memory merge) either redirect the backup to memory or put it outside of the working directory? Is it so if a there's a conflict, it will get flushed to the place the user expects?

martinvonz: Why care whether it's in the working directory when doing in-memory merge? Why not instead…
phillcoAuthorUnsubmitted
Not Done

Is it so if a there's a conflict, it will get flushed to the place the user expects?

Yes, basically.

phillco: > Is it so if a there's a conflict, it will get flushed to the place the user expects? Yes…
# If the backup file is to be in the working directory, and we're
# merging in-memory, we must redirect the backup to the memory context
# so we don't disturb the working directory.
relpath = back[len(repo.wvfs.base) + 1:]
fcd.ctx()[relpath].write(fcd.data(), fcd.flags())
return fcd.ctx()[relpath]
else:
# Otherwise, write to wherever the user specified the backups should go.
martinvonzUnsubmitted
Not Done

I mentioned on IRC the other day that this *could* be left for another patch and that I didn't expect it to be here after reading the commit message. I didn't insist that had to be done, but Phil liked the idea, so he said he'd do it. I just thought I'd point that out here too to get the status right on the dashboard.

martinvonz: I mentioned on IRC the other day that this *could* be left for another patch and that I didn't…
#
# A arbitraryfilectx is returned, so we can run the same functions on
# the backup context regardless of where it lives.
if premerge: if premerge:
util.copyfile(a, back) util.copyfile(_workingpath(repo, fcd), back)
return back return context.arbitraryfilectx(back, repo=repo)
def _maketempfiles(repo, fco, fca): def _maketempfiles(repo, fco, fca):
"""Writes out `fco` and `fca` as temporary files, so an external merge """Writes out `fco` and `fca` as temporary files, so an external merge
tool may use them. tool may use them.
""" """
def temp(prefix, ctx): def temp(prefix, ctx):
fullbase, ext = os.path.splitext(ctx.path()) fullbase, ext = os.path.splitext(ctx.path())
pre = "%s~%s." % (os.path.basename(fullbase), prefix) pre = "%s~%s." % (os.path.basename(fullbase), prefix)
if r: if r:
if onfailure: if onfailure:
ui.warn(onfailure % fd) ui.warn(onfailure % fd)
return True, r, deleted return True, r, deleted
finally: finally:
if not r and back is not None: if not r and back is not None:
util.unlink(back) back.remove()
def _check(repo, r, ui, tool, fcd, files): def _check(repo, r, ui, tool, fcd, files):
fd = fcd.path() fd = fcd.path()
unused, unused, unused, back = files unused, unused, unused, back = files
if not r and (_toolbool(ui, tool, "checkconflicts") or if not r and (_toolbool(ui, tool, "checkconflicts") or
'conflicts' in _toollist(ui, tool, "check")): 'conflicts' in _toollist(ui, tool, "check")):
if re.search("^(<<<<<<< .*|=======|>>>>>>> .*)$", fcd.data(), if re.search("^(<<<<<<< .*|=======|>>>>>>> .*)$", fcd.data(),
re.MULTILINE): re.MULTILINE):
r = 1 r = 1
checked = False checked = False
if 'prompt' in _toollist(ui, tool, "check"): if 'prompt' in _toollist(ui, tool, "check"):
checked = True checked = True
if ui.promptchoice(_("was merge of '%s' successful (yn)?" if ui.promptchoice(_("was merge of '%s' successful (yn)?"
"$$ &Yes $$ &No") % fd, 1): "$$ &Yes $$ &No") % fd, 1):
r = 1 r = 1
if not r and not checked and (_toolbool(ui, tool, "checkchanged") or if not r and not checked and (_toolbool(ui, tool, "checkchanged") or
'changed' in 'changed' in
_toollist(ui, tool, "check")): _toollist(ui, tool, "check")):
if back is not None and filecmp.cmp(_workingpath(repo, fcd), back): if back is not None and not fcd.cmp(back):
martinvonzUnsubmitted
Not Done

The documentation for filecmp.cmp() says

Unless shallow is given and is false, files with identical os.stat() signatures are taken to be equal.

Are we losing out on that optimization with this patch (when not doing in-memory merge). Do you have any sense of how relevant that optimization is?

martinvonz: The documentation for filecmp.cmp() says Unless shallow is given and is false, files with…
phillcoAuthorUnsubmitted
Not Done

Yes, and it's a bigger impact than past changes of this nature, since each merged file, and its backup file, will be read again. So I think we need some way to reintroduce this fast-path for two disk-backed files inside cmp.

I'd propose adding something like this to filectx:

def ondisk():
   """Returns True iff this filectx is directly backed by a file in the filesystem and not some other abstraction. If so, callers can run system file functions on it for better performance.
   """

It'd be True only for workingfilectxs and abstractfilectxs.

Then, inside abstractfilectx.cmp(), check if both the caller and other are on-disk and use filecmp in that case.

It's a naive first take though, so improvements are appreciated.

phillco: Yes, and it's a bigger impact than past changes of this nature, since each merged file, and its…
martinvonzUnsubmitted
Not Done

Can we not simply make arbitraryfilectx.cmp() something like

def cmp(self, fctx):
    if isinstance(fctx, arbitraryfilectx):
        return filecmp.cmp(self.path(), fctx.path())
    return self.data() != otherfilectx.data()
martinvonz: Can we not simply make arbitraryfilectx.cmp() something like def cmp(self, fctx): if…
phillcoAuthorUnsubmitted
Not Done

fcd is a workingfilectx, so it'd need to need to be something like:

return filecmp.cmp(self.path(), self._repo.wjoin(fctx.path()))

and arbitraryfilectx doesn't have a _repo because we use it in contrib/simplemerge. Maybe we could make it an optional property and raise in this case if it's missing? contrib/simplemerge doesn't need it.

phillco: `fcd` is a `workingfilectx`, so it'd need to need to be something like: ``` return filecmp.
if ui.promptchoice(_(" output file %s appears unchanged\n" if ui.promptchoice(_(" output file %s appears unchanged\n"
"was merge successful (yn)?" "was merge successful (yn)?"
"$$ &Yes $$ &No") % fd, 1): "$$ &Yes $$ &No") % fd, 1):
r = 1 r = 1
if back is not None and _toolbool(ui, tool, "fixeol"): if back is not None and _toolbool(ui, tool, "fixeol"):
_matcheol(_workingpath(repo, fcd), back) _matcheol(_workingpath(repo, fcd), back)