Re: [Bug-wget] broken progressbar in 1.16

[Darshit Shah]

On 11/15, Darshit Shah wrote:
> On 11/07, Darshit Shah wrote:
> > The last patch breaks the download but is only available on the git
> > repository and will not be included in any release.
> >
> > I will be pushing a patch soon which will remove all assert statements from
> > production releases so that the downloads continue with a minor visual
> > annoyance.
> >
> > Because real life is catching up, I've very little time till the end of
> > this year. But I'm going to try and debug this issue and fix it asap.
>
> And eventually I found time to look into this with a fresh mind.
> It turns out that I have been looking at the code in a wrong fashion, 
> and hence the assumption I wished to validate with the assert was 
> wrong too. A simple off-by-one error that made my life living hell.
>
> I've fixed the issue with the following patch. Please take a look at it.
>
> I really hope there's no more issues remaining with the progress bar.
Since no one objected, pushed!

> > > On 06-Nov-2014 7:02 pm, "Noël Köthe" <address@hidden> wrote:
> > > > Hello,
> > > >
> > > > Am Montag, den 03.11.2014, 10:12 +0530 schrieb Darshit Shah:
> > > >
> > > > > >e-flightgear-201411   0%[                      ]   1,05M   286KB/s
> > eta 1h 41m
> > > > > >wget: progress.c:1161: create_image: Проверочное утверждение
> > «count_cols (bp->buffer) <= bp->width» не выполнено.
> > > > > >zsh: abort      wget -c
> > > > > >
> > > > > >(it's [Assertion "count_cols (bp->buffer) <= bp->width" failed] I
> > think)
> > > > > This bug keeps getting more interesting and surprising. While the
> > progress bar
> > > > > draws correctly for the original file you mentioned, the assertion
> > fails with
> > > > > the new ISO (live-flightgear). The funny thing is, this bug is now
> > firing based
> > > > > on the filename of the file being downloaded, which is not something I
> > expect
> > > > > since the filename section of the progress bar is isolated from the
> > rest and
> > > > > should not create such an effect.
> > > > >
> > > > > I'm going to spend some more time debugging this issue.
> > > >
> > > > I applied the progress bar patch on 1.16 and run into the same problem.
> > > >
> > > > Connecting to fly.osdn.org.ua (fly.osdn.org.ua)|212.40.45.6|:80...
> > connected.
> > > > HTTP request sent, awaiting response... 206 Partial Content
> > > > Length: 1599602688 (1.5G), 1590685248 (1.5G) remaining
> > [application/octet-stream]
> > > > Saving to: 'live-flightgear-20141101-x86_64.iso'
> > > >
> > > > ar-20141101-x86_64.   0%[                      ]  12.47M  1.41MB/s
> >      wget: progress.c:1161: create_image: Assertion `count_cols
> > (bp->buffer) <= bp->width' failed.
> > > > Looks like shipping wget with the broken progress bar because the patch
> > > > could stop downloading.
> > > > Maybe the default could be the old but stable progress bar and when the
> > > > new feature works it could be changed to the new default?
> > > >
> > > > Regards
> > > >
> > > >        Noël
> >
> > Addendum
> > ==========
> >
> > I added the assertion there to ensure that the basic assumption about the
> > progress bar is always held. When I added the assert statement, the only
> > time it was expected to fail was when wget is executed using a non English
> > locale.
> >
> > However, through my own experience and via other reports on this mailing
> > list, I have to realize that the assert fails even on English locale and
> > when the progress bar displays correctly. The fact that the progress bar
> > manages to print on a single line even when the assertion that
> > `no_of_cols(progress_bar) <= columns_in_line` fails means that there is a
> > subtle bug hidden deep in the implementation which is beyond the changes I
> > made. It also implies that we do not currently know how this code works
> > since it performs contrary to our expectation.
> >
> > This is definitely a high priority issue that needs more investigation and
> > a fix as soon as possible.
> --- end quoted text ---
>
> --
> Thanking You,
> Darshit Shah

> From 94805ad55ae53039717a7f705fe41f0cbe0d8ebc Mon Sep 17 00:00:00 2001
> From: Darshit Shah <address@hidden>
> Date: Sat, 15 Nov 2014 00:13:13 +0530
> Subject: [PATCH 1/2] Fix progress bar assertion
>
> ---
> src/ChangeLog  |  7 ++++++-
> src/progress.c | 10 +++++++++-
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/src/ChangeLog b/src/ChangeLog
> index 32eebda..d085953 100644
> --- a/src/ChangeLog
> +++ b/src/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-11-15  Darshit Shah  <address@hidden>
> +
> +       * progress.c (create_image): Fix assertion that checks progress bar 
> length
> +       Should fix bug #43593
> +
> 2014-11-12  Tim Ruehsen  <address@hidden>
>
>         * openssl.c (ssl_init): Fix error handling for CRL loading
> @@ -7,7 +12,7 @@
>         * html-parse.c (map_html_tags): Fix range check
>
> 2014-11-11  Tim Ruehsen  <address@hidden>
> - 
> +
>         * openssl.c (ssl_init): Load CRL file given by --crl-file
>
> 2014-11-11  Tim Ruehsen  <address@hidden>
> diff --git a/src/progress.c b/src/progress.c
> index a0b48e4..d23654d 100644
> --- a/src/progress.c
> +++ b/src/progress.c
> @@ -1158,7 +1158,15 @@ create_image (struct bar_progress *bp, double 
> dl_total_time, bool done)
>   while (p - bp->buffer - bytes_cols_diff < bp->width)
>     *p++ = ' ';
>   *p = '\0';
> -  assert (count_cols (bp->buffer) <= bp->width);
> +
> +  /* 2014-11-14  Darshit Shah  <address@hidden>
> +   * Assert that the length of the progress bar is lesser than the size of the
> +   * screen with which we are dealing. This assertion *MUST* always be removed
> +   * from the release code since we do not want Wget to crash and burn when the
> +   * assertion fails. Instead Wget should continue downloading and display a
> +   * horrible and irritating progress bar that spams the screen with newlines.
> +   */
> +  assert (count_cols (bp->buffer) <= bp->width + 1);
> }
>
> /* Print the contents of the buffer as a one-line ASCII "image" so
> --
> 2.1.3



--- end quoted text ---

--
Thanking You,
Darshit Shah

pgpSvLGQErZBG.pgp
Description: PGP signature