[PATCH v4] transport-helper: report errors properly

If a push fails because the remote-helper died (with fast-export), the
user won't see any error message. So let's add one.

At the same time lets add tests to ensure this error is reported, and
while we are at it, check the error from fast-import

Suggested-by: Jeff King <redacted>
Signed-off-by: Felipe Contreras <redacted>
---
 git-remote-testgit        | 13 +++++++++++++
 t/t5801-remote-helpers.sh | 21 +++++++++++++++++++++
 transport-helper.c        |  2 +-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/git-remote-testgit b/git-remote-testgit
index b395c8d..2eb7889 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit

@@ -61,12 +61,25 @@ do
 			echo "feature import-marks=$gitmarks"
 			echo "feature export-marks=$gitmarks"
 		fi
+
+		if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+		then
+			echo "feature done"
+			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
+			sleep 1 # don't let fast-export get SIGPIPE
+			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..2dfcf64 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh

@@ -166,4 +166,25 @@ 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 2> error &&
+	cat error &&
+	grep -q "Error while running fast-import" error
+	)
+'
+
+# We sleep to give fast-export a chance to catch the SIGPIPE
+test_expect_success 'proper failure checks for pushing' '
+	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
+	export GIT_REMOTE_TESTGIT_FAILURE &&
+	cd local &&
+	test_must_fail git push --all 2> error &&
+	cat error &&
+	grep -q "Reading from remote helper failed" error
+	)
+'
+
 test_done

diff --git a/transport-helper.c b/transport-helper.c
index cb3ef7d..96081cc 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -54,7 +54,7 @@ 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);
+		die("Reading from remote helper failed");
 	}
 
 	if (debug)

-- 
1.8.2

On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras
[off-list ref] wrote:

+               die("Reading from remote helper failed");

Does the user know what a remote helper is? Could we point them at
some helpful docs in case they don't?

--
Cheers,

Sverre Rabbelier

On Mon, Apr 08, 2013 at 09:40:04AM -0500, Felipe Contreras wrote:

If a push fails because the remote-helper died (with fast-export), the
user won't see any error message. So let's add one.

At the same time lets add tests to ensure this error is reported, and
while we are at it, check the error from fast-import

Thanks, I think this patch is definitely the right direction.

It seems like there is a lot of back-story that had to be clarified
during the review/discussion. Is there a reason not to summarize it here
so later readers of this commit are enlightened?

I'm thinking something like:

  If a push fails because the remote-helper died (with fast-export), the
  user does not see any error message. We do correctly die with a failed
  exit code, as we notice that the helper has died while reading back
  the ref status from the helper. However, we don't print any message.
  This is OK if the helper itself printed a useful error message, but we
  cannot count on that; let's let the user know that the helper failed.

  In the long run, it may make more sense to propagate the error back up
  to push, so that it can present the usual status table and give a
  nicer message. But this is a much simpler fix that can help
  immediately.

  While we're adding tests, let's also confirm that the remote-helper
  dying is also detect when importing refs. We currently do so robustly
  when the helper uses the "done" feature (and that is what we test). We
  cannot do so reliably when the helper does not use the "done" feature,
  but it is not even worth testing; the right solution is for the helper
  to start using "done".

 	export)
+		if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+		then
+			sleep 1 # don't let fast-export get SIGPIPE
+			exit 1
+		fi

We can do away with this sleep with:

  while read line; do
          test "$line" = "done" && break
  done

The version I posted yesterday had both the read and the sleep, but the
sleep was only necessary there to demonstrate the race with
check_command.

+# We sleep to give fast-export a chance to catch the SIGPIPE
+test_expect_success 'proper failure checks for pushing' '

I think we can drop this comment now, right?

-Peff

On Mon, Apr 08, 2013 at 11:20:15AM -0700, Sverre Rabbelier wrote:

On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras
[off-list ref] wrote:

quoted

+               die("Reading from remote helper failed");

Does the user know what a remote helper is? Could we point them at
some helpful docs in case they don't?

That's a good point. I wonder if it would be enough to say:

  fatal: Reading from helper git-remote-X failed

That might make it more clear what the helper's role is, and showing the
command name gives the user a starting point for running "man".

