[PATCH v2 00/25] Prepare to separate out a packed_ref_store

[PATCH v2 00/25] Prepare to separate out a packed_ref_store

[Michael Haggerty]

This is the second iteration of a patch series that prepares the
ground for separating out a `packed_ref_store` and then for changing
`packed-refs` to be read using `mmap()`. Thanks to Peff, Junio,
Stefan, Brandon, and Johannes for their feedback about v1 [1]. I think
I have addressed all of your comments.

Changes since v1:

* Since v1, branch `bc/object-id` has been merged to `next`, and it
  has lots of conflicts with these changes. So I rebased this branch
  onto a merge of `master` and `bc/object-id`. (I hope this makes
  Junio's job easier.) This unfortunately causes a bit of tbdiff noise
  between v1 and v2.

* Patch [01/25]: in t3600, register the `test_when_finished` command
  before executing `chmod a-w`.

* Patch [04/25] (new patch): convert a few `die("internal error: ...")`
  to `die("BUG: ...")`.

* Patch [05/25]: Use `strlen()` rather than `memchr()` to check the
  trim length, and `die()` rather than skipping if it is longer than
  the reference name.

* Patch [08/25]: Name the log message arguments `msg` for consistency
  with existing code.

* Patch [10/25]: Rename the new member from `packlock` to
  `packed_refs_lock`.

* Patch [13/25] (new patch): Move the check for valid
  `transaction->state` from `files_transaction_commit()` to
  `ref_transaction_commit()`.

* Patch [14/25]:

  * Add more sanity checks of `transaction->state`.

  * Don't add `ref_transaction_finish()` to the public API. Instead,
    teach `ref_transaction_commit()` to do the right thing whether or
    not `ref_transaction_prepare()` has been called.

  * Add and improve docstrings.

  * Allow `ref_transaction_abort()` to be called even before
    `ref_transaction_prepare()` (in which case it just calls
    `ref_transaction_free()`).

* Lots of improvements to commit messages and comments, mostly to
  clarify points that reviewers asked about.

These changes (along with the merge commit that they are based on) are
also available as branch `packed-ref-store-prep` in my GitHub fork
[2]. If you'd like to see a preview of the rest of the changes (which
works but is not yet polished), checkout the `mmap-packed-refs` branch
from the same place.

Michael

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

Jeff King (1):
  ref-filter: limit traversal to prefix

