threads / patch / 46189

patch, 28 partsCreate a reference backend for packed refs

Subject: [PATCH 00/28] Create a reference backend for packed refs

## tl;dr

52 messages between Jun 15, 2017 and Jul 26, 2017. Diffs are folded; open one to read it.

replies: 51people: 5as markdown or json

Michael Haggerty· Jun 15, 2017, 14:47 UTC · lore

This patch series continues the saga of picking apart the code for handling packed references from the code for handling loose references, all in preparation for making big changes to how the packed-ref reading and writing works as described in [1]. As a reminder, the final goal is to read the "packed-refs" file using mmap, parsing it on the fly instead of storing it into an in-memory `ref_cache`, and to read and parse only the parts of the file that are actually needed, giving a big speedup for many operations in repositories that have lots of refs.

In this episode, we create a `packed_ref_store` class, implementing part of the `ref_store` API, that represents the packed references within a repository. The `files_ref_store` now contains an instance of `packed_ref_store` and delegates to it for the operations that have to touch the packed refs.

After this patch series, `packed_ref_store` supports:
* Iteration
* `peel_ref`
* `pack_refs` (they're already packed, so it's a NOOP)
* `read_raw_ref`
A future patch series will add support for:
* Reference transactions (`transaction_prepare`, `transaction_finish`,
  `transaction_abort`, `initial_transaction_commit`)
* `delete_refs`
Operations that `packed_ref_store` will probably never support:
* `create_symref`
* `rename_ref` (could be supported, but is probably not useful)
* Reflog-related operations

In addition, all of the packed-refs related code has been moved to a new module, "refs/packed-backend.{c,h}". This includes some functions that are still called by `files_ref_store` directly to update the packed refs.

The patch series is long, but I think relatively straightforward. In particular, patches 2-14 are quite mechanical. Its main point is to separate concerns, but it does bring one end-user advantage: if there is a problem parsing the "packed-refs" file, we now report an error and die. The old code just ignored lines that it didn't understand.

I've developed these patches on top of master plus the following patches, which are followups to mh/packed-refs-store-prep:

* lock_packed_refs(): fix cache validity check
* for_each_bisect_ref(): don't trim refnames

The patches can also be obtained from my GitHub fork [2] as branch "packed-ref-store".

Michael

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

Michael Haggerty (28):
  add_packed_ref(): teach function to overwrite existing refs
  packed_ref_store: new struct
  packed_ref_store: move `packed_refs_path` here
  packed_ref_store: move `packed_refs_lock` member here
  clear_packed_ref_cache(): take a `packed_ref_store *` parameter
  validate_packed_ref_cache(): take a `packed_ref_store *` parameter
  get_packed_ref_cache(): take a `packed_ref_store *` parameter
  get_packed_refs(): take a `packed_ref_store *` parameter
  add_packed_ref(): take a `packed_ref_store *` parameter
  lock_packed_refs(): take a `packed_ref_store *` parameter
  commit_packed_refs(): take a `packed_ref_store *` parameter
  rollback_packed_refs(): take a `packed_ref_store *` parameter
  get_packed_ref(): take a `packed_ref_store *` parameter
  repack_without_refs(): take a `packed_ref_store *` parameter
  packed_peel_ref(): new function, extracted from `files_peel_ref()`
  packed_ref_store: support iteration
  packed_read_raw_ref(): new function, replacing `resolve_packed_ref()`
  packed-backend: new module for handling packed references
  packed_ref_store: make class into a subclass of `ref_store`
  commit_packed_refs(): report errors rather than dying
  commit_packed_refs(): use a staging file separate from the lockfile
  packed_refs_lock(): function renamed from lock_packed_refs()
  packed_refs_lock(): report errors via a `struct strbuf *err`
  packed_refs_unlock(), packed_refs_is_locked(): new functions
  clear_packed_ref_cache(): don't protest if the lock is held
  commit_packed_refs(): remove call to `packed_refs_unlock()`
  repack_without_refs(): don't lock or unlock the packed refs
  read_packed_refs(): die if `packed-refs` contains bogus data
 Makefile              |   1 +
 refs.c                |  18 ++
 refs/files-backend.c  | 619 ++++-------------------------------
 refs/packed-backend.c | 868 ++++++++++++++++++++++++++++++++++++++++++++++++++
 refs/packed-backend.h |  25 ++
 refs/refs-internal.h  |  10 +
 6 files changed, 981 insertions(+), 560 deletions(-)
 create mode 100644 refs/packed-backend.c
 create mode 100644 refs/packed-backend.h
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 01/28] add_packed_ref(): teach function to overwrite existing refs

Teach `add_packed_ref()` to overwrite an existing entry if one already exists for the specified `refname`. This means that we can call it from `files_pack_refs()`, thereby reducing the amount that the latter function needs to know about the internals of packed-reference handling.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)
Show changes to refs/files-backend.c +18 −22
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b040bb3b0a..87cecde231 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -413,15 +413,16 @@ static struct ref_dir *get_packed_refs(struct files_ref_store *refs)
 }
 
 /*
- * Add a reference to the in-memory packed reference cache.  This may
- * only be called while the packed-refs file is locked (see
- * lock_packed_refs()).  To actually write the packed-refs file, call
- * commit_packed_refs().
+ * Add or overwrite a reference in the in-memory packed reference
+ * cache. This may only be called while the packed-refs file is locked
+ * (see lock_packed_refs()). To actually write the packed-refs file,
+ * call commit_packed_refs().
  */
 static void add_packed_ref(struct files_ref_store *refs,
 			   const char *refname, const struct object_id *oid)
 {
-	struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
+	struct ref_dir *packed_refs;
+	struct ref_entry *packed_entry;
 
 	if (!is_lock_file_locked(&refs->packed_refs_lock))
 		die("BUG: packed refs not locked");
@@ -429,8 +430,17 @@ static void add_packed_ref(struct files_ref_store *refs,
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
 		die("Reference has invalid format: '%s'", refname);
 
-	add_ref_entry(get_packed_ref_dir(packed_ref_cache),
-		      create_ref_entry(refname, oid, REF_ISPACKED));
+	packed_refs = get_packed_refs(refs);
+	packed_entry = find_ref_entry(packed_refs, refname);
+	if (packed_entry) {
+		/* Overwrite the existing entry: */
+		oidcpy(&packed_entry->u.value.oid, oid);
+		packed_entry->flag = REF_ISPACKED;
+		oidclr(&packed_entry->u.value.peeled);
+	} else {
+		packed_entry = create_ref_entry(refname, oid, REF_ISPACKED);
+		add_ref_entry(packed_refs, packed_entry);
+	}
 }
 
 /*
@@ -1526,12 +1536,10 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 		files_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,
 			       "pack_refs");
 	struct ref_iterator *iter;
-	struct ref_dir *packed_refs;
 	int ok;
 	struct ref_to_prune *refs_to_prune = NULL;
 
 	lock_packed_refs(refs, LOCK_DIE_ON_ERROR);
-	packed_refs = get_packed_refs(refs);
 
 	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -1540,8 +1548,6 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 		 * in the packed ref cache. If the reference should be
 		 * pruned, also add it to refs_to_prune.
 		 */
-		struct ref_entry *packed_entry;
-
 		if (!should_pack_ref(iter->refname, iter->oid, iter->flags,
 				     flags))
 			continue;
@@ -1552,17 +1558,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 		 * we don't copy the peeled status, because we want it
 		 * to be re-peeled.
 		 */
-		packed_entry = find_ref_entry(packed_refs, iter->refname);
-		if (packed_entry) {
-			/* Overwrite existing packed entry with info from loose entry */
-			packed_entry->flag = REF_ISPACKED;
-			oidcpy(&packed_entry->u.value.oid, iter->oid);
-		} else {
-			packed_entry = create_ref_entry(iter->refname, iter->oid,
-							REF_ISPACKED);
-			add_ref_entry(packed_refs, packed_entry);
-		}
-		oidclr(&packed_entry->u.value.peeled);
+		add_packed_ref(refs, iter->refname, iter->oid);
 
 		/* Schedule the loose reference for pruning if requested. */
 		if ((flags & PACK_REFS_PRUNE)) {
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 03/28] packed_ref_store: move `packed_refs_path` here

Move `packed_refs_path` from `files_ref_store` to `packed_ref_store`, and rename it to `path` since its meaning is clear from its new context.

Inline `files_packed_refs_path()`.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)
Show changes to refs/files-backend.c +12 −13
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2efb71cee9..c4b8e2f63b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -54,6 +54,9 @@ struct packed_ref_cache {
 struct packed_ref_store {
 	unsigned int store_flags;
 
+	/* The path of the "packed-refs" file: */
+	char *path;
+
 	/*
 	 * A cache of the values read from the `packed-refs` file, if
 	 * it might still be current; otherwise, NULL.
@@ -61,11 +64,13 @@ struct packed_ref_store {
 	struct packed_ref_cache *cache;
 };
 
-static struct packed_ref_store *packed_ref_store_create(unsigned int store_flags)
+static struct packed_ref_store *packed_ref_store_create(
+		const char *path, unsigned int store_flags)
 {
 	struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
 
 	refs->store_flags = store_flags;
+	refs->path = xstrdup(path);
 	return refs;
 }
 
@@ -79,7 +84,6 @@ struct files_ref_store {
 
 	char *gitdir;
 	char *gitcommondir;
-	char *packed_refs_path;
 
 	struct ref_cache *loose;
 
@@ -154,8 +158,8 @@ static struct ref_store *files_ref_store_create(const char *gitdir,
 	get_common_dir_noenv(&sb, gitdir);
 	refs->gitcommondir = strbuf_detach(&sb, NULL);
 	strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
-	refs->packed_refs_path = strbuf_detach(&sb, NULL);
-	refs->packed_ref_store = packed_ref_store_create(flags);
+	refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
+	strbuf_release(&sb);
 
 	return ref_store;
 }
@@ -343,11 +347,6 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)
 	return packed_refs;
 }
 
-static const char *files_packed_refs_path(struct files_ref_store *refs)
-{
-	return refs->packed_refs_path;
-}
-
 static void files_reflog_path(struct files_ref_store *refs,
 			      struct strbuf *sb,
 			      const char *refname)
@@ -401,7 +400,7 @@ static void validate_packed_ref_cache(struct files_ref_store *refs)
 {
 	if (refs->packed_ref_store->cache &&
 	    !stat_validity_check(&refs->packed_ref_store->cache->validity,
-				 files_packed_refs_path(refs)))
+				 refs->packed_ref_store->path))
 		clear_packed_ref_cache(refs);
 }
 
@@ -415,7 +414,7 @@ static void validate_packed_ref_cache(struct files_ref_store *refs)
  */
 static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refs)
 {
-	const char *packed_refs_file = files_packed_refs_path(refs);
+	const char *packed_refs_file = refs->packed_ref_store->path;
 
 	if (!is_lock_file_locked(&refs->packed_refs_lock))
 		validate_packed_ref_cache(refs);
@@ -1352,7 +1351,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	}
 
 	if (hold_lock_file_for_update_timeout(
-			    &refs->packed_refs_lock, files_packed_refs_path(refs),
+			    &refs->packed_refs_lock, refs->packed_ref_store->path,
 			    flags, timeout_value) < 0)
 		return -1;
 
@@ -1633,7 +1632,7 @@ static int repack_without_refs(struct files_ref_store *refs,
 		return 0; /* no refname exists in packed refs */
 
 	if (lock_packed_refs(refs, 0)) {
-		unable_to_lock_message(files_packed_refs_path(refs), errno, err);
+		unable_to_lock_message(refs->packed_ref_store->path, errno, err);
 		return -1;
 	}
 	packed = get_packed_refs(refs);
-- 
2.11.0
Stefan Beller· Jun 16, 2017, 05:30 UTC · re: Michael Haggerty · lore

Re: [PATCH 03/28] packed_ref_store: move `packed_refs_path` here

On Thu, Jun 15, 2017 at 7:47 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 37 quoted lines
> Move `packed_refs_path` from `files_ref_store` to `packed_ref_store`,
> and rename it to `path` since its meaning is clear from its new
> context.
>
> Inline `files_packed_refs_path()`.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs/files-backend.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 2efb71cee9..c4b8e2f63b 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -54,6 +54,9 @@ struct packed_ref_cache {
>  struct packed_ref_store {
>         unsigned int store_flags;
>
> +       /* The path of the "packed-refs" file: */
> +       char *path;
> +
>         /*
>          * A cache of the values read from the `packed-refs` file, if
>          * it might still be current; otherwise, NULL.
> @@ -61,11 +64,13 @@ struct packed_ref_store {
>         struct packed_ref_cache *cache;
>  };
>
> -static struct packed_ref_store *packed_ref_store_create(unsigned int store_flags)
> +static struct packed_ref_store *packed_ref_store_create(
> +               const char *path, unsigned int store_flags)
>  {
>         struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
>
>         refs->store_flags = store_flags;
> +       refs->path = xstrdup(path);

mental note: the destructor (if introduced later) has to free the path.

Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 04/28] packed_ref_store: move `packed_refs_lock` member here

Move the `packed_refs_lock` member from `files_ref_store` to `packed_ref_store`, and rename it to `lock` since it's now more obvious what it is locking.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)
Show changes to refs/files-backend.c +16 −15
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c4b8e2f63b..de8293493f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -62,6 +62,12 @@ struct packed_ref_store {
 	 * it might still be current; otherwise, NULL.
 	 */
 	struct packed_ref_cache *cache;
+
+	/*
+	 * Lock used for the "packed-refs" file. Note that this (and
+	 * thus the enclosing `packed_ref_store`) must not be freed.
+	 */
+	struct lock_file lock;
 };
 
 static struct packed_ref_store *packed_ref_store_create(
@@ -87,12 +93,6 @@ struct files_ref_store {
 
 	struct ref_cache *loose;
 
-	/*
-	 * Lock used for the "packed-refs" file. Note that this (and
-	 * thus the enclosing `files_ref_store`) must not be freed.
-	 */
-	struct lock_file packed_refs_lock;
-
 	struct packed_ref_store *packed_ref_store;
 };
 
@@ -125,7 +125,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)
 	if (refs->packed_ref_store->cache) {
 		struct packed_ref_cache *packed_refs = refs->packed_ref_store->cache;
 
-		if (is_lock_file_locked(&refs->packed_refs_lock))
+		if (is_lock_file_locked(&refs->packed_ref_store->lock))
 			die("BUG: packed-ref cache cleared while locked");
 		refs->packed_ref_store->cache = NULL;
 		release_packed_ref_cache(packed_refs);
@@ -416,7 +416,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref
 {
 	const char *packed_refs_file = refs->packed_ref_store->path;
 
-	if (!is_lock_file_locked(&refs->packed_refs_lock))
+	if (!is_lock_file_locked(&refs->packed_ref_store->lock))
 		validate_packed_ref_cache(refs);
 
 	if (!refs->packed_ref_store->cache)
@@ -447,7 +447,7 @@ static void add_packed_ref(struct files_ref_store *refs,
 	struct ref_dir *packed_refs;
 	struct ref_entry *packed_entry;
 
-	if (!is_lock_file_locked(&refs->packed_refs_lock))
+	if (!is_lock_file_locked(&refs->packed_ref_store->lock))
 		die("BUG: packed refs not locked");
 
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
@@ -1351,7 +1351,8 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	}
 
 	if (hold_lock_file_for_update_timeout(
-			    &refs->packed_refs_lock, refs->packed_ref_store->path,
+			    &refs->packed_ref_store->lock,
+			    refs->packed_ref_store->path,
 			    flags, timeout_value) < 0)
 		return -1;
 
@@ -1388,10 +1389,10 @@ static int commit_packed_refs(struct files_ref_store *refs)
 
 	files_assert_main_repository(refs, "commit_packed_refs");
 
-	if (!is_lock_file_locked(&refs->packed_refs_lock))
+	if (!is_lock_file_locked(&refs->packed_ref_store->lock))
 		die("BUG: packed-refs not locked");
 
-	out = fdopen_lock_file(&refs->packed_refs_lock, "w");
+	out = fdopen_lock_file(&refs->packed_ref_store->lock, "w");
 	if (!out)
 		die_errno("unable to fdopen packed-refs descriptor");
 
@@ -1409,7 +1410,7 @@ static int commit_packed_refs(struct files_ref_store *refs)
 	if (ok != ITER_DONE)
 		die("error while iterating over references");
 
-	if (commit_lock_file(&refs->packed_refs_lock)) {
+	if (commit_lock_file(&refs->packed_ref_store->lock)) {
 		save_errno = errno;
 		error = -1;
 	}
@@ -1430,9 +1431,9 @@ static void rollback_packed_refs(struct files_ref_store *refs)
 
 	files_assert_main_repository(refs, "rollback_packed_refs");
 
-	if (!is_lock_file_locked(&refs->packed_refs_lock))
+	if (!is_lock_file_locked(&refs->packed_ref_store->lock))
 		die("BUG: packed-refs not locked");
-	rollback_lock_file(&refs->packed_refs_lock);
+	rollback_lock_file(&refs->packed_ref_store->lock);
 	release_packed_ref_cache(packed_ref_cache);
 	clear_packed_ref_cache(refs);
 }
-- 
2.11.0
Stefan Beller· Jun 16, 2017, 05:39 UTC · re: Michael Haggerty · lore

Re: [PATCH 04/28] packed_ref_store: move `packed_refs_lock` member here

On Thu, Jun 15, 2017 at 7:47 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 45 quoted lines
> Move the `packed_refs_lock` member from `files_ref_store` to
> `packed_ref_store`, and rename it to `lock` since it's now more
> obvious what it is locking.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs/files-backend.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index c4b8e2f63b..de8293493f 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -62,6 +62,12 @@ struct packed_ref_store {
>          * it might still be current; otherwise, NULL.
>          */
>         struct packed_ref_cache *cache;
> +
> +       /*
> +        * Lock used for the "packed-refs" file. Note that this (and
> +        * thus the enclosing `packed_ref_store`) must not be freed.
> +        */
> +       struct lock_file lock;
>  };
>
>  static struct packed_ref_store *packed_ref_store_create(
> @@ -87,12 +93,6 @@ struct files_ref_store {
>
>         struct ref_cache *loose;
>
> -       /*
> -        * Lock used for the "packed-refs" file. Note that this (and
> -        * thus the enclosing `files_ref_store`) must not be freed.
> -        */
> -       struct lock_file packed_refs_lock;
> -
>         struct packed_ref_store *packed_ref_store;
>  };
>
> @@ -125,7 +125,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)
>         if (refs->packed_ref_store->cache) {
>                 struct packed_ref_cache *packed_refs = refs->packed_ref_store->cache;
>
> -               if (is_lock_file_locked(&refs->packed_refs_lock))
> +               if (is_lock_file_locked(&refs->packed_ref_store->lock))

I sort of stumble over the name due to singular/plural/genetive issues: The store contains multiple of "packed ref"(s) or the store is specialized in packed ref(s). On the other hand in English you seem to reference the specialisation of a store in singular: It's a "gun store" (not "guns store") or "furniture store" or "fresh fish store" (though that could be plural as well).

It just sounded odd to me. (though think of it as a minor nit)
Michael Haggerty· Jun 16, 2017, 06:43 UTC · re: Stefan Beller · lore

Re: [PATCH 04/28] packed_ref_store: move `packed_refs_lock` member here

On 06/16/2017 07:39 AM, Stefan Beller wrote:
Show 17 quoted lines
> On Thu, Jun 15, 2017 at 7:47 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> @@ -125,7 +125,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)
>>         if (refs->packed_ref_store->cache) {
>>                 struct packed_ref_cache *packed_refs = refs->packed_ref_store->cache;
>>
>> -               if (is_lock_file_locked(&refs->packed_refs_lock))
>> +               if (is_lock_file_locked(&refs->packed_ref_store->lock))
> 
> I sort of stumble over the name due to singular/plural/genetive issues:
> The store contains multiple of "packed ref"(s) or the store is specialized in
> packed ref(s). On the other hand in English you seem to reference the
> specialisation of a store in singular: It's a "gun store" (not "guns store")
> or "furniture store" or "fresh fish store" (though that could be
> plural as well).
> 
> It just sounded odd to me. (though think of it as a minor nit)

I chose that name because it is a `ref_store`, with `packed_` being a short prefix that tells what kind of `ref_store`.

The next question is, why `ref_store` as opposed to `refs_store`? To me it sounds more natural in English for the reasons that you mentioned.

Michael
Stefan Beller· Jun 16, 2017, 15:44 UTC · re: Michael Haggerty · lore

Re: [PATCH 04/28] packed_ref_store: move `packed_refs_lock` member here

On Thu, Jun 15, 2017 at 11:43 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 5 quoted lines
> I chose that name because it is a `ref_store`, with `packed_` being a
> short prefix that tells what kind of `ref_store`.
>
> The next question is, why `ref_store` as opposed to `refs_store`? To me
> it sounds more natural in English for the reasons that you mentioned.

Yeah after some thought on language this sounds good to me, too. I still have to work on natural. ;)

Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 07/28] get_packed_ref_cache(): take a `packed_ref_store *` parameter

It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)
Show changes to refs/files-backend.c +12 −14
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f061506bf0..b2ef7b3bb9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -404,24 +404,22 @@ static void validate_packed_ref_cache(struct packed_ref_store *refs)
 }
 
 /*
- * Get the packed_ref_cache for the specified files_ref_store,
+ * Get the packed_ref_cache for the specified packed_ref_store,
  * 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)
+static struct packed_ref_cache *get_packed_ref_cache(struct packed_ref_store *refs)
 {
-	const char *packed_refs_file = refs->packed_ref_store->path;
+	if (!is_lock_file_locked(&refs->lock))
+		validate_packed_ref_cache(refs);
 
-	if (!is_lock_file_locked(&refs->packed_ref_store->lock))
-		validate_packed_ref_cache(refs->packed_ref_store);
-
-	if (!refs->packed_ref_store->cache)
-		refs->packed_ref_store->cache = read_packed_refs(packed_refs_file);
+	if (!refs->cache)
+		refs->cache = read_packed_refs(refs->path);
 
-	return refs->packed_ref_store->cache;
+	return refs->cache;
 }
 
 static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_cache)
@@ -431,7 +429,7 @@ static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_ca
 
 static struct ref_dir *get_packed_refs(struct files_ref_store *refs)
 {
-	return get_packed_ref_dir(get_packed_ref_cache(refs));
+	return get_packed_ref_dir(get_packed_ref_cache(refs->packed_ref_store));
 }
 
 /*
@@ -1151,7 +1149,7 @@ static struct ref_iterator *files_ref_iterator_begin(
 	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
 					      prefix, 1);
 
-	iter->packed_ref_cache = get_packed_ref_cache(refs);
+	iter->packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
 	acquire_packed_ref_cache(iter->packed_ref_cache);
 	packed_iter = cache_ref_iterator_begin(iter->packed_ref_cache->cache,
 					       prefix, 0);
@@ -1365,7 +1363,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	 */
 	validate_packed_ref_cache(refs->packed_ref_store);
 
-	packed_ref_cache = get_packed_ref_cache(refs);
+	packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
 	/* Increment the reference count to prevent it from being freed: */
 	acquire_packed_ref_cache(packed_ref_cache);
 	return 0;
@@ -1380,7 +1378,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 static int commit_packed_refs(struct files_ref_store *refs)
 {
 	struct packed_ref_cache *packed_ref_cache =
-		get_packed_ref_cache(refs);
+		get_packed_ref_cache(refs->packed_ref_store);
 	int ok, error = 0;
 	int save_errno = 0;
 	FILE *out;
@@ -1426,7 +1424,7 @@ static int commit_packed_refs(struct files_ref_store *refs)
 static void rollback_packed_refs(struct files_ref_store *refs)
 {
 	struct packed_ref_cache *packed_ref_cache =
-		get_packed_ref_cache(refs);
+		get_packed_ref_cache(refs->packed_ref_store);
 
 	files_assert_main_repository(refs, "rollback_packed_refs");
 
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 08/28] get_packed_refs(): take a `packed_ref_store *` parameter

It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to refs/files-backend.c +5 −5
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b2ef7b3bb9..bc5c0de84e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -427,9 +427,9 @@ static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_ca
 	return get_ref_dir(packed_ref_cache->cache->root);
 }
 
-static struct ref_dir *get_packed_refs(struct files_ref_store *refs)
+static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)
 {
-	return get_packed_ref_dir(get_packed_ref_cache(refs->packed_ref_store));
+	return get_packed_ref_dir(get_packed_ref_cache(refs));
 }
 
 /*
@@ -450,7 +450,7 @@ static void add_packed_ref(struct files_ref_store *refs,
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
 		die("Reference has invalid format: '%s'", refname);
 
-	packed_refs = get_packed_refs(refs);
+	packed_refs = get_packed_refs(refs->packed_ref_store);
 	packed_entry = find_ref_entry(packed_refs, refname);
 	if (packed_entry) {
 		/* Overwrite the existing entry: */
@@ -592,7 +592,7 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
 static struct ref_entry *get_packed_ref(struct files_ref_store *refs,
 					const char *refname)
 {
-	return find_ref_entry(get_packed_refs(refs), refname);
+	return find_ref_entry(get_packed_refs(refs->packed_ref_store), refname);
 }
 
 /*
@@ -1633,7 +1633,7 @@ static int repack_without_refs(struct files_ref_store *refs,
 		unable_to_lock_message(refs->packed_ref_store->path, errno, err);
 		return -1;
 	}
-	packed = get_packed_refs(refs);
+	packed = get_packed_refs(refs->packed_ref_store);
 
 	/* Remove refnames from the cache */
 	for_each_string_list_item(refname, refnames)
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 09/28] add_packed_ref(): take a `packed_ref_store *` parameter