-Peff

I think this topic is close to being done, so I just wanted to move it
along.

  [1/2]: transport-helper: report errors properly

    This is Felipe's v4 patch with the adjustments I suggested in
    review. It explains more in the commit message, and should fix
    Thomas's valgrind failures (it consumes fast-export's data before
    dying rather than sleeping and hoping that fast-export is done
    writing).

  [2/2]: transport-helper: mention helper name when it dies

    This changes the error message, to help with the issue raised by
    Sverre.

-Peff

From: Felipe Contreras <redacted>

If a push fails because the remote-helper died (with
fast-export), the user does not see any error message. We do
correctly die with a failed exit code, as we notice that the
helper has died while reading back the ref status from the
helper. However, we don't print any message.  This is OK if
the helper itself printed a useful error message, but we
cannot count on that; let's let the user know that the
helper failed.

In the long run, it may make more sense to propagate the
error back up to push, so that it can present the usual
status table and give a nicer message. But this is a much
simpler fix that can help immediately.

While we're adding tests, let's also confirm that the
remote-helper dying is also detect when importing refs. We
currently do so robustly when the helper uses the "done"
feature (and that is what we test).  We cannot do so
reliably when the helper does not use the "done" feature,
but it is not even worth testing; the right solution is for
the helper to start using "done".

Suggested-by: Jeff King <redacted>
Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Jeff King <redacted>
---
Felipe,

Can you acknowledge that it's OK to stick your name on this, as it's not
exactly what you submitted before?

 git-remote-testgit        | 19 +++++++++++++++++++
 t/t5801-remote-helpers.sh | 20 ++++++++++++++++++++
 transport-helper.c        |  2 +-
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/git-remote-testgit b/git-remote-testgit
index b395c8d..5fd09f9 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit

@@ -61,12 +61,31 @@ do
 			echo "feature import-marks=$gitmarks"
 			echo "feature export-marks=$gitmarks"
 		fi
+
+		if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+		then
+			echo "feature done"
+			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
+			# consume input so fast-export doesn't get SIGPIPE;
+			# git would also notice that case, but we want
+			# to make sure we are exercising the later
+			# error checks
+			while read line; do
+				test "done" = "$line" && break
+			done
+			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..aafc46a 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh

@@ -166,4 +166,24 @@ 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 2> error &&
+	cat error &&
+	grep -q "Error while running fast-import" error
+	)
+'
+
+test_expect_success 'proper failure checks for pushing' '
+	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
+	export GIT_REMOTE_TESTGIT_FAILURE &&
+	cd local &&
+	test_must_fail git push --all 2> error &&
+	cat error &&
+	grep -q "Reading from remote helper failed" error
+	)
+'
+
 test_done

diff --git a/transport-helper.c b/transport-helper.c
index cb3ef7d..96081cc 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -54,7 +54,7 @@ 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);
+		die("Reading from remote helper failed");
 	}
 
 	if (debug)

-- 
1.8.2.rc0.33.gd915649

When we try to read from a remote-helper and get EOF or an
error, we print a message indicating that the helper died.
However, users may not know that a remote helper was in use
(e.g., when using git-over-http), or even what a remote
helper is.

Let's print the name of the helper (e.g., "git-remote-https");
this makes it more obvious what the program is for, and
provides a useful token for reporting bugs or searching for
more information (e.g., in manpages).

Signed-off-by: Jeff King <redacted>
---
 t/t5801-remote-helpers.sh | 2 +-
 transport-helper.c        | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index aafc46a..8b2cb68 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh

@@ -182,7 +182,7 @@ test_expect_success 'proper failure checks for pushing' '
 	cd local &&
 	test_must_fail git push --all 2> error &&
 	cat error &&
-	grep -q "Reading from remote helper failed" error
+	grep -q "Reading from helper .git-remote-testgit. failed" error
 	)
 '
 

diff --git a/transport-helper.c b/transport-helper.c
index 96081cc..3fc43b9 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -46,7 +46,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, const char *name)
 {
 	strbuf_reset(buffer);
 	if (debug)

@@ -54,7 +54,7 @@ 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");
-		die("Reading from remote helper failed");
+		die("Reading from helper 'git-remote-%s' failed", name);
 	}
 
 	if (debug)

