[PATCH 00/23] Prepare to separate out a packed_ref_store

[PATCH 00/23] Prepare to separate out a packed_ref_store

[Michael Haggerty]

This patch series is the next leg on a journey towards reading
`packed-refs` using `mmap()`, the most interesting aspect of which is
that we will often be able to avoid having to read the whole
`packed-refs` file if we only need a subset of references.

The first leg of the journey was separating out the reference cache
into a separate module [1]. That branch is already merged to master.

This patch series prepares the ground for separating out a
`packed_ref_store`, but doesn't yet take that step. (As you can see,
it's a long enough patch series already!) It's kind of a grab bag of
cleanup patches plus work to decouple the packed-refs handling code
from the rest of `files_ref_store`. Some highlights:

* Patch 07/23 adds a log message parameter to `refs_delete_refs()` and
  `delete_refs()`, for consistency with other reference-changing
  operations. Even though `files_ref_store` is incapable of storing
  reflogs for deleted references, that is no reason that the API
  shouldn't admit the possibility for future backends.

* Patch 12/23 breaks `ref_transaction_commit()` into multiple
  functions:

  * `ref_transaction_prepare()`: do pre-checks, obtain locks, etc.; do
    everything possible to make sure that the reference update will be
    successful.

  * `ref_transaction_finish()`: actually commit a prepared
    transaction.

  * `ref_transaction_abort()`: abort a prepared transaction.

  This separation will be useful for supporting a "compound" reference
  store composed of multiple reference stores that work together
  (i.e., one for loose refs and one for packed refs).

* Patch 17/23 changes `get_packed_ref_cache()` to skip `lstat()`ing
  the packed-refs file (to check its freshness) if we already have it
  locked.

* Patch 19/23 fixes the error handling in `read_packed_refs()`: if
  `fopen()` fails due to `ENOENT`, then there are no packed refs. But
  if it fails for another reason, that is a problem that should be
  reported to the user.

* Patch 22/23 (by Peff) changes ref-filter to attempt to limit the
  reference traversal to a prefix, if there is a single
  `match_as_path` pattern that starts with non-glob characters. This
  limits the number of loose references that need to be read when
  processing a command like `git for-each-ref refs/heads/`.

* Patch 23/23 makes `cache_ref_iterator` smarter about avoiding
  "priming" directories of loose references that won't be needed,
  further reducing the number of loose references that need to be read
  in some cases of iterating over references.

These changes 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.1490026594.git.mhagger@alum.mit.edu/
    http://public-inbox.org/git/cover.1490966385.git.mhagger@alum.mit.edu/
    http://public-inbox.org/git/cover.1492323985.git.mhagger@alum.mit.edu/

[2] https://github.com/mhagger/git

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

Michael Haggerty (22):
  t3600: clean up permissions test properly
  refs.h: clarify docstring for the ref_transaction_update()-related fns
  ref_iterator_begin_fn(): fix docstring
  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 `logmsg` 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(): break into multiple functions
  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                   |  62 ++++++++-
 refs.c                         |  83 ++++++++++--
 refs.h                         |  62 ++++++++-
 refs/files-backend.c           | 300 +++++++++++++++++++++++++----------------
 refs/iterator.c                |  14 +-
 refs/ref-cache.c               |  99 +++++++++++---
 refs/ref-cache.h               |   6 +-
 refs/refs-internal.h           |  56 ++++++--
 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, 538 insertions(+), 169 deletions(-)

-- 
2.11.0