It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to refs/files-backend.c +5 −5
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bc5c0de84e..4943207098 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -438,19 +438,19 @@ static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)
  * (see lock_packed_refs()). To actually write the packed-refs file,
  * call commit_packed_refs().
  */
-static void add_packed_ref(struct files_ref_store *refs,
+static void add_packed_ref(struct packed_ref_store *refs,
 			   const char *refname, const struct object_id *oid)
 {
 	struct ref_dir *packed_refs;
 	struct ref_entry *packed_entry;
 
-	if (!is_lock_file_locked(&refs->packed_ref_store->lock))
+	if (!is_lock_file_locked(&refs->lock))
 		die("BUG: packed refs not locked");
 
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
 		die("Reference has invalid format: '%s'", refname);
 
-	packed_refs = get_packed_refs(refs->packed_ref_store);
+	packed_refs = get_packed_refs(refs);
 	packed_entry = find_ref_entry(packed_refs, refname);
 	if (packed_entry) {
 		/* Overwrite the existing entry: */
@@ -1579,7 +1579,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 		 * we don't copy the peeled status, because we want it
 		 * to be re-peeled.
 		 */
-		add_packed_ref(refs, iter->refname, iter->oid);
+		add_packed_ref(refs->packed_ref_store, iter->refname, iter->oid);
 
 		/* Schedule the loose reference for pruning if requested. */
 		if ((flags & PACK_REFS_PRUNE)) {
@@ -3210,7 +3210,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 
 		if ((update->flags & REF_HAVE_NEW) &&
 		    !is_null_oid(&update->new_oid))
-			add_packed_ref(refs, update->refname,
+			add_packed_ref(refs->packed_ref_store, update->refname,
 				       &update->new_oid);
 	}
 
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 10/28] lock_packed_refs(): take a `packed_ref_store *` parameter

It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)
Show changes to refs/files-backend.c +22 −9
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4943207098..0d8f39089f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -80,6 +80,19 @@ static struct packed_ref_store *packed_ref_store_create(
 	return refs;
 }
 
+/*
+ * Die if refs is not the main ref store. caller is used in any
+ * necessary error messages.
+ */
+static void packed_assert_main_repository(struct packed_ref_store *refs,
+					  const char *caller)
+{
+	if (refs->store_flags & REF_STORE_MAIN)
+		return;
+
+	die("BUG: operation %s only allowed for main ref store", caller);
+}
+
 /*
  * Future: need to be in "struct repository"
  * when doing a full libification.
@@ -1334,13 +1347,13 @@ static void write_packed_entry(FILE *fh, const char *refname,
  * hold_lock_file_for_update(). Return 0 on success. On errors, set
  * errno appropriately and return a nonzero value.
  */
-static int lock_packed_refs(struct files_ref_store *refs, int flags)
+static int lock_packed_refs(struct packed_ref_store *refs, int flags)
 {
 	static int timeout_configured = 0;
 	static int timeout_value = 1000;
 	struct packed_ref_cache *packed_ref_cache;
 
-	files_assert_main_repository(refs, "lock_packed_refs");
+	packed_assert_main_repository(refs, "lock_packed_refs");
 
 	if (!timeout_configured) {
 		git_config_get_int("core.packedrefstimeout", &timeout_value);
@@ -1348,8 +1361,8 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	}
 
 	if (hold_lock_file_for_update_timeout(
-			    &refs->packed_ref_store->lock,
-			    refs->packed_ref_store->path,
+			    &refs->lock,
+			    refs->path,
 			    flags, timeout_value) < 0)
 		return -1;
 
@@ -1361,9 +1374,9 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	 * cache is still valid. We've just locked the file, but it
 	 * might have changed the moment *before* we locked it.
 	 */
-	validate_packed_ref_cache(refs->packed_ref_store);
+	validate_packed_ref_cache(refs);
 
-	packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
+	packed_ref_cache = get_packed_ref_cache(refs);
 	/* Increment the reference count to prevent it from being freed: */
 	acquire_packed_ref_cache(packed_ref_cache);
 	return 0;
@@ -1560,7 +1573,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	int ok;
 	struct ref_to_prune *refs_to_prune = NULL;
 
-	lock_packed_refs(refs, LOCK_DIE_ON_ERROR);
+	lock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
 
 	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -1629,7 +1642,7 @@ static int repack_without_refs(struct files_ref_store *refs,
 	if (!needs_repacking)
 		return 0; /* no refname exists in packed refs */
 
-	if (lock_packed_refs(refs, 0)) {
+	if (lock_packed_refs(refs->packed_ref_store, 0)) {
 		unable_to_lock_message(refs->packed_ref_store->path, errno, err);
 		return -1;
 	}
@@ -3198,7 +3211,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 		}
 	}
 
-	if (lock_packed_refs(refs, 0)) {
+	if (lock_packed_refs(refs->packed_ref_store, 0)) {
 		strbuf_addf(err, "unable to lock packed-refs file: %s",
 			    strerror(errno));
 		ret = TRANSACTION_GENERIC_ERROR;
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 11/28] commit_packed_refs(): take a `packed_ref_store *` parameter

It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
Show changes to refs/files-backend.c +9 −9
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0d8f39089f..5d159620f0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1388,21 +1388,21 @@ static int lock_packed_refs(struct packed_ref_store *refs, int flags)
  * lock_packed_refs()). Return zero on success. On errors, set errno
  * and return a nonzero value
  */
-static int commit_packed_refs(struct files_ref_store *refs)
+static int commit_packed_refs(struct packed_ref_store *refs)
 {
 	struct packed_ref_cache *packed_ref_cache =
-		get_packed_ref_cache(refs->packed_ref_store);
+		get_packed_ref_cache(refs);
 	int ok, error = 0;
 	int save_errno = 0;
 	FILE *out;
 	struct ref_iterator *iter;
 
-	files_assert_main_repository(refs, "commit_packed_refs");
+	packed_assert_main_repository(refs, "commit_packed_refs");
 
-	if (!is_lock_file_locked(&refs->packed_ref_store->lock))
+	if (!is_lock_file_locked(&refs->lock))
 		die("BUG: packed-refs not locked");
 
-	out = fdopen_lock_file(&refs->packed_ref_store->lock, "w");
+	out = fdopen_lock_file(&refs->lock, "w");
 	if (!out)
 		die_errno("unable to fdopen packed-refs descriptor");
 
@@ -1420,7 +1420,7 @@ static int commit_packed_refs(struct files_ref_store *refs)
 	if (ok != ITER_DONE)
 		die("error while iterating over references");
 
-	if (commit_lock_file(&refs->packed_ref_store->lock)) {
+	if (commit_lock_file(&refs->lock)) {
 		save_errno = errno;
 		error = -1;
 	}
@@ -1606,7 +1606,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	if (ok != ITER_DONE)
 		die("error while iterating over references");
 
-	if (commit_packed_refs(refs))
+	if (commit_packed_refs(refs->packed_ref_store))
 		die_errno("unable to overwrite old ref-pack file");
 
 	prune_refs(refs, refs_to_prune);
@@ -1662,7 +1662,7 @@ static int repack_without_refs(struct files_ref_store *refs,
 	}
 
 	/* Write what remains */
-	ret = commit_packed_refs(refs);
+	ret = commit_packed_refs(refs->packed_ref_store);
 	if (ret)
 		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
 			    strerror(errno));
@@ -3227,7 +3227,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 				       &update->new_oid);
 	}
 
-	if (commit_packed_refs(refs)) {
+	if (commit_packed_refs(refs->packed_ref_store)) {
 		strbuf_addf(err, "unable to commit packed-refs file: %s",
 			    strerror(errno));
 		ret = TRANSACTION_GENERIC_ERROR;
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 13/28] get_packed_ref(): take a `packed_ref_store *` parameter

It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
Show changes to refs/files-backend.c +7 −5
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a08d3fbadf..2b9d93d3b6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -602,10 +602,10 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
  * Return the ref_entry for the given refname from the packed
  * references.  If it does not exist, return NULL.
  */
-static struct ref_entry *get_packed_ref(struct files_ref_store *refs,
+static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,
 					const char *refname)
 {
-	return find_ref_entry(get_packed_refs(refs->packed_ref_store), refname);
+	return find_ref_entry(get_packed_refs(refs), refname);
 }
 
 /*
@@ -621,7 +621,7 @@ static int resolve_packed_ref(struct files_ref_store *refs,
 	 * The loose reference file does not exist; check for a packed
 	 * reference.
 	 */
-	entry = get_packed_ref(refs, refname);
+	entry = get_packed_ref(refs->packed_ref_store, refname);
 	if (entry) {
 		hashcpy(sha1, entry->u.value.oid.hash);
 		*flags |= REF_ISPACKED;
@@ -1044,7 +1044,9 @@ static int files_peel_ref(struct ref_store *ref_store,
 	 * have REF_KNOWS_PEELED.
 	 */
 	if (flag & REF_ISPACKED) {
-		struct ref_entry *r = get_packed_ref(refs, refname);
+		struct ref_entry *r =
+			get_packed_ref(refs->packed_ref_store, refname);
+
 		if (r) {
 			if (peel_entry(r, 0))
 				return -1;
@@ -1631,7 +1633,7 @@ static int repack_without_refs(struct files_ref_store *refs,
 
 	/* Look for a packed ref */
 	for_each_string_list_item(refname, refnames) {
-		if (get_packed_ref(refs, refname->string)) {
+		if (get_packed_ref(refs->packed_ref_store, refname->string)) {
 			needs_repacking = 1;
 			break;
 		}
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 14/28] repack_without_refs(): take a `packed_ref_store *` parameter

It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
Show changes to refs/files-backend.c +10 −10
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2b9d93d3b6..c206791b91 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1621,19 +1621,19 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
  *
  * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
  */
-static int repack_without_refs(struct files_ref_store *refs,
+static int repack_without_refs(struct packed_ref_store *refs,
 			       struct string_list *refnames, struct strbuf *err)
 {
 	struct ref_dir *packed;
 	struct string_list_item *refname;
 	int ret, needs_repacking = 0, removed = 0;
 
-	files_assert_main_repository(refs, "repack_without_refs");
+	packed_assert_main_repository(refs, "repack_without_refs");
 	assert(err);
 
 	/* Look for a packed ref */
 	for_each_string_list_item(refname, refnames) {
-		if (get_packed_ref(refs->packed_ref_store, refname->string)) {
+		if (get_packed_ref(refs, refname->string)) {
 			needs_repacking = 1;
 			break;
 		}
@@ -1643,11 +1643,11 @@ static int repack_without_refs(struct files_ref_store *refs,
 	if (!needs_repacking)
 		return 0; /* no refname exists in packed refs */
 
-	if (lock_packed_refs(refs->packed_ref_store, 0)) {
-		unable_to_lock_message(refs->packed_ref_store->path, errno, err);
+	if (lock_packed_refs(refs, 0)) {
+		unable_to_lock_message(refs->path, errno, err);
 		return -1;
 	}
-	packed = get_packed_refs(refs->packed_ref_store);
+	packed = get_packed_refs(refs);
 
 	/* Remove refnames from the cache */
 	for_each_string_list_item(refname, refnames)
@@ -1658,12 +1658,12 @@ static int repack_without_refs(struct files_ref_store *refs,
 		 * All packed entries disappeared while we were
 		 * acquiring the lock.
 		 */
-		rollback_packed_refs(refs->packed_ref_store);
+		rollback_packed_refs(refs);
 		return 0;
 	}
 
 	/* Write what remains */
-	ret = commit_packed_refs(refs->packed_ref_store);
+	ret = commit_packed_refs(refs);
 	if (ret)
 		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
 			    strerror(errno));
@@ -1681,7 +1681,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
 	if (!refnames->nr)
 		return 0;
 
-	result = repack_without_refs(refs, refnames, &err);
+	result = repack_without_refs(refs->packed_ref_store, refnames, &err);
 	if (result) {
 		/*
 		 * If we failed to rewrite the packed-refs file, then
@@ -3101,7 +3101,7 @@ static int files_transaction_finish(struct ref_store *ref_store,
 		}
 	}
 
-	if (repack_without_refs(refs, &refs_to_delete, err)) {
+	if (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 15/28] packed_peel_ref(): new function, extracted from `files_peel_ref()`

This will later become a method of `packed_ref_store`.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
Show changes to refs/files-backend.c +15 −11
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c206791b91..185d05e1d6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1013,6 +1013,18 @@ static int lock_raw_ref(struct files_ref_store *refs,
 	return ret;
 }
 
+static int packed_peel_ref(struct packed_ref_store *refs,
+			   const char *refname, unsigned char *sha1)
+{
+	struct ref_entry *r = get_packed_ref(refs, refname);
+
+	if (!r || peel_entry(r, 0))
+		return -1;
+
+	hashcpy(sha1, r->u.value.peeled.hash);
+	return 0;
+}
+
 static int files_peel_ref(struct ref_store *ref_store,
 			  const char *refname, unsigned char *sha1)
 {
@@ -1043,17 +1055,9 @@ static int files_peel_ref(struct ref_store *ref_store,
 	 * be expensive and (b) loose references anyway usually do not
 	 * have REF_KNOWS_PEELED.
 	 */
-	if (flag & REF_ISPACKED) {
-		struct ref_entry *r =
-			get_packed_ref(refs->packed_ref_store, refname);
-
-		if (r) {
-			if (peel_entry(r, 0))
-				return -1;
-			hashcpy(sha1, r->u.value.peeled.hash);
-			return 0;
-		}
-	}
+	if (flag & REF_ISPACKED &&
+	    !packed_peel_ref(refs->packed_ref_store, refname, sha1))
+		return 0;
 
 	return peel_object(base, sha1);
 }
-- 
2.11.0
Stefan Beller· Jun 16, 2017, 05:42 UTC · re: Michael Haggerty · lore

Re: [PATCH 15/28] packed_peel_ref(): new function, extracted from `files_peel_ref()`

On Thu, Jun 15, 2017 at 7:47 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> This will later become a method of `packed_ref_store`.

Also while touching it, maybe rename sha1 to object_hash (not saying object_id as that would be confusing with the actual oid struct), maybe?

Michael Haggerty· Jun 16, 2017, 06:46 UTC · re: Stefan Beller · lore

Re: [PATCH 15/28] packed_peel_ref(): new function, extracted from `files_peel_ref()`

On 06/16/2017 07:42 AM, Stefan Beller wrote:
Show 6 quoted lines
> On Thu, Jun 15, 2017 at 7:47 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> This will later become a method of `packed_ref_store`.
> 
> Also while touching it, maybe rename sha1 to object_hash
> (not saying object_id as that would be confusing with the actual
> oid struct), maybe?

Hmmm, my impression was that most of the `unsigned char *` hashes are still called sha1, and they are renamed to `oid` at the moment that they are converted to `struct object_id *`. I only see two instances of the string "object_hash" in the code:

$ git grep object_hash object.c:static void grow_object_hash(void) object.c: grow_object_hash();

Michael
Stefan Beller· Jun 16, 2017, 15:47 UTC · re: Michael Haggerty · lore

Re: [PATCH 15/28] packed_peel_ref(): new function, extracted from `files_peel_ref()`

On Thu, Jun 15, 2017 at 11:46 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 18 quoted lines
> On 06/16/2017 07:42 AM, Stefan Beller wrote:
>> On Thu, Jun 15, 2017 at 7:47 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> This will later become a method of `packed_ref_store`.
>>
>> Also while touching it, maybe rename sha1 to object_hash
>> (not saying object_id as that would be confusing with the actual
>> oid struct), maybe?
>
> Hmmm, my impression was that most of the `unsigned char *` hashes are
> still called sha1, and they are renamed to `oid` at the moment that they
> are converted to `struct object_id *`. I only see two instances of the
> string "object_hash" in the code:
>
> $ git grep object_hash
> object.c:static void grow_object_hash(void)
> object.c:               grow_object_hash();
>
> Michael

Yeah maybe we can just defer it to the proper conversion series. I just jumped the gun here.

Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 16/28] packed_ref_store: support iteration

Add the infrastructure to iterate over a `packed_ref_store`. It's a lot of boilerplate, but it's all part of a campaign to make `packed_ref_store` implement `ref_store`. In the future, this iterator will work much differently.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 98 insertions(+), 9 deletions(-)
Show changes to refs/files-backend.c +98 −9
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 185d05e1d6..e57cdeba36 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1062,10 +1062,102 @@ static int files_peel_ref(struct ref_store *ref_store,
 	return peel_object(base, sha1);
 }
 
+struct packed_ref_iterator {
+	struct ref_iterator base;
+
+	struct packed_ref_cache *cache;
+	struct ref_iterator *iter0;
+	unsigned int flags;
+};
+
+static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+	struct packed_ref_iterator *iter =
+		(struct packed_ref_iterator *)ref_iterator;
+	int ok;
+
+	while ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {
+		if (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&
+		    ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)
+			continue;
+
+		if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
+		    !ref_resolves_to_object(iter->iter0->refname,
+					    iter->iter0->oid,
+					    iter->iter0->flags))
+			continue;
+
+		iter->base.refname = iter->iter0->refname;
+		iter->base.oid = iter->iter0->oid;
+		iter->base.flags = iter->iter0->flags;
+		return ITER_OK;
+	}
+
+	iter->iter0 = NULL;
+	if (ref_iterator_abort(ref_iterator) != ITER_DONE)
+		ok = ITER_ERROR;
+
+	return ok;
+}
+
+static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,
+				   struct object_id *peeled)
+{
+	struct packed_ref_iterator *iter =
+		(struct packed_ref_iterator *)ref_iterator;
+
+	return ref_iterator_peel(iter->iter0, peeled);
+}
+
+static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)
+{
+	struct packed_ref_iterator *iter =
+		(struct packed_ref_iterator *)ref_iterator;
+	int ok = ITER_DONE;
+
+	if (iter->iter0)
+		ok = ref_iterator_abort(iter->iter0);
+
+	release_packed_ref_cache(iter->cache);
+	base_ref_iterator_free(ref_iterator);
+	return ok;
+}
+
+static struct ref_iterator_vtable packed_ref_iterator_vtable = {
+	packed_ref_iterator_advance,
+	packed_ref_iterator_peel,
+	packed_ref_iterator_abort
+};
+
+static struct ref_iterator *packed_ref_iterator_begin(
+		struct packed_ref_store *refs,
+		const char *prefix, unsigned int flags)
+{
+	struct packed_ref_iterator *iter;
+	struct ref_iterator *ref_iterator;
+
+	iter = xcalloc(1, sizeof(*iter));
+	ref_iterator = &iter->base;
+	base_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);
+
+	/*
+	 * Note that get_packed_ref_cache() internally checks whether
+	 * the packed-ref cache is up to date with what is on disk,
+	 * and re-reads it if not.
+	 */
+
+	iter->cache = get_packed_ref_cache(refs);
+	acquire_packed_ref_cache(iter->cache);
+	iter->iter0 = cache_ref_iterator_begin(iter->cache->cache, prefix, 0);
+
+	iter->flags = flags;
+
+	return ref_iterator;
+}
+
 struct files_ref_iterator {
 	struct ref_iterator base;
 
-	struct packed_ref_cache *packed_ref_cache;
 	struct ref_iterator *iter0;
 	unsigned int flags;
 };
@@ -1118,7 +1210,6 @@ static int files_ref_iterator_abort(struct ref_iterator *ref_iterator)
 	if (iter->iter0)
 		ok = ref_iterator_abort(iter->iter0);
 
-	release_packed_ref_cache(iter->packed_ref_cache);
 	base_ref_iterator_free(ref_iterator);
 	return ok;
 }
@@ -1160,18 +1251,16 @@ static struct ref_iterator *files_ref_iterator_begin(
 	 * (If they've already been read, that's OK; we only need to
 	 * guarantee that they're read before the packed refs, not
 	 * *how much* before.) After that, we call
-	 * get_packed_ref_cache(), which internally checks whether the
-	 * packed-ref cache is up to date with what is on disk, and
-	 * re-reads it if not.
+	 * packed_ref_iterator_begin(), which internally checks
+	 * whether the packed-ref cache is up to date with what is on
+	 * disk, and re-reads it if not.
 	 */
 
 	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
 					      prefix, 1);
 
-	iter->packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
-	acquire_packed_ref_cache(iter->packed_ref_cache);
-	packed_iter = cache_ref_iterator_begin(iter->packed_ref_cache->cache,
-					       prefix, 0);
+	packed_iter = packed_ref_iterator_begin(refs->packed_ref_store,
+						prefix, 0);
 
 	iter->iter0 = overlay_ref_iterator_begin(loose_iter, packed_iter);
 	iter->flags = flags;
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 17/28] packed_read_raw_ref(): new function, replacing `resolve_packed_ref()`

Add a new function, `packed_read_raw_ref()`, which is nearly a `read_raw_ref_fn`. Use it in place of `resolve_packed_ref()`.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)
Show changes to refs/files-backend.c +17 −19
diff --git a/refs/files-backend.c b/refs/files-backend.c
index e57cdeba36..ac4764f6f7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -608,27 +608,23 @@ static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,
 	return find_ref_entry(get_packed_refs(refs), refname);
 }
 
-/*
- * A loose ref file doesn't exist; check for a packed ref.
- */
-static int resolve_packed_ref(struct files_ref_store *refs,
-			      const char *refname,
-			      unsigned char *sha1, unsigned int *flags)
+static int packed_read_raw_ref(struct packed_ref_store *refs,
+			       const char *refname, unsigned char *sha1,
+			       struct strbuf *referent, unsigned int *type)
 {
 	struct ref_entry *entry;
 
-	/*
-	 * The loose reference file does not exist; check for a packed
-	 * reference.
-	 */
-	entry = get_packed_ref(refs->packed_ref_store, refname);
-	if (entry) {
-		hashcpy(sha1, entry->u.value.oid.hash);
-		*flags |= REF_ISPACKED;
-		return 0;
+	*type = 0;
+
+	entry = get_packed_ref(refs, refname);
+	if (!entry) {
+		errno = ENOENT;
+		return -1;
 	}
-	/* refname is not a packed reference. */
-	return -1;
+
+	hashcpy(sha1, entry->u.value.oid.hash);
+	*type = REF_ISPACKED;
+	return 0;
 }
 
 static int files_read_raw_ref(struct ref_store *ref_store,
@@ -674,7 +670,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	if (lstat(path, &st) < 0) {
 		if (errno != ENOENT)
 			goto out;
-		if (resolve_packed_ref(refs, refname, sha1, type)) {
+		if (packed_read_raw_ref(refs->packed_ref_store, refname,
+					sha1, referent, type)) {
 			errno = ENOENT;
 			goto out;
 		}
@@ -713,7 +710,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 		 * ref is supposed to be, there could still be a
 		 * packed ref:
 		 */
-		if (resolve_packed_ref(refs, refname, sha1, type)) {
+		if (packed_read_raw_ref(refs->packed_ref_store, refname,
+					sha1, referent, type)) {
 			errno = EISDIR;
 			goto out;
 		}
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 19/28] packed_ref_store: make class into a subclass of `ref_store`

Add the infrastructure to make `packed_ref_store` implement `ref_store`, at least formally (few of the methods are actually implemented yet). Change the functions in its interface to take `ref_store *` arguments. Change `files_ref_store` to store a pointer to `ref_store *` and to call functions via the virtual `ref_store` interface where possible. This also means that a few `packed_ref_store` functions can become static.

This is a work in progress. Some more `ref_store` methods will soon be implemented (e.g., those having to do with reference transactions). But some of them will never be implemented (e.g., those having to do with symrefs or reflogs).

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c  |  16 ++--
 refs/packed-backend.c | 231 +++++++++++++++++++++++++++++++++++++++++++++-----
 refs/packed-backend.h |  23 ++---
 refs/refs-internal.h  |   1 +
 4 files changed, 226 insertions(+), 45 deletions(-)
Show changes to 4 files +226 −45

refs/files-backend.c, refs/packed-backend.c, refs/packed-backend.h, refs/refs-internal.h

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 83ea773728..039d0343cb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -28,7 +28,7 @@ struct files_ref_store {
 
 	struct ref_cache *loose;
 
-	struct packed_ref_store *packed_ref_store;
+	struct ref_store *packed_ref_store;
 };
 
 static void clear_loose_ref_cache(struct files_ref_store *refs)
@@ -311,8 +311,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	if (lstat(path, &st) < 0) {
 		if (errno != ENOENT)
 			goto out;
-		if (packed_read_raw_ref(refs->packed_ref_store, refname,
-					sha1, referent, type)) {
+		if (refs_read_raw_ref(refs->packed_ref_store, refname,
+				      sha1, referent, type)) {
 			errno = ENOENT;
 			goto out;
 		}
@@ -351,8 +351,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 		 * ref is supposed to be, there could still be a
 		 * packed ref:
 		 */
-		if (packed_read_raw_ref(refs->packed_ref_store, refname,
-					sha1, referent, type)) {
+		if (refs_read_raw_ref(refs->packed_ref_store, refname,
+				      sha1, referent, type)) {
 			errno = EISDIR;
 			goto out;
 		}
@@ -683,7 +683,7 @@ static int files_peel_ref(struct ref_store *ref_store,
 	 * have REF_KNOWS_PEELED.
 	 */
 	if (flag & REF_ISPACKED &&
-	    !packed_peel_ref(refs->packed_ref_store, refname, sha1))
+	    !refs_peel_ref(refs->packed_ref_store, refname, sha1))
 		return 0;
 
 	return peel_object(base, sha1);
@@ -793,8 +793,8 @@ static struct ref_iterator *files_ref_iterator_begin(
 	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
 					      prefix, 1);
 
-	packed_iter = packed_ref_iterator_begin(refs->packed_ref_store,
-						prefix, 0);
+	packed_iter = refs_ref_iterator_begin(refs->packed_ref_store,
+					      prefix, 0, 0);
 
 	iter->iter0 = overlay_ref_iterator_begin(loose_iter, packed_iter);
 	iter->flags = flags;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 6fa988c953..5dbe4e4e59 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -50,6 +50,8 @@ static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)
  * `ref_store`.
  */
 struct packed_ref_store {
+	struct ref_store base;
+
 	unsigned int store_flags;
 
 	/* The path of the "packed-refs" file: */
@@ -68,14 +70,17 @@ struct packed_ref_store {
 	struct lock_file lock;
 };
 
-struct packed_ref_store *packed_ref_store_create(
-		const char *path, unsigned int store_flags)
+struct ref_store *packed_ref_store_create(const char *path,
+					  unsigned int store_flags)
 {
 	struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
+	struct ref_store *ref_store = (struct ref_store *)refs;
 
+	base_ref_store_init(ref_store, &refs_be_packed);
 	refs->store_flags = store_flags;
+
 	refs->path = xstrdup(path);
-	return refs;
+	return ref_store;
 }
 
 /*
@@ -91,6 +96,31 @@ static void packed_assert_main_repository(struct packed_ref_store *refs,
 	die("BUG: operation %s only allowed for main ref store", caller);
 }
 
+/*
+ * Downcast `ref_store` to `packed_ref_store`. Die if `ref_store` is
+ * not a `packed_ref_store`. Also die if `packed_ref_store` doesn't
+ * support at least the flags specified in `required_flags`. `caller`
+ * is used in any necessary error messages.
+ */
+static struct packed_ref_store *packed_downcast(struct ref_store *ref_store,
+						unsigned int required_flags,
+						const char *caller)
+{
+	struct packed_ref_store *refs;
+
+	if (ref_store->be != &refs_be_packed)
+		die("BUG: ref_store is type \"%s\" not \"packed\" in %s",
+		    ref_store->be->name, caller);
+
+	refs = (struct packed_ref_store *)ref_store;
+
+	if ((refs->store_flags & required_flags) != required_flags)
+		die("BUG: unallowed operation (%s), requires %x, has %x\n",
+		    caller, required_flags, refs->store_flags);
+
+	return refs;
+}
+
 static void clear_packed_ref_cache(struct packed_ref_store *refs)
 {
 	if (refs->cache) {
@@ -287,9 +317,12 @@ static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)
  * (see lock_packed_refs()). To actually write the packed-refs file,
  * call commit_packed_refs().
  */
-void add_packed_ref(struct packed_ref_store *refs,
+void add_packed_ref(struct ref_store *ref_store,
 		    const char *refname, const struct object_id *oid)
 {
+	struct packed_ref_store *refs =
+		packed_downcast(ref_store, REF_STORE_WRITE,
+				"add_packed_ref");
 	struct ref_dir *packed_refs;
 	struct ref_entry *packed_entry;
 
@@ -322,10 +355,13 @@ static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,
 	return find_ref_entry(get_packed_refs(refs), refname);
 }
 
-int packed_read_raw_ref(struct packed_ref_store *refs,
-			const char *refname, unsigned char *sha1,
-			struct strbuf *referent, unsigned int *type)
+static int packed_read_raw_ref(struct ref_store *ref_store,
+			       const char *refname, unsigned char *sha1,
+			       struct strbuf *referent, unsigned int *type)
 {
+	struct packed_ref_store *refs =
+	        packed_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
+
 	struct ref_entry *entry;
 
 	*type = 0;
@@ -341,9 +377,12 @@ int packed_read_raw_ref(struct packed_ref_store *refs,
 	return 0;
 }
 
-int packed_peel_ref(struct packed_ref_store *refs,
-		    const char *refname, unsigned char *sha1)
+static int packed_peel_ref(struct ref_store *ref_store,
+			   const char *refname, unsigned char *sha1)
 {
+	struct packed_ref_store *refs =
+	        packed_downcast(ref_store, REF_STORE_READ | REF_STORE_ODB,
+				"peel_ref");
 	struct ref_entry *r = get_packed_ref(refs, refname);
 
 	if (!r || peel_entry(r, 0))
@@ -420,12 +459,18 @@ static struct ref_iterator_vtable packed_ref_iterator_vtable = {
 	packed_ref_iterator_abort
 };
 
-struct ref_iterator *packed_ref_iterator_begin(
-		struct packed_ref_store *refs,
+static struct ref_iterator *packed_ref_iterator_begin(
+		struct ref_store *ref_store,
 		const char *prefix, unsigned int flags)
 {
+	struct packed_ref_store *refs;
 	struct packed_ref_iterator *iter;
 	struct ref_iterator *ref_iterator;
+	unsigned int required_flags = REF_STORE_READ;
+
+	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN))
+		required_flags |= REF_STORE_ODB;
+	refs = packed_downcast(ref_store, required_flags, "ref_iterator_begin");
 
 	iter = xcalloc(1, sizeof(*iter));
 	ref_iterator = &iter->base;
@@ -459,14 +504,15 @@ static void write_packed_entry(FILE *fh, const char *refname,
 		fprintf_or_die(fh, "^%s\n", sha1_to_hex(peeled));
 }
 
-int lock_packed_refs(struct packed_ref_store *refs, int flags)
+int lock_packed_refs(struct ref_store *ref_store, int flags)
 {
+	struct packed_ref_store *refs =
+	        packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
+				"lock_packed_refs");
 	static int timeout_configured = 0;
 	static int timeout_value = 1000;
 	struct packed_ref_cache *packed_ref_cache;
 
-	packed_assert_main_repository(refs, "lock_packed_refs");
-
 	if (!timeout_configured) {
 		git_config_get_int("core.packedrefstimeout", &timeout_value);
 		timeout_configured = 1;
@@ -507,8 +553,11 @@ static const char PACKED_REFS_HEADER[] =
  * lock_packed_refs()). Return zero on success. On errors, set errno
  * and return a nonzero value.
  */
-int commit_packed_refs(struct packed_ref_store *refs)
+int commit_packed_refs(struct ref_store *ref_store)
 {
+	struct packed_ref_store *refs =
+	        packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
+				"commit_packed_refs");
 	struct packed_ref_cache *packed_ref_cache =
 		get_packed_ref_cache(refs);
 	int ok, error = 0;
@@ -516,8 +565,6 @@ int commit_packed_refs(struct packed_ref_store *refs)
 	FILE *out;
 	struct ref_iterator *iter;
 
-	packed_assert_main_repository(refs, "commit_packed_refs");
-
 	if (!is_lock_file_locked(&refs->lock))
 		die("BUG: packed-refs not locked");
 
@@ -573,9 +620,12 @@ static void rollback_packed_refs(struct packed_ref_store *refs)
  *
  * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
  */
-int repack_without_refs(struct packed_ref_store *refs,
+int repack_without_refs(struct ref_store *ref_store,
 			struct string_list *refnames, struct strbuf *err)
 {
+	struct packed_ref_store *refs =
+	        packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
+				"repack_without_refs");
 	struct ref_dir *packed;
 	struct string_list_item *refname;
 	int ret, needs_repacking = 0, removed = 0;
@@ -595,7 +645,7 @@ int repack_without_refs(struct packed_ref_store *refs,
 	if (!needs_repacking)
 		return 0; /* no refname exists in packed refs */
 
-	if (lock_packed_refs(refs, 0)) {
+	if (lock_packed_refs(&refs->base, 0)) {
 		unable_to_lock_message(refs->path, errno, err);
 		return -1;
 	}
@@ -615,10 +665,151 @@ int repack_without_refs(struct packed_ref_store *refs,
 	}
 
 	/* Write what remains */
-	ret = commit_packed_refs(refs);
+	ret = commit_packed_refs(&refs->base);
 	if (ret)
 		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
 			    strerror(errno));
 	return ret;
 }
 
+static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)
+{
+	/* Nothing to do. */
+	return 0;
+}
+
+static int packed_transaction_prepare(struct ref_store *ref_store,
+				      struct ref_transaction *transaction,
+				      struct strbuf *err)
+{
+	die("BUG: not implemented yet");
+}
+
+static int packed_transaction_abort(struct ref_store *ref_store,
+				    struct ref_transaction *transaction,
+				    struct strbuf *err)
+{
+	die("BUG: not implemented yet");
+}
+
+static int packed_transaction_finish(struct ref_store *ref_store,
+				     struct ref_transaction *transaction,
+				     struct strbuf *err)
+{
+	die("BUG: not implemented yet");
+}
+
+static int packed_initial_transaction_commit(struct ref_store *ref_store,
+					    struct ref_transaction *transaction,
+					    struct strbuf *err)
+{
+	return ref_transaction_commit(transaction, err);
+}
+
+static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
+			     struct string_list *refnames, unsigned int flags)
+{
+	die("BUG: not implemented yet");
+}
+
+static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)
+{
+	/*
+	 * Packed refs are already packed. It might be that loose refs
+	 * are packed *into* a packed refs store, but that is done by
+	 * updating the packed references via a transaction.
+	 */
+	return 0;
+}
+
+static int packed_create_symref(struct ref_store *ref_store,
+			       const char *refname, const char *target,
+			       const char *logmsg)
+{
+	die("BUG: packed reference store does not support symrefs");
+}
+
+static int packed_rename_ref(struct ref_store *ref_store,
+			    const char *oldrefname, const char *newrefname,
+			    const char *logmsg)
+{
+	die("BUG: packed reference store does not support renaming references");
+}
+
+static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_store)
+{
+	return empty_ref_iterator_begin();
+}
+
+static int packed_for_each_reflog_ent(struct ref_store *ref_store,
+				      const char *refname,
+				      each_reflog_ent_fn fn, void *cb_data)
+{
+	return 0;
+}
+
+static int packed_for_each_reflog_ent_reverse(struct ref_store *ref_store,
+					      const char *refname,
+					      each_reflog_ent_fn fn,
+					      void *cb_data)
+{
+	return 0;
+}
+
+static int packed_reflog_exists(struct ref_store *ref_store,
+			       const char *refname)
+{
+	return 0;
+}
+
+static int packed_create_reflog(struct ref_store *ref_store,
+			       const char *refname, int force_create,
+			       struct strbuf *err)
+{
+	die("BUG: packed reference store does not support reflogs");
+}
+
+static int packed_delete_reflog(struct ref_store *ref_store,
+			       const char *refname)
+{
+	return 0;
+}
+
+static int packed_reflog_expire(struct ref_store *ref_store,
+				const char *refname, const unsigned char *sha1,
+				unsigned int flags,
+				reflog_expiry_prepare_fn prepare_fn,
+				reflog_expiry_should_prune_fn should_prune_fn,
+				reflog_expiry_cleanup_fn cleanup_fn,
+				void *policy_cb_data)
+{
+	return 0;
+}
+
+struct ref_storage_be refs_be_packed = {
+	NULL,
+	"packed",
+	packed_ref_store_create,
+	packed_init_db,
+	packed_transaction_prepare,
+	packed_transaction_finish,
+	packed_transaction_abort,
+	packed_initial_transaction_commit,
+
+	packed_pack_refs,
+	packed_peel_ref,
+	packed_create_symref,
+	packed_delete_refs,
+	packed_rename_ref,
+
+	packed_ref_iterator_begin,
+	packed_read_raw_ref,
+
+	packed_reflog_iterator_begin,
+	packed_for_each_reflog_ent,
+	packed_for_each_reflog_ent_reverse,
+	packed_reflog_exists,
+	packed_create_reflog,
+	packed_delete_reflog,
+	packed_reflog_expire
+};
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 22e8817ac4..beea9c14b5 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -1,33 +1,22 @@
 #ifndef REFS_PACKED_BACKEND_H
 #define REFS_PACKED_BACKEND_H
 
-struct packed_ref_store *packed_ref_store_create(
-		const char *path, unsigned int store_flags);
-
-int packed_read_raw_ref(struct packed_ref_store *refs,
-			const char *refname, unsigned char *sha1,
-			struct strbuf *referent, unsigned int *type);
-
-int packed_peel_ref(struct packed_ref_store *refs,
-		    const char *refname, unsigned char *sha1);
-
-struct ref_iterator *packed_ref_iterator_begin(
-		struct packed_ref_store *refs,
-		const char *prefix, unsigned int flags);
+struct ref_store *packed_ref_store_create(const char *path,
+					  unsigned int store_flags);
 
 /*
  * Lock the packed-refs file for writing. Flags is passed to
  * hold_lock_file_for_update(). Return 0 on success. On errors, set
  * errno appropriately and return a nonzero value.
  */
-int lock_packed_refs(struct packed_ref_store *refs, int flags);
+int lock_packed_refs(struct ref_store *ref_store, int flags);
 
-void add_packed_ref(struct packed_ref_store *refs,
+void add_packed_ref(struct ref_store *ref_store,
 		    const char *refname, const struct object_id *oid);
 
-int commit_packed_refs(struct packed_ref_store *refs);
+int commit_packed_refs(struct ref_store *ref_store);
 
-int repack_without_refs(struct packed_ref_store *refs,
+int repack_without_refs(struct ref_store *ref_store,
 			struct string_list *refnames, struct strbuf *err);
 
 #endif /* REFS_PACKED_BACKEND_H */
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 6f8f9f5619..4789106fc0 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -664,6 +664,7 @@ struct ref_storage_be {
 };
 
 extern struct ref_storage_be refs_be_files;
+extern struct ref_storage_be refs_be_packed;
 
 /*
  * A representation of the reference store for the main repository or
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 20/28] commit_packed_refs(): report errors rather than dying

Report errors via a `struct strbuf *err` rather than by calling `die()`. To enable this goal, change `write_packed_entry()` to report errors via a return value and `errno` rather than dying.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c  | 10 +++---
 refs/packed-backend.c | 85 +++++++++++++++++++++++++++++++++------------------
 refs/packed-backend.h |  2 +-
 3 files changed, 61 insertions(+), 36 deletions(-)
Show changes to 3 files +61 −36

refs/files-backend.c, refs/packed-backend.c, refs/packed-backend.h

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 039d0343cb..4323394a52 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1082,6 +1082,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	struct ref_iterator *iter;
 	int ok;
 	struct ref_to_prune *refs_to_prune = NULL;
+	struct strbuf err = STRBUF_INIT;
 
 	lock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
 
@@ -1116,10 +1117,11 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	if (ok != ITER_DONE)
 		die("error while iterating over references");
 
-	if (commit_packed_refs(refs->packed_ref_store))
-		die_errno("unable to overwrite old ref-pack file");
+	if (commit_packed_refs(refs->packed_ref_store, &err))
+		die("unable to overwrite old ref-pack file: %s", err.buf);
 
 	prune_refs(refs, refs_to_prune);
+	strbuf_release(&err);
 	return 0;
 }
 
@@ -2681,9 +2683,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 				       &update->new_oid);
 	}
 
-	if (commit_packed_refs(refs->packed_ref_store)) {
-		strbuf_addf(err, "unable to commit packed-refs file: %s",
-			    strerror(errno));
+	if (commit_packed_refs(refs->packed_ref_store, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 5dbe4e4e59..5bee49d497 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -493,15 +493,19 @@ static struct ref_iterator *packed_ref_iterator_begin(
 
 /*
  * Write an entry to the packed-refs file for the specified refname.
- * If peeled is non-NULL, write it as the entry's peeled value.
+ * If peeled is non-NULL, write it as the entry's peeled value. On
+ * error, return a nonzero value and leave errno set at the value left
+ * by the failing call to `fprintf()`.
  */
-static void write_packed_entry(FILE *fh, const char *refname,
-			       const unsigned char *sha1,
-			       const unsigned char *peeled)
+static int write_packed_entry(FILE *fh, const char *refname,
+			      const unsigned char *sha1,
+			      const unsigned char *peeled)
 {
-	fprintf_or_die(fh, "%s %s\n", sha1_to_hex(sha1), refname);
-	if (peeled)
-		fprintf_or_die(fh, "^%s\n", sha1_to_hex(peeled));
+	if (fprintf(fh, "%s %s\n", sha1_to_hex(sha1), refname) < 0 ||
+	    (peeled && fprintf(fh, "^%s\n", sha1_to_hex(peeled)) < 0))
+		return -1;
+
+	return 0;
 }
 
 int lock_packed_refs(struct ref_store *ref_store, int flags)
@@ -550,49 +554,74 @@ static const char PACKED_REFS_HEADER[] =
 /*
  * Write the current version of the packed refs cache from memory to
  * disk. The packed-refs file must already be locked for writing (see
- * lock_packed_refs()). Return zero on success. On errors, set errno
- * and return a nonzero value.
+ * lock_packed_refs()). Return zero on success. On errors, rollback
+ * the lockfile, write an error message to `err`, and return a nonzero
+ * value.
  */
-int commit_packed_refs(struct ref_store *ref_store)
+int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 {
 	struct packed_ref_store *refs =
 	        packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
 				"commit_packed_refs");
 	struct packed_ref_cache *packed_ref_cache =
 		get_packed_ref_cache(refs);
-	int ok, error = 0;
-	int save_errno = 0;
+	int ok;
+	int ret = -1;
 	FILE *out;
 	struct ref_iterator *iter;
 
 	if (!is_lock_file_locked(&refs->lock))
-		die("BUG: packed-refs not locked");
+		die("BUG: commit_packed_refs() called when unlocked");
 
 	out = fdopen_lock_file(&refs->lock, "w");
-	if (!out)
-		die_errno("unable to fdopen packed-refs descriptor");
+	if (!out) {
+		strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
+			    strerror(errno));
+		goto error;
+	}
 
-	fprintf_or_die(out, "%s", PACKED_REFS_HEADER);
+	if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
+		strbuf_addf(err, "error writing to %s: %s",
+			    get_lock_file_path(&refs->lock), strerror(errno));
+		goto error;
+	}
 
 	iter = cache_ref_iterator_begin(packed_ref_cache->cache, NULL, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
 		struct object_id peeled;
 		int peel_error = ref_iterator_peel(iter, &peeled);
 
-		write_packed_entry(out, iter->refname, iter->oid->hash,
-				   peel_error ? NULL : peeled.hash);
+		if (write_packed_entry(out, iter->refname, iter->oid->hash,
+				       peel_error ? NULL : peeled.hash)) {
+			strbuf_addf(err, "error writing to %s: %s",
+				    get_lock_file_path(&refs->lock),
+				    strerror(errno));
+			ref_iterator_abort(iter);
+			goto error;
+		}
 	}
 
-	if (ok != ITER_DONE)
-		die("error while iterating over references");
+	if (ok != ITER_DONE) {
+		strbuf_addf(err, "unable to write packed-refs file: "
+			    "error iterating over old contents");
+		goto error;
+	}
 
 	if (commit_lock_file(&refs->lock)) {
-		save_errno = errno;
-		error = -1;
+		strbuf_addf(err, "error overwriting %s: %s",
+			    refs->path, strerror(errno));
+		goto out;
 	}
+
+	ret = 0;
+	goto out;
+
+error:
+	rollback_lock_file(&refs->lock);
+
+out:
 	release_packed_ref_cache(packed_ref_cache);
-	errno = save_errno;
-	return error;
+	return ret;
 }
 
 /*
@@ -628,7 +657,7 @@ int repack_without_refs(struct ref_store *ref_store,
 				"repack_without_refs");
 	struct ref_dir *packed;
 	struct string_list_item *refname;
-	int ret, needs_repacking = 0, removed = 0;
+	int needs_repacking = 0, removed = 0;
 
 	packed_assert_main_repository(refs, "repack_without_refs");
 	assert(err);
@@ -665,11 +694,7 @@ int repack_without_refs(struct ref_store *ref_store,
 	}
 
 	/* Write what remains */
-	ret = commit_packed_refs(&refs->base);
-	if (ret)
-		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
-			    strerror(errno));
-	return ret;
+	return commit_packed_refs(&refs->base, err);
 }
 
 static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index beea9c14b5..3d4057b65b 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -14,7 +14,7 @@ int lock_packed_refs(struct ref_store *ref_store, int flags);
 void add_packed_ref(struct ref_store *ref_store,
 		    const char *refname, const struct object_id *oid);
 
-int commit_packed_refs(struct ref_store *ref_store);
+int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err);
 
 int repack_without_refs(struct ref_store *ref_store,
 			struct string_list *refnames, struct strbuf *err);
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

We will want to be able to hold the lockfile for `packed-refs` even after we have activated the new values. So use a separate tempfile, `packed-refs.new`, as a place to stage the new contents of the `packed-refs` file. For now this is all done within `commit_packed_refs()`, but that will change shortly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/packed-backend.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)
Show changes to refs/packed-backend.c +32 −8
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 5bee49d497..6619052e96 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -68,6 +68,13 @@ struct packed_ref_store {
 	 * thus the enclosing `packed_ref_store`) must not be freed.
 	 */
 	struct lock_file lock;
+
+	/*
+	 * Temporary file used when rewriting new contents to the
+	 * "packed-refs" file. Note that this (and thus the enclosing
+	 * `packed_ref_store`) must not be freed.
+	 */
+	struct tempfile tempfile;
 };
 
 struct ref_store *packed_ref_store_create(const char *path,
@@ -522,10 +529,16 @@ int lock_packed_refs(struct ref_store *ref_store, int flags)
 		timeout_configured = 1;
 	}
 
+	/*
+	 * Note that we close the lockfile immediately because we
+	 * don't write new content to it, but rather to a separate
+	 * tempfile.
+	 */
 	if (hold_lock_file_for_update_timeout(
 			    &refs->lock,
 			    refs->path,
-			    flags, timeout_value) < 0)
+			    flags, timeout_value) < 0 ||
+	    close_lock_file(&refs->lock))
 		return -1;
 
 	/*
@@ -567,13 +580,23 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 		get_packed_ref_cache(refs);
 	int ok;
 	int ret = -1;
+	struct strbuf sb = STRBUF_INIT;
 	FILE *out;
 	struct ref_iterator *iter;
 
 	if (!is_lock_file_locked(&refs->lock))
 		die("BUG: commit_packed_refs() called when unlocked");
 
-	out = fdopen_lock_file(&refs->lock, "w");
+	strbuf_addf(&sb, "%s.new", refs->path);
+	if (create_tempfile(&refs->tempfile, sb.buf) < 0) {
+		strbuf_addf(err, "unable to create file %s: %s",
+			    sb.buf, strerror(errno));
+		strbuf_release(&sb);
+		goto out;
+	}
+	strbuf_release(&sb);
+
+	out = fdopen_tempfile(&refs->tempfile, "w");
 	if (!out) {
 		strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
 			    strerror(errno));
@@ -582,7 +605,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 
 	if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
 		strbuf_addf(err, "error writing to %s: %s",
-			    get_lock_file_path(&refs->lock), strerror(errno));
+			    get_tempfile_path(&refs->tempfile), strerror(errno));
 		goto error;
 	}
 
@@ -594,7 +617,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 		if (write_packed_entry(out, iter->refname, iter->oid->hash,
 				       peel_error ? NULL : peeled.hash)) {
 			strbuf_addf(err, "error writing to %s: %s",
-				    get_lock_file_path(&refs->lock),
+				    get_tempfile_path(&refs->tempfile),
 				    strerror(errno));
 			ref_iterator_abort(iter);
 			goto error;
@@ -602,13 +625,13 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 	}
 
 	if (ok != ITER_DONE) {
-		strbuf_addf(err, "unable to write packed-refs file: "
+		strbuf_addf(err, "unable to rewrite packed-refs file: "
 			    "error iterating over old contents");
 		goto error;
 	}
 
-	if (commit_lock_file(&refs->lock)) {
-		strbuf_addf(err, "error overwriting %s: %s",
+	if (rename_tempfile(&refs->tempfile, refs->path)) {
+		strbuf_addf(err, "error replacing %s: %s",
 			    refs->path, strerror(errno));
 		goto out;
 	}
@@ -617,9 +640,10 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 	goto out;
 
 error:
-	rollback_lock_file(&refs->lock);
+	delete_tempfile(&refs->tempfile);
 
 out:
+	rollback_lock_file(&refs->lock);
 	release_packed_ref_cache(packed_ref_cache);
 	return ret;
 }
-- 
2.11.0
Junio C Hamano· Jul 20, 2017, 23:11 UTC · re: Michael Haggerty · lore

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 8 quoted lines
> We will want to be able to hold the lockfile for `packed-refs` even
> after we have activated the new values. So use a separate tempfile,
> `packed-refs.new`, as a place to stage the new contents of the
> `packed-refs` file. For now this is all done within
> `commit_packed_refs()`, but that will change shortly.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

The layout created by "contrib/workdir/git-new-workdir" will be broken by this line of change. "git worktree" is supposed to know that refs/packed-refs is a shared thing and lives in common-dir, so it shouldn't be affected.

Do we care about the ancient layout that used symlinks to emulate the more modern worktree one?

Jonathan Nieder· Jul 20, 2017, 23:30 UTC · re: Junio C Hamano · lore

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 16 quoted lines
>> We will want to be able to hold the lockfile for `packed-refs` even
>> after we have activated the new values. So use a separate tempfile,
>> `packed-refs.new`, as a place to stage the new contents of the
>> `packed-refs` file. For now this is all done within
>> `commit_packed_refs()`, but that will change shortly.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>
> The layout created by "contrib/workdir/git-new-workdir" will be
> broken by this line of change.  "git worktree" is supposed to know
> that refs/packed-refs is a shared thing and lives in common-dir,
> so it shouldn't be affected.
>
> Do we care about the ancient layout that used symlinks to emulate
> the more modern worktree one?

I think we do care. In the context of people's changing workflows, "git worktree" is a relatively new tool. Breaking the older git-new-workdir (and tools that emulate it) would affect a large number of users that don't necessarily know how to clean up the result.

Thanks, Jonathan

Junio C Hamano· Jul 21, 2017, 06:01 UTC · re: Jonathan Nieder · lore

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 8 quoted lines
>> Do we care about the ancient layout that used symlinks to emulate
>> the more modern worktree one?
>
> I think we do care.  In the context of people's changing workflows,
> "git worktree" is a relatively new tool.  Breaking the older
> git-new-workdir (and tools that emulate it) would affect a large
> number of users that don't necessarily know how to clean up the
> result.

Fair enough. Even though the git-new-workdir is on its way out, and we really do want to do certain updates under lock, we cannot just allow the topic to graduate to 'master' in its current form without some transition plan or mitigation strategy.

The general strategy we take for these atomic update of a file entity at $path is to:

 (1) try to create $path.lock exclusively; if we cannot, somebody
     else holds the lock so fail (or backoff and retry, which
     amounts to the same thing in the larger picture).
 (2) update $path.lock and close the fd.
 (3) rename $path.lock to $path atomically to unlock.

Would it be sufficent to tweak the above procedure with a new, zeroth step?

 (0) see $path is a symlink; if so, readlink it and assign the
     result to $path.  Then go on to step (1) above.

I do not think such an enhancement would break atomicity guarantee we want from the locking. When updating .git/packed-refs in an ancient new-workdir'ed directory, we would end up locking the corresponding file in the real repository, which is exactly what we want anyway. As the basic assumption of having a symbolic link in the new-workdir'ed directory is that a symlink can stay the same forever while the linked target will be updated, I think this would be a reasonable short-term "fix".

It would be ideal if we can do this at somewhat a low level, i.e. in the lockfile subsystem.

Thoughts? An even easier way out may be to just make pack-refs a no-op when .git/refs/ is a symbolic link, but as a solution that is far from satisfactory, while "locking the right file" smells right, at least to me.

Junio C Hamano· Jul 21, 2017, 07:14 UTC · re: Junio C Hamano · lore

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

Junio C Hamano <gitster@pobox.com> writes:
Show 28 quoted lines
> The general strategy we take for these atomic update of a file
> entity at $path is to:
>
>  (1) try to create $path.lock exclusively; if we cannot, somebody
>      else holds the lock so fail (or backoff and retry, which
>      amounts to the same thing in the larger picture).
>
>  (2) update $path.lock and close the fd.
>
>  (3) rename $path.lock to $path atomically to unlock.
>
> Would it be sufficent to tweak the above procedure with a new,
> zeroth step?
>
>  (0) see $path is a symlink; if so, readlink it and assign the
>      result to $path.  Then go on to step (1) above.
>
> I do not think such an enhancement would break atomicity guarantee
> we want from the locking.  When updating .git/packed-refs in an
> ancient new-workdir'ed directory, we would end up locking the
> corresponding file in the real repository, which is exactly what we
> want anyway.  As the basic assumption of having a symbolic link in
> the new-workdir'ed directory is that a symlink can stay the same
> forever while the linked target will be updated, I think this would
> be a reasonable short-term "fix".
>
> It would be ideal if we can do this at somewhat a low level, i.e. in
> the lockfile subsystem.

One thing I forgot to mention. For the above to work safely, we should think through the possible interaction this may have with the core.preferSymlinkRefs configuration. If the above is implemented naively, an update of a symref (e.g. "HEAD") that points at something to point at something else would end up taking a lock on the underlying ref and updating it, which is not what we want at all.

Perhaps it is about time we stopped supporting the configuration. We may be able to come up with a solution while keeping it alive, but dropping it would mean we would have one less thing we need to worry about.

Jeff King· Jul 24, 2017, 16:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

On Thu, Jul 20, 2017 at 11:01:45PM -0700, Junio C Hamano wrote:
Show 25 quoted lines
> The general strategy we take for these atomic update of a file
> entity at $path is to:
> 
>  (1) try to create $path.lock exclusively; if we cannot, somebody
>      else holds the lock so fail (or backoff and retry, which
>      amounts to the same thing in the larger picture).
> 
>  (2) update $path.lock and close the fd.
> 
>  (3) rename $path.lock to $path atomically to unlock.
> 
> Would it be sufficent to tweak the above procedure with a new,
> zeroth step?
> 
>  (0) see $path is a symlink; if so, readlink it and assign the
>      result to $path.  Then go on to step (1) above.
> 
> I do not think such an enhancement would break atomicity guarantee
> we want from the locking.  When updating .git/packed-refs in an
> ancient new-workdir'ed directory, we would end up locking the
> corresponding file in the real repository, which is exactly what we
> want anyway.  As the basic assumption of having a symbolic link in
> the new-workdir'ed directory is that a symlink can stay the same
> forever while the linked target will be updated, I think this would
> be a reasonable short-term "fix".

This seems like the correct path to me. If the existing behavior is to lock the referring symref, that seems like a violation of the lock procedure in the first place. Because if "A" points to "B", we take "A.lock" and then modify "B". But "B" may have any number of "A" links pointing to it, eliminating the purpose of the lock.

I thought we already did this, though. And that modifying HEAD (which might be a symlink) required LOCK_NO_DEREF.

-Peff
Jonathan Nieder· Jul 24, 2017, 17:09 UTC · re: Jeff King · lore

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

Hi,
Jeff King wrote:
Show 8 quoted lines
> This seems like the correct path to me. If the existing behavior is to
> lock the referring symref, that seems like a violation of the lock
> procedure in the first place. Because if "A" points to "B", we take
> "A.lock" and then modify "B". But "B" may have any number of "A" links
> pointing to it, eliminating the purpose of the lock.
>
> I thought we already did this, though. And that modifying HEAD (which
> might be a symlink) required LOCK_NO_DEREF.

Yes, I believe the lockfile API already does so. Since this patch creates a ".new" file, not using the lockfile API, it doesn't benefit from that, though.

Thanks, Jonathan

Jeff King· Jul 24, 2017, 17:10 UTC · re: Jonathan Nieder · lore

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

On Mon, Jul 24, 2017 at 10:09:15AM -0700, Jonathan Nieder wrote:
Show 14 quoted lines
> Jeff King wrote:
> 
> > This seems like the correct path to me. If the existing behavior is to
> > lock the referring symref, that seems like a violation of the lock
> > procedure in the first place. Because if "A" points to "B", we take
> > "A.lock" and then modify "B". But "B" may have any number of "A" links
> > pointing to it, eliminating the purpose of the lock.
> >
> > I thought we already did this, though. And that modifying HEAD (which
> > might be a symlink) required LOCK_NO_DEREF.
> 
> Yes, I believe the lockfile API already does so.  Since this patch
> creates a ".new" file, not using the lockfile API, it doesn't benefit
> from that, though.

Ah, I see. This bug makes much more sense, then. And I agree doubly that matching the lock-code's behavior is the right thing to do.

-Peff
Junio C Hamano· Jul 24, 2017, 21:43 UTC · re: Jeff King · lore

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> On Mon, Jul 24, 2017 at 10:09:15AM -0700, Jonathan Nieder wrote:
>
>> Jeff King wrote:
>> 
>> > This seems like the correct path to me. If the existing behavior is to
>> > lock the referring symref, that seems like a violation of the lock
>> > procedure in the first place. Because if "A" points to "B", we take
>> > "A.lock" and then modify "B". But "B" may have any number of "A" links
>> > pointing to it, eliminating the purpose of the lock.
>> >
>> > I thought we already did this, though. And that modifying HEAD (which
>> > might be a symlink) required LOCK_NO_DEREF.
>> 
>> Yes, I believe the lockfile API already does so.  Since this patch
>> creates a ".new" file, not using the lockfile API, it doesn't benefit
>> from that, though.
>
> Ah, I see. This bug makes much more sense, then. And I agree doubly that
> matching the lock-code's behavior is the right thing to do.

OK. Anybody wants to take a crack at it (it is of lower priority to me during the -rc freeze to deal with problems in topics on 'next' or higher)?

Thanks.
Michael Haggerty· Jul 26, 2017, 23:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 21/28] commit_packed_refs(): use a staging file separate from the lockfile

I just submitted a patch [1] that fixes the `packed-refs`-is-a-symlink problem in combination with git-new-workdir.

I haven't considered any possible problems related to `core.preferSymlinkRefs`. That behavior should be unaffected by the packed-ref-store patch series and I'm not very interested in working on it.

Michael
[1] https://public-inbox.org/git/d0da02a8b6f0272fa70ae3b1dc80fee6c6ee8d18.1501111803.git.mhagger@alum.mit.edu/
On Mon, Jul 24, 2017 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 27 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> On Mon, Jul 24, 2017 at 10:09:15AM -0700, Jonathan Nieder wrote:
>>
>>> Jeff King wrote:
>>>
>>> > This seems like the correct path to me. If the existing behavior is to
>>> > lock the referring symref, that seems like a violation of the lock
>>> > procedure in the first place. Because if "A" points to "B", we take
>>> > "A.lock" and then modify "B". But "B" may have any number of "A" links
>>> > pointing to it, eliminating the purpose of the lock.
>>> >
>>> > I thought we already did this, though. And that modifying HEAD (which
>>> > might be a symlink) required LOCK_NO_DEREF.
>>>
>>> Yes, I believe the lockfile API already does so.  Since this patch
>>> creates a ".new" file, not using the lockfile API, it doesn't benefit
>>> from that, though.
>>
>> Ah, I see. This bug makes much more sense, then. And I agree doubly that
>> matching the lock-code's behavior is the right thing to do.
>
> OK.  Anybody wants to take a crack at it (it is of lower priority to
> me during the -rc freeze to deal with problems in topics on 'next'
> or higher)?
>
> Thanks.
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 22/28] packed_refs_lock(): function renamed from lock_packed_refs()

Rename `lock_packed_refs()` to `packed_refs_lock()` for consistency with how other methods are named. Also, it's about to get some companions.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c  |  4 ++--
 refs/packed-backend.c | 10 +++++-----
 refs/packed-backend.h |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)
Show changes to 3 files +8 −8

refs/files-backend.c, refs/packed-backend.c, refs/packed-backend.h

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4323394a52..3fc2254e33 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1084,7 +1084,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	struct ref_to_prune *refs_to_prune = NULL;
 	struct strbuf err = STRBUF_INIT;
 
-	lock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
+	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
 
 	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -2667,7 +2667,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 		}
 	}
 
-	if (lock_packed_refs(refs->packed_ref_store, 0)) {
+	if (packed_refs_lock(refs->packed_ref_store, 0)) {
 		strbuf_addf(err, "unable to lock packed-refs file: %s",
 			    strerror(errno));
 		ret = TRANSACTION_GENERIC_ERROR;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 6619052e96..eb9da04576 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -321,7 +321,7 @@ static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)
 /*
  * Add or overwrite a reference in the in-memory packed reference
  * cache. This may only be called while the packed-refs file is locked
- * (see lock_packed_refs()). To actually write the packed-refs file,
+ * (see packed_refs_lock()). To actually write the packed-refs file,
  * call commit_packed_refs().
  */
 void add_packed_ref(struct ref_store *ref_store,
@@ -515,11 +515,11 @@ static int write_packed_entry(FILE *fh, const char *refname,
 	return 0;
 }
 
-int lock_packed_refs(struct ref_store *ref_store, int flags)
+int packed_refs_lock(struct ref_store *ref_store, int flags)
 {
 	struct packed_ref_store *refs =
 	        packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
-				"lock_packed_refs");
+				"packed_refs_lock");
 	static int timeout_configured = 0;
 	static int timeout_value = 1000;
 	struct packed_ref_cache *packed_ref_cache;
@@ -567,7 +567,7 @@ static const char PACKED_REFS_HEADER[] =
 /*
  * Write the current version of the packed refs cache from memory to
  * disk. The packed-refs file must already be locked for writing (see
- * lock_packed_refs()). Return zero on success. On errors, rollback
+ * packed_refs_lock()). Return zero on success. On errors, rollback
  * the lockfile, write an error message to `err`, and return a nonzero
  * value.
  */
@@ -698,7 +698,7 @@ int repack_without_refs(struct ref_store *ref_store,
 	if (!needs_repacking)
 		return 0; /* no refname exists in packed refs */
 
-	if (lock_packed_refs(&refs->base, 0)) {
+	if (packed_refs_lock(&refs->base, 0)) {
 		unable_to_lock_message(refs->path, errno, err);
 		return -1;
 	}
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 3d4057b65b..dbc00d3396 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -9,7 +9,7 @@ struct ref_store *packed_ref_store_create(const char *path,
  * hold_lock_file_for_update(). Return 0 on success. On errors, set
  * errno appropriately and return a nonzero value.
  */
-int lock_packed_refs(struct ref_store *ref_store, int flags);
+int packed_refs_lock(struct ref_store *ref_store, int flags);
 
 void add_packed_ref(struct ref_store *ref_store,
 		    const char *refname, const struct object_id *oid);
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 18/28] packed-backend: new module for handling packed references

Now that the interface between `files_ref_store` and `packed_ref_store` is relatively narrow, move the latter into a new module, "refs/packed-backend.h" and "refs/packed-backend.c". It still doesn't quite implement the `ref_store` interface, but it will soon.

This commit moves code around and adjusts its visibility, but doesn't change anything.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Makefile              |   1 +
 refs.c                |  18 ++
 refs/files-backend.c  | 640 +-------------------------------------------------
 refs/packed-backend.c | 624 ++++++++++++++++++++++++++++++++++++++++++++++++
 refs/packed-backend.h |  33 +++
 refs/refs-internal.h  |   9 +
 6 files changed, 686 insertions(+), 639 deletions(-)
 create mode 100644 refs/packed-backend.c
 create mode 100644 refs/packed-backend.h
Show changes to 6 files +686 −639

Makefile, refs.c, refs/files-backend.c, refs/packed-backend.c, refs/packed-backend.h, refs/refs-internal.h

diff --git a/Makefile b/Makefile
index 33b888730f..478a8eac5d 100644
--- a/Makefile
+++ b/Makefile
@@ -821,6 +821,7 @@ LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/files-backend.o
 LIB_OBJS += refs/iterator.o
+LIB_OBJS += refs/packed-backend.o
 LIB_OBJS += refs/ref-cache.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += remote.o
diff --git a/refs.c b/refs.c
index 32177969f0..5880c12372 100644
--- a/refs.c
+++ b/refs.c
@@ -173,6 +173,24 @@ int refname_is_safe(const char *refname)
 	return 1;
 }
 
+/*
+ * Return true if refname, which has the specified oid and flags, can
+ * be resolved to an object in the database. If the referred-to object
+ * does not exist, emit a warning and return false.
+ */
+int ref_resolves_to_object(const char *refname,
+			   const struct object_id *oid,
+			   unsigned int flags)
+{
+	if (flags & REF_ISBROKEN)
+		return 0;
+	if (!has_sha1_file(oid->hash)) {
+		error("%s does not point to a valid object!", refname);
+		return 0;
+	}
+	return 1;
+}
+
 char *refs_resolve_refdup(struct ref_store *refs,
 			  const char *refname, int resolve_flags,
 			  unsigned char *sha1, int *flags)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index ac4764f6f7..83ea773728 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2,6 +2,7 @@
 #include "../refs.h"
 #include "refs-internal.h"
 #include "ref-cache.h"
+#include "packed-backend.h"
 #include "../iterator.h"
 #include "../dir-iterator.h"
 #include "../lockfile.h"
@@ -14,85 +15,6 @@ struct ref_lock {
 	struct object_id old_oid;
 };
 
-/*
- * Return true if refname, which has the specified oid and flags, can
- * be resolved to an object in the database. If the referred-to object
- * does not exist, emit a warning and return false.
- */
-static int ref_resolves_to_object(const char *refname,
-				  const struct object_id *oid,
-				  unsigned int flags)
-{
-	if (flags & REF_ISBROKEN)
-		return 0;
-	if (!has_sha1_file(oid->hash)) {
-		error("%s does not point to a valid object!", refname);
-		return 0;
-	}
-	return 1;
-}
-
-struct packed_ref_cache {
-	struct ref_cache *cache;
-
-	/*
-	 * Count of references to the data structure in this instance,
-	 * including the pointer from files_ref_store::packed if any.
-	 * The data will not be freed as long as the reference count
-	 * is nonzero.
-	 */
-	unsigned int referrers;
-
-	/* The metadata from when this packed-refs cache was read */
-	struct stat_validity validity;
-};
-
-/*
- * A container for `packed-refs`-related data. It is not (yet) a
- * `ref_store`.
- */
-struct packed_ref_store {
-	unsigned int store_flags;
-
-	/* The path of the "packed-refs" file: */
-	char *path;
-
-	/*
-	 * A cache of the values read from the `packed-refs` file, if
-	 * it might still be current; otherwise, NULL.
-	 */
-	struct packed_ref_cache *cache;
-
-	/*
-	 * Lock used for the "packed-refs" file. Note that this (and
-	 * thus the enclosing `packed_ref_store`) must not be freed.
-	 */
-	struct lock_file lock;
-};
-
-static struct packed_ref_store *packed_ref_store_create(
-		const char *path, unsigned int store_flags)
-{
-	struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
-
-	refs->store_flags = store_flags;
-	refs->path = xstrdup(path);
-	return refs;
-}
-
-/*
- * Die if refs is not the main ref store. caller is used in any
- * necessary error messages.
- */
-static void packed_assert_main_repository(struct packed_ref_store *refs,
-					  const char *caller)
-{
-	if (refs->store_flags & REF_STORE_MAIN)
-		return;
-
-	die("BUG: operation %s only allowed for main ref store", caller);
-}
-
 /*
  * Future: need to be in "struct repository"
  * when doing a full libification.
@@ -109,42 +31,6 @@ struct files_ref_store {
 	struct packed_ref_store *packed_ref_store;
 };
 
-/*
- * Increment the reference count of *packed_refs.
- */
-static void acquire_packed_ref_cache(struct packed_ref_cache *packed_refs)
-{
-	packed_refs->referrers++;
-}
-
-/*
- * Decrease the reference count of *packed_refs.  If it goes to zero,
- * free *packed_refs and return true; otherwise return false.
- */
-static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)
-{
-	if (!--packed_refs->referrers) {
-		free_ref_cache(packed_refs->cache);
-		stat_validity_clear(&packed_refs->validity);
-		free(packed_refs);
-		return 1;
-	} else {
-		return 0;
-	}
-}
-
-static void clear_packed_ref_cache(struct packed_ref_store *refs)
-{
-	if (refs->cache) {
-		struct packed_ref_cache *cache = refs->cache;
-
-		if (is_lock_file_locked(&refs->lock))
-			die("BUG: packed-ref cache cleared while locked");
-		refs->cache = NULL;
-		release_packed_ref_cache(cache);
-	}
-}
-
 static void clear_loose_ref_cache(struct files_ref_store *refs)
 {
 	if (refs->loose) {
@@ -215,151 +101,6 @@ static struct files_ref_store *files_downcast(struct ref_store *ref_store,
 	return refs;
 }
 
-/* The length of a peeled reference line in packed-refs, including EOL: */
-#define PEELED_LINE_LENGTH 42
-
-/*
- * The packed-refs header line that we write out.  Perhaps other
- * traits will be added later.  The trailing space is required.
- */
-static const char PACKED_REFS_HEADER[] =
-	"# pack-refs with: peeled fully-peeled \n";
-
-/*
- * Parse one line from a packed-refs file.  Write the SHA1 to sha1.
- * Return a pointer to the refname within the line (null-terminated),
- * or NULL if there was a problem.
- */
-static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)
-{
-	const char *ref;
-
-	if (parse_oid_hex(line->buf, oid, &ref) < 0)
-		return NULL;
-	if (!isspace(*ref++))
-		return NULL;
-
-	if (isspace(*ref))
-		return NULL;
-
-	if (line->buf[line->len - 1] != '\n')
-		return NULL;
-	line->buf[--line->len] = 0;
-
-	return ref;
-}
-
-/*
- * 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:
- *
- *   No traits:
- *
- *      Probably no references are peeled. But if the file contains a
- *      peeled value for a reference, we will use it.
- *
- *   peeled:
- *
- *      References under "refs/tags/", if they *can* be peeled, *are*
- *      peeled in this file. References outside of "refs/tags/" are
- *      probably not peeled even if they could have been, but if we find
- *      a peeled value for such a reference we will use it.
- *
- *   fully-peeled:
- *
- *      All references in the file that can be peeled are peeled.
- *      Inversely (and this is more important), any references in the
- *      file for which no peeled value is recorded is not peelable. This
- *      trait should typically be written alongside "peeled" for
- *      compatibility with older clients, but we do not require it
- *      (i.e., "peeled" is a no-op if "fully-peeled" is set).
- */
-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) {
-		if (errno == ENOENT) {
-			/*
-			 * This is OK; it just means that no
-			 * "packed-refs" file has been written yet,
-			 * which is equivalent to it being empty.
-			 */
-			return packed_refs;
-		} else {
-			die_errno("couldn't read %s", packed_refs_file);
-		}
-	}
-
-	stat_validity_update(&packed_refs->validity, fileno(f));
-
-	dir = get_ref_dir(packed_refs->cache->root);
-	while (strbuf_getwholeline(&line, f, '\n') != EOF) {
-		struct object_id oid;
-		const char *refname;
-		const char *traits;
-
-		if (skip_prefix(line.buf, "# pack-refs with:", &traits)) {
-			if (strstr(traits, " fully-peeled "))
-				peeled = PEELED_FULLY;
-			else if (strstr(traits, " peeled "))
-				peeled = PEELED_TAGS;
-			/* perhaps other traits later as well */
-			continue;
-		}
-
-		refname = parse_ref_line(&line, &oid);
-		if (refname) {
-			int flag = REF_ISPACKED;
-
-			if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-				if (!refname_is_safe(refname))
-					die("packed refname is dangerous: %s", refname);
-				oidclr(&oid);
-				flag |= REF_BAD_NAME | REF_ISBROKEN;
-			}
-			last = create_ref_entry(refname, &oid, flag);
-			if (peeled == PEELED_FULLY ||
-			    (peeled == PEELED_TAGS && starts_with(refname, "refs/tags/")))
-				last->flag |= REF_KNOWS_PEELED;
-			add_ref_entry(dir, last);
-			continue;
-		}
-		if (last &&
-		    line.buf[0] == '^' &&
-		    line.len == PEELED_LINE_LENGTH &&
-		    line.buf[PEELED_LINE_LENGTH - 1] == '\n' &&
-		    !get_oid_hex(line.buf + 1, &oid)) {
-			oidcpy(&last->u.value.peeled, &oid);
-			/*
-			 * Regardless of what the file header said,
-			 * we definitely know the value of *this*
-			 * reference:
-			 */
-			last->flag |= REF_KNOWS_PEELED;
-		}
-	}
-
-	fclose(f);
-	strbuf_release(&line);
-
-	return packed_refs;
-}
-
 static void files_reflog_path(struct files_ref_store *refs,
 			      struct strbuf *sb,
 			      const char *refname)
@@ -405,77 +146,6 @@ static void files_ref_path(struct files_ref_store *refs,
 	}
 }
 
-/*
- * Check that the packed refs cache (if any) still reflects the
- * contents of the file. If not, clear the cache.
- */
-static void validate_packed_ref_cache(struct packed_ref_store *refs)
-{
-	if (refs->cache &&
-	    !stat_validity_check(&refs->cache->validity, refs->path))
-		clear_packed_ref_cache(refs);
-}
-
-/*
- * Get the packed_ref_cache for the specified packed_ref_store,
- * 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 packed_ref_store *refs)
-{
-	if (!is_lock_file_locked(&refs->lock))
-		validate_packed_ref_cache(refs);
-
-	if (!refs->cache)
-		refs->cache = read_packed_refs(refs->path);
-
-	return refs->cache;
-}
-
-static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_cache)
-{
-	return get_ref_dir(packed_ref_cache->cache->root);
-}
-
-static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)
-{
-	return get_packed_ref_dir(get_packed_ref_cache(refs));
-}
-
-/*
- * Add or overwrite a reference in the in-memory packed reference
- * cache. This may only be called while the packed-refs file is locked
- * (see lock_packed_refs()). To actually write the packed-refs file,
- * call commit_packed_refs().
- */
-static void add_packed_ref(struct packed_ref_store *refs,
-			   const char *refname, const struct object_id *oid)
-{
-	struct ref_dir *packed_refs;
-	struct ref_entry *packed_entry;
-
-	if (!is_lock_file_locked(&refs->lock))
-		die("BUG: packed refs not locked");
-
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
-		die("Reference has invalid format: '%s'", refname);
-
-	packed_refs = get_packed_refs(refs);
-	packed_entry = find_ref_entry(packed_refs, refname);
-	if (packed_entry) {
-		/* Overwrite the existing entry: */
-		oidcpy(&packed_entry->u.value.oid, oid);
-		packed_entry->flag = REF_ISPACKED;
-		oidclr(&packed_entry->u.value.peeled);
-	} else {
-		packed_entry = create_ref_entry(refname, oid, REF_ISPACKED);
-		add_ref_entry(packed_refs, packed_entry);
-	}
-}
-
 /*
  * Read the loose references from the namespace dirname into dir
  * (without recursing).  dirname must end with '/'.  dir must be the
@@ -598,35 +268,6 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
 	return refs->loose;
 }
 
-/*
- * Return the ref_entry for the given refname from the packed
- * references.  If it does not exist, return NULL.
- */
-static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,
-					const char *refname)
-{
-	return find_ref_entry(get_packed_refs(refs), refname);
-}
-
-static int packed_read_raw_ref(struct packed_ref_store *refs,
-			       const char *refname, unsigned char *sha1,
-			       struct strbuf *referent, unsigned int *type)
-{
-	struct ref_entry *entry;
-
-	*type = 0;
-
-	entry = get_packed_ref(refs, refname);
-	if (!entry) {
-		errno = ENOENT;
-		return -1;
-	}
-
-	hashcpy(sha1, entry->u.value.oid.hash);
-	*type = REF_ISPACKED;
-	return 0;
-}
-
 static int files_read_raw_ref(struct ref_store *ref_store,
 			      const char *refname, unsigned char *sha1,
 			      struct strbuf *referent, unsigned int *type)