@@ -64,7 +64,7 @@ static int recvline(struct helper_data *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, helper->name);
 }
 
 static void xchgline(struct helper_data *helper, struct strbuf *buffer)

@@ -536,7 +536,7 @@ static int process_connect_service(struct transport *transport,
 		goto exit;
 
 	sendline(data, &cmdbuf);
-	recvline_fh(input, &cmdbuf);
+	recvline_fh(input, &cmdbuf, name);
 	if (!strcmp(cmdbuf.buf, "")) {
 		data->no_disconnect_req = 1;
 		if (debug)

-- 
1.8.2.rc0.33.gd915649

On Wed, Apr 10, 2013 at 2:15 PM, Jeff King [off-list ref] wrote:

From: Felipe Contreras <redacted>

If a push fails because the remote-helper died (with
fast-export), the user does not see any error message. We do
correctly die with a failed exit code, as we notice that the
helper has died while reading back the ref status from the
helper. However, we don't print any message.  This is OK if
the helper itself printed a useful error message, but we
cannot count on that; let's let the user know that the
helper failed.

In the long run, it may make more sense to propagate the
error back up to push, so that it can present the usual
status table and give a nicer message. But this is a much
simpler fix that can help immediately.

While we're adding tests, let's also confirm that the
remote-helper dying is also detect when importing refs. We
currently do so robustly when the helper uses the "done"
feature (and that is what we test).  We cannot do so
reliably when the helper does not use the "done" feature,
but it is not even worth testing; the right solution is for
the helper to start using "done".

Suggested-by: Jeff King <redacted>
Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Jeff King <redacted>

The fixes you made to this patch make a lot of sense, glad to not have
a 'sleep 1' in our tests.

Acked-by: Sverre Rabbelier <redacted>

--
Cheers,

Sverre Rabbelier

On Wed, Apr 10, 2013 at 2:16 PM, Jeff King [off-list ref] wrote:

When we try to read from a remote-helper and get EOF or an
error, we print a message indicating that the helper died.
However, users may not know that a remote helper was in use
(e.g., when using git-over-http), or even what a remote
helper is.

Let's print the name of the helper (e.g., "git-remote-https");
this makes it more obvious what the program is for, and
provides a useful token for reporting bugs or searching for
more information (e.g., in manpages).

Signed-off-by: Jeff King <redacted>

Better than nothing:

Acked-by: Sverre Rabbelier <redacted>

--
Cheers,

Sverre Rabbelier

On Wed, Apr 10, 2013 at 02:23:56PM -0700, Sverre Rabbelier wrote:

On Wed, Apr 10, 2013 at 2:16 PM, Jeff King [off-list ref] wrote:

quoted

When we try to read from a remote-helper and get EOF or an
error, we print a message indicating that the helper died.
However, users may not know that a remote helper was in use
(e.g., when using git-over-http), or even what a remote
helper is.

Let's print the name of the helper (e.g., "git-remote-https");
this makes it more obvious what the program is for, and
provides a useful token for reporting bugs or searching for
more information (e.g., in manpages).

Signed-off-by: Jeff King <redacted>

Better than nothing:

Acked-by: Sverre Rabbelier <redacted>

Now that's the kind of whole-hearted endorsement I strive for. :)

If you have better wording, I'm open to it. I do note that we don't
actually have a manpage for "git-remote-https", though we do for others.
Probably "man git-remote-helpers" is the most sensible thing to point
the user to. But I don't even think this is worthy of a big advice
message. It's a bug in the helper, it shouldn't really happen, and
giving the user a token they can use to report or google for the error
is probably good enough.

-Peff

On Wed, Apr 10, 2013 at 2:28 PM, Jeff King [off-list ref] wrote:

Now that's the kind of whole-hearted endorsement I strive for. :)

It's nothing wrong with your patch, the main problem is that there's
not really a good place to point users at.

If you have better wording, I'm open to it. I do note that we don't
actually have a manpage for "git-remote-https", though we do for others.
Probably "man git-remote-helpers" is the most sensible thing to point
the user to. But I don't even think this is worthy of a big advice
message. It's a bug in the helper, it shouldn't really happen, and
giving the user a token they can use to report or google for the error
is probably good enough.

Yeah, exactly. man git-remote-helpers is more a place for developers
to read how to implement a git-remote-helper, not so much a place for
users to read what they are, and/or how to use them.

--
Cheers,

Sverre Rabbelier

On Wed, Apr 10, 2013 at 5:15 PM, Jeff King [off-list ref] wrote:

From: Felipe Contreras <redacted>

If a push fails because the remote-helper died (with
fast-export), the user does not see any error message. We do
correctly die with a failed exit code, as we notice that the
helper has died while reading back the ref status from the
helper. However, we don't print any message.  This is OK if
the helper itself printed a useful error message, but we
cannot count on that; let's let the user know that the
helper failed.

In the long run, it may make more sense to propagate the
error back up to push, so that it can present the usual
status table and give a nicer message. But this is a much
simpler fix that can help immediately.

While we're adding tests, let's also confirm that the
remote-helper dying is also detect when importing refs. We

s/detect/detected/

currently do so robustly when the helper uses the "done"
feature (and that is what we test).  We cannot do so
reliably when the helper does not use the "done" feature,
but it is not even worth testing; the right solution is for
the helper to start using "done".

Suggested-by: Jeff King <redacted>
Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Jeff King <redacted>

On Wed, Apr 10, 2013 at 04:13:20PM -0700, rh wrote:

quoted

+++ b/transport-helper.c

@@ -54,7 +54,7 @@ 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);
+		die("Reading from remote helper failed");

Do I read this correctly?  If I'm in debug mode the remote helper quit
but if not in debug mode it failed?  Debuggers never fail they only quit!

In debug mode, it prints both messages. The debug version is superfluous
at this point, though, and we can probably just drop it.

-Peff

On Wed, Apr 10, 2013 at 4:15 PM, Jeff King [off-list ref] wrote:

From: Felipe Contreras <redacted>

If a push fails because the remote-helper died (with
fast-export), the user does not see any error message. We do
correctly die with a failed exit code, as we notice that the
helper has died while reading back the ref status from the
helper. However, we don't print any message.  This is OK if
the helper itself printed a useful error message, but we
cannot count on that; let's let the user know that the
helper failed.

This explained the same thing:

If a push fails because the remote-helper died (with fast-export), the user won't see any error message. So let's add one.

Granted, depending on the way the remote-helper died, an error might
or might not been printed, so s/won't/might not/.

The fact that an exit code was returned before is not relevant,
neither is how the exit was returned, and for that matter neither is
all the other things that are happening in this code. It's just noise.

The only thing that is relevant is this:

-               exit(128);
+               die("Reading from remote helper failed");

It's a simple change, and simple to explain.

In the long run, it may make more sense to propagate the
error back up to push, so that it can present the usual
status table and give a nicer message. But this is a much
simpler fix that can help immediately.

Yes it might, and it might make sense to rewrite much of this code,
but that's not relevant.

While we're adding tests, let's also confirm that the
remote-helper dying is also detect when importing refs.

That is enough explanation.

We
currently do so robustly when the helper uses the "done"
feature (and that is what we test).  We cannot do so
reliably when the helper does not use the "done" feature,
but it is not even worth testing; the right solution is for
the helper to start using "done".

This doesn't help anyone, and it's not even accurate. I think it might
be possible enforce remote-helpers to implement the "done" feature,
and we might want to do that later. But of course, discussing what bad
things remote-helpers could do, and how we should test and babysit
them is not relevant here.

If it was important to explain the subtleties and reasoning behind
this change, it should be a separate patch.

Suggested-by: Jeff King <redacted>
Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Jeff King <redacted>

I would add:

[jk: rewrote every piece of text]

        export)
