{"thread":{"id":"63033","subject":"[GSOC][PATCH 0/1] t1403: prefer test_path_exists helper function","startedAt":"2025-03-01T10:59:16Z","lastAt":"2025-03-04T18:30:03Z","messageCount":31,"participants":["Mahendra Dani","Patrick Steinhardt","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"513328","messageId":"20250301105838.1481-1-danimahendra0904@gmail.com","threadId":"63033","inReplyTo":null,"subject":"[GSOC][PATCH 0/1] t1403: prefer test_path_exists helper function","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-01T10:58:37Z","receivedAt":"2025-03-01T10:59:16Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"test -e does not provide a nice error message when we hit test \nfailures, so use test_path_exists instead.\n\nMahendra Dani (1):\n  t1403: prefer test_path_exists helper function\n\n t/t1403-show-ref.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\n\nbase-commit: cb0ae672aeabefca9704477ea8018ac94f523970\n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"513329","messageId":"20250301105838.1481-2-danimahendra0904@gmail.com","threadId":"63033","inReplyTo":"20250301105838.1481-1-danimahendra0904@gmail.com","subject":"[PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-01T10:58:38Z","receivedAt":"2025-03-01T10:59:54Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"test -e does not provide a nice error message when\nwe hit test failures, so use test_path_exists instead.\n\nSigned-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n---\n t/t1403-show-ref.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex 9d698b3cc3..12f7b60024 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' '\n \n \tremove_object() {\n \t\tfile=$(sha1_file \"$*\") &&\n-\t\ttest -e \"$file\" &&\n+\t\ttest_path_exists \"$file\" &&\n \t\trm -f \"$file\"\n \t} &&\n \n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"513393","messageId":"Z8WD2MeHpOH7Ni8A@pks.im","threadId":"63033","inReplyTo":"20250301105838.1481-2-danimahendra0904@gmail.com","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-03T10:26:32Z","receivedAt":"2025-03-03T10:26:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Mar 01, 2025 at 04:28:38PM +0530, Mahendra Dani wrote:\n> test -e does not provide a nice error message when\n> we hit test failures, so use test_path_exists instead.\n> \n> Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n> ---\n>  t/t1403-show-ref.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> index 9d698b3cc3..12f7b60024 100755\n> --- a/t/t1403-show-ref.sh\n> +++ b/t/t1403-show-ref.sh\n> @@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' '\n>  \n>  \tremove_object() {\n>  \t\tfile=$(sha1_file \"$*\") &&\n> -\t\ttest -e \"$file\" &&\n> +\t\ttest_path_exists \"$file\" &&\n>  \t\trm -f \"$file\"\n>  \t} &&\n\nThe refactoring is true to the original spirit of the preimage indeed.\nBut we could also improve it even further if we verified that the path\nnot only exists, but exists and is a file via `test_path_is_file()`. If\nwe decide to do that we should also explain the change in the commit\nmessage.\n\nThanks!\n\nPatrick\n"},{"id":"513488","messageId":"CABGrwBB1-UiiPEOptN9csVP1WYg2X=SWeMxxiyDfz02VmDp1vA@mail.gmail.com","threadId":"63033","inReplyTo":"Z8WD2MeHpOH7Ni8A@pks.im","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T02:27:25Z","receivedAt":"2025-03-04T02:27:39Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"On Mon, Mar 3, 2025 at 3:56 PM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Sat, Mar 01, 2025 at 04:28:38PM +0530, Mahendra Dani wrote:\n> > test -e does not provide a nice error message when\n> > we hit test failures, so use test_path_exists instead.\n> >\n> > Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n> > ---\n> >  t/t1403-show-ref.sh | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> > index 9d698b3cc3..12f7b60024 100755\n> > --- a/t/t1403-show-ref.sh\n> > +++ b/t/t1403-show-ref.sh\n> > @@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' '\n> >\n> >       remove_object() {\n> >               file=$(sha1_file \"$*\") &&\n> > -             test -e \"$file\" &&\n> > +             test_path_exists \"$file\" &&\n> >               rm -f \"$file\"\n> >       } &&\n>\n> The refactoring is true to the original spirit of the preimage indeed.\n> But we could also improve it even further if we verified that the path\n> not only exists, but exists and is a file via `test_path_is_file()`. If\n> we decide to do that we should also explain the change in the commit\n> message.\n>\n> Thanks!\n>\n> Patrick\n\nYes, sure.\nI will improve it further using the `test_path_is_file()` helper\nfunction and change the commit message in v2 patch.\n\nThanks,\nMahendra.\n"},{"id":"513509","messageId":"20250304091556.22478-1-danimahendra0904@gmail.com","threadId":"63033","inReplyTo":"20250301105838.1481-1-danimahendra0904@gmail.com","subject":"[GSOC][PATCH v2 0/1] t1403: verify path exists and is a file","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T09:15:56Z","receivedAt":"2025-03-04T09:16:06Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"test -e does not provide a nice error message when we hit test failures,\nso use test_path_exists() instead.\n\nFurther, verify that if the path exists, then the path is a file using\ntest_path_is_file() helper function.\n\nThanks,\nMahendra\n\nMahendra Dani (1):\n  t1403: verify that path exists and is a file\n\n t/t1403-show-ref.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\nRange-diff against v1:\n1:  42dd686abe = 1:  42dd686abe t1403: verify that path exists and is a file\n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"513510","messageId":"20250304092722.25757-1-danimahendra0904@gmail.com","threadId":"63033","inReplyTo":"20250301105838.1481-1-danimahendra0904@gmail.com","subject":"[PATCH v2 1/1] t1403: verify that path exists and is a file","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T09:27:22Z","receivedAt":"2025-03-04T09:27:31Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"test -e does not provide a nice error message when\nwe hit test failures, so use test_path_exists() instead\nand verify that if the path exists then it is a file using test_path_is_file().\n\nSigned-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n---\n t/t1403-show-ref.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex 9d698b3cc3..4afde01a29 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '\n \n \tremove_object() {\n \t\tfile=$(sha1_file \"$*\") &&\n-\t\ttest -e \"$file\" &&\n+\t\ttest_path_exists \"$file\" &&\n+\t\ttest_path_is_file \"$file\" &&\n \t\trm -f \"$file\"\n \t} &&\n \n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"513512","messageId":"20250304094153.28959-1-danimahendra0904@gmail.com","threadId":"63033","inReplyTo":"20250304091556.22478-1-danimahendra0904@gmail.com","subject":"[GSOC][PATCH v3 0/1] t1403: verify path exists and is a file","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T09:41:52Z","receivedAt":"2025-03-04T09:42:44Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"test -e does not provide a nice error message when we hit test failures,\nso use test_path_exists() instead.\n\nFurther, verify that if the path exists, then the path is a file using\ntest_path_is_file() helper function.\n\nThis patch does not change any code in v2, but is rather submitted with proper formatting\nwhich was lacking in v2. \nI apologize for the incorrect patch submission in v2. \n\nThanks,\nMahendra\n\nMahendra Dani (1):\n  t1403: verify that path exists and is a file\n\n t/t1403-show-ref.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\nRange-diff against v2:\n1:  42dd686abe = 1:  42dd686abe t1403: verify that path exists and is a file\n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"513513","messageId":"20250304094153.28959-2-danimahendra0904@gmail.com","threadId":"63033","inReplyTo":"20250304094153.28959-1-danimahendra0904@gmail.com","subject":"[PATCH v3 1/1] t1403: verify that path exists and is a file","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T09:41:53Z","receivedAt":"2025-03-04T09:44:36Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"test -e does not provide a nice error message when\nwe hit test failures, so use test_path_exists() instead\nand verify that if the path exists then it is a file using test_path_is_file().\n\nSigned-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n---\n t/t1403-show-ref.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex 9d698b3cc3..4afde01a29 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '\n \n \tremove_object() {\n \t\tfile=$(sha1_file \"$*\") &&\n-\t\ttest -e \"$file\" &&\n+\t\ttest_path_exists \"$file\" &&\n+\t\ttest_path_is_file \"$file\" &&\n \t\trm -f \"$file\"\n \t} &&\n \n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"513520","messageId":"Z8bd3iHrhXb4WH6A@pks.im","threadId":"63033","inReplyTo":"20250304092722.25757-1-danimahendra0904@gmail.com","subject":"Re: [PATCH v2 1/1] t1403: verify that path exists and is a file","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-04T11:02:54Z","receivedAt":"2025-03-04T11:03:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 04, 2025 at 02:57:22PM +0530, Mahendra Dani wrote:\n> test -e does not provide a nice error message when\n> we hit test failures, so use test_path_exists() instead\n> and verify that if the path exists then it is a file using test_path_is_file().\n> \n> Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n> ---\n>  t/t1403-show-ref.sh | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> index 9d698b3cc3..4afde01a29 100755\n> --- a/t/t1403-show-ref.sh\n> +++ b/t/t1403-show-ref.sh\n> @@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '\n>  \n>  \tremove_object() {\n>  \t\tfile=$(sha1_file \"$*\") &&\n> -\t\ttest -e \"$file\" &&\n> +\t\ttest_path_exists \"$file\" &&\n> +\t\ttest_path_is_file \"$file\" &&\n>  \t\trm -f \"$file\"\n>  \t} &&\n\nThere is no need to execute both functions. The underlying\nimplementation of these functions use `test -e` and `test -f`,\nrespectively. The former merely checks whether a path exists, whereas\nthe latter verifies that the path is a file. It follows that when the\npath is a file it also has to exist, so using `test -e` (or rather its\nwrapper function `test_path_exists`) is redundant.\n\nPatrick\n"},{"id":"513521","messageId":"CABGrwBC2120vR6=NuXHzQnsv+Bfy=W+4Nx8zjUHqLDq+4V-FPQ@mail.gmail.com","threadId":"63033","inReplyTo":"Z8bd3iHrhXb4WH6A@pks.im","subject":"Re: [PATCH v2 1/1] t1403: verify that path exists and is a file","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T11:15:47Z","receivedAt":"2025-03-04T11:16:00Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"On Tue, Mar 4, 2025 at 4:33 PM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Tue, Mar 04, 2025 at 02:57:22PM +0530, Mahendra Dani wrote:\n> > test -e does not provide a nice error message when\n> > we hit test failures, so use test_path_exists() instead\n> > and verify that if the path exists then it is a file using test_path_is_file().\n> >\n> > Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n> > ---\n> >  t/t1403-show-ref.sh | 3 ++-\n> >  1 file changed, 2 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> > index 9d698b3cc3..4afde01a29 100755\n> > --- a/t/t1403-show-ref.sh\n> > +++ b/t/t1403-show-ref.sh\n> > @@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '\n> >\n> >       remove_object() {\n> >               file=$(sha1_file \"$*\") &&\n> > -             test -e \"$file\" &&\n> > +             test_path_exists \"$file\" &&\n> > +             test_path_is_file \"$file\" &&\n> >               rm -f \"$file\"\n> >       } &&\n>\n> There is no need to execute both functions. The underlying\n> implementation of these functions use `test -e` and `test -f`,\n> respectively. The former merely checks whether a path exists, whereas\n> the latter verifies that the path is a file. It follows that when the\n> path is a file it also has to exist, so using `test -e` (or rather its\n> wrapper function `test_path_exists`) is redundant.\n>\n> Patrick\n\nSure I will remove that test_path_exists() check in the patch v4.\n"},{"id":"513522","messageId":"20250304112323.40139-1-danimahendra0904@gmail.com","threadId":"63033","inReplyTo":"20250304094153.28959-1-danimahendra0904@gmail.com","subject":"[PATCH v4 0/1] t1403: verify path exists and is a file","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T11:23:22Z","receivedAt":"2025-03-04T11:23:35Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"Verify that if the path exists, then the path is a file using\ntest_path_is_file() helper function.\n\nMahendra Dani (1):\n  t1403: verify that path exists and is a file\n\n t/t1403-show-ref.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\nRange-diff against v3:\n1:  42dd686abe ! 1:  d181f98d1a t1403: verify that path exists and is a file\n    @@ Metadata\n      ## Commit message ##\n         t1403: verify that path exists and is a file\n     \n    -    test -e does not provide a nice error message when\n    -    we hit test failures, so use test_path_exists() instead\n    -    and verify that if the path exists then it is a file using test_path_is_file().\n    +    Verify that if the path exists then it is a file using test_path_is_file().\n     \n         Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n     \n    @@ t/t1403-show-ref.sh: test_expect_success 'show-ref --verify with dangling ref' '\n      \tremove_object() {\n      \t\tfile=$(sha1_file \"$*\") &&\n     -\t\ttest -e \"$file\" &&\n    -+\t\ttest_path_exists \"$file\" &&\n     +\t\ttest_path_is_file \"$file\" &&\n      \t\trm -f \"$file\"\n      \t} &&\n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"513523","messageId":"20250304112728.41228-1-danimahendra0904@gmail.com","threadId":"63033","inReplyTo":"20250304094153.28959-1-danimahendra0904@gmail.com","subject":"[PATCH v4 0/1] t1403: verify path exists and is a file","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T11:27:27Z","receivedAt":"2025-03-04T11:27:39Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"Verify that if the path exists, then the path is a file using\ntest_path_is_file() helper function.\n\nThanks,\nMahendra\n\nMahendra Dani (1):\n  t1403: verify that path exists and is a file\n\n t/t1403-show-ref.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\nRange-diff against v3:\n1:  42dd686abe ! 1:  d181f98d1a t1403: verify that path exists and is a file\n    @@ Metadata\n      ## Commit message ##\n         t1403: verify that path exists and is a file\n     \n    -    test -e does not provide a nice error message when\n    -    we hit test failures, so use test_path_exists() instead\n    -    and verify that if the path exists then it is a file using test_path_is_file().\n    +    Verify that if the path exists then it is a file using test_path_is_file().\n     \n         Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n     \n    @@ t/t1403-show-ref.sh: test_expect_success 'show-ref --verify with dangling ref' '\n      \tremove_object() {\n      \t\tfile=$(sha1_file \"$*\") &&\n     -\t\ttest -e \"$file\" &&\n    -+\t\ttest_path_exists \"$file\" &&\n     +\t\ttest_path_is_file \"$file\" &&\n      \t\trm -f \"$file\"\n      \t} &&\n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"513524","messageId":"20250304112728.41228-2-danimahendra0904@gmail.com","threadId":"63033","inReplyTo":"20250304112728.41228-1-danimahendra0904@gmail.com","subject":"[PATCH v4 1/1] t1403: verify that path exists and is a file","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T11:27:28Z","receivedAt":"2025-03-04T11:28:45Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"Verify that if the path exists then it is a file using test_path_is_file().\n\nSigned-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n---\n t/t1403-show-ref.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex 9d698b3cc3..9da3650e91 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' '\n \n \tremove_object() {\n \t\tfile=$(sha1_file \"$*\") &&\n-\t\ttest -e \"$file\" &&\n+\t\ttest_path_is_file \"$file\" &&\n \t\trm -f \"$file\"\n \t} &&\n \n-- \n2.39.2 (Apple Git-143)\n\n"},{"id":"513530","messageId":"xmqqldtlt304.fsf@gitster.g","threadId":"63033","inReplyTo":"CABGrwBB1-UiiPEOptN9csVP1WYg2X=SWeMxxiyDfz02VmDp1vA@mail.gmail.com","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T12:05:31Z","receivedAt":"2025-03-04T12:05:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mahendra Dani <danimahendra0904@gmail.com> writes:\n\n>> >       remove_object() {\n>> >               file=$(sha1_file \"$*\") &&\n>> > -             test -e \"$file\" &&\n>> > +             test_path_exists \"$file\" &&\n>> >               rm -f \"$file\"\n>> >       } &&\n>>\n>> The refactoring is true to the original spirit of the preimage indeed.\n>> But we could also improve it even further if we verified that the path\n>> not only exists, but exists and is a file via `test_path_is_file()`. If\n>> we decide to do that we should also explain the change in the commit\n>> message.\n>\n> Yes, sure.\n> I will improve it further using the `test_path_is_file()` helper\n> function and change the commit message in v2 patch.\n\nYou may want to think about why there is \"-f\" there.  If we remove\nit, do we still need to have any check there?\n"},{"id":"513537","messageId":"xmqqv7soss4b.fsf@gitster.g","threadId":"63033","inReplyTo":"20250304094153.28959-1-danimahendra0904@gmail.com","subject":"Re: [GSOC][PATCH v3 0/1] t1403: verify path exists and is a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T16:00:36Z","receivedAt":"2025-03-04T16:00:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mahendra Dani <danimahendra0904@gmail.com> writes:\n\n> test -e does not provide a nice error message when we hit test failures,\n> so use test_path_exists() instead.\n>\n> Further, verify that if the path exists, then the path is a file using\n> test_path_is_file() helper function.\n>\n> This patch does not change any code in v2, but is rather submitted with proper formatting\n> which was lacking in v2. \n> I apologize for the incorrect patch submission in v2. \n\nMy cursory look didn't spot anything iffy in v2 and I found it\nnicely described.  I do not see anything, other than the above\n3-line paragraph, that is different in v3 from v2, which is a bit\ncurious.\n\nThanks.\n"},{"id":"513542","messageId":"CABGrwBAaN3U8WCtPgd+UjVjsoBniCawWV3U+WdKOzBdG5LxmAg@mail.gmail.com","threadId":"63033","inReplyTo":"xmqqldtlt304.fsf@gitster.g","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T17:24:00Z","receivedAt":"2025-03-04T17:24:14Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"On Tue, Mar 4, 2025 at 5:35 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Mahendra Dani <danimahendra0904@gmail.com> writes:\n>\n> >> >       remove_object() {\n> >> >               file=$(sha1_file \"$*\") &&\n> >> > -             test -e \"$file\" &&\n> >> > +             test_path_exists \"$file\" &&\n> >> >               rm -f \"$file\"\n> >> >       } &&\n> >>\n> >> The refactoring is true to the original spirit of the preimage indeed.\n> >> But we could also improve it even further if we verified that the path\n> >> not only exists, but exists and is a file via `test_path_is_file()`. If\n> >> we decide to do that we should also explain the change in the commit\n> >> message.\n> >\n> > Yes, sure.\n> > I will improve it further using the `test_path_is_file()` helper\n> > function and change the commit message in v2 patch.\n>\n> You may want to think about why there is \"-f\" there.  If we remove\n> it, do we still need to have any check there?\n\nHere, the \"-f\" flag in `rm -f \"$file\"` does not produce an error message even\nif the file does not exist [1], thus the `test -e \"$file\"` check was redundant,\nas pointed out by Patrick in [2].\n\nHowever, switching to `test_path_is_file()` would provide additional safety by\nensuring that we only attempt to remove a regular file and not a directory.\n\n[References]\n1. https://man7.org/linux/man-pages/man1/rm.1.html\n2. https://lore.kernel.org/git/Z8bd3iHrhXb4WH6A@pks.im/\n"},{"id":"513543","messageId":"xmqqwmd4r9jx.fsf@gitster.g","threadId":"63033","inReplyTo":"CABGrwBAaN3U8WCtPgd+UjVjsoBniCawWV3U+WdKOzBdG5LxmAg@mail.gmail.com","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T17:26:58Z","receivedAt":"2025-03-04T17:27:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mahendra Dani <danimahendra0904@gmail.com> writes:\n\n> On Tue, Mar 4, 2025 at 5:35 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Mahendra Dani <danimahendra0904@gmail.com> writes:\n>>\n>> >> >       remove_object() {\n>> >> >               file=$(sha1_file \"$*\") &&\n>> >> > -             test -e \"$file\" &&\n>> >> > +             test_path_exists \"$file\" &&\n>> >> >               rm -f \"$file\"\n>> >> >       } &&\n>>\n>> You may want to think about why there is \"-f\" there.  If we remove\n>> it, do we still need to have any check there?\n>\n> Here, the \"-f\" flag in `rm -f \"$file\"` does not produce an error message even\n> if the file does not exist [1], thus the `test -e \"$file\"` check was redundant,\n> as pointed out by Patrick in [2].\n\nSo what happens if you dropped \"-f\" as I hinted?  We'll notice the\nlack of file and the command exits with non-zero status.  So \"test -e\"\nwas not necessary in the first place, was it?\n\n"},{"id":"513544","messageId":"CABGrwBBaHzM9tE6vWkuMSZuUbHVWwCZVTgkvMb04C=NYyGCLAw@mail.gmail.com","threadId":"63033","inReplyTo":"xmqqwmd4r9jx.fsf@gitster.g","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T17:35:29Z","receivedAt":"2025-03-04T17:35:44Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"On Tue, Mar 4, 2025 at 10:57 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Mahendra Dani <danimahendra0904@gmail.com> writes:\n>\n> > On Tue, Mar 4, 2025 at 5:35 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >>\n> >> Mahendra Dani <danimahendra0904@gmail.com> writes:\n> >>\n> >> >> >       remove_object() {\n> >> >> >               file=$(sha1_file \"$*\") &&\n> >> >> > -             test -e \"$file\" &&\n> >> >> > +             test_path_exists \"$file\" &&\n> >> >> >               rm -f \"$file\"\n> >> >> >       } &&\n> >>\n> >> You may want to think about why there is \"-f\" there.  If we remove\n> >> it, do we still need to have any check there?\n> >\n> > Here, the \"-f\" flag in `rm -f \"$file\"` does not produce an error message even\n> > if the file does not exist [1], thus the `test -e \"$file\"` check was redundant,\n> > as pointed out by Patrick in [2].\n>\n> So what happens if you dropped \"-f\" as I hinted?  We'll notice the\n> lack of file and the command exits with non-zero status.  So \"test -e\"\n> was not necessary in the first place, was it?\n>\n\nYes, due to the use of the \"-f\" flag, it's not necessary to explicitly\ncheck the lack of file using `test -e`.\nBut if we drop the \"-f\" flag, we will have to check the lack of file\nusing `test -e` or\n`test_path_is_file()`.\n\nThanks,\nMahendra\n"},{"id":"513545","messageId":"CAPig+cQ275qBWzaYmMXydiA_8+CTx3FhAAY+o8ro+hD03wzu0g@mail.gmail.com","threadId":"63033","inReplyTo":"xmqqldtlt304.fsf@gitster.g","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-03-04T17:35:43Z","receivedAt":"2025-03-04T17:35:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Mahendra Dani <danimahendra0904@gmail.com> writes:\n> >> >       remove_object() {\n> >> >               file=$(sha1_file \"$*\") &&\n> >> > -             test -e \"$file\" &&\n> >> > +             test_path_exists \"$file\" &&\n> >> >               rm -f \"$file\"\n> >> >       } &&\n> >>\n> >> The refactoring is true to the original spirit of the preimage indeed.\n> >> But we could also improve it even further if we verified that the path\n> >> not only exists, but exists and is a file via `test_path_is_file()`. If\n> >> we decide to do that we should also explain the change in the commit\n> >> message.\n> >\n> > I will improve it further using the `test_path_is_file()` helper\n> > function and change the commit message in v2 patch.\n>\n> You may want to think about why there is \"-f\" there.  If we remove\n> it, do we still need to have any check there?\n\nThat's a good question to ask, but isn't the implied suggestion of\ndropping \"-f\" going in the wrong direction? If I'm reading\nremove_object() correctly, `test -e` is being used as control flow,\n*not* as an assertion that the file exists. That is, the expectation\nof the caller is that the file will not exist once the call completes\nand that remove_object() will return a success code whether the file\nwas present before the call or not. By control flow, I mean that the\nfunction, as written, is the same as this more explicit version:\n\n    remove_object() {\n        file=$(sha1_file \"$*\") &&\n        if test -e \"$file\"\n        then\n            rm -f \"$file\"\n        fi\n    } &&\n\nGiven this understanding, then it becomes apparent that this GSoC\nmicroproject shouldn't be applying *any* test_path_foo() to this\nfunction. As an alternative, given that `rm -f` returns a success code\nwhether or not the file exists, the microproject could instead\n*remove* the `test -e \"$file\" &&` line entirely (and the commit\nmessage should explain why doing so is a reasonable thing to do).\n"},{"id":"513546","messageId":"xmqqsensr8r9.fsf@gitster.g","threadId":"63033","inReplyTo":"CABGrwBBaHzM9tE6vWkuMSZuUbHVWwCZVTgkvMb04C=NYyGCLAw@mail.gmail.com","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T17:44:10Z","receivedAt":"2025-03-04T17:44:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mahendra Dani <danimahendra0904@gmail.com> writes:\n\n> Yes, due to the use of the \"-f\" flag, it's not necessary to explicitly\n> check the lack of file using `test -e`.\n> But if we drop the \"-f\" flag, we will have to check the lack of file\n> using `test -e` or\n> `test_path_is_file()`.\n\nIsn't it the other way around?\n\n    $ rm -f nosuch ; echo $?\n    0\n    $ rm nosuch ; echo $?\n    rm: cannot remove 'nosuch': No such file or directory\n    1\n\n"},{"id":"513549","messageId":"xmqqcyewr8j7.fsf@gitster.g","threadId":"63033","inReplyTo":"CAPig+cQ275qBWzaYmMXydiA_8+CTx3FhAAY+o8ro+hD03wzu0g@mail.gmail.com","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T17:49:00Z","receivedAt":"2025-03-04T17:49:02Z","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> On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> Mahendra Dani <danimahendra0904@gmail.com> writes:\n>> >> >       remove_object() {\n>> >> >               file=$(sha1_file \"$*\") &&\n>> >> > -             test -e \"$file\" &&\n>> >> > +             test_path_exists \"$file\" &&\n>> >> >               rm -f \"$file\"\n>> >> >       } &&\n> That's a good question to ask, but isn't the implied suggestion of\n> dropping \"-f\" going in the wrong direction? If I'm reading\n> remove_object() correctly, `test -e` is being used as control flow,\n> *not* as an assertion that the file exists.\n\nIf sha1_file says the loose object must be at path $file, and the\ncall to test -e \"$file\" returns false, two things happen in this\nfunction:\n\n (1) control stops and \"rm -f\" does not trigger\n (2) the function returns non-zero status to the caller\n\nIf you omit the check and say rm \"$file\" instead, under the same\nscenario, (1) \"rm\" is attempted, but there is nothing to remove so\nthe command returns non-zero status, and (2) the function returns\nthat non-zero status to the caller\n\nIf the file does exist, both will remove the file, and give the\ncaller zero status return.\n\nSo?\n"},{"id":"513551","messageId":"CABGrwBBbtiWtMnwJV3Pefo8VDH2LYWsH2QKG-TG_AEdZq5585A@mail.gmail.com","threadId":"63033","inReplyTo":"xmqqsensr8r9.fsf@gitster.g","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T17:49:41Z","receivedAt":"2025-03-04T17:49:54Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"On Tue, Mar 4, 2025 at 11:14 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Mahendra Dani <danimahendra0904@gmail.com> writes:\n>\n> > Yes, due to the use of the \"-f\" flag, it's not necessary to explicitly\n> > check the lack of file using `test -e`.\n> > But if we drop the \"-f\" flag, we will have to check the lack of file\n> > using `test -e` or\n> > `test_path_is_file()`.\n>\n> Isn't it the other way around?\n>\n>     $ rm -f nosuch ; echo $?\n>     0\n>     $ rm nosuch ; echo $?\n>     rm: cannot remove 'nosuch': No such file or directory\n>     1\n>\n\nYes, you are right.\nWith the \"-f\" flag, `rm` returns exit code 0 irrespective of whether\nthe file is present or not.\nThus, the `test -e` check is _required_ if we drop the \"-f\" flag to\nreturn the correct exit code.\n\nI apologize for the mistake.\n\nThanks,\nMahendra\n"},{"id":"513553","messageId":"xmqqv7sopt7o.fsf@gitster.g","threadId":"63033","inReplyTo":"20250304094153.28959-2-danimahendra0904@gmail.com","subject":"Re: [PATCH v3 1/1] t1403: verify that path exists and is a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T18:05:15Z","receivedAt":"2025-03-04T18:05:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mahendra Dani <danimahendra0904@gmail.com> writes:\n\n> test -e does not provide a nice error message when\n> we hit test failures, so use test_path_exists() instead\n> and verify that if the path exists then it is a file using test_path_is_file().\n>\n> Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n> ---\n>  t/t1403-show-ref.sh | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> index 9d698b3cc3..4afde01a29 100755\n> --- a/t/t1403-show-ref.sh\n> +++ b/t/t1403-show-ref.sh\n> @@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '\n>  \n>  \tremove_object() {\n>  \t\tfile=$(sha1_file \"$*\") &&\n> -\t\ttest -e \"$file\" &&\n> +\t\ttest_path_exists \"$file\" &&\n> +\t\ttest_path_is_file \"$file\" &&\n>  \t\trm -f \"$file\"\n>  \t} &&\n\nMakes sense.  Will queue.\n"},{"id":"513554","messageId":"xmqqo6ygpt5i.fsf@gitster.g","threadId":"63033","inReplyTo":"xmqqv7sopt7o.fsf@gitster.g","subject":"Re: [PATCH v3 1/1] t1403: verify that path exists and is a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T18:06:33Z","receivedAt":"2025-03-04T18:06:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Mahendra Dani <danimahendra0904@gmail.com> writes:\n>\n>> test -e does not provide a nice error message when\n>> we hit test failures, so use test_path_exists() instead\n>> and verify that if the path exists then it is a file using test_path_is_file().\n>>\n>> Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n>> ---\n>>  t/t1403-show-ref.sh | 3 ++-\n>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n>> index 9d698b3cc3..4afde01a29 100755\n>> --- a/t/t1403-show-ref.sh\n>> +++ b/t/t1403-show-ref.sh\n>> @@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '\n>>  \n>>  \tremove_object() {\n>>  \t\tfile=$(sha1_file \"$*\") &&\n>> -\t\ttest -e \"$file\" &&\n>> +\t\ttest_path_exists \"$file\" &&\n>> +\t\ttest_path_is_file \"$file\" &&\n>>  \t\trm -f \"$file\"\n>>  \t} &&\n>\n> Makes sense.  Will queue.\n\nNo, no, no.  test_is_file alone is sufficient---if the thing does\nnot exist, it would not be a file anyway ;-)\n\n"},{"id":"513555","messageId":"CAPig+cS3QZhZ=W2WfM3T3EngaMOSV37KH4Pqp78QHzOAODtenA@mail.gmail.com","threadId":"63033","inReplyTo":"xmqqcyewr8j7.fsf@gitster.g","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-03-04T18:07:22Z","receivedAt":"2025-03-04T18:07:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 4, 2025 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >> Mahendra Dani <danimahendra0904@gmail.com> writes:\n> >> >> >       remove_object() {\n> >> >> >               file=$(sha1_file \"$*\") &&\n> >> >> > -             test -e \"$file\" &&\n> >> >> > +             test_path_exists \"$file\" &&\n> >> >> >               rm -f \"$file\"\n> >> >> >       } &&\n> > That's a good question to ask, but isn't the implied suggestion of\n> > dropping \"-f\" going in the wrong direction? If I'm reading\n> > remove_object() correctly, `test -e` is being used as control flow,\n> > *not* as an assertion that the file exists.\n>\n> If sha1_file says the loose object must be at path $file, and the\n> call to test -e \"$file\" returns false, two things happen in this\n> function:\n>\n>  (1) control stops and \"rm -f\" does not trigger\n>  (2) the function returns non-zero status to the caller\n\nTrue enough. I was misreading `test -e \"$file\"` as _only_ control flow.\n\nHowever, it's still not clear to me why this function is making the\n`test -e \"$file\"` assertion in the first place or why the enclosing\ntest should care, especially since that assertion is only checking\nthat `git commit` worked correctly, but that's not the intent of this\nparticular test[1]. So, `test -e \"$file\"` seems pointless or at least\nmisleading.\n\n> If you omit the check and say rm \"$file\" instead, under the same\n> scenario, (1) \"rm\" is attempted, but there is nothing to remove so\n> the command returns non-zero status, and (2) the function returns\n> that non-zero status to the caller\n\nYes, I understood the implication of your suggestion, but as mentioned\nabove, it's not clear (at least to me) why `test -e \"$file\"` is there\nat all since this test is not about checking functionality of `git\ncommit`.\n\n[1]: d01b8203ec (show-ref: detect dangling refs under --verify as\nwell, 2017-01-23) doesn't explain why `test -e \"$file\"` was used.\n"},{"id":"513556","messageId":"xmqqjz94pt3q.fsf@gitster.g","threadId":"63033","inReplyTo":"CABGrwBBbtiWtMnwJV3Pefo8VDH2LYWsH2QKG-TG_AEdZq5585A@mail.gmail.com","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T18:07:37Z","receivedAt":"2025-03-04T18:07:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mahendra Dani <danimahendra0904@gmail.com> writes:\n\n> On Tue, Mar 4, 2025 at 11:14 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Mahendra Dani <danimahendra0904@gmail.com> writes:\n>>\n>> > Yes, due to the use of the \"-f\" flag, it's not necessary to explicitly\n>> > check the lack of file using `test -e`.\n>> > But if we drop the \"-f\" flag, we will have to check the lack of file\n>> > using `test -e` or\n>> > `test_path_is_file()`.\n>>\n>> Isn't it the other way around?\n>>\n>>     $ rm -f nosuch ; echo $?\n>>     0\n>>     $ rm nosuch ; echo $?\n>>     rm: cannot remove 'nosuch': No such file or directory\n>>     1\n>>\n>\n> Yes, you are right.\n> With the \"-f\" flag, `rm` returns exit code 0 irrespective of whether\n> the file is present or not.\n> Thus, the `test -e` check is _required_ if we drop the \"-f\" flag to\n> return the correct exit code.\n>\n> I apologize for the mistake.\n\nNo need for an apology when correcting technical mistakes, which we\nall make.\n\n"},{"id":"513557","messageId":"CABGrwBBcg9DnPHtDG=OjQWJKWJy1mD26xXm7r5W80GNBgqFa-g@mail.gmail.com","threadId":"63033","inReplyTo":"xmqqo6ygpt5i.fsf@gitster.g","subject":"Re: [PATCH v3 1/1] t1403: verify that path exists and is a file","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T18:13:23Z","receivedAt":"2025-03-04T18:13:37Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"On Tue, Mar 4, 2025 at 11:36 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > Mahendra Dani <danimahendra0904@gmail.com> writes:\n> >\n> >> test -e does not provide a nice error message when\n> >> we hit test failures, so use test_path_exists() instead\n> >> and verify that if the path exists then it is a file using test_path_is_file().\n> >>\n> >> Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n> >> ---\n> >>  t/t1403-show-ref.sh | 3 ++-\n> >>  1 file changed, 2 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> >> index 9d698b3cc3..4afde01a29 100755\n> >> --- a/t/t1403-show-ref.sh\n> >> +++ b/t/t1403-show-ref.sh\n> >> @@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '\n> >>\n> >>      remove_object() {\n> >>              file=$(sha1_file \"$*\") &&\n> >> -            test -e \"$file\" &&\n> >> +            test_path_exists \"$file\" &&\n> >> +            test_path_is_file \"$file\" &&\n> >>              rm -f \"$file\"\n> >>      } &&\n> >\n> > Makes sense.  Will queue.\n>\n> No, no, no.  test_is_file alone is sufficient---if the thing does\n> not exist, it would not be a file anyway ;-)\n>\n\nYes, it was pointed out by Patrick in [1], that `test_path_is_file()`\nalone is sufficient. Hence I removed the `test_path_exists()` check in\npatch v4 [2].\n\n[References]\n1. https://lore.kernel.org/git/Z8bd3iHrhXb4WH6A@pks.im/\n2. https://lore.kernel.org/git/20250304112728.41228-2-danimahendra0904@gmail.com/\n"},{"id":"513558","messageId":"xmqq5xkopsti.fsf@gitster.g","threadId":"63033","inReplyTo":"20250304112728.41228-2-danimahendra0904@gmail.com","subject":"Re: [PATCH v4 1/1] t1403: verify that path exists and is a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T18:13:45Z","receivedAt":"2025-03-04T18:13:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mahendra Dani <danimahendra0904@gmail.com> writes:\n\n> Verify that if the path exists then it is a file using test_path_is_file().\n>\n> Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n> ---\n>  t/t1403-show-ref.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> index 9d698b3cc3..9da3650e91 100755\n> --- a/t/t1403-show-ref.sh\n> +++ b/t/t1403-show-ref.sh\n> @@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' '\n>  \n>  \tremove_object() {\n>  \t\tfile=$(sha1_file \"$*\") &&\n> -\t\ttest -e \"$file\" &&\n> +\t\ttest_path_is_file \"$file\" &&\n>  \t\trm -f \"$file\"\n>  \t} &&\n\nYup, this makes perfect sense.  I would have explained it a bit\ndifferently, perhaps like\n\n    The original uses 'test -e' to ensure that the file exists, but\n    (1) it fails silently if the expectation is not met, and (2) we\n    expect the loose object file not just to exist but to be a file\n    (in other words, the original should have been 'test -f' in the\n    first place).\n\n    Use test_path_is_file to improve on both points.\n\nor something, but the proposed commit log message is sufficiently\nreadable.\n\nWill queue.\n\nThanks.\n"},{"id":"513560","messageId":"CABGrwBAhjSkodUe-SGxJsZLRE2zgfqr+3W8U-Op+jG5iKBK80g@mail.gmail.com","threadId":"63033","inReplyTo":"xmqq5xkopsti.fsf@gitster.g","subject":"Re: [PATCH v4 1/1] t1403: verify that path exists and is a file","fromName":"Mahendra Dani","fromEmail":"danimahendra0904@gmail.com","sentAt":"2025-03-04T18:19:40Z","receivedAt":"2025-03-04T18:19:54Z","isPatch":true,"sender":{"key":"danimahendra0904@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116940083?v=4"},"body":"On Tue, Mar 4, 2025 at 11:43 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Mahendra Dani <danimahendra0904@gmail.com> writes:\n>\n> > Verify that if the path exists then it is a file using test_path_is_file().\n> >\n> > Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>\n> > ---\n> >  t/t1403-show-ref.sh | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> > index 9d698b3cc3..9da3650e91 100755\n> > --- a/t/t1403-show-ref.sh\n> > +++ b/t/t1403-show-ref.sh\n> > @@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' '\n> >\n> >       remove_object() {\n> >               file=$(sha1_file \"$*\") &&\n> > -             test -e \"$file\" &&\n> > +             test_path_is_file \"$file\" &&\n> >               rm -f \"$file\"\n> >       } &&\n>\n> Yup, this makes perfect sense.  I would have explained it a bit\n> differently, perhaps like\n>\n>     The original uses 'test -e' to ensure that the file exists, but\n>     (1) it fails silently if the expectation is not met, and (2) we\n>     expect the loose object file not just to exist but to be a file\n>     (in other words, the original should have been 'test -f' in the\n>     first place).\n>\n>     Use test_path_is_file to improve on both points.\n>\n> or something, but the proposed commit log message is sufficiently\n> readable.\n\nI will take more care while writing commit messages and cover letter\nfrom now onwards.\n>\n> Will queue.\n\nThanks.\n\n>\n> Thanks.\n\nThanks,\nMahendra\n"},{"id":"513561","messageId":"CAPig+cTUMj-QviOuXtdAN8Ao=gbfhXK=J6oEXfdjn1cAocZ3RA@mail.gmail.com","threadId":"63033","inReplyTo":"CAPig+cS3QZhZ=W2WfM3T3EngaMOSV37KH4Pqp78QHzOAODtenA@mail.gmail.com","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-03-04T18:28:42Z","receivedAt":"2025-03-04T18:28:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 4, 2025 at 1:07 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > >> >> >       remove_object() {\n> > >> >> >               file=$(sha1_file \"$*\") &&\n> > >> >> > -             test -e \"$file\" &&\n> > >> >> > +             test_path_exists \"$file\" &&\n> > >> >> >               rm -f \"$file\"\n> > >> >> >       } &&\n>\n> However, it's still not clear to me why this function is making the\n> `test -e \"$file\"` assertion in the first place or why the enclosing\n> test should care, especially since that assertion is only checking\n> that `git commit` worked correctly, but that's not the intent of this\n> particular test[1]. So, `test -e \"$file\"` seems pointless or at least\n> misleading.\n\nPerhaps, I'm falling into the trap of assuming that a lone `git\ncommit` in a new repository will unconditionally create a loose\nobject, and that that will always be the case? If, down the road, `git\ncommit` no longer works that way, then the assumption about the loose\nobject becomes invalid, in which case I can see how the `test -e\n\"$file\"` assertion is protecting the test against that future.\n"},{"id":"513562","messageId":"xmqq1pvcps2f.fsf@gitster.g","threadId":"63033","inReplyTo":"CAPig+cS3QZhZ=W2WfM3T3EngaMOSV37KH4Pqp78QHzOAODtenA@mail.gmail.com","subject":"Re: [PATCH 1/1] t1403: prefer test_path_exists helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T18:30:00Z","receivedAt":"2025-03-04T18:30:03Z","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> On Tue, Mar 4, 2025 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> Eric Sunshine <sunshine@sunshineco.com> writes:\n>> > On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> >> Mahendra Dani <danimahendra0904@gmail.com> writes:\n>> >> >> >       remove_object() {\n>> >> >> >               file=$(sha1_file \"$*\") &&\n>> >> >> > -             test -e \"$file\" &&\n>> >> >> > +             test_path_exists \"$file\" &&\n>> >> >> >               rm -f \"$file\"\n>> >> >> >       } &&\n> ...\n> Yes, I understood the implication of your suggestion, but as mentioned\n> above, it's not clear (at least to me) why `test -e \"$file\"` is there\n> at all since this test is not about checking functionality of `git\n> commit`.\n\nYup, I do not see much point in \"test -e\" there in the original, and\nit does not change even if it were \"test -f\".\n\nI would understand if the author wanted to have a \"slightly more\nintelligent 'rm -f' that knows where a loose object is located, and\nremoves the named object no matter what\", but if the objective were\nto ensure the object is missing, I wouldn't have written it to\nreturn non-zero when the object were missing in the first place.\n\nAnd if the purpose of the function is to catch unexpected cases,\nsuch as \"the loose object file should be there but isn't\" and \"we\nlocated the file but we failed to remove it\", then it shouldn't have\nthe 'test -e' guard and 'rm' shouldn't have been used with '-f'.\n\nSo, I agree with you that the original is already iffy.\n\nThanks.\n\n\n"}]}