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

Jeff King [off-list ref] writes:

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

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?

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)

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