From: Jeff King <peff@peff.net>
To: Ramkumar Ramachandra <artagnon@gmail.com>
Cc: Git List <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [BUG] clone: regression in error messages in master
Date: Fri, 21 Jun 2013 03:05:39 -0400 [thread overview]
Message-ID: <20130621070539.GA31552@sigill.intra.peff.net> (raw)
In-Reply-To: <CALkWK0k8-go979S9sMytBpqs_C9Jm6q6thqwHa4-GX-e5Czx=g@mail.gmail.com>
On Fri, Jun 21, 2013 at 12:14:33PM +0530, Ramkumar Ramachandra wrote:
> Jeff King wrote:
> > So I'm not sure if there is a better option than reverting 81d340d4 and
> > living with the lesser of two evils (no good message when the helper
> > dies silently).
>
> I dug around, but I still can't justify that there is no better
> option. Could you write a commit message for this?
I think it is something like this:
-- >8 --
Subject: [PATCH] transport-helper: be quiet on read errors from helpers
Prior to commit 81d340d4, we did not print any error message
if a remote transport helper died unexpectedly. If a helper
did not print any error message (e.g., because it crashed),
the user could be left confused. That commit tried to
rectify the situation by printing a note that the helper
exited unexpectedly.
However, this makes a much more common case worse: when a
helper does die with a useful message, we print the extra
"Reading from 'git-remote-foo failed" message. This can also
end up confusing users, as they may not even know what
remote helpers are (e.g., the fact that http support comes
through git-remote-https is purely an implementation detail
that most users do not know or care about).
Since we do not have a good way of knowing whether the
helper printed a useful error, and since the common failure
mode is for it to do so, let's default to remaining quiet.
Debuggers can dig further by setting GIT_TRANSPORT_HELPER_DEBUG.
Signed-off-by: Jeff King <peff@peff.net>
---
Note that I haven't thought too hard about this; there may be a way to
detect for specific operations that we were expecting more data from the
helper and didn't get it. But even if we do want to go that route, I
think reverting the change to recvline_fh is probably going to be the
first step.
t/t5801-remote-helpers.sh | 4 +---
transport-helper.c | 2 +-
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 4899af3..8c4c539 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -210,9 +210,7 @@ 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 helper .git-remote-testgit. failed" error
+ test_must_fail git push --all
)
'
diff --git a/transport-helper.c b/transport-helper.c
index 06c08a1..db9bd18 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -56,7 +56,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)
if (strbuf_getline(buffer, helper, '\n') == EOF) {
if (debug)
fprintf(stderr, "Debug: Remote helper quit.\n");
- die("Reading from helper 'git-remote-%s' failed", name);
+ exit(128);
}
if (debug)
--
1.8.3.rc2.14.g7eee6b3
next prev parent reply other threads:[~2013-06-21 7:05 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-20 13:16 [BUG] clone: regression in error messages in master Ramkumar Ramachandra
2013-06-20 13:34 ` Jeff King
2013-06-21 6:44 ` Ramkumar Ramachandra
2013-06-21 6:46 ` Ramkumar Ramachandra
2013-06-21 7:05 ` Jeff King [this message]
2013-06-21 16:03 ` Junio C Hamano
2013-06-21 10:07 ` John Szakmeister
2013-06-21 17:11 ` Junio C Hamano
2013-06-22 9:55 ` John Szakmeister
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130621070539.GA31552@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=artagnon@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).