[PATCH v4] transport-helper: report errors properly

[PATCH v4] transport-helper: report errors properly

[Felipe Contreras]

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 <peff@peff.net>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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

Re: [PATCH v4] transport-helper: report errors properly

[Sverre Rabbelier]

On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras
<felipe.contreras@gmail.com> 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

Re: [PATCH v4] transport-helper: report errors properly

[Jeff King]

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

> On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras
> <felipe.contreras@gmail.com> 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?

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

Re: [PATCH v4] transport-helper: report errors properly

[Jeff King]

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

Re: [PATCH v4] transport-helper: report errors properly

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> 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
[...]
> +# 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
> +	)
> +'

There appears to be a race in the version that is in today's pu
(5eb25f737b).  I reproduced with this:

  cd git/t
  i=1
  while ./t5801-remote-helpers.sh --root=/dev/shm --valgrind
  do
    i=$(($i+1))
  done

Two out of six of these loops quit within 1 and 2 iterations,
respectively, both with an error along the lines of:

  expecting success: 
          (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
          )

  error: fast-export died of signal 13
  fatal: Error while running fast-export
  not ok 21 - proper failure checks for pushing

I haven't been able to reproduce outside of valgrind tests.  Is this an
expected issue, caused by overrunning the sleep somehow?  If so, can you
increase the sleep delay under valgrind so as to not cause intermittent
failures in the test suite?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v4] transport-helper: report errors properly

[Jeff King]

On Tue, Apr 09, 2013 at 11:38:05PM +0200, Thomas Rast wrote:

> Two out of six of these loops quit within 1 and 2 iterations,
> respectively, both with an error along the lines of:
> 
>   expecting success: 
>           (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
>           )
> 
>   error: fast-export died of signal 13
>   fatal: Error while running fast-export
>   not ok 21 - proper failure checks for pushing
> 
> I haven't been able to reproduce outside of valgrind tests.  Is this an
> expected issue, caused by overrunning the sleep somehow?  If so, can you
> increase the sleep delay under valgrind so as to not cause intermittent
> failures in the test suite?

Yeah, I am not too surprised. The failing helper sleeps before exiting
so that fast-export puts all of its data into the pipe buffer before the
helper dies, and does not get SIGPIPE. But obviously the sleep is just
delaying the problem if your fast-export runs really slowly (which, if
you are running under valgrind, is a possibility).

The helper should instead just consume all of fast-export's input before
exiting, which accomplishes the same thing, finishes sooner in the
normal case, and doesn't race. And I think it also simulates a
reasonable real-world setup (a helper reads and converts the data, but
then dies while writing the output to disk, the network, or whatever).

I posted review comments, including that, and I'm assuming that Felipe
is going to re-roll at some point.

-Peff

[PATCH v5 0/2] reporting transport helper errors

[Jeff King]

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

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

[Jeff King]

From: Felipe Contreras <felipe.contreras@gmail.com>

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 <peff@peff.net>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
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

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

[Sverre Rabbelier]

On Wed, Apr 10, 2013 at 2:15 PM, Jeff King <peff@peff.net> wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> 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 <peff@peff.net>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>

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 <srabbelier@gmail.com>

--
Cheers,

Sverre Rabbelier

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

[Eric Sunshine]

On Wed, Apr 10, 2013 at 5:15 PM, Jeff King <peff@peff.net> wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> 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 <peff@peff.net>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>

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

[Felipe Contreras]

On Wed, Apr 10, 2013 at 4:15 PM, Jeff King <peff@peff.net> wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> 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 <peff@peff.net>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>

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

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

[Jeff King]

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

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

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

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

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

[Felipe Contreras]

On Thu, Apr 11, 2013 at 11:18 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 11, 2013 at 08:22:26AM -0500, Felipe Contreras wrote:
>
>> > 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.

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

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

[Jeff King]

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

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

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

[Felipe Contreras]

