[PATCH] revert: process subrepos before the local repo

[Matt Mackall]

On Thu, 2015-03-26 at 23:30 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison at yahoo.com>
> # Date 1427423049 14400
> #      Thu Mar 26 22:24:09 2015 -0400
> # Node ID 910503083e4a0f49e71b9b392913990e00b63f3c
> # Parent  30ddc3cf76df464f7b4ad38eb49e4d1f49457f01
> revert: process subrepos before the local repo
> 
> The initial match variable 'm', built from the 'pats' passed to the method, is
> replaced part way through the method.  But when the 'pats' list is replaced by
> a matcher in an upcoming patch, that will need to be narrowed and passed to the
> subrepo.  Rather than breaking naming conventions, move the subrepo processing
> to before the replacement, and revert from bottom up.

I was thinking of something more like this:

diff -r 95cbc77c0cad mercurial/cmdutil.py
--- a/mercurial/cmdutil.py	Wed Mar 25 15:53:30 2015 -0700
+++ b/mercurial/cmdutil.py	Fri Mar 27 13:37:15 2015 -0500
@@ -2811,6 +2811,10 @@
         interactive = opts.get('interactive', False)
         wctx = repo[None]
         m = scmutil.match(wctx, pats, opts)
+
+        # we'll need this later
+        targetsubs = sorted(s for s in wctx.substate if m(s))
+
         if not m.always():
             m.bad = lambda x, y: False
             for abs in repo.walk(m):
@@ -3048,10 +3052,6 @@
             _revertprefetch(repo, ctx, *[actions[name][0] for name in needdata])
             _performrevert(repo, parents, ctx, actions, interactive)
 
-        # get the list of subrepos that must be reverted
-        subrepomatch = scmutil.match(wctx, pats, opts)
-        targetsubs = sorted(s for s in wctx.substate if subrepomatch(s))
-
         if targetsubs:
             # Revert the subrepos on the revert list
             for sub in targetsubs:

I'm not clear if there's something gained in your approach?

> The additional '.hgsub not found' message is because the .hgsub file used to be
> reverted before the subrepo list was built.  So maybe there is merit to
> reverting from top down?  OTOH, reverting .hgsub to some revision where the
> subrepo didn't exist before processing subrepos will prevent that subrepo from
> being reverted.  If we are reverting based on the working directory, we probably
> need to stick with it for better or worse.
> 
> The other two .hgsub related messages in the area are from:
>    1) The repo.walk() under 'not always()'
>    2) repo.status(), just below that
> 
> Shouldn't the @propertycache on basectx.substate() cache the result of
> subrepo.state(), so that it is only read once from disk, and therefore the
> message is only displayed once?
> 
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2813,6 +2813,20 @@
>  
>          wctx = repo[None]
>          m = scmutil.match(wctx, pats, opts)
> +
> +        # get the list of subrepos that must be reverted
> +        subrepomatch = m
> +        targetsubs = sorted(s for s in wctx.substate if subrepomatch(s))
> +
> +        if targetsubs:
> +            # Revert the subrepos on the revert list
> +            for sub in targetsubs:
> +                try:
> +                    wctx.sub(sub).revert(ctx.substate[sub], *pats, **opts)
> +                except KeyError:
> +                    raise util.Abort("subrepository '%s' does not exist in %s!"
> +                                      % (sub, short(ctx.node())))
> +
>          if not m.always():
>              m.bad = lambda x, y: False
>              for abs in repo.walk(m):
> @@ -3047,19 +3061,6 @@
>              _revertprefetch(repo, ctx, *[actions[name][0] for name in needdata])
>              interactive = opts.get('interactive', False)
>              _performrevert(repo, parents, ctx, actions, interactive)
> -
> -        # get the list of subrepos that must be reverted
> -        subrepomatch = scmutil.match(wctx, pats, opts)
> -        targetsubs = sorted(s for s in wctx.substate if subrepomatch(s))
> -
> -        if targetsubs:
> -            # Revert the subrepos on the revert list
> -            for sub in targetsubs:
> -                try:
> -                    wctx.sub(sub).revert(ctx.substate[sub], *pats, **opts)
> -                except KeyError:
> -                    raise util.Abort("subrepository '%s' does not exist in %s!"
> -                                      % (sub, short(ctx.node())))
>      finally:
>          wlock.release()
>  
> diff --git a/tests/test-largefiles-misc.t b/tests/test-largefiles-misc.t
> --- a/tests/test-largefiles-misc.t
> +++ b/tests/test-largefiles-misc.t
> @@ -1035,9 +1035,9 @@
>    M large
>    M no-largefiles/normal1
>    $ hg -R subrepo-root revert --all
> -  reverting subrepo-root/.hglf/large (glob)
>    reverting subrepo no-largefiles
>    reverting subrepo-root/no-largefiles/normal1 (glob)
> +  reverting subrepo-root/.hglf/large (glob)
>  
>    $ cd ..
>  
> diff --git a/tests/test-subrepo-missing.t b/tests/test-subrepo-missing.t
> --- a/tests/test-subrepo-missing.t
> +++ b/tests/test-subrepo-missing.t
> @@ -34,6 +34,7 @@
>    $ hg revert .hgsub
>    warning: subrepo spec file .hgsub not found
>    warning: subrepo spec file .hgsub not found
> +  warning: subrepo spec file .hgsub not found
>  
>  delete .hgsubstate and revert it
>  
> diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
> --- a/tests/test-subrepo.t
> +++ b/tests/test-subrepo.t
> @@ -914,8 +914,8 @@
>    $ rm repo/s/b
>    $ touch -t 200001010000 repo/.hgsubstate
>    $ hg -R repo revert --all
> +  reverting subrepo s
>    reverting repo/.hgsubstate (glob)
> -  reverting subrepo s
>    $ hg -R repo update
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>    $ cat repo/s/b
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

-- 
Mathematics is the supreme nostalgia of our time.