Michael Haggerty (24):
  t3600: clean up permissions test properly
  refs.h: clarify docstring for the ref_transaction_update()-related fns
  ref_iterator_begin_fn(): fix docstring
  files-backend: use `die("BUG: ...")`, not `die("internal error: ...")`
  prefix_ref_iterator: don't trim too much
  refs_ref_iterator_begin(): don't check prefixes redundantly
  refs: use `size_t` indexes when iterating over ref transaction updates
  ref_store: take a `msg` parameter when deleting references
  lockfile: add a new method, is_lock_file_locked()
  files-backend: move `lock` member to `files_ref_store`
  files_ref_store: put the packed files lock directly in this struct
  files_transaction_cleanup(): new helper function
  ref_transaction_commit(): check for valid `transaction->state`
  ref_transaction_prepare(): new optional step for reference updates
  ref_update_reject_duplicates(): expose function to whole refs module
  ref_update_reject_duplicates(): use `size_t` rather than `int`
  ref_update_reject_duplicates(): add a sanity check
  should_pack_ref(): new function, extracted from `files_pack_refs()`
  get_packed_ref_cache(): assume "packed-refs" won't change while locked
  read_packed_refs(): do more of the work of reading packed refs
  read_packed_refs(): report unexpected fopen() failures
  refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA`
  create_ref_entry(): remove `check_name` option
  cache_ref_iterator_begin(): avoid priming unneeded directories

 builtin/fetch.c                |   2 +-
 builtin/remote.c               |   4 +-
 lockfile.h                     |   8 ++
 ref-filter.c                   |  64 ++++++++-
 refs.c                         | 135 ++++++++++++++++--
 refs.h                         | 149 +++++++++++++++-----
 refs/files-backend.c           | 302 +++++++++++++++++++++++++----------------
 refs/iterator.c                |  18 ++-
 refs/ref-cache.c               | 100 ++++++++++++--
 refs/ref-cache.h               |   6 +-
 refs/refs-internal.h           |  62 +++++++--
 t/helper/test-ref-store.c      |   3 +-
 t/t1405-main-ref-store.sh      |   2 +-
 t/t1406-submodule-ref-store.sh |   2 +-
 t/t3600-rm.sh                  |   4 +-
 15 files changed, 658 insertions(+), 203 deletions(-)

-- 
2.11.0

[PATCH v2 01/25] t3600: clean up permissions test properly

[Michael Haggerty]

The test of failing `git rm -f` removes the write permissions on the
test directory, but fails to restore them if the test fails. This
means that the test temporary directory cannot be cleaned up, which
means that subsequent attempts to run the test fail mysteriously.

Instead, do the cleanup in a `test_when_finished` block so that it
can't be skipped.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3600-rm.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5f9913ba33..f8568f8841 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -97,9 +97,9 @@ test_expect_success FUNNYNAMES \
 embedded'"
 
 test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
+	test_when_finished "chmod 775 ." &&
 	chmod a-w . &&
-	test_must_fail git rm -f baz &&
-	chmod 775 .
+	test_must_fail git rm -f baz
 '
 
 test_expect_success \
-- 
2.11.0

[PATCH v2 02/25] refs.h: clarify docstring for the ref_transaction_update()-related fns

[Michael Haggerty]

In particular, make it clear that they make copies of the sha1
arguments.

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

diff --git a/refs.h b/refs.h
index 685a979a0e..ec8c6bfbbb 100644
--- a/refs.h
+++ b/refs.h
@@ -427,6 +427,19 @@ 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
+ *         NULL, meaning that the reference is not changed, or
+ *         null_sha1, 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
+ *         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
+ *         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.
-- 
2.11.0

[PATCH v2 03/25] ref_iterator_begin_fn(): fix docstring

[Michael Haggerty]

The iterator returned by this function only includes references whose
names start with the whole prefix, not all of those in
`find_containing_dir(prefix)` as the old docstring claimed. This
docstring was probably copy-pasted from old ref-cache code, which had
the old specification. But now, `cache_ref_iterator_begin()`
(from which the files reference iterator gets its values)
automatically wraps its output using `prefix_ref_iterator_begin()`
when necessary, so it has the stricter behavior.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/refs-internal.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index b6b291cf00..7020e51cb7 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -515,9 +515,10 @@ typedef int rename_ref_fn(struct ref_store *ref_store,
 			  const char *logmsg);
 
 /*
- * Iterate over the references in the specified ref_store that are
- * within find_containing_dir(prefix). If prefix is NULL or the empty
- * string, iterate over all references in the submodule.
+ * Iterate over the references in `ref_store` whose names start with
+ * `prefix`. `prefix` is matched as a literal string, without regard
+ * for path separators. If prefix is NULL or the empty string, iterate
+ * over all references in `ref_store`.
  */
 typedef struct ref_iterator *ref_iterator_begin_fn(
 		struct ref_store *ref_store,
-- 
2.11.0

[PATCH v2 04/25] files-backend: use `die("BUG: ...")`, not `die("internal error: ...")`

[Michael Haggerty]

The former is by far more common in our codebase.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index cb1f528cbe..fa5d2b6f08 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -105,7 +105,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)
 		struct packed_ref_cache *packed_refs = refs->packed;
 
 		if (packed_refs->lock)
-			die("internal error: packed-ref cache cleared while locked");
+			die("BUG: packed-ref cache cleared while locked");
 		refs->packed = NULL;
 		release_packed_ref_cache(packed_refs);
 	}
@@ -397,7 +397,7 @@ static void add_packed_ref(struct files_ref_store *refs,
 	struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
 
 	if (!packed_ref_cache->lock)
-		die("internal error: packed refs not locked");
+		die("BUG: packed refs not locked");
 	add_ref_entry(get_packed_ref_dir(packed_ref_cache),
 		      create_ref_entry(refname, oid, REF_ISPACKED, 1));
 }
@@ -1324,7 +1324,7 @@ static int commit_packed_refs(struct files_ref_store *refs)
 	files_assert_main_repository(refs, "commit_packed_refs");
 
 	if (!packed_ref_cache->lock)
-		die("internal error: packed-refs not locked");
+		die("BUG: packed-refs not locked");
 
 	out = fdopen_lock_file(packed_ref_cache->lock, "w");
 	if (!out)
@@ -1367,7 +1367,7 @@ static void rollback_packed_refs(struct files_ref_store *refs)
 	files_assert_main_repository(refs, "rollback_packed_refs");
 
 	if (!packed_ref_cache->lock)
-		die("internal error: packed-refs not locked");
+		die("BUG: packed-refs not locked");
 	rollback_lock_file(packed_ref_cache->lock);
 	packed_ref_cache->lock = NULL;
 	release_packed_ref_cache(packed_ref_cache);
-- 
2.11.0

[PATCH v2 05/25] prefix_ref_iterator: don't trim too much

[Michael Haggerty]

The `trim` parameter can be set independently of `prefix`. So if some
caller were to set `trim` to be greater than `strlen(prefix)`, we
could end up pointing the `refname` field of the iterator past the NUL
of the actual reference name string.

That can't happen currently, because `trim` is always set either to
zero or to `strlen(prefix)`. But even the latter could lead to
confusion, if a refname is exactly equal to the prefix, because then
we would set the outgoing `refname` to the empty string.

And we're about to decouple the `prefix` and `trim` arguments even
more, so let's be cautious here. Report a bug if ever asked to trim a
reference whose name is not longer than `trim`.

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

diff --git a/refs/iterator.c b/refs/iterator.c
index bce1f192f7..4cf449ef66 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -292,7 +292,23 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)
 		if (!starts_with(iter->iter0->refname, iter->prefix))
 			continue;
 
-		iter->base.refname = iter->iter0->refname + iter->trim;
+		if (iter->trim) {
+			/*
+			 * It is nonsense to trim off characters that
+			 * you haven't already checked for via a
+			 * prefix check, whether via this
+			 * `prefix_ref_iterator` or upstream in
+			 * `iter0`). So if there wouldn't be at least
+			 * one character left in the refname after
+			 * trimming, report it as a bug:
+			 */
+			if (strlen(iter->iter0->refname) <= iter->trim)
+				die("BUG: attempt to trim too many characters");
+			iter->base.refname = iter->iter0->refname + iter->trim;
+		} else {
+			iter->base.refname = iter->iter0->refname;
+		}
+
 		iter->base.oid = iter->iter0->oid;
 		iter->base.flags = iter->iter0->flags;
 		return ITER_OK;
-- 
2.11.0

[PATCH v2 06/25] refs_ref_iterator_begin(): don't check prefixes redundantly

[Michael Haggerty]

The backend already correctly restricts its output to references whose
names start with the prefix. By passing the prefix again to
`prefix_ref_iterator`, we were forcing that iterator to do redundant
prefix comparisons. So set it to the empty string.

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

diff --git a/refs.c b/refs.c
index 8af9641aa1..8494b1f20d 100644
--- a/refs.c
+++ b/refs.c
@@ -1247,7 +1247,13 @@ struct ref_iterator *refs_ref_iterator_begin(
 	struct ref_iterator *iter;
 
 	iter = refs->be->iterator_begin(refs, prefix, flags);
-	iter = prefix_ref_iterator_begin(iter, prefix, trim);
+
+	/*
+	 * `iterator_begin()` already takes care of prefix, but we
+	 * might need to do some trimming:
+	 */
+	if (trim)
+		iter = prefix_ref_iterator_begin(iter, "", trim);
 
 	return iter;
 }
-- 
2.11.0

[PATCH v2 07/25] refs: use `size_t` indexes when iterating over ref transaction updates

[Michael Haggerty]

Eliminate any chance of integer overflow on platforms where the two
types have different sizes.

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

diff --git a/refs.c b/refs.c
index 8494b1f20d..71139ba74e 100644
--- a/refs.c
+++ b/refs.c
@@ -848,7 +848,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err)
 
 void ref_transaction_free(struct ref_transaction *transaction)
 {
-	int i;
+	size_t i;
 
 	if (!transaction)
 		return;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index fa5d2b6f08..b2559b5585 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2850,7 +2850,8 @@ static int files_transaction_commit(struct ref_store *ref_store,
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_WRITE,
 			       "ref_transaction_commit");
-	int ret = 0, i;
+	size_t i;
+	int ret = 0;
 	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
 	struct string_list_item *ref_to_delete;
 	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
@@ -3057,7 +3058,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_WRITE,
 			       "initial_ref_transaction_commit");
-	int ret = 0, i;
+	size_t i;
+	int ret = 0;
 	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
 
 	assert(err);
-- 
2.11.0

[PATCH v2 08/25] ref_store: take a `msg` parameter when deleting references

[Michael Haggerty]

Just because the files backend can't retain reflogs for deleted
references is no reason that they shouldn't be supported by the
virtual method interface. Also, `delete_ref()` and `refs_delete_ref()`
have already gained `msg` parameters. Now let's add them to
`delete_refs()` and `refs_delete_refs()`.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch.c                |  2 +-
 builtin/remote.c               |  4 ++--
 refs.c                         | 11 ++++++-----
 refs.h                         | 12 +++++++-----
 refs/files-backend.c           |  4 ++--
 refs/refs-internal.h           |  2 +-
 t/helper/test-ref-store.c      |  3 ++-
 t/t1405-main-ref-store.sh      |  2 +-
 t/t1406-submodule-ref-store.sh |  2 +-
 9 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index d4d573b985..47708451bc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -941,7 +941,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
 		for (ref = stale_refs; ref; ref = ref->next)
 			string_list_append(&refnames, ref->name);
 
-		result = delete_refs(&refnames, 0);
+		result = delete_refs("fetch: prune", &refnames, 0);
 		string_list_clear(&refnames, 0);
 	}
 
diff --git a/builtin/remote.c b/builtin/remote.c
index addf97ad29..5f52c5a6d6 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -786,7 +786,7 @@ static int rm(int argc, const char **argv)
 	strbuf_release(&buf);
 
 	if (!result)
-		result = delete_refs(&branches, REF_NODEREF);
+		result = delete_refs("remote: remove", &branches, REF_NODEREF);
 	string_list_clear(&branches, 0);
 
 	if (skipped.nr) {
@@ -1301,7 +1301,7 @@ static int prune_remote(const char *remote, int dry_run)
 	string_list_sort(&refs_to_prune);
 
 	if (!dry_run)
-		result |= delete_refs(&refs_to_prune, 0);
+		result |= delete_refs("remote: prune", &refs_to_prune, 0);
 
 	for_each_string_list_item(item, &states.stale) {
 		const char *refname = item->util;
diff --git a/refs.c b/refs.c
index 71139ba74e..989462c972 100644
--- a/refs.c
+++ b/refs.c
@@ -1902,15 +1902,16 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 	return refs->be->initial_transaction_commit(refs, transaction, err);
 }
 
-int refs_delete_refs(struct ref_store *refs, struct string_list *refnames,
-		     unsigned int flags)
+int refs_delete_refs(struct ref_store *refs, const char *msg,
+		     struct string_list *refnames, unsigned int flags)
 {
-	return refs->be->delete_refs(refs, refnames, flags);
+	return refs->be->delete_refs(refs, msg, refnames, flags);
 }
 
-int delete_refs(struct string_list *refnames, unsigned int flags)
+int delete_refs(const char *msg, struct string_list *refnames,
+		unsigned int flags)
 {
-	return refs_delete_refs(get_main_ref_store(), refnames, flags);
+	return refs_delete_refs(get_main_ref_store(), msg, refnames, flags);
 }
 
 int refs_rename_ref(struct ref_store *refs, const char *oldref,
diff --git a/refs.h b/refs.h
index ec8c6bfbbb..b62722fb81 100644
--- a/refs.h
+++ b/refs.h
@@ -331,7 +331,8 @@ int reflog_exists(const char *refname);
  * 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().
+ * be NULL_SHA1. msg and flags are passed through to
+ * ref_transaction_delete().
  */
 int refs_delete_ref(struct ref_store *refs, const char *msg,
 		    const char *refname,
@@ -343,12 +344,13 @@ int delete_ref(const char *msg, const char *refname,
 /*
  * Delete the specified references. If there are any problems, emit
  * errors but attempt to keep going (i.e., the deletes are not done in
- * an all-or-nothing transaction). flags is passed through to
+ * an all-or-nothing transaction). msg and flags are passed through to
  * ref_transaction_delete().
  */
-int refs_delete_refs(struct ref_store *refs, struct string_list *refnames,
-		     unsigned int flags);
-int delete_refs(struct string_list *refnames, unsigned int flags);
+int refs_delete_refs(struct ref_store *refs, const char *msg,
+		     struct string_list *refnames, unsigned int flags);
+int delete_refs(const char *msg, struct string_list *refnames,
+		unsigned int flags);
 
 /** Delete a reflog */
 int refs_delete_reflog(struct ref_store *refs, const char *refname);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b2559b5585..fce8265aa7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1595,7 +1595,7 @@ static int repack_without_refs(struct files_ref_store *refs,
 	return ret;
 }
 
-static int files_delete_refs(struct ref_store *ref_store,
+static int files_delete_refs(struct ref_store *ref_store, const char *msg,
 			     struct string_list *refnames, unsigned int flags)
 {
 	struct files_ref_store *refs =
@@ -1627,7 +1627,7 @@ static int files_delete_refs(struct ref_store *ref_store,
 	for (i = 0; i < refnames->nr; i++) {
 		const char *refname = refnames->items[i].string;
 
-		if (refs_delete_ref(&refs->base, NULL, refname, NULL, flags))
+		if (refs_delete_ref(&refs->base, msg, refname, NULL, flags))
 			result |= error(_("could not remove reference %s"), refname);
 	}
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 7020e51cb7..95edf6f234 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -508,7 +508,7 @@ typedef int create_symref_fn(struct ref_store *ref_store,
 			     const char *ref_target,
 			     const char *refs_heads_master,
 			     const char *logmsg);
-typedef int delete_refs_fn(struct ref_store *ref_store,
+typedef int delete_refs_fn(struct ref_store *ref_store, const char *msg,
 			   struct string_list *refnames, unsigned int flags);
 typedef int rename_ref_fn(struct ref_store *ref_store,
 			  const char *oldref, const char *newref,
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index fba85e7da5..05d8c4d8af 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -93,12 +93,13 @@ static int cmd_create_symref(struct ref_store *refs, const char **argv)
 static int cmd_delete_refs(struct ref_store *refs, const char **argv)
 {
 	unsigned int flags = arg_flags(*argv++, "flags");
+	const char *msg = *argv++;
 	struct string_list refnames = STRING_LIST_INIT_NODUP;
 
 	while (*argv)
 		string_list_append(&refnames, *argv++);
 
-	return refs_delete_refs(refs, &refnames, flags);
+	return refs_delete_refs(refs, msg, &refnames, flags);
 }
 
 static int cmd_rename_ref(struct ref_store *refs, const char **argv)
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 490521f8cb..e8115df5ba 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -31,7 +31,7 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' '
 test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' '
 	git rev-parse FOO -- &&
 	git rev-parse refs/tags/new-tag -- &&
-	$RUN delete-refs 0 FOO refs/tags/new-tag &&
+	$RUN delete-refs 0 nothing FOO refs/tags/new-tag &&
 	test_must_fail git rev-parse FOO -- &&
 	test_must_fail git rev-parse refs/tags/new-tag --
 '
diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index 13b5454c56..c32d4cc465 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -31,7 +31,7 @@ test_expect_success 'create_symref() not allowed' '
 '
 
 test_expect_success 'delete_refs() not allowed' '
-	test_must_fail $RUN delete-refs 0 FOO refs/tags/new-tag
+	test_must_fail $RUN delete-refs 0 nothing FOO refs/tags/new-tag
 '
 
 test_expect_success 'rename_refs() not allowed' '
-- 
2.11.0

[PATCH v2 09/25] lockfile: add a new method, is_lock_file_locked()

[Michael Haggerty]

It will soon prove useful.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 lockfile.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lockfile.h b/lockfile.h
index 7b715f9e77..572064939c 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -175,6 +175,14 @@ static inline int hold_lock_file_for_update(
 	return hold_lock_file_for_update_timeout(lk, path, flags, 0);
 }
 
+/*
+ * Return a nonzero value iff `lk` is currently locked.
+ */
+static inline int is_lock_file_locked(struct lock_file *lk)
+{
+	return is_tempfile_active(&lk->tempfile);
+}
+
 /*
  * Append an appropriate error message to `buf` following the failure
  * of `hold_lock_file_for_update()` to lock `path`. `err` should be the
-- 
2.11.0

[PATCH v2 10/25] files-backend: move `lock` member to `files_ref_store`

[Michael Haggerty]

Move the `lock` member from `packed_ref_cache` to `files_ref_store`,
since at most one cache can have a locked "packed-refs" file
associated with it. Rename it to `packed_refs_lock` to make its
purpose clearer in its new home. More changes are coming here shortly.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index fce8265aa7..bfc555a417 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -43,15 +43,6 @@ struct packed_ref_cache {
 	 */
 	unsigned int referrers;
 
-	/*
-	 * Iff the packed-refs file associated with this instance is
-	 * currently locked for writing, this points at the associated
-	 * lock (which is owned by somebody else).  The referrer count
-	 * is also incremented when the file is locked and decremented
-	 * when it is unlocked.
-	 */
-	struct lock_file *lock;
-
 	/* The metadata from when this packed-refs cache was read */
 	struct stat_validity validity;
 };
@@ -70,6 +61,13 @@ struct files_ref_store {
 
 	struct ref_cache *loose;
 	struct packed_ref_cache *packed;
+
+	/*
+	 * Iff the packed-refs file associated with this instance is
+	 * currently locked for writing, this points at the associated
+	 * lock (which is owned by somebody else).
+	 */
+	struct lock_file *packed_refs_lock;
 };
 
 /* Lock used for the main packed-refs file: */
@@ -104,7 +102,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)
 	if (refs->packed) {
 		struct packed_ref_cache *packed_refs = refs->packed;
 
-		if (packed_refs->lock)
+		if (refs->packed_refs_lock)
 			die("BUG: packed-ref cache cleared while locked");
 		refs->packed = NULL;
 		release_packed_ref_cache(packed_refs);
@@ -396,7 +394,7 @@ static void add_packed_ref(struct files_ref_store *refs,
 {
 	struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
 
-	if (!packed_ref_cache->lock)
+	if (!refs->packed_refs_lock)
 		die("BUG: packed refs not locked");
 	add_ref_entry(get_packed_ref_dir(packed_ref_cache),
 		      create_ref_entry(refname, oid, REF_ISPACKED, 1));
@@ -1300,7 +1298,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	 * the packed-refs file.
 	 */
 	packed_ref_cache = get_packed_ref_cache(refs);
-	packed_ref_cache->lock = &packlock;
+	refs->packed_refs_lock = &packlock;
 	/* Increment the reference count to prevent it from being freed: */
 	acquire_packed_ref_cache(packed_ref_cache);
 	return 0;
@@ -1323,10 +1321,10 @@ static int commit_packed_refs(struct files_ref_store *refs)
 
 	files_assert_main_repository(refs, "commit_packed_refs");
 
-	if (!packed_ref_cache->lock)
+	if (!refs->packed_refs_lock)
 		die("BUG: packed-refs not locked");
 
-	out = fdopen_lock_file(packed_ref_cache->lock, "w");
+	out = fdopen_lock_file(refs->packed_refs_lock, "w");
 	if (!out)
 		die_errno("unable to fdopen packed-refs descriptor");
 
@@ -1344,11 +1342,11 @@ static int commit_packed_refs(struct files_ref_store *refs)
 	if (ok != ITER_DONE)
 		die("error while iterating over references");
 
-	if (commit_lock_file(packed_ref_cache->lock)) {
+	if (commit_lock_file(refs->packed_refs_lock)) {
 		save_errno = errno;
 		error = -1;
 	}
-	packed_ref_cache->lock = NULL;
+	refs->packed_refs_lock = NULL;
 	release_packed_ref_cache(packed_ref_cache);
 	errno = save_errno;
 	return error;
@@ -1366,10 +1364,10 @@ static void rollback_packed_refs(struct files_ref_store *refs)
 
 	files_assert_main_repository(refs, "rollback_packed_refs");
 
-	if (!packed_ref_cache->lock)
+	if (!refs->packed_refs_lock)
 		die("BUG: packed-refs not locked");
-	rollback_lock_file(packed_ref_cache->lock);
-	packed_ref_cache->lock = NULL;
+	rollback_lock_file(refs->packed_refs_lock);
+	refs->packed_refs_lock = NULL;
 	release_packed_ref_cache(packed_ref_cache);
 	clear_packed_ref_cache(refs);
 }
-- 
2.11.0

[PATCH v2 11/25] files_ref_store: put the packed files lock directly in this struct

[Michael Haggerty]

Instead of using a global `lock_file` instance for the main
"packed-refs" file and using a pointer in `files_ref_store` to keep
track of whether it is locked, embed the `lock_file` instance directly
in the `files_ref_store` struct and use the new
`is_lock_file_locked()` function to keep track of whether it is
locked. This keeps related data together and makes the main reference
store less of a special case.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index bfc555a417..1db40432af 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -63,16 +63,12 @@ struct files_ref_store {
 	struct packed_ref_cache *packed;
 
 	/*
-	 * Iff the packed-refs file associated with this instance is
-	 * currently locked for writing, this points at the associated
-	 * lock (which is owned by somebody else).
+	 * Lock used for the "packed-refs" file. Note that this (and
+	 * thus the enclosing `files_ref_store`) must not be freed.
 	 */
-	struct lock_file *packed_refs_lock;
+	struct lock_file packed_refs_lock;
 };
 
-/* Lock used for the main packed-refs file: */
-static struct lock_file packlock;
-
 /*
  * Increment the reference count of *packed_refs.
  */
@@ -102,7 +98,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)
 	if (refs->packed) {
 		struct packed_ref_cache *packed_refs = refs->packed;
 
-		if (refs->packed_refs_lock)
+		if (is_lock_file_locked(&refs->packed_refs_lock))
 			die("BUG: packed-ref cache cleared while locked");
 		refs->packed = NULL;
 		release_packed_ref_cache(packed_refs);
@@ -394,7 +390,7 @@ static void add_packed_ref(struct files_ref_store *refs,
 {
 	struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
 
-	if (!refs->packed_refs_lock)
+	if (!is_lock_file_locked(&refs->packed_refs_lock))
 		die("BUG: packed refs not locked");
 	add_ref_entry(get_packed_ref_dir(packed_ref_cache),
 		      create_ref_entry(refname, oid, REF_ISPACKED, 1));
@@ -1288,7 +1284,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	}
 
 	if (hold_lock_file_for_update_timeout(
-			    &packlock, files_packed_refs_path(refs),
+			    &refs->packed_refs_lock, files_packed_refs_path(refs),
 			    flags, timeout_value) < 0)
 		return -1;
 	/*
@@ -1298,7 +1294,6 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	 * the packed-refs file.
 	 */
 	packed_ref_cache = get_packed_ref_cache(refs);
-	refs->packed_refs_lock = &packlock;
 	/* Increment the reference count to prevent it from being freed: */
 	acquire_packed_ref_cache(packed_ref_cache);
 	return 0;
@@ -1321,10 +1316,10 @@ static int commit_packed_refs(struct files_ref_store *refs)
 
 	files_assert_main_repository(refs, "commit_packed_refs");
 
-	if (!refs->packed_refs_lock)
+	if (!is_lock_file_locked(&refs->packed_refs_lock))
 		die("BUG: packed-refs not locked");
 
-	out = fdopen_lock_file(refs->packed_refs_lock, "w");
+	out = fdopen_lock_file(&refs->packed_refs_lock, "w");
 	if (!out)
 		die_errno("unable to fdopen packed-refs descriptor");
 
@@ -1342,11 +1337,10 @@ static int commit_packed_refs(struct files_ref_store *refs)
 	if (ok != ITER_DONE)
 		die("error while iterating over references");
 
-	if (commit_lock_file(refs->packed_refs_lock)) {
+	if (commit_lock_file(&refs->packed_refs_lock)) {
 		save_errno = errno;
 		error = -1;
 	}
-	refs->packed_refs_lock = NULL;
 	release_packed_ref_cache(packed_ref_cache);
 	errno = save_errno;
 	return error;
@@ -1364,10 +1358,9 @@ static void rollback_packed_refs(struct files_ref_store *refs)
 
 	files_assert_main_repository(refs, "rollback_packed_refs");
 
-	if (!refs->packed_refs_lock)
+	if (!is_lock_file_locked(&refs->packed_refs_lock))
 		die("BUG: packed-refs not locked");
-	rollback_lock_file(refs->packed_refs_lock);
-	refs->packed_refs_lock = NULL;
+	rollback_lock_file(&refs->packed_refs_lock);
 	release_packed_ref_cache(packed_ref_cache);
 	clear_packed_ref_cache(refs);
 }
-- 
2.11.0

[PATCH v2 12/25] files_transaction_cleanup(): new helper function

[Michael Haggerty]

Extract the cleanup functionality from `files_transaction_commit()`
into a new function. It will soon have another caller.

Use the common cleanup code even on early exit if the transaction is
empty, to reduce code duplication.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1db40432af..2c70de5209 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2834,6 +2834,27 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 	return 0;
 }
 
+/*
+ * Unlock any references in `transaction` that are still locked, and
+ * mark the transaction closed.
+ */
+static void files_transaction_cleanup(struct ref_transaction *transaction)
+{
+	size_t i;
+
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *update = transaction->updates[i];
+		struct ref_lock *lock = update->backend_data;
+
+		if (lock) {
+			unlock_ref(lock);
+			update->backend_data = NULL;
+		}
+	}
+
+	transaction->state = REF_TRANSACTION_CLOSED;
+}
+
 static int files_transaction_commit(struct ref_store *ref_store,
 				    struct ref_transaction *transaction,
 				    struct strbuf *err)
@@ -2856,10 +2877,8 @@ static int files_transaction_commit(struct ref_store *ref_store,
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: commit called for transaction that is not open");
 
-	if (!transaction->nr) {
-		transaction->state = REF_TRANSACTION_CLOSED;
-		return 0;
-	}
+	if (!transaction->nr)
+		goto cleanup;
 
 	/*
 	 * Fail if a refname appears more than once in the
@@ -3005,15 +3024,11 @@ static int files_transaction_commit(struct ref_store *ref_store,
 	clear_loose_ref_cache(refs);
 
 cleanup:
+	files_transaction_cleanup(transaction);
 	strbuf_release(&sb);
-	transaction->state = REF_TRANSACTION_CLOSED;
 
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
-		struct ref_lock *lock = update->backend_data;
-
-		if (lock)
-			unlock_ref(lock);
 
 		if (update->flags & REF_DELETED_LOOSE) {
 			/*
-- 
2.11.0

[PATCH v2 13/25] ref_transaction_commit(): check for valid `transaction->state`

[Michael Haggerty]

Move the check that `transaction->state` is valid from
`files_transaction_commit()` to `ref_transaction_commit()`, where
other future reference backends can benefit from it as well.

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

diff --git a/refs.c b/refs.c
index 989462c972..f8f41ffb04 100644
--- a/refs.c
+++ b/refs.c
@@ -1694,6 +1694,18 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 {
 	struct ref_store *refs = transaction->ref_store;
 
+	switch (transaction->state) {
+	case REF_TRANSACTION_OPEN:
+		/* Good. */
+		break;
+	case REF_TRANSACTION_CLOSED:
+		die("BUG: prepare called on a closed reference transaction");
+		break;
+	default:
+		die("BUG: unexpected reference transaction state");
+		break;
+	}
+
 	if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
 		strbuf_addstr(err,
 			      _("ref updates forbidden inside quarantine environment"));
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2c70de5209..a4261d4683 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2874,9 +2874,6 @@ static int files_transaction_commit(struct ref_store *ref_store,
 
 	assert(err);
 
-	if (transaction->state != REF_TRANSACTION_OPEN)
-		die("BUG: commit called for transaction that is not open");
-
 	if (!transaction->nr)
 		goto cleanup;
 
-- 
2.11.0

[PATCH v2 14/25] ref_transaction_prepare(): new optional step for reference updates

[Michael Haggerty]

In the future, compound reference stores will sometimes need to modify
references in two different reference stores at the same time, meaning
that a single logical reference transaction might have to be
implemented as two internal sub-transactions. They won't want to call
`ref_transaction_commit()` for the two sub-transactions one after the
other, because that wouldn't be atomic (the first commit could succeed
and the second one fail). Instead, they will want to prepare both
sub-transactions (i.e., obtain any necessary locks and do any
pre-checks), and only if both prepare steps succeed, then commit both
sub-transactions.

Start preparing for that day by adding a new, optional
`ref_transaction_prepare()` step to the reference transaction
sequence, which obtains the locks and does any prechecks, reporting
any errors that occur. Also add a `ref_transaction_abort()` function
that can be used to abort a sub-transaction even if it has already
been prepared.

That is on the side of the public-facing API. On the side of the
`ref_store` VTABLE, get rid of `transaction_commit` and instead add
methods `transaction_prepare`, `transaction_finish`, and
`transaction_abort`. A `ref_transaction_commit()` now basically calls
methods `transaction_prepare` then `transaction_finish`.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               |  74 ++++++++++++++++++++++++++++--
 refs.h               | 124 ++++++++++++++++++++++++++++++++++++++++-----------
 refs/files-backend.c |  63 ++++++++++++++++++++------
 refs/refs-internal.h |  45 ++++++++++++++-----
 4 files changed, 253 insertions(+), 53 deletions(-)

diff --git a/refs.c b/refs.c
index f8f41ffb04..b3860a9e33 100644
--- a/refs.c
+++ b/refs.c
@@ -853,6 +853,19 @@ void ref_transaction_free(struct ref_transaction *transaction)
 	if (!transaction)
 		return;
 
+	switch (transaction->state) {
+	case REF_TRANSACTION_OPEN:
+	case REF_TRANSACTION_CLOSED:
+		/* OK */
+		break;
+	case REF_TRANSACTION_PREPARED:
+		die("BUG: free called on a prepared reference transaction");
+		break;
+	default:
+		die("BUG: unexpected reference transaction state");
+		break;
+	}
+
 	for (i = 0; i < transaction->nr; i++) {
 		free(transaction->updates[i]->msg);
 		free(transaction->updates[i]);
@@ -1689,8 +1702,8 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
 				  refs_heads_master, logmsg);
 }
 
-int ref_transaction_commit(struct ref_transaction *transaction,
-			   struct strbuf *err)
+int ref_transaction_prepare(struct ref_transaction *transaction,
+			    struct strbuf *err)
 {
 	struct ref_store *refs = transaction->ref_store;
 
@@ -1698,6 +1711,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	case REF_TRANSACTION_OPEN:
 		/* Good. */
 		break;
+	case REF_TRANSACTION_PREPARED:
+		die("BUG: prepare called twice on reference transaction");
+		break;
 	case REF_TRANSACTION_CLOSED:
 		die("BUG: prepare called on a closed reference transaction");
 		break;
@@ -1712,7 +1728,59 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return -1;
 	}
 
-	return refs->be->transaction_commit(refs, transaction, err);
+	return refs->be->transaction_prepare(refs, transaction, err);
+}
+
+int ref_transaction_abort(struct ref_transaction *transaction,
+			  struct strbuf *err)
+{
+	struct ref_store *refs = transaction->ref_store;
+	int ret = 0;
+
+	switch (transaction->state) {
+	case REF_TRANSACTION_OPEN:
+		/* No need to abort explicitly. */
+		break;
+	case REF_TRANSACTION_PREPARED:
+		ret = refs->be->transaction_abort(refs, transaction, err);
+		break;
+	case REF_TRANSACTION_CLOSED:
+		die("BUG: abort called on a closed reference transaction");
+		break;
+	default:
+		die("BUG: unexpected reference transaction state");
+		break;
+	}
+
+	ref_transaction_free(transaction);
+	return ret;
+}
+
+int ref_transaction_commit(struct ref_transaction *transaction,
+			   struct strbuf *err)
+{
+	struct ref_store *refs = transaction->ref_store;
+	int ret;
+
+	switch (transaction->state) {
+	case REF_TRANSACTION_OPEN:
+		/* Need to prepare first. */
+		ret = ref_transaction_prepare(transaction, err);
+		if (ret)
+			return ret;
+		break;
+	case REF_TRANSACTION_PREPARED:
+		/* Fall through to finish. */
+		break;
+	case REF_TRANSACTION_CLOSED:
+		die("BUG: commit called on a closed reference transaction");
+		break;
+	default:
+		die("BUG: unexpected reference transaction state");
+		break;
+	}
+
+	return refs->be->transaction_finish(refs, transaction, err);
 }
 
 int refs_verify_refname_available(struct ref_store *refs,
diff --git a/refs.h b/refs.h
index b62722fb81..4be14c4b3c 100644
--- a/refs.h
+++ b/refs.h
@@ -143,30 +143,71 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
 int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
 
 /*
- * A ref_transaction represents a collection of ref updates
- * that should succeed or fail together.
+ * A ref_transaction represents a collection of reference updates that
+ * should succeed or fail together.
  *
  * Calling sequence
  * ----------------
+ *
  * - Allocate and initialize a `struct ref_transaction` by calling
  *   `ref_transaction_begin()`.
  *
- * - List intended ref updates by calling functions like
- *   `ref_transaction_update()` and `ref_transaction_create()`.
- *
- * - Call `ref_transaction_commit()` to execute the transaction.
- *   If this succeeds, the ref updates will have taken place and
- *   the transaction cannot be rolled back.
- *
- * - Instead of `ref_transaction_commit`, use
- *   `initial_ref_transaction_commit()` if the ref database is known
- *   to be empty (e.g. during clone).  This is likely to be much
- *   faster.
- *
- * - At any time call `ref_transaction_free()` to discard the
- *   transaction and free associated resources.  In particular,
- *   this rolls back the transaction if it has not been
- *   successfully committed.
+ * - Specify the intended ref updates by calling one or more of the
+ *   following functions:
+ *   - `ref_transaction_update()`
+ *   - `ref_transaction_create()`
+ *   - `ref_transaction_delete()`
+ *   - `ref_transaction_verify()`
+ *
+ * - Then either:
+ *
+ *   - Optionally call `ref_transaction_prepare()` to prepare the
+ *     transaction. This locks all references, checks preconditions,
+ *     etc. but doesn't finalize anything. If this step fails, the
+ *     transaction has been closed and can only be freed. If this step
+ *     succeeds, then `ref_transaction_commit()` is almost certain to
+ *     succeed. However, you can still call `ref_transaction_abort()`
+ *     if you decide not to commit the transaction after all.
+ *
+ *   - Call `ref_transaction_commit()` to execute the transaction,
+ *     make the changes permanent, and release all locks. If you
+ *     haven't already called `ref_transaction_prepare()`, then
+ *     `ref_transaction_commit()` calls it for you.
+ *
+ *   Or
+ *
+ *   - Call `initial_ref_transaction_commit()` if the ref database is
+ *     known to be empty and have no other writers (e.g. during
+ *     clone). This is likely to be much faster than
+ *     `ref_transaction_commit()`. `ref_transaction_prepare()` should
+ *     *not* be called before `initial_ref_transaction_commit()`.
+ *
+ * - Then finally, call `ref_transaction_free()` to free the
+ *   `ref_transaction` data structure.
+ *
+ * At any time before calling `ref_transaction_commit()`, you can call
+ * `ref_transaction_abort()` to abort the transaction, rollback any
+ * locks, and free any associated resources (including the
+ * `ref_transaction` data structure).
+ *
+ * Putting it all together, a complete reference update looks like
+ *
+ *         struct ref_transaction *transaction;
+ *         struct strbuf err = STRBUF_INIT;
+ *         int ret = 0;
+ *
+ *         transaction = ref_store_transaction_begin(refs, &err);
+ *         if (!transaction ||
+ *             ref_transaction_update(...) ||
+ *             ref_transaction_create(...) ||
+ *             ...etc... ||
+ *             ref_transaction_commit(transaction, &err)) {
+ *                 error("%s", err.buf);
+ *                 ret = -1;
+ *         }
+ *         ref_transaction_free(transaction);
+ *         strbuf_release(&err);
+ *         return ret;
  *
  * Error handling
  * --------------
@@ -183,8 +224,9 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
  * -------
  *
  * Note that no locks are taken, and no refs are read, until
- * `ref_transaction_commit` is called.  So `ref_transaction_verify`
- * won't report a verification failure until the commit is attempted.
+ * `ref_transaction_prepare()` or `ref_transaction_commit()` is
+ * called. So, for example, `ref_transaction_verify()` won't report a
+ * verification failure until the commit is attempted.
  */
 struct ref_transaction;
 
@@ -523,19 +565,47 @@ int ref_transaction_verify(struct ref_transaction *transaction,
 			   unsigned int flags,
 			   struct strbuf *err);
 
-/*
- * Commit all of the changes that have been queued in transaction, as
- * atomically as possible.
- *
- * Returns 0 for success, or one of the below error codes for errors.
- */
 /* Naming conflict (for example, the ref names A and A/B conflict). */
 #define TRANSACTION_NAME_CONFLICT -1
 /* All other errors. */
 #define TRANSACTION_GENERIC_ERROR -2
+
+/*
+ * Perform the preparatory stages of commiting `transaction`. Acquire
+ * any needed locks, check preconditions, etc.; basically, do as much
+ * as possible to ensure that the transaction will be able to go
+ * through, stopping just short of making any irrevocable or
+ * user-visible changes. The updates that this function prepares can
+ * be finished up by calling `ref_transaction_commit()` or rolled back
+ * by calling `ref_transaction_abort()`.
+ *
+ * On success, return 0 and leave the transaction in "prepared" state.
+ * On failure, abort the transaction, write an error message to `err`,
+ * and return one of the `TRANSACTION_*` constants.
+ *
+ * Callers who don't need such fine-grained control over commiting
+ * reference transactions should just call `ref_transaction_commit()`.
+ */
+int ref_transaction_prepare(struct ref_transaction *transaction,
+			    struct strbuf *err);
+
+/*
+ * Commit all of the changes that have been queued in transaction, as
+ * atomically as possible. On success, return 0 and leave the
+ * transaction in "closed" state. On failure, roll back the
+ * transaction, write an error message to `err`, and return one of the
+ * `TRANSACTION_*` constants
+ */
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err);
 
+/*
+ * Abort `transaction`, which has been begun and possibly prepared,
+ * but not yet committed.
+ */
+int ref_transaction_abort(struct ref_transaction *transaction,
+			  struct strbuf *err);
+
 /*
  * Like ref_transaction_commit(), but optimized for creating
  * references when originally initializing a repository (e.g., by "git
@@ -551,7 +621,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 				   struct strbuf *err);
 
 /*
- * Free an existing transaction and all associated data.
+ * Free `*transaction` and all associated data.
  */
 void ref_transaction_free(struct ref_transaction *transaction);
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a4261d4683..19842d2e56 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2855,22 +2855,19 @@ static void files_transaction_cleanup(struct ref_transaction *transaction)
 	transaction->state = REF_TRANSACTION_CLOSED;
 }
 
-static int files_transaction_commit(struct ref_store *ref_store,
-				    struct ref_transaction *transaction,
-				    struct strbuf *err)
+static int files_transaction_prepare(struct ref_store *ref_store,
+				     struct ref_transaction *transaction,
+				     struct strbuf *err)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_WRITE,
-			       "ref_transaction_commit");
+			       "ref_transaction_prepare");
 	size_t i;
 	int ret = 0;
-	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
-	struct string_list_item *ref_to_delete;
 	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
 	char *head_ref = NULL;
 	int head_type;
 	struct object_id head_oid;
-	struct strbuf sb = STRBUF_INIT;
 
 	assert(err);
 
@@ -2934,6 +2931,8 @@ static int files_transaction_commit(struct ref_store *ref_store,
 	 * that new values are valid, and write new values to the
 	 * lockfiles, ready to be activated. Only keep one lockfile
 	 * open at a time to avoid running out of file descriptors.
+	 * Note that lock_ref_for_update() might append more updates
+	 * to the transaction.
 	 */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
@@ -2941,7 +2940,38 @@ static int files_transaction_commit(struct ref_store *ref_store,
 		ret = lock_ref_for_update(refs, update, transaction,
 					  head_ref, &affected_refnames, err);
 		if (ret)
-			goto cleanup;
+			break;
+	}
+
+cleanup:
+	free(head_ref);
+	string_list_clear(&affected_refnames, 0);
+
+	if (ret)
+		files_transaction_cleanup(transaction);
+	else
+		transaction->state = REF_TRANSACTION_PREPARED;
+
+	return ret;
+}
+
+static int files_transaction_finish(struct ref_store *ref_store,
+				    struct ref_transaction *transaction,
+				    struct strbuf *err)
+{
+	struct files_ref_store *refs =
+		files_downcast(ref_store, 0, "ref_transaction_finish");
+	size_t i;
+	int ret = 0;
+	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref_to_delete;
+	struct strbuf sb = STRBUF_INIT;
+
+	assert(err);
+
+	if (!transaction->nr) {
+		transaction->state = REF_TRANSACTION_CLOSED;
+		return 0;
 	}
 
 	/* Perform updates first so live commits remain referenced */
@@ -3022,7 +3052,6 @@ static int files_transaction_commit(struct ref_store *ref_store,
 
 cleanup:
 	files_transaction_cleanup(transaction);
-	strbuf_release(&sb);
 
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
@@ -3039,13 +3068,19 @@ static int files_transaction_commit(struct ref_store *ref_store,
 		}
 	}
 
+	strbuf_release(&sb);
 	string_list_clear(&refs_to_delete, 0);
-	free(head_ref);
-	string_list_clear(&affected_refnames, 0);
-
 	return ret;
 }
 
+static int files_transaction_abort(struct ref_store *ref_store,
+				   struct ref_transaction *transaction,
+				   struct strbuf *err)
+{
+	files_transaction_cleanup(transaction);
+	return 0;
+}
+
 static int ref_present(const char *refname,
 		       const struct object_id *oid, int flags, void *cb_data)
 {
@@ -3316,7 +3351,9 @@ struct ref_storage_be refs_be_files = {
 	"files",
 	files_ref_store_create,
 	files_init_db,
-	files_transaction_commit,
+	files_transaction_prepare,
+	files_transaction_finish,
+	files_transaction_abort,
 	files_initial_transaction_commit,
 
 	files_pack_refs,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 95edf6f234..4d3dd17f9f 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -185,17 +185,27 @@ struct ref_update *ref_transaction_add_update(
 
 /*
  * Transaction states.
- * OPEN:   The transaction is in a valid state and can accept new updates.
- *         An OPEN transaction can be committed.
- * CLOSED: A closed transaction is no longer active and no other operations
- *         than free can be used on it in this state.
- *         A transaction can either become closed by successfully committing
- *         an active transaction or if there is a failure while building
- *         the transaction thus rendering it failed/inactive.
+ *
+ * OPEN:   The transaction is initialized and new updates can still be
+ *         added to it. An OPEN transaction can be prepared,
+ *         committed, freed, or aborted (freeing and aborting an open
+ *         transaction are equivalent).
+ *
+ * PREPARED: ref_transaction_prepare(), which locks all of the
+ *         references involved in the update and checks that the
+ *         update has no errors, has been called successfully for the
+ *         transaction. A PREPARED transaction can be committed or
+ *         aborted.
+ *
+ * CLOSED: The transaction is no longer active. A transaction becomes
+ *         CLOSED if there is a failure while building the transaction
+ *         or if a transaction is committed or aborted. A CLOSED
+ *         transaction can only be freed.
  */
 enum ref_transaction_state {
-	REF_TRANSACTION_OPEN   = 0,
-	REF_TRANSACTION_CLOSED = 1
+	REF_TRANSACTION_OPEN     = 0,
+	REF_TRANSACTION_PREPARED = 1,
+	REF_TRANSACTION_CLOSED   = 2
 };
 
 /*
@@ -497,6 +507,18 @@ typedef struct ref_store *ref_store_init_fn(const char *gitdir,
 
 typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);
 
+typedef int ref_transaction_prepare_fn(struct ref_store *refs,
+				       struct ref_transaction *transaction,
+				       struct strbuf *err);
+
+typedef int ref_transaction_finish_fn(struct ref_store *refs,
+				      struct ref_transaction *transaction,
+				      struct strbuf *err);
+
+typedef int ref_transaction_abort_fn(struct ref_store *refs,
+				     struct ref_transaction *transaction,
+				     struct strbuf *err);
+
 typedef int ref_transaction_commit_fn(struct ref_store *refs,
 				      struct ref_transaction *transaction,
 				      struct strbuf *err);
@@ -600,7 +622,10 @@ struct ref_storage_be {
 	const char *name;
 	ref_store_init_fn *init;
 	ref_init_db_fn *init_db;
-	ref_transaction_commit_fn *transaction_commit;
+
+	ref_transaction_prepare_fn *transaction_prepare;
+	ref_transaction_finish_fn *transaction_finish;
+	ref_transaction_abort_fn *transaction_abort;
 	ref_transaction_commit_fn *initial_transaction_commit;
 
 	pack_refs_fn *pack_refs;
-- 
2.11.0

[PATCH v2 15/25] ref_update_reject_duplicates(): expose function to whole refs module

[Michael Haggerty]

It will soon have some other users.

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

diff --git a/refs.c b/refs.c
index b3860a9e33..beb49fb297 100644
--- a/refs.c
+++ b/refs.c
@@ -1702,6 +1702,23 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
 				  refs_heads_master, logmsg);
 }
 
+int ref_update_reject_duplicates(struct string_list *refnames,
+				 struct strbuf *err)
+{
+	int i, n = refnames->nr;
+
+	assert(err);
+
+	for (i = 1; i < n; i++)
+		if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {
+			strbuf_addf(err,
+				    "multiple updates for ref '%s' not allowed.",
+				    refnames->items[i].string);
+			return 1;
+		}
+	return 0;
+}
+
 int ref_transaction_prepare(struct ref_transaction *transaction,
 			    struct strbuf *err)
 {
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 19842d2e56..8d0ce739a6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2512,23 +2512,6 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
 	return ref_iterator;
 }
 
-static int ref_update_reject_duplicates(struct string_list *refnames,
-					struct strbuf *err)
-{
-	int i, n = refnames->nr;
-
-	assert(err);
-
-	for (i = 1; i < n; i++)
-		if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {
-			strbuf_addf(err,
-				    "multiple updates for ref '%s' not allowed.",
-				    refnames->items[i].string);
-			return 1;
-		}
-	return 0;
-}
-
 /*
  * If update is a direct update of head_ref (the reference pointed to
  * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 4d3dd17f9f..192f9f85c9 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -169,6 +169,14 @@ int refs_read_raw_ref(struct ref_store *ref_store,
 		      const char *refname, unsigned char *sha1,
 		      struct strbuf *referent, unsigned int *type);
 
+/*
+ * Write an error to `err` and return a nonzero value iff the same
+ * refname appears multiple times in `refnames`. `refnames` must be
+ * sorted on entry to this function.
+ */
+int ref_update_reject_duplicates(struct string_list *refnames,
+				 struct strbuf *err);
+
 /*
  * Add a ref_update with the specified properties to transaction, and
  * return a pointer to the new object. This function does not verify
-- 
2.11.0

[PATCH v2 16/25] ref_update_reject_duplicates(): use `size_t` rather than `int`

[Michael Haggerty]

Eliminate a theoretical risk of integer overflow if the two types have
different sizes.

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

diff --git a/refs.c b/refs.c
index beb49fb297..143936a9c3 100644
--- a/refs.c
+++ b/refs.c
@@ -1705,7 +1705,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
 int ref_update_reject_duplicates(struct string_list *refnames,
 				 struct strbuf *err)
 {
-	int i, n = refnames->nr;
+	size_t i, n = refnames->nr;
 
 	assert(err);
 
-- 
2.11.0

[PATCH v2 17/25] ref_update_reject_duplicates(): add a sanity check

[Michael Haggerty]

It's pretty cheap to make sure that the caller didn't pass us an
unsorted list by accident, so do so.

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

diff --git a/refs.c b/refs.c
index 143936a9c3..d1c781d94e 100644
--- a/refs.c
+++ b/refs.c
@@ -1709,13 +1709,19 @@ int ref_update_reject_duplicates(struct string_list *refnames,
 
 	assert(err);
 
-	for (i = 1; i < n; i++)
-		if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {
+	for (i = 1; i < n; i++) {
+		int cmp = strcmp(refnames->items[i - 1].string,
+				 refnames->items[i].string);
+
+		if (!cmp) {
 			strbuf_addf(err,
 				    "multiple updates for ref '%s' not allowed.",
 				    refnames->items[i].string);
 			return 1;
+		} else if (cmp > 0) {
+			die("BUG: ref_update_reject_duplicates() received unsorted list");
 		}
+	}
 	return 0;
 }
 
-- 
2.11.0

[PATCH v2 18/25] should_pack_ref(): new function, extracted from `files_pack_refs()`

[Michael Haggerty]

Extract a function for deciding whether a reference should be packed.
It is a self-contained bit of logic, so splitting it out improves
readability.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8d0ce739a6..29514392b0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1455,6 +1455,32 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune *r)
 	}
 }
 
+/*
+ * Return true if the specified reference should be packed.
+ */
+static int should_pack_ref(const char *refname,
+			   const struct object_id *oid, unsigned int ref_flags,
+			   unsigned int pack_flags)
+{
+	/* Do not pack per-worktree refs: */
+	if (ref_type(refname) != REF_TYPE_NORMAL)
+		return 0;
+
+	/* Do not pack non-tags unless PACK_REFS_ALL is set: */
+	if (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/"))
+		return 0;
+
+	/* Do not pack symbolic refs: */
+	if (ref_flags & REF_ISSYMREF)
+		return 0;
+
+	/* Do not pack broken refs: */
+	if (!ref_resolves_to_object(refname, oid, ref_flags))
+		return 0;
+
+	return 1;
+}
+
 static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 {
 	struct files_ref_store *refs =
@@ -1476,21 +1502,9 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 		 * pruned, also add it to refs_to_prune.
 		 */
 		struct ref_entry *packed_entry;
-		int is_tag_ref = starts_with(iter->refname, "refs/tags/");
-
-		/* Do not pack per-worktree refs: */
-		if (ref_type(iter->refname) != REF_TYPE_NORMAL)
-			continue;
-
-		/* ALWAYS pack tags */
-		if (!(flags & PACK_REFS_ALL) && !is_tag_ref)
-			continue;
-
-		/* Do not pack symbolic or broken refs: */
-		if (iter->flags & REF_ISSYMREF)
-			continue;
 
-		if (!ref_resolves_to_object(iter->refname, iter->oid, iter->flags))
+		if (!should_pack_ref(iter->refname, iter->oid, iter->flags,
+				     flags))
 			continue;
 
 		/*
-- 
2.11.0

[PATCH v2 19/25] get_packed_ref_cache(): assume "packed-refs" won't change while locked

[Michael Haggerty]

If we've got the "packed-refs" file locked, then it can't change;
there's no need to keep calling `stat_validity_check()` on it.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 29514392b0..c4bc9550d3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -342,13 +342,18 @@ static void files_ref_path(struct files_ref_store *refs,
 
 /*
  * Get the packed_ref_cache for the specified files_ref_store,
- * creating it if necessary.
+ * creating and populating it if it hasn't been read before or if the
+ * file has been changed (according to its `validity` field) since it
+ * was last read. On the other hand, if we hold the lock, then assume
+ * that the file hasn't been changed out from under us, so skip the
+ * extra `stat()` call in `stat_validity_check()`.
  */
 static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refs)
 {
 	const char *packed_refs_file = files_packed_refs_path(refs);
 
 	if (refs->packed &&
+	    !is_lock_file_locked(&refs->packed_refs_lock) &&
 	    !stat_validity_check(&refs->packed->validity, packed_refs_file))
 		clear_packed_ref_cache(refs);
 
@@ -1288,10 +1293,11 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 			    flags, timeout_value) < 0)
 		return -1;
 	/*
-	 * Get the current packed-refs while holding the lock.  If the
-	 * packed-refs file has been modified since we last read it,
-	 * this will automatically invalidate the cache and re-read
-	 * the packed-refs file.
+	 * Get the current packed-refs while holding the lock. It is
+	 * important that we call `get_packed_ref_cache()` before
+	 * setting `packed_ref_cache->lock`, because otherwise the
+	 * former will see that the file is locked and assume that the
+	 * cache can't be stale.
 	 */
 	packed_ref_cache = get_packed_ref_cache(refs);
 	/* Increment the reference count to prevent it from being freed: */
-- 
2.11.0

[PATCH v2 20/25] read_packed_refs(): do more of the work of reading packed refs

[Michael Haggerty]

Teach `read_packed_refs()` to also

* Allocate and initialize the new `packed_ref_cache`
* Open and close the `packed-refs` file
* Update the `validity` field of the new object

This decreases the coupling between `packed_refs_cache` and
`files_ref_store` by a little bit.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 40 ++++++++++++++++++++++++----------------
 refs/ref-cache.h     |  3 ++-
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index c4bc9550d3..b4fa745cd7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -209,7 +209,9 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)
 }
 
 /*
- * Read f, which is a packed-refs file, into dir.
+ * Read from `packed_refs_file` into a newly-allocated
+ * `packed_ref_cache` and return it. The return value will already
+ * have its reference count incremented.
  *
  * A comment line of the form "# pack-refs with: " may contain zero or
  * more traits. We interpret the traits as follows:
@@ -235,12 +237,26 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)
  *      compatibility with older clients, but we do not require it
  *      (i.e., "peeled" is a no-op if "fully-peeled" is set).
  */
-static void read_packed_refs(FILE *f, struct ref_dir *dir)
+static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)
 {
+	FILE *f;
+	struct packed_ref_cache *packed_refs = xcalloc(1, sizeof(*packed_refs));
 	struct ref_entry *last = NULL;
 	struct strbuf line = STRBUF_INIT;
 	enum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;
+	struct ref_dir *dir;
 
+	acquire_packed_ref_cache(packed_refs);
+	packed_refs->cache = create_ref_cache(NULL, NULL);
+	packed_refs->cache->root->flag &= ~REF_INCOMPLETE;
+
+	f = fopen(packed_refs_file, "r");
+	if (!f)
+		return packed_refs;
+
+	stat_validity_update(&packed_refs->validity, fileno(f));
+
+	dir = get_ref_dir(packed_refs->cache->root);
 	while (strbuf_getwholeline(&line, f, '\n') != EOF) {
 		struct object_id oid;
 		const char *refname;
@@ -287,7 +303,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
 		}
 	}
 
+	fclose(f);
 	strbuf_release(&line);
+
+	return packed_refs;
 }
 
 static const char *files_packed_refs_path(struct files_ref_store *refs)
@@ -357,20 +376,9 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref
 	    !stat_validity_check(&refs->packed->validity, packed_refs_file))
 		clear_packed_ref_cache(refs);
 
-	if (!refs->packed) {
-		FILE *f;
-
-		refs->packed = xcalloc(1, sizeof(*refs->packed));
-		acquire_packed_ref_cache(refs->packed);
-		refs->packed->cache = create_ref_cache(&refs->base, NULL);
-		refs->packed->cache->root->flag &= ~REF_INCOMPLETE;
-		f = fopen(packed_refs_file, "r");
-		if (f) {
-			stat_validity_update(&refs->packed->validity, fileno(f));
-			read_packed_refs(f, get_ref_dir(refs->packed->cache->root));
-			fclose(f);
-		}
-	}
+	if (!refs->packed)
+		refs->packed = read_packed_refs(packed_refs_file);
+
 	return refs->packed;
 }
 
diff --git a/refs/ref-cache.h b/refs/ref-cache.h
index 1f65e2f9ed..fbfee7ce79 100644
--- a/refs/ref-cache.h
+++ b/refs/ref-cache.h
@@ -194,7 +194,8 @@ struct ref_entry *create_ref_entry(const char *refname,
  * function called to fill in incomplete directories in the
  * `ref_cache` when they are accessed. If it is NULL, then the whole
  * `ref_cache` must be filled (including clearing its directories'
- * `REF_INCOMPLETE` bits) before it is used.
+ * `REF_INCOMPLETE` bits) before it is used, and `refs` can be NULL,
+ * too.
  */
 struct ref_cache *create_ref_cache(struct ref_store *refs,
 				   fill_ref_dir_fn *fill_ref_dir);
-- 
2.11.0

[PATCH v2 21/25] read_packed_refs(): report unexpected fopen() failures

[Michael Haggerty]

The old code ignored any errors encountered when trying to fopen the
"packed-refs" file, treating all such failures as if the file didn't
exist. But it could be that there is some other error opening the
file (e.g., permissions problems), and we don't want to silently
ignore such problems. So report any failures that are not due to
ENOENT.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b4fa745cd7..dbfd03f989 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -251,8 +251,18 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)
 	packed_refs->cache->root->flag &= ~REF_INCOMPLETE;
 
 	f = fopen(packed_refs_file, "r");
-	if (!f)
-		return packed_refs;
+	if (!f) {
+		if (errno == ENOENT) {
+			/*
+			 * This is OK; it just means that no
+			 * "packed-refs" file has been written yet,
+			 * which is equivalent to it being empty.
+			 */
+			return packed_refs;
+		} else {
+			die_errno("couldn't read %s", packed_refs_file);
+		}
+	}
 
 	stat_validity_update(&packed_refs->validity, fileno(f));
 
-- 
2.11.0

[PATCH v2 22/25] refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA`

