[3/5] progress: assemble percentage and counters in a strbuf before printing
diff mbox series

Message ID 20190325103844.26749-4-szeder.dev@gmail.com
State New
Headers show
Series
  • Progress display fixes
Related show

Commit Message

SZEDER Gábor March 25, 2019, 10:38 a.m. UTC
The following patches in this series want to handle the progress bar's
title and changing parts (i.e. the counter and the optional percentage
and throughput combined) differently.

To prepare for those changes assemble the changing parts in a separate
strbuf before printing.

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

Comments

Jeff King March 26, 2019, 5:45 a.m. UTC | #1
On Mon, Mar 25, 2019 at 11:38:42AM +0100, SZEDER Gábor wrote:

> The following patches in this series want to handle the progress bar's
> title and changing parts (i.e. the counter and the optional percentage
> and throughput combined) differently.
> 
> To prepare for those changes assemble the changing parts in a separate
> strbuf before printing.

Makes sense. Unlike the previous patch, we're already in the "slow path"
of the function here, so we don't need to worry about adding an extra
buffer (though do still think it's worth reusing the same buffer each
time, as you do here).

-Peff
SZEDER Gábor March 27, 2019, 10:24 a.m. UTC | #2
On Tue, Mar 26, 2019 at 01:45:41AM -0400, Jeff King wrote:
> On Mon, Mar 25, 2019 at 11:38:42AM +0100, SZEDER Gábor wrote:
> 
> > The following patches in this series want to handle the progress bar's
> > title and changing parts (i.e. the counter and the optional percentage
> > and throughput combined) differently.
> > 
> > To prepare for those changes assemble the changing parts in a separate
> > strbuf before printing.
> 
> Makes sense. Unlike the previous patch, we're already in the "slow path"
> of the function here, so we don't need to worry about adding an extra
> buffer (though do still think it's worth reusing the same buffer each
> time, as you do here).

The commit message doesn't mention this, but the next patch needs the
length of the previously displayed progress bar to properly clean up
its remnants.  Or the length of its changing parts anyway.  So I could
either add a 'prev_len' field to 'struct progress', or the whole
strbuf.  The strbuf containing the throughput is already stored in
there and reused, so I just followed suit.
Jeff King March 28, 2019, 2:12 a.m. UTC | #3
On Wed, Mar 27, 2019 at 11:24:51AM +0100, SZEDER Gábor wrote:

> On Tue, Mar 26, 2019 at 01:45:41AM -0400, Jeff King wrote:
> > On Mon, Mar 25, 2019 at 11:38:42AM +0100, SZEDER Gábor wrote:
> > 
> > > The following patches in this series want to handle the progress bar's
> > > title and changing parts (i.e. the counter and the optional percentage
> > > and throughput combined) differently.
> > > 
> > > To prepare for those changes assemble the changing parts in a separate
> > > strbuf before printing.
> > 
> > Makes sense. Unlike the previous patch, we're already in the "slow path"
> > of the function here, so we don't need to worry about adding an extra
> > buffer (though do still think it's worth reusing the same buffer each
> > time, as you do here).
> 
> The commit message doesn't mention this, but the next patch needs the
> length of the previously displayed progress bar to properly clean up
> its remnants.  Or the length of its changing parts anyway.  So I could
> either add a 'prev_len' field to 'struct progress', or the whole
> strbuf.  The strbuf containing the throughput is already stored in
> there and reused, so I just followed suit.

Ah, right, that makes perfect sense. It is doubly a good idea, then. :)

-Peff

Patch
diff mbox series

diff --git a/progress.c b/progress.c
index b57c0dae16..390d2487ca 100644
--- a/progress.c
+++ b/progress.c
@@ -36,6 +36,7 @@  struct progress {
 	unsigned delay;
 	struct throughput *throughput;
 	uint64_t start_ns;
+	struct strbuf counters_sb;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -80,7 +81,9 @@  static int is_foreground_fd(int fd)
 
 static void display(struct progress *progress, uint64_t n, const char *done)
 {
-	const char *eol, *tp;
+	const char *tp;
+	struct strbuf *counters_sb = &progress->counters_sb;
+	int update = 0;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
@@ -93,25 +96,31 @@  static void display(struct progress *progress, uint64_t n, const char *done)
 	}
 
 	tp = (progress->throughput) ? progress->throughput->display.buf : "";
-	eol = done ? done : "   \r";
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s",
-				progress->title, percent,
-				(uintmax_t)n, (uintmax_t)progress->total,
-				tp, eol);
-			fflush(stderr);
-			progress_update = 0;
-			return;
+
+			strbuf_reset(counters_sb);
+			strbuf_addf(counters_sb,
+				    "%3u%% (%"PRIuMAX"/%"PRIuMAX")%s", percent,
+				    (uintmax_t)n, (uintmax_t)progress->total,
+				    tp);
+			update = 1;
 		}
 	} else if (progress_update) {
-		fprintf(stderr, "%s: %"PRIuMAX"%s%s",
-			progress->title, (uintmax_t)n, tp, eol);
+		strbuf_reset(counters_sb);
+		strbuf_addf(counters_sb, "%"PRIuMAX"%s", (uintmax_t)n, tp);
+		update = 1;
+	}
+
+	if (update) {
+		const char *eol = done ? done : "   \r";
+
+		fprintf(stderr, "%s: %s%s", progress->title, counters_sb->buf,
+			eol);
 		fflush(stderr);
 		progress_update = 0;
-		return;
 	}
 
 	return;
@@ -213,6 +222,7 @@  static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->delay = delay;
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
+	strbuf_init(&progress->counters_sb, 0);
 	set_progress_signal();
 	return progress;
 }
@@ -256,6 +266,7 @@  void stop_progress_msg(struct progress **p_progress, const char *msg)
 		free(buf);
 	}
 	clear_progress_signal();
+	strbuf_release(&progress->counters_sb);
 	if (progress->throughput)
 		strbuf_release(&progress->throughput->display);
 	free(progress->throughput);