{"thread":{"id":"50548","subject":"[RFC PATCH] http: use --stdin and --keep when downloading pack","startedAt":"2019-02-21T00:14:53Z","lastAt":"2019-02-22T05:04:11Z","messageCount":5,"participants":["Jonathan Tan","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"369753","messageId":"20190221001447.124088-1-jonathantanmy@google.com","threadId":"50548","inReplyTo":null,"subject":"[RFC PATCH] http: use --stdin and --keep when downloading pack","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-21T00:14:47Z","receivedAt":"2019-02-21T00:14:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When Git fetches a pack using dumb HTTP, it does at least 2 things\ndifferently from when it fetches using fetch-pack or receive-pack: (1)\nit reuses the server's name for the packfile (which incorporates a hash)\nfor the packfile, and (2) it does not create a .keep file to avoid race\nconditions with repack.\n\nA subsequent patch will allow downloading packs over HTTP(S) as part of\na fetch. These downloads will not necessarily be from a Git repository,\nand thus may not have a hash as part of its name. Also, generating a\n.keep file will be necessary to avoid race conditions with repack (until\nthe fetch has successfully written the new refs).\n\nThus, teach http to pass --stdin and --keep to index-pack, the former so\nthat we have no reliance on the server's name for the packfile, and the\nlatter so that we have the .keep file.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis is part of the work of CDN offloading of fetch responses.\n\nI have plans to use the http_pack_request suite of functions to\nimplement the part where we download from CDN over HTTP(S), but need\nthis change to be able to do so. I think it's better from the code\nquality perspective to reuse these functions, but this necessitates a\nbehavior change in that we no longer use the filename as declared by the\nserver, so I'm sending this as RFC to see what the community thinks.\n---\n http-push.c   |  7 ++++++-\n http-walker.c |  5 ++++-\n http.c        | 42 ++++++++++++++++++++----------------------\n http.h        |  2 +-\n 4 files changed, 31 insertions(+), 25 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex b22c7caea0..409b266b0c 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -586,11 +586,16 @@ static void finish_request(struct transfer_request *request)\n \t\t\tfprintf(stderr, \"Unable to get pack file %s\\n%s\",\n \t\t\t\trequest->url, curl_errorstr);\n \t\t} else {\n+\t\t\tchar *lockfile;\n+\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\tif (finish_http_pack_request(preq,\n+\t\t\t\t\t\t\t     &lockfile) == 0) {\n+\t\t\t\t\tunlink(lockfile);\n \t\t\t\t\tfail = 0;\n+\t\t\t\t}\n \t\t\t\trelease_http_pack_request(preq);\n \t\t\t}\n \t\t}\ndiff --git a/http-walker.c b/http-walker.c\nindex 8ae5d76c6a..804dc82304 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -425,6 +425,7 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigne\n \tint ret;\n \tstruct slot_results results;\n \tstruct http_pack_request *preq;\n+\tchar *lockfile;\n \n \tif (fetch_indices(walker, repo))\n \t\treturn -1;\n@@ -457,7 +458,9 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigne\n \t\tgoto abort;\n \t}\n \n-\tret = finish_http_pack_request(preq);\n+\tret = finish_http_pack_request(preq, &lockfile);\n+\tif (!ret)\n+\t\tunlink(lockfile);\n \trelease_http_pack_request(preq);\n \tif (ret)\n \t\treturn ret;\ndiff --git a/http.c b/http.c\nindex a32ad36ddf..5f8e602cd2 100644\n--- a/http.c\n+++ b/http.c\n@@ -2200,13 +2200,13 @@ void release_http_pack_request(struct http_pack_request *preq)\n \tfree(preq);\n }\n \n-int finish_http_pack_request(struct http_pack_request *preq)\n+int finish_http_pack_request(struct http_pack_request *preq, char **lockfile)\n {\n \tstruct packed_git **lst;\n \tstruct packed_git *p = preq->target;\n-\tchar *tmp_idx;\n-\tsize_t len;\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n+\tint tmpfile_fd;\n+\tint ret = 0;\n \n \tclose_pack_index(p);\n \n@@ -2218,35 +2218,33 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tif (!strip_suffix(preq->tmpfile.buf, \".pack.temp\", &len))\n-\t\tBUG(\"pack tmpfile does not end in .pack.temp?\");\n-\ttmp_idx = xstrfmt(\"%.*s.idx.temp\", (int)len, preq->tmpfile.buf);\n+\ttmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);\n \n \targv_array_push(&ip.args, \"index-pack\");\n-\targv_array_pushl(&ip.args, \"-o\", tmp_idx, NULL);\n-\targv_array_push(&ip.args, preq->tmpfile.buf);\n+\targv_array_push(&ip.args, \"--stdin\");\n+\targv_array_pushf(&ip.args, \"--keep=git %\"PRIuMAX, (uintmax_t)getpid());\n \tip.git_cmd = 1;\n-\tip.no_stdin = 1;\n-\tip.no_stdout = 1;\n+\tip.in = tmpfile_fd;\n+\tip.out = -1;\n \n-\tif (run_command(&ip)) {\n-\t\tunlink(preq->tmpfile.buf);\n-\t\tunlink(tmp_idx);\n-\t\tfree(tmp_idx);\n-\t\treturn -1;\n+\tif (start_command(&ip)) {\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n \n-\tunlink(sha1_pack_index_name(p->sha1));\n+\t*lockfile = index_pack_lockfile(ip.out);\n+\tclose(ip.out);\n \n-\tif (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1))\n-\t || finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n-\t\tfree(tmp_idx);\n-\t\treturn -1;\n+\tif (finish_command(&ip)) {\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n \n \tinstall_packed_git(the_repository, p);\n-\tfree(tmp_idx);\n-\treturn 0;\n+cleanup:\n+\tclose(tmpfile_fd);\n+\tunlink(preq->tmpfile.buf);\n+\treturn ret;\n }\n \n struct http_pack_request *new_http_pack_request(\ndiff --git a/http.h b/http.h\nindex 4eb4e808e5..20d1c85d0b 100644\n--- a/http.h\n+++ b/http.h\n@@ -212,7 +212,7 @@ struct http_pack_request {\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+int finish_http_pack_request(struct http_pack_request *preq, char **lockfile);\n extern void release_http_pack_request(struct http_pack_request *preq);\n \n /* Helpers for fetching object */\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"369813","messageId":"20190221140809.GA21759@sigill.intra.peff.net","threadId":"50548","inReplyTo":"20190221001447.124088-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] http: use --stdin and --keep when downloading pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-21T14:08:09Z","receivedAt":"2019-02-21T14:08:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 20, 2019 at 04:14:47PM -0800, Jonathan Tan wrote:\n\n> This is part of the work of CDN offloading of fetch responses.\n> \n> I have plans to use the http_pack_request suite of functions to\n> implement the part where we download from CDN over HTTP(S), but need\n> this change to be able to do so. I think it's better from the code\n> quality perspective to reuse these functions, but this necessitates a\n> behavior change in that we no longer use the filename as declared by the\n> server, so I'm sending this as RFC to see what the community thinks.\n\nI think it makes sense. We don't use the server names for any of the\nother protocols, and I'm happy to see one less place where we may\ninherit a stupid or malicious item of data from the server.\n\n> diff --git a/http-push.c b/http-push.c\n> index b22c7caea0..409b266b0c 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -586,11 +586,16 @@ static void finish_request(struct transfer_request *request)\n>  \t\t\tfprintf(stderr, \"Unable to get pack file %s\\n%s\",\n>  \t\t\t\trequest->url, curl_errorstr);\n>  \t\t} else {\n> +\t\t\tchar *lockfile;\n> +\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\tif (finish_http_pack_request(preq,\n> +\t\t\t\t\t\t\t     &lockfile) == 0) {\n> +\t\t\t\t\tunlink(lockfile);\n>  \t\t\t\t\tfail = 0;\n> +\t\t\t\t}\n>  \t\t\t\trelease_http_pack_request(preq);\n>  \t\t\t}\n>  \t\t}\n\nI was puzzled that you had to touch http-push.c. But indeed, it seems to\nhave some fetching code in it, too? I'm willing to throw up my hands in\ndisgust at the http-push code without looking further at this point. :)\n\n>  \targv_array_push(&ip.args, \"index-pack\");\n> -\targv_array_pushl(&ip.args, \"-o\", tmp_idx, NULL);\n> -\targv_array_push(&ip.args, preq->tmpfile.buf);\n> +\targv_array_push(&ip.args, \"--stdin\");\n> +\targv_array_pushf(&ip.args, \"--keep=git %\"PRIuMAX, (uintmax_t)getpid());\n>  \tip.git_cmd = 1;\n> -\tip.no_stdin = 1;\n> -\tip.no_stdout = 1;\n> +\tip.in = tmpfile_fd;\n> +\tip.out = -1;\n>  \n> -\tif (run_command(&ip)) {\n> -\t\tunlink(preq->tmpfile.buf);\n> -\t\tunlink(tmp_idx);\n> -\t\tfree(tmp_idx);\n> -\t\treturn -1;\n> +\tif (start_command(&ip)) {\n> +\t\tret = -1;\n> +\t\tgoto cleanup;\n>  \t}\n>  \n> -\tunlink(sha1_pack_index_name(p->sha1));\n> +\t*lockfile = index_pack_lockfile(ip.out);\n> +\tclose(ip.out);\n\nWe're now doing bi-directional I/O with index-pack. But it should be\ndeadlock-free, because we know the output is small and will only come at\nthe end after we've closed its stdin.\n\n> -\tif (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1))\n> -\t || finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n> -\t\tfree(tmp_idx);\n> -\t\treturn -1;\n> +\tif (finish_command(&ip)) {\n> +\t\tret = -1;\n> +\t\tgoto cleanup;\n>  \t}\n\nIf the command fails but we got something in *lockfile, should we clean\nit up? Likewise, do we need to be installing a signal handler to clean\nit up in case we die in other code paths (or by a signal)?\n\n-Peff\n"},{"id":"369832","messageId":"20190221184948.100083-1-jonathantanmy@google.com","threadId":"50548","inReplyTo":"20190221140809.GA21759@sigill.intra.peff.net","subject":"Re: [RFC PATCH] http: use --stdin and --keep when downloading pack","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-21T18:49:48Z","receivedAt":"2019-02-21T18:49:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Wed, Feb 20, 2019 at 04:14:47PM -0800, Jonathan Tan wrote:\n> \n> > This is part of the work of CDN offloading of fetch responses.\n> > \n> > I have plans to use the http_pack_request suite of functions to\n> > implement the part where we download from CDN over HTTP(S), but need\n> > this change to be able to do so. I think it's better from the code\n> > quality perspective to reuse these functions, but this necessitates a\n> > behavior change in that we no longer use the filename as declared by the\n> > server, so I'm sending this as RFC to see what the community thinks.\n> \n> I think it makes sense. We don't use the server names for any of the\n> other protocols, and I'm happy to see one less place where we may\n> inherit a stupid or malicious item of data from the server.\n\nThanks. That's true.\n\n> I was puzzled that you had to touch http-push.c. But indeed, it seems to\n> have some fetching code in it, too? I'm willing to throw up my hands in\n> disgust at the http-push code without looking further at this point. :)\n\n:)\n\n> >  \targv_array_push(&ip.args, \"index-pack\");\n> > -\targv_array_pushl(&ip.args, \"-o\", tmp_idx, NULL);\n> > -\targv_array_push(&ip.args, preq->tmpfile.buf);\n> > +\targv_array_push(&ip.args, \"--stdin\");\n> > +\targv_array_pushf(&ip.args, \"--keep=git %\"PRIuMAX, (uintmax_t)getpid());\n> >  \tip.git_cmd = 1;\n> > -\tip.no_stdin = 1;\n> > -\tip.no_stdout = 1;\n> > +\tip.in = tmpfile_fd;\n> > +\tip.out = -1;\n> >  \n> > -\tif (run_command(&ip)) {\n> > -\t\tunlink(preq->tmpfile.buf);\n> > -\t\tunlink(tmp_idx);\n> > -\t\tfree(tmp_idx);\n> > -\t\treturn -1;\n> > +\tif (start_command(&ip)) {\n> > +\t\tret = -1;\n> > +\t\tgoto cleanup;\n> >  \t}\n> >  \n> > -\tunlink(sha1_pack_index_name(p->sha1));\n> > +\t*lockfile = index_pack_lockfile(ip.out);\n> > +\tclose(ip.out);\n> \n> We're now doing bi-directional I/O with index-pack. But it should be\n> deadlock-free, because we know the output is small and will only come at\n> the end after we've closed its stdin.\n\nYes. This is also how fetch-pack.c does it, for example.\n\n> > -\tif (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1))\n> > -\t || finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n> > -\t\tfree(tmp_idx);\n> > -\t\treturn -1;\n> > +\tif (finish_command(&ip)) {\n> > +\t\tret = -1;\n> > +\t\tgoto cleanup;\n> >  \t}\n> \n> If the command fails but we got something in *lockfile, should we clean\n> it up? Likewise, do we need to be installing a signal handler to clean\n> it up in case we die in other code paths (or by a signal)?\n\nMy inclination is not to do it - as far as I can tell, we don't have\ncleanup or signal handlers in fetch-pack.c either.\n"},{"id":"369854","messageId":"xmqq5ztcu92n.fsf@gitster-ct.c.googlers.com","threadId":"50548","inReplyTo":"20190221001447.124088-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] http: use --stdin and --keep when downloading pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-21T21:26:40Z","receivedAt":"2019-02-21T21:26:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When Git fetches a pack using dumb HTTP, it does at least 2 things\n> differently from when it fetches using fetch-pack or receive-pack: (1)\n> it reuses the server's name for the packfile (which incorporates a hash)\n> for the packfile, and (2) it does not create a .keep file to avoid race\n> conditions with repack.\n>\n> A subsequent patch will allow downloading packs over HTTP(S) as part of\n> a fetch. These downloads will not necessarily be from a Git repository,\n> and thus may not have a hash as part of its name. Also, generating a\n> .keep file will be necessary to avoid race conditions with repack (until\n> the fetch has successfully written the new refs).\n>\n> Thus, teach http to pass --stdin and --keep to index-pack, the former so\n> that we have no reliance on the server's name for the packfile, and the\n> latter so that we have the .keep file.\n\nMakes sense.  Can a malicious dumb server mount a passive attack\nthat serves misnamed packfiles and wait for victims to come?\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> This is part of the work of CDN offloading of fetch responses.\n>\n> I have plans to use the http_pack_request suite of functions to\n> implement the part where we download from CDN over HTTP(S), but need\n> this change to be able to do so. I think it's better from the code\n> quality perspective to reuse these functions, but this necessitates a\n> behavior change in that we no longer use the filename as declared by the\n> server, so I'm sending this as RFC to see what the community thinks.\n> ---\n>  http-push.c   |  7 ++++++-\n>  http-walker.c |  5 ++++-\n>  http.c        | 42 ++++++++++++++++++++----------------------\n>  http.h        |  2 +-\n>  4 files changed, 31 insertions(+), 25 deletions(-)\n>\n> diff --git a/http-push.c b/http-push.c\n> index b22c7caea0..409b266b0c 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -586,11 +586,16 @@ static void finish_request(struct transfer_request *request)\n>  \t\t\tfprintf(stderr, \"Unable to get pack file %s\\n%s\",\n>  \t\t\t\trequest->url, curl_errorstr);\n>  \t\t} else {\n> +\t\t\tchar *lockfile;\n> +\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\tif (finish_http_pack_request(preq,\n> +\t\t\t\t\t\t\t     &lockfile) == 0) {\n> +\t\t\t\t\tunlink(lockfile);\n>  \t\t\t\t\tfail = 0;\n> +\t\t\t\t}\n>  \t\t\t\trelease_http_pack_request(preq);\n>  \t\t\t}\n>  \t\t}\n> diff --git a/http-walker.c b/http-walker.c\n> index 8ae5d76c6a..804dc82304 100644\n> --- a/http-walker.c\n> +++ b/http-walker.c\n> @@ -425,6 +425,7 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigne\n>  \tint ret;\n>  \tstruct slot_results results;\n>  \tstruct http_pack_request *preq;\n> +\tchar *lockfile;\n>  \n>  \tif (fetch_indices(walker, repo))\n>  \t\treturn -1;\n> @@ -457,7 +458,9 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigne\n>  \t\tgoto abort;\n>  \t}\n>  \n> -\tret = finish_http_pack_request(preq);\n> +\tret = finish_http_pack_request(preq, &lockfile);\n> +\tif (!ret)\n> +\t\tunlink(lockfile);\n>  \trelease_http_pack_request(preq);\n>  \tif (ret)\n>  \t\treturn ret;\n> diff --git a/http.c b/http.c\n> index a32ad36ddf..5f8e602cd2 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -2200,13 +2200,13 @@ void release_http_pack_request(struct http_pack_request *preq)\n>  \tfree(preq);\n>  }\n>  \n> -int finish_http_pack_request(struct http_pack_request *preq)\n> +int finish_http_pack_request(struct http_pack_request *preq, char **lockfile)\n>  {\n>  \tstruct packed_git **lst;\n>  \tstruct packed_git *p = preq->target;\n> -\tchar *tmp_idx;\n> -\tsize_t len;\n>  \tstruct child_process ip = CHILD_PROCESS_INIT;\n> +\tint tmpfile_fd;\n> +\tint ret = 0;\n>  \n>  \tclose_pack_index(p);\n>  \n> @@ -2218,35 +2218,33 @@ int finish_http_pack_request(struct http_pack_request *preq)\n>  \t\tlst = &((*lst)->next);\n>  \t*lst = (*lst)->next;\n>  \n> -\tif (!strip_suffix(preq->tmpfile.buf, \".pack.temp\", &len))\n> -\t\tBUG(\"pack tmpfile does not end in .pack.temp?\");\n> -\ttmp_idx = xstrfmt(\"%.*s.idx.temp\", (int)len, preq->tmpfile.buf);\n> +\ttmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);\n>  \n>  \targv_array_push(&ip.args, \"index-pack\");\n> -\targv_array_pushl(&ip.args, \"-o\", tmp_idx, NULL);\n> -\targv_array_push(&ip.args, preq->tmpfile.buf);\n> +\targv_array_push(&ip.args, \"--stdin\");\n> +\targv_array_pushf(&ip.args, \"--keep=git %\"PRIuMAX, (uintmax_t)getpid());\n>  \tip.git_cmd = 1;\n> -\tip.no_stdin = 1;\n> -\tip.no_stdout = 1;\n> +\tip.in = tmpfile_fd;\n> +\tip.out = -1;\n>  \n> -\tif (run_command(&ip)) {\n> -\t\tunlink(preq->tmpfile.buf);\n> -\t\tunlink(tmp_idx);\n> -\t\tfree(tmp_idx);\n> -\t\treturn -1;\n> +\tif (start_command(&ip)) {\n> +\t\tret = -1;\n> +\t\tgoto cleanup;\n>  \t}\n>  \n> -\tunlink(sha1_pack_index_name(p->sha1));\n> +\t*lockfile = index_pack_lockfile(ip.out);\n> +\tclose(ip.out);\n>  \n> -\tif (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1))\n> -\t || finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n> -\t\tfree(tmp_idx);\n> -\t\treturn -1;\n> +\tif (finish_command(&ip)) {\n> +\t\tret = -1;\n> +\t\tgoto cleanup;\n>  \t}\n>  \n>  \tinstall_packed_git(the_repository, p);\n> -\tfree(tmp_idx);\n> -\treturn 0;\n> +cleanup:\n> +\tclose(tmpfile_fd);\n> +\tunlink(preq->tmpfile.buf);\n> +\treturn ret;\n>  }\n\nNicely done.\n\n>  \n>  struct http_pack_request *new_http_pack_request(\n> diff --git a/http.h b/http.h\n> index 4eb4e808e5..20d1c85d0b 100644\n> --- a/http.h\n> +++ b/http.h\n> @@ -212,7 +212,7 @@ struct http_pack_request {\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> +int finish_http_pack_request(struct http_pack_request *preq, char **lockfile);\n>  extern void release_http_pack_request(struct http_pack_request *preq);\n>  \n>  /* Helpers for fetching object */\n"},{"id":"369883","messageId":"20190222050406.GA6050@sigill.intra.peff.net","threadId":"50548","inReplyTo":"20190221184948.100083-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] http: use --stdin and --keep when downloading pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-22T05:04:06Z","receivedAt":"2019-02-22T05:04:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2019 at 10:49:48AM -0800, Jonathan Tan wrote:\n\n> > > -\tif (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1))\n> > > -\t || finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n> > > -\t\tfree(tmp_idx);\n> > > -\t\treturn -1;\n> > > +\tif (finish_command(&ip)) {\n> > > +\t\tret = -1;\n> > > +\t\tgoto cleanup;\n> > >  \t}\n> > \n> > If the command fails but we got something in *lockfile, should we clean\n> > it up? Likewise, do we need to be installing a signal handler to clean\n> > it up in case we die in other code paths (or by a signal)?\n> \n> My inclination is not to do it - as far as I can tell, we don't have\n> cleanup or signal handlers in fetch-pack.c either.\n\nHmm, yeah, you're right. Nor do we do it on fetch. At first I was\nsurprised that this doesn't cause more problems (we'd leave it in place\nafter seeing a pre-receive hook fail, for instance), but in practice it\ngets cleaned up along with the rest of the quarantine area.\n\nThere's still a window where we may leave it around, though (and we\ndon't quarantine fetches at all), so it might be worth addressing at\nsome point. But I agree it is totally out of scope for this patch.\n\n-Peff\n"}]}