Re: [PATCH v5 14/16] merge-recursive: offer an option to retain the output in 'obuf'
From: Junio C Hamano <hidden>
Date: 2016-07-27 22:09:30
Johannes Schindelin [off-list ref] writes:
Since 66a155b (Enable output buffering in merge-recursive., 2007-01-14), we already accumulate the output in a buffer. The idea was to avoid interfering with the progress output that goes to stderr, which is unbuffered, when we write to stdout, which is buffered. We extend that buffering to allow the caller to handle the output (possibly suppressing it). This will help us when extending the sequencer to do rebase -i's brunt work: it does not want the picks to print anything by default but instead determine itself whether to print the output or not. Note that we also redirect the error messages into the output buffer when the caller asked not to flush the output buffer, for two reasons: 1) to retain the correct output order, and 2) to allow the caller to suppress *all* output.
I am not yet sure if it makes sense to mix both the regular output and an error into the same buffer for the callers to process (your "reason 1)" above), and this looks like a wrong way to allow a caller that wants no output (your "reason 2)" above). A caller that wants to massage the output would want to know which ones are errors and which ones are not, I would imagine, and setting a knob to make both output() and err() a no-op would be a more suitable way to give a caller a total silence. At least I cannot yet judge if this is a good change, only from the material presented here. That does not mean I have a concrete reason to say this is a bad change, either. Only from the material presented here, I cannot judge that, either.
quoted hunk
Signed-off-by: Johannes Schindelin <redacted> --- merge-recursive.c | 17 +++++++++++++---- merge-recursive.h | 2 +- 2 files changed, 14 insertions(+), 5 deletions(-)diff --git a/merge-recursive.c b/merge-recursive.c index 723b8d0..311cfa4 100644 --- a/merge-recursive.c +++ b/merge-recursive.c@@ -25,7 +25,7 @@ static void flush_output(struct merge_options *o) { - if (o->obuf.len) { + if (o->buffer_output < 2 && o->obuf.len) { fputs(o->obuf.buf, stdout); strbuf_reset(&o->obuf); }@@ -36,10 +36,19 @@ static int err(struct merge_options *o, const char *err, ...) va_list params; va_start(params, err); - flush_output(o); + if (o->buffer_output < 2) + flush_output(o); + else { + strbuf_complete(&o->obuf, '\n'); + strbuf_addstr(&o->obuf, "error: "); + } strbuf_vaddf(&o->obuf, err, params); - error("%s", o->obuf.buf); - strbuf_reset(&o->obuf); + if (o->buffer_output > 1) + strbuf_addch(&o->obuf, '\n'); + else { + error("%s", o->obuf.buf); + strbuf_reset(&o->obuf); + } va_end(params); return -1;diff --git a/merge-recursive.h b/merge-recursive.h index d415724..340704c 100644 --- a/merge-recursive.h +++ b/merge-recursive.h@@ -13,7 +13,7 @@ struct merge_options { MERGE_RECURSIVE_THEIRS } recursive_variant; const char *subtree_shift; - unsigned buffer_output : 1; + unsigned buffer_output : 2; /* 1: output at end, 2: keep buffered */ unsigned renormalize : 1;
Once a field ceases to be a boolean, it is OK not to squish it into a bitfield like this for a struct that we will have only a very small number of instances of. Treating it just like "verbosity", which occupies a whole int even though it can only get up to 5 or so, would be more appropriate.
long xdl_opts; int verbosity;