Subject: Re: [PATCH v6 25/25] refs: break out ref conflict checks

From: David Turner <[email-masked]>
Date: Thu, 05 Nov 2015 11:22:15 -0500

On Thu, 2015-11-05 at 05:00 +0100, Michael Haggerty wrote:
> On 11/04/2015 10:01 PM, David Turner wrote:
> > On Tue, 2015-11-03 at 08:40 +0100, Michael Haggerty wrote:
> >> + * extras and skip must be sorted lists of reference names. Either one
> >> + * can be NULL, signifying the empty list.
> >> + */
> > 
> > My version had:
> > 
> > "skip can be NULL; extras cannot."
> > 
> > The first thing that function does is:
> > string_list_find_insert_index(extras, dirname, 0)
> > 
> > And that crashes when extras is null.  So I think my version is correct
> > here.
> 
> We're talking about the function find_descendant_ref(), which was added
> in this patch, right? Because the first thing that function does is
> 
> +	if (!extras)
> +		return NULL;
> 
> (This guard was in your version, too.) Also, the callsite doesn't
> protect against extras==NULL. So either we're talking about two
> different things here, or I disagree with you.

You're right.  I totally missed that.  But while looking at it, I
noticed that the commit message doesn't look quite right (my fault):

> Create new function verify_no_descendants, to hold one of the ref
> conflict checks used in verify_refname_available. Multiple backends
> will need this function, so move it to the common code.

The function is find_descendant_ref not verify_no_descendants.