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

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

[Michael Haggerty]

The highlight of this patch series are that (1) peeled references are
not lost from the packed-refs file when a packed reference is deleted
and (2) there is more code sharing between the code used by "git
packed-refs" and the code used by repack_without_ref() for deleting a
packed reference.  Along the way I have added a lot of documentation
and fixed several smaller bugs.

I had to add some sleeps in test t3210 but I hope that somebody can
find a way to eliminate them.

The last patch implements a change that I brushed off a long time ago
when Heiko suggested it.  He was right (probably not for performance
reasons, but because it simplifies the code).

Summary:

Patches 1-4 document existing code.

Patches 5-6 random cleanup.

Patches 7-9 change the semantics of get_packed_ref() to make it more
reusable and change other locations to use this function.

Patch 10 extracts a function ref_resolves_to_object().

Patches 11-14 unify reference-peeling in two new functions,
peel_object() and peel_entry() (and fix an inconsistency in
peel_ref()).

Patch 15 introduces a new internal reference-iteration API and adjusts
some internal code to use it.  The new API gives the callback function
direct access to the ref_entry.  This corrects the handling of
current_ref during an iteration over only packed references.

Patches 16-17 add and fix a test of a spurious error message.

Patches 18-22 change the reference-deleting code to write peeled refs
to the packed-refs file.  (Previously, whenever a packed reference was
deleted, all of the peeled values were lost when the packed-refs file
was rewritten.)

Patches 23-31 move the code from pack-refs.{c,h} into refs.{c,h},
adjust it to its new surroundings, and change it to share some code
with repack_without_ref().

Patches 32-33 simplify access to the main reference cache.

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               | 699 ++++++++++++++++++++++++++++++++++++++-------------
 refs.h               |  35 +++
 t/t3210-pack-refs.sh |  36 +++
 t/t3211-peel-ref.sh  |   9 +
 9 files changed, 609 insertions(+), 341 deletions(-)
 delete mode 100644 pack-refs.c
 delete mode 100644 pack-refs.h

-- 
1.8.2.1

[PATCH 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 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 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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 1df1ccd..f503ec4 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,13 @@ 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.
+ */
 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

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

[Junio C Hamano]

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

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 1df1ccd..f503ec4 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,13 @@ 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.
> + */

Early termination due to "fn()" returning non-zero needs to be
documented here, no?

>  static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,
>  			   int trim, int flags, void *cb_data)
>  {

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

[Michael Haggerty]

On 04/15/2013 07:38 PM, Junio C Hamano wrote:
>> +/*
>> + * 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.
>> + */
> 
> Early termination due to "fn()" returning non-zero needs to be
> documented here, no?
> 
>>  static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,

Correct, thanks.  Will be fixed in re-roll.

Michael

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

[PATCH 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 f503ec4..88ff153 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 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 88ff153..7cbf228 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 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 7cbf228..9f508dd 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 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 9f508dd..de5dc7d 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 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 de5dc7d..cf017a5 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 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 cf017a5..c523978 100644
--- a/refs.c
+++ b/refs.c
@@ -1818,9 +1818,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 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 c523978..dfc8600 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

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

[Junio C Hamano]

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

> 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 c523978..dfc8600 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;
> +

The original says "Unless flags tell us to include broken ones,
return 0 for the broken ones and the ones that point at invalid
objects".

The updated says "Unless flags tell us to include broken ones or the
ref is a good one, return 0".

Are they the same?  If we have a good one, and if we are not told to
include broken one, the original just passes the control down to the
remainder of the function.  The updated one will return 0.

Oh, wait, that is not the case.  The OR is inside !( A || B ), so
what it really wants to say is:

	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
            !ref_resolves_to_object(entry))
		return 0;

that is, "When we are told to exclude broken ones and the one we are
looking at is broken, return 0".

Am I the only one who was confused with this, or was the way the
patch wrote this logic unnecessarily convoluted?

>  	current_ref = entry;
>  	retval = fn(entry->name + trim, entry->u.value.sha1, entry->flag, cb_data);
>  	current_ref = NULL;

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

[Michael Haggerty]

On 04/15/2013 06:51 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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 c523978..dfc8600 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;
>> +
> 
> The original says "Unless flags tell us to include broken ones,
> return 0 for the broken ones and the ones that point at invalid
> objects".
> 
> The updated says "Unless flags tell us to include broken ones or the
> ref is a good one, return 0".
> 
> Are they the same?  If we have a good one, and if we are not told to
> include broken one, the original just passes the control down to the
> remainder of the function.  The updated one will return 0.
> 
> Oh, wait, that is not the case.  The OR is inside !( A || B ), so
> what it really wants to say is:
> 
> 	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
>             !ref_resolves_to_object(entry))
> 		return 0;
> 
> that is, "When we are told to exclude broken ones and the one we are
> looking at is broken, return 0".
> 
> Am I the only one who was confused with this, or was the way the
> patch wrote this logic unnecessarily convoluted?

I find either way of writing it hard to read quickly.  I find that the
construct

    if (!(situation in which the function should continue))
        return

makes it easier to pick out the "situation in which the function should
continue".  But granted, the nesting of multiple parentheses across
lines compromises the clarity.

In projects where I can choose the coding standard, I like to use extra
whitespace to help the eye pick out the range of parentheses, like

        if (!(
                (flags & DO_FOR_EACH_INCLUDE_BROKEN) ||
                ref_resolves_to_object(entry)
                ))
                return 0;

but I've never seen this style in the Git project so I haven't used it here.

Since you prefer the other version, I will change it.

Michael

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

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

[Junio C Hamano]

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

> In projects where I can choose the coding standard, I like to use extra
> whitespace to help the eye pick out the range of parentheses, like
>
>         if (!(
>                 (flags & DO_FOR_EACH_INCLUDE_BROKEN) ||
>                 ref_resolves_to_object(entry)
>                 ))
>                 return 0;
>
> but I've never seen this style in the Git project so I haven't used it here.

I _think_ at least to me, it is not the textual grouping style but
the "unless X we skip the rest" logic itself that confused me.  It
took the same number of minutes to grok the above as the original.

