| Message ID | 20240619125255.GA346466@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 40d817875dd66b2e3f94075c19ce8972fae30134 |
| Headers | show |
| Series | t5500: fix mistaken $SERVER reference in helper function | expand |
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
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
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.
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
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(-)