Merge lp:~ed-marsfire/bzr/giveback into lp:bzr

[EdB]

[EdB (ed-marsfire)]

Hi everyone,

This a second try at changing cmd_uncommit so it uses UIFactory for input/output.

Per Martin's initial review, I have fixed the branch so it uses correct line endings. I removed my output collector class and am using ui_factory.make_output_stream() instead. I've changed the command so that the dry run case does not do a "press enter to continue" but instead both dry run and non-dry run do a get_boolean if not forced.

This should be much more to your liking. Review away and let me know what you think!

Cheers,

-Ed

[EdB (ed-marsfire)]

Oh forgot to mention, I've updated my UIFactory so that it has all the new methods, and it handles the make_output_stream nicely, which now maps into my UI. I used this to test the new changes and all output went to my UI. Yay!

[Martin Pool (mbp)]

That looks great to me. Did you try testing it?

--
Martin <http://launchpad.net/~mbp/>

[lp:~ed-marsfire/bzr/giveback]

4990. By Ed Bayiates <email address hidden> on 2010-01-28

Bug fix to previous modifications

[EdB (ed-marsfire)]

I had tested it with my UI and did not notice two small failures, that I just found testing it with Python directly, which I have fixed and pushed a new update. I used this script to test it in Python:

import sys
import bzrlib
import bzrlib.ui
import bzrlib.ui.text
import bzrlib.builtins

bzrlib.ui.ui_factory = bzrlib.ui.text.TextUIFactory(sys.stdin, sys.stdout, sys.stderr)

uncommit = bzrlib.builtins.cmd_uncommit()
uncommit.run_argv_aliases(['c:/testcode/testbranch'])

The newest merge has two small changes and should work fine.

[Martin Pool (mbp)]

I meant to suggest you should try it by running

'bzr selftest uncommit'

--
Martin <http://launchpad.net/~mbp/>

[EdB (ed-marsfire)]

I don't understand. That would run the Windows installed version, not the Python installed in site-packages. Is there some way to have the bzr commandline command use the site-packages? It doesn't right now. I installed the Windows installer to get the bzr command line, and the Python installer to get the Python code so I could integrate with Bazaar.

Just to make sure I tried it and got this error:
bzr: ERROR: No module named tests
You may need to install this Python library separately.

Sorry if I am so ignorant -- though I have 25 years of software development experience, I am very new to Python and Bazaar development.

[John A Meinel (jameinel)]

C:\Python26\python C:\Python26\Scripts\bzr selftest uncommit

However, we often just run bzr directly from the source tree, which would be closer to

C:\Python26\python .\bzr selftest uncommit

Though you may also be running into the dependencies now needed to run the test suite. I usually use 'setuptools' to install and do:

C:\Python26\python C:\Python26\Scripts\easy_install-script.py -Z -O1 testtools

(You haven to have previously installed easy_install, though...)

The other option is:

bzr branch lp:testtools
cd testtools
C:\Python26\python setup.py install
(You need testtools >= 0.9.2)

[Martin Pool (mbp)]

I think this will probably find some test failures because of some
output moving from stdout to stderr. However those things can be a
bit annoying to find and fix, so don't necessarily feel obliged to run
and fix the tests. I, or the patch pilot, will try to fix them up and
merge this sometime soon. It is useful to get set up to run selftest
though, because it does help a lot in making sure your changes are ok.

--
Martin <http://launchpad.net/~mbp/>

[EdB (ed-marsfire)]

Wow, that bzr script is neat! I set up the tests as John explains and got 1 failure, I think from what Martin said it's because of the stdout/stderr thing. Here is the total log:

C:\Python26\Scripts>bzr selftest uncommit
bzr: WARNING: bzrlib version doesn't match the bzr program.
This may indicate an installation problem.
bzrlib from ['C:\\Python26\\lib\\site-packages\\bzrlib'] is version (2, 2, 0, 'dev', 1)
QBzr: skip module bzrlib.plugins.qbzr.lib.tests.test_logmodel because PyQt4 is not installed
QBzr: skip module bzrlib.plugins.qbzr.lib.tests.test_spellcheck because PyQt4 is not installed
QBzr: skip module bzrlib.plugins.qbzr.lib.tests.test_util because PyQt4 is not installed
bzr selftest: C:/Python26/Scripts/bzr
   C:\Python26\lib\site-packages\bzrlib
   bzr-2.2.0dev1 python-2.6.4 Windows-Vista-6.0.6002-SP2

