Re: [PATCH 1/2] progress: create public humanize() to show sizes

2013-04-08 Thread Eric Sunshine
On Mon, Apr 8, 2013 at 2:18 PM, Antoine Pelisse  wrote:
> Currently, humanization of downloaded size is done in the same function
> as text formatting. This is an issue if anyone else wants to use this.
>
> Separate text formatting from size simplification and make the function
> public so that it can easily be used by other clients.
>
> We now can use humanize() for both downloaded size and download speed
> calculation. One of the drawbacks is that speed will no look like this

s/no/now/

> when download is stalled: "0 bytes/s" instead of "0 KiB/s".
>
> Signed-off-by: Antoine Pelisse 
--
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 1/2] progress: create public humanize() to show sizes

2013-04-08 Thread Junio C Hamano
Antoine Pelisse  writes:

> Currently, humanization of downloaded size is done in the same function
> as text formatting. This is an issue if anyone else wants to use this.
>
> Separate text formatting from size simplification and make the function
> public so that it can easily be used by other clients.
>
> We now can use humanize() for both downloaded size and download speed
> calculation. One of the drawbacks is that speed will no look like this
> when download is stalled: "0 bytes/s" instead of "0 KiB/s".
>
> Signed-off-by: Antoine Pelisse 
> ---

Sounds good, but I think this helper function should live in
strbuf.[ch], where many other text/string helpers that take a strbuf
as their first parameter live.

>  progress.c |   60 
> ++--
>  progress.h |2 ++
>  2 files changed, 36 insertions(+), 26 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index 3971f49..76c1e42 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -8,8 +8,11 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include 
> +

Please do not do this.

If you somehow need to have , a suitable place should be
found in git-compat-util.h; various platforms seem to have quirks
with the ordering of system header files, and git-compat-util.h is
meant to encapsulate them.

In fact, git-compat-util.h should already include it.

>  #include "git-compat-util.h"
>  #include "progress.h"
> +#include "strbuf.h"
>  
>  #define TP_IDX_MAX  8
>  
> @@ -112,34 +115,33 @@ static int display(struct progress *progress, unsigned 
> n, const char *done)
>   return 0;
>  }
>  
> -static void throughput_string(struct throughput *tp, off_t total,
> -   unsigned int rate)
> +void humanize(struct strbuf *buf, off_t bytes)
>  {
> - int l = sizeof(tp->display);
> - if (total > 1 << 30) {
> - l -= snprintf(tp->display, l, ", %u.%2.2u GiB",
> -   (int)(total >> 30),
> -   (int)(total & ((1 << 30) - 1)) / 10737419);
> - } else if (total > 1 << 20) {
> - int x = total + 5243;  /* for rounding */
> - l -= snprintf(tp->display, l, ", %u.%2.2u MiB",
> -   x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
> - } else if (total > 1 << 10) {
> - int x = total + 5;  /* for rounding */
> - l -= snprintf(tp->display, l, ", %u.%2.2u KiB",
> -   x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
> + if (bytes > 1 << 30) {
> + strbuf_addf(buf, "%u.%2.2u GiB",
> + (int)(bytes >> 30),
> + (int)(bytes & ((1 << 30) - 1)) / 10737419);
> + } else if (bytes > 1 << 20) {
> + int x = bytes + 5243;  /* for rounding */
> + strbuf_addf(buf, "%u.%2.2u MiB",
> + x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
> + } else if (bytes > 1 << 10) {
> + int x = bytes + 5;  /* for rounding */
> + strbuf_addf(buf, "%u.%2.2u KiB",
> + x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
>   } else {
> - l -= snprintf(tp->display, l, ", %u bytes", (int)total);
> + strbuf_addf(buf, "%u bytes", (int)bytes);
>   }
> +}
>  
> - if (rate > 1 << 10) {
> - int x = rate + 5;  /* for rounding */
> - snprintf(tp->display + sizeof(tp->display) - l, l,
> -  " | %u.%2.2u MiB/s",
> -  x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
> - } else if (rate)
> - snprintf(tp->display + sizeof(tp->display) - l, l,
> -  " | %u KiB/s", rate);
> +static void throughput_string(struct strbuf *buf, off_t total,
> +   unsigned int rate)
> +{
> + strbuf_addstr(buf, ", ");
> + humanize(buf, total);
> + strbuf_addstr(buf, " | ");
> + humanize(buf, rate * 1024);
> + strbuf_addstr(buf, "/s");
>  }
>  
>  void display_throughput(struct progress *progress, off_t total)
> @@ -183,6 +185,7 @@ void display_throughput(struct progress *progress, off_t 
> total)
>   misecs += (int)(tv.tv_usec - tp->prev_tv.tv_usec) / 977;
>  
>   if (misecs > 512) {
> + struct strbuf buf = STRBUF_INIT;
>   unsigned int count, rate;
>  
>   count = total - tp->prev_total;
> @@ -197,7 +200,9 @@ void display_throughput(struct progress *progress, off_t 
> total)
>   tp->last_misecs[tp->idx] = misecs;
>   tp->idx = (tp->idx + 1) % TP_IDX_MAX;
>  
> - throughput_string(tp, total, rate);
> + throughput_string(&buf, total, rate);
> + strncpy(tp->display, buf.buf, sizeof(tp->display));
> + strbuf_release(&buf);
>   if (progress->last_value != -1 && progress_update)
>   display(progress, progress->last_value, NULL);
>   }