Re: [PATCH 2/2] pack-refs: add fully-peeled trait
On Sat, Mar 16, 2013 at 03:06:22PM +0100, Michael Haggerty wrote:
refname = parse_ref_line(refline, sha1);
if (refname) {
- last = create_ref_entry(refname, sha1, flag, 1);
+ /*
+* Older git did not write peel lines for anything
+* outside of refs/tags/; if the fully-peeled trait
+* is not set, we are dealing with such an older
+* git and cannot assume an omitted peel value
+* means the ref is not a tag object.
+*/
+ int this_flag = flag;
+ if (!fully_peeled prefixcmp(refname, refs/tags/))
+ this_flag = ~REF_KNOWS_PEELED;
+
+ last = create_ref_entry(refname, sha1, this_flag, 1);
add_ref(dir, last);
continue;
}
I have to admit that I am partial to my variant of this code [1] because
the logic makes it clearer when the affirmative decision can be made to
set the REF_KNOWS_PEELED flag. But this version also looks correct to
me and equivalent (aside from the idea that a few lines later if a
peeled value is found then the REF_KNOWS_PEELED bit could also be set).
Yeah, I think they are equivalent, but I agree yours is a little more
readable. I'll switch it in my re-roll, and I will go ahead and set the
REF_KNOWS_PEELED bit when we see a peel line. That code should not be
triggered in general, but it is the sane thing for the reader to do, so
it makes the code more obvious and readable.
-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
Re: [PATCH 2/2] pack-refs: add fully-peeled trait
ACK, with one ignorable comment.
Michael
On 03/16/2013 10:01 AM, Jeff King wrote:
Older versions of pack-refs did not write peel lines for
refs outside of refs/tags. This meant that on reading the
pack-refs file, we might set the REF_KNOWS_PEELED flag for
such a ref, even though we do not know anything about its
peeled value.
The previous commit updated the writer to always peel, no
matter what the ref is. That means that packed-refs files
written by newer versions of git are fine to be read by both
old and new versions of git. However, we still have the
problem of reading packed-refs files written by older
versions of git, or by other implementations which have not
yet learned the same trick.
The simplest fix would be to always unset the
REF_KNOWS_PEELED flag for refs outside of refs/tags that do
not have a peel line (if it has a peel line, we know it is
valid, but we cannot assume a missing peel line means
anything). But that loses an important optimization, as
upload-pack should not need to load the object pointed to by
refs/heads/foo to determine that it is not a tag.
Instead, we add a fully-peeled trait to the packed-refs
file. If it is set, we know that we can trust a missing peel
line to mean that a ref cannot be peeled. Otherwise, we fall
back to assuming nothing.
Another nice, clear explanation of the issue.
Signed-off-by: Jeff King p...@peff.net
---
pack-refs.c | 2 +-
refs.c | 16 +++-
t/t3211-peel-ref.sh | 22 ++
3 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/pack-refs.c b/pack-refs.c
index 10832d7..87ca04d 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -128,7 +128,7 @@ int pack_refs(unsigned int flags)
die_errno(unable to create ref-pack file structure);
/* perhaps other traits later as well */
- fprintf(cbdata.refs_file, # pack-refs with: peeled \n);
+ fprintf(cbdata.refs_file, # pack-refs with: peeled fully-peeled \n);
for_each_ref(handle_one_ref, cbdata);
if (ferror(cbdata.refs_file))
diff --git a/refs.c b/refs.c
index 175b9fc..6a38c41 100644
--- a/refs.c
+++ b/refs.c
@@ -808,6 +808,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
struct ref_entry *last = NULL;
char refline[PATH_MAX];
int flag = REF_ISPACKED;
+ int fully_peeled = 0;
while (fgets(refline, sizeof(refline), f)) {
unsigned char sha1[20];
@@ -818,13 +819,26 @@ static void read_packed_refs(FILE *f, struct ref_dir
*dir)
const char *traits = refline + sizeof(header) - 1;
if (strstr(traits, peeled ))
flag |= REF_KNOWS_PEELED;
+ if (strstr(traits, fully-peeled ))
+ fully_peeled = 1;
/* perhaps other traits later as well */
continue;
}
refname = parse_ref_line(refline, sha1);
if (refname) {
- last = create_ref_entry(refname, sha1, flag, 1);
+ /*
+ * Older git did not write peel lines for anything
+ * outside of refs/tags/; if the fully-peeled trait
+ * is not set, we are dealing with such an older
+ * git and cannot assume an omitted peel value
+ * means the ref is not a tag object.
+ */
+ int this_flag = flag;
+ if (!fully_peeled prefixcmp(refname, refs/tags/))
+ this_flag = ~REF_KNOWS_PEELED;
+
+ last = create_ref_entry(refname, sha1, this_flag, 1);
add_ref(dir, last);
continue;
}
I have to admit that I am partial to my variant of this code [1] because
the logic makes it clearer when the affirmative decision can be made to
set the REF_KNOWS_PEELED flag. But this version also looks correct to
me and equivalent (aside from the idea that a few lines later if a
peeled value is found then the REF_KNOWS_PEELED bit could also be set).
diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
index dd5b48b..a8eb1aa 100755
--- a/t/t3211-peel-ref.sh
+++ b/t/t3211-peel-ref.sh
@@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags
(packed)' '
test_cmp expect actual
'
+test_expect_success 'create old-style pack-refs without fully-peeled' '
+ # Git no longer writes without fully-peeled, so we just write our own
+ # from scratch; we could also munge the existing file to remove the
+ # fully-peeled bits, but that seems even more prone to failure,
+ # especially if the format ever changes again. At least this way we
+ # know we are emulating exactly what an older git would have written.
+ {
+
Re: [PATCH 2/2] pack-refs: add fully-peeled trait
Jeff King p...@peff.net writes:
The simplest fix would be to always unset the
REF_KNOWS_PEELED flag for refs outside of refs/tags that do
not have a peel line (if it has a peel line, we know it is
valid, but we cannot assume a missing peel line means
anything). But that loses an important optimization, as
upload-pack should not need to load the object pointed to by
refs/heads/foo to determine that it is not a tag.
I think the patch makes sense and in line with what we already
discussed in the thread.
I however wonder if the above implies it may make sense to add this
on top? Perhaps it is not worth it, because it makes a difference
only to a repository with annotated tags outside refs/tags hierarchy
and still has the packed-refs file that was created with an older
version of Git, so we can just tell repack with new Git to users
with such a repository.
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 7f84efd..afc4dde 100644
--- a/refs.c
+++ b/refs.c
@@ -847,8 +847,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
refline[0] == '^'
strlen(refline) == 42
refline[41] == '\n'
- !get_sha1_hex(refline + 1, sha1))
+ !get_sha1_hex(refline + 1, sha1)) {
hashcpy(last-u.value.peeled, sha1);
+ last-flag |= REF_KNOWS_PEELED;
+ }
}
}
--
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 2/2] pack-refs: add fully-peeled trait
On Sat, Mar 16, 2013 at 10:50:17PM -0700, Junio C Hamano wrote:
I however wonder if the above implies it may make sense to add this
on top? Perhaps it is not worth it, because it makes a difference
only to a repository with annotated tags outside refs/tags hierarchy
and still has the packed-refs file that was created with an older
version of Git, so we can just tell repack with new Git to users
with such a repository.
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 7f84efd..afc4dde 100644
--- a/refs.c
+++ b/refs.c
@@ -847,8 +847,10 @@ static void read_packed_refs(FILE *f, struct ref_dir
*dir)
refline[0] == '^'
strlen(refline) == 42
refline[41] == '\n'
- !get_sha1_hex(refline + 1, sha1))
+ !get_sha1_hex(refline + 1, sha1)) {
hashcpy(last-u.value.peeled, sha1);
+ last-flag |= REF_KNOWS_PEELED;
+ }
}
}
Almost. The older version of Git would not have written those peel lines
in the first place. So yes, if we saw such a file, we could assume the
peel lines are valid. But nobody has ever generated that (with the
except of git between my two patches).
I do think it may be worth doing, though, just because it makes the
handling of the flag more obvious; somebody reading it later would
wonder hey, shouldn't we be setting REF_KNOWS_PEELED here?, and it is
simple and harmless to just do it, rather than confusing a later reader.
I'll re-roll in a second to incorporate the comments from Michael.
-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
