From 1962d9fbe3feeb888053d0c648e39b82870c4ef3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 5 Sep 2016 17:52:10 -0400 Subject: [PATCH 1/5] t5305: move cleanup into test block We usually try to avoid doing any significant actions outside of test blocks. Although "rm -rf" is unlikely to either fail or to generate output, moving these to the point of use makes it more clear that they are part of the overall setup of "clone.git". Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5305-include-tag.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh index f314ad5079..60184048c5 100755 --- a/t/t5305-include-tag.sh +++ b/t/t5305-include-tag.sh @@ -25,7 +25,6 @@ test_expect_success setup ' } >obj-list ' -rm -rf clone.git test_expect_success 'pack without --include-tag' ' packname_1=$(git pack-objects \ --window=0 \ @@ -33,6 +32,7 @@ test_expect_success 'pack without --include-tag' ' ' test_expect_success 'unpack objects' ' + rm -rf clone.git && ( GIT_DIR=clone.git && export GIT_DIR && @@ -51,7 +51,6 @@ test_expect_success 'check unpacked result (have commit, no tag)' ' test_cmp list.expect list.actual ' -rm -rf clone.git test_expect_success 'pack with --include-tag' ' packname_1=$(git pack-objects \ --window=0 \ @@ -60,6 +59,7 @@ test_expect_success 'pack with --include-tag' ' ' test_expect_success 'unpack objects' ' + rm -rf clone.git && ( GIT_DIR=clone.git && export GIT_DIR && From 2076353f473aa3cbcbd2623f1765432fa275acec Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 5 Sep 2016 17:52:14 -0400 Subject: [PATCH 2/5] t5305: drop "dry-run" of unpack-objects For each test we do a dry-run of unpack-objects, followed by a real run, followed by confirming that it contained the objects we expected. The dry-run is telling us nothing, as any errors it encounters would be found in the real run. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5305-include-tag.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh index 60184048c5..787fc831e3 100755 --- a/t/t5305-include-tag.sh +++ b/t/t5305-include-tag.sh @@ -37,7 +37,6 @@ test_expect_success 'unpack objects' ' GIT_DIR=clone.git && export GIT_DIR && git init && - git unpack-objects -n Date: Mon, 5 Sep 2016 17:52:17 -0400 Subject: [PATCH 3/5] t5305: use "git -C" This test unpacks objects into a separate repository, and accesses it by setting GIT_DIR in a subshell. We can do the same thing these days by using "git init " and "git -C". In most cases this is shorter, though when there are multiple commands, we may end up repeating the "-C". However, this repetition can actually be a good thing. This patch also fixes a bug introduced by 512477b (tests: use "env" to run commands with temporary env-var settings, 2014-03-18). That commit essentially converted: (GIT_DIR=...; export GIT_DIR cmd1 && cmd2) into: (GIT_DIR=... cmd1 && cmd2) which obviously loses the GIT_DIR setting for cmd2 (we never noticed the bug because it simply runs "cmd2" in the parent repo, which means we were simply failing to test anything interesting). By using "git -C" rather than a subshell, it becomes quite obvious where each command is supposed to be running. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5305-include-tag.sh | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh index 787fc831e3..089a3e9e8c 100755 --- a/t/t5305-include-tag.sh +++ b/t/t5305-include-tag.sh @@ -33,20 +33,14 @@ test_expect_success 'pack without --include-tag' ' test_expect_success 'unpack objects' ' rm -rf clone.git && - ( - GIT_DIR=clone.git && - export GIT_DIR && - git init && - git unpack-objects list.expect && - ( - test_must_fail env GIT_DIR=clone.git git cat-file -e $tag && - git rev-list --objects $commit - ) >list.actual && + test_must_fail git -C clone.git cat-file -e $tag && + git -C clone.git rev-list --objects $commit >list.actual && test_cmp list.expect list.actual ' @@ -59,21 +53,13 @@ test_expect_success 'pack with --include-tag' ' test_expect_success 'unpack objects' ' rm -rf clone.git && - ( - GIT_DIR=clone.git && - export GIT_DIR && - git init && - git unpack-objects list.expect && - ( - GIT_DIR=clone.git && - export GIT_DIR && - git rev-list --objects $tag - ) >list.actual && + git -C clone.git rev-list --objects $tag >list.actual && test_cmp list.expect list.actual ' From ab5178356cc47ea9f1725993d45941d7c41fa471 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 5 Sep 2016 17:52:22 -0400 Subject: [PATCH 4/5] t5305: simplify packname handling We generate a series of packfiles test-1-$pack, test-2-$pack, with different properties and then examine them. However we always store the packname generated by pack-objects in the variable packname_1. This probably was meant to be packname_2 in the second test, but it turns out that it doesn't matter: once we are done with the first pack, we can just keep using the same $packname variable. So let's drop the confusing "_1" parameter. At the same time, let's give test-1 and test-2 more descriptive names, which can help keep them straight (note that we _could_ likewise overwrite the packfiles in each test, but by using separate filenames, we are sure that test 2 does not accidentally use the packfile from test 1). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5305-include-tag.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh index 089a3e9e8c..e3c6c620cc 100755 --- a/t/t5305-include-tag.sh +++ b/t/t5305-include-tag.sh @@ -26,15 +26,15 @@ test_expect_success setup ' ' test_expect_success 'pack without --include-tag' ' - packname_1=$(git pack-objects \ + packname=$(git pack-objects \ --window=0 \ - test-1 Date: Mon, 5 Sep 2016 17:52:26 -0400 Subject: [PATCH 5/5] pack-objects: walk tag chains for --include-tag When pack-objects is given --include-tag, it peels each tag ref down to a non-tag object, and if that non-tag object is going to be packed, we include the tag, too. But what happens if we have a chain of tags (e.g., tag "A" points to tag "B", which points to commit "C")? We'll peel down to "C" and realize that we want to include tag "A", but we do not ever consider tag "B", leading to a broken pack (assuming "B" was not otherwise selected). Instead, we have to walk the whole chain, adding any tags we find to the pack. Interestingly, it doesn't seem possible to trigger this problem with "git fetch", but you can with "git clone --single-branch". The reason is that we generate the correct pack when the client explicitly asks for "A" (because we do a real reachability analysis there), and "fetch" is more willing to do so. There are basically two cases: 1. If "C" is already a ref tip, then the client can deduce that it needs "A" itself (via find_non_local_tags), and will ask for it explicitly rather than relying on the include-tag capability. Everything works. 2. If "C" is not already a ref tip, then we hope for include-tag to send us the correct tag. But it doesn't; it generates a broken pack. However, the next step is to do a follow-up run of find_non_local_tags(), followed by fetch_refs() to backfill any tags we learned about. In the normal case, fetch_refs() calls quickfetch(), which does a connectivity check and sees we have no new objects to fetch. We just write the refs. But for the broken-pack case, the connectivity check fails, and quickfetch will follow-up with the remote, asking explicitly for each of the ref tips. This picks up the missing object in a new pack. For a regular "git clone", we are similarly OK, because we explicitly request all of the tag refs, and get a correct pack. But with "--single-branch", we kick in tag auto-following via "include-tag", but do _not_ do a follow-up backfill. We just take whatever the server sent us via include-tag and write out tag refs for any tag objects we were sent. So prior to c6807a4 (clone: open a shortcut for connectivity check, 2013-05-26), we actually claimed the clone was a success, but the result was silently corrupted! Since c6807a4, index-pack's connectivity check catches this case, and we correctly complain. The included test directly checks that pack-objects does not generate a broken pack, but also confirms that "clone --single-branch" does not hit the bug. Note that tag chains introduce another interesting question: if we are packing the tag "B" but not the commit "C", should "A" be included? Both before and after this patch, we do not include "A", because the initial peel_ref() check only knows about the bottom-most level, "C". To realize that "B" is involved at all, we would have to switch to an incremental peel, in which we examine each tagged object, asking if it is being packed (and including the outer tag if so). But that runs contrary to the optimizations in peel_ref(), which avoid accessing the objects at all, in favor of using the value we pull from packed-refs. It's OK to walk the whole chain once we know we're going to include the tag (we have to access it anyway, so the effort is proportional to the pack we're generating). But for the initial selection, we have to look at every ref. If we're only packing a few objects, we'd still have to parse every single referenced tag object just to confirm that it isn't part of a tag chain. This could be addressed if packed-refs stored the complete tag chain for each peeled ref (in most cases, this would be the same cost as now, as each "chain" is only a single link). But given the size of that project, it's out of scope for this fix (and probably nobody cares enough anyway, as it's such an obscure situation). This commit limits itself to just avoiding the creation of a broken pack. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 31 ++++++++++++++++++++++++- t/t5305-include-tag.sh | 52 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index f854ca4256..ea5367e45d 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -2104,6 +2104,35 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size, #define ll_find_deltas(l, s, w, d, p) find_deltas(l, &s, w, d, p) #endif +static void add_tag_chain(const struct object_id *oid) +{ + struct tag *tag; + + /* + * We catch duplicates already in add_object_entry(), but we'd + * prefer to do this extra check to avoid having to parse the + * tag at all if we already know that it's being packed (e.g., if + * it was included via bitmaps, we would not have parsed it + * previously). + */ + if (packlist_find(&to_pack, oid->hash, NULL)) + return; + + tag = lookup_tag(oid->hash); + while (1) { + if (!tag || parse_tag(tag) || !tag->tagged) + die("unable to pack objects reachable from tag %s", + oid_to_hex(oid)); + + add_object_entry(tag->object.oid.hash, OBJ_TAG, NULL, 0); + + if (tag->tagged->type != OBJ_TAG) + return; + + tag = (struct tag *)tag->tagged; + } +} + static int add_ref_tag(const char *path, const struct object_id *oid, int flag, void *cb_data) { struct object_id peeled; @@ -2111,7 +2140,7 @@ static int add_ref_tag(const char *path, const struct object_id *oid, int flag, if (starts_with(path, "refs/tags/") && /* is a tag? */ !peel_ref(path, peeled.hash) && /* peelable? */ packlist_find(&to_pack, peeled.hash, NULL)) /* object packed? */ - add_object_entry(oid->hash, OBJ_TAG, NULL, 0); + add_tag_chain(oid); return 0; } diff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh index e3c6c620cc..a5eca210b8 100755 --- a/t/t5305-include-tag.sh +++ b/t/t5305-include-tag.sh @@ -63,4 +63,56 @@ test_expect_success 'check unpacked result (have commit, have tag)' ' test_cmp list.expect list.actual ' +# A tag of a tag, where the "inner" tag is not otherwise +# reachable, and a full peel points to a commit reachable from HEAD. +test_expect_success 'create hidden inner tag' ' + test_commit commit && + git tag -m inner inner HEAD && + git tag -m outer outer inner && + git tag -d inner +' + +test_expect_success 'pack explicit outer tag' ' + packname=$( + { + echo HEAD && + echo outer + } | + git pack-objects --revs test-hidden-explicit + ) +' + +test_expect_success 'unpack objects' ' + rm -rf clone.git && + git init clone.git && + git -C clone.git unpack-objects