Patchwork D1057: use arbitraryfilectx for backups

login
register
mail settings
Submitter phabricator
Date Oct. 13, 2017, 7:45 p.m.
Message ID <differential-rev-PHID-DREV-orhszjlcleaechdby4n3-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/24849/
State Superseded
Headers show

Comments

phabricator - Oct. 13, 2017, 7:45 p.m.
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/D1057

AFFECTED FILES
  mercurial/filemerge.py

CHANGE DETAILS




To: phillco, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 14, 2017, 4:53 a.m.
durin42 requested changes to this revision.
durin42 added a comment.
This revision now requires changes to proceed.


  (Feel free to push back with a TODO documenting how to do a fix or something.)

INLINE COMMENTS

> filemerge.py:604
>          return None
> -
> +    from . import context
>      a = _workingpath(repo, fcd)

Well that's a bummer. Could we move context.*filectx* into mercurial.filecontext and then avoid the need for this function-level import?

REPOSITORY
  rHG Mercurial

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

To: phillco, #hg-reviewers, durin42
Cc: durin42, mercurial-devel
phabricator - Oct. 16, 2017, 3:25 a.m.
phillco added inline comments.

INLINE COMMENTS

> durin42 wrote in filemerge.py:604
> Well that's a bummer. Could we move context.*filectx* into mercurial.filecontext and then avoid the need for this function-level import?

I don't think so, because the filectx classes reference the ctx classes in various places.

REPOSITORY
  rHG Mercurial

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

To: phillco, #hg-reviewers, durin42
Cc: durin42, mercurial-devel
phabricator - Oct. 16, 2017, 7:03 p.m.
durin42 added inline comments.

INLINE COMMENTS

> phillco wrote in filemerge.py:604
> I don't think so, because the filectx classes reference the ctx classes in various places.

Ick. Please add a TODO here to figure out a way to break the cycle (it can be done! I'm sure of it, even if I don't yet know how) and then we can move forward with this.

REPOSITORY
  rHG Mercurial

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

To: phillco, #hg-reviewers, durin42
Cc: durin42, mercurial-devel
phabricator - Oct. 16, 2017, 8:07 p.m.
phillco added inline comments.

INLINE COMMENTS

> durin42 wrote in filemerge.py:604
> Ick. Please add a TODO here to figure out a way to break the cycle (it can be done! I'm sure of it, even if I don't yet know how) and then we can move forward with this.

Sure, will do. I think most likely by breaking the context -> fileset dependency.

REPOSITORY
  rHG Mercurial

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

To: phillco, #hg-reviewers, durin42
Cc: durin42, mercurial-devel
phabricator - Oct. 16, 2017, 8:11 p.m.
phillco marked 4 inline comments as done.
phillco added inline comments.

INLINE COMMENTS

> phillco wrote in filemerge.py:604
> Sure, will do. I think most likely by breaking the context -> fileset dependency.

@durin42 done

REPOSITORY
  rHG Mercurial

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

To: phillco, #hg-reviewers, durin42
Cc: durin42, mercurial-devel

Patch

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)
@@ -505,7 +504,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 +589,24 @@ 
 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
+    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 +692,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 +720,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 +742,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)?"
                                  "$$ &Yes $$ &No") % fd, 1):