[PATCH v2 0/9] Tidy up the constants related to ref_update::flags

[PATCH v2 0/9] Tidy up the constants related to ref_update::flags

[Michael Haggerty]

This is a reroll of a patch series that tidies up some stuff around
the ref_update::flags constants. Thanks to Junio and Martin for their
comments about v1 [1].

Relative to v1, this version:

* In patch 5, cleans up the touched comments to refer to OIDs rather
  than SHA-1s.

* Adds a patch 8, which changes `write_packed_entry()` to take
  `object_id` arguments.

* Adds a patch 9, which cleans up some remaining comments across all
  of the refs-related files to refer to OIDs rather than SHA-1s.

This patch series depends on bc/object-id. The patches are also
available from my GitHub fork as branch `tidy-ref-update-flags` [2].

Michael

[1] https://public-inbox.org/git/cover.1509183413.git.mhagger@alum.mit.edu/
[2] https://github.com/mhagger/git

Michael Haggerty (9):
  files_transaction_prepare(): don't leak flags to packed transaction
  prune_ref(): call `ref_transaction_add_update()` directly
  ref_transaction_update(): die on disallowed flags
  ref_transaction_add_update(): remove a check
  refs: tidy up and adjust visibility of the `ref_update` flags
  refs: rename constant `REF_NODEREF` to `REF_NO_DEREF`
  refs: rename constant `REF_ISPRUNING` to `REF_IS_PRUNING`
  write_packed_entry(): take `object_id` arguments
  refs: update some more docs to use "oid" rather than "sha1"

 builtin/am.c           |   2 +-
 builtin/branch.c       |   2 +-
 builtin/checkout.c     |   2 +-
 builtin/clone.c        |   4 +-
 builtin/notes.c        |   2 +-
 builtin/remote.c       |   6 +--
 builtin/symbolic-ref.c |   2 +-
 builtin/update-ref.c   |   4 +-
 refs.c                 |   8 ++-
 refs.h                 |  77 ++++++++++++++++-------------
 refs/files-backend.c   | 132 +++++++++++++++++++++++++++++++++++--------------
 refs/packed-backend.c  |  18 +++----
 refs/ref-cache.c       |   4 +-
 refs/refs-internal.h   |  81 +++++++++---------------------
 sequencer.c            |   6 +--
 15 files changed, 188 insertions(+), 162 deletions(-)

-- 
2.14.1

[PATCH v2 1/9] files_transaction_prepare(): don't leak flags to packed transaction

[Michael Haggerty]

The files backend uses `ref_update::flags` for several internal flags.
But those flags have no meaning to the packed backend. So when adding
updates for the packed-refs transaction, only use flags that make
sense to the packed backend.

`REF_NODEREF` is part of the public interface, and it's logically what
we want, so include it. In fact it is actually ignored by the packed
backend (which doesn't support symbolic references), but that's its
own business.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2bd54e11ae..fadf1036d3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2594,8 +2594,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 
 			ref_transaction_add_update(
 					packed_transaction, update->refname,
-					update->flags & ~REF_HAVE_OLD,
-					&update->new_oid, &update->old_oid,
+					REF_HAVE_NEW | REF_NODEREF,
+					&update->new_oid, NULL,
 					NULL);
 		}
 	}
-- 
2.14.1

[PATCH v2 2/9] prune_ref(): call `ref_transaction_add_update()` directly

[Michael Haggerty]

`prune_ref()` needs to use the `REF_ISPRUNING` flag, but we want to
make that flag private to the files backend. So instead of calling
`ref_transaction_delete()`, which is a public function and therefore
shouldn't allow the `REF_ISPRUNING` flag, change `prune_ref()` to call
`ref_transaction_add_update()`, which is private to the refs
module. (Note that we don't need any of the other services provided by
`ref_transaction_delete()`.)

This allows us to change `ref_transaction_update()` to reject the
`REF_ISPRUNING` flag. Do so by adjusting
`REF_TRANSACTION_UPDATE_ALLOWED_FLAGS`. Also add parentheses to its
definition to avoid potential future mishaps.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.h               |  4 +---
 refs/files-backend.c | 25 ++++++++++++++++---------
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/refs.h b/refs.h
index 15fd419c7d..4ffef9502d 100644
--- a/refs.h
+++ b/refs.h
@@ -349,9 +349,7 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags);
  * Flags that can be passed in to ref_transaction_update
  */
 #define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \
-	REF_ISPRUNING |                      \
-	REF_FORCE_CREATE_REFLOG |            \
-	REF_NODEREF
+	(REF_NODEREF | REF_FORCE_CREATE_REFLOG)
 
 /*
  * Setup reflog before using. Fill in err and return -1 on failure.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index fadf1036d3..ba72d28b13 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -989,22 +989,29 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
 {
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
+	int ret = -1;
 
 	if (check_refname_format(r->name, 0))
 		return;
 
 	transaction = ref_store_transaction_begin(&refs->base, &err);
-	if (!transaction ||
-	    ref_transaction_delete(transaction, r->name, &r->oid,
-				   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
-	    ref_transaction_commit(transaction, &err)) {
-		ref_transaction_free(transaction);
+	if (!transaction)
+		goto cleanup;
+	ref_transaction_add_update(
+			transaction, r->name,
+			REF_NODEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_ISPRUNING,
+			&null_oid, &r->oid, NULL);
+	if (ref_transaction_commit(transaction, &err))
+		goto cleanup;
+
+	ret = 0;
+
+cleanup:
+	if (ret)
 		error("%s", err.buf);
-		strbuf_release(&err);
-		return;
-	}
-	ref_transaction_free(transaction);
 	strbuf_release(&err);
+	ref_transaction_free(transaction);
+	return;
 }
 
 /*
-- 
2.14.1

[PATCH v2 3/9] ref_transaction_update(): die on disallowed flags

[Michael Haggerty]

Callers shouldn't be passing disallowed flags into
`ref_transaction_update()`. So instead of masking them off, treat it
as a bug if any are set.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 62a7621025..7c1e206e08 100644
--- a/refs.c
+++ b/refs.c
@@ -940,7 +940,8 @@ int ref_transaction_update(struct ref_transaction *transaction,
 		return -1;
 	}
 
-	flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
+	if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
+		BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
 
 	flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
 
-- 
2.14.1

[PATCH v2 4/9] ref_transaction_add_update(): remove a check

[Michael Haggerty]

We want to make `REF_ISPRUNING` internal to the files backend. For
this to be possible, `ref_transaction_add_update()` mustn't know about
it. So move the check that `REF_ISPRUNING` is only used with
`REF_NODEREF` from this function to `files_transaction_prepare()`.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               | 3 ---
 refs/files-backend.c | 7 ++++++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 7c1e206e08..0d9a1348cd 100644
--- a/refs.c
+++ b/refs.c
@@ -906,9 +906,6 @@ struct ref_update *ref_transaction_add_update(
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: update called for transaction that is not open");
 
-	if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
-		die("BUG: REF_ISPRUNING set without REF_NODEREF");
-
 	FLEX_ALLOC_STR(update, refname, refname);
 	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
 	transaction->updates[transaction->nr++] = update;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index ba72d28b13..a47771e4d4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2518,13 +2518,18 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 	 * transaction. (If we end up splitting up any updates using
 	 * split_symref_update() or split_head_update(), those
 	 * functions will check that the new updates don't have the
-	 * same refname as any existing ones.)
+	 * same refname as any existing ones.) Also fail if any of the
+	 * updates use REF_ISPRUNING without REF_NODEREF.
 	 */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
 		struct string_list_item *item =
 			string_list_append(&affected_refnames, update->refname);
 