On Thu, Apr 11, 2013 at 11:59 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 11, 2013 at 11:49:11AM -0500, Felipe Contreras wrote:
>
>> > 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

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, Apr 11, 2013 at 11:59 AM, Jeff King <peff@peff.net> wrote:
>
>> 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.

Heh, it seems that I was late for the party.

Writing only minimally sufficient in the log messages is fine for
your own project. We won't decide nor dictate the policy for your
project for you.

But _this_ project wants its log messages to be understandable by
people who you may disagree with and who may have shorter memory
span than you do.  Disagreeing with that policy is fine.  You need
to learn to disagree but accept to be part of the project.

Thanks.

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

[Felipe Contreras]

On Thu, Apr 11, 2013 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Apr 11, 2013 at 11:59 AM, Jeff King <peff@peff.net> wrote:
>>
>>> 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.
>
> Heh, it seems that I was late for the party.
>
> Writing only minimally sufficient in the log messages is fine for
> your own project. We won't decide nor dictate the policy for your
> project for you.
>
> But _this_ project wants its log messages to be understandable by
> people who you may disagree with and who may have shorter memory
> span than you do.

Having a shorter memory span is irrelevant when you are _never_ going
to go back and ask the question the commit message is answering. And
if it indeed is an important question, the answer belongs in the code
comments.

> Disagreeing with that policy is fine.  You need
> to learn to disagree but accept to be part of the project.

Yeah, I accept that you will commit whatever you want, but I still
don't think this verbosity serves the purpose you think it serves.
Some one-liners deserve pages of commit messages, but this one is not
one of them. People are easily deceived, and because you saw one
commit message that needed more information, you think all of them do,
but no, some don't don't, and this is one of them. It's not serving
any real purpose.

--
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Wed, Apr 10, 2013 at 4:15 PM, Jeff King <peff@peff.net> wrote:
>> From: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> 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.

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

>> +               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"?

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

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

[Felipe Contreras]

On Thu, Apr 11, 2013 at 1:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:

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

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

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

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

Yeah, good point.

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

[Felipe Contreras]

On Thu, Apr 11, 2013 at 6:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> 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.
>
> Yeah, good point.

Moreover, I think there's a clear double standard. Consider this commit:

commit 99d3206010ba1fcc9311cbe8376c0b5e78f4a136
Author: Antoine Pelisse <apelisse@gmail.com>
Date:   Sat Mar 23 18:23:28 2013 +0100

    combine-diff: coalesce lost lines optimally

    This replaces the greedy implementation to coalesce lost lines by using
    dynamic programming to find the Longest Common Subsequence.

    The O(n²) time complexity is obviously bigger than previous
    implementation but it can produce shorter diff results (and most likely
    easier to read).

    List of lost lines is now doubly-linked because we reverse-read it when
    reading the direction matrix.

The commit message is 9 lines, and the diffstat 320 insertions(+), 64
deletions(-). Moreover, there are some important bits of information
on the mailing list that never made it to the commit message:

---
Best-case analysis:
All p parents have the same n lines.
We will find LCS and provide a n lines (the same lines) new list in
O(n²), and then run it again in O(n²) with the next parent, etc.
It will end-up being O(pn²).

Worst-case analysis:
All p parents have no lines in common.
We will find LCS and provide a 2n new list in O(n²).
Then we run it again in O(2n x n), and again O(3n x n), etc, until
O(pn x n).
When we sum these all, we end-up with O(p² x n²)
---

---
Unfortunately on a commit that would remove A LOT of lines (10000)
from 7 parents, the times goes from 0.01s to 1.5s... I'm pretty sure
that scenario is quite uncommon though.
---

This is not mentioned in the commit message; on which situations this
implementation would be worst and why it's OK either way.

---
As you can see the last test is broken because the solution is not
optimal for more than two parents. It would probably require to extend
the dynamic programming to a k-dimension matrix (for k parents) but the
result would end-up being O(n^k) (when removing n consecutives lines
from p parents). I'm not sure there is any better solution known yet to
the k-LCS problem.
Implementing the dynamic solution with the k-dimension matrix would
probably require to re-hash the strings (I guess it's already done by
xdiff), as the number of string comparisons would increase.
---