[PATCH 01/23] 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_must_fail` 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..4a35c378c8 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -98,8 +98,8 @@ embedded'"
 
 test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
 	chmod a-w . &&
-	test_must_fail git rm -f baz &&
-	chmod 775 .
+	test_when_finished "chmod 775 ." &&
+	test_must_fail git rm -f baz
 '
 
 test_expect_success \
-- 
2.11.0

[PATCH 02/23] 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 d18ef47128..a7d7b1afdf 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 03/23] 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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 12cf4e4718..e5f6bb2047 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -515,8 +515,8 @@ 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
+ * Iterate over the references in the specified ref_store whose names
+ * start with the specified prefix. If prefix is NULL or the empty
  * string, iterate over all references in the submodule.
  */
 typedef struct ref_iterator *ref_iterator_begin_fn(
-- 
2.11.0

[PATCH 04/23] 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. Skip over any references whose names
are not longer than `trim`.

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

diff --git a/refs/iterator.c b/refs/iterator.c
index bce1f192f7..f33d1b3a39 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -292,7 +292,19 @@ 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) {
+			/*
+			 * If there wouldn't be at least one character
+			 * left in the refname after trimming, skip
+			 * over it:
+			 */
+			if (memchr(iter->iter0->refname, '\0', iter->trim + 1))
+				continue;
+			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 05/23] refs_ref_iterator_begin(): don't check prefixes redundantly

[Michael Haggerty]

The backend already takes care of 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 26d40f9927..f4a485cd8a 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 06/23] refs: use `size_t` indexes when iterating over ref transaction updates

[Michael Haggerty]

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 f4a485cd8a..ea8233c67d 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 4925e698d8..893029f9dc 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2862,7 +2862,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;
@@ -3069,7 +3070,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 07/23] ref_store: take `logmsg` 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. Let's add them now before the interface
becomes truly polymorphic and increases the work.

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 5f2c2ab23e..f51314f432 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -940,7 +940,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 ea8233c67d..14dec88e7f 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 a7d7b1afdf..b2d9626be6 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 893029f9dc..7708994e82 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1607,7 +1607,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 =
@@ -1639,7 +1639,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 e5f6bb2047..9ed4387c8c 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..3eb78467c4 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 *logmsg = *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, logmsg, &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 08/23] 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 09/23] 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 `packlock` 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 7708994e82..e7ae5538cc 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 *packlock;
 };
 
 /* 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->packlock)
 			die("internal error: packed-ref cache cleared while locked");
 		refs->packed = NULL;
 		release_packed_ref_cache(packed_refs);
@@ -408,7 +406,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->packlock)
 		die("internal error: packed refs not locked");
 	add_ref_entry(get_packed_ref_dir(packed_ref_cache),
 		      create_ref_entry(refname, sha1, REF_ISPACKED, 1));
@@ -1312,7 +1310,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->packlock = &packlock;
 	/* Increment the reference count to prevent it from being freed: */
 	acquire_packed_ref_cache(packed_ref_cache);
 	return 0;
@@ -1335,10 +1333,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->packlock)
 		die("internal error: packed-refs not locked");
 
-	out = fdopen_lock_file(packed_ref_cache->lock, "w");
+	out = fdopen_lock_file(refs->packlock, "w");
 	if (!out)
 		die_errno("unable to fdopen packed-refs descriptor");
 
@@ -1356,11 +1354,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->packlock)) {
 		save_errno = errno;
 		error = -1;
 	}
-	packed_ref_cache->lock = NULL;
+	refs->packlock = NULL;
 	release_packed_ref_cache(packed_ref_cache);
 	errno = save_errno;
 	return error;
@@ -1378,10 +1376,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->packlock)
 		die("internal error: packed-refs not locked");
-	rollback_lock_file(packed_ref_cache->lock);
-	packed_ref_cache->lock = NULL;
+	rollback_lock_file(refs->packlock);
+	refs->packlock = NULL;
 	release_packed_ref_cache(packed_ref_cache);
 	clear_packed_ref_cache(refs);
 }
-- 
2.11.0

[PATCH 10/23] 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 e7ae5538cc..ba0ad0aa44 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 *packlock;
+	struct lock_file packlock;
 };
 
-/* 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->packlock)
+		if (is_lock_file_locked(&refs->packlock))
 			die("internal error: packed-ref cache cleared while locked");
 		refs->packed = NULL;
 		release_packed_ref_cache(packed_refs);
@@ -406,7 +402,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->packlock)
+	if (!is_lock_file_locked(&refs->packlock))
 		die("internal error: packed refs not locked");
 	add_ref_entry(get_packed_ref_dir(packed_ref_cache),
 		      create_ref_entry(refname, sha1, REF_ISPACKED, 1));
@@ -1300,7 +1296,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->packlock, files_packed_refs_path(refs),
 			    flags, timeout_value) < 0)
 		return -1;
 	/*
@@ -1310,7 +1306,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->packlock = &packlock;
 	/* Increment the reference count to prevent it from being freed: */
 	acquire_packed_ref_cache(packed_ref_cache);
 	return 0;
@@ -1333,10 +1328,10 @@ static int commit_packed_refs(struct files_ref_store *refs)
 
 	files_assert_main_repository(refs, "commit_packed_refs");
 
-	if (!refs->packlock)
+	if (!is_lock_file_locked(&refs->packlock))
 		die("internal error: packed-refs not locked");
 
-	out = fdopen_lock_file(refs->packlock, "w");
+	out = fdopen_lock_file(&refs->packlock, "w");
 	if (!out)
 		die_errno("unable to fdopen packed-refs descriptor");
 
@@ -1354,11 +1349,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->packlock)) {
+	if (commit_lock_file(&refs->packlock)) {
 		save_errno = errno;
 		error = -1;
 	}
-	refs->packlock = NULL;
 	release_packed_ref_cache(packed_ref_cache);
 	errno = save_errno;
 	return error;
@@ -1376,10 +1370,9 @@ static void rollback_packed_refs(struct files_ref_store *refs)
 
 	files_assert_main_repository(refs, "rollback_packed_refs");
 
-	if (!refs->packlock)
+	if (!is_lock_file_locked(&refs->packlock))
 		die("internal error: packed-refs not locked");
-	rollback_lock_file(refs->packlock);
-	refs->packlock = NULL;
+	rollback_lock_file(&refs->packlock);
 	release_packed_ref_cache(packed_ref_cache);
 	clear_packed_ref_cache(refs);
 }
-- 
2.11.0

[PATCH 11/23] files_transaction_cleanup(): new helper function

[Michael Haggerty]

Extract function from `files_transaction_commit()`. It will soon have
another caller.

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 ba0ad0aa44..7ddd4f87d5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2846,6 +2846,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)
@@ -2868,10 +2889,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
@@ -3017,15 +3036,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 12/23] ref_transaction_commit(): break into multiple functions

[Michael Haggerty]

Break the function `ref_transaction_commit()` into two functions,
`ref_transaction_prepare()` and `ref_transaction_finish()`, with a
third function, `ref_transaction_abort()`, that can be used to abort
the transaction. Break up the `ref_store` methods similarly.

This split will make it easier to implement compound reference stores,
where a transaction might have to span multiple underlying stores. In
that case, we would first want to "prepare" the sub-transactions in
each of the reference stores. If any of the "prepare" steps fails, we
would "abort" all of the sub-transactions. Only if all of the
"prepare" steps succeed would we "finish" each of them.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               | 34 ++++++++++++++++++++++---
 refs.h               | 37 ++++++++++++++++++++++++++-
 refs/files-backend.c | 71 +++++++++++++++++++++++++++++++++++++++++-----------
 refs/refs-internal.h | 42 ++++++++++++++++++++++++-------
 4 files changed, 157 insertions(+), 27 deletions(-)

diff --git a/refs.c b/refs.c
index 14dec88e7f..689362db1e 100644
--- a/refs.c
+++ b/refs.c
@@ -1689,8 +1689,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;
 
@@ -1700,7 +1700,35 @@ 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_finish(struct ref_transaction *transaction,
+			   struct strbuf *err)
+{
+	struct ref_store *refs = transaction->ref_store;
+
+	return refs->be->transaction_finish(refs, transaction, err);
+}
+
+int ref_transaction_abort(struct ref_transaction *transaction,
+			  struct strbuf *err)
+{
+	struct ref_store *refs = transaction->ref_store;
+
+	return refs->be->transaction_abort(refs, transaction, err);
+}
+
+int ref_transaction_commit(struct ref_transaction *transaction,
+			   struct strbuf *err)
+{
+	int ret;
+
+	ret = ref_transaction_prepare(transaction, err);
+	if (ret)
+		return ret;
+
+	return ref_transaction_finish(transaction, err);
 }
 
 int refs_verify_refname_available(struct ref_store *refs,
diff --git a/refs.h b/refs.h
index b2d9626be6..4139e917f5 100644
--- a/refs.h
+++ b/refs.h
@@ -525,7 +525,10 @@ int ref_transaction_verify(struct ref_transaction *transaction,
 
 /*
  * Commit all of the changes that have been queued in transaction, as
- * atomically as possible.
+ * atomically as possible. This is equivalent to calling
+ * `ref_transaction_prepare()` then `ref_transaction_finish()` (see
+ * below), which you can call yourself if you want finer control over
+ * reference updates.
  *
  * Returns 0 for success, or one of the below error codes for errors.
  */
@@ -536,6 +539,38 @@ int ref_transaction_verify(struct ref_transaction *transaction,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err);
 
+/*
+ * 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. Anything that this function does should be
+ * able to be finished up by calling `ref_transaction_finish()` or
+ * rolled back by calling `ref_transaction_abort()`.
+ *
+ * On success, return 0 and leave the transaction in "prepared" state.
+ * On failure, abort the transaction 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);
+
+/*
+ * Perform the final commit of `transaction`, which has already been
+ * prepared.
+ */
+int ref_transaction_finish(struct ref_transaction *transaction,
+			   struct strbuf *err);
+
+/*
+ * Abort `transaction`, which has already been prepared.
+ */
+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
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7ddd4f87d5..f48409132d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2867,27 +2867,24 @@ 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);
 
 	if (transaction->state != REF_TRANSACTION_OPEN)
-		die("BUG: commit called for transaction that is not open");
+		die("BUG: prepare called for transaction that is not open");
 
 	if (!transaction->nr)
 		goto cleanup;
@@ -2949,6 +2946,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];
@@ -2956,7 +2955,41 @@ 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->state != REF_TRANSACTION_PREPARED)
+		die("BUG: finish called for transaction that is not prepared");
+
+	if (!transaction->nr) {
+		transaction->state = REF_TRANSACTION_CLOSED;
+		return 0;
 	}
 
 	/* Perform updates first so live commits remain referenced */
@@ -3037,7 +3070,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];
@@ -3054,13 +3086,22 @@ 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)
+{
+	if (transaction->state != REF_TRANSACTION_PREPARED)
+		die("BUG: abort called for transaction that is not prepared");
+
+	files_transaction_cleanup(transaction);
+	return 0;
+}
+
 static int ref_present(const char *refname,
 		       const struct object_id *oid, int flags, void *cb_data)
 {
@@ -3327,7 +3368,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 9ed4387c8c..2ff65d3ebd 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -185,17 +185,26 @@ 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 in a valid state and new updates can be
+ *         added to it. An OPEN transaction can be prepared or
+ *         committed.
+ *
+ * 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.
+ *
+ * CLOSED: The transaction is no longer active. No other operations
+ *         than free can be used on it in this state. A transaction
+ *         becomes closed if there is a failure while building the
+ *         transaction, if an active transaction is committed, or if a
+ *         prepared transaction is finished.
  */
 enum ref_transaction_state {
 	REF_TRANSACTION_OPEN   = 0,
-	REF_TRANSACTION_CLOSED = 1
+	REF_TRANSACTION_PREPARED = 1,
+	REF_TRANSACTION_CLOSED = 2
 };
 
 /*
@@ -497,6 +506,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);
@@ -599,7 +620,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 13/23] 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 689362db1e..43d65bc9c6 100644
--- a/refs.c
+++ b/refs.c
@@ -1689,6 +1689,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 f48409132d..d7596b5222 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2524,23 +2524,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 2ff65d3ebd..52350c5301 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 14/23] ref_update_reject_duplicates(): use `size_t` rather than `int`

[Michael Haggerty]

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 43d65bc9c6..ffc9bd0be5 100644
--- a/refs.c
+++ b/refs.c
@@ -1692,7 +1692,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 15/23] 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 ffc9bd0be5..68a0872562 100644
--- a/refs.c
+++ b/refs.c
@@ -1696,13 +1696,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 16/23] 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 d7596b5222..c549a8d956 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1467,6 +1467,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 =
@@ -1488,21 +1514,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 17/23] 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 c549a8d956..a910c44f38 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -354,13 +354,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->packlock) &&
 	    !stat_validity_check(&refs->packed->validity, packed_refs_file))
 		clear_packed_ref_cache(refs);
 
@@ -1300,10 +1305,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 18/23] 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 a910c44f38..6a037e1d61 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -221,7 +221,9 @@ static const char *parse_ref_line(struct strbuf *line, unsigned char *sha1)
 }
 
 /*
- * 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:
@@ -247,12 +249,26 @@ static const char *parse_ref_line(struct strbuf *line, unsigned char *sha1)
  *      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) {
 		unsigned char sha1[20];
 		const char *refname;
@@ -299,7 +315,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)
@@ -369,20 +388,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 ffdc54f3f0..83e6c2dd2a 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 19/23] 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 | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6a037e1d61..eb74d1119a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -263,8 +263,19 @@ 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("couldn't read %s: %s",
+			    packed_refs_file, strerror(errno));
+		}
+	}
 
 	stat_validity_update(&packed_refs->validity, fileno(f));
 
-- 
2.11.0

[PATCH 20/23] 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 68a0872562..f4b95109af 100644
--- a/refs.c
+++ b/refs.c
@@ -1246,6 +1246,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 eb74d1119a..d3716c3a6f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1087,15 +1087,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 21/23] 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 d3716c3a6f..c36194be02 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -304,7 +304,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)
 				hashclr(sha1);
 				flag |= REF_BAD_NAME | REF_ISBROKEN;
 			}
-			last = create_ref_entry(refname, sha1, flag, 0);
+			last = create_ref_entry(refname, sha1, flag);
 			if (peeled == PEELED_FULLY ||
 			    (peeled == PEELED_TAGS && starts_with(refname, "refs/tags/")))
 				last->flag |= REF_KNOWS_PEELED;
@@ -428,8 +428,12 @@ static void add_packed_ref(struct files_ref_store *refs,
 
 	if (!is_lock_file_locked(&refs->packlock))
 		die("internal error: 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, sha1, REF_ISPACKED, 1));
+		      create_ref_entry(refname, sha1, REF_ISPACKED));
 }
 
 /*
@@ -506,7 +510,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, sha1, flag, 0));
+					 create_ref_entry(refname.buf, sha1, flag));
 		}
 		strbuf_setlen(&refname, dirnamelen);
 		strbuf_setlen(&path, path_baselen);
@@ -1554,7 +1558,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->hash,
-							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 6059362f1d..ab76b34e30 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 unsigned char *sha1, int flag,
-				   int check_name)
+				   const unsigned char *sha1, 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);
 	hashcpy(ref->u.value.oid.hash, sha1);
 	oidclr(&ref->u.value.peeled);
diff --git a/refs/ref-cache.h b/refs/ref-cache.h
index 83e6c2dd2a..9a1ceeb7a5 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 unsigned char *sha1, int flag,
-				   int check_name);
+				   const unsigned char *sha1, int flag);
 
 /*
  * Return a pointer to a new `ref_cache`. Its top-level starts out
-- 
2.11.0

[PATCH 22/23] ref-filter: limit traversal to prefix

[Michael Haggerty]

From: Jeff King <peff@peff.net>

When we are matching refnames "as path" 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.

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 | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 1fc5e9970d..54cc6bcc13 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1665,6 +1665,66 @@ 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.
+ */
+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 +1971,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 23/23] 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 | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 83 insertions(+), 10 deletions(-)

diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index ab76b34e30..257c3f9cd0 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,28 @@ 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:
+				break;
+			}
+		}
 	}
 }
 
@@ -343,6 +394,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 +422,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 +450,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 +465,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 +480,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 +566,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 +581,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 01/23] t3600: clean up permissions test properly

[Jeff King]

On Wed, May 17, 2017 at 02:05:24PM +0200, Michael Haggerty wrote:

> 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_must_fail` block so that it can't
> be skipped.

