Patchwork revert: process subrepos before the local repo

# HG changeset patch
# User Matt Harbison <matt_harbison@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.

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?

On Thu, Mar 26, 2015 at 11:30:20PM -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@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.
>
> 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.

Interesting. This might make it strictly worse, because when you
revert and it reintroduces .hgsub you end up with a partial working
copy (you'd have to revert twice), but if you revert and it removes
.hgsub the worst you end up with is some untracked non-hg subrepos.

Ideally, we could detect that .hgsub was going to disappear or appear
and do things in the right order?

Other than this, the patch seems reasonable to me, but I'm going to
let someone else take it or not, since I'm unclear on what the lesser
evil is going to be.

>
> 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@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel