[PATCH] usage: try harder to format very long error messages

[PATCH] usage: try harder to format very long error messages

[Jeff King]

We use a fixed-size buffer of 4096 bytes to format die() and
error() messages. We explicitly avoided using a strbuf or
other fanciness here, because we want to make sure that we
report the message even in the face of malloc() failure
(after all, we might even be dying due to a malloc call).

In most cases, this buffer is long enough to show any
reasonable message. However, if you are experimenting with
_unreasonable_ things (e.g., filenames approaching 4096
bytes), the messages can be truncated, making them
confusing. E.g., seeing:

  error: cannot stat 'aaaaaaaaaaaaaaaaaaaaaa

is much less useful than:

  error: cannot stat 'aaaaaaaaaaaaaaaaaaaaaaa/foo': File too long

(these are truncated for the sake of readability; obviously
real examples are much longer. Just imagine many lines full
of "a"'s).

This patch teaches vreportf and vwritef to at least try
using a dynamically sized buffer before falling back to the
fixed-size buffer. For most long errors (which are typically
reporting problems with syscalls on long filenames), this
means we'll generally see the full message. And in case that
fails, we still print the truncated message, but with a note
that it was truncated.

Note that die_errno does not need the same treatment for its
fixed-size buffers, as they are a combination of:

  - our fixed-size string constants, without placeholders
    expanded (so a literal "cannot stat '%s'", without %s
    expanded to arbitrary data)

  - the results of strerror(errno)

both of which should be predictably small.

Signed-off-by: Jeff King <peff@peff.net>
---
So this solution tries to change vreportf and vwritef as little as
possible, and ends up slightly complex as a result. But reading
vreportf's:

  vsnprintf(msg, sizeof(msg), err, params);
  fprintf(stderr, "%s%s\n", prefix, msg);

I had to wonder why this wasn't just:

  fputs(prefix, stderr);
  vfprintf(stderr, err, params);

In fact, we used to do this, but changed it in d048a96 (print
warning/error/fatal messages in one shot, 2007-11-09). I'm not sure of
the reasoning there, though. I would expect stdio to generally write
that as a single shot already, assuming:

  - there isn't anything in the stderr buffer already (i.e., we do not
    need to flush somewhere in the middle)

  - our prefix does not have a newline (which it doesn't; it is
    "error:", "fatal:", etc).

IOW, I wonder if it would be enough to simply fflush(stderr) before and
after to make sure we keep the buffer clear. I also wonder if this is
even enough as-is; if the resulting message is larger than stdio's
buffer size, I'd expect it to come through across several writes anyway.

As for vwritef, it exists solely to work over write(), _and_ it doesn't
get the "one-shot" thing right (which is probably OK, as we use it only
when exec fails). But we could probably teach run-command to fdopen(),
and get rid of it entirely (in favor of teaching vreportf to take a
FILE* handle instead of assuming stderr).

So I dunno. I think the solution here works fine, but maybe we should be
taking the opportunity to simplify.

 usage.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 64 insertions(+), 8 deletions(-)