This should be `test_when_finished` in the final paragraph, right?

The patch itself looks obviously fine.

-Peff

Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much

[Jeff King]

On Wed, May 17, 2017 at 02:05:27PM +0200, Michael Haggerty wrote:

> diff --git a/refs/iterator.c b/refs/iterator.c
> index bce1f192f7..f33d1b3a39 100644
> --- a/refs/iterator.c
> +++ b/refs/iterator.c
> @@ -292,7 +292,19 @@ 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) {
> +			/*
> +			 * If there wouldn't be at least one character
> +			 * left in the refname after trimming, skip
> +			 * over it:
> +			 */
> +			if (memchr(iter->iter0->refname, '\0', iter->trim + 1))
> +				continue;

It took me a minute to figure out the logic here. You're looking for the
end-of-string within the trim boundary, which would be an indication
that the string itself is smaller than the boundary.

But what if it returns true, and the string really is shorter than the
trim size? That would mean we pass a size to memchr that is longer than
the buffer we pass. Is that legal?

I suspect it's undefined behavior according to the standard, though I'd
guess in practice it would be fine. But if I'm understanding it
correctly, this is the same check as:

  if (strlen(iter->iter0->refname) <= iter->trim)

which seems a lot more obvious to me and doesn't fall afoul of weird
standard issues. The only downside I see is that it would read to the
end of string when yours could stop at iter->trim bytes. I have no idea
if that would be measurable (it might even be faster because strlen()
only has one condition to loop on).

-Peff

Re: [PATCH 05/23] refs_ref_iterator_begin(): don't check prefixes redundantly

[Jeff King]

On Wed, May 17, 2017 at 02:05:28PM +0200, Michael Haggerty wrote:

> The backend already takes care of 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.

Hmm. So givne a refname like "refs/heads/foo" and a prefix like
"refs/heads/", would we trim down to "foo" and then further try to
remove "refs/heads" again?

That sounds like it's not just redundant, but a bug (albeit one that is
very unlikely to come up, unless you have "refs/heads/refs/heads/foo").

Or am I misunderstanding?

-Peff

Re: [PATCH 07/23] ref_store: take `logmsg` parameter when deleting references

[Jeff King]

On Wed, May 17, 2017 at 02:05:30PM +0200, Michael Haggerty wrote:

> 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. Let's add them now before the interface
> becomes truly polymorphic and increases the work.
> 
> 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(-)

Having carried a similar patch in GitHub's fork for many years (because
we maintain an audit log of all ref updates), I expected this to be
bigger. But I forgot that we did 755b49ae9 (delete_ref: accept a reflog
message argument, 2017-02-20) a few months ago, which already hit most
of the ref-deleting callers. This is just making the plural
delete_refs() interface match.

I think your reasoning above is sound by itself, but that gives an added
interface: we are making the delete_ref() and delete_refs() interfaces
consistent.

-Peff

Re: [PATCH 08/23] lockfile: add a new method, is_lock_file_locked()

[Jeff King]

On Wed, May 17, 2017 at 02:05:31PM +0200, Michael Haggerty wrote:

> It will soon prove useful.

Very mysterious...

-Peff

Re: [PATCH 09/23] files-backend: move `lock` member to `files_ref_store`

[Jeff King]

On Wed, May 17, 2017 at 02:05:32PM +0200, Michael Haggerty wrote:

> @@ -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 *packlock;

