On Mon, Apr 1, 2013 at 11:19 PM, Felipe Contreras
<felipe.contre...@gmail.com> wrote:
> On Mon, Apr 1, 2013 at 11:01 PM, Jeff King <p...@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.

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to