Maciej W. Rozycki wrote:
On Tue, 24 Jul 2007, Matthew Woehlke wrote:
This rough patch adds a '--ask-password' option to wget. About all that can be

It's a good rule to send patches inlined rather than attached as that makes them easy to comment on.

It's also a good rule to send patches as attachments so that dumb mailers don't break them :-). (And some mailers, e.g. Thunderbird, will inline text attachments when replying anyway.)

said for it is that it works; hopefully it will serve as a useful proof of
concept and possible a starting point for a fully-developed feature.

 Hmm, what's there to prove?  Anyway...

"Demonstration implementation"?

Index: src/http.c
===================================================================
--- src/http.c   (revision 2295)
+++ src/http.c   (working copy)
@@ -31,6 +31,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <termios.h> /* FIXME probably not portable? */
 #ifdef HAVE_UNISTD_H
 # include <unistd.h>
 #endif

 AC_SYS_POSIX_TERMIOS

@@ -1419,7 +1421,29 @@
   passwd = u->passwd;
   search_netrc (u->host, (const char **)&user, (const char **)&passwd, 0);
   user = user ? user : (opt.http_user ? opt.http_user : opt.user);
+  if (opt.ask_passwd)
+    {
+      int in_fd = fileno(stdin);

Standard input is used for URLs; you probably want to use "stderr" here.

Read from stderr? I admit I've heard stderr is bi-directional but I can't say I've ever seen it used to read input.

Formatting (ditto all the function calls below).

I both dislike GNU formatting and am not familiar with it, so obviously someone that understands it will have to fix this.

+      printf("URL \"%s\"\nPassword for user \"%s\": ", u->url, user);

Standard output is used for storing remote files; you definitely want to use "stderr" here.

Good point.

+      /* TODO check error */
+      tcgetattr(in_fd, &attr);

 isatty() beforehand perhaps?

Hmm, good point #2. I did say "it works" not "it's a good patch", right? ;-)

Though I am not sure whether it is really needed given that many years have passed and nobody wanted such a feature.

Which strikes me as odd, really, but I would guess not so many people use wget for sites that need a password (I never did until recently), at least not on non-private computers. I.e. if they do, they're ok with the existing security risks.

Otherwise it looks OK, I think.
But the decision is up to the maintainer (once you sort out technical problems).

I very much doubt this patch will be accepted, there are huge problems with it yet (like, it should persist the password, work with ftp, etc). It was meant more as a starting point, and for commentary... and you've made some good comments, thanks!

--
Matthew
Microsoft: Expect the unexpected

Reply via email to