I'm glad to see you renamed this from just "lock", though the short
"pack" makes me think of packed objects. I don't know if
"packed_refs_lock" would be too verbose (I see it's just "packed" above,
so maybe "packed_lock").

</bikeshed>

-Peff

Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

[Jeff King]

On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:

> 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.

This made me wonder how we handle the locking for ref_stores besides the
main one (e.g., for submodules). The lockfile structs have to remain
valid for the length of the program. Previously those stores could have
xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
and leak their whole structs.

I suspect the answer is "we don't ever lock anything except the main ref
store because that is the only one we write to", so it doesn't matter
anyway.

-Peff

Re: [PATCH 11/23] files_transaction_cleanup(): new helper function

[Jeff King]

On Wed, May 17, 2017 at 02:05:34PM +0200, Michael Haggerty wrote:

> Extract function from `files_transaction_commit()`. It will soon have
> another caller.
> [...]
> @@ -2868,10 +2889,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;

This is slightly more than it says on the tin. I guess the reason is
that the cleanup function is going to learn about more than just
iterating over the transaction, and we'll want to trigger it even for
empty transactions.

-Peff

Re: [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures

[Jeff King]

On Wed, May 17, 2017 at 02:05:42PM +0200, Michael Haggerty wrote:

> 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.

I think there are may be other "OK" errno values here, like ENOTDIR.
That's probably pretty unlikely in practice, though, as we're opening a
file at the root of the $GIT_DIR here (so somebody would had to have
racily changed our paths). It's probably fine to just err on the side of
safety.

> +	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("couldn't read %s: %s",
> +			    packed_refs_file, strerror(errno));
> +		}

I think this could be die_errno().

-Peff

Re: [PATCH 20/23] refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA`

[Jeff King]

On Wed, May 17, 2017 at 02:05:43PM +0200, Michael Haggerty wrote:

> 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.

Good catch. This should definitely go at the outer-most layer of the ref
code.

-Peff

Re: [PATCH 22/23] ref-filter: limit traversal to prefix

[Jeff King]

On Wed, May 17, 2017 at 02:05:45PM +0200, Michael Haggerty wrote:

> From: Jeff King <peff@peff.net>

This patch did originate with me, but I know you had to fix several
things to integrate it in your series. So I'll review it anyway, and
give you full blame for any bugs. :)

> When we are matching refnames "as path" 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`.

That first sentence confused me as to what "as path" meant (I know
because I worked on this code, and even then it took me a minute to
parse it).

Maybe just "When we are matching refnames against a pattern" and then
later something like:

  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.

> +/*
> + * Find the longest prefix of pattern we can pass to
> + * for_each_fullref_in(), namely the part of pattern preceding the
> + * first glob character.
> + */
> +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);
> +}

If I were reviewing this from scratch, I'd probably ask whether it is OK
in:

  refs/heads/m*

to return "refs/heads/m" as the prefix, and not stop at the last
non-wildcard component ("refs/heads/"). But I happen to know we
discussed this off-list and you checked that for_each_ref and friends
are happy with an arbitrary prefix. But I'm calling it out here for
other reviewers.

> +/*
> + * 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,
> [...]

The rest of it looks good to me.

-Peff

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

[Jeff King]

On Wed, May 17, 2017 at 02:05:23PM +0200, Michael Haggerty wrote:

> This patch series is the next leg on a journey towards reading
> `packed-refs` using `mmap()`, the most interesting aspect of which is
> that we will often be able to avoid having to read the whole
> `packed-refs` file if we only need a subset of references.
> 
> The first leg of the journey was separating out the reference cache
> into a separate module [1]. That branch is already merged to master.
> 
> This patch series prepares the ground for separating out a
> `packed_ref_store`, but doesn't yet take that step. (As you can see,
> it's a long enough patch series already!) It's kind of a grab bag of
> cleanup patches plus work to decouple the packed-refs handling code
> from the rest of `files_ref_store`. Some highlights:

I dropped a few minor comments in individual patches, but it all looks
good to me up through patch 22. Your description in patch 23 makes
sense, but I was too frazzled by the end to carefully review the code
itself for that patch. I'll try to come back to it later.

-Peff

Re: [PATCH 01/23] t3600: clean up permissions test properly

[Michael Haggerty]

On 05/17/2017 02:42 PM, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:24PM +0200, Michael Haggerty wrote:
> 
>> 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_must_fail` block so that it can't
>> be skipped.
> 
> This should be `test_when_finished` in the final paragraph, right?

Yes, thanks. Will fix.

Michael

Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much

[Michael Haggerty]

On 05/17/2017 02:55 PM, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:27PM +0200, Michael Haggerty wrote:
> 
>> diff --git a/refs/iterator.c b/refs/iterator.c
>> index bce1f192f7..f33d1b3a39 100644
>> --- a/refs/iterator.c
>> +++ b/refs/iterator.c
>> @@ -292,7 +292,19 @@ 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) {
>> +			/*
>> +			 * If there wouldn't be at least one character
>> +			 * left in the refname after trimming, skip
>> +			 * over it:
>> +			 */
>> +			if (memchr(iter->iter0->refname, '\0', iter->trim + 1))
>> +				continue;
> 
> It took me a minute to figure out the logic here. You're looking for the
> end-of-string within the trim boundary, which would be an indication
> that the string itself is smaller than the boundary.
> 
> But what if it returns true, and the string really is shorter than the
> trim size? That would mean we pass a size to memchr that is longer than
> the buffer we pass. Is that legal?
> 
> I suspect it's undefined behavior according to the standard, though I'd
> guess in practice it would be fine. But if I'm understanding it
> correctly, this is the same check as:
> 
>   if (strlen(iter->iter0->refname) <= iter->trim)
> 
> which seems a lot more obvious to me and doesn't fall afoul of weird
> standard issues. The only downside I see is that it would read to the
> end of string when yours could stop at iter->trim bytes. I have no idea
> if that would be measurable (it might even be faster because strlen()
> only has one condition to loop on).

You are correct that I chose `memchr()` over `strlen()` to avoid
scanning a POTENTIALLY EXTREMELY LARGE NUMBER OF CHARACTERS past the
trim length, but of course in real life refnames aren't that long and
`strlen()` might actually be faster.

I *think* `memchr()` is technically OK:

> Implementations shall behave as if they read the memory byte by byte
from the beginning of the bytes pointed to by s and stop at the first
occurrence of c (if it is found in the initial n bytes).

But I agree that the `strlen()` version is also easier to read (I
actually had that version first). So I'll change it as you have suggested.

Thanks,
Michael

Re: [PATCH 05/23] refs_ref_iterator_begin(): don't check prefixes redundantly

[Michael Haggerty]

On 05/17/2017 02:59 PM, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:28PM +0200, Michael Haggerty wrote:
> 
>> The backend already takes care of 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.
> 
> Hmm. So givne a refname like "refs/heads/foo" and a prefix like
> "refs/heads/", would we trim down to "foo" and then further try to
> remove "refs/heads" again?

No, the prefix is used to choose which references to let through and the
trim value is then used to munge the refname. They are orthogonal,
though AFAIK the trim value is always set to 0 or to strlen(prefix).

`refs->be->iterator_begin()` only has the `prefix` feature; it doesn't
trim anything. So we can rely on it to discard the references that
aren't in `prefix`. All we need the `prefix_ref_iterator` for is the
trimming.

Michael

Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much

[Jeff King]

On Wed, May 17, 2017 at 04:11:15PM +0200, Michael Haggerty wrote:

> > I suspect it's undefined behavior according to the standard, though I'd
> > guess in practice it would be fine. But if I'm understanding it
> > correctly, this is the same check as:
> > 
> >   if (strlen(iter->iter0->refname) <= iter->trim)
> > 
> > which seems a lot more obvious to me and doesn't fall afoul of weird
> > standard issues. The only downside I see is that it would read to the
> > end of string when yours could stop at iter->trim bytes. I have no idea
> > if that would be measurable (it might even be faster because strlen()
> > only has one condition to loop on).
> 
> You are correct that I chose `memchr()` over `strlen()` to avoid
> scanning a POTENTIALLY EXTREMELY LARGE NUMBER OF CHARACTERS past the
> trim length, but of course in real life refnames aren't that long and
> `strlen()` might actually be faster.

