Create a reference backend for packed refs

PATCH Superseded Michael Haggerty 4 participants 52 messages Jun 23, 2017 – Jul 1, 2017 Kbuild

Share & tools

Michael Haggerty

author

2017-06-23 07:02 UTC #0 lore raw

v2 cover Create a reference backend for packed refs

This is v2 of a patch series creating a packed_ref_store reference backend. Thanks to Stefan, Junio, and Peff for their comments about v1 [1].

Changes since v1:

* Picked up some whitespace fixes that Junio applied when importing v1.

* A new patch 01 adds a test (written by Junio) that a broken packed ref doesn't cause a problem if it is overwritten by a corresponding loose ref. This test was broken by v1 of this patch series but it passes throughout v2.

* Change patch 17 "packed_ref_store: support iteration" to always iterate over the packed refs using DO_FOR_EACH_INCLUDE_BROKEN. This switches off the check in the packed-ref iterator of whether a reference is broken. This is now checked only in files_ref_iterator_advance(), after the packed and loose references have been merged together. It also saves some work.

* Rebased the patch series onto mh/packed-ref-store-prep, which is where Junio applied v1 of this patch series.

This patch series can also be obtained from my GitHub fork [2] as branch packed-ref-store.

[1] http://public-inbox.org/git/[email protected]/
[2] https://github.com/mhagger/git

Junio C Hamano (1):
t1408: add a test of stale packed refs covered by loose refs

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   | 631 ++++-------------------------------
 refs/packed-backend.c  | 868 +++++++++++++++++++++++++++++++++++++++++++++++++
 refs/packed-backend.h  |  25 ++
 refs/refs-internal.h   |  10 +
 t/t1408-packed-refs.sh |  46 +++
 7 files changed, 1039 insertions(+), 560 deletions(-)
 create mode 100644 refs/packed-backend.c
 create mode 100644 refs/packed-backend.h
 create mode 100755 t/t1408-packed-refs.sh

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #1 lore raw

PATCH v2 1/29 t1408: add a test of stale packed refs covered by loose refs

From: Junio C Hamano <[email protected]>

It is OK for the packed-refs file to contain old reference definitions that might even refer to objects that have since been garbage-collected, as long as there is a corresponding loose reference definition that overrides it. Add a test that such references don't cause problems.

Signed-off-by Junio C Hamano <[email protected]>

Signed-off-by Michael Haggerty <[email protected]>

---
 t/t1408-packed-refs.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100755 t/t1408-packed-refs.sh

t/t1408-packed-refs.sh new file +46 -0

new file mode 100755
index 0000000000..d3155be206
@@ -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

Signature · 1 line

2.11.0

Jeff King

2017-06-23 18:59 UTC #2 lore raw

Re: [PATCH v2 01/29] t1408: add a test of stale packed refs covered by loose refs

Quoted 4 lines · Michael Haggerty

t/t1408-packed-refs.sh new file +4 -0

+test_expect_success setup '
+	test_tick &&
+	git commit --allow-empty -m one &&
+	one=$(git rev-parse HEAD) &&

t/t1408-packed-refs.sh line 20

Quoted 10 lines

t/t1408-packed-refs.sh new file +10 -0

+	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 &&

I think we'd usually drop debugging "cat"s like these in the name of keeping the process count down. Unless they really are intended to confirm that .git/packed-refs exists (although test_path_is_file is a less expensive way of checking that).

That's a minor nit, though.

-Peff

Junio C Hamano

2017-06-23 20:22 UTC #3 lore raw

Re: [PATCH v2 01/29] t1408: add a test of stale packed refs covered by loose refs

Quoted 22 lines · Jeff King

Quoted 4 lines · Michael Haggerty
t/t1408-packed-refs.sh new file +4 -0
+test_expect_success setup '
+	test_tick &&
+	git commit --allow-empty -m one &&
+	one=$(git rev-parse HEAD) &&
t/t1408-packed-refs.sh line 20
Quoted 10 lines
t/t1408-packed-refs.sh new file +10 -0
+	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 &&
I think we'd usually drop debugging "cat"s like these in the name of keeping the process count down. Unless they really are intended to confirm that .git/packed-refs exists (although test_path_is_file is a less expensive way of checking that).

Quoted 1 line

That's a minor nit, though.

Sorry, these two cat's were what I used while debugging the test and should be dropped.

Will remove while queuing.

Thanks for spotting.

Michael Haggerty

author

2017-06-23 07:02 UTC #4 lore raw

PATCH v2 2/29 add_packed_ref(): teach function to overwrite existing refs

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/files-backend.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