@@ -1011,18 +652,6 @@ static int lock_raw_ref(struct files_ref_store *refs,
 	return ret;
 }
 
-static int packed_peel_ref(struct packed_ref_store *refs,
-			   const char *refname, unsigned char *sha1)
-{
-	struct ref_entry *r = get_packed_ref(refs, refname);
-
-	if (!r || peel_entry(r, 0))
-		return -1;
-
-	hashcpy(sha1, r->u.value.peeled.hash);
-	return 0;
-}
-
 static int files_peel_ref(struct ref_store *ref_store,
 			  const char *refname, unsigned char *sha1)
 {
@@ -1060,99 +689,6 @@ static int files_peel_ref(struct ref_store *ref_store,
 	return peel_object(base, sha1);
 }
 
-struct packed_ref_iterator {
-	struct ref_iterator base;
-
-	struct packed_ref_cache *cache;
-	struct ref_iterator *iter0;
-	unsigned int flags;
-};
-
-static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
-{
-	struct packed_ref_iterator *iter =
-		(struct packed_ref_iterator *)ref_iterator;
-	int ok;
-
-	while ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {
-		if (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&
-		    ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)
-			continue;
-
-		if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
-		    !ref_resolves_to_object(iter->iter0->refname,
-					    iter->iter0->oid,
-					    iter->iter0->flags))
-			continue;
-
-		iter->base.refname = iter->iter0->refname;
-		iter->base.oid = iter->iter0->oid;
-		iter->base.flags = iter->iter0->flags;
-		return ITER_OK;
-	}
-
-	iter->iter0 = NULL;
-	if (ref_iterator_abort(ref_iterator) != ITER_DONE)
-		ok = ITER_ERROR;
-
-	return ok;
-}
-
-static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,
-				   struct object_id *peeled)
-{
-	struct packed_ref_iterator *iter =
-		(struct packed_ref_iterator *)ref_iterator;
-
-	return ref_iterator_peel(iter->iter0, peeled);
-}
-
-static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)
-{
-	struct packed_ref_iterator *iter =
-		(struct packed_ref_iterator *)ref_iterator;
-	int ok = ITER_DONE;
-
-	if (iter->iter0)
-		ok = ref_iterator_abort(iter->iter0);
-
-	release_packed_ref_cache(iter->cache);
-	base_ref_iterator_free(ref_iterator);
-	return ok;
-}
-
-static struct ref_iterator_vtable packed_ref_iterator_vtable = {
-	packed_ref_iterator_advance,
-	packed_ref_iterator_peel,
-	packed_ref_iterator_abort
-};
-
-static struct ref_iterator *packed_ref_iterator_begin(
-		struct packed_ref_store *refs,
-		const char *prefix, unsigned int flags)
-{
-	struct packed_ref_iterator *iter;
-	struct ref_iterator *ref_iterator;
-
-	iter = xcalloc(1, sizeof(*iter));
-	ref_iterator = &iter->base;
-	base_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);
-
-	/*
-	 * Note that get_packed_ref_cache() internally checks whether
-	 * the packed-ref cache is up to date with what is on disk,
-	 * and re-reads it if not.
-	 */
-
-	iter->cache = get_packed_ref_cache(refs);
-	acquire_packed_ref_cache(iter->cache);
-	iter->iter0 = cache_ref_iterator_begin(iter->cache->cache, prefix, 0);
-
-	iter->flags = flags;
-
-	return ref_iterator;
-}
-
 struct files_ref_iterator {
 	struct ref_iterator base;
 
@@ -1422,124 +958,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
 	return lock;
 }
 
