Re: [PATCH v5 1/6] strbuf: add shortcut to work with error messages

[Junio C Hamano <gitster@pobox.com>]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Add function strbuf_error() that helps to save few lines of code.
> Function expands fmt with placeholders, append resulting error message
> to strbuf *err, and return error code ret.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
>  strbuf.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/strbuf.h b/strbuf.h
> index e6cae5f4398c8..fa66d4835f1a7 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -620,4 +620,17 @@ char *xstrvfmt(const char *fmt, va_list ap);
>  __attribute__((format (printf, 1, 2)))
>  char *xstrfmt(const char *fmt, ...);
>  
> +/*
> + * Expand error message, append it to strbuf *err, then return error code ret.
> + * Allow to save few lines of code.
> + */
> +static inline int strbuf_error(struct strbuf *err, int ret, const char *fmt, ...)
> +{

With this function, err does not have to be an error message, and
ret does not have to be negative.  Hence strbuf_error() is a wrong
name for the wrapper.

It somewhat is bothersome to see that this is inlined; if it is
meant for error codepath, it probably shouldn't have to be.

> +	va_list ap;
> +	va_start(ap, fmt);
> +	strbuf_vaddf(err, fmt, ap);
> +	va_end(ap);
> +	return ret;
> +}
> +
>  #endif /* STRBUF_H */

Quite honestly, I am not sure if it is worth to be in strbuf.h; it
feels a bit too specific to the immediate need for these five
patches and nowhere else.  Are there many existing calls to
strbuf_addf() immediately followed by an "return" of an integer,
that can be simplified by using this helper?  I see quite a many
instances of addf() soon followed by "return -1" in
refs/files-backend.c, but they are not immediately adjacent to each
other, and won't be helped.