refs/files-backend.c +18 ~~-22~~

index b040bb3b0a..87cecde231 100644
@@ -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)) {

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #5 lore raw

PATCH v2 3/29 packed_ref_store: new struct

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/files-backend.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

refs/files-backend.c +33 -9

index 87cecde231..2efb71cee9 100644
@@ -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)

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #6 lore raw

PATCH v2 4/29 packed_ref_store: move `packed_refs_path` here

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/files-backend.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

refs/files-backend.c +12 ~~-13~~

index 2efb71cee9..c4b8e2f63b 100644
@@ -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);

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #7 lore raw

PATCH v2 5/29 packed_ref_store: move `packed_refs_lock` member here

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/files-backend.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

refs/files-backend.c +16 ~~-15~~

index c4b8e2f63b..de8293493f 100644
@@ -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);
 }

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #8 lore raw

PATCH v2 6/29 clear_packed_ref_cache(): take a `packed_ref_store *` parameter

In reply to Michael Haggerty

It only cares about the packed-refs part of the reference store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

refs/files-backend.c +8 -8

index de8293493f..2b0db60b2b 100644
@@ -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 {

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #9 lore raw

PATCH v2 7/29 validate_packed_ref_cache(): take a `packed_ref_store *` parameter

In reply to Michael Haggerty

It only cares about the packed-refs part of the reference store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

refs/files-backend.c +6 -7

index 2b0db60b2b..f061506bf0 100644
@@ -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: */

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:03 UTC #10 lore raw

PATCH v2 8/29 get_packed_ref_cache(): take a `packed_ref_store *` parameter

In reply to Michael Haggerty

It only cares about the packed-refs part of the reference store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

refs/files-backend.c +12 ~~-14~~

index f061506bf0..b2ef7b3bb9 100644
@@ -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");

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #11 lore raw

PATCH v2 9/29 get_packed_refs(): take a `packed_ref_store *` parameter

In reply to Michael Haggerty

It only cares about the packed-refs part of the reference store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

refs/files-backend.c +5 -5

index b2ef7b3bb9..bc5c0de84e 100644
@@ -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)

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:03 UTC #12 lore raw

PATCH v2 10/29 add_packed_ref(): take a `packed_ref_store *` parameter

In reply to Michael Haggerty

It only cares about the packed-refs part of the reference store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

refs/files-backend.c +5 -5

index bc5c0de84e..4943207098 100644
@@ -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);
 	}

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:03 UTC #13 lore raw

PATCH v2 11/29 lock_packed_refs(): take a `packed_ref_store *` parameter

In reply to Michael Haggerty

It only cares about the packed-refs part of the reference store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

refs/files-backend.c +22 -9

index 4943207098..0d8f39089f 100644
@@ -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;

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #14 lore raw

PATCH v2 12/29 commit_packed_refs(): take a `packed_ref_store *` parameter

In reply to Michael Haggerty

It only cares about the packed-refs part of the reference store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

refs/files-backend.c +9 -9

index 0d8f39089f..5d159620f0 100644
@@ -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;

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #15 lore raw

PATCH v2 13/29 rollback_packed_refs(): take a `packed_ref_store *` parameter

In reply to Michael Haggerty

It only cares about the packed-refs part of the reference store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

refs/files-backend.c +7 -8

index 5d159620f0..a08d3fbadf 100644
@@ -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;
 	}

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #16 lore raw

PATCH v2 14/29 get_packed_ref(): take a `packed_ref_store *` parameter

In reply to Michael Haggerty

It only cares about the packed-refs part of the reference store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

refs/files-backend.c +7 -5

index a08d3fbadf..2b9d93d3b6 100644
@@ -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;
 		}

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:03 UTC #17 lore raw

PATCH v2 15/29 repack_without_refs(): take a `packed_ref_store *` parameter

In reply to Michael Haggerty

It only cares about the packed-refs part of the reference store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

refs/files-backend.c +10 ~~-10~~

index 2b9d93d3b6..c206791b91 100644
@@ -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;
 	}

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #18 lore raw

PATCH v2 16/29 packed_peel_ref(): new function, extracted from `files_peel_ref()`

In reply to Michael Haggerty

This will later become a method of packed_ref_store.

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

refs/files-backend.c +15 ~~-11~~

index c206791b91..185d05e1d6 100644
@@ -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);
 }

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #19 lore raw

PATCH v2 17/29 packed_ref_store: support iteration

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/files-backend.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 110 insertions(+), 9 deletions(-)

refs/files-backend.c +110 -9

