@matheusbsbs / matheus-bagliano / commits / 1bbcc224cc

http: refactor options to http_get_*

Over time, the http_get_strbuf function has grown several optional parameters. We now have a bitfield with multiple boolean options, as well as an optional strbuf for returning the content-type of the response. And a future patch in this series is going to add another strbuf option. Treating these as separate arguments has a few downsides: 1. Most call sites need to add extra NULLs and 0s for the options they aren't interested in. 2. The http_get_* functions are actually wrappers around 2 layers of low-level implementation functions. We have to pass these options through individually. 3. The http_get_strbuf wrapper learned these options, but nobody bothered to do so for http_get_file, even though it is backed by the same function that does understand the options. Let's consolidate the options into a single struct. For the common case of the default options, we'll allow callers to simply pass a NULL for the options struct. The resulting code is often a few lines longer, but it ends up being easier to read (and to change as we add new options, since we do not need to update each call site). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Jeff King committed Sep 28, 2013 at 04:31 UTC 1bbcc224ccfdccd99b1221d83150a8ffb8059ffd

4 files changed +44 -28

http-push.c

+2 -2

@@ -1543,7 +1543,7 @@ static int remote_exists(const char *path)

1543

1544 sprintf(url, "%s%s", repo->url, path);

1545

1546 - switch (http_get_strbuf(url, NULL, NULL, 0)) {

1546 + switch (http_get_strbuf(url, NULL, NULL)) {

1547 case HTTP_OK:

1548 ret = 1;

1549 break;

@@ -1567,7 +1567,7 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)

1567 url = xmalloc(strlen(repo->url) + strlen(path) + 1);

1568 sprintf(url, "%s%s", repo->url, path);

1569

1570 - if (http_get_strbuf(url, NULL, &buffer, 0) != HTTP_OK)

1570 + if (http_get_strbuf(url, &buffer, NULL) != HTTP_OK)

1571 die("Couldn't get %s for remote symref\n%s", url,

1572 curl_errorstr);

1573 free(url);

http.c

+25 -19

@@ -836,8 +836,9 @@ static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)

836 #define HTTP_REQUEST_STRBUF 0

837 #define HTTP_REQUEST_FILE 1

838

839 -static int http_request(const char *url, struct strbuf *type,

840 - void *result, int target, int options)

839 +static int http_request(const char *url,

840 + void *result, int target,

841 + const struct http_get_options *options)

842 {

843 struct active_request_slot *slot;

844 struct slot_results results;

@@ -870,9 +871,9 @@ static int http_request(const char *url, struct strbuf *type,

871 }

872

873 strbuf_addstr(&buf, "Pragma:");

873 - if (options & HTTP_NO_CACHE)

874 + if (options && options->no_cache)

875 strbuf_addstr(&buf, " no-cache");

875 - if (options & HTTP_KEEP_ERROR)

876 + if (options && options->keep_error)

877 curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);

878

879 headers = curl_slist_append(headers, buf.buf);

@@ -890,8 +891,9 @@ static int http_request(const char *url, struct strbuf *type,

891 ret = HTTP_START_FAILED;

892 }

893

893 - if (type)

894 - curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, type);

894 + if (options && options->content_type)

895 + curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,

896 + options->content_type);

897

898 curl_slist_free_all(headers);

899 strbuf_release(&buf);

@@ -900,11 +902,10 @@ static int http_request(const char *url, struct strbuf *type,

902 }

903

904 static int http_request_reauth(const char *url,

903 - struct strbuf *type,

905 void *result, int target,

905 - int options)

906 + struct http_get_options *options)

907 {

907 - int ret = http_request(url, type, result, target, options);

908 + int ret = http_request(url, result, target, options);

909 if (ret != HTTP_REAUTH)

910 return ret;

911

@@ -914,7 +915,7 @@ static int http_request_reauth(const char *url,

915 * making our next request. We only know how to do this for

916 * the strbuf case, but that is enough to satisfy current callers.

917 */

917 - if (options & HTTP_KEEP_ERROR) {

918 + if (options && options->keep_error) {

919 switch (target) {

920 case HTTP_REQUEST_STRBUF:

921 strbuf_reset(result);

@@ -923,15 +924,14 @@ static int http_request_reauth(const char *url,

924 die("BUG: HTTP_KEEP_ERROR is only supported with strbufs");

925 }

926 }

926 - return http_request(url, type, result, target, options);

927 + return http_request(url, result, target, options);

928 }

929

930 int http_get_strbuf(const char *url,

930 - struct strbuf *type,

931 - struct strbuf *result, int options)

931 + struct strbuf *result,

932 + struct http_get_options *options)

933 {

933 - return http_request_reauth(url, type, result,

934 - HTTP_REQUEST_STRBUF, options);

934 + return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);

935 }

936

937 /*

@@ -940,7 +940,8 @@ int http_get_strbuf(const char *url,

940 * If a previous interrupted download is detected (i.e. a previous temporary

941 * file is still around) the download is resumed.

942 */

943 -static int http_get_file(const char *url, const char *filename, int options)

943 +static int http_get_file(const char *url, const char *filename,

944 + struct http_get_options *options)

945 {

946 int ret;

947 struct strbuf tmpfile = STRBUF_INIT;

@@ -954,7 +955,7 @@ static int http_get_file(const char *url, const char *filename, int options)

955 goto cleanup;

956 }

957

957 - ret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);

