[PATCH] lessen the impression of unexpectedness on remote hangup

[PATCH] lessen the impression of unexpectedness on remote hangup

[Heiko Voigt]

If a server accessed through ssh is denying access git will currently
issue the message

	"fatal: The remote end hung up unexpectedly"

as the last line. This sounds as if something really ugly just happened.
Since this is a quite typical situation in which users regularly get
lets just say:

	"fatal: The remote end hung up"

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
We just got this bug report in msysgit

https://github.com/msysgit/msysgit/issues/28

and IIRC there have been more people questioning that line.

 pkt-line.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pkt-line.c b/pkt-line.c
index 5a04984..d2b8267 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -141,7 +141,7 @@ static void safe_read(int fd, void *buffer, unsigned size)
 	if (ret < 0)
 		die_errno("read error");
 	else if (ret < size)
-		die("The remote end hung up unexpectedly");
+		die("The remote end hung up");
 }
 
 static int packet_length(const char *linelen)
-- 
1.7.11.rc2.3.g15e800d

Re: [PATCH] lessen the impression of unexpectedness on remote hangup

[Jonathan Nieder]

Hi,

Heiko Voigt wrote:

> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -141,7 +141,7 @@ static void safe_read(int fd, void *buffer, unsigned size)
>  	if (ret < 0)
>  		die_errno("read error");
>  	else if (ret < size)
> -		die("The remote end hung up unexpectedly");
> +		die("The remote end hung up");

Looks good.  Incidentally, maybe this would be a good excuse to
revisit [1].

If I remember correctly, the conclusion of that discussion was that
--informative-errors is a good default for the git daemon as long as

 - there is some warning when using it without --base-path
 - there is an easy way to turn it off
 - we communicate the change clearly and in advance

Hope that helps,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/183604

Re: [PATCH] lessen the impression of unexpectedness on remote hangup

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> If a server accessed through ssh is denying access git will currently
> issue the message
>
> 	"fatal: The remote end hung up unexpectedly"
>
> as the last line. This sounds as if something really ugly just happened.
> Since this is a quite typical situation in which users regularly get
> lets just say:
>
> 	"fatal: The remote end hung up"
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---

This change makes sense, but you would need to adjust tests for the
change, perhaps at least these:

 t/t5512-ls-remote.sh    | 2 +-
 t/t5522-pull-symlink.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 5c546c9..a24eb42 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -104,7 +104,7 @@ test_expect_success 'use branch.<name>.remote if possible' '
 
 cat >exp <<EOF
 fatal: 'refs*master' does not appear to be a git repository
-fatal: The remote end hung up unexpectedly
+fatal: The remote end hung up
 EOF
 test_expect_success 'confuses pattern as remote when no remote specified' '
 	#
diff --git a/t/t5522-pull-symlink.sh b/t/t5522-pull-symlink.sh
index 8e9b204..f06ffb4 100755
--- a/t/t5522-pull-symlink.sh
+++ b/t/t5522-pull-symlink.sh
@@ -46,7 +46,7 @@ test_expect_success SYMLINKS 'pulling from real subdir' '
 # Instead, the error pull gave was:
 #
 #   fatal: 'origin': unable to chdir or not a git archive
-#   fatal: The remote end hung up unexpectedly
+#   fatal: The remote end hung up
 #
 # because git would find the .git/config for the "trash directory"
 # repo, not for the clone-repo repo.  The "trash directory" repo

Re: [PATCH] lessen the impression of unexpectedness on remote hangup

[Jeff King]

On Sun, Jun 10, 2012 at 08:23:10PM +0200, Heiko Voigt wrote:

> If a server accessed through ssh is denying access git will currently
> issue the message
> 
> 	"fatal: The remote end hung up unexpectedly"
> 
> as the last line. This sounds as if something really ugly just happened.
> Since this is a quite typical situation in which users regularly get
> lets just say:
> 
> 	"fatal: The remote end hung up"
> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
> We just got this bug report in msysgit
> 
> https://github.com/msysgit/msysgit/issues/28
> 
> and IIRC there have been more people questioning that line.

This does not seem like it would lessen the surprise all that much. I
wonder if we could use some context about where we are in the protocol
to tell more. For example, if the remote end hangs up before advertising
its refs, it is probably an authentication error or a missing
repository. And we should say that. If it happens during ref negotiation
or during the pack file, then it really is unexpected; the other end has
broken protocol, and it probably makes sense to say so.