index 185d05e1d6..0490cc087e 100644
@@ -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,28 @@ 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);
+	/*
+	 * The packed-refs file might contain broken references, for
+	 * example an old version of a reference that points at an
+	 * object that has since been garbage-collected. This is OK as
+	 * long as there is a corresponding loose reference that
+	 * overrides it, and we don't want to emit an error message in
+	 * this case. So ask the packed_ref_store for all of its
+	 * references, and (if needed) do our own check for broken
+	 * ones in files_ref_iterator_advance(), after we have merged
+	 * the packed and loose references.
+	 */
+	packed_iter = packed_ref_iterator_begin(
+			refs->packed_ref_store, prefix,
+			DO_FOR_EACH_INCLUDE_BROKEN);
 
 	iter->iter0 = overlay_ref_iterator_begin(loose_iter, packed_iter);
 	iter->flags = flags;

Signature · 1 line

2.11.0

Jeff King

2017-06-23 19:12 UTC #20 lore raw

Re: [PATCH v2 17/29] packed_ref_store: support iteration

refs/files-backend.c static int files_peel_ref(struct ref_store *ref_store, · line 1076

Quoted 4 lines · Michael Haggerty

refs/files-backend.c +4 -0

+static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+	struct packed_ref_iterator *iter =
+		(struct packed_ref_iterator *)ref_iterator;

I thought had some kind of safe downcasting mechanism for iterators, but I think I was just thinking of files_downcast() for the ref-store.

I don't mind not having that kind of safety. It seems like an uncommon error to call the packed-ref iterator function on the wrong type, especially as it's static here and only accessible as a virtual function for a packed_ref_iterator.

But something to think about, I guess, as we add more polymorphism.

-Peff

Michael Haggerty

author

2017-06-23 07:03 UTC #21 lore raw

PATCH v2 18/29 packed_read_raw_ref(): new function, replacing `resolve_packed_ref()`

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/files-backend.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

refs/files-backend.c +17 ~~-19~~

index 0490cc087e..346794cf7c 100644
@@ -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;
 		}

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #22 lore raw

PATCH v2 19/29 packed-backend: new module for handling packed references

In reply to Michael Haggerty

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 <[email protected]>

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

Makefile +1 -0

index e35542e631..aff9c6d7e2 100644
@@ -817,6 +817,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

refs.c +18 -0

index 32177969f0..5880c12372 100644
@@ -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)

refs/files-backend.c +1 ~~-639~~

700 lines not on this page · raw message

refs/packed-backend.c new file +624 -0

624 lines not on this page · raw message

refs/packed-backend.h new file +33 -0

new file mode 100644
index 0000000000..22e8817ac4
@@ -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 */

refs/refs-internal.h +9 -0

index 192f9f85c9..6f8f9f5619 100644
@@ -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,

Signature · 1 line

2.11.0

Jeff King

2017-06-23 19:35 UTC #23 lore raw

Re: [PATCH v2 19/29] packed-backend: new module for handling packed references

Quoted 7 lines · Michael Haggerty

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.

Looks good. Stefan will be happy to know that I used --color-moved to look at it. ;)

-Peff

Stefan Beller

2017-06-23 19:46 UTC #24 lore raw

Re: [PATCH v2 19/29] packed-backend: new module for handling packed references

Quoted 7 lines · Jeff King

Quoted 5 lines · Michael Haggerty
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.

Quoted 5 lines

Quoted 2 lines
This commit moves code around and adjusts its visibility, but doesn't change anything.
Looks good. Stefan will be happy to know that I used --color-moved to look at it. ;)

Hah!

As a follow up on that, let's perform a user survey :-P
* How was your review experience?
* Were you annoyed by the default colors or mode? (That is best expressed as a patch, as it seems like bikeshedding to me, but I am far from being a UX expert ;)

Just today I thought about that further:
While reviewing is one thing (which I do a lot), how can we make this work with merging changes?

I think the file rename detection is acknowledged by the merge code, such that a plain file rename and a patch to said file is easy on the maintainer, but we would want that for smaller code movements, too.

Let's take this patch as an example, if someone were to find a bug in one of the moved functions, they would send a fix based on the function in refs/files-backend.c, such that it can easily be merged down to maint, but when merging it forward with this, it may clash.

Junio C Hamano

2017-06-23 21:20 UTC #25 lore raw

Re: [PATCH v2 19/29] packed-backend: new module for handling packed references

In reply to Jeff King

Quoted 7 lines · Jeff King

Quoted 5 lines · Michael Haggerty
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.

Quoted 5 lines

Quoted 2 lines
This commit moves code around and adjusts its visibility, but doesn't change anything.
Looks good. Stefan will be happy to know that I used --color-moved to look at it. ;)

