[PATCH] revert: add support for reverting subrepos

[Angel Ezquerra]

On Wed, Oct 5, 2011 at 3:05 AM, Greg Ward <greg-hg at gerg.ca> wrote:
> On Tue, Oct 4, 2011 at 11:58 AM, Angel Ezquerra
> <angel.ezquerra at gmail.com> wrote:
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra at gmail.com>
>> # Date 1317712886 -7200
>> # Node ID 52bdfa484615d27cf82d537f7f09414c7a058c8c
>> # Parent  6dc67dced8c122f6139ae20ccdc03a6b11e8b765
>> revert: add support for reverting subrepos
>
> I found this patch very hard to read, and I think it's simply because
> you indented/unindented huge blocks of code by adding
> if/else/try/while/etc
> around them. Perhaps it would be easier to review if you factored out some
> worker functions first? Or maybe get most of revert() out of
> commands.py -- it's pretty big already, and you're making it bigger.
> I'm not sure where it should
> go; hg.py seems reasonable given that that's where update and merge live,
> but there's already something called revert() in hg.py. Hmmmm.
>
> Greg

Greg,

thank you for your comments. It is quite unfortunate that diff has no
way to mark and indentation change as such, and instead just marks
each indented line as changed, and generally results in a big and
complex diff whenever you change the indentation of your code.

I also felt that the revert function is already quite big, but I did
not want to change it much since this is one of my first attempts at
contributing to mercurial. Perhaps I could make a patch series in
which I first refactor some of the revert function without changing
its functionality and then I add my changes on top of that? For
example I could put the code that walks through the regular files
reverting them into its own function.

Other than that, would you mind to comment on the actual approach that
the patch takes to implement subrepo revert? That is, do you think
that updating the subrepo is the right approach? What about the rest
of the issues that I mention on the patch commit message? For example,
am I using the ui object right when I call ui.status?

Cheers,

Angel