[PATCH v2 00/33] Various cleanups around reference packing and peeling

[PATCH v2 00/33] Various cleanups around reference packing and peeling

[Michael Haggerty]

Thanks for the feedback; here is a re-roll.  A number of points
discussed on the mailing list were fixed.  The main change, in patch
17, is how repack_without_ref() deals with references that cannot be
peeled when re-writing the packed-refs file:

if ISBROKEN:
    emit an error and omit reference from the output
else if !has_sha1_file(...):
    if there is an overriding loose reference:
        silently omit reference from the output
    else:
        emit an error and omit reference from the output

Please note that this creates a relatively harmless race condition
very similar to the ones discussed for pack-refs; see the commit
message for patch 17 for a long explanation.  I would like to fix all
of the races as part of a separate patch series.

For now I left the sleeps in t3210.  Given that the problem will be
solved by topic jc/prune-all, building a fancier workaround into this
test for the old broken --expire behavior seems like a waste of time.
I propose that the sleeps be removed when this patch series is merged
with jc/prune-all.  (In fact, when jc/prune-all is landed, other tests
can also be simplified.)  If this suggestion is not ok, then the
easiest thing would probably be to remove the sleeps immediately and
declare jc/prune-all a prerequisite of this series.

I also removed the trailing comma from the "enum peel_status"
definition, because a recent email on the mailing list claimed that
some compilers don't like them.

Michael Haggerty (33):
  refs: document flags constants REF_*
  refs: document the fields of struct ref_value
  refs: document do_for_each_ref() and do_one_ref()
  refs: document how current_ref is used
  refs: define constant PEELED_LINE_LENGTH
  do_for_each_ref_in_dirs(): remove dead code
  get_packed_ref(): return a ref_entry
  peel_ref(): use function get_packed_ref()
  repack_without_ref(): use function get_packed_ref()
  refs: extract a function ref_resolves_to_object()
  refs: extract function peel_object()
  peel_object(): give more specific information in return value
  peel_ref(): fix return value for non-peelable, not-current reference
  refs: extract a function peel_entry()
  refs: change the internal reference-iteration API
  t3210: test for spurious error messages for dangling packed refs
  repack_without_ref(): silence errors for dangling packed refs
  search_ref_dir(): return an index rather than a pointer
  refs: change how packed refs are deleted
  t3211: demonstrate loss of peeled refs if a packed ref is deleted
  repack_without_ref(): write peeled refs in the rewritten file
  refs: extract a function write_packed_entry()
  pack-refs: rename handle_one_ref() to pack_one_ref()
  pack-refs: merge code from pack-refs.{c,h} into refs.{c,h}
  pack_one_ref(): rename "path" parameter to "refname"
  refs: use same lock_file object for both ref-packing functions
  pack_refs(): change to use do_for_each_entry()
  refs: inline function do_not_prune()
  pack_one_ref(): use function peel_entry()
  pack_one_ref(): use write_packed_entry() to do the writing
  pack_one_ref(): do some cheap tests before a more expensive one
  refs: change do_for_each_*() functions to take ref_cache arguments
  refs: handle the main ref_cache specially

 Makefile             |   2 -
 builtin/clone.c      |   1 -
 builtin/pack-refs.c  |   2 +-
 pack-refs.c          | 148 -----------
 pack-refs.h          |  18 --
 refs.c               | 733 +++++++++++++++++++++++++++++++++++++++------------
 refs.h               |  35 +++
 t/t3210-pack-refs.sh |  36 +++
 t/t3211-peel-ref.sh  |   9 +
 9 files changed, 643 insertions(+), 341 deletions(-)
 delete mode 100644 pack-refs.c
 delete mode 100644 pack-refs.h

-- 
1.8.2.1

[PATCH v2 01/33] refs: document flags constants REF_*

[Michael Haggerty]

Document the bits that can appear in the "flags" parameter passed to
an each_ref_function and/or in the ref_entry::flag field.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 12 +++++++++++-
 refs.h | 13 +++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index de2d8eb..30b4bf7 100644
--- a/refs.c
+++ b/refs.c
@@ -158,7 +158,17 @@ struct ref_dir {
 	struct ref_entry **entries;
 };
 
-/* ISSYMREF=0x01, ISPACKED=0x02, and ISBROKEN=0x04 are public interfaces */
+/*
+ * Bit values for ref_entry::flag.  REF_ISSYMREF=0x01,
+ * REF_ISPACKED=0x02, and REF_ISBROKEN=0x04 are public values; see
+ * refs.h.
+ */
+
+/*
+ * The field ref_entry->u.value.peeled of this value entry contains
+ * the correct peeled value for the reference, which might be
+ * null_sha1 if the reference is not a tag or if it is broken.
+ */
 #define REF_KNOWS_PEELED 0x08
 
 /* ref_entry represents a directory of references */
diff --git a/refs.h b/refs.h
index a35eafc..17bc1c1 100644
--- a/refs.h
+++ b/refs.h
@@ -10,8 +10,21 @@ struct ref_lock {
 	int force_write;
 };
 
+/*
+ * Bit values set in the flags argument passed to each_ref_fn():
+ */
+
+/* Reference is a symbolic reference. */
 #define REF_ISSYMREF 0x01
+
+/* Reference is a packed reference. */
 #define REF_ISPACKED 0x02
+
+/*
+ * Reference cannot be resolved to an object name: dangling symbolic
+ * reference (directly or indirectly), corrupt reference file, or
+ * symbolic reference refers to ill-formatted reference name.
+ */
 #define REF_ISBROKEN 0x04
 
 /*
-- 
1.8.2.1

[PATCH v2 02/33] refs: document the fields of struct ref_value

[Michael Haggerty]

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

diff --git a/refs.c b/refs.c
index 30b4bf7..1df1ccd 100644
--- a/refs.c
+++ b/refs.c
@@ -109,7 +109,19 @@ struct ref_entry;
  * (ref_entry->flag & REF_DIR) is zero.
  */
 struct ref_value {
+	/*
+	 * The name of the object to which this reference resolves
+	 * (which may be a tag object).  If REF_ISBROKEN, this is
+	 * null.  If REF_ISSYMREF, then this is the name of the object
+	 * referred to by the last reference in the symlink chain.
+	 */
 	unsigned char sha1[20];
+
+	/*
+	 * If REF_KNOWS_PEELED, then this field holds the peeled value
+	 * of this reference, or null if the reference is known not to
+	 * be peelable.
+	 */
 	unsigned char peeled[20];
 };
 
-- 
1.8.2.1

[PATCH v2 03/33] refs: document do_for_each_ref() and do_one_ref()

[Michael Haggerty]

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

diff --git a/refs.c b/refs.c
index 1df1ccd..44cc2fc 100644
--- a/refs.c
+++ b/refs.c
@@ -525,10 +525,14 @@ static void sort_ref_dir(struct ref_dir *dir)
 	dir->sorted = dir->nr = i;
 }
 
-#define DO_FOR_EACH_INCLUDE_BROKEN 01
+/* Include broken references in a do_for_each_ref*() iteration: */
+#define DO_FOR_EACH_INCLUDE_BROKEN 0x01
 
 static struct ref_entry *current_ref;
 
+/*
+ * Handle one reference in a do_for_each_ref*()-style iteration.
+ */
 static int do_one_ref(const char *base, each_ref_fn fn, int trim,
 		      int flags, void *cb_data, struct ref_entry *entry)
 {
@@ -1338,6 +1342,15 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)
 	for_each_rawref(warn_if_dangling_symref, &data);
 }
 
+/*
+ * Call fn for each reference in the specified submodule for which the
+ * refname begins with base.  If trim is non-zero, then trim that many
+ * characters off the beginning of each refname before passing the
+ * refname to fn.  flags can be DO_FOR_EACH_INCLUDE_BROKEN to include
+ * broken references in the iteration.  If fn ever returns a non-zero
+ * value, stop the iteration and return that value; otherwise, return
+ * 0.
+ */
 static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,
 			   int trim, int flags, void *cb_data)
 {
-- 
1.8.2.1

[PATCH v2 04/33] refs: document how current_ref is used

[Michael Haggerty]

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

diff --git a/refs.c b/refs.c
index 44cc2fc..efad658 100644
--- a/refs.c
+++ b/refs.c
@@ -528,6 +528,15 @@ static void sort_ref_dir(struct ref_dir *dir)
 /* Include broken references in a do_for_each_ref*() iteration: */
 #define DO_FOR_EACH_INCLUDE_BROKEN 0x01
 
+/*
+ * current_ref is a performance hack: when iterating over references
+ * using the for_each_ref*() functions, current_ref is set to the
+ * current reference's entry before calling the callback function.  If
+ * the callback function calls peel_ref(), then peel_ref() first
+ * checks whether the reference to be peeled is the current reference
+ * (it usually is) and if so, returns that reference's peeled version
+ * if it is available.  This avoids a refname lookup in a common case.
+ */
 static struct ref_entry *current_ref;
 
 /*
-- 
1.8.2.1

[PATCH v2 05/33] refs: define constant PEELED_LINE_LENGTH

[Michael Haggerty]

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

diff --git a/refs.c b/refs.c
index efad658..e1e9ddd 100644
--- a/refs.c
+++ b/refs.c
@@ -805,6 +805,9 @@ void invalidate_ref_cache(const char *submodule)
 	clear_loose_ref_cache(refs);
 }
 
+/* The length of a peeled reference line in packed-refs, including EOL: */
+#define PEELED_LINE_LENGTH 42
+
 /*
  * Parse one line from a packed-refs file.  Write the SHA1 to sha1.
  * Return a pointer to the refname within the line (null-terminated),
@@ -897,8 +900,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
 		}
 		if (last &&
 		    refline[0] == '^' &&
-		    strlen(refline) == 42 &&
-		    refline[41] == '\n' &&
+		    strlen(refline) == PEELED_LINE_LENGTH &&
+		    refline[PEELED_LINE_LENGTH - 1] == '\n' &&
 		    !get_sha1_hex(refline + 1, sha1)) {
 			hashcpy(last->u.value.peeled, sha1);
 			/*
-- 
1.8.2.1

[PATCH v2 06/33] do_for_each_ref_in_dirs(): remove dead code

[Michael Haggerty]

There is no way to drop out of the while loop.  This code has been
dead since 432ad41e.

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

diff --git a/refs.c b/refs.c
index e1e9ddd..7260768 100644
--- a/refs.c
+++ b/refs.c
@@ -666,13 +666,6 @@ static int do_for_each_ref_in_dirs(struct ref_dir *dir1,
 		if (retval)
 			return retval;
 	}
-	if (i1 < dir1->nr)
-		return do_for_each_ref_in_dir(dir1, i1,
-					      base, fn, trim, flags, cb_data);
-	if (i2 < dir2->nr)
-		return do_for_each_ref_in_dir(dir2, i2,
-					      base, fn, trim, flags, cb_data);
-	return 0;
 }
 
 /*
-- 
1.8.2.1

[PATCH v2 07/33] get_packed_ref(): return a ref_entry

[Michael Haggerty]

Instead of copying the reference's SHA1 into a caller-supplied
variable, just return the ref_entry itself (or NULL if there is no
such entry).  This change will allow the function to be used from
elsewhere.

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

diff --git a/refs.c b/refs.c
index 7260768..91a2940 100644
--- a/refs.c
+++ b/refs.c
@@ -1100,18 +1100,12 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh
 }
 
 /*
- * Try to read ref from the packed references.  On success, set sha1
- * and return 0; otherwise, return -1.
+ * Return the ref_entry for the given refname from the packed
+ * references.  If it does not exist, return NULL.
  */
-static int get_packed_ref(const char *refname, unsigned char *sha1)
+static struct ref_entry *get_packed_ref(const char *refname)
 {
-	struct ref_dir *packed = get_packed_refs(get_ref_cache(NULL));
-	struct ref_entry *entry = find_ref(packed, refname);
-	if (entry) {
-		hashcpy(sha1, entry->u.value.sha1);
-		return 0;
-	}
-	return -1;
+	return find_ref(get_packed_refs(get_ref_cache(NULL)), refname);
 }
 
 const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)