-/*
- * Write an entry to the packed-refs file for the specified refname.
- * If peeled is non-NULL, write it as the entry's peeled value.
- */
-static void write_packed_entry(FILE *fh, const char *refname,
-			       const unsigned char *sha1,
-			       const unsigned char *peeled)
-{
-	fprintf_or_die(fh, "%s %s\n", sha1_to_hex(sha1), refname);
-	if (peeled)
-		fprintf_or_die(fh, "^%s\n", sha1_to_hex(peeled));
-}
-
-/*
- * Lock the packed-refs file for writing. Flags is passed to
- * hold_lock_file_for_update(). Return 0 on success. On errors, set
- * errno appropriately and return a nonzero value.
- */
-static int lock_packed_refs(struct packed_ref_store *refs, int flags)
-{
-	static int timeout_configured = 0;
-	static int timeout_value = 1000;
-	struct packed_ref_cache *packed_ref_cache;
-
-	packed_assert_main_repository(refs, "lock_packed_refs");
-
-	if (!timeout_configured) {
-		git_config_get_int("core.packedrefstimeout", &timeout_value);
-		timeout_configured = 1;
-	}
-
-	if (hold_lock_file_for_update_timeout(
-			    &refs->lock,
-			    refs->path,
-			    flags, timeout_value) < 0)
-		return -1;
-
-	/*
-	 * Now that we hold the `packed-refs` lock, make sure that our
-	 * cache matches the current version of the file. Normally
-	 * `get_packed_ref_cache()` does that for us, but that
-	 * function assumes that when the file is locked, any existing
-	 * cache is still valid. We've just locked the file, but it
-	 * might have changed the moment *before* we locked it.
-	 */
-	validate_packed_ref_cache(refs);
-
-	packed_ref_cache = get_packed_ref_cache(refs);
-	/* Increment the reference count to prevent it from being freed: */
-	acquire_packed_ref_cache(packed_ref_cache);
-	return 0;
-}
-
-/*
- * Write the current version of the packed refs cache from memory to
- * disk. The packed-refs file must already be locked for writing (see
- * lock_packed_refs()). Return zero on success. On errors, set errno
- * and return a nonzero value
- */
-static int commit_packed_refs(struct packed_ref_store *refs)
-{
-	struct packed_ref_cache *packed_ref_cache =
-		get_packed_ref_cache(refs);
-	int ok, error = 0;
-	int save_errno = 0;
-	FILE *out;
-	struct ref_iterator *iter;
-
-	packed_assert_main_repository(refs, "commit_packed_refs");
-
-	if (!is_lock_file_locked(&refs->lock))
-		die("BUG: packed-refs not locked");
-
-	out = fdopen_lock_file(&refs->lock, "w");
-	if (!out)
-		die_errno("unable to fdopen packed-refs descriptor");
-
-	fprintf_or_die(out, "%s", PACKED_REFS_HEADER);
-
-	iter = cache_ref_iterator_begin(packed_ref_cache->cache, NULL, 0);
-	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
-		struct object_id peeled;
-		int peel_error = ref_iterator_peel(iter, &peeled);
-
-		write_packed_entry(out, iter->refname, iter->oid->hash,
-				   peel_error ? NULL : peeled.hash);
-	}
-
-	if (ok != ITER_DONE)
-		die("error while iterating over references");
-
-	if (commit_lock_file(&refs->lock)) {
-		save_errno = errno;
-		error = -1;
-	}
-	release_packed_ref_cache(packed_ref_cache);
-	errno = save_errno;
-	return error;
-}
-
-/*
- * Rollback the lockfile for the packed-refs file, and discard the
- * in-memory packed reference cache.  (The packed-refs file will be
- * read anew if it is needed again after this function is called.)
- */
-static void rollback_packed_refs(struct packed_ref_store *refs)
-{
-	struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
-
-	packed_assert_main_repository(refs, "rollback_packed_refs");
-
-	if (!is_lock_file_locked(&refs->lock))
-		die("BUG: packed-refs not locked");
-	rollback_lock_file(&refs->lock);
-	release_packed_ref_cache(packed_ref_cache);
-	clear_packed_ref_cache(refs);
-}
-
 struct ref_to_prune {
 	struct ref_to_prune *next;
 	unsigned char sha1[20];
@@ -1705,62 +1123,6 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	return 0;
 }
 