-Peff

Re: [PATCH] lessen the impression of unexpectedness on remote hangup

[Heiko Voigt]

On Mon, Jun 11, 2012 at 03:02:07PM -0400, Jeff King wrote:
> On Sun, Jun 10, 2012 at 08:23:10PM +0200, Heiko Voigt wrote:
> 
> > If a server accessed through ssh is denying access git will currently
> > issue the message
> > 
> > 	"fatal: The remote end hung up unexpectedly"
> > 
> > as the last line. This sounds as if something really ugly just happened.
> > Since this is a quite typical situation in which users regularly get
> > lets just say:
> > 
> > 	"fatal: The remote end hung up"
> > 
> > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> > ---
> > We just got this bug report in msysgit
> > 
> > https://github.com/msysgit/msysgit/issues/28
> > 
> > and IIRC there have been more people questioning that line.
> 
> This does not seem like it would lessen the surprise all that much. I
> wonder if we could use some context about where we are in the protocol
> to tell more. For example, if the remote end hangs up before advertising
> its refs, it is probably an authentication error or a missing
> repository. And we should say that. If it happens during ref negotiation
> or during the pack file, then it really is unexpected; the other end has
> broken protocol, and it probably makes sense to say so.

I agree that would be better. I will have a look if I can cook something
up. In the meantime I think changing the message is better than
unnecessarily worrying the user.

Cheers Heiko

[PATCH] remove the impression of unexpectedness when access is denied

[Heiko Voigt]

If a server accessed through ssh is denying access git will currently
issue the message

	"fatal: The remote end hung up unexpectedly"

as the last line. This sounds as if something really ugly just happened.
Since this is a quite typical situation in which users regularly get
lets say:

	"fatal: Could not read remote heads"

If it happens at the beginning when reading the remote heads. Since
this is the first the client does it is very likely an authentication
error or a missing repository.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
On Mon, Jun 11, 2012 at 03:02:07PM -0400, Jeff King wrote:
> On Sun, Jun 10, 2012 at 08:23:10PM +0200, Heiko Voigt wrote:
> > We just got this bug report in msysgit
> > 
> > https://github.com/msysgit/msysgit/issues/28
> > 
> > and IIRC there have been more people questioning that line.
> 
> This does not seem like it would lessen the surprise all that much. I
> wonder if we could use some context about where we are in the protocol
> to tell more. For example, if the remote end hangs up before advertising
> its refs, it is probably an authentication error or a missing
> repository. And we should say that. If it happens during ref negotiation
> or during the pack file, then it really is unexpected; the other end has
> broken protocol, and it probably makes sense to say so.

This is the shortest I was currently able to come up with. Should we
add an explanation to the die like:

	This error very likely occurred because the remote repository
	path is wrong or access was denied.

This time I ran the testsuite and it passes with this patch.

