Re: [PATCH v2 04/24] refs: convert update_ref and refs_update_ref to use struct object_id

> On 10/09/2017 03:11 AM, brian m. carlson wrote:
> > diff --git a/refs.c b/refs.c
> > index 0a5b68d6fb..51942df7b3 100644
> > --- a/refs.c
> > +++ b/refs.c
> > [...]
> > @@ -1003,12 +995,12 @@ int refs_update_ref(struct ref_store *refs, const char *msg,
> >  	int ret = 0;
> >  
> >  	if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
> > -		assert(refs == get_main_ref_store());
> 
> Was the deletion of the line above intentional?

> > -		ret = write_pseudoref(refname, new_sha1, old_sha1, &err);
> > +		ret = write_pseudoref(refname, new_oid, old_oid, &err);
> 
> This is not new to your code, but I just noticed a problem here.
> `refs_update_ref()` is documented, via its reference to
> `ref_transaction_update()`, to allow `new_sha1` (i.e., now `new_oid`) to
> be NULL. (NULL signifies that the value of the reference shouldn't be
> changed.)
> 
> But `write_pseudoref()` dereferences its `oid` argument unconditionally,
> so this call would fail if `new_oid` is NULL.
> 
> This has all been the case since `write_pseudoref()` was introduced in
> 74ec19d4be (pseudorefs: create and use pseudoref update and delete
> functions, 2015-07-31).
> 
> In my opinion, `write_pseudoref()` is broken. It should accept NULL as
> its `oid` argument.