-/*
- * Rewrite the packed-refs file, omitting any refs listed in
- * 'refnames'. On error, leave packed-refs unchanged, write an error
- * message to 'err', and return a nonzero value.
- *
- * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
- */
-static int repack_without_refs(struct packed_ref_store *refs,
-			       struct string_list *refnames, struct strbuf *err)
-{
-	struct ref_dir *packed;
-	struct string_list_item *refname;
-	int ret, needs_repacking = 0, removed = 0;
-
-	packed_assert_main_repository(refs, "repack_without_refs");
-	assert(err);
-
-	/* Look for a packed ref */
-	for_each_string_list_item(refname, refnames) {
-		if (get_packed_ref(refs, refname->string)) {
-			needs_repacking = 1;
-			break;
-		}
-	}
-
-	/* Avoid locking if we have nothing to do */
-	if (!needs_repacking)
-		return 0; /* no refname exists in packed refs */
-
-	if (lock_packed_refs(refs, 0)) {
-		unable_to_lock_message(refs->path, errno, err);
-		return -1;
-	}
-	packed = get_packed_refs(refs);
-
-	/* Remove refnames from the cache */
-	for_each_string_list_item(refname, refnames)
-		if (remove_entry_from_dir(packed, refname->string) != -1)
-			removed = 1;
-	if (!removed) {
-		/*
-		 * All packed entries disappeared while we were
-		 * acquiring the lock.
-		 */
-		rollback_packed_refs(refs);
-		return 0;
-	}
-
-	/* Write what remains */
-	ret = commit_packed_refs(refs);
-	if (ret)
-		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
-			    strerror(errno));
-	return ret;
-}
-
 static int files_delete_refs(struct ref_store *ref_store, const char *msg,
 			     struct string_list *refnames, unsigned int flags)
 {
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
new file mode 100644
index 0000000000..6fa988c953
--- /dev/null
+++ b/refs/packed-backend.c
@@ -0,0 +1,624 @@
+#include "../cache.h"
+#include "../refs.h"
+#include "refs-internal.h"
+#include "ref-cache.h"
+#include "packed-backend.h"
+#include "../iterator.h"
+#include "../lockfile.h"
+
+struct packed_ref_cache {
+	struct ref_cache *cache;
+
+	/*
+	 * Count of references to the data structure in this instance,
+	 * including the pointer from files_ref_store::packed if any.
+	 * The data will not be freed as long as the reference count
+	 * is nonzero.
+	 */
+	unsigned int referrers;
+
+	/* The metadata from when this packed-refs cache was read */
+	struct stat_validity validity;
+};
+
+/*
+ * Increment the reference count of *packed_refs.
+ */
+static void acquire_packed_ref_cache(struct packed_ref_cache *packed_refs)
+{
+	packed_refs->referrers++;
+}
+
+/*
+ * Decrease the reference count of *packed_refs.  If it goes to zero,
+ * free *packed_refs and return true; otherwise return false.
+ */
+static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)
+{
+	if (!--packed_refs->referrers) {
+		free_ref_cache(packed_refs->cache);
+		stat_validity_clear(&packed_refs->validity);
+		free(packed_refs);
+		return 1;
+	} else {
+		return 0;
+	}
+}
+
+/*
+ * A container for `packed-refs`-related data. It is not (yet) a
+ * `ref_store`.
+ */
+struct packed_ref_store {
+	unsigned int store_flags;
+
+	/* The path of the "packed-refs" file: */
+	char *path;
+
+	/*
+	 * A cache of the values read from the `packed-refs` file, if
+	 * it might still be current; otherwise, NULL.
+	 */
+	struct packed_ref_cache *cache;
+
+	/*
+	 * Lock used for the "packed-refs" file. Note that this (and
+	 * thus the enclosing `packed_ref_store`) must not be freed.
+	 */
+	struct lock_file lock;
+};
+
+struct packed_ref_store *packed_ref_store_create(
+		const char *path, unsigned int store_flags)
+{
+	struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
+
+	refs->store_flags = store_flags;
+	refs->path = xstrdup(path);
+	return refs;
+}
+
+/*
+ * Die if refs is not the main ref store. caller is used in any
+ * necessary error messages.
+ */
+static void packed_assert_main_repository(struct packed_ref_store *refs,
+					  const char *caller)
+{
+	if (refs->store_flags & REF_STORE_MAIN)
+		return;
+
+	die("BUG: operation %s only allowed for main ref store", caller);
+}
+
+static void clear_packed_ref_cache(struct packed_ref_store *refs)
+{
+	if (refs->cache) {
+		struct packed_ref_cache *cache = refs->cache;
+
+		if (is_lock_file_locked(&refs->lock))
+			die("BUG: packed-ref cache cleared while locked");
+		refs->cache = NULL;
+		release_packed_ref_cache(cache);
+	}
+}
+
+/* The length of a peeled reference line in packed-refs, including EOL: */
+#define PEELED_LINE_LENGTH 42
+
+/*
+ * Parse one line from a packed-refs file.  Write the SHA1 to sha1.
+ * Return a pointer to the refname within the line (null-terminated),
+ * or NULL if there was a problem.
+ */
+static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)
+{
+	const char *ref;
+
+	if (parse_oid_hex(line->buf, oid, &ref) < 0)
+		return NULL;
+	if (!isspace(*ref++))
+		return NULL;
+
+	if (isspace(*ref))
+		return NULL;
+
+	if (line->buf[line->len - 1] != '\n')
+		return NULL;
+	line->buf[--line->len] = 0;
+
+	return ref;
+}
+
+/*
+ * 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:
+ *
+ *   No traits:
+ *
+ *      Probably no references are peeled. But if the file contains a
+ *      peeled value for a reference, we will use it.
+ *
+ *   peeled:
+ *
+ *      References under "refs/tags/", if they *can* be peeled, *are*
+ *      peeled in this file. References outside of "refs/tags/" are
+ *      probably not peeled even if they could have been, but if we find
+ *      a peeled value for such a reference we will use it.
+ *
+ *   fully-peeled:
+ *
+ *      All references in the file that can be peeled are peeled.
+ *      Inversely (and this is more important), any references in the
+ *      file for which no peeled value is recorded is not peelable. This
+ *      trait should typically be written alongside "peeled" for
+ *      compatibility with older clients, but we do not require it
+ *      (i.e., "peeled" is a no-op if "fully-peeled" is set).
+ */
+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) {
+		if (errno == ENOENT) {
+			/*
+			 * This is OK; it just means that no
+			 * "packed-refs" file has been written yet,
+			 * which is equivalent to it being empty.
+			 */
+			return packed_refs;
+		} else {
+			die_errno("couldn't read %s", packed_refs_file);
+		}
+	}
+
+	stat_validity_update(&packed_refs->validity, fileno(f));
+
+	dir = get_ref_dir(packed_refs->cache->root);
+	while (strbuf_getwholeline(&line, f, '\n') != EOF) {
+		struct object_id oid;
+		const char *refname;
+		const char *traits;
+
+		if (skip_prefix(line.buf, "# pack-refs with:", &traits)) {
+			if (strstr(traits, " fully-peeled "))
+				peeled = PEELED_FULLY;
+			else if (strstr(traits, " peeled "))
+				peeled = PEELED_TAGS;
+			/* perhaps other traits later as well */
+			continue;
+		}
+
+		refname = parse_ref_line(&line, &oid);
+		if (refname) {
+			int flag = REF_ISPACKED;
+
+			if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+				if (!refname_is_safe(refname))
+					die("packed refname is dangerous: %s", refname);
+				oidclr(&oid);
+				flag |= REF_BAD_NAME | REF_ISBROKEN;
+			}
+			last = create_ref_entry(refname, &oid, flag);
+			if (peeled == PEELED_FULLY ||
+			    (peeled == PEELED_TAGS && starts_with(refname, "refs/tags/")))
+				last->flag |= REF_KNOWS_PEELED;
+			add_ref_entry(dir, last);
+			continue;
+		}
+		if (last &&
+		    line.buf[0] == '^' &&
+		    line.len == PEELED_LINE_LENGTH &&
+		    line.buf[PEELED_LINE_LENGTH - 1] == '\n' &&
+		    !get_oid_hex(line.buf + 1, &oid)) {
+			oidcpy(&last->u.value.peeled, &oid);
+			/*
+			 * Regardless of what the file header said,
+			 * we definitely know the value of *this*
+			 * reference:
+			 */
+			last->flag |= REF_KNOWS_PEELED;
+		}
+	}
+
+	fclose(f);
+	strbuf_release(&line);
+
+	return packed_refs;
+}
+
+/*
+ * Check that the packed refs cache (if any) still reflects the
+ * contents of the file. If not, clear the cache.
+ */
+static void validate_packed_ref_cache(struct packed_ref_store *refs)
+{
+	if (refs->cache &&
+	    !stat_validity_check(&refs->cache->validity, refs->path))
+		clear_packed_ref_cache(refs);
+}
+
+/*
+ * Get the packed_ref_cache for the specified packed_ref_store,
+ * 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 packed_ref_store *refs)
+{
+	if (!is_lock_file_locked(&refs->lock))
+		validate_packed_ref_cache(refs);
+
+	if (!refs->cache)
+		refs->cache = read_packed_refs(refs->path);
+
+	return refs->cache;
+}
+
+static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_cache)
+{
+	return get_ref_dir(packed_ref_cache->cache->root);
+}
+
+static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)
+{
+	return get_packed_ref_dir(get_packed_ref_cache(refs));
+}
+
+/*
+ * Add or overwrite a reference in the in-memory packed reference
+ * cache. This may only be called while the packed-refs file is locked
+ * (see lock_packed_refs()). To actually write the packed-refs file,
+ * call commit_packed_refs().
+ */
+void add_packed_ref(struct packed_ref_store *refs,
+		    const char *refname, const struct object_id *oid)
+{
+	struct ref_dir *packed_refs;
+	struct ref_entry *packed_entry;
+
+	if (!is_lock_file_locked(&refs->lock))
+		die("BUG: packed refs not locked");
+
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+		die("Reference has invalid format: '%s'", refname);
+
+	packed_refs = get_packed_refs(refs);
+	packed_entry = find_ref_entry(packed_refs, refname);
+	if (packed_entry) {
+		/* Overwrite the existing entry: */
+		oidcpy(&packed_entry->u.value.oid, oid);
+		packed_entry->flag = REF_ISPACKED;
+		oidclr(&packed_entry->u.value.peeled);
+	} else {
+		packed_entry = create_ref_entry(refname, oid, REF_ISPACKED);
+		add_ref_entry(packed_refs, packed_entry);
+	}
+}
+
+/*
+ * Return the ref_entry for the given refname from the packed
+ * references.  If it does not exist, return NULL.
+ */
+static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,
+					const char *refname)
+{
+	return find_ref_entry(get_packed_refs(refs), refname);
+}
+
+int packed_read_raw_ref(struct packed_ref_store *refs,
+			const char *refname, unsigned char *sha1,
+			struct strbuf *referent, unsigned int *type)
+{
+	struct ref_entry *entry;
+
+	*type = 0;
+
+	entry = get_packed_ref(refs, refname);
+	if (!entry) {
+		errno = ENOENT;
+		return -1;
+	}
+
+	hashcpy(sha1, entry->u.value.oid.hash);
+	*type = REF_ISPACKED;
+	return 0;
+}
+
+int packed_peel_ref(struct packed_ref_store *refs,
+		    const char *refname, unsigned char *sha1)
+{
+	struct ref_entry *r = get_packed_ref(refs, refname);
+
+	if (!r || peel_entry(r, 0))
+		return -1;
+
+	hashcpy(sha1, r->u.value.peeled.hash);
+	return 0;
+}
+
+struct packed_ref_iterator {
+	struct ref_iterator base;
+
+	struct packed_ref_cache *cache;
+	struct ref_iterator *iter0;
+	unsigned int flags;
+};
+
+static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+	struct packed_ref_iterator *iter =
+		(struct packed_ref_iterator *)ref_iterator;
+	int ok;
+
+	while ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {
+		if (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&
+		    ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)
+			continue;
+
+		if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
+		    !ref_resolves_to_object(iter->iter0->refname,
+					    iter->iter0->oid,
+					    iter->iter0->flags))
+			continue;
+
+		iter->base.refname = iter->iter0->refname;
+		iter->base.oid = iter->iter0->oid;
+		iter->base.flags = iter->iter0->flags;
+		return ITER_OK;
+	}
+
+	iter->iter0 = NULL;
+	if (ref_iterator_abort(ref_iterator) != ITER_DONE)
+		ok = ITER_ERROR;
+
+	return ok;
+}
+
+static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,
+				   struct object_id *peeled)
+{
+	struct packed_ref_iterator *iter =
+		(struct packed_ref_iterator *)ref_iterator;
+
+	return ref_iterator_peel(iter->iter0, peeled);
+}
+
+static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)
+{
+	struct packed_ref_iterator *iter =
+		(struct packed_ref_iterator *)ref_iterator;
+	int ok = ITER_DONE;
+
+	if (iter->iter0)
+		ok = ref_iterator_abort(iter->iter0);
+
+	release_packed_ref_cache(iter->cache);
+	base_ref_iterator_free(ref_iterator);
+	return ok;
+}
+
+static struct ref_iterator_vtable packed_ref_iterator_vtable = {
+	packed_ref_iterator_advance,
+	packed_ref_iterator_peel,
+	packed_ref_iterator_abort
+};
+
+struct ref_iterator *packed_ref_iterator_begin(
+		struct packed_ref_store *refs,
+		const char *prefix, unsigned int flags)
+{
+	struct packed_ref_iterator *iter;
+	struct ref_iterator *ref_iterator;
+
+	iter = xcalloc(1, sizeof(*iter));
+	ref_iterator = &iter->base;
+	base_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);
+
+	/*
+	 * Note that get_packed_ref_cache() internally checks whether
+	 * the packed-ref cache is up to date with what is on disk,
+	 * and re-reads it if not.
+	 */
+
+	iter->cache = get_packed_ref_cache(refs);
+	acquire_packed_ref_cache(iter->cache);
+	iter->iter0 = cache_ref_iterator_begin(iter->cache->cache, prefix, 0);
+
+	iter->flags = flags;
+
+	return ref_iterator;
+}
+
+/*
+ * Write an entry to the packed-refs file for the specified refname.
+ * If peeled is non-NULL, write it as the entry's peeled value.
+ */
+static void write_packed_entry(FILE *fh, const char *refname,
+			       const unsigned char *sha1,
+			       const unsigned char *peeled)
+{
+	fprintf_or_die(fh, "%s %s\n", sha1_to_hex(sha1), refname);
+	if (peeled)
+		fprintf_or_die(fh, "^%s\n", sha1_to_hex(peeled));
+}
+
+int lock_packed_refs(struct packed_ref_store *refs, int flags)
+{
+	static int timeout_configured = 0;
+	static int timeout_value = 1000;
+	struct packed_ref_cache *packed_ref_cache;
+
+	packed_assert_main_repository(refs, "lock_packed_refs");
+
+	if (!timeout_configured) {
+		git_config_get_int("core.packedrefstimeout", &timeout_value);
+		timeout_configured = 1;
+	}
+
+	if (hold_lock_file_for_update_timeout(
+			    &refs->lock,
+			    refs->path,
+			    flags, timeout_value) < 0)
+		return -1;
+
+	/*
+	 * Now that we hold the `packed-refs` lock, make sure that our
+	 * cache matches the current version of the file. Normally
+	 * `get_packed_ref_cache()` does that for us, but that
+	 * function assumes that when the file is locked, any existing
+	 * cache is still valid. We've just locked the file, but it
+	 * might have changed the moment *before* we locked it.
+	 */
+	validate_packed_ref_cache(refs);
+
+	packed_ref_cache = get_packed_ref_cache(refs);
+	/* Increment the reference count to prevent it from being freed: */
+	acquire_packed_ref_cache(packed_ref_cache);
+	return 0;
+}
+
+/*
+ * The packed-refs header line that we write out.  Perhaps other
+ * traits will be added later.  The trailing space is required.
+ */
+static const char PACKED_REFS_HEADER[] =
+	"# pack-refs with: peeled fully-peeled \n";
+
+/*
+ * Write the current version of the packed refs cache from memory to
+ * disk. The packed-refs file must already be locked for writing (see
+ * lock_packed_refs()). Return zero on success. On errors, set errno
+ * and return a nonzero value.
+ */
+int commit_packed_refs(struct packed_ref_store *refs)
+{
+	struct packed_ref_cache *packed_ref_cache =
+		get_packed_ref_cache(refs);
+	int ok, error = 0;
+	int save_errno = 0;
+	FILE *out;
+	struct ref_iterator *iter;
+
+	packed_assert_main_repository(refs, "commit_packed_refs");
+
+	if (!is_lock_file_locked(&refs->lock))
+		die("BUG: packed-refs not locked");
+
+	out = fdopen_lock_file(&refs->lock, "w");
+	if (!out)
+		die_errno("unable to fdopen packed-refs descriptor");
+
+	fprintf_or_die(out, "%s", PACKED_REFS_HEADER);
+
+	iter = cache_ref_iterator_begin(packed_ref_cache->cache, NULL, 0);
+	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
+		struct object_id peeled;
+		int peel_error = ref_iterator_peel(iter, &peeled);
+
+		write_packed_entry(out, iter->refname, iter->oid->hash,
+				   peel_error ? NULL : peeled.hash);
+	}
+
+	if (ok != ITER_DONE)
+		die("error while iterating over references");
+
+	if (commit_lock_file(&refs->lock)) {
+		save_errno = errno;
+		error = -1;
+	}
+	release_packed_ref_cache(packed_ref_cache);
+	errno = save_errno;
+	return error;
+}
+
+/*
+ * Rollback the lockfile for the packed-refs file, and discard the
+ * in-memory packed reference cache.  (The packed-refs file will be
+ * read anew if it is needed again after this function is called.)
+ */
+static void rollback_packed_refs(struct packed_ref_store *refs)
+{
+	struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
+
+	packed_assert_main_repository(refs, "rollback_packed_refs");
+
+	if (!is_lock_file_locked(&refs->lock))
+		die("BUG: packed-refs not locked");
+	rollback_lock_file(&refs->lock);
+	release_packed_ref_cache(packed_ref_cache);
+	clear_packed_ref_cache(refs);
+}
+
+/*
+ * Rewrite the packed-refs file, omitting any refs listed in
+ * 'refnames'. On error, leave packed-refs unchanged, write an error
+ * message to 'err', and return a nonzero value.
+ *
+ * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
+ */
+int repack_without_refs(struct packed_ref_store *refs,
+			struct string_list *refnames, struct strbuf *err)
+{
+	struct ref_dir *packed;
+	struct string_list_item *refname;
+	int ret, needs_repacking = 0, removed = 0;
+
+	packed_assert_main_repository(refs, "repack_without_refs");
+	assert(err);
+
+	/* Look for a packed ref */
+	for_each_string_list_item(refname, refnames) {
+		if (get_packed_ref(refs, refname->string)) {
+			needs_repacking = 1;
+			break;
+		}
+	}
+
+	/* Avoid locking if we have nothing to do */
+	if (!needs_repacking)
+		return 0; /* no refname exists in packed refs */
+
+	if (lock_packed_refs(refs, 0)) {
+		unable_to_lock_message(refs->path, errno, err);
+		return -1;
+	}
+	packed = get_packed_refs(refs);
+
+	/* Remove refnames from the cache */
+	for_each_string_list_item(refname, refnames)
+		if (remove_entry_from_dir(packed, refname->string) != -1)
+			removed = 1;
+	if (!removed) {
+		/*
+		 * All packed entries disappeared while we were
+		 * acquiring the lock.
+		 */
+		rollback_packed_refs(refs);
+		return 0;
+	}
+
+	/* Write what remains */
+	ret = commit_packed_refs(refs);
+	if (ret)
+		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
+			    strerror(errno));
+	return ret;
+}
+
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
new file mode 100644
index 0000000000..22e8817ac4
--- /dev/null
+++ b/refs/packed-backend.h
@@ -0,0 +1,33 @@
+#ifndef REFS_PACKED_BACKEND_H
+#define REFS_PACKED_BACKEND_H
+
+struct packed_ref_store *packed_ref_store_create(
+		const char *path, unsigned int store_flags);
+
+int packed_read_raw_ref(struct packed_ref_store *refs,
+			const char *refname, unsigned char *sha1,
+			struct strbuf *referent, unsigned int *type);
+
+int packed_peel_ref(struct packed_ref_store *refs,
+		    const char *refname, unsigned char *sha1);
+
+struct ref_iterator *packed_ref_iterator_begin(
+		struct packed_ref_store *refs,
+		const char *prefix, unsigned int flags);
+
+/*
+ * Lock the packed-refs file for writing. Flags is passed to
+ * hold_lock_file_for_update(). Return 0 on success. On errors, set
+ * errno appropriately and return a nonzero value.
+ */
+int lock_packed_refs(struct packed_ref_store *refs, int flags);
+
+void add_packed_ref(struct packed_ref_store *refs,
+		    const char *refname, const struct object_id *oid);
+
+int commit_packed_refs(struct packed_ref_store *refs);
+
+int repack_without_refs(struct packed_ref_store *refs,
+			struct string_list *refnames, struct strbuf *err);
+
+#endif /* REFS_PACKED_BACKEND_H */
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 192f9f85c9..6f8f9f5619 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -77,6 +77,15 @@
  */
 int refname_is_safe(const char *refname);
 
