Merge lp:~salgado/bzr/bug-308122 into lp:bzr

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

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]
    def assertIn(self, needle, haystack):
        """Assert that 'needle' is in 'haystack'."""
        self.assertTrue(
            needle in haystack, '%r not in %r' % (needle, haystack))

--
Guilherme Salgado <email address hidden>

[John A Meinel (jameinel)]

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_diff=False/True).

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_factory.make_output_stream()". Since that will mean that progress bars won't interfere with the diff output.

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')

review: Needs Fixing

[Guilherme Salgado (salgado)]

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_diff=False/True).
>
> 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_factory.make_output_stream()". Since that will mean
> 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.make_output_stream() if one is not given?

>
> 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)]

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_diff=False/True).
>
> 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_factory.make_output_stream()". Since that will mean
> 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.make_output_stream() if one is not given?

>
> 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)]

I agree with John's comments.

If you made it 'unshelve --preview' that would be consistent with 'merge --preview'.

review: Needs Fixing

[Guilherme Salgado (salgado)]

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.make_output_stream().

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.change_reporter, through tree_merger.make_preview_transform()),
so we need to silence it. Is it ok to silence it by stubbing out
merger.change_reporter.report inside Unshelver.write_diff()?

How does that sound?

[John A Meinel (jameinel)]

-----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.make_output_stream().

I'm not sure whether it should be on run() or as a constructor argument.

  unshelver = Unshelver.from_args(..., to_file=self.outf)
or
  Unshelver.run(to_file=self.outf)

>
> 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.change_reporter, through tree_merger.make_preview_transform()),
> so we need to silence it. Is it ok to silence it by stubbing out
> merger.change_reporter.report inside Unshelver.write_diff()?
>
> 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://enigmail.mozdev.org/

iEYEARECAAYFAksw3LgACgkQJdeBCYSNAAPNEQCgwuFfp0gNiX1GEd4HES4FJOpQ
rtgAnAkieOugIyCwcix9jK9IAz8szaCK
=0DyW
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

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.make_output_stream().
>
> I'm not sure whether it should be on run() or as a constructor argument.
>
>  unshelver = Unshelver.from_args(..., to_file=self.outf)
> or
>  Unshelver.run(to_file=self.outf)

I haven't looked at the implementation yet but maybe it would be
better to actually have Unshelver.preview(to_file=...)

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

[John A Meinel (jameinel)]

-----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.make_output_stream().
>> I'm not sure whether it should be on run() or as a constructor argument.
>>
>> unshelver = Unshelver.from_args(..., to_file=self.outf)
>> or
>> Unshelver.run(to_file=self.outf)
>
> I haven't looked at the implementation yet but maybe it would be
> better to actually have Unshelver.preview(to_file=...)
>

so the Unshelver decides what is going to do in the
"Unshelver.from_args" which takes an 'action'. (dry-run, delete, and now
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://enigmail.mozdev.org/

iEYEARECAAYFAksxhmsACgkQJdeBCYSNAAOq1wCglzDzRv5c1W1BaZy84PZ0dY3m
18UAoJJggi4ldHawXVYcdNCQer9xuqcv
=WZNo
-----END PGP SIGNATURE-----

[Guilherme Salgado (salgado)]

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.make_output_stream().
>
> I'm not sure whether it should be on run() or as a constructor argument.
>
> unshelver = Unshelver.from_args(..., to_file=self.outf)
> or
> Unshelver.run(to_file=self.outf)

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.change_reporter, through tree_merger.make_preview_transform()),
> > so we need to silence it. Is it ok to silence it by stubbing out
> > merger.change_reporter.report inside Unshelver.write_diff()?
> >
> > 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)]

-----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_to.getvalue()
self.assertContainsRe(content, '-a\n-j\n\\+z\n\\+y)

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://enigmail.mozdev.org/

iEYEARECAAYFAktE1joACgkQJdeBCYSNAAPLAACaAiybr87yIYSvCRS6xoH6FoKe
CpwAoNNxhOh76yFLoWk9gB6OZB0xzMKU
=9IQe
-----END PGP SIGNATURE-----

[Guilherme Salgado (salgado)]

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_to.getvalue()
> self.assertContainsRe(content, '-a\n-j\n\\+z\n\\+y)

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.assertContainsRe(diff, '-a\n\+z')
   self.assertContainsRe(diff, '-j\n\+y')

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)]

