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

[Angel Ezquerra]

On Wed, Mar 28, 2012 at 5:43 PM, Martin Geisler <mg at aragost.com> wrote:
> 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/

I just sent an updated patch series following your advice (except that
I did not pass the "node" variable, since it is so cheap to get it
when needed).

Comments are welcome as usual! :-)

Angel