@@ -1139,13 +1133,17 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		git_snpath(path, sizeof(path), "%s", refname);
 
 		if (lstat(path, &st) < 0) {
+			struct ref_entry *entry;
+
 			if (errno != ENOENT)
 				return NULL;
 			/*
 			 * The loose reference file does not exist;
 			 * check for a packed reference.
 			 */
-			if (!get_packed_ref(refname, sha1)) {
+			entry = get_packed_ref(refname);
+			if (entry) {
+				hashcpy(sha1, entry->u.value.sha1);
 				if (flag)
 					*flag |= REF_ISPACKED;
 				return refname;
-- 
1.8.2.1

[PATCH v2 08/33] peel_ref(): use function get_packed_ref()

[Michael Haggerty]

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

diff --git a/refs.c b/refs.c
index 91a2940..09b68e4 100644
--- a/refs.c
+++ b/refs.c
@@ -1282,10 +1282,9 @@ int peel_ref(const char *refname, unsigned char *sha1)
 		return -1;
 
 	if ((flag & REF_ISPACKED)) {
-		struct ref_dir *dir = get_packed_refs(get_ref_cache(NULL));
-		struct ref_entry *r = find_ref(dir, refname);
+		struct ref_entry *r = get_packed_ref(refname);
 
-		if (r != NULL && r->flag & REF_KNOWS_PEELED) {
+		if (r && (r->flag & REF_KNOWS_PEELED)) {
 			hashcpy(sha1, r->u.value.peeled);
 			return 0;
 		}
-- 
1.8.2.1

[PATCH v2 09/33] repack_without_ref(): use function get_packed_ref()

[Michael Haggerty]

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

diff --git a/refs.c b/refs.c
index 09b68e4..1c8edb1 100644
--- a/refs.c
+++ b/refs.c
@@ -1820,9 +1820,11 @@ static int repack_without_ref(const char *refname)
 {
 	struct repack_without_ref_sb data;
 	struct ref_cache *refs = get_ref_cache(NULL);
-	struct ref_dir *packed = get_packed_refs(refs);
-	if (find_ref(packed, refname) == NULL)
-		return 0;
+	struct ref_dir *packed;
+
+	if (!get_packed_ref(refname))
+		return 0; /* refname does not exist in packed refs */
+
 	data.refname = refname;
 	data.fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0);
 	if (data.fd < 0) {
-- 
1.8.2.1

[PATCH v2 10/33] refs: extract a function ref_resolves_to_object()

[Michael Haggerty]

It is a nice unit of work and soon will be needed from multiple
locations.

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

diff --git a/refs.c b/refs.c
index 1c8edb1..8b554d8 100644
--- a/refs.c
+++ b/refs.c
@@ -529,6 +529,22 @@ static void sort_ref_dir(struct ref_dir *dir)
 #define DO_FOR_EACH_INCLUDE_BROKEN 0x01
 
 /*
+ * Return true iff the reference described by entry can be resolved to
+ * an object in the database.  Emit a warning if the referred-to
+ * object does not exist.
+ */
+static int ref_resolves_to_object(struct ref_entry *entry)
+{
+	if (entry->flag & REF_ISBROKEN)
+		return 0;
+	if (!has_sha1_file(entry->u.value.sha1)) {
+		error("%s does not point to a valid object!", entry->name);
+		return 0;
+	}
+	return 1;
+}
+
+/*
  * current_ref is a performance hack: when iterating over references
  * using the for_each_ref*() functions, current_ref is set to the
  * current reference's entry before calling the callback function.  If
@@ -549,14 +565,10 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,
 	if (prefixcmp(entry->name, base))
 		return 0;
 
-	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {
-		if (entry->flag & REF_ISBROKEN)
-			return 0; /* ignore broken refs e.g. dangling symref */
-		if (!has_sha1_file(entry->u.value.sha1)) {
-			error("%s does not point to a valid object!", entry->name);
-			return 0;
-		}
-	}
+	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
+	      !ref_resolves_to_object(entry))
+		return 0;
+
 	current_ref = entry;
 	retval = fn(entry->name + trim, entry->u.value.sha1, entry->flag, cb_data);
 	current_ref = NULL;
-- 
1.8.2.1

[PATCH v2 11/33] refs: extract function peel_object()

[Michael Haggerty]

It is a nice, logical unit of work, and putting it in a function
removes the need to use a goto in peel_ref().  Soon it will also have
other uses.

The algorithm is unchanged.

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

diff --git a/refs.c b/refs.c
index 8b554d8..b0ef129 100644
--- a/refs.c
+++ b/refs.c
@@ -1272,11 +1272,38 @@ static int filter_refs(const char *refname, const unsigned char *sha1, int flags
 	return filter->fn(refname, sha1, flags, filter->cb_data);
 }
 
+/*
+ * Peel the named object; i.e., if the object is a tag, resolve the
+ * tag recursively until a non-tag is found.  Store the result to sha1
+ * and return 0 iff successful.  If the object is not a tag or is not
+ * valid, return -1 and leave sha1 unchanged.
+ */
+static int peel_object(const unsigned char *name, unsigned char *sha1)
+{
+	struct object *o = lookup_unknown_object(name);
+
+	if (o->type == OBJ_NONE) {
+		int type = sha1_object_info(name, NULL);
+		if (type < 0)
+			return -1;
+		o->type = type;
+	}
+
+	if (o->type != OBJ_TAG)
+		return -1;
+
+	o = deref_tag_noverify(o);
+	if (!o)
+		return -1;
+
+	hashcpy(sha1, o->sha1);
+	return 0;
+}
+
 int peel_ref(const char *refname, unsigned char *sha1)
 {
 	int flag;
 	unsigned char base[20];
-	struct object *o;
 
 	if (current_ref && (current_ref->name == refname
 		|| !strcmp(current_ref->name, refname))) {
@@ -1286,8 +1313,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
 			hashcpy(sha1, current_ref->u.value.peeled);
 			return 0;
 		}
-		hashcpy(base, current_ref->u.value.sha1);
-		goto fallback;
+		return peel_object(current_ref->u.value.sha1, sha1);
 	}
 
 	if (read_ref_full(refname, base, 1, &flag))
@@ -1302,23 +1328,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
 		}
 	}
 
-fallback:
-	o = lookup_unknown_object(base);
-	if (o->type == OBJ_NONE) {
-		int type = sha1_object_info(base, NULL);
-		if (type < 0)
-			return -1;
-		o->type = type;
-	}
-
-	if (o->type == OBJ_TAG) {
-		o = deref_tag_noverify(o);
-		if (o) {
-			hashcpy(sha1, o->sha1);
-			return 0;
-		}
-	}
-	return -1;
+	return peel_object(base, sha1);
 }
 
 struct warn_if_dangling_data {
-- 
1.8.2.1

[PATCH v2 12/33] peel_object(): give more specific information in return value

[Michael Haggerty]

Instead of just returning a success/failure bit, return an enumeration
value that explains the reason for any failure.  This will come in
handy shortly.

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

diff --git a/refs.c b/refs.c
index b0ef129..d26e847 100644
--- a/refs.c
+++ b/refs.c
@@ -1272,32 +1272,48 @@ static int filter_refs(const char *refname, const unsigned char *sha1, int flags
 	return filter->fn(refname, sha1, flags, filter->cb_data);
 }
 
+enum peel_status {
+	/* object was peeled successfully: */
+	PEEL_PEELED = 0,
+
+	/*
+	 * object cannot be peeled because the named object (or an
+	 * object referred to by a tag in the peel chain), does not
+	 * exist.
+	 */
+	PEEL_INVALID = -1,
+
+	/* object cannot be peeled because it is not a tag: */
+	PEEL_NON_TAG = -2
+};
+
 /*
  * Peel the named object; i.e., if the object is a tag, resolve the
- * tag recursively until a non-tag is found.  Store the result to sha1
- * and return 0 iff successful.  If the object is not a tag or is not
- * valid, return -1 and leave sha1 unchanged.
+ * tag recursively until a non-tag is found.  If successful, store the
+ * result to sha1 and return PEEL_PEELED.  If the object is not a tag
+ * or is not valid, return PEEL_NON_TAG or PEEL_INVALID, respectively,
+ * and leave sha1 unchanged.
  */
-static int peel_object(const unsigned char *name, unsigned char *sha1)
+static enum peel_status peel_object(const unsigned char *name, unsigned char *sha1)
 {
 	struct object *o = lookup_unknown_object(name);
 
 	if (o->type == OBJ_NONE) {
 		int type = sha1_object_info(name, NULL);
 		if (type < 0)
-			return -1;
+			return PEEL_INVALID;
 		o->type = type;
 	}
 
 	if (o->type != OBJ_TAG)
-		return -1;
+		return PEEL_NON_TAG;
 
 	o = deref_tag_noverify(o);
 	if (!o)
-		return -1;
+		return PEEL_INVALID;
 
 	hashcpy(sha1, o->sha1);
-	return 0;
+	return PEEL_PEELED;
 }
 
 int peel_ref(const char *refname, unsigned char *sha1)
-- 
1.8.2.1

[PATCH v2 13/33] peel_ref(): fix return value for non-peelable, not-current reference

[Michael Haggerty]

The old version was inconsistent: when a reference was
REF_KNOWS_PEELED but with a null peeled value, it returned non-zero
for the current reference but zero for other references.  Change the
behavior for non-current references to match that of current_ref,
which is what callers expect.  Document the behavior.

Current callers only call peel_ref() from within a for_each_ref-style
iteration and only for the current ref; therefore, the buggy code path
was never reached.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 5 ++++-
 refs.h | 8 ++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index d26e847..2f73189 100644
--- a/refs.c
+++ b/refs.c
@@ -120,7 +120,8 @@ struct ref_value {
 	/*
 	 * If REF_KNOWS_PEELED, then this field holds the peeled value
 	 * of this reference, or null if the reference is known not to
-	 * be peelable.
+	 * be peelable.  See the documentation for peel_ref() for an
+	 * exact definition of "peelable".
 	 */
 	unsigned char peeled[20];
 };
@@ -1339,6 +1340,8 @@ int peel_ref(const char *refname, unsigned char *sha1)
 		struct ref_entry *r = get_packed_ref(refname);
 
 		if (r && (r->flag & REF_KNOWS_PEELED)) {
+			if (is_null_sha1(r->u.value.peeled))
+			    return -1;
 			hashcpy(sha1, r->u.value.peeled);
 			return 0;
 		}
diff --git a/refs.h b/refs.h
index 17bc1c1..ac0ff92 100644
--- a/refs.h
+++ b/refs.h
@@ -74,6 +74,14 @@ extern void add_packed_ref(const char *refname, const unsigned char *sha1);
 
 extern int ref_exists(const char *);
 
+/*
+ * If refname is a non-symbolic reference that refers to a tag object,
+ * and the tag can be (recursively) dereferenced to a non-tag object,
+ * store the SHA1 of the referred-to object to sha1 and return 0.  If
+ * any of these conditions are not met, return a non-zero value.
+ * Symbolic references are considered unpeelable, even if they
+ * ultimately resolve to a peelable tag.
+ */
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
 /** Locks a "refs/" ref returning the lock on success and NULL on failure. **/
-- 
1.8.2.1

[PATCH v2 14/33] refs: extract a function peel_entry()

[Michael Haggerty]

Peel the entry, and as a side effect store the peeled value in the
entry.  Use this function from two places in peel_ref(); a third
caller will be added soon.

Please note that this change can lead to ref_entries for unpacked refs
being peeled.  This has no practical benefit but is harmless.

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

diff --git a/refs.c b/refs.c
index 2f73189..777a4b7 100644
--- a/refs.c
+++ b/refs.c
@@ -1285,7 +1285,17 @@ enum peel_status {
 	PEEL_INVALID = -1,
 
 	/* object cannot be peeled because it is not a tag: */
-	PEEL_NON_TAG = -2
+	PEEL_NON_TAG = -2,
+
+	/* ref_entry contains no peeled value because it is a symref: */
+	PEEL_IS_SYMREF = -3,
+
+	/*
+	 * ref_entry cannot be peeled because it is broken (i.e., the
+	 * symbolic reference cannot even be resolved to an object
+	 * name):
+	 */
+	PEEL_BROKEN = -4
 };
 
 /*
@@ -1317,31 +1327,56 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh
 	return PEEL_PEELED;
 }
 
+/*
+ * Peel the entry (if possible) and return its new peel_status.
+ */
+static enum peel_status peel_entry(struct ref_entry *entry)
+{
+	enum peel_status status;
+
+	if (entry->flag & REF_KNOWS_PEELED)
+		return is_null_sha1(entry->u.value.peeled) ?
+			PEEL_NON_TAG : PEEL_PEELED;
+	if (entry->flag & REF_ISBROKEN)
+		return PEEL_BROKEN;
+	if (entry->flag & REF_ISSYMREF)
+		return PEEL_IS_SYMREF;
+
+	status = peel_object(entry->u.value.sha1, entry->u.value.peeled);
+	if (status == PEEL_PEELED || status == PEEL_NON_TAG)
+		entry->flag |= REF_KNOWS_PEELED;
+	return status;
+}
+
 int peel_ref(const char *refname, unsigned char *sha1)
 {
 	int flag;
 	unsigned char base[20];
 
 	if (current_ref && (current_ref->name == refname
-		|| !strcmp(current_ref->name, refname))) {
-		if (current_ref->flag & REF_KNOWS_PEELED) {
-			if (is_null_sha1(current_ref->u.value.peeled))
-			    return -1;
-			hashcpy(sha1, current_ref->u.value.peeled);
-			return 0;
-		}
-		return peel_object(current_ref->u.value.sha1, sha1);
+			    || !strcmp(current_ref->name, refname))) {
+		if (peel_entry(current_ref))
+			return -1;
+		hashcpy(sha1, current_ref->u.value.peeled);
+		return 0;
 	}
 
 	if (read_ref_full(refname, base, 1, &flag))
 		return -1;
 
-	if ((flag & REF_ISPACKED)) {
+	/*
+	 * If the reference is packed, read its ref_entry from the
+	 * cache in the hope that we already know its peeled value.
+	 * We only try this optimization on packed references because
+	 * (a) forcing the filling of the loose reference cache could
+	 * 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(refname);
-
-		if (r && (r->flag & REF_KNOWS_PEELED)) {
-			if (is_null_sha1(r->u.value.peeled))
-			    return -1;
+		if (r) {
+			if (peel_entry(r))
+				return -1;
 			hashcpy(sha1, r->u.value.peeled);
 			return 0;
 		}
-- 
1.8.2.1

[PATCH v2 15/33] refs: change the internal reference-iteration API

[Michael Haggerty]

Establish an internal API for iterating over references, which gives
the callback functions direct access to the ref_entry structure
describing the reference.  (Do not change the iteration API that is
exposed outside of the module.)

Define a new internal callback signature

   int each_ref_entry_fn(struct ref_entry *entry, void *cb_data)

Change do_for_each_ref_in_dir() and do_for_each_ref_in_dirs() to
accept each_ref_entry_fn callbacks, and rename them to
do_for_each_entry_in_dir() and do_for_each_entry_in_dirs(),
respectively.  Adapt their callers accordingly.

Add a new function do_for_each_entry() analogous to do_for_each_ref()
but using the new callback style.

Change do_one_ref() into an each_ref_entry_fn that does some
bookkeeping and then calls a wrapped each_ref_fn.

Reimplement do_for_each_ref() in terms of do_for_each_entry(), using
do_one_ref() as an adapter.

Please note that the responsibility for setting current_ref remains in
do_one_ref(), which means that current_ref is *not* set when iterating
over references via the new internal API.  This is not a disadvantage,
because current_ref is not needed by callers of the internal API (they
receive a pointer to the current ref_entry anyway).  But more
importantly, this change prevents peel_ref() from returning invalid
results in the following scenario:

When iterating via the external API, the iteration always includes
both packed and loose references, and in particular never presents a
packed ref if there is a loose ref with the same name.  The internal
API, on the other hand, gives the option to iterate over only the
packed references.  During such an iteration, there is no check
whether the packed ref might be hidden by a loose ref of the same
name.  But until now the packed ref was recorded in current_ref during
the iteration.  So if peel_ref() were called with the reference name
corresponding to current ref, it would return the peeled version of
the packed ref even though there might be a loose ref that peels to a
different value.  This scenario doesn't currently occur in the code,
but fix it to prevent things from breaking in a very confusing way in
the future.

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

diff --git a/refs.c b/refs.c
index 777a4b7..ed54ed4 100644
--- a/refs.c
+++ b/refs.c
@@ -556,22 +556,34 @@ static int ref_resolves_to_object(struct ref_entry *entry)
  */
 static struct ref_entry *current_ref;
 
+typedef int each_ref_entry_fn(struct ref_entry *entry, void *cb_data);
+
+struct ref_entry_cb {
+	const char *base;
+	int trim;
+	int flags;
+	each_ref_fn *fn;
+	void *cb_data;
+};
+
 /*
- * Handle one reference in a do_for_each_ref*()-style iteration.
+ * Handle one reference in a do_for_each_ref*()-style iteration,
+ * calling an each_ref_fn for each entry.
  */
-static int do_one_ref(const char *base, each_ref_fn fn, int trim,
-		      int flags, void *cb_data, struct ref_entry *entry)
+static int do_one_ref(struct ref_entry *entry, void *cb_data)
 {
+	struct ref_entry_cb *data = cb_data;
 	int retval;
-	if (prefixcmp(entry->name, base))
+	if (prefixcmp(entry->name, data->base))
 		return 0;
 
-	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
+	if (!(data->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
 	      !ref_resolves_to_object(entry))
 		return 0;
 
 	current_ref = entry;
-	retval = fn(entry->name + trim, entry->u.value.sha1, entry->flag, cb_data);
+	retval = data->fn(entry->name + data->trim, entry->u.value.sha1,
+			  entry->flag, data->cb_data);
 	current_ref = NULL;
 	return retval;
 }
@@ -580,11 +592,11 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,
  * Call fn for each reference in dir that has index in the range
  * offset <= index < dir->nr.  Recurse into subdirectories that are in
  * that index range, sorting them before iterating.  This function
- * does not sort dir itself; it should be sorted beforehand.
+ * does not sort dir itself; it should be sorted beforehand.  fn is
+ * called for all references, including broken ones.
  */
-static int do_for_each_ref_in_dir(struct ref_dir *dir, int offset,
-				  const char *base,
-				  each_ref_fn fn, int trim, int flags, void *cb_data)
+static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,
+				    each_ref_entry_fn fn, void *cb_data)
 {
 	int i;
 	assert(dir->sorted == dir->nr);
@@ -594,10 +606,9 @@ static int do_for_each_ref_in_dir(struct ref_dir *dir, int offset,
 		if (entry->flag & REF_DIR) {
 			struct ref_dir *subdir = get_ref_dir(entry);
 			sort_ref_dir(subdir);
-			retval = do_for_each_ref_in_dir(subdir, 0,
-							base, fn, trim, flags, cb_data);
+			retval = do_for_each_entry_in_dir(subdir, 0, fn, cb_data);
 		} else {
-			retval = do_one_ref(base, fn, trim, flags, cb_data, entry);
+			retval = fn(entry, cb_data);
 		}
 		if (retval)
 			return retval;
@@ -610,12 +621,12 @@ static int do_for_each_ref_in_dir(struct ref_dir *dir, int offset,
  * by refname.  Recurse into subdirectories.  If a value entry appears
  * in both dir1 and dir2, then only process the version that is in
  * dir2.  The input dirs must already be sorted, but subdirs will be
- * sorted as needed.
+ * sorted as needed.  fn is called for all references, including
+ * broken ones.
  */
-static int do_for_each_ref_in_dirs(struct ref_dir *dir1,
-				   struct ref_dir *dir2,
-				   const char *base, each_ref_fn fn, int trim,
-				   int flags, void *cb_data)
+static int do_for_each_entry_in_dirs(struct ref_dir *dir1,
+				     struct ref_dir *dir2,
+				     each_ref_entry_fn fn, void *cb_data)
 {
 	int retval;
 	int i1 = 0, i2 = 0;
@@ -626,12 +637,10 @@ static int do_for_each_ref_in_dirs(struct ref_dir *dir1,
 		struct ref_entry *e1, *e2;
 		int cmp;
 		if (i1 == dir1->nr) {
-			return do_for_each_ref_in_dir(dir2, i2,
-						      base, fn, trim, flags, cb_data);
+			return do_for_each_entry_in_dir(dir2, i2, fn, cb_data);
 		}
 		if (i2 == dir2->nr) {
-			return do_for_each_ref_in_dir(dir1, i1,
-						      base, fn, trim, flags, cb_data);
+			return do_for_each_entry_in_dir(dir1, i1, fn, cb_data);
 		}
 		e1 = dir1->entries[i1];
 		e2 = dir2->entries[i2];
@@ -643,14 +652,13 @@ static int do_for_each_ref_in_dirs(struct ref_dir *dir1,
 				struct ref_dir *subdir2 = get_ref_dir(e2);
 				sort_ref_dir(subdir1);
 				sort_ref_dir(subdir2);
-				retval = do_for_each_ref_in_dirs(
-						subdir1, subdir2,
-						base, fn, trim, flags, cb_data);
+				retval = do_for_each_entry_in_dirs(
+						subdir1, subdir2, fn, cb_data);
 				i1++;
 				i2++;
 			} else if (!(e1->flag & REF_DIR) && !(e2->flag & REF_DIR)) {
 				/* Both are references; ignore the one from dir1. */
-				retval = do_one_ref(base, fn, trim, flags, cb_data, e2);
+				retval = fn(e2, cb_data);
 				i1++;
 				i2++;
 			} else {
@@ -669,11 +677,10 @@ static int do_for_each_ref_in_dirs(struct ref_dir *dir1,
 			if (e->flag & REF_DIR) {
 				struct ref_dir *subdir = get_ref_dir(e);
 				sort_ref_dir(subdir);
-				retval = do_for_each_ref_in_dir(
-						subdir, 0,
-						base, fn, trim, flags, cb_data);
+				retval = do_for_each_entry_in_dir(
+						subdir, 0, fn, cb_data);
 			} else {
-				retval = do_one_ref(base, fn, trim, flags, cb_data, e);
+				retval = fn(e, cb_data);
 			}
 		}
 		if (retval)
@@ -702,14 +709,13 @@ struct name_conflict_cb {
 	const char *conflicting_refname;
 };
 
-static int name_conflict_fn(const char *existingrefname, const unsigned char *sha1,
-			    int flags, void *cb_data)
+static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
 {
 	struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
-	if (data->oldrefname && !strcmp(data->oldrefname, existingrefname))
+	if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
 		return 0;
-	if (names_conflict(data->refname, existingrefname)) {
-		data->conflicting_refname = existingrefname;
+	if (names_conflict(data->refname, entry->name)) {
+		data->conflicting_refname = entry->name;
 		return 1;
 	}
 	return 0;
@@ -717,7 +723,7 @@ static int name_conflict_fn(const char *existingrefname, const unsigned char *sh
 
 /*
  * Return true iff a reference named refname could be created without
- * conflicting with the name of an existing reference in array.  If
+ * conflicting with the name of an existing reference in dir.  If
  * oldrefname is non-NULL, ignore potential conflicts with oldrefname
  * (e.g., because oldrefname is scheduled for deletion in the same
  * operation).
@@ -731,9 +737,7 @@ static int is_refname_available(const char *refname, const char *oldrefname,
 	data.conflicting_refname = NULL;
 
 	sort_ref_dir(dir);
-	if (do_for_each_ref_in_dir(dir, 0, "", name_conflict_fn,
-				   0, DO_FOR_EACH_INCLUDE_BROKEN,
-				   &data)) {
+	if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {
 		error("'%s' exists; cannot create '%s'",
 		      data.conflicting_refname, refname);
 		return 0;
@@ -1421,16 +1425,14 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)
 }
 
 /*
- * Call fn for each reference in the specified submodule for which the
- * refname begins with base.  If trim is non-zero, then trim that many
- * characters off the beginning of each refname before passing the
- * refname to fn.  flags can be DO_FOR_EACH_INCLUDE_BROKEN to include
- * broken references in the iteration.  If fn ever returns a non-zero
+ * Call fn for each reference in the specified submodule, omitting
+ * references not in the containing_dir of base.  fn is called for all
+ * references, including broken ones.  If fn ever returns a non-zero
  * value, stop the iteration and return that value; otherwise, return
  * 0.
  */
-static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,
-			   int trim, int flags, void *cb_data)
+static int do_for_each_entry(const char *submodule, const char *base,
+			     each_ref_entry_fn fn, void *cb_data)
 {
 	struct ref_cache *refs = get_ref_cache(submodule);
 	struct ref_dir *packed_dir = get_packed_refs(refs);
@@ -1445,24 +1447,43 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn
 	if (packed_dir && loose_dir) {
 		sort_ref_dir(packed_dir);
 		sort_ref_dir(loose_dir);
-		retval = do_for_each_ref_in_dirs(
-				packed_dir, loose_dir,
-				base, fn, trim, flags, cb_data);
+		retval = do_for_each_entry_in_dirs(
+				packed_dir, loose_dir, fn, cb_data);
 	} else if (packed_dir) {
 		sort_ref_dir(packed_dir);
-		retval = do_for_each_ref_in_dir(
-				packed_dir, 0,
-				base, fn, trim, flags, cb_data);
+		retval = do_for_each_entry_in_dir(
+				packed_dir, 0, fn, cb_data);
 	} else if (loose_dir) {
 		sort_ref_dir(loose_dir);
-		retval = do_for_each_ref_in_dir(
-				loose_dir, 0,
-				base, fn, trim, flags, cb_data);
+		retval = do_for_each_entry_in_dir(
+				loose_dir, 0, fn, cb_data);
 	}
 
 	return retval;
 }
 
+/*
+ * Call fn for each reference in the specified submodule for which the
+ * refname begins with base.  If trim is non-zero, then trim that many
+ * characters off the beginning of each refname before passing the
+ * refname to fn.  flags can be DO_FOR_EACH_INCLUDE_BROKEN to include
+ * broken references in the iteration.  If fn ever returns a non-zero
+ * value, stop the iteration and return that value; otherwise, return
+ * 0.
+ */
+static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,
+			   int trim, int flags, void *cb_data)
+{
+	struct ref_entry_cb data;
+	data.base = base;
+	data.trim = trim;
+	data.flags = flags;
+	data.fn = fn;
+	data.cb_data = cb_data;
+
+	return do_for_each_entry(submodule, base, do_one_ref, &data);
+}
+
 static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
 	unsigned char sha1[20];
@@ -1872,20 +1893,21 @@ struct repack_without_ref_sb {
 	int fd;
 };
 
-static int repack_without_ref_fn(const char *refname, const unsigned char *sha1,
-				 int flags, void *cb_data)
+static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)
 {
 	struct repack_without_ref_sb *data = cb_data;
 	char line[PATH_MAX + 100];
 	int len;
 
-	if (!strcmp(data->refname, refname))
+	if (!strcmp(data->refname, entry->name))
 		return 0;
+	if (!ref_resolves_to_object(entry))
+		return 0; /* Skip broken refs */
 	len = snprintf(line, sizeof(line), "%s %s\n",
-		       sha1_to_hex(sha1), refname);
+		       sha1_to_hex(entry->u.value.sha1), entry->name);
 	/* this should not happen but just being defensive */
 	if (len > sizeof(line))
-		die("too long a refname '%s'", refname);
+		die("too long a refname '%s'", entry->name);
 	write_or_die(data->fd, line, len);
 	return 0;
 }
@@ -1909,7 +1931,7 @@ static int repack_without_ref(const char *refname)
 	}
 	clear_packed_ref_cache(refs);
 	packed = get_packed_refs(refs);
-	do_for_each_ref_in_dir(packed, 0, "", repack_without_ref_fn, 0, 0, &data);
+	do_for_each_entry_in_dir(packed, 0, repack_without_ref_fn, &data);
 	return commit_lock_file(&packlock);
 }
 
-- 
1.8.2.1

[PATCH v2 16/33] t3210: test for spurious error messages for dangling packed refs

[Michael Haggerty]

A packed reference can be overridden by a loose reference, in which
case the packed reference is obsolete and is never used.  The object
pointed to by such a reference can be garbage collected.  Since
d66da478f2, this could lead to the emission of a spurious error
message:

    error: refs/heads/master does not point to a valid object!

The error is generated by repack_without_ref() if there is an obsolete
dangling packed reference in packed-refs when the packed-refs file has
to be rewritten due to the deletion of another packed reference.  Add
a failing test demonstrating this problem and some passing tests of
related scenarios.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3210-pack-refs.sh | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index cd04361..c032d88 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -118,4 +118,40 @@ test_expect_success 'pack, prune and repack' '
 	test_cmp all-of-them again
 '
 
+test_expect_success 'explicit pack-refs with dangling packed reference' '
+	git commit --allow-empty -m "soon to be garbage-collected" &&
+	git pack-refs --all &&
+	git reset --hard HEAD^ &&
+	sleep 1 &&
+	git reflog expire --expire=now --all &&
+	git prune --expire=now &&
+	git pack-refs --all 2>result &&
+	test_cmp /dev/null result
+'
+
+test_expect_success 'delete ref with dangling packed version' '
+	git checkout -b lamb &&
+	git commit --allow-empty -m "future garbage" &&
+	git pack-refs --all &&
+	git reset --hard HEAD^ &&
+	git checkout master &&
+	sleep 1 &&
+	git reflog expire --expire=now --all &&
+	git prune --expire=now &&
+	git branch -d lamb 2>result &&
+	test_cmp /dev/null result
+'
+
+test_expect_failure 'delete ref while another dangling packed ref' '
+	git branch lamb &&
+	git commit --allow-empty -m "future garbage" &&
+	git pack-refs --all &&
+	git reset --hard HEAD^ &&
+	sleep 1 &&
+	git reflog expire --expire=now --all &&
+	git prune --expire=now &&
+	git branch -d lamb 2>result &&
+	test_cmp /dev/null result
+'
+
 test_done
-- 
1.8.2.1

[PATCH v2 17/33] repack_without_ref(): silence errors for dangling packed refs

[Michael Haggerty]

Stop emitting an error message when deleting a packed reference if we
find another dangling packed reference that is overridden by a loose
reference.  See the previous commit for a longer explanation of the
issue.

We have to be careful to make sure that the invalid packed reference
really *is* overridden by a loose reference; otherwise what we have
found is repository corruption, which we *should* report.

Please note that this approach is vulnerable to a race condition
similar to the race conditions already known to affect packed
references [1]:

* Process 1 tries to peel packed reference X as part of deleting
  another packed reference.  It discovers that X does not refer to a
  valid object (because the object that it referred to has been
  garbage collected).

* Process 2 tries to delete reference X.  It starts by deleting the
  loose reference X.

* Process 1 checks whether there is a loose reference X.  There is not
  (it has just been deleted by process 2), so process 1 reports a
  spurious error "X does not point to a valid object!"

The worst case seems relatively harmless, and the fix is identical to
the fix that will be needed for the other race conditions (namely
holding a lock on the packed-refs file during *all* reference
deletions), so we leave the cleaning up of all of them as a future
project.

[1] http://thread.gmane.org/gmane.comp.version-control.git/211956

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               | 37 +++++++++++++++++++++++++++++++++++--
 t/t3210-pack-refs.sh |  2 +-
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index ed54ed4..2957f6d 100644
--- a/refs.c
+++ b/refs.c
@@ -1901,8 +1901,41 @@ static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)
 
 	if (!strcmp(data->refname, entry->name))
 		return 0;
-	if (!ref_resolves_to_object(entry))
-		return 0; /* Skip broken refs */
+	if (entry->flag & REF_ISBROKEN) {
+		/* This shouldn't happen to packed refs. */
+		error("%s is broken!", entry->name);
+		return 0;
+	}
+	if (!has_sha1_file(entry->u.value.sha1)) {
+		unsigned char sha1[20];
+		int flags;
+
+		if (read_ref_full(entry->name, sha1, 0, &flags))
+			/* We should at least have found the packed ref. */
+			die("Internal error");
+		if ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED))
+			/*
+			 * This packed reference is overridden by a
+			 * loose reference, so it is OK that its value
+			 * is no longer valid; for example, it might
+			 * refer to an object that has been garbage
+			 * collected.  For this purpose we don't even
+			 * care whether the loose reference itself is
+			 * invalid, broken, symbolic, etc.  Silently
+			 * omit the packed reference from the output.
+			 */
+			return 0;
+		/*
+		 * There is no overriding loose reference, so the fact
+		 * that this reference doesn't refer to a valid object
+		 * indicates some kind of repository corruption.
+		 * Report the problem, then omit the reference from
+		 * the output.
+		 */
+		error("%s does not point to a valid object!", entry->name);
+		return 0;
+	}
+
 	len = snprintf(line, sizeof(line), "%s %s\n",
 		       sha1_to_hex(entry->u.value.sha1), entry->name);
 	/* this should not happen but just being defensive */
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index c032d88..559f602 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -142,7 +142,7 @@ test_expect_success 'delete ref with dangling packed version' '
 	test_cmp /dev/null result
 '
 
-test_expect_failure 'delete ref while another dangling packed ref' '
+test_expect_success 'delete ref while another dangling packed ref' '
 	git branch lamb &&
 	git commit --allow-empty -m "future garbage" &&
 	git pack-refs --all &&
-- 
1.8.2.1

[PATCH v2 18/33] search_ref_dir(): return an index rather than a pointer

[Michael Haggerty]

Change search_ref_dir() to return the index of the sought entry (or -1
on error) rather than a pointer to the entry.  This will make it more
natural to use the function for removing an entry from the list.

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

diff --git a/refs.c b/refs.c
index 2957f6d..9fd49e8 100644
--- a/refs.c
+++ b/refs.c
@@ -366,18 +366,17 @@ static int ref_entry_cmp_sslice(const void *key_, const void *ent_)
 }
 
 /*
- * Return the entry with the given refname from the ref_dir
- * (non-recursively), sorting dir if necessary.  Return NULL if no
- * such entry is found.  dir must already be complete.
+ * Return the index of the entry with the given refname from the
+ * ref_dir (non-recursively), sorting dir if necessary.  Return -1 if
+ * no such entry is found.  dir must already be complete.
  */
-static struct ref_entry *search_ref_dir(struct ref_dir *dir,
-					const char *refname, size_t len)
+static int search_ref_dir(struct ref_dir *dir, const char *refname, size_t len)
 {
 	struct ref_entry **r;
 	struct string_slice key;
 
 	if (refname == NULL || !dir->nr)
-		return NULL;
+		return -1;
 
 	sort_ref_dir(dir);
 	key.len = len;
@@ -386,9 +385,9 @@ static struct ref_entry *search_ref_dir(struct ref_dir *dir,
 		    ref_entry_cmp_sslice);
 
 	if (r == NULL)
-		return NULL;
+		return -1;
 
-	return *r;
+	return r - dir->entries;
 }
 
 /*
@@ -402,8 +401,9 @@ static struct ref_dir *search_for_subdir(struct ref_dir *dir,
 					 const char *subdirname, size_t len,
 					 int mkdir)
 {
-	struct ref_entry *entry = search_ref_dir(dir, subdirname, len);
-	if (!entry) {
+	int entry_index = search_ref_dir(dir, subdirname, len);
+	struct ref_entry *entry;
+	if (entry_index == -1) {
 		if (!mkdir)
 			return NULL;
 		/*
@@ -414,6 +414,8 @@ static struct ref_dir *search_for_subdir(struct ref_dir *dir,
 		 */
 		entry = create_dir_entry(dir->ref_cache, subdirname, len, 0);
 		add_entry_to_dir(dir, entry);
+	} else {
+		entry = dir->entries[entry_index];
 	}
 	return get_ref_dir(entry);
 }
