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

Heiko Voigt [off-list ref] writes:

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?

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

Hi,

On Thu, Jun 14, 2012 at 10:11:10AM -0700, Junio C Hamano wrote:

Heiko Voigt [off-list ref] writes:

quoted

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

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

Hi,

Heiko Voigt wrote:

[...]

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