-----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_to.getvalue()
>> self.assertContainsRe(content, '-a\n-j\n\\+z\n\\+y)
>
> 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.assertContainsRe(diff, '-a\n\+z')
> self.assertContainsRe(diff, '-j\n\+y')
>
> 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.assertEqualDiff(expected, content[-len(expected):])

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://enigmail.mozdev.org/

iEYEARECAAYFAktE+iYACgkQJdeBCYSNAAMELACgsohjUD61JUUe9wglDkk0MV8R
cEMAni703d9K+R4TRRwaXFSHubGRLjtK
=rcgh
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

On Wed, 2010-01-06 at 20:42 +0000, Guilherme Salgado wrote:
self.assertThat(output.getvalue(), DocTestMatches("""=== modified file
'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)]

On Wed, 2010-01-06 at 21:21 +0000, Robert Collins wrote:

Oh yeah -
from testtools.matchers import DocTestMatches
import doctest

> self.assertThat(output.getvalue(), DocTestMatches("""=== modified file

...

-Rob

[Guilherme Salgado (salgado)]

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.assertEqualDiff(expected, content[-len(expected):])
>
> 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)]

It looks like you've addressed our concerns, so I'll land this for 2.1.0rc1.

review: Approve

[John A Meinel (jameinel)]

So I went to merge this, but I got a failure in:
======================================================================
ERROR: test_unshelve (bzrlib.tests.test_shelf_ui.TestUnshelveScripts)
----------------------------------------------------------------------
_StringException: Text attachment: log
------------
1693.261 creating repository in file:///tmp/testbzr-hEnBHb.tmp/bzrlib.tests.test_shelf_ui.TestUnshelveScripts.test_unshelve/work/tree/.bzr/.
1693.264 creating branch <bzrlib.branch.BzrBranchFormat7 object at 0x34a3890> in file:///tmp/testbzr-hEnBHb.tmp/bzrlib.tests.test_shelf_ui.TestUnshelveScripts.test_unshelve/work/tree/.bzr/
1693.274 trying to create missing lock '/tmp/testbzr-hEnBHb.tmp/bzrlib.tests.test_shelf_ui.TestUnshelveScripts.test_unshelve/work/tree/.bzr/checkout/dirstate'
1693.275 opening working tree '/tmp/testbzr-hEnBHb.tmp/bzrlib.tests.test_shelf_ui.TestUnshelveScripts.test_unshelve/work/tree'
1693.284 preparing to commit
    INFO Committing to: /tmp/testbzr-hEnBHb.tmp/bzrlib.tests.test_shelf_ui.TestUnshelveScripts.test_unshelve/work/tree/
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.get_user_encoding() 'UTF-8'
------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line 368, in _run_test_method
    testMethod()
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/test_shelf_ui.py", line 484, in test_unshelve
    shelf_ui.Unshelver(tree, manager, 1, True, True, True).run()
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/shelf_ui.py", line 462, in __init__
    self.write_diff_to = ui.ui_factory.make_output_stream()
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/ui/__init__.py", line 149, in make_output_stream
    out_stream = self._make_output_stream_explicit(encoding, encoding_type)
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/ui/__init__.py", line 153, in _make_output_stream_explicit
    raise NotImplementedError("%s doesn't support make_output_stream"
NotImplementedError: SilentUIFactory doesn't support make_output_stream
------------

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_stream' connected to stdout, but we just haven't done it yet.

review: Needs Fixing

[John A Meinel (jameinel)]

So I was able to generally fix the test suite failures with this patch:
=== modified file 'bzrlib/shelf_ui.py'
--- 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 @@
         self.read_shelf = read_shelf
         self.show_diff = show_diff
         self.write_diff_to = write_diff_to
- if self.write_diff_to is None:
- self.write_diff_to = ui.ui_factory.make_output_stream()

     def run(self):
         """Perform the unshelving operation."""
@@ -497,6 +495,8 @@
         tree_merger = merger.make_merger()
         tt = tree_merger.make_preview_transform()
         new_tree = tt.get_preview_tree()
+ if self.write_diff_to is None:
+ self.write_diff_to = ui.ui_factory.make_output_stream()
         diff.show_diff_trees(merger.this_tree, new_tree, self.write_diff_to)
         tt.finalize()

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.

review: Approve

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-12-23 05:42:33 +0000
+++ bzrlib/builtins.py	2010-01-07 14:06:26 +0000
@@ -5829,6 +5829,8 @@
             enum_switch=False, value_switches=True,
             apply="Apply changes and remove from the shelf.",
             dry_run="Show changes, but do not apply or remove them.",
+            preview="Instead of unshelving the changes, show the diff that "
+                    "would result from unshelving.",
             delete_only="Delete changes without applying them.",
             keep="Apply changes but don't delete them.",
         )

=== modified file 'bzrlib/shelf_ui.py'
--- bzrlib/shelf_ui.py	2009-12-15 17:57:26 +0000
+++ bzrlib/shelf_ui.py	2010-01-07 14:06:26 +0000
@@ -22,7 +22,6 @@
 
 from bzrlib import (
     builtins,
-    commands,
     delta,
     diff,
     errors,
@@ -383,16 +382,18 @@
     """Unshelve changes into a working tree."""
 
     @classmethod
-    def from_args(klass, shelf_id=None, action='apply', directory='.'):
+    def from_args(klass, shelf_id=None, action='apply', directory='.',
+                  write_diff_to=None):
         """Create an unshelver from commandline arguments.
 
-        The returned shelver wil have a tree that is locked and should
+        The returned shelver will have a tree that is locked and should
         be unlocked.
 
         :param shelf_id: Integer id of the shelf, as a string.
         :param action: action to perform.  May be 'apply', 'dry-run',
-            'delete'.
+            'delete', 'preview'.
         :param directory: The directory to unshelve changes into.
+        :param write_diff_to: See Unshelver.__init__().
         """
         tree, path = workingtree.WorkingTree.open_containing(directory)
         tree.lock_tree_write()
@@ -410,9 +411,14 @@
             apply_changes = True
             delete_shelf = True
             read_shelf = True
+            show_diff = False
             if action == 'dry-run':
                 apply_changes = False
                 delete_shelf = False
+            elif action == 'preview':
+                apply_changes = False
+                delete_shelf = False
+                show_diff = True
             elif action == 'delete-only':
                 apply_changes = False
                 read_shelf = False
@@ -423,10 +429,11 @@
             tree.unlock()
             raise
         return klass(tree, manager, shelf_id, apply_changes, delete_shelf,
-                     read_shelf)
+                     read_shelf, show_diff, write_diff_to)
 
     def __init__(self, tree, manager, shelf_id, apply_changes=True,
-                 delete_shelf=True, read_shelf=True):
+                 delete_shelf=True, read_shelf=True, show_diff=False,
+                 write_diff_to=None):
         """Constructor.
 
         :param tree: The working tree to unshelve into.
@@ -436,6 +443,11 @@
             working tree.
         :param delete_shelf: If True, delete the changes from the shelf.
         :param read_shelf: If True, read the changes from the shelf.
+        :param show_diff: If True, show the diff that would result from
+            unshelving the changes.
+        :param write_diff_to: A file-like object where the diff will be
+            written to. If None, ui.ui_factory.make_output_stream() will
+            be used.
         """
         self.tree = tree
         manager = tree.get_shelf_manager()