@@ -452,12 +454,16 @@ static struct ref_dir *find_containing_dir(struct ref_dir *dir,
  */
 static struct ref_entry *find_ref(struct ref_dir *dir, const char *refname)
 {
+	int entry_index;
 	struct ref_entry *entry;
 	dir = find_containing_dir(dir, refname, 0);
 	if (!dir)
 		return NULL;
-	entry = search_ref_dir(dir, refname, strlen(refname));
-	return (entry && !(entry->flag & REF_DIR)) ? entry : NULL;
+	entry_index = search_ref_dir(dir, refname, strlen(refname));
+	if (entry_index == -1)
+		return NULL;
+	entry = dir->entries[entry_index];
+	return (entry->flag & REF_DIR) ? NULL : entry;
 }
 
 /*
-- 
1.8.2.1

[PATCH v2 19/33] refs: change how packed refs are deleted

[Michael Haggerty]

Add a function remove_ref(), which removes a single entry from a
reference cache.

Use this function to reimplement repack_without_ref().  The old
version iterated over all refs, packing all of them except for the one
to be deleted, then discarded the entire packed reference cache.  The
new version deletes the doomed reference from the cache *before*
iterating.

This has two advantages:

* the code for writing packed-refs becomes simpler, because it doesn't
  have to exclude one of the references.

* it is no longer necessary to discard the packed refs cache after
  deleting a reference: symbolic refs cannot be packed, so packed
  references cannot depend on each other, so the rest of the packed
  refs cache remains valid after a reference is deleted.

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

diff --git a/refs.c b/refs.c
index 9fd49e8..51915a8 100644
--- a/refs.c
+++ b/refs.c
@@ -467,6 +467,57 @@ static struct ref_entry *find_ref(struct ref_dir *dir, const char *refname)
 }
 
 /*
+ * Remove the entry with the given name from dir, recursing into
+ * subdirectories as necessary.  If refname is the name of a directory
+ * (i.e., ends with '/'), then remove the directory and its contents.
+ * If the removal was successful, return the number of entries
+ * remaining in the directory entry that contained the deleted entry.
+ * If the name was not found, return -1.  Please note that this
+ * function only deletes the entry from the cache; it does not delete
+ * it from the filesystem or ensure that other cache entries (which
+ * might be symbolic references to the removed entry) are updated.
+ * Nor does it remove any containing dir entries that might be made
+ * empty by the removal.  dir must represent the top-level directory
+ * and must already be complete.
+ */
+static int remove_entry(struct ref_dir *dir, const char *refname)
+{
+	int refname_len = strlen(refname);
+	int entry_index;
+	struct ref_entry *entry;
+	int is_dir = refname[refname_len - 1] == '/';
+	if (is_dir) {
+		/*
+		 * refname represents a reference directory.  Remove
+		 * the trailing slash; otherwise we will get the
+		 * directory *representing* refname rather than the
+		 * one *containing* it.
+		 */
+		char *dirname = xmemdupz(refname, refname_len - 1);
+		dir = find_containing_dir(dir, dirname, 0);
+		free(dirname);
+	} else {
+		dir = find_containing_dir(dir, refname, 0);
+	}
+	if (!dir)
+		return -1;
+	entry_index = search_ref_dir(dir, refname, refname_len);
+	if (entry_index == -1)
+		return -1;
+	entry = dir->entries[entry_index];
+
+	memmove(&dir->entries[entry_index],
+		&dir->entries[entry_index + 1],
+		(dir->nr - entry_index - 1) * sizeof(*dir->entries)
+		);
+	dir->nr--;
+	if (dir->sorted > entry_index)
+		dir->sorted--;
+	free_ref_entry(entry);
+	return dir->nr;
+}
+
+/*
  * Add a ref_entry to the ref_dir (unsorted), recursing into
  * subdirectories as necessary.  dir must represent the top-level
  * directory.  Return 0 on success.
@@ -1894,19 +1945,12 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,
 	return lock_ref_sha1_basic(refname, old_sha1, flags, NULL);
 }
 
-struct repack_without_ref_sb {
-	const char *refname;
-	int fd;
-};
-
-static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)
+static int repack_ref_fn(struct ref_entry *entry, void *cb_data)
 {
-	struct repack_without_ref_sb *data = cb_data;
+	int *fd = cb_data;
 	char line[PATH_MAX + 100];
 	int len;
 
-	if (!strcmp(data->refname, entry->name))
-		return 0;
 	if (entry->flag & REF_ISBROKEN) {
 		/* This shouldn't happen to packed refs. */
 		error("%s is broken!", entry->name);
@@ -1947,7 +1991,7 @@ static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)
 	/* this should not happen but just being defensive */
 	if (len > sizeof(line))
 		die("too long a refname '%s'", entry->name);
