Re: wget | Add libproxy support (!35)

[@rockdaboot]

Merge request https://gitlab.com/gnuwget/wget/-/merge_requests/35 was reviewed 
by Tim Rühsen

--
  
Tim Rühsen started a new discussion on src/retr.c: 
https://gitlab.com/gnuwget/wget/-/merge_requests/35#note_1593463007

> +       pxProxyFactory *pf = px_proxy_factory_new ();
> +       char direct[] = "direct://";
> +       int i;

Unused variable

--
  
Tim Rühsen started a new discussion on src/retr.c: 
https://gitlab.com/gnuwget/wget/-/merge_requests/35#note_1593463013

> +       if (!pf)
> +         {
> +            debug_logprintf (_("Allocating memory for libproxy failed"));

debug_logprintf format strings doesn't need translation, please remove `_()` 
everywhere in your changes.

--
  
Tim Rühsen started a new discussion on src/retr.c: 
https://gitlab.com/gnuwget/wget/-/merge_requests/35#note_1593463018

> +         {
> +           char *check = NULL;
> +           asprintf (&check , "%s", proxies[0]);

```suggestion:-1+0
           char *check = xstrdup (proxies[0]);
```

Hm, wait. Why using `proxies[0]` directly here instead of an heap allocation?

--
  
Tim Rühsen started a new discussion on src/retr.c: 
https://gitlab.com/gnuwget/wget/-/merge_requests/35#note_1593463031

> +    {
> +       pxProxyFactory *pf = px_proxy_factory_new ();
> +       char direct[] = "direct://";

You are using this only once, so no need for an extra variable.
So further down instead of `if (strcmp (check, direct) != 0)` use `if (strcmp 
(check, "direct://") != 0)`.
But maybe you wanted to do a `if (strncmp (check, direct, sizeof(direct)) != 
0)` ? (maybe even case insensitive?)

--
  
Tim Rühsen started a new discussion on src/retr.c: 
https://gitlab.com/gnuwget/wget/-/merge_requests/35#note_1593463038

> +               asprintf (&proxy, "%s", proxies[0]);
> +               debug_logprintf (_("libproxy setting to use '%s'\n"), proxy);
> +             }

```suggestion:-3+0
             debug_logprintf ("libproxy setting to use '%s'\n", proxies[0]);
```

--
  
Tim Rühsen started a new discussion on src/retr.c: 
https://gitlab.com/gnuwget/wget/-/merge_requests/35#note_1593463045

> +
> +       debug_logprintf (_("asking libproxy about url '%s'\n"), u->url);
> +       char **proxies = px_proxy_factory_get_proxies (pf, u->url);

Makes a check sense if `proxies` is set here? Not sure if it could be NULL or 
not.




-- 
Reply to this email directly or view it on GitLab: 
https://gitlab.com/gnuwget/wget/-/merge_requests/35
You're receiving this email because of your account on gitlab.com.