Code review comment for lp:~mbp/bzr/45719-update-r

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/45719-update-r into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #45719 update command cannot take a revision number
> https://bugs.launchpad.net/bugs/45719
>
>
> This adds a 'bzr update -r' option. If the branch is bound to another, it always updates from the master branch first. This updated form of the patch addresses John's review comments in <https://bugs.edge.launchpad.net/bzr/+bug/45719/comments/20>.
>
> This is an updated version of what was one of the longest-standing merges, and it's for a five-digit bug too <https://bugs.edge.launchpad.net/bzr/+bug/45719>.
>

v- does 'change_reporter' need to be inside the try/except ? I think it
just got refactored into there.

+ try:
+ change_reporter = delta._ChangeReporter(
+ unversioned_filter=tree.is_ignored,
+ view_info=view_info)
+ conflicts = tree.update(
+ change_reporter,
+ possible_transports=possible_transports,
+ revision=rev,
+ old_tip=old_tip)
+ except errors.NoSuchRevision, e:
+ raise errors.BzrCommandError(
+ "branch has no revision %s\n"
+ "bzr update --revision only works"
+ " for a revision in the branch
history"
+ % (e.revision))

+ if revision is not None:
+ rev = revision[0].as_revision_id(branch)
+ else:
+ rev = branch.last_revision()

^- It might be clearer as 'rev_id' or 'revision_id = '. We have some
ambiguous naming (is it a Revision, revision_id, or RevSpec...) However,
this is really minor.

 review: approve
 merge: approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksw26oACgkQJdeBCYSNAAO00gCeOtH7vUpSUdkD8xTtoyOgod/g
sdIAn3ABG2q0RkqjrPIhgsm9lv5V5Ofs
=IrMf
-----END PGP SIGNATURE-----