Cheers Heiko


 builtin/archive.c        |  4 ++--
 builtin/fetch-pack.c     |  8 ++++----
 builtin/receive-pack.c   |  2 +-
 builtin/send-pack.c      |  4 ++--
 builtin/upload-archive.c |  2 +-
 connect.c                | 12 +++++++++++-
 daemon.c                 |  2 +-
 pkt-line.c               | 21 +++++++++++++++------
 pkt-line.h               |  2 +-
 remote-curl.c            |  6 +++---
 sideband.c               |  2 +-
 t/t5512-ls-remote.sh     |  2 +-
 upload-pack.c            |  4 ++--
 13 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index 931956d..2e4a784 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -53,7 +53,7 @@ static int run_remote_archiver(int argc, const char **argv,
 		packet_write(fd[1], "argument %s\n", argv[i]);
 	packet_flush(fd[1]);
 
-	len = packet_read_line(fd[0], buf, sizeof(buf));
+	len = packet_read_line(fd[0], buf, sizeof(buf), 0);
 	if (!len)
 		die(_("git archive: expected ACK/NAK, got EOF"));
 	if (buf[len-1] == '\n')
@@ -66,7 +66,7 @@ static int run_remote_archiver(int argc, const char **argv,
 		die(_("git archive: protocol error"));
 	}
 
-	len = packet_read_line(fd[0], buf, sizeof(buf));
+	len = packet_read_line(fd[0], buf, sizeof(buf), 0);
 	if (len)
 		die(_("git archive: expected a flush"));
 
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 149db88..4f0778d 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -179,7 +179,7 @@ static void consume_shallow_list(int fd)
 		 * is a block of have lines exchanged.
 		 */
 		char line[1000];
-		while (packet_read_line(fd, line, sizeof(line))) {
+		while (packet_read_line(fd, line, sizeof(line), 0)) {
 			if (!prefixcmp(line, "shallow "))
 				continue;
 			if (!prefixcmp(line, "unshallow "))
@@ -222,7 +222,7 @@ static int write_shallow_commits(struct strbuf *out, int use_pack_protocol)
 static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 {
 	static char line[1000];
-	int len = packet_read_line(fd, line, sizeof(line));
+	int len = packet_read_line(fd, line, sizeof(line), 0);
 
 	if (!len)
 		die("git fetch-pack: expected ACK/NAK, got EOF");
@@ -352,7 +352,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 		unsigned char sha1[20];
 
 		send_request(fd[1], &req_buf);
-		while (packet_read_line(fd[0], line, sizeof(line))) {
+		while (packet_read_line(fd[0], line, sizeof(line), 0)) {
 			if (!prefixcmp(line, "shallow ")) {
 				if (get_sha1_hex(line + 8, sha1))
 					die("invalid shallow line: %s", line);
@@ -990,7 +990,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			 */
 			static char line[1000];
 			for (;;) {
-				int n = packet_read_line(0, line, sizeof(line));
+				int n = packet_read_line(0, line, sizeof(line), 0);
 				if (!n)
 					break;
 				if (line[n-1] == '\n')
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0afb8b2..2267dac 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -743,7 +743,7 @@ static struct command *read_head_info(void)
 		char *refname;
 		int len, reflen;
 
-		len = packet_read_line(0, line, sizeof(line));
+		len = packet_read_line(0, line, sizeof(line), 0);
 		if (!len)
 			break;
 		if (line[len-1] == '\n')
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index d5d7105..7a8c21e 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -113,7 +113,7 @@ static int receive_status(int in, struct ref *refs)
 	struct ref *hint;
 	char line[1000];
 	int ret = 0;
-	int len = packet_read_line(in, line, sizeof(line));
+	int len = packet_read_line(in, line, sizeof(line), 0);
 	if (len < 10 || memcmp(line, "unpack ", 7))
 		return error("did not receive remote status");
 	if (memcmp(line, "unpack ok\n", 10)) {
@@ -127,7 +127,7 @@ static int receive_status(int in, struct ref *refs)
 	while (1) {
 		char *refname;
 		char *msg;
-		len = packet_read_line(in, line, sizeof(line));
+		len = packet_read_line(in, line, sizeof(line), 0);
 		if (!len)
 			break;
 		if (len < 3 ||
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index b928beb..e0e4d0c 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -40,7 +40,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 	sent_argv[0] = "git-upload-archive";
 	for (p = buf;;) {
 		/* This will die if not enough free space in buf */
-		len = packet_read_line(0, p, (buf + sizeof buf) - p);
+		len = packet_read_line(0, p, (buf + sizeof buf) - p, 0);
 		if (len == 0)
 			break;	/* got a flush */
 		if (sent_argc > MAX_ARGS - 2)
diff --git a/connect.c b/connect.c
index 912cdde..19e73d5 100644
--- a/connect.c
+++ b/connect.c
@@ -56,6 +56,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
 			      unsigned int flags,
 			      struct extra_have_objects *extra_have)
 {
+	int got_at_least_one_head = 0;
+
 	*list = NULL;
 	for (;;) {
 		struct ref *ref;
@@ -64,7 +66,14 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		char *name;
 		int len, name_len;
 
-		len = packet_read_line(in, buffer, sizeof(buffer));
+		len = packet_read_line(in, buffer, sizeof(buffer), 1);
+		if (len < 0) {
+			if (got_at_least_one_head)
+				die("The remote end hung up unexpectedly");
+			else
+				die("Could not read remote heads");
+		}
+
 		if (!len)
 			break;
 		if (buffer[len-1] == '\n')
@@ -95,6 +104,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		hashcpy(ref->old_sha1, old_sha1);
 		*list = ref;
 		list = &ref->next;
+		got_at_least_one_head = 1;
 	}
 	return list;
 }
diff --git a/daemon.c b/daemon.c
index ab21e66..a6bbfa4 100644
--- a/daemon.c
+++ b/daemon.c
@@ -539,7 +539,7 @@ static int execute(void)
 		loginfo("Connection from %s:%s", addr, port);
 
 	alarm(init_timeout ? init_timeout : timeout);
-	pktlen = packet_read_line(0, line, sizeof(line));
+	pktlen = packet_read_line(0, line, sizeof(line), 0);
 	alarm(0);
 
 	len = strlen(line);
diff --git a/pkt-line.c b/pkt-line.c
index 5a04984..533f1f6 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -135,13 +135,18 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 	strbuf_add(buf, buffer, n);
 }
 
-static void safe_read(int fd, void *buffer, unsigned size)
+static int safe_read(int fd, void *buffer, unsigned size, int return_read_fail)
 {
 	ssize_t ret = read_in_full(fd, buffer, size);
 	if (ret < 0)
 		die_errno("read error");
 	else if (ret < size)
-		die("The remote end hung up unexpectedly");
+		if (return_read_fail)
+			return -1;
+		else
+			die("The remote end hung up unexpectedly");
+
+	return ret;
 }
 
 static int packet_length(const char *linelen)
@@ -169,12 +174,14 @@ static int packet_length(const char *linelen)
 	return len;
 }
 
-int packet_read_line(int fd, char *buffer, unsigned size)
+int packet_read_line(int fd, char *buffer, unsigned size, int return_read_fail)
 {
-	int len;
+	int len, ret;
 	char linelen[4];
 
-	safe_read(fd, linelen, 4);
+	ret = safe_read(fd, linelen, 4, return_read_fail);
+	if (return_read_fail && ret < 0)
+		return ret;
 	len = packet_length(linelen);
 	if (len < 0)
 		die("protocol error: bad line length character: %.4s", linelen);
@@ -185,7 +192,9 @@ int packet_read_line(int fd, char *buffer, unsigned size)
 	len -= 4;
 	if (len >= size)
 		die("protocol error: bad line length %d", len);
-	safe_read(fd, buffer, len);
+	ret = safe_read(fd, buffer, len, return_read_fail);
+	if (return_read_fail && ret < 0)
+		return ret;
 	buffer[len] = 0;
 	packet_trace(buffer, len, 0);
 	return len;
diff --git a/pkt-line.h b/pkt-line.h
index 1e5dcfe..277dc9d 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -12,7 +12,7 @@ void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2
 void packet_buf_flush(struct strbuf *buf);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 
-int packet_read_line(int fd, char *buffer, unsigned size);
+int packet_read_line(int fd, char *buffer, unsigned size, int return_read_fail);
 int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
 ssize_t safe_write(int, const void *, ssize_t);
 
diff --git a/remote-curl.c b/remote-curl.c
index 04a9d62..40f6b2d 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -314,7 +314,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 
 	if (!avail) {
 		rpc->initial_buffer = 0;
-		avail = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+		avail = packet_read_line(rpc->out, rpc->buf, rpc->alloc, 0);
 		if (!avail)
 			return 0;
 		rpc->pos = 0;
@@ -429,7 +429,7 @@ static int post_rpc(struct rpc_state *rpc)
 			break;
 		}
 
-		n = packet_read_line(rpc->out, buf, left);
+		n = packet_read_line(rpc->out, buf, left, 0);
 		if (!n)
 			break;
 		rpc->len += n;
@@ -568,7 +568,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	rpc->hdr_accept = strbuf_detach(&buf, NULL);
 
 	while (!err) {
-		int n = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+		int n = packet_read_line(rpc->out, rpc->buf, rpc->alloc, 0);
 		if (!n)
 			break;
 		rpc->pos = 0;
diff --git a/sideband.c b/sideband.c
index d5ffa1c..1e86a6b 100644
--- a/sideband.c
+++ b/sideband.c
@@ -37,7 +37,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 
 	while (1) {
 		int band, len;
-		len = packet_read_line(in_stream, buf + pf, LARGE_PACKET_MAX);
+		len = packet_read_line(in_stream, buf + pf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 6764d51..7f165ec 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -87,7 +87,7 @@ test_expect_success 'use branch.<name>.remote if possible' '
 test_expect_success 'confuses pattern as remote when no remote specified' '
 	cat >exp <<-\EOF &&
 	fatal: '\''refs*master'\'' does not appear to be a git repository
-	fatal: The remote end hung up unexpectedly
+	fatal: Could not read remote heads
 	EOF
 	#
 	# Do not expect "git ls-remote <pattern>" to work; ls-remote, correctly,
diff --git a/upload-pack.c b/upload-pack.c
index bb08e2e..a690f63 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -426,7 +426,7 @@ static int get_common_commits(void)
 	save_commit_buffer = 0;
 
 	for (;;) {
-		int len = packet_read_line(0, line, sizeof(line));
+		int len = packet_read_line(0, line, sizeof(line), 0);
 		reset_timeout();
 
 		if (!len) {
@@ -587,7 +587,7 @@ static void receive_needs(void)
 		struct object *o;
 		const char *features;
 		unsigned char sha1_buf[20];
-		len = packet_read_line(0, line, sizeof(line));
+		len = packet_read_line(0, line, sizeof(line), 0);
 		reset_timeout();
 		if (!len)
 			break;
-- 
1.7.11.rc2.4.g4f85a3f

Re: [PATCH] remove the impression of unexpectedness when access is denied

[Heiko Voigt]

Hi,

On Thu, Jun 14, 2012 at 09:13:06AM +0200, Heiko Voigt wrote:
> This is the shortest I was currently able to come up with.

BTW, I could probably just add another function named e.g.

	packet_read_line_allow_shorter(...

and do an internal static function for handling this.

Cheers Heiko

Re: [PATCH] remove the impression of unexpectedness when access is denied

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> diff --git a/connect.c b/connect.c
> index 912cdde..19e73d5 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -56,6 +56,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
>  			      unsigned int flags,
>  			      struct extra_have_objects *extra_have)
>  {
> +	int got_at_least_one_head = 0;
> +
>  	*list = NULL;
>  	for (;;) {
>  		struct ref *ref;
> @@ -64,7 +66,14 @@ struct ref **get_remote_heads(int in, struct ref **list,
>  		char *name;
>  		int len, name_len;
>  
> -		len = packet_read_line(in, buffer, sizeof(buffer));
> +		len = packet_read_line(in, buffer, sizeof(buffer), 1);
> +		if (len < 0) {
> +			if (got_at_least_one_head)
> +				die("The remote end hung up unexpectedly");
> +			else
> +				die("Could not read remote heads");
> +		}

I do not think it is particularly interesting to know we have (or
haven't) read one packet before we got an error. It would be an
improvement if the message lets the user know at what stage of the
exchange the remote threw you a garbage, but using the same "The
remote end hung up unexpectedly" as all the other packet_read_line()
errors show makes it less useful.

How about getting rid of the new boolean variable and say

	len = packet_read(in, buffer, sizeof(buffer));
        if (len < 0)
		die("The remote end hung up upon initial contact");

or something?

>  		if (!len)
>  			break;
>  		if (buffer[len-1] == '\n')
> @@ -95,6 +104,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
>  		hashcpy(ref->old_sha1, old_sha1);
>  		*list = ref;
>  		list = &ref->next;
> +		got_at_least_one_head = 1;
>  	}
>  	return list;
>  }

It seems that all callers other than this one after this patch
behave identically as before like this patch. It would be far more
preferable to introduce a new function that does not die on errors
(including but not necessarily limited to short read situation you
are interested in this patch), and update this caller that wants to
handle these error cases to call that new function.  Perhaps

	len = packet_read(in, buffer, sizeof(buffer));

that returns negative error numbers when it sees an error, with

	#define PKTREAD_UNKNOWN_ERROR (-1)
        #define PKTREAD_SHORT_READ (-2)
        ...

and then over time we should consider converting remaining callers
of packet_read_line() to packet_read().

Re: [PATCH] remove the impression of unexpectedness when access is denied

[Heiko Voigt]

Hi,

On Thu, Jun 14, 2012 at 10:11:10AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > diff --git a/connect.c b/connect.c
> > index 912cdde..19e73d5 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -56,6 +56,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
> >  			      unsigned int flags,
> >  			      struct extra_have_objects *extra_have)
> >  {
> > +	int got_at_least_one_head = 0;
> > +
> >  	*list = NULL;
> >  	for (;;) {
> >  		struct ref *ref;
> > @@ -64,7 +66,14 @@ struct ref **get_remote_heads(int in, struct ref **list,
> >  		char *name;
> >  		int len, name_len;
> >  
> > -		len = packet_read_line(in, buffer, sizeof(buffer));
> > +		len = packet_read_line(in, buffer, sizeof(buffer), 1);
> > +		if (len < 0) {
> > +			if (got_at_least_one_head)
> > +				die("The remote end hung up unexpectedly");
> > +			else
> > +				die("Could not read remote heads");
> > +		}
> 
> I do not think it is particularly interesting to know we have (or
> haven't) read one packet before we got an error. It would be an
> improvement if the message lets the user know at what stage of the
> exchange the remote threw you a garbage, but using the same "The
> remote end hung up unexpectedly" as all the other packet_read_line()
> errors show makes it less useful.

Well I thought about the case of "access denied" or "no repository
here". I wanted to distinguish between this quite typical situation
where you did not get anything and the situation when you already got
something from the server. AFAIK its not so typical to hang up after you
got the first ref or is it?

So maybe something along the lines:

	if (got_at_least_one_head)
		die("The remote end hung up upon initial contact");
	else
		die("Could not read from remote repository.\n"
		    "\nPlease make sure you have the correct access"
		    "rights and the repository exists.");

to give the user some suggestion what might have gone wrong?

If I understand the loop correctly it reads one remote head per
iteration doesn't it?

> It seems that all callers other than this one after this patch
> behave identically as before like this patch. It would be far more
> preferable to introduce a new function that does not die on errors
> (including but not necessarily limited to short read situation you
> are interested in this patch), and update this caller that wants to
> handle these error cases to call that new function.  Perhaps
> 
> 	len = packet_read(in, buffer, sizeof(buffer));
> 
> that returns negative error numbers when it sees an error, with
> 
> 	#define PKTREAD_UNKNOWN_ERROR (-1)
>         #define PKTREAD_SHORT_READ (-2)
>         ...
> 
> and then over time we should consider converting remaining callers
> of packet_read_line() to packet_read().

Yes I agree thats what I realized to late after sending the patch. Will
implement that in the next iteration of my patch.

Cheers Heiko

[PATCH v2] remove the impression of unexpectedness when access is denied

[Heiko Voigt]

If a server accessed through ssh is denying access git will currently
issue the message

	"fatal: The remote end hung up unexpectedly"

as the last line. This sounds as if something really ugly just happened.
Since this is a quite typical situation in which users regularly get
we do not say that if it happens at the beginning when reading the
remote heads.

If its in the very first beginning of reading the remote heads it is
very likely an authentication error or a missing repository.

If it happens later during reading the remote heads we still indicate
that it happened during this initial contact phase.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 connect.c            | 18 +++++++++++++++++-
 pkt-line.c           | 32 ++++++++++++++++++++++++++------
 pkt-line.h           |  1 +
 t/t5512-ls-remote.sh |  5 ++++-
 4 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/connect.c b/connect.c
index 912cdde..3e19d67 100644
--- a/connect.c
+++ b/connect.c
@@ -49,6 +49,16 @@ static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1
 	extra->nr++;
 }
 
+static void die_initial_contact(int got_at_least_one_head)
+{
+	if (got_at_least_one_head)
+		die("The remote end hung up upon initial contact");
+	else
+		die("Could not read from remote repository.\n\n"
+		    "Please make sure you have the correct access rights\n"
+		    "and the repository exists.");
+}
+
 /*
  * Read all the refs from the other end
  */
@@ -56,6 +66,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
 			      unsigned int flags,
 			      struct extra_have_objects *extra_have)
 {
+	int got_at_least_one_head = 0;
+
 	*list = NULL;
 	for (;;) {
 		struct ref *ref;
@@ -64,7 +76,10 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		char *name;
 		int len, name_len;
 
-		len = packet_read_line(in, buffer, sizeof(buffer));
+		len = packet_read(in, buffer, sizeof(buffer));
+		if (len < 0)
+			die_initial_contact(got_at_least_one_head);
+
 		if (!len)
 			break;
 		if (buffer[len-1] == '\n')
@@ -95,6 +110,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		hashcpy(ref->old_sha1, old_sha1);
 		*list = ref;
 		list = &ref->next;
+		got_at_least_one_head = 1;
 	}
 	return list;
 }
diff --git a/pkt-line.c b/pkt-line.c
index 5a04984..eaba15f 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -135,13 +135,19 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 	strbuf_add(buf, buffer, n);
 }
 
-static void safe_read(int fd, void *buffer, unsigned size)
+static int safe_read(int fd, void *buffer, unsigned size, int return_line_fail)
 {
 	ssize_t ret = read_in_full(fd, buffer, size);
 	if (ret < 0)
 		die_errno("read error");
-	else if (ret < size)
+	else if (ret < size) {
+		if (return_line_fail)
+			return -1;
+
 		die("The remote end hung up unexpectedly");
+	}
+
+	return ret;
 }
 
 static int packet_length(const char *linelen)
@@ -169,12 +175,14 @@ static int packet_length(const char *linelen)
 	return len;
 }
 
-int packet_read_line(int fd, char *buffer, unsigned size)
+static int packet_read_internal(int fd, char *buffer, unsigned size, int return_line_fail)
 {
-	int len;
+	int len, ret;
 	char linelen[4];
 
-	safe_read(fd, linelen, 4);
+	ret = safe_read(fd, linelen, 4, return_line_fail);
+	if (return_line_fail && ret < 0)
+		return ret;
 	len = packet_length(linelen);
 	if (len < 0)
 		die("protocol error: bad line length character: %.4s", linelen);
@@ -185,12 +193,24 @@ int packet_read_line(int fd, char *buffer, unsigned size)
 	len -= 4;
 	if (len >= size)
 		die("protocol error: bad line length %d", len);
-	safe_read(fd, buffer, len);
+	ret = safe_read(fd, buffer, len, return_line_fail);
+	if (return_line_fail && ret < 0)
+		return ret;
 	buffer[len] = 0;
 	packet_trace(buffer, len, 0);
 	return len;
 }
 
+int packet_read(int fd, char *buffer, unsigned size)
+{
+	return packet_read_internal(fd, buffer, size, 1);
+}
+
+int packet_read_line(int fd, char *buffer, unsigned size)
+{
+	return packet_read_internal(fd, buffer, size, 0);
+}
+
 int packet_get_line(struct strbuf *out,
 	char **src_buf, size_t *src_len)
 {
diff --git a/pkt-line.h b/pkt-line.h
index 1e5dcfe..8cfeb0c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -13,6 +13,7 @@ void packet_buf_flush(struct strbuf *buf);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 
 int packet_read_line(int fd, char *buffer, unsigned size);
+int packet_read(int fd, char *buffer, unsigned size);
 int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
 ssize_t safe_write(int, const void *, ssize_t);
 
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 6764d51..2af5c2a 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -87,7 +87,10 @@ test_expect_success 'use branch.<name>.remote if possible' '
 test_expect_success 'confuses pattern as remote when no remote specified' '
 	cat >exp <<-\EOF &&
 	fatal: '\''refs*master'\'' does not appear to be a git repository
-	fatal: The remote end hung up unexpectedly
+	fatal: Could not read from remote repository.
+
+	Please make sure you have the correct access rights
+	and the repository exists.
 	EOF
 	#
 	# Do not expect "git ls-remote <pattern>" to work; ls-remote, correctly,
-- 
1.7.11.rc2.3.g49b071d

Re: [PATCH v2] remove the impression of unexpectedness when access is denied

[Jonathan Nieder]

Hi,

Heiko Voigt wrote:

> --- a/connect.c
> +++ b/connect.c
> @@ -49,6 +49,16 @@ static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1
>  	extra->nr++;
>  }
>  
> +static void die_initial_contact(int got_at_least_one_head)
> +{
> +	if (got_at_least_one_head)
> +		die("The remote end hung up upon initial contact");
> +	else
> +		die("Could not read from remote repository.\n\n"
> +		    "Please make sure you have the correct access rights\n"
> +		    "and the repository exists.");
> +}
[...]

I ran into this message for the first time today.

 $ git fetch --all
 Fetching origin
 remote: Counting objects: 368, done.
[...]
 Fetching gitk
 fatal: Could not read from remote repository.

 Please make sure you have the correct access rights
 and the repository exists.
 error: Could not fetch gitk
 Fetching debian
 Fetching pape
[...]

The "gitk" remote refers to git://git.kernel.org/pub/scm/gitk/gitk.
Using ls-remote to contact it produces the same result.  The message
is correct: the repository does not exist.

Impressions:

 * Looking at "Could not read", it is not clear what could not read
   and why.  GIT_TRACE_PACKET tells me the interaction was

	me> git-upload-pack /pub/scm/gitk/gitk\0host=git.kernel.org\0
	them> (hangup)

   Would it make sense for the server to send an "ERR" packet to give
   a more helpful diagnosis?

 * The spacing and capitalization is odd and makes it not flow well
   with the rest of the output.  I suspect it would be easier to read
   with the error separated from hints:

	Fetching gitk
	fatal: the remote server sent an empty response
	hint: does the repository exist?
	hint: do you have the correct access rights?
	error: Could not fetch gitk
	Fetching debian

   If a server is misconfigured and just decides to send an empty
   response for no good reason, the output would still be true.

 * The error message is the same whether the server returned no
   response or an incomplete pkt-line.  Maybe in the latter case it
   should print the "hung up unexpectedly" thing.

Thoughts?

Jonathan

Re: [PATCH v2] remove the impression of unexpectedness when access is denied

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I ran into this message for the first time today.
>
>  $ git fetch --all
>  Fetching origin
>  remote: Counting objects: 368, done.
> [...]
>  Fetching gitk
>  fatal: Could not read from remote repository.
>
>  Please make sure you have the correct access rights
>  and the repository exists.
>  error: Could not fetch gitk
>  Fetching debian
>  Fetching pape
> [...]
>
> The "gitk" remote refers to git://git.kernel.org/pub/scm/gitk/gitk.
> Using ls-remote to contact it produces the same result.  The message
> is correct: the repository does not exist.
>
> Impressions:
>
>  * Looking at "Could not read", it is not clear what could not read
>    and why.  GIT_TRACE_PACKET tells me the interaction was
>
> 	me> git-upload-pack /pub/scm/gitk/gitk\0host=git.kernel.org\0
> 	them> (hangup)
>
>    Would it make sense for the server to send an "ERR" packet to give
>    a more helpful diagnosis?

I think git-daemon does so (or at least attempts to do so);
path_ok() uses enter_repo() to check if the given path is a
repository, returns NULL to run_service(), whichh in turn calls
daemon_error() that does the ERR thing.

>  * The spacing and capitalization is odd and makes it not flow well
>    with the rest of the output.  I suspect it would be easier to read
>    with the error separated from hints:
>
> 	Fetching gitk
> 	fatal: the remote server sent an empty response
> 	hint: does the repository exist?
> 	hint: do you have the correct access rights?
> 	error: Could not fetch gitk
> 	Fetching debian
>
>    If a server is misconfigured and just decides to send an empty
>    response for no good reason, the output would still be true.

It does sound better. Also s/Could not fetch/could not fetch/.

>  * The error message is the same whether the server returned no
>    response or an incomplete pkt-line.  Maybe in the latter case it
>    should print the "hung up unexpectedly" thing.

OK.

Re: [PATCH v2] remove the impression of unexpectedness when access is denied

[Jeff King]

On Mon, May 06, 2013 at 07:02:41AM -0700, Junio C Hamano wrote:

> >    Would it make sense for the server to send an "ERR" packet to give
> >    a more helpful diagnosis?
> 
> I think git-daemon does so (or at least attempts to do so);
> path_ok() uses enter_repo() to check if the given path is a
> repository, returns NULL to run_service(), whichh in turn calls
> daemon_error() that does the ERR thing.

Yeah, that went into v1.7.8. Do we have any simple way to find out which
version kernel.org is running? They should probably also turn on the
--informative-errors option, as they do not (AFAIK) have any private
repos whose information could be leaked by better error messages.

If they are running v1.7.8 and it is not producing an ERR message, then
I think there is a bug.

> >  * The error message is the same whether the server returned no
> >    response or an incomplete pkt-line.  Maybe in the latter case it
> >    should print the "hung up unexpectedly" thing.
> 
> OK.

I made a stab at this some time ago:

  http://article.gmane.org/gmane.comp.version-control.git/112189

There were some follow-up comments, and I remember trying to make
something work with processing remote stderr, but running into
complications. Alas, I don't remember any more details than that. But
maybe it helps.

-Peff