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

[Leonard Ehrenfried]

Thanks for your comments.

I suspect that I accidentally deleted the lines when I merged in the changes
from the mainline into my branch - it's my first time using bzr.

I will correct the mistakes and resubmit the patch for reconsideration.

Lenni


On Tue, Jan 25, 2011 at 5:10 PM, Giuseppe Scrivano <address@hidden>wrote:

> 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
>