The fact that the last test is broken is not mentioned at all.

Now let's compare to the final version of my patch which is 19 lines
40 insertions(+), 1 deletion(-). The ration of commit message lines
vs. code changed lines is 19/41(0.46) whereas Antoine's patch is
3/128(0.02), a difference of over 19 times. Granted, some single-line
changes do require a good chunk of explanation, but this is not one of
them; this single line patch doesn't even change the behavior of the
code, simply changes a silent error exit to a verbose error exit,
that's all. Antoine's patch has a lot more potential to trigger
something unexpected.

And the chances that somebody would have to look at Antoine's patch is
quite high, especially since a failing test-case is introduced. The
chances that anybody would look at mine are very very low.

So either Antoine's commit message was fine, and so was mine, or it
was sorely lacking explanation.

To me, the reality is obvious: my patch didn't require such a big
commit message, the short version was fine, the only reason Jeff King
insisted on a longer version is because the patch came from me.
Antoine's patch might have benefited from a little more explanation,
but not every issue that was discussed in the mailing list was
necessary (in my patch virtually every issue discussed was added to
the commit message).

This is the definition of double standard.

Cheers.

-- 
Felipe Contreras

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

[Jeff King]

On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote:

> To me, the reality is obvious: my patch didn't require such a big
> commit message, the short version was fine, the only reason Jeff King
> insisted on a longer version is because the patch came from me.

Get over yourself. The reason I suggested a longer commit message for
your commit is because after spending several hours figuring out what
the current code did, and what it should be doing instead, I wanted to
document that effort so that I and other readers did not have to do it
again later. I didn't even review the other patch you mention, so I
could not possibly have come to the same point with it.

But hey, if you want to have paranoid fantasies that I'm persecuting you
(by writing the longer commit messages for you!), go ahead.

If you don't want me to review your patches, that's fine by me, too; our
discussions often end up frustrating, and it's clear we do not agree on
very much with respect to process or design. But if you don't want that,
please stop cc'ing me when you send out the patches.

-Peff

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

[Felipe Contreras]

On Sat, Apr 13, 2013 at 1:00 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote:
>
>> To me, the reality is obvious: my patch didn't require such a big
>> commit message, the short version was fine, the only reason Jeff King
>> insisted on a longer version is because the patch came from me.
>
> Get over yourself. The reason I suggested a longer commit message for
> your commit is because after spending several hours figuring out what
> the current code did, and what it should be doing instead, I wanted to
> document that effort so that I and other readers did not have to do it
> again later. I didn't even review the other patch you mention, so I
> could not possibly have come to the same point with it.

The double standard might not come from you, perhaps you subject all
the patches you review to the same standard, it comes from the fact
that the patches you review have an unfair disadvantage.

> But hey, if you want to have paranoid fantasies that I'm persecuting you
> (by writing the longer commit messages for you!), go ahead.

You don't persecute me, you persecute my patches. I could almost
picture the moment you see a patch is coming from me, you have already
decided to rewrite the commit message, even before reading it. Antoine
is not me, so you simply didn't review that patch.

> If you don't want me to review your patches, that's fine by me, too; our
> discussions often end up frustrating, and it's clear we do not agree on
> very much with respect to process or design. But if you don't want that,
> please stop cc'ing me when you send out the patches.

This comment was directed towards Junio, I do hope he is able to see
the double standard. As for you, I think your reviews have value, but
I also think you dwelling in irrelevant details do slow things down,
which is not too bad, what is bad is that you assume that your
opinions are facts (e.g. the commit message need to be bigger), and
get angry when somebody disagrees with them.

Cheers.

