[bzr-loom:MERGE] Show current thread in 'status' (fixes bug 232465)

[Jonathan Lange]

On Thu, May 29, 2008 at 10:13 AM, Robert Collins
<robertc at robertcollins.net> wrote:
> On Tue, 2008-05-27 at 10:04 +1000, Jonathan Lange wrote:
>> Hello all,
>>
>> I hacked this one up over the weekend. This branch adds an overlay to
>> 'bzr status' that shows the current thread if you are looking at a
>> loom.
>>
>> Output looks something like:
>>   $ bzr st
>>   Current thread: experiment-with-threads
>>   M barlib/foo.py
>>
>> This has already made my life with looms a lot easier.
>>
>> The code I've added roughly parallels the existing code to overlay the
>> 'switch' command, and the test I've added is blackbox.
>>
>> My biggest worry with the patch is that it slows down the performance
>> of status. I'd appreciate guidance on how to minimize this slowdown.
>
> I think, like pending merges, this should come after the tree status,
> not before.
>
> So:
> tree status
> pending merges
> loom status
>

Done.

> It would be good to also indicate if the loom has pending changes, but
> that can be built up later.
>

+1

> In init, please group the command decoration together (no 2 line VWS,
> this is PEP-8 blessed).
>

It took me a while to parse this. Done.


> +    """Show the status of the working tree, including the current
> thread."""
>
> How about:
>    """Show status for a loom."""
>

Changed.

> Your argument list to status is wrong - you only accept file_list, so
> the others should not be there. (hmm, I'm guessing that the base
> run_argv_aliases is passing all registered options in; if thats so, then
> can I bother you to open a bug on bzr that we need to make this more
> flexible to make command decoration more robust).
>

Accepting just file_list appears to work.

The real issue is that using a plugin to extend an existing command is
tricksy and underdocumented. I can file a bug along those lines if it
would help.

>
> +        if file_list is None:
> +            directory = '.'
> +        else:
> +            directory = file_list[0]
> +        (loom, path) = bzrlib.branch.Branch.open_containing(directory)
> +        branch.require_loom_branch(loom)
> +        loom.lock_read()
> +        try:
> +            print 'Current thread: %s' % loom.nick
> +        finally:
> +            loom.unlock()
>
> You call the path 'directory', but it may be a path (e.g. bzr status
> README). Also, as you don't use path, doing:
> loom, _ = bzrlib.branch.Branch.open_containing(path)
> would make that more clear.
>

Renamed.

> +    def run_argv_aliases(self, argv, alias_argv=None):
>
> I suggest:
>     """Decorate bzr's cmd_status to show status for looms as well."""
>

Added.

> I would like to see a total of three tests for status:
> - status on a non-loom does not error
> - status on a loom with 1 thread
> - status on a loom with 2 threads, showing the output change after
> switching threads.
>

Added.

Thanks for the review. I've attached an updated bundle and a diff of
the changes I've made in reply to your review.

jml
-------------- next part --------------
A non-text attachment was scrubbed...
Name: thread-in-status-232465.patch
Type: text/x-diff
Size: 10434 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080530/7d17f38e/attachment.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: thread-in-status-232465-progress.diff
Type: text/x-diff
Size: 3882 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080530/7d17f38e/attachment-0001.bin