Web lists-archives.com

Re: [PATCH v2 03/24] refs: convert delete_ref and refs_delete_ref to struct object_id




brian m. carlson wrote:

> Convert delete_ref and refs_delete_ref to take a pointer to struct
> object_id.  Update the documentation accordingly, including referring to
> null_oid in lowercase, as it is not a #define constant.
>
> Signed-off-by: brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx>
> ---
>  builtin/branch.c          |  2 +-
>  builtin/replace.c         |  2 +-
>  builtin/reset.c           |  2 +-
>  builtin/tag.c             |  2 +-
>  builtin/update-ref.c      |  2 +-
>  refs.c                    | 21 +++++++++++----------
>  refs.h                    | 12 ++++++------
>  refs/files-backend.c      |  2 +-
>  t/helper/test-ref-store.c |  6 +++---
>  9 files changed, 26 insertions(+), 25 deletions(-)

Was this prepared using coccinelle?  (Just curious.)

[...]
> @@ -663,12 +663,13 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
>  
>  	if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
>  		assert(refs == get_main_ref_store());
> -		return delete_pseudoref(refname, old_sha1);
> +		return delete_pseudoref(refname, old_oid);
>  	}
>  
>  	transaction = ref_store_transaction_begin(refs, &err);
>  	if (!transaction ||
> -	    ref_transaction_delete(transaction, refname, old_sha1,
> +	    ref_transaction_delete(transaction, refname,
> +				   old_oid ? old_oid->hash : NULL,
>  				   flags, msg, &err) ||

musing out loud: Distinguishing contexts where we need this kind of
change and contexts where we can use old_oid->hash directly can be
subtle.  Do we need some kind of annotation to mark which parameters
are nullable and which aren't?

[...]
> --- a/refs.h
> +++ b/refs.h
> @@ -371,19 +371,19 @@ int refs_reflog_exists(struct ref_store *refs, const char *refname);
>  int reflog_exists(const char *refname);
>  
>  /*
> - * Delete the specified reference. If old_sha1 is non-NULL, then
> + * Delete the specified reference. If old_oid is non-NULL, then
>   * verify that the current value of the reference is old_sha1 before
> - * deleting it. If old_sha1 is NULL, delete the reference if it
> - * exists, regardless of its old value. It is an error for old_sha1 to
> - * be NULL_SHA1. msg and flags are passed through to
> + * deleting it. If old_oid is NULL, delete the reference if it
> + * exists, regardless of its old value. It is an error for old_oid to
> + * be null_oid. msg and flags are passed through to
>   * ref_transaction_delete().
>   */
>  int refs_delete_ref(struct ref_store *refs, const char *msg,

Thanks for updating the comment.

Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>