Re: [PATCH 3/3] send-pack: assign remote errors to each ref

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index c7d07aa..bcf7143 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -146,19 +146,43 @@ static void get_local_heads(void)
>  	for_each_ref(one_local_ref, NULL);
>  }
>  
> -static int receive_status(int in)
> +static struct ref *set_ref_error(struct ref *refs, const char *line)
>  {
> +	struct ref *ref;
> +
> +	for (ref = refs; ref; ref = ref->next) {
> +		const char *msg;
> +		if (prefixcmp(line, ref->name))
> +			continue;
> +		msg = line + strlen(ref->name);
> +		if (*msg++ != ' ')
> +			continue;
> +		ref->status = REF_STATUS_REMOTE_REJECT;
> +		ref->error = xstrdup(msg);
> +		ref->error[strlen(ref->error)-1] = '\0';
> +		return ref;
> +	}
> +	return NULL;
> +}

It probably would not matter for sane repositories, but with
thousands of refs, strlen() and prefixcmp() may start to hurt:

	struct ref *ref;
	int reflen;
	const char *msg = strchr(line, ' ');

        if (!msg)
        	return NULL;
	reflen = msg - line;
	msg++;
	for (ref = refs; ref; ref = ref->next) {
        	if (strncmp(line, ref->name, reflen) || line[reflen] != ' ')
			continue;
		...
		return ref->next;
	}
	return NULL;

but the "hint" optimization probably make the above
micro-optimization irrelevant.

> +/* a return value of -1 indicates that an error occurred,
> + * but we were able to set individual ref errors. A return
> + * value of -2 means we couldn't even get that far. */

It is preferred to have a multi-line comment like this:

	/*
         * A return value of -1 ...
	 * ...
	 * ... couldn't even get that far.
	 */

> +static int receive_status(int in, struct ref *refs)
> ...
> +	hint = NULL;
>  	while (1) {
>  		len = packet_read_line(in, line, sizeof(line));
>  		if (!len)
> @@ -171,7 +195,10 @@ static int receive_status(int in)
>  		}
>  		if (!memcmp(line, "ok", 2))
>  			continue;
> -		fputs(line, stderr);
> +		if (hint)
> +			hint = set_ref_error(hint, line + 3);
> +		if (!hint)
> +			hint = set_ref_error(refs, line + 3);

Clever... taking advantage of the order receive-pack reports to
optimize.

Before receive_status() is called, can the refs already have the
error status and string set?

> @@ -429,12 +463,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
>  	}
>  	close(out);
>  
> -	print_push_status(dest, remote_refs);
> -
>  	if (expect_status_report) {
> -		if (receive_status(in))
> +		ret = receive_status(in, remote_refs);
> +		if (ret == -2)
>  			return -1;

Hmm.  When we did not receive status, we cannot tell what
succeeded or failed, but what we _can_ tell the user is which
refs we attempted to push.  I wonder if robbing that information
from the user with this "return -1" is a good idea.  Perhaps we
would instead want to set the status of all the refs to error
and call print_push_status() anyway?  I dunno.

>  	}
> +	else
> +		ret = 0;
> +
> +	print_push_status(dest, remote_refs);
>  
>  	if (!args.dry_run && remote) {
>  		for (ref = remote_refs; ref; ref = ref->next)