Hi Jonathan,

On Mon, 19 Aug 2013, Jonathan Nieder wrote:

> >> At [1] there's an attempt of a patch that basically strdup()s the
> >> password and username, altough it doesn't yet pass the test suite:
> >
> > I would like to see a patch that removes the fixed length limit, sure!
>
> Thanks. Here goes.

I wanted to churp up and provide my feedback as 1) I tend to look after the
email protocols and sasl code, 2) I added support for options in the URL /
Login details in curl v7.31 and know some of the URL code quite well and 3)
I also looked at removing the 255 character limit as part of the options
work but felt it was too much for that release - many thanks for tackling
this.

> That's the end of the series. Hopefully it was not too dull to read.
> Thoughts and other improvements welcome.

I have a couple of points:

* You seemed to have changed the use of Curl_safefree() to free() in the
sasl rework, when variables such as chlg and response could be NULL, but use
a mixture of free() and Curl_safefree() elsewhere. There are some cases when
the pointer is already being checked and as such free() could be more
appropriate but as Curl_safefree() also invalidates the pointer by setting
it to NULL I would recommend using this over free() - It then means you
don't need to set variables like "proxy" to NULL afterwards as
Curl_safefree() will do it for you.

* Why change the existing "if(failure), set return code, free and return" to
"if(failure), set return code, goto out and then free" ? Whilst the use of
goto is not mentioned in the coding standards web page isn't in keeping with
the curl style although there may be a few places it has been used to
simplify complex code - As such I would recommend against it and use the
existing coding style where possible. Additionally, if there is a valid
reason for changing the coding style could you please perform this as part
of a post fix tidy up in separate patches - thus keeping the fix and style
changes separate, which will allow easier reviewing and allow for the
dissecting of problems should there be any.

Additionally, is there any possibility you could attach your .patch files
rather than sending them as separate emails?

This way we don't have to try and compare emails with code and don't have to
try and save emails as .patch files where we can get the line endings wrong
or even incorrect line wrappings - From my own perspective I can then use
the built in review features of TortoiseGIT's ;-)

Many thanks

Steve
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2013-08-19