Re: [PATCH] connect.c: add a way for git-daemon to pass an error back to client

From: Tom Preston-Werner <tom@github.com>
Date: Nov 1, 2008, 06:29 UTC
Message-ID: <b97024a40810312329o53e37fd5td82aa69634ff1e6b@mail.gmail.com>
In-Reply-To: <7vy7043sy9.fsf@gitster.siamese.dyndns.org>

On Fri, Oct 31, 2008 at 10:39 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

> "Tom Preston-Werner" <tom@github.com> writes:
>
>> Example
>>
>> S: ERR No matching repository.
>> C: fatal: remote error: No matching repository.
>
> I like what this tries to do.
>
> I briefly wondered if this should be restricted to the very first message
> from the other end, but I think it is not necessary.  If the remote throws
> a few valid looking "SHA-1 SP refname" lines and then said "ERR" (which
> cannot be the beginning of a valid SHA-1), we can safely and unambiguously
> declare that this is an error message from the remote end.
>
>> diff --git a/connect.c b/connect.c
>> index 0c50d0a..3af91d6 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -70,6 +70,9 @@ struct ref **get_remote_heads(int in, struct ref **list,
>>               if (buffer[len-1] == '\n')
>>                       buffer[--len] = 0;
>>
>> +             if (len > 4 && !memcmp("ERR", buffer, 3))
>
> Would matching 4 bytes "ERR " here an improvement?  You are expecting
> buffer+4 is where the message begins in die() anyway, and otherwise you
> would show the message without "N" if you got "ERRNo matching repo".

I saw several methods of testing for a specific prefix in connect.c. Looking more closely at the source, the closest similar call is actually the test for ACK:

	if (!prefixcmp(line, "ACK ")) {
		if (!get_sha1_hex(line+4, result_sha1)) {
			if (strstr(line+45, "continue"))
				return 2;
			return 1;
		}
	}

Explicitly testing for "ERR " (including the space) does seem like the more correct thing to do. Would you like me to resubmit a modified patch that uses prefixcmp()?

Tom