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>

Michael Haggerty committed Jun 22, 2015 at 16:03 UTC 1c03c4d34771db20b78231359caa6fda28e2d9fe
4 files changed +14 -15
builtin/branch.c
+2 -1
@@ -253,7 +253,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
253 continue;
254 }
255
256 - if (delete_ref(name, sha1, REF_NODEREF)) {
256 + if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
257 + REF_NODEREF)) {
258 error(remote_branch
259 ? _("Error deleting remote-tracking branch '%s'")
260 : _("Error deleting branch '%s'"),
builtin/update-ref.c
+7 -1
@@ -422,7 +422,13 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
422 if (no_deref)
423 flags = REF_NODEREF;
424 if (delete)
425 - return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
425 + /*
426 + * For purposes of backwards compatibility, we treat
427 + * NULL_SHA1 as "don't care" here:
428 + */
429 + return delete_ref(refname,
430 + (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
431 + flags);
432 else
433 return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
434 flags, UPDATE_REFS_DIE_ON_ERR);
refs.c
-8
@@ -2833,14 +2833,6 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,
2833 struct ref_transaction *transaction;
2834 struct strbuf err = STRBUF_INIT;
2835
2836 - /*
2837 - * Treat NULL_SHA1 and NULL alike, to mean "we don't care what
2838 - * the old value of the reference was (or even if it didn't
2839 - * exist)":
2840 - */
2841 - if (old_sha1 && is_null_sha1(old_sha1))
2842 - old_sha1 = NULL;
2843 -
2836 transaction = ref_transaction_begin(&err);
2837 if (!transaction ||
2838 ref_transaction_delete(transaction, refname, old_sha1,
refs.h
+5 -5
@@ -240,11 +240,11 @@ extern int read_ref_at(const char *refname, unsigned int flags,
240 extern int reflog_exists(const char *refname);
241
242 /*
243 - * Delete the specified reference. If old_sha1 is non-NULL and not
244 - * NULL_SHA1, then verify that the current value of the reference is
245 - * old_sha1 before deleting it. If old_sha1 is NULL or NULL_SHA1,
246 - * delete the reference if it exists, regardless of its old value.
247 - * flags is passed through to ref_transaction_delete().
243 + * Delete the specified reference. If old_sha1 is non-NULL, then
244 + * verify that the current value of the reference is old_sha1 before
245 + * deleting it. If old_sha1 is NULL, delete the reference if it
246 + * exists, regardless of its old value. It is an error for old_sha1 to
247 + * be NULL_SHA1. flags is passed through to ref_transaction_delete().
248 */
249 extern int delete_ref(const char *refname, const unsigned char *old_sha1,
250 unsigned int flags);