{"thread":{"id":"33196","subject":"[PATCH 0/2] peel-ref optimization fixes","startedAt":"2013-03-16T09:00:19Z","lastAt":"2013-03-18T16:26:48Z","messageCount":19,"participants":["Jeff King","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"211443","messageId":"20130316090018.GA26708@sigill.intra.peff.net","threadId":"33196","inReplyTo":null,"subject":"[PATCH 0/2] peel-ref optimization fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-16T09:00:19Z","receivedAt":"2013-03-16T09:00:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These patches fix the issue with peel-ref noticed recently by Michael\n(namely that we fail to correctly peel packed refs outside of\nrefs/tags).  The problem has been there since we added peeling support\nto pack-refs, but traditionally \"show-ref -d\" was the only caller that\nactually triggered the issue. Between that and the fact that most people\nonly put annotated tags into refs/tags, nobody really noticed.\n\nSince my 435c833 (upload-pack: use peel_ref for ref advertisements,\n2012-10-04), which is in v1.8.1, upload-pack can trigger the problem,\ntoo, but I haven't actually heard of any reports in the wild.\n\nI split it into two patches; the first one is the minimal fix that makes\ngit work properly going forward. The second one helps git be more robust\nwhen reading packed-refs files generated by older git (or other\nimplementations). The second one depends semantically but not textually\non the first one; if you're worried about that, they can be squashed.\n\n  [1/2]: pack-refs: write peeled entry for non-tags\n  [2/2]: pack-refs: add fully-peeled trait\n\nI think Michael may be rewriting some of this code, but if we are going\nto go this direction with the fix (and I think we should), this is the\nchange that should go to maint in the meantime.\n\n-Peff\n"},{"id":"211444","messageId":"20130316090103.GA26855@sigill.intra.peff.net","threadId":"33196","inReplyTo":"20130316090018.GA26708@sigill.intra.peff.net","subject":"[PATCH 1/2] pack-refs: write peeled entry for non-tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-16T09:01:03Z","receivedAt":"2013-03-16T09:01:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we pack an annotated tag ref, we write not only the\nsha1 of the tag object along with the ref, but also the sha1\nobtained by peeling the tag. This lets readers of the\npack-refs file know the peeled value without having to\nactually load the object, speeding up upload-pack's ref\nadvertisement.\n\nThe writer marks a packed-refs file with peeled refs using\nthe \"peeled\" trait at the top of the file. When the reader\nsees this trait, it knows that each ref is either followed\nby its peeled value, or it is not an annotated tag.\n\nHowever, there is a mismatch between the assumptions of the\nreader and writer. The writer will only peel refs under\nrefs/tags, but the reader does not know this; it will assume\na ref without a peeled value must not be a tag object. Thus\nan annotated tag object placed outside of the refs/tags\nhierarchy will not have its peeled value printed by\nupload-pack.\n\nThe simplest way to fix this is to start writing peel values\nfor all refs. This matches what the reader expects for both\nnew and old versions of git.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-refs.c         | 16 ++++++++--------\n t/t3211-peel-ref.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 50 insertions(+), 8 deletions(-)\n create mode 100755 t/t3211-peel-ref.sh\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex f09a054..10832d7 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -27,6 +27,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n \t\t\t  int flags, void *cb_data)\n {\n \tstruct pack_refs_cb_data *cb = cb_data;\n+\tstruct object *o;\n \tint is_tag_ref;\n \n \t/* Do not pack the symbolic refs */\n@@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n \t\treturn 0;\n \n \tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(sha1), path);\n-\tif (is_tag_ref) {\n-\t\tstruct object *o = parse_object(sha1);\n-\t\tif (o->type == OBJ_TAG) {\n-\t\t\to = deref_tag(o, path, 0);\n-\t\t\tif (o)\n-\t\t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n-\t\t\t\t\tsha1_to_hex(o->sha1));\n-\t\t}\n+\n+\to = parse_object(sha1);\n+\tif (o->type == OBJ_TAG) {\n+\t\to = deref_tag(o, path, 0);\n+\t\tif (o)\n+\t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n+\t\t\t\tsha1_to_hex(o->sha1));\n \t}\n \n \tif ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {\ndiff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh\nnew file mode 100755\nindex 0000000..dd5b48b\n--- /dev/null\n+++ b/t/t3211-peel-ref.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='tests for the peel_ref optimization of packed-refs'\n+. ./test-lib.sh\n+\n+test_expect_success 'create annotated tag in refs/tags' '\n+\ttest_commit base &&\n+\tgit tag -m annotated foo\n+'\n+\n+test_expect_success 'create annotated tag outside of refs/tags' '\n+\tgit update-ref refs/outside/foo refs/tags/foo\n+'\n+\n+# This matches show-ref's output\n+print_ref() {\n+\techo \"`git rev-parse \"$1\"` $1\"\n+}\n+\n+test_expect_success 'set up expected show-ref output' '\n+\t{\n+\t\tprint_ref \"refs/heads/master\" &&\n+\t\tprint_ref \"refs/outside/foo\" &&\n+\t\tprint_ref \"refs/outside/foo^{}\" &&\n+\t\tprint_ref \"refs/tags/base\" &&\n+\t\tprint_ref \"refs/tags/foo\" &&\n+\t\tprint_ref \"refs/tags/foo^{}\"\n+\t} >expect\n+'\n+\n+test_expect_success 'refs are peeled outside of refs/tags (loose)' '\n+\tgit show-ref -d >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'refs are peeled outside of refs/tags (packed)' '\n+\tgit pack-refs --all &&\n+\tgit show-ref -d >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.2.rc2.7.gef06216\n"},{"id":"211445","messageId":"20130316090116.GB26855@sigill.intra.peff.net","threadId":"33196","inReplyTo":"20130316090018.GA26708@sigill.intra.peff.net","subject":"[PATCH 2/2] pack-refs: add fully-peeled trait","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-16T09:01:16Z","receivedAt":"2013-03-16T09:01:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Older versions of pack-refs did not write peel lines for\nrefs outside of refs/tags. This meant that on reading the\npack-refs file, we might set the REF_KNOWS_PEELED flag for\nsuch a ref, even though we do not know anything about its\npeeled value.\n\nThe previous commit updated the writer to always peel, no\nmatter what the ref is. That means that packed-refs files\nwritten by newer versions of git are fine to be read by both\nold and new versions of git. However, we still have the\nproblem of reading packed-refs files written by older\nversions of git, or by other implementations which have not\nyet learned the same trick.\n\nThe simplest fix would be to always unset the\nREF_KNOWS_PEELED flag for refs outside of refs/tags that do\nnot have a peel line (if it has a peel line, we know it is\nvalid, but we cannot assume a missing peel line means\nanything). But that loses an important optimization, as\nupload-pack should not need to load the object pointed to by\nrefs/heads/foo to determine that it is not a tag.\n\nInstead, we add a \"fully-peeled\" trait to the packed-refs\nfile. If it is set, we know that we can trust a missing peel\nline to mean that a ref cannot be peeled. Otherwise, we fall\nback to assuming nothing.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-refs.c         |  2 +-\n refs.c              | 16 +++++++++++++++-\n t/t3211-peel-ref.sh | 22 ++++++++++++++++++++++\n 3 files changed, 38 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex 10832d7..87ca04d 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -128,7 +128,7 @@ int pack_refs(unsigned int flags)\n \t\tdie_errno(\"unable to create ref-pack file structure\");\n \n \t/* perhaps other traits later as well */\n-\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled \\n\");\n+\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n \n \tfor_each_ref(handle_one_ref, &cbdata);\n \tif (ferror(cbdata.refs_file))\ndiff --git a/refs.c b/refs.c\nindex 175b9fc..6a38c41 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -808,6 +808,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \tstruct ref_entry *last = NULL;\n \tchar refline[PATH_MAX];\n \tint flag = REF_ISPACKED;\n+\tint fully_peeled = 0;\n \n \twhile (fgets(refline, sizeof(refline), f)) {\n \t\tunsigned char sha1[20];\n@@ -818,13 +819,26 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\t\tconst char *traits = refline + sizeof(header) - 1;\n \t\t\tif (strstr(traits, \" peeled \"))\n \t\t\t\tflag |= REF_KNOWS_PEELED;\n+\t\t\tif (strstr(traits, \" fully-peeled \"))\n+\t\t\t\tfully_peeled = 1;\n \t\t\t/* perhaps other traits later as well */\n \t\t\tcontinue;\n \t\t}\n \n \t\trefname = parse_ref_line(refline, sha1);\n \t\tif (refname) {\n-\t\t\tlast = create_ref_entry(refname, sha1, flag, 1);\n+\t\t\t/*\n+\t\t\t * Older git did not write peel lines for anything\n+\t\t\t * outside of refs/tags/; if the fully-peeled trait\n+\t\t\t * is not set, we are dealing with such an older\n+\t\t\t * git and cannot assume an omitted peel value\n+\t\t\t * means the ref is not a tag object.\n+\t\t\t */\n+\t\t\tint this_flag = flag;\n+\t\t\tif (!fully_peeled && prefixcmp(refname, \"refs/tags/\"))\n+\t\t\t\tthis_flag &= ~REF_KNOWS_PEELED;\n+\n+\t\t\tlast = create_ref_entry(refname, sha1, this_flag, 1);\n \t\t\tadd_ref(dir, last);\n \t\t\tcontinue;\n \t\t}\ndiff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh\nindex dd5b48b..a8eb1aa 100755\n--- a/t/t3211-peel-ref.sh\n+++ b/t/t3211-peel-ref.sh\n@@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags (packed)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'create old-style pack-refs without fully-peeled' '\n+\t# Git no longer writes without fully-peeled, so we just write our own\n+\t# from scratch; we could also munge the existing file to remove the\n+\t# fully-peeled bits, but that seems even more prone to failure,\n+\t# especially if the format ever changes again. At least this way we\n+\t# know we are emulating exactly what an older git would have written.\n+\t{\n+\t\techo \"# pack-refs with: peeled \" &&\n+\t\tprint_ref \"refs/heads/master\" &&\n+\t\tprint_ref \"refs/outside/foo\" &&\n+\t\tprint_ref \"refs/tags/base\" &&\n+\t\tprint_ref \"refs/tags/foo\" &&\n+\t\techo \"^$(git rev-parse \"refs/tags/foo^{}\")\"\n+\t} >tmp &&\n+\tmv tmp .git/packed-refs\n+'\n+\n+test_expect_success 'refs are peeled outside of refs/tags (old packed)' '\n+\tgit show-ref -d >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.2.rc2.7.gef06216\n"},{"id":"211459","messageId":"514478C0.6060008@alum.mit.edu","threadId":"33196","inReplyTo":"20130316090103.GA26855@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] pack-refs: write peeled entry for non-tags","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-03-16T13:50:56Z","receivedAt":"2013-03-16T13:50:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Looks good aside from a couple of minor points mentioned below.\n\nOn 03/16/2013 10:01 AM, Jeff King wrote:\n> When we pack an annotated tag ref, we write not only the\n> sha1 of the tag object along with the ref, but also the sha1\n> obtained by peeling the tag. This lets readers of the\n> pack-refs file know the peeled value without having to\n> actually load the object, speeding up upload-pack's ref\n> advertisement.\n> \n> The writer marks a packed-refs file with peeled refs using\n> the \"peeled\" trait at the top of the file. When the reader\n> sees this trait, it knows that each ref is either followed\n> by its peeled value, or it is not an annotated tag.\n> \n> However, there is a mismatch between the assumptions of the\n> reader and writer. The writer will only peel refs under\n> refs/tags, but the reader does not know this; it will assume\n> a ref without a peeled value must not be a tag object. Thus\n> an annotated tag object placed outside of the refs/tags\n> hierarchy will not have its peeled value printed by\n> upload-pack.\n> \n> The simplest way to fix this is to start writing peel values\n> for all refs. This matches what the reader expects for both\n> new and old versions of git.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n\nI like your explanation of the problem.\n\n> ---\n>  pack-refs.c         | 16 ++++++++--------\n>  t/t3211-peel-ref.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 50 insertions(+), 8 deletions(-)\n>  create mode 100755 t/t3211-peel-ref.sh\n> \n> diff --git a/pack-refs.c b/pack-refs.c\n> index f09a054..10832d7 100644\n> --- a/pack-refs.c\n> +++ b/pack-refs.c\n> @@ -27,6 +27,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n>  \t\t\t  int flags, void *cb_data)\n>  {\n>  \tstruct pack_refs_cb_data *cb = cb_data;\n> +\tstruct object *o;\n>  \tint is_tag_ref;\n>  \n>  \t/* Do not pack the symbolic refs */\n> @@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n>  \t\treturn 0;\n>  \n>  \tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(sha1), path);\n> -\tif (is_tag_ref) {\n> -\t\tstruct object *o = parse_object(sha1);\n> -\t\tif (o->type == OBJ_TAG) {\n> -\t\t\to = deref_tag(o, path, 0);\n> -\t\t\tif (o)\n> -\t\t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n> -\t\t\t\t\tsha1_to_hex(o->sha1));\n> -\t\t}\n> +\n> +\to = parse_object(sha1);\n> +\tif (o->type == OBJ_TAG) {\n\nYou suggested that I add a test (o != NULL) at the equivalent place in\nmy code (which was derived from this code).  Granted, my code was\nexplicitly intending to pass invalid SHA1 values to parse_object().  But\nwouldn't it be a good defensive step to add the same check here?\n\n> +\t\to = deref_tag(o, path, 0);\n> +\t\tif (o)\n> +\t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n> +\t\t\t\tsha1_to_hex(o->sha1));\n>  \t}\n>  \n>  \tif ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {\n> diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh\n> new file mode 100755\n> index 0000000..dd5b48b\n> --- /dev/null\n> +++ b/t/t3211-peel-ref.sh\n> @@ -0,0 +1,42 @@\n> +#!/bin/sh\n> +\n> +test_description='tests for the peel_ref optimization of packed-refs'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'create annotated tag in refs/tags' '\n> +\ttest_commit base &&\n> +\tgit tag -m annotated foo\n> +'\n> +\n> +test_expect_success 'create annotated tag outside of refs/tags' '\n> +\tgit update-ref refs/outside/foo refs/tags/foo\n> +'\n> +\n> +# This matches show-ref's output\n> +print_ref() {\n> +\techo \"`git rev-parse \"$1\"` $1\"\n> +}\n> +\n\nCodingGuidelines prefers $() over ``.\n\n> +test_expect_success 'set up expected show-ref output' '\n> +\t{\n> +\t\tprint_ref \"refs/heads/master\" &&\n> +\t\tprint_ref \"refs/outside/foo\" &&\n> +\t\tprint_ref \"refs/outside/foo^{}\" &&\n> +\t\tprint_ref \"refs/tags/base\" &&\n> +\t\tprint_ref \"refs/tags/foo\" &&\n> +\t\tprint_ref \"refs/tags/foo^{}\"\n> +\t} >expect\n> +'\n> +\n> +test_expect_success 'refs are peeled outside of refs/tags (loose)' '\n> +\tgit show-ref -d >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'refs are peeled outside of refs/tags (packed)' '\n> +\tgit pack-refs --all &&\n> +\tgit show-ref -d >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_done\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"211460","messageId":"51447C5E.3050808@alum.mit.edu","threadId":"33196","inReplyTo":"20130316090116.GB26855@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pack-refs: add fully-peeled trait","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-03-16T14:06:22Z","receivedAt":"2013-03-16T14:06:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"ACK, with one ignorable comment.\n\nMichael\n\nOn 03/16/2013 10:01 AM, Jeff King wrote:\n> Older versions of pack-refs did not write peel lines for\n> refs outside of refs/tags. This meant that on reading the\n> pack-refs file, we might set the REF_KNOWS_PEELED flag for\n> such a ref, even though we do not know anything about its\n> peeled value.\n> \n> The previous commit updated the writer to always peel, no\n> matter what the ref is. That means that packed-refs files\n> written by newer versions of git are fine to be read by both\n> old and new versions of git. However, we still have the\n> problem of reading packed-refs files written by older\n> versions of git, or by other implementations which have not\n> yet learned the same trick.\n> \n> The simplest fix would be to always unset the\n> REF_KNOWS_PEELED flag for refs outside of refs/tags that do\n> not have a peel line (if it has a peel line, we know it is\n> valid, but we cannot assume a missing peel line means\n> anything). But that loses an important optimization, as\n> upload-pack should not need to load the object pointed to by\n> refs/heads/foo to determine that it is not a tag.\n> \n> Instead, we add a \"fully-peeled\" trait to the packed-refs\n> file. If it is set, we know that we can trust a missing peel\n> line to mean that a ref cannot be peeled. Otherwise, we fall\n> back to assuming nothing.\n\nAnother nice, clear explanation of the issue.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  pack-refs.c         |  2 +-\n>  refs.c              | 16 +++++++++++++++-\n>  t/t3211-peel-ref.sh | 22 ++++++++++++++++++++++\n>  3 files changed, 38 insertions(+), 2 deletions(-)\n> \n> diff --git a/pack-refs.c b/pack-refs.c\n> index 10832d7..87ca04d 100644\n> --- a/pack-refs.c\n> +++ b/pack-refs.c\n> @@ -128,7 +128,7 @@ int pack_refs(unsigned int flags)\n>  \t\tdie_errno(\"unable to create ref-pack file structure\");\n>  \n>  \t/* perhaps other traits later as well */\n> -\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled \\n\");\n> +\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n>  \n>  \tfor_each_ref(handle_one_ref, &cbdata);\n>  \tif (ferror(cbdata.refs_file))\n> diff --git a/refs.c b/refs.c\n> index 175b9fc..6a38c41 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -808,6 +808,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  \tstruct ref_entry *last = NULL;\n>  \tchar refline[PATH_MAX];\n>  \tint flag = REF_ISPACKED;\n> +\tint fully_peeled = 0;\n>  \n>  \twhile (fgets(refline, sizeof(refline), f)) {\n>  \t\tunsigned char sha1[20];\n> @@ -818,13 +819,26 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  \t\t\tconst char *traits = refline + sizeof(header) - 1;\n>  \t\t\tif (strstr(traits, \" peeled \"))\n>  \t\t\t\tflag |= REF_KNOWS_PEELED;\n> +\t\t\tif (strstr(traits, \" fully-peeled \"))\n> +\t\t\t\tfully_peeled = 1;\n>  \t\t\t/* perhaps other traits later as well */\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n>  \t\trefname = parse_ref_line(refline, sha1);\n>  \t\tif (refname) {\n> -\t\t\tlast = create_ref_entry(refname, sha1, flag, 1);\n> +\t\t\t/*\n> +\t\t\t * Older git did not write peel lines for anything\n> +\t\t\t * outside of refs/tags/; if the fully-peeled trait\n> +\t\t\t * is not set, we are dealing with such an older\n> +\t\t\t * git and cannot assume an omitted peel value\n> +\t\t\t * means the ref is not a tag object.\n> +\t\t\t */\n> +\t\t\tint this_flag = flag;\n> +\t\t\tif (!fully_peeled && prefixcmp(refname, \"refs/tags/\"))\n> +\t\t\t\tthis_flag &= ~REF_KNOWS_PEELED;\n> +\n> +\t\t\tlast = create_ref_entry(refname, sha1, this_flag, 1);\n>  \t\t\tadd_ref(dir, last);\n>  \t\t\tcontinue;\n>  \t\t}\n\nI have to admit that I am partial to my variant of this code [1] because\nthe logic makes it clearer when the affirmative decision can be made to\nset the REF_KNOWS_PEELED flag.  But this version also looks correct to\nme and equivalent (aside from the idea that a few lines later if a\npeeled value is found then the REF_KNOWS_PEELED bit could also be set).\n\n> diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh\n> index dd5b48b..a8eb1aa 100755\n> --- a/t/t3211-peel-ref.sh\n> +++ b/t/t3211-peel-ref.sh\n> @@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags (packed)' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'create old-style pack-refs without fully-peeled' '\n> +\t# Git no longer writes without fully-peeled, so we just write our own\n> +\t# from scratch; we could also munge the existing file to remove the\n> +\t# fully-peeled bits, but that seems even more prone to failure,\n> +\t# especially if the format ever changes again. At least this way we\n> +\t# know we are emulating exactly what an older git would have written.\n> +\t{\n> +\t\techo \"# pack-refs with: peeled \" &&\n> +\t\tprint_ref \"refs/heads/master\" &&\n> +\t\tprint_ref \"refs/outside/foo\" &&\n> +\t\tprint_ref \"refs/tags/base\" &&\n> +\t\tprint_ref \"refs/tags/foo\" &&\n> +\t\techo \"^$(git rev-parse \"refs/tags/foo^{}\")\"\n> +\t} >tmp &&\n> +\tmv tmp .git/packed-refs\n> +'\n> +\n> +test_expect_success 'refs are peeled outside of refs/tags (old packed)' '\n> +\tgit show-ref -d >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> \n\n[1]\nhttps://github.com/mhagger/git/commit/1c8d4daa2de15a03637d753471a9e5222b01b968\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"211490","messageId":"7vvc8qshgm.fsf@alter.siamese.dyndns.org","threadId":"33196","inReplyTo":"20130316090116.GB26855@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pack-refs: add fully-peeled trait","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-17T05:50:17Z","receivedAt":"2013-03-17T05:50:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The simplest fix would be to always unset the\n> REF_KNOWS_PEELED flag for refs outside of refs/tags that do\n> not have a peel line (if it has a peel line, we know it is\n> valid, but we cannot assume a missing peel line means\n> anything). But that loses an important optimization, as\n> upload-pack should not need to load the object pointed to by\n> refs/heads/foo to determine that it is not a tag.\n\nI think the patch makes sense and in line with what we already\ndiscussed in the thread.\n\nI however wonder if the above implies it may make sense to add this\non top?  Perhaps it is not worth it, because it makes a difference\nonly to a repository with annotated tags outside refs/tags hierarchy\nand still has the packed-refs file that was created with an older\nversion of Git, so we can just tell \"repack with new Git\" to users\nwith such a repository.\n\n refs.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7f84efd..afc4dde 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -847,8 +847,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\t    refline[0] == '^' &&\n \t\t    strlen(refline) == 42 &&\n \t\t    refline[41] == '\\n' &&\n-\t\t    !get_sha1_hex(refline + 1, sha1))\n+\t\t    !get_sha1_hex(refline + 1, sha1)) {\n \t\t\thashcpy(last->u.value.peeled, sha1);\n+\t\t\tlast->flag |= REF_KNOWS_PEELED;\n+\t\t}\n \t}\n }\n \n"},{"id":"211491","messageId":"20130317055520.GC16070@sigill.intra.peff.net","threadId":"33196","inReplyTo":"7vvc8qshgm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] pack-refs: add fully-peeled trait","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-17T05:55:20Z","receivedAt":"2013-03-17T05:55:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 16, 2013 at 10:50:17PM -0700, Junio C Hamano wrote:\n\n> I however wonder if the above implies it may make sense to add this\n> on top?  Perhaps it is not worth it, because it makes a difference\n> only to a repository with annotated tags outside refs/tags hierarchy\n> and still has the packed-refs file that was created with an older\n> version of Git, so we can just tell \"repack with new Git\" to users\n> with such a repository.\n> \n>  refs.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 7f84efd..afc4dde 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -847,8 +847,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  \t\t    refline[0] == '^' &&\n>  \t\t    strlen(refline) == 42 &&\n>  \t\t    refline[41] == '\\n' &&\n> -\t\t    !get_sha1_hex(refline + 1, sha1))\n> +\t\t    !get_sha1_hex(refline + 1, sha1)) {\n>  \t\t\thashcpy(last->u.value.peeled, sha1);\n> +\t\t\tlast->flag |= REF_KNOWS_PEELED;\n> +\t\t}\n>  \t}\n>  }\n>  \n\nAlmost. The older version of Git would not have written those peel lines\nin the first place. So yes, if we saw such a file, we could assume the\npeel lines are valid. But nobody has ever generated that (with the\nexcept of git between my two patches).\n\nI do think it may be worth doing, though, just because it makes the\nhandling of the flag more obvious; somebody reading it later would\nwonder \"hey, shouldn't we be setting REF_KNOWS_PEELED here?\", and it is\nsimple and harmless to just do it, rather than confusing a later reader.\n\nI'll re-roll in a second to incorporate the comments from Michael.\n\n-Peff\n"},{"id":"211492","messageId":"20130317060228.GD16070@sigill.intra.peff.net","threadId":"33196","inReplyTo":"514478C0.6060008@alum.mit.edu","subject":"Re: [PATCH 1/2] pack-refs: write peeled entry for non-tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-17T06:02:28Z","receivedAt":"2013-03-17T06:02:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 16, 2013 at 02:50:56PM +0100, Michael Haggerty wrote:\n\n> > @@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n> >  \t\treturn 0;\n> >  \n> >  \tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(sha1), path);\n> > -\tif (is_tag_ref) {\n> > -\t\tstruct object *o = parse_object(sha1);\n> > -\t\tif (o->type == OBJ_TAG) {\n> > -\t\t\to = deref_tag(o, path, 0);\n> > -\t\t\tif (o)\n> > -\t\t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n> > -\t\t\t\t\tsha1_to_hex(o->sha1));\n> > -\t\t}\n> > +\n> > +\to = parse_object(sha1);\n> > +\tif (o->type == OBJ_TAG) {\n> \n> You suggested that I add a test (o != NULL) at the equivalent place in\n> my code (which was derived from this code).  Granted, my code was\n> explicitly intending to pass invalid SHA1 values to parse_object().  But\n> wouldn't it be a good defensive step to add the same check here?\n\nHmm, yeah. That is not new code, but rather just reindented from above\n(\"diff -w\" makes it much more obvious what is going on).\n\nIt is probably worth dying rather than segfaulting, though it should be\na separate patch (and I do not think it is sane to do anything except\ndie here). I almost wonder if parse_object should die by default on\nbogus or missing objects, and the few callers who really want to handle\nthe error can call parse_object_gently. I do not relish analyzing each\ncaller, though. It would be simpler to add parse_object_or_die.\n\n> > +# This matches show-ref's output\n> > +print_ref() {\n> > +\techo \"`git rev-parse \"$1\"` $1\"\n> > +}\n> > +\n> \n> CodingGuidelines prefers $() over ``.\n\nOld habits die hard. :)\n\nI'll re-roll with your suggestions in a moment.\n\n-Peff\n"},{"id":"211493","messageId":"20130317060408.GE16070@sigill.intra.peff.net","threadId":"33196","inReplyTo":"51447C5E.3050808@alum.mit.edu","subject":"Re: [PATCH 2/2] pack-refs: add fully-peeled trait","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-17T06:04:08Z","receivedAt":"2013-03-17T06:04:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 16, 2013 at 03:06:22PM +0100, Michael Haggerty wrote:\n\n> >  \t\trefname = parse_ref_line(refline, sha1);\n> >  \t\tif (refname) {\n> > -\t\t\tlast = create_ref_entry(refname, sha1, flag, 1);\n> > +\t\t\t/*\n> > +\t\t\t * Older git did not write peel lines for anything\n> > +\t\t\t * outside of refs/tags/; if the fully-peeled trait\n> > +\t\t\t * is not set, we are dealing with such an older\n> > +\t\t\t * git and cannot assume an omitted peel value\n> > +\t\t\t * means the ref is not a tag object.\n> > +\t\t\t */\n> > +\t\t\tint this_flag = flag;\n> > +\t\t\tif (!fully_peeled && prefixcmp(refname, \"refs/tags/\"))\n> > +\t\t\t\tthis_flag &= ~REF_KNOWS_PEELED;\n> > +\n> > +\t\t\tlast = create_ref_entry(refname, sha1, this_flag, 1);\n> >  \t\t\tadd_ref(dir, last);\n> >  \t\t\tcontinue;\n> >  \t\t}\n> \n> I have to admit that I am partial to my variant of this code [1] because\n> the logic makes it clearer when the affirmative decision can be made to\n> set the REF_KNOWS_PEELED flag.  But this version also looks correct to\n> me and equivalent (aside from the idea that a few lines later if a\n> peeled value is found then the REF_KNOWS_PEELED bit could also be set).\n\nYeah, I think they are equivalent, but I agree yours is a little more\nreadable. I'll switch it in my re-roll, and I will go ahead and set the\nREF_KNOWS_PEELED bit when we see a peel line. That code should not be\ntriggered in general, but it is the sane thing for the reader to do, so\nit makes the code more obvious and readable.\n\n-Peff\n"},{"id":"211500","messageId":"20130317082139.GA29505@sigill.intra.peff.net","threadId":"33196","inReplyTo":"20130316090018.GA26708@sigill.intra.peff.net","subject":"[PATCH v2 0/4] peel-ref optimization fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-17T08:21:39Z","receivedAt":"2013-03-17T08:21:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll that takes into account the feedback from round 1:\n\n  [1/4]: avoid segfaults on parse_object failure\n  [2/4]: use parse_object_or_die instead of die(\"bad object\")\n\nThese two patches are new; they are conceptually independent of the rest\nof the series, but there's a textual dependency in later patches.\n\n  [3/4]: pack-refs: write peeled entry for non-tags\n\nSame as before, but rebased on patch 1, and s/``/$()/.\n\n  [4/4]: pack-refs: add fully-peeled trait\n\nRewritten using Michael's approach, which is more readable.\n\n-Peff\n"},{"id":"211501","messageId":"20130317082236.GA29550@sigill.intra.peff.net","threadId":"33196","inReplyTo":"20130317082139.GA29505@sigill.intra.peff.net","subject":"[PATCH v2 1/4] avoid segfaults on parse_object failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-17T08:22:36Z","receivedAt":"2013-03-17T08:22:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many call-sites of parse_object assume that they will get a\nnon-NULL return value; this is not the case if we encounter\nan error while parsing the object.\n\nThis patch adds a wrapper function around parse_object that\nhandles dying automatically, and uses it anywhere we\nimmediately try to access the return value as a non-NULL\npointer (i.e., anywhere that we would currently segfault).\n\nThis wrapper may also be useful in other places. The most\nobvious one is code like:\n\n  o = parse_object(sha1);\n  if (!o)\n\t  die(...);\n\nHowever, these should not be mechanically converted to\nparse_object_or_die, as the die message is sometimes\ncustomized. Later patches can address these sites on a\ncase-by-case basis.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n bundle.c    |  6 +++---\n object.c    | 10 ++++++++++\n object.h    | 13 ++++++++++++-\n pack-refs.c |  2 +-\n 4 files changed, 26 insertions(+), 5 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 8d12816..26ceebd 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -279,12 +279,12 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\tif (buf.len > 0 && buf.buf[0] == '-') {\n \t\t\twrite_or_die(bundle_fd, buf.buf, buf.len);\n \t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n-\t\t\t\tstruct object *object = parse_object(sha1);\n+\t\t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n \t\t\t\tobject->flags |= UNINTERESTING;\n \t\t\t\tadd_pending_object(&revs, object, xstrdup(buf.buf));\n \t\t\t}\n \t\t} else if (!get_sha1_hex(buf.buf, sha1)) {\n-\t\t\tstruct object *object = parse_object(sha1);\n+\t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n \t\t\tobject->flags |= SHOWN;\n \t\t}\n \t}\n@@ -361,7 +361,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\t\t\t * end up triggering \"empty bundle\"\n \t\t\t\t * error.\n \t\t\t\t */\n-\t\t\t\tobj = parse_object(sha1);\n+\t\t\t\tobj = parse_object_or_die(sha1, e->name);\n \t\t\t\tobj->flags |= SHOWN;\n \t\t\t\tadd_pending_object(&revs, obj, e->name);\n \t\t\t}\ndiff --git a/object.c b/object.c\nindex 4af3451..20703f5 100644\n--- a/object.c\n+++ b/object.c\n@@ -185,6 +185,16 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \treturn obj;\n }\n \n+struct object *parse_object_or_die(const unsigned char *sha1,\n+\t\t\t\t   const char *name)\n+{\n+\tstruct object *o = parse_object(sha1);\n+\tif (o)\n+\t\treturn o;\n+\n+\tdie(_(\"unable to parse object: %s\"), name ? name : sha1_to_hex(sha1));\n+}\n+\n struct object *parse_object(const unsigned char *sha1)\n {\n \tunsigned long size;\ndiff --git a/object.h b/object.h\nindex 6a97b6b..97d384b 100644\n--- a/object.h\n+++ b/object.h\n@@ -54,9 +54,20 @@ struct object *parse_object(const unsigned char *sha1);\n \n extern void *create_object(const unsigned char *sha1, int type, void *obj);\n \n-/** Returns the object, having parsed it to find out what it is. **/\n+/*\n+ * Returns the object, having parsed it to find out what it is.\n+ *\n+ * Returns NULL if the object is missing or corrupt.\n+ */\n struct object *parse_object(const unsigned char *sha1);\n \n+/*\n+ * Like parse_object, but will die() instead of returning NULL. If the\n+ * \"name\" parameter is not NULL, it is included in the error message\n+ * (otherwise, the sha1 hex is given).\n+ */\n+struct object *parse_object_or_die(const unsigned char *sha1, const char *name);\n+\n /* Given the result of read_sha1_file(), returns the object after\n  * parsing it.  eaten_p indicates if the object has a borrowed copy\n  * of buffer and the caller should not free() it.\ndiff --git a/pack-refs.c b/pack-refs.c\nindex f09a054..6a689f3 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -40,7 +40,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n \n \tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(sha1), path);\n \tif (is_tag_ref) {\n-\t\tstruct object *o = parse_object(sha1);\n+\t\tstruct object *o = parse_object_or_die(sha1, path);\n \t\tif (o->type == OBJ_TAG) {\n \t\t\to = deref_tag(o, path, 0);\n \t\t\tif (o)\n-- \n1.8.2.rc2.7.gef06216\n"},{"id":"211502","messageId":"20130317082331.GB29550@sigill.intra.peff.net","threadId":"33196","inReplyTo":"20130317082139.GA29505@sigill.intra.peff.net","subject":"[PATCH v2 2/4] use parse_object_or_die instead of die(\"bad object\")","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-17T08:23:31Z","receivedAt":"2013-03-17T08:23:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some call-sites do:\n\n  o = parse_object(sha1);\n  if (!o)\n\t  die(\"bad object %s\", some_name);\n\nWe can now handle that as a one-liner, and get more\nconsistent output.\n\nIn the third case of this patch, it looks like we are losing\ninformation, as the existing message also outputs the sha1\nhex; however, parse_object will already have written a more\nspecific complaint about the sha1, so there is no point in\nrepeating it here.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/grep.c  | 4 +---\n builtin/prune.c | 4 +---\n reachable.c     | 4 +---\n 3 files changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 8025964..159e65d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -820,9 +820,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tunsigned char sha1[20];\n \t\t/* Is it a rev? */\n \t\tif (!get_sha1(arg, sha1)) {\n-\t\t\tstruct object *object = parse_object(sha1);\n-\t\t\tif (!object)\n-\t\t\t\tdie(_(\"bad object %s\"), arg);\n+\t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n \t\t\tif (!seen_dashdash)\n \t\t\t\tverify_non_filename(prefix, arg);\n \t\t\tadd_object_array(object, arg, &list);\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 8cb8b91..85843d4 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -149,9 +149,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\tconst char *name = *argv++;\n \n \t\tif (!get_sha1(name, sha1)) {\n-\t\t\tstruct object *object = parse_object(sha1);\n-\t\t\tif (!object)\n-\t\t\t\tdie(\"bad object: %s\", name);\n+\t\t\tstruct object *object = parse_object_or_die(sha1, name);\n \t\t\tadd_pending_object(&revs, object, \"\");\n \t\t}\n \t\telse\ndiff --git a/reachable.c b/reachable.c\nindex bf79706..e7e6a1e 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -152,11 +152,9 @@ static int add_one_ref(const char *path, const unsigned char *sha1, int flag, vo\n \n static int add_one_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)\n {\n-\tstruct object *object = parse_object(sha1);\n+\tstruct object *object = parse_object_or_die(sha1, path);\n \tstruct rev_info *revs = (struct rev_info *)cb_data;\n \n-\tif (!object)\n-\t\tdie(\"bad object ref: %s:%s\", path, sha1_to_hex(sha1));\n \tadd_pending_object(revs, object, \"\");\n \n \treturn 0;\n-- \n1.8.2.rc2.7.gef06216\n"},{"id":"211503","messageId":"20130317082346.GC29550@sigill.intra.peff.net","threadId":"33196","inReplyTo":"20130317082139.GA29505@sigill.intra.peff.net","subject":"[PATCH v2 3/4] pack-refs: write peeled entry for non-tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-17T08:23:46Z","receivedAt":"2013-03-17T08:23:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we pack an annotated tag ref, we write not only the\nsha1 of the tag object along with the ref, but also the sha1\nobtained by peeling the tag. This lets readers of the\npack-refs file know the peeled value without having to\nactually load the object, speeding up upload-pack's ref\nadvertisement.\n\nThe writer marks a packed-refs file with peeled refs using\nthe \"peeled\" trait at the top of the file. When the reader\nsees this trait, it knows that each ref is either followed\nby its peeled value, or it is not an annotated tag.\n\nHowever, there is a mismatch between the assumptions of the\nreader and writer. The writer will only peel refs under\nrefs/tags, but the reader does not know this; it will assume\na ref without a peeled value must not be a tag object. Thus\nan annotated tag object placed outside of the refs/tags\nhierarchy will not have its peeled value printed by\nupload-pack.\n\nThe simplest way to fix this is to start writing peel values\nfor all refs. This matches what the reader expects for both\nnew and old versions of git.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-refs.c         | 16 ++++++++--------\n t/t3211-peel-ref.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 50 insertions(+), 8 deletions(-)\n create mode 100755 t/t3211-peel-ref.sh\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex 6a689f3..ebde785 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -27,6 +27,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n \t\t\t  int flags, void *cb_data)\n {\n \tstruct pack_refs_cb_data *cb = cb_data;\n+\tstruct object *o;\n \tint is_tag_ref;\n \n \t/* Do not pack the symbolic refs */\n@@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,\n \t\treturn 0;\n \n \tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(sha1), path);\n-\tif (is_tag_ref) {\n-\t\tstruct object *o = parse_object_or_die(sha1, path);\n-\t\tif (o->type == OBJ_TAG) {\n-\t\t\to = deref_tag(o, path, 0);\n-\t\t\tif (o)\n-\t\t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n-\t\t\t\t\tsha1_to_hex(o->sha1));\n-\t\t}\n+\n+\to = parse_object_or_die(sha1, path);\n+\tif (o->type == OBJ_TAG) {\n+\t\to = deref_tag(o, path, 0);\n+\t\tif (o)\n+\t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n+\t\t\t\tsha1_to_hex(o->sha1));\n \t}\n \n \tif ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {\ndiff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh\nnew file mode 100755\nindex 0000000..85f09be\n--- /dev/null\n+++ b/t/t3211-peel-ref.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='tests for the peel_ref optimization of packed-refs'\n+. ./test-lib.sh\n+\n+test_expect_success 'create annotated tag in refs/tags' '\n+\ttest_commit base &&\n+\tgit tag -m annotated foo\n+'\n+\n+test_expect_success 'create annotated tag outside of refs/tags' '\n+\tgit update-ref refs/outside/foo refs/tags/foo\n+'\n+\n+# This matches show-ref's output\n+print_ref() {\n+\techo \"$(git rev-parse \"$1\") $1\"\n+}\n+\n+test_expect_success 'set up expected show-ref output' '\n+\t{\n+\t\tprint_ref \"refs/heads/master\" &&\n+\t\tprint_ref \"refs/outside/foo\" &&\n+\t\tprint_ref \"refs/outside/foo^{}\" &&\n+\t\tprint_ref \"refs/tags/base\" &&\n+\t\tprint_ref \"refs/tags/foo\" &&\n+\t\tprint_ref \"refs/tags/foo^{}\"\n+\t} >expect\n+'\n+\n+test_expect_success 'refs are peeled outside of refs/tags (loose)' '\n+\tgit show-ref -d >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'refs are peeled outside of refs/tags (packed)' '\n+\tgit pack-refs --all &&\n+\tgit show-ref -d >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.2.rc2.7.gef06216\n"},{"id":"211504","messageId":"20130317082829.GD29550@sigill.intra.peff.net","threadId":"33196","inReplyTo":"20130317082139.GA29505@sigill.intra.peff.net","subject":"[PATCH v2 4/4] pack-refs: add fully-peeled trait","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-17T08:28:29Z","receivedAt":"2013-03-17T08:28:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nOlder versions of pack-refs did not write peel lines for\nrefs outside of refs/tags. This meant that on reading the\npack-refs file, we might set the REF_KNOWS_PEELED flag for\nsuch a ref, even though we do not know anything about its\npeeled value.\n\nThe previous commit updated the writer to always peel, no\nmatter what the ref is. That means that packed-refs files\nwritten by newer versions of git are fine to be read by both\nold and new versions of git. However, we still have the\nproblem of reading packed-refs files written by older\nversions of git, or by other implementations which have not\nyet learned the same trick.\n\nThe simplest fix would be to always unset the\nREF_KNOWS_PEELED flag for refs outside of refs/tags that do\nnot have a peel line (if it has a peel line, we know it is\nvalid, but we cannot assume a missing peel line means\nanything). But that loses an important optimization, as\nupload-pack should not need to load the object pointed to by\nrefs/heads/foo to determine that it is not a tag.\n\nInstead, we add a \"fully-peeled\" trait to the packed-refs\nfile. If it is set, we know that we can trust a missing peel\nline to mean that a ref cannot be peeled. Otherwise, we fall\nback to assuming nothing.\n\n[commit message and tests by Jeff King <peff@peff.net>]\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis uses Michael's approach for managing the flags within\nread_packed_refs, which is more readable. As I picked up his\ncode and comments, I realized that there was basically\nnothing of mine left, so I switched the authorship. But do\nnote:\n\n  1. It should have Michael's signoff, which was not present\n     in the commit I lifted the code from.\n\n  2. I tweaked the big comment above read_packed_refs to\n     reduce some ambiguities. Please double-check that I am\n     not putting inaccurate words in your mouth. :)\n\n pack-refs.c         |  2 +-\n refs.c              | 43 +++++++++++++++++++++++++++++++++++++++++--\n t/t3211-peel-ref.sh | 22 ++++++++++++++++++++++\n 3 files changed, 64 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex ebde785..4461f71 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -128,7 +128,7 @@ int pack_refs(unsigned int flags)\n \t\tdie_errno(\"unable to create ref-pack file structure\");\n \n \t/* perhaps other traits later as well */\n-\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled \\n\");\n+\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n \n \tfor_each_ref(handle_one_ref, &cbdata);\n \tif (ferror(cbdata.refs_file))\ndiff --git a/refs.c b/refs.c\nindex 175b9fc..bdeac28 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -803,11 +803,39 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \treturn line;\n }\n \n+/*\n+ * Read f, which is a packed-refs file, into dir.\n+ *\n+ * A comment line of the form \"# pack-refs with: \" may contain zero or\n+ * more traits. We interpret the traits as follows:\n+ *\n+ *   No traits:\n+ *\n+ *\tProbably no references are peeled. But if the file contains a\n+ *\tpeeled value for a reference, we will use it.\n+ *\n+ *   peeled:\n+ *\n+ *      References under \"refs/tags/\", if they *can* be peeled, *are*\n+ *      peeled in this file. References outside of \"refs/tags/\" are\n+ *      probably not peeled even if they could have been, but if we find\n+ *      a peeled value for such a reference we will use it.\n+ *\n+ *   fully-peeled:\n+ *\n+ *      All references in the file that can be peeled are peeled.\n+ *      Inversely (and this is more important, any references in the\n+ *      file for which no peeled value is recorded is not peelable. This\n+ *      trait should typically be written alongside \"fully-peeled\" for\n+ *      compatibility with older clients, but we do not require it\n+ *      (i.e., \"peeled\" is a no-op if \"fully-peeled\" is set).\n+ */\n static void read_packed_refs(FILE *f, struct ref_dir *dir)\n {\n \tstruct ref_entry *last = NULL;\n \tchar refline[PATH_MAX];\n \tint flag = REF_ISPACKED;\n+\tint refs_tags_peeled = 0;\n \n \twhile (fgets(refline, sizeof(refline), f)) {\n \t\tunsigned char sha1[20];\n@@ -816,8 +844,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \n \t\tif (!strncmp(refline, header, sizeof(header)-1)) {\n \t\t\tconst char *traits = refline + sizeof(header) - 1;\n-\t\t\tif (strstr(traits, \" peeled \"))\n+\t\t\tif (strstr(traits, \" fully-peeled \"))\n \t\t\t\tflag |= REF_KNOWS_PEELED;\n+\t\t\telse if (strstr(traits, \" peeled \"))\n+\t\t\t\trefs_tags_peeled = 1;\n \t\t\t/* perhaps other traits later as well */\n \t\t\tcontinue;\n \t\t}\n@@ -825,6 +855,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\trefname = parse_ref_line(refline, sha1);\n \t\tif (refname) {\n \t\t\tlast = create_ref_entry(refname, sha1, flag, 1);\n+\t\t\tif (refs_tags_peeled && !prefixcmp(refname, \"refs/tags/\"))\n+\t\t\t\tlast->flag |= REF_KNOWS_PEELED;\n \t\t\tadd_ref(dir, last);\n \t\t\tcontinue;\n \t\t}\n@@ -832,8 +864,15 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\t    refline[0] == '^' &&\n \t\t    strlen(refline) == 42 &&\n \t\t    refline[41] == '\\n' &&\n-\t\t    !get_sha1_hex(refline + 1, sha1))\n+\t\t    !get_sha1_hex(refline + 1, sha1)) {\n \t\t\thashcpy(last->u.value.peeled, sha1);\n+\t\t\t/*\n+\t\t\t * Regardless of what the file header said,\n+\t\t\t * we definitely know the value of *this*\n+\t\t\t * reference:\n+\t\t\t */\n+\t\t\tlast->flag |= REF_KNOWS_PEELED;\n+\t\t}\n \t}\n }\n \ndiff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh\nindex 85f09be..d4d7792 100755\n--- a/t/t3211-peel-ref.sh\n+++ b/t/t3211-peel-ref.sh\n@@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags (packed)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'create old-style pack-refs without fully-peeled' '\n+\t# Git no longer writes without fully-peeled, so we just write our own\n+\t# from scratch; we could also munge the existing file to remove the\n+\t# fully-peeled bits, but that seems even more prone to failure,\n+\t# especially if the format ever changes again. At least this way we\n+\t# know we are emulating exactly what an older git would have written.\n+\t{\n+\t\techo \"# pack-refs with: peeled \" &&\n+\t\tprint_ref \"refs/heads/master\" &&\n+\t\tprint_ref \"refs/outside/foo\" &&\n+\t\tprint_ref \"refs/tags/base\" &&\n+\t\tprint_ref \"refs/tags/foo\" &&\n+\t\techo \"^$(git rev-parse \"refs/tags/foo^{}\")\"\n+\t} >tmp &&\n+\tmv tmp .git/packed-refs\n+'\n+\n+test_expect_success 'refs are peeled outside of refs/tags (old packed)' '\n+\tgit show-ref -d >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.2.rc2.7.gef06216\n"},{"id":"211524","messageId":"7vli9lre1j.fsf@alter.siamese.dyndns.org","threadId":"33196","inReplyTo":"20130317082829.GD29550@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-17T20:01:44Z","receivedAt":"2013-03-17T20:01:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> From: Michael Haggerty <mhagger@alum.mit.edu>\n>\n> Older versions of pack-refs did not write peel lines for\n> refs outside of refs/tags. This meant that on reading the\n> pack-refs file, we might set the REF_KNOWS_PEELED flag for\n> such a ref, even though we do not know anything about its\n> peeled value.\n>\n> The previous commit updated the writer to always peel, no\n> matter what the ref is. That means that packed-refs files\n> written by newer versions of git are fine to be read by both\n> old and new versions of git. However, we still have the\n> problem of reading packed-refs files written by older\n> versions of git, or by other implementations which have not\n> yet learned the same trick.\n>\n> The simplest fix would be to always unset the\n> REF_KNOWS_PEELED flag for refs outside of refs/tags that do\n> not have a peel line (if it has a peel line, we know it is\n> valid, but we cannot assume a missing peel line means\n> anything). But that loses an important optimization, as\n> upload-pack should not need to load the object pointed to by\n> refs/heads/foo to determine that it is not a tag.\n>\n> Instead, we add a \"fully-peeled\" trait to the packed-refs\n> file. If it is set, we know that we can trust a missing peel\n> line to mean that a ref cannot be peeled. Otherwise, we fall\n> back to assuming nothing.\n>\n> [commit message and tests by Jeff King <peff@peff.net>]\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This uses Michael's approach for managing the flags within\n> read_packed_refs, which is more readable. As I picked up his\n> code and comments, I realized that there was basically\n> nothing of mine left, so I switched the authorship. But do\n> note:\n>\n>   1. It should have Michael's signoff, which was not present\n>      in the commit I lifted the code from.\n>\n>   2. I tweaked the big comment above read_packed_refs to\n>      reduce some ambiguities. Please double-check that I am\n>      not putting inaccurate words in your mouth. :)\n>\n>  pack-refs.c         |  2 +-\n>  refs.c              | 43 +++++++++++++++++++++++++++++++++++++++++--\n>  t/t3211-peel-ref.sh | 22 ++++++++++++++++++++++\n>  3 files changed, 64 insertions(+), 3 deletions(-)\n>\n> diff --git a/pack-refs.c b/pack-refs.c\n> index ebde785..4461f71 100644\n> --- a/pack-refs.c\n> +++ b/pack-refs.c\n> @@ -128,7 +128,7 @@ int pack_refs(unsigned int flags)\n>  \t\tdie_errno(\"unable to create ref-pack file structure\");\n>  \n>  \t/* perhaps other traits later as well */\n> -\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled \\n\");\n> +\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n>  \n>  \tfor_each_ref(handle_one_ref, &cbdata);\n>  \tif (ferror(cbdata.refs_file))\n> diff --git a/refs.c b/refs.c\n> index 175b9fc..bdeac28 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -803,11 +803,39 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  \treturn line;\n>  }\n>  \n> +/*\n> + * Read f, which is a packed-refs file, into dir.\n> + *\n> + * A comment line of the form \"# pack-refs with: \" may contain zero or\n> + * more traits. We interpret the traits as follows:\n> + *\n> + *   No traits:\n> + *\n> + *\tProbably no references are peeled. But if the file contains a\n> + *\tpeeled value for a reference, we will use it.\n> + *\n> + *   peeled:\n> + *\n> + *      References under \"refs/tags/\", if they *can* be peeled, *are*\n> + *      peeled in this file. References outside of \"refs/tags/\" are\n> + *      probably not peeled even if they could have been, but if we find\n> + *      a peeled value for such a reference we will use it.\n> + *\n> + *   fully-peeled:\n> + *\n> + *      All references in the file that can be peeled are peeled.\n> + *      Inversely (and this is more important, any references in the\n\nA missing closing paren after \"more important\".  Also the e-mail\nquote reveals there is some inconsistent indentation (HTs vs runs of\nSPs) here.\n\n> + *      file for which no peeled value is recorded is not peelable. This\n> + *      trait should typically be written alongside \"fully-peeled\" for\n\nAlongside \"peeled\", no?\n\n> @@ -816,8 +844,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  \n>  \t\tif (!strncmp(refline, header, sizeof(header)-1)) {\n>  \t\t\tconst char *traits = refline + sizeof(header) - 1;\n> -\t\t\tif (strstr(traits, \" peeled \"))\n> +\t\t\tif (strstr(traits, \" fully-peeled \"))\n>  \t\t\t\tflag |= REF_KNOWS_PEELED;\n> +\t\t\telse if (strstr(traits, \" peeled \"))\n> +\t\t\t\trefs_tags_peeled = 1;\n>  \t\t\t/* perhaps other traits later as well */\n>  \t\t\tcontinue;\n>  \t\t}\n> @@ -825,6 +855,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  \t\trefname = parse_ref_line(refline, sha1);\n>  \t\tif (refname) {\n>  \t\t\tlast = create_ref_entry(refname, sha1, flag, 1);\n> +\t\t\tif (refs_tags_peeled && !prefixcmp(refname, \"refs/tags/\"))\n> +\t\t\t\tlast->flag |= REF_KNOWS_PEELED;\n\nI am not sure why you find this any more readable.\n\nThe \"flag\" is set earlier to contain REF_KNOWS_PEELED only when we\nhave fully-peeled trait, and peeled trait is recorded as a separate\nlocal variable.  The fully-peeled case sets the flag by passing the\nflag to create_ref_entry() but the peeled case adds it to last->flag\nmanually after the fact.\n\nIf you set two local variables when you read the traits (iow, no\nfutzing with \"flag\" there), this part would become either:\n\n\tlast = create_ref_entry(refname, sha1, REF_ISPACKED, 1);\n\tif (refs_fully_peeled ||\n            (refs_tags_peeled && !prefixcmp(refname, \"refs/tags/\")))\n\t    last->flag |= REF_KNOWS_PEELED;\n\nor\n\n\tflag = REF_ISPACKED;\n\tif (refs_fully_peeled ||\n            (refs_tags_peeled && !prefixcmp(refname, \"refs/tags/\")))\n\t    flag |= REF_KNOWS_PEELED;\n\tlast = create_ref_entry(refname, sha1, flag, 1);\n\neither of which would be much more readable at least to me.\n"},{"id":"211542","messageId":"51468615.70503@alum.mit.edu","threadId":"33196","inReplyTo":"20130317082829.GD29550@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-03-18T03:12:21Z","receivedAt":"2013-03-18T03:12:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nand ACK for the whole series, once Junio's points are addressed.\n\nRegarding Junio's readability suggestion: I agree that his versions are\na bit more readable, albeit at the expense of having to evaluate a bit\nmore logic for each reference rather than just once when the header line\nis handled.  So I don't have a preference either way.\n\nMichael\n\nOn 03/17/2013 09:28 AM, Jeff King wrote:\n> From: Michael Haggerty <mhagger@alum.mit.edu>\n> \n> Older versions of pack-refs did not write peel lines for\n> refs outside of refs/tags. This meant that on reading the\n> pack-refs file, we might set the REF_KNOWS_PEELED flag for\n> such a ref, even though we do not know anything about its\n> peeled value.\n> \n> The previous commit updated the writer to always peel, no\n> matter what the ref is. That means that packed-refs files\n> written by newer versions of git are fine to be read by both\n> old and new versions of git. However, we still have the\n> problem of reading packed-refs files written by older\n> versions of git, or by other implementations which have not\n> yet learned the same trick.\n> \n> The simplest fix would be to always unset the\n> REF_KNOWS_PEELED flag for refs outside of refs/tags that do\n> not have a peel line (if it has a peel line, we know it is\n> valid, but we cannot assume a missing peel line means\n> anything). But that loses an important optimization, as\n> upload-pack should not need to load the object pointed to by\n> refs/heads/foo to determine that it is not a tag.\n> \n> Instead, we add a \"fully-peeled\" trait to the packed-refs\n> file. If it is set, we know that we can trust a missing peel\n> line to mean that a ref cannot be peeled. Otherwise, we fall\n> back to assuming nothing.\n> \n> [commit message and tests by Jeff King <peff@peff.net>]\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This uses Michael's approach for managing the flags within\n> read_packed_refs, which is more readable. As I picked up his\n> code and comments, I realized that there was basically\n> nothing of mine left, so I switched the authorship. But do\n> note:\n> \n>   1. It should have Michael's signoff, which was not present\n>      in the commit I lifted the code from.\n> \n>   2. I tweaked the big comment above read_packed_refs to\n>      reduce some ambiguities. Please double-check that I am\n>      not putting inaccurate words in your mouth. :)\n> \n>  pack-refs.c         |  2 +-\n>  refs.c              | 43 +++++++++++++++++++++++++++++++++++++++++--\n>  t/t3211-peel-ref.sh | 22 ++++++++++++++++++++++\n>  3 files changed, 64 insertions(+), 3 deletions(-)\n> \n> diff --git a/pack-refs.c b/pack-refs.c\n> index ebde785..4461f71 100644\n> --- a/pack-refs.c\n> +++ b/pack-refs.c\n> @@ -128,7 +128,7 @@ int pack_refs(unsigned int flags)\n>  \t\tdie_errno(\"unable to create ref-pack file structure\");\n>  \n>  \t/* perhaps other traits later as well */\n> -\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled \\n\");\n> +\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n>  \n>  \tfor_each_ref(handle_one_ref, &cbdata);\n>  \tif (ferror(cbdata.refs_file))\n> diff --git a/refs.c b/refs.c\n> index 175b9fc..bdeac28 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -803,11 +803,39 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  \treturn line;\n>  }\n>  \n> +/*\n> + * Read f, which is a packed-refs file, into dir.\n> + *\n> + * A comment line of the form \"# pack-refs with: \" may contain zero or\n> + * more traits. We interpret the traits as follows:\n> + *\n> + *   No traits:\n> + *\n> + *\tProbably no references are peeled. But if the file contains a\n> + *\tpeeled value for a reference, we will use it.\n> + *\n> + *   peeled:\n> + *\n> + *      References under \"refs/tags/\", if they *can* be peeled, *are*\n> + *      peeled in this file. References outside of \"refs/tags/\" are\n> + *      probably not peeled even if they could have been, but if we find\n> + *      a peeled value for such a reference we will use it.\n> + *\n> + *   fully-peeled:\n> + *\n> + *      All references in the file that can be peeled are peeled.\n> + *      Inversely (and this is more important, any references in the\n> + *      file for which no peeled value is recorded is not peelable. This\n> + *      trait should typically be written alongside \"fully-peeled\" for\n> + *      compatibility with older clients, but we do not require it\n> + *      (i.e., \"peeled\" is a no-op if \"fully-peeled\" is set).\n> + */\n>  static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  {\n>  \tstruct ref_entry *last = NULL;\n>  \tchar refline[PATH_MAX];\n>  \tint flag = REF_ISPACKED;\n> +\tint refs_tags_peeled = 0;\n>  \n>  \twhile (fgets(refline, sizeof(refline), f)) {\n>  \t\tunsigned char sha1[20];\n> @@ -816,8 +844,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  \n>  \t\tif (!strncmp(refline, header, sizeof(header)-1)) {\n>  \t\t\tconst char *traits = refline + sizeof(header) - 1;\n> -\t\t\tif (strstr(traits, \" peeled \"))\n> +\t\t\tif (strstr(traits, \" fully-peeled \"))\n>  \t\t\t\tflag |= REF_KNOWS_PEELED;\n> +\t\t\telse if (strstr(traits, \" peeled \"))\n> +\t\t\t\trefs_tags_peeled = 1;\n>  \t\t\t/* perhaps other traits later as well */\n>  \t\t\tcontinue;\n>  \t\t}\n> @@ -825,6 +855,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  \t\trefname = parse_ref_line(refline, sha1);\n>  \t\tif (refname) {\n>  \t\t\tlast = create_ref_entry(refname, sha1, flag, 1);\n> +\t\t\tif (refs_tags_peeled && !prefixcmp(refname, \"refs/tags/\"))\n> +\t\t\t\tlast->flag |= REF_KNOWS_PEELED;\n>  \t\t\tadd_ref(dir, last);\n>  \t\t\tcontinue;\n>  \t\t}\n> @@ -832,8 +864,15 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n>  \t\t    refline[0] == '^' &&\n>  \t\t    strlen(refline) == 42 &&\n>  \t\t    refline[41] == '\\n' &&\n> -\t\t    !get_sha1_hex(refline + 1, sha1))\n> +\t\t    !get_sha1_hex(refline + 1, sha1)) {\n>  \t\t\thashcpy(last->u.value.peeled, sha1);\n> +\t\t\t/*\n> +\t\t\t * Regardless of what the file header said,\n> +\t\t\t * we definitely know the value of *this*\n> +\t\t\t * reference:\n> +\t\t\t */\n> +\t\t\tlast->flag |= REF_KNOWS_PEELED;\n> +\t\t}\n>  \t}\n>  }\n>  \n> diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh\n> index 85f09be..d4d7792 100755\n> --- a/t/t3211-peel-ref.sh\n> +++ b/t/t3211-peel-ref.sh\n> @@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags (packed)' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'create old-style pack-refs without fully-peeled' '\n> +\t# Git no longer writes without fully-peeled, so we just write our own\n> +\t# from scratch; we could also munge the existing file to remove the\n> +\t# fully-peeled bits, but that seems even more prone to failure,\n> +\t# especially if the format ever changes again. At least this way we\n> +\t# know we are emulating exactly what an older git would have written.\n> +\t{\n> +\t\techo \"# pack-refs with: peeled \" &&\n> +\t\tprint_ref \"refs/heads/master\" &&\n> +\t\tprint_ref \"refs/outside/foo\" &&\n> +\t\tprint_ref \"refs/tags/base\" &&\n> +\t\tprint_ref \"refs/tags/foo\" &&\n> +\t\techo \"^$(git rev-parse \"refs/tags/foo^{}\")\"\n> +\t} >tmp &&\n> +\tmv tmp .git/packed-refs\n> +'\n> +\n> +test_expect_success 'refs are peeled outside of refs/tags (old packed)' '\n> +\tgit show-ref -d >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"211557","messageId":"20130318113732.GA14789@sigill.intra.peff.net","threadId":"33196","inReplyTo":"7vli9lre1j.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3 4/4] pack-refs: add fully-peeled trait","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-18T11:37:32Z","receivedAt":"2013-03-18T11:37:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 17, 2013 at 01:01:44PM -0700, Junio C Hamano wrote:\n\n> > + *      All references in the file that can be peeled are peeled.\n> > + *      Inversely (and this is more important, any references in the\n> \n> A missing closing paren after \"more important\".  Also the e-mail\n> quote reveals there is some inconsistent indentation (HTs vs runs of\n> SPs) here.\n\nThanks, will fix (the whitespace damage is due to me cutting and pasting\nfrom Michael's commit).\n\n> \n> > + *      file for which no peeled value is recorded is not peelable. This\n> > + *      trait should typically be written alongside \"fully-peeled\" for\n> \n> Alongside \"peeled\", no?\n\nUrgh, yes. Will fix.\n\n> [...]\n> I am not sure why you find this any more readable.\n\nI was trying to avoid the \"set PEELED globally, but sometimes unset it\nfor specific refs\" pattern which I think is confusing to the reader. But\nI think what you wrote is even better. I used an enum rather than two\nvariables to make it clear that only ones takes effect. I had wanted to\nuse a switch, also, but you end up either repeating yourself, or doing\nthis gross fall-through:\n\n  switch (peeled) {\n  case PEELED_TAGS:\n          if (prefixcmp(refname, \"refs/tags/\"))\n                  break;\n          /* fall-through */\n  case PEELED_FULLY:\n          last->ref |= REF_KNOWS_PEELED;\n          break;\n  default:\n          /* we know nothing */\n  }\n\nSo I just stuck with the conditional.\n\nHere's the re-roll.\n\n-- >8 --\nFrom: Michael Haggerty <mhagger@alum.mit.edu>\nSubject: [PATCH] pack-refs: add fully-peeled trait\n\nOlder versions of pack-refs did not write peel lines for\nrefs outside of refs/tags. This meant that on reading the\npack-refs file, we might set the REF_KNOWS_PEELED flag for\nsuch a ref, even though we do not know anything about its\npeeled value.\n\nThe previous commit updated the writer to always peel, no\nmatter what the ref is. That means that packed-refs files\nwritten by newer versions of git are fine to be read by both\nold and new versions of git. However, we still have the\nproblem of reading packed-refs files written by older\nversions of git, or by other implementations which have not\nyet learned the same trick.\n\nThe simplest fix would be to always unset the\nREF_KNOWS_PEELED flag for refs outside of refs/tags that do\nnot have a peel line (if it has a peel line, we know it is\nvalid, but we cannot assume a missing peel line means\nanything). But that loses an important optimization, as\nupload-pack should not need to load the object pointed to by\nrefs/heads/foo to determine that it is not a tag.\n\nInstead, we add a \"fully-peeled\" trait to the packed-refs\nfile. If it is set, we know that we can trust a missing peel\nline to mean that a ref cannot be peeled. Otherwise, we fall\nback to assuming nothing.\n\n[commit message and tests by Jeff King <peff@peff.net>]\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-refs.c         |  2 +-\n refs.c              | 49 ++++++++++++++++++++++++++++++++++++++++++++-----\n t/t3211-peel-ref.sh | 22 ++++++++++++++++++++++\n 3 files changed, 67 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex ebde785..4461f71 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -128,7 +128,7 @@ int pack_refs(unsigned int flags)\n \t\tdie_errno(\"unable to create ref-pack file structure\");\n \n \t/* perhaps other traits later as well */\n-\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled \\n\");\n+\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n \n \tfor_each_ref(handle_one_ref, &cbdata);\n \tif (ferror(cbdata.refs_file))\ndiff --git a/refs.c b/refs.c\nindex 175b9fc..e2b760d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -803,11 +803,38 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \treturn line;\n }\n \n+/*\n+ * Read f, which is a packed-refs file, into dir.\n+ *\n+ * A comment line of the form \"# pack-refs with: \" may contain zero or\n+ * more traits. We interpret the traits as follows:\n+ *\n+ *   No traits:\n+ *\n+ *      Probably no references are peeled. But if the file contains a\n+ *      peeled value for a reference, we will use it.\n+ *\n+ *   peeled:\n+ *\n+ *      References under \"refs/tags/\", if they *can* be peeled, *are*\n+ *      peeled in this file. References outside of \"refs/tags/\" are\n+ *      probably not peeled even if they could have been, but if we find\n+ *      a peeled value for such a reference we will use it.\n+ *\n+ *   fully-peeled:\n+ *\n+ *      All references in the file that can be peeled are peeled.\n+ *      Inversely (and this is more important), any references in the\n+ *      file for which no peeled value is recorded is not peelable. This\n+ *      trait should typically be written alongside \"peeled\" for\n+ *      compatibility with older clients, but we do not require it\n+ *      (i.e., \"peeled\" is a no-op if \"fully-peeled\" is set).\n+ */\n static void read_packed_refs(FILE *f, struct ref_dir *dir)\n {\n \tstruct ref_entry *last = NULL;\n \tchar refline[PATH_MAX];\n-\tint flag = REF_ISPACKED;\n+\tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;\n \n \twhile (fgets(refline, sizeof(refline), f)) {\n \t\tunsigned char sha1[20];\n@@ -816,15 +843,20 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \n \t\tif (!strncmp(refline, header, sizeof(header)-1)) {\n \t\t\tconst char *traits = refline + sizeof(header) - 1;\n-\t\t\tif (strstr(traits, \" peeled \"))\n-\t\t\t\tflag |= REF_KNOWS_PEELED;\n+\t\t\tif (strstr(traits, \" fully-peeled \"))\n+\t\t\t\tpeeled = PEELED_FULLY;\n+\t\t\telse if (strstr(traits, \" peeled \"))\n+\t\t\t\tpeeled = PEELED_TAGS;\n \t\t\t/* perhaps other traits later as well */\n \t\t\tcontinue;\n \t\t}\n \n \t\trefname = parse_ref_line(refline, sha1);\n \t\tif (refname) {\n-\t\t\tlast = create_ref_entry(refname, sha1, flag, 1);\n+\t\t\tlast = create_ref_entry(refname, sha1, REF_ISPACKED, 1);\n+\t\t\tif (peeled == PEELED_FULLY ||\n+\t\t\t    (peeled == PEELED_TAGS && !prefixcmp(refname, \"refs/tags/\")))\n+\t\t\t\tlast->flag |= REF_KNOWS_PEELED;\n \t\t\tadd_ref(dir, last);\n \t\t\tcontinue;\n \t\t}\n@@ -832,8 +864,15 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\t    refline[0] == '^' &&\n \t\t    strlen(refline) == 42 &&\n \t\t    refline[41] == '\\n' &&\n-\t\t    !get_sha1_hex(refline + 1, sha1))\n+\t\t    !get_sha1_hex(refline + 1, sha1)) {\n \t\t\thashcpy(last->u.value.peeled, sha1);\n+\t\t\t/*\n+\t\t\t * Regardless of what the file header said,\n+\t\t\t * we definitely know the value of *this*\n+\t\t\t * reference:\n+\t\t\t */\n+\t\t\tlast->flag |= REF_KNOWS_PEELED;\n+\t\t}\n \t}\n }\n \ndiff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh\nindex 85f09be..d4d7792 100755\n--- a/t/t3211-peel-ref.sh\n+++ b/t/t3211-peel-ref.sh\n@@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags (packed)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'create old-style pack-refs without fully-peeled' '\n+\t# Git no longer writes without fully-peeled, so we just write our own\n+\t# from scratch; we could also munge the existing file to remove the\n+\t# fully-peeled bits, but that seems even more prone to failure,\n+\t# especially if the format ever changes again. At least this way we\n+\t# know we are emulating exactly what an older git would have written.\n+\t{\n+\t\techo \"# pack-refs with: peeled \" &&\n+\t\tprint_ref \"refs/heads/master\" &&\n+\t\tprint_ref \"refs/outside/foo\" &&\n+\t\tprint_ref \"refs/tags/base\" &&\n+\t\tprint_ref \"refs/tags/foo\" &&\n+\t\techo \"^$(git rev-parse \"refs/tags/foo^{}\")\"\n+\t} >tmp &&\n+\tmv tmp .git/packed-refs\n+'\n+\n+test_expect_success 'refs are peeled outside of refs/tags (old packed)' '\n+\tgit show-ref -d >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.2.rc2.7.gef06216\n"},{"id":"211577","messageId":"7v8v5kpwrd.fsf@alter.siamese.dyndns.org","threadId":"33196","inReplyTo":"51468615.70503@alum.mit.edu","subject":"Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-18T15:12:38Z","receivedAt":"2013-03-18T15:12:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n>\n> and ACK for the whole series, once Junio's points are addressed.\n>\n> Regarding Junio's readability suggestion: I agree that his versions are\n> a bit more readable, albeit at the expense of having to evaluate a bit\n> more logic for each reference rather than just once when the header line\n> is handled.  So I don't have a preference either way.\n\nThe way the conditional is written, in the longer term we\nwill almost always compare \"peeled == PEELED_FULLY\", and otherwise\nwe will do the same !prefixcmp(refs/tags/), so I do not think there\nis \"more logic\" that matters compared to the original.\n\nThanks, both; will replace what was queued with \"SQUASH???\".\n"},{"id":"211580","messageId":"7vzjy0oerb.fsf@alter.siamese.dyndns.org","threadId":"33196","inReplyTo":"20130318113732.GA14789@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] pack-refs: add fully-peeled trait","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-18T16:26:48Z","receivedAt":"2013-03-18T16:26:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here's the re-roll.\n\nAbove reasoning (elided) look sensible.  Thanks; will replace.\n"}]}