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

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Jun 14, 2012, 20:37 UTC
Message-ID: <20120614203712.GA51783@book.hvoigt.net>
In-Reply-To: <7v395xg6oh.fsf@alter.siamese.dyndns.org>

Hi,

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

Show 34 quoted lines

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

Show 17 quoted lines

> 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