Re: [Bug-wget] [PATCH] replaced read_whole_file() by getline()

[Giuseppe Scrivano]

thanks for the patch.  I have few comments:


Tim Rühsen <address@hidden> writes:

> diff --git a/src/ChangeLog b/src/ChangeLog
> index 84a9645..3b6733e 100644
> --- a/src/ChangeLog
> +++ b/src/ChangeLog
> @@ -1,5 +1,10 @@
>  2013-05-09  Tim Ruehsen  <address@hidden>
>  
> +     * cookies.c, ftp-ls.c, ftp.c, init.c, netrc.c, utils.c, utils.h:
> +     replaced read_whole_file() by getline().
> +
> +2013-05-09  Tim Ruehsen  <address@hidden>
> +

please fix it specifying the functions too, as done in the ChangeLog file.


> diff --git a/src/ftp-ls.c b/src/ftp-ls.c
> index 3056651..401ae77 100644
> --- a/src/ftp-ls.c
> +++ b/src/ftp-ls.c
> @@ -68,16 +68,17 @@ symperms (const char *s)
>     replaces all <TAB> character with <SPACE>. Returns the length of the
>     modified line. */
>  static int
> -clean_line(char *line)
> +clean_line(char *line, int len)

since you are touching it, please add a space before '('

> +  while ((len = getline (&line, &bufsize, fp)) > 0)
>      {
> -      len = clean_line (line);
> +      len = clean_line (line, len);
>        /* Skip if total...  */
>        if (!strncasecmp (line, "total", 5))
> -        {
> -          xfree (line);
>            continue;
> -        }

please fix the indentation for the "continue" statement.


>        /* Get the first token (permissions).  */
>        tok = strtok (line, " ");
>        if (!tok)
> -        {
> -          xfree (line);
>            continue;
> -        }
>  

same here.

> +             i = clean_line (line, i);
> +             if (i <= 0)
> +                      continue; /* Ignore blank line. */

and here.


> +
> +             if ((j == 0) && (line[i - 1] == ']'))
> +               {
> +                      /* Found Directory heading line.  Next non-blank line
> +                      is significant. */
> +                      j = 1;
> +               }
> +             else if (!strncmp (line, "Total of ", 9))
> +               {
> +                      /* Found "Total of ..." footing line.  No valid data
> +                              will follow (empty directory). */
> +                      i = 0; /* Arrange for early exit. */
> +                      break;
> +               }
> +             else
> +               {
> +                      break; /* Must be significant data. */
> +               }
>      }

please not use tabs to indent here.


> -          if (!line)
> +                      i = getline (&line, &bufsize, fp);

and here.

> +          if (i <= 0)
>              {
>                DEBUGP (("EOF.  Leaving listing parser.\n"));
>                break;
> @@ -853,14 +828,14 @@ ftp_parse_vms_ls (const char *file)
>            /* Second line must begin with " ".  Otherwise, it's a first
>               line (and we may be confused).
>            */
> +                      i = clean_line (line, i);

here too.


> -              for (i = 0; i < strlen( tok); i++)
> +              for (i = 0; i < strlen(tok); i++)

thanks :-)

> -      if (line != NULL)
> +             i = getline (&line, &bufsize, fp);

tab.


> diff --git a/src/ftp.c b/src/ftp.c
> index b585631..fbfbe8f 100644
> --- a/src/ftp.c
> +++ b/src/ftp.c
> @@ -1367,18 +1367,20 @@ Error in server response, closing control 
> connection.\n"));
>          logprintf (LOG_ALWAYS, "%s: %s\n", con->target, strerror (errno));
>        else
>          {
> -          char *line;
> -          /* The lines are being read with read_whole_line because of
> +          char *line = NULL;
> +                      size_t bufsize = 0;
> +                      ssize_t len;

it doesn't seem correctly indented.

Except these minor comments, it seems correct.  Thanks again for your 
contribution.

-- 
Giuseppe