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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 14, 2012, 17:11 UTC
Message-ID: <7v395xg6oh.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120614071259.GA51076@book.hvoigt.net>

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 25 quoted lines

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

Show 11 quoted lines

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