We skip the rest of this function unless the condition inside !()
holds, and the condition is.... we are told to include broken ones,
in which case we know we will do the remainder without checking
anything else, or we do the checking and find that it is not broken.

I suspect the root cause of the confusion to force the double
negation is INCLUDE_BROKEN; we have to express "when we are told to
ignore broken one and this thing is broken, ignore it" without
negation, i.e.

	if ( !(flags & INCLUDE_BROKEN) && is_broken(thing) )
		return;

which would have been much more pleasant to read if it were

	if ( (flags & IGNORE_BROKEN) && is_broken(thing) )
		return;

Unfortunately, we cannot change it to IGNORE_BROKEN and flip the
meaning of the bit, because almost all callers do want to ignore
broken ones.

Either way is fine by me, even though I find that !(A || B) needs a
bit more mental exercise to grok than (!A && !B).  Perhaps it is
just me who is not so strong at math ;-)

[PATCH 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 dfc8600..a1fe6b0 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 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 a1fe6b0..84c2497 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

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

[Junio C Hamano]

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

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

This is a valid rewrite because all existing callers check if the
return value is 0 and does not check the error code against -1.

Looking good so far.

[PATCH 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 did not trigger the previously-buggy behavior.

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 84c2497..a0d8e32 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

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

[Junio C Hamano]

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

> 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 did not trigger the previously-buggy behavior.

Is that because we were lucky by codeflow, or is it just that we
didn't have a testcase to trigger the behaviour?

>
> 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 84c2497..a0d8e32 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. **/

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

[Michael Haggerty]

On 04/15/2013 07:38 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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 did not trigger the previously-buggy behavior.
> 
> Is that because we were lucky by codeflow, or is it just that we
> didn't have a testcase to trigger the behaviour?

Existing callers only called peel_ref() from within a for_each_ref-style
iteration and only for the current ref.  Therefore the buggy code path
was impossible to reach.

I will note that in the commit message.

Michael

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

[PATCH 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 | 61 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 13 deletions(-)

diff --git a/refs.c b/refs.c
index a0d8e32..8cc6109 100644
--- a/refs.c
+++ b/refs.c
@@ -1286,6 +1286,16 @@ enum peel_status {
 
 	/* object cannot be peeled because it is not a tag: */
 	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

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

[Junio C Hamano]

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

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

This code makes the reader wonder what happens when a new loose ref
masks a stale packed ref, but the worry is unfounded because the
read_ref_full() wouldn't have gave us REF_ISPACKED in the flag in
such a case.

But somehow the calling sequence looks like such a mistake waiting
to happen.  It would be much more clear if a function that returns a
"struct ref_entry *" is used instead of read_ref_full() above, and
we checked (r->flag & REF_ISPACKED) in the conditional, without a
separate 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;
>  		}

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

[Michael Haggerty]

On 04/15/2013 07:38 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>  	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);
> 
> This code makes the reader wonder what happens when a new loose ref
> masks a stale packed ref, but the worry is unfounded because the
> read_ref_full() wouldn't have gave us REF_ISPACKED in the flag in
> such a case.
> 
> But somehow the calling sequence looks like such a mistake waiting
> to happen.  It would be much more clear if a function that returns a
> "struct ref_entry *" is used instead of read_ref_full() above, and
> we checked (r->flag & REF_ISPACKED) in the conditional, without a
> separate get_packed_ref(refname).

As I'm sure you realize, I didn't change the code that you are referring
to; I just added a comment.

But yes, I sympathize with your complaint.  Additionally, the code has
the drawback that get_packed_ref() is called twice: once in
read_ref_full() and again in the if block here.  Unfortunately, this
isn't so easy to fix because read_ref_full() doesn't use the loose
reference cache, so the reference that it returns might not even have a
ref_entry associated with it (specifically, unless the returned flag
value has REF_ISPACKED set).  So there are a couple options:

* Always read loose references through the cache; that way there would
always be a ref_entry in which the return value could be presented.
This would not be a good idea at the moment because the loose reference
cache is populated one directory at a time, and reading a whole
directory of loose references could be expensive.  So before
implementing this, it would be advisable to change the code to populate
the loose reference cache more selectively when single loose references
are needed.  -> This approach would be well beyond the scope of this
patch series.

* Implement a function like read_ref_full() with an additional (struct
ref_entry **entry) argument that is written to *in the case* that the
reference that was returned has a ref_entry associated with it, and NULL
otherwise.  This would have to be an internal function because we don't
want to expose the ref_entry structure outside of refs.c.
read_ref_full() would be implemented on top of the new function.

Either way, I'd rather put this idea on my TODO list for another time.

Michael

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

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

[Junio C Hamano]

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

> On 04/15/2013 07:38 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>>>  	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);
>> 
>> This code makes the reader wonder what happens when a new loose ref
>> masks a stale packed ref, but the worry is unfounded because the
>> read_ref_full() wouldn't have gave us REF_ISPACKED in the flag in
>> such a case.
>> 
>> But somehow the calling sequence looks like such a mistake waiting
>> to happen.  It would be much more clear if a function that returns a
>> "struct ref_entry *" is used instead of read_ref_full() above, and
>> we checked (r->flag & REF_ISPACKED) in the conditional, without a
>> separate get_packed_ref(refname).
>
> As I'm sure you realize, I didn't change the code that you are referring
> to; I just added a comment.

Yes.

> But yes, I sympathize with your complaint.  Additionally, the code has
> the drawback that get_packed_ref() is called twice: once in
> read_ref_full() and again in the if block here.  Unfortunately, this
> isn't so easy to fix because read_ref_full() doesn't use the loose
> reference cache, so the reference that it returns might not even have a
> ref_entry associated with it (specifically, unless the returned flag
> value has REF_ISPACKED set).  So there are a couple options:
>
> * Always read loose references through the cache; that way there would
> always be a ref_entry in which the return value could be presented.
> This would not be a good idea at the moment because the loose reference
> cache is populated one directory at a time, and reading a whole
> directory of loose references could be expensive.  So before
> implementing this, it would be advisable to change the code to populate
> the loose reference cache more selectively when single loose references
> are needed.  -> This approach would be well beyond the scope of this
> patch series.
>
> * Implement a function like read_ref_full() with an additional (struct
> ref_entry **entry) argument that is written to *in the case* that the
> reference that was returned has a ref_entry associated with it, and NULL
> otherwise.  This would have to be an internal function because we don't
> want to expose the ref_entry structure outside of refs.c.
> read_ref_full() would be implemented on top of the new function.

Isn't there another?

Instead of using read_ref_full() at this callsite, can it call a
function, given a refname, returns a pointer to "struct ref_entry",
using the proper "a loose ref covers the packed ref with the same
name" semantics?

I guess that may need the same machinery for your first option to
implement it efficiently; right now read_ref_full() goes directly to
the single file that would hold the named ref without scanning and
populating sibling refs in the in-core loose ref hierarchy, and
without doing so you cannot return a struct ref_entry.  Hmm...

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

[Michael Haggerty]

On 04/16/2013 07:55 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> On 04/15/2013 07:38 PM, Junio C Hamano wrote:
>>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>>
>>>>  	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);
>>>
>>> This code makes the reader wonder what happens when a new loose ref
>>> masks a stale packed ref, but the worry is unfounded because the
>>> read_ref_full() wouldn't have gave us REF_ISPACKED in the flag in
>>> such a case.
>>>
>>> But somehow the calling sequence looks like such a mistake waiting
>>> to happen.  It would be much more clear if a function that returns a
>>> "struct ref_entry *" is used instead of read_ref_full() above, and
>>> we checked (r->flag & REF_ISPACKED) in the conditional, without a
>>> separate get_packed_ref(refname).
>>
>> As I'm sure you realize, I didn't change the code that you are referring
>> to; I just added a comment.
> 
> Yes.
> 
>> But yes, I sympathize with your complaint.  Additionally, the code has
>> the drawback that get_packed_ref() is called twice: once in
>> read_ref_full() and again in the if block here.  Unfortunately, this
>> isn't so easy to fix because read_ref_full() doesn't use the loose
>> reference cache, so the reference that it returns might not even have a
>> ref_entry associated with it (specifically, unless the returned flag
>> value has REF_ISPACKED set).  So there are a couple options:
>>
>> * Always read loose references through the cache; that way there would
>> always be a ref_entry in which the return value could be presented.
>> This would not be a good idea at the moment because the loose reference
>> cache is populated one directory at a time, and reading a whole
>> directory of loose references could be expensive.  So before
>> implementing this, it would be advisable to change the code to populate
>> the loose reference cache more selectively when single loose references
>> are needed.  -> This approach would be well beyond the scope of this
>> patch series.
>>
>> * Implement a function like read_ref_full() with an additional (struct
>> ref_entry **entry) argument that is written to *in the case* that the
>> reference that was returned has a ref_entry associated with it, and NULL
>> otherwise.  This would have to be an internal function because we don't
>> want to expose the ref_entry structure outside of refs.c.
>> read_ref_full() would be implemented on top of the new function.
> 
> Isn't there another?
> 
> Instead of using read_ref_full() at this callsite, can it call a
> function, given a refname, returns a pointer to "struct ref_entry",
> using the proper "a loose ref covers the packed ref with the same
> name" semantics?
> 
> I guess that may need the same machinery for your first option to
> implement it efficiently; right now read_ref_full() goes directly to
> the single file that would hold the named ref without scanning and
> populating sibling refs in the in-core loose ref hierarchy, and
> without doing so you cannot return a struct ref_entry.  Hmm...

