[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
0001-Display-progress-information-in-Windows-console-titl.patch
Description: Text Data