+/*
+ * Helper function: return true if refname, which has the specified
+ * oid and flags, can be resolved to an object in the database. If the
+ * referred-to object does not exist, emit a warning and return false.
+ */
+int ref_resolves_to_object(const char *refname,
+			   const struct object_id *oid,
+			   unsigned int flags);
+
 enum peel_status {
 	/* object was peeled successfully: */
 	PEEL_PEELED = 0,
-- 
2.11.0
Stefan Beller· Jun 16, 2017, 05:48 UTC · re: Michael Haggerty · lore

Re: [PATCH 18/28] packed-backend: new module for handling packed references

On Thu, Jun 15, 2017 at 7:47 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 7 quoted lines
> Now that the interface between `files_ref_store` and
> `packed_ref_store` is relatively narrow, move the latter into a new
> module, "refs/packed-backend.h" and "refs/packed-backend.c". It still
> doesn't quite implement the `ref_store` interface, but it will soon.
>
> This commit moves code around and adjusts its visibility, but doesn't
> change anything.

I did not look into it in detail (as the origin/sb/diff-color-move series ought to help here, but I am in the mid of rewriting that again to have a better design; also on my laptop, such that I'd have install that series).

I think this commit represents another common case of code movement, where you would not just move around code, but also add/remove some keywords (e.g. the static word in C, or a class name changes in Java/C++)

Maybe we would want to not just color up moved blocks but show the adjacant lines of moved blocks in a word diff (such that the class name change would be highlighted) in a comment or such.

Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 24/28] packed_refs_unlock(), packed_refs_is_locked(): new functions

Add two new public functions, `packed_refs_unlock()` and `packed_refs_is_locked()`, with which callers can manage and query the `packed-refs` lock externally.

Call `packed_refs_unlock()` from `commit_packed_refs()` and `rollback_packed_refs()`.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/packed-backend.c | 31 +++++++++++++++++++++++++------
 refs/packed-backend.h |  3 +++
 2 files changed, 28 insertions(+), 6 deletions(-)
Show changes to 2 files +28 −6

refs/packed-backend.c, refs/packed-backend.h

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 24451343b8..ff6326ddb8 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -563,6 +563,29 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
 	return 0;
 }
 