Yes, that's the crux.  Without the ref_cache, there is no persistent
ref_entry that can be returned to the caller.

Somewhere I have a prototype patch series (never submitted) that would
make it reasonable to access all references via the cache.  For single
reference accesses, it wouldn't read all of the loose references in the
whole directory but rather only create stub REF_INCOMPLETE ref_entries
for the refs that are not needed immediately.  These entries would be
filled when they are accessed.  There was also going to be a bulk read
command to be used when it is expected that the whole directory will be
needed.  With this change we really would have a ref_entry for every
reference and your suggestion would become plausible.  Someday...

But for the current issue it would be totally sufficient to return an
additional pointer to the ref_entry *if it is available*, because for
packed refs it will always be available.

Actually, for the current patch series I don't see the urgency of
changing *anything*.  The code works as is and is no worse than the old
code.  I'd rather do this change (if I get to it) in a separate series.

Michael

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

[PATCH 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 | 142 +++++++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 81 insertions(+), 61 deletions(-)

diff --git a/refs.c b/refs.c
index 8cc6109..51f68d3 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,14 +1425,12 @@ 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.
+ * 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.
  */
-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);
@@ -1443,24 +1445,41 @@ 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.
+ */
+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];
@@ -1870,20 +1889,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;
 }
@@ -1907,7 +1927,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

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

[Junio C Hamano]

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

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

Nicely done.

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

Hopefully that means "in later patches in this series" ;-)

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

[Michael Haggerty]

On 04/15/2013 07:38 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> [...]  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.
> 
> Hopefully that means "in later patches in this series" ;-)

I don't think that the rest of the series would have triggered this
problem either.  In fact, if I had written repack_without_ref()'s
peeling functionality using peel_ref(), then it would have *depended* on
this bug for its proper operation...otherwise it would have written the
peeled version of the loose ref to the packed-ref file.  Of course, it's
all pretty academic because the peeled version of a packed ref should
never be used when it is overridden by a loose ref, so the incorrect
peeled values in the packed-ref file shouldn't have any observable effects.

The real problem is that calling the old peel_ref() function on a packed
reference was illegitimate because the function only knew how to peel a
ref that was still active.  Plus it's kindof silly tucking away the
current reference in a global variable then looking it up again instead
of passing the ref_entry around.

Callers outside of refs.c could also not have triggered this bug because
they have no way to access overridden packed refs.

Michael

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

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

[Junio C Hamano]

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

>>> ...  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.
>> 
>> Hopefully that means "in later patches in this series" ;-)
>
> I don't think that the rest of the series would have triggered this
> problem either.

Yeah, I misread the message when I said "Hopefully". I somehow
thought it was saying "we will fix it in the future; otherwise
things can break in a confusing way", which is not the case.

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

