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

[Junio C Hamano]

Johannes Schindelin  writes:

>> 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 (, 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.
> ...
> 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.

Sounds like we are desperately trying to find an excuse to do a
wrong thing by finding an existing piece of code that did a wrong
thing already.

That leaves a bad taste in my mouth, but as "rebase -i" is meant to
be an "interactive" command, I would imagine that nobody would have
expected to run it as "git rebase -i >/dev/null" in order to view
only the error messages (or vice versa with "2>errs").

So OK then, at least for now.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Johannes Schindelin  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.

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.

I however do not think an array of  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".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

Hi Junio,

On Wed, 27 Jul 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > 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.

I changed it to an int.

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

Hi Junio,

On Wed, 27 Jul 2016, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > 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.
> 
> 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.

> That however brings me back to the "reason 1" thing.  Shouldn't the
> spooling be done not just with a single strbuf, but with an array of
>  tuples, where the bool says if it is for the
> standard output, and the string holds the message?  output() would
> mark its element in the array with true, while err() would do so
> with false.

I would like to *not* deviate further from my original focus. My
willingness to address comments that have little to nothing to do with
accelerating the interactive rebase has cost me already over a month, with
this patch series alone.

Do not get me wrong: I think you are right in your assessment that a
future caller of the recursive merge might need to be able to tell apart
which lines of the output are error messages from the rest, and still
retain the order.

But then, maybe no future caller will require this.

What is certain: right now, no caller requires it, and neither do my
queued-up patches to speed up the interactive rebase.

And another thing is also certain: *iff* a future caller really will
require that fine-grained information of the output, then it will be
dramatically easier to implement this on top of the patches we are
discussing right now.

In short: I hope you agree with me that it is safe to defer the  tuple implementation to the time when we will *actually* need
it.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Junio C Hamano  writes:

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

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 however brings me back to the "reason 1" thing.  Shouldn't the
spooling be done not just with a single strbuf, but with an array of
 tuples, where the bool says if it is for the
standard output, and the string holds the message?  output() would
mark its element in the array with true, while err() would do so
with false.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Johannes Schindelin  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.

> Signed-off-by: Johannes Schindelin 
> ---
>  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(>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(>obuf, '\n');
> + strbuf_addstr(>obuf, "error: ");
> + }
>   strbuf_vaddf(>obuf, err, params);
> - error("%s", o->obuf.buf);
> - strbuf_reset(>obuf);
> + if (o->buffer_output > 1)
> + strbuf_addch(>obuf, '\n');
> + else {
> + error("%s", o->obuf.buf);
> + strbuf_reset(>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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

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.

Signed-off-by: Johannes Schindelin 
---
 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(>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(>obuf, '\n');
+   strbuf_addstr(>obuf, "error: ");
+   }
strbuf_vaddf(>obuf, err, params);
-   error("%s", o->obuf.buf);
-   strbuf_reset(>obuf);
+   if (o->buffer_output > 1)
+   strbuf_addch(>obuf, '\n');
+   else {
+   error("%s", o->obuf.buf);
+   strbuf_reset(>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;
long xdl_opts;
int verbosity;
-- 
2.9.0.281.g286a8d9


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html