Re: [PATCH 2/2] remote-curl.c: fix rpc_out()

From: Shawn O. Pearce <spearce@spearce.org>
Date: Nov 23, 2009, 21:04 UTC
Message-ID: <20091123210415.GH11919@spearce.org>
In-Reply-To: <20091123110338.2b230359.rctay89@gmail.com>

If I understand the change right, the only thing that really matters here is removing the extra ';' in the if (max < avail) condition.

That bug was the only reason why rpc->len < rpc->pos, and is thus the only reason why avail would "go negative" (which it can't do since its unsigned, it just wrapped around to a massive value).

Tay Ray Chuan <rctay89@gmail.com> wrote:

> Use comparisons between rpc->len and rpc->pos, rather than computing
> their difference. This avoids potential errors when this value is
> negative and we access it.

I don't see this as being very relevant here.

> Use an int to store the return value from packet_read_line(), instead
> of a size_t.

Why? The code is actually harder to follow. packet_read_line does not return a negative value.

> Handle the errorneous condition where rpc->pos exceeds rpc->len by
> printing a message and aborting the transfer (return 0).

This condition should be impossible.  It only occurred because of
the bug you were trying to fix, which is this one next item:

> Remove extraneous semicolon (';') at the end of the if statement, that
> prevented code in its block from executing.

Right.  This bug is really bad and should be fixed.  But the message
above make this some little after-thought and doesn't explain what
was wrong with this being here.

> ---
> 
>   Users might experience issues when pushing with chunked encoding when
>   size_t avail is negative.

Shouldn't this be in the commit message?  Or something about how
pushing with chunked encoding was broken if the push was larger
than the default buffer size of 1 MiB?

-- 
Shawn.