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>

diff --git a/http-push.c b/http-push.c
index 6dad188b5f31d47fdce86e5104aa3c6b899ebed7..09ebb8f48c80dac22bd5c7cab77e1cfcf19933ed 100644 (file)
--- a/http-push.c
+++ b/http-push.c
@@ -1543,7 +1543,7 @@ static int remote_exists(const char *path)
 
        sprintf(url, "%s%s", repo->url, path);
 
-       switch (http_get_strbuf(url, NULL, NULL, 0)) {
+       switch (http_get_strbuf(url, NULL, NULL)) {
        case HTTP_OK:
                ret = 1;
                break;
@@ -1567,7 +1567,7 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
        url = xmalloc(strlen(repo->url) + strlen(path) + 1);
        sprintf(url, "%s%s", repo->url, path);
 
-       if (http_get_strbuf(url, NULL, &buffer, 0) != HTTP_OK)
+       if (http_get_strbuf(url, &buffer, NULL) != HTTP_OK)
                die("Couldn't get %s for remote symref\n%s", url,
                    curl_errorstr);
        free(url);

diff --git a/http.c b/http.c
index 202468a9807bbbddec6474ed2518e3b13c4f8098..6bb2e456ff7a26f33038bde8acc4dd9089cbb858 100644 (file)
--- a/http.c
+++ b/http.c
@@ -836,8 +836,9 @@ static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 #define HTTP_REQUEST_STRBUF    0
 #define HTTP_REQUEST_FILE      1
 
-static int http_request(const char *url, struct strbuf *type,
-                       void *result, int target, int options)
+static int http_request(const char *url,
+                       void *result, int target,
+                       const struct http_get_options *options)
 {
        struct active_request_slot *slot;
        struct slot_results results;
@@ -870,9 +871,9 @@ static int http_request(const char *url, struct strbuf *type,
        }
 
        strbuf_addstr(&buf, "Pragma:");
-       if (options & HTTP_NO_CACHE)
+       if (options && options->no_cache)
                strbuf_addstr(&buf, " no-cache");
-       if (options & HTTP_KEEP_ERROR)
+       if (options && options->keep_error)
                curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 
        headers = curl_slist_append(headers, buf.buf);
@@ -890,8 +891,9 @@ static int http_request(const char *url, struct strbuf *type,
                ret = HTTP_START_FAILED;
        }
 
-       if (type)
-               curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, type);
+       if (options && options->content_type)
+               curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,
+                               options->content_type);
 
        curl_slist_free_all(headers);
        strbuf_release(&buf);
@@ -900,11 +902,10 @@ static int http_request(const char *url, struct strbuf *type,
 }
 
 static int http_request_reauth(const char *url,
-                              struct strbuf *type,
                               void *result, int target,
-                              int options)
+                              struct http_get_options *options)
 {
-       int ret = http_request(url, type, result, target, options);
+       int ret = http_request(url, result, target, options);
        if (ret != HTTP_REAUTH)
                return ret;
 
@@ -914,7 +915,7 @@ static int http_request_reauth(const char *url,
         * making our next request. We only know how to do this for
         * the strbuf case, but that is enough to satisfy current callers.
         */
-       if (options & HTTP_KEEP_ERROR) {
+       if (options && options->keep_error) {
                switch (target) {
                case HTTP_REQUEST_STRBUF:
                        strbuf_reset(result);
@@ -923,15 +924,14 @@ static int http_request_reauth(const char *url,
                        die("BUG: HTTP_KEEP_ERROR is only supported with strbufs");
                }
        }
-       return http_request(url, type, result, target, options);
+       return http_request(url, result, target, options);
 }
 
 int http_get_strbuf(const char *url,
-                   struct strbuf *type,
-                   struct strbuf *result, int options)
+                   struct strbuf *result,
+                   struct http_get_options *options)
 {
-       return http_request_reauth(url, type, result,
-                                  HTTP_REQUEST_STRBUF, options);
+       return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);
 }
 
 /*
@@ -940,7 +940,8 @@ int http_get_strbuf(const char *url,
  * If a previous interrupted download is detected (i.e. a previous temporary
  * file is still around) the download is resumed.
  */
-static int http_get_file(const char *url, const char *filename, int options)
+static int http_get_file(const char *url, const char *filename,
+                        struct http_get_options *options)
 {
        int ret;
        struct strbuf tmpfile = STRBUF_INIT;
@@ -954,7 +955,7 @@ static int http_get_file(const char *url, const char *filename, int options)
                goto cleanup;
        }
 
-       ret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);
+       ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);
        fclose(result);
 
        if (ret == HTTP_OK && move_temp_to_file(tmpfile.buf, filename))
