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

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>