Re: [PATCH v5 14/16] merge-recursive: offer an option to retain the output in 'obuf'

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 2, 2016, 08:01 UTC
Message-ID: <alpine.DEB.2.20.1608020948220.79248@virtualbox>
In-Reply-To: <xmqqbn1cjogd.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Mon, 1 Aug 2016, Junio C Hamano wrote:

Show 16 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I actually now see how this would work well for "reason 2)".  If a
> >> caller wants to run the function and wants to pretend as if it did
> >> not run anything when it failed, for example, using this to spool all
> >> output and error to a strbuf and discard it when the function returns
> >> an error, and emit the spooled output to standard output and standard
> >> error in the order the lines were collected when the function returns
> >> a success, would be a good way to do so.
> >
> > That is actually the exact opposite of the intended usage: when any
> > `pick` in an interactive rebase succeeds, its output is discarded so
> > as not to bother the user. We show the complete output only when it
> > fails.
> 
> Oh, it makes sense, too, to show the output only when there is an error.

Thanks ;-)

> But in that case, there would be both messages meant for the
> standard output and also meant for the standard error, and we need
> some way to make sure they go to the right channel.

Not necessarily. Let's have a look at our existing code in git-rebase.sh:

	output () {
		case "$verbose" in
		'')
			output=$("$@" 2>&1 )
			status=$?
			test $status != 0 && printf "%s\n" "$output"
			return $status
			;;
		*)
			"$@"
			;;
		esac
	}

This incredibly well-named function (</sarcasm>, my fault: dfa49f3 (Shut "git rebase -i" up when no --verbose was given, 2007-07-23)) accumulates all output, both stdout and stderr, and shows it only in case of an error.

Crucially, *all* output goes to stdout. No distinction is being made between stdout and stderr.

This is the existing behavior of rebase -i.

Show 18 quoted lines

> I however do not think an array of <bool, const char *> is the only
> way to achieve that.  We can get away by a single strbuf that
> accumulates all output() that we have seen so far, i.e. "we only
> accumulate output() and ignore flush() as long as what we see are
> only from output()" mode.
> 
> Then the err() routine operating under this new mode can show what
> has been accumulated to the standard output (because with this tweak
> I am outlining here, by definition, the strbuf will only keep the
> output() material and not err() things), show the err() message, and
> switch back to the normal "we accumulate output() and honor flush()"
> mode.  Of course, when we are doing multiple rounds, the mode must
> be reset to "accumulate output and ignore flush" mode at the
> beginning of each rouhd.
> 
> That would give us "silence if there is no error, but if we are
> showing error, show them to the standard error, while giving
> non-error message to the standard output".

It all makes sense what you say. In case you want to preserve the channel in some future modification.

However, I am right now most concerned about keeping existing behavior as faithfully as possible (with the exception of execution speed, which I want to improve dramatically).

As such, it would be a serious mistake to implement that mode and use it in the rebase--helper: it would very likely cause regressions in existing scripts, probably even my own.

So I do understand your concern, and I agree that it would make for a fine design, in a different context than this patch series.

Ciao, Dscho