* [PATCH] t5500: fix mistaken $SERVER reference in helper function
@ 2024-06-19 12:52 Jeff King
2024-06-20 9:27 ` Jonathan Nieder
0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2024-06-19 12:52 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] t5500: fix mistaken $SERVER reference in helper function
2024-06-19 12:52 [PATCH] t5500: fix mistaken $SERVER reference in helper function Jeff King
@ 2024-06-20 9:27 ` Jonathan Nieder
2024-06-20 15:22 ` Jeff King
2024-06-20 18:06 ` Junio C Hamano
0 siblings, 2 replies; 4+ messages in thread
From: Jonathan Nieder @ 2024-06-20 9:27 UTC (permalink / raw)
To: Jeff King; +Cc: git
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] t5500: fix mistaken $SERVER reference in helper function
2024-06-20 9:27 ` Jonathan Nieder
@ 2024-06-20 15:22 ` Jeff King
2024-06-20 18:06 ` Junio C Hamano
1 sibling, 0 replies; 4+ messages in thread
From: Jeff King @ 2024-06-20 15:22 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] t5500: fix mistaken $SERVER reference in helper function
2024-06-20 9:27 ` Jonathan Nieder
2024-06-20 15:22 ` Jeff King
@ 2024-06-20 18:06 ` Junio C Hamano
1 sibling, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2024-06-20 18:06 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jeff King, git
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-06-20 18:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-19 12:52 [PATCH] t5500: fix mistaken $SERVER reference in helper function Jeff King
2024-06-20 9:27 ` Jonathan Nieder
2024-06-20 15:22 ` Jeff King
2024-06-20 18:06 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox