{"thread":{"id":"49510","subject":"[PATCH] unpack-trees: allow missing CE_SKIP_WORKTREE objs","startedAt":"2018-10-08T21:48:23Z","lastAt":"2018-10-10T01:19:24Z","messageCount":7,"participants":["Jonathan Tan","Junio C Hamano","Ben Peart"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"359854","messageId":"20181008214816.42856-1-jonathantanmy@google.com","threadId":"49510","inReplyTo":null,"subject":"[PATCH] unpack-trees: allow missing CE_SKIP_WORKTREE objs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-08T21:48:16Z","receivedAt":"2018-10-08T21:48:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Whenever a sparse checkout occurs, the existence of all blobs in the\nindex is verified, whether or not they are included or excluded by the\n.git/info/sparse-checkout specification. This degrades performance,\nsignificantly in the case of a partial clone, because a lazy fetch\noccurs whenever the existence of a missing blob is checked.\n\nAt the point of invoking cache_tree_update() in unpack_trees(),\nCE_SKIP_WORKTREE is already set on all excluded blobs\n(mark_new_skip_worktree() is called twice to set CE_NEW_SKIP_WORKTREE,\nthen apply_sparse_checkout() is called which copies over\nCE_NEW_SKIP_WORKTREE to CE_SKIP_WORKTREE). cache_tree_update() can use\nthis information to know which blobs are excluded, and thus skip the\nchecking of these.\n\nBecause cache_tree_update() is used from multiple places, this new\nbehavior is guarded by a new flag WRITE_TREE_SKIP_WORKTREE_MISSING_OK.\nImplement this new flag, and teach unpack_trees() to invoke\ncache_tree_update() with this new flag.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n cache-tree.c                     |  6 +++++-\n cache-tree.h                     |  4 ++++\n t/t1090-sparse-checkout-scope.sh | 33 ++++++++++++++++++++++++++++++++\n unpack-trees.c                   |  1 +\n 4 files changed, 43 insertions(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 5ce51468f0..340caf2d34 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -246,6 +246,8 @@ static int update_one(struct cache_tree *it,\n \tint missing_ok = flags & WRITE_TREE_MISSING_OK;\n \tint dryrun = flags & WRITE_TREE_DRY_RUN;\n \tint repair = flags & WRITE_TREE_REPAIR;\n+\tint skip_worktree_missing_ok =\n+\t\tflags & WRITE_TREE_SKIP_WORKTREE_MISSING_OK;\n \tint to_invalidate = 0;\n \tint i;\n \n@@ -356,7 +358,9 @@ static int update_one(struct cache_tree *it,\n \t\t}\n \n \t\tif (is_null_oid(oid) ||\n-\t\t    (mode != S_IFGITLINK && !missing_ok && !has_object_file(oid))) {\n+\t\t    (mode != S_IFGITLINK && !missing_ok &&\n+\t\t     !(skip_worktree_missing_ok && ce_skip_worktree(ce)) &&\n+\t\t     !has_object_file(oid))) {\n \t\t\tstrbuf_release(&buffer);\n \t\t\tif (expected_missing)\n \t\t\t\treturn -1;\ndiff --git a/cache-tree.h b/cache-tree.h\nindex 0ab6784ffe..655d487619 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -40,6 +40,10 @@ void cache_tree_verify(struct index_state *);\n #define WRITE_TREE_DRY_RUN 4\n #define WRITE_TREE_SILENT 8\n #define WRITE_TREE_REPAIR 16\n+/*\n+ * Do not check for the presence of cache entry objects with CE_SKIP_WORKTREE.\n+ */\n+#define WRITE_TREE_SKIP_WORKTREE_MISSING_OK 32\n \n /* error return codes */\n #define WRITE_TREE_UNREADABLE_INDEX (-1)\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 25d7c700f6..090b7fc3d3 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -63,4 +63,37 @@ test_expect_success 'return to full checkout of master' '\n \ttest \"$(cat b)\" = \"modified\"\n '\n \n+test_expect_success 'in partial clone, sparse checkout only fetches needed blobs' '\n+\ttest_create_repo server &&\n+\tgit clone \"file://$(pwd)/server\" client &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\techo a >server/a &&\n+\techo bb >server/b &&\n+\tmkdir server/c &&\n+\techo ccc >server/c/c &&\n+\tgit -C server add a b c/c &&\n+\tgit -C server commit -m message &&\n+\n+\ttest_config -C client core.sparsecheckout 1 &&\n+\ttest_config -C client extensions.partialclone origin &&\n+\techo \"!/*\" >client/.git/info/sparse-checkout &&\n+\techo \"/a\" >>client/.git/info/sparse-checkout &&\n+\tgit -C client fetch --filter=blob:none origin &&\n+\tgit -C client checkout FETCH_HEAD &&\n+\n+\tgit -C client rev-list HEAD \\\n+\t\t--quiet --objects --missing=print >unsorted_actual &&\n+\t(\n+\t\tprintf \"?\" &&\n+\t\tgit hash-object server/b &&\n+\t\tprintf \"?\" &&\n+\t\tgit hash-object server/c/c\n+\t) >unsorted_expect &&\n+\tsort unsorted_actual >actual &&\n+\tsort unsorted_expect >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 51bfac6aa0..39e0e7a6c7 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1635,6 +1635,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t\t\to->result.cache_tree = cache_tree();\n \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n \t\t\t\tcache_tree_update(&o->result,\n+\t\t\t\t\t\t  WRITE_TREE_SKIP_WORKTREE_MISSING_OK |\n \t\t\t\t\t\t  WRITE_TREE_SILENT |\n \t\t\t\t\t\t  WRITE_TREE_REPAIR);\n \t\t}\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"359911","messageId":"xmqqtvlvpj0m.fsf@gitster-ct.c.googlers.com","threadId":"49510","inReplyTo":"20181008214816.42856-1-jonathantanmy@google.com","subject":"Re: [PATCH] unpack-trees: allow missing CE_SKIP_WORKTREE objs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-09T09:27:21Z","receivedAt":"2018-10-09T09:27:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Because cache_tree_update() is used from multiple places, this new\n> behavior is guarded by a new flag WRITE_TREE_SKIP_WORKTREE_MISSING_OK.\n\nThe name of the new flag is mouthful, but we know we do not need to\nmaterialize these blobs (exactly because the skip-worktree bit is\nset, so we do not need to know what's in these blobs) and it is OK\nfor these to be missing (to put it differently, we do not care if\nthey exist or not---hence we short-circuit the otherwise required\ncall to has_object_file()), iow, the name of the mode is \"A missing\nobject with skip-worktree bit set is OK\", which makes sense to me.\n\n>  \t\tif (is_null_oid(oid) ||\n> -\t\t    (mode != S_IFGITLINK && !missing_ok && !has_object_file(oid))) {\n> +\t\t    (mode != S_IFGITLINK && !missing_ok &&\n> +\t\t     !(skip_worktree_missing_ok && ce_skip_worktree(ce)) &&\n> +\t\t     !has_object_file(oid))) {\n\nFor a non-gitlink entry, if the caller does not say \"any missing\nobject is OK\", we normally check has_object_file().  But now\nhas_object_file() call happens only when ...\n\nHmph, isn't this new condition somewhat wrong?  We do not want to\nskip it for entries without skip-worktree bit set.  We only do not\ncare if we are operating in skip-worktree-missing-ok mode and the\nbit is set on ce.  IOW:\n\n\tif ((skip_worktree_missing_ok && ce_skip_worktree(ce)) ||\n            has_object_file(oid))\n\t\t/* then we are happy */\n\nbut the whole thing above tries to catch problematic case, so I'd\nneed to negate that, i.e.\n\n\tif ( ... &&\n\t    !((skip_worktree_missing_ok && ce_skip_worktree(ce)) ||\n             has_object_file(oid)))\n\t\t/* we are in trouble */\n\nand pushing the negation down, we get\n\n\tif ( ... &&\n\t    (!(skip_worktree_missing_ok && ce_skip_worktree(ce)) &&\n             !has_object_file(oid)))\n\t\t/* we are in trouble */\n\nOK.  The logic in the patch is correct.  I however feel that it is a\nbit too dense to make sense of.\n\nThanks, will queue.\n"},{"id":"359912","messageId":"xmqqpnwjpivb.fsf@gitster-ct.c.googlers.com","threadId":"49510","inReplyTo":"20181008214816.42856-1-jonathantanmy@google.com","subject":"Re: [PATCH] unpack-trees: allow missing CE_SKIP_WORKTREE objs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-09T09:30:32Z","receivedAt":"2018-10-09T09:30:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> @@ -1635,6 +1635,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>  \t\t\t\to->result.cache_tree = cache_tree();\n>  \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n>  \t\t\t\tcache_tree_update(&o->result,\n> +\t\t\t\t\t\t  WRITE_TREE_SKIP_WORKTREE_MISSING_OK |\n>  \t\t\t\t\t\t  WRITE_TREE_SILENT |\n>  \t\t\t\t\t\t  WRITE_TREE_REPAIR);\n>  \t\t}\n\nHmmmm.  Should this be passing the bit unconditionally?  Shouldn't\nit be set only when we are doing lazy clone?  A non-lazy clone that\nmerely uses sparse checkout has nowhere else to turn to if it loses\na blob object that currently is not necessary to complete a checkout,\nunlike a repository with promisor.\n"},{"id":"359925","messageId":"7ac4ea1d-596d-cffd-337f-bf1879d982e1@gmail.com","threadId":"49510","inReplyTo":"20181008214816.42856-1-jonathantanmy@google.com","subject":"Re: [PATCH] unpack-trees: allow missing CE_SKIP_WORKTREE objs","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-10-09T14:48:34Z","receivedAt":"2018-10-09T14:48:39Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 10/8/2018 5:48 PM, Jonathan Tan wrote:\n> Whenever a sparse checkout occurs, the existence of all blobs in the\n> index is verified, whether or not they are included or excluded by the\n> .git/info/sparse-checkout specification. This degrades performance,\n> significantly in the case of a partial clone, because a lazy fetch\n> occurs whenever the existence of a missing blob is checked.\n> \n> At the point of invoking cache_tree_update() in unpack_trees(),\n> CE_SKIP_WORKTREE is already set on all excluded blobs\n> (mark_new_skip_worktree() is called twice to set CE_NEW_SKIP_WORKTREE,\n> then apply_sparse_checkout() is called which copies over\n> CE_NEW_SKIP_WORKTREE to CE_SKIP_WORKTREE). cache_tree_update() can use\n> this information to know which blobs are excluded, and thus skip the\n> checking of these.\n> \n> Because cache_tree_update() is used from multiple places, this new\n> behavior is guarded by a new flag WRITE_TREE_SKIP_WORKTREE_MISSING_OK.\n> Implement this new flag, and teach unpack_trees() to invoke\n> cache_tree_update() with this new flag.\n> \n\nI wonder if preventing the download of all missing blobs should be \nlimited to only the checkout command.  When you looked at the other \nplaces that call cache_tree_update(), does it make sense that they \ntrigger the download of all the missing blobs?  For example, I suspect \nyou would not want them all downloaded just to do a merge-recursive.\n\nIn full disclosure, we implemented this a couple of years ago [1] for \nGVFS and opted to do the logic a little differently.  We found that we \ndidn't want to trigger the download of all missing blobs in \ncache_tree_update() for _any_ command that was executing in a partially \ncloned repo.  This is safe because if any individual blob is actually \nneeded, it will get downloaded \"on demand\" already.\n\n[1] https://github.com/Microsoft/git/commit/ec865500d98\n\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>   cache-tree.c                     |  6 +++++-\n>   cache-tree.h                     |  4 ++++\n>   t/t1090-sparse-checkout-scope.sh | 33 ++++++++++++++++++++++++++++++++\n>   unpack-trees.c                   |  1 +\n>   4 files changed, 43 insertions(+), 1 deletion(-)\n> \n> diff --git a/cache-tree.c b/cache-tree.c\n> index 5ce51468f0..340caf2d34 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -246,6 +246,8 @@ static int update_one(struct cache_tree *it,\n>   \tint missing_ok = flags & WRITE_TREE_MISSING_OK;\n>   \tint dryrun = flags & WRITE_TREE_DRY_RUN;\n>   \tint repair = flags & WRITE_TREE_REPAIR;\n> +\tint skip_worktree_missing_ok =\n> +\t\tflags & WRITE_TREE_SKIP_WORKTREE_MISSING_OK;\n>   \tint to_invalidate = 0;\n>   \tint i;\n>   \n> @@ -356,7 +358,9 @@ static int update_one(struct cache_tree *it,\n>   \t\t}\n>   \n>   \t\tif (is_null_oid(oid) ||\n> -\t\t    (mode != S_IFGITLINK && !missing_ok && !has_object_file(oid))) {\n> +\t\t    (mode != S_IFGITLINK && !missing_ok &&\n> +\t\t     !(skip_worktree_missing_ok && ce_skip_worktree(ce)) &&\n> +\t\t     !has_object_file(oid))) {\n>   \t\t\tstrbuf_release(&buffer);\n>   \t\t\tif (expected_missing)\n>   \t\t\t\treturn -1;\n> diff --git a/cache-tree.h b/cache-tree.h\n> index 0ab6784ffe..655d487619 100644\n> --- a/cache-tree.h\n> +++ b/cache-tree.h\n> @@ -40,6 +40,10 @@ void cache_tree_verify(struct index_state *);\n>   #define WRITE_TREE_DRY_RUN 4\n>   #define WRITE_TREE_SILENT 8\n>   #define WRITE_TREE_REPAIR 16\n> +/*\n> + * Do not check for the presence of cache entry objects with CE_SKIP_WORKTREE.\n> + */\n> +#define WRITE_TREE_SKIP_WORKTREE_MISSING_OK 32\n>   \n>   /* error return codes */\n>   #define WRITE_TREE_UNREADABLE_INDEX (-1)\n> diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\n> index 25d7c700f6..090b7fc3d3 100755\n> --- a/t/t1090-sparse-checkout-scope.sh\n> +++ b/t/t1090-sparse-checkout-scope.sh\n> @@ -63,4 +63,37 @@ test_expect_success 'return to full checkout of master' '\n>   \ttest \"$(cat b)\" = \"modified\"\n>   '\n>   \n> +test_expect_success 'in partial clone, sparse checkout only fetches needed blobs' '\n> +\ttest_create_repo server &&\n> +\tgit clone \"file://$(pwd)/server\" client &&\n> +\n> +\ttest_config -C server uploadpack.allowfilter 1 &&\n> +\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n> +\techo a >server/a &&\n> +\techo bb >server/b &&\n> +\tmkdir server/c &&\n> +\techo ccc >server/c/c &&\n> +\tgit -C server add a b c/c &&\n> +\tgit -C server commit -m message &&\n> +\n> +\ttest_config -C client core.sparsecheckout 1 &&\n> +\ttest_config -C client extensions.partialclone origin &&\n> +\techo \"!/*\" >client/.git/info/sparse-checkout &&\n> +\techo \"/a\" >>client/.git/info/sparse-checkout &&\n> +\tgit -C client fetch --filter=blob:none origin &&\n> +\tgit -C client checkout FETCH_HEAD &&\n> +\n> +\tgit -C client rev-list HEAD \\\n> +\t\t--quiet --objects --missing=print >unsorted_actual &&\n> +\t(\n> +\t\tprintf \"?\" &&\n> +\t\tgit hash-object server/b &&\n> +\t\tprintf \"?\" &&\n> +\t\tgit hash-object server/c/c\n> +\t) >unsorted_expect &&\n> +\tsort unsorted_actual >actual &&\n> +\tsort unsorted_expect >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>   test_done\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 51bfac6aa0..39e0e7a6c7 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1635,6 +1635,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>   \t\t\t\to->result.cache_tree = cache_tree();\n>   \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n>   \t\t\t\tcache_tree_update(&o->result,\n> +\t\t\t\t\t\t  WRITE_TREE_SKIP_WORKTREE_MISSING_OK |\n>   \t\t\t\t\t\t  WRITE_TREE_SILENT |\n>   \t\t\t\t\t\t  WRITE_TREE_REPAIR);\n>   \t\t}\n> \n"},{"id":"359926","messageId":"56cc6e57-d1e3-1182-4290-c8b6fa796534@gmail.com","threadId":"49510","inReplyTo":"xmqqpnwjpivb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] unpack-trees: allow missing CE_SKIP_WORKTREE objs","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-10-09T14:49:24Z","receivedAt":"2018-10-09T14:49:29Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 10/9/2018 5:30 AM, Junio C Hamano wrote:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n>> @@ -1635,6 +1635,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>>   \t\t\t\to->result.cache_tree = cache_tree();\n>>   \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n>>   \t\t\t\tcache_tree_update(&o->result,\n>> +\t\t\t\t\t\t  WRITE_TREE_SKIP_WORKTREE_MISSING_OK |\n>>   \t\t\t\t\t\t  WRITE_TREE_SILENT |\n>>   \t\t\t\t\t\t  WRITE_TREE_REPAIR);\n>>   \t\t}\n> \n> Hmmmm.  Should this be passing the bit unconditionally?  Shouldn't\n> it be set only when we are doing lazy clone?  A non-lazy clone that\n> merely uses sparse checkout has nowhere else to turn to if it loses\n> a blob object that currently is not necessary to complete a checkout,\n> unlike a repository with promisor.\n> \n\nI agree.  I believe this logic should only be triggered when running in \na partial clone repo. Otherwise, we're potentially changing the behavior \nof 'normal' repos unnecessarily.\n\n"},{"id":"359936","messageId":"20181009184037.246871-1-jonathantanmy@google.com","threadId":"49510","inReplyTo":"20181008214816.42856-1-jonathantanmy@google.com","subject":"[PATCH v2] cache-tree: skip some blob checks in partial clone","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-09T18:40:37Z","receivedAt":"2018-10-09T18:40:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a partial clone, whenever a sparse checkout occurs, the existence of\nall blobs in the index is verified, whether they are included or\nexcluded by the .git/info/sparse-checkout specification. This\nsignificantly degrades performance because a lazy fetch occurs whenever\nthe existence of a missing blob is checked.\n\nThis is because cache_tree_update() checks the existence of all objects\nin the index, whether or not CE_SKIP_WORKTREE is set on them. Teach\ncache_tree_update() to skip checking CE_SKIP_WORKTREE objects when the\nrepository is a partial clone. This improves performance for sparse\ncheckout and also other operations that use cache_tree_update().\n\nInstead of completely removing the check, an argument could be made that\nthe check should instead be replaced by a check that the blob is\npromised, but for performance reasons, I decided not to do this.\nIf the user needs to verify the repository, it can be done using fsck\n(which will notify if a tree points to a missing and non-promised blob,\nwhether the blob is included or excluded by the sparse-checkout\nspecification).\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nChanges from v1:\n\nAfter feedback, I restricted this to partial clone. Once restricted, I\nagree with Ben that this can be done for all users of\ncache_tree_update(), not just unpack-trees, so I have removed the\nability to control the behavior using a flag.\n\nI also took the opportunity to simplify the missing check by using a\nvariable.\n---\n cache-tree.c                     |  6 +++++-\n t/t1090-sparse-checkout-scope.sh | 33 ++++++++++++++++++++++++++++++++\n 2 files changed, 38 insertions(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 5ce51468f0..f210481f9b 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -326,6 +326,7 @@ static int update_one(struct cache_tree *it,\n \t\tunsigned mode;\n \t\tint expected_missing = 0;\n \t\tint contains_ita = 0;\n+\t\tint ce_missing_ok;\n \n \t\tpath = ce->name;\n \t\tpathlen = ce_namelen(ce);\n@@ -355,8 +356,11 @@ static int update_one(struct cache_tree *it,\n \t\t\ti++;\n \t\t}\n \n+\t\tce_missing_ok = mode == S_IFGITLINK || missing_ok ||\n+\t\t\t(repository_format_partial_clone &&\n+\t\t\t ce_skip_worktree(ce));\n \t\tif (is_null_oid(oid) ||\n-\t\t    (mode != S_IFGITLINK && !missing_ok && !has_object_file(oid))) {\n+\t\t    (!ce_missing_ok && !has_object_file(oid))) {\n \t\t\tstrbuf_release(&buffer);\n \t\t\tif (expected_missing)\n \t\t\t\treturn -1;\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 25d7c700f6..090b7fc3d3 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -63,4 +63,37 @@ test_expect_success 'return to full checkout of master' '\n \ttest \"$(cat b)\" = \"modified\"\n '\n \n+test_expect_success 'in partial clone, sparse checkout only fetches needed blobs' '\n+\ttest_create_repo server &&\n+\tgit clone \"file://$(pwd)/server\" client &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\techo a >server/a &&\n+\techo bb >server/b &&\n+\tmkdir server/c &&\n+\techo ccc >server/c/c &&\n+\tgit -C server add a b c/c &&\n+\tgit -C server commit -m message &&\n+\n+\ttest_config -C client core.sparsecheckout 1 &&\n+\ttest_config -C client extensions.partialclone origin &&\n+\techo \"!/*\" >client/.git/info/sparse-checkout &&\n+\techo \"/a\" >>client/.git/info/sparse-checkout &&\n+\tgit -C client fetch --filter=blob:none origin &&\n+\tgit -C client checkout FETCH_HEAD &&\n+\n+\tgit -C client rev-list HEAD \\\n+\t\t--quiet --objects --missing=print >unsorted_actual &&\n+\t(\n+\t\tprintf \"?\" &&\n+\t\tgit hash-object server/b &&\n+\t\tprintf \"?\" &&\n+\t\tgit hash-object server/c/c\n+\t) >unsorted_expect &&\n+\tsort unsorted_actual >actual &&\n+\tsort unsorted_expect >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"359972","messageId":"xmqqh8huoay1.fsf@gitster-ct.c.googlers.com","threadId":"49510","inReplyTo":"20181009184037.246871-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] cache-tree: skip some blob checks in partial clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-10T01:19:18Z","receivedAt":"2018-10-10T01:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> After feedback, I restricted this to partial clone. Once restricted, I\n> agree with Ben that this can be done for all users of\n> cache_tree_update(), not just unpack-trees, so I have removed the\n> ability to control the behavior using a flag.\n\nMakes sense.  Great.\n\n> I also took the opportunity to simplify the missing check by using a\n> variable.\n>  \n> +\t\tce_missing_ok = mode == S_IFGITLINK || missing_ok ||\n> +\t\t\t(repository_format_partial_clone &&\n> +\t\t\t ce_skip_worktree(ce));\n>  \t\tif (is_null_oid(oid) ||\n> -\t\t    (mode != S_IFGITLINK && !missing_ok && !has_object_file(oid))) {\n> +\t\t    (!ce_missing_ok && !has_object_file(oid))) {\n\nOK.  \"An attempt to check out null object is bad, and otherwise,\nunless we determined that it is OK to lack the object recorded in\nce, it is bad too.  By the way, the way we determine if it is OK to\nbe missing the object is given above\".  Easier to read than the\noriginal.\n"}]}