How can I get rid of the sleeps in these tests?  I couldn't find
another portable way of reliably ensuring that commits get
garbage-collected.  (Some other tests use similar code without the
sleeps, but empirically the sleeps are necessary to get the tests to
work reliably.  Perhaps the other tests are not doing what they
think.)

 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

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

[Junio C Hamano]

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

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

That is one nasty recent bug.

>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>
> How can I get rid of the sleeps in these tests?

Would test-chmtime help?

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

[Michael Haggerty]

On 04/15/2013 07:39 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> How can I get rid of the sleeps in these tests?
> 
> Would test-chmtime help?

Maybe I should take a step back and ask why it isn't easier to expire
things regardless of age, which is sometimes a reasonable thing to do
even outside of test suites.  In particular, it seem to me that the most
obvious interpretation of

	git reflog expire --expire=now --all

would be that it expires *everything*.  But in fact it seems to only
expire things that are at least one second old, which doesn't seem at
all useful in the real world.  "--expire=all" is accepted without
complaint but doesn't do what one would hope.  Something like
"--expire=$(($(date +%s)+3600))" works, but it is not very convenient
(is it portable?).

I guess I can use test-chmtime for my particular test, though I will
have to pass it the explicit names of the logfile(s), like

    find .git/logs -type f -print0 | xargs -0 test-chmtime =-60

I guess that's what I'll do if no better solution comes up.

Michael

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

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

[Junio C Hamano]

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

> would be that it expires *everything*.  But in fact it seems to only
> expire things that are at least one second old, which doesn't seem at
> all useful in the real world.  "--expire=all" is accepted without
> complaint but doesn't do what one would hope.

Perhaps that is worth fixing, independent of this topic.

Approxidate gives the current time for anything it does not
understand, and that is how --expire=all is interpreted as "anything
older than now".  For that matter, even a string "now" has long been
interpreted as the current time with the same "I do not understand
it, so I'll give you the current timestamp" logic, until we added an
official support for "now" at 93cfa7c7a85e (approxidate_careful()
reports errorneous date string, 2010-01-26) for entirely different
reasons.

A completely untested patch for your enjoyment...

 builtin/reflog.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 72a0af7..9fa0e41 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -492,15 +492,28 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)
 	return ent;
 }
 
+static void parse_expire_value(const char *value, unsigned long *expire)
+{
+	if (!strcmp(value, "never") || !strcmp(value, "false"))
+		*expire = 0;
+	else if (!strcmp(value, "all") || !strcmp(value, "now"))
+		/*
+		 * We take over "now" here, which usually translates
+		 * to the current timestamp, because the user really
+		 * means everything she has done in the past, and by
+		 * definition reflogs are the record of the past,
+		 * there is nothing from the future to be kept.
+		 */
+		*expire = ULONG_MAX;
+	else
+		*expire = approxidate(value);
+}
+
 static int parse_expire_cfg_value(const char *var, const char *value, unsigned long *expire)
 {
 	if (!value)
 		return config_error_nonbool(var);
-	if (!strcmp(value, "never") || !strcmp(value, "false")) {
-		*expire = 0;
-		return 0;
-	}
-	*expire = approxidate(value);
+	parse_expire_value(value, expire);
 	return 0;
 }
 
@@ -614,11 +627,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
 			cb.dry_run = 1;
 		else if (!prefixcmp(arg, "--expire=")) {
-			cb.expire_total = approxidate(arg + 9);
+			parse_expire_value(arg + 9, &cb.expire_total);
 			explicit_expiry |= EXPIRE_TOTAL;
 		}
 		else if (!prefixcmp(arg, "--expire-unreachable=")) {
-			cb.expire_unreachable = approxidate(arg + 21);
+			parse_expire_value(arg + 21, &cb.expire_unreachable);
 			explicit_expiry |= EXPIRE_UNREACH;
 		}
 		else if (!strcmp(arg, "--stale-fix"))

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

[Jeff King]

On Tue, Apr 16, 2013 at 04:22:25PM -0700, Junio C Hamano wrote:

> +static void parse_expire_value(const char *value, unsigned long *expire)
> +{
> +	if (!strcmp(value, "never") || !strcmp(value, "false"))
> +		*expire = 0;
> +	else if (!strcmp(value, "all") || !strcmp(value, "now"))
> +		/*
> +		 * We take over "now" here, which usually translates
> +		 * to the current timestamp, because the user really
> +		 * means everything she has done in the past, and by
> +		 * definition reflogs are the record of the past,
> +		 * there is nothing from the future to be kept.
> +		 */
> +		*expire = ULONG_MAX;
> +	else
> +		*expire = approxidate(value);
> +}

Do we want to use approxidate_careful here to catch other junk?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Apr 16, 2013 at 04:22:25PM -0700, Junio C Hamano wrote:
>
>> +static void parse_expire_value(const char *value, unsigned long *expire)
>> +{
>> +	if (!strcmp(value, "never") || !strcmp(value, "false"))
>> +		*expire = 0;
>> +	else if (!strcmp(value, "all") || !strcmp(value, "now"))
>> +		/*
>> +		 * We take over "now" here, which usually translates
>> +		 * to the current timestamp, because the user really
>> +		 * means everything she has done in the past, and by
>> +		 * definition reflogs are the record of the past,
>> +		 * there is nothing from the future to be kept.
>> +		 */
>> +		*expire = ULONG_MAX;
>> +	else
>> +		*expire = approxidate(value);
>> +}
>
> Do we want to use approxidate_careful here to catch other junk?

We can catch a misspelt argument or configuration value that way.
That would be a good idea.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Do we want to use approxidate_careful here to catch other junk?
>
> We can catch a misspelt argument or configuration value that way.
> That would be a good idea.

-- >8 --
Subject: date.c: add parse_expiry_date()

"git reflog --expire=all" tries to expire reflog entries up to the
current second, because the approxidate() parser gives the current
timestamp for anything it does not understand (and it does not know
what time "all" means).  When the user tells us to expire "all" (or
set the expiration time to "now"), the user wants to remove all the
reflog entries (no reflog entry should record future time).

Just set it to ULONG_MAX and to let everything that is older that
timestamp expire.

