[PATCH] t5500: fix mistaken $SERVER reference in helper function

[PATCH] t5500: fix mistaken $SERVER reference in helper function

[Jeff King]

The end of t5500 contains two tests which use a single helper function,
fetch_filter_blob_limit_zero(). It takes a parameter to point to the
path of the server repository, which we store locally as $SERVER. The
first caller uses the relative path "server", while the second points
into the httpd document root.

Commit 07ef3c6604 (fetch test: use more robust test for filtered
objects, 2019-12-23) refactored some lines, but accidentally switched
"$SERVER" to "server" in one spot. That means the second caller is
looking at the server directory from the previous test rather than its
own.

This happens to work out because the "server" directory from the first
test is still hanging around, and the contents of the two are identical.
But it was clearly not the intended behavior, and is fragile to cleaning
up the leftovers from the first test.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5500-fetch-pack.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 1bc15a3f08..b26f367620 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -1046,7 +1046,7 @@ fetch_filter_blob_limit_zero () {
 
 	# Ensure that commit is fetched, but blob is not
 	commit=$(git -C "$SERVER" rev-parse two) &&
-	blob=$(git hash-object server/two.t) &&
+	blob=$(git hash-object "$SERVER/two.t") &&
 	git -C client rev-list --objects --missing=allow-any "$commit" >oids &&
 	grep "$commit" oids &&
 	! grep "$blob" oids
-- 
2.45.2.949.g1c649f6aed

Re: [PATCH] t5500: fix mistaken $SERVER reference in helper function

[Jonathan Nieder]

Hi,

Jeff King wrote:

> This happens to work out because the "server" directory from the first
> test is still hanging around, and the contents of the two are identical.
> But it was clearly not the intended behavior, and is fragile to cleaning
> up the leftovers from the first test.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t5500-fetch-pack.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Restating to make sure I understand correctly.

fetch_filter_blob_limit_zero is a helper for parameterized tests
taking two parameters.  The first is the path to a repository we will
clone from, and the second is a clone URL to clone from that
repository.  (That way, we can reuse the same logic for multiple URL
schemes.)

Those tests each do the following:

- set up $SERVER containing a test commit and allowing partial clone
- clone from $URL to client
- make a new commit in $SERVER, that client doesn't have
- fetch to catch up, with --filter=blob:none
- assert that the new commit was fetched and new blob wasn't

And in that assertion, we want to get the name of the new commit and
new blob from $SERVER, not client, since we wouldn't want a side
effect of causing them to be fetched in the process.

Alas, in a copy-and-paste gone wrong, 07ef3c6604 gets the name of the
blob (but not the commit) from "server" instead of $SERVER.  And this
happens to work because the first time we call this helper, $SERVER is
"server".  The only reason this happens to work at all is that we're
looking at a blob id; if we looked at the commit id, then the
timestamps wouldn't have matched.

Thanks, the fix is obviously correct.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Particularly telling that the author of 07ef3c6604 introduced this
typo while trying to make the tests _more_ robust.

Once the library code is ready for it, this might be a good candidate
for moving most of the test cases into unit tests and just having one
or two less repetitive integration tests.

Thanks for catching and fixing it!

Jonathan

Re: [PATCH] t5500: fix mistaken $SERVER reference in helper function

[Jeff King]

On Thu, Jun 20, 2024 at 11:27:58AM +0200, Jonathan Nieder wrote:

> Alas, in a copy-and-paste gone wrong, 07ef3c6604 gets the name of the
> blob (but not the commit) from "server" instead of $SERVER.  And this
> happens to work because the first time we call this helper, $SERVER is
> "server".  The only reason this happens to work at all is that we're
> looking at a blob id; if we looked at the commit id, then the
> timestamps wouldn't have matched.

Yep, exactly.

> Particularly telling that the author of 07ef3c6604 introduced this
> typo while trying to make the tests _more_ robust.

:)

> Once the library code is ready for it, this might be a good candidate
> for moving most of the test cases into unit tests and just having one
> or two less repetitive integration tests.

Maybe. The subtlety fixed by 07ef3c6604 was that Git was lazy-fetching
objects when we didn't want it to, and the solution was to acquire the
needed data from outside the repository/process entirely. Sticking it
all in a single process creates more risks there (though I agree in a
robust lib-ified world you would have two separate "struct repository"
handles).

-Peff

Re: [PATCH] t5500: fix mistaken $SERVER reference in helper function

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Alas, in a copy-and-paste gone wrong, 07ef3c6604 gets the name of the
> blob (but not the commit) from "server" instead of $SERVER.  And this
> happens to work because the first time we call this helper, $SERVER is
> "server".  The only reason this happens to work at all is that we're
> looking at a blob id; if we looked at the commit id, then the
> timestamps wouldn't have matched.
>
> Thanks, the fix is obviously correct.
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Particularly telling that the author of 07ef3c6604 introduced this
> typo while trying to make the tests _more_ robust.

;-)

Thanks both.