-	write_or_die(data->fd, line, len);
+	write_or_die(*fd, line, len);
 	return 0;
 }
 
@@ -1955,22 +1999,30 @@ static struct lock_file packlock;
 
 static int repack_without_ref(const char *refname)
 {
-	struct repack_without_ref_sb data;
+	int fd;
 	struct ref_cache *refs = get_ref_cache(NULL);
 	struct ref_dir *packed;
 
 	if (!get_packed_ref(refname))
 		return 0; /* refname does not exist in packed refs */
 
-	data.refname = refname;
-	data.fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0);
-	if (data.fd < 0) {
+	fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0);
+	if (fd < 0) {
 		unable_to_lock_error(git_path("packed-refs"), errno);
 		return error("cannot delete '%s' from packed refs", refname);
 	}
 	clear_packed_ref_cache(refs);
 	packed = get_packed_refs(refs);
-	do_for_each_entry_in_dir(packed, 0, repack_without_ref_fn, &data);
+	/* Remove refname from the cache. */
+	if (remove_entry(packed, refname) == -1) {
+		/*
+		 * The packed entry disappeared while we were
+		 * acquiring the lock.
+		 */
+		rollback_lock_file(&packlock);
+		return 0;
+	}
+	do_for_each_entry_in_dir(packed, 0, repack_ref_fn, &fd);
 	return commit_lock_file(&packlock);
 }
 
@@ -1999,7 +2051,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	ret |= repack_without_ref(lock->ref_name);
 
 	unlink_or_warn(git_path("logs/%s", lock->ref_name));
-	invalidate_ref_cache(NULL);
+	clear_loose_ref_cache(get_ref_cache(NULL));
 	unlock_ref(lock);
 	return ret;
 }
-- 
1.8.2.1

[PATCH v2 20/33] t3211: demonstrate loss of peeled refs if a packed ref is deleted

[Michael Haggerty]

