Hi Kushagra,
I made a few tests with and without UTF-8 locale - and they all succeeded here. (@Darshit: Could you reproduce the test failures outside Travis ?) What about the '#ifdef HAVE_SSL' ? Don't we need the check always ? Imagine you have some secure cookies saved and then try a non-SSL version of Wget. Should the secure cookies eventually be overwritten in this case ? > As for testing the code, should I follow the way its being done in > Test-*.py files? Yes, please. Tim On Wednesday 27 January 2016 05:12:12 Kushagra Singh wrote: > Hi, > > You're absolutely right, I have merged the commits into one patch, and > removed the trailing whitespaces in the patch. Please find it attached, I > hope it's okay now. > > I have fixed the error which was breaking the build when configured with > "--without-ssl". I can't figure out why the build fails on the other two > cases. I am unable to replicate them as well. The tests which are failing > in the build are Test-cookie and Test-cookie-401, both of which run > successfully when I execute make check. Am I missing something here? > > As for testing the code, should I follow the way its being done in > Test-*.py files? > > Thanks, > Kush > > On Sun, Jan 24, 2016 at 6:14 PM, Darshit Shah <[email protected]> wrote: > > Hi Kushagra, > > > > Thanks for the patches! A couple of remarks on your patches before I > > dive into the code: > > > > 1. Please merge your changes into fewer, more logical patches. Making > > small patches when working locally is a good idea, but when you submit > > them for merging, they should be reorganized into logical changes. In > > the case of your patches, one adds the new function, and the next > > patch removes it. We don't need this in the git history. > > 2. Your patches have trailing whitespace in them. Please configure > > your editor to either alert you about this, or fix it silently. Most > > good text editors have these features. You can even configure your Git > > to complain if you try to commit anything with whitespace errors. > > Trailing whitespaces can be a major pain when merging larger patches > > or branches in the future. > > 3. A good patch would also contain a test case that fails before. but > > passes after the patch is applied. This way we can verify the > > correctness of the patch and ensure that no regressions occur in the > > future. > > > > Apart from these, the build failed on Travis with the --without-ssl > > configure option. That is an issue worth looking into. > > The other two builds on travis failed inside the multi-byte locale > > tests. It's probably some subtle bug, but we'll have to fix that as > > well. > > > > The code itself looks good. Will provide a deeper review once the > > larger, more obvious issues have been sorted. > > > > > > On 24 January 2016 at 12:38, Kushagra Singh > > > > <[email protected]> wrote: > > > I have added the first two out the three recommendations in the draft. > > > > The > > > > > third one is relevant when cookies have to be removed in case the total > > > number of cookies hit a predefined upper bound, I'm not sure whether we > > > do that in wget? > > > > > > As you mentioned, I had to change some method prototypes to get the uri > > > scheme. I made sure that I replaced all instances of those function > > > calls > > > with the right call. The tests run fine, so hopefully I haven't broken > > > anything. > > > > > > I am attaching the patch files, please review them. > > > > > > Thanks, > > > Kush > > > > > > On Sun, Jan 24, 2016 at 4:39 AM, Darshit Shah <[email protected]> wrote: > > >> On 23 January 2016 at 23:36, Kushagra Singh > > >> > > >> <[email protected]> wrote: > > >> > Thanks a lot for the help! > > >> > > > >> > I've made some progress, but have a couple of more questions > > >> > > > >> > - I can't manage to find the http-only-flag in the cookie struct, do > > > > we > > > > >> not > > >> > > >> > store this? > > >> > > >> Since Wget supports only HTTP, this is not required. The HttpOnly > > >> attribute prevents access to script code, but since Wget never > > >> executes them it is not necessary at all. Although, it may be a good > > >> idea to explicitly store the flag for Wget saves the cookies to a > > >> file. Maybe, we should add this. > > >> > > >> > - The draft asks to check whether the "scheme" component of the > > >> > "request-uri" denotes a secure protocol or not. Currently I am > > > > checking > > > > >> > using "#ifdef HAVE_SSL". I am not sure whether this is the right way > > > > to > > > > >> do > > >> > > >> > so, since having SSL with wget does not necessarily mean that the > > > > current > > > > >> > connection is secure. > > >> > > >> Ideally, a code base should have as few #ifdef statements as possible. > > >> They make reading the code very difficult for a human. That said, in > > >> this scenario it is the absolute wrong technique. You will want to > > >> access the scheme from the request URI. Find a way to access this > > >> information, you may need to change some method prototypes to make > > >> this happen. > > >> > > >> > - To check whether there exists a cookie whose domain, domain-matches > > > > the > > > > >> > domain of a new cookie, we should iterate through the chains returned > > > > by > > > > >> > find_chains_of_host right? > > >> > > >> That ought to work, I think. > > >> > > >> > Regards, > > >> > Kush > > >> > > >> -- > > >> Thanking You, > > >> Darshit Shah > > > > -- > > Thanking You, > > Darshit Shah
