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

[Jonathan Nieder]

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 
> ---
>  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, );
>   if (!transaction ||
> - ref_transaction_delete(transaction, refname, old_sha1,
> + ref_transaction_delete(transaction, refname,
> +old_oid ? old_oid->hash : NULL,
>  flags, msg, ) ||

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 

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

[brian m. carlson]

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 
---
 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(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 355f9ef5da..6031b74d68 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -256,7 +256,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
goto next;
}
 
-   if (delete_ref(NULL, name, is_null_oid() ? NULL : oid.hash,
+   if (delete_ref(NULL, name, is_null_oid() ? NULL : ,
   REF_NODEREF)) {
error(remote_branch
  ? _("Error deleting remote-tracking branch '%s'")
diff --git a/builtin/replace.c b/builtin/replace.c
index 3e71a77152..2854eaa0f3 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -128,7 +128,7 @@ static int for_each_replace_name(const char **argv, 
each_replace_name_fn fn)
 static int delete_replace_ref(const char *name, const char *ref,
  const struct object_id *oid)
 {
-   if (delete_ref(NULL, ref, oid->hash, 0))
+   if (delete_ref(NULL, ref, oid, 0))
return 1;
printf("Deleted replace ref '%s'\n", name);
return 0;
diff --git a/builtin/reset.c b/builtin/reset.c
index 9cd89b2305..5f3632e05b 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -269,7 +269,7 @@ static int reset_refs(const char *rev, const struct 
object_id *oid)
update_ref_oid(msg.buf, "ORIG_HEAD", orig, old_orig, 0,
   UPDATE_REFS_MSG_ON_ERR);
} else if (old_orig)
-   delete_ref(NULL, "ORIG_HEAD", old_orig->hash, 0);
+   delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
set_reflog_message(, "updating HEAD", rev);
update_ref_status = update_ref_oid(msg.buf, "HEAD", oid, orig, 0,
   UPDATE_REFS_MSG_ON_ERR);
diff --git a/builtin/tag.c b/builtin/tag.c
index c627794181..46ff4ca736 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -97,7 +97,7 @@ static int for_each_tag_name(const char **argv, 
each_tag_name_fn fn,
 static int delete_tag(const char *name, const char *ref,
  const struct object_id *oid, const void *cb_data)
 {
-   if (delete_ref(NULL, ref, oid->hash, 0))
+   if (delete_ref(NULL, ref, oid, 0))
return 1;
printf(_("Deleted tag '%s' (was %s)\n"), name, 
find_unique_abbrev(oid->hash, DEFAULT_ABBREV));
return 0;
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6b90c5dead..bf0f80ebae 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -434,7 +434,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
 * NULL_SHA1 as "don't care" here:
 */
return delete_ref(msg, refname,
- (oldval && !is_null_oid()) ? 
oldoid.hash : NULL,
+ (oldval && !is_null_oid()) ?  : 
NULL,
  flags);
else
return update_ref(msg, refname, oid.hash, oldval ? oldoid.hash 
: NULL,
diff --git a/refs.c b/refs.c
index 6042645c40..0a5b68d6fb 100644
--- a/refs.c
+++ b/refs.c
@@ -620,25 +620,25 @@ static int write_pseudoref(const char *pseudoref, const 
unsigned char *sha1,
return ret;
 }
 
-static int delete_pseudoref(const char *pseudoref, const unsigned char 
*old_sha1)
+static int delete_pseudoref(const char *pseudoref, const struct object_id 
*old_oid)
 {
static struct lock_file lock;
const char *filename;
 
filename = git_path("%s", pseudoref);
 
-   if (old_sha1 && !is_null_sha1(old_sha1)) {
+   if (old_oid && !is_null_oid(old_oid)) {
int fd;
-   unsigned char actual_old_sha1[20];
+   struct object_id actual_old_oid;
 
fd = hold_lock_file_for_update_timeout(
, filename, LOCK_DIE_ON_ERROR,
get_files_ref_lock_timeout_ms());
if (fd < 0)
die_errno(_("Could not open '%s' for writing"), 
filename);
-   if (read_ref(pseudoref, actual_old_sha1))
+   if (read_ref(pseudoref, actual_old_oid.hash))
die("could not read ref '%s'", pseudoref);
-   if (hashcmp(actual_old_sha1,