diff --git a/usage.c b/usage.c
index ed14645..78b0d75 100644
--- a/usage.c
+++ b/usage.c
@@ -6,23 +6,79 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
+/*
+ * This buffer allows you to try formatting a full message, but if malloc
+ * fails, will fall back to a fixed-size buffer and truncate the message.
+ * If we truncate the message, it adds a note indicating so.
+ *
+ * No initialization is necessary before robust_buf_fmt, and after it returns,
+ * "buf" points to the contents (whether truncated or not). You should always
+ * robust_buf_free the result, which handles both cases.
+ */
+struct robust_buf {
+	char *buf;
+	char fixed[4096];
+};
+
+static int robust_buf_fmt(struct robust_buf *rb,
+			  const char *fmt, va_list ap)
+{
+	static const char trunc[] = " [message truncated]";
+	static const char broken[] = "BUG: your vsnprintf is broken";
+	va_list cp;
+	int len;
+
+	va_copy(cp, ap);
+	len = vsnprintf(rb->fixed, sizeof(rb->fixed), fmt, cp);
+	va_end(cp);
+
+	if (len < 0) {
+		memcpy(rb->fixed, broken, sizeof(broken));
+		rb->buf = rb->fixed;
+		return sizeof(broken) - 1;
+	}
+	if (len < sizeof(rb->fixed)) {
+		rb->buf = rb->fixed;
+		return len;
+	}
+
+	rb->buf = malloc(len + 1);
+	if (!rb->buf) {
+		memcpy(rb->fixed + sizeof(rb->fixed) - sizeof(trunc),
+		       trunc, sizeof(trunc));
+		rb->buf = rb->fixed;
+		return sizeof(rb->fixed) - 1;
+	}
+
+	if (vsnprintf(rb->buf, len + 1, fmt, ap) >= len)
+		; /* insatiable vsnprintf, nothing we can do */
+	return len;
+}
+
+static void robust_buf_free(struct robust_buf *rb)
+{
+	if (rb->buf != rb->fixed)
+		free(rb->buf);
+}
+
 void vreportf(const char *prefix, const char *err, va_list params)
 {
-	char msg[4096];
-	vsnprintf(msg, sizeof(msg), err, params);
-	fprintf(stderr, "%s%s\n", prefix, msg);
+	struct robust_buf msg;
+	robust_buf_fmt(&msg, err, params);
+	fprintf(stderr, "%s%s\n", prefix, msg.buf);
+	robust_buf_free(&msg);
 }
 
 void vwritef(int fd, const char *prefix, const char *err, va_list params)
 {
-	char msg[4096];
-	int len = vsnprintf(msg, sizeof(msg), err, params);
-	if (len > sizeof(msg))
-		len = sizeof(msg);
+	struct robust_buf msg;
+	int len = robust_buf_fmt(&msg, err, params);
 
 	write_in_full(fd, prefix, strlen(prefix));
-	write_in_full(fd, msg, len);
+	write_in_full(fd, msg.buf, len);
 	write_in_full(fd, "\n", 1);
+
+	robust_buf_free(&msg);
 }
 
 static NORETURN void usage_builtin(const char *err, va_list params)
-- 
2.5.0.417.g2db689c

Re: [PATCH] usage: try harder to format very long error messages

[Stefan Beller]

On Tue, Aug 11, 2015 at 9:17 AM, Jeff King <peff@peff.net> wrote:
> We use a fixed-size buffer of 4096 bytes to format die() and
> error() messages. We explicitly avoided using a strbuf or
> other fanciness here, because we want to make sure that we
> report the message even in the face of malloc() failure
> (after all, we might even be dying due to a malloc call).

Would it make sense to allocate memory in the early startup phase
for a possible error message?
So instead of putting 4kb on the stack we'd just have an unused 16kb
on the heap.

