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

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

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

> 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.