+void packed_refs_unlock(struct ref_store *ref_store)
+{
+	struct packed_ref_store *refs = packed_downcast(
+			ref_store,
+			REF_STORE_READ | REF_STORE_WRITE,
+			"packed_refs_unlock");
+
+	if (!is_lock_file_locked(&refs->lock))
+		die("BUG: packed_refs_unlock() called when not locked");
+	rollback_lock_file(&refs->lock);
+	release_packed_ref_cache(refs->cache);
+}
+
+int packed_refs_is_locked(struct ref_store *ref_store)
+{
+	struct packed_ref_store *refs = packed_downcast(
+			ref_store,
+			REF_STORE_READ | REF_STORE_WRITE,
+			"packed_refs_is_locked");
+
+	return is_lock_file_locked(&refs->lock);
+}
+
 /*
  * The packed-refs header line that we write out.  Perhaps other
  * traits will be added later.  The trailing space is required.
@@ -649,8 +672,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 	delete_tempfile(&refs->tempfile);
 
 out:
-	rollback_lock_file(&refs->lock);
-	release_packed_ref_cache(packed_ref_cache);
+	packed_refs_unlock(ref_store);
 	return ret;
 }
 
@@ -661,14 +683,11 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
  */
 static void rollback_packed_refs(struct packed_ref_store *refs)
 {
-	struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
-
 	packed_assert_main_repository(refs, "rollback_packed_refs");
 
 	if (!is_lock_file_locked(&refs->lock))
 		die("BUG: packed-refs not locked");
-	rollback_lock_file(&refs->lock);
-	release_packed_ref_cache(packed_ref_cache);
+	packed_refs_unlock(&refs->base);
 	clear_packed_ref_cache(refs);
 }
 
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 210e3f35ce..03b7c1de95 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -11,6 +11,9 @@ struct ref_store *packed_ref_store_create(const char *path,
  */
 int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err);
 
+void packed_refs_unlock(struct ref_store *ref_store);
+int packed_refs_is_locked(struct ref_store *ref_store);
+
 void add_packed_ref(struct ref_store *ref_store,
 		    const char *refname, const struct object_id *oid);
 
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 26/28] commit_packed_refs(): remove call to `packed_refs_unlock()`

Instead, change the callers of `commit_packed_refs()` to call `packed_refs_unlock()`.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c  |  2 ++
 refs/packed-backend.c | 18 ++++++++----------
 2 files changed, 10 insertions(+), 10 deletions(-)
Show changes to 2 files +10 −10

refs/files-backend.c, refs/packed-backend.c

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 802ed9e2e9..09dad2806e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1119,6 +1119,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 
 	if (commit_packed_refs(refs->packed_ref_store, &err))
 		die("unable to overwrite old ref-pack file: %s", err.buf);
+	packed_refs_unlock(refs->packed_ref_store);
 
 	prune_refs(refs, refs_to_prune);
 	strbuf_release(&err);
@@ -2687,6 +2688,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 	}
 
 cleanup:
+	packed_refs_unlock(refs->packed_ref_store);
 	transaction->state = REF_TRANSACTION_CLOSED;
 	string_list_clear(&affected_refnames, 0);
 	return ret;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 1732e3aad4..54b48d1f02 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -606,7 +606,6 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 	struct packed_ref_cache *packed_ref_cache =
 		get_packed_ref_cache(refs);
 	int ok;
-	int ret = -1;
 	struct strbuf sb = STRBUF_INIT;
 	FILE *out;
 	struct ref_iterator *iter;
@@ -619,7 +618,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 		strbuf_addf(err, "unable to create file %s: %s",
 			    sb.buf, strerror(errno));
 		strbuf_release(&sb);
-		goto out;
+		return -1;
 	}
 	strbuf_release(&sb);
 
@@ -660,18 +659,14 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 	if (rename_tempfile(&refs->tempfile, refs->path)) {
 		strbuf_addf(err, "error replacing %s: %s",
 			    refs->path, strerror(errno));
-		goto out;
+		return -1;
 	}
 
-	ret = 0;
-	goto out;
+	return 0;
 
 error:
 	delete_tempfile(&refs->tempfile);
-
-out:
-	packed_refs_unlock(ref_store);
-	return ret;
+	return -1;
 }
 
 /*
@@ -705,6 +700,7 @@ int repack_without_refs(struct ref_store *ref_store,
 	struct ref_dir *packed;
 	struct string_list_item *refname;
 	int needs_repacking = 0, removed = 0;
+	int ret;
 
 	packed_assert_main_repository(refs, "repack_without_refs");
 	assert(err);
@@ -740,7 +736,9 @@ int repack_without_refs(struct ref_store *ref_store,
 	}
 
 	/* Write what remains */
-	return commit_packed_refs(&refs->base, err);
+	ret = commit_packed_refs(&refs->base, err);
+	packed_refs_unlock(ref_store);
+	return ret;
 }
 
 static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 28/28] read_packed_refs(): die if `packed-refs` contains bogus data

The old code ignored any lines that it didn't understand. This is dangerous. Instead, `die()` if the `packed-refs` file contains any lines that we don't know how to handle.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/packed-backend.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to refs/packed-backend.c +3 −3
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 721afd066a..311fd014ce 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -253,9 +253,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)
 			    (peeled == PEELED_TAGS && starts_with(refname, "refs/tags/")))
 				last->flag |= REF_KNOWS_PEELED;
 			add_ref_entry(dir, last);
-			continue;
-		}
-		if (last &&
+		} else if (last &&
 		    line.buf[0] == '^' &&
 		    line.len == PEELED_LINE_LENGTH &&
 		    line.buf[PEELED_LINE_LENGTH - 1] == '\n' &&
@@ -267,6 +265,8 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)
 			 * reference:
 			 */
 			last->flag |= REF_KNOWS_PEELED;
+		} else {
+			die("unexpected line in %s: %s", packed_refs_file, line.buf);
 		}
 	}
 
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 23/28] packed_refs_lock(): report errors via a `struct strbuf *err`

That way the callers don't have to come up with error messages themselves.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c  |  6 ++----
 refs/packed-backend.c | 17 +++++++++++------
 refs/packed-backend.h |  6 +++---
 3 files changed, 16 insertions(+), 13 deletions(-)
Show changes to 3 files +16 −13

refs/files-backend.c, refs/packed-backend.c, refs/packed-backend.h

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3fc2254e33..802ed9e2e9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1084,7 +1084,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	struct ref_to_prune *refs_to_prune = NULL;
 	struct strbuf err = STRBUF_INIT;
 
-	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
+	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
 
 	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -2667,9 +2667,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 		}
 	}
 
-	if (packed_refs_lock(refs->packed_ref_store, 0)) {
-		strbuf_addf(err, "unable to lock packed-refs file: %s",
-			    strerror(errno));
+	if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index eb9da04576..24451343b8 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -515,7 +515,7 @@ static int write_packed_entry(FILE *fh, const char *refname,
 	return 0;
 }
 
-int packed_refs_lock(struct ref_store *ref_store, int flags)
+int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
 {
 	struct packed_ref_store *refs =
 	        packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
@@ -537,9 +537,15 @@ int packed_refs_lock(struct ref_store *ref_store, int flags)
 	if (hold_lock_file_for_update_timeout(
 			    &refs->lock,
 			    refs->path,
-			    flags, timeout_value) < 0 ||
-	    close_lock_file(&refs->lock))
+			    flags, timeout_value) < 0) {
+		unable_to_lock_message(refs->path, errno, err);
+		return -1;
+	}
+
+	if (close_lock_file(&refs->lock)) {
+		strbuf_addf(err, "unable to close %s: %s", refs->path, strerror(errno));
 		return -1;
+	}
 
 	/*
 	 * Now that we hold the `packed-refs` lock, make sure that our
@@ -698,10 +704,9 @@ int repack_without_refs(struct ref_store *ref_store,
 	if (!needs_repacking)
 		return 0; /* no refname exists in packed refs */
 
-	if (packed_refs_lock(&refs->base, 0)) {
-		unable_to_lock_message(refs->path, errno, err);
+	if (packed_refs_lock(&refs->base, 0, err))
 		return -1;
-	}
+
 	packed = get_packed_refs(refs);
 
 	/* Remove refnames from the cache */
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index dbc00d3396..210e3f35ce 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -6,10 +6,10 @@ struct ref_store *packed_ref_store_create(const char *path,
 
 /*
  * Lock the packed-refs file for writing. Flags is passed to
- * hold_lock_file_for_update(). Return 0 on success. On errors, set
- * errno appropriately and return a nonzero value.
+ * hold_lock_file_for_update(). Return 0 on success. On errors, write
+ * an error message to `err` and return a nonzero value.
  */
-int packed_refs_lock(struct ref_store *ref_store, int flags);
+int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err);
 
 void add_packed_ref(struct ref_store *ref_store,
 		    const char *refname, const struct object_id *oid);
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 27/28] repack_without_refs(): don't lock or unlock the packed refs

Change `repack_without_refs()` to expect the packed-refs lock to be held already, and not to release the lock before returning. Change the callers to deal with lock management.

This change makes it possible for callers to hold the packed-refs lock for a longer span of time, a possibility that will eventually make it possible to fix some longstanding races.

The only semantic change here is that `repack_without_refs()` used to forgot to release the lock in the `if (!removed)` exit path. That omission is now fixed.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c  | 47 +++++++++++++++++++++++++++++++----------------
 refs/packed-backend.c | 32 ++++++++------------------------
 2 files changed, 39 insertions(+), 40 deletions(-)
Show changes to 2 files +39 −40

refs/files-backend.c, refs/packed-backend.c

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 09dad2806e..972dd5c0c9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1137,24 +1137,16 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
 	if (!refnames->nr)
 		return 0;
 
-	result = repack_without_refs(refs->packed_ref_store, refnames, &err);
-	if (result) {
-		/*
-		 * If we failed to rewrite the packed-refs file, then
-		 * it is unsafe to try to remove loose refs, because
-		 * doing so might expose an obsolete packed value for
-		 * a reference that might even point at an object that
-		 * has been garbage collected.
-		 */
-		if (refnames->nr == 1)
-			error(_("could not delete reference %s: %s"),
-			      refnames->items[0].string, err.buf);
-		else
-			error(_("could not delete references: %s"), err.buf);
+	if (packed_refs_lock(refs->packed_ref_store, 0, &err))
+		goto error;
 
-		goto out;
+	if (repack_without_refs(refs->packed_ref_store, refnames, &err)) {
+		packed_refs_unlock(refs->packed_ref_store);
+		goto error;
 	}
 
+	packed_refs_unlock(refs->packed_ref_store);
+
 	for (i = 0; i < refnames->nr; i++) {
 		const char *refname = refnames->items[i].string;
 
@@ -1162,9 +1154,24 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
 			result |= error(_("could not remove reference %s"), refname);
 	}
 
-out:
 	strbuf_release(&err);
 	return result;
+
+error:
+	/*
+	 * If we failed to rewrite the packed-refs file, then it is
+	 * unsafe to try to remove loose refs, because doing so might
+	 * expose an obsolete packed value for a reference that might
+	 * even point at an object that has been garbage collected.
+	 */
+	if (refnames->nr == 1)
+		error(_("could not delete reference %s: %s"),
+		      refnames->items[0].string, err.buf);
+	else
+		error(_("could not delete references: %s"), err.buf);
+
+	strbuf_release(&err);
+	return -1;
 }
 
 /*
@@ -2557,11 +2564,19 @@ static int files_transaction_finish(struct ref_store *ref_store,
 		}
 	}
 
+	if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
+		ret = TRANSACTION_GENERIC_ERROR;
+		goto cleanup;
+	}
+
 	if (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
+		packed_refs_unlock(refs->packed_ref_store);
 		goto cleanup;
 	}
 
+	packed_refs_unlock(refs->packed_ref_store);
+
 	/* Delete the reflogs of any references that were deleted: */
 	for_each_string_list_item(ref_to_delete, &refs_to_delete) {
 		strbuf_reset(&sb);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 54b48d1f02..721afd066a 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -669,25 +669,12 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 	return -1;
 }
 
