D674: filemerge: use arbitraryfilectx for backup files
phillco abandoned this revision. phillco added a comment. I've split this into https://phab.mercurial-scm.org/D1056, https://phab.mercurial-scm.org/D1057, https://phab.mercurial-scm.org/D1058, https://phab.mercurial-scm.org/D1059, https://phab.mercurial-scm.org/D1060, so abandoning this version. @martinvonz REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D674 To: phillco, #hg-reviewers, martinvonz Cc: sid0, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
martinvonz requested changes to this revision. martinvonz added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > filemerge.py:617-624 > +if isinstance(fcd, context.overlayworkingfilectx) and inworkingdir: > +# 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:] > +wctx[relpath].write(fcd.data(), fcd.flags()) > +return wctx[relpath] 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D674 To: phillco, #hg-reviewers, martinvonz Cc: sid0, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
phillco updated this revision to Diff 2088.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D674?vs=1989=2088
REVISION DETAIL
https://phab.mercurial-scm.org/D674
AFFECTED FILES
mercurial/context.py
mercurial/filemerge.py
CHANGE DETAILS
diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -7,7 +7,6 @@
from __future__ import absolute_import
-import filecmp
import os
import re
import tempfile
@@ -226,9 +225,9 @@
return '\n'
return None # unknown
-def _matcheol(file, origfile):
+def _matcheol(file, back):
"Convert EOL markers in a file to match origfile"
-tostyle = _eoltype(util.readfile(origfile))
+tostyle = _eoltype(back.data()) # No repo.wread filters?
if tostyle:
data = util.readfile(file)
style = _eoltype(data)
@@ -468,6 +467,12 @@
a = _workingpath(repo, fcd)
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())
repo.wwrite(fd + ".other", fco.data(), fco.flags())
repo.wwrite(fd + ".base", fca.data(), fca.flags())
@@ -505,7 +510,9 @@
args = _toolstr(ui, tool, "args", '$local $base $other')
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}
args = util.interpolate(r'\$', replace, args,
lambda s: util.shellquote(util.localpath(s)))
@@ -588,24 +595,40 @@
def _restorebackup(fcd, back):
# TODO: Add a workingfilectx.write(otherfilectx) path so we can use
# util.copy here instead.
-fcd.write(util.readfile(back), fcd.flags())
+fcd.write(back.data(), fcd.flags())
-def _makebackup(repo, ui, fcd, premerge):
-"""Makes a backup of the local `fcd` file prior to merging.
+def _makebackup(repo, ui, wctx, fcd, premerge):
+"""Makes and returns a filectx-like object for ``fcd``'s backup file.
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
merge changed anything, and determine what line endings the new file should
have.
"""
if fcd.isabsent():
return None
-
+from . import context
a = _workingpath(repo, fcd)
back = scmutil.origpath(ui, repo, a)
-if premerge:
-util.copyfile(a, back)
-return back
+
+inworkingdir = (back.startswith(repo.wvfs.base) and not
+back.startswith(repo.vfs.base))
+
+if isinstance(fcd, context.overlayworkingfilectx) and inworkingdir:
+# 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:]
+wctx[relpath].write(fcd.data(), fcd.flags())
+return wctx[relpath]
+else:
+# Otherwise, write to wherever the user specified the backups should
go.
+#
+# A arbitraryfilectx is returned, so we can run the same functions on
+# the backup context regardless of where it lives.
+if premerge:
+util.copyfile(a, back)
+return context.arbitraryfilectx(back, repo=repo)
def _maketempfiles(repo, fco, fca):
"""Writes out `fco` and `fca` as temporary files, so an external merge
@@ -691,7 +714,7 @@
ui.warn(onfailure % fd)
return True, 1, False
-back = _makebackup(repo, ui, fcd, premerge)
+back = _makebackup(repo, ui, wctx, fcd, premerge)
files = (None, None, None, back)
r = 1
try:
@@ -719,7 +742,7 @@
return True, r, deleted
finally:
if not r and back is not None:
-util.unlink(back)
+back.remove()
def _check(repo, r, ui, tool, fcd, files):
fd = fcd.path()
@@ -741,7 +764,7 @@
if not r and not checked and (_toolbool(ui, tool, "checkchanged") or
'changed' in
_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):
if ui.promptchoice(_(" output file %s appears unchanged\n"
"was merge successful (yn)?"
"$$ $$ ") % fd, 1):
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -8,6 +8,7 @@
from
D674: filemerge: use arbitraryfilectx for backup files
phillco added a comment. > 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. INLINE COMMENTS > martinvonz wrote in 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)? Yeah, not sure why I reverted to wjoin. I'll switch it back. > martinvonz wrote in filemerge.py: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? > Is it so if a there's a conflict, it will get flushed to the place the user > expects? Yes, basically. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D674 To: phillco, #hg-reviewers Cc: sid0, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
martinvonz added inline comments. INLINE COMMENTS > filemerge.py:611 > +from . import context > +back = scmutil.origpath(ui, repo, repo.wjoin(fcd.path())) > 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)? > filemerge.py:616 > + > +if isinstance(fcd, context.overlayworkingfilectx) and inworkingdir: > +# If the backup file is to be in the working directory, and we're 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? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D674 To: phillco, #hg-reviewers Cc: sid0, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
durin42 added a comment. One extra comment: maybe include some "why" as well as "what" in your commit message. :) REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D674 To: phillco, #hg-reviewers Cc: durin42, sid0, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
phillco updated this revision to Diff 1874.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D674?vs=1871=1874
REVISION DETAIL
https://phab.mercurial-scm.org/D674
AFFECTED FILES
mercurial/context.py
mercurial/filemerge.py
CHANGE DETAILS
diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -7,7 +7,6 @@
from __future__ import absolute_import
-import filecmp
import os
import re
import tempfile
@@ -226,9 +225,9 @@
return '\n'
return None # unknown
-def _matcheol(file, origfile):
+def _matcheol(file, back):
"Convert EOL markers in a file to match origfile"
-tostyle = _eoltype(util.readfile(origfile))
+tostyle = _eoltype(back.data()) # No repo.wread filters?
if tostyle:
data = util.readfile(file)
style = _eoltype(data)
@@ -468,6 +467,12 @@
a = _workingpath(repo, fcd)
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())
repo.wwrite(fd + ".other", fco.data(), fco.flags())
repo.wwrite(fd + ".base", fca.data(), fca.flags())
@@ -505,7 +510,9 @@
args = _toolstr(ui, tool, "args", '$local $base $other')
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}
args = util.interpolate(r'\$', replace, args,
lambda s: util.shellquote(util.localpath(s)))
@@ -588,24 +595,39 @@
def _restorebackup(fcd, back):
# TODO: Add a workingfilectx.write(otherfilectx) path so we can use
# util.copy here instead.
-fcd.write(util.readfile(back), fcd.flags())
+fcd.write(back.data(), fcd.flags())
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`
(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
have.
"""
if fcd.isabsent():
return None
+from . import context
+back = scmutil.origpath(ui, repo, repo.wjoin(fcd.path()))
-a = _workingpath(repo, fcd)
-back = scmutil.origpath(ui, repo, a)
-if premerge:
-util.copyfile(a, back)
-return back
+inworkingdir = (back.startswith(repo.wvfs.base) and not
+back.startswith(repo.vfs.base))
+
+if isinstance(fcd, context.overlayworkingfilectx) and inworkingdir:
+# 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.
+#
+# A arbitraryfilectx is returned, so we can run the same functions on
+# the backup context regardless of where it lives.
+if premerge:
+util.copyfile(_workingpath(repo, fcd), back)
+return context.arbitraryfilectx(back, repo=repo)
def _maketempfiles(repo, fco, fca):
"""Writes out `fco` and `fca` as temporary files, so an external merge
@@ -719,7 +741,7 @@
return True, r, deleted
finally:
if not r and back is not None:
-util.unlink(back)
+back.remove()
def _check(repo, r, ui, tool, fcd, files):
fd = fcd.path()
@@ -741,7 +763,7 @@
if not r and not checked and (_toolbool(ui, tool, "checkchanged") or
'changed' in
_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):
if ui.promptchoice(_(" output file %s appears unchanged\n"
"was merge successful (yn)?"
"$$ $$ ") % fd, 1):
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -8,6 +8,7 @@
from __future__ import absolute_import
import errno
+import filecmp
import os
import re
import stat
@@ -697,7 +698,11 @@
def matches(self, match):
return self.walk(match)
-class basefilectx(object):
D674: filemerge: use arbitraryfilectx for backup files
phillco updated this revision to Diff 1871.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D674?vs=1830=1871
REVISION DETAIL
https://phab.mercurial-scm.org/D674
AFFECTED FILES
mercurial/context.py
mercurial/filemerge.py
CHANGE DETAILS
diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -7,7 +7,6 @@
from __future__ import absolute_import
-import filecmp
import os
import re
import tempfile
@@ -226,9 +225,9 @@
return '\n'
return None # unknown
-def _matcheol(file, origfile):
+def _matcheol(file, back):
"Convert EOL markers in a file to match origfile"
-tostyle = _eoltype(util.readfile(origfile))
+tostyle = _eoltype(back.data()) # No repo.wread filters?
if tostyle:
data = util.readfile(file)
style = _eoltype(data)
@@ -468,6 +467,12 @@
a = _workingpath(repo, fcd)
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())
repo.wwrite(fd + ".other", fco.data(), fco.flags())
repo.wwrite(fd + ".base", fca.data(), fca.flags())
@@ -505,7 +510,9 @@
args = _toolstr(ui, tool, "args", '$local $base $other')
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}
args = util.interpolate(r'\$', replace, args,
lambda s: util.shellquote(util.localpath(s)))
@@ -588,24 +595,39 @@
def _restorebackup(fcd, back):
# TODO: Add a workingfilectx.write(otherfilectx) path so we can use
# util.copy here instead.
-fcd.write(util.readfile(back), fcd.flags())
+fcd.write(back.data(), fcd.flags())
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`
(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
have.
"""
if fcd.isabsent():
return None
+from . import context
+back = scmutil.origpath(ui, repo, repo.wjoin(fcd.path()))
-a = _workingpath(repo, fcd)
-back = scmutil.origpath(ui, repo, a)
-if premerge:
-util.copyfile(a, back)
-return back
+inworkingdir = (back.startswith(repo.wvfs.base) and not
+back.startswith(repo.vfs.base))
+
+if isinstance(fcd, context.overlayworkingfilectx) and inworkingdir:
+# 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.
+#
+# A arbitraryfilectx is returned, so we can run the same functions on
+# the backup context regardless of where it lives.
+if premerge:
+util.copyfile(_workingpath(repo, fcd), back)
+return context.arbitraryfilectx(back, repo=repo)
def _maketempfiles(repo, fco, fca):
"""Writes out `fco` and `fca` as temporary files, so an external merge
@@ -719,7 +741,7 @@
return True, r, deleted
finally:
if not r and back is not None:
-util.unlink(back)
+back.remove()
def _check(repo, r, ui, tool, fcd, files):
fd = fcd.path()
@@ -741,7 +763,7 @@
if not r and not checked and (_toolbool(ui, tool, "checkchanged") or
'changed' in
_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):
if ui.promptchoice(_(" output file %s appears unchanged\n"
"was merge successful (yn)?"
"$$ $$ ") % fd, 1):
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -8,6 +8,7 @@
from __future__ import absolute_import
import errno
+import filecmp
import os
import re
import stat
@@ -697,7 +698,11 @@
def matches(self, match):
return self.walk(match)
-class basefilectx(object):
D674: filemerge: use arbitraryfilectx for backup files
phillco planned changes to this revision. phillco added a comment. Putting back in my queue -- still have two comments of Martin's to respond to. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D674 To: phillco, #hg-reviewers Cc: sid0, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
phillco updated this revision to Diff 1830.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D674?vs=1724=1830
REVISION DETAIL
https://phab.mercurial-scm.org/D674
AFFECTED FILES
mercurial/context.py
mercurial/filemerge.py
CHANGE DETAILS
diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -7,7 +7,6 @@
from __future__ import absolute_import
-import filecmp
import os
import re
import tempfile
@@ -226,9 +225,9 @@
return '\n'
return None # unknown
-def _matcheol(file, origfile):
+def _matcheol(file, back):
"Convert EOL markers in a file to match origfile"
-tostyle = _eoltype(util.readfile(origfile))
+tostyle = _eoltype(back.data()) # No repo.wread filters?
if tostyle:
data = util.readfile(file)
style = _eoltype(data)
@@ -468,6 +467,12 @@
a = _workingpath(repo, fcd)
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())
repo.wwrite(fd + ".other", fco.data(), fco.flags())
repo.wwrite(fd + ".base", fca.data(), fca.flags())
@@ -505,7 +510,9 @@
args = _toolstr(ui, tool, "args", '$local $base $other')
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}
args = util.interpolate(r'\$', replace, args,
lambda s: util.shellquote(util.localpath(s)))
@@ -588,24 +595,39 @@
def _restorebackup(fcd, back):
# TODO: Add a workingfilectx.write(otherfilectx) path so we can use
# util.copy here instead.
-fcd.write(util.readfile(back), fcd.flags())
+fcd.write(back.data(), fcd.flags())
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`
(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
have.
"""
if fcd.isabsent():
return None
+from . import context
+back = scmutil.origpath(ui, repo, repo.wjoin(fcd.path()))
-a = _workingpath(repo, fcd)
-back = scmutil.origpath(ui, repo, a)
-if premerge:
-util.copyfile(a, back)
-return back
+inworkingdir = (back.startswith(repo.wvfs.base) and not
+back.startswith(repo.vfs.base))
+
+if isinstance(fcd, context.overlayworkingfilectx) and inworkingdir:
+# 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.
+#
+# A arbitraryfilectx is returned, so we can run the same functions on
+# the backup context regardless of where it lives.
+if premerge:
+util.copyfile(_workingpath(repo, fcd), back)
+return context.arbitraryfilectx(back, repo=repo)
def _maketempfiles(repo, fco, fca):
"""Writes out `fco` and `fca` as temporary files, so an external merge
@@ -719,7 +741,7 @@
return True, r, deleted
finally:
if not r and back is not None:
-util.unlink(back)
+back.remove()
def _check(repo, r, ui, tool, fcd, files):
fd = fcd.path()
@@ -741,7 +763,7 @@
if not r and not checked and (_toolbool(ui, tool, "checkchanged") or
'changed' in
_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):
if ui.promptchoice(_(" output file %s appears unchanged\n"
"was merge successful (yn)?"
"$$ $$ ") % fd, 1):
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -8,6 +8,7 @@
from __future__ import absolute_import
import errno
+import filecmp
import os
import re
import stat
@@ -697,7 +698,11 @@
def matches(self, match):
return self.walk(match)
-class basefilectx(object):
D674: filemerge: use arbitraryfilectx for backup files
phillco added inline comments. INLINE COMMENTS > martinvonz wrote in 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() `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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D674 To: phillco, #hg-reviewers Cc: sid0, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
martinvonz added inline comments. INLINE COMMENTS > phillco wrote in 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 `workingfilectx`s and `abstractfilectx`s. > > 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. 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() REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D674 To: phillco, #hg-reviewers Cc: sid0, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
phillco added inline comments. INLINE COMMENTS > martinvonz wrote in 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? 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 `workingfilectx`s and `abstractfilectx`s. 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D674 To: phillco, #hg-reviewers Cc: sid0, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
phillco added a subscriber: sid0. phillco added inline comments. INLINE COMMENTS > martinvonz wrote in test-dirstate-race.t:85 > Why did this change? 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 `IOError`s, 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D674 To: phillco, #hg-reviewers Cc: sid0, martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
martinvonz added inline comments.
INLINE COMMENTS
> context.py:700
>
> -class basefilectx(object):
> +class abstractfilectx(object):
> +def data(self):
Feel like checking out the "abc" module and see if that's helpful here?
> context.py:854
> """
> if fctx._customcmp:
> return fctx.cmp(self)
Looks like either _customcmp should be part of abstractfilectx or checked for
here (util.safehasattr())
> filemerge.py:744
>_toollist(ui, tool, "check")):
> -if back is not None and filecmp.cmp(_workingpath(repo, fcd), back):
> if ui.promptchoice(_(" output file %s appears unchanged\n"
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?
> test-dirstate-race.t:85
>! dir1/c
> + ! e
>$ hg debugdirstate
Why did this change?
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D674
To: phillco, #hg-reviewers
Cc: martinvonz, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D674: filemerge: use arbitraryfilectx for backup files
phillco updated this revision to Diff 1724.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D674?vs=1704=1724
REVISION DETAIL
https://phab.mercurial-scm.org/D674
AFFECTED FILES
mercurial/context.py
mercurial/filemerge.py
tests/test-dirstate-race.t
CHANGE DETAILS
diff --git a/tests/test-dirstate-race.t b/tests/test-dirstate-race.t
--- a/tests/test-dirstate-race.t
+++ b/tests/test-dirstate-race.t
@@ -80,9 +80,9 @@
$ hg status --config extensions.dirstaterace=$TESTTMP/dirstaterace.py
M d
- M e
! b
! dir1/c
+ ! e
$ hg debugdirstate
n 644 2 * a (glob)
n 0 -1 unset b
diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -7,7 +7,6 @@
from __future__ import absolute_import
-import filecmp
import os
import re
import tempfile
@@ -226,9 +225,9 @@
return '\n'
return None # unknown
-def _matcheol(file, origfile):
+def _matcheol(file, back):
"Convert EOL markers in a file to match origfile"
-tostyle = _eoltype(util.readfile(origfile))
+tostyle = _eoltype(back.data()) # No repo.wread filters?
if tostyle:
data = util.readfile(file)
style = _eoltype(data)
@@ -468,6 +467,12 @@
a = _workingpath(repo, fcd)
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())
repo.wwrite(fd + ".other", fco.data(), fco.flags())
repo.wwrite(fd + ".base", fca.data(), fca.flags())
@@ -505,7 +510,9 @@
args = _toolstr(ui, tool, "args", '$local $base $other')
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}
args = util.interpolate(r'\$', replace, args,
lambda s: util.shellquote(util.localpath(s)))
@@ -588,24 +595,39 @@
def _restorebackup(fcd, back):
# TODO: Add a workingfilectx.write(otherfilectx) path so we can use
# util.copy here instead.
-fcd.write(util.readfile(back), fcd.flags())
+fcd.write(back.data(), fcd.flags())
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`
(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
have.
"""
if fcd.isabsent():
return None
+from . import context
+back = scmutil.origpath(ui, repo, repo.wjoin(fcd.path()))
-a = _workingpath(repo, fcd)
-back = scmutil.origpath(ui, repo, a)
-if premerge:
-util.copyfile(a, back)
-return back
+inworkingdir = (back.startswith(repo.wvfs.base) and not
+back.startswith(repo.vfs.base))
+
+if isinstance(fcd, context.overlayworkingfilectx) and inworkingdir:
+# 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.
+#
+# A arbitraryfilectx is returned, so we can run the same functions on
+# the backup context regardless of where it lives.
+if premerge:
+util.copyfile(_workingpath(repo, fcd), back)
+return context.arbitraryfilectx(back)
def _maketempfiles(repo, fco, fca):
"""Writes out `fco` and `fca` as temporary files, so an external merge
@@ -719,7 +741,7 @@
return True, r, deleted
finally:
if not r and back is not None:
-util.unlink(back)
+back.remove()
def _check(repo, r, ui, tool, fcd, files):
fd = fcd.path()
@@ -741,7 +763,7 @@
if not r and not checked and (_toolbool(ui, tool, "checkchanged") or
'changed' in
_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):
if ui.promptchoice(_(" output file %s appears unchanged\n"
"was merge successful (yn)?"
D674: filemerge: use arbitraryfilectx for backup files
phillco created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D674
AFFECTED FILES
mercurial/context.py
mercurial/filemerge.py
tests/test-dirstate-race.t
CHANGE DETAILS
diff --git a/tests/test-dirstate-race.t b/tests/test-dirstate-race.t
--- a/tests/test-dirstate-race.t
+++ b/tests/test-dirstate-race.t
@@ -80,9 +80,9 @@
$ hg status --config extensions.dirstaterace=$TESTTMP/dirstaterace.py
M d
- M e
! b
! dir1/c
+ ! e
$ hg debugdirstate
n 644 2 * a (glob)
n 0 -1 unset b
diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -7,7 +7,6 @@
from __future__ import absolute_import
-import filecmp
import os
import re
import tempfile
@@ -226,9 +225,9 @@
return '\n'
return None # unknown
-def _matcheol(file, origfile):
+def _matcheol(file, back):
"Convert EOL markers in a file to match origfile"
-tostyle = _eoltype(util.readfile(origfile))
+tostyle = _eoltype(back.data()) # No repo.wread filters?
if tostyle:
data = util.readfile(file)
style = _eoltype(data)
@@ -468,6 +467,12 @@
a = _workingpath(repo, fcd)
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())
repo.wwrite(fd + ".other", fco.data(), fco.flags())
repo.wwrite(fd + ".base", fca.data(), fca.flags())
@@ -505,7 +510,9 @@
args = _toolstr(ui, tool, "args", '$local $base $other')
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}
args = util.interpolate(r'\$', replace, args,
lambda s: util.shellquote(util.localpath(s)))
@@ -588,24 +595,39 @@
def _restorebackup(fcd, back):
# TODO: Add a workingfilectx.write(otherfilectx) path so we can use
# util.copy here instead.
-fcd.write(util.readfile(back), fcd.flags())
+fcd.write(back.data(), fcd.flags())
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`
(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
have.
"""
if fcd.isabsent():
return None
+from . import context
+back = scmutil.origpath(ui, repo, repo.wjoin(fcd.path()))
-a = _workingpath(repo, fcd)
-back = scmutil.origpath(ui, repo, a)
-if premerge:
-util.copyfile(a, back)
-return back
+inworkingdir = (back.startswith(repo.wvfs.base) and not
+back.startswith(repo.vfs.base))
+
+if isinstance(fcd, context.overlayworkingfilectx) and inworkingdir:
+# 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.
+#
+# A arbitraryfilectx is returned, so we can run the same functions on
+# the backup context regardless of where it lives.
+if premerge:
+util.copyfile(_workingpath(repo, fcd), back)
+return context.arbitraryfilectx(back)
def _maketempfiles(repo, fco, fca):
"""Writes out `fco` and `fca` as temporary files, so an external merge
@@ -719,7 +741,7 @@
return True, r, deleted
finally:
if not r and back is not None:
-util.unlink(back)
+back.remove()
def _check(repo, r, ui, tool, fcd, files):
fd = fcd.path()
@@ -741,7 +763,7 @@
if not r and not checked and (_toolbool(ui, tool, "checkchanged") or
'changed' in
_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):
if ui.promptchoice(_(" output file %s appears unchanged\n"
"was merge successful (yn)?"
