Merge lp:~guilhem-bichot/bzr/bzr-pull-verbose-shows-tip-before-pull into lp:bzr

In "bzr pull":
- show the tip revision of the destination branch before pulling, if --verbose and something was pulled
- write it to .bzr.log, unconditionally.
Interest: If a pull brings in some bug, the printed revision can serve as a point to revert to;
if a pull brings in some new revisions to review, the printed revision can serve as a low bound for what
revisions to review.
Printing to stdout is convenient; printing to .bzr.log survives a bit longer (useful if you discover a bug in the pulled revisions only after some time).
Could somebody please give some feedback?

Thanks for working on that !

This looks quite good but I've made a few tweaks at
lp:~bzr/bzr/bzr-pull-verbose-shows-tip-before-pull :
- both the revno and the revid are displayed,
- the tests use predefined revids to simplify their writing,
- I remove the test checking the *absence* of the message when not using -v
 (other tests will (or should !) fail if we do),
- I add one checking that the message is not displayed if nothing is pulled

I can add them when merging (they sounded easier to do than to describe but I think
you may be interested), but it would be easier for the second reviewer if you can merge
them to your branch, modifying them as you see fit of course and push the result
(you won't need to resubmit, just leave a comment so we know the proposal has been updated).

Also, can you complete a contributor agreement?
<http://www.canonical.com/contributors>

Finally, if the second reviewer agrees on that patch, it may be worth doing the same for push, but that can be another submission.

Bonjour Vincent. Thanks for the updates, I merged them in my branch (only fixed a typo in NEWS and added a comma there). Regarding "other tests will (or should !) fail if we do" yes two tests would fail as I verified.
I'll get started on the contributor agreement.
Regarding doing the same for "push", do you mean printing the revid of the target branch before the push occurs? Is that info easily available to the push-er?

> Bonjour Vincent. Thanks for the updates, I merged them in my branch (only
> fixed a typo in NEWS and added a comma there).

There are conflicts there but they can be solved at merge time.

> Regarding "other tests will (or
> should !) fail if we do" yes two tests would fail as I verified.

Great, thanks for checking !

> I'll get started on the contributor agreement.

Thanks.

> Regarding doing the same for "push", do you mean printing the revid of the
> target branch before the push occurs? Is that info easily available to the
> push-er?

It should since that's how we know what to push, if it's not available already, a call to last_revision_info on the remote branch should tell you.

But let's land this patch first.

I have a second thought, and suggest that printing revno can be misleading.
A revision can have its revno changed by a pull, for example: I pull the parent branch PB; right after that the tip is the revision of revid R, of revno 10. Some time passes. Someone else merges PB into his own branch and pushes the result into PB (assume no append_revisions_only here; we do that in MySQL - which is a problem but for later). Then in PB, revision of revid R now has a dotted revno as it's not mainline anymore. It's now another revision of PB, S, which has revno 10.
Now I pull PB into my local branch; "bzr pull -v" prints that the tip is (revno 10, revid R). After the pull I find that this pull introduced a bug, I want to revert; if I do:
bzr revert -r revno:10
then I'm going to revert to S, which wasn't the intention: I wanted to revert to the tip before pulling: R.
So I suggest printing only the revid. What do you think?

I think you can get bitten once and you learn how valuable a trunk with
append_revisions_only set is. The case you outline above can only occur
with a branch shared between several devs that doesn't try to preserve
a linear history on that branch.

Trying to revnos in such a context is bound to fail.

As a UI rule, we don't show revids until required, I think this patch deserves
the exception (but it's activated by -v so one can argue that it's required :)
since some branches doesn't have a linear history, but not
displaying the revno at all, will be a violation of this rule.

Trying to *use* revnos in such a context is bound to fail.

Bonjour Vincent. Yes using append_revisions_only would be great. However, it would require changing the years-old habits of 100 people. They would be problems in the beginning; I would have to dissipate quite some energy to explain, re-explain, fix, and that's not something I can do short-term. Even though it comes back regularly on my long-term plans.
Yes our devs should know that revnos are unreliable in our case, and some do know. I was more worried about the case where one forgets about this; it's not so easy to remember that the revno which "bzr pull -v" prints at the end of its execution is already out-of-date when it's printed (i.e. is unusable).
But if there is a UI rule that the revno has to be displayed... case is closed, let's print it :-)

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

