{"thread":{"id":"52474","subject":"[PATCH 00/15] t: replace incorrect test_must_fail usage (part 1)","startedAt":"2019-12-17T12:01:45Z","lastAt":"2019-12-20T18:14:55Z","messageCount":64,"participants":["Denton Liu","Junio C Hamano","Eric Sunshine","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"388356","messageId":"cover.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":null,"subject":"[PATCH 00/15] t: replace incorrect test_must_fail usage (part 1)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:25Z","receivedAt":"2019-12-17T12:01:45Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The overall scope of these patches is to replace inappropriate uses of\ntest_must_fail. IOW, we should only allow test_must_fail to run on `git`\nand `test-tool`. Ultimately, we will conclude by making test_must_fail\nerror out on non-git commands. An advance view of the final series can\nbe found here[1].\n\nThis is the first part. It focuses on t[01]*.sh and also t/lib-git-p4.\n\n[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2\n\nDenton Liu (15):\n test-lib-functions: introduce test_non_git_might_fail()\n t/lib-git-p4: use test_path_is_missing()\n t0000: replace test_must_fail with ! for run_sub_test_lib_test()\n t0003: use named parameters in attr_check()\n t0003: use test_must_be_empty()\n t0003: don't use `test_must_fail attr_check`\n t0020: drop redirections to /dev/null\n t0020: s/test_must_fail has_cr/! has_cr/\n t0020: use ! check_packed_refs_marked\n t1306: convert `test_might_fail rm` to `rm -f`\n t1307: reorder `nongit test_must_fail`\n t1409: let sed open its own files\n t1409: use test_path_is_missing()\n t1501: remove use of `test_might_fail cp`\n t1507: teach full_name() to accept `!` arg\n\n t/lib-git-p4.sh | 2 +-\n t/t0000-basic.sh | 14 +++++-----\n t/t0003-attributes.sh | 51 ++++++++++++++++++-----------------\n t/t0020-crlf.sh | 20 +++++++-------\n t/t1306-xdg-files.sh | 8 +++---\n t/t1307-config-blob.sh | 2 +-\n t/t1409-avoid-packing-refs.sh | 16 +++++------\n t/t1501-work-tree.sh | 2 +-\n t/t1507-rev-parse-upstream.sh | 14 +++++++---\n t/test-lib-functions.sh | 9 +++++++\n 10 files changed, 77 insertions(+), 61 deletions(-)\n\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388357","messageId":"85cee927656cebed34bc93280d0b2d89e0caa570.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 01/15] test-lib-functions: introduce test_non_git_might_fail()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:26Z","receivedAt":"2019-12-17T12:01:47Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will be preventing the use of the\ntest_must_fail()-family of functions (including test_might_fail()) on\nnon-git comands. To prep for this, introduce the\ntest_non_git_might_fail() function which is used to replace non-git\ninvocations of test_might_fail().\n\nThe test_non_git_might_fail() function is a lightweight replacement,\nalways masking the return status of the command and returning a\nnon-error exit code. Unlike test_might_fail(), it does not check for\nabnormal exit conditions such as a segv. This is because we are not in\nthe business of checking the sanity of the external environment and we\ncan assume that it works properly.\n\nSigned-off-by: Denton Liu \n---\n t/test-lib-functions.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 284c52d076..61d27f1ec6 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -891,6 +891,15 @@ test_expect_code () {\n \treturn 1\n } 7>&2 2>&4\n \n+# Similar to test_might_fail, but much simpler. This is intended for use\n+# with non-git commands that we can assume will work sanely so we don't\n+# need to check for conditions such as a segv.\n+\n+test_non_git_might_fail () {\n+\t\"$@\" 2>&7\n+\treturn 0\n+}\n+\n # test_cmp is a helper function to compare actual and expected output.\n # You can use it like:\n #\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388358","messageId":"fcfccebd7aaf120674691ba92a657802c2482d7e.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 03/15] t0000: replace test_must_fail with ! for run_sub_test_lib_test()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:28Z","receivedAt":"2019-12-17T12:01:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. We use\ntest_must_fail to test run_sub_test_lib_test() but that function does\nnot invoke any git commands internally. Replace these instances of\n`test_must_fail` with `!`.\n\nSigned-off-by: Denton Liu \n---\n t/t0000-basic.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 8a81a249d0..d60ad4b78b 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -155,7 +155,7 @@ test_expect_success 'pretend we have a fully passing test suite' \"\n \"\n \n test_expect_success 'pretend we have a partially passing test suite' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\t! run_sub_test_lib_test \\\n \t\tpartial-pass '2/3 tests passing' <<-\\\\EOF &&\n \ttest_expect_success 'passing test #1' 'true'\n \ttest_expect_success 'failing test #2' 'false'\n@@ -219,7 +219,7 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su\n \"\n \n test_expect_success 'pretend we have a pass, fail, and known breakage' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\t! run_sub_test_lib_test \\\n \t\tmixed-results1 'mixed results #1' <<-\\\\EOF &&\n \ttest_expect_success 'passing test' 'true'\n \ttest_expect_success 'failing test' 'false'\n@@ -238,7 +238,7 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' \"\n \"\n \n test_expect_success 'pretend we have a mix of all possible results' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\t! run_sub_test_lib_test \\\n \t\tmixed-results2 'mixed results #2' <<-\\\\EOF &&\n \ttest_expect_success 'passing test' 'true'\n \ttest_expect_success 'passing test' 'true'\n@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' \"\n \"\n \n test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n-\ttest_must_fail run_sub_test_lib_test \\\n+\t! run_sub_test_lib_test \\\n \t\tt1234-verbose \"test verbose\" --verbose <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\n \ttest_expect_success \"test with output\" \"echo foo\"\n@@ -301,7 +301,7 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n '\n \n test_expect_success 'test --verbose-only' '\n-\ttest_must_fail run_sub_test_lib_test \\\n+\t! run_sub_test_lib_test \\\n \t\tt2345-verbose-only-2 \"test verbose-only=2\" \\\n \t\t--verbose-only=2 <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\n@@ -834,7 +834,7 @@ then\n fi\n \n test_expect_success 'tests clean up even on failures' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\t! run_sub_test_lib_test \\\n \t\tfailing-cleanup 'Failing tests with cleanup commands' <<-\\\\EOF &&\n \ttest_expect_success 'tests clean up even after a failure' '\n \t\ttouch clean-after-failure &&\n@@ -863,7 +863,7 @@ test_expect_success 'tests clean up even on failures' \"\n \"\n \n test_expect_success 'test_atexit is run' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\t! run_sub_test_lib_test \\\n \t\tatexit-cleanup 'Run atexit commands' -i <<-\\\\EOF &&\n \ttest_expect_success 'tests clean up even after a failure' '\n \t\t> ../../clean-atexit &&\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388359","messageId":"055caa5c08d9a53e5e1415d540024d54d0916252.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 05/15] t0003: use test_must_be_empty()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:30Z","receivedAt":"2019-12-17T12:01:49Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In several places, we used `test_line_count = 0` to check for an empty\nfile. Although this is correct, it's overkill. Use test_must_be_empty()\ninstead because it's more suited for this purpose.\n\nSigned-off-by: Denton Liu \n---\n t/t0003-attributes.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex c47d4cfbcd..53a730e2ee 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -12,7 +12,7 @@ attr_check () {\n \tgit $git_opts check-attr test -- \"$path\" >actual 2>err &&\n \techo \"$path: test: $expect\" >expect &&\n \ttest_cmp expect actual &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n }\n \n attr_check_quote () {\n@@ -245,7 +245,7 @@ EOF\n \tgit check-attr foo -- \"a/b/f\" >>actual 2>>err &&\n \tgit check-attr foo -- \"a/b/c/f\" >>actual 2>>err &&\n \ttest_cmp expect actual &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n \n test_expect_success '\"**\" with no slashes test' '\n@@ -266,7 +266,7 @@ EOF\n \tgit check-attr foo -- \"a/b/f\" >>actual 2>>err &&\n \tgit check-attr foo -- \"a/b/c/f\" >>actual 2>>err &&\n \ttest_cmp expect actual &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n \n test_expect_success 'using --git-dir and --work-tree' '\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388360","messageId":"f1acb2a0dfb39a4ff047d721edde821e9e296e72.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 04/15] t0003: use named parameters in attr_check()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:29Z","receivedAt":"2019-12-17T12:01:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We had named the parameters in attr_check() but $2 was being used\ninstead of $expect. Make all variable accesses in attr_check() use named\nvariables instead of numbered arguments for clarity.\n\nWhile we're at it, add variable assignments to the &&-chain. These\naren't ever expected to fail but for stylistic purposes, include them\nanyway for stylistic purposes.\n\nSigned-off-by: Denton Liu \n---\n t/t0003-attributes.sh | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 71e63d8b50..c47d4cfbcd 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -5,19 +5,20 @@ test_description=gitattributes\n . ./test-lib.sh\n \n attr_check () {\n-\tpath=\"$1\" expect=\"$2\"\n+\tpath=\"$1\" &&\n+\texpect=\"$2\" &&\n+\tgit_opts=\"$3\" &&\n \n-\tgit $3 check-attr test -- \"$path\" >actual 2>err &&\n-\techo \"$path: test: $2\" >expect &&\n+\tgit $git_opts check-attr test -- \"$path\" >actual 2>err &&\n+\techo \"$path: test: $expect\" >expect &&\n \ttest_cmp expect actual &&\n \ttest_line_count = 0 err\n }\n \n attr_check_quote () {\n-\n-\tpath=\"$1\"\n-\tquoted_path=\"$2\"\n-\texpect=\"$3\"\n+\tpath=\"$1\" &&\n+\tquoted_path=\"$2\" &&\n+\texpect=\"$3\" &&\n \n \tgit check-attr test -- \"$path\" >actual &&\n \techo \"\\\"$quoted_path\\\": test: $expect\" >expect &&\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388361","messageId":"d228dcfdc7d3f41c53a3813c52e56638cd8dd95e.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 07/15] t0020: drop redirections to /dev/null","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:32Z","receivedAt":"2019-12-17T12:01:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Since output is silenced when running without `-v` and debugging output\nis useful with `-v`, remove redirections to /dev/null as it is not\nuseful.\n\nSigned-off-by: Denton Liu \n---\n t/t0020-crlf.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 854da0ae16..8281babde0 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -5,7 +5,7 @@ test_description='CRLF conversion'\n . ./test-lib.sh\n \n has_cr() {\n-\ttr '\\015' Q <\"$1\" | grep Q >/dev/null\n+\ttr '\\015' Q <\"$1\" | grep Q\n }\n \n # add or remove CRs to disk file in-place\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388362","messageId":"3afa3a16ca2fc2fecba19ff9741925d513f68562.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 06/15] t0003: don't use `test_must_fail attr_check`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:31Z","receivedAt":"2019-12-17T12:01:51Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In an effort to remove test_must_fail for all invocations not related to\ngit or test-tool, replace invocations of `test_must_fail attr_check`\nwith a plain attr_check call with the $expect argument set to the\nactual value output by git.\n\nSigned-off-by: Denton Liu \n---\n t/t0003-attributes.sh | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 53a730e2ee..8d4343afdb 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -28,7 +28,7 @@ attr_check_quote () {\n \n test_expect_success 'open-quoted pathname' '\n \techo \"\\\"a test=a\" >.gitattributes &&\n-\ttest_must_fail attr_check a a\n+\tattr_check a unspecified\n '\n \n \n@@ -113,20 +113,20 @@ test_expect_success 'attribute test' '\n \n test_expect_success 'attribute matching is case sensitive when core.ignorecase=0' '\n \n-\ttest_must_fail attr_check F f \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/F f \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/c/F f \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/G a/g \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/B/g a/b/g \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/G a/b/g \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/H a/b/h \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/D/g \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check oNoFf unset \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check oFfOn set \"-c core.ignorecase=0\" &&\n+\tattr_check F unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/F unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/c/F unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/G unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/B/g a/g \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/G unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/H unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/D/g a/g \"-c core.ignorecase=0\" &&\n+\tattr_check oNoFf unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check oFfOn unspecified \"-c core.ignorecase=0\" &&\n \tattr_check NO unspecified \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/D/NO \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/D/NO unspecified \"-c core.ignorecase=0\" &&\n \tattr_check a/b/d/YES a/b/d/* \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/E/f \"A/e/F\" \"-c core.ignorecase=0\"\n+\tattr_check a/E/f f \"-c core.ignorecase=0\"\n \n '\n \n@@ -150,8 +150,8 @@ test_expect_success 'attribute matching is case insensitive when core.ignorecase\n '\n \n test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' '\n-\ttest_must_fail attr_check a/B/D/g \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check A/B/D/NO \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n+\tattr_check a/B/D/g a/g \"-c core.ignorecase=0\" &&\n+\tattr_check A/B/D/NO unspecified \"-c core.ignorecase=0\" &&\n \tattr_check A/b/h a/b/h \"-c core.ignorecase=1\" &&\n \tattr_check a/B/D/g \"a/b/d/*\" \"-c core.ignorecase=1\" &&\n \tattr_check A/B/D/NO \"a/b/d/*\" \"-c core.ignorecase=1\"\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388363","messageId":"8adc5cd5aaeef76ddeef459c79ecec7a05a1fd7b.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 08/15] t0020: s/test_must_fail has_cr/! has_cr/","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:33Z","receivedAt":"2019-12-17T12:01:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. Since has_cr() just\nwraps a tr and grep pipeline, replace `test_must_fail has_cr` with\n`! has_cr`.\n\nSigned-off-by: Denton Liu \n---\n t/t0020-crlf.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 8281babde0..4940e05a2e 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -159,8 +159,8 @@ test_expect_success 'checkout with autocrlf=input' '\n \trm -f tmp one dir/two three &&\n \tgit config core.autocrlf input &&\n \tgit read-tree --reset -u HEAD &&\n-\ttest_must_fail has_cr one &&\n-\ttest_must_fail has_cr dir/two &&\n+\t! has_cr one &&\n+\t! has_cr dir/two &&\n \tgit update-index -- one dir/two &&\n \ttest \"$one\" = $(git hash-object --stdin .gitattributes &&\n \tgit read-tree --reset -u HEAD &&\n \n-\ttest_must_fail has_cr dir/two\n+\t! has_cr dir/two\n '\n \n test_expect_success '.gitattributes says two and three are text' '\n@@ -270,7 +270,7 @@ test_expect_success 'in-tree .gitattributes (1)' '\n \trm -rf tmp one dir .gitattributes patch.file three &&\n \tgit read-tree --reset -u HEAD &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n@@ -280,7 +280,7 @@ test_expect_success 'in-tree .gitattributes (2)' '\n \tgit read-tree --reset HEAD &&\n \tgit checkout-index -f -q -u -a &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n@@ -291,7 +291,7 @@ test_expect_success 'in-tree .gitattributes (3)' '\n \tgit checkout-index -u .gitattributes &&\n \tgit checkout-index -u one dir/two three &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n@@ -302,7 +302,7 @@ test_expect_success 'in-tree .gitattributes (4)' '\n \tgit checkout-index -u one dir/two three &&\n \tgit checkout-index -u .gitattributes &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388364","messageId":"2dd91c5568835f8263d18db62c787deb1eeb1650.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 11/15] t1307: reorder `nongit test_must_fail`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:36Z","receivedAt":"2019-12-17T12:01:54Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the future, we plan on only allowing `test_must_fail` to work on a\nrestricted subset of commands, including `git`. Reorder the commands so\nthat `nongit` comes before `test_must_fail`. This way, `test_must_fail`\noperates on a git command.\n\nSigned-off-by: Denton Liu \n---\n t/t1307-config-blob.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh\nindex 37dc689d8c..002e6d3388 100755\n--- a/t/t1307-config-blob.sh\n+++ b/t/t1307-config-blob.sh\n@@ -74,7 +74,7 @@ test_expect_success 'can parse blob ending with CR' '\n '\n \n test_expect_success 'config --blob outside of a repository is an error' '\n-\ttest_must_fail nongit git config --blob=foo --list\n+\tnongit test_must_fail git config --blob=foo --list\n '\n \n test_done\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388365","messageId":"d39422505f16a14c64514b8a78ae351f41b75c44.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 10/15] t1306: convert `test_might_fail rm` to `rm -f`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:35Z","receivedAt":"2019-12-17T12:01:55Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() family of functions (including test_might_fail())\nshould only be used on git commands. Replace `test_might_fail rm` with\n`rm -f` so that we don't use `test_might_fail` on a non-git command.\n\nSigned-off-by: Denton Liu \n---\n t/t1306-xdg-files.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh\nindex 21e139a313..dd87b43be1 100755\n--- a/t/t1306-xdg-files.sh\n+++ b/t/t1306-xdg-files.sh\n@@ -153,7 +153,7 @@ test_expect_success 'Checking attributes in both XDG and local attributes files'\n \n \n test_expect_success 'Checking attributes in a non-XDG global attributes file' '\n-\ttest_might_fail rm .gitattributes &&\n+\trm -f .gitattributes &&\n \techo \"f attr_f=test\" >\"$HOME\"/my_gitattributes &&\n \tgit config core.attributesfile \"$HOME\"/my_gitattributes &&\n \techo \"f: attr_f: test\" >expected &&\n@@ -165,7 +165,7 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '\n test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\\''t' '\n \tmkdir -p \"$HOME\"/.config/git &&\n \t>\"$HOME\"/.config/git/config &&\n-\ttest_might_fail rm \"$HOME\"/.gitconfig &&\n+\trm -f \"$HOME\"/.gitconfig &&\n \tgit config --global user.name \"write_config\" &&\n \techo \"[user]\" >expected &&\n \techo \"\tname = write_config\" >>expected &&\n@@ -183,8 +183,8 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '\n \n \n test_expect_success 'write: ~/.config/git/ exists and config file doesn'\\''t' '\n-\ttest_might_fail rm \"$HOME\"/.gitconfig &&\n-\ttest_might_fail rm \"$HOME\"/.config/git/config &&\n+\trm -f \"$HOME\"/.gitconfig &&\n+\trm -f \"$HOME\"/.config/git/config &&\n \tgit config --global user.name \"write_gitconfig\" &&\n \techo \"[user]\" >expected &&\n \techo \"\tname = write_gitconfig\" >>expected &&\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388366","messageId":"b77b474422061960615449318b00488fb0f5277b.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 09/15] t0020: use ! check_packed_refs_marked","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:34Z","receivedAt":"2019-12-17T12:01:56Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. Since\ncheck_packed_refs_marked() just wraps a grep invocation, replace\n`test_must_fail check_packed_refs_marked` with\n`! check_packed_refs_marked`.\n\nSigned-off-by: Denton Liu \n---\n t/t1409-avoid-packing-refs.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex e5cb8a252d..c46848eb8e 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -46,7 +46,7 @@ test_expect_success 'check that marking the packed-refs file works' '\n \tgit for-each-ref >actual &&\n \ttest_cmp expected actual &&\n \tgit pack-refs --all &&\n-\ttest_must_fail check_packed_refs_marked &&\n+\t! check_packed_refs_marked &&\n \tgit for-each-ref >actual2 &&\n \ttest_cmp expected actual2\n '\n@@ -80,7 +80,7 @@ test_expect_success 'touch packed-refs on delete of packed' '\n \tgit pack-refs --all &&\n \tmark_packed_refs &&\n \tgit update-ref -d refs/heads/packed-delete &&\n-\ttest_must_fail check_packed_refs_marked\n+\t! check_packed_refs_marked\n '\n \n test_expect_success 'leave packed-refs untouched on update of loose' '\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388367","messageId":"27b32962427e5b74569b08dc4c0de40b67388615.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 13/15] t1409: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:38Z","receivedAt":"2019-12-17T12:01:57Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() function should only be used for git commands since\nwe should assume that external commands work sanely. Replace\n`test_must_fail test -f` with `test_path_is_missing` since we expect\nthese files to not exist.\n\nSigned-off-by: Denton Liu \n---\n t/t1409-avoid-packing-refs.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex f74d890e82..be12fb6350 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -27,15 +27,15 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'do not create packed-refs file gratuitously' '\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref refs/heads/foo $A &&\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref refs/heads/foo $B &&\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref refs/heads/foo $C $B &&\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref -d refs/heads/foo &&\n-\ttest_must_fail test -f .git/packed-refs\n+\ttest_path_is_missing .git/packed-refs\n '\n \n test_expect_success 'check that marking the packed-refs file works' '\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388368","messageId":"3d36511d5d95d2a2713b3cc8e2138b689d381c79.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 14/15] t1501: remove use of `test_might_fail cp`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:39Z","receivedAt":"2019-12-17T12:01:57Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() family of functions (including test_might_fail())\nshould only be used on git commands. Replace test_might_fail() with\ntest_non_git_might_fail().\n\nSigned-off-by: Denton Liu \n---\n t/t1501-work-tree.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\nindex 3498d3d55e..067301a5ab 100755\n--- a/t/t1501-work-tree.sh\n+++ b/t/t1501-work-tree.sh\n@@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '\n \tmkdir work &&\n \tmkdir -p repo.git/repos/foo &&\n \tcp repo.git/HEAD repo.git/index repo.git/repos/foo &&\n-\ttest_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n+\ttest_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n \tsane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE\n '\n \n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388369","messageId":"cd392a74acb1bc7f3b09f167278afd5959a21fca.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 15/15] t1507: teach full_name() to accept `!` arg","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:40Z","receivedAt":"2019-12-17T12:01:59Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, we were running `test_must_fail full_name`. However,\n`test_must_fail` should only be used on git commands. Teach full_name()\nto accept `!` as a potential first argument which will prepend\n`test_must_fail` to the enclosed git command. This increases the\ngranularity of test_must_fail since it no longer runs on the `cd` as\nwell.\n\nSigned-off-by: Denton Liu \n---\n t/t1507-rev-parse-upstream.sh | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex 8b4cf8a6e3..9a941d68d8 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -29,8 +29,14 @@ test_expect_success 'setup' '\n '\n \n full_name () {\n+\tfail= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tfail=test_must_fail &&\n+\t\tshift\n+\tfi &&\n \t(cd clone &&\n-\t git rev-parse --symbolic-full-name \"$@\")\n+\t $fail git rev-parse --symbolic-full-name \"$@\")\n }\n \n commit_subject () {\n@@ -79,7 +85,7 @@ test_expect_success 'upstream of branch with @ at end' '\n '\n \n test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' '\n-\ttest_must_fail full_name refs/heads/my-side@{upstream}\n+\tfull_name ! refs/heads/my-side@{upstream}\n '\n \n test_expect_success 'my-side@{u} resolves to correct commit' '\n@@ -91,9 +97,9 @@ test_expect_success 'my-side@{u} resolves to correct commit' '\n '\n \n test_expect_success 'not-tracking@{u} fails' '\n-\ttest_must_fail full_name non-tracking@{u} &&\n+\tfull_name ! non-tracking@{u} &&\n \t(cd clone && git checkout --no-track -b non-tracking) &&\n-\ttest_must_fail full_name non-tracking@{u}\n+\tfull_name ! non-tracking@{u}\n '\n \n test_expect_success '@{u}@{1} resolves correctly' '\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388370","messageId":"0b7d19a7e1498238f3d6d4d9bb07b3345cbb345f.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 12/15] t1409: let sed open its own files","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:37Z","receivedAt":"2019-12-17T12:02:00Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In one case, we were using a redirection operator to feed input into\nsed. However, since sed is capable of opening its own files, make sed\nopen its own files instead of redirecting input into it.\n\nSigned-off-by: Denton Liu \n---\n t/t1409-avoid-packing-refs.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex c46848eb8e..f74d890e82 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -8,7 +8,7 @@ test_description='avoid rewriting packed-refs unnecessarily'\n # shouldn't upset readers, and it should be omitted if the file is\n # ever rewritten.\n mark_packed_refs () {\n-\tsed -e \"s/^\\(#.*\\)/\\1 t1409 /\" <.git/packed-refs >.git/packed-refs.new &&\n+\tsed -e \"s/^\\(#.*\\)/\\1 t1409 /\" .git/packed-refs >.git/packed-refs.new &&\n \tmv .git/packed-refs.new .git/packed-refs\n }\n \n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388371","messageId":"3d64837af1bc2af972ae1415502bbbde26ee3784.1576583819.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH 02/15] t/lib-git-p4: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-17T12:01:27Z","receivedAt":"2019-12-17T12:02:01Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Previously, cleanup_git() would use `test_must_fail test -d` to ensure\nthat the directory is removed. However, test_must_fail should only be\nused for git commands. Use test_path_is_missing() instead to check that\nthe directory has been removed.\n\nSigned-off-by: Denton Liu \n---\n t/lib-git-p4.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex 547b9f88e1..5aff2abe8b 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -175,7 +175,7 @@ stop_and_cleanup_p4d () {\n \n cleanup_git () {\n \tretry_until_success rm -r \"$git\"\n-\ttest_must_fail test -d \"$git\" &&\n+\ttest_path_is_missing \"$git\" &&\n \tretry_until_success mkdir \"$git\"\n }\n \n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388384","messageId":"xmqqwoauhknb.fsf@gitster-ct.c.googlers.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-17T18:28:08Z","receivedAt":"2019-12-17T18:28:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> The overall scope of these patches is to replace inappropriate uses of\n> test_must_fail. IOW, we should only allow test_must_fail to run on `git`\n> and `test-tool`. Ultimately, we will conclude by making test_must_fail\n> error out on non-git commands. An advance view of the final series can\n> be found here[1].\n\nWow. It is a bit sad to see that we had so many of them.\n\n"},{"id":"388385","messageId":"xmqqsglihkab.fsf@gitster-ct.c.googlers.com","threadId":"52474","inReplyTo":"f1acb2a0dfb39a4ff047d721edde821e9e296e72.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 04/15] t0003: use named parameters in attr_check()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-17T18:35:56Z","receivedAt":"2019-12-17T18:36:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> We had named the parameters in attr_check() but $2 was being used\n> instead of $expect. Make all variable accesses in attr_check() use named\n> variables instead of numbered arguments for clarity.\n>\n> While we're at it, add variable assignments to the &&-chain. These\n> aren't ever expected to fail but for stylistic purposes, include them\n> anyway for stylistic purposes.\n\nThat is OK but do so like this instead for brevity?\n\n attr_check () {\n- \tpath=\"$1\" expect=\"$2\"\n+ \tpath=\"$1\" expect=\"$2\" git_opts=\"$3\" &&\n\nGiving (temporary) names to incoming parameters is so common that\nsuch a convention to save vertical screen real estate may be worth\nit.\n\nIt is sad that you can pass only $IFS-free things in $3 due to the\nway it is used---perhaps it is sufficient for the purpose of tests\nin this script, but it still looks sad (just a comment without\nanything that is actionable).\n\n> Signed-off-by: Denton Liu \n> ---\n> t/t0003-attributes.sh | 15 ++++++++-------\n> 1 file changed, 8 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> index 71e63d8b50..c47d4cfbcd 100755\n> --- a/t/t0003-attributes.sh\n> +++ b/t/t0003-attributes.sh\n> @@ -5,19 +5,20 @@ test_description=gitattributes\n> . ./test-lib.sh\n> \n> attr_check () {\n> -\tpath=\"$1\" expect=\"$2\"\n> +\tpath=\"$1\" &&\n> +\texpect=\"$2\" &&\n> +\tgit_opts=\"$3\" &&\n> \n> -\tgit $3 check-attr test -- \"$path\" >actual 2>err &&\n> -\techo \"$path: test: $2\" >expect &&\n> +\tgit $git_opts check-attr test -- \"$path\" >actual 2>err &&\n> +\techo \"$path: test: $expect\" >expect &&\n> \ttest_cmp expect actual &&\n> \ttest_line_count = 0 err\n> }\n> \n> attr_check_quote () {\n> -\n> -\tpath=\"$1\"\n> -\tquoted_path=\"$2\"\n> -\texpect=\"$3\"\n> +\tpath=\"$1\" &&\n> +\tquoted_path=\"$2\" &&\n> +\texpect=\"$3\" &&\n> \n> \tgit check-attr test -- \"$path\" >actual &&\n> \techo \"\\\"$quoted_path\\\": test: $expect\" >expect &&\n"},{"id":"388387","messageId":"CAPig+cQ4=CeGJmVp4S9xdR9UVN2mLRndX7+cCdSn8rhaEgdcmg@mail.gmail.com","threadId":"52474","inReplyTo":"f1acb2a0dfb39a4ff047d721edde821e9e296e72.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 04/15] t0003: use named parameters in attr_check()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-17T18:43:02Z","receivedAt":"2019-12-17T18:43:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 17, 2019 at 7:01 AM Denton Liu wrote:\n> We had named the parameters in attr_check() but $2 was being used\n> instead of $expect. Make all variable accesses in attr_check() use named\n> variables instead of numbered arguments for clarity.\n>\n> While we're at it, add variable assignments to the &&-chain. These\n> aren't ever expected to fail but for stylistic purposes, include them\n> anyway for stylistic purposes.\n\nAs a justification, \"stylistic purposes\" isn't very strong. However, a\nsolid justification is that keeping the &&-chain intact even for these\nassignments means that if someone comes along in the future and\ninserts code _above_ the assignments, and if that code could fail in\nsome way, then the intact &&-chain will ensure that that failure is\nnoticed rather than going unnoticed.\n\n> Signed-off-by: Denton Liu \n"},{"id":"388388","messageId":"xmqqo8w6hjwm.fsf@gitster-ct.c.googlers.com","threadId":"52474","inReplyTo":"3afa3a16ca2fc2fecba19ff9741925d513f68562.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 06/15] t0003: don't use `test_must_fail attr_check`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-17T18:44:09Z","receivedAt":"2019-12-17T18:44:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> test_expect_success 'open-quoted pathname' '\n> \techo \"\\\"a test=a\" >.gitattributes &&\n> -\ttest_must_fail attr_check a a\n> +\tattr_check a unspecified\n> '\n\nI offhand do not think of a reason why \"git check-attr\" must be\ngiven freedom to choose from more than one answers (which is what\nthe original test that said \"as long as it does not report 'a', we\nare happy\" allowed the command to), so it probably is OK to make the\ntests more strict like this patch does.\n\nThanks.\n"},{"id":"388391","messageId":"xmqqk16uhjdo.fsf@gitster-ct.c.googlers.com","threadId":"52474","inReplyTo":"d39422505f16a14c64514b8a78ae351f41b75c44.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 10/15] t1306: convert `test_might_fail rm` to `rm -f`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-17T18:55:31Z","receivedAt":"2019-12-17T18:55:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> The test_must_fail() family of functions (including test_might_fail())\n> should only be used on git commands. Replace `test_might_fail rm` with\n> `rm -f` so that we don't use `test_might_fail` on a non-git command.\n\n\"rm -f X\" can return non-zero status if \"X\" exists and cannot be\nremoved (e.g. \"X\" is a directory, X is in a directory you cannot\nwrite to). The only thing \"-f\" prevents the command from returning\nnon-zero status is when \"X\" does not exist.\n\nThat means that this change will change the behaviour. Let's see if\nit does in a good way or a bad way.\n\n> test_expect_success 'Checking attributes in a non-XDG global attributes file' '\n> -\ttest_might_fail rm .gitattributes &&\n> +\trm -f .gitattributes &&\n\nThis is so that leftover .gitattributes from previous tests will not\naffect the outcome of this test. If .gitattributes left by earlier\ntests cannot be removed for whatever reason, we would want to know\nabout it, so changing to \"rm -f\" to make the tests more strict is a\ngood move.\n\n> \techo \"f attr_f=test\" >\"$HOME\"/my_gitattributes &&\n> \tgit config core.attributesfile \"$HOME\"/my_gitattributes &&\n> \techo \"f: attr_f: test\" >expected &&\n> @@ -165,7 +165,7 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '\n> test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\\''t' '\n> \tmkdir -p \"$HOME\"/.config/git &&\n> \t>\"$HOME\"/.config/git/config &&\n> -\ttest_might_fail rm \"$HOME\"/.gitconfig &&\n> +\trm -f \"$HOME\"/.gitconfig &&\n\nLikewise.\n\n> \tgit config --global user.name \"write_config\" &&\n> \techo \"[user]\" >expected &&\n> \techo \"\tname = write_config\" >>expected &&\n> @@ -183,8 +183,8 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '\n> \n> \n> test_expect_success 'write: ~/.config/git/ exists and config file doesn'\\''t' '\n> -\ttest_might_fail rm \"$HOME\"/.gitconfig &&\n> -\ttest_might_fail rm \"$HOME\"/.config/git/config &&\n> +\trm -f \"$HOME\"/.gitconfig &&\n> +\trm -f \"$HOME\"/.config/git/config &&\n\nLikewise, but I think it makes more sense to remove them with a\nsingle invocation of \"rm -f\".\n\n> \tgit config --global user.name \"write_gitconfig\" &&\n> \techo \"[user]\" >expected &&\n> \techo \"\tname = write_gitconfig\" >>expected &&\n\nThanks. In short, I think all of the changes in the patch are good.\n"},{"id":"388392","messageId":"xmqqfthihj9k.fsf@gitster-ct.c.googlers.com","threadId":"52474","inReplyTo":"0b7d19a7e1498238f3d6d4d9bb07b3345cbb345f.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 12/15] t1409: let sed open its own files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-17T18:57:59Z","receivedAt":"2019-12-17T18:58:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> Subject: Re: [PATCH 12/15] t1409: let sed open its own files\n\ns/files/input file/;\n\nThis is borderline Meh to me, but since the patch has already been\nwritten, let's use it ;-)\n\n> In one case, we were using a redirection operator to feed input into\n> sed. However, since sed is capable of opening its own files, make sed\n> open its own files instead of redirecting input into it.\n>\n> Signed-off-by: Denton Liu \n> ---\n> t/t1409-avoid-packing-refs.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\n> index c46848eb8e..f74d890e82 100755\n> --- a/t/t1409-avoid-packing-refs.sh\n> +++ b/t/t1409-avoid-packing-refs.sh\n> @@ -8,7 +8,7 @@ test_description='avoid rewriting packed-refs unnecessarily'\n> # shouldn't upset readers, and it should be omitted if the file is\n> # ever rewritten.\n> mark_packed_refs () {\n> -\tsed -e \"s/^\\(#.*\\)/\\1 t1409 /\" <.git/packed-refs >.git/packed-refs.new &&\n> +\tsed -e \"s/^\\(#.*\\)/\\1 t1409 /\" .git/packed-refs >.git/packed-refs.new &&\n> \tmv .git/packed-refs.new .git/packed-refs\n> }\n"},{"id":"388393","messageId":"xmqqbls6hj3w.fsf@gitster-ct.c.googlers.com","threadId":"52474","inReplyTo":"27b32962427e5b74569b08dc4c0de40b67388615.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 13/15] t1409: use test_path_is_missing()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-17T19:01:23Z","receivedAt":"2019-12-17T19:01:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> The test_must_fail() function should only be used for git commands since\n> we should assume that external commands work sanely. Replace\n> `test_must_fail test -f` with `test_path_is_missing` since we expect\n> these files to not exist.\n\ns/these files/these paths/; \n\n\"! test -f FOO\" allowed FOO to be a directory, but by using\ntest_path_is_missing we are no longer allowing FOO to exist in any\nform. I think the changes are correct, but rephrasing \"files\" to\n\"paths\" would emphasize the reason why it is a good change, I would\nsay.\n\nThanks.\n\n\n> Signed-off-by: Denton Liu \n> ---\n> t/t1409-avoid-packing-refs.sh | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\n> index f74d890e82..be12fb6350 100755\n> --- a/t/t1409-avoid-packing-refs.sh\n> +++ b/t/t1409-avoid-packing-refs.sh\n> @@ -27,15 +27,15 @@ test_expect_success 'setup' '\n> '\n> \n> test_expect_success 'do not create packed-refs file gratuitously' '\n> -\ttest_must_fail test -f .git/packed-refs &&\n> +\ttest_path_is_missing .git/packed-refs &&\n> \tgit update-ref refs/heads/foo $A &&\n> -\ttest_must_fail test -f .git/packed-refs &&\n> +\ttest_path_is_missing .git/packed-refs &&\n> \tgit update-ref refs/heads/foo $B &&\n> -\ttest_must_fail test -f .git/packed-refs &&\n> +\ttest_path_is_missing .git/packed-refs &&\n> \tgit update-ref refs/heads/foo $C $B &&\n> -\ttest_must_fail test -f .git/packed-refs &&\n> +\ttest_path_is_missing .git/packed-refs &&\n> \tgit update-ref -d refs/heads/foo &&\n> -\ttest_must_fail test -f .git/packed-refs\n> +\ttest_path_is_missing .git/packed-refs\n> '\n> \n> test_expect_success 'check that marking the packed-refs file works' '\n"},{"id":"388394","messageId":"CAPig+cQJRCAYkn+9CoK0YOXa66B7ocv2qXGLBXy8f-sgE3ipBA@mail.gmail.com","threadId":"52474","inReplyTo":"d228dcfdc7d3f41c53a3813c52e56638cd8dd95e.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 07/15] t0020: drop redirections to /dev/null","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-17T19:32:09Z","receivedAt":"2019-12-17T19:32:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 17, 2019 at 7:02 AM Denton Liu wrote:\n> Since output is silenced when running without `-v` and debugging output\n> is useful with `-v`, remove redirections to /dev/null as it is not\n> useful.\n>\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\n> @@ -5,7 +5,7 @@ test_description='CRLF conversion'\n> has_cr() {\n> - tr '\\015' Q <\"$1\" | grep Q >/dev/null\n> + tr '\\015' Q <\"$1\" | grep Q\n> }\n\nI'm not sure that I agree with this change since dropping >/dev/null\ndoesn't improve the situation when someone is trying to debug a\nfailing test. What this change will do is fill the -v output with a\nbunch of lines ending with \"Q\" when CR is expected -- the normal\n_successful_ case for about half the calls to has_cr() -- so -v output\nwill become a lot noisier without really adding much, if any,\ndebugging value. If you really want to help people trying to diagnose\nfailures, I could see you replacing has_cr() with two new functions\nwhich actually provide useful diagnostic output; for instance\nsomething like:\n\n expect_cr () {\n if ! tr '\\015' Q <\"$1\" | grep Q >/dev/null\n then\n echo \"missing CR termination in: $1\" >&2 &&\n return 1\n fi\n }\n\n expect_no_cr () {\n if tr '\\015' Q <\"$1\" | grep Q >/dev/null\n then\n echo \"unexpected CR termination in: $1\" >&2 &&\n return 1\n fi\n }\n\nHowever, I'm not convinced that introducing and debugging these\nfunctions is worth the effort over simply dropping this patch from the\nseries.\n"},{"id":"388396","messageId":"CAPig+cRupjwrfoccta1JyjuB1hNhVGOP=as+dDQ8BhUVcYX3nA@mail.gmail.com","threadId":"52474","inReplyTo":"8adc5cd5aaeef76ddeef459c79ecec7a05a1fd7b.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 08/15] t0020: s/test_must_fail has_cr/! has_cr/","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-17T19:36:19Z","receivedAt":"2019-12-17T19:36:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 17, 2019 at 7:02 AM Denton Liu wrote:\n> t0020: s/test_must_fail has_cr/! has_cr/\n\nCryptic patch subject. How about being consistent with other patch\nsubjects in this series?\n\n t0020: don't use `test_must_fail has_cr`\n\n> The test_must_fail function should only be used for git commands since\n> we should assume that external commands work sanely. Since has_cr() just\n> wraps a tr and grep pipeline, replace `test_must_fail has_cr` with\n> `! has_cr`.\n>\n> Signed-off-by: Denton Liu \n"},{"id":"388398","messageId":"CAPig+cSsJW3=3cYyFzwr2Lf0AcH+XUyrXMkHBGtNa4a5Smfp6w@mail.gmail.com","threadId":"52474","inReplyTo":"3d36511d5d95d2a2713b3cc8e2138b689d381c79.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 14/15] t1501: remove use of `test_might_fail cp`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-17T19:46:34Z","receivedAt":"2019-12-17T19:46:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 17, 2019 at 7:02 AM Denton Liu wrote:\n> The test_must_fail() family of functions (including test_might_fail())\n> should only be used on git commands. Replace test_might_fail() with\n> test_non_git_might_fail().\n>\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\n> @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '\n> cp repo.git/HEAD repo.git/index repo.git/repos/foo &&\n> - test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n> + test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n\nCan you update the commit message to show that this change is indeed\nthe correct choice so that readers of this patch don't have to go\nthrough the same investigative reasoning process you went through to\nconvince yourself that it is correct. In other words, why was\ntest_might_fail() used in the first place? Is it because there might\nnot be a sharedindex.* file? Or is it because repo.git/repos/foo might\nnot be writable? Or what? The answer to these questions should explain\nto the reader, for instance, why you chose to use\n`test_non_git_might_fail cp ...` as opposed to a simple `cp -f ...`.\n"},{"id":"388399","messageId":"CAPig+cQiNpgEURfNr91V=j2mk6Y3c28NPgKj9NigU3XFRS=ZbA@mail.gmail.com","threadId":"52474","inReplyTo":"cd392a74acb1bc7f3b09f167278afd5959a21fca.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 15/15] t1507: teach full_name() to accept `!` arg","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-17T19:54:06Z","receivedAt":"2019-12-17T19:54:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 17, 2019 at 7:02 AM Denton Liu wrote:\n> Before, we were running `test_must_fail full_name`. However,\n> `test_must_fail` should only be used on git commands. Teach full_name()\n> to accept `!` as a potential first argument which will prepend\n> `test_must_fail` to the enclosed git command. This increases the\n> granularity of test_must_fail since it no longer runs on the `cd` as\n> well.\n>\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\n> @@ -29,8 +29,14 @@ test_expect_success 'setup' '\n> full_name () {\n> + fail= &&\n> + if test \"x$1\" = 'x!'\n> + then\n> + fail=test_must_fail &&\n> + shift\n> + fi &&\n> (cd clone &&\n> - git rev-parse --symbolic-full-name \"$@\")\n> + $fail git rev-parse --symbolic-full-name \"$@\")\n> }\n\nYuck. These days this is entirely unnecessary. My suggestion is to\ndrop the full_name() function altogether and just invoke git-rev-parse\ndirectly at the (few) call sites, taking advantage of the fact that we\nnow have -C. So...\n\n> @@ -79,7 +85,7 @@ test_expect_success 'upstream of branch with @ at end' '\n> test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' '\n> - test_must_fail full_name refs/heads/my-side@{upstream}\n> + full_name ! refs/heads/my-side@{upstream}\n> '\n\nThis just becomes:\n\n test_must_fail git -C clone rev-parse --symbolic-full-name\nrefs/heads/my-side@{upstream}\n\nSimilarly for the other call sites.\n\n> @@ -91,9 +97,9 @@ test_expect_success 'my-side@{u} resolves to correct commit' '\n> test_expect_success 'not-tracking@{u} fails' '\n> - test_must_fail full_name non-tracking@{u} &&\n> + full_name ! non-tracking@{u} &&\n> (cd clone && git checkout --no-track -b non-tracking) &&\n> - test_must_fail full_name non-tracking@{u}\n> + full_name ! non-tracking@{u}\n> '\n"},{"id":"388405","messageId":"de3d194a-bb05-8049-673b-267f53a271d6@kdbg.org","threadId":"52474","inReplyTo":"fcfccebd7aaf120674691ba92a657802c2482d7e.1576583819.git.liu.denton@gmail.com","subject":"Re: [PATCH 03/15] t0000: replace test_must_fail with ! for run_sub_test_lib_test()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-12-17T20:23:57Z","receivedAt":"2019-12-17T20:24:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.12.19 um 13:01 schrieb Denton Liu:\n> The test_must_fail function should only be used for git commands since\n> we should assume that external commands work sanely. We use\n> test_must_fail to test run_sub_test_lib_test() but that function does\n> not invoke any git commands internally. Replace these instances of\n> `test_must_fail` with `!`.\n> \n> Signed-off-by: Denton Liu \n> ---\n> t/t0000-basic.sh | 14 +++++++-------\n> 1 file changed, 7 insertions(+), 7 deletions(-)\n> \n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 8a81a249d0..d60ad4b78b 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -155,7 +155,7 @@ test_expect_success 'pretend we have a fully passing test suite' \"\n> \"\n> \n> test_expect_success 'pretend we have a partially passing test suite' \"\n> -\ttest_must_fail run_sub_test_lib_test \\\n> +\t! run_sub_test_lib_test \\\n> \t\tpartial-pass '2/3 tests passing' <<-\\\\EOF &&\n\nIt is a very uncommon situation (read: I doubt that it ever occurs) in\nour test suite that we expect a shell function to fail, but that we do\n*not* care at all which of its sub-commands actually failed. We actually\ndo care which sub-command failed. Therefore, we have, e.g., the idiom\n\"test_i18n_grep ! ...\". And in fact, in the case of\nrun_sub_test_lib_test we have the form run_sub_test_lib_test_err to\ncheck for error exit in the subordinate test. All of the cases you\nchange here should use it.\n\n> \ttest_expect_success 'passing test #1' 'true'\n> \ttest_expect_success 'failing test #2' 'false'\n> @@ -219,7 +219,7 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su\n> \"\n> \n> test_expect_success 'pretend we have a pass, fail, and known breakage' \"\n> -\ttest_must_fail run_sub_test_lib_test \\\n> +\t! run_sub_test_lib_test \\\n> \t\tmixed-results1 'mixed results #1' <<-\\\\EOF &&\n> \ttest_expect_success 'passing test' 'true'\n> \ttest_expect_success 'failing test' 'false'\n> @@ -238,7 +238,7 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' \"\n> \"\n> \n> test_expect_success 'pretend we have a mix of all possible results' \"\n> -\ttest_must_fail run_sub_test_lib_test \\\n> +\t! run_sub_test_lib_test \\\n> \t\tmixed-results2 'mixed results #2' <<-\\\\EOF &&\n> \ttest_expect_success 'passing test' 'true'\n> \ttest_expect_success 'passing test' 'true'\n> @@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' \"\n> \"\n> \n> test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n> -\ttest_must_fail run_sub_test_lib_test \\\n> +\t! run_sub_test_lib_test \\\n> \t\tt1234-verbose \"test verbose\" --verbose <<-\\EOF &&\n> \ttest_expect_success \"passing test\" true\n> \ttest_expect_success \"test with output\" \"echo foo\"\n> @@ -301,7 +301,7 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n> '\n> \n> test_expect_success 'test --verbose-only' '\n> -\ttest_must_fail run_sub_test_lib_test \\\n> +\t! run_sub_test_lib_test \\\n> \t\tt2345-verbose-only-2 \"test verbose-only=2\" \\\n> \t\t--verbose-only=2 <<-\\EOF &&\n> \ttest_expect_success \"passing test\" true\n> @@ -834,7 +834,7 @@ then\n> fi\n> \n> test_expect_success 'tests clean up even on failures' \"\n> -\ttest_must_fail run_sub_test_lib_test \\\n> +\t! run_sub_test_lib_test \\\n> \t\tfailing-cleanup 'Failing tests with cleanup commands' <<-\\\\EOF &&\n> \ttest_expect_success 'tests clean up even after a failure' '\n> \t\ttouch clean-after-failure &&\n> @@ -863,7 +863,7 @@ test_expect_success 'tests clean up even on failures' \"\n> \"\n> \n> test_expect_success 'test_atexit is run' \"\n> -\ttest_must_fail run_sub_test_lib_test \\\n> +\t! run_sub_test_lib_test \\\n> \t\tatexit-cleanup 'Run atexit commands' -i <<-\\\\EOF &&\n> \ttest_expect_success 'tests clean up even after a failure' '\n> \t\t> ../../clean-atexit &&\n> \n\n-- Hannes\n"},{"id":"388580","messageId":"85cee927656cebed34bc93280d0b2d89e0caa570.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 01/16] test-lib-functions: introduce test_non_git_might_fail()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:36Z","receivedAt":"2019-12-19T22:21:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will be preventing the use of the\ntest_must_fail()-family of functions (including test_might_fail()) on\nnon-git comands. To prep for this, introduce the\ntest_non_git_might_fail() function which is used to replace non-git\ninvocations of test_might_fail().\n\nThe test_non_git_might_fail() function is a lightweight replacement,\nalways masking the return status of the command and returning a\nnon-error exit code. Unlike test_might_fail(), it does not check for\nabnormal exit conditions such as a segv. This is because we are not in\nthe business of checking the sanity of the external environment and we\ncan assume that it works properly.\n\nSigned-off-by: Denton Liu \n---\n t/test-lib-functions.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 284c52d076..61d27f1ec6 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -891,6 +891,15 @@ test_expect_code () {\n \treturn 1\n } 7>&2 2>&4\n \n+# Similar to test_might_fail, but much simpler. This is intended for use\n+# with non-git commands that we can assume will work sanely so we don't\n+# need to check for conditions such as a segv.\n+\n+test_non_git_might_fail () {\n+\t\"$@\" 2>&7\n+\treturn 0\n+}\n+\n # test_cmp is a helper function to compare actual and expected output.\n # You can use it like:\n #\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388581","messageId":"3d64837af1bc2af972ae1415502bbbde26ee3784.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 02/16] t/lib-git-p4: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:37Z","receivedAt":"2019-12-19T22:21:40Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Previously, cleanup_git() would use `test_must_fail test -d` to ensure\nthat the directory is removed. However, test_must_fail should only be\nused for git commands. Use test_path_is_missing() instead to check that\nthe directory has been removed.\n\nSigned-off-by: Denton Liu \n---\n t/lib-git-p4.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex 547b9f88e1..5aff2abe8b 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -175,7 +175,7 @@ stop_and_cleanup_p4d () {\n \n cleanup_git () {\n \tretry_until_success rm -r \"$git\"\n-\ttest_must_fail test -d \"$git\" &&\n+\ttest_path_is_missing \"$git\" &&\n \tretry_until_success mkdir \"$git\"\n }\n \n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388582","messageId":"cover.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576583819.git.liu.denton@gmail.com","subject":"[PATCH v2 00/16] t: replace incorrect test_must_fail usage (part 1)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:35Z","receivedAt":"2019-12-19T22:21:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The overall scope of these patches is to replace inappropriate uses of\ntest_must_fail. IOW, we should only allow test_must_fail to run on `git`\nand `test-tool`. Ultimately, we will conclude by making test_must_fail\nerror out on non-git commands. An advance view of the final series can\nbe found here[1].\n\nThis is the first part. It focuses on t[01]*.sh and also t/lib-git-p4.\n\nChanges since v1:\n\n* Incorporate review comments by Junio, Eric and J6t\n\n* Further cleanup of t1507 before inlining full_name() (since we didn't\n want to inline full_name() into a bad command substitution)\n\n[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2\n\nDenton Liu (16):\n test-lib-functions: introduce test_non_git_might_fail()\n t/lib-git-p4: use test_path_is_missing()\n t0000: replace test_must_fail with run_sub_test_lib_test_err()\n t0003: use named parameters in attr_check()\n t0003: use test_must_be_empty()\n t0003: don't use `test_must_fail attr_check`\n t0020: don't use `test_must_fail has_cr`\n t0020: use ! check_packed_refs_marked\n t1306: convert `test_might_fail rm` to `rm -f`\n t1307: reorder `nongit test_must_fail`\n t1409: let sed open its own input file\n t1409: use test_path_is_missing()\n t1501: remove use of `test_might_fail cp`\n t1507: stop losing return codes of git commands\n t1507: run commands within test_expect_success\n t1507: inline full_name()\n\n t/lib-git-p4.sh | 2 +-\n t/t0000-basic.sh | 14 ++---\n t/t0003-attributes.sh | 47 +++++++--------\n t/t0020-crlf.sh | 18 +++---\n t/t1306-xdg-files.sh | 8 +--\n t/t1307-config-blob.sh | 2 +-\n t/t1409-avoid-packing-refs.sh | 16 +++---\n t/t1501-work-tree.sh | 2 +-\n t/t1507-rev-parse-upstream.sh | 104 +++++++++++++++++++---------------\n t/test-lib-functions.sh | 9 +++\n 10 files changed, 121 insertions(+), 101 deletions(-)\n\nRange-diff against v1:\n 1: fcfccebd7a ! 1: 778ae9052b t0000: replace test_must_fail with ! for run_sub_test_lib_test()\n @@ Metadata\n Author: Denton Liu \n \n ## Commit message ##\n - t0000: replace test_must_fail with ! for run_sub_test_lib_test()\n + t0000: replace test_must_fail with run_sub_test_lib_test_err()\n \n The test_must_fail function should only be used for git commands since\n we should assume that external commands work sanely. We use\n test_must_fail to test run_sub_test_lib_test() but that function does\n - not invoke any git commands internally. Replace these instances of\n - `test_must_fail` with `!`.\n + not invoke any git commands internally. Even better, we have a function\n + that's exactly meant to be used when we expect to have a failing test\n + suite: run_sub_test_lib_test_err()!\n +\n + Replace `test_must_fail run_sub_test_lib_test` with\n + `run_sub_test_lib_test_err`.\n \n ## t/t0000-basic.sh ##\n @@ t/t0000-basic.sh: test_expect_success 'pretend we have a fully passing test suite' \"\n @@ t/t0000-basic.sh: test_expect_success 'pretend we have a fully passing test suit\n \n test_expect_success 'pretend we have a partially passing test suite' \"\n -\ttest_must_fail run_sub_test_lib_test \\\n -+\t! run_sub_test_lib_test \\\n ++\trun_sub_test_lib_test_err \\\n \t\tpartial-pass '2/3 tests passing' <<-\\\\EOF &&\n \ttest_expect_success 'passing test #1' 'true'\n \ttest_expect_success 'failing test #2' 'false'\n @@ t/t0000-basic.sh: test_expect_success 'pretend we have fixed one of two known br\n \n test_expect_success 'pretend we have a pass, fail, and known breakage' \"\n -\ttest_must_fail run_sub_test_lib_test \\\n -+\t! run_sub_test_lib_test \\\n ++\trun_sub_test_lib_test_err \\\n \t\tmixed-results1 'mixed results #1' <<-\\\\EOF &&\n \ttest_expect_success 'passing test' 'true'\n \ttest_expect_success 'failing test' 'false'\n @@ t/t0000-basic.sh: test_expect_success 'pretend we have a pass, fail, and known b\n \n test_expect_success 'pretend we have a mix of all possible results' \"\n -\ttest_must_fail run_sub_test_lib_test \\\n -+\t! run_sub_test_lib_test \\\n ++\trun_sub_test_lib_test_err \\\n \t\tmixed-results2 'mixed results #2' <<-\\\\EOF &&\n \ttest_expect_success 'passing test' 'true'\n \ttest_expect_success 'passing test' 'true'\n @@ t/t0000-basic.sh: test_expect_success 'pretend we have a mix of all possible res\n \n test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n -\ttest_must_fail run_sub_test_lib_test \\\n -+\t! run_sub_test_lib_test \\\n ++\trun_sub_test_lib_test_err \\\n \t\tt1234-verbose \"test verbose\" --verbose <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\n \ttest_expect_success \"test with output\" \"echo foo\"\n @@ t/t0000-basic.sh: test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n \n test_expect_success 'test --verbose-only' '\n -\ttest_must_fail run_sub_test_lib_test \\\n -+\t! run_sub_test_lib_test \\\n ++\trun_sub_test_lib_test_err \\\n \t\tt2345-verbose-only-2 \"test verbose-only=2\" \\\n \t\t--verbose-only=2 <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\n @@ t/t0000-basic.sh: then\n \n test_expect_success 'tests clean up even on failures' \"\n -\ttest_must_fail run_sub_test_lib_test \\\n -+\t! run_sub_test_lib_test \\\n ++\trun_sub_test_lib_test_err \\\n \t\tfailing-cleanup 'Failing tests with cleanup commands' <<-\\\\EOF &&\n \ttest_expect_success 'tests clean up even after a failure' '\n \t\ttouch clean-after-failure &&\n @@ t/t0000-basic.sh: test_expect_success 'tests clean up even on failures' \"\n \n test_expect_success 'test_atexit is run' \"\n -\ttest_must_fail run_sub_test_lib_test \\\n -+\t! run_sub_test_lib_test \\\n ++\trun_sub_test_lib_test_err \\\n \t\tatexit-cleanup 'Run atexit commands' -i <<-\\\\EOF &&\n \ttest_expect_success 'tests clean up even after a failure' '\n \t\t> ../../clean-atexit &&\n 2: f1acb2a0df ! 2: dbc82d45c6 t0003: use named parameters in attr_check()\n @@ Commit message\n variables instead of numbered arguments for clarity.\n \n While we're at it, add variable assignments to the &&-chain. These\n - aren't ever expected to fail but for stylistic purposes, include them\n - anyway for stylistic purposes.\n + aren't ever expected to fail but if a future developer ever adds some\n + code above the assignments and they could fail in some way, the intact\n + &&-chain will ensure that the failure is caught.\n \n ## t/t0003-attributes.sh ##\n @@ t/t0003-attributes.sh: test_description=gitattributes\n @@ t/t0003-attributes.sh: test_description=gitattributes\n \n attr_check () {\n -\tpath=\"$1\" expect=\"$2\"\n -+\tpath=\"$1\" &&\n -+\texpect=\"$2\" &&\n -+\tgit_opts=\"$3\" &&\n ++\tpath=\"$1\" expect=\"$2\" git_opts=\"$3\" &&\n \n -\tgit $3 check-attr test -- \"$path\" >actual 2>err &&\n -\techo \"$path: test: $2\" >expect &&\n @@ t/t0003-attributes.sh: test_description=gitattributes\n -\tpath=\"$1\"\n -\tquoted_path=\"$2\"\n -\texpect=\"$3\"\n -+\tpath=\"$1\" &&\n -+\tquoted_path=\"$2\" &&\n -+\texpect=\"$3\" &&\n ++\tpath=\"$1\" quoted_path=\"$2\" expect=\"$3\" &&\n \n \tgit check-attr test -- \"$path\" >actual &&\n \techo \"\\\"$quoted_path\\\": test: $expect\" >expect &&\n 3: 055caa5c08 = 3: e06a06cff5 t0003: use test_must_be_empty()\n 4: 3afa3a16ca = 4: 219011f983 t0003: don't use `test_must_fail attr_check`\n 5: d228dcfdc7 < -: ---------- t0020: drop redirections to /dev/null\n 6: 8adc5cd5aa ! 5: 8da6c96b39 t0020: s/test_must_fail has_cr/! has_cr/\n @@ Metadata\n Author: Denton Liu \n \n ## Commit message ##\n - t0020: s/test_must_fail has_cr/! has_cr/\n + t0020: don't use `test_must_fail has_cr`\n \n The test_must_fail function should only be used for git commands since\n we should assume that external commands work sanely. Since has_cr() just\n 7: b77b474422 = 6: 27550eaae6 t0020: use ! check_packed_refs_marked\n 8: d39422505f = 7: c19f6344a4 t1306: convert `test_might_fail rm` to `rm -f`\n 9: 2dd91c5568 = 8: d6ea8a6df0 t1307: reorder `nongit test_must_fail`\n10: 0b7d19a7e1 ! 9: d57dfe95e2 t1409: let sed open its own files\n @@ Metadata\n Author: Denton Liu \n \n ## Commit message ##\n - t1409: let sed open its own files\n + t1409: let sed open its own input file\n \n In one case, we were using a redirection operator to feed input into\n - sed. However, since sed is capable of opening its own files, make sed\n - open its own files instead of redirecting input into it.\n + sed. However, since sed is capable of opening its own input file, make\n + sed do that instead of redirecting input into it.\n \n ## t/t1409-avoid-packing-refs.sh ##\n @@ t/t1409-avoid-packing-refs.sh: test_description='avoid rewriting packed-refs unnecessarily'\n11: 27b3296242 ! 10: eacf4e0fb4 t1409: use test_path_is_missing()\n @@ Commit message\n The test_must_fail() function should only be used for git commands since\n we should assume that external commands work sanely. Replace\n `test_must_fail test -f` with `test_path_is_missing` since we expect\n - these files to not exist.\n + these paths to not exist.\n \n ## t/t1409-avoid-packing-refs.sh ##\n @@ t/t1409-avoid-packing-refs.sh: test_expect_success 'setup' '\n12: 3d36511d5d ! 11: 83e47748bc t1501: remove use of `test_might_fail cp`\n @@ Commit message\n should only be used on git commands. Replace test_might_fail() with\n test_non_git_might_fail().\n \n + The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix\n + test with split index, 2015-03-24). It is necessary because there might\n + exist some index files in `repo.git/sharedindex.*` and, if they exist,\n + we want to copy them over. However, if they don't exist, we don't want\n + to error out because we expect that possibility. As a result, we want to\n + keep the \"might fail\" semantics so we use test_non_git_might_fail().\n +\n ## t/t1501-work-tree.sh ##\n @@ t/t1501-work-tree.sh: test_expect_success 'Multi-worktree setup' '\n \tmkdir work &&\n13: cd392a74ac < -: ---------- t1507: teach full_name() to accept `!` arg\n -: ---------- > 12: 9e20865f94 t1507: stop losing return codes of git commands\n -: ---------- > 13: 7c61ac6b67 t1507: run commands within test_expect_success\n -: ---------- > 14: d09370455f t1507: inline full_name()\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388583","messageId":"219011f983e759338d41b661f480b1880bdec6ff.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 06/16] t0003: don't use `test_must_fail attr_check`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:41Z","receivedAt":"2019-12-19T22:21:45Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In an effort to remove test_must_fail for all invocations not related to\ngit or test-tool, replace invocations of `test_must_fail attr_check`\nwith a plain attr_check call with the $expect argument set to the\nactual value output by git.\n\nSigned-off-by: Denton Liu \n---\n t/t0003-attributes.sh | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex c30c736d3f..b660593c20 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -24,7 +24,7 @@ attr_check_quote () {\n \n test_expect_success 'open-quoted pathname' '\n \techo \"\\\"a test=a\" >.gitattributes &&\n-\ttest_must_fail attr_check a a\n+\tattr_check a unspecified\n '\n \n \n@@ -109,20 +109,20 @@ test_expect_success 'attribute test' '\n \n test_expect_success 'attribute matching is case sensitive when core.ignorecase=0' '\n \n-\ttest_must_fail attr_check F f \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/F f \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/c/F f \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/G a/g \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/B/g a/b/g \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/G a/b/g \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/H a/b/h \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/D/g \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check oNoFf unset \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check oFfOn set \"-c core.ignorecase=0\" &&\n+\tattr_check F unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/F unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/c/F unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/G unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/B/g a/g \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/G unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/H unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/D/g a/g \"-c core.ignorecase=0\" &&\n+\tattr_check oNoFf unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check oFfOn unspecified \"-c core.ignorecase=0\" &&\n \tattr_check NO unspecified \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/D/NO \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/D/NO unspecified \"-c core.ignorecase=0\" &&\n \tattr_check a/b/d/YES a/b/d/* \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/E/f \"A/e/F\" \"-c core.ignorecase=0\"\n+\tattr_check a/E/f f \"-c core.ignorecase=0\"\n \n '\n \n@@ -146,8 +146,8 @@ test_expect_success 'attribute matching is case insensitive when core.ignorecase\n '\n \n test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' '\n-\ttest_must_fail attr_check a/B/D/g \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check A/B/D/NO \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n+\tattr_check a/B/D/g a/g \"-c core.ignorecase=0\" &&\n+\tattr_check A/B/D/NO unspecified \"-c core.ignorecase=0\" &&\n \tattr_check A/b/h a/b/h \"-c core.ignorecase=1\" &&\n \tattr_check a/B/D/g \"a/b/d/*\" \"-c core.ignorecase=1\" &&\n \tattr_check A/B/D/NO \"a/b/d/*\" \"-c core.ignorecase=1\"\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388584","messageId":"8da6c96b39183bc1f6b8b40a2345876017d53921.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 07/16] t0020: don't use `test_must_fail has_cr`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:42Z","receivedAt":"2019-12-19T22:21:46Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. Since has_cr() just\nwraps a tr and grep pipeline, replace `test_must_fail has_cr` with\n`! has_cr`.\n\nSigned-off-by: Denton Liu \n---\n t/t0020-crlf.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 854da0ae16..b63ba62e5d 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -159,8 +159,8 @@ test_expect_success 'checkout with autocrlf=input' '\n \trm -f tmp one dir/two three &&\n \tgit config core.autocrlf input &&\n \tgit read-tree --reset -u HEAD &&\n-\ttest_must_fail has_cr one &&\n-\ttest_must_fail has_cr dir/two &&\n+\t! has_cr one &&\n+\t! has_cr dir/two &&\n \tgit update-index -- one dir/two &&\n \ttest \"$one\" = $(git hash-object --stdin .gitattributes &&\n \tgit read-tree --reset -u HEAD &&\n \n-\ttest_must_fail has_cr dir/two\n+\t! has_cr dir/two\n '\n \n test_expect_success '.gitattributes says two and three are text' '\n@@ -270,7 +270,7 @@ test_expect_success 'in-tree .gitattributes (1)' '\n \trm -rf tmp one dir .gitattributes patch.file three &&\n \tgit read-tree --reset -u HEAD &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n@@ -280,7 +280,7 @@ test_expect_success 'in-tree .gitattributes (2)' '\n \tgit read-tree --reset HEAD &&\n \tgit checkout-index -f -q -u -a &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n@@ -291,7 +291,7 @@ test_expect_success 'in-tree .gitattributes (3)' '\n \tgit checkout-index -u .gitattributes &&\n \tgit checkout-index -u one dir/two three &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n@@ -302,7 +302,7 @@ test_expect_success 'in-tree .gitattributes (4)' '\n \tgit checkout-index -u one dir/two three &&\n \tgit checkout-index -u .gitattributes &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388586","messageId":"dbc82d45c613e82a1905314e7b40eaf096c73102.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 04/16] t0003: use named parameters in attr_check()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:39Z","receivedAt":"2019-12-19T22:21:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We had named the parameters in attr_check() but $2 was being used\ninstead of $expect. Make all variable accesses in attr_check() use named\nvariables instead of numbered arguments for clarity.\n\nWhile we're at it, add variable assignments to the &&-chain. These\naren't ever expected to fail but if a future developer ever adds some\ncode above the assignments and they could fail in some way, the intact\n&&-chain will ensure that the failure is caught.\n\nSigned-off-by: Denton Liu \n---\n t/t0003-attributes.sh | 11 ++++-------\n 1 file changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 71e63d8b50..3569bef75d 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -5,19 +5,16 @@ test_description=gitattributes\n . ./test-lib.sh\n \n attr_check () {\n-\tpath=\"$1\" expect=\"$2\"\n+\tpath=\"$1\" expect=\"$2\" git_opts=\"$3\" &&\n \n-\tgit $3 check-attr test -- \"$path\" >actual 2>err &&\n-\techo \"$path: test: $2\" >expect &&\n+\tgit $git_opts check-attr test -- \"$path\" >actual 2>err &&\n+\techo \"$path: test: $expect\" >expect &&\n \ttest_cmp expect actual &&\n \ttest_line_count = 0 err\n }\n \n attr_check_quote () {\n-\n-\tpath=\"$1\"\n-\tquoted_path=\"$2\"\n-\texpect=\"$3\"\n+\tpath=\"$1\" quoted_path=\"$2\" expect=\"$3\" &&\n \n \tgit check-attr test -- \"$path\" >actual &&\n \techo \"\\\"$quoted_path\\\": test: $expect\" >expect &&\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388585","messageId":"778ae9052bb75c4f574519027917a78d6d61596b.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 03/16] t0000: replace test_must_fail with run_sub_test_lib_test_err()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:38Z","receivedAt":"2019-12-19T22:21:49Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. We use\ntest_must_fail to test run_sub_test_lib_test() but that function does\nnot invoke any git commands internally. Even better, we have a function\nthat's exactly meant to be used when we expect to have a failing test\nsuite: run_sub_test_lib_test_err()!\n\nReplace `test_must_fail run_sub_test_lib_test` with\n`run_sub_test_lib_test_err`.\n\nSigned-off-by: Denton Liu \n---\n t/t0000-basic.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 8a81a249d0..3e440c078d 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -155,7 +155,7 @@ test_expect_success 'pretend we have a fully passing test suite' \"\n \"\n \n test_expect_success 'pretend we have a partially passing test suite' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tpartial-pass '2/3 tests passing' <<-\\\\EOF &&\n \ttest_expect_success 'passing test #1' 'true'\n \ttest_expect_success 'failing test #2' 'false'\n@@ -219,7 +219,7 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su\n \"\n \n test_expect_success 'pretend we have a pass, fail, and known breakage' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tmixed-results1 'mixed results #1' <<-\\\\EOF &&\n \ttest_expect_success 'passing test' 'true'\n \ttest_expect_success 'failing test' 'false'\n@@ -238,7 +238,7 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' \"\n \"\n \n test_expect_success 'pretend we have a mix of all possible results' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tmixed-results2 'mixed results #2' <<-\\\\EOF &&\n \ttest_expect_success 'passing test' 'true'\n \ttest_expect_success 'passing test' 'true'\n@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' \"\n \"\n \n test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tt1234-verbose \"test verbose\" --verbose <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\n \ttest_expect_success \"test with output\" \"echo foo\"\n@@ -301,7 +301,7 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n '\n \n test_expect_success 'test --verbose-only' '\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tt2345-verbose-only-2 \"test verbose-only=2\" \\\n \t\t--verbose-only=2 <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\n@@ -834,7 +834,7 @@ then\n fi\n \n test_expect_success 'tests clean up even on failures' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tfailing-cleanup 'Failing tests with cleanup commands' <<-\\\\EOF &&\n \ttest_expect_success 'tests clean up even after a failure' '\n \t\ttouch clean-after-failure &&\n@@ -863,7 +863,7 @@ test_expect_success 'tests clean up even on failures' \"\n \"\n \n test_expect_success 'test_atexit is run' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tatexit-cleanup 'Run atexit commands' -i <<-\\\\EOF &&\n \ttest_expect_success 'tests clean up even after a failure' '\n \t\t> ../../clean-atexit &&\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388587","messageId":"d6ea8a6df0517c34136b0bddbce0948f9c8d448e.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 10/16] t1307: reorder `nongit test_must_fail`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:45Z","receivedAt":"2019-12-19T22:21:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the future, we plan on only allowing `test_must_fail` to work on a\nrestricted subset of commands, including `git`. Reorder the commands so\nthat `nongit` comes before `test_must_fail`. This way, `test_must_fail`\noperates on a git command.\n\nSigned-off-by: Denton Liu \n---\n t/t1307-config-blob.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh\nindex 37dc689d8c..002e6d3388 100755\n--- a/t/t1307-config-blob.sh\n+++ b/t/t1307-config-blob.sh\n@@ -74,7 +74,7 @@ test_expect_success 'can parse blob ending with CR' '\n '\n \n test_expect_success 'config --blob outside of a repository is an error' '\n-\ttest_must_fail nongit git config --blob=foo --list\n+\tnongit test_must_fail git config --blob=foo --list\n '\n \n test_done\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388588","messageId":"d57dfe95e2ace0b543ddb4fba6f9c14cac5f02fc.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 11/16] t1409: let sed open its own input file","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:46Z","receivedAt":"2019-12-19T22:21:52Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In one case, we were using a redirection operator to feed input into\nsed. However, since sed is capable of opening its own input file, make\nsed do that instead of redirecting input into it.\n\nSigned-off-by: Denton Liu \n---\n t/t1409-avoid-packing-refs.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex c46848eb8e..f74d890e82 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -8,7 +8,7 @@ test_description='avoid rewriting packed-refs unnecessarily'\n # shouldn't upset readers, and it should be omitted if the file is\n # ever rewritten.\n mark_packed_refs () {\n-\tsed -e \"s/^\\(#.*\\)/\\1 t1409 /\" <.git/packed-refs >.git/packed-refs.new &&\n+\tsed -e \"s/^\\(#.*\\)/\\1 t1409 /\" .git/packed-refs >.git/packed-refs.new &&\n \tmv .git/packed-refs.new .git/packed-refs\n }\n \n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388589","messageId":"eacf4e0fb499ab7dd4b7dad4826b1ea73d120ebc.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 12/16] t1409: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:47Z","receivedAt":"2019-12-19T22:21:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() function should only be used for git commands since\nwe should assume that external commands work sanely. Replace\n`test_must_fail test -f` with `test_path_is_missing` since we expect\nthese paths to not exist.\n\nSigned-off-by: Denton Liu \n---\n t/t1409-avoid-packing-refs.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex f74d890e82..be12fb6350 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -27,15 +27,15 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'do not create packed-refs file gratuitously' '\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref refs/heads/foo $A &&\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref refs/heads/foo $B &&\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref refs/heads/foo $C $B &&\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref -d refs/heads/foo &&\n-\ttest_must_fail test -f .git/packed-refs\n+\ttest_path_is_missing .git/packed-refs\n '\n \n test_expect_success 'check that marking the packed-refs file works' '\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388590","messageId":"c19f6344a4f76ec00c88581c1f05f4f48baaea39.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 09/16] t1306: convert `test_might_fail rm` to `rm -f`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:44Z","receivedAt":"2019-12-19T22:21:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() family of functions (including test_might_fail())\nshould only be used on git commands. Replace `test_might_fail rm` with\n`rm -f` so that we don't use `test_might_fail` on a non-git command.\n\nSigned-off-by: Denton Liu \n---\n t/t1306-xdg-files.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh\nindex 21e139a313..dd87b43be1 100755\n--- a/t/t1306-xdg-files.sh\n+++ b/t/t1306-xdg-files.sh\n@@ -153,7 +153,7 @@ test_expect_success 'Checking attributes in both XDG and local attributes files'\n \n \n test_expect_success 'Checking attributes in a non-XDG global attributes file' '\n-\ttest_might_fail rm .gitattributes &&\n+\trm -f .gitattributes &&\n \techo \"f attr_f=test\" >\"$HOME\"/my_gitattributes &&\n \tgit config core.attributesfile \"$HOME\"/my_gitattributes &&\n \techo \"f: attr_f: test\" >expected &&\n@@ -165,7 +165,7 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '\n test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\\''t' '\n \tmkdir -p \"$HOME\"/.config/git &&\n \t>\"$HOME\"/.config/git/config &&\n-\ttest_might_fail rm \"$HOME\"/.gitconfig &&\n+\trm -f \"$HOME\"/.gitconfig &&\n \tgit config --global user.name \"write_config\" &&\n \techo \"[user]\" >expected &&\n \techo \"\tname = write_config\" >>expected &&\n@@ -183,8 +183,8 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '\n \n \n test_expect_success 'write: ~/.config/git/ exists and config file doesn'\\''t' '\n-\ttest_might_fail rm \"$HOME\"/.gitconfig &&\n-\ttest_might_fail rm \"$HOME\"/.config/git/config &&\n+\trm -f \"$HOME\"/.gitconfig &&\n+\trm -f \"$HOME\"/.config/git/config &&\n \tgit config --global user.name \"write_gitconfig\" &&\n \techo \"[user]\" >expected &&\n \techo \"\tname = write_gitconfig\" >>expected &&\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388591","messageId":"27550eaae6d4944be6090541a4940c818099216c.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 08/16] t0020: use ! check_packed_refs_marked","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:43Z","receivedAt":"2019-12-19T22:21:54Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. Since\ncheck_packed_refs_marked() just wraps a grep invocation, replace\n`test_must_fail check_packed_refs_marked` with\n`! check_packed_refs_marked`.\n\nSigned-off-by: Denton Liu \n---\n t/t1409-avoid-packing-refs.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex e5cb8a252d..c46848eb8e 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -46,7 +46,7 @@ test_expect_success 'check that marking the packed-refs file works' '\n \tgit for-each-ref >actual &&\n \ttest_cmp expected actual &&\n \tgit pack-refs --all &&\n-\ttest_must_fail check_packed_refs_marked &&\n+\t! check_packed_refs_marked &&\n \tgit for-each-ref >actual2 &&\n \ttest_cmp expected actual2\n '\n@@ -80,7 +80,7 @@ test_expect_success 'touch packed-refs on delete of packed' '\n \tgit pack-refs --all &&\n \tmark_packed_refs &&\n \tgit update-ref -d refs/heads/packed-delete &&\n-\ttest_must_fail check_packed_refs_marked\n+\t! check_packed_refs_marked\n '\n \n test_expect_success 'leave packed-refs untouched on update of loose' '\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388592","messageId":"e06a06cff5206d963d24074bdac06d679540d483.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 05/16] t0003: use test_must_be_empty()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:40Z","receivedAt":"2019-12-19T22:21:55Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In several places, we used `test_line_count = 0` to check for an empty\nfile. Although this is correct, it's overkill. Use test_must_be_empty()\ninstead because it's more suited for this purpose.\n\nSigned-off-by: Denton Liu \n---\n t/t0003-attributes.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 3569bef75d..c30c736d3f 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -10,7 +10,7 @@ attr_check () {\n \tgit $git_opts check-attr test -- \"$path\" >actual 2>err &&\n \techo \"$path: test: $expect\" >expect &&\n \ttest_cmp expect actual &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n }\n \n attr_check_quote () {\n@@ -241,7 +241,7 @@ EOF\n \tgit check-attr foo -- \"a/b/f\" >>actual 2>>err &&\n \tgit check-attr foo -- \"a/b/c/f\" >>actual 2>>err &&\n \ttest_cmp expect actual &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n \n test_expect_success '\"**\" with no slashes test' '\n@@ -262,7 +262,7 @@ EOF\n \tgit check-attr foo -- \"a/b/f\" >>actual 2>>err &&\n \tgit check-attr foo -- \"a/b/c/f\" >>actual 2>>err &&\n \ttest_cmp expect actual &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n \n test_expect_success 'using --git-dir and --work-tree' '\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388593","messageId":"83e47748bc9c541c725f6c42c553b1a69fd717ac.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 13/16] t1501: remove use of `test_might_fail cp`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:48Z","receivedAt":"2019-12-19T22:21:57Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() family of functions (including test_might_fail())\nshould only be used on git commands. Replace test_might_fail() with\ntest_non_git_might_fail().\n\nThe `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix\ntest with split index, 2015-03-24). It is necessary because there might\nexist some index files in `repo.git/sharedindex.*` and, if they exist,\nwe want to copy them over. However, if they don't exist, we don't want\nto error out because we expect that possibility. As a result, we want to\nkeep the \"might fail\" semantics so we use test_non_git_might_fail().\n\nSigned-off-by: Denton Liu \n---\n t/t1501-work-tree.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\nindex 3498d3d55e..067301a5ab 100755\n--- a/t/t1501-work-tree.sh\n+++ b/t/t1501-work-tree.sh\n@@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '\n \tmkdir work &&\n \tmkdir -p repo.git/repos/foo &&\n \tcp repo.git/HEAD repo.git/index repo.git/repos/foo &&\n-\ttest_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n+\ttest_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n \tsane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE\n '\n \n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388594","messageId":"9e20865f94bd187dd1e50c1ddd57c435882cd6a0.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 14/16] t1507: stop losing return codes of git commands","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:49Z","receivedAt":"2019-12-19T22:21:57Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The return code of git commands are lost when a command is in a\nnon-assignment command substitution in favour of the surrounding\ncommand's. Rewrite instances of this so that git commands run\non their own.\n\nIn commit_subject(), use a `tformat` instead of `format` since,\npreviously, we were testing the output of a command substitution which\ndidn't care if there was a trailing newline since it was automatically\nstripped. Since we use test_cmp() now, the trailing newline matters so\nuse `tformat` to always output it.\n\nSigned-off-by: Denton Liu \n---\n t/t1507-rev-parse-upstream.sh | 45 +++++++++++++++++++++++++----------\n 1 file changed, 32 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex 8b4cf8a6e3..d81f289ace 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -35,7 +35,7 @@ full_name () {\n \n commit_subject () {\n \t(cd clone &&\n-\t git show -s --pretty=format:%s \"$@\")\n+\t git show -s --pretty=tformat:%s \"$@\")\n }\n \n error_message () {\n@@ -44,18 +44,27 @@ error_message () {\n }\n \n test_expect_success '@{upstream} resolves to correct full name' '\n-\ttest refs/remotes/origin/master = \"$(full_name @{upstream})\" &&\n-\ttest refs/remotes/origin/master = \"$(full_name @{UPSTREAM})\" &&\n-\ttest refs/remotes/origin/master = \"$(full_name @{UpSTReam})\"\n+\techo refs/remotes/origin/master >expect &&\n+\tfull_name @{upstream} >actual &&\n+\ttest_cmp expect actual &&\n+\tfull_name @{UPSTREAM} >actual &&\n+\ttest_cmp expect actual &&\n+\tfull_name @{UpSTReam} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '@{u} resolves to correct full name' '\n-\ttest refs/remotes/origin/master = \"$(full_name @{u})\" &&\n-\ttest refs/remotes/origin/master = \"$(full_name @{U})\"\n+\techo refs/remotes/origin/master >expect &&\n+\tfull_name @{u} >actual &&\n+\ttest_cmp expect actual &&\n+\tfull_name @{U} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'my-side@{upstream} resolves to correct full name' '\n-\ttest refs/remotes/origin/side = \"$(full_name my-side@{u})\"\n+\techo refs/remotes/origin/side >expect &&\n+\tfull_name my-side@{u} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'upstream of branch with @ in middle' '\n@@ -86,8 +95,11 @@ test_expect_success 'my-side@{u} resolves to correct commit' '\n \tgit checkout side &&\n \ttest_commit 5 &&\n \t(cd clone && git fetch) &&\n-\ttest 2 = \"$(commit_subject my-side)\" &&\n-\ttest 5 = \"$(commit_subject my-side@{u})\"\n+\techo 2 >expect &&\n+\tcommit_subject my-side >actual &&\n+\ttest_cmp expect actual &&\n+\techo 5 >expect &&\n+\tcommit_subject my-side@{u} >actual\n '\n \n test_expect_success 'not-tracking@{u} fails' '\n@@ -99,8 +111,11 @@ test_expect_success 'not-tracking@{u} fails' '\n test_expect_success '@{u}@{1} resolves correctly' '\n \ttest_commit 6 &&\n \t(cd clone && git fetch) &&\n-\ttest 5 = $(commit_subject my-side@{u}@{1}) &&\n-\ttest 5 = $(commit_subject my-side@{U}@{1})\n+\techo 5 >expect &&\n+\tcommit_subject my-side@{u}@{1} >actual &&\n+\ttest_cmp expect actual &&\n+\tcommit_subject my-side@{U}@{1} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '@{u} without specifying branch fails on a detached HEAD' '\n@@ -149,7 +164,9 @@ test_expect_success 'checkout other@{u}' '\n '\n \n test_expect_success 'branch@{u} works when tracking a local branch' '\n-\ttest refs/heads/master = \"$(full_name local-master@{u})\"\n+\techo refs/heads/master >expect &&\n+\tfull_name local-master@{u} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'branch@{u} error message when no upstream' '\n@@ -203,7 +220,9 @@ test_expect_success 'pull works when tracking a local branch' '\n \n # makes sense if the previous one succeeded\n test_expect_success '@{u} works when tracking a local branch' '\n-\ttest refs/heads/master = \"$(full_name @{u})\"\n+\techo refs/heads/master >expect &&\n+\tfull_name @{u} >actual &&\n+\ttest_cmp expect actual\n '\n \n commit=$(git rev-parse HEAD)\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388595","messageId":"d09370455ffc9de30c0ca7ecce100754f725728a.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 16/16] t1507: inline full_name()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:51Z","receivedAt":"2019-12-19T22:21:58Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, we were running `test_must_fail full_name`. However,\n`test_must_fail` should only be used on git commands. Inline full_name()\nso that we can use test_must_fail on the git command directly.\n\nWhen full_name() was introduced in 28fb84382b (Introduce\n@{upstream} notation, 2009-09-10), the `git -C` option wasn't\navailable yet (since it was introduced in 44e1e4d67d (git: run in a\ndirectory given with -C option, 2013-09-09)). As a result, the helper\nfunction removed the need to manually cd each time. However, since\n`git -C` is available now, we can just use that instead and inline\nfull_name().\n\nAn alternate approach was taken where we taught full_name() to accept an\noptional `!` arg to trigger test_must_fail behavior. However, this added\nmore unnecessary complexity than inlining so we inline instead.\n\nSigned-off-by: Denton Liu \n---\n t/t1507-rev-parse-upstream.sh | 35 +++++++++++++++--------------------\n 1 file changed, 15 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex f68b77e7ba..dfc0d96d8a 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -28,11 +28,6 @@ test_expect_success 'setup' '\n \t)\n '\n \n-full_name () {\n-\t(cd clone &&\n-\t git rev-parse --symbolic-full-name \"$@\")\n-}\n-\n commit_subject () {\n \t(cd clone &&\n \t git show -s --pretty=tformat:%s \"$@\")\n@@ -45,50 +40,50 @@ error_message () {\n \n test_expect_success '@{upstream} resolves to correct full name' '\n \techo refs/remotes/origin/master >expect &&\n-\tfull_name @{upstream} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{upstream} >actual &&\n \ttest_cmp expect actual &&\n-\tfull_name @{UPSTREAM} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{UPSTREAM} >actual &&\n \ttest_cmp expect actual &&\n-\tfull_name @{UpSTReam} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{UpSTReam} >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '@{u} resolves to correct full name' '\n \techo refs/remotes/origin/master >expect &&\n-\tfull_name @{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{u} >actual &&\n \ttest_cmp expect actual &&\n-\tfull_name @{U} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{U} >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'my-side@{upstream} resolves to correct full name' '\n \techo refs/remotes/origin/side >expect &&\n-\tfull_name my-side@{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name my-side@{u} >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'upstream of branch with @ in middle' '\n-\tfull_name fun@ny@{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name fun@ny@{u} >actual &&\n \techo refs/remotes/origin/side >expect &&\n \ttest_cmp expect actual &&\n-\tfull_name fun@ny@{U} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name fun@ny@{U} >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'upstream of branch with @ at start' '\n-\tfull_name @funny@{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @funny@{u} >actual &&\n \techo refs/remotes/origin/side >expect &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'upstream of branch with @ at end' '\n-\tfull_name funny@@{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name funny@@{u} >actual &&\n \techo refs/remotes/origin/side >expect &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' '\n-\ttest_must_fail full_name refs/heads/my-side@{upstream}\n+\ttest_must_fail git -C clone rev-parse --symbolic-full-name refs/heads/my-side@{upstream}\n '\n \n test_expect_success 'my-side@{u} resolves to correct commit' '\n@@ -103,9 +98,9 @@ test_expect_success 'my-side@{u} resolves to correct commit' '\n '\n \n test_expect_success 'not-tracking@{u} fails' '\n-\ttest_must_fail full_name non-tracking@{u} &&\n+\ttest_must_fail git -C clone rev-parse --symbolic-full-name non-tracking@{u} &&\n \t(cd clone && git checkout --no-track -b non-tracking) &&\n-\ttest_must_fail full_name non-tracking@{u}\n+\ttest_must_fail git -C clone rev-parse --symbolic-full-name non-tracking@{u}\n '\n \n test_expect_success '@{u}@{1} resolves correctly' '\n@@ -165,7 +160,7 @@ test_expect_success 'checkout other@{u}' '\n \n test_expect_success 'branch@{u} works when tracking a local branch' '\n \techo refs/heads/master >expect &&\n-\tfull_name local-master@{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name local-master@{u} >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -221,7 +216,7 @@ test_expect_success 'pull works when tracking a local branch' '\n # makes sense if the previous one succeeded\n test_expect_success '@{u} works when tracking a local branch' '\n \techo refs/heads/master >expect &&\n-\tfull_name @{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{u} >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388596","messageId":"7c61ac6b67a32da86fcdafaf4a69f2662efb4bd0.1576794144.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v2 15/16] t1507: run commands within test_expect_success","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:22:50Z","receivedAt":"2019-12-19T22:21:59Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The expected test style is to have all commands tested within a\ntest_expect_success block. Move the generation of the 'expect' text into\ntheir corresponding blocks. While we're at it, insert a second\n`commit=$(git rev-parse HEAD)` into the next test case so that it's\nclear where $commit is coming from.\n\nThe biggest advantage of doing this is that we now check the return code\nof `git rev-parse HEAD` so we can catch it in case it fails.\n\nThis patch is best viewed with `--color-moved --ignore-all-space`.\n\nSigned-off-by: Denton Liu \n---\n t/t1507-rev-parse-upstream.sh | 40 +++++++++++++++++------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex d81f289ace..f68b77e7ba 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -225,32 +225,32 @@ test_expect_success '@{u} works when tracking a local branch' '\n \ttest_cmp expect actual\n '\n \n-commit=$(git rev-parse HEAD)\n-cat >expect <)\n-Reflog message: branch: Created from HEAD\n-Author: A U Thor \n-Date: Thu Apr 7 15:15:13 2005 -0700\n-\n- 3\n-EOF\n test_expect_success 'log -g other@{u}' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect <<-EOF &&\n+\tcommit $commit\n+\tReflog: master@{0} (C O Mitter )\n+\tReflog message: branch: Created from HEAD\n+\tAuthor: A U Thor \n+\tDate: Thu Apr 7 15:15:13 2005 -0700\n+\n+\t 3\n+\tEOF\n \tgit log -1 -g other@{u} >actual &&\n \ttest_cmp expect actual\n '\n \n-cat >expect <)\n-Reflog message: branch: Created from HEAD\n-Author: A U Thor \n-Date: Thu Apr 7 15:15:13 2005 -0700\n-\n- 3\n-EOF\n-\n test_expect_success 'log -g other@{u}@{now}' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect <<-EOF &&\n+\tcommit $commit\n+\tReflog: master@{Thu Apr 7 15:17:13 2005 -0700} (C O Mitter )\n+\tReflog message: branch: Created from HEAD\n+\tAuthor: A U Thor \n+\tDate: Thu Apr 7 15:15:13 2005 -0700\n+\n+\t 3\n+\tEOF\n \tgit log -1 -g other@{u}@{now} >actual &&\n \ttest_cmp expect actual\n '\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388600","messageId":"CAPig+cQuPDDun3NxnTwvmjo9zvPzSq+5f2utkHkS0f1c1Nwbvg@mail.gmail.com","threadId":"52474","inReplyTo":"83e47748bc9c541c725f6c42c553b1a69fd717ac.1576794144.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 13/16] t1501: remove use of `test_might_fail cp`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-19T22:52:56Z","receivedAt":"2019-12-19T22:53:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 19, 2019 at 5:22 PM Denton Liu wrote:\n> The test_must_fail() family of functions (including test_might_fail())\n> should only be used on git commands. Replace test_might_fail() with\n> test_non_git_might_fail().\n>\n> The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix\n> test with split index, 2015-03-24). It is necessary because there might\n> exist some index files in `repo.git/sharedindex.*` and, if they exist,\n> we want to copy them over. However, if they don't exist, we don't want\n> to error out because we expect that possibility. As a result, we want to\n> keep the \"might fail\" semantics so we use test_non_git_might_fail().\n\nThanks for adding this paragraph to help the reader understand why you\nchose this transformation. However...\n\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\n> @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '\n> cp repo.git/HEAD repo.git/index repo.git/repos/foo &&\n> - test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n> + test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n\nI can't say I'm a fan of patch 1 introducing the function\ntest_non_git_might_fail() for this one particular case. I'd rather see\nthis case follow existing precedence by being written this way:\n\n { cp repo.git/sharedindex.* repo.git/repos/foo || :; } &&\n\nwhich is the idiomatic way this sort of thing is handled in existing tests.\n\nWhile it's true that it may look a bit cryptic to people new to shell\nscripting, as with any idiom, it's understood at a glance by people\nfamiliar with it. That bit about \"at a glance\" is important: it's much\neasier to comprehend idiomatic code than code which you have to spend\na lot of time _reading_ (and \"test_non_git_might_fail\" is quite a\nmouthful, or eyeful, or something, which takes a lot more effort to\nread and understand).\n"},{"id":"388601","messageId":"20191219231940.GA67903@generichostname","threadId":"52474","inReplyTo":"CAPig+cQuPDDun3NxnTwvmjo9zvPzSq+5f2utkHkS0f1c1Nwbvg@mail.gmail.com","subject":"Re: [PATCH v2 13/16] t1501: remove use of `test_might_fail cp`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T23:19:40Z","receivedAt":"2019-12-19T23:18:08Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Eric,\n\nOn Thu, Dec 19, 2019 at 05:52:56PM -0500, Eric Sunshine wrote:\n> > diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\n> > @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '\n> > cp repo.git/HEAD repo.git/index repo.git/repos/foo &&\n> > - test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n> > + test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n> \n> I can't say I'm a fan of patch 1 introducing the function\n> test_non_git_might_fail() for this one particular case. I'd rather see\n> this case follow existing precedence by being written this way:\n> \n> { cp repo.git/sharedindex.* repo.git/repos/foo || :; } &&\n> \n> which is the idiomatic way this sort of thing is handled in existing tests.\n> \n> While it's true that it may look a bit cryptic to people new to shell\n> scripting, as with any idiom, it's understood at a glance by people\n> familiar with it. That bit about \"at a glance\" is important: it's much\n> easier to comprehend idiomatic code than code which you have to spend\n> a lot of time _reading_ (and \"test_non_git_might_fail\" is quite a\n> mouthful, or eyeful, or something, which takes a lot more effort to\n> read and understand).\n\nThe reason why I chose to do this was because I found myself writing the\nabove many times in (currently unsent) later test cases that I cleaned\nup. As a result, I felt like it could be wrapped up more nicely with a\nhelper function. That being said, if you think that open coding the\nidiom looks nicer, I can reroll to inline it.\n"},{"id":"388633","messageId":"CAPig+cSy7HX9K=fAfgQk1en4zzu1HYKbSmjSZHhAVqg-RcAcPw@mail.gmail.com","threadId":"52474","inReplyTo":"20191219231940.GA67903@generichostname","subject":"Re: [PATCH v2 13/16] t1501: remove use of `test_might_fail cp`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-20T17:32:57Z","receivedAt":"2019-12-20T17:33:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 19, 2019 at 6:18 PM Denton Liu wrote:\n> On Thu, Dec 19, 2019 at 05:52:56PM -0500, Eric Sunshine wrote:\n> > [...] I'd rather see:\n> > { cp repo.git/sharedindex.* repo.git/repos/foo || :; } &&\n> > which is the idiomatic way this sort of thing is handled in existing tests.\n> >\n> > While it's true that it may look a bit cryptic to people new to shell\n> > scripting, as with any idiom, it's understood at a glance by people\n> > familiar with it. That bit about \"at a glance\" is important: it's much\n> > easier to comprehend idiomatic code than code which you have to spend\n> > a lot of time _reading_ [...]\n>\n> The reason why I chose to do this was because I found myself writing the\n> above many times in (currently unsent) later test cases that I cleaned\n> up. As a result, I felt like it could be wrapped up more nicely with a\n> helper function. That being said, if you think that open coding the\n> idiom looks nicer, I can reroll to inline it.\n\nI wouldn't say that the idiom \"looks nicer\", but rather that it has\nvalue specifically because it is an idiom, therefore it reduces\ncognitive load (as explained above).\n\nIdioms aside, the new function test_non_git_might_fail() may increase\ncognitive load because the reader needs to remember its purpose and\nbehavior since it's a black box compared to the open-coded version,\nyet adds no actual value. Compare that with, say, test_must_fail()\nwhich not only adds value but would significantly increase cognitive\nload if open-coded, thus is well justified. That's not to say that\none-liner functions can't necessarily have value; a well named\nfunction or one which introduces an important abstraction may very\nwell be worthwhile, but I can't convince myself that\ntest_non_git_might_fail() succeeds in that way.\n\nSo, to answer your question, my preference leans toward open-coding this.\n"},{"id":"388637","messageId":"cover.1576865663.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576794144.git.liu.denton@gmail.com","subject":"[PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:47Z","receivedAt":"2019-12-20T18:14:34Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The overall scope of these patches is to replace inappropriate uses of\ntest_must_fail. IOW, we should only allow test_must_fail to run on `git`\nand `test-tool`. Ultimately, we will conclude by making test_must_fail\nerror out on non-git commands. An advance view of the final series can\nbe found here[1].\n\nThis is the first part. It focuses on t[01]*.sh and also t/lib-git-p4.\n\nChanges since v2:\n\n* Drop the inclusion of test_non_git_must_fail() and open code it\n instead\n\nChanges since v1:\n\n* Incorporate review comments by Junio, Eric and J6t\n\n* Further cleanup of t1507 before inlining full_name() (since we didn't\n want to inline full_name() into a bad command substitution)\n\n[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2\n\nDenton Liu (15):\n t/lib-git-p4: use test_path_is_missing()\n t0000: replace test_must_fail with run_sub_test_lib_test_err()\n t0003: use named parameters in attr_check()\n t0003: use test_must_be_empty()\n t0003: don't use `test_must_fail attr_check`\n t0020: don't use `test_must_fail has_cr`\n t0020: use ! check_packed_refs_marked\n t1306: convert `test_might_fail rm` to `rm -f`\n t1307: reorder `nongit test_must_fail`\n t1409: let sed open its own input file\n t1409: use test_path_is_missing()\n t1501: remove use of `test_might_fail cp`\n t1507: stop losing return codes of git commands\n t1507: run commands within test_expect_success\n t1507: inline full_name()\n\n t/lib-git-p4.sh | 2 +-\n t/t0000-basic.sh | 14 ++---\n t/t0003-attributes.sh | 47 +++++++--------\n t/t0020-crlf.sh | 18 +++---\n t/t1306-xdg-files.sh | 8 +--\n t/t1307-config-blob.sh | 2 +-\n t/t1409-avoid-packing-refs.sh | 16 +++---\n t/t1501-work-tree.sh | 2 +-\n t/t1507-rev-parse-upstream.sh | 104 +++++++++++++++++++---------------\n 9 files changed, 112 insertions(+), 101 deletions(-)\n\nRange-diff against v2:\n 1: 85cee92765 < -: ---------- test-lib-functions: introduce test_non_git_might_fail()\n 2: 3d64837af1 = 1: cf3dd04b8a t/lib-git-p4: use test_path_is_missing()\n 3: 778ae9052b = 2: 51a2226726 t0000: replace test_must_fail with run_sub_test_lib_test_err()\n 4: dbc82d45c6 = 3: 9374fcd8db t0003: use named parameters in attr_check()\n 5: e06a06cff5 = 4: 7f8808a850 t0003: use test_must_be_empty()\n 6: 219011f983 = 5: b725d53dbe t0003: don't use `test_must_fail attr_check`\n 7: 8da6c96b39 = 6: f6ef6d245c t0020: don't use `test_must_fail has_cr`\n 8: 27550eaae6 = 7: fc32b8d584 t0020: use ! check_packed_refs_marked\n 9: c19f6344a4 = 8: 7e29b0154e t1306: convert `test_might_fail rm` to `rm -f`\n10: d6ea8a6df0 = 9: cf43579d65 t1307: reorder `nongit test_must_fail`\n11: d57dfe95e2 = 10: e2fe278bfa t1409: let sed open its own input file\n12: eacf4e0fb4 = 11: 1eef3f4bc5 t1409: use test_path_is_missing()\n13: 83e47748bc ! 12: fddd224225 t1501: remove use of `test_might_fail cp`\n @@ Commit message\n \n The test_must_fail() family of functions (including test_might_fail())\n should only be used on git commands. Replace test_might_fail() with\n - test_non_git_might_fail().\n + a compound command wrapping the old cp invocation that always returns 0.\n \n The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix\n test with split index, 2015-03-24). It is necessary because there might\n exist some index files in `repo.git/sharedindex.*` and, if they exist,\n we want to copy them over. However, if they don't exist, we don't want\n to error out because we expect that possibility. As a result, we want to\n - keep the \"might fail\" semantics so we use test_non_git_might_fail().\n + keep the \"might fail\" semantics so we always return 0, even if the\n + underlying cp errors out.\n \n ## t/t1501-work-tree.sh ##\n @@ t/t1501-work-tree.sh: test_expect_success 'Multi-worktree setup' '\n @@ t/t1501-work-tree.sh: test_expect_success 'Multi-worktree setup' '\n \tmkdir -p repo.git/repos/foo &&\n \tcp repo.git/HEAD repo.git/index repo.git/repos/foo &&\n -\ttest_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n -+\ttest_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n ++\t{ cp repo.git/sharedindex.* repo.git/repos/foo || :; } &&\n \tsane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE\n '\n \n14: 9e20865f94 = 13: 63ca18207d t1507: stop losing return codes of git commands\n15: 7c61ac6b67 = 14: 44a410d57a t1507: run commands within test_expect_success\n16: d09370455f = 15: 4fe445279b t1507: inline full_name()\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388638","messageId":"cf3dd04b8af8c23b242584f29d1dcfbc0b59d331.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 01/15] t/lib-git-p4: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:48Z","receivedAt":"2019-12-20T18:14:36Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Previously, cleanup_git() would use `test_must_fail test -d` to ensure\nthat the directory is removed. However, test_must_fail should only be\nused for git commands. Use test_path_is_missing() instead to check that\nthe directory has been removed.\n\nSigned-off-by: Denton Liu \n---\n t/lib-git-p4.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex 547b9f88e1..5aff2abe8b 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -175,7 +175,7 @@ stop_and_cleanup_p4d () {\n \n cleanup_git () {\n \tretry_until_success rm -r \"$git\"\n-\ttest_must_fail test -d \"$git\" &&\n+\ttest_path_is_missing \"$git\" &&\n \tretry_until_success mkdir \"$git\"\n }\n \n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388639","messageId":"51a2226726c941c1229e6aafa355376fdb6c61c5.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 02/15] t0000: replace test_must_fail with run_sub_test_lib_test_err()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:49Z","receivedAt":"2019-12-20T18:14:38Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. We use\ntest_must_fail to test run_sub_test_lib_test() but that function does\nnot invoke any git commands internally. Even better, we have a function\nthat's exactly meant to be used when we expect to have a failing test\nsuite: run_sub_test_lib_test_err()!\n\nReplace `test_must_fail run_sub_test_lib_test` with\n`run_sub_test_lib_test_err`.\n\nSigned-off-by: Denton Liu \n---\n t/t0000-basic.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 8a81a249d0..3e440c078d 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -155,7 +155,7 @@ test_expect_success 'pretend we have a fully passing test suite' \"\n \"\n \n test_expect_success 'pretend we have a partially passing test suite' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tpartial-pass '2/3 tests passing' <<-\\\\EOF &&\n \ttest_expect_success 'passing test #1' 'true'\n \ttest_expect_success 'failing test #2' 'false'\n@@ -219,7 +219,7 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su\n \"\n \n test_expect_success 'pretend we have a pass, fail, and known breakage' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tmixed-results1 'mixed results #1' <<-\\\\EOF &&\n \ttest_expect_success 'passing test' 'true'\n \ttest_expect_success 'failing test' 'false'\n@@ -238,7 +238,7 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' \"\n \"\n \n test_expect_success 'pretend we have a mix of all possible results' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tmixed-results2 'mixed results #2' <<-\\\\EOF &&\n \ttest_expect_success 'passing test' 'true'\n \ttest_expect_success 'passing test' 'true'\n@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' \"\n \"\n \n test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tt1234-verbose \"test verbose\" --verbose <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\n \ttest_expect_success \"test with output\" \"echo foo\"\n@@ -301,7 +301,7 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n '\n \n test_expect_success 'test --verbose-only' '\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tt2345-verbose-only-2 \"test verbose-only=2\" \\\n \t\t--verbose-only=2 <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\n@@ -834,7 +834,7 @@ then\n fi\n \n test_expect_success 'tests clean up even on failures' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tfailing-cleanup 'Failing tests with cleanup commands' <<-\\\\EOF &&\n \ttest_expect_success 'tests clean up even after a failure' '\n \t\ttouch clean-after-failure &&\n@@ -863,7 +863,7 @@ test_expect_success 'tests clean up even on failures' \"\n \"\n \n test_expect_success 'test_atexit is run' \"\n-\ttest_must_fail run_sub_test_lib_test \\\n+\trun_sub_test_lib_test_err \\\n \t\tatexit-cleanup 'Run atexit commands' -i <<-\\\\EOF &&\n \ttest_expect_success 'tests clean up even after a failure' '\n \t\t> ../../clean-atexit &&\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388640","messageId":"9374fcd8db831b411c1485c204cf8d029426e966.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 03/15] t0003: use named parameters in attr_check()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:50Z","receivedAt":"2019-12-20T18:14:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We had named the parameters in attr_check() but $2 was being used\ninstead of $expect. Make all variable accesses in attr_check() use named\nvariables instead of numbered arguments for clarity.\n\nWhile we're at it, add variable assignments to the &&-chain. These\naren't ever expected to fail but if a future developer ever adds some\ncode above the assignments and they could fail in some way, the intact\n&&-chain will ensure that the failure is caught.\n\nSigned-off-by: Denton Liu \n---\n t/t0003-attributes.sh | 11 ++++-------\n 1 file changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 71e63d8b50..3569bef75d 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -5,19 +5,16 @@ test_description=gitattributes\n . ./test-lib.sh\n \n attr_check () {\n-\tpath=\"$1\" expect=\"$2\"\n+\tpath=\"$1\" expect=\"$2\" git_opts=\"$3\" &&\n \n-\tgit $3 check-attr test -- \"$path\" >actual 2>err &&\n-\techo \"$path: test: $2\" >expect &&\n+\tgit $git_opts check-attr test -- \"$path\" >actual 2>err &&\n+\techo \"$path: test: $expect\" >expect &&\n \ttest_cmp expect actual &&\n \ttest_line_count = 0 err\n }\n \n attr_check_quote () {\n-\n-\tpath=\"$1\"\n-\tquoted_path=\"$2\"\n-\texpect=\"$3\"\n+\tpath=\"$1\" quoted_path=\"$2\" expect=\"$3\" &&\n \n \tgit check-attr test -- \"$path\" >actual &&\n \techo \"\\\"$quoted_path\\\": test: $expect\" >expect &&\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388641","messageId":"7f8808a850cd01a605794fc5b5bb276330d7abb5.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 04/15] t0003: use test_must_be_empty()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:51Z","receivedAt":"2019-12-20T18:14:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In several places, we used `test_line_count = 0` to check for an empty\nfile. Although this is correct, it's overkill. Use test_must_be_empty()\ninstead because it's more suited for this purpose.\n\nSigned-off-by: Denton Liu \n---\n t/t0003-attributes.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 3569bef75d..c30c736d3f 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -10,7 +10,7 @@ attr_check () {\n \tgit $git_opts check-attr test -- \"$path\" >actual 2>err &&\n \techo \"$path: test: $expect\" >expect &&\n \ttest_cmp expect actual &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n }\n \n attr_check_quote () {\n@@ -241,7 +241,7 @@ EOF\n \tgit check-attr foo -- \"a/b/f\" >>actual 2>>err &&\n \tgit check-attr foo -- \"a/b/c/f\" >>actual 2>>err &&\n \ttest_cmp expect actual &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n \n test_expect_success '\"**\" with no slashes test' '\n@@ -262,7 +262,7 @@ EOF\n \tgit check-attr foo -- \"a/b/f\" >>actual 2>>err &&\n \tgit check-attr foo -- \"a/b/c/f\" >>actual 2>>err &&\n \ttest_cmp expect actual &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n \n test_expect_success 'using --git-dir and --work-tree' '\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388642","messageId":"f6ef6d245c59b618964099a1d477b4c78889a715.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 06/15] t0020: don't use `test_must_fail has_cr`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:53Z","receivedAt":"2019-12-20T18:14:42Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. Since has_cr() just\nwraps a tr and grep pipeline, replace `test_must_fail has_cr` with\n`! has_cr`.\n\nSigned-off-by: Denton Liu \n---\n t/t0020-crlf.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 854da0ae16..b63ba62e5d 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -159,8 +159,8 @@ test_expect_success 'checkout with autocrlf=input' '\n \trm -f tmp one dir/two three &&\n \tgit config core.autocrlf input &&\n \tgit read-tree --reset -u HEAD &&\n-\ttest_must_fail has_cr one &&\n-\ttest_must_fail has_cr dir/two &&\n+\t! has_cr one &&\n+\t! has_cr dir/two &&\n \tgit update-index -- one dir/two &&\n \ttest \"$one\" = $(git hash-object --stdin .gitattributes &&\n \tgit read-tree --reset -u HEAD &&\n \n-\ttest_must_fail has_cr dir/two\n+\t! has_cr dir/two\n '\n \n test_expect_success '.gitattributes says two and three are text' '\n@@ -270,7 +270,7 @@ test_expect_success 'in-tree .gitattributes (1)' '\n \trm -rf tmp one dir .gitattributes patch.file three &&\n \tgit read-tree --reset -u HEAD &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n@@ -280,7 +280,7 @@ test_expect_success 'in-tree .gitattributes (2)' '\n \tgit read-tree --reset HEAD &&\n \tgit checkout-index -f -q -u -a &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n@@ -291,7 +291,7 @@ test_expect_success 'in-tree .gitattributes (3)' '\n \tgit checkout-index -u .gitattributes &&\n \tgit checkout-index -u one dir/two three &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n@@ -302,7 +302,7 @@ test_expect_success 'in-tree .gitattributes (4)' '\n \tgit checkout-index -u one dir/two three &&\n \tgit checkout-index -u .gitattributes &&\n \n-\ttest_must_fail has_cr one &&\n+\t! has_cr one &&\n \tverbose has_cr three\n '\n \n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388643","messageId":"b725d53dbec9cfce5954eb00c8728fe0c1f2e3f9.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 05/15] t0003: don't use `test_must_fail attr_check`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:52Z","receivedAt":"2019-12-20T18:14:42Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In an effort to remove test_must_fail for all invocations not related to\ngit or test-tool, replace invocations of `test_must_fail attr_check`\nwith a plain attr_check call with the $expect argument set to the\nactual value output by git.\n\nSigned-off-by: Denton Liu \n---\n t/t0003-attributes.sh | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex c30c736d3f..b660593c20 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -24,7 +24,7 @@ attr_check_quote () {\n \n test_expect_success 'open-quoted pathname' '\n \techo \"\\\"a test=a\" >.gitattributes &&\n-\ttest_must_fail attr_check a a\n+\tattr_check a unspecified\n '\n \n \n@@ -109,20 +109,20 @@ test_expect_success 'attribute test' '\n \n test_expect_success 'attribute matching is case sensitive when core.ignorecase=0' '\n \n-\ttest_must_fail attr_check F f \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/F f \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/c/F f \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/G a/g \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/B/g a/b/g \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/G a/b/g \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/H a/b/h \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/D/g \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check oNoFf unset \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check oFfOn set \"-c core.ignorecase=0\" &&\n+\tattr_check F unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/F unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/c/F unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/G unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/B/g a/g \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/G unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/H unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/D/g a/g \"-c core.ignorecase=0\" &&\n+\tattr_check oNoFf unspecified \"-c core.ignorecase=0\" &&\n+\tattr_check oFfOn unspecified \"-c core.ignorecase=0\" &&\n \tattr_check NO unspecified \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/b/D/NO \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n+\tattr_check a/b/D/NO unspecified \"-c core.ignorecase=0\" &&\n \tattr_check a/b/d/YES a/b/d/* \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check a/E/f \"A/e/F\" \"-c core.ignorecase=0\"\n+\tattr_check a/E/f f \"-c core.ignorecase=0\"\n \n '\n \n@@ -146,8 +146,8 @@ test_expect_success 'attribute matching is case insensitive when core.ignorecase\n '\n \n test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' '\n-\ttest_must_fail attr_check a/B/D/g \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n-\ttest_must_fail attr_check A/B/D/NO \"a/b/d/*\" \"-c core.ignorecase=0\" &&\n+\tattr_check a/B/D/g a/g \"-c core.ignorecase=0\" &&\n+\tattr_check A/B/D/NO unspecified \"-c core.ignorecase=0\" &&\n \tattr_check A/b/h a/b/h \"-c core.ignorecase=1\" &&\n \tattr_check a/B/D/g \"a/b/d/*\" \"-c core.ignorecase=1\" &&\n \tattr_check A/B/D/NO \"a/b/d/*\" \"-c core.ignorecase=1\"\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388644","messageId":"7e29b0154ef71a0a5743f9a2b281b40ba3337a07.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 08/15] t1306: convert `test_might_fail rm` to `rm -f`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:55Z","receivedAt":"2019-12-20T18:14:44Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() family of functions (including test_might_fail())\nshould only be used on git commands. Replace `test_might_fail rm` with\n`rm -f` so that we don't use `test_might_fail` on a non-git command.\n\nSigned-off-by: Denton Liu \n---\n t/t1306-xdg-files.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh\nindex 21e139a313..dd87b43be1 100755\n--- a/t/t1306-xdg-files.sh\n+++ b/t/t1306-xdg-files.sh\n@@ -153,7 +153,7 @@ test_expect_success 'Checking attributes in both XDG and local attributes files'\n \n \n test_expect_success 'Checking attributes in a non-XDG global attributes file' '\n-\ttest_might_fail rm .gitattributes &&\n+\trm -f .gitattributes &&\n \techo \"f attr_f=test\" >\"$HOME\"/my_gitattributes &&\n \tgit config core.attributesfile \"$HOME\"/my_gitattributes &&\n \techo \"f: attr_f: test\" >expected &&\n@@ -165,7 +165,7 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '\n test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\\''t' '\n \tmkdir -p \"$HOME\"/.config/git &&\n \t>\"$HOME\"/.config/git/config &&\n-\ttest_might_fail rm \"$HOME\"/.gitconfig &&\n+\trm -f \"$HOME\"/.gitconfig &&\n \tgit config --global user.name \"write_config\" &&\n \techo \"[user]\" >expected &&\n \techo \"\tname = write_config\" >>expected &&\n@@ -183,8 +183,8 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '\n \n \n test_expect_success 'write: ~/.config/git/ exists and config file doesn'\\''t' '\n-\ttest_might_fail rm \"$HOME\"/.gitconfig &&\n-\ttest_might_fail rm \"$HOME\"/.config/git/config &&\n+\trm -f \"$HOME\"/.gitconfig &&\n+\trm -f \"$HOME\"/.config/git/config &&\n \tgit config --global user.name \"write_gitconfig\" &&\n \techo \"[user]\" >expected &&\n \techo \"\tname = write_gitconfig\" >>expected &&\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388645","messageId":"fc32b8d5847f57e7ea64e3be339cf7451ecd919e.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 07/15] t0020: use ! check_packed_refs_marked","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:54Z","receivedAt":"2019-12-20T18:14:45Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail function should only be used for git commands since\nwe should assume that external commands work sanely. Since\ncheck_packed_refs_marked() just wraps a grep invocation, replace\n`test_must_fail check_packed_refs_marked` with\n`! check_packed_refs_marked`.\n\nSigned-off-by: Denton Liu \n---\n t/t1409-avoid-packing-refs.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex e5cb8a252d..c46848eb8e 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -46,7 +46,7 @@ test_expect_success 'check that marking the packed-refs file works' '\n \tgit for-each-ref >actual &&\n \ttest_cmp expected actual &&\n \tgit pack-refs --all &&\n-\ttest_must_fail check_packed_refs_marked &&\n+\t! check_packed_refs_marked &&\n \tgit for-each-ref >actual2 &&\n \ttest_cmp expected actual2\n '\n@@ -80,7 +80,7 @@ test_expect_success 'touch packed-refs on delete of packed' '\n \tgit pack-refs --all &&\n \tmark_packed_refs &&\n \tgit update-ref -d refs/heads/packed-delete &&\n-\ttest_must_fail check_packed_refs_marked\n+\t! check_packed_refs_marked\n '\n \n test_expect_success 'leave packed-refs untouched on update of loose' '\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388646","messageId":"1eef3f4bc5f93b8a0a6ce8b26a208cba4f06a56c.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 11/15] t1409: use test_path_is_missing()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:58Z","receivedAt":"2019-12-20T18:14:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() function should only be used for git commands since\nwe should assume that external commands work sanely. Replace\n`test_must_fail test -f` with `test_path_is_missing` since we expect\nthese paths to not exist.\n\nSigned-off-by: Denton Liu \n---\n t/t1409-avoid-packing-refs.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex f74d890e82..be12fb6350 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -27,15 +27,15 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'do not create packed-refs file gratuitously' '\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref refs/heads/foo $A &&\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref refs/heads/foo $B &&\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref refs/heads/foo $C $B &&\n-\ttest_must_fail test -f .git/packed-refs &&\n+\ttest_path_is_missing .git/packed-refs &&\n \tgit update-ref -d refs/heads/foo &&\n-\ttest_must_fail test -f .git/packed-refs\n+\ttest_path_is_missing .git/packed-refs\n '\n \n test_expect_success 'check that marking the packed-refs file works' '\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388647","messageId":"e2fe278bfabc802681eb1c6325d54c4af4bfec8e.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 10/15] t1409: let sed open its own input file","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:57Z","receivedAt":"2019-12-20T18:14:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In one case, we were using a redirection operator to feed input into\nsed. However, since sed is capable of opening its own input file, make\nsed do that instead of redirecting input into it.\n\nSigned-off-by: Denton Liu \n---\n t/t1409-avoid-packing-refs.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex c46848eb8e..f74d890e82 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -8,7 +8,7 @@ test_description='avoid rewriting packed-refs unnecessarily'\n # shouldn't upset readers, and it should be omitted if the file is\n # ever rewritten.\n mark_packed_refs () {\n-\tsed -e \"s/^\\(#.*\\)/\\1 t1409 /\" <.git/packed-refs >.git/packed-refs.new &&\n+\tsed -e \"s/^\\(#.*\\)/\\1 t1409 /\" .git/packed-refs >.git/packed-refs.new &&\n \tmv .git/packed-refs.new .git/packed-refs\n }\n \n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388648","messageId":"fddd2242259ccd8ec76954942901177b4f063a90.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 12/15] t1501: remove use of `test_might_fail cp`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:59Z","receivedAt":"2019-12-20T18:14:49Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() family of functions (including test_might_fail())\nshould only be used on git commands. Replace test_might_fail() with\na compound command wrapping the old cp invocation that always returns 0.\n\nThe `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix\ntest with split index, 2015-03-24). It is necessary because there might\nexist some index files in `repo.git/sharedindex.*` and, if they exist,\nwe want to copy them over. However, if they don't exist, we don't want\nto error out because we expect that possibility. As a result, we want to\nkeep the \"might fail\" semantics so we always return 0, even if the\nunderlying cp errors out.\n\nSigned-off-by: Denton Liu \n---\n t/t1501-work-tree.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh\nindex 3498d3d55e..b75558040f 100755\n--- a/t/t1501-work-tree.sh\n+++ b/t/t1501-work-tree.sh\n@@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '\n \tmkdir work &&\n \tmkdir -p repo.git/repos/foo &&\n \tcp repo.git/HEAD repo.git/index repo.git/repos/foo &&\n-\ttest_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&\n+\t{ cp repo.git/sharedindex.* repo.git/repos/foo || :; } &&\n \tsane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE\n '\n \n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388649","messageId":"63ca18207d58a846f47887926582af9b3c55284e.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 13/15] t1507: stop losing return codes of git commands","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:16:00Z","receivedAt":"2019-12-20T18:14:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The return code of git commands are lost when a command is in a\nnon-assignment command substitution in favour of the surrounding\ncommand's. Rewrite instances of this so that git commands run\non their own.\n\nIn commit_subject(), use a `tformat` instead of `format` since,\npreviously, we were testing the output of a command substitution which\ndidn't care if there was a trailing newline since it was automatically\nstripped. Since we use test_cmp() now, the trailing newline matters so\nuse `tformat` to always output it.\n\nSigned-off-by: Denton Liu \n---\n t/t1507-rev-parse-upstream.sh | 45 +++++++++++++++++++++++++----------\n 1 file changed, 32 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex 8b4cf8a6e3..d81f289ace 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -35,7 +35,7 @@ full_name () {\n \n commit_subject () {\n \t(cd clone &&\n-\t git show -s --pretty=format:%s \"$@\")\n+\t git show -s --pretty=tformat:%s \"$@\")\n }\n \n error_message () {\n@@ -44,18 +44,27 @@ error_message () {\n }\n \n test_expect_success '@{upstream} resolves to correct full name' '\n-\ttest refs/remotes/origin/master = \"$(full_name @{upstream})\" &&\n-\ttest refs/remotes/origin/master = \"$(full_name @{UPSTREAM})\" &&\n-\ttest refs/remotes/origin/master = \"$(full_name @{UpSTReam})\"\n+\techo refs/remotes/origin/master >expect &&\n+\tfull_name @{upstream} >actual &&\n+\ttest_cmp expect actual &&\n+\tfull_name @{UPSTREAM} >actual &&\n+\ttest_cmp expect actual &&\n+\tfull_name @{UpSTReam} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '@{u} resolves to correct full name' '\n-\ttest refs/remotes/origin/master = \"$(full_name @{u})\" &&\n-\ttest refs/remotes/origin/master = \"$(full_name @{U})\"\n+\techo refs/remotes/origin/master >expect &&\n+\tfull_name @{u} >actual &&\n+\ttest_cmp expect actual &&\n+\tfull_name @{U} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'my-side@{upstream} resolves to correct full name' '\n-\ttest refs/remotes/origin/side = \"$(full_name my-side@{u})\"\n+\techo refs/remotes/origin/side >expect &&\n+\tfull_name my-side@{u} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'upstream of branch with @ in middle' '\n@@ -86,8 +95,11 @@ test_expect_success 'my-side@{u} resolves to correct commit' '\n \tgit checkout side &&\n \ttest_commit 5 &&\n \t(cd clone && git fetch) &&\n-\ttest 2 = \"$(commit_subject my-side)\" &&\n-\ttest 5 = \"$(commit_subject my-side@{u})\"\n+\techo 2 >expect &&\n+\tcommit_subject my-side >actual &&\n+\ttest_cmp expect actual &&\n+\techo 5 >expect &&\n+\tcommit_subject my-side@{u} >actual\n '\n \n test_expect_success 'not-tracking@{u} fails' '\n@@ -99,8 +111,11 @@ test_expect_success 'not-tracking@{u} fails' '\n test_expect_success '@{u}@{1} resolves correctly' '\n \ttest_commit 6 &&\n \t(cd clone && git fetch) &&\n-\ttest 5 = $(commit_subject my-side@{u}@{1}) &&\n-\ttest 5 = $(commit_subject my-side@{U}@{1})\n+\techo 5 >expect &&\n+\tcommit_subject my-side@{u}@{1} >actual &&\n+\ttest_cmp expect actual &&\n+\tcommit_subject my-side@{U}@{1} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '@{u} without specifying branch fails on a detached HEAD' '\n@@ -149,7 +164,9 @@ test_expect_success 'checkout other@{u}' '\n '\n \n test_expect_success 'branch@{u} works when tracking a local branch' '\n-\ttest refs/heads/master = \"$(full_name local-master@{u})\"\n+\techo refs/heads/master >expect &&\n+\tfull_name local-master@{u} >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'branch@{u} error message when no upstream' '\n@@ -203,7 +220,9 @@ test_expect_success 'pull works when tracking a local branch' '\n \n # makes sense if the previous one succeeded\n test_expect_success '@{u} works when tracking a local branch' '\n-\ttest refs/heads/master = \"$(full_name @{u})\"\n+\techo refs/heads/master >expect &&\n+\tfull_name @{u} >actual &&\n+\ttest_cmp expect actual\n '\n \n commit=$(git rev-parse HEAD)\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388650","messageId":"cf43579d654387ca19d3d8b10c4e775a8f663621.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 09/15] t1307: reorder `nongit test_must_fail`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:15:56Z","receivedAt":"2019-12-20T18:14:52Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the future, we plan on only allowing `test_must_fail` to work on a\nrestricted subset of commands, including `git`. Reorder the commands so\nthat `nongit` comes before `test_must_fail`. This way, `test_must_fail`\noperates on a git command.\n\nSigned-off-by: Denton Liu \n---\n t/t1307-config-blob.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh\nindex 37dc689d8c..002e6d3388 100755\n--- a/t/t1307-config-blob.sh\n+++ b/t/t1307-config-blob.sh\n@@ -74,7 +74,7 @@ test_expect_success 'can parse blob ending with CR' '\n '\n \n test_expect_success 'config --blob outside of a repository is an error' '\n-\ttest_must_fail nongit git config --blob=foo --list\n+\tnongit test_must_fail git config --blob=foo --list\n '\n \n test_done\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388651","messageId":"44a410d57a3e9c25c609a23619d48b471e5057b5.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 14/15] t1507: run commands within test_expect_success","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:16:01Z","receivedAt":"2019-12-20T18:14:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The expected test style is to have all commands tested within a\ntest_expect_success block. Move the generation of the 'expect' text into\ntheir corresponding blocks. While we're at it, insert a second\n`commit=$(git rev-parse HEAD)` into the next test case so that it's\nclear where $commit is coming from.\n\nThe biggest advantage of doing this is that we now check the return code\nof `git rev-parse HEAD` so we can catch it in case it fails.\n\nThis patch is best viewed with `--color-moved --ignore-all-space`.\n\nSigned-off-by: Denton Liu \n---\n t/t1507-rev-parse-upstream.sh | 40 +++++++++++++++++------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex d81f289ace..f68b77e7ba 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -225,32 +225,32 @@ test_expect_success '@{u} works when tracking a local branch' '\n \ttest_cmp expect actual\n '\n \n-commit=$(git rev-parse HEAD)\n-cat >expect <)\n-Reflog message: branch: Created from HEAD\n-Author: A U Thor \n-Date: Thu Apr 7 15:15:13 2005 -0700\n-\n- 3\n-EOF\n test_expect_success 'log -g other@{u}' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect <<-EOF &&\n+\tcommit $commit\n+\tReflog: master@{0} (C O Mitter )\n+\tReflog message: branch: Created from HEAD\n+\tAuthor: A U Thor \n+\tDate: Thu Apr 7 15:15:13 2005 -0700\n+\n+\t 3\n+\tEOF\n \tgit log -1 -g other@{u} >actual &&\n \ttest_cmp expect actual\n '\n \n-cat >expect <)\n-Reflog message: branch: Created from HEAD\n-Author: A U Thor \n-Date: Thu Apr 7 15:15:13 2005 -0700\n-\n- 3\n-EOF\n-\n test_expect_success 'log -g other@{u}@{now}' '\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect <<-EOF &&\n+\tcommit $commit\n+\tReflog: master@{Thu Apr 7 15:17:13 2005 -0700} (C O Mitter )\n+\tReflog message: branch: Created from HEAD\n+\tAuthor: A U Thor \n+\tDate: Thu Apr 7 15:15:13 2005 -0700\n+\n+\t 3\n+\tEOF\n \tgit log -1 -g other@{u}@{now} >actual &&\n \ttest_cmp expect actual\n '\n-- \n2.24.1.703.g2f499f1283\n\n"},{"id":"388652","messageId":"4fe445279b715b3a49e7635adc1af152de183bff.1576865664.git.liu.denton@gmail.com","threadId":"52474","inReplyTo":"cover.1576865663.git.liu.denton@gmail.com","subject":"[PATCH v3 15/15] t1507: inline full_name()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-20T18:16:02Z","receivedAt":"2019-12-20T18:14:55Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, we were running `test_must_fail full_name`. However,\n`test_must_fail` should only be used on git commands. Inline full_name()\nso that we can use test_must_fail on the git command directly.\n\nWhen full_name() was introduced in 28fb84382b (Introduce\n@{upstream} notation, 2009-09-10), the `git -C` option wasn't\navailable yet (since it was introduced in 44e1e4d67d (git: run in a\ndirectory given with -C option, 2013-09-09)). As a result, the helper\nfunction removed the need to manually cd each time. However, since\n`git -C` is available now, we can just use that instead and inline\nfull_name().\n\nAn alternate approach was taken where we taught full_name() to accept an\noptional `!` arg to trigger test_must_fail behavior. However, this added\nmore unnecessary complexity than inlining so we inline instead.\n\nSigned-off-by: Denton Liu \n---\n t/t1507-rev-parse-upstream.sh | 35 +++++++++++++++--------------------\n 1 file changed, 15 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex f68b77e7ba..dfc0d96d8a 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -28,11 +28,6 @@ test_expect_success 'setup' '\n \t)\n '\n \n-full_name () {\n-\t(cd clone &&\n-\t git rev-parse --symbolic-full-name \"$@\")\n-}\n-\n commit_subject () {\n \t(cd clone &&\n \t git show -s --pretty=tformat:%s \"$@\")\n@@ -45,50 +40,50 @@ error_message () {\n \n test_expect_success '@{upstream} resolves to correct full name' '\n \techo refs/remotes/origin/master >expect &&\n-\tfull_name @{upstream} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{upstream} >actual &&\n \ttest_cmp expect actual &&\n-\tfull_name @{UPSTREAM} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{UPSTREAM} >actual &&\n \ttest_cmp expect actual &&\n-\tfull_name @{UpSTReam} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{UpSTReam} >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '@{u} resolves to correct full name' '\n \techo refs/remotes/origin/master >expect &&\n-\tfull_name @{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{u} >actual &&\n \ttest_cmp expect actual &&\n-\tfull_name @{U} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{U} >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'my-side@{upstream} resolves to correct full name' '\n \techo refs/remotes/origin/side >expect &&\n-\tfull_name my-side@{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name my-side@{u} >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'upstream of branch with @ in middle' '\n-\tfull_name fun@ny@{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name fun@ny@{u} >actual &&\n \techo refs/remotes/origin/side >expect &&\n \ttest_cmp expect actual &&\n-\tfull_name fun@ny@{U} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name fun@ny@{U} >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'upstream of branch with @ at start' '\n-\tfull_name @funny@{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @funny@{u} >actual &&\n \techo refs/remotes/origin/side >expect &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'upstream of branch with @ at end' '\n-\tfull_name funny@@{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name funny@@{u} >actual &&\n \techo refs/remotes/origin/side >expect &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' '\n-\ttest_must_fail full_name refs/heads/my-side@{upstream}\n+\ttest_must_fail git -C clone rev-parse --symbolic-full-name refs/heads/my-side@{upstream}\n '\n \n test_expect_success 'my-side@{u} resolves to correct commit' '\n@@ -103,9 +98,9 @@ test_expect_success 'my-side@{u} resolves to correct commit' '\n '\n \n test_expect_success 'not-tracking@{u} fails' '\n-\ttest_must_fail full_name non-tracking@{u} &&\n+\ttest_must_fail git -C clone rev-parse --symbolic-full-name non-tracking@{u} &&\n \t(cd clone && git checkout --no-track -b non-tracking) &&\n-\ttest_must_fail full_name non-tracking@{u}\n+\ttest_must_fail git -C clone rev-parse --symbolic-full-name non-tracking@{u}\n '\n \n test_expect_success '@{u}@{1} resolves correctly' '\n@@ -165,7 +160,7 @@ test_expect_success 'checkout other@{u}' '\n \n test_expect_success 'branch@{u} works when tracking a local branch' '\n \techo refs/heads/master >expect &&\n-\tfull_name local-master@{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name local-master@{u} >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -221,7 +216,7 @@ test_expect_success 'pull works when tracking a local branch' '\n # makes sense if the previous one succeeded\n test_expect_success '@{u} works when tracking a local branch' '\n \techo refs/heads/master >expect &&\n-\tfull_name @{u} >actual &&\n+\tgit -C clone rev-parse --symbolic-full-name @{u} >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.24.1.703.g2f499f1283\n\n"}]}