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

-- 
Martin Geisler

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