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

[Sean Karlage]

# HG changeset patch
# User skarlage <skarlage at fb.com>
# Date 1467301099 25200
#      Thu Jun 30 08:38:19 2016 -0700
# Node ID 15417d1e4d3c1d3fb0108df57cf9ecff4bf50357
# Parent  ecbbf4d56ee82b48fe4fa19d68056b9d17eff570
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 @@
         # 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 @@
             # 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 @@
                     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 @@
         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 @@
     """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 @@
             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 @@
   
   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