Re: [PATCH 1/2] pack-refs: write peeled entry for non-tags
On Sat, Mar 16, 2013 at 02:50:56PM +0100, Michael Haggerty wrote:
@@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const
unsigned char *sha1,
return 0;
fprintf(cb-refs_file, %s %s\n, sha1_to_hex(sha1), path);
- if (is_tag_ref) {
- struct object *o = parse_object(sha1);
- if (o-type == OBJ_TAG) {
- o = deref_tag(o, path, 0);
- if (o)
- fprintf(cb-refs_file, ^%s\n,
- sha1_to_hex(o-sha1));
- }
+
+ o = parse_object(sha1);
+ if (o-type == OBJ_TAG) {
You suggested that I add a test (o != NULL) at the equivalent place in
my code (which was derived from this code). Granted, my code was
explicitly intending to pass invalid SHA1 values to parse_object(). But
wouldn't it be a good defensive step to add the same check here?
Hmm, yeah. That is not new code, but rather just reindented from above
(diff -w makes it much more obvious what is going on).
It is probably worth dying rather than segfaulting, though it should be
a separate patch (and I do not think it is sane to do anything except
die here). I almost wonder if parse_object should die by default on
bogus or missing objects, and the few callers who really want to handle
the error can call parse_object_gently. I do not relish analyzing each
caller, though. It would be simpler to add parse_object_or_die.
+# This matches show-ref's output
+print_ref() {
+ echo `git rev-parse $1` $1
+}
+
CodingGuidelines prefers $() over ``.
Old habits die hard. :)
I'll re-roll with your suggestions in a moment.
-Peff
--
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 1/2] pack-refs: write peeled entry for non-tags
When we pack an annotated tag ref, we write not only the
sha1 of the tag object along with the ref, but also the sha1
obtained by peeling the tag. This lets readers of the
pack-refs file know the peeled value without having to
actually load the object, speeding up upload-pack's ref
advertisement.
The writer marks a packed-refs file with peeled refs using
the peeled trait at the top of the file. When the reader
sees this trait, it knows that each ref is either followed
by its peeled value, or it is not an annotated tag.
However, there is a mismatch between the assumptions of the
reader and writer. The writer will only peel refs under
refs/tags, but the reader does not know this; it will assume
a ref without a peeled value must not be a tag object. Thus
an annotated tag object placed outside of the refs/tags
hierarchy will not have its peeled value printed by
upload-pack.
The simplest way to fix this is to start writing peel values
for all refs. This matches what the reader expects for both
new and old versions of git.
Signed-off-by: Jeff King p...@peff.net
---
pack-refs.c | 16
t/t3211-peel-ref.sh | 42 ++
2 files changed, 50 insertions(+), 8 deletions(-)
create mode 100755 t/t3211-peel-ref.sh
diff --git a/pack-refs.c b/pack-refs.c
index f09a054..10832d7 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -27,6 +27,7 @@ static int handle_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 */
@@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned
char *sha1,
return 0;
fprintf(cb-refs_file, %s %s\n, sha1_to_hex(sha1), path);
- if (is_tag_ref) {
- struct object *o = parse_object(sha1);
- if (o-type == OBJ_TAG) {
- o = deref_tag(o, path, 0);
- if (o)
- fprintf(cb-refs_file, ^%s\n,
- sha1_to_hex(o-sha1));
- }
+
+ o = parse_object(sha1);
+ 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)) {
diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
new file mode 100755
index 000..dd5b48b
--- /dev/null
+++ b/t/t3211-peel-ref.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='tests for the peel_ref optimization of packed-refs'
+. ./test-lib.sh
+
+test_expect_success 'create annotated tag in refs/tags' '
+ test_commit base
+ git tag -m annotated foo
+'
+
+test_expect_success 'create annotated tag outside of refs/tags' '
+ git update-ref refs/outside/foo refs/tags/foo
+'
+
+# This matches show-ref's output
+print_ref() {
+ echo `git rev-parse $1` $1
+}
+
+test_expect_success 'set up expected show-ref output' '
+ {
+ print_ref refs/heads/master
+ print_ref refs/outside/foo
+ print_ref refs/outside/foo^{}
+ print_ref refs/tags/base
+ print_ref refs/tags/foo
+ print_ref refs/tags/foo^{}
+ } expect
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (loose)' '
+ git show-ref -d actual
+ test_cmp expect actual
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (packed)' '
+ git pack-refs --all
+ git show-ref -d actual
+ test_cmp expect actual
+'
+
+test_done
--
1.8.2.rc2.7.gef06216
--
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
Re: [PATCH 1/2] pack-refs: write peeled entry for non-tags
Looks good aside from a couple of minor points mentioned below.
On 03/16/2013 10:01 AM, Jeff King wrote:
When we pack an annotated tag ref, we write not only the
sha1 of the tag object along with the ref, but also the sha1
obtained by peeling the tag. This lets readers of the
pack-refs file know the peeled value without having to
actually load the object, speeding up upload-pack's ref
advertisement.
The writer marks a packed-refs file with peeled refs using
the peeled trait at the top of the file. When the reader
sees this trait, it knows that each ref is either followed
by its peeled value, or it is not an annotated tag.
However, there is a mismatch between the assumptions of the
reader and writer. The writer will only peel refs under
refs/tags, but the reader does not know this; it will assume
a ref without a peeled value must not be a tag object. Thus
an annotated tag object placed outside of the refs/tags
hierarchy will not have its peeled value printed by
upload-pack.
The simplest way to fix this is to start writing peel values
for all refs. This matches what the reader expects for both
new and old versions of git.
Signed-off-by: Jeff King p...@peff.net
I like your explanation of the problem.
---
pack-refs.c | 16
t/t3211-peel-ref.sh | 42 ++
2 files changed, 50 insertions(+), 8 deletions(-)
create mode 100755 t/t3211-peel-ref.sh
diff --git a/pack-refs.c b/pack-refs.c
index f09a054..10832d7 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -27,6 +27,7 @@ static int handle_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 */
@@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const
unsigned char *sha1,
return 0;
fprintf(cb-refs_file, %s %s\n, sha1_to_hex(sha1), path);
- if (is_tag_ref) {
- struct object *o = parse_object(sha1);
- if (o-type == OBJ_TAG) {
- o = deref_tag(o, path, 0);
- if (o)
- fprintf(cb-refs_file, ^%s\n,
- sha1_to_hex(o-sha1));
- }
+
+ o = parse_object(sha1);
+ if (o-type == OBJ_TAG) {
You suggested that I add a test (o != NULL) at the equivalent place in
my code (which was derived from this code). Granted, my code was
explicitly intending to pass invalid SHA1 values to parse_object(). But
wouldn't it be a good defensive step to add the same check here?
+ 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)) {
diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
new file mode 100755
index 000..dd5b48b
--- /dev/null
+++ b/t/t3211-peel-ref.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='tests for the peel_ref optimization of packed-refs'
+. ./test-lib.sh
+
+test_expect_success 'create annotated tag in refs/tags' '
+ test_commit base
+ git tag -m annotated foo
+'
+
+test_expect_success 'create annotated tag outside of refs/tags' '
+ git update-ref refs/outside/foo refs/tags/foo
+'
+
+# This matches show-ref's output
+print_ref() {
+ echo `git rev-parse $1` $1
+}
+
CodingGuidelines prefers $() over ``.
+test_expect_success 'set up expected show-ref output' '
+ {
+ print_ref refs/heads/master
+ print_ref refs/outside/foo
+ print_ref refs/outside/foo^{}
+ print_ref refs/tags/base
+ print_ref refs/tags/foo
+ print_ref refs/tags/foo^{}
+ } expect
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (loose)' '
+ git show-ref -d actual
+ test_cmp expect actual
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (packed)' '
+ git pack-refs --all
+ git show-ref -d actual
+ test_cmp expect actual
+'
+
+test_done
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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