[Michael Haggerty]

Instead of handling `GIT_REF_PARANOIA` in
`files_ref_iterator_begin()`, handle it in
`refs_ref_iterator_begin()`, where it will cover all reference stores.

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

diff --git a/refs.c b/refs.c
index d1c781d94e..f0685c9251 100644
--- a/refs.c
+++ b/refs.c
@@ -1259,6 +1259,11 @@ struct ref_iterator *refs_ref_iterator_begin(
 {
 	struct ref_iterator *iter;
 
+	if (ref_paranoia < 0)
+		ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
+	if (ref_paranoia)
+		flags |= DO_FOR_EACH_INCLUDE_BROKEN;
+
 	iter = refs->be->iterator_begin(refs, prefix, flags);
 
 	/*
diff --git a/refs/files-backend.c b/refs/files-backend.c
index dbfd03f989..5de36fc335 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1074,15 +1074,12 @@ static struct ref_iterator *files_ref_iterator_begin(
 	struct ref_iterator *loose_iter, *packed_iter;
 	struct files_ref_iterator *iter;
 	struct ref_iterator *ref_iterator;
+	unsigned int required_flags = REF_STORE_READ;
 
-	if (ref_paranoia < 0)
-		ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
-	if (ref_paranoia)
-		flags |= DO_FOR_EACH_INCLUDE_BROKEN;
+	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN))
+		required_flags |= REF_STORE_ODB;
 
-	refs = files_downcast(ref_store,
-			      REF_STORE_READ | (ref_paranoia ? 0 : REF_STORE_ODB),
-			      "ref_iterator_begin");
+	refs = files_downcast(ref_store, required_flags, "ref_iterator_begin");
 
 	iter = xcalloc(1, sizeof(*iter));
 	ref_iterator = &iter->base;
-- 
2.11.0

[PATCH v2 23/25] create_ref_entry(): remove `check_name` option

[Michael Haggerty]

Only one caller was using it, so move the check to that caller.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 12 ++++++++----
 refs/ref-cache.c     |  6 +-----
 refs/ref-cache.h     |  3 +--
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5de36fc335..d8b3f73147 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -291,7 +291,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)
 				oidclr(&oid);
 				flag |= REF_BAD_NAME | REF_ISBROKEN;
 			}
-			last = create_ref_entry(refname, &oid, flag, 0);
+			last = create_ref_entry(refname, &oid, flag);
 			if (peeled == PEELED_FULLY ||
 			    (peeled == PEELED_TAGS && starts_with(refname, "refs/tags/")))
 				last->flag |= REF_KNOWS_PEELED;
@@ -415,8 +415,12 @@ static void add_packed_ref(struct files_ref_store *refs,
 
 	if (!is_lock_file_locked(&refs->packed_refs_lock))
 		die("BUG: packed refs not locked");
+
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+		die("Reference has invalid format: '%s'", refname);
+
 	add_ref_entry(get_packed_ref_dir(packed_ref_cache),
-		      create_ref_entry(refname, oid, REF_ISPACKED, 1));
+		      create_ref_entry(refname, oid, REF_ISPACKED));
 }
 
 /*
@@ -493,7 +497,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 				flag |= REF_BAD_NAME | REF_ISBROKEN;
 			}
 			add_entry_to_dir(dir,
-					 create_ref_entry(refname.buf, &oid, flag, 0));
+					 create_ref_entry(refname.buf, &oid, flag));
 		}
 		strbuf_setlen(&refname, dirnamelen);
 		strbuf_setlen(&path, path_baselen);
@@ -1541,7 +1545,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 			oidcpy(&packed_entry->u.value.oid, iter->oid);
 		} else {
 			packed_entry = create_ref_entry(iter->refname, iter->oid,
-							REF_ISPACKED, 0);
+							REF_ISPACKED);
 			add_ref_entry(packed_refs, packed_entry);
 		}
 		oidclr(&packed_entry->u.value.peeled);
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index 6b11d9cd12..ec97f3a38a 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -32,14 +32,10 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry)
 }
 
 struct ref_entry *create_ref_entry(const char *refname,
-				   const struct object_id *oid, int flag,
-				   int check_name)
+				   const struct object_id *oid, int flag)
 {
 	struct ref_entry *ref;
 
-	if (check_name &&
-	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
-		die("Reference has invalid format: '%s'", refname);
 	FLEX_ALLOC_STR(ref, name, refname);
 	oidcpy(&ref->u.value.oid, oid);
 	oidclr(&ref->u.value.peeled);
diff --git a/refs/ref-cache.h b/refs/ref-cache.h
index fbfee7ce79..794f000fd3 100644
--- a/refs/ref-cache.h
+++ b/refs/ref-cache.h
@@ -185,8 +185,7 @@ struct ref_entry *create_dir_entry(struct ref_cache *cache,
 				   int incomplete);
 
 struct ref_entry *create_ref_entry(const char *refname,
-				   const struct object_id *oid, int flag,
-				   int check_name);
+				   const struct object_id *oid, int flag);
 
 /*
  * Return a pointer to a new `ref_cache`. Its top-level starts out
-- 
2.11.0

[PATCH v2 24/25] ref-filter: limit traversal to prefix

[Michael Haggerty]

From: Jeff King <peff@peff.net>

When we are matching refnames against a pattern, then we know that the
beginning of any refname that can match the pattern has to match the
part of the pattern up to the first glob character. For example, if
the pattern is `refs/heads/foo*bar`, then it can only match a
reference that has the prefix `refs/heads/foo`.

So pass that prefix to `for_each_fullref_in()`. This lets the ref code
avoid passing us the full set of refs, and in some cases avoid reading
them in the first place.

Note that this applies only when the `match_as_path` flag is set
(i.e., when `for-each-ref` is the caller), as the matching rules for
git-branch and git-tag are subtly different.

This could be generalized to the case of multiple patterns, but (a) it
probably doesn't come up that often, and (b) it is more awkward to
deal with multiple patterns (e.g., the patterns might not be
disjoint). So, since this is just an optimization, punt on the case of
multiple patterns.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 ref-filter.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 6cc93dcd9f..25ca56d62f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1665,6 +1665,68 @@ static int filter_pattern_match(struct ref_filter *filter, const char *refname)
 	return match_pattern(filter, refname);
 }
 
+/*
+ * Find the longest prefix of pattern we can pass to
+ * `for_each_fullref_in()`, namely the part of pattern preceding the
+ * first glob character. (Note that `for_each_fullref_in()` is
+ * perfectly happy working with a prefix that doesn't end at a
+ * pathname component boundary.)
+ */
+static void find_longest_prefix(struct strbuf *out, const char *pattern)
+{
+	const char *p;
+
+	for (p = pattern; *p && !is_glob_special(*p); p++)
+		;
+
+	strbuf_add(out, pattern, p - pattern);
+}
+
+/*
+ * This is the same as for_each_fullref_in(), but it tries to iterate
+ * only over the patterns we'll care about. Note that it _doesn't_ do a full
+ * pattern match, so the callback still has to match each ref individually.
+ */
+static int for_each_fullref_in_pattern(struct ref_filter *filter,
+				       each_ref_fn cb,
+				       void *cb_data,
+				       int broken)
+{
+	struct strbuf prefix = STRBUF_INIT;
+	int ret;
+
+	if (!filter->match_as_path) {
+		/*
+		 * in this case, the patterns are applied after
+		 * prefixes like "refs/heads/" etc. are stripped off,
+		 * so we have to look at everything:
+		 */
+		return for_each_fullref_in("", cb, cb_data, broken);
+	}
+
+	if (!filter->name_patterns[0]) {
+		/* no patterns; we have to look at everything */
+		return for_each_fullref_in("", cb, cb_data, broken);
+	}
+
+	if (filter->name_patterns[1]) {
+		/*
+		 * multiple patterns; in theory this could still work as long
+		 * as the patterns are disjoint. We'd just make multiple calls
+		 * to for_each_ref(). But if they're not disjoint, we'd end up
+		 * reporting the same ref multiple times. So let's punt on that
+		 * for now.
+		 */
+		return for_each_fullref_in("", cb, cb_data, broken);
+	}
+
+	find_longest_prefix(&prefix, filter->name_patterns[0]);
+
+	ret = for_each_fullref_in(prefix.buf, cb, cb_data, broken);
+	strbuf_release(&prefix);
+	return ret;
+}
+
 /*
  * Given a ref (sha1, refname), check if the ref belongs to the array
  * of sha1s. If the given ref is a tag, check if the given tag points
@@ -1911,7 +1973,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 		else if (filter->kind == FILTER_REFS_TAGS)
 			ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, broken);
 		else if (filter->kind & FILTER_REFS_ALL)
-			ret = for_each_fullref_in("", ref_filter_handler, &ref_cbdata, broken);
+			ret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata, broken);
 		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
 			head_ref(ref_filter_handler, &ref_cbdata);
 	}
-- 
2.11.0

[PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories

[Michael Haggerty]

When iterating over references, reference priming is used to make sure
that loose references are read into the ref-cache before packed
references, to avoid races. It used to be that the prefix passed to
reference iterators almost always ended in `/`, for example
`refs/heads/`. In that case, the priming code would read all loose
references under `find_containing_dir("refs/heads/")`, which is
"refs/heads/". That's just what we want.

But now that `ref-filter` knows how to pass refname prefixes to
`for_each_fullref_in()`, the prefix might come from user input; for
example,

    git for-each-ref refs/heads

Since the argument doesn't include a trailing slash, the reference
iteration code would prime all of the loose references under
`find_containing_dir("refs/heads")`, which is "refs/". Thus we would
unnecessarily read tags, remote-tracking references, etc., when the
user is only interested in branches.

It is a bit awkward to get around this problem. We can't just append a
slash to the argument, because we don't know ab initio whether an
argument like `refs/tags/release` corresponds to a single tag or to a
directory containing tags.

Moreover, until now a `prefix_ref_iterator` was used to make the final
decision about which references fall within the prefix (the
`cache_ref_iterator` only did a rough cut). This is also inefficient,
because the `prefix_ref_iterator` can't know, for example, that while
you are in a subdirectory that is completely within the prefix, you
don't have to do the prefix check.

So:

* Move the responsibility for doing the prefix check directly to
  `cache_ref_iterator`. This means that `cache_ref_iterator_begin()`
  never has to wrap its return value in a `prefix_ref_iterator`.

* Teach `cache_ref_iterator_begin()` (and `prime_ref_dir()`) to be
  stricter about what they iterate over and what directories they
  prime.

* Teach `cache_ref_iterator` to keep track of whether the current
  `cache_ref_iterator_level` is fully within the prefix. If so, skip
  the prefix checks entirely.

The main benefit of these optimizations is for loose references, since
packed references are always read all at once.

Note that after this change, `prefix_ref_iterator` is only ever used
for its trimming feature and not for its "prefix" feature. But I'm not
ripping out the latter yet, because it might be useful for another
patch series that I'm working on.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/ref-cache.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 84 insertions(+), 10 deletions(-)

diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index ec97f3a38a..fda3942dbe 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -312,11 +312,42 @@ static void sort_ref_dir(struct ref_dir *dir)
 	dir->sorted = dir->nr = i;
 }
 
+enum prefix_state {
+	/* All refs within the directory would match prefix: */
+	PREFIX_CONTAINS_DIR,
+
+	/* Some, but not all, refs within the directory might match prefix: */
+	PREFIX_WITHIN_DIR,
+
+	/* No refs within the directory could possibly match prefix: */
+	PREFIX_EXCLUDES_DIR
+};
+
 /*
- * Load all of the refs from `dir` (recursively) into our in-memory
- * cache.
+ * Return a `prefix_state` constant describing the relationship
+ * between the directory with the specified `dirname` and `prefix`.
  */
-static void prime_ref_dir(struct ref_dir *dir)
+static enum prefix_state overlaps_prefix(const char *dirname,
+					 const char *prefix)
+{
+	while (*prefix && *dirname == *prefix) {
+		dirname++;
+		prefix++;
+	}
+	if (!*prefix)
+		return PREFIX_CONTAINS_DIR;
+	else if (!*dirname)
+		return PREFIX_WITHIN_DIR;
+	else
+		return PREFIX_EXCLUDES_DIR;
+}
+
+/*
+ * Load all of the refs from `dir` (recursively) that could possibly
+ * contain references matching `prefix` into our in-memory cache. If
+ * `prefix` is NULL, prime unconditionally.
+ */
+static void prime_ref_dir(struct ref_dir *dir, const char *prefix)
 {
 	/*
 	 * The hard work of loading loose refs is done by get_ref_dir(), so we
@@ -327,8 +358,29 @@ static void prime_ref_dir(struct ref_dir *dir)
 	int i;
 	for (i = 0; i < dir->nr; i++) {
 		struct ref_entry *entry = dir->entries[i];
-		if (entry->flag & REF_DIR)
-			prime_ref_dir(get_ref_dir(entry));
+		if (!(entry->flag & REF_DIR)) {
+			/* Not a directory; no need to recurse. */
+		} else if (!prefix) {
+			/* Recurse in any case: */
+			prime_ref_dir(get_ref_dir(entry), NULL);
+		} else {
+			switch (overlaps_prefix(entry->name, prefix)) {
+			case PREFIX_CONTAINS_DIR:
+				/*
+				 * Recurse, and from here down we
+				 * don't have to check the prefix
+				 * anymore:
+				 */
+				prime_ref_dir(get_ref_dir(entry), NULL);
+				break;
+			case PREFIX_WITHIN_DIR:
+				prime_ref_dir(get_ref_dir(entry), prefix);
+				break;
+			case PREFIX_EXCLUDES_DIR:
+				/* No need to prime this directory. */
+				break;
+			}
+		}
 	}
 }
 