+               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+               then
+                       # consume input so fast-export doesn't get SIGPIPE;

I think this is explanation enough.

+                       # git would also notice that case, but we want
+                       # to make sure we are exercising the later
+                       # error checks

I don't understand what is being said here. What is "that case"?

+                       while read line; do
+                               test "done" = "$line" && break
+                       done
+                       exit

LGTM.

Cheers.

--
Felipe Contreras

On Thu, Apr 11, 2013 at 08:22:26AM -0500, Felipe Contreras wrote:

quoted

We
currently do so robustly when the helper uses the "done"
feature (and that is what we test).  We cannot do so
reliably when the helper does not use the "done" feature,
but it is not even worth testing; the right solution is for
the helper to start using "done".

This doesn't help anyone, and it's not even accurate. I think it might
be possible enforce remote-helpers to implement the "done" feature,
and we might want to do that later. But of course, discussing what bad
things remote-helpers could do, and how we should test and babysit
them is not relevant here.

If it was important to explain the subtleties and reasoning behind
this change, it should be a separate patch.

I am OK with adding the test for import as a separate patch. What I am
not OK with (and this goes for the rest of the commit message, too) is
failing to explain any back-story at all for why the change is done in
the way it is.

_You_ may understand it _right now_, but that is not the primary
audience of the message. The primary audience is somebody else a year
from now who is wondering why this patch was done the way it was. When
they are trying to find out why git does not detect errors in a helper,
and they notice that our test for failure only check the "done" case,
isn't it more helpful to say "we considered the other case, but it was
not worth fixing" rather than leaving them to guess?

