On Mon, Jun 17, 2013 at 2:05 AM, Giuseppe Scrivano wrote:
> Thanks, the code seems correct but since you are going to write a lot of
> code during your Summer of Code project, I will be very pedantic. This
> time I have just amended the changes before push your patch.
>
>
> Darshit Shah writes:
Thanks, the code seems correct but since you are going to write a lot of
code during your Summer of Code project, I will be very pedantic. This
time I have just amended the changes before push your patch.
Darshit Shah writes:
> Please be aware that Wget needs to know the size of the POST data
> please follow the GNU Coding standards here. Empty space between
> function name and '(' and also leave spaces around the operator !=.
>
> Sorry for missing these. Fixed in all the relevant locations in the patch.
> Could you please also add some comments in the documentation?
>
Added the chan
thanks for your contribution, some comments inline:
Darshit Shah writes:
> - if (statcode == HTTP_STATUS_TEMPORARY_REDIRECT)
> -return NEWLOCATION_KEEP_POST;
> + switch (statcode)
> + {
> +case HTTP_STATUS_TEMPORARY_REDIRECT:
> + re
Attaching a patch that should (hopefully) fix all the problems listed here.
I have tested it and it looks good to me.
On Thu, May 9, 2013 at 9:55 AM, Darshit Shah wrote:
>
> > We currently suspend any method if the response code is a non 307
>> > redirect. That I believe is wrong. We should onl
> > We currently suspend any method if the response code is a non 307
> > redirect. That I believe is wrong. We should only be suspending the
> > POST method and not others.
>
> It sounds good, but we must be careful also with 303.
>
> Do you agree that the matrix for the method to use on the next
Darshit Shah writes:
>
> We suspend the post data only when we receive a 307, or do you
> mean we
> shouldn't suspend in this case too?
>
> It's the other way round. A 307 response code is used when the server
> wishes to explicitly ask the client to not suspend. And that is
>
> We suspend the post data only when we receive a 307, or do you mean we
> shouldn't suspend in this case too?
>
> It's the other way round. A 307 response code is used when the server
wishes to explicitly ask the client to not suspend. And that is also the
current behaviour.
We currently suspen
Darshit Shah writes:
> While DELETE *may* be redirected, the document did not mention
> anything explicitly about it. From what I gauge, we should not suspend
> a DELETE command upon a redirect.
We suspend the post data only when we receive a 307, or do you mean we
shouldn't suspend in this case
> This paragraph:
>
> "The 307 (Temporary Redirect) status code indicates that the target
> resource resides temporarily under a different URI and the user agent
> MUST NOT change the request method if it performs an automatic
> redirection to that URI. Since the redirection can change over tim
Darshit Shah writes:
>while I agree that your patch makes the code more readable and it is
>
> good to apply it, I don't see how the previous version was broken.
> What
> compiler have you used?
>
>I see you pushed the patch. But have we gotten to the bottom of the
>cau
>
> while I agree that your patch makes the code more readable and it is
> good to apply it, I don't see how the previous version was broken. What
> compiler have you used?
>
> I see you pushed the patch. But have we gotten to the bottom of the cause?
If the result changes with the GCC version, we
Darshit Shah writes:
> I also think some more explicit sanity checks are needed in the code.
> Like sending data with a HEAD request makes absolutely no sense (does
> it?).
no, it doesn't, same for GET.
--
Giuseppe
Hi Giuseppe,
while I agree that your patch makes the code more readable and it is
> good to apply it, I don't see how the previous version was broken. What
> compiler have you used?
>
This occurs when compiling on gcc 4.8.0. The following output ensues:
Setting --method (method) to head
DEBUG o
Hi Darshit,
Darshit Shah writes:
> The patch supplied by you adding cmd_uppercase_String seems to break
> the header generation since (*q++) causes the pointer to increment
> before the c_toupper() method returns.
>
> We should instead increment it only after the statement successfully
> retur
Hi Giuseppe,
The patch supplied by you adding cmd_uppercase_String seems to break the
header generation since (*q++) causes the pointer to increment before the
c_toupper() method returns.
We should instead increment it only after the statement successfully
returns.
I have also converted the loop
Daniel Stenberg writes:
> On Thu, 2 May 2013, Giuseppe Scrivano wrote:
>
>> RFC 2606 doesn't seem very clear about it, and I can't find anywhere
>> that PUT/OPTIONS/ANYTHING should be handled differently than POST
>> wrt redirections. I don't see why suspending a PUT request would be
>> incorrec
On Thu, 2 May 2013, Giuseppe Scrivano wrote:
RFC 2606 doesn't seem very clear about it, and I can't find anywhere that
PUT/OPTIONS/ANYTHING should be handled differently than POST wrt
redirections. I don't see why suspending a PUT request would be incorrect.
Darshit, do you have any pointer?
Gijs van Tulder writes:
> Hi Darshit,
>
> Darshit Shah wrote:
>> Unless we explicitly check for opt.method = POST, this will cause a lot
>> of issues. Since the macro is being called on every redirect, even
>> HEAD/PUT/OPTIONS headers will get suspended to give way to GET. That is
>> not the beha
Hi Darshit,
Darshit Shah wrote:
> Unless we explicitly check for opt.method = POST, this will cause a lot
> of issues. Since the macro is being called on every redirect, even
> HEAD/PUT/OPTIONS headers will get suspended to give way to GET. That is
> not the behaviour we want.
Ah, I see it's mor
>
> Still, even if the sanitization is removed: I think it would be better if
> RESTORE_POST_DATA restores the previous value of opt.method, instead of
> overwriting it with a hardcoded "POST". Isn't it?
>
> As double safety yes. Maybe we should do that.
> A related question: how is a redirect re
Hi Giuseppe,
Dropping the bit that sanitizes the opt.method is probably a good idea.
(Perhaps I shouldn't have replied to your patch directly.)
Still, even if the sanitization is removed: I think it would be better
if RESTORE_POST_DATA restores the previous value of opt.method, instead
of ov
On Thu, May 2, 2013 at 12:56 AM, Gijs van Tulder wrote:
> Giuseppe Scrivano wrote:
> > what about this patch? Any comment?
>
> Another suggestion: why not save the original opt.method, set opt.method
> to NULL and put the original opt.method back later?
>
> Gijs
>
Unless we explicitly check for
hi Gijs,
Gijs van Tulder writes:
> Giuseppe Scrivano wrote:
>> what about this patch? Any comment?
>
> Another suggestion: why not save the original opt.method, set
> opt.method to NULL and put the original opt.method back later?
thanks for your suggestion but I think we should drop the code t
Giuseppe Scrivano wrote:
> what about this patch? Any comment?
Another suggestion: why not save the original opt.method, set opt.method
to NULL and put the original opt.method back later?
Gijs
diff --git a/src/retr.c b/src/retr.c
index d51b7e7..2aee578 100644
--- a/src/retr.c
+++ b/src/retr
Am Mittwoch, 1. Mai 2013 schrieb Darshit Shah:
> First, sorry for the quick and dirty hack which was the perfect example of
> how NOT to do things.
Than it was a good example ;-)
> Secondly, it lies upon me that this feature wasn't tested before submitting
> the patch. I had however relied on the
Darshit Shah writes:
> I am fixing this issue, but it is a terribly ugly hack. If someone could
> help improve it I'd be most truly grateful.
> I have a couple of ideas, but I will need to work them out and implement
> them when I have the time.
>
> The reason it has to be so ugly is that, we can
Couple of things.
First, sorry for the quick and dirty hack which was the perfect example of
how NOT to do things.
Secondly, it lies upon me that this feature wasn't tested before submitting
the patch. I had however relied on the Test Environment and since it passed
everything there, I thought it
Hi Darshit,
I understand that your patch was just a quick hack.
But even than you should avoid doing
opt.method == "POST"
for string comparisons.
This definitely not portable.
Not every compiler/linker aggregates two occurrences of the same static string
into one address in memory.
You
On 04/30/2013 07:01 PM, Darshit Shah wrote:
> Okay, I could not prevent myself from looking into it.
>
> It seems as if the SUSPEND_POST_DATA macro was being called during a
> recursive download attempt.
>
> Attaching a hack around the situation. Will look more deeply when I have
> more time to i
Okay, I could not prevent myself from looking into it.
It seems as if the SUSPEND_POST_DATA macro was being called during a
recursive download attempt.
Attaching a hack around the situation. Will look more deeply when I have
more time to identify what caused the regression.
On Tue, Apr 30, 2013
I'm in the middle of University exams at the moment. I'll still have a look
at it tomorrow when I get a breather.
However, it looks like wget is converting any method to a POST request
which is weird since that should have caused it to fail most of the tests.
I'll have to look into it and check w
Hi,
you can even reproduce it with a simple
wget -r http://translationproject.org/latest/make
Darshit, maybe you can have a look at it. It has something to do with
opt.method (set to read-only "POST" in http.c, line 1772).
Even with --method=GET opt.method points to "POST". And the code
Here is the reproducer:
$ wget --passive-ftp -nv --recursive --level=1 --no-directories --no-parent \
--no-check-certificate -A '*.po'
http://translationproject.org/latest/make
2013-04-30 16:41:25 URL:https://translationproject.org/latest/make/
[5489/5489] -> "make" [1]
Segmentati
34 matches
Mail list logo
