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

[Angel Ezquerra Moreu]

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:
>
>> On Mon, Mar 26, 2012 at 10:29 AM, Martin Geisler <mg at aragost.com> wrote:
>>> Angel Ezquerra Moreu <angel.ezquerra at gmail.com> writes:
>>>
>>>>> When I implemented the --subrepos flags originally, I made sure to
>>>>> let the right subclass of abstractsubrepo in subrepo.py manage the
>>>>> recursion. That way the hgsubrepo class can trigger further
>>>>> recursive calls by calling back to the existing implementation (see
>>>>> hgsubrepo.status) and the gitsubrepo class can implement the logic
>>>>> it needs and stop the recursion.
>>>>
>>>> Are you suggesting that I should add a "revert" method to the
>>>> subrepo classes, which would, if implemented, call commands.revert?
>>>
>>> Yes, more or less. I think the core revert code should go from
>>> commands.py to cmdutil.py first so that hgsubrepo can easily reuse
>>> the logic it needs.
>>
>> Martin,
>>
>> moving the subrepo revert logic into a subrepo class method as you
>> suggested makes the code much simpler. Thanks.
>
> Cool! :)
>
>> I got a couple of questions regarding your previous feedback:
>>
>> 1. If I am to move most or all the revert code into cmdutil, would it
>> make sense to do that on its own patch, in which no code is changed at
>> all, and then modify the code to add subrepo support on a separate
>> patch? I think it would make the review easier.
>
> Yes, please make your patches easy to review! (of course)
>
> Having a patch that just moves the code with no functional changes (no
> changes to the test suite) would be great.
>
>> 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.

Out of these ctx seems the most expensive to get, since it requires a call to:

   ctx = scmutil.revsingle(repo, opts.get('rev'))

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).

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.

Angel