# HG changeset patch
# User skarlage <skarlage@fb.com>
# Date 2016-06-30 15:38:19
# Node ID 1b38cfde9530331d8d5767aa09a0de7d90931845
# Parent  ee2027195847c389e64ab2ed9714f7a938a8d42d

revert: don't backup if no files reverted in interactive mode (issue4793)

When reverting interactively, we always backup files before prompting the user
to find out if they actually want to revert them. This can create spurious
*.orig files if a user enters an interactive revert session and then doesn't
revert any files. Instead, we should only backup files that are actually being
touched.

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -3143,11 +3143,17 @@ def revert(ui, repo, ctx, parents, *pats
         # All set to `discard` if `no-backup` is set do avoid checking
         # no_backup lower in the code.
         # These values are ordered for comparison purposes
+        backupinteractive = 3 # do backup if interactively modified
         backup = 2  # unconditionally do backup
         check = 1   # check if the existing file differs from target
         discard = 0 # never do backup
         if opts.get('no_backup'):
-            backup = check = discard
+            backupinteractive = backup = check = discard
+        if interactive:
+            dsmodifiedbackup = backupinteractive
+        else:
+            dsmodifiedbackup = backup
+        tobackup = set()
 
         backupanddel = actions['remove']
         if not opts.get('no_backup'):
@@ -3165,7 +3171,7 @@ def revert(ui, repo, ctx, parents, *pats
             # Modified compared to target, but local file is deleted
             (deleted,       actions['revert'],   discard),
             # Modified compared to target, local change
-            (dsmodified,    actions['revert'],   backup),
+            (dsmodified,    actions['revert'],   dsmodifiedbackup),
             # Added since target
             (added,         actions['remove'],   discard),
             # Added in working directory
@@ -3200,8 +3206,12 @@ def revert(ui, repo, ctx, parents, *pats
                     continue
                 if xlist is not None:
                     xlist.append(abs)
-                    if dobackup and (backup <= dobackup
-                                     or wctx[abs].cmp(ctx[abs])):
+                    if dobackup:
+                        # If in interactive mode, don't automatically create
+                        # .orig files (issue4793)
+                        if dobackup == backupinteractive:
+                            tobackup.add(abs)
+                        elif (backup <= dobackup or wctx[abs].cmp(ctx[abs])):
                             bakname = scmutil.origpath(ui, repo, rel)
                             ui.note(_('saving current version of %s as %s\n') %
                                     (rel, bakname))
@@ -3221,7 +3231,7 @@ def revert(ui, repo, ctx, parents, *pats
         if not opts.get('dry_run'):
             needdata = ('revert', 'add', 'undelete')
             _revertprefetch(repo, ctx, *[actions[name][0] for name in needdata])
-            _performrevert(repo, parents, ctx, actions, interactive)
+            _performrevert(repo, parents, ctx, actions, interactive, tobackup)
 
         if targetsubs:
             # Revert the subrepos on the revert list
@@ -3236,7 +3246,8 @@ def _revertprefetch(repo, ctx, *files):
     """Let extension changing the storage layer prefetch content"""
     pass
 
-def _performrevert(repo, parents, ctx, actions, interactive=False):
+def _performrevert(repo, parents, ctx, actions, interactive=False,
+                   tobackup=None):
     """function that actually perform all the actions computed for revert
 
     This is an independent function to let extension to plug in and react to
@@ -3316,9 +3327,18 @@ def _performrevert(repo, parents, ctx, a
             raise error.Abort(_('error parsing patch: %s') % err)
 
         newlyaddedandmodifiedfiles = newandmodified(chunks, originalchunks)
+        if tobackup is None:
+            tobackup = set()
         # Apply changes
         fp = stringio()
         for c in chunks:
+            # Create a backup file only if this hunk should be backed up
+            if ishunk(c) and c.header.filename() in tobackup:
+                abs = c.header.filename()
+                target = repo.wjoin(abs)
+                bakname = scmutil.origpath(repo.ui, repo, m.rel(abs))
+                util.copyfile(target, bakname)
+                tobackup.remove(abs)
             c.write(fp)
         dopatch = fp.tell()
         fp.seek(0)
diff --git a/tests/test-revert-interactive.t b/tests/test-revert-interactive.t
--- a/tests/test-revert-interactive.t
+++ b/tests/test-revert-interactive.t
@@ -134,7 +134,41 @@ Test that --interactive lift the need fo
   
   abort: user quit
   [255]
-  $ rm folder1/g.orig
+  $ ls folder1/
+  g
+
+Test that a noop revert doesn't do an unecessary backup
+  $ (echo y; echo n) | hg revert -i -r 2 folder1/g
+  diff --git a/folder1/g b/folder1/g
+  1 hunks, 1 lines changed
+  examine changes to 'folder1/g'? [Ynesfdaq?] y
+  
+  @@ -3,3 +3,4 @@
+   3
+   4
+   5
+  +d
+  revert this change to 'folder1/g'? [Ynesfdaq?] n
+  
+  $ ls folder1/
+  g
+
+Test --no-backup
+  $ (echo y; echo y) | hg revert -i -C -r 2 folder1/g
+  diff --git a/folder1/g b/folder1/g
+  1 hunks, 1 lines changed
+  examine changes to 'folder1/g'? [Ynesfdaq?] y
+  
+  @@ -3,3 +3,4 @@
+   3
+   4
+   5
+  +d
+  revert this change to 'folder1/g'? [Ynesfdaq?] y
+  
+  $ ls folder1/
+  g
+  >>> open('folder1/g', 'wb').write("1\n2\n3\n4\n5\nd\n")
 
 
   $ hg update -C 6