Re: [PATCH v5 12/16] merge-recursive: flush output buffer before printing error messages

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin  writes:
> 
> > The data structure passed to the recursive merge machinery has a feature
> > where the caller can ask for the output to be buffered into a strbuf, by
> > setting the field 'buffer_output'.
> >
> > Previously, we simply swallowed the buffered output when showing error
> > messages. With this patch, we show the output first, and only then print
> > the error message.
> 
> I didn't quite understand this paragraph until I realized that you
> meant "when showing die message".  We died without flushing, losing
> accumulated output.

I rephrased it, using your explanation.

> > +static int err(struct merge_options *o, const char *err, ...)
> > +{
> > +   va_list params;
> > +
> > +   va_start(params, err);
> > +   flush_output(o);
> 
> I would have written the above two swapped; va_start() logically
> is about what happens in the next four lines.

For some reason, I thought that `va_start()` must be the first statement
of the function. Fixed.

> > +   strbuf_vaddf(&o->obuf, err, params);
> > +   error("%s", o->obuf.buf);
> > +   strbuf_reset(&o->obuf);
> 
> Sneaky ;-)

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 12/16] merge-recursive: flush output buffer before printing error messages

[Junio C Hamano]

On Wed, Jul 27, 2016 at 2:37 PM, Junio C Hamano  wrote:
>
>> + strbuf_vaddf(&o->obuf, err, params);
>> + error("%s", o->obuf.buf);
>> + strbuf_reset(&o->obuf);
>
> Sneaky ;-)

Just to avoid confusion, I am _fine_ with this "we happen to have
a strbuf that we know to be empty at this point, so let's reuse it
and clean after ourselves before returning".

I just found it somewhere between clever and ugly, and "sneaky"
was the first word that came to my mind.
--
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 12/16] merge-recursive: flush output buffer before printing error messages

[Junio C Hamano]

Johannes Schindelin  writes:

> The data structure passed to the recursive merge machinery has a feature
> where the caller can ask for the output to be buffered into a strbuf, by
> setting the field 'buffer_output'.
>
> Previously, we simply swallowed the buffered output when showing error
> messages. With this patch, we show the output first, and only then print
> the error message.

I didn't quite understand this paragraph until I realized that you
meant "when showing die message".  We died without flushing, losing
accumulated output.

> +static int err(struct merge_options *o, const char *err, ...)
> +{
> + va_list params;
> +
> + va_start(params, err);
> + flush_output(o);

I would have written the above two swapped; va_start() logically
is about what happens in the next four lines.

> + strbuf_vaddf(&o->obuf, err, params);
> + error("%s", o->obuf.buf);
> + strbuf_reset(&o->obuf);

Sneaky ;-)

The remainder replaces error(...) with err(o, ...) and updates the
callchain to pass the merge_options around, which looked good.

Thanks.
--
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 12/16] merge-recursive: flush output buffer before printing error messages

[Johannes Schindelin]

The data structure passed to the recursive merge machinery has a feature
where the caller can ask for the output to be buffered into a strbuf, by
setting the field 'buffer_output'.

Previously, we simply swallowed the buffered output when showing error
messages. With this patch, we show the output first, and only then print
the error message.

Currently, the only user of that buffering is merge_recursive() itself,
to avoid the progress output to interfere.

In the next patches, we will introduce a new buffer_output mode that
forces merge_recursive() to retain the output buffer for further
processing by the caller. If the caller asked for that, we will then
also write the error messages into the output buffer. This is necessary
to give the caller more control not only how to react in case of errors
but also control how/if to display the error messages.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 116 --
 1 file changed, 68 insertions(+), 48 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index bc59815..71a0aa0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -23,6 +23,28 @@
 #include "dir.h"
 #include "submodule.h"
 
+static void flush_output(struct merge_options *o)
+{
+   if (o->obuf.len) {
+   fputs(o->obuf.buf, stdout);
+   strbuf_reset(&o->obuf);
+   }
+}
+
+static int err(struct merge_options *o, const char *err, ...)
+{
+   va_list params;
+
+   va_start(params, err);
+   flush_output(o);
+   strbuf_vaddf(&o->obuf, err, params);
+   error("%s", o->obuf.buf);
+   strbuf_reset(&o->obuf);
+   va_end(params);
+
+   return -1;
+}
+
 static struct tree *shift_tree_object(struct tree *one, struct tree *two,
  const char *subtree_shift)
 {
@@ -148,14 +170,6 @@ static int show(struct merge_options *o, int v)
return (!o->call_depth && o->verbosity >= v) || o->verbosity >= 5;
 }
 
-static void flush_output(struct merge_options *o)
-{
-   if (o->obuf.len) {
-   fputs(o->obuf.buf, stdout);
-   strbuf_reset(&o->obuf);
-   }
-}
-
 __attribute__((format (printf, 3, 4)))
 static void output(struct merge_options *o, int v, const char *fmt, ...)
 {
@@ -198,7 +212,8 @@ static void output_commit_title(struct merge_options *o, 
struct commit *commit)
}
 }
 
-static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
+static int add_cacheinfo(struct merge_options *o,
+   unsigned int mode, const struct object_id *oid,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
@@ -206,7 +221,7 @@ static int add_cacheinfo(unsigned int mode, const struct 
object_id *oid,
 
ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage, 
0);
if (!ce)
-   return error(_("addinfo_cache failed for path '%s'"), path);
+   return err(o, _("addinfo_cache failed for path '%s'"), path);
 
ret = add_cache_entry(ce, options);
if (refresh) {
@@ -276,7 +291,7 @@ struct tree *write_tree_from_memory(struct merge_options *o)
 
if (!cache_tree_fully_valid(active_cache_tree) &&
cache_tree_update(&the_index, 0) < 0) {
-   error(_("error building trees"));
+   err(o, _("error building trees"));
return NULL;
}
 
@@ -544,7 +559,8 @@ static struct string_list *get_renames(struct merge_options 
*o,
return renames;
 }
 
-static int update_stages(const char *path, const struct diff_filespec *o,
+static int update_stages(struct merge_options *opt, const char *path,
+const struct diff_filespec *o,
 const struct diff_filespec *a,
 const struct diff_filespec *b)
 {
@@ -563,13 +579,13 @@ static int update_stages(const char *path, const struct 
diff_filespec *o,
if (remove_file_from_cache(path))
return -1;
if (o)
-   if (add_cacheinfo(o->mode, &o->oid, path, 1, 0, options))
+   if (add_cacheinfo(opt, o->mode, &o->oid, path, 1, 0, options))
return -1;
if (a)
-   if (add_cacheinfo(a->mode, &a->oid, path, 2, 0, options))
+   if (add_cacheinfo(opt, a->mode, &a->oid, path, 2, 0, options))
return -1;
if (b)
-   if (add_cacheinfo(b->mode, &b->oid, path, 3, 0, options))
+   if (add_cacheinfo(opt, b->mode, &b->oid, path, 3, 0, options))
return -1;
return 0;
 }
@@ -720,8 +736,8 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
if (status) {
if (status == SCLD_EXISTS)
/* something else exists */
-   return error(msg, path, _(": perhaps a D/F confl