Add a test that demonstrates that the peeled values recorded in
packed-refs are lost if a packed ref is deleted.  (The code in
repack_without_ref() doesn't even attempt to write peeled refs.)  This
will be fixed in a moment.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3211-peel-ref.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
index d4d7792..cca1acb 100755
--- a/t/t3211-peel-ref.sh
+++ b/t/t3211-peel-ref.sh
@@ -61,4 +61,13 @@ test_expect_success 'refs are peeled outside of refs/tags (old packed)' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'peeled refs survive deletion of packed ref' '
+	git pack-refs --all &&
+	cp .git/packed-refs fully-peeled &&
+	git branch yadda &&
+	git pack-refs --all &&
+	git branch -d yadda &&
+	test_cmp fully-peeled .git/packed-refs
+'
+
 test_done
-- 
1.8.2.1

[PATCH v2 21/33] repack_without_ref(): write peeled refs in the rewritten file

[Michael Haggerty]

When a reference that existed in the packed-refs file is deleted, the
packed-refs file must be rewritten.  Previously, the file was
rewritten without any peeled refs, even if the file contained peeled
refs when it was read.  This was not a bug, because the packed-refs
file header didn't claim that the file contained peeled values.  But
it had a performance cost, because the repository would lose the
benefit of having precomputed peeled references until pack-refs was
run again.

Teach repack_without_ref() to write peeled refs to the packed-refs
file (regardless of whether they were present in the old version of
the file).

This means that if the old version of the packed-refs file was not
fully peeled, then repack_without_ref() will have to peel references.
To avoid the expense of reading lots of loose references, we take two
shortcuts relative to pack-refs:

* If the peeled value of a reference is already known (i.e., because
  it was read from the old version of the packed-refs file), then
  output that peeled value again without any checks.  This is the
  usual code path and should avoid any noticeable overhead.  (This is
  different than pack-refs, which always re-peels references.)

* We don't verify that the packed ref is still current.  It could be
  that a packed references is overridden by a loose reference, in
  which case the packed ref is no longer needed and might even refer
  to an object that has been garbage collected.  But we don't check;
  instead, we just try to peel all references.  If peeling is
  successful, the peeled value is written out (even though it might
  not be needed any more); if not, then the reference is silently
  omitted from the output.

The extra overhead of peeling references in repack_without_ref()
should only be incurred the first time the packed-refs file is written
by a version of Git that knows about the "fully-peeled" attribute.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c              | 23 +++++++++++++++++++++++
 t/t3211-peel-ref.sh |  2 +-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 51915a8..ff4c5f1 100644
--- a/refs.c
+++ b/refs.c
@@ -876,6 +876,13 @@ void invalidate_ref_cache(const char *submodule)
 #define PEELED_LINE_LENGTH 42
 
 /*
+ * The packed-refs header line that we write out.  Perhaps other
+ * traits will be added later.  The trailing space is required.
+ */
+static const char PACKED_REFS_HEADER[] =
+	"# pack-refs with: peeled fully-peeled \n";
+
+/*
  * Parse one line from a packed-refs file.  Write the SHA1 to sha1.
  * Return a pointer to the refname within the line (null-terminated),
  * or NULL if there was a problem.
@@ -1390,6 +1397,12 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh
 
 /*
  * Peel the entry (if possible) and return its new peel_status.
+ *
+ * It is OK to call this function with a packed reference entry that
+ * might be stale and might even refer to an object that has since
+ * been garbage-collected.  In such a case, if the entry has
+ * REF_KNOWS_PEELED then leave the status unchanged and return
+ * PEEL_PEELED or PEEL_NON_TAG; otherwise, return PEEL_INVALID.
  */
 static enum peel_status peel_entry(struct ref_entry *entry)
 {
@@ -1992,6 +2005,15 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)
 	if (len > sizeof(line))
 		die("too long a refname '%s'", entry->name);
 	write_or_die(*fd, line, len);
+	if (!peel_entry(entry)) {
+		/* This reference could be peeled; write the peeled value: */
+		if (snprintf(line, sizeof(line), "^%s\n",
+			     sha1_to_hex(entry->u.value.peeled)) !=
+		    PEELED_LINE_LENGTH)
+			die("internal error");
+		write_or_die(*fd, line, PEELED_LINE_LENGTH);
+	}
+
 	return 0;
 }
 
@@ -2022,6 +2044,7 @@ static int repack_without_ref(const char *refname)
 		rollback_lock_file(&packlock);
 		return 0;
 	}
+	write_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
 	do_for_each_entry_in_dir(packed, 0, repack_ref_fn, &fd);
 	return commit_lock_file(&packlock);
 }
diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
index cca1acb..3b7caca 100755
--- a/t/t3211-peel-ref.sh
+++ b/t/t3211-peel-ref.sh
@@ -61,7 +61,7 @@ test_expect_success 'refs are peeled outside of refs/tags (old packed)' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'peeled refs survive deletion of packed ref' '
+test_expect_success 'peeled refs survive deletion of packed ref' '
 	git pack-refs --all &&
 	cp .git/packed-refs fully-peeled &&
 	git branch yadda &&
-- 
1.8.2.1

[PATCH v2 22/33] refs: extract a function write_packed_entry()

[Michael Haggerty]

Extract the I/O code from the "business logic" in repack_ref_fn().
Later there will be another caller for this function.

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

diff --git a/refs.c b/refs.c
index ff4c5f1..42298db 100644
--- a/refs.c
+++ b/refs.c
@@ -1958,12 +1958,36 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,
 	return lock_ref_sha1_basic(refname, old_sha1, flags, NULL);
 }
 
-static int repack_ref_fn(struct ref_entry *entry, void *cb_data)
+/*
+ * Write an entry to the packed-refs file for the specified refname.
+ * If peeled is non-NULL, write it as the entry's peeled value.
+ */
+static void write_packed_entry(int fd, char *refname, unsigned char *sha1,
+			       unsigned char *peeled)
 {
-	int *fd = cb_data;
 	char line[PATH_MAX + 100];
 	int len;
 
+	len = snprintf(line, sizeof(line), "%s %s\n",
+		       sha1_to_hex(sha1), refname);
+	/* this should not happen but just being defensive */
+	if (len > sizeof(line))
+		die("too long a refname '%s'", refname);
+	write_or_die(fd, line, len);
+
+	if (peeled) {
+		if (snprintf(line, sizeof(line), "^%s\n",
+			     sha1_to_hex(peeled)) != PEELED_LINE_LENGTH)
+			die("internal error");
+		write_or_die(fd, line, PEELED_LINE_LENGTH);
+	}
+}
+
+static int repack_ref_fn(struct ref_entry *entry, void *cb_data)
+{
+	int *fd = cb_data;
+	enum peel_status peel_status;
+
 	if (entry->flag & REF_ISBROKEN) {
 		/* This shouldn't happen to packed refs. */
 		error("%s is broken!", entry->name);
@@ -1999,20 +2023,10 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)
 		return 0;
 	}
 
-	len = snprintf(line, sizeof(line), "%s %s\n",
-		       sha1_to_hex(entry->u.value.sha1), entry->name);
-	/* this should not happen but just being defensive */
-	if (len > sizeof(line))
-		die("too long a refname '%s'", entry->name);
-	write_or_die(*fd, line, len);
-	if (!peel_entry(entry)) {
-		/* This reference could be peeled; write the peeled value: */
-		if (snprintf(line, sizeof(line), "^%s\n",
-			     sha1_to_hex(entry->u.value.peeled)) !=
-		    PEELED_LINE_LENGTH)
-			die("internal error");
-		write_or_die(*fd, line, PEELED_LINE_LENGTH);
-	}
+	peel_status = peel_entry(entry);
+	write_packed_entry(*fd, entry->name, entry->u.value.sha1,
+			   peel_status == PEEL_PEELED ?
+			   entry->u.value.peeled : NULL);
 
 	return 0;
 }
-- 
1.8.2.1

[PATCH v2 23/33] pack-refs: rename handle_one_ref() to pack_one_ref()

[Michael Haggerty]

This code is about to be moved, so name the function more
distinctively.

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

diff --git a/pack-refs.c b/pack-refs.c
index 4461f71..d840055 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -23,7 +23,7 @@ static int do_not_prune(int flags)
 	return (flags & (REF_ISSYMREF|REF_ISPACKED));
 }
 
-static int handle_one_ref(const char *path, const unsigned char *sha1,
+static int pack_one_ref(const char *path, const unsigned char *sha1,
 			  int flags, void *cb_data)
 {
 	struct pack_refs_cb_data *cb = cb_data;
@@ -130,7 +130,7 @@ int pack_refs(unsigned int flags)
 	/* perhaps other traits later as well */
 	fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n");
 
-	for_each_ref(handle_one_ref, &cbdata);
+	for_each_ref(pack_one_ref, &cbdata);
 	if (ferror(cbdata.refs_file))
 		die("failed to write ref-pack file");
 	if (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))
-- 
1.8.2.1

[PATCH v2 24/33] pack-refs: merge code from pack-refs.{c,h} into refs.{c,h}

[Michael Haggerty]

pack-refs.c doesn't contain much code, and the code it does contain is
closely related to reference handling.  Moreover, there is some
duplication between pack_refs() and repack_without_ref().  Therefore,
merge pack-refs.c into refs.c and pack-refs.h into refs.h.

The code duplication will be addressed in future commits.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Makefile            |   2 -
 builtin/clone.c     |   1 -
 builtin/pack-refs.c |   2 +-
 pack-refs.c         | 148 ----------------------------------------------------
 pack-refs.h         |  18 -------
 refs.c              | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
 refs.h              |  14 +++++
 7 files changed, 159 insertions(+), 170 deletions(-)
 delete mode 100644 pack-refs.c
 delete mode 100644 pack-refs.h

diff --git a/Makefile b/Makefile
index 0f931a2..de38539 100644
--- a/Makefile
+++ b/Makefile
@@ -684,7 +684,6 @@ LIB_H += notes-cache.h
 LIB_H += notes-merge.h
 LIB_H += notes.h
 LIB_H += object.h
-LIB_H += pack-refs.h
 LIB_H += pack-revindex.h
 LIB_H += pack.h
 LIB_H += parse-options.h
@@ -817,7 +816,6 @@ LIB_OBJS += notes-cache.o
 LIB_OBJS += notes-merge.o
 LIB_OBJS += object.o
 LIB_OBJS += pack-check.o
-LIB_OBJS += pack-refs.o
 LIB_OBJS += pack-revindex.o
 LIB_OBJS += pack-write.o
 LIB_OBJS += pager.o
diff --git a/builtin/clone.c b/builtin/clone.c
index f9c380e..883cf2a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -18,7 +18,6 @@
 #include "transport.h"
 #include "strbuf.h"
 #include "dir.h"
-#include "pack-refs.h"
 #include "sigchain.h"
 #include "branch.h"
 #include "remote.h"
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index b5a0f88..b20b1ec 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -1,6 +1,6 @@
 #include "builtin.h"
 #include "parse-options.h"
-#include "pack-refs.h"
+#include "refs.h"
 
 static char const * const pack_refs_usage[] = {
 	N_("git pack-refs [options]"),
diff --git a/pack-refs.c b/pack-refs.c
deleted file mode 100644
index d840055..0000000
--- a/pack-refs.c
+++ /dev/null
@@ -1,148 +0,0 @@
-#include "cache.h"
-#include "refs.h"
-#include "tag.h"
-#include "pack-refs.h"
-
-struct ref_to_prune {
-	struct ref_to_prune *next;
-	unsigned char sha1[20];
-	char name[FLEX_ARRAY];
-};
-
-struct pack_refs_cb_data {
-	unsigned int flags;
-	struct ref_to_prune *ref_to_prune;
-	FILE *refs_file;
-};
-
-static int do_not_prune(int flags)
-{
-	/* If it is already packed or if it is a symref,
-	 * do not prune it.
-	 */
-	return (flags & (REF_ISSYMREF|REF_ISPACKED));
-}
-
-static int pack_one_ref(const char *path, const unsigned char *sha1,
-			  int flags, void *cb_data)
-{
-	struct pack_refs_cb_data *cb = cb_data;
-	struct object *o;
-	int is_tag_ref;
-
-	/* Do not pack the symbolic refs */
-	if ((flags & REF_ISSYMREF))
-		return 0;
-	is_tag_ref = !prefixcmp(path, "refs/tags/");
-
-	/* ALWAYS pack refs that were already packed or are tags */
-	if (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref && !(flags & REF_ISPACKED))
-		return 0;
-
-	fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), path);
-
-	o = parse_object_or_die(sha1, path);
-	if (o->type == OBJ_TAG) {
-		o = deref_tag(o, path, 0);
-		if (o)
-			fprintf(cb->refs_file, "^%s\n",
-				sha1_to_hex(o->sha1));
-	}
-
-	if ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {
-		int namelen = strlen(path) + 1;
-		struct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);
-		hashcpy(n->sha1, sha1);
-		strcpy(n->name, path);
-		n->next = cb->ref_to_prune;
-		cb->ref_to_prune = n;
-	}
-	return 0;
-}
-
-/*
- * Remove empty parents, but spare refs/ and immediate subdirs.
- * Note: munges *name.
- */
-static void try_remove_empty_parents(char *name)
-{
-	char *p, *q;
-	int i;
-	p = name;
-	for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
-		while (*p && *p != '/')
-			p++;
-		/* tolerate duplicate slashes; see check_refname_format() */
-		while (*p == '/')
-			p++;
-	}
-	for (q = p; *q; q++)
-		;
-	while (1) {
-		while (q > p && *q != '/')
-			q--;
-		while (q > p && *(q-1) == '/')
-			q--;
-		if (q == p)
-			break;
-		*q = '\0';
-		if (rmdir(git_path("%s", name)))
-			break;
-	}
-}
-
-/* make sure nobody touched the ref, and unlink */
-static void prune_ref(struct ref_to_prune *r)
-{
-	struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
-
-	if (lock) {
-		unlink_or_warn(git_path("%s", r->name));
-		unlock_ref(lock);
-		try_remove_empty_parents(r->name);
-	}
-}
-
-static void prune_refs(struct ref_to_prune *r)
-{
-	while (r) {
-		prune_ref(r);
-		r = r->next;
-	}
-}
-
-static struct lock_file packed;
-
-int pack_refs(unsigned int flags)
-{
-	int fd;
-	struct pack_refs_cb_data cbdata;
-
-	memset(&cbdata, 0, sizeof(cbdata));
-	cbdata.flags = flags;
-
-	fd = hold_lock_file_for_update(&packed, git_path("packed-refs"),
-				       LOCK_DIE_ON_ERROR);
-	cbdata.refs_file = fdopen(fd, "w");
-	if (!cbdata.refs_file)
-		die_errno("unable to create ref-pack file structure");
-
-	/* perhaps other traits later as well */
-	fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n");
-
-	for_each_ref(pack_one_ref, &cbdata);
-	if (ferror(cbdata.refs_file))
-		die("failed to write ref-pack file");
-	if (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))
-		die_errno("failed to write ref-pack file");
-	/*
-	 * Since the lock file was fdopen()'ed and then fclose()'ed above,
-	 * assign -1 to the lock file descriptor so that commit_lock_file()
-	 * won't try to close() it.
-	 */
-	packed.fd = -1;
-	if (commit_lock_file(&packed) < 0)
-		die_errno("unable to overwrite old ref-pack file");
-	prune_refs(cbdata.ref_to_prune);
-	return 0;
-}
diff --git a/pack-refs.h b/pack-refs.h
deleted file mode 100644
index 518acfb..0000000
--- a/pack-refs.h
+++ /dev/null
@@ -1,18 +0,0 @@
-#ifndef PACK_REFS_H
-#define PACK_REFS_H
-
-/*
- * Flags for controlling behaviour of pack_refs()
- * PACK_REFS_PRUNE: Prune loose refs after packing
- * PACK_REFS_ALL:   Pack _all_ refs, not just tags and already packed refs
- */
-#define PACK_REFS_PRUNE 0x0001
-#define PACK_REFS_ALL   0x0002
-
-/*
- * Write a packed-refs file for the current repository.
- * flags: Combination of the above PACK_REFS_* flags.
- */
-int pack_refs(unsigned int flags);
-
-#endif /* PACK_REFS_H */
diff --git a/refs.c b/refs.c
index 42298db..268c1a0 100644
--- a/refs.c
+++ b/refs.c
@@ -1983,6 +1983,150 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,
 	}
 }
 
