{"thread":{"id":"19483","subject":"[PATCH 17/18] http*: add helper methods for fetching packs","startedAt":"2009-05-24T14:20:06Z","lastAt":"2009-05-24T20:44:03Z","messageCount":2,"participants":["Tay Ray Chuan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":18},"messages":[{"id":"114574","messageId":"20090524222006.cd71f8dc.rctay89@gmail.com","threadId":"19483","inReplyTo":null,"subject":"[PATCH 17/18] http*: add helper methods for fetching packs","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-05-24T14:20:06Z","receivedAt":"2009-05-24T14:20:06Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"The code handling the fetching of packs in http-push.c and\nhttp-walker.c have been refactored into new methods and a new struct\n(http_pack_request) in http.c. They are not meant to be invoked\nelsewhere.\n\nThe new methods in http.c are\n *new_http_pack_request\n *finish_http_pack_request\n *release_http_pack_request\n\nand the new struct is http_pack_request.\n\n***http-push.c***\nThe local_stream member of the transfer_request has been removed, as\nthe packfile pointer will be managed in the struct http_pack_request.\n\nThe code moved out from start_fetch_packed to new_http_pack_request\ndeals with filenames, file and curl options, and does not change\nits behaviour.\n\nThe logic moved out from finish_request is similar to that in\nfinish_http_pack_request. If the invocation fails, due to conditions\nlike a failed rename or failed pack verification,\nrepo->can_update_info_refs is set to 0, as before.\n\nThe closing of the file descriptor (originally local_stream in\ntransfer_request) in release_request is now done in\nfinish_http_pack_request and release_http_pack_request.\n\nWhen cleaning up, release_http_pack_request is invoked separately from\nrelease_request.\n\n***http-walker.c***\nThe code moved out from fetch_pack to new_http_pack_request deals with\nfilenames, file and curl options; they do not change its behaviour.\n\nThe code moved out from fetch_pack to finish_http_pack_request deals\nwith renaming the pack, advancing the pack list, and installing the\npack; they do not change its behaviour.\n\nWhen cleaning up, release_http_pack_request is invoked separately from\nrelease_request.\n\nSigned-off-by: Tay Ray Chuan <rctay89@gmail.com>\n---\n http-push.c   |  107 ++++++++++++++------------------------------------------\n http-walker.c |   82 +++++++++---------------------------------\n http.c        |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h        |   17 +++++++++\n 4 files changed, 167 insertions(+), 147 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex df3e8bf..3b681b8 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1,6 +1,5 @@\n #include \"cache.h\"\n #include \"commit.h\"\n-#include \"pack.h\"\n #include \"tag.h\"\n #include \"blob.h\"\n #include \"http.h\"\n@@ -119,7 +118,6 @@ struct transfer_request\n \tchar filename[PATH_MAX];\n \tchar tmpfile[PATH_MAX];\n \tint local_fileno;\n-\tFILE *local_stream;\n \tenum transfer_state state;\n \tCURLcode curl_result;\n \tchar errorstr[CURL_ERROR_SIZE];\n@@ -452,16 +450,10 @@ static void start_mkcol(struct transfer_request *request)\n\n static void start_fetch_packed(struct transfer_request *request)\n {\n-\tchar *url;\n \tstruct packed_git *target;\n-\tFILE *packfile;\n-\tchar *filename;\n-\tlong prev_posn = 0;\n-\tchar range[RANGE_HEADER_SIZE];\n-\tstruct curl_slist *range_header = NULL;\n\n \tstruct transfer_request *check_request = request_queue_head;\n-\tstruct active_request_slot *slot;\n+\tstruct http_pack_request *preq;\n\n \ttarget = find_sha1_pack(request->obj->sha1, repo->packs);\n \tif (!target) {\n@@ -474,68 +466,35 @@ static void start_fetch_packed(struct transfer_request *request)\n \tfprintf(stderr,\t\"Fetching pack %s\\n\", sha1_to_hex(target->sha1));\n \tfprintf(stderr, \" which contains %s\\n\", sha1_to_hex(request->obj->sha1));\n\n-\tfilename = sha1_pack_name(target->sha1);\n-\tsnprintf(request->filename, sizeof(request->filename), \"%s\", filename);\n-\tsnprintf(request->tmpfile, sizeof(request->tmpfile),\n-\t\t \"%s.temp\", filename);\n-\n-\turl = xmalloc(strlen(repo->url) + 64);\n-\tsprintf(url, \"%sobjects/pack/pack-%s.pack\",\n-\t\trepo->url, sha1_to_hex(target->sha1));\n+\tpreq = new_http_pack_request(target, repo->url);\n+\tif (preq == NULL) {\n+\t\trelease_http_pack_request(preq);\n+\t\trepo->can_update_info_refs = 0;\n+\t\treturn;\n+\t}\n+\tpreq->lst = &repo->packs;\n\n \t/* Make sure there isn't another open request for this pack */\n \twhile (check_request) {\n \t\tif (check_request->state == RUN_FETCH_PACKED &&\n-\t\t    !strcmp(check_request->url, url)) {\n-\t\t\tfree(url);\n+\t\t    !strcmp(check_request->url, preq->url)) {\n+\t\t\trelease_http_pack_request(preq);\n \t\t\trelease_request(request);\n \t\t\treturn;\n \t\t}\n \t\tcheck_request = check_request->next;\n \t}\n\n-\tpackfile = fopen(request->tmpfile, \"a\");\n-\tif (!packfile) {\n-\t\tfprintf(stderr, \"Unable to open local file %s for pack\",\n-\t\t\trequest->tmpfile);\n-\t\trepo->can_update_info_refs = 0;\n-\t\tfree(url);\n-\t\treturn;\n-\t}\n-\n-\tslot = get_active_slot();\n-\tslot->callback_func = process_response;\n-\tslot->callback_data = request;\n-\trequest->slot = slot;\n-\trequest->local_stream = packfile;\n-\trequest->userData = target;\n-\n-\trequest->url = url;\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, packfile);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_URL, url);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, no_pragma_header);\n-\tslot->local = packfile;\n-\n-\t/*\n-\t * If there is data present from a previous transfer attempt,\n-\t * resume where it left off\n-\t */\n-\tprev_posn = ftell(packfile);\n-\tif (prev_posn>0) {\n-\t\tif (push_verbosely)\n-\t\t\tfprintf(stderr,\n-\t\t\t\t\"Resuming fetch of pack %s at byte %ld\\n\",\n-\t\t\t\tsha1_to_hex(target->sha1), prev_posn);\n-\t\tsprintf(range, \"Range: bytes=%ld-\", prev_posn);\n-\t\trange_header = curl_slist_append(range_header, range);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, range_header);\n-\t}\n+\tpreq->slot->callback_func = process_response;\n+\tpreq->slot->callback_data = request;\n+\trequest->slot = preq->slot;\n+\trequest->userData = preq;\n\n \t/* Try to get the request started, abort the request on error */\n \trequest->state = RUN_FETCH_PACKED;\n-\tif (!start_active_slot(slot)) {\n+\tif (!start_active_slot(preq->slot)) {\n \t\tfprintf(stderr, \"Unable to start GET request\\n\");\n+\t\trelease_http_pack_request(preq);\n \t\trepo->can_update_info_refs = 0;\n \t\trelease_request(request);\n \t}\n@@ -718,8 +677,6 @@ static void release_request(struct transfer_request *request)\n\n \tif (request->local_fileno != -1)\n \t\tclose(request->local_fileno);\n-\tif (request->local_stream)\n-\t\tfclose(request->local_stream);\n \tfree(request->url);\n \tfree(request);\n }\n@@ -727,8 +684,7 @@ static void release_request(struct transfer_request *request)\n static void finish_request(struct transfer_request *request)\n {\n \tstruct stat st;\n-\tstruct packed_git *target;\n-\tstruct packed_git **lst;\n+\tstruct http_pack_request *preq;\n\n \trequest->curl_result = request->slot->curl_result;\n \trequest->http_code = request->slot->http_code;\n@@ -819,30 +775,21 @@ static void finish_request(struct transfer_request *request)\n \t\t\tstart_fetch_packed(request);\n\n \t} else if (request->state == RUN_FETCH_PACKED) {\n+\t\tint fail = 1;\n \t\tif (request->curl_result != CURLE_OK) {\n \t\t\tfprintf(stderr, \"Unable to get pack file %s\\n%s\",\n \t\t\t\trequest->url, curl_errorstr);\n-\t\t\trepo->can_update_info_refs = 0;\n \t\t} else {\n-\t\t\toff_t pack_size = ftell(request->local_stream);\n-\n-\t\t\tfclose(request->local_stream);\n-\t\t\trequest->local_stream = NULL;\n-\t\t\tif (!move_temp_to_file(request->tmpfile,\n-\t\t\t\t\t       request->filename)) {\n-\t\t\t\ttarget = (struct packed_git *)request->userData;\n-\t\t\t\ttarget->pack_size = pack_size;\n-\t\t\t\tlst = &repo->packs;\n-\t\t\t\twhile (*lst != target)\n-\t\t\t\t\tlst = &((*lst)->next);\n-\t\t\t\t*lst = (*lst)->next;\n-\n-\t\t\t\tif (!verify_pack(target))\n-\t\t\t\t\tinstall_packed_git(target);\n-\t\t\t\telse\n-\t\t\t\t\trepo->can_update_info_refs = 0;\n+\t\t\tpreq = (struct http_pack_request *)request->userData;\n+\n+\t\t\tif (preq) {\n+\t\t\t\tif (finish_http_pack_request(preq) > 0)\n+\t\t\t\t\tfail = 0;\n+\t\t\t\trelease_http_pack_request(preq);\n \t\t\t}\n \t\t}\n+\t\tif (fail)\n+\t\t\trepo->can_update_info_refs = 0;\n \t\trelease_request(request);\n \t}\n }\n@@ -897,7 +844,6 @@ static void add_fetch_request(struct object *obj)\n \trequest->lock = NULL;\n \trequest->headers = NULL;\n \trequest->local_fileno = -1;\n-\trequest->local_stream = NULL;\n \trequest->state = NEED_FETCH;\n \trequest->next = request_queue_head;\n \trequest_queue_head = request;\n@@ -937,7 +883,6 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)\n \trequest->lock = lock;\n \trequest->headers = NULL;\n \trequest->local_fileno = -1;\n-\trequest->local_stream = NULL;\n \trequest->state = NEED_PUSH;\n \trequest->next = request_queue_head;\n \trequest_queue_head = request;\ndiff --git a/http-walker.c b/http-walker.c\nindex 2aed391..cea2755 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -1,6 +1,5 @@\n #include \"cache.h\"\n #include \"commit.h\"\n-#include \"pack.h\"\n #include \"walker.h\"\n #include \"http.h\"\n\n@@ -590,19 +589,10 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)\n\n static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned char *sha1)\n {\n-\tchar *url;\n \tstruct packed_git *target;\n-\tstruct packed_git **lst;\n-\tFILE *packfile;\n-\tchar *filename;\n-\tchar tmpfile[PATH_MAX];\n \tint ret;\n-\tlong prev_posn = 0;\n-\tchar range[RANGE_HEADER_SIZE];\n-\tstruct curl_slist *range_header = NULL;\n-\n-\tstruct active_request_slot *slot;\n \tstruct slot_results results;\n+\tstruct http_pack_request *preq;\n\n \tif (fetch_indices(walker, repo))\n \t\treturn -1;\n@@ -617,69 +607,33 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha\n \t\t\tsha1_to_hex(sha1));\n \t}\n\n-\turl = xmalloc(strlen(repo->base) + 65);\n-\tsprintf(url, \"%s/objects/pack/pack-%s.pack\",\n-\t\trepo->base, sha1_to_hex(target->sha1));\n+\tpreq = new_http_pack_request(target, repo->base);\n+\tif (preq == NULL)\n+\t\tgoto abort;\n+\tpreq->lst = &repo->packs;\n+\tpreq->slot->results = &results;\n\n-\tfilename = sha1_pack_name(target->sha1);\n-\tsnprintf(tmpfile, sizeof(tmpfile), \"%s.temp\", filename);\n-\tpackfile = fopen(tmpfile, \"a\");\n-\tif (!packfile)\n-\t\treturn error(\"Unable to open local file %s for pack\",\n-\t\t\t     tmpfile);\n-\n-\tslot = get_active_slot();\n-\tslot->results = &results;\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, packfile);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_URL, url);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, no_pragma_header);\n-\tslot->local = packfile;\n-\n-\t/*\n-\t * If there is data present from a previous transfer attempt,\n-\t * resume where it left off\n-\t */\n-\tprev_posn = ftell(packfile);\n-\tif (prev_posn>0) {\n-\t\tif (walker->get_verbosely)\n-\t\t\tfprintf(stderr,\n-\t\t\t\t\"Resuming fetch of pack %s at byte %ld\\n\",\n-\t\t\t\tsha1_to_hex(target->sha1), prev_posn);\n-\t\tsprintf(range, \"Range: bytes=%ld-\", prev_posn);\n-\t\trange_header = curl_slist_append(range_header, range);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, range_header);\n-\t}\n-\n-\tif (start_active_slot(slot)) {\n-\t\trun_active_slot(slot);\n+\tif (start_active_slot(preq->slot)) {\n+\t\trun_active_slot(preq->slot);\n \t\tif (results.curl_result != CURLE_OK) {\n-\t\t\tfclose(packfile);\n-\t\t\treturn error(\"Unable to get pack file %s\\n%s\", url,\n-\t\t\t\t     curl_errorstr);\n+\t\t\terror(\"Unable to get pack file %s\\n%s\", preq->url,\n+\t\t\t      curl_errorstr);\n+\t\t\tgoto abort;\n \t\t}\n \t} else {\n-\t\tfclose(packfile);\n-\t\treturn error(\"Unable to start request\");\n+\t\terror(\"Unable to start request\");\n+\t\tgoto abort;\n \t}\n\n-\ttarget->pack_size = ftell(packfile);\n-\tfclose(packfile);\n-\n-\tret = move_temp_to_file(tmpfile, filename);\n+\tret = finish_http_pack_request(preq);\n+\trelease_http_pack_request(preq);\n \tif (ret)\n \t\treturn ret;\n\n-\tlst = &repo->packs;\n-\twhile (*lst != target)\n-\t\tlst = &((*lst)->next);\n-\t*lst = (*lst)->next;\n-\n-\tif (verify_pack(target))\n-\t\treturn -1;\n-\tinstall_packed_git(target);\n-\n \treturn 0;\n+\n+abort:\n+\treturn -1;\n }\n\n static void abort_object_request(struct object_request *obj_req)\ndiff --git a/http.c b/http.c\nindex c3f7456..2b4aaaa 100644\n--- a/http.c\n+++ b/http.c\n@@ -1,4 +1,5 @@\n #include \"http.h\"\n+#include \"pack.h\"\n\n int data_received;\n int active_requests;\n@@ -832,8 +833,8 @@ cleanup:\n \treturn ret;\n }\n\n-int fetch_http_pack_index(struct packed_git **packs_head, unsigned char *sha1,\n-\tconst char *base_url)\n+static int fetch_http_pack_index(struct packed_git **packs_head,\n+\tunsigned char *sha1, const char *base_url)\n {\n \tstruct packed_git *new_pack;\n\n@@ -892,3 +893,106 @@ cleanup:\n \tfree(url);\n \treturn ret;\n }\n+\n+void release_http_pack_request(struct http_pack_request *preq)\n+{\n+\tif (preq->packfile != NULL) {\n+\t\tfclose(preq->packfile);\n+\t\tpreq->packfile = NULL;\n+\t}\n+\tif (preq->range_header != NULL) {\n+\t\tcurl_slist_free_all(preq->range_header);\n+\t\tpreq->range_header = NULL;\n+\t}\n+\tpreq->slot = NULL;\n+\tfree(preq->url);\n+}\n+\n+int finish_http_pack_request(struct http_pack_request *preq)\n+{\n+\tint ret;\n+\tstruct packed_git **lst;\n+\n+\tpreq->target->pack_size = ftell(preq->packfile);\n+\n+\tif (preq->packfile != NULL) {\n+\t\tfclose(preq->packfile);\n+\t\tpreq->packfile = NULL;\n+\t}\n+\n+\tret = move_temp_to_file(preq->tmpfile, preq->filename);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tlst = preq->lst;\n+\twhile (*lst != preq->target)\n+\t\tlst = &((*lst)->next);\n+\t*lst = (*lst)->next;\n+\n+\tif (verify_pack(preq->target))\n+\t\treturn -1;\n+\tinstall_packed_git(preq->target);\n+\n+\treturn 0;\n+}\n+\n+struct http_pack_request *new_http_pack_request(\n+\tstruct packed_git *target, const char *base_url)\n+{\n+\tchar *url;\n+\tchar *filename;\n+\tlong prev_posn = 0;\n+\tchar range[RANGE_HEADER_SIZE];\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct http_pack_request *preq;\n+\n+\tpreq = xmalloc(sizeof(*preq));\n+\tpreq->target = target;\n+\tpreq->range_header = NULL;\n+\n+\tend_url_with_slash(&buf, base_url);\n+\tstrbuf_addf(&buf, \"objects/pack/pack-%s.pack\",\n+\t\tsha1_to_hex(target->sha1));\n+\turl = strbuf_detach(&buf, NULL);\n+\tpreq->url = xstrdup(url);\n+\n+\tfilename = sha1_pack_name(target->sha1);\n+\tsnprintf(preq->filename, sizeof(preq->filename), \"%s\", filename);\n+\tsnprintf(preq->tmpfile, sizeof(preq->tmpfile), \"%s.temp\", filename);\n+\tpreq->packfile = fopen(preq->tmpfile, \"a\");\n+\tif (!preq->packfile) {\n+\t\terror(\"Unable to open local file %s for pack\",\n+\t\t      preq->tmpfile);\n+\t\tgoto abort;\n+\t}\n+\n+\tpreq->slot = get_active_slot();\n+\tpreq->slot->local = preq->packfile;\n+\tcurl_easy_setopt(preq->slot->curl, CURLOPT_FILE, preq->packfile);\n+\tcurl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);\n+\tcurl_easy_setopt(preq->slot->curl, CURLOPT_URL, url);\n+\tcurl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER,\n+\t\tno_pragma_header);\n+\n+\t/*\n+\t * If there is data present from a previous transfer attempt,\n+\t * resume where it left off\n+\t */\n+\tprev_posn = ftell(preq->packfile);\n+\tif (prev_posn>0) {\n+\t\tif (http_is_verbose)\n+\t\t\tfprintf(stderr,\n+\t\t\t\t\"Resuming fetch of pack %s at byte %ld\\n\",\n+\t\t\t\tsha1_to_hex(target->sha1), prev_posn);\n+\t\tsprintf(range, \"Range: bytes=%ld-\", prev_posn);\n+\t\tpreq->range_header = curl_slist_append(NULL, range);\n+\t\tcurl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER,\n+\t\t\tpreq->range_header);\n+\t}\n+\n+\treturn preq;\n+\n+abort:\n+\tfree(filename);\n+\treturn NULL;\n+}\ndiff --git a/http.h b/http.h\nindex 180b1c2..511c0c4 100644\n--- a/http.h\n+++ b/http.h\n@@ -150,4 +150,21 @@ extern int http_fetch_ref(const char *base, struct ref *ref);\n extern int http_get_info_packs(const char *base_url,\n \tstruct packed_git **packs_head);\n\n+struct http_pack_request\n+{\n+\tchar *url;\n+\tstruct packed_git *target;\n+\tstruct packed_git **lst;\n+\tFILE *packfile;\n+\tchar filename[PATH_MAX];\n+\tchar tmpfile[PATH_MAX];\n+\tstruct curl_slist *range_header;\n+\tstruct active_request_slot *slot;\n+};\n+\n+extern struct http_pack_request *new_http_pack_request(\n+\tstruct packed_git *target, const char *base_url);\n+extern int finish_http_pack_request(struct http_pack_request *preq);\n+extern void release_http_pack_request(struct http_pack_request *preq);\n+\n #endif /* HTTP_H */\n--\n1.6.3.1\n"},{"id":"114594","messageId":"7v7i06teng.fsf@alter.siamese.dyndns.org","threadId":"19483","inReplyTo":"20090524222006.cd71f8dc.rctay89@gmail.com","subject":"Re: [PATCH 17/18] http*: add helper methods for fetching packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-24T20:44:03Z","receivedAt":"2009-05-24T20:44:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan <rctay89@gmail.com> writes:\n\n> The code handling the fetching of packs in http-push.c and\n> http-walker.c have been refactored into new methods and a new struct\n> (http_pack_request) in http.c. They are not meant to be invoked\n> elsewhere.\n>\n> The new methods in http.c are\n>  *new_http_pack_request\n>  *finish_http_pack_request\n>  *release_http_pack_request\n> and the new struct is http_pack_request.\n\nPlease do not use '*' not followed by any whitespace for enumerations; use\nof '*' when you talk about code is confusing (e.g \"is he talking about a\npointer?\").\n\n> ***http-push.c***\n> The local_stream member of the transfer_request has been removed, as\n> the packfile pointer will be managed in the struct http_pack_request.\n>\n> The code moved out from start_fetch_packed to new_http_pack_request\n> deals with filenames, file and curl options, and does not change\n> its behaviour.\n\nSorry, I cannot parse these.  Do you mean\n\n\tRefactor code from start_fetch_packed() to a new function,\n\tnew_http_pack_request(); this deals with filenames, file and curl\n\toptions.\n\nI am a bit puzzled about the description split along the filename boundary\nof the source files.  You also describe the new_http_pack_request()\nfunction below, where you talk about changes to http-walker.c.  It would\nbe much easier to review if you don't split them, e.g.\n\n\tAdd a function, new_http_pack_request(), that deals with the\n\tdetails of coming up with the filename to store the retrieved\n\tpackfile, resuming a previously aborted request, and making a new\n\tcurl request.  Update start_fetch_packed() in http-push.c and\n\tfetch_pack() in http-walker.c to use this.\n"}]}