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@xxxxxxxx> > --- > 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@xxxxxxxxx> 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
