On Sat, Nov 17, 2007 at 05:03:57PM -0800, Junio C Hamano wrote:
> > + 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.
> > 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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