GuilhemBichot wrote:
> Bonjour Vincent. Yes using append_revisions_only would be great. However, it would require changing the years-old habits of 100 people. They would be problems in the beginning; I would have to dissipate quite some energy to explain, re-explain, fix, and that's not something I can do short-term. Even though it comes back regularly on my long-term plans.
> Yes our devs should know that revnos are unreliable in our case, and some do know. I was more worried about the case where one forgets about this; it's not so easy to remember that the revno which "bzr pull -v" prints at the end of its execution is already out-of-date when it's printed (i.e. is unusable).
> But if there is a UI rule that the revno has to be displayed... case is closed, let's print it :-)

So one option is to look up the revno *after* pull has completed. It
will be a bit slower if the revno ends up being a merge revision, but
you should be able to do:

old_rev_revno = branch.revision_id_to_dotted_revno(original_revision_id)

That was actually the recommendation back when we discussed this in the
past.

And in fact, we would rather show that then the revision_id (we
generally avoid showing revision-ids in the UI where a revno can be
used.) I think it would be great to log it via mutter(). But showing it
to the user should be avoided.

John
=:->

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

iEYEARECAAYFAkseivMACgkQJdeBCYSNAAMstwCghi7LEaaWs8DWdAffgrsCXvTH
/oEAnRqTtExNqpu6x2EGQ+t3MoFmEI0V
=kKt+
-----END PGP SIGNATURE-----

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

John A Meinel wrote:
> GuilhemBichot wrote:
>> Bonjour Vincent. Yes using append_revisions_only would be great. However, it would require changing the years-old habits of 100 people. They would be problems in the beginning; I would have to dissipate quite some energy to explain, re-explain, fix, and that's not something I can do short-term. Even though it comes back regularly on my long-term plans.
>> Yes our devs should know that revnos are unreliable in our case, and some do know. I was more worried about the case where one forgets about this; it's not so easy to remember that the revno which "bzr pull -v" prints at the end of its execution is already out-of-date when it's printed (i.e. is unusable).
>> But if there is a UI rule that the revno has to be displayed... case is closed, let's print it :-)
>
> So one option is to look up the revno *after* pull has completed. It
> will be a bit slower if the revno ends up being a merge revision, but
> you should be able to do:
>
> old_rev_revno = branch.revision_id_to_dotted_revno(original_revision_id)
>
> That was actually the recommendation back when we discussed this in the
> past.
>
> And in fact, we would rather show that then the revision_id (we
> generally avoid showing revision-ids in the UI where a revno can be
> used.) I think it would be great to log it via mutter(). But showing it
> to the user should be avoided.
>
> John
> =:->
>

You might try:
lp:///~jameinel/bzr/2.1.0b4-pull-v

Which changes it to do exactly as I mentioned. This means that you get:

Tip revision before pull was: 1.1.4

Which means:

1) We don't have to show the revision-id, because you can get back to
the original revno in the new context.
2) Changes mutter to put something like this into .bzr.log:

0.545 In branch C:/Users/jameinel/dev/,tmp/b/ pulled
 from revid:<email address hidden>
   => revid:<email address hidden>

We *could* put the revnos in there, but I'm fine with revids in the log
file.

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

iEYEARECAAYFAksekhMACgkQJdeBCYSNAAPuTACfQxQ6VdUV5Qusp2g1tjoowf86
LOAAn25qylFRDLQhCfgpl8UVcEBQn7C8
=Y+L2
-----END PGP SIGNATURE-----

Hi John. I'm fine with your changes. Two comments:
- do we really need to initialize "result" to None?
- the place where you print to stdout is before listing all pulled revisions (log.show_branch_change()). The output looks like:
  long list of changed files
  tip before pull
  long list of pulled revisions.