@@ -343,6 +395,8 @@ struct cache_ref_iterator_level {
 	 */
 	struct ref_dir *dir;
 
+	enum prefix_state prefix_state;
+
 	/*
 	 * The index of the current entry within dir (which might
 	 * itself be a directory). If index == -1, then the iteration
@@ -369,6 +423,13 @@ struct cache_ref_iterator {
 	/* The number of levels that have been allocated on the stack */
 	size_t levels_alloc;
 
+	/*
+	 * Only include references with this prefix in the iteration.
+	 * The prefix is matched textually, without regard for path
+	 * component boundaries.
+	 */
+	const char *prefix;
+
 	/*
 	 * A stack of levels. levels[0] is the uppermost level that is
 	 * being iterated over in this iteration. (This is not
@@ -390,6 +451,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
 			&iter->levels[iter->levels_nr - 1];
 		struct ref_dir *dir = level->dir;
 		struct ref_entry *entry;
+		enum prefix_state entry_prefix_state;
 
 		if (level->index == -1)
 			sort_ref_dir(dir);
@@ -404,6 +466,14 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
 
 		entry = dir->entries[level->index];
 
+		if (level->prefix_state == PREFIX_WITHIN_DIR) {
+			entry_prefix_state = overlaps_prefix(entry->name, iter->prefix);
+			if (entry_prefix_state == PREFIX_EXCLUDES_DIR)
+				continue;
+		} else {
+			entry_prefix_state = level->prefix_state;
+		}
+
 		if (entry->flag & REF_DIR) {
 			/* push down a level */
 			ALLOC_GROW(iter->levels, iter->levels_nr + 1,
@@ -411,6 +481,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
 
 			level = &iter->levels[iter->levels_nr++];
 			level->dir = get_ref_dir(entry);
+			level->prefix_state = entry_prefix_state;
 			level->index = -1;
 		} else {
 			iter->base.refname = entry->name;
@@ -496,10 +567,10 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,
 		dir = find_containing_dir(dir, prefix, 0);
 	if (!dir)
 		/* There's nothing to iterate over. */
-		return  empty_ref_iterator_begin();
+		return empty_ref_iterator_begin();
 
 	if (prime_dir)
-		prime_ref_dir(dir);
+		prime_ref_dir(dir, prefix);
 
 	iter = xcalloc(1, sizeof(*iter));
 	ref_iterator = &iter->base;
@@ -511,9 +582,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,
 	level->index = -1;
 	level->dir = dir;
 
-	if (prefix && *prefix)
-		ref_iterator = prefix_ref_iterator_begin(ref_iterator,
-							 prefix, 0);
+	if (prefix && *prefix) {
+		iter->prefix = xstrdup(prefix);
+		level->prefix_state = PREFIX_WITHIN_DIR;
+	} else {
+		level->prefix_state = PREFIX_CONTAINS_DIR;
+	}
 
 	return ref_iterator;
 }
-- 
2.11.0

Re: [PATCH v2 00/25] Prepare to separate out a packed_ref_store

[Stefan Beller]

On Mon, May 22, 2017 at 7:17 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> This is the second iteration of a patch series that prepares the
> ground for separating out a `packed_ref_store` and then for changing
> `packed-refs` to be read using `mmap()`. Thanks to Peff, Junio,
> Stefan, Brandon, and Johannes for their feedback about v1 [1]. I think
> I have addressed all of your comments.
>
> Changes since v1:
>
> * Since v1, branch `bc/object-id` has been merged to `next`, and it
>   has lots of conflicts with these changes. So I rebased this branch
>   onto a merge of `master` and `bc/object-id`. (I hope this makes
>   Junio's job easier.) This unfortunately causes a bit of tbdiff noise
>   between v1 and v2.
>
> * Patch [01/25]: in t3600, register the `test_when_finished` command
>   before executing `chmod a-w`.
>
> * Patch [04/25] (new patch): convert a few `die("internal error: ...")`
>   to `die("BUG: ...")`.
>
> * Patch [05/25]: Use `strlen()` rather than `memchr()` to check the
>   trim length, and `die()` rather than skipping if it is longer than
>   the reference name.
>
> * Patch [08/25]: Name the log message arguments `msg` for consistency
>   with existing code.
>
> * Patch [10/25]: Rename the new member from `packlock` to
>   `packed_refs_lock`.
>
> * Patch [13/25] (new patch): Move the check for valid
>   `transaction->state` from `files_transaction_commit()` to
>   `ref_transaction_commit()`.
>
> * Patch [14/25]:
>
>   * Add more sanity checks of `transaction->state`.
>
>   * Don't add `ref_transaction_finish()` to the public API. Instead,
>     teach `ref_transaction_commit()` to do the right thing whether or
>     not `ref_transaction_prepare()` has been called.
>
>   * Add and improve docstrings.
>
>   * Allow `ref_transaction_abort()` to be called even before
>     `ref_transaction_prepare()` (in which case it just calls
>     `ref_transaction_free()`).
>
> * Lots of improvements to commit messages and comments, mostly to
>   clarify points that reviewers asked about.
>
> These changes (along with the merge commit that they are based on) are
> also available as branch `packed-ref-store-prep` in my GitHub fork
> [2]. If you'd like to see a preview of the rest of the changes (which
> works but is not yet polished), checkout the `mmap-packed-refs` branch
> from the same place.

I have read this version and quite like it. I did not have any nits to remark.

Thanks,
Stefan

Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories

[Jeff King]

On Mon, May 22, 2017 at 04:17:55PM +0200, Michael Haggerty wrote:

> So:
> 
> * Move the responsibility for doing the prefix check directly to
>   `cache_ref_iterator`. This means that `cache_ref_iterator_begin()`
>   never has to wrap its return value in a `prefix_ref_iterator`.
> 
> * Teach `cache_ref_iterator_begin()` (and `prime_ref_dir()`) to be
>   stricter about what they iterate over and what directories they
>   prime.
> 
> * Teach `cache_ref_iterator` to keep track of whether the current
>   `cache_ref_iterator_level` is fully within the prefix. If so, skip
>   the prefix checks entirely.

As promised, I came back to this one with a more careful eye. These
changes all make sense to me, and the implementation matches.

My only question is:

> @@ -511,9 +582,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,
>  	level->index = -1;
>  	level->dir = dir;
>  
> -	if (prefix && *prefix)
> -		ref_iterator = prefix_ref_iterator_begin(ref_iterator,
> -							 prefix, 0);
> +	if (prefix && *prefix) {
> +		iter->prefix = xstrdup(prefix);
> +		level->prefix_state = PREFIX_WITHIN_DIR;
> +	} else {
> +		level->prefix_state = PREFIX_CONTAINS_DIR;
> +	}

Who frees the prefix? Does this need:

diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index fda3942db..a3efc5c51 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -542,6 +542,7 @@ static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)
 	struct cache_ref_iterator *iter =
 		(struct cache_ref_iterator *)ref_iterator;
 
