bug-wget
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Bug-wget] --progress should not be overridden by --quiet


From: Darshit Shah
Subject: Re: [Bug-wget] --progress should not be overridden by --quiet
Date: Sat, 3 May 2014 09:06:33 +0200

I just went through the patch your submitted. I've attached a copy of the
patch file as generated through git format-patch for anyone who may want to
try it out.

There's a few comments I have. Please find them inline.

Here is a patch that doesn't use any global data. My idea is that creating +

> free'ing a 'hurl' is inexpensive in terms of CPU and memory. So it could be
> done irrespective of 'opt.verbose'. Like so:
>
> diff -Hb -u3 ../Git-latest/src/ftp.c ./ftp.c
> --- ../Git-latest/src/ftp.c     2014-05-01 14:54:39 +0000
> +++ ./ftp.c     2014-05-01 16:11:13 +0000
> @@ -1594,6 +1594,8 @@
>   /* THE loop.  */
>   do
>     {
> +      char *hurl;
> +
>       /* Increment the pass counter.  */
>       ++count;
>       sleep_between_retrievals (count);
> @@ -1652,21 +1654,26 @@
>
>       /* Get the current time string.  */
>       tms = datetime_str (time (NULL));
> +
> +      hurl = url_string (u, URL_AUTH_HIDE_PASSWD);
> +
>       /* Print fetch message, if opt.verbose.  */
>       if (opt.verbose)
>         {
> -          char *hurl = url_string (u, URL_AUTH_HIDE_PASSWD);
>           char tmp[256];
>           strcpy (tmp, "        ");
>           if (count > 1)
>             sprintf (tmp, _("(try:%2d)"), count);
>           logprintf (LOG_VERBOSE, "--%s--  %s\n  %s => %s\n",
>                      tms, hurl, tmp, quote (locf));
> +        }
> +
> #ifdef WINDOWS
> +      if (!opt.quiet || opt.show_progress)
>           ws_changetitle (hurl);
> #endif
>           xfree (hurl);
> -        }
> +
>       /* Send getftp the proper length, if fileinfo was provided.  */
>       if (f && f->type != FT_SYMLINK)
>         len = f->size;
>
> diff -Hb -u3 ../Git-latest/src/http.c ./http.c
> --- ../Git-latest/src/http.c    2014-05-01 14:54:39 +0000
> +++ ./http.c    2014-05-01 16:11:09 +0000
> @@ -3088,6 +3088,8 @@
>   /* THE loop */
>   do
>     {
> +      char *hurl;
> +
>       /* Increment the pass counter.  */
>       ++count;
>       sleep_between_retrievals (count);
> @@ -3099,11 +3101,11 @@
>         logprintf (LOG_VERBOSE, _("\
> Spider mode enabled. Check if remote file exists.\n"));
>
> +      hurl = url_string (u, URL_AUTH_HIDE_PASSWD);
> +
>       /* Print fetch message, if opt.verbose.  */
>       if (opt.verbose)
>         {
> -          char *hurl = url_string (u, URL_AUTH_HIDE_PASSWD);
> -
>           if (count > 1)
>             {
>               char tmp[256];
> @@ -3116,12 +3118,13 @@
>               logprintf (LOG_NOTQUIET, "--%s--  %s\n",
>                          tms, hurl);
>             }
> +        }
>
> #ifdef WINDOWS
> +       if (!opt.quiet || opt.show_progress)
>           ws_changetitle (hurl);
> #endif
>           xfree (hurl);
> -        }
>
> -----------
>
> Would it not be a better idea to wrap this segment completely into a
if(opt.show_progress || opt.verbose) block?
That way, the hurl is generated only if it is required.

What I suggest is something along these lines:
diff --git a/src/ftp.c b/src/ftp.c
index c78256f..9315c8f 100644
--- a/src/ftp.c
+++ b/src/ftp.c
@@ -1594,7 +1594,6 @@ ftp_loop_internal (struct url *u, struct fileinfo *f,
ccon *con, char **local_fi
   /* THE loop.  */
   do
     {
-      char *hurl;

       /* Increment the pass counter.  */
       ++count;
@@ -1655,7 +1654,9 @@ ftp_loop_internal (struct url *u, struct fileinfo *f,
ccon *con, char **local_fi
       /* Get the current time string.  */
       tms = datetime_str (time (NULL));

-      hurl = url_string (u, URL_AUTH_HIDE_PASSWD);
+      if (opt.show_progress || opt.verbose)
+        {
+          char *hurl = url_string (u, URL_AUTH_HIDE_PASSWD);

           /* Print fetch message, if opt.verbose.  */
           if (opt.verbose)
@@ -1669,10 +1670,11 @@ ftp_loop_internal (struct url *u, struct fileinfo
*f, ccon *con, char **local_fi
             }

 #ifdef WINDOWS
-      if (!opt.quiet || opt.show_progress)
+          if (opt.show_progress)
             ws_changetitle (hurl);
 #endif
           xfree (hurl);
+        }

       /* Send getftp the proper length, if fileinfo was provided.  */
       if (f && f->type != FT_SYMLINK)

There's a few salient points in this I'd like to point out.
As of now, opt.verbose unconditionally imples that opt.show_progress is
set. This is because --no-show-progress does not currently work. However,
we still have the (opt.show_progress || opt.verbose) since, in the future
we may have --no-show-progress working and then the logic here should not
break.

But encapsulating this into a single block, we create and destroy the hurl
only of required.

I've also changed the conditional in the #ifdef WINDOWS section.
According to your original patch, the console title is updated when the
user invokes wget only with --no-verbose too. This is contrary to what I'd
expect since the progress bar isn't displayed in that use case. If I
understand correctly, you only want to update the console title when the
progress bar is being displayed, we only need check opt.show_progress for
that. Or did I miss something?

And the most important diff:
>
> diff -Hb -u3 ../Git-latest/src/retr.c ./retr.c
> --- ../Git-latest/src/retr.c    2014-05-01 14:54:39 +0000
> +++ ./retr.c    2014-05-01 15:54:30 +0000
> @@ -412,7 +412,7 @@
>       if (progress)
>         progress_update (progress, ret, ptimer_read (timer));
> #ifdef WINDOWS
> -      if (toread > 0 && opt.show_progress)
> +      if (toread > 0 && (opt.show_progress || !opt.quiet))
>
>         ws_percenttitle (100.0 *
>                          (startpos + sum_read) / (startpos + toread));
> #endif
>
> -----------
>
> As you see, my local files are not under Git-control. So it's hard for me
> to
> give you any "git diff" patches. And since I suspect src/Changelog is
> created from a mysterious git-command, I don't know how you want a
> change-entry from me. The 'git-format-patch' won't work here. But here is a
> suggestion:
>
> 2014-05-01  Gisle Vanem  <address@hidden>
>
>  * ftp.c (ftp_loop_internal): Call url_string() to retrieve a 'hurl'
> incase WINDOWS
>    needs to set the console-title.
>  * http.c (http_loop): Call url_string() to retrieve a 'hurl' incase
> WINDOWS
>     needs to set the console-title.
>  * retr.c (fd_read_body): Update the read-percentage for WINDOWS if
> '--show-progress' or NOT '--quiet' are set.
>
> But hopefully you get the idea.
>
>
>  Please do check the attached patch. It should fix the erroneous empty
>> lines you reported.
>>
>
> Looks fine now.
>
> --gv
>



-- 
Thanking You,
Darshit Shah

Attachment: 0001-Display-progress-information-in-Windows-console-titl.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]