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

2013-04-02 Thread Felipe Contreras 
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 
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, )) {

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

2013-04-01 Thread Felipe Contreras 
Otherwise transport-helper will continue checking for refs and other
things what will confuse the user more.
---
 git-remote-testgit| 11 +++
 t/t5801-remote-helpers.sh | 19 +++
 transport-helper.c|  8 
 3 files changed, 38 insertions(+)

diff --git a/git-remote-testgit b/git-remote-testgit
index b395c8d..ca0cf09 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -61,12 +61,23 @@ do
echo feature import-marks=$gitmarks
echo feature export-marks=$gitmarks
fi
+
+   if test -n $GIT_REMOTE_TESTGIT_FAILURE
+   then
+   exit -1
+   fi
+
echo feature done
git fast-export ${testgitmarks_args[@]} $refs |
sed -e s#refs/heads/#${prefix}/heads/#g
echo done
;;
export)
+   if test -n $GIT_REMOTE_TESTGIT_FAILURE
+   then
+   exit -1
+   fi
+
before=$(git for-each-ref --format='%(refname) %(objectname)')
 
git fast-import ${testgitmarks_args[@]} --quiet
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index f387027..26e9a5b 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' '
compare_refs local dup server dup
 '
 
+test_expect_success 'proper failure checks for fetching' '
+   (GIT_REMOTE_TESTGIT_FAILURE=1 
+   export GIT_REMOTE_TESTGIT_FAILURE 
+   cd local 
+   test_must_fail git fetch 21 | \
+   grep Error while running helper
+   )
+'
+
+# We sleep to give fast-export a chance to catch the SIGPIPE
+test_expect_failure 'proper failure checks for pushing' '
+   (GIT_REMOTE_TESTGIT_FAILURE=1 
+   export GIT_REMOTE_TESTGIT_FAILURE 
+   cd local 
+   test_must_fail git push --all 21 | \
+   grep Error while running helper
+   )
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index cb3ef7d..dfdfa7a 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport,
 
if (finish_command(fastimport))
die(Error while running fast-import);
+
+   if (!check_command(data-helper))
+   die(Error while running helper);
+
argv_array_free_detached(fastimport.argv);
 
