{"thread":{"id":"57594","subject":"[PATCH] test-lib-functions: fix test_subcommand_inexact","startedAt":"2022-03-21T20:34:26Z","lastAt":"2022-03-30T02:44:29Z","messageCount":30,"participants":["Derrick Stolee via GitGitGadget","Derrick Stolee","Junio C Hamano","Taylor Blau","Abhradeep Chakraborty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"451760","messageId":"pull.1185.git.1647894845421.gitgitgadget@gmail.com","threadId":"57594","inReplyTo":null,"subject":"[PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-21T20:34:05Z","receivedAt":"2022-03-21T20:34:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe implementation of test_subcommand_inexact() was originally\nintroduced in e4d0c11c0 (repack: respect kept objects with '--write-midx\n-b', 2021-12-20) with the intention to allow finding a subcommand based\non an initial set of arguments. The inexactness was intended as a way to\nallow flexible options beyond that initial set, as opposed to\ntest_subcommand() which requires that the full list of options is\nprovided in its entirety.\n\nThe implementation began by copying test_subcommand() and replaced the\nrepeated argument 'printf' statement to append \".*\" instead of \",\" to\neach argument. This has a few drawbacks:\n\n1. Most importantly, this repeats the use of \".*\" within 'expr', so the\n inexact match is even more flexible than expected. It allows the list\n of arguments to exist as a subsequence (with any other items included\n between those arguments).\n\n2. The line 'expr=\"$(expr%,}\"' that previously removed a trailing comma\n now no longer does anything, since the string ends with \".*\".\n\nBoth of these issues are fixed by keeping the addition of the comma in\nthe printf statement, then adding \".*\" after stripping out the trailing\ncomma.\n\nAll existing tests continue to pass with this change, since none of them\nwere taking advantage of this unintended flexibility.\n\nSigned-off-by: Derrick Stolee \n---\n test-lib-functions: fix test_subcommand_inexact\n \n This fixes a concern discovered by Junio in [1].\n \n [1] https://lore.kernel.org/git/xmqq4k41vdwe.fsf@gitster.g/\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1185%2Fderrickstolee%2Ftest-subcommand-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1185/derrickstolee/test-subcommand-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1185\n\n t/test-lib-functions.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 0f439c99d61..8f0e5da8727 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1811,8 +1811,8 @@ test_subcommand_inexact () {\n \t\tshift\n \tfi\n \n-\tlocal expr=$(printf '\"%s\".*' \"$@\")\n-\texpr=\"${expr%,}\"\n+\tlocal expr=$(printf '\"%s\",' \"$@\")\n+\texpr=\"${expr%,}.*\"\n \n \tif test -n \"$negate\"\n \tthen\n\nbase-commit: 74cc1aa55f30ed76424a0e7226ab519aa6265061\n-- \ngitgitgadget\n"},{"id":"451849","messageId":"540936ba-7287-77fa-9cee-e257ed3c119d@github.com","threadId":"57594","inReplyTo":"pull.1185.git.1647894845421.gitgitgadget@gmail.com","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-22T15:17:51Z","receivedAt":"2022-03-22T15:17:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/21/22 4:34 PM, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The implementation of test_subcommand_inexact() was originally\n> introduced in e4d0c11c0 (repack: respect kept objects with '--write-midx\n> -b', 2021-12-20) with the intention to allow finding a subcommand based\n> on an initial set of arguments. The inexactness was intended as a way to\n> allow flexible options beyond that initial set, as opposed to\n> test_subcommand() which requires that the full list of options is\n> provided in its entirety.\n> \n> The implementation began by copying test_subcommand() and replaced the\n> repeated argument 'printf' statement to append \".*\" instead of \",\" to\n> each argument. This has a few drawbacks:\n> \n> 1. Most importantly, this repeats the use of \".*\" within 'expr', so the\n> inexact match is even more flexible than expected. It allows the list\n> of arguments to exist as a subsequence (with any other items included\n> between those arguments).\n> \n> 2. The line 'expr=\"$(expr%,}\"' that previously removed a trailing comma\n> now no longer does anything, since the string ends with \".*\".\n> \n> Both of these issues are fixed by keeping the addition of the comma in\n> the printf statement, then adding \".*\" after stripping out the trailing\n> comma.\n> \n> All existing tests continue to pass with this change, since none of them\n> were taking advantage of this unintended flexibility.\n\nFor some reason, I thought I had run all tests on my machine to\npassing, but I was wrong.\n\nInstead, t7700-repack.sh fails because of this test:\n\ntest_expect_success '--write-midx -b packs non-kept objects' '\n\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n\t\tgit repack --write-midx -a -b &&\n\ttest_subcommand_inexact git pack-objects --honor-pack-keep writes:\n\n> So, this patch is incorrect about keeping things working. The\n> options are:\n>\n> 1. Keep the repeated \".*\" and be clear about the expectations.\n> (This could drop the \"remove trailing comma\" line.)\n>\n> 2. Find another way to test this --write-midx behavior while\n> keeping the stricter test_subcommand_inexact helper.\n>\n> 3. Something else???\n\nThe result of doing #1 is still \"inexact\" but at that point it is\nunclear if we are being way too inexact to be useful. If the\nlooseness bothers us too much, we may decide that #1 is not worth\ndoing. But obviously the looseness did not bother us that much\nuntil last week, so probably an obvious #3, do nothing, letting the\nsleeping dog lie, might be what we want to do?\n\nIf we were to pursue #2, then, would we tightening the test for the\nwrite-midx using the \"stricter\" helper, or would the stricter one be\ntoo strict to be useful for that case? If we are rewriting the\nwrite-midx test by not using the \"stricter\" helper, then we would be\ncreating a stricter one nobody uses, which sounds quite wasteful.\n\nIt seems that the only case that could result in a result that is\nbetter than \"do nothing\" is if we can use a different pattern with\nthe \"stricter\" helper to express what \"write-midx\" test wanted to\ndo, but because what we need to fuzzily match on the command line in\nthat test includes a generated temporary filename, I do not think\nit is likely to be easily doable.\n\nSo, perhaps #3 ;-)?\n"},{"id":"451994","messageId":"e5331972-512a-c498-6a1b-927f21ef9de2@github.com","threadId":"57594","inReplyTo":"xmqqmthgu3e6.fsf@gitster.g","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-23T14:55:37Z","receivedAt":"2022-03-23T14:55:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/23/2022 10:53 AM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> So, this patch is incorrect about keeping things working. The\n>> options are:\n>>\n>> 1. Keep the repeated \".*\" and be clear about the expectations.\n>> (This could drop the \"remove trailing comma\" line.)\n>>\n>> 2. Find another way to test this --write-midx behavior while\n>> keeping the stricter test_subcommand_inexact helper.\n>>\n>> 3. Something else???\n> \n> The result of doing #1 is still \"inexact\" but at that point it is\n> unclear if we are being way too inexact to be useful. If the\n> looseness bothers us too much, we may decide that #1 is not worth\n> doing. But obviously the looseness did not bother us that much\n> until last week, so probably an obvious #3, do nothing, letting the\n> sleeping dog lie, might be what we want to do?\n> \n> If we were to pursue #2, then, would we tightening the test for the\n> write-midx using the \"stricter\" helper, or would the stricter one be\n> too strict to be useful for that case? If we are rewriting the\n> write-midx test by not using the \"stricter\" helper, then we would be\n> creating a stricter one nobody uses, which sounds quite wasteful.\n> \n> It seems that the only case that could result in a result that is\n> better than \"do nothing\" is if we can use a different pattern with\n> the \"stricter\" helper to express what \"write-midx\" test wanted to\n> do, but because what we need to fuzzily match on the command line in\n> that test includes a generated temporary filename, I do not think\n> it is likely to be easily doable.\n> \n> So, perhaps #3 ;-)?\n\nI'll default to #3 (do nothing), but if this shows up again\nI'll plan on adding a comment to the helper to be clear on\nhow \"inexact\" the helper really is.\n\nThanks,\n-Stolee\n"},{"id":"452074","messageId":"YjuVAgjaiqrcT7P1@nand.local","threadId":"57594","inReplyTo":"e5331972-512a-c498-6a1b-927f21ef9de2@github.com","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-23T21:45:38Z","receivedAt":"2022-03-23T21:45:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 23, 2022 at 10:55:37AM -0400, Derrick Stolee wrote:\n> > So, perhaps #3 ;-)?\n>\n> I'll default to #3 (do nothing), but if this shows up again\n> I'll plan on adding a comment to the helper to be clear on\n> how \"inexact\" the helper really is.\n\nI wonder if we could sidestep the whole issue with\ntest_subcommand_inexact by testing this behavior by looking at the\ncontents of the packs themselves.\n\nIf we have a kept pack, and then add some new objects, and run \"git\nrepack --write-midx -adb\", the new pack should not contain any of the\nobjects found in the old (kept) pack. And that's the case after this\npatch, but was broken before it.\n\nHere's a test which constructs that scenario and then asserts that there\nisn't any overlap between the newly created pack and the old, kept pack.\n\n--- 8< ---\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 5922fb5bdd..96812fa226 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -370,9 +370,31 @@ test_expect_success '--write-midx with preferred bitmap tips' '\n '\n\n test_expect_success '--write-midx -b packs non-kept objects' '\n-\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n-\t\tgit repack --write-midx -a -b &&\n-\ttest_subcommand_inexact git pack-objects --honor-pack-keep before &&\n+\t\t>$objdir/pack/$(basename $(cat before) .idx).keep &&\n+\n+\t\ttest_commit other &&\n+\t\tgit repack --write-midx -a -b -d &&\n+\n+\t\tfind $objdir/pack -name \"*.idx\" | sort >after &&\n+\n+\t\tgit show-index <$(cat before) >old.raw &&\n+\t\tgit show-index <$(comm -13 before after) >new.raw &&\n+\n+\t\tcut -d\" \" -f2 old.raw | sort >old.objects &&\n+\t\tcut -d\" \" -f2 new.raw | sort >new.objects &&\n+\n+\t\tcomm -12 old.objects new.objects >shared.objects &&\n+\t\ttest_must_be_empty shared.objects\n+\t)\n '\n\n test_expect_success TTY '--quiet disables progress' '\n\n--- >8 ---\n\nIt does seem a little word-y to me, but I think you could clean it up a\nlittle bit, too. If you want to take that patch, I think we could\nreasonably use the above diff as a first patch, and then remove the\ndeclaration of test_subcommand_inexact in a second patch.\n\n(Feel free to forge my s-o-b if you do want to pick that up).\n\nThanks,\nTaylor\n"},{"id":"452078","messageId":"xmqqfsn8p8nr.fsf@gitster.g","threadId":"57594","inReplyTo":"YjuVAgjaiqrcT7P1@nand.local","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-23T23:10:32Z","receivedAt":"2022-03-23T23:10:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Wed, Mar 23, 2022 at 10:55:37AM -0400, Derrick Stolee wrote:\n>> > So, perhaps #3 ;-)?\n>>\n>> I'll default to #3 (do nothing), but if this shows up again\n>> I'll plan on adding a comment to the helper to be clear on\n>> how \"inexact\" the helper really is.\n>\n> I wonder if we could sidestep the whole issue with\n> test_subcommand_inexact by testing this behavior by looking at the\n> contents of the packs themselves.\n>\n> If we have a kept pack, and then add some new objects, and run \"git\n> repack --write-midx -adb\", the new pack should not contain any of the\n> objects found in the old (kept) pack. And that's the case after this\n> patch, but was broken before it.\n\nSounds quite sensible.\n\nInstead of saying \"we are happy as long as we internally run this\ncommand, as that _should_ give us the desired outcome\", we check the\nresulting packs ourselves, and we do not really care how the\n\"repack\" command gave us that desired outcome.\n"},{"id":"452124","messageId":"72c54461-8af7-29fc-04da-f435adee9bbe@github.com","threadId":"57594","inReplyTo":"xmqqfsn8p8nr.fsf@gitster.g","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-24T15:42:44Z","receivedAt":"2022-03-24T15:42:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/23/2022 7:10 PM, Junio C Hamano wrote:\n> Taylor Blau writes:\n> \n>> On Wed, Mar 23, 2022 at 10:55:37AM -0400, Derrick Stolee wrote:\n>>>> So, perhaps #3 ;-)?\n>>>\n>>> I'll default to #3 (do nothing), but if this shows up again\n>>> I'll plan on adding a comment to the helper to be clear on\n>>> how \"inexact\" the helper really is.\n>>\n>> I wonder if we could sidestep the whole issue with\n>> test_subcommand_inexact by testing this behavior by looking at the\n>> contents of the packs themselves.\n>>\n>> If we have a kept pack, and then add some new objects, and run \"git\n>> repack --write-midx -adb\", the new pack should not contain any of the\n>> objects found in the old (kept) pack. And that's the case after this\n>> patch, but was broken before it.\n> \n> Sounds quite sensible.\n> \n> Instead of saying \"we are happy as long as we internally run this\n> command, as that _should_ give us the desired outcome\", we check the\n> resulting packs ourselves, and we do not really care how the\n> \"repack\" command gave us that desired outcome.\n\nSounds good. It's all about a balance: using test_subcommand[_inexact]\ngives us a way to check \"Did we trigger this other command that we\ntrust works correctly from other tests?\" without the more complicated\nwork of doing a full post-condition check. It's a bit more of a unit-\nlevel check than most Git tests.\n\nThe full post-condition check requires more test code, but that's not\nreally a problem. The problem comes in if that test is now too rigid\nto future changes in that subcommand. What if the post-conditions\nchange in a subtle way because of the subcommand does something\ndifferently, but in a way that is not of importance to the top\ncommand?\n\nIn this specific case, the test name says that it \"packs non-kept\nobjects\", so we can do more here to validate that post-condition\nthat we care about.\n\nAs I'm looking at Taylor's test case example, the one thing I notice\nis that there is only one pack-file before the repack. It would be\ngood to have a non-kept packfile get repacked in the process, not\njust the loose objects added by the test_commit. I'll take a look at\nwhat can be done here.\n\nThanks,\n-Stolee\n"},{"id":"452125","messageId":"YjyWFqjkaGpC8NxQ@nand.local","threadId":"57594","inReplyTo":"72c54461-8af7-29fc-04da-f435adee9bbe@github.com","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-24T16:02:30Z","receivedAt":"2022-03-24T16:02:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 24, 2022 at 11:42:44AM -0400, Derrick Stolee wrote:\n> As I'm looking at Taylor's test case example, the one thing I notice\n> is that there is only one pack-file before the repack. It would be\n> good to have a non-kept packfile get repacked in the process, not\n> just the loose objects added by the test_commit. I'll take a look at\n> what can be done here.\n\nI think you are too good at nerd-sniping me ;-). Here's a more robust\ntest, that I think reads a little cleaner than the previous round. Let\nme know what you think:\n\n--- 8< ---\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 5922fb5bdd..1ed9a98a36 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -369,10 +369,36 @@ test_expect_success '--write-midx with preferred bitmap tips' '\n \t)\n '\n\n+packdir=$objdir/pack\n+\n test_expect_success '--write-midx -b packs non-kept objects' '\n-\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n-\t\tgit repack --write-midx -a -b &&\n-\ttest_subcommand_inexact git pack-objects --honor-pack-keep $packdir/$(basename $packdir/pack-*.pack .pack).keep &&\n+\n+\t\ttest_commit unkept &&\n+\t\tgit repack -d &&\n+\n+\t\ttest_commit new &&\n+\n+\t\tfind $packdir -type f -name \"pack-*.idx\" | sort >before &&\n+\t\tgit repack --write-midx -a -b -d &&\n+\t\tfind $packdir -type f -name \"pack-*.idx\" | sort >after &&\n+\n+\t\tgit rev-list --objects --no-object-names kept.. >expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\tgit show-index <$(comm -13 before after) >actual.raw &&\n+\t\tcut -d\" \" -f2 actual.raw >actual &&\n+\n+\t\ttest_cmp expect actual\n+\t)\n '\n\n test_expect_success TTY '--quiet disables progress' '\n\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"452127","messageId":"xmqqtubnl2zi.fsf@gitster.g","threadId":"57594","inReplyTo":"72c54461-8af7-29fc-04da-f435adee9bbe@github.com","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-24T16:38:57Z","receivedAt":"2022-03-24T16:39:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> The full post-condition check requires more test code, but that's not\n> really a problem. The problem comes in if that test is now too rigid\n> to future changes in that subcommand. What if the post-conditions\n> change in a subtle way because of the subcommand does something\n> differently, but in a way that is not of importance to the top\n> command?\n>\n> In this specific case, the test name says that it \"packs non-kept\n> objects\", so we can do more here to validate that post-condition\n> that we care about.\n\nThanks for clearly laying out the way to think about the issue. I\nagree with all of it, of course ;-)\n\n"},{"id":"452128","messageId":"4897c433-92a5-3bfb-ba9f-44000748fcc5@github.com","threadId":"57594","inReplyTo":"YjyWFqjkaGpC8NxQ@nand.local","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-24T16:39:59Z","receivedAt":"2022-03-24T16:40:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/24/2022 12:02 PM, Taylor Blau wrote:\n> On Thu, Mar 24, 2022 at 11:42:44AM -0400, Derrick Stolee wrote:\n>> As I'm looking at Taylor's test case example, the one thing I notice\n>> is that there is only one pack-file before the repack. It would be\n>> good to have a non-kept packfile get repacked in the process, not\n>> just the loose objects added by the test_commit. I'll take a look at\n>> what can be done here.\n> \n> I think you are too good at nerd-sniping me ;-). Here's a more robust\n> test, that I think reads a little cleaner than the previous round. Let\n> me know what you think:\n\nFinally, you found my most redeeming quality!\n\n> +\t\ttest_commit kept &&\n> +\t\tgit repack -ad &&\n> +\n> +\t\t>$packdir/$(basename $packdir/pack-*.pack .pack).keep &&\n> +\n> +\t\ttest_commit unkept &&\n> +\t\tgit repack -d &&\n> +\n> +\t\ttest_commit new &&\n> +\n> +\t\tfind $packdir -type f -name \"pack-*.idx\" | sort >before &&\n> +\t\tgit repack --write-midx -a -b -d &&\n> +\t\tfind $packdir -type f -name \"pack-*.idx\" | sort >after &&\n> +\n> +\t\tgit rev-list --objects --no-object-names kept.. >expect.raw &&\n> +\t\tsort expect.raw >expect &&\n\nThis is an interesting way to get this set of objects without storing\nthe original pack name. It might be good to keep consistent with the\nway we get the new objects, though.\n\n> +\n> +\t\tgit show-index <$(comm -13 before after) >actual.raw &&\n> +\t\tcut -d\" \" -f2 actual.raw >actual &&\n> +\n> +\t\ttest_cmp expect actual\n> +\t)\n\nI've got a modification of your original design prepared in my GGG PR\nand will send a v2 including it after it passes all of the builds.\n\nThanks,\n-Stolee\n"},{"id":"452204","messageId":"20220324181056.53824-1-chakrabortyabhradeep79@gmail.com","threadId":"57594","inReplyTo":"xmqqmthgu3e6.fsf@gitster.g","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-03-24T18:10:56Z","receivedAt":"2022-03-24T18:11:43Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Junio C Hamano wrote:\n\n> The result of doing #1 is still \"inexact\" but at that point it is\n> unclear if we are being way too inexact to be useful. If the\n> looseness bothers us too much, we may decide that #1 is not worth\n> doing. But obviously the looseness did not bother us that much\n> until last week, so probably an obvious #3, do nothing, letting the\n> sleeping dog lie, might be what we want to do?\n\nPersonally, I would prefer #3 i.e. do nothing (even in the future; unless\nit is removed all together). I also think that the current behaviour is not\n\"too inexact\". Rather it would be too strict for `test_subcommand_inexact`\nif we remove the \".*\" thing here.\n\nInexact means that the line needs not to be exactly same - there may be\nsome words in between the desired words (in this case, any flags that come\nbetween the desired sub-commands). The current behaviour (i.e. \n`local expr=$(printf '\"%s\".*' \"$@\")`) is justifying the name of the function.\nReplacing \".*\" with \",\" will therefore not work as the name of the function\nsuggests - it will rather work as `test_subcommand_starts_with`.\n\nOnly the `expr=${expr%,}.*\" line needs to be changed, I think.\n\nThanks :)\n\n"},{"id":"452208","messageId":"pull.1185.v2.git.1648146897.gitgitgadget@gmail.com","threadId":"57594","inReplyTo":"pull.1185.git.1647894845421.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] test-lib-functions: fix test_subcommand_inexact","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-24T18:34:55Z","receivedAt":"2022-03-24T18:35:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Junio discovered in [1] that test_subcommand_inexact is more flexible than\ninitially intended.\n\n[1] https://lore.kernel.org/git/xmqq4k41vdwe.fsf@gitster.g/\n\nThe intention was that we do not need to specify the remaining arguments for\na subcommand, but instead the current behavior is to allow the given\narguments to appear as any subsequence within the command (except that the\nfirst \"git\" instance must be the first argument).\n\nThis changes the helper to be more rigid.\n\n\nChanges in v2\n=============\n\n * After noticing that t7700 fails with the helper change, Taylor wrote a\n modification of the test to check the post-conditions directly and avoid\n the helper. This is included as Patch 1 with some modifications to be\n easier to read.\n\n * Patch 2 is the v1 patch with some change to the commit message about why\n the tests pass after patch 1.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n t7700: check post-condition in kept-pack test\n test-lib-functions: fix test_subcommand_inexact\n\n t/t7700-repack.sh | 52 ++++++++++++++++++++++++++++++++++++++---\n t/test-lib-functions.sh | 4 ++--\n 2 files changed, 51 insertions(+), 5 deletions(-)\n\n\nbase-commit: a68dfadae5e95c7f255cf38c9efdcbc2e36d1931\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1185%2Fderrickstolee%2Ftest-subcommand-fix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1185/derrickstolee/test-subcommand-fix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1185\n\nRange-diff vs v1:\n\n -: ----------- > 1: f2f8d12929b t7700: check post-condition in kept-pack test\n 1: 50176722bbf ! 2: ed67b748971 test-lib-functions: fix test_subcommand_inexact\n @@ Commit message\n the printf statement, then adding \".*\" after stripping out the trailing\n comma.\n \n - All existing tests continue to pass with this change, since none of them\n - were taking advantage of this unintended flexibility.\n + All existing tests continue to pass with this change. There was one\n + instance from t7700-repack.sh that was taking advantage of this\n + flexibility, but it was removed in the previous change.\n \n + Helped-by: Taylor Blau \n Signed-off-by: Derrick Stolee \n \n ## t/test-lib-functions.sh ##\n\n-- \ngitgitgadget\n"},{"id":"452209","messageId":"f2f8d12929bcbd630b2de3ce770a6763989ffcff.1648146897.git.gitgitgadget@gmail.com","threadId":"57594","inReplyTo":"pull.1185.v2.git.1648146897.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] t7700: check post-condition in kept-pack test","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-24T18:34:56Z","receivedAt":"2022-03-24T18:35:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe '--write-midx -b packs non-kept objects' test in t7700-repack.sh\nuses test_subcommand_inexact to check that 'git repack' properly adds\nthe '--honor-pack-keep' flag to the 'git pack-objects' subcommand.\nHowever, the test_subcommand_inexact helper is more flexible than\ninitially designed, and this instance is the only one that makes use of\nit: there are additional arguments between 'git pack-objects' and the\n'--honor-pack-keep' flag. In order to make test_subcommand_inexact more\nstrict, we need to fix this instance.\n\nThis test checks that 'git repack --write-midx -a -b -d' will create a\nnew pack-file that does not contain the objects within the kept pack.\nThis behavior is possible because of the multi-pack-index bitmap that\nwill bitmap objects against multiple packs. Without --write-midx, the\nobjects in the kept pack would be duplicated so the resulting pack is\nclosed under reachability and bitmaps can be created against it. This is\ndiscussed in more detail in e4d0c11c0 (repack: respect kept objects with\n'--write-midx -b', 2021-12-20) which also introduced this instance of\ntest_subcommand_inexact.\n\nTo better verify the intended post-conditions while also removing this\ninstance of test_subcommand_inexact, rewrite the test to check the list\nof packed objects in the kept pack and the list of the objects in the\nnewly-repacked pack-file _other_ than the kept pack. These lists should\nbe disjoint.\n\nBe sure to include a non-kept pack-file and loose objects to be extra\ncareful that this is properly behaving with kept packs and not just\navoiding repacking all pack-files.\n\nCo-authored-by: Taylor Blau \nSigned-off-by: Taylor Blau \nSigned-off-by: Derrick Stolee \n---\n t/t7700-repack.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 49 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 770d1432046..73452e23896 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -369,10 +369,56 @@ test_expect_success '--write-midx with preferred bitmap tips' '\n \t)\n '\n \n+get_sorted_objects_from_packs () {\n+\tgit show-index <$(cat) >raw &&\n+\tcut -d\" \" -f2 raw | sort\n+}\n+\n test_expect_success '--write-midx -b packs non-kept objects' '\n-\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n-\t\tgit repack --write-midx -a -b &&\n-\ttest_subcommand_inexact git pack-objects --honor-pack-keep before &&\n+\t\t>$objdir/pack/$(basename $(cat before) .idx).keep &&\n+\n+\t\t# Create a non-kept pack-file\n+\t\ttest_commit other &&\n+\t\tgit repack &&\n+\n+\t\t# Create loose objects\n+\t\ttest_commit loose &&\n+\n+\t\t# Repack everything\n+\t\tgit repack --write-midx -a -b -d &&\n+\n+\t\t# There should be two pack-files now, the\n+\t\t# old, kept pack and the new, non-kept pack.\n+\t\tfind $objdir/pack -name \"*.idx\" | sort >after &&\n+\t\ttest_line_count = 2 after &&\n+\t\tfind $objdir/pack -name \"*.keep\" >kept &&\n+\t\ttest_line_count = 1 kept &&\n+\n+\t\t# Get object list from the kept pack.\n+\t\tget_sorted_objects_from_packs \\\n+\t\t\told.objects &&\n+\n+\t\t# Get object list from the one non-kept pack-file\n+\t\tcomm -13 before after >new-pack &&\n+\t\tget_sorted_objects_from_packs \\\n+\t\t\tnew.objects &&\n+\n+\t\t# None of the objects in the new pack should\n+\t\t# exist within the kept pack.\n+\t\tcomm -12 old.objects new.objects >shared.objects &&\n+\t\ttest_must_be_empty shared.objects\n+\t)\n '\n \n test_expect_success TTY '--quiet disables progress' '\n-- \ngitgitgadget\n\n"},{"id":"452210","messageId":"ed67b7489719a01c88d7a6765e7499c1157da32e.1648146897.git.gitgitgadget@gmail.com","threadId":"57594","inReplyTo":"pull.1185.v2.git.1648146897.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] test-lib-functions: fix test_subcommand_inexact","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-24T18:34:57Z","receivedAt":"2022-03-24T18:35:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe implementation of test_subcommand_inexact() was originally\nintroduced in e4d0c11c0 (repack: respect kept objects with '--write-midx\n-b', 2021-12-20) with the intention to allow finding a subcommand based\non an initial set of arguments. The inexactness was intended as a way to\nallow flexible options beyond that initial set, as opposed to\ntest_subcommand() which requires that the full list of options is\nprovided in its entirety.\n\nThe implementation began by copying test_subcommand() and replaced the\nrepeated argument 'printf' statement to append \".*\" instead of \",\" to\neach argument. This has a few drawbacks:\n\n1. Most importantly, this repeats the use of \".*\" within 'expr', so the\n inexact match is even more flexible than expected. It allows the list\n of arguments to exist as a subsequence (with any other items included\n between those arguments).\n\n2. The line 'expr=\"$(expr%,}\"' that previously removed a trailing comma\n now no longer does anything, since the string ends with \".*\".\n\nBoth of these issues are fixed by keeping the addition of the comma in\nthe printf statement, then adding \".*\" after stripping out the trailing\ncomma.\n\nAll existing tests continue to pass with this change. There was one\ninstance from t7700-repack.sh that was taking advantage of this\nflexibility, but it was removed in the previous change.\n\nHelped-by: Taylor Blau \nSigned-off-by: Derrick Stolee \n---\n t/test-lib-functions.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 0f439c99d61..8f0e5da8727 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1811,8 +1811,8 @@ test_subcommand_inexact () {\n \t\tshift\n \tfi\n \n-\tlocal expr=$(printf '\"%s\".*' \"$@\")\n-\texpr=\"${expr%,}\"\n+\tlocal expr=$(printf '\"%s\",' \"$@\")\n+\texpr=\"${expr%,}.*\"\n \n \tif test -n \"$negate\"\n \tthen\n-- \ngitgitgadget\n"},{"id":"452213","messageId":"Yjy9QhGpd2C23ltg@nand.local","threadId":"57594","inReplyTo":"ed67b7489719a01c88d7a6765e7499c1157da32e.1648146897.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] test-lib-functions: fix test_subcommand_inexact","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-24T18:49:38Z","receivedAt":"2022-03-24T18:49:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 24, 2022 at 06:34:57PM +0000, Derrick Stolee via GitGitGadget wrote:\n> All existing tests continue to pass with this change. There was one\n> instance from t7700-repack.sh that was taking advantage of this\n> flexibility, but it was removed in the previous change.\n\nIn my tree, the test modified by the previous patch was the only caller\nof `test_subcommand_inexact()`. Looking through the output of:\n\n git for-each-ref refs/remotes/origin/ --format='%(refname)' | xargs -L1 \\\n git -P grep -l test_subcommand_inexact -- t | sort -u\n\nit doesn't look like there are any other topics in flight [1] that call\ntest_subcommand_inexact(), either.\n\nUnless you have any pending series which want to call\ntest_subcommand_inexact, I'd be just as happy to get rid of the function\nentirely.\n\nThanks,\nTaylor\n\n[1]: My `origin` points to Junio's tree, so I'm looking through all of\nthe topic branches before integration, not just the standard \"master\",\n\"next\", etc.\n"},{"id":"452214","messageId":"Yjy/UIydKw7v+4+7@nand.local","threadId":"57594","inReplyTo":"f2f8d12929bcbd630b2de3ce770a6763989ffcff.1648146897.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-24T18:58:24Z","receivedAt":"2022-03-24T18:58:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 24, 2022 at 06:34:56PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> The '--write-midx -b packs non-kept objects' test in t7700-repack.sh\n> uses test_subcommand_inexact to check that 'git repack' properly adds\n> the '--honor-pack-keep' flag to the 'git pack-objects' subcommand.\n> However, the test_subcommand_inexact helper is more flexible than\n> initially designed, and this instance is the only one that makes use of\n> it: there are additional arguments between 'git pack-objects' and the\n> '--honor-pack-keep' flag. In order to make test_subcommand_inexact more\n> strict, we need to fix this instance.\n>\n> This test checks that 'git repack --write-midx -a -b -d' will create a\n> new pack-file that does not contain the objects within the kept pack.\n> This behavior is possible because of the multi-pack-index bitmap that\n> will bitmap objects against multiple packs. Without --write-midx, the\n> objects in the kept pack would be duplicated so the resulting pack is\n> closed under reachability and bitmaps can be created against it. This is\n> discussed in more detail in e4d0c11c0 (repack: respect kept objects with\n> '--write-midx -b', 2021-12-20) which also introduced this instance of\n> test_subcommand_inexact.\n>\n> To better verify the intended post-conditions while also removing this\n> instance of test_subcommand_inexact, rewrite the test to check the list\n> of packed objects in the kept pack and the list of the objects in the\n> newly-repacked pack-file _other_ than the kept pack. These lists should\n> be disjoint.\n>\n> Be sure to include a non-kept pack-file and loose objects to be extra\n> careful that this is properly behaving with kept packs and not just\n> avoiding repacking all pack-files.\n\nNicely put.\n\n> Co-authored-by: Taylor Blau \n> Signed-off-by: Taylor Blau \n\nAnd to be clear, I am totally OK with this usage of my signed-off-by.\n\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 770d1432046..73452e23896 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -369,10 +369,56 @@ test_expect_success '--write-midx with preferred bitmap tips' '\n> \t)\n> '\n>\n> +get_sorted_objects_from_packs () {\n> +\tgit show-index <$(cat) >raw &&\n\nIt seems a little odd to me to pass the name of a single file as input\nto get_sorted_objects_from_packs over stdin. I probably would have\nexpected something like `git show-index <\"$1\" >raw && ...` instead.\n\nWe may also want to s/packs/pack, since this function only will handle\none index at a time.\n\n> +\tcut -d\" \" -f2 raw | sort\n\nHaving the sort in there is my fault, but after reading this more\ncarefully it's definitely unnecessary, since show-index will give us\nthe results in lexical order by object name already.\n\n> +}\n> +\n> test_expect_success '--write-midx -b packs non-kept objects' '\n> -\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n> -\t\tgit repack --write-midx -a -b &&\n> -\ttest_subcommand_inexact git pack-objects --honor-pack-keep +\tgit init repo &&\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\t# Create a kept pack-file\n> +\t\ttest_commit base &&\n> +\t\tgit repack -ad &&\n> +\t\tfind $objdir/pack -name \"*.idx\" >before &&\n\nI thought that here it might be easier to say:\n\n before=\"$(find $objdir/pack -name \"*.idx\")\"\n\n> +\t\t>$objdir/pack/$(basename $(cat before) .idx).keep &&\n\n...and then replace \"$(cat before)\" with \"$before\", along with the\nother uses of the before file below. But it gets a little funny when\nyou want to discover which is the new pack, where it is more natural to\ndump the output of comm into a file.\n\n> +\t\t# Get object list from the one non-kept pack-file\n> +\t\tcomm -13 before after >new-pack &&\n\nYou could write \"new_pack=$(comm -13 before after)\", but debugging this\ntest would be difficult if the output of comm there contained more than\none line.\n\n> +\t\tget_sorted_objects_from_packs \\\n> +\t\t\t writes:\n\n> From: Derrick Stolee \n>\n> The implementation of test_subcommand_inexact() was originally\n> introduced in e4d0c11c0 (repack: respect kept objects with '--write-midx\n> -b', 2021-12-20) with the intention to allow finding a subcommand based\n> on an initial set of arguments. The inexactness was intended as a way to\n> allow flexible options beyond that initial set, as opposed to\n> test_subcommand() which requires that the full list of options is\n> provided in its entirety.\n>\n> The implementation began by copying test_subcommand() and replaced the\n> repeated argument 'printf' statement to append \".*\" instead of \",\" to\n> each argument. This has a few drawbacks:\n>\n> 1. Most importantly, this repeats the use of \".*\" within 'expr', so the\n> inexact match is even more flexible than expected. It allows the list\n> of arguments to exist as a subsequence (with any other items included\n> between those arguments).\n>\n> 2. The line 'expr=\"$(expr%,}\"' that previously removed a trailing comma\n> now no longer does anything, since the string ends with \".*\".\n>\n> Both of these issues are fixed by keeping the addition of the comma in\n> the printf statement, then adding \".*\" after stripping out the trailing\n> comma.\n>\n> All existing tests continue to pass with this change. There was one\n> instance from t7700-repack.sh that was taking advantage of this\n> flexibility, but it was removed in the previous change.\n\nOf course all existing tests continue to pass, as we no longer have\nany user of test_subcommand_inexact after the previous step ;-).\n\nAmong\n\n (1) doing nothing,\n (2) removing, and\n (3) clarifying the implementation,\n\nmy preference would be 2 > 1 > 3. If we add\n\n (4) clarify the implementation and document what kind of inexactness we\n tolerate with an updated comment\"\n\nto the mix, that would come before all 3 others, though.\n\nPerhaps squash something like this in?\n\n t/test-lib-functions.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git i/t/test-lib-functions.sh w/t/test-lib-functions.sh\nindex 0f439c99d6..6f6afae847 100644\n--- i/t/test-lib-functions.sh\n+++ w/t/test-lib-functions.sh\n@@ -1789,8 +1789,8 @@ test_subcommand () {\n }\n \n # Check that the given command was invoked as part of the\n-# trace2-format trace on stdin, but without an exact set of\n-# arguments.\n+# trace2-format trace on stdin, but only require that the\n+# initial arguments are given as specified.\n #\n #\ttest_subcommand [!] ... < \n #\n"},{"id":"452241","messageId":"xmqqy20yeuri.fsf@gitster.g","threadId":"57594","inReplyTo":"20220324181056.53824-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-25T00:33:05Z","receivedAt":"2022-03-25T00:33:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhradeep Chakraborty writes:\n\n> Inexact means that the line needs not to be exactly same - there may be\n> some words in between the desired words (in this case, any flags that come\n> between the desired sub-commands). The current behaviour (i.e. \n> `local expr=$(printf '\"%s\".*' \"$@\")`) is justifying the name of the function.\n\nThe current name may justify what it does, but so does \"declare\nmatch randomly\" would. How much fuzziness we tolerate is something\nthat must be made clear to help developers who may potentially want\nto use it, and the phrase \"inexact\" alone would not help us.\n\n"},{"id":"452256","messageId":"20220325081314.1500-1-chakrabortyabhradeep79@gmail.com","threadId":"57594","inReplyTo":"xmqqy20yeuri.fsf@gitster.g","subject":"Re: [PATCH] test-lib-functions: fix test_subcommand_inexact","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-03-25T08:13:14Z","receivedAt":"2022-03-25T08:13:41Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Junio C Hamano wrote:\n\n> ... How much fuzziness we tolerate is something\n> that must be made clear to help developers who may potentially want\n> to use it, and the phrase \"inexact\" alone would not help us.\n\nYep! I agree.\n\nThanks :)\n\n"},{"id":"452262","messageId":"7ea0f7e4-180d-307d-2e2e-d33c3343317c@github.com","threadId":"57594","inReplyTo":"Yjy/UIydKw7v+4+7@nand.local","subject":"Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-25T13:55:21Z","receivedAt":"2022-03-25T13:55:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/24/2022 2:58 PM, Taylor Blau wrote:\n> On Thu, Mar 24, 2022 at 06:34:56PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n\n>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n>> index 770d1432046..73452e23896 100755\n>> --- a/t/t7700-repack.sh\n>> +++ b/t/t7700-repack.sh\n>> @@ -369,10 +369,56 @@ test_expect_success '--write-midx with preferred bitmap tips' '\n>> \t)\n>> '\n>>\n>> +get_sorted_objects_from_packs () {\n>> +\tgit show-index <$(cat) >raw &&\n> \n> It seems a little odd to me to pass the name of a single file as input\n> to get_sorted_objects_from_packs over stdin. I probably would have\n> expected something like `git show-index <\"$1\" >raw && ...` instead.\n\nBased on the way we are creating a file whose contents is the name\nof the .idx file, we would at least use '$(cat \"$1\")'. I kind of like\nthe symmetry of the input/output redirection when using the helper, but\nI can easily change this.\n\n> We may also want to s/packs/pack, since this function only will handle\n> one index at a time.\n\nYes.\n\n>> +\tcut -d\" \" -f2 raw | sort\n> \n> Having the sort in there is my fault, but after reading this more\n> carefully it's definitely unnecessary, since show-index will give us\n> the results in lexical order by object name already.\n\nCool. Will drop.\n\n>> +}\n>> +\n>> test_expect_success '--write-midx -b packs non-kept objects' '\n>> -\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n>> -\t\tgit repack --write-midx -a -b &&\n>> -\ttest_subcommand_inexact git pack-objects --honor-pack-keep > +\tgit init repo &&\n>> +\ttest_when_finished \"rm -fr repo\" &&\n>> +\t(\n>> +\t\tcd repo &&\n>> +\n>> +\t\t# Create a kept pack-file\n>> +\t\ttest_commit base &&\n>> +\t\tgit repack -ad &&\n>> +\t\tfind $objdir/pack -name \"*.idx\" >before &&\n> \n> I thought that here it might be easier to say:\n> \n> before=\"$(find $objdir/pack -name \"*.idx\")\"\n> \n>> +\t\t>$objdir/pack/$(basename $(cat before) .idx).keep &&\n> \n> ...and then replace \"$(cat before)\" with \"$before\", along with the\n> other uses of the before file below. But it gets a little funny when\n> you want to discover which is the new pack, where it is more natural to\n> dump the output of comm into a file.\n\nFor this reason, I'll continue to store the .idx names in files.\n\n>> +\t\t# Get object list from the one non-kept pack-file\n>> +\t\tcomm -13 before after >new-pack &&\n> \n> You could write \"new_pack=$(comm -13 before after)\", but debugging this\n> test would be difficult if the output of comm there contained more than\n> one line.\n>\n>> +\t\tget_sorted_objects_from_packs \\\n>> +\t\t\t \n> Though we probably want to check that we only get one line anyway here,\n> since get_sorted_objects_from_packs will barf if we had more than one\n> line in file new-pack here, too.\n\nThanks. Easy to add a test_line_count before this check.\n\n-Stolee\n"},{"id":"452263","messageId":"fc1db75c-868c-08c6-7087-b70460653460@github.com","threadId":"57594","inReplyTo":"xmqqtubnf568.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] test-lib-functions: fix test_subcommand_inexact","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-25T14:03:33Z","receivedAt":"2022-03-25T14:03:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/24/2022 4:48 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n\n>> All existing tests continue to pass with this change. There was one\n>> instance from t7700-repack.sh that was taking advantage of this\n>> flexibility, but it was removed in the previous change.\n> \n> Of course all existing tests continue to pass, as we no longer have\n> any user of test_subcommand_inexact after the previous step ;-).\n\nYeah, I definitely should have checked to see if there were other\nuses of this. I thought there was, but I was mistaken.\n\n> Among\n> \n> (1) doing nothing,\n> (2) removing, and\n> (3) clarifying the implementation,\n> \n> my preference would be 2 > 1 > 3. If we add\n\nI agree that (2) is the best option here.\n \n> (4) clarify the implementation and document what kind of inexactness we\n> tolerate with an updated comment\"\n> \n> to the mix, that would come before all 3 others, though.\n\nIs there value in fixing the implementation and adding this comment\nif we are to just delete the helper? I suppose that we could prevent\na future contribution from reintroducing the broken implementation.\n \n> Perhaps squash something like this in?\n> \n> t/test-lib-functions.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git i/t/test-lib-functions.sh w/t/test-lib-functions.sh\n> index 0f439c99d6..6f6afae847 100644\n> --- i/t/test-lib-functions.sh\n> +++ w/t/test-lib-functions.sh\n> @@ -1789,8 +1789,8 @@ test_subcommand () {\n> }\n> \n> # Check that the given command was invoked as part of the\n> -# trace2-format trace on stdin, but without an exact set of\n> -# arguments.\n> +# trace2-format trace on stdin, but only require that the\n> +# initial arguments are given as specified.\n\nThis is an accurate description of what the fixed implementation\ndoes.\n\nMy current feeling is that we should just delete this and refer\nto that deletion if anyone considers needing something like it.\n\nThanks,\n-Stolee\n"},{"id":"452277","messageId":"xmqqmthearlz.fsf@gitster.g","threadId":"57594","inReplyTo":"f2f8d12929bcbd630b2de3ce770a6763989ffcff.1648146897.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-25T17:07:04Z","receivedAt":"2022-03-25T17:07:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> +get_sorted_objects_from_packs () {\n> +\tgit show-index <$(cat) >raw &&\n> +\tcut -d\" \" -f2 raw | sort\n> +}\n\nAs pointed out by Taylor, this \"the standard input gives us the name\nof a file to be read\" looks strange. It may work, and it may even\ngive the easiest interface to use by all the callers, but if we were\ndesigning a more generic helper function suitable to be added to the\ntest-lib*.sh, we wouldn't design it like so---instead it would be\neither \"we read the contents of the .idx file from the standard\ninput\" or \"the first argument is the name of the .idx file\".\n\n> test_expect_success '--write-midx -b packs non-kept objects' '\n> +\tgit init repo &&\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\t# Create a kept pack-file\n> +\t\ttest_commit base &&\n> +\t\tgit repack -ad &&\n> +\t\tfind $objdir/pack -name \"*.idx\" >before &&\n> +\t\t>$objdir/pack/$(basename $(cat before) .idx).keep &&\n\nWe probably want to sanity check \"repack -a\" by insisting \"before\"\nhas found exactly one .idx file, before using it this way.\n\n\t\ttest_line_count = 1 before &&\n\t\tbefore=$(cat before) &&\n\t\t>\"${before%.idx}.keep\"\n\n> +\t\t# Create a non-kept pack-file\n> +\t\ttest_commit other &&\n> +\t\tgit repack &&\n> +\n> +\t\t# Create loose objects\n> +\t\ttest_commit loose &&\n> +\n> +\t\t# Repack everything\n> +\t\tgit repack --write-midx -a -b -d &&\n> +\n> +\t\t# There should be two pack-files now, the\n> +\t\t# old, kept pack and the new, non-kept pack.\n> +\t\tfind $objdir/pack -name \"*.idx\" | sort >after &&\n> +\t\ttest_line_count = 2 after &&\n\nOK. \"after\" gets sorted because we will pass it to \"comm\" later.\n\n> +\t\tfind $objdir/pack -name \"*.keep\" >kept &&\n> +\t\ttest_line_count = 1 kept &&\n\nSince we've made sure \"before\" is a one-liner earlier, we could just\nsay\n\n\t\ttest_cmp before kept &&\n\ninstead, no?\n\n> +\t\t# Get object list from the kept pack.\n> +\t\tget_sorted_objects_from_packs \\\n> +\t\t\t +\t\t\t>old.objects &&\n\t\t\nOK.\n\n> +\t\t# Get object list from the one non-kept pack-file\n> +\t\tcomm -13 before after >new-pack &&\n\nOK. This should give only one line of output but that is merely\nassumed. We know after has 2 and before has 1, but haven't made\nsure that before is a subset of after.\n\n\t\ttest_line_count = 1 new-pack &&\n\n> +\t\tget_sorted_objects_from_packs \\\n> +\t\t\t +\t\t\t>new.objects &&\n\nOK.\n\n> +\t\t# None of the objects in the new pack should\n> +\t\t# exist within the kept pack.\n> +\t\tcomm -12 old.objects new.objects >shared.objects &&\n\nGreat.\n\n> +\t\ttest_must_be_empty shared.objects\n> +\t)\n> '\n> \n> test_expect_success TTY '--quiet disables progress' '\n"},{"id":"452287","messageId":"3fab1246-1451-597c-4359-c01f9675e3f1@github.com","threadId":"57594","inReplyTo":"xmqqmthearlz.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-25T17:23:27Z","receivedAt":"2022-03-25T17:29:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/25/2022 1:07 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> +get_sorted_objects_from_packs () {\n>> +\tgit show-index <$(cat) >raw &&\n>> +\tcut -d\" \" -f2 raw | sort\n>> +}\n> \n> As pointed out by Taylor, this \"the standard input gives us the name\n> of a file to be read\" looks strange. It may work, and it may even\n> give the easiest interface to use by all the callers, but if we were\n> designing a more generic helper function suitable to be added to the\n> test-lib*.sh, we wouldn't design it like so---instead it would be\n> either \"we read the contents of the .idx file from the standard\n> input\" or \"the first argument is the name of the .idx file\".\n\nOk. Can do.\n \n>> test_expect_success '--write-midx -b packs non-kept objects' '\n>> +\tgit init repo &&\n>> +\ttest_when_finished \"rm -fr repo\" &&\n>> +\t(\n>> +\t\tcd repo &&\n>> +\n>> +\t\t# Create a kept pack-file\n>> +\t\ttest_commit base &&\n>> +\t\tgit repack -ad &&\n>> +\t\tfind $objdir/pack -name \"*.idx\" >before &&\n>> +\t\t>$objdir/pack/$(basename $(cat before) .idx).keep &&\n> \n> We probably want to sanity check \"repack -a\" by insisting \"before\"\n> has found exactly one .idx file, before using it this way.\n\n> \t\ttest_line_count = 1 before &&\n> \t\tbefore=$(cat before) &&\n> \t\t>\"${before%.idx}.keep\"\n\nGood idea. This mixture of a file and variable sharing\na name is a bit muddy for me, though. Using \"before_name\"\nas the variable would be enough of a differentiator.\n\n>> +\t\tfind $objdir/pack -name \"*.keep\" >kept &&\n>> +\t\ttest_line_count = 1 kept &&\n> \n> Since we've made sure \"before\" is a one-liner earlier, we could just\n> say\n \n> \t\ttest_cmp before kept &&\n> \n> instead, no?\n\n'before' contains a .idx name and 'kept' contains a .keep name,\nso this direct comparison does not work. We could do that\nadditional check like this:\n\n\tkept_name=$(cat kept) &&\n\techo ${kept_name%.keep}.idx >kept-idx &&\n\ttest_cmp before kept-idx &&\n\nThanks for taking the time to clean this up, as it might become\na good example for these kinds of post-condition checks of the packs\ndirectory in the future.\n\nThanks,\n-Stolee\n"},{"id":"452302","messageId":"Yj39lEzd/9AwQJaP@nand.local","threadId":"57594","inReplyTo":"3fab1246-1451-597c-4359-c01f9675e3f1@github.com","subject":"Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-25T17:36:20Z","receivedAt":"2022-03-25T17:41:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 25, 2022 at 01:23:27PM -0400, Derrick Stolee wrote:\n> > Since we've made sure \"before\" is a one-liner earlier, we could just\n> > say\n>\n> > \t\ttest_cmp before kept &&\n> >\n> > instead, no?\n>\n> 'before' contains a .idx name and 'kept' contains a .keep name,\n> so this direct comparison does not work. We could do that\n> additional check like this:\n>\n> \tkept_name=$(cat kept) &&\n> \techo ${kept_name%.keep}.idx >kept-idx &&\n> \ttest_cmp before kept-idx &&\n\nI think keeping this kind of post-condition check pretty minimal is\nfavorable, since this is functionality of `git repack` (i.e., that `-a`\nleaves you with one kept) that is already tested thoroughly elsewhere.\n\nSo I'd probably avoid checking the output altogether, but if we did want\nto test it, I think something as quick and cheap as:\n\n test_line_count = 1 before\n\nwould do the trick.\n\nThanks,\nTaylor\n"},{"id":"452306","messageId":"xmqqils2aqrc.fsf@gitster.g","threadId":"57594","inReplyTo":"fc1db75c-868c-08c6-7087-b70460653460@github.com","subject":"Re: [PATCH v2 2/2] test-lib-functions: fix test_subcommand_inexact","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-25T17:25:27Z","receivedAt":"2022-03-25T19:25:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 3/24/2022 4:48 PM, Junio C Hamano wrote:\n>> \"Derrick Stolee via GitGitGadget\" writes:\n>\n>>> All existing tests continue to pass with this change. There was one\n>>> instance from t7700-repack.sh that was taking advantage of this\n>>> flexibility, but it was removed in the previous change.\n>> \n>> Of course all existing tests continue to pass, as we no longer have\n>> any user of test_subcommand_inexact after the previous step ;-).\n>\n> Yeah, I definitely should have checked to see if there were other\n> uses of this. I thought there was, but I was mistaken.\n>\n>> Among\n>> \n>> (1) doing nothing,\n>> (2) removing, and\n>> (3) clarifying the implementation,\n>> \n>> my preference would be 2 > 1 > 3. If we add\n>\n> I agree that (2) is the best option here.\n> \n>> (4) clarify the implementation and document what kind of inexactness we\n>> tolerate with an updated comment\"\n>> \n>> to the mix, that would come before all 3 others, though.\n>\n> Is there value in fixing the implementation and adding this comment\n> if we are to just delete the helper? I suppose that we could prevent\n> a future contribution from reintroducing the broken implementation.\n\nThat is a good thoguth to take into account.\n\n> My current feeling is that we should just delete this and refer\n> to that deletion if anyone considers needing something like it.\n\nI am very much in favor of deleting it.\n\nThanks.\n"},{"id":"452309","messageId":"xmqq7d8hc2o2.fsf@gitster.g","threadId":"57594","inReplyTo":"Yj39lEzd/9AwQJaP@nand.local","subject":"Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-25T18:22:53Z","receivedAt":"2022-03-25T19:26:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Fri, Mar 25, 2022 at 01:23:27PM -0400, Derrick Stolee wrote:\n>> > Since we've made sure \"before\" is a one-liner earlier, we could just\n>> > say\n>>\n>> > \t\ttest_cmp before kept &&\n>> >\n>> > instead, no?\n>>\n>> 'before' contains a .idx name and 'kept' contains a .keep name,\n>> so this direct comparison does not work. We could do that\n>> additional check like this:\n>>\n>> \tkept_name=$(cat kept) &&\n>> \techo ${kept_name%.keep}.idx >kept-idx &&\n>> \ttest_cmp before kept-idx &&\n>\n> I think keeping this kind of post-condition check pretty minimal is\n> favorable, since this is functionality of `git repack` (i.e., that `-a`\n> leaves you with one kept) that is already tested thoroughly elsewhere.\n\nIt is nice in principle, but for this particular script, whose\nhelper function's implementation relies on these \"assumptions\" to\nwork correctly (e.g. imagine what happens when 'before' file had 0\nor 2 lines in it and you called the get_sorted_objects_from_packs\nhelper), we should be more defensive, and that is where all my\nsuggestions in the thread come from.\n"},{"id":"452315","messageId":"pull.1185.v3.git.1648234967.gitgitgadget@gmail.com","threadId":"57594","inReplyTo":"pull.1185.v2.git.1648146897.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] test-lib-functions: fix test_subcommand_inexact","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-25T19:02:45Z","receivedAt":"2022-03-25T19:27:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Junio discovered in [1] that test_subcommand_inexact is more flexible than\ninitially intended.\n\n[1] https://lore.kernel.org/git/xmqq4k41vdwe.fsf@gitster.g/\n\nThe intention was that we do not need to specify the remaining arguments for\na subcommand, but instead the current behavior is to allow the given\narguments to appear as any subsequence within the command (except that the\nfirst \"git\" instance must be the first argument).\n\nBy changing the test that needed the helper, we can avoid the helper in the\nfirst place. Modify the test and remove the helper.\n\n\nChanges in v3\n=============\n\n * Significant edits to the test in t7700 based on Junio and Taylor's\n feedback.\n\n * Patch 2 now deletes the helper as it is not used anywhere.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n t7700: check post-condition in kept-pack test\n test-lib-functions: remove test_subcommand_inexact\n\n t/t7700-repack.sh | 57 ++++++++++++++++++++++++++++++++++++++---\n t/test-lib-functions.sh | 34 ------------------------\n 2 files changed, 54 insertions(+), 37 deletions(-)\n\n\nbase-commit: a68dfadae5e95c7f255cf38c9efdcbc2e36d1931\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1185%2Fderrickstolee%2Ftest-subcommand-fix-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1185/derrickstolee/test-subcommand-fix-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1185\n\nRange-diff vs v2:\n\n 1: f2f8d12929b ! 1: fb2c550512e t7700: check post-condition in kept-pack test\n @@ t/t7700-repack.sh: test_expect_success '--write-midx with preferred bitmap tips'\n \t)\n '\n \n -+get_sorted_objects_from_packs () {\n -+\tgit show-index <$(cat) >raw &&\n -+\tcut -d\" \" -f2 raw | sort\n ++# The first argument is expected to be a filename\n ++# and that file should contain the name of a .idx\n ++# file. Send the list of objects in that .idx file\n ++# into stdout.\n ++get_sorted_objects_from_pack () {\n ++\tgit show-index <$(cat \"$1\") >raw &&\n ++\tcut -d\" \" -f2 raw\n +}\n +\n test_expect_success '--write-midx -b packs non-kept objects' '\n @@ t/t7700-repack.sh: test_expect_success '--write-midx with preferred bitmap tips'\n +\t\ttest_commit base &&\n +\t\tgit repack -ad &&\n +\t\tfind $objdir/pack -name \"*.idx\" >before &&\n -+\t\t>$objdir/pack/$(basename $(cat before) .idx).keep &&\n ++\t\ttest_line_count = 1 before &&\n ++\t\tbefore_name=$(cat before) &&\n ++\t\t>${before_name%.idx}.keep &&\n +\n +\t\t# Create a non-kept pack-file\n +\t\ttest_commit other &&\n @@ t/t7700-repack.sh: test_expect_success '--write-midx with preferred bitmap tips'\n +\t\tfind $objdir/pack -name \"*.idx\" | sort >after &&\n +\t\ttest_line_count = 2 after &&\n +\t\tfind $objdir/pack -name \"*.keep\" >kept &&\n -+\t\ttest_line_count = 1 kept &&\n ++\t\tkept_name=$(cat kept) &&\n ++\t\techo ${kept_name%.keep}.idx >kept-idx &&\n ++\t\ttest_cmp before kept-idx &&\n +\n +\t\t# Get object list from the kept pack.\n -+\t\tget_sorted_objects_from_packs \\\n -+\t\t\told.objects &&\n ++\t\tget_sorted_objects_from_pack before >old.objects &&\n +\n +\t\t# Get object list from the one non-kept pack-file\n +\t\tcomm -13 before after >new-pack &&\n -+\t\tget_sorted_objects_from_packs \\\n -+\t\t\tnew.objects &&\n ++\t\ttest_line_count = 1 new-pack &&\n ++\t\tget_sorted_objects_from_pack new-pack >new.objects &&\n +\n +\t\t# None of the objects in the new pack should\n +\t\t# exist within the kept pack.\n 2: ed67b748971 < -: ----------- test-lib-functions: fix test_subcommand_inexact\n -: ----------- > 2: f5a96a121a5 test-lib-functions: remove test_subcommand_inexact\n\n-- \ngitgitgadget\n"},{"id":"452316","messageId":"fb2c550512e1ab4dccee805db6121bc1d7e7d48f.1648234967.git.gitgitgadget@gmail.com","threadId":"57594","inReplyTo":"pull.1185.v3.git.1648234967.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] t7700: check post-condition in kept-pack test","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-25T19:02:46Z","receivedAt":"2022-03-25T19:27:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe '--write-midx -b packs non-kept objects' test in t7700-repack.sh\nuses test_subcommand_inexact to check that 'git repack' properly adds\nthe '--honor-pack-keep' flag to the 'git pack-objects' subcommand.\nHowever, the test_subcommand_inexact helper is more flexible than\ninitially designed, and this instance is the only one that makes use of\nit: there are additional arguments between 'git pack-objects' and the\n'--honor-pack-keep' flag. In order to make test_subcommand_inexact more\nstrict, we need to fix this instance.\n\nThis test checks that 'git repack --write-midx -a -b -d' will create a\nnew pack-file that does not contain the objects within the kept pack.\nThis behavior is possible because of the multi-pack-index bitmap that\nwill bitmap objects against multiple packs. Without --write-midx, the\nobjects in the kept pack would be duplicated so the resulting pack is\nclosed under reachability and bitmaps can be created against it. This is\ndiscussed in more detail in e4d0c11c0 (repack: respect kept objects with\n'--write-midx -b', 2021-12-20) which also introduced this instance of\ntest_subcommand_inexact.\n\nTo better verify the intended post-conditions while also removing this\ninstance of test_subcommand_inexact, rewrite the test to check the list\nof packed objects in the kept pack and the list of the objects in the\nnewly-repacked pack-file _other_ than the kept pack. These lists should\nbe disjoint.\n\nBe sure to include a non-kept pack-file and loose objects to be extra\ncareful that this is properly behaving with kept packs and not just\navoiding repacking all pack-files.\n\nCo-authored-by: Taylor Blau \nSigned-off-by: Taylor Blau \nSigned-off-by: Derrick Stolee \n---\n t/t7700-repack.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 54 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 770d1432046..ca45c4cd2c1 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -369,10 +369,61 @@ test_expect_success '--write-midx with preferred bitmap tips' '\n \t)\n '\n \n+# The first argument is expected to be a filename\n+# and that file should contain the name of a .idx\n+# file. Send the list of objects in that .idx file\n+# into stdout.\n+get_sorted_objects_from_pack () {\n+\tgit show-index <$(cat \"$1\") >raw &&\n+\tcut -d\" \" -f2 raw\n+}\n+\n test_expect_success '--write-midx -b packs non-kept objects' '\n-\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" \\\n-\t\tgit repack --write-midx -a -b &&\n-\ttest_subcommand_inexact git pack-objects --honor-pack-keep before &&\n+\t\ttest_line_count = 1 before &&\n+\t\tbefore_name=$(cat before) &&\n+\t\t>${before_name%.idx}.keep &&\n+\n+\t\t# Create a non-kept pack-file\n+\t\ttest_commit other &&\n+\t\tgit repack &&\n+\n+\t\t# Create loose objects\n+\t\ttest_commit loose &&\n+\n+\t\t# Repack everything\n+\t\tgit repack --write-midx -a -b -d &&\n+\n+\t\t# There should be two pack-files now, the\n+\t\t# old, kept pack and the new, non-kept pack.\n+\t\tfind $objdir/pack -name \"*.idx\" | sort >after &&\n+\t\ttest_line_count = 2 after &&\n+\t\tfind $objdir/pack -name \"*.keep\" >kept &&\n+\t\tkept_name=$(cat kept) &&\n+\t\techo ${kept_name%.keep}.idx >kept-idx &&\n+\t\ttest_cmp before kept-idx &&\n+\n+\t\t# Get object list from the kept pack.\n+\t\tget_sorted_objects_from_pack before >old.objects &&\n+\n+\t\t# Get object list from the one non-kept pack-file\n+\t\tcomm -13 before after >new-pack &&\n+\t\ttest_line_count = 1 new-pack &&\n+\t\tget_sorted_objects_from_pack new-pack >new.objects &&\n+\n+\t\t# None of the objects in the new pack should\n+\t\t# exist within the kept pack.\n+\t\tcomm -12 old.objects new.objects >shared.objects &&\n+\t\ttest_must_be_empty shared.objects\n+\t)\n '\n \n test_expect_success TTY '--quiet disables progress' '\n-- \ngitgitgadget\n\n"},{"id":"452317","messageId":"f5a96a121a56b7d04cbd8fa20ab9d79e8d811115.1648234967.git.gitgitgadget@gmail.com","threadId":"57594","inReplyTo":"pull.1185.v3.git.1648234967.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] test-lib-functions: remove test_subcommand_inexact","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-25T19:02:47Z","receivedAt":"2022-03-25T19:27:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe implementation of test_subcommand_inexact() was originally\nintroduced in e4d0c11c0 (repack: respect kept objects with '--write-midx\n-b', 2021-12-20) with the intention to allow finding a subcommand based\non an initial set of arguments. The inexactness was intended as a way to\nallow flexible options beyond that initial set, as opposed to\ntest_subcommand() which requires that the full list of options is\nprovided in its entirety.\n\nThe implementation began by copying test_subcommand() and replaced the\nrepeated argument 'printf' statement to append \".*\" instead of \",\" to\neach argument. This caused it to be more flexible than initially\nintended.\n\nThe previous change deleted the only use of test_subcommand_inexact, so\ninstead of editing the helper, delete it.\n\nHelped-by: Taylor Blau \nSigned-off-by: Derrick Stolee \n---\n t/test-lib-functions.sh | 34 ----------------------------------\n 1 file changed, 34 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 0f439c99d61..2501fc5706f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1788,40 +1788,6 @@ test_subcommand () {\n \tfi\n }\n \n-# Check that the given command was invoked as part of the\n-# trace2-format trace on stdin, but without an exact set of\n-# arguments.\n-#\n-#\ttest_subcommand [!] ... < \n-#\n-# For example, to look for an invocation of \"git pack-objects\"\n-# with the \"--honor-pack-keep\" argument, use\n-#\n-#\tGIT_TRACE2_EVENT=event.log git repack ... &&\n-#\ttest_subcommand git pack-objects --honor-pack-keep Junio discovered in [1] that test_subcommand_inexact is more flexible than\n> initially intended.\n>\n> [1] https://lore.kernel.org/git/xmqq4k41vdwe.fsf@gitster.g/\n>\n> The intention was that we do not need to specify the remaining arguments for\n> a subcommand, but instead the current behavior is to allow the given\n> arguments to appear as any subsequence within the command (except that the\n> first \"git\" instance must be the first argument).\n>\n> By changing the test that needed the helper, we can avoid the helper in the\n> first place. Modify the test and remove the helper.\n\nI reviewed both patches carefully, and this version looks great to me.\nThanks for all of your patience in cleaning up that sketch of the new\ntest; you version in v3 here is delightfully easy to follow.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"}]}