958 + ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);

959 fclose(result);

960

961 if (ret == HTTP_OK && move_temp_to_file(tmpfile.buf, filename))

@@ -966,12 +967,15 @@ cleanup:

967

968 int http_fetch_ref(const char *base, struct ref *ref)

969 {

970 + struct http_get_options options = {0};

971 char *url;

972 struct strbuf buffer = STRBUF_INIT;

973 int ret = -1;

974

975 + options.no_cache = 1;

976 +

977 url = quote_ref_url(base, ref->name);

974 - if (http_get_strbuf(url, NULL, &buffer, HTTP_NO_CACHE) == HTTP_OK) {

978 + if (http_get_strbuf(url, &buffer, &options) == HTTP_OK) {

979 strbuf_rtrim(&buffer);

980 if (buffer.len == 40)

981 ret = get_sha1_hex(buffer.buf, ref->old_sha1);

@@ -1055,6 +1059,7 @@ add_pack:

1059

1060 int http_get_info_packs(const char *base_url, struct packed_git **packs_head)

1061 {

1062 + struct http_get_options options = {0};

1063 int ret = 0, i = 0;

1064 char *url, *data;

1065 struct strbuf buf = STRBUF_INIT;

@@ -1064,7 +1069,8 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)

1069 strbuf_addstr(&buf, "objects/info/packs");

1070 url = strbuf_detach(&buf, NULL);

1071

1067 - ret = http_get_strbuf(url, NULL, &buf, HTTP_NO_CACHE);

1072 + options.no_cache = 1;

1073 + ret = http_get_strbuf(url, &buf, &options);

1074 if (ret != HTTP_OK)

1075 goto cleanup;

1076

http.h

+10 -5

@@ -125,11 +125,16 @@ extern void append_remote_object_url(struct strbuf *buf, const char *url,

125 extern char *get_remote_object_url(const char *url, const char *hex,

126 int only_two_digit_prefix);

127

128 -/* Options for http_request_*() */

129 -#define HTTP_NO_CACHE 1

130 -#define HTTP_KEEP_ERROR 2

128 +/* Options for http_get_*() */

129 +struct http_get_options {

130 + unsigned no_cache:1,

131 + keep_error:1;

132

132 -/* Return values for http_request_*() */

133 + /* If non-NULL, returns the content-type of the response. */

134 + struct strbuf *content_type;

135 +};

136 +

137 +/* Return values for http_get_*() */

138 #define HTTP_OK 0

139 #define HTTP_MISSING_TARGET 1

140 #define HTTP_ERROR 2

@@ -142,7 +147,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,

147 *

148 * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.

149 */

145 -int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options);

150 +int http_get_strbuf(const char *url, struct strbuf *result, struct http_get_options *options);

151

152 extern int http_fetch_ref(const char *base, struct ref *ref);

153

remote-curl.c

+7 -2

@@ -187,6 +187,7 @@ static struct discovery* discover_refs(const char *service, int for_push)

187 struct discovery *last = last_discovery;

188 char *refs_url;

189 int http_ret, maybe_smart = 0;

190 + struct http_get_options options;

191

192 if (last && !strcmp(service, last->service))

193 return last;

@@ -204,8 +205,12 @@ static struct discovery* discover_refs(const char *service, int for_push)

205 }

206 refs_url = strbuf_detach(&buffer, NULL);

207

207 - http_ret = http_get_strbuf(refs_url, &type, &buffer,

208 - HTTP_NO_CACHE | HTTP_KEEP_ERROR);

208 + memset(&options, 0, sizeof(options));

209 + options.content_type = &type;

210 + options.no_cache = 1;

211 + options.keep_error = 1;

212 +

213 + http_ret = http_get_strbuf(refs_url, &buffer, &options);

214 switch (http_ret) {

215 case HTTP_OK:

216 break;