{"thread":{"id":"62721","subject":"[PATCH] t7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing.","startedAt":"2025-01-01T22:59:22Z","lastAt":"2025-01-03T19:07:32Z","messageCount":8,"participants":["matteobagnolini","Patrick Steinhardt","Matteo Bagnolini","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"509783","messageId":"20250101225915.65185-1-matteobagnolini2003@gmail.com","threadId":"62721","inReplyTo":null,"subject":"[PATCH] t7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing.","fromName":"matteobagnolini","fromEmail":"matteobagnolini2003@gmail.com","sentAt":"2025-01-01T22:59:14Z","receivedAt":"2025-01-01T22:59:22Z","isPatch":true,"sender":{"key":"matteobagnolini2003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142099002?v=4"},"body":"`test -f` and `! test -f` do not provide clear error messages when they fail.\nTo enanche debuggability, use `test_path_is_file` and `test_path_is_missing`,\nwhich instead provides more informative error messages.\nNote that `! test -f` checks if a path is not a file, while\n`test_path_is_missing` verifies that a path does not exist. In this specific\ncase the tests are meant to check the absence of the path, making\n`test_path_is_missing` a valid replacement.\n\nSigned-off-by: matteobagnolini \n---\n t/t7110-reset-merge.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh\nindex 61669a2d21..9a335071af 100755\n--- a/t/t7110-reset-merge.sh\n+++ b/t/t7110-reset-merge.sh\n@@ -270,13 +270,13 @@ test_expect_success '--merge is ok with added/deleted merge' '\n \tgit reset --hard third &&\n \trm -f file2 &&\n \ttest_must_fail git merge branch3 &&\n-\t! test -f file2 &&\n-\ttest -f file3 &&\n+\ttest_path_is_missing file2 &&\n+\ttest_path_is_file file3 &&\n \tgit diff --exit-code file3 &&\n \tgit diff --exit-code branch3 file3 &&\n \tgit reset --merge HEAD &&\n-\t! test -f file3 &&\n-\t! test -f file2 &&\n+\ttest_path_is_missing file3 &&\n+\ttest_path_is_missing file2 &&\n \tgit diff --exit-code --cached\n '\n \n@@ -284,8 +284,8 @@ test_expect_success '--keep fails with added/deleted merge' '\n \tgit reset --hard third &&\n \trm -f file2 &&\n \ttest_must_fail git merge branch3 &&\n-\t! test -f file2 &&\n-\ttest -f file3 &&\n+\ttest_path_is_missing file2 &&\n+\ttest_path_is_file file3 &&\n \tgit diff --exit-code file3 &&\n \tgit diff --exit-code branch3 file3 &&\n \ttest_must_fail git reset --keep HEAD 2>err.log &&\n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"509839","messageId":"Z3fM1bT2Z_0GoD4w@pks.im","threadId":"62721","inReplyTo":"20250101225915.65185-1-matteobagnolini2003@gmail.com","subject":"Re: [PATCH] t7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing.","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T11:41:24Z","receivedAt":"2025-01-03T11:41:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 01, 2025 at 11:59:14PM +0100, matteobagnolini wrote:\n\nThe commit subject is overly long. We typically try to restrict them to\nat most 72 characters. Furthermore, the part after the subsystem should\nstart with a lower-case letter. Suggestion:\n\n t7110: replace `test -f` with `test_path_is_*` helpers\n\n> `test -f` and `! test -f` do not provide clear error messages when they fail.\n> To enanche debuggability, use `test_path_is_file` and `test_path_is_missing`,\n\ns/enanche/enhance\n\n> which instead provides more informative error messages.\n\ns/provides/provide\n\nIt looks like you intended to start a new paragraph here. These should\ntypically be separated by an empty line.\n\n> Note that `! test -f` checks if a path is not a file, while\n> `test_path_is_missing` verifies that a path does not exist. In this specific\n> case the tests are meant to check the absence of the path, making\n> `test_path_is_missing` a valid replacement.\n\nMakes sense.\n\n> Signed-off-by: matteobagnolini \n\nWe typically prefer proper legal names in the SOB.\n\n> diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh\n> index 61669a2d21..9a335071af 100755\n> --- a/t/t7110-reset-merge.sh\n> +++ b/t/t7110-reset-merge.sh\n> @@ -270,13 +270,13 @@ test_expect_success '--merge is ok with added/deleted merge' '\n> \tgit reset --hard third &&\n> \trm -f file2 &&\n> \ttest_must_fail git merge branch3 &&\n> -\t! test -f file2 &&\n> -\ttest -f file3 &&\n> +\ttest_path_is_missing file2 &&\n> +\ttest_path_is_file file3 &&\n> \tgit diff --exit-code file3 &&\n> \tgit diff --exit-code branch3 file3 &&\n> \tgit reset --merge HEAD &&\n> -\t! test -f file3 &&\n> -\t! test -f file2 &&\n> +\ttest_path_is_missing file3 &&\n> +\ttest_path_is_missing file2 &&\n> \tgit diff --exit-code --cached\n> '\n> \n> @@ -284,8 +284,8 @@ test_expect_success '--keep fails with added/deleted merge' '\n> \tgit reset --hard third &&\n> \trm -f file2 &&\n> \ttest_must_fail git merge branch3 &&\n> -\t! test -f file2 &&\n> -\ttest -f file3 &&\n> +\ttest_path_is_missing file2 &&\n> +\ttest_path_is_file file3 &&\n> \tgit diff --exit-code file3 &&\n> \tgit diff --exit-code branch3 file3 &&\n> \ttest_must_fail git reset --keep HEAD 2>err.log &&\n\nThe changes themselves look obviously good to me, thanks!\n\nPatrick\n"},{"id":"509844","messageId":"20250103130035.79376-1-matteobagnolini2003@gmail.com","threadId":"62721","inReplyTo":"Z3fM1bT2Z_0GoD4w@pks.im","subject":"[PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers","fromName":"Matteo Bagnolini","fromEmail":"matteobagnolini2003@gmail.com","sentAt":"2025-01-03T13:00:35Z","receivedAt":"2025-01-03T13:01:43Z","isPatch":true,"sender":{"key":"matteobagnolini2003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142099002?v=4"},"body":"From: matteobagnolini \n\n`test -f` and `! test -f` do not provide clear error messages when they fail.\nTo enhance debuggability, use `test_path_is_file` and `test_path_is_missing`,\nwhich instead provide more informative error messages.\n\nNote that `! test -f` checks if a path is not a file, while\n`test_path_is_missing` verifies that a path does not exist. In this specific\ncase the tests are meant to check the absence of the path, making\n`test_path_is_missing` a valid replacement.\n\nSigned-off-by: Matteo Bagnolini \n---\nSorry for avoidable spelling mistakes.\nUpdated commit message according to review.\n t/t7110-reset-merge.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh\nindex 61669a2d21..9a335071af 100755\n--- a/t/t7110-reset-merge.sh\n+++ b/t/t7110-reset-merge.sh\n@@ -270,13 +270,13 @@ test_expect_success '--merge is ok with added/deleted merge' '\n \tgit reset --hard third &&\n \trm -f file2 &&\n \ttest_must_fail git merge branch3 &&\n-\t! test -f file2 &&\n-\ttest -f file3 &&\n+\ttest_path_is_missing file2 &&\n+\ttest_path_is_file file3 &&\n \tgit diff --exit-code file3 &&\n \tgit diff --exit-code branch3 file3 &&\n \tgit reset --merge HEAD &&\n-\t! test -f file3 &&\n-\t! test -f file2 &&\n+\ttest_path_is_missing file3 &&\n+\ttest_path_is_missing file2 &&\n \tgit diff --exit-code --cached\n '\n \n@@ -284,8 +284,8 @@ test_expect_success '--keep fails with added/deleted merge' '\n \tgit reset --hard third &&\n \trm -f file2 &&\n \ttest_must_fail git merge branch3 &&\n-\t! test -f file2 &&\n-\ttest -f file3 &&\n+\ttest_path_is_missing file2 &&\n+\ttest_path_is_file file3 &&\n \tgit diff --exit-code file3 &&\n \tgit diff --exit-code branch3 file3 &&\n \ttest_must_fail git reset --keep HEAD 2>err.log &&\n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"509846","messageId":"Z3fiREGwXdILl-M1@pks.im","threadId":"62721","inReplyTo":"20250103130035.79376-1-matteobagnolini2003@gmail.com","subject":"Re: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T13:12:36Z","receivedAt":"2025-01-03T13:12:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:\n> From: matteobagnolini \n> \n> `test -f` and `! test -f` do not provide clear error messages when they fail.\n> To enhance debuggability, use `test_path_is_file` and `test_path_is_missing`,\n> which instead provide more informative error messages.\n> \n> Note that `! test -f` checks if a path is not a file, while\n> `test_path_is_missing` verifies that a path does not exist. In this specific\n> case the tests are meant to check the absence of the path, making\n> `test_path_is_missing` a valid replacement.\n\nThanks, this version looks good to me.\n\nPatrick\n"},{"id":"509865","messageId":"xmqqfrlzsw4j.fsf@gitster.g","threadId":"62721","inReplyTo":"Z3fiREGwXdILl-M1@pks.im","subject":"Re: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-03T16:25:00Z","receivedAt":"2025-01-03T16:25:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:\n>> From: matteobagnolini \n\nThis must match the author ident on the Signed-off-by: line.\n\n>> `test -f` and `! test -f` do not provide clear error messages when they fail.\n>> To enhance debuggability, use `test_path_is_file` and `test_path_is_missing`,\n>> which instead provide more informative error messages.\n>> \n>> Note that `! test -f` checks if a path is not a file, while\n>> `test_path_is_missing` verifies that a path does not exist. In this specific\n>> case the tests are meant to check the absence of the path, making\n>> `test_path_is_missing` a valid replacement.\n>\n> Thanks, this version looks good to me.\n>\n> Patrick\n\nThanks for writing, and thanks for reviewing.\n\n"},{"id":"509869","messageId":"CADyPGv0T7Wwrf_FidWDtLGOH7A88DcisAG4-jU3eEax106XEcQ@mail.gmail.com","threadId":"62721","inReplyTo":"xmqqfrlzsw4j.fsf@gitster.g","subject":"Re: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers","fromName":"Matteo Bagnolini","fromEmail":"matteobagnolini2003@gmail.com","sentAt":"2025-01-03T17:27:45Z","receivedAt":"2025-01-03T17:27:57Z","isPatch":true,"sender":{"key":"matteobagnolini2003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142099002?v=4"},"body":"Junio C Hamano writes:\n\n> Patrick Steinhardt writes:\n>\n> > On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:\n> >> From: matteobagnolini \n>\n> This must match the author ident on the Signed-off-by: line.\n\nSo should I send another patch with this correction?\nSorry for questioning, but as you might have noticed, this is my first time\ncontributing and I'm slowly trying to learn the process.\n\nMatteo\n"},{"id":"509871","messageId":"xmqq7c7bsrel.fsf@gitster.g","threadId":"62721","inReplyTo":"CADyPGv0T7Wwrf_FidWDtLGOH7A88DcisAG4-jU3eEax106XEcQ@mail.gmail.com","subject":"Re: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-03T18:06:58Z","receivedAt":"2025-01-03T18:07:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matteo Bagnolini writes:\n\n> Junio C Hamano writes:\n>\n>> Patrick Steinhardt writes:\n>>\n>> > On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote:\n>> >> From: matteobagnolini \n>>\n>> This must match the author ident on the Signed-off-by: line.\n>\n> So should I send another patch with this correction?\n> Sorry for questioning, but as you might have noticed, this is my first time\n> contributing and I'm slowly trying to learn the process.\n\nThanks for asking; if there are no other things that need fixing, I\ncan fix up while queuing.\n"},{"id":"509880","messageId":"CADyPGv2onyb-n80LMp+LNRij8oGZLstUkNpJXQcLKbVqcPzpEQ@mail.gmail.com","threadId":"62721","inReplyTo":"xmqq7c7bsrel.fsf@gitster.g","subject":"Re: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers","fromName":"Matteo Bagnolini","fromEmail":"matteobagnolini2003@gmail.com","sentAt":"2025-01-03T19:07:21Z","receivedAt":"2025-01-03T19:07:32Z","isPatch":true,"sender":{"key":"matteobagnolini2003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142099002?v=4"},"body":"Junio C Hamano writes:\n\n> Thanks for asking; if there are no other things that need fixing, I\n> can fix up while queuing.\n\nYes, everything else should be OK. Thank you.\n\nMatteo\n"}]}