-- 
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Sat, Apr 13, 2013 at 1:00 AM, Jeff King <peff@peff.net> wrote:
>> On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote:
>>
>>> To me, the reality is obvious: my patch didn't require such a big
>>> commit message, the short version was fine, the only reason Jeff King
>>> insisted on a longer version is because the patch came from me.
>>
>> Get over yourself. The reason I suggested a longer commit message for
>> your commit is because after spending several hours figuring out what
>> the current code did, and what it should be doing instead, I wanted to
>> document that effort so that I and other readers did not have to do it
>> again later.
> ...
> The double standard might not come from you, perhaps you subject all
> the patches you review to the same standard, it comes from the fact
> that the patches you review have an unfair disadvantage.

There are reviewers who share the basic values [*1*] with I and the
tradition of this project and whose judgement I can trust.

When somebody (like Peff) whose judgement I trust spends time to
review a series, and writes his thought process to the degree that
he thinks is appropriate, that's his judgement and I trust it.

You cannot expect perfect evenness from multiple people.  For that
matter, you cannot expect perfect evenness even from a single
person, either.

When reviewing a proposed change to an area that I am intimately
familiar with, I may immediately know some subtleties involved in a
proposed solution without even reading the patch, and I may not even
realize that such subtleties are hard to know without being somebody
who are already familiar with that part of the codebase, and either
in-code comment or in the log message may better spelled them out.
On the other hand, when the change is in another area that I am not
familiar with, I may request more explanation, if only for me to
understand the issue.

I try to avoid the "I may know too well" pitfalls, but I am not
perfect. I will not speak for Peff or any other reviewers whose
jugement I trust, but I would be very surprised if any of them
claimed he is perfectly even.

"Double" may only be showing that we do not have enough trusted
maintainers; ideally I would like it to have "Triple" or more.


[Footnote]

*1* A few examples of core values.

 - we should make sure that future developers who wonder why a part
   of the code is how it is can find out what thought process
   brought the code into the current shape.

 - when we add something, we try not to overengineer or to shoot for
   unattainable perfection, but we still try to make sure we will
   not paint ourselves into an unescapable corner when we later want
   to extend it.

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

[Felipe Contreras]

On Sun, Apr 14, 2013 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:

> "Double" may only be showing that we do not have enough trusted
> maintainers; ideally I would like it to have "Triple" or more.

A double or triple review raises *a single* standard higher, but
having more than one standard is never good. But apparently your trust
in Jeff means more than caring about the double standard.

But good to know, my patches will always have an unfair disadvantages
to everybody else's.

>  - when we add something, we try not to overengineer or to shoot for
>    unattainable perfection, but we still try to make sure we will
>    not paint ourselves into an unescapable corner when we later want
>    to extend it.

And there is such a thing as being too cautious, to the point where
you walk WAY too slowly for the fear of tumbling, because it happened
a few times in the fast, when you were running. But how would you even
know that you are being too cautions? If you don't dare to walk a bit
faster to find out.

Cheers.

-- 
Felipe Contreras

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

[Jeff King]

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 <peff@peff.net>
---
 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

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

[Sverre Rabbelier]

On Wed, Apr 10, 2013 at 2:16 PM, Jeff King <peff@peff.net> 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 <peff@peff.net>

Better than nothing:

Acked-by: Sverre Rabbelier <srabbelier@gmail.com>

--
Cheers,

Sverre Rabbelier

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

[Jeff King]

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

> On Wed, Apr 10, 2013 at 2:16 PM, Jeff King <peff@peff.net> 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 <peff@peff.net>
> 
> Better than nothing:
> 
> Acked-by: Sverre Rabbelier <srabbelier@gmail.com>

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

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

[Sverre Rabbelier]

On Wed, Apr 10, 2013 at 2:28 PM, Jeff King <peff@peff.net> 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

Re: [PATCH v4] transport-helper: report errors properly

[rh]

On Mon,  8 Apr 2013 09:40:04 -0500
Felipe Contreras <felipe.contreras@gmail.com> 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
> 

....

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


>  	}
>  
>  	if (debug)

Re: [PATCH v4] transport-helper: report errors properly

[Jeff King]

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

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