Re: [Bug-wget] [PATCH v2] wget: Add --use-askpass support

[Liam R. Howlett]

* Steven Schubiger <address@hidden> [160725 16:49]:
> For what it is worth,
> 
> Liam R. Howlett <address@hidden> wrote:
> > +void
> > +run_use_askpass(char *question, char **answer)
> 
> Use a space between the function name and opening parenthesis.
> 
> > +  char * const argv[] = {question, NULL};
> 
> Ideally a space after opening and before closing brace.
> 
> > +  if (pipe(com) == -1)
> 
> Insert a space between function name and opening parenthesis.
> 
> > +  {
> > +    fprintf(stderr, _("Cannot create pipe"));
> > +    exit (WGET_EXIT_GENERIC_ERROR);
> > +  }
> 
> Indent both braces with two spaces.
> 
> Ideally :-), this "should" be also applied to the rest of the patch
> to be generally consistent with the remaining code.
> 
> Of course, correct me if I'm wrong.
> 
> -- 
> The truth may be out there,
> but lies are inside your head.
>     -- Terry Pratchett

Thank you Steven.  I will fix these issues in my next revision.

Ángel, I was not CC'ed on your response and so I will address your
comments here as well:


> I like that better. Still, perhaps allowing the parameter to provide the
> binary would be beneficial for configurating in .wgetrc instead of
> requiring an env variable.

I'd like to keep the same functionality as the git implementation as
the goal here is to keep the user experience as close as possible.  I'm
not against adding a .wgetrc option that could also be checked for the
external application.

> 
> 
> There are a few spots that require C99 (char * const argv, unsigned int
> bytes), but I'm unsure if we ending up dropping C90 support or not.

I'd be happy to do this - however I need help doing so.  The issue is
that changing char * question to a const causes compile issues with the
char * const argv[] discarding the const qualifier.  argv must be of
this type for the call to posix_spawn.  The only way I've found to be
C99 and avoid compile issues is to pass char **question, but then it's
being passed in so that it can be edited only to avoid complaints on
compile and for C99 which seems less than ideal.

> 
> 
> There's a possible overflow of question in use_askpass sprintf calls. I
> recommend you to change it to snprintf. Also, those prompts should be
> wrapped with _() for localisation.

Thank you, I will fix this.

> 
> 
> If you use posix_spawnp, you don't call execlp.

Again, thanks - I'll fix this and the code will be cleaner.

> 
> There are a few spacing issues. Eg. compare your getenvs.

Thanks, Steven also pointed out a lot of spacing issues that I hadn't
noticed.  I'll address all of these.



> 
> Regards