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

2013-04-15 Thread Junio C Hamano
Michael Haggerty mhag...@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).

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-04-14 Thread 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 mhag...@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