[PATCH 0/5] transport-helper: serious crash fix

STALE3723d

15 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCH 0/5] transport-helper: serious crash fix

From: Felipe Contreras <hidden>
Date: 2016-06-15 23:00:41 

One of the most serious recurring issues[1][2][3] with remote helpers is that
marks get out of sync. The way to analize and reproduce these wasn't trivial,
but the culprit seems to be a crash while doing `git push`. It has been known
already how exactly that happens, but no simple way how to fix it.

This is the simplest way so far; tell `git fast-export` to export the marks to
a temporary file, and move it to the right location only *after* the remote
helper has finished its job without errors.

Since the code wasn't prepared for a change like this, some reorganization
changes are needed. More changes might be welcome to further propagate the
errors properly through the code, but for the moment the errors are propagated
to the right location, in order to fix this specific problem.

[1] http://article.gmane.org/gmane.comp.version-control.git/223962
[2] http://article.gmane.org/gmane.comp.version-control.git/241707
[3] https://github.com/felipec/git/issues/56

Felipe Contreras (5):
  transport-helper: remove barely used xchgline()
  remote-helpers: make recvline return an error
  transport-helper: propagate recvline() error pushing
  transport-helper: trivial cleanup
  transport-helper: fix sync issue on crashes

 t/t5801-remote-helpers.sh | 17 ++++++++++-
 transport-helper.c        | 72 ++++++++++++++++++++++++++++-------------------
 2 files changed, 59 insertions(+), 30 deletions(-)

-- 
1.9.1+fc3.9.gc73078e

[PATCH 1/5] transport-helper: remove barely used xchgline()

From: Felipe Contreras <hidden>
Date: 2016-06-15 23:00:41 

It's only used once, we can just call the two functions inside directly.

Signed-off-by: Felipe Contreras <redacted>
---
 transport-helper.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index ad72fbd..bf329fd 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -71,12 +71,6 @@ static int recvline(struct helper_data *helper, struct strbuf *buffer)
 	return recvline_fh(helper->out, buffer, helper->name);
 }
 
