Patchwork D1806: filemerge: fix backing up an in-memory file to a custom location

login
register
mail settings
Submitter phabricator
Date Jan. 4, 2018, 7:36 p.m.
Message ID <differential-rev-PHID-DREV-xtgzqtrqnt6aieltbz23-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/26541/
State Superseded
Headers show

Comments

phabricator - Jan. 4, 2018, 7:36 p.m.
phillco created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  If the user specifies a ui.origbackuppath, we used to always copy the file
  there, but if the source file is in memory we must write it instead of copying.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/filemerge.py

CHANGE DETAILS




To: phillco, #hg-reviewers
Cc: mercurial-devel
Josef 'Jeff' Sipek - Jan. 4, 2018, 7:56 p.m.
On Thu, Jan 04, 2018 at 07:36:55PM +0000, phillco (Phil Cohen) wrote:
> phillco created this revision.
> Herald added a subscriber: mercurial-devel.
> Herald added a reviewer: hg-reviewers.
> 
> REVISION SUMMARY
>   If the user specifies a ui.origbackuppath, we used to always copy the file
>   there, but if the source file is in memory we must write it instead of copying.
> 
> REPOSITORY
>   rHG Mercurial
> 
> REVISION DETAIL
>   https://phab.mercurial-scm.org/D1806
> 
> AFFECTED FILES
>   mercurial/filemerge.py
> 
> CHANGE DETAILS
> 
> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -618,6 +618,9 @@
>      (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.
> +
> +    Backups are only need to be written for the premerge, and not again during

"Backups are only need to be" needs grammar :)

How about "Backups only need to be..."?

...
> +        if premerge:
> +            # Otherwise, write to wherever path the user specified the backups

s/wherever/whichever/ ?

Jeff.
phabricator - Jan. 4, 2018, 8:17 p.m.
durham accepted this revision.
durham added a comment.


  Accepting, since it seems more correct than before.
  
  Should we even be calling _makebackup in the case of an inmemory merge?  Like, maybe the makebackup should be conditional based on if the source file context is actually a workingctx?

INLINE COMMENTS

> filemerge.py:623
> +    Backups are only need to be written for the premerge, and not again during
> +    the main merge.
>      """

I know you're just documenting the parameter that already existed, but might be nice to explain why this is (if you happen to know why right now).

> filemerge.py:643
> +        if premerge:
> +            # Otherwise, write to wherever path the user specified the backups
> +            # should go. We still need to switch based on whether the source is

s/wherever/whatever/

REPOSITORY
  rHG Mercurial

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

To: phillco, #hg-reviewers, durham
Cc: durham, mercurial-devel
phabricator - Jan. 4, 2018, 8:18 p.m.
phillco added a comment.


  > Should we even be calling _makebackup in the case of an inmemory merge? Like, maybe the makebackup should be conditional based on if the source file context is actually a workingctx?
  
  The merge process itself uses the backup, it's not just for the user.

INLINE COMMENTS

> durham wrote in filemerge.py:623
> I know you're just documenting the parameter that already existed, but might be nice to explain why this is (if you happen to know why right now).

I think I can

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -618,6 +618,9 @@ 
     (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.
+
+    Backups are only need to be written for the premerge, and not again during
+    the main merge.
     """
     if fcd.isabsent():
         return None
@@ -628,21 +631,25 @@ 
     back = scmutil.origpath(ui, repo, a)
     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.
-        #
+        if premerge:
+            # Otherwise, write to wherever path the user specified the backups
+            # should go. We still need to switch based on whether the source is
+            # in-memory so we can use the fast path of ``util.copy`` if both are
+            # on disk.
+            if isinstance(fcd, context.overlayworkingfilectx):
+                util.writefile(back, fcd.data())
+            else:
+                util.copyfile(a, back)
         # 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):