+struct ref_to_prune {
+	struct ref_to_prune *next;
+	unsigned char sha1[20];
+	char name[FLEX_ARRAY];
+};
+
+struct pack_refs_cb_data {
+	unsigned int flags;
+	struct ref_to_prune *ref_to_prune;
+	FILE *refs_file;
+};
+
+static int do_not_prune(int flags)
+{
+	/* If it is already packed or if it is a symref,
+	 * do not prune it.
+	 */
+	return (flags & (REF_ISSYMREF|REF_ISPACKED));
+}
+
+static int pack_one_ref(const char *path, const unsigned char *sha1,
+			  int flags, void *cb_data)
+{
+	struct pack_refs_cb_data *cb = cb_data;
+	struct object *o;
+	int is_tag_ref;
+
+	/* Do not pack the symbolic refs */
+	if ((flags & REF_ISSYMREF))
+		return 0;
+	is_tag_ref = !prefixcmp(path, "refs/tags/");
+
+	/* ALWAYS pack refs that were already packed or are tags */
+	if (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref && !(flags & REF_ISPACKED))
+		return 0;
+
+	fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), path);
+
+	o = parse_object_or_die(sha1, path);
+	if (o->type == OBJ_TAG) {
+		o = deref_tag(o, path, 0);
+		if (o)
+			fprintf(cb->refs_file, "^%s\n",
+				sha1_to_hex(o->sha1));
+	}
+
+	if ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {
+		int namelen = strlen(path) + 1;
+		struct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);
+		hashcpy(n->sha1, sha1);
+		strcpy(n->name, path);
+		n->next = cb->ref_to_prune;
+		cb->ref_to_prune = n;
+	}
+	return 0;
+}
+
+/*
+ * Remove empty parents, but spare refs/ and immediate subdirs.
+ * Note: munges *name.
+ */
+static void try_remove_empty_parents(char *name)
+{
+	char *p, *q;
+	int i;
+	p = name;
+	for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
+		while (*p && *p != '/')
+			p++;
+		/* tolerate duplicate slashes; see check_refname_format() */
+		while (*p == '/')
+			p++;
+	}
+	for (q = p; *q; q++)
+		;
+	while (1) {
+		while (q > p && *q != '/')
+			q--;
+		while (q > p && *(q-1) == '/')
+			q--;
+		if (q == p)
+			break;
+		*q = '\0';
+		if (rmdir(git_path("%s", name)))
+			break;
+	}
+}
+
+/* make sure nobody touched the ref, and unlink */
+static void prune_ref(struct ref_to_prune *r)
+{
+	struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
+
+	if (lock) {
+		unlink_or_warn(git_path("%s", r->name));
+		unlock_ref(lock);
+		try_remove_empty_parents(r->name);
+	}
+}
+
+static void prune_refs(struct ref_to_prune *r)
+{
+	while (r) {
+		prune_ref(r);
+		r = r->next;
+	}
+}
+
+static struct lock_file packed;
+
+int pack_refs(unsigned int flags)
+{
+	int fd;
+	struct pack_refs_cb_data cbdata;
+
+	memset(&cbdata, 0, sizeof(cbdata));
+	cbdata.flags = flags;
+
+	fd = hold_lock_file_for_update(&packed, git_path("packed-refs"),
+				       LOCK_DIE_ON_ERROR);
+	cbdata.refs_file = fdopen(fd, "w");
+	if (!cbdata.refs_file)
+		die_errno("unable to create ref-pack file structure");
+
+	/* perhaps other traits later as well */
+	fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n");
+
+	for_each_ref(pack_one_ref, &cbdata);
+	if (ferror(cbdata.refs_file))
+		die("failed to write ref-pack file");
+	if (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))
+		die_errno("failed to write ref-pack file");
+	/*
+	 * Since the lock file was fdopen()'ed and then fclose()'ed above,
+	 * assign -1 to the lock file descriptor so that commit_lock_file()
+	 * won't try to close() it.
+	 */
+	packed.fd = -1;
+	if (commit_lock_file(&packed) < 0)
+		die_errno("unable to overwrite old ref-pack file");
+	prune_refs(cbdata.ref_to_prune);
+	return 0;
+}
+
 static int repack_ref_fn(struct ref_entry *entry, void *cb_data)
 {
 	int *fd = cb_data;
diff --git a/refs.h b/refs.h
index ac0ff92..8060ed8 100644
--- a/refs.h
+++ b/refs.h
@@ -72,6 +72,20 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn
  */
 extern void add_packed_ref(const char *refname, const unsigned char *sha1);
 
+/*
+ * Flags for controlling behaviour of pack_refs()
+ * PACK_REFS_PRUNE: Prune loose refs after packing
+ * PACK_REFS_ALL:   Pack _all_ refs, not just tags and already packed refs
+ */
+#define PACK_REFS_PRUNE 0x0001
+#define PACK_REFS_ALL   0x0002
+
+/*
+ * Write a packed-refs file for the current repository.
+ * flags: Combination of the above PACK_REFS_* flags.
+ */
+int pack_refs(unsigned int flags);
+
 extern int ref_exists(const char *);
 
 /*
-- 
1.8.2.1

[PATCH v2 25/33] pack_one_ref(): rename "path" parameter to "refname"

[Michael Haggerty]

Make this function conform to the naming convention established in
65385ef7d4 for the rest of the refs.c file.

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

diff --git a/refs.c b/refs.c
index 268c1a0..b41dd5e 100644
--- a/refs.c
+++ b/refs.c
@@ -2003,7 +2003,7 @@ static int do_not_prune(int flags)
 	return (flags & (REF_ISSYMREF|REF_ISPACKED));
 }
 
-static int pack_one_ref(const char *path, const unsigned char *sha1,
+static int pack_one_ref(const char *refname, const unsigned char *sha1,
 			  int flags, void *cb_data)
 {
 	struct pack_refs_cb_data *cb = cb_data;
@@ -2013,27 +2013,27 @@ static int pack_one_ref(const char *path, const unsigned char *sha1,
 	/* Do not pack the symbolic refs */
 	if ((flags & REF_ISSYMREF))
 		return 0;
-	is_tag_ref = !prefixcmp(path, "refs/tags/");
+	is_tag_ref = !prefixcmp(refname, "refs/tags/");
 
 	/* ALWAYS pack refs that were already packed or are tags */
 	if (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref && !(flags & REF_ISPACKED))
 		return 0;
 
-	fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), path);
+	fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), refname);
 
-	o = parse_object_or_die(sha1, path);
+	o = parse_object_or_die(sha1, refname);
 	if (o->type == OBJ_TAG) {
-		o = deref_tag(o, path, 0);
+		o = deref_tag(o, refname, 0);
 		if (o)
 			fprintf(cb->refs_file, "^%s\n",
 				sha1_to_hex(o->sha1));
 	}
 
 	if ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {
-		int namelen = strlen(path) + 1;
+		int namelen = strlen(refname) + 1;
 		struct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);
 		hashcpy(n->sha1, sha1);
-		strcpy(n->name, path);
+		strcpy(n->name, refname);
 		n->next = cb->ref_to_prune;
 		cb->ref_to_prune = n;
 	}
-- 
1.8.2.1

[PATCH v2 26/33] refs: use same lock_file object for both ref-packing functions

[Michael Haggerty]

Use a single struct lock_file for both pack_refs() and
repack_without_ref().

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

diff --git a/refs.c b/refs.c
index b41dd5e..850df8e 100644
--- a/refs.c
+++ b/refs.c
@@ -2091,7 +2091,7 @@ static void prune_refs(struct ref_to_prune *r)
 	}
 }
 
-static struct lock_file packed;
+static struct lock_file packlock;
 
 int pack_refs(unsigned int flags)
 {
@@ -2101,7 +2101,7 @@ int pack_refs(unsigned int flags)
 	memset(&cbdata, 0, sizeof(cbdata));
 	cbdata.flags = flags;
 
-	fd = hold_lock_file_for_update(&packed, git_path("packed-refs"),
+	fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"),
 				       LOCK_DIE_ON_ERROR);
 	cbdata.refs_file = fdopen(fd, "w");
 	if (!cbdata.refs_file)
@@ -2120,8 +2120,8 @@ int pack_refs(unsigned int flags)
 	 * assign -1 to the lock file descriptor so that commit_lock_file()
 	 * won't try to close() it.
 	 */
-	packed.fd = -1;
-	if (commit_lock_file(&packed) < 0)
+	packlock.fd = -1;
+	if (commit_lock_file(&packlock) < 0)
 		die_errno("unable to overwrite old ref-pack file");
 	prune_refs(cbdata.ref_to_prune);
 	return 0;
@@ -2175,8 +2175,6 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-static struct lock_file packlock;
-
 static int repack_without_ref(const char *refname)
 {
 	int fd;
-- 
1.8.2.1

[PATCH v2 27/33] pack_refs(): change to use do_for_each_entry()

[Michael Haggerty]

pack_refs() was not using any of the extra features of for_each_ref(),
so change it to use do_for_each_entry().  This also gives it access to
the ref_entry and in particular its peeled field, which will be taken
advantage of in the next commit.

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

diff --git a/refs.c b/refs.c
index 850df8e..88875d1 100644
--- a/refs.c
+++ b/refs.c
@@ -2003,37 +2003,38 @@ static int do_not_prune(int flags)
 	return (flags & (REF_ISSYMREF|REF_ISPACKED));
 }
 
-static int pack_one_ref(const char *refname, const unsigned char *sha1,
-			  int flags, void *cb_data)
+static int pack_one_ref(struct ref_entry *entry, void *cb_data)
 {
 	struct pack_refs_cb_data *cb = cb_data;
 	struct object *o;
 	int is_tag_ref;
 
-	/* Do not pack the symbolic refs */
-	if ((flags & REF_ISSYMREF))
+	/* Do not pack symbolic or broken refs: */
+	if ((entry->flag & REF_ISSYMREF) || !ref_resolves_to_object(entry))
 		return 0;
-	is_tag_ref = !prefixcmp(refname, "refs/tags/");
+	is_tag_ref = !prefixcmp(entry->name, "refs/tags/");
 
 	/* ALWAYS pack refs that were already packed or are tags */
-	if (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref && !(flags & REF_ISPACKED))
+	if (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref &&
+	    !(entry->flag & REF_ISPACKED))
 		return 0;
 
-	fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), refname);
+	fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(entry->u.value.sha1),
+		entry->name);
 
-	o = parse_object_or_die(sha1, refname);
+	o = parse_object_or_die(entry->u.value.sha1, entry->name);
 	if (o->type == OBJ_TAG) {
-		o = deref_tag(o, refname, 0);
+		o = deref_tag(o, entry->name, 0);
 		if (o)
 			fprintf(cb->refs_file, "^%s\n",
 				sha1_to_hex(o->sha1));
 	}
 
-	if ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {
-		int namelen = strlen(refname) + 1;
+	if ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(entry->flag)) {
+		int namelen = strlen(entry->name) + 1;
 		struct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);
-		hashcpy(n->sha1, sha1);
-		strcpy(n->name, refname);
+		hashcpy(n->sha1, entry->u.value.sha1);
+		strcpy(n->name, entry->name);
 		n->next = cb->ref_to_prune;
 		cb->ref_to_prune = n;
 	}
@@ -2110,7 +2111,7 @@ int pack_refs(unsigned int flags)
 	/* perhaps other traits later as well */
 	fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n");
 
-	for_each_ref(pack_one_ref, &cbdata);
+	do_for_each_entry(NULL, "", pack_one_ref, &cbdata);
 	if (ferror(cbdata.refs_file))
 		die("failed to write ref-pack file");
 	if (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))
-- 
1.8.2.1

[PATCH v2 28/33] refs: inline function do_not_prune()

[Michael Haggerty]

Function do_not_prune() was redundantly checking REF_ISSYMREF, which
was already tested at the top of pack_one_ref(), so remove that check.
And the rest was trivial, so inline the function.

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

diff --git a/refs.c b/refs.c
index 88875d1..f78e955 100644
--- a/refs.c
+++ b/refs.c
@@ -1995,14 +1995,6 @@ struct pack_refs_cb_data {
 	FILE *refs_file;
 };
 
-static int do_not_prune(int flags)
-{
-	/* If it is already packed or if it is a symref,
-	 * do not prune it.
-	 */
-	return (flags & (REF_ISSYMREF|REF_ISPACKED));
-}
-
 static int pack_one_ref(struct ref_entry *entry, void *cb_data)
 {
 	struct pack_refs_cb_data *cb = cb_data;
@@ -2030,7 +2022,8 @@ static int pack_one_ref(struct ref_entry *entry, void *cb_data)
 				sha1_to_hex(o->sha1));
 	}
 
-	if ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(entry->flag)) {
+	/* If the ref was already packed, there is no need to prune it. */
+	if ((cb->flags & PACK_REFS_PRUNE) && !(entry->flag & REF_ISPACKED)) {
 		int namelen = strlen(entry->name) + 1;
 		struct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);
 		hashcpy(n->sha1, entry->u.value.sha1);
-- 
1.8.2.1

[PATCH v2 29/33] pack_one_ref(): use function peel_entry()

[Michael Haggerty]

Change pack_one_ref() to call peel_entry() rather than using its own
code for peeling references.  Aside from sharing code, this lets it
take advantage of the optimization introduced by 6c4a060d7d.

Please note that we *could* use any peeled values that happen to
already be stored in the ref_entries, which would avoid some object
lookups for references that were already packed.  But doing so would
also propagate any peeling errors across runs of "git pack-refs" and
give no way to recover from such errors.  And "git pack-refs" isn't
run often enough that the performance cost is a problem.  So instead,
add a new option to peel_entry() to force the entry to be re-peeled,
and call it with that option set.

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

diff --git a/refs.c b/refs.c
index f78e955..f2d83f3 100644
--- a/refs.c
+++ b/refs.c
@@ -1396,7 +1396,9 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh
 }
 
 /*
- * Peel the entry (if possible) and return its new peel_status.
+ * Peel the entry (if possible) and return its new peel_status.  If
+ * repeel is true, re-peel the entry even if there is an old peeled
+ * value that is already stored in it.
  *
  * It is OK to call this function with a packed reference entry that
  * might be stale and might even refer to an object that has since
@@ -1404,13 +1406,19 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh
  * REF_KNOWS_PEELED then leave the status unchanged and return
  * PEEL_PEELED or PEEL_NON_TAG; otherwise, return PEEL_INVALID.
  */
