Code review comment for lp:~salgado/bzr/bug-308122

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