{"thread":{"id":"61648","subject":"[PATCH] t5500: fix mistaken $SERVER reference in helper function","startedAt":"2024-06-19T12:52:58Z","lastAt":"2024-06-20T18:06:48Z","messageCount":4,"participants":["Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"497342","messageId":"20240619125255.GA346466@coredump.intra.peff.net","threadId":"61648","inReplyTo":null,"subject":"[PATCH] t5500: fix mistaken $SERVER reference in helper function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-19T12:52:55Z","receivedAt":"2024-06-19T12:52:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The end of t5500 contains two tests which use a single helper function,\nfetch_filter_blob_limit_zero(). It takes a parameter to point to the\npath of the server repository, which we store locally as $SERVER. The\nfirst caller uses the relative path \"server\", while the second points\ninto the httpd document root.\n\nCommit 07ef3c6604 (fetch test: use more robust test for filtered\nobjects, 2019-12-23) refactored some lines, but accidentally switched\n\"$SERVER\" to \"server\" in one spot. That means the second caller is\nlooking at the server directory from the previous test rather than its\nown.\n\nThis happens to work out because the \"server\" directory from the first\ntest is still hanging around, and the contents of the two are identical.\nBut it was clearly not the intended behavior, and is fragile to cleaning\nup the leftovers from the first test.\n\nSigned-off-by: Jeff King \n---\n t/t5500-fetch-pack.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 1bc15a3f08..b26f367620 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -1046,7 +1046,7 @@ fetch_filter_blob_limit_zero () {\n \n \t# Ensure that commit is fetched, but blob is not\n \tcommit=$(git -C \"$SERVER\" rev-parse two) &&\n-\tblob=$(git hash-object server/two.t) &&\n+\tblob=$(git hash-object \"$SERVER/two.t\") &&\n \tgit -C client rev-list --objects --missing=allow-any \"$commit\" >oids &&\n \tgrep \"$commit\" oids &&\n \t! grep \"$blob\" oids\n-- \n2.45.2.949.g1c649f6aed\n"},{"id":"497393","messageId":"ZnM3I11IRporu4sj@google.com","threadId":"61648","inReplyTo":"20240619125255.GA346466@coredump.intra.peff.net","subject":"Re: [PATCH] t5500: fix mistaken $SERVER reference in helper function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2024-06-20T09:27:58Z","receivedAt":"2024-06-20T09:28:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> This happens to work out because the \"server\" directory from the first\n> test is still hanging around, and the contents of the two are identical.\n> But it was clearly not the intended behavior, and is fragile to cleaning\n> up the leftovers from the first test.\n>\n> Signed-off-by: Jeff King \n> ---\n> t/t5500-fetch-pack.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nRestating to make sure I understand correctly.\n\nfetch_filter_blob_limit_zero is a helper for parameterized tests\ntaking two parameters. The first is the path to a repository we will\nclone from, and the second is a clone URL to clone from that\nrepository. (That way, we can reuse the same logic for multiple URL\nschemes.)\n\nThose tests each do the following:\n\n- set up $SERVER containing a test commit and allowing partial clone\n- clone from $URL to client\n- make a new commit in $SERVER, that client doesn't have\n- fetch to catch up, with --filter=blob:none\n- assert that the new commit was fetched and new blob wasn't\n\nAnd in that assertion, we want to get the name of the new commit and\nnew blob from $SERVER, not client, since we wouldn't want a side\neffect of causing them to be fetched in the process.\n\nAlas, in a copy-and-paste gone wrong, 07ef3c6604 gets the name of the\nblob (but not the commit) from \"server\" instead of $SERVER. And this\nhappens to work because the first time we call this helper, $SERVER is\n\"server\". The only reason this happens to work at all is that we're\nlooking at a blob id; if we looked at the commit id, then the\ntimestamps wouldn't have matched.\n\nThanks, the fix is obviously correct.\n\nReviewed-by: Jonathan Nieder \n\nParticularly telling that the author of 07ef3c6604 introduced this\ntypo while trying to make the tests _more_ robust.\n\nOnce the library code is ready for it, this might be a good candidate\nfor moving most of the test cases into unit tests and just having one\nor two less repetitive integration tests.\n\nThanks for catching and fixing it!\n\nJonathan\n"},{"id":"497403","messageId":"20240620152242.GA1555496@coredump.intra.peff.net","threadId":"61648","inReplyTo":"ZnM3I11IRporu4sj@google.com","subject":"Re: [PATCH] t5500: fix mistaken $SERVER reference in helper function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-20T15:22:42Z","receivedAt":"2024-06-20T15:22:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2024 at 11:27:58AM +0200, Jonathan Nieder wrote:\n\n> Alas, in a copy-and-paste gone wrong, 07ef3c6604 gets the name of the\n> blob (but not the commit) from \"server\" instead of $SERVER. And this\n> happens to work because the first time we call this helper, $SERVER is\n> \"server\". The only reason this happens to work at all is that we're\n> looking at a blob id; if we looked at the commit id, then the\n> timestamps wouldn't have matched.\n\nYep, exactly.\n\n> Particularly telling that the author of 07ef3c6604 introduced this\n> typo while trying to make the tests _more_ robust.\n\n:)\n\n> Once the library code is ready for it, this might be a good candidate\n> for moving most of the test cases into unit tests and just having one\n> or two less repetitive integration tests.\n\nMaybe. The subtlety fixed by 07ef3c6604 was that Git was lazy-fetching\nobjects when we didn't want it to, and the solution was to acquire the\nneeded data from outside the repository/process entirely. Sticking it\nall in a single process creates more risks there (though I agree in a\nrobust lib-ified world you would have two separate \"struct repository\"\nhandles).\n\n-Peff\n"},{"id":"497420","messageId":"xmqqv823v68c.fsf@gitster.g","threadId":"61648","inReplyTo":"ZnM3I11IRporu4sj@google.com","subject":"Re: [PATCH] t5500: fix mistaken $SERVER reference in helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-20T18:06:43Z","receivedAt":"2024-06-20T18:06:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Alas, in a copy-and-paste gone wrong, 07ef3c6604 gets the name of the\n> blob (but not the commit) from \"server\" instead of $SERVER. And this\n> happens to work because the first time we call this helper, $SERVER is\n> \"server\". The only reason this happens to work at all is that we're\n> looking at a blob id; if we looked at the commit id, then the\n> timestamps wouldn't have matched.\n>\n> Thanks, the fix is obviously correct.\n>\n> Reviewed-by: Jonathan Nieder \n>\n> Particularly telling that the author of 07ef3c6604 introduced this\n> typo while trying to make the tests _more_ robust.\n\n;-)\n\nThanks both.\n"}]}