[PATCH] revert: add support for reverting subrepos

[Angel Ezquerra]

On Fri, Oct 7, 2011 at 3:04 AM, Greg Ward <greg-hg at gerg.ca> wrote:
> [original patch from angel.ezquerra at gmail.com]
>> # 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
>
> [my feedback]
>> 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? [...]
>
> [Angel replies]
>> 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.
>
> Yup. This is probably the best argument for visual diff tools like
> meld, ediff, or kdiff3.
>
>> 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.
>
> This is definitely best practices around here. Heck, it's best practices
> anywhere, but it's more explicitly enforced when hacking on Mercurial.
> Not only do you get bonus points for cleaning up hairy code, you learn
> your way around the code more by refactoring it. Everyone wins.
>
>> 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?
>
> Err... can I plead "no comment"? I have dabbled in subrepos as a user,
> but never really touched the code. And this is the first time I've ever
> looked at how 'revert' is implemented. I'm pretty sure you know more about
> both than I do.
>
>> 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?
>
> http://mercurial.selenic.com/wiki/MercurialApi#Communicating_with_the_user
>
> My personal guidelines: use ui.status() for something that most users want to
> know most of the time. If they don't like it, they can shut it up with -q.
>
> In contrast, use ui.write() when this output is the raison d'etre of the
> command, e.g. "hg status" or "hg cat". And ui.note() is for something
> that is occasionally interesting, but must be specifically turned on with -v.
>
> As for how these methods interact with TortoiseHg... I have no idea. Sorry.
>
> If you were hoping for more feedback to your patch, it's probably because
> it was too big. Split it up into digestible pieces, and you are
> much more likely to get a useful review.
>
> Greg
>

Thank you Greg!

hopefully the second version of the patch is easier to read. I'll see
if someone on the tortoisehg mailing list can help with the ui.status
issue.

Cheers,

Angel