Heh. Yeah, I actually did dig up a glibc thread that says that strlen()
is even faster than rawmemchr(). But they were discussing buffer sizes
of at least 1K. For refnames I doubt it matters much at all.

> I *think* `memchr()` is technically OK:
> 
> > Implementations shall behave as if they read the memory byte by byte
> from the beginning of the bytes pointed to by s and stop at the first
> occurrence of c (if it is found in the initial n bytes).

Interesting. I don't really see how else you'd implement it, so that
makes sense (your quote is from POSIX; I first looked in C99, but
couldn't find any similar language there).

-Peff

Re: [PATCH 07/23] ref_store: take `logmsg` parameter when deleting references

[Michael Haggerty]

On 05/17/2017 03:12 PM, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:30PM +0200, Michael Haggerty wrote:
> 
>> 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. Let's add them now before the interface
>> becomes truly polymorphic and increases the work.
>>
>> 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(-)
> 
> Having carried a similar patch in GitHub's fork for many years (because
> we maintain an audit log of all ref updates), I expected this to be
> bigger. But I forgot that we did 755b49ae9 (delete_ref: accept a reflog
> message argument, 2017-02-20) a few months ago, which already hit most
> of the ref-deleting callers. This is just making the plural
> delete_refs() interface match.
> 
> I think your reasoning above is sound by itself, but that gives an added
> interface: we are making the delete_ref() and delete_refs() interfaces
> consistent.

I think you meant s/interface/justification/, in which case I agree with
you and I'll mention it in v2. I also noticed that the parameters are
named inconsistently. I'll fix that too.

Michael

Re: [PATCH 07/23] ref_store: take `logmsg` parameter when deleting references

[Jeff King]

On Wed, May 17, 2017 at 05:01:41PM +0200, Michael Haggerty wrote:

> On 05/17/2017 03:12 PM, Jeff King wrote:
> > On Wed, May 17, 2017 at 02:05:30PM +0200, Michael Haggerty wrote:
> > 
> >> 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. Let's add them now before the interface
> >> becomes truly polymorphic and increases the work.
> >>
> >> 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(-)
> > 
> > Having carried a similar patch in GitHub's fork for many years (because
> > we maintain an audit log of all ref updates), I expected this to be
> > bigger. But I forgot that we did 755b49ae9 (delete_ref: accept a reflog
> > message argument, 2017-02-20) a few months ago, which already hit most
> > of the ref-deleting callers. This is just making the plural
> > delete_refs() interface match.
> > 
> > I think your reasoning above is sound by itself, but that gives an added
> > interface: we are making the delete_ref() and delete_refs() interfaces
> > consistent.
> 
> I think you meant s/interface/justification/, in which case I agree with
> you and I'll mention it in v2. I also noticed that the parameters are
> named inconsistently. I'll fix that too.

Oops, I think I meant "incentive". But yes, you managed to decipher my
rambling correctly.

-Peff

Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

[Michael Haggerty]

On 05/17/2017 03:17 PM, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:
> 
>> 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.
> 
> This made me wonder how we handle the locking for ref_stores besides the
> main one (e.g., for submodules). The lockfile structs have to remain
> valid for the length of the program. Previously those stores could have
> xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
> and leak their whole structs.
> 
> I suspect the answer is "we don't ever lock anything except the main ref
> store because that is the only one we write to", so it doesn't matter
> anyway.

Correct. If that ever changes, we'll be ready!

Michael

Re: [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures

[Michael Haggerty]

On 05/17/2017 03:28 PM, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:42PM +0200, Michael Haggerty wrote:
> 
>> 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.
> 
> I think there are may be other "OK" errno values here, like ENOTDIR.
> That's probably pretty unlikely in practice, though, as we're opening a
> file at the root of the $GIT_DIR here (so somebody would had to have
> racily changed our paths). It's probably fine to just err on the side of
> safety.

Yikes, I think if somebody swaps $GIT_DIR out from under a working git
command, the user would want to know about it. How would you possibly
recover, anyway?

>> +	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("couldn't read %s: %s",
>> +			    packed_refs_file, strerror(errno));
>> +		}
> 
> I think this could be die_errno().

Ah yes. I'll change it.

Thanks,
Michael

Re: [PATCH 09/23] files-backend: move `lock` member to `files_ref_store`

[Michael Haggerty]

On 05/17/2017 03:15 PM, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:32PM +0200, Michael Haggerty wrote:
> 
>> @@ -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 *packlock;
> 
> I'm glad to see you renamed this from just "lock", though the short
> "pack" makes me think of packed objects. I don't know if
> "packed_refs_lock" would be too verbose (I see it's just "packed" above,
> so maybe "packed_lock").
> 
> </bikeshed>

I'll rename it to `packed_refs_lock`.

Michael

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

[Stefan Beller]

On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> In particular, make it clear that they make copies of the sha1
> arguments.

A couple weeks ago we had plans on getting rid of SHA1 in
"the near future" IIRC.  Would it make sense to not go down
the SHA1 road further and document this in a more abstract way?

    s/SHA1/object name/

essentially, but I guess one of Brians future series' may pick this
up as well.

I am just hesitant to introduce more sha1-ism at this point.

Thanks,
Stefan

Re: [PATCH 06/23] refs: use `size_t` indexes when iterating over ref transaction updates

[Stefan Beller]

On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Now this would want to have some selling words for it?
I do not see an advantage of this patch as-is.

I mean technically we don't need a sign, so we use that extra bit
to be able to process transactions up to twice the size. But I doubt
that is the real reason. I'll read on, maybe a later patch will explain
why we do this here.

Stefan

Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

[Stefan Beller]

On Wed, May 17, 2017 at 6:17 AM, Jeff King <peff@peff.net> wrote:
> On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:
>
>> 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.
>
> This made me wonder how we handle the locking for ref_stores besides the
> main one (e.g., for submodules). The lockfile structs have to remain
> valid for the length of the program. Previously those stores could have
> xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
> and leak their whole structs.

+cc Brandon, who is eager to go down that road.

> I suspect the answer is "we don't ever lock anything except the main ref
> store because that is the only one we write to", so it doesn't matter
> anyway.
>
> -Peff


> @@ -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->packlock)
> +               if (is_lock_file_locked(&refs->packlock))
>                        die("internal error: packed-ref cache cleared while locked");

I think the error message needs adjustment here as well? Maybe

     die("internal error: packed refs locked in cleanup");

Thanks,
Stefan

Re: [PATCH 11/23] files_transaction_cleanup(): new helper function

[Stefan Beller]

On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Extract function from `files_transaction_commit()`. It will soon have
> another caller.

This sounds odd to me. Maybe it is missing words?
of s/function/the functionality to cleanup/

Code makes sense to me.

Stefan

Re: [PATCH 12/23] ref_transaction_commit(): break into multiple functions

[Stefan Beller]

On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Break the function `ref_transaction_commit()` into two functions,
> `ref_transaction_prepare()` and `ref_transaction_finish()`, with a
> third function, `ref_transaction_abort()`, that can be used to abort
> the transaction. Break up the `ref_store` methods similarly.
>
> This split will make it easier to implement compound reference stores,
> where a transaction might have to span multiple underlying stores. In
> that case, we would first want to "prepare" the sub-transactions in
> each of the reference stores. If any of the "prepare" steps fails, we
> would "abort" all of the sub-transactions. Only if all of the
> "prepare" steps succeed would we "finish" each of them.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

Code looks good to me.

