{"thread":{"id":"61029","subject":"[GSoC][PATCH 0/1] microproject: Use test_path_is_* functions in test scripts","startedAt":"2024-02-29T15:05:07Z","lastAt":"2024-03-05T11:42:12Z","messageCount":26,"participants":["shejialuo","Eric Sunshine","Junio C Hamano","Patrick Steinhardt","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"489663","messageId":"20240229150442.490649-1-shejialuo@gmail.com","threadId":"61029","inReplyTo":null,"subject":"[GSoC][PATCH 0/1] microproject: Use test_path_is_* functions in test scripts","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-02-29T15:04:41Z","receivedAt":"2024-02-29T15:05:07Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Hello everyone,\n\nMy name is Jialuo She, mastering in the software engineering. This is my\nlast semester. And I will graduate this summer and works as a full time\nemployee. So I wanna make good use of my time by contributing to open\nsource software, and take this opportunity to continue contributing to\nGit after I start working in the future.\n\nMy reason for choosing to participate in Git is actually quite simple.\nIt's because I once wrote a toy version of Git myself.Throught this\nproject, https://github.com/shejialuo/ugit-cpp. I came to understand\nthe magic of Git. I also want to do my part and contribute to something\nmeaningful.\n\nFor myself, the most attractive GSoC idea for me is \"Implement\nconsistency checks for refs\". I will dive into this idea soon.\n\nAt last, Wish everyone good health and happiness every day.\n\nshejialuo (1):\n  [GSoC][PATCH] t3070: refactor test -e command\n\n t/t3070-wildmatch.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n-- \n2.44.0\n\n"},{"id":"489664","messageId":"20240229150442.490649-2-shejialuo@gmail.com","threadId":"61029","inReplyTo":"20240229150442.490649-1-shejialuo@gmail.com","subject":"[PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-02-29T15:04:42Z","receivedAt":"2024-02-29T15:05:20Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"The \"test_path_exists\" function was proposed at 7e9055b. It provides\nparameter number check and more robust error messages.\n\nThis patch converts all \"test -e\" into \"test_path_exists\" to improve\ntest debug when failure.\n\nSigned-off-by: shejialuo <shejialuo@gmail.com>\n---\n t/t3070-wildmatch.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh\nindex 4dd42df38c..d18ddc1a52 100755\n--- a/t/t3070-wildmatch.sh\n+++ b/t/t3070-wildmatch.sh\n@@ -107,7 +107,7 @@ match_with_ls_files() {\n \n \tif test \"$match_expect\" = 'E'\n \tthen\n-\t\tif test -e .git/created_test_file\n+\t\tif test_path_exists .git/created_test_file\n \t\tthen\n \t\t\ttest_expect_success EXPENSIVE_ON_WINDOWS \"$match_function (via ls-files): match dies on '$pattern' '$text'\" \"\n \t\t\t\tprintf '%s' '$text' >expect &&\n@@ -118,7 +118,7 @@ match_with_ls_files() {\n \t\tfi\n \telif test \"$match_expect\" = 1\n \tthen\n-\t\tif test -e .git/created_test_file\n+\t\tif test_path_exists .git/created_test_file\n \t\tthen\n \t\t\ttest_expect_success EXPENSIVE_ON_WINDOWS \"$match_function (via ls-files): match '$pattern' '$text'\" \"\n \t\t\t\tprintf '%s' '$text' >expect &&\n@@ -130,7 +130,7 @@ match_with_ls_files() {\n \t\tfi\n \telif test \"$match_expect\" = 0\n \tthen\n-\t\tif test -e .git/created_test_file\n+\t\tif test_path_exists .git/created_test_file\n \t\tthen\n \t\t\ttest_expect_success EXPENSIVE_ON_WINDOWS \"$match_function (via ls-files): no match '$pattern' '$text'\" \"\n \t\t\t\t>expect &&\n@@ -175,7 +175,7 @@ match() {\n \tfi\n \n \ttest_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '\n-\t\tif test -e .git/created_test_file\n+\t\tif test_path_exists .git/created_test_file\n \t\tthen\n \t\t\tgit reset &&\n \t\t\tgit clean -df\n@@ -198,7 +198,7 @@ match() {\n \t\t\tfi &&\n \t\t\tgit add -A &&\n \t\t\tprintf \"%s\" \"$file\" >.git/created_test_file\n-\t\telif test -e .git/created_test_file\n+\t\telif test_path_exists .git/created_test_file\n \t\tthen\n \t\t\trm .git/created_test_file\n \t\tfi\n-- \n2.44.0\n\n"},{"id":"489670","messageId":"CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com","threadId":"61029","inReplyTo":"20240229150442.490649-2-shejialuo@gmail.com","subject":"Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-29T17:58:03Z","receivedAt":"2024-02-29T17:58:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 29, 2024 at 10:05 AM shejialuo <shejialuo@gmail.com> wrote:\n> t3070: refactor test -e command\n>\n> The \"test_path_exists\" function was proposed at 7e9055b. It provides\n> parameter number check and more robust error messages.\n>\n> This patch converts all \"test -e\" into \"test_path_exists\" to improve\n> test debug when failure.\n\nThanks for providing this GSoC submission. The aim of this patch makes\nsense, but it turns out that t3070 is not a good choice for this\nexercise. Before getting into that, though, a few minor comments about\nthe commit message.\n\nThis patch isn't actually refactoring the code, so using \"refactor\" in\nthe title is misleading.\n\nRather than mentioning only the object-ID, we normally reference other\ncommits like this (using `git log --pretty=reference -1 <object-id>`):\n\n    7e9055bb00 (t7406: prefer test_* helper functions to test -[feds],\n2018-08-08)\n\nIn this case, it's not clear why you chose to reference that\nparticular commit over any of the others which make similar changes.\nIt probably would be simpler to drop mention of that commit and just\ncopy its reasoning into your commit message.\n\nTaking all the above into account, a possible rewrite of the commit\nmessage might be:\n\n    t3070: prefer test_path_exists helper function\n\n    test -e does not provide a nice error message when we hit test\n    failures, so use test_path_exists instead.\n\n> Signed-off-by: shejialuo <shejialuo@gmail.com>\n> ---\n> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh\n> @@ -107,7 +107,7 @@ match_with_ls_files() {\n>         if test \"$match_expect\" = 'E'\n>         then\n> -               if test -e .git/created_test_file\n> +               if test_path_exists .git/created_test_file\n>                 then\n>                         test_expect_success EXPENSIVE_ON_WINDOWS \"$match_function (via ls-files): match dies on '$pattern' '$text'\" \"\n\nThe point of functions such as test_path_exists() is to _assert_ that\nsome condition is true, thus allowing the test to succeed; if the\ncondition is not true, then the function prints an error message and\nthe test aborts and fails. Here is how test_path_exists() is defined:\n\n    test_path_exists () {\n        test \"$#\" -ne 1 && BUG \"1 param\"\n        if ! test -e \"$1\"\n        then\n            echo \"Path $1 doesn't exist\"\n            false\n        fi\n    }\n\nIt is meant to replace noisy code such as:\n\n    if ! test -e bloop\n    then\n        echo >&2 \"error message\" &&\n        exit 1\n    fi &&\n    other-code\n\nwith much simpler:\n\n    test_path_exists bloop &&\n    other-code\n\nIt is also meant to be used within `test_expect_success` (or\n`test_expect_failure`) blocks. So, the changes made by this patch are\nundesirable for a couple reasons...\n\nFirst, this code is outside a `test_expect_success` (or\n`test_expect_failure`) block.\n\nSecond, as noted above, test_path_exists() is an _assertion_ which\nrequires the file to exist, and aborts the test if the file does not\nexist. But the `test -e` being changed here is part of the proper\ncontrol-flow of this logic; it is not asserting anything, but merely\nbranching to one or another part of the code depending upon the result\nof the `test -e` test. Thus, replacing this control-flow check with\nthe assertion function test_path_exists() changes the logic in an\nundesirable way.\n\nThe above comments are applicable to most of the changes made by this\npatch. The only exceptions are the last two changes...\n\n> @@ -175,7 +175,7 @@ match() {\n>         test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '\n> -               if test -e .git/created_test_file\n> +               if test_path_exists .git/created_test_file\n>                 then\n>                         git reset &&\n\n... which _do_ use test_path_exists() within a `test_expect_success`\nblock. However, the changes are still undesirable because, as above,\nthis `test -e` is merely part of the normal control-flow; it's not\nacting as an assertion, thus test_path_exists() -- which is an\nassertion -- is not correct.\n\nUnfortunately, none of the uses of`test -e` in t3070 are being used as\nassertions worthy of replacement with test_path_exists(), thus this\nisn't a good script in which to make such changes. If you reroll, you\nmay be able to find a good candidate script by searching for code\nwhich looks something like this:\n\n    foo &&\n    test -e path &&\n    bar &&\n\nand replacing it with:\n\n    foo &&\n    test_path_exists path &&\n    bar &&\n"},{"id":"489677","messageId":"xmqqzfvjf5tq.fsf@gitster.g","threadId":"61029","inReplyTo":"CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com","subject":"Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-29T19:06:41Z","receivedAt":"2024-02-29T19:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> @@ -175,7 +175,7 @@ match() {\n>>         test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '\n>> -               if test -e .git/created_test_file\n>> +               if test_path_exists .git/created_test_file\n>>                 then\n>>                         git reset &&\n>\n> ... which _do_ use test_path_exists() within a `test_expect_success`\n> block. However, the changes are still undesirable because, as above,\n> this `test -e` is merely part of the normal control-flow; it's not\n> acting as an assertion, thus test_path_exists() -- which is an\n> assertion -- is not correct.\n>\n> Unfortunately, none of the uses of`test -e` in t3070 are being used as\n> assertions worthy of replacement with test_path_exists(), thus this\n> isn't a good script in which to make such changes.\n\nIt seems that there is a recurring confusion among mentorship\nprogram applicants that use test_path_* helpers as their practice\nmaterial.  Perhaps the source of the information that suggests it as\na microproject is poorly phrased and needs to be rewritten to avoid\nmisleading them.\n\nI found one at https://git.github.io/Outreachy-23-Microprojects/,\nwhich can be one source of such confusion:\n\n    Find one test script that verifies the presence/absence of\n    files/directories with ‘test -(e|f|d|…)’ and replace them\n    with the appropriate test_path_is_file, test_path_is_dir,\n    etc. helper functions.\n\nbut there may be others.\n\nThis task specification does not differenciate \"test -[efdx]\" used\nas a conditional of a control flow statement (which should never be\nreplaced by test_path_* helpers) and those used to directly fail the\n&&-chain in test_expect_success with their exit status (which is the\ntarget that test_path_* helpers are meant to improve).\n"},{"id":"489722","messageId":"20240301025034.26592-1-shejialuo@gmail.com","threadId":"61029","inReplyTo":"CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com","subject":"Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-01T02:50:34Z","receivedAt":"2024-03-01T02:50:43Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Thanks for your comment, I will find a candidate script later and submit\na new version patch.\n"},{"id":"489723","messageId":"20240301034606.69673-1-shejialuo@gmail.com","threadId":"61029","inReplyTo":"20240229150442.490649-1-shejialuo@gmail.com","subject":"[PATCH V2 0/1] [GSoC][PATCH] t9117: prefer test_path_* helper functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-01T03:46:05Z","receivedAt":"2024-03-01T03:46:17Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"As discussed, the original patch is unsutiable, t9117 is a good\ncandidate script.\n\nshejialuo (1):\n  t9117: prefer test_path_* helper functions\n\n t/t9117-git-svn-init-clone.sh | 40 +++++++++++++++++------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n-- \n2.44.0\n\n"},{"id":"489724","messageId":"20240301034606.69673-2-shejialuo@gmail.com","threadId":"61029","inReplyTo":"20240301034606.69673-1-shejialuo@gmail.com","subject":"[PATCH 1/1] t9117: prefer test_path_* helper functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-01T03:46:06Z","receivedAt":"2024-03-01T03:46:37Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"test -(e|f|d) does not provide a nice error message when we hit test\nfailures, so use test_path_exists, test_path_is_dir and\ntest_path_is_file instead.\n\nSigned-off-by: shejialuo <shejialuo@gmail.com>\n---\n t/t9117-git-svn-init-clone.sh | 40 +++++++++++++++++------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh\nindex 62de819a44..2f964f66aa 100755\n--- a/t/t9117-git-svn-init-clone.sh\n+++ b/t/t9117-git-svn-init-clone.sh\n@@ -15,39 +15,39 @@ test_expect_success 'setup svnrepo' '\n \t'\n \n test_expect_success 'basic clone' '\n-\ttest ! -d trunk &&\n+\t! test_path_is_dir trunk &&\n \tgit svn clone \"$svnrepo\"/project/trunk &&\n-\ttest -d trunk/.git/svn &&\n-\ttest -e trunk/foo &&\n+\ttest_path_is_dir trunk/.git/svn &&\n+\ttest_path_exists trunk/foo &&\n \trm -rf trunk\n \t'\n \n test_expect_success 'clone to target directory' '\n-\ttest ! -d target &&\n+\t! test_path_is_dir target &&\n \tgit svn clone \"$svnrepo\"/project/trunk target &&\n-\ttest -d target/.git/svn &&\n-\ttest -e target/foo &&\n+\ttest_path_is_dir target/.git/svn &&\n+\ttest_path_exists target/foo &&\n \trm -rf target\n \t'\n \n test_expect_success 'clone with --stdlayout' '\n-\ttest ! -d project &&\n+\t! test_path_is_dir project &&\n \tgit svn clone -s \"$svnrepo\"/project &&\n-\ttest -d project/.git/svn &&\n-\ttest -e project/foo &&\n+\ttest_path_is_dir project/.git/svn &&\n+\ttest_path_exists project/foo &&\n \trm -rf project\n \t'\n \n test_expect_success 'clone to target directory with --stdlayout' '\n-\ttest ! -d target &&\n+\t! test_path_is_dir target &&\n \tgit svn clone -s \"$svnrepo\"/project target &&\n-\ttest -d target/.git/svn &&\n-\ttest -e target/foo &&\n+\ttest_path_is_dir target/.git/svn &&\n+\ttest_path_exists target/foo &&\n \trm -rf target\n \t'\n \n test_expect_success 'init without -s/-T/-b/-t does not warn' '\n-\ttest ! -d trunk &&\n+\t! test_path_is_dir trunk &&\n \tgit svn init \"$svnrepo\"/project/trunk trunk 2>warning &&\n \t! grep -q prefix warning &&\n \trm -rf trunk &&\n@@ -55,7 +55,7 @@ test_expect_success 'init without -s/-T/-b/-t does not warn' '\n \t'\n \n test_expect_success 'clone without -s/-T/-b/-t does not warn' '\n-\ttest ! -d trunk &&\n+\t! test_path_is_dir trunk &&\n \tgit svn clone \"$svnrepo\"/project/trunk 2>warning &&\n \t! grep -q prefix warning &&\n \trm -rf trunk &&\n@@ -69,7 +69,7 @@ project/trunk:refs/remotes/${prefix}trunk\n project/branches/*:refs/remotes/${prefix}*\n project/tags/*:refs/remotes/${prefix}tags/*\n EOF\n-\ttest ! -f actual &&\n+\t! test_path_is_file actual &&\n \tgit --git-dir=project/.git config svn-remote.svn.fetch >>actual &&\n \tgit --git-dir=project/.git config svn-remote.svn.branches >>actual &&\n \tgit --git-dir=project/.git config svn-remote.svn.tags >>actual &&\n@@ -78,7 +78,7 @@ EOF\n }\n \n test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '\n-\ttest ! -d project &&\n+\t! test_path_is_dir project &&\n \tgit svn init -s \"$svnrepo\"/project project 2>warning &&\n \t! grep -q prefix warning &&\n \ttest_svn_configured_prefix \"origin/\" &&\n@@ -87,7 +87,7 @@ test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '\n \t'\n \n test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '\n-\ttest ! -d project &&\n+\t! test_path_is_dir project &&\n \tgit svn clone -s \"$svnrepo\"/project 2>warning &&\n \t! grep -q prefix warning &&\n \ttest_svn_configured_prefix \"origin/\" &&\n@@ -96,7 +96,7 @@ test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '\n \t'\n \n test_expect_success 'init with -s/-T/-b/-t and --prefix \"\" still works' '\n-\ttest ! -d project &&\n+\t! test_path_is_dir project &&\n \tgit svn init -s \"$svnrepo\"/project project --prefix \"\" 2>warning &&\n \t! grep -q prefix warning &&\n \ttest_svn_configured_prefix \"\" &&\n@@ -105,7 +105,7 @@ test_expect_success 'init with -s/-T/-b/-t and --prefix \"\" still works' '\n \t'\n \n test_expect_success 'clone with -s/-T/-b/-t and --prefix \"\" still works' '\n-\ttest ! -d project &&\n+\t! test_path_is_dir project &&\n \tgit svn clone -s \"$svnrepo\"/project --prefix \"\" 2>warning &&\n \t! grep -q prefix warning &&\n \ttest_svn_configured_prefix \"\" &&\n@@ -114,7 +114,7 @@ test_expect_success 'clone with -s/-T/-b/-t and --prefix \"\" still works' '\n \t'\n \n test_expect_success 'init with -T as a full url works' '\n-\ttest ! -d project &&\n+\t! test_path_is_dir project &&\n \tgit svn init -T \"$svnrepo\"/project/trunk project &&\n \trm -rf project\n \t'\n-- \n2.44.0\n\n"},{"id":"489725","messageId":"CAPig+cRfO8t1tdCL6MB4b9XopF3HkZ==hU83AFZ38b-2zsXDjQ@mail.gmail.com","threadId":"61029","inReplyTo":"20240301034606.69673-2-shejialuo@gmail.com","subject":"Re: [PATCH 1/1] t9117: prefer test_path_* helper functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-01T04:44:53Z","receivedAt":"2024-03-01T04:45:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 29, 2024 at 10:46 PM shejialuo <shejialuo@gmail.com> wrote:\n> test -(e|f|d) does not provide a nice error message when we hit test\n> failures, so use test_path_exists, test_path_is_dir and\n> test_path_is_file instead.\n\nThanks for rerolling. t9117 is indeed a better choice[1] than t3070\nfor the exercise of replacing `test -blah` with `test_path_foo`.\n\n[1]: https://lore.kernel.org/git/CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com/\n\n> Signed-off-by: shejialuo <shejialuo@gmail.com>\n> ---\n> diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh\n> @@ -15,39 +15,39 @@ test_expect_success 'setup svnrepo' '\n>  test_expect_success 'basic clone' '\n> -       test ! -d trunk &&\n> +       ! test_path_is_dir trunk &&\n\nGenerally speaking, you don't want to use `!` to negate the result of\na `path_is_foo` assertion function. To understand why, take a look at\nthe definition of `test_path_is_dir`:\n\n    test_path_is_dir () {\n        if ! test -d \"$1\"\n        then\n            echo \"Directory $1 doesn't exist\"\n            false\n        fi\n    }\n\nThe test in question (t9117: \"basic clone\") is using `test ! -d` to\nassert that the directory `trunk` does not yet exist when the test\nbegins; indeed, under normal circumstances, this directory should not\nyet be present. However, the call to test_path_is_dir() asserts that\nthe directory _does_ exist, which is the opposite of `test ! -d`, and\ncomplains (\"Directory trunk doesn't exist\") when it doesn't exist. So,\nin the normal and typical case for all the tests in this script,\n`test_path_is_dir` is going to be complaining even though the\nnon-existence of that directory is an expected condition.\n\nAlthough you make the test pass by using `!` to invert the result of\n`test_path_is_dir`, the complaint will nevertheless get lodged, and\nmay very well be confusing for anyone scrutinizing the output of the\ntests when running the script with `-v` or `-x`.\n\nSo, `test_path_is_dir` is not a good fit for this case which wants to\nassert that the path `trunk` does not yet exist. A better choice for\nthis particular case would be `test_path_is_missing`.\n\n>         git svn clone \"$svnrepo\"/project/trunk &&\n> -       test -d trunk/.git/svn &&\n> -       test -e trunk/foo &&\n> +       test_path_is_dir trunk/.git/svn &&\n> +       test_path_exists trunk/foo &&\n\nThese two changes make sense and the intent directly corresponds to\nthe original code.\n\n>  test_expect_success 'clone to target directory' '\n> -       test ! -d target &&\n> +       ! test_path_is_dir target &&\n>         git svn clone \"$svnrepo\"/project/trunk target &&\n> -       test -d target/.git/svn &&\n> -       test -e target/foo &&\n> +       test_path_is_dir target/.git/svn &&\n> +       test_path_exists target/foo &&\n>         rm -rf target\n>         '\n\nWhat follows is probably beyond the scope of your GSoC microproject,\nbut there is a bit more of interest to note about these tests.\n\nRather than asserting some initial condition at the start of the test,\nit is more common and more robust simply to _ensure_ that the desired\ninitial condition holds. So, for instance, instead of asserting `test\n! -d target`, modern practice is to ensure that `target` doesn't\nexist. Thus:\n\n    test_expect_success 'clone to target directory' '\n        rm -rf target &&\n        git svn clone \"$svnrepo\"/project/trunk target &&\n        ...\n\nis a more robust implementation. This also addresses the problem that\nthe `rm -rf target` at the very end of each test won't be executed if\nany command earlier in the test fails (due to the short-circuiting\nbehavior of the &&-operator).\n\nAs noted, this type of cleanup is probably overkill for your GSoC\nmicroproject so you need not tackle it. I mention it only for\ncompleteness. Also, if someone does tackle such a cleanup, it should\nbe done as multiple patches, each making one distinct change (i.e. one\npatch dropping `test !-d` and moving `rm -rf` to the start of the\ntest, and one which employs `test_path_foo` for the remaining `test\n-blah` invocations).\n"},{"id":"489726","messageId":"xmqqwmqm8rmr.fsf@gitster.g","threadId":"61029","inReplyTo":"20240301034606.69673-2-shejialuo@gmail.com","subject":"Re: [PATCH 1/1] t9117: prefer test_path_* helper functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-01T05:09:48Z","receivedAt":"2024-03-01T05:09:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo <shejialuo@gmail.com> writes:\n\n>  test_expect_success 'basic clone' '\n> -\ttest ! -d trunk &&\n> +\t! test_path_is_dir trunk &&\n\nThis is not quite right.  Step back and think why we are trying to\nuse the test_path_* helpers instead of \"test [!] -d\".  What are the\ndifferences between them?\n\nThe answer is that, unlike \"test [!] -d dir\" that is silent whether\n\"dir\" exists or missing, \"test_path_is_dir dir\" is *not* always\nsilent.  It gives useful messages as necessary.  When does it do so?\n\nHere is the definition, from t/test-lib-functions.sh around line\n930:\n\n        test_path_is_dir () {\n                test \"$#\" -ne 1 && BUG \"1 param\"\n                if ! test -d \"$1\"\n                then\n                        echo \"Directory $1 doesn't exist\"\n                        false\n                fi\n        }\n\nIt succeeds silently when \"test -d dir\" is true, but it complains\nloudly when \"test -d dir\" does not hold.  You will be told that the\ntest is unhappy because \"dir\" does not exist.  That would be easier\nto debug than one step among many in &&-chain silently fails.\n\nNow, let's look at the original you rewrote again:\n\n> -\ttest ! -d trunk &&\n\nIt says \"it is a failure if 'trunk' exists as a directory\".  If\n'trunk' does not exist, it is a very happy state for us.  So instead\nof silently failing when 'trunk' exists as a directory, you would\nwant to improve it so that you will get a complaint in such a case,\nsaying \"trunk should *not* exist but it does\".\n\nDid you succeed to do so with this rewrite?\n\n> +\t! test_path_is_dir trunk &&\n\nThe helper \"test_path_is_dir\" is called with \"trunk\".  As we saw, we\nwill see complaint when \"trunk\" does *NOT* exist.  When \"trunk\" does\nexist, it will be silent and \"test_path_is_dir\" will return a success,\nwhich will be inverted with \"!\" to make it a failure, causing &&-chain\nto fail.\n\nSo the exit status is not wrong, but it issues a complaint under the\nwrong condition.  That is not an improvement.\n\nLet's step back one more time.  Is the original test happy when\n\"trunk\" existed as a regular file?  \"test ! -d trunk\" says so, but\nshould it really be?  Think.\n\nI suspect that the test is not happy as long as 'trunk' exists,\nwhether it is a directory or a regular file or a symbolic link.\nIOW, it says \"I am unhappy if 'trunk' is a directory\", but what it\nreally meant to say was \"I am unhappy if there is anything at the\npath 'trunk'\".  IOW, \"test ! -e trunk\" would be what it really\nmeant, no?\n\nSo the correct rewrite for it would rather be something like\n\n\ttest_path_is_missing trunk &&\n\ninstead.  This will fail if anything is at path 'trunk', with an\nerror message saying there shouldn't be anything but there is.\n\nIn a peculiar case, which I do not think this one is, a test may\nlegitimately accept \"path\" to either (1) exist as long as it is not\na directory, or (2) be missing, as success.  In such a case, the\noriginal construct '! test -d path\" (or \"test ! -d path\") would be\nappropriate.\n\nBut I do not think we have a suitable wrapper to express such a\ncase, i.e. we do not have a helper like this.\n\n\ttest_path_is_not_dir () {\n\t\tif test -d \"$1\"\n\t\tthen\n\t\t\techo \"$1 is a directory but it should not be\"\n\t\t\tfalse\n\t\tfi\n\t}\n\nIf such a use case were common, we might even do this:\n\n\t# \"test_path_is_dir <dir>\" expects <dir> to be a directory.\n\t# \"test_path_is_dir ! <dir>\"  expects <dir> not to be a\n\t# directory.\n\t# In either case, complain only when the expectation is not met.\n\ttest_path_is_dir () {\n\t\tif test \"$1\" = \"!\"\n\t\tthen\n\t\t\tshift\n                        if test -d \"$1\"\n\t\t\tthen\n\t\t\t\techo \"$1 is a directory but it should not be\"\n\t\t\t\treturn 1\n\t\t\tfi\n\t\telse\n\t\t\tif test ! -d \"$1\"\n\t\t\tthen\n\t\t\t\techo \"$1 is not a directory\"\n\t\t\t\treturn 1\n\t\t\tfi\n\t\tfi\n\t\ttrue\n\t}\n\nbut \"we are happy even if path exists as long as it is not a\ndirectory\" is a very uncommon thing we want to say in our tests, so\nthat is why we do not have such a helper function.\n\nHTH.\n"},{"id":"489734","messageId":"20240301112914.121184-1-shejialuo@gmail.com","threadId":"61029","inReplyTo":"CAPig+cRfO8t1tdCL6MB4b9XopF3HkZ==hU83AFZ38b-2zsXDjQ@mail.gmail.com","subject":"Re: [PATCH 1/1] t9117: prefer test_path_* helper functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-01T11:29:14Z","receivedAt":"2024-03-01T11:29:22Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Thanks for your comment.\n\n> Although you make the test pass by using `!` to invert the result of\n> `test_path_is_dir`, the complaint will nevertheless get lodged, and\n> may very well be confusing for anyone scrutinizing the output of the\n> tests when running the script with `-v` or `-x`.\n\nI have run the script with `-v`, I have got the following result:\n\n  Directory trunk doesn't exist\n\nI come to realisize the fault with your dedicated comments. An assertion\nis an assertion.\n\nAnd I am impressed by the following idea:\n\n> Rather than asserting some initial condition at the start of the test,\n> it is more common and more robust simply to _ensure_ that the desired\n> initial condition holds. So, for instance, instead of asserting `test\n> ! -d target`, modern practice is to ensure that `target` doesn't\n> exist. Thus:\n>\n>    test_expect_success 'clone to target directory' '\n>        rm -rf target &&\n>        git svn clone \"$svnrepo\"/project/trunk target &&\n>        ...\n>\n> is a more robust implementation. This also addresses the problem that\n> the `rm -rf target` at the very end of each test won't be executed if\n> any command earlier in the test fails (due to the short-circuiting\n> behavior of the &&-operator).\n\nThe command `rm -rf target` ensures an exit status of 0 regardless of\nwhether the `target` exists. Thus the code will elegant make sure the\ninitial condition holds. I think I could add a patch to clean the code.\n\n"},{"id":"489735","messageId":"20240301113631.122477-1-shejialuo@gmail.com","threadId":"61029","inReplyTo":"xmqqwmqm8rmr.fsf@gitster.g","subject":"Re: [PATCH 1/1] t9117: prefer test_path_* helper functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-01T11:36:31Z","receivedAt":"2024-03-01T11:36:38Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Thanks for your wonderful comments. I have known that semantics is\nimportant not only the functionality. I will send a new patch at now.\n"},{"id":"489736","messageId":"20240301130334.135773-1-shejialuo@gmail.com","threadId":"61029","inReplyTo":"20240301034606.69673-1-shejialuo@gmail.com","subject":"[PATCH v3 0/1] t9117: prefer test_path_* helper functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-01T13:03:33Z","receivedAt":"2024-03-01T13:03:44Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"As discussed in v2, it is improper to use ! test_path_is_dir to replace\nthe test ! -f. This patch reverts the code.\n\nshejialuo (1):\n  t9117: prefer test_path_* helper functions\n\n t/t9117-git-svn-init-clone.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n-- \n2.44.0\n\n"},{"id":"489737","messageId":"20240301130334.135773-2-shejialuo@gmail.com","threadId":"61029","inReplyTo":"20240301130334.135773-1-shejialuo@gmail.com","subject":"[PATCH v3 1/1] [PATCH] t9117: prefer test_path_* helper functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-01T13:03:34Z","receivedAt":"2024-03-01T13:03:50Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"test -(e|f) does not provide a nice error message when we hit test\nfailures, so use test_path_exists, test_path_is_dir instead.\n\nSigned-off-by: shejialuo <shejialuo@gmail.com>\n---\n t/t9117-git-svn-init-clone.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh\nindex 62de819a44..3b038c338f 100755\n--- a/t/t9117-git-svn-init-clone.sh\n+++ b/t/t9117-git-svn-init-clone.sh\n@@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '\n test_expect_success 'basic clone' '\n \ttest ! -d trunk &&\n \tgit svn clone \"$svnrepo\"/project/trunk &&\n-\ttest -d trunk/.git/svn &&\n-\ttest -e trunk/foo &&\n+\ttest_path_is_dir trunk/.git/svn &&\n+\ttest_path_exists trunk/foo &&\n \trm -rf trunk\n \t'\n \n test_expect_success 'clone to target directory' '\n \ttest ! -d target &&\n \tgit svn clone \"$svnrepo\"/project/trunk target &&\n-\ttest -d target/.git/svn &&\n-\ttest -e target/foo &&\n+\ttest_path_is_dir target/.git/svn &&\n+\ttest_path_exists target/foo &&\n \trm -rf target\n \t'\n \n test_expect_success 'clone with --stdlayout' '\n \ttest ! -d project &&\n \tgit svn clone -s \"$svnrepo\"/project &&\n-\ttest -d project/.git/svn &&\n-\ttest -e project/foo &&\n+\ttest_path_is_dir project/.git/svn &&\n+\ttest_path_exists project/foo &&\n \trm -rf project\n \t'\n \n test_expect_success 'clone to target directory with --stdlayout' '\n \ttest ! -d target &&\n \tgit svn clone -s \"$svnrepo\"/project target &&\n-\ttest -d target/.git/svn &&\n-\ttest -e target/foo &&\n+\ttest_path_is_dir target/.git/svn &&\n+\ttest_path_exists target/foo &&\n \trm -rf target\n \t'\n \n-- \n2.44.0\n\n"},{"id":"489849","messageId":"84995a068640c72c8f17406ffa0441c7fdba4bdc.1709543804.git.ps@pks.im","threadId":"61029","inReplyTo":"xmqqzfvjf5tq.fsf@gitster.g","subject":"[PATCH] SoC 2024: clarify `test_path_is_*` conversion microproject","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T09:16:55Z","receivedAt":"2024-03-04T09:17:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"One of our proposed microprojects is to convert instances of `test -e`\nand related functions to instead use `test_path_exists` or similar. This\nconversion is only feasible when `test -e` is not used as part of a\ncontrol statement, as the replacement is used to _assert_ a condition\ninstead of merely testing for it.\n\nClarify the microproject's description accordingly.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n SoC-2024-Microprojects.md | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/SoC-2024-Microprojects.md b/SoC-2024-Microprojects.md\nindex 644c0a6..782441f 100644\n--- a/SoC-2024-Microprojects.md\n+++ b/SoC-2024-Microprojects.md\n@@ -41,7 +41,10 @@ to search, so that we can remove this microproject idea.\n Find one test script that verifies the presence/absence of\n files/directories with 'test -(e|f|d|...)' and replace them with the\n appropriate `test_path_is_file`, `test_path_is_dir`, etc. helper\n-functions.\n+functions. Note that this conversion does not directly apply to control\n+flow constructs like `if test -e ./path; then ...; fi` because the\n+replacements are intended to assert the condition instead of merely\n+testing for it.\n \n If you can't find one please tell us, along with the command you used\n to search, so that we can remove this microproject idea.\n-- \n2.44.0\n\n"},{"id":"489850","messageId":"ZeWRnGSU-eZq8WyE@tanuki","threadId":"61029","inReplyTo":"xmqqzfvjf5tq.fsf@gitster.g","subject":"Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T09:17:16Z","receivedAt":"2024-03-04T09:17:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 29, 2024 at 11:06:41AM -0800, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n> >> @@ -175,7 +175,7 @@ match() {\n> >>         test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '\n> >> -               if test -e .git/created_test_file\n> >> +               if test_path_exists .git/created_test_file\n> >>                 then\n> >>                         git reset &&\n> >\n> > ... which _do_ use test_path_exists() within a `test_expect_success`\n> > block. However, the changes are still undesirable because, as above,\n> > this `test -e` is merely part of the normal control-flow; it's not\n> > acting as an assertion, thus test_path_exists() -- which is an\n> > assertion -- is not correct.\n> >\n> > Unfortunately, none of the uses of`test -e` in t3070 are being used as\n> > assertions worthy of replacement with test_path_exists(), thus this\n> > isn't a good script in which to make such changes.\n> \n> It seems that there is a recurring confusion among mentorship\n> program applicants that use test_path_* helpers as their practice\n> material.  Perhaps the source of the information that suggests it as\n> a microproject is poorly phrased and needs to be rewritten to avoid\n> misleading them.\n> \n> I found one at https://git.github.io/Outreachy-23-Microprojects/,\n> which can be one source of such confusion:\n> \n>     Find one test script that verifies the presence/absence of\n>     files/directories with ‘test -(e|f|d|…)’ and replace them\n>     with the appropriate test_path_is_file, test_path_is_dir,\n>     etc. helper functions.\n> \n> but there may be others.\n> \n> This task specification does not differenciate \"test -[efdx]\" used\n> as a conditional of a control flow statement (which should never be\n> replaced by test_path_* helpers) and those used to directly fail the\n> &&-chain in test_expect_success with their exit status (which is the\n> target that test_path_* helpers are meant to improve).\n\nGood point. I've sent a patch in reply to your message that hopefully\nclarifies this a bit. Thanks!\n\nPatrick\n"},{"id":"489851","messageId":"ZeWTM3biKg_bsGaj@tanuki","threadId":"61029","inReplyTo":"20240301130334.135773-2-shejialuo@gmail.com","subject":"Re: [PATCH v3 1/1] [PATCH] t9117: prefer test_path_* helper functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T09:24:03Z","receivedAt":"2024-03-04T09:24:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 01, 2024 at 09:03:34PM +0800, shejialuo wrote:\n> test -(e|f) does not provide a nice error message when we hit test\n> failures, so use test_path_exists, test_path_is_dir instead.\n\nNit: you mention `test -e` and `test -f`, but then talk about\n`test_path_exists` (correct) and `test_path_is_dir` (wrong). You\nprobably meant to write `test -(e|d)`.\n\nOther than that all the conversions look correct to me. Thanks!\n\nPatrick\n\n> \n> Signed-off-by: shejialuo <shejialuo@gmail.com>\n> ---\n>  t/t9117-git-svn-init-clone.sh | 16 ++++++++--------\n>  1 file changed, 8 insertions(+), 8 deletions(-)\n> \n> diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh\n> index 62de819a44..3b038c338f 100755\n> --- a/t/t9117-git-svn-init-clone.sh\n> +++ b/t/t9117-git-svn-init-clone.sh\n> @@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '\n>  test_expect_success 'basic clone' '\n>  \ttest ! -d trunk &&\n>  \tgit svn clone \"$svnrepo\"/project/trunk &&\n> -\ttest -d trunk/.git/svn &&\n> -\ttest -e trunk/foo &&\n> +\ttest_path_is_dir trunk/.git/svn &&\n> +\ttest_path_exists trunk/foo &&\n>  \trm -rf trunk\n>  \t'\n>  \n>  test_expect_success 'clone to target directory' '\n>  \ttest ! -d target &&\n>  \tgit svn clone \"$svnrepo\"/project/trunk target &&\n> -\ttest -d target/.git/svn &&\n> -\ttest -e target/foo &&\n> +\ttest_path_is_dir target/.git/svn &&\n> +\ttest_path_exists target/foo &&\n>  \trm -rf target\n>  \t'\n>  \n>  test_expect_success 'clone with --stdlayout' '\n>  \ttest ! -d project &&\n>  \tgit svn clone -s \"$svnrepo\"/project &&\n> -\ttest -d project/.git/svn &&\n> -\ttest -e project/foo &&\n> +\ttest_path_is_dir project/.git/svn &&\n> +\ttest_path_exists project/foo &&\n>  \trm -rf project\n>  \t'\n>  \n>  test_expect_success 'clone to target directory with --stdlayout' '\n>  \ttest ! -d target &&\n>  \tgit svn clone -s \"$svnrepo\"/project target &&\n> -\ttest -d target/.git/svn &&\n> -\ttest -e target/foo &&\n> +\ttest_path_is_dir target/.git/svn &&\n> +\ttest_path_exists target/foo &&\n>  \trm -rf target\n>  \t'\n>  \n> -- \n> 2.44.0\n> \n> \n"},{"id":"489857","messageId":"20240304095436.56399-1-shejialuo@gmail.com","threadId":"61029","inReplyTo":"20240301130334.135773-1-shejialuo@gmail.com","subject":"[PATCH v4 0/1] Change commit message","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-04T09:54:35Z","receivedAt":"2024-03-04T09:54:49Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"This version changes the last version's error message.\n\nshejialuo (1):\n  t9117: prefer test_path_* helper functions\n\n t/t9117-git-svn-init-clone.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n-- \n2.44.0\n\n"},{"id":"489858","messageId":"20240304095436.56399-2-shejialuo@gmail.com","threadId":"61029","inReplyTo":"20240304095436.56399-1-shejialuo@gmail.com","subject":"[PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-04T09:54:36Z","receivedAt":"2024-03-04T09:55:02Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"test -(e|d) does not provide a nice error message when we hit test\nfailures, so use test_path_exists, test_path_is_dir instead.\n\nSigned-off-by: shejialuo <shejialuo@gmail.com>\n---\n t/t9117-git-svn-init-clone.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh\nindex 62de819a44..3b038c338f 100755\n--- a/t/t9117-git-svn-init-clone.sh\n+++ b/t/t9117-git-svn-init-clone.sh\n@@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '\n test_expect_success 'basic clone' '\n \ttest ! -d trunk &&\n \tgit svn clone \"$svnrepo\"/project/trunk &&\n-\ttest -d trunk/.git/svn &&\n-\ttest -e trunk/foo &&\n+\ttest_path_is_dir trunk/.git/svn &&\n+\ttest_path_exists trunk/foo &&\n \trm -rf trunk\n \t'\n \n test_expect_success 'clone to target directory' '\n \ttest ! -d target &&\n \tgit svn clone \"$svnrepo\"/project/trunk target &&\n-\ttest -d target/.git/svn &&\n-\ttest -e target/foo &&\n+\ttest_path_is_dir target/.git/svn &&\n+\ttest_path_exists target/foo &&\n \trm -rf target\n \t'\n \n test_expect_success 'clone with --stdlayout' '\n \ttest ! -d project &&\n \tgit svn clone -s \"$svnrepo\"/project &&\n-\ttest -d project/.git/svn &&\n-\ttest -e project/foo &&\n+\ttest_path_is_dir project/.git/svn &&\n+\ttest_path_exists project/foo &&\n \trm -rf project\n \t'\n \n test_expect_success 'clone to target directory with --stdlayout' '\n \ttest ! -d target &&\n \tgit svn clone -s \"$svnrepo\"/project target &&\n-\ttest -d target/.git/svn &&\n-\ttest -e target/foo &&\n+\ttest_path_is_dir target/.git/svn &&\n+\ttest_path_exists target/foo &&\n \trm -rf target\n \t'\n \n-- \n2.44.0\n\n"},{"id":"489862","messageId":"ZeWbdvFmhUYN9ekE@tanuki","threadId":"61029","inReplyTo":"20240304095436.56399-2-shejialuo@gmail.com","subject":"Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T09:59:18Z","receivedAt":"2024-03-04T09:59:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 04, 2024 at 05:54:36PM +0800, shejialuo wrote:\n> test -(e|d) does not provide a nice error message when we hit test\n> failures, so use test_path_exists, test_path_is_dir instead.\n> \n> Signed-off-by: shejialuo <shejialuo@gmail.com>\n\nThis version looks good to me, thanks!\n\nOne suggestion for potential future contributions by you: it's always\nhelpful to create a \"range-diff\" of what has changed between the\nprevious version of your patch series and the next one. Like this,\nreviewers can immediately see what the difference is between the two\nversions, which helps them to get the review done faster.\n\nAssuming you use git-format-patch(1) you can generate such a range diff\nwith the `--range-diff=` parameter.\n\nPatrick\n\n> ---\n>  t/t9117-git-svn-init-clone.sh | 16 ++++++++--------\n>  1 file changed, 8 insertions(+), 8 deletions(-)\n> \n> diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh\n> index 62de819a44..3b038c338f 100755\n> --- a/t/t9117-git-svn-init-clone.sh\n> +++ b/t/t9117-git-svn-init-clone.sh\n> @@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '\n>  test_expect_success 'basic clone' '\n>  \ttest ! -d trunk &&\n>  \tgit svn clone \"$svnrepo\"/project/trunk &&\n> -\ttest -d trunk/.git/svn &&\n> -\ttest -e trunk/foo &&\n> +\ttest_path_is_dir trunk/.git/svn &&\n> +\ttest_path_exists trunk/foo &&\n>  \trm -rf trunk\n>  \t'\n>  \n>  test_expect_success 'clone to target directory' '\n>  \ttest ! -d target &&\n>  \tgit svn clone \"$svnrepo\"/project/trunk target &&\n> -\ttest -d target/.git/svn &&\n> -\ttest -e target/foo &&\n> +\ttest_path_is_dir target/.git/svn &&\n> +\ttest_path_exists target/foo &&\n>  \trm -rf target\n>  \t'\n>  \n>  test_expect_success 'clone with --stdlayout' '\n>  \ttest ! -d project &&\n>  \tgit svn clone -s \"$svnrepo\"/project &&\n> -\ttest -d project/.git/svn &&\n> -\ttest -e project/foo &&\n> +\ttest_path_is_dir project/.git/svn &&\n> +\ttest_path_exists project/foo &&\n>  \trm -rf project\n>  \t'\n>  \n>  test_expect_success 'clone to target directory with --stdlayout' '\n>  \ttest ! -d target &&\n>  \tgit svn clone -s \"$svnrepo\"/project target &&\n> -\ttest -d target/.git/svn &&\n> -\ttest -e target/foo &&\n> +\ttest_path_is_dir target/.git/svn &&\n> +\ttest_path_exists target/foo &&\n>  \trm -rf target\n>  \t'\n>  \n> -- \n> 2.44.0\n> \n"},{"id":"489890","messageId":"20240304114531.62770-1-shejialuo@gmail.com","threadId":"61029","inReplyTo":"ZeWbdvFmhUYN9ekE@tanuki","subject":"Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-04T11:45:31Z","receivedAt":"2024-03-04T11:45:40Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Thanks for your advice. I will remember this suggestion.\n\n"},{"id":"489891","messageId":"CAP8UFD2Qzw8p5dTjTAjuKaSoHu22LBcJGUf3PG-NaK5CZOX0fw@mail.gmail.com","threadId":"61029","inReplyTo":"84995a068640c72c8f17406ffa0441c7fdba4bdc.1709543804.git.ps@pks.im","subject":"Re: [PATCH] SoC 2024: clarify `test_path_is_*` conversion microproject","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-03-04T13:42:40Z","receivedAt":"2024-03-04T13:42:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Mar 4, 2024 at 10:17 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> One of our proposed microprojects is to convert instances of `test -e`\n> and related functions to instead use `test_path_exists` or similar. This\n> conversion is only feasible when `test -e` is not used as part of a\n> control statement, as the replacement is used to _assert_ a condition\n> instead of merely testing for it.\n>\n> Clarify the microproject's description accordingly.\n\nApplied and pushed, thanks!\n\nI wonder if it would be better to create a PR in\nhttps://github.com/git/git.github.io/ and perhaps just send a link to\nit, rather than sending patches to the mailing list, as patches on the\nmailing list could be mistaken by tools and perhaps people as applying\nto the Git code base.\n"},{"id":"489901","messageId":"xmqqo7buq6b8.fsf@gitster.g","threadId":"61029","inReplyTo":"84995a068640c72c8f17406ffa0441c7fdba4bdc.1709543804.git.ps@pks.im","subject":"Re: [PATCH] SoC 2024: clarify `test_path_is_*` conversion microproject","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T17:02:03Z","receivedAt":"2024-03-04T17:02:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> One of our proposed microprojects is to convert instances of `test -e`\n> and related functions to instead use `test_path_exists` or similar. This\n> conversion is only feasible when `test -e` is not used as part of a\n> control statement, as the replacement is used to _assert_ a condition\n> instead of merely testing for it.\n>\n> Clarify the microproject's description accordingly.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  SoC-2024-Microprojects.md | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/SoC-2024-Microprojects.md b/SoC-2024-Microprojects.md\n> index 644c0a6..782441f 100644\n> --- a/SoC-2024-Microprojects.md\n> +++ b/SoC-2024-Microprojects.md\n> @@ -41,7 +41,10 @@ to search, so that we can remove this microproject idea.\n>  Find one test script that verifies the presence/absence of\n>  files/directories with 'test -(e|f|d|...)' and replace them with the\n>  appropriate `test_path_is_file`, `test_path_is_dir`, etc. helper\n> -functions.\n> +functions. Note that this conversion does not directly apply to control\n> +flow constructs like `if test -e ./path; then ...; fi` because the\n> +replacements are intended to assert the condition instead of merely\n> +testing for it.\n\nThanks for picking it up.  Of course there is one case in which we\nshould use test_path_* helpers to replace such an if...then...fi\nconstruct; e.g., c431a235 (t9146: replace test -d/-e/-f with\nappropriate test_path_is_* function, 2024-02-14) did exactly that.\n\nI am not sure how best to express that in the already crowded\ndescription above, though.  Rewriting the existing test this way\n\n\tFind one test script that uses 'test [!] -(e|f|d|...)' to\n\tassert the presence/absense of files/directories to make the\n\ttest fail directly with the exit status of such \"test\"\n\tcommands, and replace them with the appropriate helper\n\tfunctions like `test_path_is_file`, that give more\n\tinformative error messages when they fail.\n\nwould exclude use of \"test -e\" as a conditional in control statements,\nso we could mention what c431a235 did as an exception to the rule,\nperhaps like\n\n\tNote that the above excludes \"test -f\" and friends used as a\n\tcondition in control statements such as \"if test -e path\n\t...\", but as an exception, if such a \"if\" statement just\n\topen-codes what these helpers do, replacing it is warranted.\n\nBut that does not read very well, even to myself.  Sigh....\n\nThanks.\n"},{"id":"489904","messageId":"xmqqjzmhrk3b.fsf@gitster.g","threadId":"61029","inReplyTo":"20240304095436.56399-1-shejialuo@gmail.com","subject":"Re: [PATCH v4 0/1] Change commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T17:19:04Z","receivedAt":"2024-03-04T17:19:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo <shejialuo@gmail.com> writes:\n\n> This version changes the last version's error message.\n\nA cover letter is way overkill to tell the above to those who have\nread the previous iteration (which is minority of the reviewer\npopulation).  A comment after the three-dash line in the main patch\nwould be more appropriate.\n\nIf you need to have a cover letter, its title shouldn't be about the\ndifferences between the previous round and this round.  It should be\nabout the topic of the \"series\".\n\nThanks.\n"},{"id":"489905","messageId":"xmqq7cihrjxj.fsf@gitster.g","threadId":"61029","inReplyTo":"20240304095436.56399-2-shejialuo@gmail.com","subject":"Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T17:22:32Z","receivedAt":"2024-03-04T17:22:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo <shejialuo@gmail.com> writes:\n\n> test -(e|d) does not provide a nice error message when we hit test\n> failures, so use test_path_exists, test_path_is_dir instead.\n\nOK.\n\n>\n> Signed-off-by: shejialuo <shejialuo@gmail.com>\n> ---\n\nJust for the next single-patch topic you'd work on, here below the\nthree-dash line is where you may mention what's different between\nthe previous iteration and this one, if you wanted to, instead of\nhaving a separate cover-letter message.\n\n>  t/t9117-git-svn-init-clone.sh | 16 ++++++++--------\n>  1 file changed, 8 insertions(+), 8 deletions(-)\n\nThe patch looks good to me.  Thanks (and thanks for all the\nreviewers of the previous rounds).\n\n"},{"id":"489906","messageId":"xmqq34t5rina.fsf@gitster.g","threadId":"61029","inReplyTo":"ZeWbdvFmhUYN9ekE@tanuki","subject":"Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T17:50:17Z","receivedAt":"2024-03-04T17:50:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> This version looks good to me, thanks!\n>\n> One suggestion for potential future contributions by you: it's always\n> helpful to create a \"range-diff\" of what has changed between the\n> previous version of your patch series and the next one. Like this,\n> reviewers can immediately see what the difference is between the two\n> versions, which helps them to get the review done faster.\n>\n> Assuming you use git-format-patch(1) you can generate such a range diff\n> with the `--range-diff=` parameter.\n\nThanks for a review.\n"},{"id":"489950","messageId":"20240305114204.153171-1-shejialuo@gmail.com","threadId":"61029","inReplyTo":"xmqq7cihrjxj.fsf@gitster.g","subject":"Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-03-05T11:42:04Z","receivedAt":"2024-03-05T11:42:12Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"> Just for the next single-patch topic you'd work on, here below the\n> three-dash line is where you may mention what's different between\n> the previous iteration and this one, if you wanted to, instead of\n> having a separate cover-letter message.\n\nThanks for your suggestions.\n\nAt last, Thank every reviewer for your dedicated comments which make me\nlearn a lot.\n\n"}]}