partial-clone: add a partial-clone test case

From: Abhradeep Chakraborty <[email protected]>

In a blobless-cloned repo, git log --follow -- <path> (<path> have an exact OID rename) doesn't download blob of the file from where the new file is renamed.

+test_expect_success 'git log --follow does not download blobs if an exact OID rename found (blobless clone)' '

+'

t/t0410-partial-clone.sh +1 -0

+
 test_expect_success 'lazy-fetch when accessing object not in the_repository' '
 	rm -rf full partial.git &&
 	test_create_repo full &&

t/test-lib-functions.sh +0 -0

index 0f439c99d61..07a2b60c103 100644

Is this "doesn't" (documenting current behaviour, without saying if it is wrong or is desired) or "shouldn't" (documenting the desired behaviour, which the current implementation may or may not satisfy)?

That's mouthful.

Lose SP after '>'.

		git -C partial.git log --follow -- new-file.txt >"$(pwd)/trace.txt" &&

Looking at the implementation of the helper, it seems to be prepared to handle negation itself. Shouldn't this be

test_subcommand_inexact ! fetch <trace.txt

instead?

Why can't you specify what should NOT come before "fetch" in your use of this helper?

I wonder if it was more like this that the original wanted to grep for:

	grep '"event":"child_start".*\["git","pack-objects",.*\]'

Quoted 2 lines · Junio C Hamano

Why can't you specify what should NOT come before "fetch" in your use of this helper?

Below is the event triggered for non-exact OID rename -

	git -c fetch.negotiationAlgorithm=noop fetch origin --no-tags --no-write-fetch-head --recurse-submodules=no --filter=blob:none --stdin

Derrick told me to not depend on other flags like
-c fetch.negotiationAlgorithm etc. as they might be changed or omitted and as it makes sense to me also. That's why I didn't specify those things.

Quoted 3 lines

I wonder if it was more like this that the original wanted to grep for:

	grep '"event":"child_start".*\["git","pack-objects",.*\]'

I don't know about other cases, but in my case, atleast I really wanted it.

So, In this scenerio, should I stick with test_subcommand_inexact or I have to see other helper functions (or make my own) for it?

Quoted 5 lines

	git -c fetch.negotiationAlgorithm=noop fetch origin --no-tags --no-write-fetch-head --recurse-submodules=no --filter=blob:none --stdin

Derrick told me to not depend on other flags like
-c fetch.negotiationAlgorithm etc. as they might be changed or omitted and as it makes sense to me also. That's why I didn't specify those things.

This reason is something that could be mentioned in the commit message to motivate the change to the helper.

Quoted 9 lines

Quoted 3 lines

I wonder if it was more like this that the original wanted to grep for:

	grep '"event":"child_start".*\["git","pack-objects",.*\]'

I don't know about other cases, but in my case, atleast I really wanted it.

So, In this scenerio, should I stick with test_subcommand_inexact or I have to see other helper functions (or make my own) for it?

As I mentioned earlier, it seems that

test_subcommand_inexact ! git fetch

would actually work for your needs without changing the helper. We will see whether or not the helper needs to be updated in a way that that line would not work anymore.

This reason is something that could be mentioned in the commit message to motivate the change to the helper.

As I mentioned earlier, it seems that

test_subcommand_inexact ! git fetch

would actually work for your needs without changing the helper. We will see whether or not the helper needs to be updated in a way that that line would not work anymore.

Quoted 3 lines · Junio C Hamano

Is this "doesn't" (documenting current behaviour, without saying if it is wrong or is desired) or "shouldn't" (documenting the desired behaviour, which the current implementation may or may not satisfy)?

The current behaviour is okay and this commit adds the test case for it. So, in that sense, I think "shouldn't" is better word.

Quoted 1 line

That's mouthful.

Sorry if the test name is very long. But, I couldn't think of shorter test name than this - to explain what the test case is.

Quoted 3 lines

Lose SP after '>'.

		git -C partial.git log --follow -- new-file.txt >"$(pwd)/trace.txt" &&

Okay.

Quoted 6 lines

Looking at the implementation of the helper, it seems to be prepared to handle negation itself. Shouldn't this be

test_subcommand_inexact ! fetch <trace.txt

instead?

Oops, completely missed it. Correcting it :)

Quoted 2 lines

Why can't you specify what should NOT come before "fetch" in your use of this helper?

Below is the event triggered for non-exact OID rename -

	git -c fetch.negotiationAlgorithm=noop fetch origin --no-tags --no-write-fetch-head --recurse-submodules=no --filter=blob:none --stdin

