delete_ref(): use the usual convention for old_sha1

The ref_transaction_update() family of functions use the following
convention for their old_sha1 parameters:

* old_sha1 == NULL: Don't check the old value at all.
* is_null_sha1(old_sha1): Ensure that the reference didn't exist
  before the transaction.
* otherwise: Ensure that the reference had the specified value before
  the transaction.

delete_ref() had a different convention, namely treating
is_null_sha1(old_sha1) as "don't care". Change it to adhere to the
standard convention to reduce the scope for confusion.

Please note that it is now a bug to pass old_sha1=NULL_SHA1 to
delete_ref() (because it doesn't make sense to delete a reference that
you already know doesn't exist). This is consistent with the behavior
of ref_transaction_delete().

Most of the callers of delete_ref() never pass old_sha1=NULL_SHA1 to
delete_ref(), and are therefore unaffected by this change. The
two exceptions are:

* The call in cmd_update_ref(), which passed NULL_SHA1 if the old
  value passed in on the command line was 0{40} or the empty string.
  Change that caller to pass NULL in those cases.

  Arguably, it should be an error to call "update-ref -d" with the old
  value set to "does not exist", just as it is for the `--stdin`
  command "delete". But since this usage was accepted until now,
  continue to accept it.

* The call in delete_branches(), which could pass NULL_SHA1 if
  deleting a broken or symbolic ref. Change it to pass NULL in these
  cases.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/branch.c b/builtin/branch.c
index 47e3eb92235a0bd0ef1b248eabca8ef0c0da51a0..58aa84f1e85212772d15175679e95263eae75123 100644 (file)
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -253,7 +253,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
                        continue;
                }
 
-               if (delete_ref(name, sha1, REF_NODEREF)) {
+               if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
+                              REF_NODEREF)) {
                        error(remote_branch
                              ? _("Error deleting remote-tracking branch '%s'")
                              : _("Error deleting branch '%s'"),

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 160c7ac1d3c63d6acfea5794958e27eac9ab159e..6763cf1837db99442a7140fe5bb87a0b3a1248fd 100644 (file)
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -422,7 +422,13 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
        if (no_deref)
                flags = REF_NODEREF;
        if (delete)
-               return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
+               /*
+                * For purposes of backwards compatibility, we treat
+                * NULL_SHA1 as "don't care" here:
+                */
+               return delete_ref(refname,
+                                 (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
+                                 flags);
        else
                return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
                                  flags, UPDATE_REFS_DIE_ON_ERR);

diff --git a/refs.c b/refs.c
index d5c3f8de1ba25c1ecc6b556103cd437cfa08922c..c5086ae70c11527ec0ffc15312ab8683bb719c41 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -2833,14 +2833,6 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,
        struct ref_transaction *transaction;
        struct strbuf err = STRBUF_INIT;
 
-       /*
-        * Treat NULL_SHA1 and NULL alike, to mean "we don't care what
-        * the old value of the reference was (or even if it didn't
-        * exist)":
-        */
-       if (old_sha1 && is_null_sha1(old_sha1))
-               old_sha1 = NULL;
-
        transaction = ref_transaction_begin(&err);
        if (!transaction ||
            ref_transaction_delete(transaction, refname, old_sha1,

diff --git a/refs.h b/refs.h
index c9596ea69ab6b9bb813a564cf6d9dacb45d4b5a0..e82fca51f501a8297977e1315146c14cafca725b 100644 (file)
--- a/refs.h
+++ b/refs.h
@@ -240,11 +240,11 @@ extern int read_ref_at(const char *refname, unsigned int flags,
 extern int reflog_exists(const char *refname);
 
 /*
- * Delete the specified reference. If old_sha1 is non-NULL and not
- * NULL_SHA1, then verify that the current value of the reference is
- * old_sha1 before deleting it. If old_sha1 is NULL or NULL_SHA1,
- * delete the reference if it exists, regardless of its old value.
- * flags is passed through to ref_transaction_delete().
+ * Delete the specified reference. If old_sha1 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. flags is passed through to ref_transaction_delete().
  */
 extern int delete_ref(const char *refname, const unsigned char *old_sha1,
                      unsigned int flags);