While at it, allow "now" to be treated the same way for callers that
parse expiry date timestamp with this function.  Also use an error
reporting version of approxidate() to report misspelled date.  When
the user says e.g. "--expire=mnoday" to delete entries two days or
older on Wednesday, we wouldn't want the "unknown, default to now"
logic to kick in.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * adoption to more users and tests are left as an exercise to the
   reader.

 builtin/reflog.c | 14 +++++++-------
 cache.h          |  1 +
 date.c           | 22 ++++++++++++++++++++++
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index b3c9e27..44700f9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -496,11 +496,9 @@ static int parse_expire_cfg_value(const char *var, const char *value, unsigned l
 {
 	if (!value)
 		return config_error_nonbool(var);
-	if (!strcmp(value, "never") || !strcmp(value, "false")) {
-		*expire = 0;
-		return 0;
-	}
-	*expire = approxidate(value);
+	if (parse_expiry_date(value, expire))
+		return error(_("%s' for '%s' is not a valid timestamp"),
+			     value, var);
 	return 0;
 }
 
@@ -613,11 +611,13 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
 			cb.dry_run = 1;
 		else if (!prefixcmp(arg, "--expire=")) {
-			cb.expire_total = approxidate(arg + 9);
+			if (parse_expiry_date(arg + 9, &cb.expire_total))
+				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_TOTAL;
 		}
 		else if (!prefixcmp(arg, "--expire-unreachable=")) {
-			cb.expire_unreachable = approxidate(arg + 21);
+			if (parse_expiry_date(arg + 21, &cb.expire_unreachable))
+				die(_("'%s' is not a valid timestamp"), arg);
 			explicit_expiry |= EXPIRE_UNREACH;
 		}
 		else if (!strcmp(arg, "--stale-fix"))
diff --git a/cache.h b/cache.h
index 3622e18..f43f6d9 100644
--- a/cache.h
+++ b/cache.h
@@ -878,6 +878,7 @@ void show_date_relative(unsigned long time, int tz, const struct timeval *now,
 			struct strbuf *timebuf);
 int parse_date(const char *date, char *buf, int bufsize);
 int parse_date_basic(const char *date, unsigned long *timestamp, int *offset);
+int parse_expiry_date(const char *date, unsigned long *timestamp);
 void datestamp(char *buf, int bufsize);
 #define approxidate(s) approxidate_careful((s), NULL)
 unsigned long approxidate_careful(const char *, int *);
diff --git a/date.c b/date.c
index 57331ed..876d679 100644
--- a/date.c
+++ b/date.c
@@ -705,6 +705,28 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset)
 	return 0; /* success */
 }
 
+int parse_expiry_date(const char *date, unsigned long *timestamp)
+{
+	int errors = 0;
+
+	if (!strcmp(date, "never") || !strcmp(date, "false"))
+		*timestamp = 0;
+	else if (!strcmp(date, "all") || !strcmp(date, "now"))
+		/*
+		 * We take over "now" here, which usually translates
+		 * to the current timestamp.  This is because the user
+		 * really means to expire everything she has done in
+		 * the past, and by definition reflogs are the record
+		 * of the past, and there is nothing from the future
+		 * to be kept.
+		 */
+		*timestamp = ULONG_MAX;
+	else
+		*timestamp = approxidate_careful(date, &errors);
+
+	return errors;
+}
+
 int parse_date(const char *date, char *result, int maxlen)
 {
 	unsigned long timestamp;

[PATCH 0/2] Add documentation for new expiry option values

[Michael Haggerty]

The first patch changes the manpages for "git gc" and "git reflog" to
document the new expiry options made possible by Junio's "date.c: add
parse_expiry_date()".  Feel free to squash this patch onto yours.

The second patch changes the parse_options() API documentation to
mention that a "no-" prefix can also be used for non-boolean options.
It is related only by the fact that I was confused by this omission
when I was trying to understand your patch.

Michael Haggerty (2):
  git-gc.txt, git-reflog.txt: document new expiry options
  api-parse-options.txt: document "no-" for non-boolean options

 Documentation/git-gc.txt                      | 5 +++--
 Documentation/git-reflog.txt                  | 9 +++++++--
 Documentation/technical/api-parse-options.txt | 2 ++
 3 files changed, 12 insertions(+), 4 deletions(-)

-- 
1.8.2.1

[PATCH 1/2] git-gc.txt, git-reflog.txt: document new expiry options

[Michael Haggerty]

Document the new values that can be used for expiry values where their
use makes sense:

* git reflog expire --expire=[all|never]
* git reflog expire --expire-unreachable=[all|never]
* git gc --prune=all

Other combinations aren't useful and therefore no documentation is
added (even though they are allowed):

* git gc --prune=never

  is redundant with "git gc --no-prune"

* git prune --expire=all

  is equivalent to providing no --expire option

* git prune --expire=never

  is a NOP

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/git-gc.txt     | 5 +++--
 Documentation/git-reflog.txt | 9 +++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index b370b02..2402ed6 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -62,8 +62,9 @@ automatic consolidation of packs.
 
 --prune=<date>::
 	Prune loose objects older than date (default is 2 weeks ago,
-	overridable by the config variable `gc.pruneExpire`).  This
-	option is on by default.
+	overridable by the config variable `gc.pruneExpire`).
+	--prune=all prunes loose objects regardless of their age.
+	--prune is on by default.
 
 --no-prune::
 	Do not prune any loose objects.
diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index fb8697e..70791b9 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -67,14 +67,19 @@ them.
 --expire=<time>::
 	Entries older than this time are pruned.  Without the
 	option it is taken from configuration `gc.reflogExpire`,
-	which in turn defaults to 90 days.
+	which in turn defaults to 90 days.  --expire=all prunes
+	entries regardless of their age; --expire=never turns off
+	pruning of reachable entries (but see --expire-unreachable).
 
 --expire-unreachable=<time>::
 	Entries older than this time and not reachable from
 	the current tip of the branch are pruned.  Without the
 	option it is taken from configuration
 	`gc.reflogExpireUnreachable`, which in turn defaults to