-static enum peel_status peel_entry(struct ref_entry *entry)
+static enum peel_status peel_entry(struct ref_entry *entry, int repeel)
 {
 	enum peel_status status;
 
-	if (entry->flag & REF_KNOWS_PEELED)
-		return is_null_sha1(entry->u.value.peeled) ?
-			PEEL_NON_TAG : PEEL_PEELED;
+	if (entry->flag & REF_KNOWS_PEELED) {
+		if (repeel) {
+			entry->flag &= ~REF_KNOWS_PEELED;
+			hashclr(entry->u.value.peeled);
+		} else {
+			return is_null_sha1(entry->u.value.peeled) ?
+				PEEL_NON_TAG : PEEL_PEELED;
+		}
+	}
 	if (entry->flag & REF_ISBROKEN)
 		return PEEL_BROKEN;
 	if (entry->flag & REF_ISSYMREF)
@@ -1429,7 +1437,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
 
 	if (current_ref && (current_ref->name == refname
 			    || !strcmp(current_ref->name, refname))) {
-		if (peel_entry(current_ref))
+		if (peel_entry(current_ref, 0))
 			return -1;
 		hashcpy(sha1, current_ref->u.value.peeled);
 		return 0;
@@ -1449,7 +1457,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
 	if (flag & REF_ISPACKED) {
 		struct ref_entry *r = get_packed_ref(refname);
 		if (r) {
-			if (peel_entry(r))
+			if (peel_entry(r, 0))
 				return -1;
 			hashcpy(sha1, r->u.value.peeled);
 			return 0;
@@ -1998,7 +2006,7 @@ struct pack_refs_cb_data {
 static int pack_one_ref(struct ref_entry *entry, void *cb_data)
 {
 	struct pack_refs_cb_data *cb = cb_data;
-	struct object *o;
+	enum peel_status peel_status;
 	int is_tag_ref;
 
 	/* Do not pack symbolic or broken refs: */
@@ -2014,13 +2022,12 @@ static int pack_one_ref(struct ref_entry *entry, void *cb_data)
 	fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(entry->u.value.sha1),
 		entry->name);
 
-	o = parse_object_or_die(entry->u.value.sha1, entry->name);
-	if (o->type == OBJ_TAG) {
-		o = deref_tag(o, entry->name, 0);
-		if (o)
-			fprintf(cb->refs_file, "^%s\n",
-				sha1_to_hex(o->sha1));
-	}
+	peel_status = peel_entry(entry, 1);
+	if (peel_status == PEEL_PEELED)
+		fprintf(cb->refs_file, "^%s\n", sha1_to_hex(entry->u.value.peeled));
+	else if (peel_status != PEEL_NON_TAG)
+		die("internal error peeling reference %s (%s)",
+		    entry->name, sha1_to_hex(entry->u.value.sha1));
 
 	/* If the ref was already packed, there is no need to prune it. */
 	if ((cb->flags & PACK_REFS_PRUNE) && !(entry->flag & REF_ISPACKED)) {
@@ -2161,7 +2168,7 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)
 		return 0;
 	}
 
-	peel_status = peel_entry(entry);
+	peel_status = peel_entry(entry, 0);
 	write_packed_entry(*fd, entry->name, entry->u.value.sha1,
 			   peel_status == PEEL_PEELED ?
 			   entry->u.value.peeled : NULL);
-- 
1.8.2.1

[PATCH v2 30/33] pack_one_ref(): use write_packed_entry() to do the writing

[Michael Haggerty]

Change pack_refs() to work with a file descriptor instead of a FILE*
(making the file-locking code less awkward) and use
write_packed_entry() to do the writing.

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

diff --git a/refs.c b/refs.c
index f2d83f3..df670cb 100644
--- a/refs.c
+++ b/refs.c
@@ -2000,7 +2000,7 @@ struct ref_to_prune {
 struct pack_refs_cb_data {
 	unsigned int flags;
 	struct ref_to_prune *ref_to_prune;
-	FILE *refs_file;
+	int fd;
 };
 
 static int pack_one_ref(struct ref_entry *entry, void *cb_data)
@@ -2019,15 +2019,13 @@ static int pack_one_ref(struct ref_entry *entry, void *cb_data)
 	    !(entry->flag & REF_ISPACKED))
 		return 0;
 
-	fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(entry->u.value.sha1),
-		entry->name);
-
 	peel_status = peel_entry(entry, 1);
-	if (peel_status == PEEL_PEELED)
-		fprintf(cb->refs_file, "^%s\n", sha1_to_hex(entry->u.value.peeled));
-	else if (peel_status != PEEL_NON_TAG)
+	if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
 		die("internal error peeling reference %s (%s)",
 		    entry->name, sha1_to_hex(entry->u.value.sha1));
+	write_packed_entry(cb->fd, entry->name, entry->u.value.sha1,
+			   peel_status == PEEL_PEELED ?
+			   entry->u.value.peeled : NULL);
 
 	/* If the ref was already packed, there is no need to prune it. */
 	if ((cb->flags & PACK_REFS_PRUNE) && !(entry->flag & REF_ISPACKED)) {
@@ -2096,32 +2094,17 @@ static struct lock_file packlock;
 
 int pack_refs(unsigned int flags)
 {
-	int fd;
 	struct pack_refs_cb_data cbdata;
 
 	memset(&cbdata, 0, sizeof(cbdata));
 	cbdata.flags = flags;
 
-	fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"),
-				       LOCK_DIE_ON_ERROR);
-	cbdata.refs_file = fdopen(fd, "w");
-	if (!cbdata.refs_file)
-		die_errno("unable to create ref-pack file structure");
+	cbdata.fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"),
+					      LOCK_DIE_ON_ERROR);
 
-	/* perhaps other traits later as well */
-	fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n");
+	write_or_die(cbdata.fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
 
 	do_for_each_entry(NULL, "", pack_one_ref, &cbdata);
-	if (ferror(cbdata.refs_file))
-		die("failed to write ref-pack file");
-	if (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))
-		die_errno("failed to write ref-pack file");
-	/*
-	 * Since the lock file was fdopen()'ed and then fclose()'ed above,
-	 * assign -1 to the lock file descriptor so that commit_lock_file()
-	 * won't try to close() it.
-	 */
-	packlock.fd = -1;
 	if (commit_lock_file(&packlock) < 0)
 		die_errno("unable to overwrite old ref-pack file");
 	prune_refs(cbdata.ref_to_prune);
-- 
1.8.2.1

[PATCH v2 31/33] pack_one_ref(): do some cheap tests before a more expensive one

[Michael Haggerty]

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

diff --git a/refs.c b/refs.c
index df670cb..b1cda1b 100644
--- a/refs.c
+++ b/refs.c
@@ -2007,18 +2007,17 @@ static int pack_one_ref(struct ref_entry *entry, void *cb_data)
 {
 	struct pack_refs_cb_data *cb = cb_data;
 	enum peel_status peel_status;
-	int is_tag_ref;
-
-	/* Do not pack symbolic or broken refs: */
-	if ((entry->flag & REF_ISSYMREF) || !ref_resolves_to_object(entry))
-		return 0;
-	is_tag_ref = !prefixcmp(entry->name, "refs/tags/");
+	int is_tag_ref = !prefixcmp(entry->name, "refs/tags/");
 
 	/* ALWAYS pack refs that were already packed or are tags */
 	if (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref &&
 	    !(entry->flag & REF_ISPACKED))
 		return 0;
 
+	/* Do not pack symbolic or broken refs: */
+	if ((entry->flag & REF_ISSYMREF) || !ref_resolves_to_object(entry))
+		return 0;
+
 	peel_status = peel_entry(entry, 1);
 	if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
 		die("internal error peeling reference %s (%s)",
-- 
1.8.2.1

[PATCH v2 32/33] refs: change do_for_each_*() functions to take ref_cache arguments

[Michael Haggerty]

Change the callers convert submodule names into ref_cache pointers.

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

diff --git a/refs.c b/refs.c
index b1cda1b..f246b77 100644
--- a/refs.c
+++ b/refs.c
@@ -1503,16 +1503,15 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)
 }
 
 /*
- * Call fn for each reference in the specified submodule, omitting
+ * Call fn for each reference in the specified ref_cache, omitting
  * references not in the containing_dir of base.  fn is called for all
  * references, including broken ones.  If fn ever returns a non-zero
  * value, stop the iteration and return that value; otherwise, return
  * 0.
  */
-static int do_for_each_entry(const char *submodule, const char *base,
+static int do_for_each_entry(struct ref_cache *refs, const char *base,
 			     each_ref_entry_fn fn, void *cb_data)
 {
-	struct ref_cache *refs = get_ref_cache(submodule);
 	struct ref_dir *packed_dir = get_packed_refs(refs);
 	struct ref_dir *loose_dir = get_loose_refs(refs);
 	int retval = 0;
@@ -1541,7 +1540,7 @@ static int do_for_each_entry(const char *submodule, const char *base,
 }
 
 /*
- * Call fn for each reference in the specified submodule for which the
+ * Call fn for each reference in the specified ref_cache for which the
  * refname begins with base.  If trim is non-zero, then trim that many
  * characters off the beginning of each refname before passing the
  * refname to fn.  flags can be DO_FOR_EACH_INCLUDE_BROKEN to include
@@ -1549,8 +1548,8 @@ static int do_for_each_entry(const char *submodule, const char *base,
  * value, stop the iteration and return that value; otherwise, return
  * 0.
  */
-static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,
-			   int trim, int flags, void *cb_data)
+static int do_for_each_ref(struct ref_cache *refs, const char *base,
+			   each_ref_fn fn, int trim, int flags, void *cb_data)
 {
 	struct ref_entry_cb data;
 	data.base = base;
@@ -1559,7 +1558,7 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn
 	data.fn = fn;
 	data.cb_data = cb_data;
 
-	return do_for_each_entry(submodule, base, do_one_ref, &data);
+	return do_for_each_entry(refs, base, do_one_ref, &data);
 }
 
 static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
@@ -1592,23 +1591,23 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
 
 int for_each_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(NULL, "", fn, 0, 0, cb_data);
+	return do_for_each_ref(get_ref_cache(NULL), "", fn, 0, 0, cb_data);
 }
 
 int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(submodule, "", fn, 0, 0, cb_data);
+	return do_for_each_ref(get_ref_cache(submodule), "", fn, 0, 0, cb_data);
 }
 
 int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(NULL, prefix, fn, strlen(prefix), 0, cb_data);
+	return do_for_each_ref(get_ref_cache(NULL), prefix, fn, strlen(prefix), 0, cb_data);
 }
 
 int for_each_ref_in_submodule(const char *submodule, const char *prefix,
 		each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(submodule, prefix, fn, strlen(prefix), 0, cb_data);
+	return do_for_each_ref(get_ref_cache(submodule), prefix, fn, strlen(prefix), 0, cb_data);
 }
 
 int for_each_tag_ref(each_ref_fn fn, void *cb_data)
@@ -1643,7 +1642,7 @@ int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void *c
 
 int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(NULL, "refs/replace/", fn, 13, 0, cb_data);
+	return do_for_each_ref(get_ref_cache(NULL), "refs/replace/", fn, 13, 0, cb_data);
 }
 
 int head_ref_namespaced(each_ref_fn fn, void *cb_data)
@@ -1666,7 +1665,7 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
 	struct strbuf buf = STRBUF_INIT;
 	int ret;
 	strbuf_addf(&buf, "%srefs/", get_git_namespace());
-	ret = do_for_each_ref(NULL, buf.buf, fn, 0, 0, cb_data);
+	ret = do_for_each_ref(get_ref_cache(NULL), buf.buf, fn, 0, 0, cb_data);
 	strbuf_release(&buf);
 	return ret;
 }
@@ -1708,7 +1707,7 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
 
 int for_each_rawref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(NULL, "", fn, 0,
+	return do_for_each_ref(get_ref_cache(NULL), "", fn, 0,
 			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
 }
 
@@ -2103,7 +2102,7 @@ int pack_refs(unsigned int flags)
 
 	write_or_die(cbdata.fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
 
-	do_for_each_entry(NULL, "", pack_one_ref, &cbdata);
+	do_for_each_entry(get_ref_cache(NULL), "", pack_one_ref, &cbdata);
 	if (commit_lock_file(&packlock) < 0)
 		die_errno("unable to overwrite old ref-pack file");
 	prune_refs(cbdata.ref_to_prune);
-- 
1.8.2.1

[PATCH v2 33/33] refs: handle the main ref_cache specially

[Michael Haggerty]

Hold the ref_cache instance for the main repository in a dedicated,
statically-allocated instance to avoid the need for a function call
and a linked-list traversal when it is needed.

Suggested by: Heiko Voigt <hvoigt@hvoigt.net>

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

diff --git a/refs.c b/refs.c
index f246b77..d17931a 100644
--- a/refs.c
+++ b/refs.c
@@ -810,9 +810,13 @@ static struct ref_cache {
 	struct ref_cache *next;
 	struct ref_entry *loose;
 	struct ref_entry *packed;
-	/* The submodule name, or "" for the main repo. */
-	char name[FLEX_ARRAY];
-} *ref_cache;
+	/*
+	 * The submodule name, or "" for the main repo.  We allocate
+	 * length 1 rather than FLEX_ARRAY so that the main ref_cache
+	 * is initialized correctly.
+	 */
+	char name[1];
+} ref_cache, *submodule_ref_caches;
 
 static void clear_packed_ref_cache(struct ref_cache *refs)
 {
@@ -850,18 +854,18 @@ static struct ref_cache *create_ref_cache(const char *submodule)
  */
 static struct ref_cache *get_ref_cache(const char *submodule)
 {
-	struct ref_cache *refs = ref_cache;
-	if (!submodule)
-		submodule = "";
-	while (refs) {
+	struct ref_cache *refs;
+
+	if (!submodule || !*submodule)
+		return &ref_cache;
+
+	for (refs = submodule_ref_caches; refs; refs = refs->next)
 		if (!strcmp(submodule, refs->name))
 			return refs;
-		refs = refs->next;
-	}
 
 	refs = create_ref_cache(submodule);
-	refs->next = ref_cache;
-	ref_cache = refs;
+	refs->next = submodule_ref_caches;
+	submodule_ref_caches = refs;
 	return refs;
 }
 
@@ -1010,8 +1014,8 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)
 
 void add_packed_ref(const char *refname, const unsigned char *sha1)
 {
-	add_ref(get_packed_refs(get_ref_cache(NULL)),
-			create_ref_entry(refname, sha1, REF_ISPACKED, 1));
+	add_ref(get_packed_refs(&ref_cache),
+		create_ref_entry(refname, sha1, REF_ISPACKED, 1));
 }
 
 /*
@@ -1186,7 +1190,7 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh
  */
 static struct ref_entry *get_packed_ref(const char *refname)
 {
-	return find_ref(get_packed_refs(get_ref_cache(NULL)), refname);
+	return find_ref(get_packed_refs(&ref_cache), refname);
 }
 
 const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)
