{"thread":{"id":"56907","subject":"[PATCH 0/2] test-lib: improve missing prereq handling","startedAt":"2021-11-15T16:08:25Z","lastAt":"2021-11-17T09:05:32Z","messageCount":11,"participants":["Fabian Stelzer","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"441144","messageId":"20211115160750.1208940-1-fs@gigacodes.de","threadId":"56907","inReplyTo":null,"subject":"[PATCH 0/2] test-lib: improve missing prereq handling","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-15T16:07:48Z","receivedAt":"2021-11-15T16:08:25Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"The ssh signing feature was breaking tests when the broken openssh-8.7\nwas used. We have now fixed that by checking for this exact case in the\nGPGSSH prereq and I will improve that check further in a future patch.\nHowever we are now in a situation where a broken openssh in the future\nwill result in successfull tests but not a working git build afterwards\n(either not compiling in the expected feature or like in the ssh case\nruntime failures) resulting in a false sense of security in the tests.\nThis patches try to improve this situation by showing which prereqs\nfailed in the test summary and by adding an environment variable to\nenforce certain prereqs to succeed or abort the test otherwise.\n\nSee also:\nhttps://public-inbox.org/git/xmqqv916wh7t.fsf@gitster.g/\n\nFabian Stelzer (2):\n  test-lib: show missing prereq summary\n  test-lib: introduce required prereq for test runs\n\n t/README                |  6 ++++++\n t/aggregate-results.sh  | 17 +++++++++++++++++\n t/test-lib-functions.sh | 11 +++++++++++\n t/test-lib.sh           | 11 +++++++++++\n 4 files changed, 45 insertions(+)\n\n\nbase-commit: 5a73c6bdc717127c2da99f57bc630c4efd8aed02\n-- \n2.31.1\n\n"},{"id":"441145","messageId":"20211115160750.1208940-2-fs@gigacodes.de","threadId":"56907","inReplyTo":"20211115160750.1208940-1-fs@gigacodes.de","subject":"[PATCH 1/2] test-lib: show missing prereq summary","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-15T16:07:49Z","receivedAt":"2021-11-15T16:08:40Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"When running the full test suite many tests can be skipped because of\nmissing prerequisites. It not easy right now to get an overview of which\nones are missing.\nWhen switching to a new machine or environment some libraries and tools\nmight be missing or maybe a dependency broke completely. In this case\nthe tests would indicate nothing since all dependant tests are simply\nskipped. This could hide broken behaviour or missing features in the\nbuild. Therefore this patch summarizes the missing prereqs at the end of\nthe test run making it easier to spot such cases.\n\n - Add failed prereqs to the test results.\n - Aggregate and then show them with the totals.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/aggregate-results.sh | 17 +++++++++++++++++\n t/test-lib.sh          | 11 +++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\nindex 7913e206ed..87c16fcee1 100755\n--- a/t/aggregate-results.sh\n+++ b/t/aggregate-results.sh\n@@ -6,6 +6,7 @@ success=0\n failed=0\n broken=0\n total=0\n+missing_prereq=\n \n while read file\n do\n@@ -30,10 +31,26 @@ do\n \t\t\tbroken=$(($broken + $value)) ;;\n \t\ttotal)\n \t\t\ttotal=$(($total + $value)) ;;\n+\t\tmissing_prereq)\n+\t\t\tmissing_prereq=\"$missing_prereq,$value\" ;;\n \t\tesac\n \tdone <\"$file\"\n done\n \n+if test -n \"$missing_prereq\"\n+then\n+\tunique_missing_prereq=$(\n+\t\techo $missing_prereq |\n+\t\ttr -s \",\" \"\\n\" |\n+\t\tgrep -v '^$' |\n+\t\tsort -u |\n+\t\tpaste -s -d ',')\n+\tif test -n $unique_missing_prereq\n+\tthen\n+\t\tprintf \"\\nmissing prereq: $unique_missing_prereq\\n\\n\"\n+\tfi\n+fi\n+\n if test -n \"$failed_tests\"\n then\n \tprintf \"\\nfailed test(s):$failed_tests\\n\\n\"\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 2679a7596a..f61da562f6 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -669,6 +669,8 @@ test_fixed=0\n test_broken=0\n test_success=0\n \n+test_missing_prereq=\n+\n test_external_has_tap=0\n \n die () {\n@@ -1069,6 +1071,14 @@ test_skip () {\n \t\t\tof_prereq=\" of $test_prereq\"\n \t\tfi\n \t\tskipped_reason=\"missing $missing_prereq${of_prereq}\"\n+\n+\t\t# Keep a list of all the missing prereq for result aggregation\n+\t\tif test -z \"$missing_prereq\"\n+\t\tthen\n+\t\t\ttest_missing_prereq=$missing_prereq\n+\t\telse\n+\t\t\ttest_missing_prereq=\"$test_missing_prereq,$missing_prereq\"\n+\t\tfi\n \tfi\n \n \tcase \"$to_skip\" in\n@@ -1175,6 +1185,7 @@ test_done () {\n \t\tfixed $test_fixed\n \t\tbroken $test_broken\n \t\tfailed $test_failure\n+\t\tmissing_prereq $test_missing_prereq\n \n \t\tEOF\n \tfi\n-- \n2.31.1\n\n"},{"id":"441146","messageId":"20211115160750.1208940-3-fs@gigacodes.de","threadId":"56907","inReplyTo":"20211115160750.1208940-1-fs@gigacodes.de","subject":"[PATCH 2/2] test-lib: introduce required prereq for test runs","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-15T16:07:50Z","receivedAt":"2021-11-15T16:08:46Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"In certain environments or for specific test scenarios we might expect a\nspecific prerequisite check to be succeed. Therefore we would like to\ntrigger an error when running our tests if this is not the case.\n\nTo remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\nwhich can be set to a comma separated list of prereqs. If one of these\nprereq tests fail then the whole test run will abort.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/README                |  6 ++++++\n t/test-lib-functions.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 29f72354bf..18ce75976e 100644\n--- a/t/README\n+++ b/t/README\n@@ -466,6 +466,12 @@ explicitly providing repositories when accessing submodule objects is\n complete or needs to be abandoned for whatever reason (in which case the\n migrated codepaths still retain their performance benefits).\n \n+GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a comma speparated list of\n+prereqs that are required to succeed. If a prereq in this list is triggered by\n+a test and then fails then the whole test run will abort. This can help to make\n+sure the expected tests are executed and not silently skipped when their\n+dependency breaks or is simply not present in a new environment.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex eef2262a36..2c8abf3420 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -680,6 +680,17 @@ test_have_prereq () {\n \t\t\t# Keep a list of missing prerequisites; restore\n \t\t\t# the negative marker if necessary.\n \t\t\tprerequisite=${negative_prereq:+!}$prerequisite\n+\n+\t\t\t# Abort if this prereq was marked as required\n+\t\t\tif test -n $GIT_TEST_REQUIRE_PREREQ\n+\t\t\tthen\n+\t\t\t\tcase \",$GIT_TEST_REQUIRE_PREREQ,\" in\n+\t\t\t\t*,$prerequisite,*)\n+\t\t\t\t\terror \"required prereq $prerequisite failed\"\n+\t\t\t\t\t;;\n+\t\t\t\tesac\n+\t\t\tfi\n+\n \t\t\tif test -z \"$missing_prereq\"\n \t\t\tthen\n \t\t\t\tmissing_prereq=$prerequisite\n-- \n2.31.1\n\n"},{"id":"441151","messageId":"xmqq8rxpfgqh.fsf@gitster.g","threadId":"56907","inReplyTo":"211115.865ysts45o.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] test-lib: show missing prereq summary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-15T17:53:10Z","receivedAt":"2021-11-15T17:54:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> +if test -n \"$missing_prereq\"\n>> +then\n>> +\tunique_missing_prereq=$(\n>> +\t\techo $missing_prereq |\n>> +\t\ttr -s \",\" \"\\n\" |\n>> +\t\tgrep -v '^$' |\n>> +\t\tsort -u |\n>> +\t\tpaste -s -d ',')\n>\n> What is paste? Some out-of-tree debugging utility?\n\nhttps://pubs.opengroup.org/onlinepubs/9699919799/utilities/paste.html\n\nDon't feel bad for not knowing it.  I usually do not use cut or paste\nmyself and I had to look it up the other day while reviewing the RFC\nphase of this series.\n\nI am not sure '\\n' is a good idea from portability perspective.  I\nthought I wrote '\\012' in the illustration in my review?\n"},{"id":"441158","messageId":"20211115195609.g6vq6qfjhyootcqt@fs","threadId":"56907","inReplyTo":"xmqq8rxpfgqh.fsf@gitster.g","subject":"Re: [PATCH 1/2] test-lib: show missing prereq summary","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-15T19:56:09Z","receivedAt":"2021-11-15T20:34:35Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 15.11.2021 09:53, Junio C Hamano wrote:\n>Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>> +if test -n \"$missing_prereq\"\n>>> +then\n>>> +\tunique_missing_prereq=$(\n>>> +\t\techo $missing_prereq |\n>>> +\t\ttr -s \",\" \"\\n\" |\n>>> +\t\tgrep -v '^$' |\n>>> +\t\tsort -u |\n>>> +\t\tpaste -s -d ',')\n>>\n>> What is paste? Some out-of-tree debugging utility?\n>\n>https://pubs.opengroup.org/onlinepubs/9699919799/utilities/paste.html\n>\n>Don't feel bad for not knowing it.  I usually do not use cut or paste\n>myself and I had to look it up the other day while reviewing the RFC\n>phase of this series.\n>\n>I am not sure '\\n' is a good idea from portability perspective.  I\n>thought I wrote '\\012' in the illustration in my review?\n\nYes, i was wondering why you did that. When i played around with your\nvariant i used \\n since it's what i commonly use and find more readable.\nAnd i'm by far no expert on partability. What platforms would have an\nissue with \\n ?\n\nI'll take a look at Ævars generate-cmdlist code and will use \\012 in a\nreroll.\n\nThanks\n"},{"id":"441166","messageId":"xmqqk0h9cboc.fsf@gitster.g","threadId":"56907","inReplyTo":"20211115195609.g6vq6qfjhyootcqt@fs","subject":"Re: [PATCH 1/2] test-lib: show missing prereq summary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-15T22:10:43Z","receivedAt":"2021-11-15T22:13:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer <fs@gigacodes.de> writes:\n\n>>I am not sure '\\n' is a good idea from portability perspective.  I\n>>thought I wrote '\\012' in the illustration in my review?\n>\n> Yes, i was wondering why you did that. When i played around with your\n> variant i used \\n since it's what i commonly use and find more readable.\n> And i'm by far no expert on partability. What platforms would have an\n> issue with \\n ?\n\nI think I misremembered.  b3b753b1 (Fit to Plan 9's ANSI/POSIX\ncompatibility layer, 2020-09-10) does talk about a system whose\n\"tr\" does not fully emulate POSIX and wants an octal, but that\nis not a platform we target for to begin with.\n\n$ git grep '^[      ]*tr .*\\\\012['\\''\"]'\n$ git grep '^[      ]*tr .*\\\\n['\\''\"]'\n\nshow the same number of hits, even back in v2.0.0.  You have to go\nback to v1.6.0 (which I consider is the oldest and still usable\nrelease of significance) to see the source tree without any hit for\nthe latter.  The first introduction of tr \"\\n\" (which I consider is\na mistake---if we write octal, we do not have to worry about anybody\nnot supporting it) seems to be dea4562b (rerere forget path: forget\nrecorded resolution, 2009-12-25) made by me X-<.\n\n"},{"id":"441221","messageId":"211115.865ysts45o.gmgdl@evledraar.gmail.com","threadId":"56907","inReplyTo":"20211115160750.1208940-2-fs@gigacodes.de","subject":"Re: [PATCH 1/2] test-lib: show missing prereq summary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-15T17:44:14Z","receivedAt":"2021-11-16T01:33:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 15 2021, Fabian Stelzer wrote:\n\n> When running the full test suite many tests can be skipped because of\n> missing prerequisites. It not easy right now to get an overview of which\n> ones are missing.\n> When switching to a new machine or environment some libraries and tools\n> might be missing or maybe a dependency broke completely. In this case\n> the tests would indicate nothing since all dependant tests are simply\n> skipped. This could hide broken behaviour or missing features in the\n> build. Therefore this patch summarizes the missing prereqs at the end of\n> the test run making it easier to spot such cases.\n>\n>  - Add failed prereqs to the test results.\n>  - Aggregate and then show them with the totals.\n>\n> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>\n> ---\n>  t/aggregate-results.sh | 17 +++++++++++++++++\n>  t/test-lib.sh          | 11 +++++++++++\n>  2 files changed, 28 insertions(+)\n>\n> diff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\n> index 7913e206ed..87c16fcee1 100755\n> --- a/t/aggregate-results.sh\n> +++ b/t/aggregate-results.sh\n> @@ -6,6 +6,7 @@ success=0\n>  failed=0\n>  broken=0\n>  total=0\n> +missing_prereq=\n>  \n>  while read file\n>  do\n> @@ -30,10 +31,26 @@ do\n>  \t\t\tbroken=$(($broken + $value)) ;;\n>  \t\ttotal)\n>  \t\t\ttotal=$(($total + $value)) ;;\n> +\t\tmissing_prereq)\n> +\t\t\tmissing_prereq=\"$missing_prereq,$value\" ;;\n>  \t\tesac\n>  \tdone <\"$file\"\n>  done\n>  \n> +if test -n \"$missing_prereq\"\n> +then\n> +\tunique_missing_prereq=$(\n> +\t\techo $missing_prereq |\n> +\t\ttr -s \",\" \"\\n\" |\n> +\t\tgrep -v '^$' |\n> +\t\tsort -u |\n> +\t\tpaste -s -d ',')\n\nWhat is paste? Some out-of-tree debugging utility?\n\nI think you might find a better way to do this shown in my\n\"ab/generate-command-list\" topic, currently in seen. It removed most of\nthe same sort of tr|grep|sort etc. chain in generate-cmdlist.sh.\n"},{"id":"441256","messageId":"xmqqpmr0bpiz.fsf@gitster.g","threadId":"56907","inReplyTo":"20211115160750.1208940-3-fs@gigacodes.de","subject":"Re: [PATCH 2/2] test-lib: introduce required prereq for test runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-16T06:09:08Z","receivedAt":"2021-11-16T06:09:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer <fs@gigacodes.de> writes:\n\n> In certain environments or for specific test scenarios we might expect a\n> specific prerequisite check to be succeed. Therefore we would like to\n\n\"to be succeed\" -> \"to succeed\"?\n\n> trigger an error when running our tests if this is not the case.\n>\n> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\n> which can be set to a comma separated list of prereqs. If one of these\n> prereq tests fail then the whole test run will abort.\n>\n> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>\n> ---\n>  t/README                |  6 ++++++\n>  t/test-lib-functions.sh | 11 +++++++++++\n>  2 files changed, 17 insertions(+)\n>\n> diff --git a/t/README b/t/README\n> index 29f72354bf..18ce75976e 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -466,6 +466,12 @@ explicitly providing repositories when accessing submodule objects is\n>  complete or needs to be abandoned for whatever reason (in which case the\n>  migrated codepaths still retain their performance benefits).\n>  \n> +GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a comma speparated list of\n> +prereqs that are required to succeed. If a prereq in this list is triggered by\n> +a test and then fails then the whole test run will abort. This can help to make\n> +sure the expected tests are executed and not silently skipped when their\n> +dependency breaks or is simply not present in a new environment.\n> +\n>  Naming Tests\n>  ------------\n>  \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index eef2262a36..2c8abf3420 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -680,6 +680,17 @@ test_have_prereq () {\n>  \t\t\t# Keep a list of missing prerequisites; restore\n>  \t\t\t# the negative marker if necessary.\n>  \t\t\tprerequisite=${negative_prereq:+!}$prerequisite\n> +\n> +\t\t\t# Abort if this prereq was marked as required\n> +\t\t\tif test -n $GIT_TEST_REQUIRE_PREREQ\n> +\t\t\tthen\n> +\t\t\t\tcase \",$GIT_TEST_REQUIRE_PREREQ,\" in\n> +\t\t\t\t*,$prerequisite,*)\n> +\t\t\t\t\terror \"required prereq $prerequisite failed\"\n> +\t\t\t\t\t;;\n> +\t\t\t\tesac\n> +\t\t\tfi\n> +\n>  \t\t\tif test -z \"$missing_prereq\"\n>  \t\t\tthen\n>  \t\t\t\tmissing_prereq=$prerequisite\n"},{"id":"441327","messageId":"20211116141938.gbaufny5o2boptvh@fs","threadId":"56907","inReplyTo":"211115.865ysts45o.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] test-lib: show missing prereq summary","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-16T14:19:38Z","receivedAt":"2021-11-16T14:20:36Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 15.11.2021 18:44, Ævar Arnfjörð Bjarmason wrote:\n>\n>On Mon, Nov 15 2021, Fabian Stelzer wrote:\n>\n>> +if test -n \"$missing_prereq\"\n>> +then\n>> +\tunique_missing_prereq=$(\n>> +\t\techo $missing_prereq |\n>> +\t\ttr -s \",\" \"\\n\" |\n>> +\t\tgrep -v '^$' |\n>> +\t\tsort -u |\n>> +\t\tpaste -s -d ',')\n>\n>What is paste? Some out-of-tree debugging utility?\n>\n>I think you might find a better way to do this shown in my\n>\"ab/generate-command-list\" topic, currently in seen. It removed most of\n>the same sort of tr|grep|sort etc. chain in generate-cmdlist.sh.\n\nI've looked at the generate-command-list code and TBH i still think this\nis a better solution. If I read your change correctly you've removed the\nsort and unique completely since it was not necessary for the use-case.\nIn this case i think it is. Since we call tr with `-s` the grep -v might\nnot be strictly necessary though. Also in this case these commands are\nonly called once at the end of the test run and not in any kind of loop\nlike in the cmdlist code so i think this variant is much easier to read\nand debug with a negligible performance impact.\n\nI tried writing a sh only variant and this is what i came up with. Not\nsure if this could be much more simplified. It looses the sort though.\n\ninput=\"PCRE,JGIT2,JGIT2,,PCRE,JGIT2,PCRE,PCRE2,!PCRE,!WINDOWS,GPG,GPGSSH,PCRE,!GPG,GPG,JGIT2\"\n\nunique=\nsave_IFS=$IFS\nIFS=,\nfor prereq in $input\ndo\n\tcase \"$prereq\" in\n\t'')\n\t\t# Skip empty entries\n\t\t;;\n\t*)\n\t\tcase \",$unique,\" in\n\t\t*,$prereq,*)\n\t\t\t# Skip over duplicates\n\t\t\t;;\n\t\t*)\n\t\t\tif test -z \"$unique\"\n\t\t\tthen\n\t\t\t\tunique=\"$prereq\"\n\t\t\telse\n\t\t\t\tunique=\"$unique,$prereq\"\n\t\t\tfi\n\t\t\t;;\n\t\tesac\n\tesac\ndone\nIFS=$save_IFS\necho $unique\n\n"},{"id":"441412","messageId":"xmqqpmqz42kj.fsf@gitster.g","threadId":"56907","inReplyTo":"20211116141938.gbaufny5o2boptvh@fs","subject":"Re: [PATCH 1/2] test-lib: show missing prereq summary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-17T08:19:08Z","receivedAt":"2021-11-17T08:19:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer <fs@gigacodes.de> writes:\n\n> On 15.11.2021 18:44, Ævar Arnfjörð Bjarmason wrote:\n>>\n>>On Mon, Nov 15 2021, Fabian Stelzer wrote:\n>>\n>>> +if test -n \"$missing_prereq\"\n>>> +then\n>>> +\tunique_missing_prereq=$(\n>>> +\t\techo $missing_prereq |\n>>> +\t\ttr -s \",\" \"\\n\" |\n>>> +\t\tgrep -v '^$' |\n>>> +\t\tsort -u |\n>>> +\t\tpaste -s -d ',')\n>>\n>>What is paste? Some out-of-tree debugging utility?\n>>\n>>I think you might find a better way to do this shown in my\n>>\"ab/generate-command-list\" topic, currently in seen. It removed most of\n>>the same sort of tr|grep|sort etc. chain in generate-cmdlist.sh.\n>\n> I've looked at the generate-command-list code and TBH i still think this\n> is a better solution. If I read your change correctly you've removed the\n> sort and unique completely since it was not necessary for the use-case.\n> In this case i think it is. Since we call tr with `-s` the grep -v might\n> not be strictly necessary though. Also in this case these commands are\n> only called once at the end of the test run and not in any kind of loop\n> like in the cmdlist code so i think this variant is much easier to read\n> and debug with a negligible performance impact.\n\nI tend to agree with you.  The snippet we see above is quite\nstraight-forward not over-engineered.\n\n> I tried writing a sh only variant and this is what i came up with. Not\n> sure if this could be much more simplified. It looses the sort though.\n\nFun, but I'd rather not go there, unless this is a performance\ncritical bit, which it is not.\n\nThanks.\n\n\n>\n> input=\"PCRE,JGIT2,JGIT2,,PCRE,JGIT2,PCRE,PCRE2,!PCRE,!WINDOWS,GPG,GPGSSH,PCRE,!GPG,GPG,JGIT2\"\n>\n> unique=\n> save_IFS=$IFS\n> IFS=,\n> for prereq in $input\n> do\n> \tcase \"$prereq\" in\n> \t'')\n> \t\t# Skip empty entries\n> \t\t;;\n> \t*)\n> \t\tcase \",$unique,\" in\n> \t\t*,$prereq,*)\n> \t\t\t# Skip over duplicates\n> \t\t\t;;\n> \t\t*)\n> \t\t\tif test -z \"$unique\"\n> \t\t\tthen\n> \t\t\t\tunique=\"$prereq\"\n> \t\t\telse\n> \t\t\t\tunique=\"$unique,$prereq\"\n> \t\t\tfi\n> \t\t\t;;\n> \t\tesac\n> \tesac\n> done\n> IFS=$save_IFS\n> echo $unique\n"},{"id":"441421","messageId":"20211117090526.ldy5eonhbdgo2nyo@fs","threadId":"56907","inReplyTo":"xmqqpmqz42kj.fsf@gitster.g","subject":"Re: [PATCH 1/2] test-lib: show missing prereq summary","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-17T09:05:26Z","receivedAt":"2021-11-17T09:05:32Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 17.11.2021 00:19, Junio C Hamano wrote:\n>Fabian Stelzer <fs@gigacodes.de> writes:\n>\n>> On 15.11.2021 18:44, Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>>On Mon, Nov 15 2021, Fabian Stelzer wrote:\n>>>\n>>>> +if test -n \"$missing_prereq\"\n>>>> +then\n>>>> +\tunique_missing_prereq=$(\n>>>> +\t\techo $missing_prereq |\n>>>> +\t\ttr -s \",\" \"\\n\" |\n>>>> +\t\tgrep -v '^$' |\n>>>> +\t\tsort -u |\n>>>> +\t\tpaste -s -d ',')\n>>>\n>>>What is paste? Some out-of-tree debugging utility?\n>>>\n>>>I think you might find a better way to do this shown in my\n>>>\"ab/generate-command-list\" topic, currently in seen. It removed most of\n>>>the same sort of tr|grep|sort etc. chain in generate-cmdlist.sh.\n>>\n>> I've looked at the generate-command-list code and TBH i still think this\n>> is a better solution. If I read your change correctly you've removed the\n>> sort and unique completely since it was not necessary for the use-case.\n>> In this case i think it is. Since we call tr with `-s` the grep -v might\n>> not be strictly necessary though. Also in this case these commands are\n>> only called once at the end of the test run and not in any kind of loop\n>> like in the cmdlist code so i think this variant is much easier to read\n>> and debug with a negligible performance impact.\n>\n>I tend to agree with you.  The snippet we see above is quite\n>straight-forward not over-engineered.\n>\n>> I tried writing a sh only variant and this is what i came up with. Not\n>> sure if this could be much more simplified. It looses the sort though.\n>\n>Fun, but I'd rather not go there, unless this is a performance\n>critical bit, which it is not.\n>\n>Thanks.\n>\n\nOk, i've sent an update fixing the commit msg typo and with \\012 instead\nof \\n. Even if might not be an issue it won't hurt.\n\nThanks\n"}]}