-	30 days.
+	30 days.  --expire-unreachable=all prunes unreachable
+	entries regardless of their age; --expire-unreachable=never
+	turns off early pruning of unreachable entries (but see
+	--expire).
 
 --all::
 	Instead of listing <refs> explicitly, prune all refs.
-- 
1.8.2.1

[PATCH 2/2] api-parse-options.txt: document "no-" for non-boolean options

[Michael Haggerty]

Document that the "no-" prefix can also be used for non-boolean
options.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-parse-options.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 32ddc1c..2ff368e 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -41,6 +41,8 @@ The parse-options API allows:
 * Boolean long options can be 'negated' (or 'unset') by prepending
   `no-`, e.g. `--no-abbrev` instead of `--abbrev`. Conversely,
   options that begin with `no-` can be 'negated' by removing it.
+  Other long options can be unset (e.g., set string to NULL, set
+  integer to 0) by prepending `no-`.
 
 * Options and non-option arguments can clearly be separated using the `--`
   option, e.g. `-a -b --option -- --this-is-a-file` indicates that
-- 
1.8.2.1

[PATCH] prune: introduce OPT_EXPIRY_DATE() and use it

[Junio C Hamano]

Earlier we added support for --expire=all (or --expire=now) that
considers all crufts, regardless of their age, as eligible for
garbage collection by turning command argument parsers that use
approxidate() to use parse_expiry_date(), but "git prune" used a
built-in parse-options facility OPT_DATE() and did not benefit from
the new function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/technical/api-parse-options.txt | 4 ++++
 builtin/prune.c                               | 4 ++--
 parse-options-cb.c                            | 6 ++++++
 parse-options.h                               | 4 ++++
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index facc8c8..a8bae69 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -176,6 +176,10 @@ There are some macros to easily define options:
 	Introduce an option with date argument, see `approxidate()`.
 	The timestamp is put into `int_var`.
 
+`OPT_EXPIRY_DATE(short, long, &int_var, description)`::
+	Introduce an option with expiry date argument, see `parse_expiry_date()`.
+	The timestamp is put into `int_var`.
+
 `OPT_CALLBACK(short, long, &var, arg_str, description, func_ptr)`::
 	Introduce an option with argument.
 	The argument will be fed into the function given by `func_ptr`
diff --git a/builtin/prune.c b/builtin/prune.c
index 85843d4..b90e5cc 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -132,8 +132,8 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 		OPT__DRY_RUN(&show_only, N_("do not remove, show only")),
 		OPT__VERBOSE(&verbose, N_("report pruned objects")),
 		OPT_BOOL(0, "progress", &show_progress, N_("show progress")),
-		OPT_DATE(0, "expire", &expire,
-			 N_("expire objects older than <time>")),
+		OPT_EXPIRY_DATE(0, "expire", &expire,
+				N_("expire objects older than <time>")),
 		OPT_END()
 	};
 	char *s;
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 0de5fb1..be8c413 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -33,6 +33,12 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
 	return 0;
 }
 
+int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,
+			     int unset)
+{
+	return parse_expiry_date(arg, (unsigned long *)opt->value);
+}
+
 int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
 			    int unset)
 {
diff --git a/parse-options.h b/parse-options.h
index 71a39c6..8541811 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -140,6 +140,9 @@ struct option {
 #define OPT_DATE(s, l, v, h) \
 	{ OPTION_CALLBACK, (s), (l), (v), N_("time"),(h), 0,	\
 	  parse_opt_approxidate_cb }
+#define OPT_EXPIRY_DATE(s, l, v, h) \
+	{ OPTION_CALLBACK, (s), (l), (v), N_("expiry date"),(h), 0,	\
+	  parse_opt_expiry_date_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) \
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }
 #define OPT_NUMBER_CALLBACK(v, h, f) \
@@ -215,6 +218,7 @@ extern int parse_options_concat(struct option *dst, size_t, struct option *src);
 /*----- some often used options -----*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
 extern int parse_opt_approxidate_cb(const struct option *, const char *, int);
+extern int parse_opt_expiry_date_cb(const struct option *, const char *, int);
 extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
 extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
 extern int parse_opt_with_commit(const struct option *, const char *, int);

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

[Michael Haggerty]

On 04/17/2013 01:22 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> would be that it expires *everything*.  But in fact it seems to only
>> expire things that are at least one second old, which doesn't seem at
>> all useful in the real world.  "--expire=all" is accepted without
>> complaint but doesn't do what one would hope.
> 
> Perhaps that is worth fixing, independent of this topic.
> 
> Approxidate gives the current time for anything it does not
> understand, and that is how --expire=all is interpreted as "anything
> older than now".  For that matter, even a string "now" has long been
> interpreted as the current time with the same "I do not understand
> it, so I'll give you the current timestamp" logic, until we added an
> official support for "now" at 93cfa7c7a85e (approxidate_careful()
> reports errorneous date string, 2010-01-26) for entirely different
> reasons.
> 
> A completely untested patch for your enjoyment...

I enjoy it :-)  But it would be better to put the the function in the
date module ("approxidate_expiry_careful()"?) and also use it from other
places where an expiry date is being parsed, like

    prune --expire=<date>
    reflog expire --expire=<date>/--expire-unreachable=<date>
    gc --prune=<date>

(maybe there are others).  And the special values "all" etc. would need
to be documented.

Michael

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

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

[Michael Haggerty]

Stop emitting an error message for dangling packed references found
when deleting another packed reference.  See the previous commit for a
longer explanation of the issue.

Change repack_without_ref_fn() to silently ignore dangling packed
references.

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

diff --git a/refs.c b/refs.c
index 51f68d3..eadbc2a 100644
--- a/refs.c
+++ b/refs.c
@@ -531,15 +531,17 @@ static void sort_ref_dir(struct ref_dir *dir)
 
 /*
  * 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.
+ * an object in the database.  If report_errors is true, emit a
+ * warning if the referred-to object does not exist.
  */
-static int ref_resolves_to_object(struct ref_entry *entry)
+static int ref_resolves_to_object(struct ref_entry *entry, int report_errors)
 {
 	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);
+		if (report_errors)
+			error("%s does not point to a valid object!",
+			      entry->name);
 		return 0;
 	}
 	return 1;