/*
@@ -818,6 +822,10 @@ static int push_refs_with_export(struct transport 
*transport,
 
if (finish_command(exporter))
die(Error while running fast-export);
+
+   if (!check_command(data-helper))
+   die(Error while running helper);
+
push_update_refs_status(data, remote_refs);
return 0;
 }
-- 
1.8.2

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

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

2013-04-01 Thread Jeff King 
On Mon, Apr 01, 2013 at 03:46:42PM -0600, Felipe Contreras wrote:

 Otherwise transport-helper will continue checking for refs and other
 things what will confuse the user more.
 [...]
 diff --git a/transport-helper.c b/transport-helper.c
 index cb3ef7d..dfdfa7a 100644
 --- a/transport-helper.c
 +++ b/transport-helper.c
 @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport,
  
   if (finish_command(fastimport))
   die(Error while running fast-import);
 +
 + if (!check_command(data-helper))
 + die(Error while running helper);
 +
   argv_array_free_detached(fastimport.argv);

Can you be more specific about what happens when we miss the death here,
what happens next, etc?

Checking asynchronously for death like this is subject to a race
condition; the helper may be about to die but not have died yet. In
practice we may catch some cases, but this seems like an indication that
the protocol is not well thought-out. Usually we would wait for a
confirmation over the read pipe from a child, and know that the child
failed when either:

  1. It tells us so on the pipe.

  2. The pipe closes (at which point we know it is time to reap the
 child).

Why doesn't that scheme work here? I am not doubting you that it does
not; the import helper protocol is a bit of a mess, and I can easily
believe it has such a problem. But I'm wondering if it's possible to
improve it in a more robust way.

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

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

2013-04-01 Thread Felipe Contreras 
On Mon, Apr 1, 2013 at 5:33 PM, Jeff King p...@peff.net wrote:
 On Mon, Apr 01, 2013 at 03:46:42PM -0600, Felipe Contreras wrote:

 Otherwise transport-helper will continue checking for refs and other
 things what will confuse the user more.
 [...]
 diff --git a/transport-helper.c b/transport-helper.c
 index cb3ef7d..dfdfa7a 100644
 --- a/transport-helper.c
 +++ b/transport-helper.c
 @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport 
 *transport,

   if (finish_command(fastimport))
   die(Error while running fast-import);
 +
 + if (!check_command(data-helper))
 + die(Error while running helper);
 +
   argv_array_free_detached(fastimport.argv);

 Can you be more specific about what happens when we miss the death here,
 what happens next, etc?

I have seen problems sporadically, like git trying to update refs to
object that don't exist. I also remember seeing mismatches between the
marks and the remote branches refs.

 Checking asynchronously for death like this is subject to a rac
 condition; the helper may be about to die but not have died yet. In
 practice we may catch some cases, but this seems like an indication that
 the protocol is not well thought-out. Usually we would wait for a
 confirmation over the read pipe from a child, and know that the child
 failed when either:

   1. It tells us so on the pipe.

   2. The pipe closes (at which point we know it is time to reap the
  child).

 Why doesn't that scheme work here? I am not doubting you that it does
 not; the import helper protocol is a bit of a mess, and I can easily
 believe it has such a problem. But I'm wondering if it's possible to
 improve it in a more robust way.

The pipe is between fast-export and the remote-helper, we
(transport-helper) are not part of the pipe any more. That's the
problem.

Cheers.

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

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

2013-04-01 Thread Junio C Hamano 
Felipe Contreras felipe.contre...@gmail.com writes:

 Otherwise transport-helper will continue checking for refs and other
 things what will confuse the user more.
 ---

Sign-off?

  git-remote-testgit| 11 +++
  t/t5801-remote-helpers.sh | 19 +++
  transport-helper.c|  8 
  3 files changed, 38 insertions(+)

 diff --git a/git-remote-testgit b/git-remote-testgit
 index b395c8d..ca0cf09 100755
 --- a/git-remote-testgit
 +++ b/git-remote-testgit
 @@ -61,12 +61,23 @@ do
   echo feature import-marks=$gitmarks
   echo feature export-marks=$gitmarks
   fi
 +
 + if test -n $GIT_REMOTE_TESTGIT_FAILURE
 + then
 + exit -1
 + fi
 +
   echo feature done
   git fast-export ${testgitmarks_args[@]} $refs |
   sed -e s#refs/heads/#${prefix}/heads/#g
   echo done
   ;;
   export)
 + if test -n $GIT_REMOTE_TESTGIT_FAILURE
 + then
 + exit -1
 + fi
 +
   before=$(git for-each-ref --format='%(refname) %(objectname)')
  
   git fast-import ${testgitmarks_args[@]} --quiet
 diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
 index f387027..26e9a5b 100755
 --- a/t/t5801-remote-helpers.sh
 +++ b/t/t5801-remote-helpers.sh
 @@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' '
   compare_refs local dup server dup
  '
  
 +test_expect_success 'proper failure checks for fetching' '
 + (GIT_REMOTE_TESTGIT_FAILURE=1 
 + export GIT_REMOTE_TESTGIT_FAILURE 
 + cd local 
 + test_must_fail git fetch 21 | \
 + grep Error while running helper

This will not care if git fetch succeeds or fails and returns the
exit code from grep.  Perhaps something like this instead?

(
GIT_REMOTE_TESTGIT_FAILURE=1 
export GIT_REMOTE_TESTGIT_FAILURE 
cd local 
test_must_fail git fetch 2error 
grep Error while running helper error
)


 +# We sleep to give fast-export a chance to catch the SIGPIPE
 +test_expect_failure 'proper failure checks for pushing' '
 + (GIT_REMOTE_TESTGIT_FAILURE=1 
 + export GIT_REMOTE_TESTGIT_FAILURE 
 + cd local 
 + test_must_fail git push --all 21 | \
 + grep Error while running helper

Ditto.

 + )
 +'
 +
  test_done
 diff --git a/transport-helper.c b/transport-helper.c
 index cb3ef7d..dfdfa7a 100644
 --- a/transport-helper.c
 +++ b/transport-helper.c
 @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport,
  
   if (finish_command(fastimport))
   die(Error while running fast-import);
 +
 + if (!check_command(data-helper))
 + die(Error while running helper);
 +
   argv_array_free_detached(fastimport.argv);
  
   /*
 @@ -818,6 +822,10 @@ static int push_refs_with_export(struct transport 
 *transport,
  
   if (finish_command(exporter))
   die(Error while running fast-export);
 +
 + if (!check_command(data-helper))
 + die(Error while running helper);
 +
   push_update_refs_status(data, remote_refs);
   return 0;
  }

OK, so the idea is that fetch_with_import() does

 - get_helper(transport), which spawns a helper process;

 - get_importer(transport, fastimport), which spawns a fast-import
   and make it read from the output of the helper process;

 - we did finish_command() to wait for the fast-import to finish,
   expecting that the fast-import would finish when the helper stops
   feeding it, which in turn would mean the helper would have died.

The same for the pushing side.

Shouldn't transport_disconnect() have called release_helper() which
in turn calls disconnect_helper() to call finish_command() on the
helper to wait for that procesanyway?  Is somebody discarding return
value from transport_disconnect() or the current calling site of
transport_disconnect() is too late to notice the error?

Puzzled...


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

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

2013-04-01 Thread Jeff King 
On Mon, Apr 01, 2013 at 06:12:45PM -0600, Felipe Contreras wrote:

  Checking asynchronously for death like this is subject to a rac
  condition; the helper may be about to die but not have died yet. In
  practice we may catch some cases, but this seems like an indication that
  the protocol is not well thought-out. Usually we would wait for a
  confirmation over the read pipe from a child, and know that the child
  failed when either:
 
1. It tells us so on the pipe.
 
2. The pipe closes (at which point we know it is time to reap the
   child).
 
  Why doesn't that scheme work here? I am not doubting you that it does
  not; the import helper protocol is a bit of a mess, and I can easily
  believe it has such a problem. But I'm wondering if it's possible to
  improve it in a more robust way.
 
 The pipe is between fast-export and the remote-helper, we
 (transport-helper) are not part of the pipe any more. That's the
 problem.

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?

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

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

2013-04-01 Thread Felipe Contreras 
On Mon, Apr 1, 2013 at 8:30 PM, Jeff King p...@peff.net wrote:
 On Mon, Apr 01, 2013 at 06:12:45PM -0600, Felipe Contreras wrote:

  Checking asynchronously for death like this is subject to a rac
  condition; the helper may be about to die but not have died yet. In
  practice we may catch some cases, but this seems like an indication that
  the protocol is not well thought-out. Usually we would wait for a
  confirmation over the read pipe from a child, and know that the child
  failed when either:
 
1. It tells us so on the pipe.
 
2. The pipe closes (at which point we know it is time to reap the
   child).
 
  Why doesn't that scheme work here? I am not doubting you that it does
  not; the import helper protocol is a bit of a mess, and I can easily
  believe it has such a problem. But I'm wondering if it's possible to
  improve it in a more robust way.

 The pipe is between fast-export and the remote-helper, we
 (transport-helper) are not part of the pipe any more. That's the
 problem.

 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. Also, the
design is such that the remote-helper stays alive, even after
fast-export has finished.

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

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

2013-04-01 Thread Felipe Contreras 
On Mon, Apr 1, 2013 at 6:26 PM, Junio C Hamano gits...@pobox.com wrote:

 OK, so the idea is that fetch_with_import() does

  - get_helper(transport), which spawns a helper process;

  - get_importer(transport, fastimport), which spawns a fast-import
and make it read from the output of the helper process;

  - we did finish_command() to wait for the fast-import to finish,
expecting that the fast-import would finish when the helper stops
feeding it, which in turn would mean the helper would have died.

 The same for the pushing side.

The difference with the pushing side is that it's the helper the one
waiting for fast-export and it can easily die.

 Shouldn't transport_disconnect() have called release_helper() which
 in turn calls disconnect_helper() to call finish_command() on the
 helper to wait for that procesanyway?  Is somebody discarding return
 value from transport_disconnect() or the current calling site of
 transport_disconnect() is too late to notice the error?

It's too late to notice the error. However, only in the case of pushing.

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

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

2013-04-01 Thread Jeff King 
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?

 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?

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

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

2013-04-01 Thread Felipe Contreras 
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.

 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.

Cheers.

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