Re: [PATCH 2/4] Use strbuf in http code

[Junio C Hamano <gitster@pobox.com>]

Mike Hommey <mh@glandium.org> writes:

> Also, replace whitespaces with tabs in some places
>
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>
>  While testing this, I noticed 3 things:
>  - CURL_MULTI makes the code very racy
>  - a lot of the code doesn't do anything useful without CURL_MULTI
>  - the code is redundant

Yeah, there does seem to be a lot of duplicated code that does common
setup with slightly different request string.

> @@ -1115,16 +1109,11 @@ static char *quote_ref_url(const char *base, const char *ref)
>  
>  int fetch_ref(char *ref, unsigned char *sha1)
>  {
> -        char *url;
> -        char hex[42];
> -        struct buffer buffer;
> +	char *url;
> +	struct strbuf buffer = STRBUF_INIT;
>  	char *base = remote->url;
>  	struct active_request_slot *slot;
>  	struct slot_results results;
> -        buffer.size = 41;
> -        buffer.posn = 0;
> -        buffer.buffer = hex;
> -        hex[41] = '\0';
>  
>  	url = quote_ref_url(base, ref);
>  	slot = get_active_slot();
> @@ -1142,9 +1131,9 @@ int fetch_ref(char *ref, unsigned char *sha1)
>  		return error("Unable to start request");
>  	}
>  
> -        hex[40] = '\0';
> -        get_sha1_hex(hex, sha1);
> -        return 0;
> +	buffer.buf[40] = '\0';
> +	get_sha1_hex(buffer.buf, sha1);
> +	return 0;
>  }

A conversion like this is worrysome and needs to be rethought I think.

At least with the old code, we knew hex[40] was a safe location to make
assignment to, even though we did not check if what it contained made
sense --- the other end might have had a garbage in that URL (but the
caller hopefully would be responsible for noticing that).  But with your
change, I do not think you have that guarantee.  fwrite_buffer() may
have extended the buffer using strbuf API, but it may have received less
than what you are expecting, in which case you may not have buf[40]
touchable for you, no?

I at the same time think the original code is buggy.  It initializes
buffer.buffer to the on-stack storage hex[], but lets fwrite_buffer() to
call xrealloc() on it.