@@ -578,7 +580,7 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)
 		return 0;
 
 	if (!((data->flags & DO_FOR_EACH_INCLUDE_BROKEN) ||
-	      ref_resolves_to_object(entry)))
+	      ref_resolves_to_object(entry, 1)))
 		return 0;
 
 	current_ref = entry;
@@ -1897,8 +1899,9 @@ 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 */
+	/* Silently skip broken refs: */
+	if (!ref_resolves_to_object(entry, 0))
+		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

Re: [PATCH 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 for dangling packed references found
> when deleting another packed reference.  See the previous commit for a
> longer explanation of the issue.
>
> Change repack_without_ref_fn() to silently ignore dangling packed
> references.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Somehow this feels as if it is sweeping the problem under the rug.

If you ignore a ref for which a loose ref exists when you update a
packed refs file, whether the stale "packed" one points at an object
that is still there or an object that has been garbage collected,
you would not even have to check if the "ref" resolves to object or
anything like that, no?

Am I missing something?

This one feels iffy in the otherwise pleasant-to-read series.

> ---
>  refs.c               | 17 ++++++++++-------
>  t/t3210-pack-refs.sh |  2 +-
>  2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 51f68d3..eadbc2a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -531,15 +531,17 @@ static void sort_ref_dir(struct ref_dir *dir)
>  
>  /*
>   * 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.
> + * an object in the database.  If report_errors is true, emit a
> + * warning if the referred-to object does not exist.
>   */
> -static int ref_resolves_to_object(struct ref_entry *entry)
> +static int ref_resolves_to_object(struct ref_entry *entry, int report_errors)
>  {
>  	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);
> +		if (report_errors)
> +			error("%s does not point to a valid object!",
> +			      entry->name);
>  		return 0;
>  	}
>  	return 1;
> @@ -578,7 +580,7 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)
>  		return 0;
>  
>  	if (!((data->flags & DO_FOR_EACH_INCLUDE_BROKEN) ||
> -	      ref_resolves_to_object(entry)))
> +	      ref_resolves_to_object(entry, 1)))
>  		return 0;
>  
>  	current_ref = entry;
> @@ -1897,8 +1899,9 @@ 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 */
> +	/* Silently skip broken refs: */
> +	if (!ref_resolves_to_object(entry, 0))
> +		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 17/33] repack_without_ref(): silence errors for dangling packed refs

[Michael Haggerty]

On 04/15/2013 07:39 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Stop emitting an error message for dangling packed references found
>> when deleting another packed reference.  See the previous commit for a
>> longer explanation of the issue.
>>
>> Change repack_without_ref_fn() to silently ignore dangling packed
>> references.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> 
> Somehow this feels as if it is sweeping the problem under the rug.
> 
> If you ignore a ref for which a loose ref exists when you update a
> packed refs file, whether the stale "packed" one points at an object
> that is still there or an object that has been garbage collected,
> you would not even have to check if the "ref" resolves to object or
> anything like that, no?
> 
> Am I missing something?
> 
> This one feels iffy in the otherwise pleasant-to-read series.

The usual situation when this code would be triggered would be that the
packed reference is overridden by a loose ref and points at an object
that has been garbage collected.  In that case it is definitely
incorrect to emit an error message.

But the fact that we don't explicitly verify that there is an overriding
loose reference means that it is possible that the failure to resolve
the packed ref comes from some kind of repository corruption, and you
are correct that such a problem would be swept under the rug by my change.

I've been trying to minimize the extra work that repack_without_ref()
needs to do to write peeled references, to avoid stretching out the
delay that can now occur when deleting a reference.  Thus I was trying
to save a check of loose references during this operation.  But I guess
I agree that a little bit more caution would be prudent.

I can think of a few ways to avoid sweeping possible indications of repo
corruption under the rug, in order of increasing run-time:

1. If a packed ref's SHA-1 cannot be resolved, write the packed ref to
the new packed-refs file anyway with SHA-1 but without a peeled value.
This would avoid having to check the loose references and avoid erasing
possible evidence of corruption, but would delay an actual check for
corruption until a later time.  It would be a quick fix, effectively
kicking the can down the road instead of sweeping it under the rug.

Minor pitfall: a reference that is listed without a peeled value in a
fully-peeled pack-refs file tells future readers that the corresponding
SHA-1 *cannot* be peeled.  IF the named object would somehow reappear in
the repository (e.g., via a fetch) and IF the object is peelable and IF
there is in fact no loose ref overriding the packed ref, then the final
result would be that one form of corruption (reference points to
non-existent object) would be converted to another form (reference
falsely believed to be non-peelable).  I think this is an acceptable
risk because (a) it would only happen in an unlikely series of events in
a repo that was already corrupt, and (b) because falsely believing a
reference to be non-peelable wouldn't have terrible consequences.

2. Whenever a packed reference cannot be resolved to an object, verify
that there is indeed a loose reference overriding it; if not, emit an
error and in either case omit the packed ref from the output.

3. Check for an overriding loose reference *before* trying to peel a
packed reference, and omit any overridden loose references from the
output packed-refs file.  This would be close to running "pack-refs
--no-prune" without the "is_tag_ref" test and with reuse of available
peeled values.  This approach would tidy up the packed-refs file a bit
more than (2) because it would cause the deletion of more overridden
packed refs, but only as part of first peeling them, which should only
happen once in a repo, and only if the first peeling occurs within
repack_without_ref() as opposed to an explicit pack_refs().  So it's a
negligible improvement over (2).

4. Further along the "correctness" spectrum, one could check for
overriding loose references *every* time the packed-refs file is
rewritten by repack_without_ref(), even for references whose peeled
values are already known.  But this would add overhead to every deletion
of a packed reference, which is probably not justified.

I'm worried that implementing 2-4 would introduce new race conditions of
the type that Peff discovered recently, unless we fix the locking policy
first (which is also on my TODO list).  So my suggestion is to implement
1 now and implement 2 sometime in the future.

Opinions?

Michael

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

[PATCH 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 eadbc2a..0c0668b 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 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 0c0668b..3c20853 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.
@@ -1892,19 +1943,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;
 	/* Silently skip broken refs: */
 	if (!ref_resolves_to_object(entry, 0))
 		return 0;
@@ -1913,7 +1957,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;
 }
 
