Code review comment for lp:~gagern/bzr/bug560030-include-bash-completion-plugin

I'm going to assume that someone else has done a more thorough review of the bash side of things, as I don't really know how things integrate together.

The test infrastructure looks decent.

I do wonder about using "os.access('/bin/bash', X_OK)" rather than just:

subprocess.Popen(['bash', '-c', 'echo hello'], shell=True)

Partially because I just tested it, and under Windows if you have a bash.exe in your path, this succeeds. And in theory, that means that you could run the tests on all platforms, rather than requiring a specific path to be available.

That can be updated/added later, though.

Note that if I hack in that 'bash' is the path that can be accessed, most of the tests pass on Windows except for a couple. However, they're really quite slow:

...lugins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_commit_dashm OK 2163ms
...plugins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_global_opts OK 3039ms
...ugins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_init_format_2 OK 1719ms
...ins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_init_format_any OK 1786ms
...b.plugins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_init_opts OK 1808ms
...lugins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_simple_scipt OK 65ms
...gins.bash_completion.tests.test_bashcomp.TestBashCompletion.test_status_negated OK 1822ms
..._completion.tests.test_bashcomp.TestBashCompletionInvoking.test_revspec_tag_all FAIL 3407ms
    Text attachment: log
------------
15.616 creating repository in file:///C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp/.bzr/.
15.632 creating branch <bzrlib.branch.BzrBranchFormat7 object at 0x01FA9110> in file:///C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp/
15.725 opening working tree 'C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp'
15.999 creating repository in file:///C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp/nInvoking.test_revspec_tag_all/work/.bzr/.
16.180 creating branch <bzrlib.branch.BzrBranchFormat6 object at 0x033F5E50> in file:///C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp/nInvoking.test_revspec_tag_all/work/
16.292 opening working tree 'C:/users/jameinel/appdata/local/temp/testbzr-06_rhc.tmp/nInvoking.test_revspec_tag_all/work'
------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "c:\Python26\lib\site-packages\testtools-0.9.2-py2.6.egg\testtools\runtest.py", line 128, in _run_user
    return fn(*args)
  File "c:\Python26\lib\site-packages\testtools-0.9.2-py2.6.egg\testtools\testcase.py", line 368, in _run_test_method
    testMethod()
  File "C:\Users\jameinel\dev\bzr\jam-integration\bzrlib\plugins\bash_completion\tests\test_bashcomp.py", line 197, in test_revspec_tag_all
    self.assertCompletionEquals('tag1', 'tag2', '3tag')
  File "C:\Users\jameinel\dev\bzr\jam-integration\bzrlib\plugins\bash_completion\tests\test_bashcomp.py", line 97, in assertCompletionEquals
    self.assertEqual(set(words), self.completion_result)
AssertionError: not equal:
a = set(['3tag', 'tag1', 'tag2'])
b = set()

I'm certainly hesitant to be adding tests that take multiple seconds to run. Though it may just be a win32 bash thing. Can someone else run "bzr selftest -s bp.bash" and let me know?

The README clearly doesn't apply as-is anymore.

The basic concept seems ok. And we can likely merge this as-is. Though it would be nice to have it play a bit better with other things first.