Patchwork [1,of,5] revert: simplify handling of `added` files

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 15, 2014, 4:48 p.m.
Message ID <2eb66256ddb71face40e.1408121323@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/5431/
State Accepted
Headers show

Comments

Pierre-Yves David - Aug. 15, 2014, 4:48 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1407004344 25200
#      Sat Aug 02 11:32:24 2014 -0700
# Node ID 2eb66256ddb71face40ed113a98fd465aa9579da
# Parent  1a4de7e50e27a568b77854e0bbfc894d48a726b0
revert: simplify handling of `added` files

They are multiple possible cases for added files. But its all handled by magic
much lower in the stack. We document them, simplify the codes and move on.
Augie Fackler - Aug. 18, 2014, 10:26 p.m.
On Fri, Aug 15, 2014 at 09:48:43AM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1407004344 25200
> #      Sat Aug 02 11:32:24 2014 -0700
> # Node ID 2eb66256ddb71face40ed113a98fd465aa9579da
> # Parent  1a4de7e50e27a568b77854e0bbfc894d48a726b0
> revert: simplify handling of `added` files

queued the lot, thanks

(I'm now a little curious what the goal of all this revert refactoring
is, but I'm glad to see it in any case.)

>
> They are multiple possible cases for added files. But its all handled by magic
> much lower in the stack. We document them, simplify the codes and move on.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2398,23 +2398,39 @@ def revert(ui, repo, ctx, parents, *pats
>          else:
>              changes = repo.status(node1=parent, match=m)
>              dsmodified = set(changes[0])
>              dsadded    = set(changes[1])
>              dsremoved  = set(changes[2])
> -            dsadded |= _deletedadded
>
>              # only take in account for remove that between wc and target.
>              clean |= dsremoved - removed
>              dsremoved &= removed
>              # distinct between dirstate remove and other
>              removed -= dsremoved
>
> +
>              # tell newly modified appart.
>              dsmodified &= modified
>              dsmodified |= modified & dsadded # dirstate added may needs backup
>              modified -= dsmodified
>
> +            # there is three categories of added files
> +            #
> +            # 1. addition, that just happened in the dirstate
> +            #    (should be forgetten)
> +            # 2. file is added since target revision and have local changes
> +            #    (should be backed up and removed)
> +            # 3. file is added since target revision and is clean
> +            #    (should be removed)
> +            #
> +            # However we do not need to split them yet. The current revert code
> +            # will automatically recognise (1) when performing operation. And
> +            # the backup system is currently unabled to handle (2).
> +            #
> +            # So we just put them all in the same group
> +            dsadded = added
> +
>          # if f is a rename, update `names` to also revert the source
>          cwd = repo.getcwd()
>          for f in dsadded:
>              src = repo.dirstate.copied(f)
>              if src and src not in names and repo.dirstate[src] == 'r':
> @@ -2428,12 +2444,10 @@ def revert(ui, repo, ctx, parents, *pats
>                  return _('forgetting %s\n')
>              return _('removing %s\n')
>
>          missingmodified = dsmodified - smf
>          dsmodified -= missingmodified
> -        missingadded = dsadded - smf
> -        dsadded -= missingadded
>
>          # action to be actually performed by revert
>          # (<list of file>, message>) tuple
>          actions = {'revert': ([], _('reverting %s\n')),
>                     'add': ([], _('adding %s\n')),
> @@ -2446,12 +2460,11 @@ def revert(ui, repo, ctx, parents, *pats
>              #   action
>              #   make backup
>              (modified,         (actions['revert'],   False)),
>              (dsmodified,       (actions['revert'],   True)),
>              (missingmodified,  (actions['remove'],   True)),
> -            (dsadded,          (actions['revert'],   True)),
> -            (missingadded,     (actions['remove'],   False)),
> +            (dsadded,          (actions['remove'],   True)),
>              (removed,          (actions['add'],      True)),
>              (dsremoved,        (actions['undelete'], True)),
>              (clean,            (None,                False)),
>              )
>
> diff --git a/tests/test-revert.t b/tests/test-revert.t
> --- a/tests/test-revert.t
> +++ b/tests/test-revert.t
> @@ -906,16 +906,10 @@ Test revert --all to "base" content
>    $ cp -r revert-ref revert-base-all
>    $ cd revert-base-all
>
>  check revert output
>
> -Misbehavior:
> -
> -- report "reverting" when file needs no changes
> -|
> -| - reverting removed_revert
> -
>    $ hg revert --all --rev 'desc(base)'
>    removing added_clean
>    removing added_deleted
>    removing added_wc
>    reverting clean_deleted
> @@ -934,11 +928,10 @@ Misbehavior:
>    undeleting modified_untracked-wc
>    reverting modified_wc
>    adding removed_clean
>    reverting removed_deleted
>    adding removed_removed
> -  reverting removed_revert
>    adding removed_untracked-clean
>    adding removed_untracked-revert
>    adding removed_untracked-wc
>    reverting removed_wc
>
> @@ -1096,16 +1089,10 @@ Test revert to "base" content with expli
>    $ cd revert-base-explicit
>
>  revert all files individually and check the output
>  (output is expected to be different than in the --all case)
>
> -Misbehavior:
> -
> -- fails to report no change to revert for
> -|
> -| - removed_revert
> -
>    $ for file in `python ../gen-revert-cases.py filelist`; do
>    >   echo '### revert for:' $file;
>    >   hg revert $file --rev 'desc(base)';
>    >   echo
>    > done
> @@ -1192,10 +1179,11 @@ Misbehavior:
>    ### revert for: removed_deleted
>
>    ### revert for: removed_removed
>
>    ### revert for: removed_revert
> +  no changes needed to removed_revert
>
>    ### revert for: removed_untracked-clean
>
>    ### revert for: removed_untracked-revert
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Aug. 18, 2014, 10:34 p.m.
On Fri, Aug 15, 2014 at 09:48:43AM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1407004344 25200
> #      Sat Aug 02 11:32:24 2014 -0700
> # Node ID 2eb66256ddb71face40ed113a98fd465aa9579da
> # Parent  1a4de7e50e27a568b77854e0bbfc894d48a726b0
> revert: simplify handling of `added` files

Series looks fine, but I have conflicts in cmdutil - can you resend?

>
> They are multiple possible cases for added files. But its all handled by magic
> much lower in the stack. We document them, simplify the codes and move on.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2398,23 +2398,39 @@ def revert(ui, repo, ctx, parents, *pats
>          else:
>              changes = repo.status(node1=parent, match=m)
>              dsmodified = set(changes[0])
>              dsadded    = set(changes[1])
>              dsremoved  = set(changes[2])
> -            dsadded |= _deletedadded
>
>              # only take in account for remove that between wc and target.
>              clean |= dsremoved - removed
>              dsremoved &= removed
>              # distinct between dirstate remove and other
>              removed -= dsremoved
>
> +
>              # tell newly modified appart.
>              dsmodified &= modified
>              dsmodified |= modified & dsadded # dirstate added may needs backup
>              modified -= dsmodified
>
> +            # there is three categories of added files
> +            #
> +            # 1. addition, that just happened in the dirstate
> +            #    (should be forgetten)
> +            # 2. file is added since target revision and have local changes
> +            #    (should be backed up and removed)
> +            # 3. file is added since target revision and is clean
> +            #    (should be removed)
> +            #
> +            # However we do not need to split them yet. The current revert code
> +            # will automatically recognise (1) when performing operation. And
> +            # the backup system is currently unabled to handle (2).
> +            #
> +            # So we just put them all in the same group
> +            dsadded = added
> +
>          # if f is a rename, update `names` to also revert the source
>          cwd = repo.getcwd()
>          for f in dsadded:
>              src = repo.dirstate.copied(f)
>              if src and src not in names and repo.dirstate[src] == 'r':
> @@ -2428,12 +2444,10 @@ def revert(ui, repo, ctx, parents, *pats
>                  return _('forgetting %s\n')
>              return _('removing %s\n')
>
>          missingmodified = dsmodified - smf
>          dsmodified -= missingmodified
> -        missingadded = dsadded - smf
> -        dsadded -= missingadded
>
>          # action to be actually performed by revert
>          # (<list of file>, message>) tuple
>          actions = {'revert': ([], _('reverting %s\n')),
>                     'add': ([], _('adding %s\n')),
> @@ -2446,12 +2460,11 @@ def revert(ui, repo, ctx, parents, *pats
>              #   action
>              #   make backup
>              (modified,         (actions['revert'],   False)),
>              (dsmodified,       (actions['revert'],   True)),
>              (missingmodified,  (actions['remove'],   True)),
> -            (dsadded,          (actions['revert'],   True)),
> -            (missingadded,     (actions['remove'],   False)),
> +            (dsadded,          (actions['remove'],   True)),
>              (removed,          (actions['add'],      True)),
>              (dsremoved,        (actions['undelete'], True)),
>              (clean,            (None,                False)),
>              )
>
> diff --git a/tests/test-revert.t b/tests/test-revert.t
> --- a/tests/test-revert.t
> +++ b/tests/test-revert.t
> @@ -906,16 +906,10 @@ Test revert --all to "base" content
>    $ cp -r revert-ref revert-base-all
>    $ cd revert-base-all
>
>  check revert output
>
> -Misbehavior:
> -
> -- report "reverting" when file needs no changes
> -|
> -| - reverting removed_revert
> -
>    $ hg revert --all --rev 'desc(base)'
>    removing added_clean
>    removing added_deleted
>    removing added_wc
>    reverting clean_deleted
> @@ -934,11 +928,10 @@ Misbehavior:
>    undeleting modified_untracked-wc
>    reverting modified_wc
>    adding removed_clean
>    reverting removed_deleted
>    adding removed_removed
> -  reverting removed_revert
>    adding removed_untracked-clean
>    adding removed_untracked-revert
>    adding removed_untracked-wc
>    reverting removed_wc
>
> @@ -1096,16 +1089,10 @@ Test revert to "base" content with expli
>    $ cd revert-base-explicit
>
>  revert all files individually and check the output
>  (output is expected to be different than in the --all case)
>
> -Misbehavior:
> -
> -- fails to report no change to revert for
> -|
> -| - removed_revert
> -
>    $ for file in `python ../gen-revert-cases.py filelist`; do
>    >   echo '### revert for:' $file;
>    >   hg revert $file --rev 'desc(base)';
>    >   echo
>    > done
> @@ -1192,10 +1179,11 @@ Misbehavior:
>    ### revert for: removed_deleted
>
>    ### revert for: removed_removed
>
>    ### revert for: removed_revert
> +  no changes needed to removed_revert
>
>    ### revert for: removed_untracked-clean
>
>    ### revert for: removed_untracked-revert
>
> _______________________________________________
> 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
@@ -2398,23 +2398,39 @@  def revert(ui, repo, ctx, parents, *pats
         else:
             changes = repo.status(node1=parent, match=m)
             dsmodified = set(changes[0])
             dsadded    = set(changes[1])
             dsremoved  = set(changes[2])
-            dsadded |= _deletedadded
 
             # only take in account for remove that between wc and target.
             clean |= dsremoved - removed
             dsremoved &= removed
             # distinct between dirstate remove and other
             removed -= dsremoved
 
+
             # tell newly modified appart.
             dsmodified &= modified
             dsmodified |= modified & dsadded # dirstate added may needs backup
             modified -= dsmodified
 
+            # there is three categories of added files
+            #
+            # 1. addition, that just happened in the dirstate
+            #    (should be forgetten)
+            # 2. file is added since target revision and have local changes
+            #    (should be backed up and removed)
+            # 3. file is added since target revision and is clean
+            #    (should be removed)
+            #
+            # However we do not need to split them yet. The current revert code
+            # will automatically recognise (1) when performing operation. And
+            # the backup system is currently unabled to handle (2).
+            #
+            # So we just put them all in the same group
+            dsadded = added
+
         # if f is a rename, update `names` to also revert the source
         cwd = repo.getcwd()
         for f in dsadded:
             src = repo.dirstate.copied(f)
             if src and src not in names and repo.dirstate[src] == 'r':
@@ -2428,12 +2444,10 @@  def revert(ui, repo, ctx, parents, *pats
                 return _('forgetting %s\n')
             return _('removing %s\n')
 
         missingmodified = dsmodified - smf
         dsmodified -= missingmodified
-        missingadded = dsadded - smf
-        dsadded -= missingadded
 
         # action to be actually performed by revert
         # (<list of file>, message>) tuple
         actions = {'revert': ([], _('reverting %s\n')),
                    'add': ([], _('adding %s\n')),
@@ -2446,12 +2460,11 @@  def revert(ui, repo, ctx, parents, *pats
             #   action
             #   make backup
             (modified,         (actions['revert'],   False)),
             (dsmodified,       (actions['revert'],   True)),
             (missingmodified,  (actions['remove'],   True)),
-            (dsadded,          (actions['revert'],   True)),
-            (missingadded,     (actions['remove'],   False)),
+            (dsadded,          (actions['remove'],   True)),
             (removed,          (actions['add'],      True)),
             (dsremoved,        (actions['undelete'], True)),
             (clean,            (None,                False)),
             )
 
diff --git a/tests/test-revert.t b/tests/test-revert.t
--- a/tests/test-revert.t
+++ b/tests/test-revert.t
@@ -906,16 +906,10 @@  Test revert --all to "base" content
   $ cp -r revert-ref revert-base-all
   $ cd revert-base-all
 
 check revert output
 
-Misbehavior:
-
-- report "reverting" when file needs no changes
-|
-| - reverting removed_revert
-
   $ hg revert --all --rev 'desc(base)'
   removing added_clean
   removing added_deleted
   removing added_wc
   reverting clean_deleted
@@ -934,11 +928,10 @@  Misbehavior:
   undeleting modified_untracked-wc
   reverting modified_wc
   adding removed_clean
   reverting removed_deleted
   adding removed_removed
-  reverting removed_revert
   adding removed_untracked-clean
   adding removed_untracked-revert
   adding removed_untracked-wc
   reverting removed_wc
 
@@ -1096,16 +1089,10 @@  Test revert to "base" content with expli
   $ cd revert-base-explicit
 
 revert all files individually and check the output
 (output is expected to be different than in the --all case)
 
-Misbehavior:
-
-- fails to report no change to revert for
-|
-| - removed_revert
-
   $ for file in `python ../gen-revert-cases.py filelist`; do
   >   echo '### revert for:' $file;
   >   hg revert $file --rev 'desc(base)';
   >   echo
   > done
@@ -1192,10 +1179,11 @@  Misbehavior:
   ### revert for: removed_deleted
   
   ### revert for: removed_removed
   
   ### revert for: removed_revert
+  no changes needed to removed_revert
   
   ### revert for: removed_untracked-clean
   
   ### revert for: removed_untracked-revert