Merge lp:~salgado/bzr/bug-308122 into lp:bzr
- bug-308122
- Merge into bzr.dev
Status: | Merged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Merged at revision: | not available | ||||||||
Proposed branch: | lp:~salgado/bzr/bug-308122 | ||||||||
Merge into: | lp:bzr | ||||||||
Diff against target: |
173 lines (+67/-6) 3 files modified
bzrlib/builtins.py (+2/-0) bzrlib/shelf_ui.py (+32/-6) bzrlib/tests/test_shelf_ui.py (+33/-0) |
||||||||
To merge this branch: | bzr merge lp:~salgado/bzr/bug-308122 | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
John A Meinel | Approve | ||
Martin Pool | Needs Fixing | ||
Review via email: mp+16430@code.launchpad.net |
Commit message
Description of the change
Guilherme Salgado (salgado) wrote : | # |
John A Meinel (jameinel) wrote : | # |
I think the helper would be nice, but not critical. I've wanted it from time to time as well.
I think that displaying the diff vs just the overview should probably be a flag on Unshelver, though it may just always get set by the same command-line flag. Something like:
show_changes(self, merger, include_
This may be a YAGNI, but it seems like better layering. Arguably if we show the diff, we *shouldn't* show the summary.
Actually, I think my bigger concern is writing directly to stdout. Consider that something like "bzr qshelve" might really like this functionality, I think we should pass in the file to write to.
And if you did that, you wouldn't need the "replace_stdout()" helper.
Looking at cmd_unshelve, though I see why this is slightly harder. cmd_unshelve defers everything to 'Unshelver.run()' which doesn't take anything like an output file.
I think it should, though. Another possibility is that we should at least use "ui.ui_
I'm a little concerned about the UI impact. In that this changes 'unshelve --dry-run' to *always* show the diff, when it did not used to. (--dry-run --verbose?). I personally think always showing the diff is probably fine, as I don't really see the short form being very informative. (Consider if we changed 'bzr st' to suddenly be equivalent to 'bzr diff')
Guilherme Salgado (salgado) wrote : | # |
On Mon, 2009-12-21 at 17:21 +0000, John A Meinel wrote:
> Review: Needs Fixing
> I think the helper would be nice, but not critical. I've wanted it
> from time to time as well.
>
> I think that displaying the diff vs just the overview should probably
> be a flag on Unshelver, though it may just always get set by the same
> command-line flag. Something like:
>
> show_changes(self, merger, include_
>
> This may be a YAGNI, but it seems like better layering. Arguably if we
> show the diff, we *shouldn't* show the summary.
Sounds like a YAGNI to me, but I wouldn't mind doing it.
>
> Actually, I think my bigger concern is writing directly to stdout.
> Consider that something like "bzr qshelve" might really like this
> functionality, I think we should pass in the file to write to.
Yeah, that might be a problem indeed.
>
> And if you did that, you wouldn't need the "replace_stdout()" helper.
>
> Looking at cmd_unshelve, though I see why this is slightly harder.
> cmd_unshelve defers everything to 'Unshelver.run()' which doesn't take
> anything like an output file.
Is it a problem if I change Unshelver.run() to accept an (optional) file
descriptor and pass it to show_changes(), which would write to it,
falling back to sys.stdout when the fd is not passed?
One caveat I see is that that fd would be used only to write the diff,
whereas the rest of the output would still be written to stdout (or
whatever's specified in the ui_factory that's in use, IIUC).
>
> I think it should, though. Another possibility is that we should at
> least use "ui.ui_
> that progress bars won't interfere with the diff output.
Ok, so show_changes() would write to the given fd, falling back to
ui_factory.
>
> I'm a little concerned about the UI impact. In that this changes
> 'unshelve --dry-run' to *always* show the diff, when it did not used
> to. (--dry-run --verbose?). I personally think always showing the diff
> is probably fine, as I don't really see the short form being very
> informative. (Consider if we changed 'bzr st' to suddenly be
> equivalent to 'bzr diff')
Given that "unshelve --dry-run" is currently not so useful, I'd be in
favour of just changing its behaviour and, if people complain, change it
to only output the diff when --verbose is specified. In practice,
though, I wouldn't mind having to pass an extra --verbose to see what's
in my shelved changes, as long as there's a way to do that.
Another option would be to create another command (show-shelve?) which
just outputs the diff.
--
Guilherme Salgado <email address hidden>
Guilherme Salgado (salgado) wrote : | # |
On Mon, 2009-12-21 at 17:21 +0000, John A Meinel wrote:
> Review: Needs Fixing
> I think the helper would be nice, but not critical. I've wanted it
> from time to time as well.
>
> I think that displaying the diff vs just the overview should probably
> be a flag on Unshelver, though it may just always get set by the same
> command-line flag. Something like:
>
> show_changes(self, merger, include_
>
> This may be a YAGNI, but it seems like better layering. Arguably if we
> show the diff, we *shouldn't* show the summary.
Sounds like a YAGNI to me, but I wouldn't mind doing it.
>
> Actually, I think my bigger concern is writing directly to stdout.
> Consider that something like "bzr qshelve" might really like this
> functionality, I think we should pass in the file to write to.
Yeah, that might be a problem indeed.
>
> And if you did that, you wouldn't need the "replace_stdout()" helper.
>
> Looking at cmd_unshelve, though I see why this is slightly harder.
> cmd_unshelve defers everything to 'Unshelver.run()' which doesn't take
> anything like an output file.
Is it a problem if I change Unshelver.run() to accept an (optional) file
descriptor and pass it to show_changes(), which would write to it,
falling back to sys.stdout when the fd is not passed?
One caveat I see is that that fd would be used only to write the diff,
whereas the rest of the output would still be written to stdout (or
whatever's specified in the ui_factory that's in use, IIUC).
>
> I think it should, though. Another possibility is that we should at
> least use "ui.ui_
> that progress bars won't interfere with the diff output.
Ok, so show_changes() would write to the given fd, falling back to
ui_factory.
>
> I'm a little concerned about the UI impact. In that this changes
> 'unshelve --dry-run' to *always* show the diff, when it did not used
> to. (--dry-run --verbose?). I personally think always showing the diff
> is probably fine, as I don't really see the short form being very
> informative. (Consider if we changed 'bzr st' to suddenly be
> equivalent to 'bzr diff')
Given that "unshelve --dry-run" is currently not so useful, I'd be in
favour of just changing its behaviour and, if people complain, change it
to only output the diff when --verbose is specified. In practice,
though, I wouldn't mind having to pass an extra --verbose to see what's
in my shelved changes, as long as there's a way to do that.
Another option would be to create another command (show-shelve?) which
just outputs the diff.
--
Guilherme Salgado <email address hidden>
Martin Pool (mbp) wrote : | # |
I agree with John's comments.
If you made it 'unshelve --preview' that would be consistent with 'merge --preview'.
Guilherme Salgado (salgado) wrote : | # |
So, to summarize, this is how it's going to look like, in the end:
1. cmd_unshelve will grow a 'preview' action
2. Unshelver.run() will accept an optional file descriptor, where a diff
will be written when the preview action is specified. When the fd is not
given it will fall back to ui_factory.
3. Unshelver will grow a write_diff(merger, diff_output=None) method, to
be called when the 'preview' action is specified.
4. The preview action will cause the diff to be written to the fd, but
we don't want to output the changes (implicitly done by
merger.
so we need to silence it. Is it ok to silence it by stubbing out
merger.
How does that sound?
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Guilherme Salgado wrote:
> So, to summarize, this is how it's going to look like, in the end:
>
> 1. cmd_unshelve will grow a 'preview' action
>
> 2. Unshelver.run() will accept an optional file descriptor, where a diff
> will be written when the preview action is specified. When the fd is not
> given it will fall back to ui_factory.
I'm not sure whether it should be on run() or as a constructor argument.
unshelver = Unshelver.
or
Unshelver.
>
> 3. Unshelver will grow a write_diff(merger, diff_output=None) method, to
> be called when the 'preview' action is specified.
>
Seems reasonable.
> 4. The preview action will cause the diff to be written to the fd, but
> we don't want to output the changes (implicitly done by
> merger.
> so we need to silence it. Is it ok to silence it by stubbing out
> merger.
>
> How does that sound?
>
I don't like the idea of stubbing it out, as it seems brittle at best.
If it is going to be difficult, then I would just say let it do the
report and don't worry about it. (One *can* think of it as showing the
diffstat before the diff...)
I *think* that if we want to properly silence it, then we should just
set the change reporter to the NullChangeReporter at the appropriate
time. If that isn't obvious, then I would punt and just let the output
get written to the terminal.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAks
rtgAnAkieOugIyC
=0DyW
-----END PGP SIGNATURE-----
Martin Pool (mbp) wrote : | # |
2009/12/23 John A Meinel <email address hidden>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Guilherme Salgado wrote:
>> So, to summarize, this is how it's going to look like, in the end:
>>
>> 1. cmd_unshelve will grow a 'preview' action
>>
>> 2. Unshelver.run() will accept an optional file descriptor, where a diff
>> will be written when the preview action is specified. When the fd is not
>> given it will fall back to ui_factory.
>
> I'm not sure whether it should be on run() or as a constructor argument.
>
> unshelver = Unshelver.
> or
> Unshelver.
I haven't looked at the implementation yet but maybe it would be
better to actually have Unshelver.
--
Martin <http://
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Martin Pool wrote:
> 2009/12/23 John A Meinel <email address hidden>:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Guilherme Salgado wrote:
>>> So, to summarize, this is how it's going to look like, in the end:
>>>
>>> 1. cmd_unshelve will grow a 'preview' action
>>>
>>> 2. Unshelver.run() will accept an optional file descriptor, where a diff
>>> will be written when the preview action is specified. When the fd is not
>>> given it will fall back to ui_factory.
>> I'm not sure whether it should be on run() or as a constructor argument.
>>
>> unshelver = Unshelver.
>> or
>> Unshelver.
>
> I haven't looked at the implementation yet but maybe it would be
> better to actually have Unshelver.
>
so the Unshelver decides what is going to do in the
"Unshelver.
preview.)
I believe this is because Unshelver is meant to be a UI class, and not a
lower level implementation class. As in, you probably can't really
re-use Unshelver in gui code, as it is an implementation of a text ui
itself.
Which feels like we are missing the non-ui code (how *should* qunshelve
interact with the shelf if not via Unshelver.)
One hint to that is that the class is in "bzrlib.shelf_ui" and not
"bzrlib.shelf".
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAks
18UAoJJggi4ldHa
=WZNo
-----END PGP SIGNATURE-----
Guilherme Salgado (salgado) wrote : | # |
On Tue, 2009-12-22 at 14:51 +0000, John A Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Guilherme Salgado wrote:
> > So, to summarize, this is how it's going to look like, in the end:
> >
> > 1. cmd_unshelve will grow a 'preview' action
> >
> > 2. Unshelver.run() will accept an optional file descriptor, where a diff
> > will be written when the preview action is specified. When the fd is not
> > given it will fall back to ui_factory.
>
> I'm not sure whether it should be on run() or as a constructor argument.
>
> unshelver = Unshelver.
> or
> Unshelver.
I think the former would be nicer, but only because we already have the
show_diff argument on __init__(), so I did that change.
>
> >
> > 3. Unshelver will grow a write_diff(merger, diff_output=None) method, to
> > be called when the 'preview' action is specified.
> >
>
> Seems reasonable.
>
> > 4. The preview action will cause the diff to be written to the fd, but
> > we don't want to output the changes (implicitly done by
> > merger.
> > so we need to silence it. Is it ok to silence it by stubbing out
> > merger.
> >
> > How does that sound?
> >
>
> I don't like the idea of stubbing it out, as it seems brittle at best.
> If it is going to be difficult, then I would just say let it do the
> report and don't worry about it. (One *can* think of it as showing the
> diffstat before the diff...)
>
> I *think* that if we want to properly silence it, then we should just
> set the change reporter to the NullChangeReporter at the appropriate
> time. If that isn't obvious, then I would punt and just let the output
> get written to the terminal.
I could do that, but I'd prefer to just let the changes get written to
the terminal if you don't see that as a problem.
I've just pushed these changes, but I'm not quite happy with the
multiple assertions in the same test method to make sure the diff is
correct, so I'd appreciate suggestions to make it better.
--
Guilherme Salgado <email address hidden>
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
...
>
> I could do that, but I'd prefer to just let the changes get written to
> the terminal if you don't see that as a problem.
Having discussed it a bit with Aaron, it seems that shelf_ui.Unshelver
is meant to be only used for command-line operation, while
shelf.Unshelver is the class meant to be used for guis, etc. (The actual
functionality is supposed to be in the latter, managing it from a CLI is
meant to be in the former.)
As such, we wouldn't have to pass the output file after all, as it is
okay for Unshelver to determine this itself.
That said, it is certainly easier to test it, if we allow it to be
passed in. :)
>
> I've just pushed these changes, but I'm not quite happy with the
> multiple assertions in the same test method to make sure the diff is
> correct, so I'd appreciate suggestions to make it better.
>
>
The first thing I would do is copy the "dry_run" test rather than
replacing it completely. Since we still have '--dry-run' functionality.
I would then probably not use "lines = ....split()" and instead do
something like:
content = write_diff_
self.assertCont
I would generally have recommended "assertEqualDiff" as I think it makes
the prettiest failures, etc, but you have some timestamps, etc, in the
output that we don't want the test to worry about.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
CpwAoNNxhOh76yF
=9IQe
-----END PGP SIGNATURE-----
Guilherme Salgado (salgado) wrote : | # |
On Wed, 2010-01-06 at 18:30 +0000, John A Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> ...
>
> >
> > I could do that, but I'd prefer to just let the changes get written to
> > the terminal if you don't see that as a problem.
>
> Having discussed it a bit with Aaron, it seems that shelf_ui.Unshelver
> is meant to be only used for command-line operation, while
> shelf.Unshelver is the class meant to be used for guis, etc. (The actual
> functionality is supposed to be in the latter, managing it from a CLI is
> meant to be in the former.)
>
> As such, we wouldn't have to pass the output file after all, as it is
> okay for Unshelver to determine this itself.
>
> That said, it is certainly easier to test it, if we allow it to be
> passed in. :)
Given that it'll probably be used only for testing, I think it'd make
sense to have only run() and write_diff() accept the optional file-like
object instead of having it in from_args(), __init__() and as an
instance variable. Do you agree?
>
>
> >
> > I've just pushed these changes, but I'm not quite happy with the
> > multiple assertions in the same test method to make sure the diff is
> > correct, so I'd appreciate suggestions to make it better.
> >
> >
>
> The first thing I would do is copy the "dry_run" test rather than
> replacing it completely. Since we still have '--dry-run' functionality.
>
Erm, I really didn't mean to do that. I'd forgotten that was an
existing test and then just changed it to suit my needs.
> I would then probably not use "lines = ....split()" and instead do
> something like:
>
> content = write_diff_
> self.assertCont
That's a nice helper, but I couldn't get a regexp to match the diff. I
thought '-a\n\+z.*-j\n\+y' would, but it doesn't. After playing around
with that for some time I gave up and went with
self.
self.
Following is the actual diff, in case it's of any help.
=== modified file 'foo'
--- a/foo2010-01-06 20:20:00 +0000
+++ b/foo2010-01-06 20:20:00 +0000
@@ -1,4 +1,4 @@
-a
+z
b
c
d
@@ -7,4 +7,4 @@
g
h
i
-j
+y
--
Guilherme Salgado <email address hidden>
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Guilherme Salgado wrote:
> On Wed, 2010-01-06 at 18:30 +0000, John A Meinel wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>>
>> ...
>>
>>> I could do that, but I'd prefer to just let the changes get written to
>>> the terminal if you don't see that as a problem.
>> Having discussed it a bit with Aaron, it seems that shelf_ui.Unshelver
>> is meant to be only used for command-line operation, while
>> shelf.Unshelver is the class meant to be used for guis, etc. (The actual
>> functionality is supposed to be in the latter, managing it from a CLI is
>> meant to be in the former.)
>>
>> As such, we wouldn't have to pass the output file after all, as it is
>> okay for Unshelver to determine this itself.
>>
>> That said, it is certainly easier to test it, if we allow it to be
>> passed in. :)
>
> Given that it'll probably be used only for testing, I think it'd make
> sense to have only run() and write_diff() accept the optional file-like
> object instead of having it in from_args(), __init__() and as an
> instance variable. Do you agree?
>
At this point, I don't care either way. Do what feels good to you :).
>>
>>> I've just pushed these changes, but I'm not quite happy with the
>>> multiple assertions in the same test method to make sure the diff is
>>> correct, so I'd appreciate suggestions to make it better.
>>>
>>>
>> The first thing I would do is copy the "dry_run" test rather than
>> replacing it completely. Since we still have '--dry-run' functionality.
>>
>
> Erm, I really didn't mean to do that. I'd forgotten that was an
> existing test and then just changed it to suit my needs.
>
>> I would then probably not use "lines = ....split()" and instead do
>> something like:
>>
>> content = write_diff_
>> self.assertCont
>
> That's a nice helper, but I couldn't get a regexp to match the diff. I
> thought '-a\n\+z.*-j\n\+y' would, but it doesn't. After playing around
> with that for some time I gave up and went with
>
> self.assertCont
> self.assertCont
>
> Following is the actual diff, in case it's of any help.
>
> === modified file 'foo'
> --- a/foo2010-01-06 20:20:00 +0000
> +++ b/foo2010-01-06 20:20:00 +0000
> @@ -1,4 +1,4 @@
> -a
> +z
> b
> c
> d
> @@ -7,4 +7,4 @@
> g
> h
> i
> -j
> +y
>
>
>
>
The main issue is that regexes don't really like to match across lines.
A different possibility would be:
expected = """\
@@ -1,4 +1,4 @@
- -a
+z
b
c
d
@@ -7,4 +7,4 @@
g
h
i
- -j
+y
"""
self.assertEqua
eg, match the last bytes of content and make sure they match what you
expect.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
cEMAni703d9K+
=rcgh
-----END PGP SIGNATURE-----
Robert Collins (lifeless) wrote : | # |
On Wed, 2010-01-06 at 20:42 +0000, Guilherme Salgado wrote:
self.assertThat
'foo'
--- a/foo...
+++ b/foo...
@@ -1,4 +1,4 @@
-a
+z
b
c
d
@@ -7,4 +7,4 @@
g
h
i
-j
+y
""", doctest.ELLIPSIS)
-Rob
Robert Collins (lifeless) wrote : | # |
On Wed, 2010-01-06 at 21:21 +0000, Robert Collins wrote:
Oh yeah -
from testtools.matchers import DocTestMatches
import doctest
> self.assertThat
...
-Rob
Guilherme Salgado (salgado) wrote : | # |
On Wed, 2010-01-06 at 21:03 +0000, John A Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Guilherme Salgado wrote:
> > On Wed, 2010-01-06 at 18:30 +0000, John A Meinel wrote:
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >>
> >> ...
> >>
> >>> I could do that, but I'd prefer to just let the changes get written to
> >>> the terminal if you don't see that as a problem.
> >> Having discussed it a bit with Aaron, it seems that shelf_ui.Unshelver
> >> is meant to be only used for command-line operation, while
> >> shelf.Unshelver is the class meant to be used for guis, etc. (The actual
> >> functionality is supposed to be in the latter, managing it from a CLI is
> >> meant to be in the former.)
> >>
> >> As such, we wouldn't have to pass the output file after all, as it is
> >> okay for Unshelver to determine this itself.
> >>
> >> That said, it is certainly easier to test it, if we allow it to be
> >> passed in. :)
> >
> > Given that it'll probably be used only for testing, I think it'd make
> > sense to have only run() and write_diff() accept the optional file-like
> > object instead of having it in from_args(), __init__() and as an
> > instance variable. Do you agree?
> >
>
> At this point, I don't care either way. Do what feels good to you :).
Ok, I'll leave it as is, then.
>
>
> The main issue is that regexes don't really like to match across lines.
> A different possibility would be:
>
> expected = """\
> @@ -1,4 +1,4 @@
> - -a
> +z
> b
> c
> d
> @@ -7,4 +7,4 @@
> g
> h
> i
> - -j
> +y
> """
>
> self.assertEqua
>
> eg, match the last bytes of content and make sure they match what you
> expect.
That works; I just used dedent() so that I could have the multi-line
string properly indented.
Shall I re-submit this m-p?
--
Guilherme Salgado <email address hidden>
John A Meinel (jameinel) wrote : | # |
It looks like you've addressed our concerns, so I'll land this for 2.1.0rc1.
John A Meinel (jameinel) wrote : | # |
So I went to merge this, but I got a failure in:
=======
ERROR: test_unshelve (bzrlib.
-------
_StringException: Text attachment: log
------------
1693.261 creating repository in file://
1693.264 creating branch <bzrlib.
1693.274 trying to create missing lock '/tmp/testbzr-
1693.275 opening working tree '/tmp/testbzr-
1693.284 preparing to commit
INFO Committing to: /tmp/testbzr-
1693.287 Selecting files for commit with filter None
INFO added foo
INFO Committed revision 1.
INFO Selected changes:
INFO M foo
INFO Changes shelved with id "1".
1693.311 encoding stdout as osutils.
------------
Text attachment: traceback
------------
Traceback (most recent call last):
File "/usr/lib/
return fn(*args)
File "/usr/lib/
testMethod()
File "/home/
shelf_
File "/home/
self.
File "/home/
out_stream = self._make_
File "/home/
raise NotImplementedE
NotImplementedE
------------
I believe to test this you need to run "bzr selftest >to_file.txt".
Also, I think Martin has agreed that SilentUIFactory *should* provide 'make_output_
John A Meinel (jameinel) wrote : | # |
So I was able to generally fix the test suite failures with this patch:
=== modified file 'bzrlib/
--- bzrlib/shelf_ui.py 2010-01-12 22:36:23 +0000
+++ bzrlib/shelf_ui.py 2010-01-12 22:51:31 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008 Canonical Ltd
+# Copyright (C) 2008, 2009, 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
@@ -458,8 +458,6 @@
- if self.write_diff_to is None:
- self.write_diff_to = ui.ui_factory.
def run(self):
"""Perform the unshelving operation."""
@@ -497,6 +495,8 @@
tt = tree_merger.
new_tree = tt.get_
+ if self.write_diff_to is None:
+ self.write_diff_to = ui.ui_factory.
Namely, instead of grabbing the output_stream unconditionally, only grab it when we actually go to write to the output file. Which actually makes reasonable sense, we don't really need to hold open a file handle that we'll never write to.
I'm submitting this now. Hopefully it will go through this time.
Preview Diff
1 | === modified file 'bzrlib/builtins.py' | |||
2 | --- bzrlib/builtins.py 2009-12-23 05:42:33 +0000 | |||
3 | +++ bzrlib/builtins.py 2010-01-07 14:06:26 +0000 | |||
4 | @@ -5829,6 +5829,8 @@ | |||
5 | 5829 | enum_switch=False, value_switches=True, | 5829 | enum_switch=False, value_switches=True, |
6 | 5830 | apply="Apply changes and remove from the shelf.", | 5830 | apply="Apply changes and remove from the shelf.", |
7 | 5831 | dry_run="Show changes, but do not apply or remove them.", | 5831 | dry_run="Show changes, but do not apply or remove them.", |
8 | 5832 | preview="Instead of unshelving the changes, show the diff that " | ||
9 | 5833 | "would result from unshelving.", | ||
10 | 5832 | delete_only="Delete changes without applying them.", | 5834 | delete_only="Delete changes without applying them.", |
11 | 5833 | keep="Apply changes but don't delete them.", | 5835 | keep="Apply changes but don't delete them.", |
12 | 5834 | ) | 5836 | ) |
13 | 5835 | 5837 | ||
14 | === modified file 'bzrlib/shelf_ui.py' | |||
15 | --- bzrlib/shelf_ui.py 2009-12-15 17:57:26 +0000 | |||
16 | +++ bzrlib/shelf_ui.py 2010-01-07 14:06:26 +0000 | |||
17 | @@ -22,7 +22,6 @@ | |||
18 | 22 | 22 | ||
19 | 23 | from bzrlib import ( | 23 | from bzrlib import ( |
20 | 24 | builtins, | 24 | builtins, |
21 | 25 | commands, | ||
22 | 26 | delta, | 25 | delta, |
23 | 27 | diff, | 26 | diff, |
24 | 28 | errors, | 27 | errors, |
25 | @@ -383,16 +382,18 @@ | |||
26 | 383 | """Unshelve changes into a working tree.""" | 382 | """Unshelve changes into a working tree.""" |
27 | 384 | 383 | ||
28 | 385 | @classmethod | 384 | @classmethod |
30 | 386 | def from_args(klass, shelf_id=None, action='apply', directory='.'): | 385 | def from_args(klass, shelf_id=None, action='apply', directory='.', |
31 | 386 | write_diff_to=None): | ||
32 | 387 | """Create an unshelver from commandline arguments. | 387 | """Create an unshelver from commandline arguments. |
33 | 388 | 388 | ||
35 | 389 | The returned shelver wil have a tree that is locked and should | 389 | The returned shelver will have a tree that is locked and should |
36 | 390 | be unlocked. | 390 | be unlocked. |
37 | 391 | 391 | ||
38 | 392 | :param shelf_id: Integer id of the shelf, as a string. | 392 | :param shelf_id: Integer id of the shelf, as a string. |
39 | 393 | :param action: action to perform. May be 'apply', 'dry-run', | 393 | :param action: action to perform. May be 'apply', 'dry-run', |
41 | 394 | 'delete'. | 394 | 'delete', 'preview'. |
42 | 395 | :param directory: The directory to unshelve changes into. | 395 | :param directory: The directory to unshelve changes into. |
43 | 396 | :param write_diff_to: See Unshelver.__init__(). | ||
44 | 396 | """ | 397 | """ |
45 | 397 | tree, path = workingtree.WorkingTree.open_containing(directory) | 398 | tree, path = workingtree.WorkingTree.open_containing(directory) |
46 | 398 | tree.lock_tree_write() | 399 | tree.lock_tree_write() |
47 | @@ -410,9 +411,14 @@ | |||
48 | 410 | apply_changes = True | 411 | apply_changes = True |
49 | 411 | delete_shelf = True | 412 | delete_shelf = True |
50 | 412 | read_shelf = True | 413 | read_shelf = True |
51 | 414 | show_diff = False | ||
52 | 413 | if action == 'dry-run': | 415 | if action == 'dry-run': |
53 | 414 | apply_changes = False | 416 | apply_changes = False |
54 | 415 | delete_shelf = False | 417 | delete_shelf = False |
55 | 418 | elif action == 'preview': | ||
56 | 419 | apply_changes = False | ||
57 | 420 | delete_shelf = False | ||
58 | 421 | show_diff = True | ||
59 | 416 | elif action == 'delete-only': | 422 | elif action == 'delete-only': |
60 | 417 | apply_changes = False | 423 | apply_changes = False |
61 | 418 | read_shelf = False | 424 | read_shelf = False |
62 | @@ -423,10 +429,11 @@ | |||
63 | 423 | tree.unlock() | 429 | tree.unlock() |
64 | 424 | raise | 430 | raise |
65 | 425 | return klass(tree, manager, shelf_id, apply_changes, delete_shelf, | 431 | return klass(tree, manager, shelf_id, apply_changes, delete_shelf, |
67 | 426 | read_shelf) | 432 | read_shelf, show_diff, write_diff_to) |
68 | 427 | 433 | ||
69 | 428 | def __init__(self, tree, manager, shelf_id, apply_changes=True, | 434 | def __init__(self, tree, manager, shelf_id, apply_changes=True, |
71 | 429 | delete_shelf=True, read_shelf=True): | 435 | delete_shelf=True, read_shelf=True, show_diff=False, |
72 | 436 | write_diff_to=None): | ||
73 | 430 | """Constructor. | 437 | """Constructor. |
74 | 431 | 438 | ||
75 | 432 | :param tree: The working tree to unshelve into. | 439 | :param tree: The working tree to unshelve into. |
76 | @@ -436,6 +443,11 @@ | |||
77 | 436 | working tree. | 443 | working tree. |
78 | 437 | :param delete_shelf: If True, delete the changes from the shelf. | 444 | :param delete_shelf: If True, delete the changes from the shelf. |
79 | 438 | :param read_shelf: If True, read the changes from the shelf. | 445 | :param read_shelf: If True, read the changes from the shelf. |
80 | 446 | :param show_diff: If True, show the diff that would result from | ||
81 | 447 | unshelving the changes. | ||
82 | 448 | :param write_diff_to: A file-like object where the diff will be | ||
83 | 449 | written to. If None, ui.ui_factory.make_output_stream() will | ||
84 | 450 | be used. | ||
85 | 439 | """ | 451 | """ |
86 | 440 | self.tree = tree | 452 | self.tree = tree |
87 | 441 | manager = tree.get_shelf_manager() | 453 | manager = tree.get_shelf_manager() |
88 | @@ -444,6 +456,10 @@ | |||
89 | 444 | self.apply_changes = apply_changes | 456 | self.apply_changes = apply_changes |
90 | 445 | self.delete_shelf = delete_shelf | 457 | self.delete_shelf = delete_shelf |
91 | 446 | self.read_shelf = read_shelf | 458 | self.read_shelf = read_shelf |
92 | 459 | self.show_diff = show_diff | ||
93 | 460 | self.write_diff_to = write_diff_to | ||
94 | 461 | if self.write_diff_to is None: | ||
95 | 462 | self.write_diff_to = ui.ui_factory.make_output_stream() | ||
96 | 447 | 463 | ||
97 | 448 | def run(self): | 464 | def run(self): |
98 | 449 | """Perform the unshelving operation.""" | 465 | """Perform the unshelving operation.""" |
99 | @@ -463,6 +479,8 @@ | |||
100 | 463 | merger.change_reporter = change_reporter | 479 | merger.change_reporter = change_reporter |
101 | 464 | if self.apply_changes: | 480 | if self.apply_changes: |
102 | 465 | merger.do_merge() | 481 | merger.do_merge() |
103 | 482 | elif self.show_diff: | ||
104 | 483 | self.write_diff(merger) | ||
105 | 466 | else: | 484 | else: |
106 | 467 | self.show_changes(merger) | 485 | self.show_changes(merger) |
107 | 468 | finally: | 486 | finally: |
108 | @@ -474,6 +492,14 @@ | |||
109 | 474 | for cleanup in reversed(cleanups): | 492 | for cleanup in reversed(cleanups): |
110 | 475 | cleanup() | 493 | cleanup() |
111 | 476 | 494 | ||
112 | 495 | def write_diff(self, merger): | ||
113 | 496 | """Write this operation's diff to self.write_diff_to.""" | ||
114 | 497 | tree_merger = merger.make_merger() | ||
115 | 498 | tt = tree_merger.make_preview_transform() | ||
116 | 499 | new_tree = tt.get_preview_tree() | ||
117 | 500 | diff.show_diff_trees(merger.this_tree, new_tree, self.write_diff_to) | ||
118 | 501 | tt.finalize() | ||
119 | 502 | |||
120 | 477 | def show_changes(self, merger): | 503 | def show_changes(self, merger): |
121 | 478 | """Show the changes that this operation specifies.""" | 504 | """Show the changes that this operation specifies.""" |
122 | 479 | tree_merger = merger.make_merger() | 505 | tree_merger = merger.make_merger() |
123 | 480 | 506 | ||
124 | === modified file 'bzrlib/tests/test_shelf_ui.py' | |||
125 | --- bzrlib/tests/test_shelf_ui.py 2009-12-16 15:56:25 +0000 | |||
126 | +++ bzrlib/tests/test_shelf_ui.py 2010-01-07 14:06:26 +0000 | |||
127 | @@ -18,6 +18,7 @@ | |||
128 | 18 | from cStringIO import StringIO | 18 | from cStringIO import StringIO |
129 | 19 | import os | 19 | import os |
130 | 20 | import sys | 20 | import sys |
131 | 21 | from textwrap import dedent | ||
132 | 21 | 22 | ||
133 | 22 | from bzrlib import ( | 23 | from bzrlib import ( |
134 | 23 | errors, | 24 | errors, |
135 | @@ -504,6 +505,38 @@ | |||
136 | 504 | self.assertFileEqual(LINES_AJ, 'tree/foo') | 505 | self.assertFileEqual(LINES_AJ, 'tree/foo') |
137 | 505 | self.assertEqual(1, tree.get_shelf_manager().last_shelf()) | 506 | self.assertEqual(1, tree.get_shelf_manager().last_shelf()) |
138 | 506 | 507 | ||
139 | 508 | def test_unshelve_args_preview(self): | ||
140 | 509 | tree = self.create_tree_with_shelf() | ||
141 | 510 | write_diff_to = StringIO() | ||
142 | 511 | unshelver = shelf_ui.Unshelver.from_args( | ||
143 | 512 | directory='tree', action='preview', write_diff_to=write_diff_to) | ||
144 | 513 | try: | ||
145 | 514 | unshelver.run() | ||
146 | 515 | finally: | ||
147 | 516 | unshelver.tree.unlock() | ||
148 | 517 | # The changes were not unshelved. | ||
149 | 518 | self.assertFileEqual(LINES_AJ, 'tree/foo') | ||
150 | 519 | self.assertEqual(1, tree.get_shelf_manager().last_shelf()) | ||
151 | 520 | |||
152 | 521 | # But the diff was written to write_diff_to. | ||
153 | 522 | diff = write_diff_to.getvalue() | ||
154 | 523 | expected = dedent("""\ | ||
155 | 524 | @@ -1,4 +1,4 @@ | ||
156 | 525 | -a | ||
157 | 526 | +z | ||
158 | 527 | b | ||
159 | 528 | c | ||
160 | 529 | d | ||
161 | 530 | @@ -7,4 +7,4 @@ | ||
162 | 531 | g | ||
163 | 532 | h | ||
164 | 533 | i | ||
165 | 534 | -j | ||
166 | 535 | +y | ||
167 | 536 | |||
168 | 537 | """) | ||
169 | 538 | self.assertEqualDiff(expected, diff[-len(expected):]) | ||
170 | 539 | |||
171 | 507 | def test_unshelve_args_delete_only(self): | 540 | def test_unshelve_args_delete_only(self): |
172 | 508 | tree = self.make_branch_and_tree('tree') | 541 | tree = self.make_branch_and_tree('tree') |
173 | 509 | manager = tree.get_shelf_manager() | 542 | manager = tree.get_shelf_manager() |
This branch fixes bug 317896 by changing Unshelver. show_changes( ) to
write the diff to stdout. It also removes an unused import from
bzrlib/shelf_ui.py
I left an XXX in there because a helper method used in my test was
copied from test_osutils.py, and I think it should be moved to a common
base class, but I'm not sure which one.
Also, my test uses a bunch of self.assertTrue ('foo' in l) assertions,
which would benefit from a custom assertion method like LP's assertIn()
[1]. Would something like that be a nice addition to bzr's custom
TestCase class?
[1]
self.assertTru e(
def assertIn(self, needle, haystack):
"""Assert that 'needle' is in 'haystack'."""
needle in haystack, '%r not in %r' % (needle, haystack))
--
Guilherme Salgado <email address hidden>