======================================================================
FAIL: bzrlib.tests.blackbox.test_uncommit.TestUncommit.test_uncommit_shows_log_with_revision_id
----------------------------------------------------------------------
_StringException: Text attachment: log
------------
74.777 creating repository in file:///C:/windows/temp/testbzr-lb7tyl.tmp/mit_shows_log_with_revision_id/work/tree/.bzr/.
74.849 creating branch <bzrlib.branch.BzrBranchFormat7 object at 0x06516D70> in file:///C:/windows/temp/testbzr-lb7tyl.tmp/mit_shows_log_with_revision_id/work/tree/.bzr/
74.963 opening working tree 'C:/windows/temp/testbzr-lb7tyl.tmp/mit_shows_log_with_revision_id/work/tree'
75.137 preparing to commit
    INFO Committing to: C:/windows/temp/testbzr-lb7tyl.tmp/mit_shows_log_with_revision_id/work/tree/
75.141 Selecting files for commit with filter None
    INFO added a
    INFO added b
    INFO added c
    INFO Committed revision 1.
75.370 preparing to commit
    INFO Committing to: C:/windows/temp/testbzr-lb7tyl.tmp/mit_shows_log_with_revision_id/work/tree/
75.374 Selecting files for commit with filter None
    INFO modified a
    INFO Committed revision 2.
75.530 run bzr: ['uncommit', '--force']
75.530 bazaar version: 2.2.0dev1
75.530 bzr arguments: ['uncommit', '--force']
75.532 encoding stdout as sys.stdout encoding 'cp1252'
75.536 opening working tree 'C:/windows/temp/testbzr-lb7tyl.tmp/mit_shows_log_with_revision_id/work/tree'
75.601 encoding stdout as sys.stdout encoding 'cp1252'
75.603 Uncommitting from {a2} to {a2}
75.671 output:
' 2 bayiatee\t2010-01-28\n second commit\n\nThe above revision(s) will be removed.\nYou can restore the old tip by running:\n bzr pull . -r revid:a2\n'

------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "C:\Python26\lib\site-packages\testtools\runtest.py",
 line 129, in _run_user
    return fn(*args)
  File "C:\Python26\lib\site-packages\testtools\testcase.py", line 406, in _run_test_method
    testMethod()
  File "C:\Python26\lib\site-packages\bzrlib\tests\blackbox\test_uncommit.py", line 221, in test_uncommit_shows_log_with_revision_id
    self.assertContainsRe(err, r'You can restore the old tip by running')
AssertionError: pattern "You can restore the old tip by...

[Ian Clatworthy (ian-clatworthy)]

EdB wrote:
> Wow, that bzr script is neat! I set up the tests as John explains and got 1 failure, I think from what Martin said it's because of the stdout/stderr thing. Here is the total log:

Well done!

> C:\Python26\Scripts>bzr selftest uncommit
[snip]
> FAILED (failures=1)

Given you've come this far, I assume you're keen to try fixing this
yourself? If you need assistance, just let us know.

Ian C.
(Patch pilot this week)

[EdB (ed-marsfire)]

Do you want me to fix the test, meaning its expectations? Or do you want me to fix the functionality, meaning make sure that message goes to stderr?

[Martin Pool (mbp)]

2010/1/29 EdB <email address hidden>:
> Do you want me to fix the test, meaning its expectations?  Or do you want me to fix the functionality, meaning make sure that message goes to stderr?

I think it's reasonable that the user interaction start going to
stderr (which I think your patch will do) but the test needs to be
updated to allow this.

--
Martin <http://launchpad.net/~mbp/>

[Martin Pool (mbp)]

I'll have a go at fixing up the tests.

[Martin Pool (mbp)]

Here's the change I made to the test:

