[PATCH 2 of 3 RFC] revert: add support for reverting subrepos without --no-backup and/or --all

[Martin Geisler]

Angel Ezquerra Moreu <angel.ezquerra at gmail.com> writes:

> On Wed, Mar 28, 2012 at 10:28 AM, Martin Geisler <mg at aragost.com> wrote:
>> Angel Ezquerra Moreu <angel.ezquerra at gmail.com> writes:
>>
>>> 2. Is it ok to use an "opts" dict on the cmdutil.revert code? That
>>> is, is it ok to basically copy commands.revert into cmdutil.revert,
>>> and have commands.revert simply call cmdutil.revert?
>>>
>>> commands.revert takes quite a bit of options and converting all of
>>> them into proper cmdutil.revert named parameters may be tedious
>>> (usage wise, when calling cmdutil.revert from subrepo.revert).
>>
>> Yeah, I guess that's fine. Sometimes the commands do some up-front
>> checking of the command line flags to make sure they are valid. The
>> addremove function does this before calling scmutil.addremove. That
>> kind of logic should not be in cmdutil -- it's okay for that function
>> to assume that its callers know what they're doing.
>
> This is also the case for commands.revert(). I can keep the upfront
> checking in commands.revert and move the rest of the code into
> cmdutil.revert() as you suggest.
>
> The only small problem is that in order to do this upfront checking
> the code needs to initialize several variables that are later used to
> perform the actual revert operation: ctx, node, parent, p2.

I see... the following is my thought after looking at this for 5
minuttes. So I might have overlooked something :)

> Out of these ctx seems the most expensive to get, since it requires a
> call to:
>
>    ctx = scmutil.revsingle(repo, opts.get('rev'))

So ctx is the target revision we're reverting back to.

I think this would be an argument to cmdutil.revert in any case:
commands.revert does error checking and computes an initial ctx,
subrepo.hgsubrepo.revert will compute a subrepo-specific ctx for it's
own subrepo and pass that to cmdutil.revert.

> which internally calls revrange() which may need to parse a revset.
>
> The others are quite trivial to get (although parent and p2 require a
> call to some "dirstate._validate()" method).

The two parent variables will probably also be something that
subrepo.hgsubrepo.revert will want to pass to cmdutil.revert, right?

The cmdutil.revert code need to revert some or all file in a Mercurial
repo to a specific revision. If seems that you need to know the target
revision (ctx) and the current parent revisions (parent and p2) to do
this efficiently.

> Do you think that I need to avoid initializing these variables twice
> (once in commands.revert() and once in cmdutil.revert())?
>
> As I see it I have 4 options:
>
> 1. Initialize all those values twice.
> 2. Initialize all those values once, in commands.revert() and pass
> them to cmdutil.revert() as parameters.
> 3. Initialize the ctx variable once in commands.revert(), pass it to
> cmdutil.revert() as a parameter, calculate the rest of the variables
> twice.
> 4. Move of the checking into cmdutil.revert (initializing the required
> variables just once in cmdutil.revert).
>
> Which one do you think is best? I ask this in advance to avoid an
> extra patch roundtrip... :-) Personally I'd probably go with #3.

It sounds like #2 is best, and that #3 is another possibility if we can
just lookup the parents as needed in cmdutil.revert.

-- 
Martin Geisler

aragost Trifork
Professional Mercurial support
http://www.aragost.com/mercurial/