Re: [PATCH] Segmentation Fault on non-commit --branch clone

[Jeff King <peff@peff.net>]

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