{"thread":{"id":"63648","subject":"[PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'","startedAt":"2025-06-16T02:08:33Z","lastAt":"2025-06-16T04:17:16Z","messageCount":6,"participants":["Rodrigo Michelassi","Eric Sunshine","Carlo Marcelo Arenas Belón","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"520277","messageId":"20250616020827.10820-1-rodmichelassi@gmail.com","threadId":"63648","inReplyTo":null,"subject":"[PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'","fromName":"Rodrigo Michelassi","fromEmail":"rodmichelassi@gmail.com","sentAt":"2025-06-16T02:08:27Z","receivedAt":"2025-06-16T02:08:33Z","isPatch":true,"sender":{"key":"rodmichelassi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/101156335?v=4"},"body":"From: rodrigocmichelassi <rodmichelassi@gmail.com>\n\n'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach\n\nSigned-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>\n\nCo-authored-by: Isabella Caselli <icaselli@usp.br>\nSigned-off-by: Isabella Caselli <icaselli@usp.br>\n---\n t/t2400-worktree-add.sh | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 90638fa886..7ab38ac4c4 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -42,8 +42,8 @@ test_expect_success '\"add\" using - shorthand' '\n \n test_expect_success '\"add\" refuses to checkout locked branch' '\n \ttest_must_fail git worktree add zere main &&\n-\t! test -d zere &&\n-\t! test -d .git/worktrees/zere\n+\t! test_path_is_dir zere &&\n+\t! test_path_is_dir .git/worktrees/zere\n '\n \n test_expect_success 'checking out paths not complaining about linked checkouts' '\n@@ -70,14 +70,14 @@ test_expect_success '\"add\" worktree' '\n test_expect_success '\"add\" worktree with lock' '\n \tgit worktree add --detach --lock here-with-lock main &&\n \ttest_when_finished \"git worktree unlock here-with-lock || :\" &&\n-\ttest -f .git/worktrees/here-with-lock/locked\n+\ttest_path_is_file .git/worktrees/here-with-lock/locked\n '\n \n test_expect_success '\"add\" worktree with lock and reason' '\n \tlock_reason=\"why not\" &&\n \tgit worktree add --detach --lock --reason \"$lock_reason\" here-with-lock-reason main &&\n \ttest_when_finished \"git worktree unlock here-with-lock-reason || :\" &&\n-\ttest -f .git/worktrees/here-with-lock-reason/locked &&\n+\ttest_path_is_file .git/worktrees/here-with-lock-reason/locked &&\n \techo \"$lock_reason\" >expect &&\n \ttest_cmp expect .git/worktrees/here-with-lock-reason/locked\n '\n@@ -412,14 +412,14 @@ test_expect_success '\"add --orphan\" with empty repository' '\n test_expect_success '\"add\" worktree with orphan branch and lock' '\n \tgit worktree add --lock --orphan -b orphanbr orphan-with-lock &&\n \ttest_when_finished \"git worktree unlock orphan-with-lock || :\" &&\n-\ttest -f .git/worktrees/orphan-with-lock/locked\n+\ttest_path_is_file .git/worktrees/orphan-with-lock/locked\n '\n \n test_expect_success '\"add\" worktree with orphan branch, lock, and reason' '\n \tlock_reason=\"why not\" &&\n \tgit worktree add --detach --lock --reason \"$lock_reason\" orphan-with-lock-reason main &&\n \ttest_when_finished \"git worktree unlock orphan-with-lock-reason || :\" &&\n-\ttest -f .git/worktrees/orphan-with-lock-reason/locked &&\n+\ttest_path_is_file .git/worktrees/orphan-with-lock-reason/locked &&\n \techo \"$lock_reason\" >expect &&\n \ttest_cmp expect .git/worktrees/orphan-with-lock-reason/locked\n '\n@@ -474,7 +474,7 @@ test_expect_success 'local clone --shared from linked checkout' '\n \n test_expect_success '\"add\" worktree with --no-checkout' '\n \tgit worktree add --no-checkout -b swamp swamp &&\n-\t! test -e swamp/init.t &&\n+\t! test_path_is_executable swamp/init.t &&\n \tgit -C swamp reset --hard &&\n \ttest_cmp init.t swamp/init.t\n '\n@@ -497,7 +497,7 @@ test_expect_success 'put a worktree under rebase' '\n \n test_expect_success 'add a worktree, checking out a rebased branch' '\n \ttest_must_fail git worktree add new-rebase under-rebase &&\n-\t! test -d new-rebase\n+\t! test_path_is_dir new-rebase\n '\n \n test_expect_success 'checking out a rebased branch from another worktree' '\n@@ -535,7 +535,7 @@ test_expect_success 'checkout a branch under bisect' '\n \t\tgit worktree list >actual &&\n \t\tgrep \"under-bisect.*detached HEAD\" actual &&\n \t\ttest_must_fail git worktree add new-bisect under-bisect &&\n-\t\t! test -d new-bisect\n+\t\t! test_path_is_dir new-bisect\n \t)\n '\n \n@@ -1165,7 +1165,7 @@ test_expect_success '\"add\" not tripped up by magic worktree matching\"' '\n \n test_expect_success FUNNYNAMES 'sanitize generated worktree name' '\n \tgit worktree add --detach \".  weird*..?.lock.lock\" &&\n-\ttest -d .git/worktrees/---weird-.-\n+\ttest_path_is_dir .git/worktrees/---weird-.-\n '\n \n test_expect_success '\"add\" should not fail because of another bad worktree' '\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"520278","messageId":"CAPig+cT1VfY8QiUvrrV3-obTBP1439b6iwaebJtGwML5MScnQA@mail.gmail.com","threadId":"63648","inReplyTo":"20250616020827.10820-1-rodmichelassi@gmail.com","subject":"Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-06-16T03:39:56Z","receivedAt":"2025-06-16T03:40:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the patch. See below for some comments...\n\nOn Sun, Jun 15, 2025 at 10:08 PM Rodrigo Michelassi\n<rodmichelassi@gmail.com> wrote:\n> From: rodrigocmichelassi <rodmichelassi@gmail.com>\n\nThe From: header name/address should match your Signed-off-by:\ntrailer, so you'll probably need to adjust your mailer settings.\n\n> replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'\n\nLet's prefix the subject with the area you're touching. In this case,\nthe test number would be appropriate, so:\n\n    t2400: replace 'test -[efd]' with test_path_* calls\n\n> 'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach\n\nA better way to convince reviewers that this is a good idea is to\nexplain why these functions are superior. In this case, it's because\nthey emit useful diagnostic information when they detect a failing\ncondition, whereas `test` itself does not.\n\nPlease wrap the commit message at about the 72-column mark.\n\n> Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>\n>\n> Co-authored-by: Isabella Caselli <icaselli@usp.br>\n> Signed-off-by: Isabella Caselli <icaselli@usp.br>\n\nYou'll probably want to order these trailers like this:\n\n    Co-authored-by: Isabella Caselli <icaselli@usp.br>\n    Signed-off-by: Isabella Caselli <icaselli@usp.br>\n    Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>\n\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> @@ -42,8 +42,8 @@ test_expect_success '\"add\" using - shorthand' '\n>  test_expect_success '\"add\" refuses to checkout locked branch' '\n>         test_must_fail git worktree add zere main &&\n> -       ! test -d zere &&\n> -       ! test -d .git/worktrees/zere\n> +       ! test_path_is_dir zere &&\n> +       ! test_path_is_dir .git/worktrees/zere\n\nTake a look at the definition of `test_path_is_dir` from\nt/test-lib-functions.sh to see why this change is undesirable:\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\nThe test wants to assert that those directories do *not* exist, which\nmeans that if `git worktree add` is working correctly, then the\ndirectories indeed will not exist. However, `test_path_is_dir` is\ngoing to complain that they don't exist, which is the opposite of what\nwe want. So, even though the test will continue to pass following this\nchange (due to the negation `!`), it's going to be spewing unwanted\nand confusing error messages.\n\nWhat you want instead is a test_path_* function which asserts that a\npath does not exist. Probably the closest we have is\n`test_path_is_missing` which seems a good semantic match for what this\ntest intends with regards to those directories.\n\n> @@ -474,7 +474,7 @@ test_expect_success 'local clone --shared from linked checkout' '\n>  test_expect_success '\"add\" worktree with --no-checkout' '\n>         git worktree add --no-checkout -b swamp swamp &&\n> -       ! test -e swamp/init.t &&\n> +       ! test_path_is_executable swamp/init.t &&\n\nIf you look at the definition of `test_path_is_executable` in\nt/test-lib-functions.sh, you'll see that this change is similarly\nundesirable.\n\nThe same comments apply to several other changes in this patch.\n"},{"id":"520279","messageId":"cioavyfxwgdhfzuodb7dnwzmvsui4xcxi6mljnlszjwebogajg@taejma43hgtx","threadId":"63648","inReplyTo":"20250616020827.10820-1-rodmichelassi@gmail.com","subject":"Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-16T03:53:42Z","receivedAt":"2025-06-16T03:53:44Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sun, Jun 15, 2025 at 11:08:27PM -0800, Rodrigo Michelassi wrote:\n> From: rodrigocmichelassi <rodmichelassi@gmail.com>\n> \n> 'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach\n\nSplitting this long line, into multiple lines of about 70ish columns is\nbetter, see the relevant documents in Documentation/ for useful suggestions.\n\n> Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>\n\nThis certifies that you are the author of the code, an therefore should go\nafter Isabella's, who might be the original author which you improved upon.\n\n> @@ -474,7 +474,7 @@ test_expect_success 'local clone --shared from linked checkout' '\n>  \n>  test_expect_success '\"add\" worktree with --no-checkout' '\n>  \tgit worktree add --no-checkout -b swamp swamp &&\n> -\t! test -e swamp/init.t &&\n> +\t! test_path_is_executable swamp/init.t &&\n\nthis is not acurate translation, `test -e` is true if there is any \"file\"\nwith that name, the equivalent for that helper function would be `test -x`\n\nCarlo\n"},{"id":"520280","messageId":"xmqq1prk490o.fsf@gitster.g","threadId":"63648","inReplyTo":"CAPig+cT1VfY8QiUvrrV3-obTBP1439b6iwaebJtGwML5MScnQA@mail.gmail.com","subject":"Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-16T04:15:19Z","receivedAt":"2025-06-16T04:15:22Z","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> Thanks for the patch. See below for some comments...\n>\n> On Sun, Jun 15, 2025 at 10:08 PM Rodrigo Michelassi\n> <rodmichelassi@gmail.com> wrote:\n>> From: rodrigocmichelassi <rodmichelassi@gmail.com>\n>\n> The From: header name/address should match your Signed-off-by:\n> trailer, so you'll probably need to adjust your mailer settings.\n\nThis is an in-body header most likely added by git-send-email, so\nthe name string is what the commit object recorded as its author.\nWhat needs to be adjusted is not the mailer settings, but user.name,\nif that is indeed the case.\n\n>> replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'\n>\n> Let's prefix the subject with the area you're touching. In this case,\n> the test number would be appropriate, so:\n>\n>     t2400: replace 'test -[efd]' with test_path_* calls\n\nExcellent.\n\n>> 'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach\n>\n> A better way to convince reviewers that this is a good idea is to\n> explain why these functions are superior. In this case, it's because\n> they emit useful diagnostic information when they detect a failing\n> condition, whereas `test` itself does not.\n>\n> Please wrap the commit message at about the 72-column mark.\n\nGood.\n\n>> Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>\n>>\n>> Co-authored-by: Isabella Caselli <icaselli@usp.br>\n>> Signed-off-by: Isabella Caselli <icaselli@usp.br>\n>\n> You'll probably want to order these trailers like this:\n>\n>     Co-authored-by: Isabella Caselli <icaselli@usp.br>\n>     Signed-off-by: Isabella Caselli <icaselli@usp.br>\n>     Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>\n\nVery true.  And without a blank line in between.\n\nI too spotted the misuse of negation in the test script you spotted.\nThanks for explaining why they are bad and what should be used\ninstead.\n\n"},{"id":"520281","messageId":"xmqqwm9c2uef.fsf@gitster.g","threadId":"63648","inReplyTo":"cioavyfxwgdhfzuodb7dnwzmvsui4xcxi6mljnlszjwebogajg@taejma43hgtx","subject":"Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-16T04:16:24Z","receivedAt":"2025-06-16T04:16:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:\n\n> On Sun, Jun 15, 2025 at 11:08:27PM -0800, Rodrigo Michelassi wrote:\n>> From: rodrigocmichelassi <rodmichelassi@gmail.com>\n>> \n>> 'test_path_is_file', 'test_path_is_dir' and 'test_file_is_executable' are modern path checking methods in Git's development. Replace the basic shell commands 'test -f', 'test -d' and 'test -e', respectively, with this approach\n>\n> Splitting this long line, into multiple lines of about 70ish columns is\n> better, see the relevant documents in Documentation/ for useful suggestions.\n>\n>> Signed-off-by: Rodrigo Michelassi <rodmichelassi@gmail.com>\n>\n> This certifies that you are the author of the code, an therefore should go\n> after Isabella's, who might be the original author which you improved upon.\n>\n>> @@ -474,7 +474,7 @@ test_expect_success 'local clone --shared from linked checkout' '\n>>  \n>>  test_expect_success '\"add\" worktree with --no-checkout' '\n>>  \tgit worktree add --no-checkout -b swamp swamp &&\n>> -\t! test -e swamp/init.t &&\n>> +\t! test_path_is_executable swamp/init.t &&\n>\n> this is not acurate translation, `test -e` is true if there is any \"file\"\n> with that name, the equivalent for that helper function would be `test -x`\n\nYes, \"test_path_is_missing\" is what you want here, without any \"!\",\nto make sure that the path does not exist.\n"},{"id":"520282","messageId":"CAPig+cQv80O+6y_LkyW0dOOYAaY3iFNva0CDLM9WSb_J9oT0sQ@mail.gmail.com","threadId":"63648","inReplyTo":"xmqq1prk490o.fsf@gitster.g","subject":"Re: [PATCH] replace 'test -[efd]' with 'test_path_is_[file,dir,executable]'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-06-16T04:17:04Z","receivedAt":"2025-06-16T04:17:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 16, 2025 at 12:15 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> >> From: rodrigocmichelassi <rodmichelassi@gmail.com>\n> >\n> > The From: header name/address should match your Signed-off-by:\n> > trailer, so you'll probably need to adjust your mailer settings.\n>\n> This is an in-body header most likely added by git-send-email, so\n> the name string is what the commit object recorded as its author.\n> What needs to be adjusted is not the mailer settings, but user.name,\n> if that is indeed the case.\n\nThanks for the correction.\n"}]}