-/*
- * Rollback the lockfile for the packed-refs file, and discard the
- * in-memory packed reference cache.  (The packed-refs file will be
- * read anew if it is needed again after this function is called.)
- */
-static void rollback_packed_refs(struct packed_ref_store *refs)
-{
-	packed_assert_main_repository(refs, "rollback_packed_refs");
-
-	if (!is_lock_file_locked(&refs->lock))
-		die("BUG: packed-refs not locked");
-	packed_refs_unlock(&refs->base);
-	clear_packed_ref_cache(refs);
-}
-
 /*
  * Rewrite the packed-refs file, omitting any refs listed in
  * 'refnames'. On error, leave packed-refs unchanged, write an error
- * message to 'err', and return a nonzero value.
+ * message to 'err', and return a nonzero value. The packed refs lock
+ * must be held when calling this function; it will still be held when
+ * the function returns.
  *
  * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
  */
@@ -700,11 +687,13 @@ int repack_without_refs(struct ref_store *ref_store,
 	struct ref_dir *packed;
 	struct string_list_item *refname;
 	int needs_repacking = 0, removed = 0;
-	int ret;
 
 	packed_assert_main_repository(refs, "repack_without_refs");
 	assert(err);
 
+	if (!is_lock_file_locked(&refs->lock))
+		die("BUG: repack_without_refs called without holding lock");
+
 	/* Look for a packed ref */
 	for_each_string_list_item(refname, refnames) {
 		if (get_packed_ref(refs, refname->string)) {
@@ -717,9 +706,6 @@ int repack_without_refs(struct ref_store *ref_store,
 	if (!needs_repacking)
 		return 0; /* no refname exists in packed refs */
 
-	if (packed_refs_lock(&refs->base, 0, err))
-		return -1;
-
 	packed = get_packed_refs(refs);
 
 	/* Remove refnames from the cache */
@@ -731,14 +717,12 @@ int repack_without_refs(struct ref_store *ref_store,
 		 * All packed entries disappeared while we were
 		 * acquiring the lock.
 		 */
-		rollback_packed_refs(refs);
+		clear_packed_ref_cache(refs);
 		return 0;
 	}
 
 	/* Write what remains */
-	ret = commit_packed_refs(&refs->base, err);
-	packed_refs_unlock(ref_store);
-	return ret;
+	return commit_packed_refs(&refs->base, err);
 }
 
 static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 25/28] clear_packed_ref_cache(): don't protest if the lock is held

The existing callers already check that the lock isn't held just before calling `clear_packed_ref_cache()`, and in the near future we want to be able to call this function when the lock is held.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/packed-backend.c | 2 --
 1 file changed, 2 deletions(-)
Show changes to refs/packed-backend.c +0 −2
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index ff6326ddb8..1732e3aad4 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -133,8 +133,6 @@ static void clear_packed_ref_cache(struct packed_ref_store *refs)
 	if (refs->cache) {
 		struct packed_ref_cache *cache = refs->cache;
 
-		if (is_lock_file_locked(&refs->lock))
-			die("BUG: packed-ref cache cleared while locked");
 		refs->cache = NULL;
 		release_packed_ref_cache(cache);
 	}
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 12/28] rollback_packed_refs(): take a `packed_ref_store *` parameter

It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
Show changes to refs/files-backend.c +7 −8
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5d159620f0..a08d3fbadf 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1434,18 +1434,17 @@ static int commit_packed_refs(struct packed_ref_store *refs)
  * in-memory packed reference cache.  (The packed-refs file will be
  * read anew if it is needed again after this function is called.)
  */
-static void rollback_packed_refs(struct files_ref_store *refs)
+static void rollback_packed_refs(struct packed_ref_store *refs)
 {
-	struct packed_ref_cache *packed_ref_cache =
-		get_packed_ref_cache(refs->packed_ref_store);
+	struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
 
-	files_assert_main_repository(refs, "rollback_packed_refs");
+	packed_assert_main_repository(refs, "rollback_packed_refs");
 
-	if (!is_lock_file_locked(&refs->packed_ref_store->lock))
+	if (!is_lock_file_locked(&refs->lock))
 		die("BUG: packed-refs not locked");
-	rollback_lock_file(&refs->packed_ref_store->lock);
+	rollback_lock_file(&refs->lock);
 	release_packed_ref_cache(packed_ref_cache);
-	clear_packed_ref_cache(refs->packed_ref_store);
+	clear_packed_ref_cache(refs);
 }
 
 struct ref_to_prune {
@@ -1657,7 +1656,7 @@ static int repack_without_refs(struct files_ref_store *refs,
 		 * All packed entries disappeared while we were
 		 * acquiring the lock.
 		 */
-		rollback_packed_refs(refs);
+		rollback_packed_refs(refs->packed_ref_store);
 		return 0;
 	}
 
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 02/28] packed_ref_store: new struct

Start extracting the packed-refs-related data structures into a new class, `packed_ref_store`. It doesn't yet implement `ref_store`, but it will.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)
Show changes to refs/files-backend.c +33 −9
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 87cecde231..2efb71cee9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -47,6 +47,28 @@ struct packed_ref_cache {
 	struct stat_validity validity;
 };
 
+/*
+ * A container for `packed-refs`-related data. It is not (yet) a
+ * `ref_store`.
+ */
+struct packed_ref_store {
+	unsigned int store_flags;
+
+	/*
+	 * A cache of the values read from the `packed-refs` file, if
+	 * it might still be current; otherwise, NULL.
+	 */
+	struct packed_ref_cache *cache;
+};
+
+static struct packed_ref_store *packed_ref_store_create(unsigned int store_flags)
+{
+	struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
+
+	refs->store_flags = store_flags;
+	return refs;
+}
+
 /*
  * Future: need to be in "struct repository"
  * when doing a full libification.
@@ -60,13 +82,14 @@ struct files_ref_store {
 	char *packed_refs_path;
 
 	struct ref_cache *loose;
-	struct packed_ref_cache *packed;
 
 	/*
 	 * Lock used for the "packed-refs" file. Note that this (and
 	 * thus the enclosing `files_ref_store`) must not be freed.
 	 */
 	struct lock_file packed_refs_lock;
+
+	struct packed_ref_store *packed_ref_store;
 };
 
 /*
@@ -95,12 +118,12 @@ static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)
 
 static void clear_packed_ref_cache(struct files_ref_store *refs)
 {
-	if (refs->packed) {
-		struct packed_ref_cache *packed_refs = refs->packed;
+	if (refs->packed_ref_store->cache) {
+		struct packed_ref_cache *packed_refs = refs->packed_ref_store->cache;
 
 		if (is_lock_file_locked(&refs->packed_refs_lock))
 			die("BUG: packed-ref cache cleared while locked");
-		refs->packed = NULL;
+		refs->packed_ref_store->cache = NULL;
 		release_packed_ref_cache(packed_refs);
 	}
 }
@@ -132,6 +155,7 @@ static struct ref_store *files_ref_store_create(const char *gitdir,
 	refs->gitcommondir = strbuf_detach(&sb, NULL);
 	strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
 	refs->packed_refs_path = strbuf_detach(&sb, NULL);
+	refs->packed_ref_store = packed_ref_store_create(flags);
 
 	return ref_store;
 }
@@ -375,8 +399,8 @@ static void files_ref_path(struct files_ref_store *refs,
  */
 static void validate_packed_ref_cache(struct files_ref_store *refs)
 {
-	if (refs->packed &&
-	    !stat_validity_check(&refs->packed->validity,
+	if (refs->packed_ref_store->cache &&
+	    !stat_validity_check(&refs->packed_ref_store->cache->validity,
 				 files_packed_refs_path(refs)))
 		clear_packed_ref_cache(refs);
 }
@@ -396,10 +420,10 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref
 	if (!is_lock_file_locked(&refs->packed_refs_lock))
 		validate_packed_ref_cache(refs);
 
-	if (!refs->packed)
-		refs->packed = read_packed_refs(packed_refs_file);
+	if (!refs->packed_ref_store->cache)
+		refs->packed_ref_store->cache = read_packed_refs(packed_refs_file);
 
-	return refs->packed;
+	return refs->packed_ref_store->cache;
 }
 
 static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_cache)
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 06/28] validate_packed_ref_cache(): take a `packed_ref_store *` parameter

It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
Show changes to refs/files-backend.c +6 −7
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2b0db60b2b..f061506bf0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -396,12 +396,11 @@ static void files_ref_path(struct files_ref_store *refs,
  * Check that the packed refs cache (if any) still reflects the
  * contents of the file. If not, clear the cache.
  */
-static void validate_packed_ref_cache(struct files_ref_store *refs)
+static void validate_packed_ref_cache(struct packed_ref_store *refs)
 {
-	if (refs->packed_ref_store->cache &&
-	    !stat_validity_check(&refs->packed_ref_store->cache->validity,
-				 refs->packed_ref_store->path))
-		clear_packed_ref_cache(refs->packed_ref_store);
+	if (refs->cache &&
+	    !stat_validity_check(&refs->cache->validity, refs->path))
+		clear_packed_ref_cache(refs);
 }
 
 /*
@@ -417,7 +416,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref
 	const char *packed_refs_file = refs->packed_ref_store->path;
 
 	if (!is_lock_file_locked(&refs->packed_ref_store->lock))
-		validate_packed_ref_cache(refs);
+		validate_packed_ref_cache(refs->packed_ref_store);
 
 	if (!refs->packed_ref_store->cache)
 		refs->packed_ref_store->cache = read_packed_refs(packed_refs_file);
@@ -1364,7 +1363,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	 * cache is still valid. We've just locked the file, but it
 	 * might have changed the moment *before* we locked it.
 	 */
-	validate_packed_ref_cache(refs);
+	validate_packed_ref_cache(refs->packed_ref_store);
 
 	packed_ref_cache = get_packed_ref_cache(refs);
 	/* Increment the reference count to prevent it from being freed: */
-- 
2.11.0
Michael Haggerty· Jun 15, 2017, 14:47 UTC · re: Michael Haggerty · lore

[PATCH 05/28] clear_packed_ref_cache(): take a `packed_ref_store *` parameter

It only cares about the packed-refs part of the reference store.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
Show changes to refs/files-backend.c +8 −8
diff --git a/refs/files-backend.c b/refs/files-backend.c
index de8293493f..2b0db60b2b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -120,15 +120,15 @@ static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)
 	}
 }
 
-static void clear_packed_ref_cache(struct files_ref_store *refs)
+static void clear_packed_ref_cache(struct packed_ref_store *refs)
 {
-	if (refs->packed_ref_store->cache) {
-		struct packed_ref_cache *packed_refs = refs->packed_ref_store->cache;
+	if (refs->cache) {
+		struct packed_ref_cache *cache = refs->cache;
 
-		if (is_lock_file_locked(&refs->packed_ref_store->lock))
+		if (is_lock_file_locked(&refs->lock))
 			die("BUG: packed-ref cache cleared while locked");
-		refs->packed_ref_store->cache = NULL;
-		release_packed_ref_cache(packed_refs);
+		refs->cache = NULL;
+		release_packed_ref_cache(cache);
 	}
 }
 
@@ -401,7 +401,7 @@ static void validate_packed_ref_cache(struct files_ref_store *refs)
 	if (refs->packed_ref_store->cache &&
 	    !stat_validity_check(&refs->packed_ref_store->cache->validity,
 				 refs->packed_ref_store->path))
-		clear_packed_ref_cache(refs);
+		clear_packed_ref_cache(refs->packed_ref_store);
 }
 
 /*
@@ -1435,7 +1435,7 @@ static void rollback_packed_refs(struct files_ref_store *refs)
 		die("BUG: packed-refs not locked");
 	rollback_lock_file(&refs->packed_ref_store->lock);
 	release_packed_ref_cache(packed_ref_cache);
-	clear_packed_ref_cache(refs);
+	clear_packed_ref_cache(refs->packed_ref_store);
 }
 
 struct ref_to_prune {
-- 
2.11.0
Junio C Hamano· Jun 19, 2017, 18:51 UTC · re: Michael Haggerty · lore

Re: [PATCH 00/28] Create a reference backend for packed refs

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 13 quoted lines
> I've developed these patches on top of master plus the following
> patches, which are followups to mh/packed-refs-store-prep:
>
> * lock_packed_refs(): fix cache validity check
> * for_each_bisect_ref(): don't trim refnames
>
> The patches can also be obtained from my GitHub fork [2] as branch
> "packed-ref-store".
>
> Michael
>
> [1] http://public-inbox.org/git/cover.1490026594.git.mhagger@alum.mit.edu/
> [2] https://github.com/mhagger/git
Thanks.

Both in the version I queued and a fresh fetching of c13b2fad ("read_packed_refs(): die if `packed-refs` contains bogus data", 2017-06-12) from your https://github.com/mhagger/git repository seems to exhibit an annoying error message in my local repository I use for the primary work:

    $ git fetch https://github.com/mhagger/git packed-ref-store
    $ git checkout FETCH_HEAD
    $ make
    $ ./git describe next
    error: refs/notes/amlog does not point to a valid object!
    v2.13.1-611-g7e3b11ae1b
    $ grep refs/notes/amlog .git/packed-refs
    ed07e83cff8e407464fb2f5e84bd311da9c87565 refs/notes/amlog
    $ git rev-parse refs/notes/amlog
    b3079212325398e406078585c785c892d6e572f0
    $ git cat-file -t ed07e83cff8e407464fb2f5e84bd311da9c87565
    fatal: git cat-file: could not get object info
    $ git cat-file -t b3079212325398e406078585c785c892d6e572f0
    commit

Is the iterator over packed-refs correctly skipping over what are covered by loose refs? The entries in the packed-refs file that are superseded by loose refs should be allowed to point at an already expired object.

Junio C Hamano· Jun 19, 2017, 19:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/28] Create a reference backend for packed refs

Junio C Hamano <gitster@pobox.com> writes:
> Is the iterator over packed-refs correctly skipping over what are
> covered by loose refs?  The entries in the packed-refs file that are
> superseded by loose refs should be allowed to point at an already
> expired object.
Here it is in a test form for easier diagnosis.
 t/t1408-packed-refs.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
Show changes to t/t1408-packed-refs.sh +46 −0
diff --git a/t/t1408-packed-refs.sh b/t/t1408-packed-refs.sh
new file mode 100755
index 0000000000..35533c8593
--- /dev/null
+++ b/t/t1408-packed-refs.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='packed-refs entries are covered by loose refs'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_tick &&
+	git commit --allow-empty -m one &&
+	one=$(git rev-parse HEAD) && 
+	git for-each-ref >actual &&
+	echo "$one commit	refs/heads/master" >expect &&
+	test_cmp expect actual &&
+
+	git pack-refs --all &&
+	git for-each-ref >actual &&
+	echo "$one commit	refs/heads/master" >expect &&
+	test_cmp expect actual &&
+
+	cat .git/packed-refs &&
+
+	git checkout --orphan another &&
+	test_tick &&
+	git commit --allow-empty -m two &&
+	two=$(git rev-parse HEAD) && 
+	git checkout -B master &&
+	git branch -D another &&
+
+	cat .git/packed-refs &&
+
+	git for-each-ref >actual &&
+	echo "$two commit	refs/heads/master" >expect &&
+	test_cmp expect actual &&
+
+	git reflog expire --expire=now --all &&
+	git prune &&
+	git tag -m v1.0 v1.0 master
+'
+
+test_expect_success 'no error from stale entry in packed-refs' '
+	git describe master >actual 2>&1 &&
+	echo "v1.0" >expect &&
+	test_cmp expect actual
+'
+
+test_done
Jeff King· Jun 19, 2017, 19:43 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/28] Create a reference backend for packed refs

On Mon, Jun 19, 2017 at 12:25:07PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Is the iterator over packed-refs correctly skipping over what are
> > covered by loose refs?  The entries in the packed-refs file that are
> > superseded by loose refs should be allowed to point at an already
> > expired object.
> 
> Here it is in a test form for easier diagnosis.

Thanks, I was just starting to do that myself. The problem is in ca6b06eb7 (packed_ref_store: support iteration, 2017-05-15) and is pretty obvious: the packed_ref iterator checks whether the entry resolves.

I think that _neither_ of the loose and packed iterators should be checking this. It's only the merged result (where loose trumps packed) that should bother checking.

-Peff
Jeff King· Jun 19, 2017, 19:53 UTC · re: Jeff King · lore

Re: [PATCH 00/28] Create a reference backend for packed refs

On Mon, Jun 19, 2017 at 03:43:15PM -0400, Jeff King wrote:
Show 15 quoted lines
> > > Is the iterator over packed-refs correctly skipping over what are
> > > covered by loose refs?  The entries in the packed-refs file that are
> > > superseded by loose refs should be allowed to point at an already
> > > expired object.
> > 
> > Here it is in a test form for easier diagnosis.
> 
> Thanks, I was just starting to do that myself. The problem is in
> ca6b06eb7 (packed_ref_store: support iteration, 2017-05-15) and is
> pretty obvious: the packed_ref iterator checks whether the entry
> resolves.
> 
> I think that _neither_ of the loose and packed iterators should be
> checking this. It's only the merged result (where loose trumps packed)
> that should bother checking.

It looks like this is mostly already the case. files_ref_iterator combines the two and does check. The loose iteration is done by cache_ref_iterator[1], and does not. So it's just this new packed_refs iterator that is wrong, and we just need:

Show changes to refs/packed-backend.c +0 −9
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 311fd014c..ad1143e64 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -416,12 +416,6 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
 		    ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)
 			continue;
 
-		if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
-		    !ref_resolves_to_object(iter->iter0->refname,
-					    iter->iter0->oid,
-					    iter->iter0->flags))
-			continue;
-
 		iter->base.refname = iter->iter0->refname;
 		iter->base.oid = iter->iter0->oid;
 		iter->base.flags = iter->iter0->flags;
@@ -473,8 +467,6 @@ static struct ref_iterator *packed_ref_iterator_begin(
 	struct ref_iterator *ref_iterator;
 	unsigned int required_flags = REF_STORE_READ;
 
-	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN))
-		required_flags |= REF_STORE_ODB;
 	refs = packed_downcast(ref_store, required_flags, "ref_iterator_begin");
 
 	iter = xcalloc(1, sizeof(*iter));

At least that makes sense to me and passes the tests (including the new
one). I haven't actually reviewed the patches yet.

-Peff
Michael Haggerty· Jun 22, 2017, 07:36 UTC · re: Jeff King · lore

Re: [PATCH 00/28] Create a reference backend for packed refs

On 06/19/2017 09:53 PM, Jeff King wrote:
Show 17 quoted lines
> On Mon, Jun 19, 2017 at 03:43:15PM -0400, Jeff King wrote:
> 
>>>> Is the iterator over packed-refs correctly skipping over what are
>>>> covered by loose refs?  The entries in the packed-refs file that are
>>>> superseded by loose refs should be allowed to point at an already
>>>> expired object.
>>>
>>> Here it is in a test form for easier diagnosis.
>>
>> Thanks, I was just starting to do that myself. The problem is in
>> ca6b06eb7 (packed_ref_store: support iteration, 2017-05-15) and is
>> pretty obvious: the packed_ref iterator checks whether the entry
>> resolves.
>>
>> I think that _neither_ of the loose and packed iterators should be
>> checking this. It's only the merged result (where loose trumps packed)
>> that should bother checking.
Thanks for the bug report and the analysis, which is exactly right.

But I'd like to fix the problem a *little* differently than Peff suggested. To keep `packed_ref_store` from deviating more than necessary from the `ref_store` interface, I propose that we leave the code for rejecting broken refs where it is, and instead invoke `packed_ref_iterator_begin()` with the `DO_FOR_EACH_INCLUDE_BROKEN` flag.

I have prepared a re-roll of the patch series, but I can't submit it until I have Junio's signoff on the test that he suggested [1]. Junio?

Thanks, Michael

[1] http://public-inbox.org/git/xmqqvanrsru4.fsf@gitster.mtv.corp.google.com/

Junio C Hamano· Jun 22, 2017, 15:33 UTC · re: Michael Haggerty · lore

Re: [PATCH 00/28] Create a reference backend for packed refs

Michael Haggerty <mhagger@alum.mit.edu> writes:
> I have prepared a re-roll of the patch series, but I can't submit it
> until I have Junio's signoff on the test that he suggested [1]. Junio?
Sure, and thanks.

← back to recent threads