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

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

[Felipe Contreras]

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()

[Felipe Contreras]

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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

[Felipe Contreras]

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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 3/5] transport-helper: propagate recvline() error pushing

[Felipe Contreras]

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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

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

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
---
 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

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> 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 <felipe.contreras@gmail.com>
> ---
>  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.

>  
>  	/* 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;

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

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
---
 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

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> 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 <felipe.contreras@gmail.com>
> ---

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.

>  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,

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> 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.

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
in the version that has been parked on 'pu'.

After an positive Ack (or "you botched the forward-porting; here is
a rebased one you should replace them with"), I'd like to merge it
to 'next' and then to 'master' by -rc0 (or -rc1 at the latest) so
that we have enough time to expose the updated code before 2.0.

Thanks.

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

[Junio C Hamano]

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 1/5] transport-helper: remove barely used xchgline()

[Junio C Hamano]

From: Felipe Contreras <felipe.contreras@gmail.com>

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 2/5] remote-helpers: make recvline return an error

[Junio C Hamano]

From: Felipe Contreras <felipe.contreras@gmail.com>

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 3/5] transport-helper: propagate recvline() error pushing

[Junio C Hamano]

From: Felipe Contreras <felipe.contreras@gmail.com>

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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

[Junio C Hamano]

From: Felipe Contreras <felipe.contreras@gmail.com>

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 <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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

[Junio C Hamano]

From: Felipe Contreras <felipe.contreras@gmail.com>

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 <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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

[Felipe Contreras]

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

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> 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,...

What do you mean by that, exactly?  As long as your "don't help
much" is "would not hurt and will help some even for a small subset
of audience", that would be OK, but I am puzzled.

My reading of your responses to bug reports and the cover letter of
the series has been that these were real fixes to a real problem
without downsides, and that you consider that they are good changes
to have in the upcoming release.

I am hoping that you did not mean that we shouldn't merge it to the
'next' and 'master' branches, but if that is what you meant, can we
hear what the downsides of the series are?
Are they more churn than they are worth without solving the real
issue?  Do they regress for some repositories/workflows while
improving for others?  I didn't get such an impression.


> ..., I did and interdiff with my own fixes and this is the result:
> ...  I don't like it, but it's OK.

Correct.  Following the coding style of the project is not the
matter for your liking or not liking.  It is part of being on the
same codebase with other participant of this project.

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

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > 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,...
> 
> What do you mean by that, exactly?  As long as your "don't help
> much" is "would not hurt and will help some even for a small subset
> of audience", that would be OK, but I am puzzled.

I mean if purpose of these was for me to review the changes you did, it doesn't
help as much as an interdiff does.

> My reading of your responses to bug reports and the cover letter of
> the series has been that these were real fixes to a real problem
> without downsides, and that you consider that they are good changes
> to have in the upcoming release.
> 
> I am hoping that you did not mean that we shouldn't merge it to the
> 'next' and 'master' branches, but if that is what you meant, can we
> hear what the downsides of the series are?
> Are they more churn than they are worth without solving the real
> issue?  Do they regress for some repositories/workflows while
> improving for others?  I didn't get such an impression.

That's not what I meant. The patches are good.

> > ..., I did and interdiff with my own fixes and this is the result:
> > ...  I don't like it, but it's OK.
> 
> Correct.  Following the coding style of the project is not the
> matter for your liking or not liking.  It is part of being on the
> same codebase with other participant of this project.

There's nothing in Documentation/CodingGuidelines that says multiple piped
commands should be one on each line even if togther the line doesn't exceed 80
characters, nor does it says that file names should be between quotes, even if
the file name can't possibly have spaces.

These are just your arbitrary rules, not the guidelines of the project.

And BTW, the fact that something is a rule doesn't mean it's good; rules change.

-- 
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Junio C Hamano wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> > 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,...
>> 
>> What do you mean by that, exactly?  As long as your "don't help
>> much" is "would not hurt and will help some even for a small subset
>> of audience", that would be OK, but I am puzzled.
>
> I mean if purpose of these was for me to review the changes you did, it doesn't
> help as much as an interdiff does.

Ah, I misunderstood.  The thing was, that the original was based on
older codebase and I couldn't apply them to my tree cleanly.  It
would be unfair for you to expect _me_ to give you an interdiff in
such a situation, especially when I am asking you to double check
the conflict resolution based on that original.

OK, so I'll advance the topic to the 'next' and hopefully we can
merge it to 'master' before rc1.

> There's nothing in Documentation/CodingGuidelines that says multiple piped
> commands should be one on each line even if togther the line doesn't exceed 80
> characters, nor does it says that file names should be between quotes, even if
> the file name can't possibly have spaces.

Heh, we don't spell out every possible rules. That is where "do as
surrounding code" comes in.

As to "$1" vs $1 without quotes, I had to check the calling sites of
clean_mark and cmp_marks, primarily because these functions do not
say what their "$1" is, to see if an unquoted form is safe.  The
next person who reads the script has to do the same and quoting is
an obvious way to avoid it.

If the functions said "$1 is a branch name", it would have been
clear that unquoted $1 would be OK, but still, what these functions
has to take (especially the "clean" one that takes pathnames) can
change in the future, and a quoted form is an obvious way to
futureproof and relieve readers and programmers from having to worry
about the quoting issues.

So I think it is better to quote these in this particular case.

The pipe is purely subjective readability. I can go either way, but
since I was the one who was applying the patch that needed other
changes anyway... ;-)

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

[Felipe Contreras]

Junio C Hamano wrote:
> So I think it is better to quote these in this particular case.
> 
> The pipe is purely subjective readability. I can go either way, but
> since I was the one who was applying the patch that needed other
> changes anyway... ;-)

You started by saying these were "not the matter for your liking or not
liking", and now you are explaining why you like one better than the other.

I don't think it's particularly helpful to criticize one person's preferences
because they don't match yours.

You like one, I like another. That's it.

-- 
Felipe Contreras