@@ -1591,7 +1595,7 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
 
 int for_each_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(get_ref_cache(NULL), "", fn, 0, 0, cb_data);
+	return do_for_each_ref(&ref_cache, "", fn, 0, 0, cb_data);
 }
 
 int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
@@ -1601,7 +1605,7 @@ int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
 
 int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(get_ref_cache(NULL), prefix, fn, strlen(prefix), 0, cb_data);
+	return do_for_each_ref(&ref_cache, prefix, fn, strlen(prefix), 0, cb_data);
 }
 
 int for_each_ref_in_submodule(const char *submodule, const char *prefix,
@@ -1642,7 +1646,7 @@ int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void *c
 
 int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(get_ref_cache(NULL), "refs/replace/", fn, 13, 0, cb_data);
+	return do_for_each_ref(&ref_cache, "refs/replace/", fn, 13, 0, cb_data);
 }
 
 int head_ref_namespaced(each_ref_fn fn, void *cb_data)
@@ -1665,7 +1669,7 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
 	struct strbuf buf = STRBUF_INIT;
 	int ret;
 	strbuf_addf(&buf, "%srefs/", get_git_namespace());
-	ret = do_for_each_ref(get_ref_cache(NULL), buf.buf, fn, 0, 0, cb_data);
+	ret = do_for_each_ref(&ref_cache, buf.buf, fn, 0, 0, cb_data);
 	strbuf_release(&buf);
 	return ret;
 }
@@ -1707,7 +1711,7 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
 
 int for_each_rawref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(get_ref_cache(NULL), "", fn, 0,
+	return do_for_each_ref(&ref_cache, "", fn, 0,
 			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
 }
 
@@ -1913,7 +1917,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	 * name is a proper prefix of our refname.
 	 */
 	if (missing &&
-	     !is_refname_available(refname, NULL, get_packed_refs(get_ref_cache(NULL)))) {
+	     !is_refname_available(refname, NULL, get_packed_refs(&ref_cache))) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -2102,7 +2106,7 @@ int pack_refs(unsigned int flags)
 
 	write_or_die(cbdata.fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
 
-	do_for_each_entry(get_ref_cache(NULL), "", pack_one_ref, &cbdata);
+	do_for_each_entry(&ref_cache, "", pack_one_ref, &cbdata);
 	if (commit_lock_file(&packlock) < 0)
 		die_errno("unable to overwrite old ref-pack file");
 	prune_refs(cbdata.ref_to_prune);
@@ -2160,7 +2164,6 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)
 static int repack_without_ref(const char *refname)
 {
 	int fd;
-	struct ref_cache *refs = get_ref_cache(NULL);
 	struct ref_dir *packed;
 
 	if (!get_packed_ref(refname))
@@ -2171,8 +2174,8 @@ static int repack_without_ref(const char *refname)
 		unable_to_lock_error(git_path("packed-refs"), errno);
 		return error("cannot delete '%s' from packed refs", refname);
 	}
-	clear_packed_ref_cache(refs);
-	packed = get_packed_refs(refs);
+	clear_packed_ref_cache(&ref_cache);
+	packed = get_packed_refs(&ref_cache);
 	/* Remove refname from the cache. */
 	if (remove_entry(packed, refname) == -1) {
 		/*
@@ -2212,7 +2215,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	ret |= repack_without_ref(lock->ref_name);
 
 	unlink_or_warn(git_path("logs/%s", lock->ref_name));
-	clear_loose_ref_cache(get_ref_cache(NULL));
+	clear_loose_ref_cache(&ref_cache);
 	unlock_ref(lock);
 	return ret;
 }
@@ -2234,7 +2237,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
 	const char *symref = NULL;
-	struct ref_cache *refs = get_ref_cache(NULL);
 
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldrefname);
@@ -2246,10 +2248,10 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	if (!symref)
 		return error("refname %s not found", oldrefname);
 
-	if (!is_refname_available(newrefname, oldrefname, get_packed_refs(refs)))
+	if (!is_refname_available(newrefname, oldrefname, get_packed_refs(&ref_cache)))
 		return 1;
 
-	if (!is_refname_available(newrefname, oldrefname, get_loose_refs(refs)))
+	if (!is_refname_available(newrefname, oldrefname, get_loose_refs(&ref_cache)))
 		return 1;
 
 	if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
@@ -2505,7 +2507,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	clear_loose_ref_cache(get_ref_cache(NULL));
+	clear_loose_ref_cache(&ref_cache);
 	if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
 	    (strcmp(lock->ref_name, lock->orig_ref_name) &&
 	     log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
-- 
1.8.2.1

Re: [PATCH v2 17/33] repack_without_ref(): silence errors for dangling packed refs

[Junio C Hamano]

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

> Stop emitting an error message when deleting a packed reference if we
> find another dangling packed reference that is overridden by a loose
> reference.  See the previous commit for a longer explanation of the
> issue.
>
> We have to be careful to make sure that the invalid packed reference
> really *is* overridden by a loose reference; otherwise what we have
> found is repository corruption, which we *should* report.
>
> Please note that this approach is vulnerable to a race condition
> similar to the race conditions already known to affect packed
> references [1]:
>
> * Process 1 tries to peel packed reference X as part of deleting
>   another packed reference.  It discovers that X does not refer to a
>   valid object (because the object that it referred to has been
>   garbage collected).
>
> * Process 2 tries to delete reference X.  It starts by deleting the
>   loose reference X.
>
> * Process 1 checks whether there is a loose reference X.  There is not
>   (it has just been deleted by process 2), so process 1 reports a
>   spurious error "X does not point to a valid object!"
>
> The worst case seems relatively harmless, and the fix is identical to
> the fix that will be needed for the other race conditions (namely
> holding a lock on the packed-refs file during *all* reference
> deletions), so we leave the cleaning up of all of them as a future
> project.
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/211956
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

Clean and straight-forward implementation that is very well
explained.  I like this much better than the previous round ;-).

Thanks, will re-queue.

>  refs.c               | 37 +++++++++++++++++++++++++++++++++++--
>  t/t3210-pack-refs.sh |  2 +-
>  2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index ed54ed4..2957f6d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1901,8 +1901,41 @@ static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)
>  
>  	if (!strcmp(data->refname, entry->name))
>  		return 0;
> -	if (!ref_resolves_to_object(entry))
> -		return 0; /* Skip broken refs */
> +	if (entry->flag & REF_ISBROKEN) {
> +		/* This shouldn't happen to packed refs. */
> +		error("%s is broken!", entry->name);
> +		return 0;
> +	}
> +	if (!has_sha1_file(entry->u.value.sha1)) {
> +		unsigned char sha1[20];
> +		int flags;
> +
> +		if (read_ref_full(entry->name, sha1, 0, &flags))
> +			/* We should at least have found the packed ref. */
> +			die("Internal error");
> +		if ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED))
> +			/*
> +			 * This packed reference is overridden by a
> +			 * loose reference, so it is OK that its value
> +			 * is no longer valid; for example, it might
> +			 * refer to an object that has been garbage
> +			 * collected.  For this purpose we don't even
> +			 * care whether the loose reference itself is
> +			 * invalid, broken, symbolic, etc.  Silently
> +			 * omit the packed reference from the output.
> +			 */
> +			return 0;
> +		/*
> +		 * There is no overriding loose reference, so the fact
> +		 * that this reference doesn't refer to a valid object
> +		 * indicates some kind of repository corruption.
> +		 * Report the problem, then omit the reference from
> +		 * the output.
> +		 */
> +		error("%s does not point to a valid object!", entry->name);
> +		return 0;
> +	}
> +
>  	len = snprintf(line, sizeof(line), "%s %s\n",
>  		       sha1_to_hex(entry->u.value.sha1), entry->name);
>  	/* this should not happen but just being defensive */
> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
> index c032d88..559f602 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t3210-pack-refs.sh
> @@ -142,7 +142,7 @@ test_expect_success 'delete ref with dangling packed version' '
>  	test_cmp /dev/null result
>  '
>  
> -test_expect_failure 'delete ref while another dangling packed ref' '
> +test_expect_success 'delete ref while another dangling packed ref' '
>  	git branch lamb &&
>  	git commit --allow-empty -m "future garbage" &&
>  	git pack-refs --all &&

Re: [PATCH v2 00/33] Various cleanups around reference packing and peeling

[Junio C Hamano]

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

> For now I left the sleeps in t3210.  Given that the problem will be
> solved by topic jc/prune-all, building a fancier workaround into this
> test for the old broken --expire behavior seems like a waste of time.
> I propose that the sleeps be removed when this patch series is merged
> with jc/prune-all.

I wouldn't mind to queue this on top of the "prune --expire=all" topic
at all, especially if that makes the tests easier.

Overall looks very cleanly done (I only let my eyes coast over them,
and need another round to read it over).

Thanks.

[PATCH] fixup! t3210: test for spurious error messages for dangling packed refs

[Michael Haggerty]

On 04/23/2013 12:23 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> For now I left the sleeps in t3210.  Given that the problem will be
>> solved by topic jc/prune-all, building a fancier workaround into this
>> test for the old broken --expire behavior seems like a waste of time.
>> I propose that the sleeps be removed when this patch series is merged
>> with jc/prune-all.
> 
> I wouldn't mind to queue this on top of the "prune --expire=all" topic
> at all, especially if that makes the tests easier.

OK then let's do it that way.  The prune-all branch is not risky so I
can't imagine that it will hold up my changes.

If you rebase my patches on top of jc/prune-all, then you can
autosquash this patch to remove the sleeps from

    [PATCH v2 16/33] t3210: test for spurious error messages for dangling packed refs

Let me know if you would prefer that I re-roll.

Michael

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3210-pack-refs.sh | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 559f602..1a2080e 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -122,9 +122,8 @@ test_expect_success 'explicit pack-refs with dangling packed reference' '
 	git commit --allow-empty -m "soon to be garbage-collected" &&
 	git pack-refs --all &&
 	git reset --hard HEAD^ &&
-	sleep 1 &&
-	git reflog expire --expire=now --all &&
-	git prune --expire=now &&
+	git reflog expire --expire=all --all &&
+	git prune --expire=all &&
 	git pack-refs --all 2>result &&
 	test_cmp /dev/null result
 '
@@ -135,9 +134,8 @@ test_expect_success 'delete ref with dangling packed version' '
 	git pack-refs --all &&
 	git reset --hard HEAD^ &&
 	git checkout master &&
-	sleep 1 &&
-	git reflog expire --expire=now --all &&
-	git prune --expire=now &&
+	git reflog expire --expire=all --all &&
+	git prune --expire=all &&
 	git branch -d lamb 2>result &&
 	test_cmp /dev/null result
 '
@@ -147,9 +145,8 @@ test_expect_success 'delete ref while another dangling packed ref' '
 	git commit --allow-empty -m "future garbage" &&
 	git pack-refs --all &&
 	git reset --hard HEAD^ &&
-	sleep 1 &&
-	git reflog expire --expire=now --all &&
-	git prune --expire=now &&
+	git reflog expire --expire=all --all &&
+	git prune --expire=all &&
 	git branch -d lamb 2>result &&
 	test_cmp /dev/null result
 '
-- 
1.8.2.1

Re: [PATCH] fixup! t3210: test for spurious error messages for dangling packed refs

[Junio C Hamano]

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

> Let me know if you would prefer that I re-roll.

Your fix-up cleanly applied to the result of applying the series up
to 16/33 and it was trivial to squash it in.

Please holler if what I push out on 'pu' in 8 hours or so looks
wrong.

Thanks.

Re: [PATCH] fixup! t3210: test for spurious error messages for dangling packed refs

[Michael Haggerty]

On 04/23/2013 07:50 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Let me know if you would prefer that I re-roll.
> 
> Your fix-up cleanly applied to the result of applying the series up
> to 16/33 and it was trivial to squash it in.
> 
> Please holler if what I push out on 'pu' in 8 hours or so looks
> wrong.

It looks like you did the right thing, except using v1 of my patch
series rather than v2.  Please do the same procedure with v2: apply the
v2 series on top of jc/prune-all, apply the fixup commit "fixup! t3210",
and rebase the fixup to autosquash it onto patch 16/33.

Thanks,
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH] fixup! t3210: test for spurious error messages for dangling packed refs

[Michael Haggerty]

On 04/24/2013 05:25 PM, Michael Haggerty wrote:
> On 04/23/2013 07:50 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> Let me know if you would prefer that I re-roll.
>>
>> Your fix-up cleanly applied to the result of applying the series up
>> to 16/33 and it was trivial to squash it in.
>>
>> Please holler if what I push out on 'pu' in 8 hours or so looks
>> wrong.
> 
> It looks like you did the right thing, except using v1 of my patch
> series rather than v2.  Please do the same procedure with v2: apply the
> v2 series on top of jc/prune-all, apply the fixup commit "fixup! t3210",
> and rebase the fixup to autosquash it onto patch 16/33.

The new version (0859ff6fe6) that I just fetched from you looks right.

Thanks!
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/