[v2,3/4] progress: clear previous progress update dynamically
diff mbox series

Message ID 20190401115217.3423-4-szeder.dev@gmail.com
State New
Headers show
Series
  • [v2,1/4] progress: make display_progress() return void
Related show

Commit Message

SZEDER Gábor April 1, 2019, 11:52 a.m. UTC
When the progress bar includes throughput, its length can shorten as
the unit of display changes from KiB to MiB.  To cover up remnants of
the previous progress bar when such a change of units happens we
always print three spaces at the end of the progress bar.

Alas, covering only three characters is not quite enough: when both
the total and the throughput happen to change units from KiB to MiB in
the same update, then the progress bar's length is shortened by four
characters (or maybe even more!):

  Receiving objects:  25% (2901/11603), 772.01 KiB | 733.00 KiB/s
  Receiving objects:  27% (3133/11603), 1.43 MiB | 1.16 MiB/s   s

and a stray 's' is left behind.

So instead of hard-coding the three characters to cover, let's compare
the length of the current progress bar with the previous one, and
cover up as many characters as needed.

Sure, it would be much simpler to just print more spaces at the end of
the progress bar, but this approach is more future-proof, and it won't
print extra spaces when none are needed, notably when the progress bar
doesn't show throughput and thus never shrinks.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Jeff King April 1, 2019, 1:30 p.m. UTC | #1
On Mon, Apr 01, 2019 at 01:52:16PM +0200, SZEDER Gábor wrote:

> diff --git a/progress.c b/progress.c
> index 842db14b72..3149ecd460 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -84,6 +84,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)
>  	const char *tp;
>  	struct strbuf *counters_sb = &progress->counters_sb;
>  	int show_update = 0;
> +	int last_count_len = counters_sb->len;

I don't think it could matter here, as these are meant to be smallish
strings, but I think we should get into the habit of using size_t
consistently to hold string lengths.

It makes auditing for integer overflow problems much simpler (this is on
my mind as I happen to be tracing some bugs around this the past few
days).

(There are a few instances in the next patch, too. Other than this nit,
though, your series looks good to me).

-Peff
SZEDER Gábor April 1, 2019, 2:15 p.m. UTC | #2
On Mon, Apr 01, 2019 at 09:30:00AM -0400, Jeff King wrote:
> On Mon, Apr 01, 2019 at 01:52:16PM +0200, SZEDER Gábor wrote:
> 
> > diff --git a/progress.c b/progress.c
> > index 842db14b72..3149ecd460 100644
> > --- a/progress.c
> > +++ b/progress.c
> > @@ -84,6 +84,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)
> >  	const char *tp;
> >  	struct strbuf *counters_sb = &progress->counters_sb;
> >  	int show_update = 0;
> > +	int last_count_len = counters_sb->len;
> 
> I don't think it could matter here, as these are meant to be smallish
> strings, but I think we should get into the habit of using size_t
> consistently to hold string lengths.
> 
> It makes auditing for integer overflow problems much simpler (this is on
> my mind as I happen to be tracing some bugs around this the past few
> days).
> 
> (There are a few instances in the next patch, too. Other than this nit,
> though, your series looks good to me).

I started with using size_t, but then switched to int, because:

  - After a bit of arithmetic I had to compare to term_columns()'s int
    return value anyway (in the next patch).

  - The second hunk in this patch adds these lines:

        int clear_len = counters_sb->len < last_count_len ?
                        last_count_len - counters_sb->len : 0;
        fprintf(stderr, "%s: %s%-*s", progress->title,
                counters_sb->buf, clear_len, eol);

    Here 'clear_len' has to be int, because the printf() format "%-*s"
    expects an int, and otherwise -Werror=format= errors ensue.  OK,
    it could be size_t, but then it must be casted to an int upon
    passing it to fprintf(), and after the next patch there will be
    three such calls.

I could resend using size_t.  Should I resend using size_t? :)
Jeff King April 2, 2019, 2:27 p.m. UTC | #3
On Mon, Apr 01, 2019 at 04:15:42PM +0200, SZEDER Gábor wrote:

> > I don't think it could matter here, as these are meant to be smallish
> > strings, but I think we should get into the habit of using size_t
> > consistently to hold string lengths.
> > 
> > It makes auditing for integer overflow problems much simpler (this is on
> > my mind as I happen to be tracing some bugs around this the past few
> > days).
> > 
> > (There are a few instances in the next patch, too. Other than this nit,
> > though, your series looks good to me).
> 
> I started with using size_t, but then switched to int, because:
> 
>   - After a bit of arithmetic I had to compare to term_columns()'s int
>     return value anyway (in the next patch).
> 
>   - The second hunk in this patch adds these lines:
> 
>         int clear_len = counters_sb->len < last_count_len ?
>                         last_count_len - counters_sb->len : 0;
>         fprintf(stderr, "%s: %s%-*s", progress->title,
>                 counters_sb->buf, clear_len, eol);
> 
>     Here 'clear_len' has to be int, because the printf() format "%-*s"
>     expects an int, and otherwise -Werror=format= errors ensue.  OK,
>     it could be size_t, but then it must be casted to an int upon
>     passing it to fprintf(), and after the next patch there will be
>     three such calls.
> 
> I could resend using size_t.  Should I resend using size_t? :)

IMHO it's better to keep it as a size_t for as long as possible, and
then cast when we pass to printf, for a few reasons:

  1. The cast is made explicitly, so it calls attention to it.

  2. We know that a cast to int there can at worst produce truncated
     output, and not lead to any kind of memory error. (And if we really
     care about that, it's easy to convert it to an fwrite() at that
     point, though I would not bother in this case).

I think the comparison to "cols" is OK, because we are just checking
whether cols is smaller than us. If we assume that size_t is at least as
big as an int (which I think is a reasonable assumption to make, and
certainly holds true for all platforms I know) then there's no
possibility of logic errors.

I wouldn't even bother with a cast there. Probably -Wsign-compare would
complain, but we are pretty far from enabling that. And I think the
right solution is for term_columns() to return an unsigned, anyway. ;)

I admit all of this is academic enough that I can live with it either
way (there are definitely places where it is _not_ academic, so I am
mostly just trying to encourage a general style).

-Peff

Patch
diff mbox series

diff --git a/progress.c b/progress.c
index 842db14b72..3149ecd460 100644
--- a/progress.c
+++ b/progress.c
@@ -84,6 +84,7 @@  static void display(struct progress *progress, uint64_t n, const char *done)
 	const char *tp;
 	struct strbuf *counters_sb = &progress->counters_sb;
 	int show_update = 0;
+	int last_count_len = counters_sb->len;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
@@ -110,10 +111,11 @@  static void display(struct progress *progress, uint64_t n, const char *done)
 
 	if (show_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
-			const char *eol = done ? done : "   \r";
-
-			fprintf(stderr, "%s: %s%s", progress->title,
-				counters_sb->buf, eol);
+			const char *eol = done ? done : "\r";
+			int clear_len = counters_sb->len < last_count_len ?
+					last_count_len - counters_sb->len : 0;
+			fprintf(stderr, "%s: %s%-*s", progress->title,
+				counters_sb->buf, clear_len, eol);
 			fflush(stderr);
 		}
 		progress_update = 0;