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

From: Jeff King <peff@peff.net>
Date: Nov 18, 2007, 02:39 UTC
Message-ID: <20071118023942.GA4560@sigill.intra.peff.net>
In-Reply-To: <7vir40z7nm.fsf@gitster.siamese.dyndns.org>

On Sat, Nov 17, 2007 at 05:03:57PM -0800, Junio C Hamano wrote:

Show 7 quoted lines

> > +	for (ref = refs; ref; ref = ref->next) {
> > +		const char *msg;
> > +		if (prefixcmp(line, ref->name))
> > +			continue;
> 
> It probably would not matter for sane repositories, but with
> thousands of refs, strlen() and prefixcmp() may start to hurt:

It is actually _just_ prefixcmp. Or do you mean the strlen we call in prefixcmp? If so, I think the right solution is to make prefixcmp faster. :)

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

Agreed.

Show 7 quoted lines

> It is preferred to have a multi-line comment like this:
> 
> 	/*
>          * A return value of -1 ...
> 	 * ...
> 	 * ... couldn't even get that far.
> 	 */

OK. Since it is already in next, do you want a style fixup patch?

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

Nothing else sets the string, so the latter is not possible (perhaps it should be "remote_error" for clarity). It is less clear that we are not overwriting another status; however, if you look at do_send_pack, we only actually send the remote refs that are getting REF_STATUS_OK.

A broken or malicious remote could change the push status of an arbitrary ref to rejection, but I don't really see the point. We could explicitly check that we are changing from OK to REMOTE_REJECTED in set_ref_error.

Show 12 quoted lines

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

That is a reasonable behavior (although they have already seen an "error: " message, I think). We might also consider returning something besides "-1" to differentiate "ok, but some refs failed" from "terribly broken". The old code used to use "-2" and "-4", but I checked and all of the error checking paths seemed to end up as a boolean.

I can work up a patch if there is consensus.

-Peff