This patch shows OK, but when applied to other changes in the series, e.g. "packed_ref_store: make class into a subclass of ref_store", it was somewhat irritating that all new blank lines are shown as a copy-move of a single deleted blank line (also a single "return refs" in an entirely new function being a copy/move looked confusing).

Michael Haggerty

author

2017-06-23 07:02 UTC #26 lore raw

PATCH v2 20/29 packed_ref_store: make class into a subclass of `ref_store`

In reply to Michael Haggerty

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 <[email protected]>

---
 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(-)

refs/files-backend.c +8 -8

index 7df9747798..60f4fa5e7a 100644
@@ -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);

@@ -804,8 +804,8 @@ static struct ref_iterator *files_ref_iterator_begin(
 	 * ones in files_ref_iterator_advance(), after we have merged
 	 * the packed and loose references.
 	 */
-	packed_iter = packed_ref_iterator_begin(
-			refs->packed_ref_store, prefix,
+	packed_iter = refs_ref_iterator_begin(
+			refs->packed_ref_store, prefix, 0,
 			DO_FOR_EACH_INCLUDE_BROKEN);
 
 	iter->iter0 = overlay_ref_iterator_begin(loose_iter, packed_iter);

refs/packed-backend.c +211 ~~-20~~

333 lines not on this page · raw message

refs/packed-backend.h +6 ~~-17~~

index 22e8817ac4..beea9c14b5 100644
@@ -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 */

refs/refs-internal.h +1 -0

index 6f8f9f5619..4789106fc0 100644
@@ -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

Signature · 1 line

2.11.0

Jeff King

2017-06-23 19:40 UTC #27 lore raw

Re: [PATCH v2 20/29] packed_ref_store: make class into a subclass of `ref_store`

Quoted 7 lines · Michael Haggerty

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.

By itself this looks correct (as do the patches leading up to it). But I think some of the "big picture" is lost. Why do we want it to look like a ref store?

I suspect the answer is too big to go into this individual commit message. But I went back and re-read the cover letter, and I don't think it's really explained there, either.

So I'm not really sure where it ought to be covered (or if I simply missed it somewhere, or if it's coming up; I'm reading the patches linearly).

-Peff

Michael Haggerty

author

2017-06-23 07:02 UTC #28 lore raw

PATCH v2 21/29 commit_packed_refs(): report errors rather than dying

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/files-backend.c  | 10 +++---
 refs/packed-backend.c | 85 +++++++++++++++++++++++++++++++++------------------
 refs/packed-backend.h |  2 +-
 3 files changed, 61 insertions(+), 36 deletions(-)

refs/files-backend.c +5 -5

index 60f4fa5e7a..2810785efc 100644
@@ -1094,6 +1094,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);

@@ -1128,10 +1129,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;
 }

@@ -2693,9 +2695,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;
 	}

refs/packed-backend.c +55 ~~-30~~

137 lines not on this page · raw message

refs/packed-backend.h +1 -1

index beea9c14b5..3d4057b65b 100644
@@ -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);

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #29 lore raw

PATCH v2 22/29 commit_packed_refs(): use a staging file separate from the lockfile

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/packed-backend.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

refs/packed-backend.c +32 -8

index 18ce47fcb7..71f92ed6f0 100644
@@ -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;
 }

Signature · 1 line

2.11.0

Jeff King

2017-06-23 19:46 UTC #30 lore raw

Re: [PATCH v2 22/29] commit_packed_refs(): use a staging file separate from the lockfile

Quoted 6 lines · Michael Haggerty

refs/packed-backend.c +2 -0

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

refs/packed-backend.c int lock_packed_refs(struct ref_store *ref_store, int flags) · line 542

Quoted 10 lines

refs/packed-backend.c +5 -1

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

I was wondering whether we'd catch a case which accidentally wrote to the lockfile (instead of the new tempfile, but this close() is a good safety check).

refs/packed-backend.c int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err) · line 637

Quoted 7 lines

refs/packed-backend.c +2 -2

-	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/packed-backend.c +0 -0

 			    refs->path, strerror(errno));
 		goto out;
 	}

So our idea of committing now is the tempfile rename, and then...

refs/packed-backend.c int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err) · line 646

Quoted 9 lines

refs/packed-backend.c +2 -1

@@ -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);

We always rollback the lockfile regardless, because committing it would overwrite our new content with an empty file. There's no real safety against somebody calling commit_lock_file() on it, but it also seems like an uncommon error to make.

So this all looks good to me.

-Peff

Michael Haggerty

author

2017-06-24 11:43 UTC #31 lore raw

