{"thread":{"id":"36403","subject":"[PATCH 0/5] transport-helper: serious crash fix","startedAt":"2014-04-12T20:33:27Z","lastAt":"2014-04-20T21:54:27Z","messageCount":20,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"238788","messageId":"1397334812-12215-1-git-send-email-felipe.contreras@gmail.com","threadId":"36403","inReplyTo":null,"subject":"[PATCH 0/5] transport-helper: serious crash fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-12T20:33:27Z","receivedAt":"2014-04-12T20:33:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"One of the most serious recurring issues[1][2][3] with remote helpers is that\nmarks get out of sync. The way to analize and reproduce these wasn't trivial,\nbut the culprit seems to be a crash while doing `git push`. It has been known\nalready how exactly that happens, but no simple way how to fix it.\n\nThis is the simplest way so far; tell `git fast-export` to export the marks to\na temporary file, and move it to the right location only *after* the remote\nhelper has finished its job without errors.\n\nSince the code wasn't prepared for a change like this, some reorganization\nchanges are needed. More changes might be welcome to further propagate the\nerrors properly through the code, but for the moment the errors are propagated\nto the right location, in order to fix this specific problem.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/223962\n[2] http://article.gmane.org/gmane.comp.version-control.git/241707\n[3] https://github.com/felipec/git/issues/56\n\nFelipe Contreras (5):\n  transport-helper: remove barely used xchgline()\n  remote-helpers: make recvline return an error\n  transport-helper: propagate recvline() error pushing\n  transport-helper: trivial cleanup\n  transport-helper: fix sync issue on crashes\n\n t/t5801-remote-helpers.sh | 17 ++++++++++-\n transport-helper.c        | 72 ++++++++++++++++++++++++++++-------------------\n 2 files changed, 59 insertions(+), 30 deletions(-)\n\n-- \n1.9.1+fc3.9.gc73078e\n"},{"id":"238789","messageId":"1397334812-12215-2-git-send-email-felipe.contreras@gmail.com","threadId":"36403","inReplyTo":"1397334812-12215-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/5] transport-helper: remove barely used xchgline()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-12T20:33:28Z","receivedAt":"2014-04-12T20:33:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It's only used once, we can just call the two functions inside directly.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n transport-helper.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex ad72fbd..bf329fd 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -71,12 +71,6 @@ static int recvline(struct helper_data *helper, struct strbuf *buffer)\n \treturn recvline_fh(helper->out, buffer, helper->name);\n }\n \n-static void xchgline(struct helper_data *helper, struct strbuf *buffer)\n-{\n-\tsendline(helper, buffer);\n-\trecvline(helper, buffer);\n-}\n-\n static void write_constant(int fd, const char *str)\n {\n \tif (debug)\n@@ -307,7 +301,8 @@ static int set_helper_option(struct transport *transport,\n \t\tquote_c_style(value, &buf, NULL, 0);\n \tstrbuf_addch(&buf, '\\n');\n \n-\txchgline(data, &buf);\n+\tsendline(data, &buf);\n+\trecvline(data, &buf);\n \n \tif (!strcmp(buf.buf, \"ok\"))\n \t\tret = 0;\n-- \n1.9.1+fc3.9.gc73078e\n"},{"id":"238790","messageId":"1397334812-12215-3-git-send-email-felipe.contreras@gmail.com","threadId":"36403","inReplyTo":"1397334812-12215-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/5] remote-helpers: make recvline return an error","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-12T20:33:29Z","receivedAt":"2014-04-12T20:33:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Instead of exiting directly, make it the duty of the caller to do so.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n transport-helper.c | 21 ++++++++++++++-------\n 1 file changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex bf329fd..1432a6d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -58,7 +58,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)\n \tif (strbuf_getline(buffer, helper, '\\n') == EOF) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Remote helper quit.\\n\");\n-\t\texit(128);\n+\t\treturn 1;\n \t}\n \n \tif (debug)\n@@ -157,7 +157,8 @@ static struct child_process *get_helper(struct transport *transport)\n \twhile (1) {\n \t\tconst char *capname;\n \t\tint mandatory = 0;\n-\t\trecvline(data, &buf);\n+\t\tif (recvline(data, &buf))\n+\t\t\texit(128);\n \n \t\tif (!*buf.buf)\n \t\t\tbreak;\n@@ -302,7 +303,8 @@ static int set_helper_option(struct transport *transport,\n \tstrbuf_addch(&buf, '\\n');\n \n \tsendline(data, &buf);\n-\trecvline(data, &buf);\n+\tif (recvline(data, &buf))\n+\t\texit(128);\n \n \tif (!strcmp(buf.buf, \"ok\"))\n \t\tret = 0;\n@@ -374,7 +376,8 @@ static int fetch_with_fetch(struct transport *transport,\n \tsendline(data, &buf);\n \n \twhile (1) {\n-\t\trecvline(data, &buf);\n+\t\tif (recvline(data, &buf))\n+\t\t\texit(128);\n \n \t\tif (starts_with(buf.buf, \"lock \")) {\n \t\t\tconst char *name = buf.buf + 5;\n@@ -558,7 +561,9 @@ static int process_connect_service(struct transport *transport,\n \t\tgoto exit;\n \n \tsendline(data, &cmdbuf);\n-\trecvline_fh(input, &cmdbuf, name);\n+\tif (recvline_fh(input, &cmdbuf, name))\n+\t\texit(128);\n+\n \tif (!strcmp(cmdbuf.buf, \"\")) {\n \t\tdata->no_disconnect_req = 1;\n \t\tif (debug)\n@@ -736,7 +741,8 @@ static void push_update_refs_status(struct helper_data *data,\n \tfor (;;) {\n \t\tchar *private;\n \n-\t\trecvline(data, &buf);\n+\t\tif (recvline(data, &buf))\n+\t\t\texit(128);\n \t\tif (!buf.len)\n \t\t\tbreak;\n \n@@ -960,7 +966,8 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n \n \twhile (1) {\n \t\tchar *eov, *eon;\n-\t\trecvline(data, &buf);\n+\t\tif (recvline(data, &buf))\n+\t\t\texit(128);\n \n \t\tif (!*buf.buf)\n \t\t\tbreak;\n-- \n1.9.1+fc3.9.gc73078e\n"},{"id":"238793","messageId":"1397334812-12215-4-git-send-email-felipe.contreras@gmail.com","threadId":"36403","inReplyTo":"1397334812-12215-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 3/5] transport-helper: propagate recvline() error pushing","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-12T20:33:30Z","receivedAt":"2014-04-12T20:33:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It's cleaner, and will allow us to do something sensible on errors\nlater.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n transport-helper.c | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 1432a6d..b068ea5 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -733,16 +733,21 @@ static int push_update_ref_status(struct strbuf *buf,\n \treturn !(status == REF_STATUS_OK);\n }\n \n-static void push_update_refs_status(struct helper_data *data,\n+static int push_update_refs_status(struct helper_data *data,\n \t\t\t\t    struct ref *remote_refs)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ref *ref = remote_refs;\n+\tint ret = 0;\n+\n \tfor (;;) {\n \t\tchar *private;\n \n-\t\tif (recvline(data, &buf))\n-\t\t\texit(128);\n+\t\tif (recvline(data, &buf)) {\n+\t\t\tret = 1;\n+\t\t\tbreak;\n+\t\t}\n+\n \t\tif (!buf.len)\n \t\t\tbreak;\n \n@@ -760,6 +765,7 @@ static void push_update_refs_status(struct helper_data *data,\n \t\tfree(private);\n \t}\n \tstrbuf_release(&buf);\n+\treturn ret;\n }\n \n static int push_refs_with_push(struct transport *transport,\n@@ -840,8 +846,7 @@ static int push_refs_with_push(struct transport *transport,\n \tsendline(data, &buf);\n \tstrbuf_release(&buf);\n \n-\tpush_update_refs_status(data, remote_refs);\n-\treturn 0;\n+\treturn push_update_refs_status(data, remote_refs);\n }\n \n static int push_refs_with_export(struct transport *transport,\n@@ -897,8 +902,7 @@ static int push_refs_with_export(struct transport *transport,\n \n \tif (finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n-\tpush_update_refs_status(data, remote_refs);\n-\treturn 0;\n+\treturn push_update_refs_status(data, remote_refs);\n }\n \n static int push_refs(struct transport *transport,\n-- \n1.9.1+fc3.9.gc73078e\n"},{"id":"238792","messageId":"1397334812-12215-5-git-send-email-felipe.contreras@gmail.com","threadId":"36403","inReplyTo":"1397334812-12215-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 4/5] transport-helper: trivial cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-12T20:33:31Z","receivedAt":"2014-04-12T20:33:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It's simpler to store the file names directly, and form the fast-export\narguments only when needed, and re-use the same strbuf with a format.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n transport-helper.c | 23 +++++++++++------------\n 1 file changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex b068ea5..2747f98 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -195,15 +195,9 @@ static struct child_process *get_helper(struct transport *transport)\n \t\t} else if (!strcmp(capname, \"signed-tags\")) {\n \t\t\tdata->signed_tags = 1;\n \t\t} else if (starts_with(capname, \"export-marks \")) {\n-\t\t\tstruct strbuf arg = STRBUF_INIT;\n-\t\t\tstrbuf_addstr(&arg, \"--export-marks=\");\n-\t\t\tstrbuf_addstr(&arg, capname + strlen(\"export-marks \"));\n-\t\t\tdata->export_marks = strbuf_detach(&arg, NULL);\n+\t\t\tdata->export_marks = xstrdup(capname + strlen(\"export-marks \"));\n \t\t} else if (starts_with(capname, \"import-marks\")) {\n-\t\t\tstruct strbuf arg = STRBUF_INIT;\n-\t\t\tstrbuf_addstr(&arg, \"--import-marks=\");\n-\t\t\tstrbuf_addstr(&arg, capname + strlen(\"import-marks \"));\n-\t\t\tdata->import_marks = strbuf_detach(&arg, NULL);\n+\t\t\tdata->import_marks = xstrdup(capname + strlen(\"import-marks \"));\n \t\t} else if (starts_with(capname, \"no-private-update\")) {\n \t\t\tdata->no_private_update = 1;\n \t\t} else if (mandatory) {\n@@ -429,6 +423,7 @@ static int get_exporter(struct transport *transport,\n \tstruct child_process *helper = get_helper(transport);\n \tint argc = 0, i;\n \tmemset(fastexport, 0, sizeof(*fastexport));\n+\tstruct strbuf tmp = STRBUF_INIT;\n \n \t/* we need to duplicate helper->in because we want to use it after\n \t * fastexport is done with it. */\n@@ -438,10 +433,14 @@ static int get_exporter(struct transport *transport,\n \tfastexport->argv[argc++] = \"--use-done-feature\";\n \tfastexport->argv[argc++] = data->signed_tags ?\n \t\t\"--signed-tags=verbatim\" : \"--signed-tags=warn-strip\";\n-\tif (data->export_marks)\n-\t\tfastexport->argv[argc++] = data->export_marks;\n-\tif (data->import_marks)\n-\t\tfastexport->argv[argc++] = data->import_marks;\n+\tif (data->export_marks) {\n+\t\tstrbuf_addf(&tmp, \"--export-marks=%s\", data->export_marks);\n+\t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n+\t}\n+\tif (data->import_marks) {\n+\t\tstrbuf_addf(&tmp, \"--import-marks=%s\", data->import_marks);\n+\t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n+\t}\n \n \tfor (i = 0; i < revlist_args->nr; i++)\n \t\tfastexport->argv[argc++] = revlist_args->items[i].string;\n-- \n1.9.1+fc3.9.gc73078e\n"},{"id":"238791","messageId":"1397334812-12215-6-git-send-email-felipe.contreras@gmail.com","threadId":"36403","inReplyTo":"1397334812-12215-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 5/5] transport-helper: fix sync issue on crashes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-12T20:33:32Z","receivedAt":"2014-04-12T20:33:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"When a remote helper crashes while pushing we should revert back to the\nstate before the push, however, it's possible that `git fast-export`\nalready finished its job, and therefore has exported the marks already.\n\nThis creates a synchronization problem because from that moment on\n`git fast-{import,export}` will have marks that the remote helper is not\naware of and all further commands fail (if those marks are referenced).\n\nThe fix is to tell `git fast-export` to export to a temporary file, and\nonly after the remote helper has finishes successfully, move to the\nfinal destination.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n t/t5801-remote-helpers.sh | 17 ++++++++++++++++-\n transport-helper.c        | 13 +++++++++++--\n 2 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 613f69a..cf7fd43 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -207,6 +207,17 @@ test_expect_success 'push update refs failure' '\n \t)\n '\n \n+clean_mark () {\n+\tcut -f 2 -d ' ' $1 | git cat-file --batch-check | grep commit | sort > $(basename $1)\n+}\n+\n+cmp_marks () {\n+\ttest_when_finished \"rm -rf git.marks testgit.marks\" &&\n+\tclean_mark .git/testgit/$1/git.marks &&\n+\tclean_mark .git/testgit/$1/testgit.marks &&\n+\ttest_cmp git.marks testgit.marks\n+}\n+\n test_expect_success 'proper failure checks for fetching' '\n \t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n \texport GIT_REMOTE_TESTGIT_FAILURE &&\n@@ -221,7 +232,11 @@ test_expect_success 'proper failure checks for pushing' '\n \t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n \texport GIT_REMOTE_TESTGIT_FAILURE &&\n \tcd local &&\n-\ttest_must_fail git push --all\n+\tgit checkout -b crash master &&\n+\techo crash >> file &&\n+\tgit commit -a -m crash &&\n+\ttest_must_fail git push --all &&\n+\tcmp_marks origin\n \t)\n '\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2747f98..090c863 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -434,7 +434,7 @@ static int get_exporter(struct transport *transport,\n \tfastexport->argv[argc++] = data->signed_tags ?\n \t\t\"--signed-tags=verbatim\" : \"--signed-tags=warn-strip\";\n \tif (data->export_marks) {\n-\t\tstrbuf_addf(&tmp, \"--export-marks=%s\", data->export_marks);\n+\t\tstrbuf_addf(&tmp, \"--export-marks=%s.tmp\", data->export_marks);\n \t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n \t}\n \tif (data->import_marks) {\n@@ -901,7 +901,16 @@ static int push_refs_with_export(struct transport *transport,\n \n \tif (finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n-\treturn push_update_refs_status(data, remote_refs);\n+\tif (push_update_refs_status(data, remote_refs))\n+\t\treturn 1;\n+\n+\tif (data->export_marks) {\n+\t\tstrbuf_addf(&buf, \"%s.tmp\", data->export_marks);\n+\t\trename(buf.buf, data->export_marks);\n+\t\tstrbuf_release(&buf);\n+\t}\n+\n+\treturn 0;\n }\n \n static int push_refs(struct transport *transport,\n-- \n1.9.1+fc3.9.gc73078e\n"},{"id":"238866","messageId":"xmqqha5vmx55.fsf@gitster.dls.corp.google.com","threadId":"36403","inReplyTo":"1397334812-12215-5-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 4/5] transport-helper: trivial cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-14T21:14:30Z","receivedAt":"2014-04-14T21:14:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> It's simpler to store the file names directly, and form the fast-export\n> arguments only when needed, and re-use the same strbuf with a format.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  transport-helper.c | 23 +++++++++++------------\n>  1 file changed, 11 insertions(+), 12 deletions(-)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index b068ea5..2747f98 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -195,15 +195,9 @@ static struct child_process *get_helper(struct transport *transport)\n>  \t\t} else if (!strcmp(capname, \"signed-tags\")) {\n>  \t\t\tdata->signed_tags = 1;\n>  \t\t} else if (starts_with(capname, \"export-marks \")) {\n> -\t\t\tstruct strbuf arg = STRBUF_INIT;\n> -\t\t\tstrbuf_addstr(&arg, \"--export-marks=\");\n> -\t\t\tstrbuf_addstr(&arg, capname + strlen(\"export-marks \"));\n> -\t\t\tdata->export_marks = strbuf_detach(&arg, NULL);\n> +\t\t\tdata->export_marks = xstrdup(capname + strlen(\"export-marks \"));\n>  \t\t} else if (starts_with(capname, \"import-marks\")) {\n> -\t\t\tstruct strbuf arg = STRBUF_INIT;\n> -\t\t\tstrbuf_addstr(&arg, \"--import-marks=\");\n> -\t\t\tstrbuf_addstr(&arg, capname + strlen(\"import-marks \"));\n> -\t\t\tdata->import_marks = strbuf_detach(&arg, NULL);\n> +\t\t\tdata->import_marks = xstrdup(capname + strlen(\"import-marks \"));\n>  \t\t} else if (starts_with(capname, \"no-private-update\")) {\n>  \t\t\tdata->no_private_update = 1;\n>  \t\t} else if (mandatory) {\n> @@ -429,6 +423,7 @@ static int get_exporter(struct transport *transport,\n>  \tstruct child_process *helper = get_helper(transport);\n>  \tint argc = 0, i;\n>  \tmemset(fastexport, 0, sizeof(*fastexport));\n> +\tstruct strbuf tmp = STRBUF_INIT;\n\nWill fix up decl-after-stmt while queuing.\n\n>  \n>  \t/* we need to duplicate helper->in because we want to use it after\n>  \t * fastexport is done with it. */\n> @@ -438,10 +433,14 @@ static int get_exporter(struct transport *transport,\n>  \tfastexport->argv[argc++] = \"--use-done-feature\";\n>  \tfastexport->argv[argc++] = data->signed_tags ?\n>  \t\t\"--signed-tags=verbatim\" : \"--signed-tags=warn-strip\";\n> -\tif (data->export_marks)\n> -\t\tfastexport->argv[argc++] = data->export_marks;\n> -\tif (data->import_marks)\n> -\t\tfastexport->argv[argc++] = data->import_marks;\n> +\tif (data->export_marks) {\n> +\t\tstrbuf_addf(&tmp, \"--export-marks=%s\", data->export_marks);\n> +\t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n> +\t}\n> +\tif (data->import_marks) {\n> +\t\tstrbuf_addf(&tmp, \"--import-marks=%s\", data->import_marks);\n> +\t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n> +\t}\n>  \n>  \tfor (i = 0; i < revlist_args->nr; i++)\n>  \t\tfastexport->argv[argc++] = revlist_args->items[i].string;\n"},{"id":"238869","messageId":"xmqq38hfmwml.fsf@gitster.dls.corp.google.com","threadId":"36403","inReplyTo":"1397334812-12215-6-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 5/5] transport-helper: fix sync issue on crashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-14T21:25:38Z","receivedAt":"2014-04-14T21:25:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> When a remote helper crashes while pushing we should revert back to the\n> state before the push, however, it's possible that `git fast-export`\n> already finished its job, and therefore has exported the marks already.\n>\n> This creates a synchronization problem because from that moment on\n> `git fast-{import,export}` will have marks that the remote helper is not\n> aware of and all further commands fail (if those marks are referenced).\n>\n> The fix is to tell `git fast-export` to export to a temporary file, and\n> only after the remote helper has finishes successfully, move to the\n> final destination.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n\nThis seems to be based on a somewhat older codebase; I tried to be\ncareful while adjusting the patch to the current codebase, but\nplease give it an eyeball to see if I didn't make any silly mistake\nwhen I push today's integration result out in a few hours.\n\nThanks.\n\n>  t/t5801-remote-helpers.sh | 17 ++++++++++++++++-\n>  transport-helper.c        | 13 +++++++++++--\n>  2 files changed, 27 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index 613f69a..cf7fd43 100755\n> --- a/t/t5801-remote-helpers.sh\n> +++ b/t/t5801-remote-helpers.sh\n> @@ -207,6 +207,17 @@ test_expect_success 'push update refs failure' '\n>  \t)\n>  '\n>  \n> +clean_mark () {\n> +\tcut -f 2 -d ' ' $1 | git cat-file --batch-check | grep commit | sort > $(basename $1)\n> +}\n> +\n> +cmp_marks () {\n> +\ttest_when_finished \"rm -rf git.marks testgit.marks\" &&\n> +\tclean_mark .git/testgit/$1/git.marks &&\n> +\tclean_mark .git/testgit/$1/testgit.marks &&\n> +\ttest_cmp git.marks testgit.marks\n> +}\n> +\n>  test_expect_success 'proper failure checks for fetching' '\n>  \t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n>  \texport GIT_REMOTE_TESTGIT_FAILURE &&\n> @@ -221,7 +232,11 @@ test_expect_success 'proper failure checks for pushing' '\n>  \t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n>  \texport GIT_REMOTE_TESTGIT_FAILURE &&\n>  \tcd local &&\n> -\ttest_must_fail git push --all\n> +\tgit checkout -b crash master &&\n> +\techo crash >> file &&\n> +\tgit commit -a -m crash &&\n> +\ttest_must_fail git push --all &&\n> +\tcmp_marks origin\n>  \t)\n>  '\n>  \n> diff --git a/transport-helper.c b/transport-helper.c\n> index 2747f98..090c863 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -434,7 +434,7 @@ static int get_exporter(struct transport *transport,\n>  \tfastexport->argv[argc++] = data->signed_tags ?\n>  \t\t\"--signed-tags=verbatim\" : \"--signed-tags=warn-strip\";\n>  \tif (data->export_marks) {\n> -\t\tstrbuf_addf(&tmp, \"--export-marks=%s\", data->export_marks);\n> +\t\tstrbuf_addf(&tmp, \"--export-marks=%s.tmp\", data->export_marks);\n>  \t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n>  \t}\n>  \tif (data->import_marks) {\n> @@ -901,7 +901,16 @@ static int push_refs_with_export(struct transport *transport,\n>  \n>  \tif (finish_command(&exporter))\n>  \t\tdie(\"Error while running fast-export\");\n> -\treturn push_update_refs_status(data, remote_refs);\n> +\tif (push_update_refs_status(data, remote_refs))\n> +\t\treturn 1;\n> +\n> +\tif (data->export_marks) {\n> +\t\tstrbuf_addf(&buf, \"%s.tmp\", data->export_marks);\n> +\t\trename(buf.buf, data->export_marks);\n> +\t\tstrbuf_release(&buf);\n> +\t}\n> +\n> +\treturn 0;\n>  }\n>  \n>  static int push_refs(struct transport *transport,\n"},{"id":"238905","messageId":"xmqqbnw2jn9u.fsf@gitster.dls.corp.google.com","threadId":"36403","inReplyTo":"1397334812-12215-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 0/5] transport-helper: serious crash fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-15T21:28:13Z","receivedAt":"2014-04-15T21:28:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> One of the most serious recurring issues[1][2][3] with remote helpers is that\n> marks get out of sync. The way to analize and reproduce these wasn't trivial,\n> but the culprit seems to be a crash while doing `git push`. It has been known\n> already how exactly that happens, but no simple way how to fix it.\n>\n> This is the simplest way so far; tell `git fast-export` to export the marks to\n> a temporary file, and move it to the right location only *after* the remote\n> helper has finished its job without errors.\n>\n> Since the code wasn't prepared for a change like this, some reorganization\n> changes are needed. More changes might be welcome to further propagate the\n> errors properly through the code, but for the moment the errors are propagated\n> to the right location, in order to fix this specific problem.\n\nThis seems to be based on a somewhat older codebase; I tried to be\ncareful while adjusting the patch to the current codebase, but\nplease give it an eyeball to see if I didn't make any silly mistake\nin the version that has been parked on 'pu'.\n\nAfter an positive Ack (or \"you botched the forward-porting; here is\na rebased one you should replace them with\"), I'd like to merge it\nto 'next' and then to 'master' by -rc0 (or -rc1 at the latest) so\nthat we have enough time to expose the updated code before 2.0.\n\nThanks.\n"},{"id":"239098","messageId":"1397890843-27035-1-git-send-email-gitster@pobox.com","threadId":"36403","inReplyTo":"1397334812-12215-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 0/5] fc/transport-helper-sync-error-fix rebased","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-19T07:00:38Z","receivedAt":"2014-04-19T07:00:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"As I have said in the recent What's cooking reports, the original\nposted here were based on older codebase and needed to be rebased,\nbut it had some conflicts and I wanted to see the result double\nchecked by the original author before we can merge it to 'next',\ncooked there and hopefully merged to 'master' before tagging -rc1.\n\nSo here is the series that has been queued in 'pu' for the past\nseveral days.\n\nFelipe, can you double check it?\n\nThanks.\n\nFelipe Contreras (5):\n  transport-helper: remove barely used xchgline()\n  remote-helpers: make recvline return an error\n  transport-helper: propagate recvline() error pushing\n  transport-helper: trivial cleanup\n  transport-helper: fix sync issue on crashes\n\n t/t5801-remote-helpers.sh | 20 ++++++++++++-\n transport-helper.c        | 73 ++++++++++++++++++++++++++++-------------------\n 2 files changed, 63 insertions(+), 30 deletions(-)\n\n-- \n1.9.2-459-g68773ac\n"},{"id":"239100","messageId":"1397890843-27035-2-git-send-email-gitster@pobox.com","threadId":"36403","inReplyTo":"1397890843-27035-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/5] transport-helper: remove barely used xchgline()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-19T07:00:39Z","receivedAt":"2014-04-19T07:00:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Felipe Contreras <felipe.contreras@gmail.com>\n\nIt's only used once, we can just call the two functions inside directly.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n transport-helper.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 86e1679..892107c 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -71,12 +71,6 @@ static int recvline(struct helper_data *helper, struct strbuf *buffer)\n \treturn recvline_fh(helper->out, buffer, helper->name);\n }\n \n-static void xchgline(struct helper_data *helper, struct strbuf *buffer)\n-{\n-\tsendline(helper, buffer);\n-\trecvline(helper, buffer);\n-}\n-\n static void write_constant(int fd, const char *str)\n {\n \tif (debug)\n@@ -307,7 +301,8 @@ static int set_helper_option(struct transport *transport,\n \t\tquote_c_style(value, &buf, NULL, 0);\n \tstrbuf_addch(&buf, '\\n');\n \n-\txchgline(data, &buf);\n+\tsendline(data, &buf);\n+\trecvline(data, &buf);\n \n \tif (!strcmp(buf.buf, \"ok\"))\n \t\tret = 0;\n-- \n1.9.2-459-g68773ac\n"},{"id":"239099","messageId":"1397890843-27035-3-git-send-email-gitster@pobox.com","threadId":"36403","inReplyTo":"1397890843-27035-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/5] remote-helpers: make recvline return an error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-19T07:00:40Z","receivedAt":"2014-04-19T07:00:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Felipe Contreras <felipe.contreras@gmail.com>\n\nInstead of exiting directly, make it the duty of the caller to do so.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n transport-helper.c | 21 ++++++++++++++-------\n 1 file changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 892107c..f0d7fc7 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -58,7 +58,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)\n \tif (strbuf_getline(buffer, helper, '\\n') == EOF) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Remote helper quit.\\n\");\n-\t\texit(128);\n+\t\treturn 1;\n \t}\n \n \tif (debug)\n@@ -157,7 +157,8 @@ static struct child_process *get_helper(struct transport *transport)\n \twhile (1) {\n \t\tconst char *capname;\n \t\tint mandatory = 0;\n-\t\trecvline(data, &buf);\n+\t\tif (recvline(data, &buf))\n+\t\t\texit(128);\n \n \t\tif (!*buf.buf)\n \t\t\tbreak;\n@@ -302,7 +303,8 @@ static int set_helper_option(struct transport *transport,\n \tstrbuf_addch(&buf, '\\n');\n \n \tsendline(data, &buf);\n-\trecvline(data, &buf);\n+\tif (recvline(data, &buf))\n+\t\texit(128);\n \n \tif (!strcmp(buf.buf, \"ok\"))\n \t\tret = 0;\n@@ -374,7 +376,8 @@ static int fetch_with_fetch(struct transport *transport,\n \tsendline(data, &buf);\n \n \twhile (1) {\n-\t\trecvline(data, &buf);\n+\t\tif (recvline(data, &buf))\n+\t\t\texit(128);\n \n \t\tif (starts_with(buf.buf, \"lock \")) {\n \t\t\tconst char *name = buf.buf + 5;\n@@ -558,7 +561,9 @@ static int process_connect_service(struct transport *transport,\n \t\tgoto exit;\n \n \tsendline(data, &cmdbuf);\n-\trecvline_fh(input, &cmdbuf, name);\n+\tif (recvline_fh(input, &cmdbuf, name))\n+\t\texit(128);\n+\n \tif (!strcmp(cmdbuf.buf, \"\")) {\n \t\tdata->no_disconnect_req = 1;\n \t\tif (debug)\n@@ -743,7 +748,8 @@ static void push_update_refs_status(struct helper_data *data,\n \tfor (;;) {\n \t\tchar *private;\n \n-\t\trecvline(data, &buf);\n+\t\tif (recvline(data, &buf))\n+\t\t\texit(128);\n \t\tif (!buf.len)\n \t\t\tbreak;\n \n@@ -969,7 +975,8 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n \n \twhile (1) {\n \t\tchar *eov, *eon;\n-\t\trecvline(data, &buf);\n+\t\tif (recvline(data, &buf))\n+\t\t\texit(128);\n \n \t\tif (!*buf.buf)\n \t\t\tbreak;\n-- \n1.9.2-459-g68773ac\n"},{"id":"239101","messageId":"1397890843-27035-4-git-send-email-gitster@pobox.com","threadId":"36403","inReplyTo":"1397890843-27035-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/5] transport-helper: propagate recvline() error pushing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-19T07:00:41Z","receivedAt":"2014-04-19T07:00:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Felipe Contreras <felipe.contreras@gmail.com>\n\nIt's cleaner, and will allow us to do something sensible on errors\nlater.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n transport-helper.c | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex f0d7fc7..e91bc9a 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -739,17 +739,22 @@ static int push_update_ref_status(struct strbuf *buf,\n \treturn !(status == REF_STATUS_OK);\n }\n \n-static void push_update_refs_status(struct helper_data *data,\n+static int push_update_refs_status(struct helper_data *data,\n \t\t\t\t    struct ref *remote_refs,\n \t\t\t\t    int flags)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ref *ref = remote_refs;\n+\tint ret = 0;\n+\n \tfor (;;) {\n \t\tchar *private;\n \n-\t\tif (recvline(data, &buf))\n-\t\t\texit(128);\n+\t\tif (recvline(data, &buf)) {\n+\t\t\tret = 1;\n+\t\t\tbreak;\n+\t\t}\n+\n \t\tif (!buf.len)\n \t\t\tbreak;\n \n@@ -767,6 +772,7 @@ static void push_update_refs_status(struct helper_data *data,\n \t\tfree(private);\n \t}\n \tstrbuf_release(&buf);\n+\treturn ret;\n }\n \n static int push_refs_with_push(struct transport *transport,\n@@ -847,8 +853,7 @@ static int push_refs_with_push(struct transport *transport,\n \tsendline(data, &buf);\n \tstrbuf_release(&buf);\n \n-\tpush_update_refs_status(data, remote_refs, flags);\n-\treturn 0;\n+\treturn push_update_refs_status(data, remote_refs, flags);\n }\n \n static int push_refs_with_export(struct transport *transport,\n@@ -906,8 +911,7 @@ static int push_refs_with_export(struct transport *transport,\n \n \tif (finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n-\tpush_update_refs_status(data, remote_refs, flags);\n-\treturn 0;\n+\treturn push_update_refs_status(data, remote_refs, flags);\n }\n \n static int push_refs(struct transport *transport,\n-- \n1.9.2-459-g68773ac\n"},{"id":"239102","messageId":"1397890843-27035-5-git-send-email-gitster@pobox.com","threadId":"36403","inReplyTo":"1397890843-27035-1-git-send-email-gitster@pobox.com","subject":"[PATCH 4/5] transport-helper: trivial cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-19T07:00:42Z","receivedAt":"2014-04-19T07:00:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Felipe Contreras <felipe.contreras@gmail.com>\n\nIt's simpler to store the file names directly, and form the fast-export\narguments only when needed, and re-use the same strbuf with a format.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n transport-helper.c | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex e91bc9a..c890db6 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -195,15 +195,9 @@ static struct child_process *get_helper(struct transport *transport)\n \t\t} else if (!strcmp(capname, \"signed-tags\")) {\n \t\t\tdata->signed_tags = 1;\n \t\t} else if (starts_with(capname, \"export-marks \")) {\n-\t\t\tstruct strbuf arg = STRBUF_INIT;\n-\t\t\tstrbuf_addstr(&arg, \"--export-marks=\");\n-\t\t\tstrbuf_addstr(&arg, capname + strlen(\"export-marks \"));\n-\t\t\tdata->export_marks = strbuf_detach(&arg, NULL);\n+\t\t\tdata->export_marks = xstrdup(capname + strlen(\"export-marks \"));\n \t\t} else if (starts_with(capname, \"import-marks\")) {\n-\t\t\tstruct strbuf arg = STRBUF_INIT;\n-\t\t\tstrbuf_addstr(&arg, \"--import-marks=\");\n-\t\t\tstrbuf_addstr(&arg, capname + strlen(\"import-marks \"));\n-\t\t\tdata->import_marks = strbuf_detach(&arg, NULL);\n+\t\t\tdata->import_marks = xstrdup(capname + strlen(\"import-marks \"));\n \t\t} else if (starts_with(capname, \"no-private-update\")) {\n \t\t\tdata->no_private_update = 1;\n \t\t} else if (mandatory) {\n@@ -428,6 +422,8 @@ static int get_exporter(struct transport *transport,\n \tstruct helper_data *data = transport->data;\n \tstruct child_process *helper = get_helper(transport);\n \tint argc = 0, i;\n+\tstruct strbuf tmp = STRBUF_INIT;\n+\n \tmemset(fastexport, 0, sizeof(*fastexport));\n \n \t/* we need to duplicate helper->in because we want to use it after\n@@ -438,10 +434,14 @@ static int get_exporter(struct transport *transport,\n \tfastexport->argv[argc++] = \"--use-done-feature\";\n \tfastexport->argv[argc++] = data->signed_tags ?\n \t\t\"--signed-tags=verbatim\" : \"--signed-tags=warn-strip\";\n-\tif (data->export_marks)\n-\t\tfastexport->argv[argc++] = data->export_marks;\n-\tif (data->import_marks)\n-\t\tfastexport->argv[argc++] = data->import_marks;\n+\tif (data->export_marks) {\n+\t\tstrbuf_addf(&tmp, \"--export-marks=%s\", data->export_marks);\n+\t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n+\t}\n+\tif (data->import_marks) {\n+\t\tstrbuf_addf(&tmp, \"--import-marks=%s\", data->import_marks);\n+\t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n+\t}\n \n \tfor (i = 0; i < revlist_args->nr; i++)\n \t\tfastexport->argv[argc++] = revlist_args->items[i].string;\n-- \n1.9.2-459-g68773ac\n"},{"id":"239103","messageId":"1397890843-27035-6-git-send-email-gitster@pobox.com","threadId":"36403","inReplyTo":"1397890843-27035-1-git-send-email-gitster@pobox.com","subject":"[PATCH 5/5] transport-helper: fix sync issue on crashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-19T07:00:43Z","receivedAt":"2014-04-19T07:00:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Felipe Contreras <felipe.contreras@gmail.com>\n\nWhen a remote helper crashes while pushing we should revert back to the\nstate before the push, however, it's possible that `git fast-export`\nalready finished its job, and therefore has exported the marks already.\n\nThis creates a synchronization problem because from that moment on\n`git fast-{import,export}` will have marks that the remote helper is not\naware of and all further commands fail (if those marks are referenced).\n\nThe fix is to tell `git fast-export` to export to a temporary file, and\nonly after the remote helper has finishes successfully, move to the\nfinal destination.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t5801-remote-helpers.sh | 20 +++++++++++++++++++-\n transport-helper.c        | 13 +++++++++++--\n 2 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 25fd2e7..aa3e223 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -220,6 +220,20 @@ test_expect_success 'push update refs failure' '\n \t)\n '\n \n+clean_mark () {\n+\tcut -f 2 -d ' ' \"$1\" |\n+\tgit cat-file --batch-check |\n+\tgrep commit |\n+\tsort >$(basename \"$1\")\n+}\n+\n+cmp_marks () {\n+\ttest_when_finished \"rm -rf git.marks testgit.marks\" &&\n+\tclean_mark \".git/testgit/$1/git.marks\" &&\n+\tclean_mark \".git/testgit/$1/testgit.marks\" &&\n+\ttest_cmp git.marks testgit.marks\n+}\n+\n test_expect_success 'proper failure checks for fetching' '\n \t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n \texport GIT_REMOTE_TESTGIT_FAILURE &&\n@@ -232,7 +246,11 @@ test_expect_success 'proper failure checks for fetching' '\n \n test_expect_success 'proper failure checks for pushing' '\n \t(cd local &&\n-\ttest_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all\n+\tgit checkout -b crash master &&\n+\techo crash >>file &&\n+\tgit commit -a -m crash &&\n+\ttest_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all &&\n+\tcmp_marks origin\n \t)\n '\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex c890db6..b468e4f 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -435,7 +435,7 @@ static int get_exporter(struct transport *transport,\n \tfastexport->argv[argc++] = data->signed_tags ?\n \t\t\"--signed-tags=verbatim\" : \"--signed-tags=warn-strip\";\n \tif (data->export_marks) {\n-\t\tstrbuf_addf(&tmp, \"--export-marks=%s\", data->export_marks);\n+\t\tstrbuf_addf(&tmp, \"--export-marks=%s.tmp\", data->export_marks);\n \t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n \t}\n \tif (data->import_marks) {\n@@ -911,7 +911,16 @@ static int push_refs_with_export(struct transport *transport,\n \n \tif (finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n-\treturn push_update_refs_status(data, remote_refs, flags);\n+\tif (push_update_refs_status(data, remote_refs, flags))\n+\t\treturn 1;\n+\n+\tif (data->export_marks) {\n+\t\tstrbuf_addf(&buf, \"%s.tmp\", data->export_marks);\n+\t\trename(buf.buf, data->export_marks);\n+\t\tstrbuf_release(&buf);\n+\t}\n+\n+\treturn 0;\n }\n \n static int push_refs(struct transport *transport,\n-- \n1.9.2-459-g68773ac\n"},{"id":"239153","messageId":"535413cbc7eaa_1f08dd12eca5@nysa.notmuch","threadId":"36403","inReplyTo":"1397890843-27035-1-git-send-email-gitster@pobox.com","subject":"RE: [PATCH 0/5] fc/transport-helper-sync-error-fix rebased","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-20T18:36:59Z","receivedAt":"2014-04-20T18:36:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> As I have said in the recent What's cooking reports, the original\n> posted here were based on older codebase and needed to be rebased,\n> but it had some conflicts and I wanted to see the result double\n> checked by the original author before we can merge it to 'next',\n> cooked there and hopefully merged to 'master' before tagging -rc1.\n> \n> So here is the series that has been queued in 'pu' for the past\n> several days.\n> \n> Felipe, can you double check it?\n\nThese patches don't help much, I did and interdiff with my own fixes and this\nis the result:\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 284bf3f..aa3e223 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -221,13 +221,16 @@ test_expect_success 'push update refs failure' '\n '\n \n clean_mark () {\n-       cut -f 2 -d ' ' $1 | git cat-file --batch-check | grep commit | sort > $(basename $1)\n+       cut -f 2 -d ' ' \"$1\" |\n+       git cat-file --batch-check |\n+       grep commit |\n+       sort >$(basename \"$1\")\n }\n \n cmp_marks () {\n        test_when_finished \"rm -rf git.marks testgit.marks\" &&\n-       clean_mark .git/testgit/$1/git.marks &&\n-       clean_mark .git/testgit/$1/testgit.marks &&\n+       clean_mark \".git/testgit/$1/git.marks\" &&\n+       clean_mark \".git/testgit/$1/testgit.marks\" &&\n        test_cmp git.marks testgit.marks\n }\n \n@@ -244,7 +247,7 @@ test_expect_success 'proper failure checks for fetching' '\n test_expect_success 'proper failure checks for pushing' '\n        (cd local &&\n        git checkout -b crash master &&\n-       echo crash >> file &&\n+       echo crash >>file &&\n        git commit -a -m crash &&\n        test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all &&\n        cmp_marks origin\n\nI don't like it, but it's OK.\n\n-- \nFelipe Contreras\n"},{"id":"239181","messageId":"7voazvlna8.fsf@alter.siamese.dyndns.org","threadId":"36403","inReplyTo":"535413cbc7eaa_1f08dd12eca5@nysa.notmuch","subject":"Re: [PATCH 0/5] fc/transport-helper-sync-error-fix rebased","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-20T21:10:55Z","receivedAt":"2014-04-20T21:10:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> As I have said in the recent What's cooking reports, the original\n>> posted here were based on older codebase and needed to be rebased,\n>> but it had some conflicts and I wanted to see the result double\n>> checked by the original author before we can merge it to 'next',\n>> cooked there and hopefully merged to 'master' before tagging -rc1.\n>> \n>> So here is the series that has been queued in 'pu' for the past\n>> several days.\n>> \n>> Felipe, can you double check it?\n>\n> These patches don't help much,...\n\nWhat do you mean by that, exactly?  As long as your \"don't help\nmuch\" is \"would not hurt and will help some even for a small subset\nof audience\", that would be OK, but I am puzzled.\n\nMy reading of your responses to bug reports and the cover letter of\nthe series has been that these were real fixes to a real problem\nwithout downsides, and that you consider that they are good changes\nto have in the upcoming release.\n\nI am hoping that you did not mean that we shouldn't merge it to the\n'next' and 'master' branches, but if that is what you meant, can we\nhear what the downsides of the series are?\nAre they more churn than they are worth without solving the real\nissue?  Do they regress for some repositories/workflows while\nimproving for others?  I didn't get such an impression.\n\n\n> ..., I did and interdiff with my own fixes and this is the result:\n> ...  I don't like it, but it's OK.\n\nCorrect.  Following the coding style of the project is not the\nmatter for your liking or not liking.  It is part of being on the\nsame codebase with other participant of this project.\n"},{"id":"239183","messageId":"535438246097c_4c30158d2ec11@nysa.notmuch","threadId":"36403","inReplyTo":"7voazvlna8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/5] fc/transport-helper-sync-error-fix rebased","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-20T21:12:04Z","receivedAt":"2014-04-20T21:12:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> \n> > Junio C Hamano wrote:\n> >> As I have said in the recent What's cooking reports, the original\n> >> posted here were based on older codebase and needed to be rebased,\n> >> but it had some conflicts and I wanted to see the result double\n> >> checked by the original author before we can merge it to 'next',\n> >> cooked there and hopefully merged to 'master' before tagging -rc1.\n> >> \n> >> So here is the series that has been queued in 'pu' for the past\n> >> several days.\n> >> \n> >> Felipe, can you double check it?\n> >\n> > These patches don't help much,...\n> \n> What do you mean by that, exactly?  As long as your \"don't help\n> much\" is \"would not hurt and will help some even for a small subset\n> of audience\", that would be OK, but I am puzzled.\n\nI mean if purpose of these was for me to review the changes you did, it doesn't\nhelp as much as an interdiff does.\n\n> My reading of your responses to bug reports and the cover letter of\n> the series has been that these were real fixes to a real problem\n> without downsides, and that you consider that they are good changes\n> to have in the upcoming release.\n> \n> I am hoping that you did not mean that we shouldn't merge it to the\n> 'next' and 'master' branches, but if that is what you meant, can we\n> hear what the downsides of the series are?\n> Are they more churn than they are worth without solving the real\n> issue?  Do they regress for some repositories/workflows while\n> improving for others?  I didn't get such an impression.\n\nThat's not what I meant. The patches are good.\n\n> > ..., I did and interdiff with my own fixes and this is the result:\n> > ...  I don't like it, but it's OK.\n> \n> Correct.  Following the coding style of the project is not the\n> matter for your liking or not liking.  It is part of being on the\n> same codebase with other participant of this project.\n\nThere's nothing in Documentation/CodingGuidelines that says multiple piped\ncommands should be one on each line even if togther the line doesn't exceed 80\ncharacters, nor does it says that file names should be between quotes, even if\nthe file name can't possibly have spaces.\n\nThese are just your arbitrary rules, not the guidelines of the project.\n\nAnd BTW, the fact that something is a rule doesn't mean it's good; rules change.\n\n-- \nFelipe Contreras\n"},{"id":"239189","messageId":"7v7g6jllck.fsf@alter.siamese.dyndns.org","threadId":"36403","inReplyTo":"535438246097c_4c30158d2ec11@nysa.notmuch","subject":"Re: [PATCH 0/5] fc/transport-helper-sync-error-fix rebased","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-20T21:52:43Z","receivedAt":"2014-04-20T21:52:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>> \n>> > Junio C Hamano wrote:\n>> >> As I have said in the recent What's cooking reports, the original\n>> >> posted here were based on older codebase and needed to be rebased,\n>> >> but it had some conflicts and I wanted to see the result double\n>> >> checked by the original author before we can merge it to 'next',\n>> >> cooked there and hopefully merged to 'master' before tagging -rc1.\n>> >> \n>> >> So here is the series that has been queued in 'pu' for the past\n>> >> several days.\n>> >> \n>> >> Felipe, can you double check it?\n>> >\n>> > These patches don't help much,...\n>> \n>> What do you mean by that, exactly?  As long as your \"don't help\n>> much\" is \"would not hurt and will help some even for a small subset\n>> of audience\", that would be OK, but I am puzzled.\n>\n> I mean if purpose of these was for me to review the changes you did, it doesn't\n> help as much as an interdiff does.\n\nAh, I misunderstood.  The thing was, that the original was based on\nolder codebase and I couldn't apply them to my tree cleanly.  It\nwould be unfair for you to expect _me_ to give you an interdiff in\nsuch a situation, especially when I am asking you to double check\nthe conflict resolution based on that original.\n\nOK, so I'll advance the topic to the 'next' and hopefully we can\nmerge it to 'master' before rc1.\n\n> There's nothing in Documentation/CodingGuidelines that says multiple piped\n> commands should be one on each line even if togther the line doesn't exceed 80\n> characters, nor does it says that file names should be between quotes, even if\n> the file name can't possibly have spaces.\n\nHeh, we don't spell out every possible rules. That is where \"do as\nsurrounding code\" comes in.\n\nAs to \"$1\" vs $1 without quotes, I had to check the calling sites of\nclean_mark and cmp_marks, primarily because these functions do not\nsay what their \"$1\" is, to see if an unquoted form is safe.  The\nnext person who reads the script has to do the same and quoting is\nan obvious way to avoid it.\n\nIf the functions said \"$1 is a branch name\", it would have been\nclear that unquoted $1 would be OK, but still, what these functions\nhas to take (especially the \"clean\" one that takes pathnames) can\nchange in the future, and a quoted form is an obvious way to\nfutureproof and relieve readers and programmers from having to worry\nabout the quoting issues.\n\nSo I think it is better to quote these in this particular case.\n\nThe pipe is purely subjective readability. I can go either way, but\nsince I was the one who was applying the patch that needed other\nchanges anyway... ;-)\n"},{"id":"239190","messageId":"53544213e6635_a81169b2ece1@nysa.notmuch","threadId":"36403","inReplyTo":"7v7g6jllck.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/5] fc/transport-helper-sync-error-fix rebased","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-04-20T21:54:27Z","receivedAt":"2014-04-20T21:54:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> So I think it is better to quote these in this particular case.\n> \n> The pipe is purely subjective readability. I can go either way, but\n> since I was the one who was applying the patch that needed other\n> changes anyway... ;-)\n\nYou started by saying these were \"not the matter for your liking or not\nliking\", and now you are explaining why you like one better than the other.\n\nI don't think it's particularly helpful to criticize one person's preferences\nbecause they don't match yours.\n\nYou like one, I like another. That's it.\n\n-- \nFelipe Contreras\n"}]}