{"thread":{"id":"63151","subject":"[PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)","startedAt":"2025-03-18T11:58:35Z","lastAt":"2025-03-24T14:23:22Z","messageCount":5,"participants":["Sampriyo Guin via GitGitGadget","Eric Sunshine","Junio C Hamano","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"514514","messageId":"pull.1918.git.git.1742299111861.gitgitgadget@gmail.com","threadId":"63151","inReplyTo":null,"subject":"[PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)","fromName":"Sampriyo Guin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-18T11:58:31Z","receivedAt":"2025-03-18T11:58:35Z","isPatch":true,"sender":{"key":"sampriyoguin@gmail.com","avatar":null},"body":"From: rimo \n\ntest -e changed to test_path_exists\ntest -f changed to test_path_is_file\n\nSigned-off-by: Sampriyo Guin \n---\n [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite\n \n This is my first git contribution. A simple fix as specified in Git\n Microprojects. I have tested using Github Actions on my private\n repository. Your comments and feedbacks are much appreciated. Thanks!\n \n , Jialuo She shejialuo@gmail.com , Christian Couder\n christian.couder@gmail.com, Ghanshyam Thakkar shyamthakkar001@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1918%2FRimoGuin%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1918/RimoGuin/master-v1\nPull-Request: https://github.com/git/git/pull/1918\n\n t/chainlint/chained-subshell.expect | 2 +-\n t/chainlint/chained-subshell.test | 2 +-\n t/chainlint/function.expect | 2 +-\n t/chainlint/function.test | 2 +-\n 4 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect\nindex 93fb1a6578b..76393efcd20 100644\n--- a/t/chainlint/chained-subshell.expect\n+++ b/t/chainlint/chained-subshell.expect\n@@ -5,6 +5,6 @@\n 6 ) &&\n 7 \n 8 cut \"-d \" -f actual | (read s1 s2 s3 &&\n-9 test -f $s1 ?!LINT: missing '&&'?!\n+9 test_path_is_file $s1 ?!LINT: missing '&&'?!\n 10 test $(cat $s2) = tree2path1 &&\n 11 test $(cat $s3) = tree3path1)\ndiff --git a/t/chainlint/chained-subshell.test b/t/chainlint/chained-subshell.test\nindex 1f11f653982..997f30eadd7 100644\n--- a/t/chainlint/chained-subshell.test\n+++ b/t/chainlint/chained-subshell.test\n@@ -8,7 +8,7 @@ mkdir sub && (\n \n # LINT: preceding command pipes to subshell on same line\n cut \"-d \" -f actual | (read s1 s2 s3 &&\n-test -f $s1\n+test_path_is_file $s1\n test $(cat $s2) = tree2path1 &&\n # LINT: closing subshell \")\" correctly detected on same line as \"$(...)\"\n test $(cat $s3) = tree3path1)\ndiff --git a/t/chainlint/function.expect b/t/chainlint/function.expect\nindex 9e46a3554a1..2edbeb5e4e2 100644\n--- a/t/chainlint/function.expect\n+++ b/t/chainlint/function.expect\n@@ -4,7 +4,7 @@\n 5 \n 6 remove_object() {\n 7 \tfile=$(sha1_file \"$*\") &&\n-8 \ttest -e \"$file\" ?!LINT: missing '&&'?!\n+8 \ttest_path_exists \"$file\" ?!LINT: missing '&&'?!\n 9 \trm -f \"$file\"\n 10 } ?!LINT: missing '&&'?!\n 11 \ndiff --git a/t/chainlint/function.test b/t/chainlint/function.test\nindex 763fcf3f878..2f2a5c251f4 100644\n--- a/t/chainlint/function.test\n+++ b/t/chainlint/function.test\n@@ -7,7 +7,7 @@ sha1_file() {\n # LINT: broken &&-chain in function and after function\n remove_object() {\n \tfile=$(sha1_file \"$*\") &&\n-\ttest -e \"$file\"\n+\ttest_path_exists \"$file\"\n \trm -f \"$file\"\n }\n \n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \ngitgitgadget\n"},{"id":"514541","messageId":"CAPig+cQrCdtN4+hyZyPWQuvnoetarbcgkwKU7cD-TpzfzK=jzw@mail.gmail.com","threadId":"63151","inReplyTo":"pull.1918.git.git.1742299111861.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-03-18T17:39:35Z","receivedAt":"2025-03-18T17:39:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for submitting this GSoC microproject. See comments below...\n\nOn Tue, Mar 18, 2025 at 7:58 AM Sampriyo Guin via GitGitGadget\n wrote:\n> From: rimo \n\nThis name should match the Signed-off-by: name. Since the \"From:\"\nheader is generated from the author information in the commit, you\nprobably need to adjust your \"user.name\" configuration to fix this.\n\n> test -e changed to test_path_exists\n> test -f changed to test_path_is_file\n\nPeople reading the patch would like to know why a change is being\nmade, so this is where you should explain the reason (for instance,\n\"the test_path_* functions provide better diagnostics upon failure\" or\nsuch). As Karthik mentioned[*], read the \"Describe your changes well\"\nsection in Documentation/SubmittingPatches to learn how to craft a\ngood commit message.\n\n[*]: https://lore.kernel.org/git/CAOLa=ZSkMp+H9PZeBZXK47=fx1sH=S54AuPT=oUosm7F7V8MGg@mail.gmail.com/\n\n> Signed-off-by: Sampriyo Guin \n> ---\n> , Jialuo She shejialuo@gmail.com , Christian Couder\n> christian.couder@gmail.com, Ghanshyam Thakkar shyamthakkar001@gmail.com\n\nIt appears that GitGitGadget didn't like how this list was formatted.\nInstead, place each recipient on its own Cc: line.\n\n> t/chainlint/chained-subshell.expect | 2 +-\n> t/chainlint/chained-subshell.test | 2 +-\n> t/chainlint/function.expect | 2 +-\n> t/chainlint/function.test | 2 +-\n> 4 files changed, 4 insertions(+), 4 deletions(-)\n\nLet's not touch any of the \"chainlint\" files; they are checking\nvalidity of a completely separate tool (\"chainlint\"), and have nothing\nto do with checking Git itself. Instead, pick one of the t/t???-*.sh\nfiles.\n"},{"id":"514548","messageId":"xmqqiko62kw5.fsf@gitster.g","threadId":"63151","inReplyTo":"CAPig+cQrCdtN4+hyZyPWQuvnoetarbcgkwKU7cD-TpzfzK=jzw@mail.gmail.com","subject":"Re: [PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-18T19:34:18Z","receivedAt":"2025-03-18T19:34:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Thanks for submitting this GSoC microproject. See comments below...\n>\n> On Tue, Mar 18, 2025 at 7:58 AM Sampriyo Guin via GitGitGadget\n> wrote:\n>> From: rimo \n>\n> This name should match the Signed-off-by: name. Since the \"From:\"\n> header is generated from the author information in the commit, you\n> probably need to adjust your \"user.name\" configuration to fix this.\n>\n>> test -e changed to test_path_exists\n>> test -f changed to test_path_is_file\n>\n> People reading the patch would like to know why a change is being\n> made, so this is where you should explain the reason (for instance,\n> \"the test_path_* functions provide better diagnostics upon failure\" or\n> such). As Karthik mentioned[*], read the \"Describe your changes well\"\n> section in Documentation/SubmittingPatches to learn how to craft a\n> good commit message.\n>\n> [*]: https://lore.kernel.org/git/CAOLa=ZSkMp+H9PZeBZXK47=fx1sH=S54AuPT=oUosm7F7V8MGg@mail.gmail.com/\n>\n>> Signed-off-by: Sampriyo Guin \n>> ---\n>> , Jialuo She shejialuo@gmail.com , Christian Couder\n>> christian.couder@gmail.com, Ghanshyam Thakkar shyamthakkar001@gmail.com\n>\n> It appears that GitGitGadget didn't like how this list was formatted.\n> Instead, place each recipient on its own Cc: line.\n>\n>> t/chainlint/chained-subshell.expect | 2 +-\n>> t/chainlint/chained-subshell.test | 2 +-\n>> t/chainlint/function.expect | 2 +-\n>> t/chainlint/function.test | 2 +-\n>> 4 files changed, 4 insertions(+), 4 deletions(-)\n>\n> Let's not touch any of the \"chainlint\" files; they are checking\n> validity of a completely separate tool (\"chainlint\"), and have nothing\n> to do with checking Git itself. Instead, pick one of the t/t???-*.sh\n> files.\n\nYeah, these changes to make them use test_path_* are not \"fixes\" but\nsomething else. The first step for a contributor is to understand\nwhy \"test_path_*\" are preferred over \"test -[def]\" and in what\ncontext, but touching these files shows that such understanding is\nmissing, unfortunately.\n\nI find the \"as specified in Git Microprojects\" in the patch\ndescription the most disturbing,\n\n A simple fix as specified in Git Microprojects.\n\nas it may be an indication that some introductory write-up is\nmisleading potential students in a wrong direction. Our mentors may\nneed a bit more handholding at this early stage of dipping your toes\nin the water step, perhaps? Or is it up to the aspiring students to\ndo their homework?\n\n\n\n"},{"id":"514557","messageId":"CAPig+cRm+sc+Rk-4SuQ5CrPeZLG2Nzz9B7+6OZxCq7tV5mzmBA@mail.gmail.com","threadId":"63151","inReplyTo":"xmqqiko62kw5.fsf@gitster.g","subject":"Re: [PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-03-18T21:36:46Z","receivedAt":"2025-03-18T21:36:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 18, 2025 at 3:34 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > On Tue, Mar 18, 2025 at 7:58 AM Sampriyo Guin via GitGitGadget\n> > wrote:\n> >> t/chainlint/chained-subshell.expect | 2 +-\n> >> t/chainlint/chained-subshell.test | 2 +-\n> >\n> > Let's not touch any of the \"chainlint\" files; they are checking\n> > validity of a completely separate tool (\"chainlint\"), and have nothing\n> > to do with checking Git itself. Instead, pick one of the t/t???-*.sh\n> > files.\n>\n> Yeah, these changes to make them use test_path_* are not \"fixes\" but\n> something else. The first step for a contributor is to understand\n> why \"test_path_*\" are preferred over \"test -[def]\" and in what\n> context, but touching these files shows that such understanding is\n> missing, unfortunately.\n>\n> I find the \"as specified in Git Microprojects\" in the patch\n> description the most disturbing,\n>\n> A simple fix as specified in Git Microprojects.\n>\n> as it may be an indication that some introductory write-up is\n> misleading potential students in a wrong direction. Our mentors may\n> need a bit more handholding at this early stage of dipping your toes\n> in the water step, perhaps? Or is it up to the aspiring students to\n> do their homework?\n\nI'm not sure where the GSoC microproject ideas are maintained these\ndays, but it may indeed be the case that (at least this microproject)\ncould be spelled out in more detail to help lead newcomers in the\ncorrect direction. If not already mentioned, at least these\nclarifications probably ought to be made:\n\n* only work on t/t????-*.sh scripts\n\n* pick just one script (so as to avoid exhausting the pool for other candidates)\n\n* only convert `test -[def]` instances which semantically are\nassertions (i.e. used as part of a &&-chain)\n"},{"id":"514930","messageId":"CAOLa=ZTGKDG9tfO-DRqz6A6V7BtQYx-HBS19KvZmM4dH+=BZQA@mail.gmail.com","threadId":"63151","inReplyTo":"CAPig+cRm+sc+Rk-4SuQ5CrPeZLG2Nzz9B7+6OZxCq7tV5mzmBA@mail.gmail.com","subject":"Re: [PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-24T14:23:20Z","receivedAt":"2025-03-24T14:23:22Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Mar 18, 2025 at 3:34 PM Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>> > On Tue, Mar 18, 2025 at 7:58 AM Sampriyo Guin via GitGitGadget\n>> > wrote:\n>> >> t/chainlint/chained-subshell.expect | 2 +-\n>> >> t/chainlint/chained-subshell.test | 2 +-\n>> >\n>> > Let's not touch any of the \"chainlint\" files; they are checking\n>> > validity of a completely separate tool (\"chainlint\"), and have nothing\n>> > to do with checking Git itself. Instead, pick one of the t/t???-*.sh\n>> > files.\n>>\n>> Yeah, these changes to make them use test_path_* are not \"fixes\" but\n>> something else. The first step for a contributor is to understand\n>> why \"test_path_*\" are preferred over \"test -[def]\" and in what\n>> context, but touching these files shows that such understanding is\n>> missing, unfortunately.\n>>\n>> I find the \"as specified in Git Microprojects\" in the patch\n>> description the most disturbing,\n>>\n>> A simple fix as specified in Git Microprojects.\n>>\n>> as it may be an indication that some introductory write-up is\n>> misleading potential students in a wrong direction. Our mentors may\n>> need a bit more handholding at this early stage of dipping your toes\n>> in the water step, perhaps? Or is it up to the aspiring students to\n>> do their homework?\n>\n> I'm not sure where the GSoC microproject ideas are maintained these\n> days, but it may indeed be the case that (at least this microproject)\n> could be spelled out in more detail to help lead newcomers in the\n> correct direction. If not already mentioned, at least these\n> clarifications probably ought to be made:\n>\n> * only work on t/t????-*.sh scripts\n>\n> * pick just one script (so as to avoid exhausting the pool for other candidates)\n>\n> * only convert `test -[def]` instances which semantically are\n> assertions (i.e. used as part of a &&-chain)\n\nI think these are really good points, we ought to add to the\nmicroproject page, I've raised a pull request on the repository to do so\n[1].\n\n[1]: https://github.com/git/git.github.io/pull/760\n"}]}