Re: [PATCH v2 22/29] commit_packed_refs(): use a staging file separate from the lockfile

Quoted 40 lines · Jeff King

Quoted 6 lines · Michael Haggerty
refs/packed-backend.c +2 -0
@@ -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
refs/packed-backend.c int lock_packed_refs(struct ref_store *ref_store, int flags) · line 542
Quoted 10 lines
refs/packed-backend.c +5 -1
+	 * 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;
I was wondering whether we'd catch a case which accidentally wrote to the lockfile (instead of the new tempfile, but this close() is a good safety check).
refs/packed-backend.c int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err) · line 637
Quoted 7 lines
refs/packed-backend.c +2 -2
-	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/packed-backend.c +0 -0
 			    refs->path, strerror(errno));
 		goto out;
 	}
So our idea of committing now is the tempfile rename, and then...
Quoted 7 lines
refs/packed-backend.c +1 -1
@@ -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);
 

Quoted 7 lines

refs/packed-backend.c int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err) · line 646
Quoted 2 lines
refs/packed-backend.c +1 -0
 out:
+	rollback_lock_file(&refs->lock);
We always rollback the lockfile regardless, because committing it would overwrite our new content with an empty file. There's no real safety against somebody calling commit_lock_file() on it, but it also seems like an uncommon error to make.

If this seems too dangerous, we could add a LOCK_NO_COMMIT flag for hold_lock_file_for_update() and hold_lock_file_for_update_timeout(), which would die() if somebody tries to commit the associated lockfile. I think we can live without it. I hope that any such bugs would be caught immediately by CI. But I admit that the prospect of renaming an empty file on top of a packed-refs file is quite sobering, so if anybody is worried about this, let me know and I'll implement it.

Michael

Jeff King

2017-06-24 11:57 UTC #32 lore raw

Re: [PATCH v2 22/29] commit_packed_refs(): use a staging file separate from the lockfile

Quoted 8 lines · Michael Haggerty

Quoted 7 lines
refs/packed-backend.c int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err) · line 646
Quoted 2 lines
out:
refs/packed-backend.c +1 -0
+	rollback_lock_file(&refs->lock);
We always rollback the lockfile regardless, because committing it would overwrite our new content with an empty file. There's no real safety against somebody calling commit_lock_file() on it, but it also seems like an uncommon error to make.

Quoted 7 lines

If this seems too dangerous, we could add a LOCK_NO_COMMIT flag for hold_lock_file_for_update() and hold_lock_file_for_update_timeout(), which would die() if somebody tries to commit the associated lockfile. I think we can live without it. I hope that any such bugs would be caught immediately by CI. But I admit that the prospect of renaming an empty file on top of a packed-refs file is quite sobering, so if anybody is worried about this, let me know and I'll implement it.

Yeah, that was the protection I was thinking of. Reflecting on it more, though, it's not really any different than somebody calling commit_lock_file() when we haven't correctly written out the whole content. It's probably not worth adding code to protect against this special case.

-Peff

Michael Haggerty

author

2017-06-23 07:03 UTC #33 lore raw

PATCH v2 23/29 packed_refs_lock(): function renamed from lock_packed_refs()

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/files-backend.c  |  4 ++--
 refs/packed-backend.c | 10 +++++-----
 refs/packed-backend.h |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

refs/files-backend.c +2 -2

index 2810785efc..88de907148 100644
@@ -1096,7 +1096,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) {

@@ -2679,7 +2679,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;

refs/packed-backend.c +5 -5

index 71f92ed6f0..cd214e07a1 100644
@@ -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;
 	}

refs/packed-backend.h +1 -1

index 3d4057b65b..dbc00d3396 100644
@@ -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);

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #34 lore raw

PATCH v2 24/29 packed_refs_lock(): report errors via a `struct strbuf *err`

In reply to Michael Haggerty

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

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c  |  6 ++----
 refs/packed-backend.c | 17 +++++++++++------
 refs/packed-backend.h |  6 +++---
 3 files changed, 16 insertions(+), 13 deletions(-)

refs/files-backend.c +2 -4

index 88de907148..8ea4e9ab05 100644
@@ -1096,7 +1096,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) {

@@ -2679,9 +2679,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;
 	}

refs/packed-backend.c +11 -6

index cd214e07a1..78e877a9e3 100644
@@ -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 */

refs/packed-backend.h +3 -3

index dbc00d3396..210e3f35ce 100644
@@ -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);

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:03 UTC #35 lore raw

