Re: [PATCH 4/4] promisor-remote: teach lazy-fetch in any repo
From: Taylor Blau <hidden>
Date: 2021-06-04 21:26:05
On Tue, Jun 01, 2021 at 02:34:19PM -0700, Jonathan Tan wrote:
This is one step towards supporting partial clone submodules. Even after this patch, we will still lack partial clone submodules support, primarily because a lot of Git code that accesses submodule objects does so by adding their object stores as alternates, meaning that any lazy fetches that would occur in the submodule would be done based on the config of the superproject, not of the submodule. This also prevents testing of the functionality in this patch by user-facing commands. So for now, test this mechanism using a test helper.
OK. Everything you wrote seemed reasonable to me, but I did have a couple of questions on the test you added:
quoted hunk ↗ jump to hunk
diff --git a/t/helper/test-partial-clone.c b/t/helper/test-partial-clone.c new file mode 100644 index 0000000000..e7bc7eb21f --- /dev/null +++ b/t/helper/test-partial-clone.c@@ -0,0 +1,34 @@ +#include "cache.h" +#include "test-tool.h" +#include "repository.h" +#include "object-store.h" + +static void object_info(const char *gitdir, const char *oid_hex) +{ + struct repository r; + struct object_id oid; + unsigned long size; + struct object_info oi = {.sizep = &size}; + const char *p; + + if (repo_init(&r, gitdir, NULL)) + die("could not init repo"); + if (parse_oid_hex(oid_hex, &oid, &p)) + die("could not parse oid"); + if (oid_object_info_extended(&r, &oid, &oi, 0)) + die("could not obtain object info"); + printf("%d\n", (int) size); +}
Hmm. Is there a reason that the same couldn't be implemented by calling "git cat-file -s" from the partial clone?
quoted hunk ↗ jump to hunk
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 584a039b85..e804d267e6 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh@@ -604,6 +604,30 @@ test_expect_success 'do not fetch when checking existence of tree we construct o git -C repo cherry-pick side1 ' +test_expect_success 'lazy-fetch when accessing object not in the_repository' ' + rm -rf full partial.git && + test_create_repo full && + printf 12345 >full/file.txt && + git -C full add file.txt && + git -C full commit -m "first commit" &&
This is a stylistic nit, but I think using test_commit is better here
for a non-superficial reason. My guess is that you wanted to avoid
specifying a message and file (which are required positional arguments
to test_commit before you can specify the contents). But I think there
are two good reasons to use test_commit here:
- It saves three lines of test script here.
- You don't have to make the expected size a magic number (i.e.,
because you knew ahead of time that the contents was "12345").
I probably would have expected this test to end with:
git -C full cat-file -s $FILE_HASH >expect &&
git -C partial.git cat-file -s $FILE_HASH >actual &&
test_cmp expect actual
which reads more clearly to me (although I think the much more important
test is that $FILE_HASH doesn't show up in the output of the rev-list
--missing=print that is run in the partial clone).
+ + test_config -C full uploadpack.allowfilter 1 && + test_config -C full uploadpack.allowanysha1inwant 1 && + git clone --filter=blob:none --bare "file://$(pwd)/full" partial.git && + FILE_HASH=$(git hash-object --stdin <full/file.txt) &&
This works for me, although I wouldn't have been sad to see the sub-shell contain "git -C full rev-parse HEAD:file.txt" instead.
+ # Sanity check that the file is missing + git -C partial.git rev-list --objects --missing=print HEAD >out && + grep "[?]$FILE_HASH" out && + + OUT=$(test-tool partial-clone object-info partial.git "$FILE_HASH") &&
Coming back to my point about the utility of the partial-clone helper, could this be replaced by saying just OUT="$(git -C partial.git cat-file -s "$FILE_HASH")" instead? Thanks, Taylor