I find that the tip gets drawn in the middle of those two lists, and requires a long scrolling up the shell window in order to be found. I liked printing it after the list of pulled revisions, it was easier to spot.

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

GuilhemBichot wrote:
> Hi John. I'm fine with your changes. Two comments:
> - do we really need to initialize "result" to None?
> - the place where you print to stdout is before listing all pulled revisions (log.show_branch_change()). The output looks like:
> long list of changed files
> tip before pull
> long list of pulled revisions.
> I find that the tip gets drawn in the middle of those two lists, and requires a long scrolling up the shell window in order to be found. I liked printing it after the list of pulled revisions, it was easier to spot.

I had originally put it at the very end, but there was a test that
asserts that it is before the list of new revisions (but not before the
old revs?)

We don't need to set result to None, that was done when I put the mutter
in a finally statement, and found it broke when there was a problem with
the pull.

I don't really care where the info goes, feel free to move it around. It
was more about the content of the message than where it was put.

John
=:->

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

iEYEARECAAYFAksesu8ACgkQJdeBCYSNAAMPtwCfcRJikxb3gRNm98zSijcCVrCY
FggAn1JZtUA4BR/j9+KcwWu51uvQfIj8
=vKnt
-----END PGP SIGNATURE-----

I merged your changes, removed "result = None", changed the printing order (not yet ran the testsuite and not yet committed); regarding this comment:
                # Encode target because it may have non-ascii characters, and
                # so can the revids.
                target = target.encode('utf-8')
A simple test says that before running "encode()", target is a unicode string (ok) but result.(old|new)_revid are str. So maybe "so can the revids" should be removed? On the other hand, if it is really a correct comment, shouldn't there be an encode() on those revids before printing them?

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

GuilhemBichot wrote:
> I merged your changes, removed "result = None", changed the printing order (not yet ran the testsuite and not yet committed); regarding this comment:
> # Encode target because it may have non-ascii characters, and
> # so can the revids.
> target = target.encode('utf-8')
> A simple test says that before running "encode()", target is a unicode string (ok) but result.(old|new)_revid are str. So maybe "so can the revids" should be removed? On the other hand, if it is really a correct comment, shouldn't there be an encode() on those revids before printing them?

Revision ids are defined as utf-8 encoded strings. (*never* unicode.) So
we don't want to mix non-ascii utf-8 strings with Unicode as it causes
python to try to decode them as ascii. (Now it turns out that in
practice 99% of all revision-ids are pure ascii, but that isn't the api
definition.)

So the comment stands. :)

mutter() is taking them as 8-bit (utf-8) strings, which should be fine.

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

iEYEARECAAYFAksewRsACgkQJdeBCYSNAANJ7QCffD27+go9zaJtCQlTlu+O2PbC
kykAnRieLvBtdH8WApDFN48Q/aBhDAnu
=2K6c
-----END PGP SIGNATURE-----

Hello John. Thanks much about the explanation about encoding of revids, I had mixed things a bit :-)
This test fails:
./bzr selftest -v bzrlib.tests.blackbox.test_too_much.TestCommands.test_pull_verbose
with this error:
Traceback (most recent call last):
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/tests/blackbox/test_too_much.py", line 150, in test_pull_verbose
    out = self.run_bzr('pull --overwrite --verbose ../a')[0]
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/tests/__init__.py", line 1887, in run_bzr
    working_dir=working_dir,
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/tests/__init__.py", line 1798, in _run_bzr_autosplit
    encoding=encoding, stdin=stdin, working_dir=working_dir,
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/tests/__init__.py", line 1831, in _run_bzr_core
    args)
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/tests/__init__.py", line 2125, in apply_redirected
    return a_callable(*args, **kwargs)
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/commands.py", line 1139, in run_bzr_catch_user_errors
    return run_bzr(argv)
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/commands.py", line 1037, in run_bzr
    ret = run(*run_argv)
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/commands.py", line 654, in run_argv_aliases
    return self.run(**all_cmd_args)
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/builtins.py", line 1034, in run
    result.old_revid)
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/decorators.py", line 140, in read_locked
    result = unbound(self, *args, **kwargs)
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/branch.py", line 346, in revision_id_to_dotted_revno
    return self._do_revision_id_to_dotted_revno(revision_id)
  File "/home/mysql_src/logiciels/bzr_versions/bzr-pull-verbose-shows-tip-before-pull/bzrlib/branch.py", line 366, in _do_revision_id_to_dotted_revno
    raise errors.NoSuchRevision(self, revision_id)
