Re: [PATCH 2/4] transport-helper: check if remote helper is alive

On Mon, Apr 1, 2013 at 11:19 PM, Felipe Contreras
[off-list ref] wrote:

On Mon, Apr 1, 2013 at 11:01 PM, Jeff King [off-list ref] wrote:

quoted

On Mon, Apr 01, 2013 at 10:51:20PM -0600, Felipe Contreras wrote:

quoted

quoted

So in fetch_with_import, we have a remote-helper, and we have a
bidirectional pipe to it. We then call get_importer, which starts
fast-import, whose stdin is connected to the stdout of the remote
helper. We tell the remote-helper to run the import, then we wait for
fast-import to finish (and complain if it fails).

Then what? We seem to do some more work, which I think is what causes
the errors you see; but should we instead be reaping the helper at this
point unconditionally? Its stdout has presumably been flushed out to
fast-import; is there anything else for us to get from it besides its
exit code?

The problem is not with import, since fast-import would generally wait
properly for a 'done' status, the problem is with export.

Your patch modified fetch_with_import. Are you saying that it isn't
necessary to do so?

It's not, I added it for symmetry. But that's the case *if* the
remote-helper is properly using the "done" feature.

Actually, it is a problem, because without this check the
transport-helper just goes on without realizing the whole thing has
failed and doesn't produce a proper error message:

fatal: bad object 0000000000000000000000000000000000000000
error: testgit::/home/felipec/dev/git/t/trash
directory.t5801-remote-helpers/server did not send all necessary
objects

It's possible to send a ping command to the remote-helper, but doing
so triggers a SIGPIPE. I would rather show a proper error message as
my patch suggests by just checking if the command is running.

quoted

quoted

Also, the design is such that the remote-helper stays alive, even
after fast-export has finished.

So if we expect to be able to communicate with the remote-helper after
fast-export has exited, is it a protocol failure that the helper does
not say "yes, I finished the export" or similar? If so, can we fix that?

I am not too familiar with this protocol, but it looks like we read from
helper->out right after closing the exporter, to get the ref statuses.
Shouldn't we be detecting the error if the helper hangs up there?

I guess that should be possible, I'll give that a try.

I gave this a try and it does work, but it seems rather convoluted to me:

diff --git a/transport-helper.c b/transport-helper.c
index f0d28aa..10b7842 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -25,7 +25,8 @@ struct helper_data {
 		option : 1,
 		push : 1,
 		connect : 1,
-		no_disconnect_req : 1;
+		no_disconnect_req : 1,
+		done_export : 1;
 	char *export_marks;
 	char *import_marks;
 	/* These go from remote name (as in "list") to private name */

@@ -46,7 +47,7 @@ static void sendline(struct helper_data *helper,

struct strbuf *buffer)
 		die_errno("Full write to remote helper failed");
 }

-static int recvline_fh(FILE *helper, struct strbuf *buffer)
+static int recvline_fh(FILE *helper, struct strbuf *buffer, int safe)
 {
 	strbuf_reset(buffer);
 	if (debug)

@@ -54,7 +55,10 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)
 	if (strbuf_getline(buffer, helper, '\n') == EOF) {
 		if (debug)
 			fprintf(stderr, "Debug: Remote helper quit.\n");
-		exit(128);
+		if (safe)
+			return -1;
+		else
+			exit(128);
 	}

 	if (debug)

@@ -64,7 +68,12 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)

 static int recvline(struct helper_data *helper, struct strbuf *buffer)
 {
-	return recvline_fh(helper->out, buffer);
+	return recvline_fh(helper->out, buffer, 0);
+}
+
+static int recvline_safe(struct helper_data *helper, struct strbuf *buffer)
+{
+	return recvline_fh(helper->out, buffer, 1);
 }

 static void xchgline(struct helper_data *helper, struct strbuf *buffer)

@@ -201,6 +210,8 @@ static struct child_process *get_helper(struct

transport *transport)
 			strbuf_addstr(&arg, "--import-marks=");
 			strbuf_addstr(&arg, capname + strlen("import-marks "));
 			data->import_marks = strbuf_detach(&arg, NULL);
+		} else if (!strcmp(capname, "done-export")) {
+			data->done_export = 1;
 		} else if (mandatory) {
 			die("Unknown mandatory capability %s. This remote "
 			    "helper probably needs newer version of Git.",

@@ -540,7 +551,7 @@ static int process_connect_service(struct

transport *transport,
 		goto exit;

 	sendline(data, &cmdbuf);
-	recvline_fh(input, &cmdbuf);
+	recvline_fh(input, &cmdbuf, 0);
 	if (!strcmp(cmdbuf.buf, "")) {
 		data->no_disconnect_req = 1;
 		if (debug)

@@ -704,19 +715,30 @@ static void push_update_ref_status(struct strbuf *buf,
 	(*ref)->remote_status = msg;
 }

-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 done = 0;
+
 	for (;;) {
-		recvline(data, &buf);
-		if (!buf.len)
+		if (recvline_safe(data, &buf) || !buf.len)
+			break;
+
+		if (!strncmp(buf.buf, "done", 4)) {
+			done = 1;
 			break;
+		}

 		push_update_ref_status(&buf, &ref, remote_refs);
 	}
 	strbuf_release(&buf);
+
+	if (!data->done_export)
+		return 0;
+
+	return !done;
 }

 static int push_refs_with_push(struct transport *transport,

@@ -776,8 +798,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,

@@ -829,8 +850,7 @@ static int push_refs_with_export(struct transport

*transport,
 	if (!check_command(data->helper))
 		die("Error while running helper");

-	push_update_refs_status(data, remote_refs);
-	return 0;
+	return push_update_refs_status(data, remote_refs);
 }

 static int push_refs(struct transport *transport,

-- 
Felipe Contreras