+	free(iter->prefix);
 	free(iter->levels);
 	base_ref_iterator_free(ref_iterator);
 	return ITER_DONE;

-Peff

Re: [PATCH v2 00/25] Prepare to separate out a packed_ref_store

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> * Since v1, branch `bc/object-id` has been merged to `next`, and it
>   has lots of conflicts with these changes. So I rebased this branch
>   onto a merge of `master` and `bc/object-id`. (I hope this makes
>   Junio's job easier.) This unfortunately causes a bit of tbdiff noise
>   between v1 and v2.

Heh, that gives me an excellent excuse to procrastinate, as I am
planning to merge that topic to 'master' by the end of this week ;-)

Jokes aside...

> * Patch [01/25]: in t3600, register the `test_when_finished` command
>   before executing `chmod a-w`.
>
> * Patch [04/25] (new patch): convert a few `die("internal error: ...")`
>   to `die("BUG: ...")`.
>
> * Patch [05/25]: Use `strlen()` rather than `memchr()` to check the
>   trim length, and `die()` rather than skipping if it is longer than
>   the reference name.
>
> * Patch [08/25]: Name the log message arguments `msg` for consistency
>   with existing code.
>
> * Patch [10/25]: Rename the new member from `packlock` to
>   `packed_refs_lock`.
>
> * Patch [13/25] (new patch): Move the check for valid
>   `transaction->state` from `files_transaction_commit()` to
>   `ref_transaction_commit()`.

