[PATCH V2] revert: add support for reverting subrepos

Fri Oct 14 17:27:44 CDT 2011

On Tue, Oct 11, 2011 at 4:44 PM, Matt Mackall <mpm at selenic.com> wrote:
> On Thu, 2011-10-06 at 17:48 +0200, Angel Ezquerra wrote:
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra at gmail.com>
>> # Date 1317712886 -7200
>> # Node ID 48f9111208bfbbe831bb514bc133b854c3838d2d
>> # Parent  6dc67dced8c122f6139ae20ccdc03a6b11e8b765
>> revert: add support for reverting subrepos
>>
>> Reverting a subrepo is done by updating it to the revision that is selected on
>> the parent repo .hgsubstate file.
>
> I've been putting off reviewing this a bit. Revert is one of my
> least-favorite pieces of code and has been on my
> needs-rewriting-from-scratch list for years and making it more
> complicated just scares me. But this is surprisingly not so bad.

:-)

Thank you for the detailed review. It was very helpful!

> I suspect this doesn't handle subrepos getting added and removed at all.

No, it did not. Handling added added and removed subrepos may involve
manually modifying the .hgsub and .hgsubsate files. Does that make
sense?

> If your "separating file and subrepo patterns" helper is useful, then it
> should probably live in subrepo.py. It should probably work something
> like this:
>
> subpats, otherpats = subrepo.splitpats(pats)

OK. I will probably not need it at all though, thanks to your suggestions below.

>> - The behavior of the --all flag has not been changed: The --all flag will not
>> revert the state of the subrepos. This could be changed as well, but it is left
>> for a later patch (if considered appropriate).
>
> Seems appropriate to me? Is there a reason it wouldn't be?

I also think that that is the right thing to do. The only thing that
worries me is that it is a behaviour change which may lead to data
loss. Currently doing "hg revert --all" will leave all subrepos alone.
If we add subrepo support for the --all flag then people may lose
changes in their subrepos by accident, if they relied in the old
(arguably wrong) behaviour.

>> - I've used ui.status() to show a message while the subrepos are being
>> reverted. However TortoiseHg does not properly capture this message (it shows a
>> dialog box rather than showing the message on its console).
>
> Odd. But I bet I know why...

Steve told me why. It seems that the revert command is one of the few
places left in TortoiseHg where mercurial is called directly via
mercurial.commands.revert(), rather than through the runAction()
function.

>> +        hint = _("there are subrepos on the revert list, "
>> +            "use --no-backup to revert them")
>> +        raise util.Abort(msg, hint=hint)
>> +
>> +    # We currently do not support reverting non mercurial subrepos, so check
>> +    # that all subrepos on the subrepo list are mercurial subrepos
>
> Hmm, this says "this patch is doing something wrong".

I agree that we should be able to revert all kinds of subrepos.
However I am not so sure that we'll be able to support not using the
--no-backup option with non mercurial subrepos.

Unless we use the --no-backup option, we are supposed to backup each
modified file. To do so I thought that we could run revert on each
subrepo and then update the subrepo to the right revision. Does that
make sense? However that does not work for non mercurial subrepos,
does it?

> Since this isn't using match objects, I'm guessing it doesn't work with
> wildcards or other patterns. That needs fixing. Search around for
> 'match' in this file for examples of how to do this.
>
> But this is probably harder than it looks. If you have libfoo (subrepo)
> and libbar (normal) and someone reverts lib*, you've got a problem. So I
> don't think this notion of splitting patterns actually makes sense.
>
> A better approach is probably:
>
> - move the pattern matcher that's built lower down up
> - subs = [s for s in ctx.substate() if match(s)]

Thank you for this advice. This will definitely make this patch much simpler!

I did not know about match objects. I had a little trouble
understanding how to use them at first, but now that (I believe) I do,
I find them quite easy to use and very powerful.

> Never use print in Mercurial. This would be caught by the test suite -
> always run the test suite.

Doh! Some debug code slipped through!

I've never done run the test suite. I'll have to learn how.

>> +    wlock = repo.wlock()
>
> This lock should be in the top-level function. Releasing and retaking a
> lock in the middle of an operation is just as bad as not taking it all.

I don't really know what lock does (though I kind of suspect it) but
it'd be nice to get a short explanation :-)

> And this is the wrongness I figured I'd find above. There's no reason at
> all this should be Mercurial-subrepo specific. I think you should just
> be able to just do:
>
> ctx.sub(sname).get(state, overwrite=True)
>
> Read this, it's the closest thing we have to a fully-documented API in
> the entire system:
>
> http://www.selenic.com/hg/file/c81dce8a7bb6/mercurial/subrepo.py#l269

OK, so if I understood this properly, commands.update() is not
supposed to work with non mercurial subrepos. Instead we must use
subrepo.get(), right?

> I'd like you to approach this in a few patches:
>
> patch 1: add code that finds the subs that match the given pattern, warn
> that they're not reverted
> patch 2: add code that walks the affected subrepos and reverts them

I'm already on it. I think I can make a patch series that will support
patterns and non mercurial subrepos. I may leave support for handling
removed and added subrepos and for not using the --no-backup option
for a later patch, if that is OK.

However I don't think I'll make it before the code freeze. I guess
that means this feature will not make it until mercurial 2.1? :-(

> I think you'll find that both of these can be quite a bit simpler than
> the above and can be done without adding helpers or reindenting.

Thank you _a lot_ for the review and your pointers. The new version of
the patch is shaping up to be way simpler :-)

Cheers,

Angel