Re: [Bug-wget] Please review my patch for bug #29518

[Giuseppe Scrivano]

Hi Leonard,

Leonard Ehrenfried <address@hidden> writes:

> I'd be greatful for a ruthless code review - it's the only way to learn!

I have some comments about your patch:
> 
> === modified file 'src/main.c'
> --- src/main.c        2011-01-01 12:19:37 +0000
> +++ src/main.c        2011-01-25 14:35:05 +0000
> @@ -54,7 +54,6 @@
>  #include "convert.h"
>  #include "spider.h"
>  #include "http.h"               /* for save_cookies */
> -#include "ptimer.h"

Why are you removing this line?


  
>  #include <getopt.h>
>  #include <getpass.h>
> @@ -164,7 +163,6 @@
>      { IF_SSL ("certificate-type"), 0, OPT_VALUE, "certificatetype", -1 },
>      { IF_SSL ("check-certificate"), 0, OPT_BOOLEAN, "checkcertificate", -1 },
>      { "clobber", 0, OPT__CLOBBER, NULL, optional_argument },
> -    { "config", 0, OPT_VALUE, "chooseconfig", -1 },

Same here.



> -    N_("\
> -       --config=FILE         Specify config file to use.\n"), 

And here.



> -  struct ptimer *timer = ptimer_new ();
>       [....]
> -      xfree (wall_time);
> -      xfree (download_time);


And this whole block?

Please fix your patch to don't modify code which is not part of your patch.



> === modified file 'src/options.h'
> --- src/options.h     2011-01-01 12:19:37 +0000
> +++ src/options.h     2011-01-25 14:35:05 +0000
> @@ -59,6 +59,7 @@
>    char *dir_prefix;          /* The top of directory tree */
>    char *lfilename;           /* Log filename */
>    char *input_filename;              /* Input filename */
> +  bool purge_input_file;     /* Remove donwloaded URLs from the input file */
>    char *choose_config;               /* Specified config file */
>    bool force_html;           /* Is the input file an HTML file? */
>  
> 
> === modified file 'src/retr.c'
> --- src/retr.c        2011-01-01 12:19:37 +0000
> +++ src/retr.c        2011-01-25 14:35:05 +0000
> @@ -922,7 +922,7 @@
>     If opt.recursive is set, call retrieve_tree() for each file.  */
>  
>  uerr_t
> -retrieve_from_file (const char *file, bool html, int *count)
> +retrieve_from_file (const char *file, bool html, bool purge, int *count)
>  {
>    uerr_t status;
>    struct urlpos *url_list, *cur_url;
> @@ -943,6 +943,7 @@
>        int dt,url_err;
>        uerr_t status;
>        struct url * url_parsed = url_parse(url, &url_err, iri, true);
> +      
>  
>        if (!url_parsed)
>          {
> @@ -987,6 +988,7 @@
>  
>    for (cur_url = url_list; cur_url; cur_url = cur_url->next, ++*count)
>      {
> +      
>        char *filename = NULL, *new_file = NULL;
>        int dt;
>        struct iri *tmpiri = iri_dup (iri);
> @@ -1024,6 +1026,62 @@
>                                 cur_url->url->url, &filename,
>                                 &new_file, NULL, &dt, opt.recursive, tmpiri,
>                                 true);
> +      /*
> +       * --purge-input-file handling
> +       */
> +      if(RETROK == status && purge)
> +      {
> +        FILE *infile;
> +        infile = fopen( input_file, "r");
> +        char line[2000];

Please remove this magic number and use instead the readline module
offered by gnulib.

Please format all your code following the GNU coding standards:

  http://www.gnu.org/prep/standards/standards.html#Formatting


I would like to release wget as soon as possible, I don't think we will
be able to include your patch in the next wget release; the good thing
is that you'll have more time to work on it ;-)

I'll send you in another e-mail details to start the copyright
assignment process to the FSF (the main reason why I think we will not
be able to include your patch in the next release...).

Cheers,
Giuseppe