>
> In most cases, this buffer is long enough to show any
> reasonable message. However, if you are experimenting with
> _unreasonable_ things (e.g., filenames approaching 4096
> bytes), the messages can be truncated, making them
> confusing. E.g., seeing:
>
>   error: cannot stat 'aaaaaaaaaaaaaaaaaaaaaa
>
> is much less useful than:
>
>   error: cannot stat 'aaaaaaaaaaaaaaaaaaaaaaa/foo': File too long
>
> (these are truncated for the sake of readability; obviously
> real examples are much longer. Just imagine many lines full
> of "a"'s).
>
> This patch teaches vreportf and vwritef to at least try
> using a dynamically sized buffer before falling back to the
> fixed-size buffer. For most long errors (which are typically
> reporting problems with syscalls on long filenames), this
> means we'll generally see the full message. And in case that
> fails, we still print the truncated message, but with a note
> that it was truncated.
>
> Note that die_errno does not need the same treatment for its
> fixed-size buffers, as they are a combination of:
>
>   - our fixed-size string constants, without placeholders
>     expanded (so a literal "cannot stat '%s'", without %s
>     expanded to arbitrary data)
>
>   - the results of strerror(errno)
>
> both of which should be predictably small.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> So this solution tries to change vreportf and vwritef as little as
> possible, and ends up slightly complex as a result. But reading
> vreportf's:
>
>   vsnprintf(msg, sizeof(msg), err, params);
>   fprintf(stderr, "%s%s\n", prefix, msg);
>
> I had to wonder why this wasn't just:
>
>   fputs(prefix, stderr);
>   vfprintf(stderr, err, params);
>
> In fact, we used to do this, but changed it in d048a96 (print
> warning/error/fatal messages in one shot, 2007-11-09). I'm not sure of
> the reasoning there, though. I would expect stdio to generally write
> that as a single shot already, assuming:
>
>   - there isn't anything in the stderr buffer already (i.e., we do not
>     need to flush somewhere in the middle)
>
>   - our prefix does not have a newline (which it doesn't; it is
>     "error:", "fatal:", etc).
>
> IOW, I wonder if it would be enough to simply fflush(stderr) before and
> after to make sure we keep the buffer clear. I also wonder if this is
> even enough as-is; if the resulting message is larger than stdio's
> buffer size, I'd expect it to come through across several writes anyway.
>
> As for vwritef, it exists solely to work over write(), _and_ it doesn't
> get the "one-shot" thing right (which is probably OK, as we use it only
> when exec fails). But we could probably teach run-command to fdopen(),
> and get rid of it entirely (in favor of teaching vreportf to take a
> FILE* handle instead of assuming stderr).
>
> So I dunno. I think the solution here works fine, but maybe we should be
> taking the opportunity to simplify.
>
>  usage.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 64 insertions(+), 8 deletions(-)
>
> diff --git a/usage.c b/usage.c
> index ed14645..78b0d75 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -6,23 +6,79 @@
>  #include "git-compat-util.h"
>  #include "cache.h"
>
> +/*
> + * This buffer allows you to try formatting a full message, but if malloc
> + * fails, will fall back to a fixed-size buffer and truncate the message.
> + * If we truncate the message, it adds a note indicating so.
> + *
> + * No initialization is necessary before robust_buf_fmt, and after it returns,
> + * "buf" points to the contents (whether truncated or not). You should always
> + * robust_buf_free the result, which handles both cases.
> + */
> +struct robust_buf {
> +       char *buf;
> +       char fixed[4096];
> +};
> +
> +static int robust_buf_fmt(struct robust_buf *rb,
> +                         const char *fmt, va_list ap)
> +{
> +       static const char trunc[] = " [message truncated]";
> +       static const char broken[] = "BUG: your vsnprintf is broken";
> +       va_list cp;
> +       int len;
> +
> +       va_copy(cp, ap);
> +       len = vsnprintf(rb->fixed, sizeof(rb->fixed), fmt, cp);
> +       va_end(cp);
> +
> +       if (len < 0) {
> +               memcpy(rb->fixed, broken, sizeof(broken));
> +               rb->buf = rb->fixed;
> +               return sizeof(broken) - 1;
> +       }
> +       if (len < sizeof(rb->fixed)) {
> +               rb->buf = rb->fixed;
> +               return len;
> +       }
> +
> +       rb->buf = malloc(len + 1);
> +       if (!rb->buf) {
> +               memcpy(rb->fixed + sizeof(rb->fixed) - sizeof(trunc),
> +                      trunc, sizeof(trunc));
> +               rb->buf = rb->fixed;
> +               return sizeof(rb->fixed) - 1;
> +       }
> +
> +       if (vsnprintf(rb->buf, len + 1, fmt, ap) >= len)
> +               ; /* insatiable vsnprintf, nothing we can do */
> +       return len;
> +}
> +
> +static void robust_buf_free(struct robust_buf *rb)
> +{
> +       if (rb->buf != rb->fixed)
> +               free(rb->buf);
> +}
> +
>  void vreportf(const char *prefix, const char *err, va_list params)
>  {
> -       char msg[4096];
> -       vsnprintf(msg, sizeof(msg), err, params);
> -       fprintf(stderr, "%s%s\n", prefix, msg);
> +       struct robust_buf msg;
> +       robust_buf_fmt(&msg, err, params);
> +       fprintf(stderr, "%s%s\n", prefix, msg.buf);
> +       robust_buf_free(&msg);
>  }
>
>  void vwritef(int fd, const char *prefix, const char *err, va_list params)
>  {
> -       char msg[4096];
> -       int len = vsnprintf(msg, sizeof(msg), err, params);
> -       if (len > sizeof(msg))
> -               len = sizeof(msg);
> +       struct robust_buf msg;
> +       int len = robust_buf_fmt(&msg, err, params);
>
>         write_in_full(fd, prefix, strlen(prefix));
> -       write_in_full(fd, msg, len);
> +       write_in_full(fd, msg.buf, len);
>         write_in_full(fd, "\n", 1);
> +
> +       robust_buf_free(&msg);
>  }
>
>  static NORETURN void usage_builtin(const char *err, va_list params)
> --
> 2.5.0.417.g2db689c
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usage: try harder to format very long error messages

