Re: [PATCH 2/2] pack-refs: add fully-peeled trait

Jeff King [off-list ref] 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;
+		}
 	}
 }
 

On Sat, Mar 16, 2013 at 10:50:17PM -0700, Junio C Hamano wrote:

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