1- some output is expected on stdout not stderr
2- change from direct string comparison to a scriptrunner which makes it easier to read (imo)

I'll submit this.

Thanks, and good luck with the other commands.

=== modified file 'bzrlib/tests/blackbox/test_uncommit.py'
--- bzrlib/tests/blackbox/test_uncommit.py 2009-10-06 14:40:37 +0000
+++ bzrlib/tests/blackbox/test_uncommit.py 2010-02-05 15:45:32 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2008 Canonical Ltd
+# Copyright (C) 2005, 2006, 2008, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -22,6 +22,7 @@
 from bzrlib.bzrdir import BzrDirMetaFormat1
 from bzrlib.errors import BzrError, BoundBranchOutOfDate
 from bzrlib.tests import TestCaseWithTransport
+from bzrlib.tests.script import ScriptRunner

 class TestUncommit(TestCaseWithTransport):
@@ -215,11 +216,18 @@

     def test_uncommit_shows_log_with_revision_id(self):
         wt = self.create_simple_tree()
-
- out, err = self.run_bzr('uncommit --force', working_dir='tree')
- self.assertContainsRe(out, r'second commit')
- self.assertContainsRe(err, r'You can restore the old tip by running')
- self.assertContainsRe(err, r'bzr pull . -r revid:a2')
+
+ script = ScriptRunner()
+ script.run_script(self, """
+$ cd tree
+$ bzr uncommit --force
+ 2 Martin Pool\t2010-02-05
+ second commit
+...
+The above revision(s) will be removed.
+You can restore the old tip by running:
+ bzr pull . -r revid:a2
+""")

     def test_uncommit_octopus_merge(self):
         # Check that uncommit keeps the pending merges in the same order

review: Approve

[Martin Pool (mbp)]

This fails because of insufficient test isolation. I'll have a look at it but maybe not today.

 File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/script.py", line 233, in _check_output
   test_case.assertEqualDiff(expected, actual)
AssertionError: texts not equal:
- 2 Martin Pool 2010-02-05
+ 2 Patch Queue Manager 2010-02-05
       second commit
- ...
+
 The above revision(s) will be removed.
 You can restore the old tip by running:
   bzr pull . -r revid:a2

------------

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-01-22 14:32:37 +0000
+++ bzrlib/builtins.py	2010-01-28 15:58:14 +0000
@@ -4719,11 +4719,12 @@
                 rev_id = b.get_rev_id(revno)
 
         if rev_id is None or _mod_revision.is_null(rev_id):
-            self.outf.write('No revisions to uncommit.\n')
+            ui.ui_factory.note('No revisions to uncommit.')
             return 1
 
+        log_collector = ui.ui_factory.make_output_stream()
         lf = log_formatter('short',
-                           to_file=self.outf,
+                           to_file=log_collector,
                            show_timezone='original')
 
         show_log(b,
@@ -4734,23 +4735,21 @@
                  end_revision=last_revno)
 
         if dry_run:
-            print 'Dry-run, pretending to remove the above revisions.'
-            if not force:
-                val = raw_input('Press <enter> to continue')
+            ui.ui_factory.note('Dry-run, pretending to remove the above revisions.')
         else:
-            print 'The above revision(s) will be removed.'
-            if not force:
-                val = raw_input('Are you sure [y/N]? ')
-                if val.lower() not in ('y', 'yes'):
-                    print 'Canceled'
-                    return 0
+            ui.ui_factory.note('The above revision(s) will be removed.')
+
+        if not force:
+            if not ui.ui_factory.get_boolean('Are you sure [y/N]? '):
+                ui.ui_factory.note('Canceled')
+                return 0
 
         mutter('Uncommitting from {%s} to {%s}',
                last_rev_id, rev_id)
         uncommit(b, tree=tree, dry_run=dry_run, verbose=verbose,
                  revno=revno, local=local)
-        note('You can restore the old tip by running:\n'
-             '  bzr pull . -r revid:%s', last_rev_id)
+        ui.ui_factory.note('You can restore the old tip by running:\n'
+             '  bzr pull . -r revid:%s' % last_rev_id)
 
 
 class cmd_break_lock(Command):