[Jeff King]

On Tue, Aug 11, 2015 at 09:28:34AM -0700, Stefan Beller wrote:

> On Tue, Aug 11, 2015 at 9:17 AM, Jeff King <peff@peff.net> wrote:
> > We use a fixed-size buffer of 4096 bytes to format die() and
> > error() messages. We explicitly avoided using a strbuf or
> > other fanciness here, because we want to make sure that we
> > report the message even in the face of malloc() failure
> > (after all, we might even be dying due to a malloc call).
> 
> Would it make sense to allocate memory in the early startup phase
> for a possible error message?
> So instead of putting 4kb on the stack we'd just have an unused 16kb
> on the heap.

Isn't that just punting on the problem? Now your 16kb filename will get
truncated messages (in general we cannot even work with such files, but
it is nice if the error message telling us so is readable).

If stack space is the problem, we can just put 16kb in BSS. But I think
we really do want something that grows to the appropriate size. Or we
need to start being more clever about our truncation. E.g., printing:

  error: unable to stat 'aaaaaaaaa[...]aaaaaa/foo': File too long

where the "[...]" is literally what we would print. The trouble with
that approach is that it is hard to intercept large strings without
re-implementing all of stdio's formatting.

-Peff

Re: [PATCH] usage: try harder to format very long error messages

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We use a fixed-size buffer of 4096 bytes to format die() and
> error() messages. We explicitly avoided using a strbuf or
> other fanciness here, because we want to make sure that we
> report the message even in the face of malloc() failure
> (after all, we might even be dying due to a malloc call).

Yes, that was the rationale behind the "we use a fixed-buffer and
try not to allocate in this codepath", I think.

> As for vwritef, it exists solely to work over write(), _and_ it doesn't
> get the "one-shot" thing right (which is probably OK, as we use it only
> when exec fails). But we could probably teach run-command to fdopen(),
> and get rid of it entirely (in favor of teaching vreportf to take a
> FILE* handle instead of assuming stderr).

Sounds like a plan ;-)

The patch looks very sensible.

Re: [PATCH] usage: try harder to format very long error messages

[Jeff King]

On Tue, Aug 11, 2015 at 09:34:31AM -0700, Junio C Hamano wrote:

> > As for vwritef, it exists solely to work over write(), _and_ it doesn't
> > get the "one-shot" thing right (which is probably OK, as we use it only
> > when exec fails). But we could probably teach run-command to fdopen(),
> > and get rid of it entirely (in favor of teaching vreportf to take a
> > FILE* handle instead of assuming stderr).
> 
> Sounds like a plan ;-)

So here's an alternative series that does this, along with the
single-fprintf thing I mentioned. The first patch is actually orthogonal
to the second; we would probably want it whichever path we choose to fix
vreportf's truncation (I'd just have to rebase the earlier patch on top
of it if we go that route).

  [1/2]: vreportf: report to arbitrary filehandles
  [2/2]: vreportf: avoid intermediate buffer