PATCH v2 25/29 packed_refs_unlock(), packed_refs_is_locked(): new functions

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/packed-backend.c | 31 +++++++++++++++++++++++++------
 refs/packed-backend.h |  3 +++
 2 files changed, 28 insertions(+), 6 deletions(-)

refs/packed-backend.c +25 -6

index 78e877a9e3..f27943f9a1 100644
@@ -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);
 }

refs/packed-backend.h +3 -0

index 210e3f35ce..03b7c1de95 100644
@@ -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);

Signature · 1 line

2.11.0

Michael Haggerty

author

2017-06-23 07:02 UTC #36 lore raw

PATCH v2 26/29 clear_packed_ref_cache(): don't protest if the lock is held

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/packed-backend.c | 2 --
 1 file changed, 2 deletions(-)

refs/packed-backend.c +0 -2

index f27943f9a1..96d92a5eea 100644
@@ -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);
 	}

Signature · 1 line

2.11.0

Jeff King

2017-06-23 19:49 UTC #37 lore raw

Re: [PATCH v2 26/29] clear_packed_ref_cache(): don't protest if the lock is held

Quoted 3 lines · Michael Haggerty

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.

OK. It's not immediately obvious that this is true, because some of the relationships are a bit buried, but I double-checked and I think it is the case.

-Peff

Michael Haggerty

author

2017-06-23 07:03 UTC #38 lore raw

PATCH v2 27/29 commit_packed_refs(): remove call to `packed_refs_unlock()`

In reply to Michael Haggerty

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

Signed-off-by Michael Haggerty <[email protected]>

---
 refs/files-backend.c  |  2 ++
 refs/packed-backend.c | 18 ++++++++----------
 2 files changed, 10 insertions(+), 10 deletions(-)

refs/files-backend.c +2 -0

index 8ea4e9ab05..93bdc8f0c8 100644
@@ -1131,6 +1131,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);

@@ -2699,6 +2700,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;

refs/packed-backend.c +8 ~~-10~~

index 96d92a5eea..5cf6b3d40e 100644
@@ -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)

Signature · 1 line

2.11.0

Jeff King

2017-06-23 19:51 UTC #39 lore raw

Re: [PATCH v2 27/29] commit_packed_refs(): remove call to `packed_refs_unlock()`

Quoted 2 lines · Michael Haggerty

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

Why? I can guess that the reason is probably "because we're going to add a new caller that doesn't want to do that", but it's nice to hear even that. Those kinds of clues help make a more coherent narrative when you're reading through 29 refactoring patches. ;)

-Peff

Michael Haggerty

author

2017-06-23 07:03 UTC #40 lore raw

PATCH v2 28/29 repack_without_refs(): don't lock or unlock the packed refs

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/files-backend.c  | 47 +++++++++++++++++++++++++++++++----------------
 refs/packed-backend.c | 32 ++++++++------------------------
 2 files changed, 39 insertions(+), 40 deletions(-)

refs/files-backend.c +31 ~~-16~~

index 93bdc8f0c8..e9b95592b6 100644
@@ -1149,24 +1149,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;