All of these feel familiar ;-)

> * Patch [14/25]:
>
>   * Add more sanity checks of `transaction->state`.
>
>   * Don't add `ref_transaction_finish()` to the public API. Instead,
>     teach `ref_transaction_commit()` to do the right thing whether or
>     not `ref_transaction_prepare()` has been called.
>
>   * Add and improve docstrings.
>
>   * Allow `ref_transaction_abort()` to be called even before
>     `ref_transaction_prepare()` (in which case it just calls
>     `ref_transaction_free()`).
>
> * Lots of improvements to commit messages and comments, mostly to
>   clarify points that reviewers asked about.

Overall this looked like quite a quality work to me.

> These changes (along with the merge commit that they are based on) are
> also available as branch `packed-ref-store-prep` in my GitHub fork
> [2]. If you'd like to see a preview of the rest of the changes (which
> works but is not yet polished), checkout the `mmap-packed-refs` branch
> from the same place.

Thanks.

Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories

[Michael Haggerty]

On 05/23/2017 09:45 PM, Jeff King wrote:
> On Mon, May 22, 2017 at 04:17:55PM +0200, Michael Haggerty wrote:
> 
>> So:
>>
>> * Move the responsibility for doing the prefix check directly to
>>   `cache_ref_iterator`. This means that `cache_ref_iterator_begin()`
>>   never has to wrap its return value in a `prefix_ref_iterator`.
>>
>> * Teach `cache_ref_iterator_begin()` (and `prime_ref_dir()`) to be
>>   stricter about what they iterate over and what directories they
>>   prime.
>>
>> * Teach `cache_ref_iterator` to keep track of whether the current
>>   `cache_ref_iterator_level` is fully within the prefix. If so, skip
>>   the prefix checks entirely.
> 
> As promised, I came back to this one with a more careful eye. These
> changes all make sense to me, and the implementation matches.
> 
> My only question is:
> 
>> @@ -511,9 +582,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,
>>  	level->index = -1;
>>  	level->dir = dir;
>>  
>> -	if (prefix && *prefix)
>> -		ref_iterator = prefix_ref_iterator_begin(ref_iterator,
>> -							 prefix, 0);
>> +	if (prefix && *prefix) {
>> +		iter->prefix = xstrdup(prefix);
>> +		level->prefix_state = PREFIX_WITHIN_DIR;
>> +	} else {
>> +		level->prefix_state = PREFIX_CONTAINS_DIR;
>> +	}
> 
> Who frees the prefix? Does this need:
> 
> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> index fda3942db..a3efc5c51 100644
> --- a/refs/ref-cache.c
> +++ b/refs/ref-cache.c
> @@ -542,6 +542,7 @@ static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)
>  	struct cache_ref_iterator *iter =
>  		(struct cache_ref_iterator *)ref_iterator;
>  
> +	free(iter->prefix);
>  	free(iter->levels);
>  	base_ref_iterator_free(ref_iterator);
>  	return ITER_DONE;

Yes, you are right. Thanks for catching this.

Note: it has to be

        free((char *)iter->prefix);

because `prefix` is const.

Junio, if a reroll is not needed for other reasons, would you mind
squashing this into the last patch of my series?

Michael

Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Junio, if a reroll is not needed for other reasons, would you mind
> squashing this into the last patch of my series?

Will do, but won't happen until morning in Tokyo.  I've just
finished today's integration cycle.

Thanks.