-Peff

[PATCH 1/2] vreportf: report to arbitrary filehandles

[Jeff King]

The vreportf function always goes to stderr, but run-command
wants child errors to go to the parent's original stderr. To
solve this, commit a5487dd duplicates the stderr fd and
installs die and error handlers to direct the output
appropriately (which later turned into the vwritef
function). This has two downsides, though:

  - we make multiple calls to write(), which contradicts the
    "write at once" logic from d048a96 (print
    warning/error/fatal messages in one shot, 2007-11-09).

  - the custom handlers basically duplicate the normal
    handlers.  They're only a few lines of code, but we
    should not have to repeat the magic "exit(128)", for
    example.

We can solve the first by using fdopen() on the duplicated
descriptor. We can't pass this to vreportf, but we could
introduce a new vreportf_to to handle it.

However, to fix the second problem, we instead introduce a
new "set_error_handle" function, which lets the normal
vreportf calls output to a handle besides stderr. Thus we
can get rid of our custom handlers entirely, and just ask
the regular handlers to output to our new descriptor.

And as vwritef has no more callers, it can just go away.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h |  2 +-
 run-command.c     | 17 ++---------------
 usage.c           | 22 +++++++++-------------
 3 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index c6d391f..076461e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -389,7 +389,6 @@ struct strbuf;
 
 /* General helper functions */
 extern void vreportf(const char *prefix, const char *err, va_list params);
-extern void vwritef(int fd, const char *prefix, const char *err, va_list params);
 extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
@@ -425,6 +424,7 @@ static inline int const_error(void)
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list params));
 extern void set_die_is_recursing_routine(int (*routine)(void));
+extern void set_error_handle(FILE *);
 
 extern int starts_with(const char *str, const char *prefix);
 
diff --git a/run-command.c b/run-command.c
index 4d73e90..0d01671 100644
--- a/run-command.c
+++ b/run-command.c
@@ -200,7 +200,6 @@ static int execv_shell_cmd(const char **argv)
 #endif
 
 #ifndef GIT_WINDOWS_NATIVE
-static int child_err = 2;
 static int child_notifier = -1;
 
 static void notify_parent(void)
@@ -212,17 +211,6 @@ static void notify_parent(void)
 	 */
 	xwrite(child_notifier, "", 1);
 }
-
-static NORETURN void die_child(const char *err, va_list params)
-{
-	vwritef(child_err, "fatal: ", err, params);
-	exit(128);
-}
-
-static void error_child(const char *err, va_list params)
-{
-	vwritef(child_err, "error: ", err, params);
-}
 #endif
 
 static inline void set_cloexec(int fd)
@@ -362,11 +350,10 @@ fail_pipe:
 		 * in subsequent call paths use the parent's stderr.
 		 */
 		if (cmd->no_stderr || need_err) {
-			child_err = dup(2);
+			int child_err = dup(2);
 			set_cloexec(child_err);
+			set_error_handle(fdopen(child_err, "w"));
 		}
-		set_die_routine(die_child);
-		set_error_routine(error_child);
 
 		close(notify_pipe[0]);
 		set_cloexec(notify_pipe[1]);
diff --git a/usage.c b/usage.c
index ed14645..e4fa6d2 100644
--- a/usage.c
+++ b/usage.c
@@ -6,23 +6,14 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
+static FILE *error_handle;
+
 void vreportf(const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
+	FILE *fh = error_handle ? error_handle : stderr;
 	vsnprintf(msg, sizeof(msg), err, params);
-	fprintf(stderr, "%s%s\n", prefix, msg);
-}
-
-void vwritef(int fd, const char *prefix, const char *err, va_list params)
-{
-	char msg[4096];
-	int len = vsnprintf(msg, sizeof(msg), err, params);
-	if (len > sizeof(msg))
-		len = sizeof(msg);
-
-	write_in_full(fd, prefix, strlen(prefix));
-	write_in_full(fd, msg, len);
-	write_in_full(fd, "\n", 1);
+	fprintf(fh, "%s%s\n", prefix, msg);
 }
 
 static NORETURN void usage_builtin(const char *err, va_list params)
