On Mon, Apr 1, 2013 at 11:19 PM, Felipe Contreras
<felipe.contreras@xxxxxxxxx> wrote:
> On Mon, Apr 1, 2013 at 11:01 PM, Jeff King <peff@xxxxxxxx> 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.
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.
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html