>  /*
>   * 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 in a valid state and new updates can be
> + *         added to it. An OPEN transaction can be prepared or
> + *         committed.

All of these states are valid, OPEN is not more valid than the others. ;)
Maybe:

    OPEN: initial state. new updates can be added to it. An OPEN
        transaction can be prepared, committed.

Is it possible to also abort/free an open transaction?

> + *
> + * 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.

Maybe add that it cannot be altered any more? Possible ways out are
commit and abort IIUC?


> + *
> + * CLOSED: The transaction is no longer active. No other operations
> + *         than free can be used on it in this state. A transaction
> + *         becomes closed if there is a failure while building the
> + *         transaction, if an active transaction is committed,  or if a

The wording here is off as it doesn't use the lingo as introduced before.
(What is an "active" transaction?  I think you mean OPEN in this case)



> + *         prepared transaction is finished.
>   */


Also s/prepared/PREPARED/, add "or aborted"(?)
as that is also a viable way IIUC.

Thanks,
Stefan

Re: [PATCH 17/23] get_packed_ref_cache(): assume "packed-refs" won't change while locked

[Stefan Beller]

On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 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.

This change will work in a world where all Git implementations
obey a lock. If there is at least one implementation that doesn't
care about the existence of lock files we may introduce a race
here.

I am not sure if it is worth to be extra careful in the common case
though. But I could imagine some people using a git repo on an
NFS concurrently with different implementations and one of them
is an old / careless lock-ignoring implementation.

My opinion is not strong enough that I'd veto such a patch
just food for thought.

Thanks,
Stefan

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

[Stefan Beller]

On Wed, May 17, 2017 at 6:42 AM, Jeff King <peff@peff.net> wrote:
> On Wed, May 17, 2017 at 02:05:23PM +0200, Michael Haggerty wrote:
>
>> This patch series is the next leg on a journey towards reading
>> `packed-refs` using `mmap()`, the most interesting aspect of which is
>> that we will often be able to avoid having to read the whole
>> `packed-refs` file if we only need a subset of references.
>>
>> The first leg of the journey was separating out the reference cache
>> into a separate module [1]. That branch is already merged to master.
>>
>> This patch series prepares the ground for separating out a
>> `packed_ref_store`, but doesn't yet take that step. (As you can see,
>> it's a long enough patch series already!) It's kind of a grab bag of
>> cleanup patches plus work to decouple the packed-refs handling code
>> from the rest of `files_ref_store`. Some highlights:
>
> I dropped a few minor comments in individual patches, but it all looks
> good to me up through patch 22. Your description in patch 23 makes
> sense, but I was too frazzled by the end to carefully review the code
> itself for that patch. I'll try to come back to it later.

Same here.

Thanks,
Stefan

Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

[Brandon Williams]

On 05/17, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:
> 
> > 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.
> 
> This made me wonder how we handle the locking for ref_stores besides the
> main one (e.g., for submodules). The lockfile structs have to remain
> valid for the length of the program. Previously those stores could have
> xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
> and leak their whole structs.

Wait, why would this be the case?  I would assume that you would
allocate a ref_store (including a lockfile for it) and then once you are
done with the ref_store, you free it (and unlock and free the lockfile).
I'm definitely not versed in ref handling code though so I may be
missing something.

> 
> I suspect the answer is "we don't ever lock anything except the main ref
> store because that is the only one we write to", so it doesn't matter
> anyway.

Really? I can envision a case in the future where we'd want to update
a ref, or create a ref inside a submodule.

-- 
Brandon Williams

Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

[Brandon Williams]

On 05/17, Stefan Beller wrote:
> On Wed, May 17, 2017 at 6:17 AM, Jeff King <peff@peff.net> wrote:
> > On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:
> >
> >> 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.
> >
> > This made me wonder how we handle the locking for ref_stores besides the
> > main one (e.g., for submodules). The lockfile structs have to remain
> > valid for the length of the program. Previously those stores could have
> > xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
> > and leak their whole structs.
> 
> +cc Brandon, who is eager to go down that road.

I'm probably too eager haha.  But I still think its something to slowly
work towards.

> 
> > I suspect the answer is "we don't ever lock anything except the main ref
> > store because that is the only one we write to", so it doesn't matter
> > anyway.
> >
> > -Peff
> 
> 
> > @@ -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->packlock)
> > +               if (is_lock_file_locked(&refs->packlock))
> >                        die("internal error: packed-ref cache cleared while locked");
> 
> I think the error message needs adjustment here as well? Maybe
> 
>      die("internal error: packed refs locked in cleanup");
> 
> Thanks,
> Stefan

-- 
Brandon Williams

Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

[Jeff King]

On Wed, May 17, 2017 at 05:17:17PM -0700, Brandon Williams wrote:

> > This made me wonder how we handle the locking for ref_stores besides the
> > main one (e.g., for submodules). The lockfile structs have to remain
> > valid for the length of the program. Previously those stores could have
> > xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
> > and leak their whole structs.
> 
> Wait, why would this be the case?  I would assume that you would
> allocate a ref_store (including a lockfile for it) and then once you are
> done with the ref_store, you free it (and unlock and free the lockfile).
> I'm definitely not versed in ref handling code though so I may be
> missing something.

