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

From

Felipe Contreras <felipe.contreras@gmail.com>

Date

Apr 2, 2013, 09:36 UTC

Message-ID

<CAMP44s2PT4YWBNJGTaYS5oy2jBEqkGFhwGjmZZc+HLsS8MO_xA@mail.gmail.com>

In-Reply-To

<CAMP44s3yNGfW4aoHB90_D-OFrzchgvAugzdmOYHOey3wYkUxjg@mail.gmail.com>

> On Mon, Apr 1, 2013 at 11:01 PM, Jeff King <peff@peff.net> wrote:
>> On Mon, Apr 01, 2013 at 10:51:20PM -0600, Felipe Contreras wrote:
>>
>>> > 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.

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