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

[Angel Ezquerra Moreu]

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:
>
>> On Mon, Mar 26, 2012 at 9:23 AM, Martin Geisler <mg at aragost.com> wrote:
>>> Angel Ezquerra <angel.ezquerra at gmail.com> writes:
>>>
>>>> +                # 1. if the no_backup is not set, revert all modified files inside the subrepo
>>>> +                # 2. update the subrepo to the revision specified in the corresponding substate dictionary
>>>> +
>>>> +                # the revert function name is shadowed
>>>
>>> The lines are very long here and check-code will complain.
>>
>> I did not know about check code. Actually I had heard of it on this
>> list, but I had not used it before. Thanks for the pointer.
>
> Please read
>
>  http://mercurial.selenic.com/wiki/ContributingChanges
>  http://mercurial.selenic.com/wiki/CodingStyle
>
> if you haven't ready read them.
>
>>>> +                        if revert_opts['all']:
>>>> +                            revert(ui, ctx.sub(sname)._repo, **revert_opts)
>>>> +                        else:
>>>> +                            revert(ui, ctx.sub(sname)._repo, 'set:modified()', **revert_opts)
>>>
>>> The _repo field is private and I don't think this will work for
>>> non-hg subrepos?
>>>
>>> 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.

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.

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

Thanks,

Angel