I may be more verbose than necessary in some of my commit messages, but
I would much rather err on the side of explaining too much than too
little.

quoted

        export)
+               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+               then
+                       # consume input so fast-export doesn't get SIGPIPE;

I think this is explanation enough.

quoted

+                       # git would also notice that case, but we want
+                       # to make sure we are exercising the later
+                       # error checks

I don't understand what is being said here. What is "that case"?

The case that fast-export gets SIGPIPE. I was trying to explain not
just _what_ we are doing, but _why_ it is important. Perhaps a better
wording would be:

  # consume input so fast-export doesn't get SIGPIPE;
  # we do not technically need to do so in order for
  # git to notice the failure to export, as it will
  # detect problems either with fast-export or with
  # the helper failing to report ref status. But since
  # we are trying to demonstrate that the latter
  # check works, we must avoid the SIGPIPE, which would
  # trigger the former.

-Peff

On Thu, Apr 11, 2013 at 11:18 AM, Jeff King [off-list ref] wrote:

On Thu, Apr 11, 2013 at 08:22:26AM -0500, Felipe Contreras wrote:

quoted

quoted

We
currently do so robustly when the helper uses the "done"
feature (and that is what we test).  We cannot do so
reliably when the helper does not use the "done" feature,
but it is not even worth testing; the right solution is for
the helper to start using "done".

This doesn't help anyone, and it's not even accurate. I think it might
be possible enforce remote-helpers to implement the "done" feature,
and we might want to do that later. But of course, discussing what bad
things remote-helpers could do, and how we should test and babysit
them is not relevant here.

If it was important to explain the subtleties and reasoning behind
this change, it should be a separate patch.

I am OK with adding the test for import as a separate patch. What I am
not OK with (and this goes for the rest of the commit message, too) is
failing to explain any back-story at all for why the change is done in
the way it is.

_You_ may understand it _right now_, but that is not the primary
audience of the message. The primary audience is somebody else a year
from now who is wondering why this patch was done the way it was.

Who would be this person? Somebody who wonders why this test is using
"feature done"? I doubt such a person would exist, as using this
feature is standard, as can be seen below this chunk. *If* the test
was *not* using this "feature done", *then* sure, an explanation would
be needed.

But why is this test doing something expected is not a question
anybody would benefit from asking.

When
they are trying to find out why git does not detect errors in a helper,
and they notice that our test for failure only check the "done" case,
isn't it more helpful to say "we considered the other case, but it was
not worth fixing" rather than leaving them to guess?

If you are worried about such hypothetical people, they would be
better served by a comment in the source code of the test, or even
better, the c file, or even better, to document that remote helpers
should use this feature. But wait:

---
Just like 'push', a batch sequence of one or more 'import' is
terminated with a blank line. For each batch of 'import', the remote
helper should produce a fast-import stream terminated by a 'done'
command.
---

So it's already explained, if somebody fails to follow this
documentation, it's dubious a commit message that introduces a test
would help. Surely, the writer of this bad remote helper would _never_
look there.