@@ -76,6 +67,11 @@ void set_die_is_recursing_routine(int (*routine)(void))
 	die_is_recursing = routine;
 }
 
+void set_error_handle(FILE *fh)
+{
+	error_handle = fh;
+}
+
 void NORETURN usagef(const char *err, ...)
 {
 	va_list params;
-- 
2.5.0.417.g2db689c

[PATCH 2/2] vreportf: avoid intermediate buffer

[Jeff King]

When we call "die(fmt, args...)", we end up in vreportf with
two pieces of information:

  1. The prefix "fatal: "

  2. The original fmt and va_list of args.

We format item (2) into a temporary buffer, and then fprintf
the prefix and the temporary buffer, along with a newline.
This has the unfortunate side effect of truncating any error
messages that are longer than 4096 bytes.

Instead, let's use separate calls for the prefix and
newline, letting us hand the item (2) directly to vfprintf.
This is essentially undoing d048a96 (print
warning/error/fatal messages in one shot, 2007-11-09), which
tried to have the whole output end up in a single `write`
call.

But we can address this instead by explicitly requesting
line-buffering for the output handle, and by making sure
that the buffer is empty before we start (so that outputting
the prefix does not cause a flush due to hitting the buffer
limit).

We may still break the output into two writes if the content
is larger than our buffer, but there's not much we can do
there; depending on the stdio implementation, that might
have happened even with a single fprintf call.

Signed-off-by: Jeff King <peff@peff.net>
---
I am not 100% sure on the last statement. On my system, it seems that:

  fprintf(stderr, "%s%s\n", prefix, msg);

will generally result in a single write when stderr is unbuffered (i.e.,
it's default state). Which feels very magical, since it clearly must be
preparing the output in a single buffer to feed to write, and the
contents of prefix and msg may easily exceed BUFSIZ. It looks like
glibc internally uses an 8K buffer to generate "unbuffered" content.
E.g., if I do:

  strace -o /dev/fd/3 -e write \
  git --no-pager log --$(perl -e 'print "a" x 4096') \
  3>&2 2>/dev/null

I get:

  write(2, "fatal: unrecognized argument: --"..., 4129) = 4129

and if I bump the 4096 to 16384, I get:

  write(2, "fatal: unrecognized argument: --"..., 8192) = 8192
  write(2, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 8192) = 8192
  write(2, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 33) = 33

So that's kind of weird. I suspect other stdio implementations may
behave differently, and AFAIK the standard is quiet on the subject (so
it would be OK for an implementation to output the prefix, the msg, and
the newline in separate writes, no matter their length).

 usage.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/usage.c b/usage.c
index e4fa6d2..82ff131 100644
--- a/usage.c
+++ b/usage.c
@@ -7,13 +7,21 @@
 #include "cache.h"
 
 static FILE *error_handle;
+static int tweaked_error_buffering;
 
 void vreportf(const char *prefix, const char *err, va_list params)
 {
-	char msg[4096];
 	FILE *fh = error_handle ? error_handle : stderr;
-	vsnprintf(msg, sizeof(msg), err, params);
-	fprintf(fh, "%s%s\n", prefix, msg);
+
+	fflush(fh);
+	if (!tweaked_error_buffering) {
+		setvbuf(fh, NULL, _IOLBF, 0);
+		tweaked_error_buffering = 1;
+	}
+
+	fputs(prefix, fh);
+	vfprintf(fh, err, params);
+	fputc('\n', fh);
 }
 
 static NORETURN void usage_builtin(const char *err, va_list params)
@@ -70,6 +78,7 @@ void set_die_is_recursing_routine(int (*routine)(void))
 void set_error_handle(FILE *fh)
 {
 	error_handle = fh;
+	tweaked_error_buffering = 0;
 }
 
 void NORETURN usagef(const char *err, ...)
-- 
2.5.0.417.g2db689c