[v2,17/23] t5616: use correct filter syntax
diff mbox series

Message ID 20200125230035.136348-19-sandals@crustytoothpaste.net
State New
Headers show
Series
  • SHA-256 test fixes, part 8
Related show

Commit Message

brian m. carlson Jan. 25, 2020, 11 p.m. UTC
In the setup steps for the promisor remote tests, we clone a repository
and filter out all trees with depth greater than or equal to zero, which
also filters out all blobs.

With SHA-1, this test passes because the object we happen to request
from the server is the blob that the promisor remote has.  However, due
to a different ordering with SHA-256, we request the tree containing
that blob, which the promisor remote does not have.  As a consequence,
we fail with a "not our ref" error.

Since what we want to test is that the blob is transferred, let's adjust
the filter to just filter out blobs, not trees.  That means that we'll
transfer the previously problematic tree as part of the normal clone,
and we can then test that the blob is fetched from the promisor remote
as expected.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t5616-partial-clone.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Jan. 28, 2020, 7:06 p.m. UTC | #1
"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> In the setup steps for the promisor remote tests, we clone a repository
> and filter out all trees with depth greater than or equal to zero, which
> also filters out all blobs.
>
> With SHA-1, this test passes because the object we happen to request
> from the server is the blob that the promisor remote has.  However, due
> to a different ordering with SHA-256, we request the tree containing
> that blob, which the promisor remote does not have.  As a consequence,
> we fail with a "not our ref" error.

Sorry, but I do not understand this part.

The object name of the original blob (which is the only thing
"promisor-remote" is given) may sort earlier or later than other
objects that are missing in the "client" repository, but it is not
clear how it makes difference in the final outcome---even if the
blob is asked first (in the SHA-1 version), wouldn't we need to
fetch the tree after that, and wouldn't that fail?  If the SHA-256
version that happens to ask for the tree first and fails, wouldn't
that mean we need to fetch both anyway?

Is it that the current test with SHA-1 is broken in that it lets the
lazy fetch fail (due to missing tree) but because the failure happens
after the blob gets feteched, and it ignores the failure of the lazy
fetch, and only checks if the blob got fetched, it happens to "pass"?

> Since what we want to test is that the blob is transferred, let's adjust
> the filter to just filter out blobs, not trees.  That means that we'll
> transfer the previously problematic tree as part of the normal clone,
> and we can then test that the blob is fetched from the promisor remote
> as expected.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  t/t5616-partial-clone.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index fea56cda6d..9fd6e780f9 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -317,7 +317,7 @@ setup_triangle () {
>  	cp big-blob.txt server &&
>  	git -C server add big-blob.txt &&
>  	git -C server commit -m "initial" &&
> -	git clone --bare --filter=tree:0 "file://$(pwd)/server" client &&
> +	git clone --bare --filter=blob:none "file://$(pwd)/server" client &&
>  	echo another line >>server/big-blob.txt &&
>  	git -C server commit -am "append line to big blob" &&
>
brian m. carlson Jan. 29, 2020, 3:53 a.m. UTC | #2
On 2020-01-28 at 19:06:01, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > In the setup steps for the promisor remote tests, we clone a repository
> > and filter out all trees with depth greater than or equal to zero, which
> > also filters out all blobs.
> >
> > With SHA-1, this test passes because the object we happen to request
> > from the server is the blob that the promisor remote has.  However, due
> > to a different ordering with SHA-256, we request the tree containing
> > that blob, which the promisor remote does not have.  As a consequence,
> > we fail with a "not our ref" error.
> 
> Sorry, but I do not understand this part.
> 
> The object name of the original blob (which is the only thing
> "promisor-remote" is given) may sort earlier or later than other
> objects that are missing in the "client" repository, but it is not
> clear how it makes difference in the final outcome---even if the
> blob is asked first (in the SHA-1 version), wouldn't we need to
> fetch the tree after that, and wouldn't that fail?  If the SHA-256
> version that happens to ask for the tree first and fails, wouldn't
> that mean we need to fetch both anyway?
> 
> Is it that the current test with SHA-1 is broken in that it lets the
> lazy fetch fail (due to missing tree) but because the failure happens
> after the blob gets feteched, and it ignores the failure of the lazy
> fetch, and only checks if the blob got fetched, it happens to "pass"?

I think Jonathan Tan figured out that my analysis was wrong, and that
the issue is that the larger object ID length causes deltification to
happen.  The test assumes that the tree is sent as a non-delta object,
and when it's sent as a deltaed object instead, we fail.  He explains
this quite well in <20200113202823.228062-1-jonathantanmy@google.com>,
which you seem to have picked up.

Since my analysis was wrong here and he's provided a patch which fixes
the issue in a much more robust way, I'm dropping this patch in v3.

Patch
diff mbox series

diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index fea56cda6d..9fd6e780f9 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -317,7 +317,7 @@  setup_triangle () {
 	cp big-blob.txt server &&
 	git -C server add big-blob.txt &&
 	git -C server commit -m "initial" &&
-	git clone --bare --filter=tree:0 "file://$(pwd)/server" client &&
+	git clone --bare --filter=blob:none "file://$(pwd)/server" client &&
 	echo another line >>server/big-blob.txt &&
 	git -C server commit -am "append line to big blob" &&