[pacman-dev] [PATCH] Fixes bug FS 59201
Signed-off-by: Sever Oraz <severoraz@gmail.com> --- src/pacman/callback.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/pacman/callback.c b/src/pacman/callback.c index ee75297c..c0f5ade1 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -837,16 +837,16 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) xfered_human = humanize_size(xfered, '\0', -1, &xfered_label); printf(" %ls%-*s ", wcfname, padwid, ""); - /* We will show 1.62M/s, 11.6M/s, but 116K/s and 1116K/s */ + /* We will show 1.62 MiB/s, 11.6 MiB/s, but 116 KiB/s and 1116 KiB/s */ if(rate_human < 9.995) { - printf("%6.1f %3s %4.2f%c/s ", - xfered_human, xfered_label, rate_human, rate_label[0]); + printf("%6.1f %3s %4.2f %3s/s ", + xfered_human, xfered_label, rate_human, rate_label); } else if(rate_human < 99.95) { - printf("%6.1f %3s %4.1f%c/s ", - xfered_human, xfered_label, rate_human, rate_label[0]); + printf("%6.1f %3s %4.1f %3s/s ", + xfered_human, xfered_label, rate_human, rate_label); } else { - printf("%6.1f %3s %4.f%c/s ", - xfered_human, xfered_label, rate_human, rate_label[0]); + printf("%6.1f %3s %4.f %3s/s ", + xfered_human, xfered_label, rate_human, rate_label); } if(eta_h == 0) { printf("%02u:%02u", eta_m, eta_s); -- 2.20.1
On 10/2/19 10:11 pm, Sever Oraz wrote:
Signed-off-by: Sever Oraz <severoraz@gmail.com>
For context, bug 59201 points out that the K/s used in the download speed is unclear. We have no idea what K means - is it KB or KiB? Looking at actual output: archlinux-keyring-2... 685.1 KiB 694K/s 00:01 [######################] 100% libsystemd-240.93-1... 388.0 KiB 693K/s 00:01 [######################] 100% cryptsetup-2.1.0-1-... 392.7 KiB 865K/s 00:00 [######################] 100% Clearly it is *iB and not *B, just like the unit 6 characters to the left. I'm not keen on removing two columns (plus a space in this patch) from the progress bar to add this in. Also, this patch is clearly not tested. I now get this: :: Retrieving packages... systemd-240.93-1-x86_64 0.0 B 0.00 B/s 00:00 [----------------------] systemd-240.93-1-x86_64 80.0 KiB 117 KiB/s 00:33 [----------------------] systemd-240.93-1-x86_64 160.0 KiB 196 KiB/s 00:19 [----------------------] systemd-240.93-1-x86_64 256.0 KiB 283 KiB/s 00:13 [#---------------------] systemd-240.93-1-x86_64 352.0 KiB 336 KiB/s 00:10 [#---------------------] systemd-240.93-1-x86_64 448.0 KiB 384 KiB/s 00:09 [##--------------------] systemd-240.93-1-x86_64 543.9 KiB 416 KiB/s 00:08 [##--------------------] systemd-240.93-1-x86_64 672.0 KiB 484 KiB/s 00:06 [###-------------------] systemd-240.93-1-x86_64 783.9 KiB 504 KiB/s 00:06 [####------------------] systemd-240.93-1-x86_64 928.0 KiB 560 KiB/s 00:05 [#####-----------------] systemd-240.93-1-x86_64 1071.9 KiB 592 KiB/s 00:04 [#####-----------------] systemd-240.93-1-x86_64 1232.0 KiB 645 KiB/s 00:04 [######----------------] systemd-240.93-1-x86_64 1391.9 KiB 696 KiB/s 00:03 [#######---------------] systemd-240.93-1-x86_64 1568.0 KiB 740 KiB/s 00:03 [########--------------] Allan
Hello Allan, thank you very much for responding to my patch submission. The unit in every measurement should be clearly written and comply with existing norms. The only way to actually prove (without assumptions) that the speed is actually in KiB/s, in your example, would be to measure for an interval of known length the data transfer speed and the difference in transferred data. It is not uncommon for data sizes to be in IEC byte multiples, and transfer speeds to be in bits per second. K/s has been certainly used before by someone without high regard for norm compliance to refer to kilobits per second. Additionally, it seems to me that as long as there is a progress bar spanning multiple columns, the information provided by a correctly formatted download speed is more important than the difference in visual aid 3 columns less in an already long progress bar will make. The space between a value and a unit is also required by norms ISO IEC 80000, and is of standard use in the International System of Units. Could you please clarify what is not working in the provided output? Thank you very much for your input. Sever
On 2/10/19 9:03 AM, Sever Oraz wrote:
Could you please clarify what is not working in the provided output?
Did you try building and running it? That's one single package being formatted there. For a lark I tried it with texlive-core, which is somewhat of an extreme case at 152.51 MiB worth of download. The progress bar for that one spanned over 200 lines in my terminal, with every couple of couple of lines slowly advancing the progressbar by one. This is happening because the padding is off, and therefore the percentage indicator is overflowing the screen and advancing to the next line. As a result every time pacman updates the progressbar it writes the update to the next line down, rather than overwriting the current line. -- Eli Schwartz Bug Wrangler and Trusted User
Signed-off-by: Sever Oraz <severoraz@gmail.com> --- src/pacman/callback.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/pacman/callback.c b/src/pacman/callback.c index ee75297c..4ee17d13 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -800,9 +800,11 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) fname[len] = '\0'; /* 1 space + filenamelen + 1 space + 6 for size + 1 space + 3 for label + - * + 2 spaces + 4 for rate + 1 for label + 2 for /s + 1 space + - * 8 for eta, gives us the magic 30 */ - filenamelen = infolen - 30; + * + 2 spaces + 4 for rate + 1 space + 3 for label + 2 for /s + 1 space + + * 8 for eta, gives us the magic 33 */ + /* > filename 1000.0 MiB 1000 KiB/s HH:MM:SS + * >1 1 61 3 2 41 3 21 8 */ + filenamelen = infolen - 33; /* see printf() code, we omit 'HH:' in these conditions */ if(eta_h == 0 || eta_h >= 100) { filenamelen += 3; @@ -837,16 +839,16 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) xfered_human = humanize_size(xfered, '\0', -1, &xfered_label); printf(" %ls%-*s ", wcfname, padwid, ""); - /* We will show 1.62M/s, 11.6M/s, but 116K/s and 1116K/s */ + /* We will show 1.62 MiB/s, 11.6 MiB/s, but 116 KiB/s and 1116 KiB/s */ if(rate_human < 9.995) { - printf("%6.1f %3s %4.2f%c/s ", - xfered_human, xfered_label, rate_human, rate_label[0]); + printf("%6.1f %3s %4.2f %3s/s ", + xfered_human, xfered_label, rate_human, rate_label); } else if(rate_human < 99.95) { - printf("%6.1f %3s %4.1f%c/s ", - xfered_human, xfered_label, rate_human, rate_label[0]); + printf("%6.1f %3s %4.1f %3s/s ", + xfered_human, xfered_label, rate_human, rate_label); } else { - printf("%6.1f %3s %4.f%c/s ", - xfered_human, xfered_label, rate_human, rate_label[0]); + printf("%6.1f %3s %4.f %3s/s ", + xfered_human, xfered_label, rate_human, rate_label); } if(eta_h == 0) { printf("%02u:%02u", eta_m, eta_s); -- 2.20.1
On 2/12/19 5:27 PM, Sever Oraz wrote:
Signed-off-by: Sever Oraz <severoraz@gmail.com> ---
The new patch seems to have the advertised result without side effects. As a general rule when it comes to submitting patches, the commit message should not record the fact that it is a second attempt, nor how the first attempt was buggy. I would suggest a commit message like: ``` Use standard, consistent units in the download progress. Rather than use M/s which can be either MB or MiB, specify that it uses MiB (consistent with the displayed total size). Fixes FS#59201 ``` Comments that are pertinent to the mailing list discussion but not to the commit message can be inserted after the --- and before the diff header, as such: --- [additional comments go here] src/pacman/callback.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) git send-email has an --annotate option that lets you add comments before submitting. -- Eli Schwartz Bug Wrangler and Trusted User
Thanks for the feedback Eli, noted! -- Sever Oraz
On 13/2/19 8:27 am, Sever Oraz wrote:
Signed-off-by: Sever Oraz <severoraz@gmail.com> ---
Accepting this and using Eli's suggested commit message. Additional comment below.
src/pacman/callback.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c index ee75297c..4ee17d13 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -800,9 +800,11 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) fname[len] = '\0';
/* 1 space + filenamelen + 1 space + 6 for size + 1 space + 3 for label + - * + 2 spaces + 4 for rate + 1 for label + 2 for /s + 1 space + - * 8 for eta, gives us the magic 30 */ - filenamelen = infolen - 30; + * + 2 spaces + 4 for rate + 1 space + 3 for label + 2 for /s + 1 space + + * 8 for eta, gives us the magic 33 */ + /* > filename 1000.0 MiB 1000 KiB/s HH:MM:SS + * >1 1 61 3 2 41 3 21 8 */
I am removing this two line addition - it repeats details in the two line comment above it, and the line with numbers is very confusing (e.g. 61 is 6 and 1)
+ filenamelen = infolen - 33; /* see printf() code, we omit 'HH:' in these conditions */ if(eta_h == 0 || eta_h >= 100) { filenamelen += 3; @@ -837,16 +839,16 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) xfered_human = humanize_size(xfered, '\0', -1, &xfered_label);
printf(" %ls%-*s ", wcfname, padwid, ""); - /* We will show 1.62M/s, 11.6M/s, but 116K/s and 1116K/s */ + /* We will show 1.62 MiB/s, 11.6 MiB/s, but 116 KiB/s and 1116 KiB/s */ if(rate_human < 9.995) { - printf("%6.1f %3s %4.2f%c/s ", - xfered_human, xfered_label, rate_human, rate_label[0]); + printf("%6.1f %3s %4.2f %3s/s ", + xfered_human, xfered_label, rate_human, rate_label); } else if(rate_human < 99.95) { - printf("%6.1f %3s %4.1f%c/s ", - xfered_human, xfered_label, rate_human, rate_label[0]); + printf("%6.1f %3s %4.1f %3s/s ", + xfered_human, xfered_label, rate_human, rate_label); } else { - printf("%6.1f %3s %4.f%c/s ", - xfered_human, xfered_label, rate_human, rate_label[0]); + printf("%6.1f %3s %4.f %3s/s ", + xfered_human, xfered_label, rate_human, rate_label); } if(eta_h == 0) { printf("%02u:%02u", eta_m, eta_s);
participants (3)
-
Allan McRae
-
Eli Schwartz
-
Sever Oraz