+		if ((update->flags & REF_ISPRUNING) &&
+		    !(update->flags & REF_NODEREF))
+			BUG("REF_ISPRUNING set without REF_NODEREF");
+
 		/*
 		 * We store a pointer to update in item->util, but at
 		 * the moment we never use the value of this field
-- 
2.14.1

[PATCH v2 5/9] refs: tidy up and adjust visibility of the `ref_update` flags

[Michael Haggerty]

The constants used for `ref_update::flags` were rather disorganized:

* The definitions in `refs.h` were not close to the functions that
  used them.

* Maybe constants were defined in `refs-internal.h`, making them
  visible to the whole refs module, when in fact they only made sense
  for the files backend.

* Their documentation wasn't very consistent and partly still referred
  to sha1s rather than oids.

* The numerical values followed no rational scheme

Fix all of these problems. The main functional improvement is that
some constants' visibility is now limited to `files-backend.c`.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.h               | 67 ++++++++++++++++++++++++++++++----------------------
 refs/files-backend.c | 45 +++++++++++++++++++++++++++++++++++
 refs/refs-internal.h | 67 ++++++++++++----------------------------------------
 3 files changed, 99 insertions(+), 80 deletions(-)

diff --git a/refs.h b/refs.h
index 4ffef9502d..261d46c10c 100644
--- a/refs.h
+++ b/refs.h
@@ -335,22 +335,6 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,
  */
 int refs_pack_refs(struct ref_store *refs, unsigned int flags);
 
-/*
- * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
- * REF_NODEREF: act on the ref directly, instead of dereferencing
- *              symbolic references.
- *
- * Other flags are reserved for internal use.
- */
-#define REF_NODEREF	0x01
-#define REF_FORCE_CREATE_REFLOG 0x40
-
-/*
- * Flags that can be passed in to ref_transaction_update
- */
-#define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \
-	(REF_NODEREF | REF_FORCE_CREATE_REFLOG)
-
 /*
  * Setup reflog before using. Fill in err and return -1 on failure.
  */
@@ -478,22 +462,23 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
  *
  *     refname -- the name of the reference to be affected.
  *
- *     new_sha1 -- the SHA-1 that should be set to be the new value of
- *         the reference. Some functions allow this parameter to be
+ *     new_oid -- the object ID that should be set to be the new value
+ *         of the reference. Some functions allow this parameter to be
  *         NULL, meaning that the reference is not changed, or
- *         null_sha1, meaning that the reference should be deleted. A
+ *         null_oid, meaning that the reference should be deleted. A
  *         copy of this value is made in the transaction.
  *
- *     old_sha1 -- the SHA-1 value that the reference must have before
+ *     old_oid -- the object ID that the reference must have before
  *         the update. Some functions allow this parameter to be NULL,
  *         meaning that the old value of the reference is not checked,
- *         or null_sha1, meaning that the reference must not exist
+ *         or null_oid, meaning that the reference must not exist
  *         before the update. A copy of this value is made in the
  *         transaction.
  *
  *     flags -- flags affecting the update, passed to
- *         update_ref_lock(). Can be REF_NODEREF, which means that
- *         symbolic references should not be followed.
+ *         update_ref_lock(). Possible flags: REF_NODEREF,
+ *         REF_FORCE_CREATE_REFLOG. See those constants for more
+ *         information.
  *
  *     msg -- a message describing the change (for the reflog).
  *