@@ -1921,22 +1965,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);
 }
 
@@ -1965,7 +2017,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 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 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>
---

This change could cause a noticeable delay on deleting a reference if
it causes a non-fully-peeled packed-refs file to be fully peeled.  But
after the file has been fully peeled once, deleting a reference should
be as fast as before.  In exchange, reference advertising should be
faster after this change because peeled values will not be lost
whenever a packed reference is deleted.

 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 3c20853..140360f 100644
--- a/refs.c
+++ b/refs.c
@@ -878,6 +878,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.
@@ -1392,6 +1399,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)
 {
@@ -1958,6 +1971,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;
 }
 
@@ -1988,6 +2010,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

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

[Junio C Hamano]

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

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

Good.

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

[PATCH 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 | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/refs.c b/refs.c
index 140360f..9e3eff2 100644
--- a/refs.c
+++ b/refs.c
@@ -1956,29 +1956,43 @@ 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;
 
-	/* Silently skip broken refs: */
-	if (!ref_resolves_to_object(entry, 0))
-		return 0;
 	len = snprintf(line, sizeof(line), "%s %s\n",
-		       sha1_to_hex(entry->u.value.sha1), entry->name);
+		       sha1_to_hex(sha1), refname);
 	/* 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: */
+		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(entry->u.value.peeled)) !=
-		    PEELED_LINE_LENGTH)
+			     sha1_to_hex(peeled)) != PEELED_LINE_LENGTH)
 			die("internal error");
-		write_or_die(*fd, line, PEELED_LINE_LENGTH);
+		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;
+
+	/* Silently skip broken refs: */
+	if (!ref_resolves_to_object(entry, 0))
+		return 0;
+	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 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 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 9e3eff2..6c7f92c 100644
--- a/refs.c
+++ b/refs.c
@@ -1981,6 +1981,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 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 6c7f92c..16c237c 100644
--- a/refs.c
+++ b/refs.c
@@ -2001,7 +2001,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;
@@ -2011,27 +2011,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 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 16c237c..e0411c5 100644
--- a/refs.c
+++ b/refs.c
@@ -2089,7 +2089,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)
 {
@@ -2099,7 +2099,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)
@@ -2118,8 +2118,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;
@@ -2141,8 +2141,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 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 e0411c5..471d71d 100644
--- a/refs.c
+++ b/refs.c
@@ -2001,37 +2001,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, 0))
 		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;
 	}
@@ -2108,7 +2109,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 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 471d71d..e1b8a56 100644
--- a/refs.c
+++ b/refs.c
@@ -1993,14 +1993,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;
@@ -2028,7 +2020,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 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>
---

I am not so familiar with the object DB API so it would be good for
somebody to verify that the slightly different method that pack-refs
now uses to peel references is equivalent to the old method.

Please note that this version of the patch series (in contrast to one
I shared before via github) re-peels all references when pack-refs is
run rather than reusing any available peeled values.

 refs.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index e1b8a56..6397e29 100644
--- a/refs.c
+++ b/refs.c
@@ -1398,7 +1398,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
@@ -1406,13 +1408,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)
@@ -1431,7 +1439,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;
@@ -1451,7 +1459,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;
@@ -1996,7 +2004,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: */
@@ -2012,13 +2020,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)) {
@@ -2127,7 +2134,7 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)
 	/* Silently skip broken refs: */
 	if (!ref_resolves_to_object(entry, 0))
 		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 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>
---

Please checked that I am using the file-locking API correctly; I'm not
that familiar with it.

 refs.c | 33 ++++++++-------------------------
 1 file changed, 8 insertions(+), 25 deletions(-)

diff --git a/refs.c b/refs.c
index 6397e29..923d34f 100644
--- a/refs.c
+++ b/refs.c
@@ -1998,7 +1998,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)
@@ -2017,15 +2017,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)) {
@@ -2094,32 +2092,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 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 | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 923d34f..361a28f 100644
--- a/refs.c
+++ b/refs.c
@@ -2005,16 +2005,15 @@ 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;
+	int is_tag_ref = !prefixcmp(entry->name, "refs/tags/");
 
-	/* Do not pack symbolic or broken refs: */
-	if ((entry->flag & REF_ISSYMREF) || !ref_resolves_to_object(entry, 0))
+	/* 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;
-	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))
+	/* Do not pack symbolic or broken refs: */
+	if ((entry->flag & REF_ISSYMREF) || !ref_resolves_to_object(entry, 0))
 		return 0;
 
 	peel_status = peel_entry(entry, 1);
-- 
1.8.2.1

[PATCH 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 361a28f..cfcb42f 100644
--- a/refs.c
+++ b/refs.c
@@ -1505,14 +1505,13 @@ 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.
  */
-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,14 +1540,14 @@ 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
  * broken references in the iteration.
  */
-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;
@@ -1557,7 +1556,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)
@@ -1590,23 +1589,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)
@@ -1641,7 +1640,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)
@@ -1664,7 +1663,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;
 }
@@ -1706,7 +1705,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);
 }
 
@@ -2101,7 +2100,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

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

[Junio C Hamano]

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

> Change the callers convert submodule names into ref_cache pointers.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

A nice cleanup.

[PATCH 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 cfcb42f..13e5bc8 100644
--- a/refs.c
+++ b/refs.c
@@ -812,9 +812,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)
 {
@@ -852,18 +856,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;
 }
 
@@ -1012,8 +1016,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));
 }
 
 /*
@@ -1188,7 +1192,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)
@@ -1589,7 +1593,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)
@@ -1599,7 +1603,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,
@@ -1640,7 +1644,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)
@@ -1663,7 +1667,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;
 }
@@ -1705,7 +1709,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);
 }
 
@@ -1911,7 +1915,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;
 	}
@@ -2100,7 +2104,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);
@@ -2126,7 +2130,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))
@@ -2137,8 +2140,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) {
 		/*
@@ -2178,7 +2181,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;
 }
@@ -2200,7 +2203,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);
@@ -2212,10 +2214,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)))
@@ -2471,7 +2473,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