-static void xchgline(struct helper_data *helper, struct strbuf *buffer)
-{
-	sendline(helper, buffer);
-	recvline(helper, buffer);
-}
-
 static void write_constant(int fd, const char *str)
 {
 	if (debug)
@@ -307,7 +301,8 @@ static int set_helper_option(struct transport *transport,
 		quote_c_style(value, &buf, NULL, 0);
 	strbuf_addch(&buf, '\n');
 
-	xchgline(data, &buf);
+	sendline(data, &buf);
+	recvline(data, &buf);
 
 	if (!strcmp(buf.buf, "ok"))
 		ret = 0;
-- 
1.9.1+fc3.9.gc73078e

[PATCH 2/5] remote-helpers: make recvline return an error

From: Felipe Contreras <hidden>
Date: 2016-06-15 23:00:41 

Instead of exiting directly, make it the duty of the caller to do so.

Signed-off-by: Felipe Contreras <redacted>
---
 transport-helper.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index bf329fd..1432a6d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -58,7 +58,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)
 	if (strbuf_getline(buffer, helper, '\n') == EOF) {
 		if (debug)
 			fprintf(stderr, "Debug: Remote helper quit.\n");
-		exit(128);
+		return 1;
 	}
 
 	if (debug)
@@ -157,7 +157,8 @@ static struct child_process *get_helper(struct transport *transport)
 	while (1) {
 		const char *capname;
 		int mandatory = 0;
-		recvline(data, &buf);
+		if (recvline(data, &buf))
+			exit(128);
 
 		if (!*buf.buf)
 			break;
@@ -302,7 +303,8 @@ static int set_helper_option(struct transport *transport,
 	strbuf_addch(&buf, '\n');
 
 	sendline(data, &buf);
-	recvline(data, &buf);
+	if (recvline(data, &buf))
+		exit(128);
 
 	if (!strcmp(buf.buf, "ok"))
 		ret = 0;
@@ -374,7 +376,8 @@ static int fetch_with_fetch(struct transport *transport,
 	sendline(data, &buf);
 
 	while (1) {
-		recvline(data, &buf);
+		if (recvline(data, &buf))
+			exit(128);
 
 		if (starts_with(buf.buf, "lock ")) {
 			const char *name = buf.buf + 5;
@@ -558,7 +561,9 @@ static int process_connect_service(struct transport *transport,
 		goto exit;
 
 	sendline(data, &cmdbuf);
-	recvline_fh(input, &cmdbuf, name);
+	if (recvline_fh(input, &cmdbuf, name))
+		exit(128);
+
 	if (!strcmp(cmdbuf.buf, "")) {
 		data->no_disconnect_req = 1;
 		if (debug)
@@ -736,7 +741,8 @@ static void push_update_refs_status(struct helper_data *data,
 	for (;;) {
 		char *private;
 
-		recvline(data, &buf);
+		if (recvline(data, &buf))
+			exit(128);
 		if (!buf.len)
 			break;
 
@@ -960,7 +966,8 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 
 	while (1) {
 		char *eov, *eon;
-		recvline(data, &buf);
+		if (recvline(data, &buf))
+			exit(128);
 
 		if (!*buf.buf)
 			break;
-- 
1.9.1+fc3.9.gc73078e

[PATCH 5/5] transport-helper: fix sync issue on crashes

From: Felipe Contreras <hidden>
Date: 2016-06-15 23:00:41 

When a remote helper crashes while pushing we should revert back to the
state before the push, however, it's possible that `git fast-export`
already finished its job, and therefore has exported the marks already.

This creates a synchronization problem because from that moment on
`git fast-{import,export}` will have marks that the remote helper is not
aware of and all further commands fail (if those marks are referenced).

The fix is to tell `git fast-export` to export to a temporary file, and
only after the remote helper has finishes successfully, move to the
final destination.

Signed-off-by: Felipe Contreras <redacted>
---
 t/t5801-remote-helpers.sh | 17 ++++++++++++++++-
 transport-helper.c        | 13 +++++++++++--
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 613f69a..cf7fd43 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -207,6 +207,17 @@ test_expect_success 'push update refs failure' '
 	)
 '
 
+clean_mark () {
+	cut -f 2 -d ' ' $1 | git cat-file --batch-check | grep commit | sort > $(basename $1)
+}
+
+cmp_marks () {
+	test_when_finished "rm -rf git.marks testgit.marks" &&
+	clean_mark .git/testgit/$1/git.marks &&
+	clean_mark .git/testgit/$1/testgit.marks &&
+	test_cmp git.marks testgit.marks
+}
+
 test_expect_success 'proper failure checks for fetching' '
 	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
 	export GIT_REMOTE_TESTGIT_FAILURE &&
@@ -221,7 +232,11 @@ test_expect_success 'proper failure checks for pushing' '
 	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
 	export GIT_REMOTE_TESTGIT_FAILURE &&
 	cd local &&
-	test_must_fail git push --all
+	git checkout -b crash master &&
+	echo crash >> file &&
+	git commit -a -m crash &&
+	test_must_fail git push --all &&
+	cmp_marks origin
 	)
 '
 
diff --git a/transport-helper.c b/transport-helper.c
index 2747f98..090c863 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -434,7 +434,7 @@ static int get_exporter(struct transport *transport,
 	fastexport->argv[argc++] = data->signed_tags ?
 		"--signed-tags=verbatim" : "--signed-tags=warn-strip";
 	if (data->export_marks) {
-		strbuf_addf(&tmp, "--export-marks=%s", data->export_marks);
+		strbuf_addf(&tmp, "--export-marks=%s.tmp", data->export_marks);
 		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
 	}
 	if (data->import_marks) {
@@ -901,7 +901,16 @@ static int push_refs_with_export(struct transport *transport,
 
 	if (finish_command(&exporter))
 		die("Error while running fast-export");
-	return push_update_refs_status(data, remote_refs);
+	if (push_update_refs_status(data, remote_refs))
+		return 1;
+
+	if (data->export_marks) {
+		strbuf_addf(&buf, "%s.tmp", data->export_marks);
+		rename(buf.buf, data->export_marks);
+		strbuf_release(&buf);
+	}
+
+	return 0;
 }
 
 static int push_refs(struct transport *transport,
-- 
1.9.1+fc3.9.gc73078e

[PATCH 4/5] transport-helper: trivial cleanup

From: Felipe Contreras <hidden>
Date: 2016-06-15 23:00:41 

It's simpler to store the file names directly, and form the fast-export
arguments only when needed, and re-use the same strbuf with a format.

Signed-off-by: Felipe Contreras <redacted>
---
 transport-helper.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index b068ea5..2747f98 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -195,15 +195,9 @@ static struct child_process *get_helper(struct transport *transport)
 		} else if (!strcmp(capname, "signed-tags")) {
 			data->signed_tags = 1;
 		} else if (starts_with(capname, "export-marks ")) {
-			struct strbuf arg = STRBUF_INIT;
-			strbuf_addstr(&arg, "--export-marks=");
-			strbuf_addstr(&arg, capname + strlen("export-marks "));
-			data->export_marks = strbuf_detach(&arg, NULL);
+			data->export_marks = xstrdup(capname + strlen("export-marks "));
 		} else if (starts_with(capname, "import-marks")) {
-			struct strbuf arg = STRBUF_INIT;
-			strbuf_addstr(&arg, "--import-marks=");
-			strbuf_addstr(&arg, capname + strlen("import-marks "));
-			data->import_marks = strbuf_detach(&arg, NULL);
+			data->import_marks = xstrdup(capname + strlen("import-marks "));
 		} else if (starts_with(capname, "no-private-update")) {
 			data->no_private_update = 1;
 		} else if (mandatory) {
@@ -429,6 +423,7 @@ static int get_exporter(struct transport *transport,
 	struct child_process *helper = get_helper(transport);
 	int argc = 0, i;
 	memset(fastexport, 0, sizeof(*fastexport));
+	struct strbuf tmp = STRBUF_INIT;
 
 	/* we need to duplicate helper->in because we want to use it after
 	 * fastexport is done with it. */
@@ -438,10 +433,14 @@ static int get_exporter(struct transport *transport,
 	fastexport->argv[argc++] = "--use-done-feature";
 	fastexport->argv[argc++] = data->signed_tags ?
 		"--signed-tags=verbatim" : "--signed-tags=warn-strip";
-	if (data->export_marks)
-		fastexport->argv[argc++] = data->export_marks;
-	if (data->import_marks)
-		fastexport->argv[argc++] = data->import_marks;
+	if (data->export_marks) {
+		strbuf_addf(&tmp, "--export-marks=%s", data->export_marks);
+		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
+	}
+	if (data->import_marks) {
+		strbuf_addf(&tmp, "--import-marks=%s", data->import_marks);
+		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
+	}
 
 	for (i = 0; i < revlist_args->nr; i++)
 		fastexport->argv[argc++] = revlist_args->items[i].string;
-- 
1.9.1+fc3.9.gc73078e

[PATCH 3/5] transport-helper: propagate recvline() error pushing

From: Felipe Contreras <hidden>
Date: 2016-06-15 23:00:41 

It's cleaner, and will allow us to do something sensible on errors
later.

Signed-off-by: Felipe Contreras <redacted>
---
 transport-helper.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 1432a6d..b068ea5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -733,16 +733,21 @@ static int push_update_ref_status(struct strbuf *buf,
 	return !(status == REF_STATUS_OK);
 }
 
-static void push_update_refs_status(struct helper_data *data,
+static int push_update_refs_status(struct helper_data *data,
 				    struct ref *remote_refs)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct ref *ref = remote_refs;
+	int ret = 0;
+
 	for (;;) {
 		char *private;
 
-		if (recvline(data, &buf))
-			exit(128);
+		if (recvline(data, &buf)) {
+			ret = 1;
+			break;
+		}
+
 		if (!buf.len)
 			break;
 
@@ -760,6 +765,7 @@ static void push_update_refs_status(struct helper_data *data,
 		free(private);
 	}
 	strbuf_release(&buf);
+	return ret;
 }
 
 static int push_refs_with_push(struct transport *transport,
@@ -840,8 +846,7 @@ static int push_refs_with_push(struct transport *transport,
 	sendline(data, &buf);
 	strbuf_release(&buf);
 
-	push_update_refs_status(data, remote_refs);
-	return 0;
+	return push_update_refs_status(data, remote_refs);
 }
 
 static int push_refs_with_export(struct transport *transport,
@@ -897,8 +902,7 @@ static int push_refs_with_export(struct transport *transport,
 
 	if (finish_command(&exporter))
 		die("Error while running fast-export");
-	push_update_refs_status(data, remote_refs);
-	return 0;
+	return push_update_refs_status(data, remote_refs);
 }
 
 static int push_refs(struct transport *transport,
-- 
1.9.1+fc3.9.gc73078e

Re: [PATCH 4/5] transport-helper: trivial cleanup

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:00:42 

Felipe Contreras [off-list ref] writes:
quoted hunk
It's simpler to store the file names directly, and form the fast-export
arguments only when needed, and re-use the same strbuf with a format.

Signed-off-by: Felipe Contreras <redacted>
---
 transport-helper.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index b068ea5..2747f98 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -195,15 +195,9 @@ static struct child_process *get_helper(struct transport *transport)
 		} else if (!strcmp(capname, "signed-tags")) {
 			data->signed_tags = 1;
 		} else if (starts_with(capname, "export-marks ")) {
-			struct strbuf arg = STRBUF_INIT;
-			strbuf_addstr(&arg, "--export-marks=");
-			strbuf_addstr(&arg, capname + strlen("export-marks "));
-			data->export_marks = strbuf_detach(&arg, NULL);
+			data->export_marks = xstrdup(capname + strlen("export-marks "));
 		} else if (starts_with(capname, "import-marks")) {
-			struct strbuf arg = STRBUF_INIT;
-			strbuf_addstr(&arg, "--import-marks=");
-			strbuf_addstr(&arg, capname + strlen("import-marks "));
-			data->import_marks = strbuf_detach(&arg, NULL);
+			data->import_marks = xstrdup(capname + strlen("import-marks "));
 		} else if (starts_with(capname, "no-private-update")) {
 			data->no_private_update = 1;
 		} else if (mandatory) {
@@ -429,6 +423,7 @@ static int get_exporter(struct transport *transport,
 	struct child_process *helper = get_helper(transport);
 	int argc = 0, i;
 	memset(fastexport, 0, sizeof(*fastexport));
+	struct strbuf tmp = STRBUF_INIT;
Will fix up decl-after-stmt while queuing.
quoted hunk
 
 	/* we need to duplicate helper->in because we want to use it after
 	 * fastexport is done with it. */
@@ -438,10 +433,14 @@ static int get_exporter(struct transport *transport,
 	fastexport->argv[argc++] = "--use-done-feature";
 	fastexport->argv[argc++] = data->signed_tags ?
 		"--signed-tags=verbatim" : "--signed-tags=warn-strip";
-	if (data->export_marks)
-		fastexport->argv[argc++] = data->export_marks;
-	if (data->import_marks)
-		fastexport->argv[argc++] = data->import_marks;
+	if (data->export_marks) {
+		strbuf_addf(&tmp, "--export-marks=%s", data->export_marks);
+		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
+	}
+	if (data->import_marks) {
+		strbuf_addf(&tmp, "--import-marks=%s", data->import_marks);
+		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
+	}
 
 	for (i = 0; i < revlist_args->nr; i++)
 		fastexport->argv[argc++] = revlist_args->items[i].string;

Re: [PATCH 5/5] transport-helper: fix sync issue on crashes

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:00:42 

Felipe Contreras [off-list ref] writes:

When a remote helper crashes while pushing we should revert back to the
state before the push, however, it's possible that `git fast-export`
already finished its job, and therefore has exported the marks already.

This creates a synchronization problem because from that moment on
`git fast-{import,export}` will have marks that the remote helper is not
aware of and all further commands fail (if those marks are referenced).

The fix is to tell `git fast-export` to export to a temporary file, and
only after the remote helper has finishes successfully, move to the
final destination.

Signed-off-by: Felipe Contreras <redacted>
---
This seems to be based on a somewhat older codebase; I tried to be
careful while adjusting the patch to the current codebase, but
please give it an eyeball to see if I didn't make any silly mistake
when I push today's integration result out in a few hours.

Thanks.
quoted hunk
 t/t5801-remote-helpers.sh | 17 ++++++++++++++++-
 transport-helper.c        | 13 +++++++++++--
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 613f69a..cf7fd43 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -207,6 +207,17 @@ test_expect_success 'push update refs failure' '
 	)
 '
 
+clean_mark () {
+	cut -f 2 -d ' ' $1 | git cat-file --batch-check | grep commit | sort > $(basename $1)
+}
+
+cmp_marks () {
+	test_when_finished "rm -rf git.marks testgit.marks" &&
+	clean_mark .git/testgit/$1/git.marks &&
+	clean_mark .git/testgit/$1/testgit.marks &&
+	test_cmp git.marks testgit.marks
+}
+
 test_expect_success 'proper failure checks for fetching' '
 	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
 	export GIT_REMOTE_TESTGIT_FAILURE &&
@@ -221,7 +232,11 @@ test_expect_success 'proper failure checks for pushing' '
 	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
 	export GIT_REMOTE_TESTGIT_FAILURE &&
 	cd local &&
-	test_must_fail git push --all
+	git checkout -b crash master &&
+	echo crash >> file &&
+	git commit -a -m crash &&
+	test_must_fail git push --all &&
+	cmp_marks origin
 	)
 '
 
diff --git a/transport-helper.c b/transport-helper.c
index 2747f98..090c863 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -434,7 +434,7 @@ static int get_exporter(struct transport *transport,
 	fastexport->argv[argc++] = data->signed_tags ?
 		"--signed-tags=verbatim" : "--signed-tags=warn-strip";
 	if (data->export_marks) {
-		strbuf_addf(&tmp, "--export-marks=%s", data->export_marks);
+		strbuf_addf(&tmp, "--export-marks=%s.tmp", data->export_marks);
 		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
 	}
 	if (data->import_marks) {
@@ -901,7 +901,16 @@ static int push_refs_with_export(struct transport *transport,
 
 	if (finish_command(&exporter))
 		die("Error while running fast-export");
-	return push_update_refs_status(data, remote_refs);
+	if (push_update_refs_status(data, remote_refs))
+		return 1;
+
+	if (data->export_marks) {
+		strbuf_addf(&buf, "%s.tmp", data->export_marks);
+		rename(buf.buf, data->export_marks);
+		strbuf_release(&buf);
+	}
+
+	return 0;
 }
 
 static int push_refs(struct transport *transport,

[PATCH 0/5] fc/transport-helper-sync-error-fix rebased

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:00:44 

As I have said in the recent What's cooking reports, the original
posted here were based on older codebase and needed to be rebased,
but it had some conflicts and I wanted to see the result double
checked by the original author before we can merge it to 'next',
cooked there and hopefully merged to 'master' before tagging -rc1.

So here is the series that has been queued in 'pu' for the past
several days.

Felipe, can you double check it?

Thanks.

Felipe Contreras (5):
  transport-helper: remove barely used xchgline()
  remote-helpers: make recvline return an error
  transport-helper: propagate recvline() error pushing
  transport-helper: trivial cleanup
  transport-helper: fix sync issue on crashes

 t/t5801-remote-helpers.sh | 20 ++++++++++++-
 transport-helper.c        | 73 ++++++++++++++++++++++++++++-------------------
 2 files changed, 63 insertions(+), 30 deletions(-)

-- 
1.9.2-459-g68773ac

[PATCH 2/5] remote-helpers: make recvline return an error

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:00:44 

From: Felipe Contreras <redacted>

Instead of exiting directly, make it the duty of the caller to do so.

Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 transport-helper.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 892107c..f0d7fc7 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -58,7 +58,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)
 	if (strbuf_getline(buffer, helper, '\n') == EOF) {
 		if (debug)
 			fprintf(stderr, "Debug: Remote helper quit.\n");
-		exit(128);
+		return 1;
 	}
 
 	if (debug)
@@ -157,7 +157,8 @@ static struct child_process *get_helper(struct transport *transport)
 	while (1) {
 		const char *capname;
 		int mandatory = 0;
-		recvline(data, &buf);
+		if (recvline(data, &buf))
+			exit(128);
 
 		if (!*buf.buf)
 			break;
@@ -302,7 +303,8 @@ static int set_helper_option(struct transport *transport,
 	strbuf_addch(&buf, '\n');
 
 	sendline(data, &buf);
-	recvline(data, &buf);
+	if (recvline(data, &buf))
+		exit(128);
 
 	if (!strcmp(buf.buf, "ok"))
 		ret = 0;
@@ -374,7 +376,8 @@ static int fetch_with_fetch(struct transport *transport,
 	sendline(data, &buf);
 
 	while (1) {
-		recvline(data, &buf);
+		if (recvline(data, &buf))
+			exit(128);
 
 		if (starts_with(buf.buf, "lock ")) {
 			const char *name = buf.buf + 5;
@@ -558,7 +561,9 @@ static int process_connect_service(struct transport *transport,
 		goto exit;
 
 	sendline(data, &cmdbuf);
-	recvline_fh(input, &cmdbuf, name);
+	if (recvline_fh(input, &cmdbuf, name))
+		exit(128);
+
 	if (!strcmp(cmdbuf.buf, "")) {
 		data->no_disconnect_req = 1;
 		if (debug)
@@ -743,7 +748,8 @@ static void push_update_refs_status(struct helper_data *data,
 	for (;;) {
 		char *private;
 
-		recvline(data, &buf);
+		if (recvline(data, &buf))
+			exit(128);
 		if (!buf.len)
 			break;
 
@@ -969,7 +975,8 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 
 	while (1) {
 		char *eov, *eon;
-		recvline(data, &buf);
+		if (recvline(data, &buf))
+			exit(128);
 
 		if (!*buf.buf)
 			break;
-- 
1.9.2-459-g68773ac

[PATCH 1/5] transport-helper: remove barely used xchgline()

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:00:44 

From: Felipe Contreras <redacted>

It's only used once, we can just call the two functions inside directly.

Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 transport-helper.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 86e1679..892107c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -71,12 +71,6 @@ static int recvline(struct helper_data *helper, struct strbuf *buffer)
 	return recvline_fh(helper->out, buffer, helper->name);
 }
 
-static void xchgline(struct helper_data *helper, struct strbuf *buffer)
-{
-	sendline(helper, buffer);
-	recvline(helper, buffer);
-}
-
 static void write_constant(int fd, const char *str)
 {
 	if (debug)
@@ -307,7 +301,8 @@ static int set_helper_option(struct transport *transport,
 		quote_c_style(value, &buf, NULL, 0);
 	strbuf_addch(&buf, '\n');
 
-	xchgline(data, &buf);
+	sendline(data, &buf);
+	recvline(data, &buf);
 
 	if (!strcmp(buf.buf, "ok"))
 		ret = 0;
-- 
1.9.2-459-g68773ac

[PATCH 3/5] transport-helper: propagate recvline() error pushing

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:00:44 

From: Felipe Contreras <redacted>

It's cleaner, and will allow us to do something sensible on errors
later.

Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 transport-helper.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index f0d7fc7..e91bc9a 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -739,17 +739,22 @@ static int push_update_ref_status(struct strbuf *buf,
 	return !(status == REF_STATUS_OK);
 }
 
-static void push_update_refs_status(struct helper_data *data,
+static int push_update_refs_status(struct helper_data *data,
 				    struct ref *remote_refs,
 				    int flags)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct ref *ref = remote_refs;
+	int ret = 0;
+
 	for (;;) {
 		char *private;
 
-		if (recvline(data, &buf))
-			exit(128);
+		if (recvline(data, &buf)) {
+			ret = 1;
+			break;
+		}
+
 		if (!buf.len)
 			break;
 
@@ -767,6 +772,7 @@ static void push_update_refs_status(struct helper_data *data,
 		free(private);
 	}
 	strbuf_release(&buf);
+	return ret;
 }
 
 static int push_refs_with_push(struct transport *transport,
@@ -847,8 +853,7 @@ static int push_refs_with_push(struct transport *transport,
 	sendline(data, &buf);
 	strbuf_release(&buf);
 
-	push_update_refs_status(data, remote_refs, flags);
-	return 0;
+	return push_update_refs_status(data, remote_refs, flags);
 }
 
 static int push_refs_with_export(struct transport *transport,
@@ -906,8 +911,7 @@ static int push_refs_with_export(struct transport *transport,
 
 	if (finish_command(&exporter))
 		die("Error while running fast-export");
-	push_update_refs_status(data, remote_refs, flags);
-	return 0;
+	return push_update_refs_status(data, remote_refs, flags);
 }
 
 static int push_refs(struct transport *transport,
-- 
1.9.2-459-g68773ac

[PATCH 4/5] transport-helper: trivial cleanup

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:00:44 

From: Felipe Contreras <redacted>

It's simpler to store the file names directly, and form the fast-export
arguments only when needed, and re-use the same strbuf with a format.

Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 transport-helper.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index e91bc9a..c890db6 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -195,15 +195,9 @@ static struct child_process *get_helper(struct transport *transport)
 		} else if (!strcmp(capname, "signed-tags")) {
 			data->signed_tags = 1;
 		} else if (starts_with(capname, "export-marks ")) {
-			struct strbuf arg = STRBUF_INIT;
-			strbuf_addstr(&arg, "--export-marks=");
-			strbuf_addstr(&arg, capname + strlen("export-marks "));
-			data->export_marks = strbuf_detach(&arg, NULL);
+			data->export_marks = xstrdup(capname + strlen("export-marks "));
 		} else if (starts_with(capname, "import-marks")) {
-			struct strbuf arg = STRBUF_INIT;
-			strbuf_addstr(&arg, "--import-marks=");
-			strbuf_addstr(&arg, capname + strlen("import-marks "));
-			data->import_marks = strbuf_detach(&arg, NULL);
+			data->import_marks = xstrdup(capname + strlen("import-marks "));
 		} else if (starts_with(capname, "no-private-update")) {
 			data->no_private_update = 1;
 		} else if (mandatory) {
@@ -428,6 +422,8 @@ static int get_exporter(struct transport *transport,
 	struct helper_data *data = transport->data;
 	struct child_process *helper = get_helper(transport);
 	int argc = 0, i;
+	struct strbuf tmp = STRBUF_INIT;
+
 	memset(fastexport, 0, sizeof(*fastexport));
 
 	/* we need to duplicate helper->in because we want to use it after
@@ -438,10 +434,14 @@ static int get_exporter(struct transport *transport,
 	fastexport->argv[argc++] = "--use-done-feature";
 	fastexport->argv[argc++] = data->signed_tags ?
 		"--signed-tags=verbatim" : "--signed-tags=warn-strip";
-	if (data->export_marks)
-		fastexport->argv[argc++] = data->export_marks;
-	if (data->import_marks)
-		fastexport->argv[argc++] = data->import_marks;
+	if (data->export_marks) {
+		strbuf_addf(&tmp, "--export-marks=%s", data->export_marks);
+		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
+	}
+	if (data->import_marks) {
+		strbuf_addf(&tmp, "--import-marks=%s", data->import_marks);
+		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
+	}
 
 	for (i = 0; i < revlist_args->nr; i++)
 		fastexport->argv[argc++] = revlist_args->items[i].string;
-- 
1.9.2-459-g68773ac

[PATCH 5/5] transport-helper: fix sync issue on crashes

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:00:44 

From: Felipe Contreras <redacted>

When a remote helper crashes while pushing we should revert back to the
state before the push, however, it's possible that `git fast-export`
already finished its job, and therefore has exported the marks already.

This creates a synchronization problem because from that moment on
`git fast-{import,export}` will have marks that the remote helper is not
aware of and all further commands fail (if those marks are referenced).

The fix is to tell `git fast-export` to export to a temporary file, and
only after the remote helper has finishes successfully, move to the
final destination.

Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 t/t5801-remote-helpers.sh | 20 +++++++++++++++++++-
 transport-helper.c        | 13 +++++++++++--
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 25fd2e7..aa3e223 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -220,6 +220,20 @@ test_expect_success 'push update refs failure' '
 	)
 '
 
+clean_mark () {
+	cut -f 2 -d ' ' "$1" |
+	git cat-file --batch-check |
+	grep commit |
+	sort >$(basename "$1")
+}
+
+cmp_marks () {
+	test_when_finished "rm -rf git.marks testgit.marks" &&
+	clean_mark ".git/testgit/$1/git.marks" &&
+	clean_mark ".git/testgit/$1/testgit.marks" &&
+	test_cmp git.marks testgit.marks
+}
+
 test_expect_success 'proper failure checks for fetching' '
 	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
 	export GIT_REMOTE_TESTGIT_FAILURE &&
@@ -232,7 +246,11 @@ test_expect_success 'proper failure checks for fetching' '
 
 test_expect_success 'proper failure checks for pushing' '
 	(cd local &&
-	test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all
+	git checkout -b crash master &&
+	echo crash >>file &&
+	git commit -a -m crash &&
+	test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all &&
+	cmp_marks origin
 	)
 '
 
diff --git a/transport-helper.c b/transport-helper.c
index c890db6..b468e4f 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -435,7 +435,7 @@ static int get_exporter(struct transport *transport,
 	fastexport->argv[argc++] = data->signed_tags ?
 		"--signed-tags=verbatim" : "--signed-tags=warn-strip";
 	if (data->export_marks) {
-		strbuf_addf(&tmp, "--export-marks=%s", data->export_marks);
+		strbuf_addf(&tmp, "--export-marks=%s.tmp", data->export_marks);
 		fastexport->argv[argc++] = strbuf_detach(&tmp, NULL);
 	}
 	if (data->import_marks) {
@@ -911,7 +911,16 @@ static int push_refs_with_export(struct transport *transport,
 
 	if (finish_command(&exporter))
 		die("Error while running fast-export");
-	return push_update_refs_status(data, remote_refs, flags);
+	if (push_update_refs_status(data, remote_refs, flags))
+		return 1;
+
+	if (data->export_marks) {
+		strbuf_addf(&buf, "%s.tmp", data->export_marks);
+		rename(buf.buf, data->export_marks);
+		strbuf_release(&buf);
+	}
+
+	return 0;
 }
 
 static int push_refs(struct transport *transport,
-- 
1.9.2-459-g68773ac

RE: [PATCH 0/5] fc/transport-helper-sync-error-fix rebased

From: Felipe Contreras <hidden>
Date: 2016-06-15 23:00:44 

Junio C Hamano wrote:
As I have said in the recent What's cooking reports, the original
posted here were based on older codebase and needed to be rebased,
but it had some conflicts and I wanted to see the result double
checked by the original author before we can merge it to 'next',
cooked there and hopefully merged to 'master' before tagging -rc1.

So here is the series that has been queued in 'pu' for the past
several days.

Felipe, can you double check it?
These patches don't help much, I did and interdiff with my own fixes and this
is the result:

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 284bf3f..aa3e223 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -221,13 +221,16 @@ test_expect_success 'push update refs failure' '
 '
 
 clean_mark () {
-       cut -f 2 -d ' ' $1 | git cat-file --batch-check | grep commit | sort > $(basename $1)
+       cut -f 2 -d ' ' "$1" |
+       git cat-file --batch-check |
+       grep commit |
+       sort >$(basename "$1")
 }
 
 cmp_marks () {
        test_when_finished "rm -rf git.marks testgit.marks" &&
-       clean_mark .git/testgit/$1/git.marks &&
-       clean_mark .git/testgit/$1/testgit.marks &&
+       clean_mark ".git/testgit/$1/git.marks" &&
+       clean_mark ".git/testgit/$1/testgit.marks" &&
        test_cmp git.marks testgit.marks
 }
 
@@ -244,7 +247,7 @@ test_expect_success 'proper failure checks for fetching' '
 test_expect_success 'proper failure checks for pushing' '
        (cd local &&
        git checkout -b crash master &&
-       echo crash >> file &&
+       echo crash >>file &&
        git commit -a -m crash &&
        test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all &&
        cmp_marks origin

I don't like it, but it's OK.

-- 
Felipe Contreras
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help