@@ -509,11 +494,37 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
  */
 
 /*
- * Add a reference update to transaction. new_oid is the value that
- * the reference should have after the update, or null_oid if it
- * should be deleted. If new_oid is NULL, then the reference is not
- * changed at all. old_oid is the value that the reference must have
- * before the update, or null_oid if it must not have existed
+ * The following flags can be passed to ref_transaction_update() etc.
+ * Internally, they are stored in `ref_update::flags`, along with some
+ * internal flags.
+ */
+
+/*
+ * Act on the ref directly; i.e., without dereferencing symbolic refs.
+ * If this flag is not specified, then symbolic references are
+ * dereferenced and the update is applied to the referent.
+ */
+#define REF_NODEREF (1 << 0)
+
+/*
+ * Force the creation of a reflog for this reference, even if it
+ * didn't previously have a reflog.
+ */
+#define REF_FORCE_CREATE_REFLOG (1 << 1)
+
+/*
+ * Bitmask of all of the flags that are allowed to be passed in to
+ * ref_transaction_update() and friends:
+ */
+#define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \
+	(REF_NODEREF | REF_FORCE_CREATE_REFLOG)
+
+/*
+ * Add a reference update to transaction. `new_oid` is the value that
+ * the reference should have after the update, or `null_oid` if it
+ * should be deleted. If `new_oid` is NULL, then the reference is not
+ * changed at all. `old_oid` is the value that the reference must have
+ * before the update, or `null_oid` if it must not have existed
  * beforehand. The old value is checked after the lock is taken to
  * prevent races. If the old value doesn't agree with old_oid, the
  * whole transaction fails. If old_oid is NULL, then the previous
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a47771e4d4..bbeafe1db7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -10,6 +10,51 @@
 #include "../object.h"
 #include "../dir.h"
 
+/*
+ * This backend uses the following flags in `ref_update::flags` for
+ * internal bookkeeping purposes. Their numerical values must not
+ * conflict with REF_NODEREF, REF_FORCE_CREATE_REFLOG, REF_HAVE_NEW,
+ * REF_HAVE_OLD, or REF_ISPRUNING, which are also stored in
+ * `ref_update::flags`.
+ */
+
+/*
+ * Used as a flag in ref_update::flags when a loose ref is being
+ * pruned. This flag must only be used when REF_NODEREF is set.
+ */
+#define REF_ISPRUNING (1 << 4)
+
+/*
+ * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken
+ * refs (i.e., because the reference is about to be deleted anyway).
+ */
+#define REF_DELETING (1 << 5)
+
+/*
+ * Used as a flag in ref_update::flags when the lockfile needs to be
+ * committed.
+ */
+#define REF_NEEDS_COMMIT (1 << 6)
+
+/*
+ * Used as a flag in ref_update::flags when we want to log a ref
+ * update but not actually perform it.  This is used when a symbolic
+ * ref update is split up.
+ */
+#define REF_LOG_ONLY (1 << 7)
+
+/*
+ * Used as a flag in ref_update::flags when the ref_update was via an
+ * update to HEAD.
+ */
+#define REF_UPDATE_VIA_HEAD (1 << 8)
+
+/*
+ * Used as a flag in ref_update::flags when the loose reference has
+ * been deleted.
+ */
+#define REF_DELETED_LOOSE (1 << 9)
+
 struct ref_lock {
 	char *ref_name;
 	struct lock_file lk;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index b0f3e300c7..2ea02dcbe4 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -8,58 +8,22 @@
  */
 
 /*
- * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken
- * refs (i.e., because the reference is about to be deleted anyway).
+ * The following flags can appear in `ref_update::flags`. Their
+ * numerical values must not conflict with those of REF_NODEREF and
+ * REF_FORCE_CREATE_REFLOG, which are also stored in
+ * `ref_update::flags`.
  */
-#define REF_DELETING	0x02
 
 /*
- * Used as a flag in ref_update::flags when a loose ref is being
- * pruned. This flag must only be used when REF_NODEREF is set.
+ * The reference should be updated to new_sha1.
  */
-#define REF_ISPRUNING	0x04
+#define REF_HAVE_NEW (1 << 2)
 
 /*
- * Used as a flag in ref_update::flags when the reference should be
- * updated to new_sha1.
+ * The current reference's value should be checked to make sure that
+ * it agrees with old_sha1.
  */
-#define REF_HAVE_NEW	0x08
-
-/*
- * Used as a flag in ref_update::flags when old_sha1 should be
- * checked.
- */
-#define REF_HAVE_OLD	0x10
-
-/*
- * Used as a flag in ref_update::flags when the lockfile needs to be
- * committed.
- */
-#define REF_NEEDS_COMMIT 0x20
-
-/*
- * 0x40 is REF_FORCE_CREATE_REFLOG, so skip it if you're adding a
- * value to ref_update::flags
- */
-
-/*
- * Used as a flag in ref_update::flags when we want to log a ref
- * update but not actually perform it.  This is used when a symbolic
- * ref update is split up.
- */
-#define REF_LOG_ONLY 0x80
-
-/*
- * Internal flag, meaning that the containing ref_update was via an
- * update to HEAD.
- */
-#define REF_UPDATE_VIA_HEAD 0x100
-
-/*
- * Used as a flag in ref_update::flags when the loose reference has
- * been deleted.
- */
-#define REF_DELETED_LOOSE 0x200
+#define REF_HAVE_OLD (1 << 3)
 
 /*
  * Return the length of time to retry acquiring a loose reference lock
@@ -141,23 +105,22 @@ int copy_reflog_msg(char *buf, const char *msg);
  * not exist before update.
  */
 struct ref_update {
-
 	/*
-	 * If (flags & REF_HAVE_NEW), set the reference to this value:
+	 * If (flags & REF_HAVE_NEW), set the reference to this value
+	 * (or delete it, if `new_oid` is `null_oid`).
 	 */
 	struct object_id new_oid;
 
 	/*
 	 * If (flags & REF_HAVE_OLD), check that the reference
-	 * previously had this value:
+	 * previously had this value (or didn't previously exist, if
+	 * `old_oid` is `null_oid`).
 	 */
 	struct object_id old_oid;
 
 	/*
-	 * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
-	 * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY,
-	 * REF_UPDATE_VIA_HEAD, REF_NEEDS_COMMIT, and
-	 * REF_DELETED_LOOSE:
+	 * One or more of REF_NODEREF, REF_FORCE_CREATE_REFLOG,
+	 * REF_HAVE_NEW, REF_HAVE_OLD, or backend-specific flags.
 	 */
 	unsigned int flags;
 
-- 
2.14.1

[PATCH v2 6/9] refs: rename constant `REF_NODEREF` to `REF_NO_DEREF`

[Michael Haggerty]

Even after working with this code for years, I still see this constant
name as "ref node ref". Rename it to make it's meaning clearer.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/am.c           |  2 +-
 builtin/branch.c       |  2 +-
 builtin/checkout.c     |  2 +-
 builtin/clone.c        |  4 ++--
 builtin/notes.c        |  2 +-
 builtin/remote.c       |  6 +++---
 builtin/symbolic-ref.c |  2 +-
 builtin/update-ref.c   |  4 ++--
 refs.h                 |  6 +++---
 refs/files-backend.c   | 40 ++++++++++++++++++++--------------------
 refs/refs-internal.h   |  4 ++--
 sequencer.c            |  6 +++---
 12 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index c9bb14a6c2..894290e2d3 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2151,7 +2151,7 @@ static void am_abort(struct am_state *state)
 			   has_curr_head ? &curr_head : NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
 	else if (curr_branch)
-		delete_ref(NULL, curr_branch, NULL, REF_NODEREF);
+		delete_ref(NULL, curr_branch, NULL, REF_NO_DEREF);
 
 	free(curr_branch);
 	am_destroy(state);
diff --git a/builtin/branch.c b/builtin/branch.c
index b1ed649300..33fd5fcfd1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -258,7 +258,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		}
 
 		if (delete_ref(NULL, name, is_null_oid(&oid) ? NULL : &oid,
-			       REF_NODEREF)) {
+			       REF_NO_DEREF)) {
 			error(remote_branch
 			      ? _("Error deleting remote-tracking branch '%s'")
 			      : _("Error deleting branch '%s'"),
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 463a337e5d..114028ee01 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -665,7 +665,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 		/* Nothing to do. */
 	} else if (opts->force_detach || !new->path) {	/* No longer on any branch. */
 		update_ref(msg.buf, "HEAD", &new->commit->object.oid, NULL,
-			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+			   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);
 		if (!opts->quiet) {
 			if (old->path &&
 			    advice_detached_head && !opts->force_detach)
diff --git a/builtin/clone.c b/builtin/clone.c
index 695bdd7046..557c6c3c06 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -689,7 +689,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
 	} else if (our) {
 		struct commit *c = lookup_commit_reference(&our->old_oid);
 		/* --branch specifies a non-branch (i.e. tags), detach HEAD */
-		update_ref(msg, "HEAD", &c->object.oid, NULL, REF_NODEREF,
+		update_ref(msg, "HEAD", &c->object.oid, NULL, REF_NO_DEREF,
 			   UPDATE_REFS_DIE_ON_ERR);
 	} else if (remote) {
 		/*
@@ -697,7 +697,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		 * HEAD points to a branch but we don't know which one.
 		 * Detach HEAD in all these cases.
 		 */
-		update_ref(msg, "HEAD", &remote->old_oid, NULL, REF_NODEREF,
+		update_ref(msg, "HEAD", &remote->old_oid, NULL, REF_NO_DEREF,
 			   UPDATE_REFS_DIE_ON_ERR);
 	}
 }
diff --git a/builtin/notes.c b/builtin/notes.c
index 12afdf1907..d7754db143 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -686,7 +686,7 @@ static int merge_abort(struct notes_merge_options *o)
 
 	if (delete_ref(NULL, "NOTES_MERGE_PARTIAL", NULL, 0))
 		ret += error(_("failed to delete ref NOTES_MERGE_PARTIAL"));
-	if (delete_ref(NULL, "NOTES_MERGE_REF", NULL, REF_NODEREF))
+	if (delete_ref(NULL, "NOTES_MERGE_REF", NULL, REF_NO_DEREF))
 		ret += error(_("failed to delete ref NOTES_MERGE_REF"));
 	if (notes_merge_abort(o))
 		ret += error(_("failed to remove 'git notes merge' worktree"));
diff --git a/builtin/remote.c b/builtin/remote.c
index 0fddc64461..3d38c6150c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -693,7 +693,7 @@ static int mv(int argc, const char **argv)
 		read_ref_full(item->string, RESOLVE_REF_READING, &oid, &flag);
 		if (!(flag & REF_ISSYMREF))
 			continue;
-		if (delete_ref(NULL, item->string, NULL, REF_NODEREF))
+		if (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))
 			die(_("deleting '%s' failed"), item->string);
 	}
 	for (i = 0; i < remote_branches.nr; i++) {
@@ -788,7 +788,7 @@ static int rm(int argc, const char **argv)
 	strbuf_release(&buf);
 
 	if (!result)
-		result = delete_refs("remote: remove", &branches, REF_NODEREF);
+		result = delete_refs("remote: remove", &branches, REF_NO_DEREF);
 	string_list_clear(&branches, 0);
 
 	if (skipped.nr) {
@@ -1255,7 +1255,7 @@ static int set_head(int argc, const char **argv)
 			head_name = xstrdup(states.heads.items[0].string);
 		free_remote_ref_states(&states);
 	} else if (opt_d && !opt_a && argc == 1) {
-		if (delete_ref(NULL, buf.buf, NULL, REF_NODEREF))
+		if (delete_ref(NULL, buf.buf, NULL, REF_NO_DEREF))
 			result |= error(_("Could not delete %s"), buf.buf);
 	} else
 		usage_with_options(builtin_remote_sethead_usage, options);
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 17aabaa679..80237f0df1 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -58,7 +58,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 			die("Cannot delete %s, not a symbolic ref", argv[0]);
 		if (!strcmp(argv[0], "HEAD"))
 			die("deleting '%s' is not allowed", argv[0]);
-		return delete_ref(NULL, argv[0], NULL, REF_NODEREF);
+		return delete_ref(NULL, argv[0], NULL, REF_NO_DEREF);
 	}
 
 	switch (argc) {
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index cf1552b478..4b4714b3fd 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -312,7 +312,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 static const char *parse_cmd_option(struct strbuf *input, const char *next)
 {
 	if (!strncmp(next, "no-deref", 8) && next[8] == line_termination)
-		update_flags |= REF_NODEREF;
+		update_flags |= REF_NO_DEREF;
 	else
 		die("option unknown: %s", next);
 	return next + 8;
@@ -427,7 +427,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	}
 
 	if (no_deref)
-		flags = REF_NODEREF;
+		flags = REF_NO_DEREF;
 	if (delete)
 		/*
 		 * For purposes of backwards compatibility, we treat
diff --git a/refs.h b/refs.h
index 261d46c10c..d396012367 100644
--- a/refs.h
+++ b/refs.h
@@ -476,7 +476,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
  *         transaction.
  *
  *     flags -- flags affecting the update, passed to
- *         update_ref_lock(). Possible flags: REF_NODEREF,
+ *         update_ref_lock(). Possible flags: REF_NO_DEREF,
  *         REF_FORCE_CREATE_REFLOG. See those constants for more
  *         information.
  *
@@ -504,7 +504,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
  * If this flag is not specified, then symbolic references are
  * dereferenced and the update is applied to the referent.
  */
-#define REF_NODEREF (1 << 0)
+#define REF_NO_DEREF (1 << 0)
 
 /*
  * Force the creation of a reflog for this reference, even if it
@@ -517,7 +517,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
  * ref_transaction_update() and friends:
  */
 #define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \
-	(REF_NODEREF | REF_FORCE_CREATE_REFLOG)
+	(REF_NO_DEREF | REF_FORCE_CREATE_REFLOG)
 
 /*
  * Add a reference update to transaction. `new_oid` is the value that
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bbeafe1db7..71e088e811 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -13,14 +13,14 @@
 /*
  * This backend uses the following flags in `ref_update::flags` for
  * internal bookkeeping purposes. Their numerical values must not
- * conflict with REF_NODEREF, REF_FORCE_CREATE_REFLOG, REF_HAVE_NEW,
+ * conflict with REF_NO_DEREF, REF_FORCE_CREATE_REFLOG, REF_HAVE_NEW,
  * REF_HAVE_OLD, or REF_ISPRUNING, which are also stored in
  * `ref_update::flags`.
  */
 
 /*
  * Used as a flag in ref_update::flags when a loose ref is being
- * pruned. This flag must only be used when REF_NODEREF is set.
+ * pruned. This flag must only be used when REF_NO_DEREF is set.
  */
 #define REF_ISPRUNING (1 << 4)
 
@@ -1044,7 +1044,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
 		goto cleanup;
 	ref_transaction_add_update(
 			transaction, r->name,
-			REF_NODEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_ISPRUNING,
+			REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_ISPRUNING,
 			&null_oid, &r->oid, NULL);
 	if (ref_transaction_commit(transaction, &err))
 		goto cleanup;
@@ -1133,7 +1133,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 		 */
 		if (ref_transaction_update(transaction, iter->refname,
 					   iter->oid, NULL,
-					   REF_NODEREF, NULL, &err))
+					   REF_NO_DEREF, NULL, &err))
 			die("failure preparing to create packed reference %s: %s",
 			    iter->refname, err.buf);
 