I may be more verbose than necessary in some of my commit messages, but
I would much rather err on the side of explaining too much than too
little.

I wouldn't. The only thing an overload of information achieves is that
the reader would simply skip or skim it.

quoted

quoted

        export)
+               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+               then
+                       # consume input so fast-export doesn't get SIGPIPE;

I think this is explanation enough.

quoted

+                       # git would also notice that case, but we want
+                       # to make sure we are exercising the later
+                       # error checks

I don't understand what is being said here. What is "that case"?

The case that fast-export gets SIGPIPE.

If we are trying to avoid SIGPIPE wouldn't that imply that git notices
the SIGPIPE?

  # consume input so fast-export doesn't get SIGPIPE;
  # we do not technically need to do so in order for
  # git to notice the failure to export, as it will
  # detect problems either with fast-export or with
  # the helper failing to report ref status. But since
  # we are trying to demonstrate that the latter
  # check works, we must avoid the SIGPIPE, which would
  # trigger the former.

# consume input so fast-export doesn't get SIGPIPE; we want to test
the remote-helper's code after fast-export.

--
Felipe Contreras

On Thu, Apr 11, 2013 at 11:49:11AM -0500, Felipe Contreras wrote:

quoted

I am OK with adding the test for import as a separate patch. What I am
not OK with (and this goes for the rest of the commit message, too) is
failing to explain any back-story at all for why the change is done in
the way it is.

_You_ may understand it _right now_, but that is not the primary
audience of the message. The primary audience is somebody else a year
from now who is wondering why this patch was done the way it was.

Who would be this person? Somebody who wonders why this test is using
"feature done"? I doubt such a person would exist, as using this
feature is standard, as can be seen below this chunk. *If* the test
was *not* using this "feature done", *then* sure, an explanation would
be needed.

If it was so obvious, why did your initial patch not use "feature done"?
If it was so obvious, why did our email discussion go back and forth so
many times before arriving at this patch?

It was certainly not obvious to me when this email thread started. So in
response to your question: *I* am that person. I was him two weeks ago,
and there is a good chance that I will be him a year from now. Much of
my work on git is spent tracking down bugs in older code, and those
commit messages are extremely valuable to me in understanding what
happened at the time.

But I give up on you. I find most of your commit messages lacking in
details and motivation, making assumptions that the reader is as
familiar with the code when reading the commit as you are when you wrote
it. I tried to help by suggesting in review that you elaborate. That
didn't work. So I tried to help by writing the text myself. But clearly
I am not going to convince you that it is valuable, even if it requires
no work at all from you, so I have nothing else to say on the matter.

-Peff

On Thu, Apr 11, 2013 at 11:59 AM, Jeff King [off-list ref] wrote:

On Thu, Apr 11, 2013 at 11:49:11AM -0500, Felipe Contreras wrote:

quoted

quoted

I am OK with adding the test for import as a separate patch. What I am
not OK with (and this goes for the rest of the commit message, too) is
failing to explain any back-story at all for why the change is done in
the way it is.

_You_ may understand it _right now_, but that is not the primary
audience of the message. The primary audience is somebody else a year
from now who is wondering why this patch was done the way it was.

Who would be this person? Somebody who wonders why this test is using
"feature done"? I doubt such a person would exist, as using this
feature is standard, as can be seen below this chunk. *If* the test
was *not* using this "feature done", *then* sure, an explanation would
be needed.

If it was so obvious, why did your initial patch not use "feature done"?

Because I didn't want to test the obvious, I wanted to test something else.

If it was so obvious, why did our email discussion go back and forth so
many times before arriving at this patch?

This patch has absolutely nothing to do with that, in fact, forget
about it, such a minor check is not worth this time and effort:
http://article.gmane.org/gmane.comp.version-control.git/220899

It was certainly not obvious to me when this email thread started. So in
response to your question: *I* am that person. I was him two weeks ago,
and there is a good chance that I will be him a year from now.

No, you are not. I didn't send a patch with "feature done" originally,
the only reason you wondered about the patch with "feature done" is
that you saw one without it. It will _never_ happen again.

Much of
my work on git is spent tracking down bugs in older code, and those
commit messages are extremely valuable to me in understanding what
happened at the time.

