Re: [PATCH v2 2/7] strbuf: split out logic to humanise byte values

[Patrick Steinhardt <ps@pks.im>]

On Fri, Dec 12, 2025 at 04:36:39PM -0600, Justin Tobler wrote:
> diff --git a/strbuf.c b/strbuf.c
> index 6c3851a7f8..1fb47bf21b 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -836,55 +836,49 @@ void strbuf_addstr_urlencode(struct strbuf *sb, const char *s,
>  	strbuf_add_urlencode(sb, s, strlen(s), allow_unencoded_fn);
>  }
>  
> -static void strbuf_humanise(struct strbuf *buf, off_t bytes,
> -				 int humanise_rate)
> +char *strbuf_humanise_bytes_value(struct strbuf *buf, off_t bytes, unsigned flags)
>  {
> +	int humanise_rate = flags & STRBUF_HUMANISE_RATE;
> +
>  	if (bytes > 1 << 30) {
> -		strbuf_addf(buf,
> -				humanise_rate == 0 ?
> -					/* TRANSLATORS: IEC 80000-13:2008 gibibyte */
> -					_("%u.%2.2u GiB") :
> -					/* TRANSLATORS: IEC 80000-13:2008 gibibyte/second */
> -					_("%u.%2.2u GiB/s"),
> -			    (unsigned)(bytes >> 30),
> +		strbuf_addf(buf, "%u.%2.2u", (unsigned)(bytes >> 30),
>  			    (unsigned)(bytes & ((1 << 30) - 1)) / 10737419);
> +		/* TRANSLATORS: IEC 80000-13:2008 gibibyte/second and gibibyte */
> +		return humanise_rate ? xstrfmt(_("GiB/s")) : xstrfmt(_("GiB"));
>  	} else if (bytes > 1 << 20) {
> -		unsigned x = bytes + 5243;  /* for rounding */
> -		strbuf_addf(buf,
> -				humanise_rate == 0 ?
> -					/* TRANSLATORS: IEC 80000-13:2008 mebibyte */
> -					_("%u.%2.2u MiB") :
> -					/* TRANSLATORS: IEC 80000-13:2008 mebibyte/second */
> -					_("%u.%2.2u MiB/s"),
> -			    x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
> +		unsigned x = bytes + 5243; /* for rounding */
> +		strbuf_addf(buf, "%u.%2.2u", x >> 20,
> +			    ((x & ((1 << 20) - 1)) * 100) >> 20);
> +		/* TRANSLATORS: IEC 80000-13:2008 mebibyte/second and mebibyte */
> +		return humanise_rate ? xstrfmt(_("MiB/s")) : xstrfmt(_("MiB"));
>  	} else if (bytes > 1 << 10) {
> -		unsigned x = bytes + 5;  /* for rounding */
> -		strbuf_addf(buf,
> -				humanise_rate == 0 ?
> -					/* TRANSLATORS: IEC 80000-13:2008 kibibyte */
> -					_("%u.%2.2u KiB") :
> -					/* TRANSLATORS: IEC 80000-13:2008 kibibyte/second */
> -					_("%u.%2.2u KiB/s"),
> -			    x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
> +		unsigned x = bytes + 5; /* for rounding */
> +		strbuf_addf(buf, "%u.%2.2u", x >> 10,
> +			    ((x & ((1 << 10) - 1)) * 100) >> 10);
> +		/* TRANSLATORS: IEC 80000-13:2008 kibibyte/second and kibibyte */
> +		return humanise_rate ? xstrfmt(_("KiB/s")) : xstrfmt(_("KiB"));
>  	} else {
> -		strbuf_addf(buf,
> -				humanise_rate == 0 ?
> -					/* TRANSLATORS: IEC 80000-13:2008 byte */
> -					Q_("%u byte", "%u bytes", bytes) :
> -					/* TRANSLATORS: IEC 80000-13:2008 byte/second */
> -					Q_("%u byte/s", "%u bytes/s", bytes),
> -				(unsigned)bytes);
> +		strbuf_addf(buf, "%u", (unsigned)bytes);
> +		return humanise_rate ?
> +			       /* TRANSLATORS: IEC 80000-13:2008 byte/second */
> +			       xstrfmt(Q_("byte/s", "bytes/s", bytes)) :
> +			       /* TRANSLATORS: IEC 80000-13:2008 byte */
> +			       xstrfmt(Q_("byte", "bytes", bytes));
>  	}
>  }

All branches use `xstrfmt()` with strings that are essentially
constants, except for the translation part. So isn't it possible to drop
all these allocations and have the function return a `const char *`
instead?

> diff --git a/strbuf.h b/strbuf.h
> index a580ac6084..a5e3ab0cb4 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -367,6 +367,15 @@ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);
>   */
>  void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags);
>  
> +#define STRBUF_HUMANISE_RATE 1 << 0

I think nowadays it's a bit more common to use an enum, and I think we
should also document what the flag does:

    enum strbuf_humanise_flags {
        /*
         * Frobnicate the string.
         */
        STRBUF_HUMANISE_RATE = (1 << 0),
    };

Patrick