@@ -1336,7 +1336,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	}
 
 	if (!copy && refs_delete_ref(&refs->base, logmsg, oldrefname,
-			    &orig_oid, REF_NODEREF)) {
+			    &orig_oid, REF_NO_DEREF)) {
 		error("unable to delete old %s", oldrefname);
 		goto rollback;
 	}
@@ -1352,7 +1352,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 				RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
 				&oid, NULL) &&
 	    refs_delete_ref(&refs->base, NULL, newrefname,
-			    NULL, REF_NODEREF)) {
+			    NULL, REF_NO_DEREF)) {
 		if (errno == EISDIR) {
 			struct strbuf path = STRBUF_INIT;
 			int result;
@@ -1377,7 +1377,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	logmoved = log;
 
 	lock = lock_ref_oid_basic(refs, newrefname, NULL, NULL, NULL,
-				  REF_NODEREF, NULL, &err);
+				  REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		if (copy)
 			error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1400,7 +1400,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
 
  rollback:
 	lock = lock_ref_oid_basic(refs, oldrefname, NULL, NULL, NULL,
-				  REF_NODEREF, NULL, &err);
+				  REF_NO_DEREF, NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
 		strbuf_release(&err);
@@ -1816,7 +1816,7 @@ static int files_create_symref(struct ref_store *ref_store,
 	int ret;
 
 	lock = lock_ref_oid_basic(refs, refname, NULL,
-				  NULL, NULL, REF_NODEREF, NULL,
+				  NULL, NULL, REF_NO_DEREF, NULL,
 				  &err);
 	if (!lock) {
 		error("%s", err.buf);
@@ -2200,7 +2200,7 @@ static int split_head_update(struct ref_update *update,
 
 	new_update = ref_transaction_add_update(
 			transaction, "HEAD",
-			update->flags | REF_LOG_ONLY | REF_NODEREF,
+			update->flags | REF_LOG_ONLY | REF_NO_DEREF,
 			&update->new_oid, &update->old_oid,
 			update->msg);
 
@@ -2219,8 +2219,8 @@ static int split_head_update(struct ref_update *update,
 
 /*
  * update is for a symref that points at referent and doesn't have
- * REF_NODEREF set. Split it into two updates:
- * - The original update, but with REF_LOG_ONLY and REF_NODEREF set
+ * REF_NO_DEREF set. Split it into two updates:
+ * - The original update, but with REF_LOG_ONLY and REF_NO_DEREF set
  * - A new, separate update for the referent reference
  * Note that the new update will itself be subject to splitting when
  * the iteration gets to it.
@@ -2275,7 +2275,7 @@ static int split_symref_update(struct files_ref_store *refs,
 	 * doesn't need to check its old SHA-1 value, as that will be
 	 * done when new_update is processed.
 	 */
-	update->flags |= REF_LOG_ONLY | REF_NODEREF;
+	update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
 	update->flags &= ~REF_HAVE_OLD;
 
 	/*
@@ -2344,7 +2344,7 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid,
  * - Check that its old SHA-1 value (if specified) is correct, and in
  *   any case record it in update->lock->old_oid for later use when
  *   writing the reflog.
- * - If it is a symref update without REF_NODEREF, split it up into a
+ * - If it is a symref update without REF_NO_DEREF, split it up into a
  *   REF_LOG_ONLY update of the symref and add a separate update for
  *   the referent to transaction.
  * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY
@@ -2392,7 +2392,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 	update->backend_data = lock;
 
 	if (update->type & REF_ISSYMREF) {
-		if (update->flags & REF_NODEREF) {
+		if (update->flags & REF_NO_DEREF) {
 			/*
 			 * We won't be reading the referent as part of
 			 * the transaction, so we have to read it here
@@ -2564,7 +2564,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 	 * split_symref_update() or split_head_update(), those
 	 * functions will check that the new updates don't have the
 	 * same refname as any existing ones.) Also fail if any of the
-	 * updates use REF_ISPRUNING without REF_NODEREF.
+	 * updates use REF_ISPRUNING without REF_NO_DEREF.
 	 */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
@@ -2572,8 +2572,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 			string_list_append(&affected_refnames, update->refname);
 
 		if ((update->flags & REF_ISPRUNING) &&
-		    !(update->flags & REF_NODEREF))
-			BUG("REF_ISPRUNING set without REF_NODEREF");
+		    !(update->flags & REF_NO_DEREF))
+			BUG("REF_ISPRUNING set without REF_NO_DEREF");
 
 		/*
 		 * We store a pointer to update in item->util, but at
@@ -2651,7 +2651,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 
 			ref_transaction_add_update(
 					packed_transaction, update->refname,
-					REF_HAVE_NEW | REF_NODEREF,
+					REF_HAVE_NEW | REF_NO_DEREF,
 					&update->new_oid, NULL,
 					NULL);
 		}
@@ -2995,7 +2995,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	 * reference if --updateref was specified:
 	 */
 	lock = lock_ref_oid_basic(refs, refname, oid,
-				  NULL, NULL, REF_NODEREF,
+				  NULL, NULL, REF_NO_DEREF,
 				  &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 2ea02dcbe4..f9c6e72c97 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -9,7 +9,7 @@
 
 /*
  * The following flags can appear in `ref_update::flags`. Their
- * numerical values must not conflict with those of REF_NODEREF and
+ * numerical values must not conflict with those of REF_NO_DEREF and
  * REF_FORCE_CREATE_REFLOG, which are also stored in
  * `ref_update::flags`.
  */
@@ -119,7 +119,7 @@ struct ref_update {
 	struct object_id old_oid;
 
 	/*
-	 * One or more of REF_NODEREF, REF_FORCE_CREATE_REFLOG,
+	 * One or more of REF_NO_DEREF, REF_FORCE_CREATE_REFLOG,
 	 * REF_HAVE_NEW, REF_HAVE_OLD, or backend-specific flags.
 	 */
 	unsigned int flags;
diff --git a/sequencer.c b/sequencer.c
index 64abaad0e8..3b88ab2f9c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1116,11 +1116,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 	 */
 	if (command == TODO_PICK && !opts->no_commit && (res == 0 || res == 1) &&
 	    update_ref(NULL, "CHERRY_PICK_HEAD", &commit->object.oid, NULL,
-		       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+		       REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))
 		res = -1;
 	if (command == TODO_REVERT && ((opts->no_commit && res == 0) || res == 1) &&
 	    update_ref(NULL, "REVERT_HEAD", &commit->object.oid, NULL,
-		       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+		       REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))
 		res = -1;
 
 	if (res) {
@@ -2125,7 +2125,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 			msg = reflog_message(opts, "finish", "%s onto %s",
 				head_ref.buf, buf.buf);
 			if (update_ref(msg, head_ref.buf, &head, &orig,
-				       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) {
+				       REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR)) {
 				res = error(_("could not update %s"),
 					head_ref.buf);
 				goto cleanup_head_ref;
-- 
2.14.1

[PATCH v2 7/9] refs: rename constant `REF_ISPRUNING` to `REF_IS_PRUNING`

[Michael Haggerty]

Underscores are cheap, and help readability.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 71e088e811..bb10b715a8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -14,7 +14,7 @@
  * This backend uses the following flags in `ref_update::flags` for
  * internal bookkeeping purposes. Their numerical values must not
  * conflict with REF_NO_DEREF, REF_FORCE_CREATE_REFLOG, REF_HAVE_NEW,
- * REF_HAVE_OLD, or REF_ISPRUNING, which are also stored in
+ * REF_HAVE_OLD, or REF_IS_PRUNING, which are also stored in
  * `ref_update::flags`.
  */
 
@@ -22,7 +22,7 @@
  * Used as a flag in ref_update::flags when a loose ref is being
  * pruned. This flag must only be used when REF_NO_DEREF is set.
  */
-#define REF_ISPRUNING (1 << 4)
+#define REF_IS_PRUNING (1 << 4)
 
 /*
  * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken
@@ -1044,7 +1044,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
 		goto cleanup;
 	ref_transaction_add_update(
 			transaction, r->name,
-			REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_ISPRUNING,
+			REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_IS_PRUNING,
 			&null_oid, &r->oid, NULL);
 	if (ref_transaction_commit(transaction, &err))
 		goto cleanup;
@@ -2177,7 +2177,7 @@ static int split_head_update(struct ref_update *update,
 	struct ref_update *new_update;
 
 	if ((update->flags & REF_LOG_ONLY) ||
-	    (update->flags & REF_ISPRUNING) ||
+	    (update->flags & REF_IS_PRUNING) ||
 	    (update->flags & REF_UPDATE_VIA_HEAD))
 		return 0;
 
@@ -2564,16 +2564,16 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 	 * split_symref_update() or split_head_update(), those
 	 * functions will check that the new updates don't have the
 	 * same refname as any existing ones.) Also fail if any of the
-	 * updates use REF_ISPRUNING without REF_NO_DEREF.
+	 * updates use REF_IS_PRUNING without REF_NO_DEREF.
 	 */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
 		struct string_list_item *item =
 			string_list_append(&affected_refnames, update->refname);
 
-		if ((update->flags & REF_ISPRUNING) &&
+		if ((update->flags & REF_IS_PRUNING) &&
 		    !(update->flags & REF_NO_DEREF))
-			BUG("REF_ISPRUNING set without REF_NO_DEREF");
+			BUG("REF_IS_PRUNING set without REF_NO_DEREF");
 
 		/*
 		 * We store a pointer to update in item->util, but at
@@ -2632,7 +2632,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY) &&
-		    !(update->flags & REF_ISPRUNING)) {
+		    !(update->flags & REF_IS_PRUNING)) {
 			/*
 			 * This reference has to be deleted from
 			 * packed-refs if it exists there.
@@ -2749,7 +2749,7 @@ static int files_transaction_finish(struct ref_store *ref_store,
 		struct ref_update *update = transaction->updates[i];
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY) &&
-		    !(update->flags & REF_ISPRUNING)) {
+		    !(update->flags & REF_IS_PRUNING)) {
 			strbuf_reset(&sb);
 			files_reflog_path(refs, &sb, update->refname);
 			if (!unlink_or_warn(sb.buf))
-- 
2.14.1

[PATCH v2 8/9] write_packed_entry(): take `object_id` arguments

[Michael Haggerty]

Change `write_packed_entry()` to take `struct object_id *` rather than
`unsigned char *` arguments.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/packed-backend.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 74f1dea0f4..43ad74fc5a 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -961,11 +961,11 @@ static struct ref_iterator *packed_ref_iterator_begin(
  * by the failing call to `fprintf()`.
  */
 static int write_packed_entry(FILE *fh, const char *refname,
-			      const unsigned char *sha1,
-			      const unsigned char *peeled)
+			      const struct object_id *oid,
+			      const struct object_id *peeled)
 {
-	if (fprintf(fh, "%s %s\n", sha1_to_hex(sha1), refname) < 0 ||
-	    (peeled && fprintf(fh, "^%s\n", sha1_to_hex(peeled)) < 0))
+	if (fprintf(fh, "%s %s\n", oid_to_hex(oid), refname) < 0 ||
+	    (peeled && fprintf(fh, "^%s\n", oid_to_hex(peeled)) < 0))
 		return -1;
 
 	return 0;
@@ -1203,8 +1203,8 @@ static int write_with_updates(struct packed_ref_store *refs,
 			int peel_error = ref_iterator_peel(iter, &peeled);
 
 			if (write_packed_entry(out, iter->refname,
-					       iter->oid->hash,
-					       peel_error ? NULL : peeled.hash))
+					       iter->oid,
+					       peel_error ? NULL : &peeled))
 				goto write_error;
 
 			if ((ok = ref_iterator_advance(iter)) != ITER_OK)
@@ -1224,8 +1224,8 @@ static int write_with_updates(struct packed_ref_store *refs,
 						     &peeled);
 
 			if (write_packed_entry(out, update->refname,
-					       update->new_oid.hash,
-					       peel_error ? NULL : peeled.hash))
+					       &update->new_oid,
+					       peel_error ? NULL : &peeled))
 				goto write_error;
 
 			i++;
-- 
2.14.1

[PATCH v2 9/9] refs: update some more docs to use "oid" rather than "sha1"

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c                |  2 +-
 refs.h                |  8 ++++----
 refs/files-backend.c  | 19 +++++++++----------
 refs/packed-backend.c |  2 +-
 refs/ref-cache.c      |  4 ++--
 refs/refs-internal.h  | 18 +++++++++---------
 6 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/refs.c b/refs.c
index 0d9a1348cd..339d4318ee 100644
--- a/refs.c
+++ b/refs.c
@@ -770,7 +770,7 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
 		if (cb->cutoff_cnt)
 			*cb->cutoff_cnt = cb->reccnt - 1;
 		/*
-		 * we have not yet updated cb->[n|o]sha1 so they still
+		 * we have not yet updated cb->[n|o]oid so they still
 		 * hold the values for the previous record.
 		 */
 		if (!is_null_oid(&cb->ooid)) {
diff --git a/refs.h b/refs.h
index d396012367..18582a408c 100644
--- a/refs.h
+++ b/refs.h
@@ -126,7 +126,7 @@ int peel_ref(const char *refname, struct object_id *oid);
 /**
  * Resolve refname in the nested "gitlink" repository in the specified
  * submodule (which must be non-NULL). If the resolution is
- * successful, return 0 and set sha1 to the name of the object;
+ * successful, return 0 and set oid to the name of the object;
  * otherwise, return a non-zero value.
  */
 int resolve_gitlink_ref(const char *submodule, const char *refname,
@@ -260,7 +260,7 @@ struct ref_transaction;
 
 /*
  * The signature for the callback function for the for_each_*()
- * functions below.  The memory pointed to by the refname and sha1
+ * functions below.  The memory pointed to by the refname and oid
  * arguments is only guaranteed to be valid for the duration of a
  * single callback invocation.
  */
@@ -354,7 +354,7 @@ int reflog_exists(const char *refname);
 
 /*
  * Delete the specified reference. If old_oid is non-NULL, then
- * verify that the current value of the reference is old_sha1 before
+ * verify that the current value of the reference is old_oid before
  * 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
@@ -633,7 +633,7 @@ int ref_transaction_abort(struct ref_transaction *transaction,
  * It is a bug to call this function when there might be other
  * processes accessing the repository or if there are existing
  * references that might conflict with the ones being created. All
- * old_sha1 values must either be absent or NULL_SHA1.
+ * old_oid values must either be absent or null_oid.
  */
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
 				   struct strbuf *err);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bb10b715a8..2298f900dd 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -240,7 +240,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 			} else if (is_null_oid(&oid)) {
 				/*
 				 * It is so astronomically unlikely
-				 * that NULL_SHA1 is the SHA-1 of an
+				 * that null_oid is the OID of an
 				 * actual object that we consider its
 				 * appearance in a loose reference
 				 * file to be repo corruption
@@ -473,7 +473,7 @@ static void unlock_ref(struct ref_lock *lock)
  * are passed to refs_verify_refname_available() for this check.
  *
  * If mustexist is not set and the reference is not found or is
- * broken, lock the reference anyway but clear sha1.
+ * broken, lock the reference anyway but clear old_oid.
  *
  * Return 0 on success. On failure, write an error message to err and
  * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR.
@@ -1648,9 +1648,8 @@ static int files_log_ref_write(struct files_ref_store *refs,
 }
 
 /*
- * Write sha1 into the open lockfile, then close the lockfile. On
- * errors, rollback the lockfile, fill in *err and
- * return -1.
+ * Write oid into the open lockfile, then close the lockfile. On
+ * errors, rollback the lockfile, fill in *err and return -1.
  */
 static int write_ref_to_lockfile(struct ref_lock *lock,
 				 const struct object_id *oid, struct strbuf *err)
@@ -2272,7 +2271,7 @@ static int split_symref_update(struct files_ref_store *refs,
 
 	/*
 	 * Change the symbolic ref update to log only. Also, it
-	 * doesn't need to check its old SHA-1 value, as that will be
+	 * doesn't need to check its old OID value, as that will be
 	 * done when new_update is processed.
 	 */
 	update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
@@ -2341,7 +2340,7 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid,
  * Prepare for carrying out update:
  * - Lock the reference referred to by update.
  * - Read the reference under lock.
- * - Check that its old SHA-1 value (if specified) is correct, and in
+ * - Check that its old OID value (if specified) is correct, and in
  *   any case record it in update->lock->old_oid for later use when
  *   writing the reflog.
  * - If it is a symref update without REF_NO_DEREF, split it up into a
@@ -2396,7 +2395,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 			/*
 			 * We won't be reading the referent as part of
 			 * the transaction, so we have to read it here
-			 * to record and possibly check old_sha1:
+			 * to record and possibly check old_oid:
 			 */
 			if (refs_read_ref_full(&refs->base,
 					       referent.buf, 0,
@@ -2416,7 +2415,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 			/*
 			 * Create a new update for the reference this
 			 * symref is pointing at. Also, we will record
-			 * and verify old_sha1 for this update as part
+			 * and verify old_oid for this update as part
 			 * of processing the split-off update, so we
 			 * don't have to do it here.
 			 */
@@ -2436,7 +2435,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 
 		/*
 		 * If this update is happening indirectly because of a
-		 * symref update, record the old SHA-1 in the parent
+		 * symref update, record the old OID in the parent
 		 * update:
 		 */
 		for (parent_update = update->parent_update;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 43ad74fc5a..72164a1d64 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -744,7 +744,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,
 /*
  * This value is set in `base.flags` if the peeled value of the
  * current reference is known. In that case, `peeled` contains the
- * correct peeled value for the reference, which might be `null_sha1`
+ * correct peeled value for the reference, which might be `null_oid`
  * if the reference is not a tag or if it is broken.
  */
 #define REF_KNOWS_PEELED 0x40
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index 043eb83748..82c1cf90a7 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -260,8 +260,8 @@ int add_ref_entry(struct ref_dir *dir, struct ref_entry *ref)
 
 /*
  * Emit a warning and return true iff ref1 and ref2 have the same name
- * and the same sha1.  Die if they have the same name but different
- * sha1s.
+ * and the same oid. Die if they have the same name but different
+ * oids.
  */
 static int is_dup_ref(const struct ref_entry *ref1, const struct ref_entry *ref2)
 {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index f9c6e72c97..dd834314bd 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -15,13 +15,13 @@
  */
 
 /*
- * The reference should be updated to new_sha1.
+ * The reference should be updated to new_oid.
  */
 #define REF_HAVE_NEW (1 << 2)
 
 /*
  * The current reference's value should be checked to make sure that
- * it agrees with old_sha1.
+ * it agrees with old_oid.
  */
 #define REF_HAVE_OLD (1 << 3)
 
@@ -86,7 +86,7 @@ enum peel_status {
  * tag recursively until a non-tag is found.  If successful, store the
  * result to oid and return PEEL_PEELED.  If the object is not a tag
  * or is not valid, return PEEL_NON_TAG or PEEL_INVALID, respectively,
- * and leave sha1 unchanged.
+ * and leave oid unchanged.
  */
 enum peel_status peel_object(const struct object_id *name, struct object_id *oid);
 
@@ -98,11 +98,11 @@ enum peel_status peel_object(const struct object_id *name, struct object_id *oid
 int copy_reflog_msg(char *buf, const char *msg);
 
 /**
- * Information needed for a single ref update. Set new_sha1 to the new
- * value or to null_sha1 to delete the ref. To check the old value
- * while the ref is locked, set (flags & REF_HAVE_OLD) and set
- * old_sha1 to the old value, or to null_sha1 to ensure the ref does
- * not exist before update.
+ * Information needed for a single ref update. Set new_oid to the new
+ * value or to null_oid to delete the ref. To check the old value
+ * while the ref is locked, set (flags & REF_HAVE_OLD) and set old_oid
+ * to the old value, or to null_oid to ensure the ref does not exist
+ * before update.
  */
 struct ref_update {
 	/*
@@ -158,7 +158,7 @@ int ref_update_reject_duplicates(struct string_list *refnames,
 /*
  * Add a ref_update with the specified properties to transaction, and
  * return a pointer to the new object. This function does not verify
- * that refname is well-formed. new_sha1 and old_sha1 are only
+ * that refname is well-formed. new_oid and old_oid are only
  * dereferenced if the REF_HAVE_NEW and REF_HAVE_OLD bits,
  * respectively, are set in flags.
  */
-- 
2.14.1

Re: [PATCH v2 3/9] ref_transaction_update(): die on disallowed flags

[Martin Ågren]

On 5 November 2017 at 09:42, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Callers shouldn't be passing disallowed flags into
> `ref_transaction_update()`. So instead of masking them off, treat it
> as a bug if any are set.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 62a7621025..7c1e206e08 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -940,7 +940,8 @@ int ref_transaction_update(struct ref_transaction *transaction,
>                 return -1;
>         }
>
> -       flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
> +       if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
> +               BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
>
>         flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);

The masking out is for sanity, but also partly to squelch a
compiler-warning. Thomas reported [1] that dieing does not make the
warning go away, but that masking out does. Of course, avoiding warnings
is not the ultimate goal, and -Wnonnull is not part of DEVELOPER_CFLAGS.
Thomas reluctantly suggested that one could do your check and then do
the masking...

Maybe it would be worth a note in the commit message. But blaming these
lines quickly leads to c788c54cd (refs: strip out not allowed flags from
ref_transaction_update, 2017-09-12), which describes this already. OTOH,
since the warning does not hit these lines, but a bit below, maybe it's
even worth a comment in the code.

I'm not saying we should sprinkle comments for each warning we hit...
Anyway, those were the thoughts than ran through my mind.

[1] https://public-inbox.org/git/20170924204541.GA2853@hank/

Re: [PATCH v2 3/9] ref_transaction_update(): die on disallowed flags

[Thomas Gummerer]

On 11/07, Martin Ågren wrote:
> On 5 November 2017 at 09:42, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> > Callers shouldn't be passing disallowed flags into
> > `ref_transaction_update()`. So instead of masking them off, treat it
> > as a bug if any are set.
> >
> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> > ---
> >  refs.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/refs.c b/refs.c
> > index 62a7621025..7c1e206e08 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -940,7 +940,8 @@ int ref_transaction_update(struct ref_transaction *transaction,
> >                 return -1;
> >         }
> >
> > -       flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
> > +       if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
> > +               BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
> >
> >         flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
> 
> The masking out is for sanity, but also partly to squelch a
> compiler-warning. Thomas reported [1] that dieing does not make the
> warning go away, but that masking out does. Of course, avoiding warnings
> is not the ultimate goal, and -Wnonnull is not part of DEVELOPER_CFLAGS.
> Thomas reluctantly suggested that one could do your check and then do
> the masking...

Thanks for doing the digging and cc'ing me here.

Interestingly enough  my compiler no longer complains about this.
Looking at what changed, gcc got upgraded to 7.2.0 (from 7.1.1) in
between then and now, but I can still reproduce this if I base this
patch on top of the old commit.

So bisecting leads me to 89f3bbdd3b ("refs: update ref transactions to
use struct object_id", 2017-10-15).  It's too late for me to dig in to
what in this made the compiler warning go away, but it seems
reasonable enough.

With that said I think the patch we have here is definitely an
improvement, I would have prefered to do it this way in the first
place, but the compiler was standing in my way :)

Thanks Michael for cleaning this up!

> Maybe it would be worth a note in the commit message. But blaming these
> lines quickly leads to c788c54cd (refs: strip out not allowed flags from
> ref_transaction_update, 2017-09-12), which describes this already. OTOH,
> since the warning does not hit these lines, but a bit below, maybe it's
> even worth a comment in the code.
> 
> I'm not saying we should sprinkle comments for each warning we hit...
> Anyway, those were the thoughts than ran through my mind.
> 
> [1] https://public-inbox.org/git/20170924204541.GA2853@hank/