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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Oct 11, 2017, 06:33 UTC
Message-ID: <9d4c3854-38fb-d5b4-0436-0eee707201ab@alum.mit.edu>
In-Reply-To: <20171009011132.675341-5-sandals@crustytoothpaste.net>

On 10/09/2017 03:11 AM, brian m. carlson wrote:

Show 36 quoted lines

> Convert update_ref, refs_update_ref, and write_pseudoref to use struct
> object_id.  Update the existing callers as well.  Remove update_ref_oid,
> as it is no longer needed.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  bisect.c                  |  5 +++--
>  builtin/am.c              | 14 +++++++-------
>  builtin/checkout.c        |  3 +--
>  builtin/clone.c           | 14 +++++++-------
>  builtin/merge.c           | 13 ++++++-------
>  builtin/notes.c           | 10 +++++-----
>  builtin/pull.c            |  2 +-
>  builtin/reset.c           |  4 ++--
>  builtin/update-ref.c      |  2 +-
>  notes-cache.c             |  2 +-
>  notes-utils.c             |  2 +-
>  refs.c                    | 40 ++++++++++++++++------------------------
>  refs.h                    |  5 +----
>  sequencer.c               |  9 +++------
>  t/helper/test-ref-store.c | 10 +++++-----
>  transport-helper.c        |  3 ++-
>  transport.c               |  4 ++--
>  17 files changed, 64 insertions(+), 78 deletions(-)
> 
> [...]
> 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.

Show 10 quoted lines

>  	} else {
>  		t = ref_store_transaction_begin(refs, &err);
>  		if (!t ||
> -		    ref_transaction_update(t, refname, new_sha1, old_sha1,
> +		    ref_transaction_update(t, refname, new_oid ? new_oid->hash : NULL,
> +					   old_oid ? old_oid->hash : NULL,
>  					   flags, msg, &err) ||
>  		    ref_transaction_commit(t, &err)) {
>  			ret = 1;
> [...]

Michael