{"thread":{"id":"44015","subject":"[PATCH 0/5] handle tag recursion in pack-objects --include-tag","startedAt":"2016-09-05T21:51:49Z","lastAt":"2016-09-07T19:46:45Z","messageCount":10,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"301143","messageId":"20160905215141.b6unqtjqko7775is@sigill.intra.peff.net","threadId":"44015","inReplyTo":null,"subject":"[PATCH 0/5] handle tag recursion in pack-objects --include-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-05T21:51:41Z","receivedAt":"2016-09-05T21:51:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you have a tag of a tag of a commit, and the middle tag isn't\notherwise reachable, then \"pack-objects --include-tag\" will not include\nthe middle tag, and certain invocations of \"clone\" will fail. See the\nfinal patch below for the gory details.\n\nThe first five patches are just test cleanup and modernization in\npreparation (though note that the 3rd one actually fixes a minor bug in\nthe test script).\n\n  [1/5]: t5305: move cleanup into test block\n  [2/5]: t5305: drop \"dry-run\" of unpack-objects\n  [3/5]: t5305: use \"git -C\"\n  [4/5]: t5305: simplify packname handling\n  [5/5]: pack-objects: walk tag chains for --include-tag\n\n-Peff\n"},{"id":"301144","messageId":"20160905215209.pzuxtfprvfoppalt@sigill.intra.peff.net","threadId":"44015","inReplyTo":"20160905215141.b6unqtjqko7775is@sigill.intra.peff.net","subject":"[PATCH 1/5] t5305: move cleanup into test block","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-05T21:52:10Z","receivedAt":"2016-09-05T21:52:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We usually try to avoid doing any significant actions\noutside of test blocks. Although \"rm -rf\" is unlikely to\neither fail or to generate output, moving these to the\npoint of use makes it more clear that they are part of the\noverall setup of \"clone.git\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5305-include-tag.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh\nindex f314ad5..6018404 100755\n--- a/t/t5305-include-tag.sh\n+++ b/t/t5305-include-tag.sh\n@@ -25,7 +25,6 @@ test_expect_success setup '\n \t} >obj-list\n '\n \n-rm -rf clone.git\n test_expect_success 'pack without --include-tag' '\n \tpackname_1=$(git pack-objects \\\n \t\t--window=0 \\\n@@ -33,6 +32,7 @@ test_expect_success 'pack without --include-tag' '\n '\n \n test_expect_success 'unpack objects' '\n+\trm -rf clone.git &&\n \t(\n \t\tGIT_DIR=clone.git &&\n \t\texport GIT_DIR &&\n@@ -51,7 +51,6 @@ test_expect_success 'check unpacked result (have commit, no tag)' '\n \ttest_cmp list.expect list.actual\n '\n \n-rm -rf clone.git\n test_expect_success 'pack with --include-tag' '\n \tpackname_1=$(git pack-objects \\\n \t\t--window=0 \\\n@@ -60,6 +59,7 @@ test_expect_success 'pack with --include-tag' '\n '\n \n test_expect_success 'unpack objects' '\n+\trm -rf clone.git &&\n \t(\n \t\tGIT_DIR=clone.git &&\n \t\texport GIT_DIR &&\n-- \n2.10.0.rc2.154.gb4a4b8b\n\n"},{"id":"301145","messageId":"20160905215213.apdmh7nubqxokcxw@sigill.intra.peff.net","threadId":"44015","inReplyTo":"20160905215141.b6unqtjqko7775is@sigill.intra.peff.net","subject":"[PATCH 2/5] t5305: drop \"dry-run\" of unpack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-05T21:52:14Z","receivedAt":"2016-09-05T21:52:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For each test we do a dry-run of unpack-objects, followed by\na real run, followed by confirming that it contained the\nobjects we expected. The dry-run is telling us nothing, as\nany errors it encounters would be found in the real run.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5305-include-tag.sh | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh\nindex 6018404..787fc83 100755\n--- a/t/t5305-include-tag.sh\n+++ b/t/t5305-include-tag.sh\n@@ -37,7 +37,6 @@ test_expect_success 'unpack objects' '\n \t\tGIT_DIR=clone.git &&\n \t\texport GIT_DIR &&\n \t\tgit init &&\n-\t\tgit unpack-objects -n <test-1-${packname_1}.pack &&\n \t\tgit unpack-objects <test-1-${packname_1}.pack\n \t)\n '\n@@ -64,7 +63,6 @@ test_expect_success 'unpack objects' '\n \t\tGIT_DIR=clone.git &&\n \t\texport GIT_DIR &&\n \t\tgit init &&\n-\t\tgit unpack-objects -n <test-2-${packname_1}.pack &&\n \t\tgit unpack-objects <test-2-${packname_1}.pack\n \t)\n '\n-- \n2.10.0.rc2.154.gb4a4b8b\n\n"},{"id":"301146","messageId":"20160905215217.6jg2tois6uavwmkr@sigill.intra.peff.net","threadId":"44015","inReplyTo":"20160905215141.b6unqtjqko7775is@sigill.intra.peff.net","subject":"[PATCH 3/5] t5305: use \"git -C\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-05T21:52:17Z","receivedAt":"2016-09-05T21:52:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This test unpacks objects into a separate repository, and\naccesses it by setting GIT_DIR in a subshell. We can do the\nsame thing these days by using \"git init <repo>\" and \"git\n-C\". In most cases this is shorter, though when there are\nmultiple commands, we may end up repeating the \"-C\".\n\nHowever, this repetition can actually be a good thing. This\npatch also fixes a bug introduced by 512477b (tests: use\n\"env\" to run commands with temporary env-var settings,\n2014-03-18). That commit essentially converted:\n\n   (GIT_DIR=...; export GIT_DIR\n    cmd1 &&\n    cmd2)\n\ninto:\n\n   (GIT_DIR=... cmd1 &&\n    cmd2)\n\nwhich obviously loses the GIT_DIR setting for cmd2 (we never\nnoticed the bug because it simply runs \"cmd2\" in the parent\nrepo, which means we were simply failing to test anything\ninteresting). By using \"git -C\" rather than a subshell, it\nbecomes quite obvious where each command is supposed to be\nrunning.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5305-include-tag.sh | 28 +++++++---------------------\n 1 file changed, 7 insertions(+), 21 deletions(-)\n\ndiff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh\nindex 787fc83..089a3e9 100755\n--- a/t/t5305-include-tag.sh\n+++ b/t/t5305-include-tag.sh\n@@ -33,20 +33,14 @@ test_expect_success 'pack without --include-tag' '\n \n test_expect_success 'unpack objects' '\n \trm -rf clone.git &&\n-\t(\n-\t\tGIT_DIR=clone.git &&\n-\t\texport GIT_DIR &&\n-\t\tgit init &&\n-\t\tgit unpack-objects <test-1-${packname_1}.pack\n-\t)\n+\tgit init clone.git &&\n+\tgit -C clone.git unpack-objects <test-1-${packname_1}.pack\n '\n \n test_expect_success 'check unpacked result (have commit, no tag)' '\n \tgit rev-list --objects $commit >list.expect &&\n-\t(\n-\t\ttest_must_fail env GIT_DIR=clone.git git cat-file -e $tag &&\n-\t\tgit rev-list --objects $commit\n-\t) >list.actual &&\n+\ttest_must_fail git -C clone.git cat-file -e $tag &&\n+\tgit -C clone.git rev-list --objects $commit >list.actual &&\n \ttest_cmp list.expect list.actual\n '\n \n@@ -59,21 +53,13 @@ test_expect_success 'pack with --include-tag' '\n \n test_expect_success 'unpack objects' '\n \trm -rf clone.git &&\n-\t(\n-\t\tGIT_DIR=clone.git &&\n-\t\texport GIT_DIR &&\n-\t\tgit init &&\n-\t\tgit unpack-objects <test-2-${packname_1}.pack\n-\t)\n+\tgit init clone.git &&\n+\tgit -C clone.git unpack-objects <test-2-${packname_1}.pack\n '\n \n test_expect_success 'check unpacked result (have commit, have tag)' '\n \tgit rev-list --objects mytag >list.expect &&\n-\t(\n-\t\tGIT_DIR=clone.git &&\n-\t\texport GIT_DIR &&\n-\t\tgit rev-list --objects $tag\n-\t) >list.actual &&\n+\tgit -C clone.git rev-list --objects $tag >list.actual &&\n \ttest_cmp list.expect list.actual\n '\n \n-- \n2.10.0.rc2.154.gb4a4b8b\n\n"},{"id":"301147","messageId":"20160905215221.ok72rarfk6kl2yi2@sigill.intra.peff.net","threadId":"44015","inReplyTo":"20160905215141.b6unqtjqko7775is@sigill.intra.peff.net","subject":"[PATCH 4/5] t5305: simplify packname handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-05T21:52:22Z","receivedAt":"2016-09-05T21:52:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generate a series of packfiles test-1-$pack,\ntest-2-$pack, with different properties and then examine\nthem. However we always store the packname generated by\npack-objects in the variable packname_1. This probably was\nmeant to be packname_2 in the second test, but it turns out\nthat it doesn't matter: once we are done with the first\npack, we can just keep using the same $packname variable.\n\nSo let's drop the confusing \"_1\" parameter. At the same\ntime, let's give test-1 and test-2 more descriptive names,\nwhich can help keep them straight (note that we _could_\nlikewise overwrite the packfiles in each test, but by using\nseparate filenames, we are sure that test 2 does not\naccidentally use the packfile from test 1).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5305-include-tag.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh\nindex 089a3e9..e3c6c62 100755\n--- a/t/t5305-include-tag.sh\n+++ b/t/t5305-include-tag.sh\n@@ -26,15 +26,15 @@ test_expect_success setup '\n '\n \n test_expect_success 'pack without --include-tag' '\n-\tpackname_1=$(git pack-objects \\\n+\tpackname=$(git pack-objects \\\n \t\t--window=0 \\\n-\t\ttest-1 <obj-list)\n+\t\ttest-no-include <obj-list)\n '\n \n test_expect_success 'unpack objects' '\n \trm -rf clone.git &&\n \tgit init clone.git &&\n-\tgit -C clone.git unpack-objects <test-1-${packname_1}.pack\n+\tgit -C clone.git unpack-objects <test-no-include-${packname}.pack\n '\n \n test_expect_success 'check unpacked result (have commit, no tag)' '\n@@ -45,16 +45,16 @@ test_expect_success 'check unpacked result (have commit, no tag)' '\n '\n \n test_expect_success 'pack with --include-tag' '\n-\tpackname_1=$(git pack-objects \\\n+\tpackname=$(git pack-objects \\\n \t\t--window=0 \\\n \t\t--include-tag \\\n-\t\ttest-2 <obj-list)\n+\t\ttest-include <obj-list)\n '\n \n test_expect_success 'unpack objects' '\n \trm -rf clone.git &&\n \tgit init clone.git &&\n-\tgit -C clone.git unpack-objects <test-2-${packname_1}.pack\n+\tgit -C clone.git unpack-objects <test-include-${packname}.pack\n '\n \n test_expect_success 'check unpacked result (have commit, have tag)' '\n-- \n2.10.0.rc2.154.gb4a4b8b\n\n"},{"id":"301148","messageId":"20160905215226.m6vv2tk5pe2qt4ui@sigill.intra.peff.net","threadId":"44015","inReplyTo":"20160905215141.b6unqtjqko7775is@sigill.intra.peff.net","subject":"[PATCH 5/5] pack-objects: walk tag chains for --include-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-05T21:52:26Z","receivedAt":"2016-09-05T21:52:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When pack-objects is given --include-tag, it peels each tag\nref down to a non-tag object, and if that non-tag object is\ngoing to be packed, we include the tag, too. But what\nhappens if we have a chain of tags (e.g., tag \"A\" points to\ntag \"B\", which points to commit \"C\")?\n\nWe'll peel down to \"C\" and realize that we want to include\ntag \"A\", but we do not ever consider tag \"B\", leading to a\nbroken pack (assuming \"B\" was not otherwise selected).\nInstead, we have to walk the whole chain, adding any tags we\nfind to the pack.\n\nInterestingly, it doesn't seem possible to trigger this\nproblem with \"git fetch\", but you can with \"git clone\n--single-branch\". The reason is that we generate the correct\npack when the client explicitly asks for \"A\" (because we do\na real reachability analysis there), and \"fetch\" is more\nwilling to do so. There are basically two cases:\n\n  1. If \"C\" is already a ref tip, then the client can deduce\n     that it needs \"A\" itself (via find_non_local_tags), and\n     will ask for it explicitly rather than relying on the\n     include-tag capability. Everything works.\n\n  2. If \"C\" is not already a ref tip, then we hope for\n     include-tag to send us the correct tag. But it doesn't;\n     it generates a broken pack. However, the next step is\n     to do a follow-up run of find_non_local_tags(),\n     followed by fetch_refs() to backfill any tags we\n     learned about.\n\n     In the normal case, fetch_refs() calls quickfetch(),\n     which does a connectivity check and sees we have no\n     new objects to fetch. We just write the refs.\n\n     But for the broken-pack case, the connectivity check\n     fails, and quickfetch will follow-up with the remote,\n     asking explicitly for each of the ref tips. This picks\n     up the missing object in a new pack.\n\nFor a regular \"git clone\", we are similarly OK, because we\nexplicitly request all of the tag refs, and get a correct\npack. But with \"--single-branch\", we kick in tag\nauto-following via \"include-tag\", but do _not_ do a\nfollow-up backfill. We just take whatever the server sent us\nvia include-tag and write out tag refs for any tag objects\nwe were sent. So prior to c6807a4 (clone: open a shortcut\nfor connectivity check, 2013-05-26), we actually claimed the\nclone was a success, but the result was silently\ncorrupted!  Since c6807a4, index-pack's connectivity\ncheck catches this case, and we correctly complain.\n\nThe included test directly checks that pack-objects does not\ngenerate a broken pack, but also confirms that \"clone\n--single-branch\" does not hit the bug.\n\nNote that tag chains introduce another interesting question:\nif we are packing the tag \"B\" but not the commit \"C\", should\n\"A\" be included?\n\nBoth before and after this patch, we do not include \"A\",\nbecause the initial peel_ref() check only knows about the\nbottom-most level, \"C\". To realize that \"B\" is involved at\nall, we would have to switch to an incremental peel, in\nwhich we examine each tagged object, asking if it is being\npacked (and including the outer tag if so).\n\nBut that runs contrary to the optimizations in peel_ref(),\nwhich avoid accessing the objects at all, in favor of using\nthe value we pull from packed-refs. It's OK to walk the\nwhole chain once we know we're going to include the tag (we\nhave to access it anyway, so the effort is proportional to\nthe pack we're generating). But for the initial selection,\nwe have to look at every ref. If we're only packing a few\nobjects, we'd still have to parse every single referenced\ntag object just to confirm that it isn't part of a tag\nchain.\n\nThis could be addressed if packed-refs stored the complete\ntag chain for each peeled ref (in most cases, this would be\nthe same cost as now, as each \"chain\" is only a single\nlink). But given the size of that project, it's out of scope\nfor this fix (and probably nobody cares enough anyway, as\nit's such an obscure situation). This commit limits itself\nto just avoiding the creation of a broken pack.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c | 31 +++++++++++++++++++++++++++++-\n t/t5305-include-tag.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 82 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 4a63398..0954375 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2123,6 +2123,35 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,\n #define ll_find_deltas(l, s, w, d, p)\tfind_deltas(l, &s, w, d, p)\n #endif\n \n+static void add_tag_chain(const struct object_id *oid)\n+{\n+\tstruct tag *tag;\n+\n+\t/*\n+\t * We catch duplicates already in add_object_entry(), but we'd\n+\t * prefer to do this extra check to avoid having to parse the\n+\t * tag at all if we already know that it's being packed (e.g., if\n+\t * it was included via bitmaps, we would not have parsed it\n+\t * previously).\n+\t */\n+\tif (packlist_find(&to_pack, oid->hash, NULL))\n+\t\treturn;\n+\n+\ttag = lookup_tag(oid->hash);\n+\twhile (1) {\n+\t\tif (!tag || parse_tag(tag) || !tag->tagged)\n+\t\t\tdie(\"unable to pack objects reachable from tag %s\",\n+\t\t\t    oid_to_hex(oid));\n+\n+\t\tadd_object_entry(tag->object.oid.hash, OBJ_TAG, NULL, 0);\n+\n+\t\tif (tag->tagged->type != OBJ_TAG)\n+\t\t\treturn;\n+\n+\t\ttag = (struct tag *)tag->tagged;\n+\t}\n+}\n+\n static int add_ref_tag(const char *path, const struct object_id *oid, int flag, void *cb_data)\n {\n \tstruct object_id peeled;\n@@ -2130,7 +2159,7 @@ static int add_ref_tag(const char *path, const struct object_id *oid, int flag,\n \tif (starts_with(path, \"refs/tags/\") && /* is a tag? */\n \t    !peel_ref(path, peeled.hash)    && /* peelable? */\n \t    packlist_find(&to_pack, peeled.hash, NULL))      /* object packed? */\n-\t\tadd_object_entry(oid->hash, OBJ_TAG, NULL, 0);\n+\t\tadd_tag_chain(oid);\n \treturn 0;\n }\n \ndiff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh\nindex e3c6c62..a5eca21 100755\n--- a/t/t5305-include-tag.sh\n+++ b/t/t5305-include-tag.sh\n@@ -63,4 +63,56 @@ test_expect_success 'check unpacked result (have commit, have tag)' '\n \ttest_cmp list.expect list.actual\n '\n \n+# A tag of a tag, where the \"inner\" tag is not otherwise\n+# reachable, and a full peel points to a commit reachable from HEAD.\n+test_expect_success 'create hidden inner tag' '\n+\ttest_commit commit &&\n+\tgit tag -m inner inner HEAD &&\n+\tgit tag -m outer outer inner &&\n+\tgit tag -d inner\n+'\n+\n+test_expect_success 'pack explicit outer tag' '\n+\tpackname=$(\n+\t\t{\n+\t\t\techo HEAD &&\n+\t\t\techo outer\n+\t\t} |\n+\t\tgit pack-objects --revs test-hidden-explicit\n+\t)\n+'\n+\n+test_expect_success 'unpack objects' '\n+\trm -rf clone.git &&\n+\tgit init clone.git &&\n+\tgit -C clone.git unpack-objects <test-hidden-explicit-${packname}.pack\n+'\n+\n+test_expect_success 'check unpacked result (have all objects)' '\n+\tgit -C clone.git rev-list --objects $(git rev-parse outer HEAD)\n+'\n+\n+test_expect_success 'pack implied outer tag' '\n+\tpackname=$(\n+\t\techo HEAD |\n+\t\tgit pack-objects --revs --include-tag test-hidden-implied\n+\t)\n+'\n+\n+test_expect_success 'unpack objects' '\n+\trm -rf clone.git &&\n+\tgit init clone.git &&\n+\tgit -C clone.git unpack-objects <test-hidden-implied-${packname}.pack\n+'\n+\n+test_expect_success 'check unpacked result (have all objects)' '\n+\tgit -C clone.git rev-list --objects $(git rev-parse outer HEAD)\n+'\n+\n+test_expect_success 'single-branch clone can transfer tag' '\n+\trm -rf clone.git &&\n+\tgit clone --no-local --single-branch -b master . clone.git &&\n+\tgit -C clone.git fsck\n+'\n+\n test_done\n-- \n2.10.0.rc2.154.gb4a4b8b\n"},{"id":"301149","messageId":"20160905215411.zu5urzi26gpbsj6j@sigill.intra.peff.net","threadId":"44015","inReplyTo":"20160905215141.b6unqtjqko7775is@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] handle tag recursion in pack-objects --include-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-05T21:54:11Z","receivedAt":"2016-09-05T21:54:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2016 at 05:51:41PM -0400, Jeff King wrote:\n\n> If you have a tag of a tag of a commit, and the middle tag isn't\n> otherwise reachable, then \"pack-objects --include-tag\" will not include\n> the middle tag, and certain invocations of \"clone\" will fail. See the\n> final patch below for the gory details.\n> \n> The first five patches are just test cleanup and modernization in\n> preparation (though note that the 3rd one actually fixes a minor bug in\n> the test script).\n> \n>   [1/5]: t5305: move cleanup into test block\n>   [2/5]: t5305: drop \"dry-run\" of unpack-objects\n>   [3/5]: t5305: use \"git -C\"\n>   [4/5]: t5305: simplify packname handling\n>   [5/5]: pack-objects: walk tag chains for --include-tag\n\nBTW, this is not a new regression; the problem dates back to the origin\nof the \"include-tag\" feature. So there is no urgency to the fix, as it\nhas been with us for almost a decade. I built it on master, but it\nshould apply cleanly to maint.\n\n-Peff\n"},{"id":"301151","messageId":"20160905215939.hriu6ev3m332qhp6@sigill.intra.peff.net","threadId":"44015","inReplyTo":"20160905215226.m6vv2tk5pe2qt4ui@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] pack-objects: walk tag chains for --include-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-05T21:59:39Z","receivedAt":"2016-09-05T21:59:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2016 at 05:52:26PM -0400, Jeff King wrote:\n\n> When pack-objects is given --include-tag, it peels each tag\n> ref down to a non-tag object, and if that non-tag object is\n> going to be packed, we include the tag, too. But what\n> happens if we have a chain of tags (e.g., tag \"A\" points to\n> tag \"B\", which points to commit \"C\")?\n> \n> We'll peel down to \"C\" and realize that we want to include\n> tag \"A\", but we do not ever consider tag \"B\", leading to a\n> broken pack (assuming \"B\" was not otherwise selected).\n> Instead, we have to walk the whole chain, adding any tags we\n> find to the pack.\n\nSo technically one might argue that this pack isn't \"broken\", in that it\n_is_ a valid pack. It's just that it doesn't contain what the user was\nasking for.\n\nAs explained further in the commit message, \"fetch\" is robust to this,\nbecause it does a real connectivity check and follow-on fetch before\nwriting anything it thinks it got via include-tag. So perhaps one could\nargue that pack-objects is correct; include-tag is best-effort, and it\nis the client's job to make sure it has everything it needs. And that\nwould mean the bug is in git-clone, which should be doing the\nconnectivity check and follow-on fetch.\n\nI dunno. This seems like the most elegant place to fix it, though it\ndoes mean that pack-objects will go to some slight extra work when\nauto-packing a tag (it has to parse the tag to find out whether it's a\nchain). I'm doubt it matters much in practice.\n\n-Peff\n"},{"id":"301313","messageId":"xmqqzinjlf47.fsf@gitster.mtv.corp.google.com","threadId":"44015","inReplyTo":"20160905215939.hriu6ev3m332qhp6@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] pack-objects: walk tag chains for --include-tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T18:49:28Z","receivedAt":"2016-09-07T18:49:36Z","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> As explained further in the commit message, \"fetch\" is robust to this,\n> because it does a real connectivity check and follow-on fetch before\n> writing anything it thinks it got via include-tag. So perhaps one could\n> argue that pack-objects is correct; include-tag is best-effort, and it\n> is the client's job to make sure it has everything it needs. And that\n> would mean the bug is in git-clone, which should be doing the\n> connectivity check and follow-on fetch.\n\nI think that is probably a more technically correct interpretation\nof the history.\n\nI think upgrading \"best-effort\" to \"guarantee\" like you did is a\nright approach nevertheless.  I think the \"best-effort\" we initially\ndid was merely us being lazy.\n"},{"id":"301318","messageId":"20160907194542.rubdx37mwnho6lrr@sigill.intra.peff.net","threadId":"44015","inReplyTo":"xmqqzinjlf47.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] pack-objects: walk tag chains for --include-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T19:45:42Z","receivedAt":"2016-09-07T19:46:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2016 at 11:49:28AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > As explained further in the commit message, \"fetch\" is robust to this,\n> > because it does a real connectivity check and follow-on fetch before\n> > writing anything it thinks it got via include-tag. So perhaps one could\n> > argue that pack-objects is correct; include-tag is best-effort, and it\n> > is the client's job to make sure it has everything it needs. And that\n> > would mean the bug is in git-clone, which should be doing the\n> > connectivity check and follow-on fetch.\n> \n> I think that is probably a more technically correct interpretation\n> of the history.\n> \n> I think upgrading \"best-effort\" to \"guarantee\" like you did is a\n> right approach nevertheless.  I think the \"best-effort\" we initially\n> did was merely us being lazy.\n\nYeah, after sleeping on it, the conclusion I came to was that it does\nnot _hurt_ to have include-tag be a bit more careful.\n\nI also wondered about the corner case I noted in the commit message.  If\nyou have a tag chain of A->B->C, and you already have \"C\" (a commit),\nbut are fetching \"B\" (a tag), then include-tag does not notice \"A\".\n\nThat's OK for git-fetch. It will collect \"A\" during its backfill phase\n(not because of \"B\" at all, but because it knows that \"A\" eventually\npeels to \"C\", which it already has). \"git-clone\" does not have a\nbackfill, of course. But neither can it \"already have\" a commit. So\neither we get \"C\" as part of the clone (in which case include-tag will\ninclude \"A\"), or it does not (in which case we cannot be getting \"B\"\neither, because \"C\" is reachable from it).\n\nAnd of course that's only when single-branch is in use. Normally\ngit-clone just grabs all the tags blindly. :)\n\nSo I think everything Just Works after my patch, though we do still rely\non fetch backfill to pick up some obscure cases.\n\n-Peff\n"}]}