@@ -966,12 +967,15 @@ static int http_get_file(const char *url, const char *filename, int options)
 
 int http_fetch_ref(const char *base, struct ref *ref)
 {
+       struct http_get_options options = {0};
        char *url;
        struct strbuf buffer = STRBUF_INIT;
        int ret = -1;
 
+       options.no_cache = 1;
+
        url = quote_ref_url(base, ref->name);
-       if (http_get_strbuf(url, NULL, &buffer, HTTP_NO_CACHE) == HTTP_OK) {
+       if (http_get_strbuf(url, &buffer, &options) == HTTP_OK) {
                strbuf_rtrim(&buffer);
                if (buffer.len == 40)
                        ret = get_sha1_hex(buffer.buf, ref->old_sha1);
@@ -1055,6 +1059,7 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 
 int http_get_info_packs(const char *base_url, struct packed_git **packs_head)
 {
+       struct http_get_options options = {0};
        int ret = 0, i = 0;
        char *url, *data;
        struct strbuf buf = STRBUF_INIT;
@@ -1064,7 +1069,8 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)
        strbuf_addstr(&buf, "objects/info/packs");
        url = strbuf_detach(&buf, NULL);
 
-       ret = http_get_strbuf(url, NULL, &buf, HTTP_NO_CACHE);
+       options.no_cache = 1;
+       ret = http_get_strbuf(url, &buf, &options);
        if (ret != HTTP_OK)
                goto cleanup;
 

diff --git a/http.h b/http.h
index d77c1b54f24d142461b9e1a0d11cf6de041bc59f..40404b4836b5599bff75badc9e762d6e8594665f 100644 (file)
--- a/http.h
+++ b/http.h
@@ -125,11 +125,16 @@ extern void append_remote_object_url(struct strbuf *buf, const char *url,
 extern char *get_remote_object_url(const char *url, const char *hex,
                                   int only_two_digit_prefix);
 
-/* Options for http_request_*() */
-#define HTTP_NO_CACHE          1
-#define HTTP_KEEP_ERROR                2
+/* Options for http_get_*() */
+struct http_get_options {
+       unsigned no_cache:1,
+                keep_error:1;
 
-/* Return values for http_request_*() */
+       /* If non-NULL, returns the content-type of the response. */
+       struct strbuf *content_type;
+};
+
+/* Return values for http_get_*() */
 #define HTTP_OK                        0
 #define HTTP_MISSING_TARGET    1
 #define HTTP_ERROR             2
@@ -142,7 +147,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,
  *
  * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.
  */
-int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options);
+int http_get_strbuf(const char *url, struct strbuf *result, struct http_get_options *options);
 
 extern int http_fetch_ref(const char *base, struct ref *ref);
 

diff --git a/remote-curl.c b/remote-curl.c
index 5b3ce9eed299e312c132b590ea4926cfb019a271..ed1499b62cbf76001cdaa6db0595d613862ee3c1 100644 (file)
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -187,6 +187,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
        struct discovery *last = last_discovery;
        char *refs_url;
        int http_ret, maybe_smart = 0;
+       struct http_get_options options;
 
        if (last && !strcmp(service, last->service))
                return last;
@@ -204,8 +205,12 @@ static struct discovery* discover_refs(const char *service, int for_push)
        }
        refs_url = strbuf_detach(&buffer, NULL);
 
-       http_ret = http_get_strbuf(refs_url, &type, &buffer,
-                                  HTTP_NO_CACHE | HTTP_KEEP_ERROR);
+       memset(&options, 0, sizeof(options));
+       options.content_type = &type;
+       options.no_cache = 1;
+       options.keep_error = 1;
+
+       http_ret = http_get_strbuf(refs_url, &buffer, &options);
        switch (http_ret) {
        case HTTP_OK:
                break;