[PATCH] revert: add support for reverting subrepos

Greg Ward greg-hg at gerg.ca
Thu Oct 6 20:04:25 CDT 2011


[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


More information about the Mercurial-devel mailing list