On Fri, Nov 01, 2019 at 01:24:32AM +0100, Davide Berardi wrote:
> Fixed segmentation fault that can be triggered using
> $ git clone --branch $object $repository
> with object pointing to a non-commit ref (e.g. a blob).
Thanks for working on this. I left some thoughts on the overall fallback
scheme in the other part of the thread, and other than I agree with all
of Dscho's review.
A few other comments:
> +static int fallback_on_noncommit(const struct ref *check,
> + const struct ref *remote,
> + const char *msg)
> +{
> + if (check == NULL)
> + return 1;
I wondered in what circumstances "check" would be NULL. In the first
conditional we pass "our" after checking it's non-NULL:
> if (our && skip_prefix(our->name, "refs/heads/", &head)) {
> + if (fallback_on_noncommit(our, remote, msg) != 0)
and then again in the second case-arm:
> } else if (our) {
> + if (fallback_on_noncommit(our, remote, msg) != 0)
and then in the third we pass remote after checking that it's not NULL:
> } else if (remote) {
> + if (fallback_on_noncommit(remote, remote, msg) != 0)
> + return;
So I think this NULL check can go away. In general I don't mind
functions being defensive, but it's hard to decide whether it's doing
the right thing since it's not a case we think can come up (it could be
marked with a BUG() assertion, but IMHO it's not worth it; most
functions require their arguments to be non-NULL, so checking it would
be unusual in our code base).
> +static int fallback_on_noncommit(const struct ref *check,
> + const struct ref *remote,
> + const char *msg)
> [...]
> + return c == NULL;
The return value for this function is unusual for our code base. If it's
just an error return, we'd usually use "0" for success and a negative
value for errors (i.e., mimicking system calls).
> diff --git a/refs.h b/refs.h
> index 730d05ad91..35ee6eb058 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -497,6 +497,13 @@ enum action_on_err {
> UPDATE_REFS_QUIET_ON_ERR
> };
>
> +/*
> + * In case of a remote HEAD pointing to a non-commit update_head
> + * will make HEAD reference fallback to this value, master ref
> + * should be safe.
> + */
> +#define FALLBACK_REF "refs/heads/master"
> +
> /*
Since this is only used in one spot, I think it's better to keep it
localized to that function. E.g., with:
static const char fallback_ref[] = "refs/heads/master";
That way it's clear that no other code depends on it.
-Peff
