Patchwork [4,of,4] revert: properly backup added files with local modification

login
register
mail settings
Submitter Pierre-Yves David
Date Sept. 20, 2014, 1:05 a.m.
Message ID <eec4521582edd77c27b4.1411175103@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/5897/
State Superseded
Headers show

Comments

Pierre-Yves David - Sept. 20, 2014, 1:05 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1409482860 -7200
#      Sun Aug 31 13:01:00 2014 +0200
# Node ID eec4521582edd77c27b40dab25dc743fdd816f80
# Parent  d615331a58705680a54d72ee07269083018a4e60
revert: properly backup added files with local modification

This files were previously not backed up because the backup mechanism was not
smart enough. This lead to data lost from user since uncommitted content were
lost.

We now properly move the modified version to <filename>.orig before deleting it.

We have to use a small hack to do a different action if "--no-backup" is
specified. This is needed because the backup process is actually a move (not a
copy) so the file is already missing when we backup. The internet kitten is a
bit disapointed about that, but such is live.

This patch concludes the "lets refactor revert" phases. We can now open the
"Lets find stupid bug with renames and merge" phases.

I'm sure that now that the code is clearer we could do it in another simpler
way, but I consider the current improvement good enough for now.
Augie Fackler - Sept. 24, 2014, 3:33 p.m.
On Fri, Sep 19, 2014 at 06:05:03PM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1409482860 -7200
> #      Sun Aug 31 13:01:00 2014 +0200
> # Node ID eec4521582edd77c27b40dab25dc743fdd816f80
> # Parent  d615331a58705680a54d72ee07269083018a4e60
> revert: properly backup added files with local modification

Overall decent, but the magic number stuff worries me, so I await the
shedding of that bike.

>
> This files were previously not backed up because the backup mechanism was not
> smart enough. This lead to data lost from user since uncommitted content were
> lost.
>
> We now properly move the modified version to <filename>.orig before deleting it.
>
> We have to use a small hack to do a different action if "--no-backup" is
> specified. This is needed because the backup process is actually a move (not a
> copy) so the file is already missing when we backup. The internet kitten is a
> bit disapointed about that, but such is live.
>
> This patch concludes the "lets refactor revert" phases. We can now open the
> "Lets find stupid bug with renames and merge" phases.
>
> I'm sure that now that the code is clearer we could do it in another simpler
> way, but I consider the current improvement good enough for now.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2630,10 +2630,15 @@ def revert(ui, repo, ctx, parents, *pats
>          check = 1
>          discard = 0
>          if opts.get('no_backup'):
>              check = backup = discard
>
> +
> +        backupanddel = actions['remove']
> +        if not opts.get('no_backup'):
> +            backupanddel = actions['drop']
> +
>          disptable = (
>              # dispatch table:
>              #   file state
>              #   action
>              #   make backup
> @@ -2648,11 +2653,11 @@ def revert(ui, repo, ctx, parents, *pats
>              # Added since target
>              (added,         actions['remove'],   discard),
>              # Added in working directory
>              (dsadded,       actions['forget'],   discard),
>              # Added since target, have local modification
> -            (modadded,      actions['remove'],   discard),
> +            (modadded,      backupanddel,        backup),
>              # Added since target but file is missing in working directory
>              (deladded,      actions['drop'],   discard),
>              # Removed since  target, before working copy parent
>              (removed,       actions['add'],      discard),
>              # Same as `removed` but an unknown file exists at the same path
> diff --git a/tests/test-revert.t b/tests/test-revert.t
> --- a/tests/test-revert.t
> +++ b/tests/test-revert.t
> @@ -948,10 +948,11 @@ Misbehavior:
>    $ python ../dircontent.py > ../content-base-all.txt
>    $ cd ..
>    $ diff -U 0 -- content-base.txt content-base-all.txt | grep _
>    +parent added_untracked-clean
>    +wc     added_untracked-wc
> +  +wc     added_wc.orig
>    +wc     clean_untracked-wc.orig
>    +wc     clean_wc.orig
>    +wc     missing_untracked-wc
>    +wc     missing_wc
>    +parent modified_untracked-clean.orig
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2630,10 +2630,15 @@  def revert(ui, repo, ctx, parents, *pats
         check = 1
         discard = 0
         if opts.get('no_backup'):
             check = backup = discard
 
+
+        backupanddel = actions['remove']
+        if not opts.get('no_backup'):
+            backupanddel = actions['drop']
+
         disptable = (
             # dispatch table:
             #   file state
             #   action
             #   make backup
@@ -2648,11 +2653,11 @@  def revert(ui, repo, ctx, parents, *pats
             # Added since target
             (added,         actions['remove'],   discard),
             # Added in working directory
             (dsadded,       actions['forget'],   discard),
             # Added since target, have local modification
-            (modadded,      actions['remove'],   discard),
+            (modadded,      backupanddel,        backup),
             # Added since target but file is missing in working directory
             (deladded,      actions['drop'],   discard),
             # Removed since  target, before working copy parent
             (removed,       actions['add'],      discard),
             # Same as `removed` but an unknown file exists at the same path
diff --git a/tests/test-revert.t b/tests/test-revert.t
--- a/tests/test-revert.t
+++ b/tests/test-revert.t
@@ -948,10 +948,11 @@  Misbehavior:
   $ python ../dircontent.py > ../content-base-all.txt
   $ cd ..
   $ diff -U 0 -- content-base.txt content-base-all.txt | grep _
   +parent added_untracked-clean
   +wc     added_untracked-wc
+  +wc     added_wc.orig
   +wc     clean_untracked-wc.orig
   +wc     clean_wc.orig
   +wc     missing_untracked-wc
   +wc     missing_wc
   +parent modified_untracked-clean.orig