Derrick told me to not depend on other flags like
-c fetch.negotiationAlgorithm etc. as they might be changed or omitted and as it makes sense to me also. That's why I didn't specify those things.

Quoted 2 lines

I wonder if it was more like this that the original wanted to grep for:

Quoted 1 line

	grep '"event":"child_start".*\["git","pack-objects",.*\]'

I don't know about other cases, but in my case, atleast I really wanted it.

So, In this scenerio, should I stick with test_subcommand_inexact or I have to see other helper functions (or make my own) for it?

"Abhradeep Chakraborty via GitGitGadget" <[email protected]>

t/test-lib-functions.sh test_subcommand_inexact () { · line 1814

Quoted 10 lines · writes:

t/test-lib-functions.sh +1 -1

index 0f439c99d61..07a2b60c103 100644
@@ -1811,7 +1811,7 @@ test_subcommand_inexact () {
 		shift
 	fi
 
-	local expr=$(printf '"%s".*' "$@")
+	local expr=$(printf '.*"%s".*' "$@")

The original wanted to make sure that the arguments to the helper are initial items of a comma separated list, and an existing caller, for example, i.e.

  test_subcommand_inexact git pack-objects --honor-pack-keep <trace

is relying on the behaviour to make sure 'git', 'pack-objects', ... appear at the beginning of "[...]" enclosed list. This change breaks its ability to notice that an insertion of unrelated token before 'git' as an error.

In other words, it looks like an uncalled-for selfish change.

Why can't you specify what should NOT come before "fetch" in your use of this helper?

t/test-lib-functions.sh test_subcommand_inexact () { · line 1815

Quoted 1 line

t/test-lib-functions.sh +0 -0

 	expr="${expr%,}"

The preimage already has this problem, but the stripping of trailing comma here is a result of mistaken copy-and-paste from the exact variant, I think. test_subcommand uses

	local expr=$(printf '"%s",' "$@")

to concatenate "$@" into a single comma-separated string, so it perfectly makes sense to drop the last one here, but with or without your change here, neither is adding a comma that need to be stripped.

It is not _your_ theme, but I think this helper is poorly designed, especially compared to the original "exact" variant.

        test_subcommand_inexact () {
                local negate=
                if test "$1" = "!"
                then
                        negate=t
                        shift
                fi

                local expr=$(printf '"%s".*' "$@")
                expr="${expr%,}"

                if test -n "$negate"
                then
                        ! grep "\"event\":\"child_start\".*\[$expr\]"
                else
                        grep "\"event\":\"child_start\".*\[$expr\]"
                fi
        }

I've already touched that "${expr%,}" there is a totally useful statement that will always be a no-op.

When "test_subcommand_inexact git pack-objects" is run, the printf assigns to $expr:

		expr='"git".*"pack-objects".*'

and the actual grep command invoked becomes

	grep '"event":"child_start".*\["git".*"pack-objects".*\]'

I am not sure if that is what we really want.

I wonder if it was more like this that the original wanted to grep for:

	grep '"event":"child_start".*\["git","pack-objects",.*\]'

in which case the two lines there should be more like

	local expr=$(printf '"%s",' "$@")
	expr="${expr%,}.*"

I would think. This comes from Derrick's e4d0c11c (repack: respect kept objects with '--write-midx -b', 2021-12-20).

Quoted 9 lines

When "test_subcommand_inexact git pack-objects" is run, the printf assigns to $expr:

		expr='"git".*"pack-objects".*'

and the actual grep command invoked becomes

	grep '"event":"child_start".*\["git".*"pack-objects".*\]'

Quoted 1 line

I am not sure if that is what we really want.

Ah, yes this certainly seems to not be the expected plan. It does allow for more flexibility than intended: the intention was to add flexibility at the end of the command, but instead adds flexibility throughout, only caring that a certain list of options is present as a subsequence (except that the first item is the first item, namely "git" in most cases).

That unintended flexibility would allow the current needs to use

test_subcommand_inexact ! git fetch

as desired, but there is the additional worries about whether it is too flexible for the existing uses.

If you think that we should fix the helper to work differently, then I can work on a patch to do so, so Abhradeep doesn't get too sidetracked on that.

I found it quite a roundabout way to look into trace to see if a "fetch" was run to determine if we are doing the right thing.

Regardless of whatever mechanism is used to lazily fetch objects that have become necessary from the promisor remotes, what we want to ensure is that the blob object HEAD:new-file.txt is still missing in our object store after running "log --follow", isn't it? In a future version of "git", our on-demand lazy fetch mechanism may not even invoke "git fetch" under the hood, after all.

Don't we have a more direct way to ask "does this object exist in our object store, or is it merely left as a promise?" without triggering a lazy fetching that we can use in this test? I think such a direct approach is what we want to use in this test.

Quoted 10 lines · Junio C Hamano

I found it quite a roundabout way to look into trace to see if a "fetch" was run to determine if we are doing the right thing.

Regardless of whatever mechanism is used to lazily fetch objects that have become necessary from the promisor remotes, what we want to ensure is that the blob object HEAD:new-file.txt is still missing in our object store after running "log --follow", isn't it? In a future version of "git", our on-demand lazy fetch mechanism may not even invoke "git fetch" under the hood, after all.

Quoted 4 lines

Don't we have a more direct way to ask "does this object exist in our object store, or is it merely left as a promise?" without triggering a lazy fetching that we can use in this test? I think such a direct approach is what we want to use in this test.

I did think of other ways to detect the downloading before. Initially I thought of using grep functionality to see if any download related messages are printed or not. But I found that git log doesn't print any download related messages (e.g. like "enumerating ...."). So, I dropped that idea.

The next idea that came into my mind was to detect if the previous file (the file from where the new file is renamed) is still missing ( you're suggesting the same approach). But the problem I faced with this apprach is I didn't find a way to detect if the file is missing.

I tried to use git rev-list --objects --missing=print with HEAD and first commit hash. But in both cases, I didn't found a missing [?] sign before <blob-hash-ID> file.txt. That means, both blob objects ( or I think the same blob object) exists in the local repo.

Most probably, this is because the content of these two files are same. Blob object is never modified. So, as far as I think, both base trees ( one which is pointed by the initial commit and other which is pointed by the latest commit) is pointing to the same blob object. As a result, the file is not missing. ( I am not sure if git really does it as I said; sorry if I am wrong here)

That's why I asked Derrick to help me detecting the download and he suggested me this. It is true that this is not ideal (use of fetch may change later). But this was the only option that I can go with.

If you find another/better way or if you think I tried to use git rev-list ... in the wrong way - please tell me. I would be very happy.

Ah! This method really should have worked. And the test as written is not testing the right thing, because we also skip downloading the blobs if we already have them.

I think the key issue is that your clone says this:

t/t0410-partial-clone.sh +1 -0

+	git clone --filter=blob:none "file://$(pwd)/repo" partial.git &&

which will do a checkout and download the blobs at tip.

If you add a "--bare" to this clone command, then no blobs should be downloaded, and your rev-list command should show the missing objects.

Quoted 4 lines

I tried to use git rev-list --objects --missing=print with HEAD and first commit hash. But in both cases, I didn't found a missing [?] sign before <blob-hash-ID> file.txt. That means, both blob objects ( or I think the same blob object) exists in the local repo.

...

I think the key issue is that your clone says this:

t/t0410-partial-clone.sh +1 -0

+	git clone --filter=blob:none "file://$(pwd)/repo" partial.git &&

which will do a checkout and download the blobs at tip.

If you add a "--bare" to this clone command, then no blobs should be downloaded, and your rev-list command should show the missing objects.

Hopefully my suggestion to use --bare will help.

That sounds like pointing at a different issue. If the test repository downloads the blobs at the tip, then the fact that the trace output did not have "fetch" in it does not mean much, does it? It could be that we refrained from lazily download the blob because we did not need its contents for the purpose of following through an exact rename, but it could also be that we did not need to lazily download it because we already had it.

So regardless of "--missing=print" vs "grep in trace", there was a bug in the test set-up, and we caught it in this discussion, which is excellent.

From: Abhradeep Chakraborty <[email protected]>

In a blobless-cloned repo, git log --follow -- <path> (<path> have an exact OID rename) shouldn't download blob of the file from where the new file is renamed.

Add a test case to verify it.

Signed-off-by Abhradeep Chakraborty <[email protected]>

---
partial-clone: add a partial-clone test case

changes since v1:

1. remove the event track method to detect the downloading as it is not future proof ( and buggy).
2. Instead see if the file is missing initially and after running the git log --follow ... command.

t/t0410-partial-clone.sh +9 -0

+test_expect_success 'exact rename does not need to fetch the blob lazily' '
+	rm -rf repo partial.git &&
+	test_create_repo repo &&
+	content="some dummy content" &&
+	test_commit -C repo create-a-file file.txt "$content" &&
+	git -C repo mv file.txt new-file.txt &&
+	git -C repo commit -m rename-the-file &&
+	FILE_HASH=$(git -C repo rev-parse HEAD:new-file.txt) &&
+	test_config -C repo uploadpack.allowfilter 1 &&