Re: [PATCH] http-push: refactor request url creation

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 28, 2009, 22:51 UTC
Message-ID: <7vvdrzjb80.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <4980D9FF.7020307@gmail.com>

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

Show 13 quoted lines

> Currently, functions that deal with objects on the remote repository have to allocate and do strcpys to generate the URL.
>
> This patch saves them this trouble, by providing two functions, "append_remote_object_url" and "get_remote_object_url".
>
> Both generate a URL, with either the object's 2-digit hex directory (eg. /objects/a1/), or the complete object location (eg. /objects/a1/b2).
>
> However, they differ in that "append_remote_object_url" appends this URL to a strbuf, while "get_remote_object_url" wraps around the former and returns the URL directly in char*. Users usually would use "get_remote_object_url", but may find "append_remote_object_url" useful if they require further string operations on the URL.
>
> PS. In "start_fetch_loose", the variable "url" which is passed to "curl_easy_setopt" is removed, and in its place "request->url" is used. This is safe, since curl strdup's it.
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Acked-by: Junio C Hamano <gitster@pobox.com>

What's with these loooooooooooooooooooooooooooooong lines?

I thought at least you did not have these overlong lines in your earlier attempts, and Dscho may have acked one of those, but I doubt he would give his Ack to this one. I certainly wouldn't Ack it myself.

By the way, aren't you sending format="flowed"? Please don't. It damages whitespaces.

Daniel Stenberg did a research on the safety of your "since curl stdrup's it" claim, and found that it unsafe for earlier versions of the library before 7.17.0.

It seems that we earlier found out that anything older than 7.16 were not usable for git-http-push (see Release Notes for 1.5.4), but 7.16 is still older than 7.17, so either we declare you _must_ have 7.17 or newer to use http-push, or keep an extra copy around and free it later like the original code does.

Even Debian is at 7.18.2 these days, so requiring 7.17 or newer may not be an issue in practice, but there are people who keep running things on older distros with proven stability (and known features limitation).

The refactoring looked sane otherwise, but I think we would want to opt for safety by keeping an extra string around.

Thanks.