Re: [Bug-wget] Conditional GET requests

[Hubert Tarasiuk]

W dniu 18.05.2015 o 21:34, Tim Rühsen pisze:
> In file included from http.c:32:0:
> wget.h:335:32: warning: comma at end of enumerator list [-Wpedantic]
>    IF_MODIF_SINCE       = 0x0080,        /* use if-modified-since
header */
I have repaired this problem. And by the way do you know how to silence
the variable-sized array GCC warnings without silencing other ISO
warnings (like the one above)?


> Is there any reason to abbreviate MODIFIED to MODIF ? If not,
> IF_MODIFIED_SINCE is slightly more readable, at least to me.
> Same goes to the opt member variable.
Not really. I changed it to if_modified_since.

I am having some doubts about this part of code:
>   /* Handle the case when server ignores If-Modified-Since header.  */
>   if (cond_get && statcode == HTTP_STATUS_OK && hs->remote_time)
>     {
>       time_t tmr;
>       tmr = http_atotm (hs->remote_time);
>       if (tmr != (time_t) -1 && tmr <= hs->orig_file_tstamp
>           && (contlen == -1 || contlen == hs->orig_file_size))
>         {
>           logprintf (LOG_VERBOSE, _("Server ignored If-Modified-Since header "
>                                     "for file %s. "
>                                     "You might want to add 
> --no-if-modified-since "
>                                     "option.\n\n"), quote(hs->local_file));
>           *dt |= RETROKF;
>           CLOSE_INVALIDATE (sock);
>           retval = RETRUNNEEDED;
>           goto cleanup;
>         }
>     }

Do you think that it is ok? Specifically in warc mode. It is kind of
fall-back to old-style timestamping (there would be no body at all in
response to head request), but I am wondering if it is ok to discard
response body in this case (when warc mode is enabled). Or perhaps
should we store it as usual?

Thank you for the review,
Hubert

W dniu 18.05.2015 o 21:34, Tim Rühsen pisze:
> Hi Hubert,
> 
> you patches look very good now.
> 
> I tested them and had a quick look on the changes.
> 
> Just could find these very minor points:
> 
> In file included from http.c:32:0:
> wget.h:335:32: warning: comma at end of enumerator list [-Wpedantic]
>    IF_MODIF_SINCE       = 0x0080,        /* use if-modified-since header */
> 
> Is there any reason to abbreviate MODIFIED to MODIF ? If not, 
> IF_MODIFIED_SINCE is slightly more readable, at least to me.
> Same goes to the opt member variable.
> 
> Regards, Tim
> 
> Am Montag, 18. Mai 2015, 13:38:33 schrieb Hubert Tarasiuk:
>> Sorry, I found and fixed another spelling error.
>>
>> W dniu 18.05.2015 o 13:11, Hubert Tarasiuk pisze:
>>> I have reworked my patches. Specifically:
>>> 1) --if-modified-since option is enabled by default and has only effect
>>> in timestamping mode. And yes, --no-if-modified-since is added
>>> automatically.
>>> 2) I added all legal date formats to my test.
>>> 3) I added another case to my test (local file is strictly newer than
>>> remote file).
>>> 3) If time_to_rfc1123 fails, there is simple fall back behavior.
>>> 4) I added work around behavior for servers ignoring If-Modified-Since
>>> (like for example our Perl test server).
>>>
>>> Patches are attached here as well as on Github for easy viewing.
>>> https://github.com/jy987321/Wget/commits/master-hubert
>>>
>>> Thank you,
>>> Hubert
>>>
>>> W dniu 14.05.2015 o 22:35, Hubert Tarasiuk pisze:
>>>> W dniu 14.05.2015 o 21:12, Tim Rühsen pisze:
>>>>> Am Donnerstag, 14. Mai 2015, 15:43:54 schrieb Hubert Tarasiuk:
>>>>>> W dniu 13.05.2015 o 13:28, Ander Juaristi pisze:
>>>>>>> And second, I'm not really sure whether --condget is the best name for
>>>>>>> the switch.
>>>>>>> Requests that include any of If-Unmodified-Since, If-Match,
>>>>>>> If-None-Match, or If-Range
>>>>>>> header fields are also "conditional GETs" as well.
>>>>>>> We might want to implement one of those in the future and we'd be
>>>>>>> forced
>>>>>>> to choose a name which could easily be
>>>>>>> inconsistent/confusing with --condget. Or maybe we won't. But we don't
>>>>>>> know that now, so I think
>>>>>>> it's better to choose a switch more specific to the fact that an
>>>>>>> If-Modified-Since header will be sent
>>>>>>> so as to avoid confusion.
>>>>>>
>>>>>> Do you have an idea for a better switch name that would not be too
>>>>>> long?
>>>>>> I have noticed that issue earlier, but could not think of a better name
>>>>>> that would not be too long. :D
>>>>>>
>>>>>> Thank you for the suggestions,
>>>>>
>>>>> Hi Hubert,
>>>>>
>>>>> why not --if-modified-since as a boolean option ?
>>>>
>>>> Sounds good.
>>>>
>>>>> I personally would set it to true by default, since it is a very
>>>>> common/basic HTTP 1.1 header.
>>>>
>>>> Ok, I will name the option "--no-if-modified-since" and will enable that
>>>> by default.
>>>>
>>>>> Regards, Tim

0001-Implement-timestamp-support-for-local-files-in-teste.patch
Description: Text Data

0002-Support-conditional-GET-in-testenv-server.patch
Description: Text Data

0003-Add-test-for-condget-requests.patch
Description: Text Data

0004-Add-if-modified-since-option.patch
Description: Text Data

0005-Prototype-of-If-Modified-Since.patch
Description: Text Data

0006-Include-if-modified-since-option-in-user-manual.patch
Description: Text Data

signature.asc
Description: OpenPGP digital signature