Lets make a bet. Let's push the simpler version, and when you hit this
commit message retrospectively and find that you don't understand what
is happening, I loose, and I will forever accept verbose commit
messages. It will never happen.

But I give up on you. I find most of your commit messages lacking in
details and motivation, making assumptions that the reader is as
familiar with the code when reading the commit as you are when you wrote
it. I tried to help by suggesting in review that you elaborate. That
didn't work. So I tried to help by writing the text myself. But clearly
I am not going to convince you that it is valuable, even if it requires
no work at all from you, so I have nothing else to say on the matter.

Me neither. I picked your solution, but that's not enough, you
*always* want me to do EXACTLY what you want, and never argue back.

It's not going to happen. There's nothing wrong with disagreeing.

Cheers.

--
Felipe Contreras

Felipe Contreras [off-list ref] writes:

On Wed, Apr 10, 2013 at 4:15 PM, Jeff King [off-list ref] wrote:

quoted

From: Felipe Contreras <redacted>

If a push fails because the remote-helper died (with
fast-export), the user does not see any error message. We do

I agree with you that s/does not see/may not see/ would be more
helpful here, so I'll squash it in while queuing.

quoted

In the long run, it may make more sense to propagate the
error back up to push, so that it can present the usual
status table and give a nicer message. But this is a much
simpler fix that can help immediately.

Yes it might, and it might make sense to rewrite much of this code,
but that's not relevant.

It is a good reminder for people who later inspect this part of the
code and wonder if it was a conscious design choice not to propagate
the error or just being "simple and sufficient for now", I think.
It would help them by making it clear that it is the latter, no?

... I think it might
be possible enforce remote-helpers to implement the "done" feature,
and we might want to do that later.

Yes, all these are possible and I think writing it down explicitly
will serve as a reminder for our future selves, I think.

quoted

+               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+               then
+                       # consume input so fast-export doesn't get SIGPIPE;

I think this is explanation enough.

quoted

+                       # git would also notice that case, but we want
+                       # to make sure we are exercising the later
+                       # error checks

I don't understand what is being said here. What is "that case"?

In my first reading, it felt to me that it was natural to interpret
that this is "even if we didn't have this loop that avoids killing
fast-export with SIGPIPE, we would notice death of fast-export by
SIGPIPE".

On Thu, Apr 11, 2013 at 1:44 PM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

quoted

In the long run, it may make more sense to propagate the
error back up to push, so that it can present the usual
status table and give a nicer message. But this is a much
simpler fix that can help immediately.

Yes it might, and it might make sense to rewrite much of this code,
but that's not relevant.

It is a good reminder for people who later inspect this part of the
code and wonder if it was a conscious design choice not to propagate
the error or just being "simple and sufficient for now", I think.
It would help them by making it clear that it is the latter, no?

No. Design choices is what code comments are for, of which Git only
has too few, according to ohloh[1]. No wonder they are so few, people
are spending time writing novels on commit messages and forgetting
there's also code where you should clarify things.

quoted

... I think it might
be possible enforce remote-helpers to implement the "done" feature,
and we might want to do that later.

Yes, all these are possible and I think writing it down explicitly
will serve as a reminder for our future selves, I think.

Yes, but not writing them here. By spending so much time in commit
messages you neglect the code, and the wiki (which is actually the
place to write these things on.

And if all you want is to write them down, we already did, right here.
There's no need to punish the readers of the commit messages in the
future only so we can flex our memory, because we already did.

And if you must, you might was well label them with "REMINDER", no,
wait, that's what "TODO" comments are for, where people can see them,
and not *forget* them.

Cheers.

[1] https://www.ohloh.net/p/git/factoids#FactoidCommentsLow

--
Felipe Contreras

On Mon,  8 Apr 2013 09:40:04 -0500
Felipe Contreras [off-list ref] wrote:

If a push fails because the remote-helper died (with fast-export), the
user won't see any error message. So let's add one.

At the same time lets add tests to ensure this error is reported, and
while we are at it, check the error from fast-import

....

Do I read this correctly?  If I'm in debug mode the remote helper quit
but if not in debug mode it failed?  Debuggers never fail they only quit!

 	}
 
 	if (debug)