| Message ID | 20190401115217.3423-4-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/4] progress: make display_progress() return void | expand |
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
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? :)
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
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;
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(-)