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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 20, 2024, 09:27 UTC
Message-ID: <ZnM3I11IRporu4sj@google.com>
In-Reply-To: <20240619125255.GA346466@coredump.intra.peff.net>

Hi,

Jeff King wrote:

Show 9 quoted lines

> 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