NoSuchRevision: BzrBranch7('file:///tmp/testbzr-JIm8BM.tmp/bzrlib.tests.blackbox.test_too_much.TestCommands.test_pull_verbose/work/b/') has no revision guilhem@guilhem-laptop-20091209125135-072j1altswyqu916

The reason is that this is a pull *--overwrite*, so this new code:
                    old_dotted_revno = branch_to.revision_id_to_dotted_revno(
                        result.old_revid)
fails, because the branch after pull doesn't have this revid anymore (it was overwritten).
I see two solutions:
1) print the revid (even though it's not in the branch anymore, it can still be used to recreate the past branch from another location)
2) print a message saying that the old revision is not there anymore (less helpful but does not violate UI policies).

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

GuilhemBichot wrote:
> Hello John. Thanks much about the explanation about encoding of revids, I had mixed things a bit :-)
> This test fails:
> ./bzr selftest -v bzrlib.tests.blackbox.test_too_much.TestCommands.test_pull_verbose
> with this error:
...

> raise errors.NoSuchRevision(self, revision_id)
> NoSuchRevision: BzrBranch7('file:///tmp/testbzr-JIm8BM.tmp/bzrlib.tests.blackbox.test_too_much.TestCommands.test_pull_verbose/work/b/') has no revision guilhem@guilhem-laptop-20091209125135-072j1altswyqu916
>
> The reason is that this is a pull *--overwrite*, so this new code:
> old_dotted_revno = branch_to.revision_id_to_dotted_revno(
> result.old_revid)
> fails, because the branch after pull doesn't have this revid anymore (it was overwritten).
> I see two solutions:
> 1) print the revid (even though it's not in the branch anymore, it can still be used to recreate the past branch from another location)
> 2) print a message saying that the old revision is not there anymore (less helpful but does not violate UI policies).

Sorry, I meant to test this case and forgot about it by the end of it.

I could go either way. We do have '--show-ids' and 'bzr uncommit' does
show the revid because the revision cannot be accessed by any other means.

