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

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

quoted

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

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.

quoted

 	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