@@ -444,6 +456,10 @@
         self.apply_changes = apply_changes
         self.delete_shelf = delete_shelf
         self.read_shelf = read_shelf
+        self.show_diff = show_diff
+        self.write_diff_to = write_diff_to
+        if self.write_diff_to is None:
+            self.write_diff_to = ui.ui_factory.make_output_stream()
 
     def run(self):
         """Perform the unshelving operation."""
@@ -463,6 +479,8 @@
                     merger.change_reporter = change_reporter
                     if self.apply_changes:
                         merger.do_merge()
+                    elif self.show_diff:
+                        self.write_diff(merger)
                     else:
                         self.show_changes(merger)
                 finally:
@@ -474,6 +492,14 @@
             for cleanup in reversed(cleanups):
                 cleanup()
 
+    def write_diff(self, merger):
+        """Write this operation's diff to self.write_diff_to."""
+        tree_merger = merger.make_merger()
+        tt = tree_merger.make_preview_transform()
+        new_tree = tt.get_preview_tree()
+        diff.show_diff_trees(merger.this_tree, new_tree, self.write_diff_to)
+        tt.finalize()
+
     def show_changes(self, merger):
         """Show the changes that this operation specifies."""
         tree_merger = merger.make_merger()

=== modified file 'bzrlib/tests/test_shelf_ui.py'
--- bzrlib/tests/test_shelf_ui.py	2009-12-16 15:56:25 +0000
+++ bzrlib/tests/test_shelf_ui.py	2010-01-07 14:06:26 +0000
@@ -18,6 +18,7 @@
 from cStringIO import StringIO
 import os
 import sys
+from textwrap import dedent
 
 from bzrlib import (
     errors,
@@ -504,6 +505,38 @@
         self.assertFileEqual(LINES_AJ, 'tree/foo')
         self.assertEqual(1, tree.get_shelf_manager().last_shelf())
 
+    def test_unshelve_args_preview(self):
+        tree = self.create_tree_with_shelf()
+        write_diff_to = StringIO()
+        unshelver = shelf_ui.Unshelver.from_args(
+            directory='tree', action='preview', write_diff_to=write_diff_to)
+        try:
+            unshelver.run()
+        finally:
+            unshelver.tree.unlock()
+        # The changes were not unshelved.
+        self.assertFileEqual(LINES_AJ, 'tree/foo')
+        self.assertEqual(1, tree.get_shelf_manager().last_shelf())
+
+        # But the diff was written to write_diff_to.
+        diff = write_diff_to.getvalue()
+        expected = dedent("""\
+            @@ -1,4 +1,4 @@
+            -a
+            +z
+             b
+             c
+             d
+            @@ -7,4 +7,4 @@
+             g
+             h
+             i
+            -j
+            +y
+
+            """)
+        self.assertEqualDiff(expected, diff[-len(expected):])
+
     def test_unshelve_args_delete_only(self):
         tree = self.make_branch_and_tree('tree')
         manager = tree.get_shelf_manager()