One used, you are not allowed to free a lockfile struct (actually these
days it's just the "tempfile" part of it), because it lives on the
cleanup-handler's tempfile_list forever.

This is a holdover from the early days of the lockfile code, but I think
we could loosen it (and that's the right solution in the long run).

> > I suspect the answer is "we don't ever lock anything except the main ref
> > store because that is the only one we write to", so it doesn't matter
> > anyway.
> 
> Really? I can envision a case in the future where we'd want to update
> a ref, or create a ref inside a submodule.

Oh, I agree it's a probable thing for the future. I was mostly wondering
about the immediate change. I think this patch makes things slightly
worse for that future, but the right fix is to remove the weird tempfile
lifetime requirement in the first place.

-Peff

Re: [PATCH 17/23] get_packed_ref_cache(): assume "packed-refs" won't change while locked

[Jeff King]

On Wed, May 17, 2017 at 10:57:34AM -0700, Stefan Beller wrote:

> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> > 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.
> 
> This change will work in a world where all Git implementations
> obey a lock. If there is at least one implementation that doesn't
> care about the existence of lock files we may introduce a race
> here.
> 
> I am not sure if it is worth to be extra careful in the common case
> though. But I could imagine some people using a git repo on an
> NFS concurrently with different implementations and one of them
> is an old / careless lock-ignoring implementation.
> 
> My opinion is not strong enough that I'd veto such a patch
> just food for thought.

You're so unbelievably screwed if somebody is not respecting the lock
that I don't think it's worth considering.

This change just drops the stat_validity_check(), so you may fail to
realize that somebody racily (without holding the lock!) changed the
packed refs, and may omit a ref from your traversal if it moved from
loose to packed. That _can_ have lasting corruption effects if your
operation is something like "git prune" that is computing full
reachability.

But even without this change, somebody writing the packed-refs file
without lock is likely to hose over simultaneous writes and lose ref
updates (or even lose refs entirely). So anybody who doesn't respect the
locks is broken, period, and needs to be fixed.

-Peff

Re: [PATCH 01/23] t3600: clean up permissions test properly

[Junio C Hamano]

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

> 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_must_fail` 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..4a35c378c8 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -98,8 +98,8 @@ embedded'"
>  
>  test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
>  	chmod a-w . &&
> -	test_must_fail git rm -f baz &&
> -	chmod 775 .
> +	test_when_finished "chmod 775 ." &&
> +	test_must_fail git rm -f baz
>  '

Obviously a good idea.

In this case it would not matter very much, but I think it is a
better style to have when-finished _before_ "chmod a-w ." that
introduces the state we want to revert out of.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> In particular, make it clear that they make copies of the sha1
>> arguments.
>
> A couple weeks ago we had plans on getting rid of SHA1 in
> "the near future" IIRC.  Would it make sense to not go down
> the SHA1 road further and document this in a more abstract way?
>
>     s/SHA1/object name/
>
> essentially, but I guess one of Brians future series' may pick this
> up as well.
>
> I am just hesitant to introduce more sha1-ism at this point.

Don't worry too much about it.  These new paragraphs explain
existing new_sha1 and old_sha1 parameters, and when they are updated
to new_oid/old_oid, the comment will get updated at the same time to
match.

Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much

[Junio C Hamano]

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

> 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. Skip over any references whose names
> are not longer than `trim`.

Should we be silently continuing, or should we use die("BUG") here
instead, if the motivation is to be cautions?  Personally, I do not
find this memchr() implementation too bad, especially when our
objective is to play cautious, but strlen() based one is fine, too.

It's not like refname field would point at a run of non-NUL bytes at
the end of the last-mapped page and taking strlen() would segfault,
right?

> +		if (iter->trim) {
> +			/*
> +			 * If there wouldn't be at least one character
> +			 * left in the refname after trimming, skip
> +			 * over it:
> +			 */
> +			if (memchr(iter->iter0->refname, '\0', iter->trim + 1))
> +				continue;
> +			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;

Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much

[Michael Haggerty]

On 05/18/2017 06:19 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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. Skip over any references whose names
>> are not longer than `trim`.
> 
> Should we be silently continuing, or should we use die("BUG") here
> instead, if the motivation is to be cautions?  Personally, I do not
> find this memchr() implementation too bad, especially when our
> objective is to play cautious, but strlen() based one is fine, too.

True; it would not make much sense to trim off more characters than a
prefix that you have selected for. This also implies that callers should
only check-and-trim a prefix that ends with an explicit "/". Otherwise,
say for example if a caller tries to check-and-trim the prefix
"refs/bisect" and the user has a reference named "refs/bisect", the
result could be the empty string.

I'll change this to die("BUG") and document the above.

> It's not like refname field would point at a run of non-NUL bytes at
> the end of the last-mapped page and taking strlen() would segfault,
> right?

No, refname should be a legit string.

Michael

Re: [PATCH 06/23] refs: use `size_t` indexes when iterating over ref transaction updates

[Michael Haggerty]

On 05/17/2017 06:59 PM, Stefan Beller wrote:
> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 
> Now this would want to have some selling words for it?
> I do not see an advantage of this patch as-is.
> 
> I mean technically we don't need a sign, so we use that extra bit
> to be able to process transactions up to twice the size. But I doubt
> that is the real reason. I'll read on, maybe a later patch will explain
> why we do this here.

The reason to use `size_t` is not signedness but rather that it might be
larger than `int` (e.g., 64 vs 32 bits), so you could theoretically get
an integer overflow otherwise. It's unlikely here, because it would be
hard to initiate an update of more than (2³¹-1) references in a single
update, but it's good hygiene anyway.

I'll mention that in the commit message.

Michael

Re: [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, May 17, 2017 at 02:05:42PM +0200, Michael Haggerty wrote:
>
>> 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.
>
> I think there are may be other "OK" errno values here, like ENOTDIR.
> That's probably pretty unlikely in practice, though, as we're opening a
> file at the root of the $GIT_DIR here (so somebody would had to have
> racily changed our paths). It's probably fine to just err on the side of
> safety.
>
>> +	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("couldn't read %s: %s",
>> +			    packed_refs_file, strerror(errno));
>> +		}
>
> I think this could be die_errno().

I wonder what the endgame shape of this code should be, when it and
nd/fopen-errors topic both graduate.  We cannot use fopen_or_warn(),
as we not just want to warn but want to die, e.g.

	f = fopen(...);
	if (!f) {
        	if (warn_on_fopen_errors(...))
                	die_erno(...);
		return packed_refs;
	}

Re: [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures

[Jeff King]

On Thu, May 18, 2017 at 01:57:45PM +0900, Junio C Hamano wrote:

> >> +	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("couldn't read %s: %s",
> >> +			    packed_refs_file, strerror(errno));
> >> +		}
> >
> > I think this could be die_errno().
> 
> I wonder what the endgame shape of this code should be, when it and
> nd/fopen-errors topic both graduate.  We cannot use fopen_or_warn(),
> as we not just want to warn but want to die, e.g.
> 
> 	f = fopen(...);
> 	if (!f) {
>         	if (warn_on_fopen_errors(...))
>                 	die_erno(...);
> 		return packed_refs;
> 	}

If we go that route I think the die becomes just:
 
   if (warn_on_fopen_errors(...))
	die("unable to open packed refs");

or something. If we want a single die, perhaps the cleanest thing is to
put the logic into its own function:

  static int missing_file_errno(int err)
  {
	return errno == ENOENT || errno == ENOTDIR;
  }
  ...
  if (!missing_file_errno(errno))
	die_errno("couldn't read %s", packed_refs_file);

It feels a little silly to make such a trivial helper, but the point is
to encapsulate that logic. And I think it would be used inside
fopen_or_warn(), and possibly other places. The name might need some
work; that was the best I could come up with.

-Peff

Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

[Brandon Williams]

On 05/17, Jeff King wrote:
> On Wed, May 17, 2017 at 05:17:17PM -0700, Brandon Williams wrote:
> 
> > > This made me wonder how we handle the locking for ref_stores besides the
> > > main one (e.g., for submodules). The lockfile structs have to remain
> > > valid for the length of the program. Previously those stores could have
> > > xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
> > > and leak their whole structs.
> > 
> > Wait, why would this be the case?  I would assume that you would
> > allocate a ref_store (including a lockfile for it) and then once you are
> > done with the ref_store, you free it (and unlock and free the lockfile).
> > I'm definitely not versed in ref handling code though so I may be
> > missing something.
> 
> One used, you are not allowed to free a lockfile struct (actually these
> days it's just the "tempfile" part of it), because it lives on the
> cleanup-handler's tempfile_list forever.
> 
> This is a holdover from the early days of the lockfile code, but I think
> we could loosen it (and that's the right solution in the long run).
> 

Ah ok, thanks for the info!

> > > I suspect the answer is "we don't ever lock anything except the main ref
> > > store because that is the only one we write to", so it doesn't matter
> > > anyway.
> > 
> > Really? I can envision a case in the future where we'd want to update
> > a ref, or create a ref inside a submodule.
> 
> Oh, I agree it's a probable thing for the future. I was mostly wondering
> about the immediate change. I think this patch makes things slightly
> worse for that future, but the right fix is to remove the weird tempfile
> lifetime requirement in the first place.
> 
> -Peff

-- 
Brandon Williams

Re: [PATCH 17/23] get_packed_ref_cache(): assume "packed-refs" won't change while locked

[Stefan Beller]

On Wed, May 17, 2017 at 6:15 PM, Jeff King <peff@peff.net> wrote:
> On Wed, May 17, 2017 at 10:57:34AM -0700, Stefan Beller wrote:
>
>> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> > 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.
>>
>> This change will work in a world where all Git implementations
>> obey a lock. If there is at least one implementation that doesn't
>> care about the existence of lock files we may introduce a race
>> here.
>>
>> I am not sure if it is worth to be extra careful in the common case
>> though. But I could imagine some people using a git repo on an
>> NFS concurrently with different implementations and one of them
>> is an old / careless lock-ignoring implementation.
>>
>> My opinion is not strong enough that I'd veto such a patch
>> just food for thought.
>
> You're so unbelievably screwed if somebody is not respecting the lock
> that I don't think it's worth considering.
>
> This change just drops the stat_validity_check(), so you may fail to
> realize that somebody racily (without holding the lock!) changed the
> packed refs, and may omit a ref from your traversal if it moved from
> loose to packed. That _can_ have lasting corruption effects if your
> operation is something like "git prune" that is computing full
> reachability.
>
> But even without this change, somebody writing the packed-refs file
> without lock is likely to hose over simultaneous writes and lose ref
> updates (or even lose refs entirely). So anybody who doesn't respect the
> locks is broken, period, and needs to be fixed.

ok, in that case I see no objection for this patch.

Stefan

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

[Johannes Sixt]

Am 17.05.2017 um 14:05 schrieb Michael Haggerty:
> This patch series is the next leg on a journey towards reading
> `packed-refs` using `mmap()`, the most interesting aspect of which is
> that we will often be able to avoid having to read the whole
> `packed-refs` file if we only need a subset of references.

Which features of mmap() are you going to use? We have only MAP_PRIVATE 
on Windows. We do emulate PROT_WRITE (without PROT_READ), but I don't 
think that code is exercised anywhere. See also compat/win32mmap.c.

-- Hannes

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

[Jeff King]

On Thu, May 18, 2017 at 07:14:38PM +0200, Johannes Sixt wrote:

> Am 17.05.2017 um 14:05 schrieb Michael Haggerty:
> > This patch series is the next leg on a journey towards reading
> > `packed-refs` using `mmap()`, the most interesting aspect of which is
> > that we will often be able to avoid having to read the whole
> > `packed-refs` file if we only need a subset of references.
> 
> Which features of mmap() are you going to use? We have only MAP_PRIVATE on
> Windows. We do emulate PROT_WRITE (without PROT_READ), but I don't think
> that code is exercised anywhere. See also compat/win32mmap.c.

It's just on the reading side, so it should be the same as the mmaps we
do for reading packfiles, the index, etc. You can see the actual call in
the upcoming series that Michael linked earlier:

  https://github.com/mhagger/git/commit/d22420b5f3e967d24aba019a0916a5a93ab00e03

-Peff

Re: [PATCH 01/23] t3600: clean up permissions test properly

[Michael Haggerty]

On 05/18/2017 06:10 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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_must_fail` 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..4a35c378c8 100755
>> --- a/t/t3600-rm.sh
>> +++ b/t/t3600-rm.sh
>> @@ -98,8 +98,8 @@ embedded'"
>>  
>>  test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
>>  	chmod a-w . &&
>> -	test_must_fail git rm -f baz &&
>> -	chmod 775 .
>> +	test_when_finished "chmod 775 ." &&
>> +	test_must_fail git rm -f baz
>>  '
> 
> Obviously a good idea.
> 
> In this case it would not matter very much, but I think it is a
> better style to have when-finished _before_ "chmod a-w ." that
> introduces the state we want to revert out of.

OK, I'll change this in v2.

Michael

Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

[Michael Haggerty]

On 05/17/2017 07:18 PM, Stefan Beller wrote:
> On Wed, May 17, 2017 at 6:17 AM, Jeff King <peff@peff.net> wrote:
>> On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:
>>
>>> 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.
>>
>> This made me wonder how we handle the locking for ref_stores besides the
>> main one (e.g., for submodules). The lockfile structs have to remain
>> valid for the length of the program. Previously those stores could have
>> xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()
>> and leak their whole structs.
> 
> +cc Brandon, who is eager to go down that road.
> 
>> I suspect the answer is "we don't ever lock anything except the main ref
>> store because that is the only one we write to", so it doesn't matter
>> anyway.
>>
>> -Peff
> 
> 
>> @@ -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->packlock)
>> +               if (is_lock_file_locked(&refs->packlock))
>>                        die("internal error: packed-ref cache cleared while locked");
> 
> I think the error message needs adjustment here as well? Maybe
> 
>      die("internal error: packed refs locked in cleanup");

Hmmm, I don't see what's wrong with the current error message (except
for s/internal error/BUG/). In any case, this is meant to be a sanity
check that users should never see, so I wouldn't worry too much about it
either way.

Michael

Re: [PATCH 11/23] files_transaction_cleanup(): new helper function

[Michael Haggerty]

On 05/17/2017 07:26 PM, Stefan Beller wrote:
> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Extract function from `files_transaction_commit()`. It will soon have
>> another caller.
> 
> This sounds odd to me. Maybe it is missing words?
> of s/function/the functionality to cleanup/

This is basically the same as the standard refactoring step "extract
method", except applied to a function, not a method. I thought these
terms would be standard enough to not need further explanation, but I'll
add some more words.

Michael

Re: [PATCH 11/23] files_transaction_cleanup(): new helper function

[Michael Haggerty]

On 05/17/2017 03:19 PM, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:34PM +0200, Michael Haggerty wrote:
> 
>> Extract function from `files_transaction_commit()`. It will soon have
>> another caller.
>> [...]
>> @@ -2868,10 +2889,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;
> 
> This is slightly more than it says on the tin. I guess the reason is
> that the cleanup function is going to learn about more than just
> iterating over the transaction, and we'll want to trigger it even for
> empty transactions.

Mostly this is just DRY. For example, later the cleanup will be changed
to set the state to `REF_TRANSACTION_PREPARED` rather than
`REF_TRANSACTION_CLOSED`, and that change will be able to be made in one
place rather than two. I'll mention this in the commit message.

Michael

Re: [PATCH 12/23] ref_transaction_commit(): break into multiple functions

[Michael Haggerty]

On 05/17/2017 07:44 PM, Stefan Beller wrote:
> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Break the function `ref_transaction_commit()` into two functions,
>> `ref_transaction_prepare()` and `ref_transaction_finish()`, with a
>> third function, `ref_transaction_abort()`, that can be used to abort
>> the transaction. Break up the `ref_store` methods similarly.
>>
>> This split will make it easier to implement compound reference stores,
>> where a transaction might have to span multiple underlying stores. In
>> that case, we would first want to "prepare" the sub-transactions in
>> each of the reference stores. If any of the "prepare" steps fails, we
>> would "abort" all of the sub-transactions. Only if all of the
>> "prepare" steps succeed would we "finish" each of them.
> [...]

Thanks for your comments. While I was incorporating them, I realized
that other parts of the documentation needed updates, too. And while I
was fixing that, I noticed that the interface was unnecessarily
complicated. The old version required either `commit` or `prepare`
followed by `finish`. But there is no reason that the public API has to
expose `finish`. So instead, let's call `prepare` an optional step that
is allowed to precede `commit`, and make `commit` smart enough to call
`prepare` if it hasn't been called already, and then call `finish`.

Furthermore, let's make it possible to call `abort` when the transaction
is in OPEN as well as PREPARED state rather than requiring `free` in
OPEN state and `abort` in PREPARED state.

I will make these changes and revamp the docs for v2.

Michael

Re: [PATCH 22/23] ref-filter: limit traversal to prefix

[Michael Haggerty]

On 05/17/2017 03:38 PM, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:45PM +0200, Michael Haggerty wrote:
> 
>> From: Jeff King <peff@peff.net>
> 
> This patch did originate with me, but I know you had to fix several
> things to integrate it in your series. So I'll review it anyway, and
> give you full blame for any bugs. :)
> 
>> When we are matching refnames "as path" 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`.
> 
> That first sentence confused me as to what "as path" meant (I know
> because I worked on this code, and even then it took me a minute to
> parse it).
> 
> Maybe just "When we are matching refnames against a pattern" and then
> later something like:
> 
>   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.

That is clearer. I'll make the change.

>> +/*
>> + * Find the longest prefix of pattern we can pass to
>> + * for_each_fullref_in(), namely the part of pattern preceding the
>> + * first glob character.
>> + */
>> +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);
>> +}
> 
> If I were reviewing this from scratch, I'd probably ask whether it is OK
> in:
> 
>   refs/heads/m*
> 
> to return "refs/heads/m" as the prefix, and not stop at the last
> non-wildcard component ("refs/heads/"). But I happen to know we
> discussed this off-list and you checked that for_each_ref and friends
> are happy with an arbitrary prefix. But I'm calling it out here for
> other reviewers.

I'll add a comment about this, too.

Thanks,
Michael