@@ -1174,9 +1166,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;
 }
 
 /*

@@ -2569,11 +2576,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);

refs/packed-backend.c +8 ~~-24~~

index 5cf6b3d40e..377c775adb 100644
@@ -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)

Signature · 1 line

2.11.0

Jeff King

2017-06-23 19:56 UTC #41 lore raw

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

Quoted 7 lines · Michael Haggerty

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.

This is the sort of clue I was thinking about in my last email. :)

Quoted 3 lines

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.

s/used to forgot/previously forgot/ or similar?

refs/packed-backend.c int repack_without_refs(struct ref_store *ref_store, · line 721

Quoted 7 lines

refs/packed-backend.c +1 -1

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

And this is the reason for the earlier "you should be able to clear the packed ref cache without holding the lock" commit, I guess. Makes sense.

-Peff

Michael Haggerty

author

2017-07-01 17:11 UTC #42 lore raw

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

Quoted 11 lines · Jeff King

Quoted 7 lines · Michael Haggerty
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.
This is the sort of clue I was thinking about in my last email. :)

I'll try to be better about that in the future.

And the reason to make packed_ref_store fulfill the ref_store interface is mostly indirect at the moment. It was important (to my sanity, if nothing else) to simplify the interface to the packed-refs code before rewriting its innards. Since the ref_store interface is familiar and is fairly close to what is needed, it seemed logical to use it.

Quoted 5 lines

Quoted 3 lines
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.
s/used to forgot/previously forgot/ or similar?

Thanks; will fix.

Michael

Michael Haggerty

author

2017-06-23 07:03 UTC #43 lore raw

PATCH v2 29/29 read_packed_refs(): die if `packed-refs` contains bogus data

In reply to Michael Haggerty

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 <[email protected]>

---
 refs/packed-backend.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

refs/packed-backend.c +3 -3

index 377c775adb..1cbaf036df 100644
@@ -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);
 		}
 	}

Signature · 1 line

2.11.0

Jeff King

2017-06-23 19:58 UTC #44 lore raw

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

Quoted 3 lines · Michael Haggerty

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.

This seems like a big improvement. Is it worth adding a test for a corrupted file?

I assume this isn't something you saw in the wild, but just a deficiency you noticed while reading the code.

I suspect this laxness may have been what allowed us to add the optional peeled values long ago. But I think I'd rather see us be more strict and notice corruption or nonsense rather than quietly ignoring (especially because an operation like "git pack-refs" would then overwrite it, dropping whatever entries we didn't understand).

-Peff

Michael Haggerty

author

2017-07-01 18:15 UTC #45 lore raw

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

Quoted 8 lines · Jeff King

Quoted 3 lines · Michael Haggerty
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.
This seems like a big improvement. Is it worth adding a test for a corrupted file?

That's a good idea. While I'm at it, I'll distinguish between unterminated lines and lines that are invalid for other reasons, because the error message should differ for these cases.

Quoted 2 lines

I assume this isn't something you saw in the wild, but just a deficiency you noticed while reading the code.

Correct, I've never seen this problem in the wild. Though, since Git would have covered up the problem while it existed and obliterated it at the next pack-refs, it's the kind of thing that would be easy to overlook and hard to prove afterwards.

Quoted 5 lines

I suspect this laxness may have been what allowed us to add the optional peeled values long ago. But I think I'd rather see us be more strict and notice corruption or nonsense rather than quietly ignoring (especially because an operation like "git pack-refs" would then overwrite it, dropping whatever entries we didn't understand).

That's an interesting consideration. I suppose we could plan in some way to extend the packed-refs format in a backwards-transparent fashion, if there is ever an extension that old versions of git would be free to ignore, while leaving the format strict enough that genuine corruption would be unlikely to be overlooked. For example, we could reserve one or more special characters which, when they appear at the beginning of a line, mean that the line should be ignored.

But such an extension would only be practical if other Git implementations are similarly lax. But libgit2 is currently strict about rejecting unrecognized lines, so such an extension couldn't be backwards-compatible with old versions of libgit2. So it doesn't seem very useful.

Michael

Jeff King

2017-06-23 19:02 UTC #46 lore raw

In reply to Michael Haggerty

Quoted 6 lines · Michael Haggerty

* Change patch 17 "packed_ref_store: support iteration" to always iterate over the packed refs using DO_FOR_EACH_INCLUDE_BROKEN. This switches off the check in the packed-ref iterator of whether a reference is broken. This is now checked only in files_ref_iterator_advance(), after the packed and loose references have been merged together. It also saves some work.

I'm curious why you prefer this solution to just removing the code entirely. Wouldn't it be an error to call the packed ref iterator without INCLUDE_BROKEN? The "entries may not be valid" thing is a property of the packed-refs concept itself, not a particular caller's view of it.

-Peff

Jeff King

2017-06-23 20:00 UTC #47 lore raw

Quoted 9 lines · Jeff King

Quoted 6 lines · Michael Haggerty
* Change patch 17 "packed_ref_store: support iteration" to always iterate over the packed refs using DO_FOR_EACH_INCLUDE_BROKEN. This switches off the check in the packed-ref iterator of whether a reference is broken. This is now checked only in files_ref_iterator_advance(), after the packed and loose references have been merged together. It also saves some work.

Quoted 5 lines

I'm curious why you prefer this solution to just removing the code entirely. Wouldn't it be an error to call the packed ref iterator without INCLUDE_BROKEN? The "entries may not be valid" thing is a property of the packed-refs concept itself, not a particular caller's view of it.

Speculating on my own question. I guess it would prepare us for a day when a possible ref store is to use a packed-refs _without_ loose refs. IOW, the property is defined on packed-refs today, but a possible future direction would be to use it by itself. But maybe I'm just making things up.

At any rate, I've read through the whole series and dropped a few comments in there. Overall it looks good. If I had one complaint, it's that the individual commits all look obviously correct but it is hard to judge whether the bigger steps they are taking are the right thing. I mostly have faith in you, as I know that your end goal is a good one, and that you're very familiar with this code. But just something I noticed while reviewing.

-Peff

Junio C Hamano

2017-06-23 21:47 UTC #48 lore raw

Quoted 17 lines · Jeff King

Quoted 9 lines · Jeff King
Quoted 6 lines · Michael Haggerty
* Change patch 17 "packed_ref_store: support iteration" to always iterate over the packed refs using DO_FOR_EACH_INCLUDE_BROKEN. This switches off the check in the packed-ref iterator of whether a reference is broken. This is now checked only in files_ref_iterator_advance(), after the packed and loose references have been merged together. It also saves some work.
Quoted 5 lines
I'm curious why you prefer this solution to just removing the code entirely. Wouldn't it be an error to call the packed ref iterator without INCLUDE_BROKEN? The "entries may not be valid" thing is a property of the packed-refs concept itself, not a particular caller's view of it.

Quoted 5 lines

Speculating on my own question. I guess it would prepare us for a day when a possible ref store is to use a packed-refs _without_ loose refs. IOW, the property is defined on packed-refs today, but a possible future direction would be to use it by itself. But maybe I'm just making things up.

OK. In other words, it's not a packed-refs's characteristics that cruft are allowed. It's that a ref storage that is implemented as an overlay of one storage (which happens to be the loose one) on top of another (which happens to be the packed refs file) allows the latter one to have cruft if (and only if) that broken one is covered by the former one.

Quoted 7 lines

At any rate, I've read through the whole series and dropped a few comments in there. Overall it looks good. If I had one complaint, it's that the individual commits all look obviously correct but it is hard to judge whether the bigger steps they are taking are the right thing. I mostly have faith in you, as I know that your end goal is a good one, and that you're very familiar with this code. But just something I noticed while reviewing.

Thanks.

Jeff King

2017-06-24 01:11 UTC #49 lore raw

Quoted 6 lines · Junio C Hamano

Quoted 5 lines
Speculating on my own question. I guess it would prepare us for a day when a possible ref store is to use a packed-refs _without_ loose refs. IOW, the property is defined on packed-refs today, but a possible future direction would be to use it by itself. But maybe I'm just making things up.

Quoted 6 lines

OK. In other words, it's not a packed-refs's characteristics that cruft are allowed. It's that a ref storage that is implemented as an overlay of one storage (which happens to be the loose one) on top of another (which happens to be the packed refs file) allows the latter one to have cruft if (and only if) that broken one is covered by the former one.

Thanks, that's a much better way of saying what I was trying to get at. I don't know if that's Michael's argument or not, but it's certainly one I find reasonable. :)

-Peff

Michael Haggerty

author

2017-06-24 11:14 UTC #50 lore raw

Quoted 15 lines · Jeff King

Quoted 6 lines · Junio C Hamano
Quoted 5 lines
Speculating on my own question. I guess it would prepare us for a day when a possible ref store is to use a packed-refs _without_ loose refs. IOW, the property is defined on packed-refs today, but a possible future direction would be to use it by itself. But maybe I'm just making things up.
Quoted 6 lines
OK. In other words, it's not a packed-refs's characteristics that cruft are allowed. It's that a ref storage that is implemented as an overlay of one storage (which happens to be the loose one) on top of another (which happens to be the packed refs file) allows the latter one to have cruft if (and only if) that broken one is covered by the former one.

Quoted 3 lines

Thanks, that's a much better way of saying what I was trying to get at. I don't know if that's Michael's argument or not, but it's certainly one I find reasonable. :)

That was exactly my thinking. A packed-without-loose storage scheme might, for example, be interesting for people with case-insensitive or strangely-Unicode-normalized filesystems but have colleagues who like to use case or Unicode in their reference names. (Of course that would still require a way to store symbolic refs and reflogs, so I'm not saying that we're there yet.)

I also think it is a good idea to keep the backends' interfaces as similar as possible to reduce the number of quirks that the reader has to keep in mind.

Michael

Junio C Hamano

2017-06-23 20:25 UTC #51 lore raw

In reply to Jeff King

Quoted 9 lines · Jeff King

Quoted 6 lines · Michael Haggerty
* Change patch 17 "packed_ref_store: support iteration" to always iterate over the packed refs using DO_FOR_EACH_INCLUDE_BROKEN. This switches off the check in the packed-ref iterator of whether a reference is broken. This is now checked only in files_ref_iterator_advance(), after the packed and loose references have been merged together. It also saves some work.

Quoted 5 lines

I'm curious why you prefer this solution to just removing the code entirely. Wouldn't it be an error to call the packed ref iterator without INCLUDE_BROKEN? The "entries may not be valid" thing is a property of the packed-refs concept itself, not a particular caller's view of it.

Thanks for pointing it out. I was wondering about the same thing and you phrased it a lot more succinctly than the draft I was writing.