So probably show "revid:..." since that is consistent enough with
uncommit and others. (Only show revids when there isn't another way.)

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

iEYEARECAAYFAksfyTsACgkQJdeBCYSNAAPyHwCfeni1WzTaNbXXr1v2M7lmL9dI
ZbUAmwfQ9Cz6ySX0UcVmdrH+aXQ2ZIsT
=p3ak
-----END PGP SIGNATURE-----

Hi John. By "Only show revids when there isn't another way" do you mean that "bzr pull -v" should display the old tip's revid only if this revision isn't present in the branch anymore?
Or do you mean that we should just print revid of old and new tips, and no revnos, no matter if the old revision exists or not? Or something else (if you have an example)?

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

GuilhemBichot wrote:
> Hi John. By "Only show revids when there isn't another way" do you mean that "bzr pull -v" should display the old tip's revid only if this revision isn't present in the branch anymore?
> Or do you mean that we should just print revid of old and new tips, and no revnos, no matter if the old revision exists or not? Or something else (if you have an example)?

Print the dotted revno unless the old tip is no longer in the history,
then display revid:...

Just catch the exception, and print a different message indicating that
the old revision isn't in the ancestry any more, and this is what it was.

I'll put a simple example in my branch:
> === modified file 'bzrlib/builtins.py'
> --- bzrlib/builtins.py 2009-12-08 17:48:04 +0000
> +++ bzrlib/builtins.py 2009-12-09 17:29:36 +0000
> @@ -1031,11 +1031,16 @@
> % (target, result.old_revid, result.new_revid))
> result.report(self.outf)
> if verbose and result.old_revid != result.new_revid:
> - old_dotted_revno = branch_to.revision_id_to_dotted_revno(
> - result.old_revid)
> - old_dotted_revno = '.'.join([str(i) for i in old_dotted_revno])
> - self.outf.write('Tip revision before pull was: %s\n'
> - % (old_dotted_revno,))
> + try:
> + old_revno_in_to = branch_to.revision_id_to_dotted_revno(
> + result.old_revid)
> + old_revstr = '.'.join([str(i) for i in old_revno_in_to])
> + except errors.NoSuchRevision:
> + self.outf.write('Tip revision no longer in ancestry.'
> + ' Was revid:%s\n' % (result.old_revid,))
> + else:
> + self.outf.write('Tip revision before pull was: %s\n'
> + % (old_revstr,))
> log.show_branch_change(
> branch_to, self.outf, result.old_revno,
> result.old_revid)
>
> === modified file 'bzrlib/tests/blackbox/test_pull.py'
> --- bzrlib/tests/blackbox/test_pull.py 2009-12-08 17:41:06 +0000
> +++ bzrlib/tests/blackbox/test_pull.py 2009-12-09 17:29:36 +0000
> @@ -415,6 +415,23 @@
> out = self.run_bzr('pull -d target -v')[0]
> self.assertContainsRe(out, 'Tip revision before pull was: 1.1.2')
>
> + def test_pull_verbose_prints_not_in_ancestry_tip(self):
> + builder = self.make_branch_builder('source')
> + builder.start_series()
> + builder.build_snapshot('A', None, [
> + ('add', ('', 'TREE_ROOT', 'directory', None))])
> + builder.build_snapshot('B', ['A'], [])
> + builder.build_snapshot('C', ['B'], [])
> + builder.build_snapshot('D', ['A', 'B'], [])
> + builder.finish_series()
> + b = builder.get_branch()
> + b.lock_write()
> + self.addCleanup(b.unlock)
> + b.bzrdir.sprout('target', revision_id='C')
> + out = self.run_...

Thanks John. I merged your changes above, only changed the phrasing of "Tip revision no longer in ancestry", like this:
- self.outf.write('Tip revision no longer in ancestry.'
+ self.outf.write('Tip revision before pull is no longer in this branch.'
Tests work fine. I have nothing to add.
By the way, this whole patch has received so many changes from you and Vincent that it would deserve your names in the NEWS entry.

If I understand this, you changed it to put the 'tip revision was' message at the very end. For whatever reason I thought you wanted to put it before all of the log messages.

Anyway, I'm fine with this.

yes, I put it at the end and I forgot to mention this change, sorry. I found that putting it before the log messages would make it almost invisible when there is more than a screen long of log messages (frequent case with pulls that I do).

Well done ! I'll merge.

That is, as soon as we get your contributor agreement:
<http://www.canonical.com/contributors>

yes, let's put this into dormant state until I manage to sign this agreement (need to consult Sun first).

Have you managed to work out an agreement yet? It would be nice to have this in 2.1.

I will have to look at the agreement only later - alas.
Regarding the patch: how about enabling the printout even without -v? I find this is just a line, so not really verbose...

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

GuilhemBichot wrote:
> I will have to look at the agreement only later - alas.
> Regarding the patch: how about enabling the printout even without -v? I find this is just a line, so not really verbose...

I believe other commands (like update) similarly print out what revision
you end up on when the command completes. I think if we incorporate that
so that we have "pulled from X to Y" as the information line, it would
be fine to include that even when -v is not supplied.

A partial concern is that computing a dotted revno is potentially
expensive on large projects. However, it isn't expected to happen often,
so the tradeoff is probably ok.

John
=:->

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

iEYEARECAAYFAktYthkACgkQJdeBCYSNAAMn5QCfYqaqiQIHzuO99hymp0v7uFfz
EOkAnj+d06w7IHwYpKLsEvnLRKtKxmHL
=Y+ko
-----END PGP SIGNATURE-----