| Message ID | 20250304112728.41228-2-danimahendra0904@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/1] t1403: verify that path exists and is a file | expand |
Mahendra Dani <danimahendra0904@gmail.com> writes: > Verify that if the path exists then it is a file using test_path_is_file(). > > Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com> > --- > t/t1403-show-ref.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh > index 9d698b3cc3..9da3650e91 100755 > --- a/t/t1403-show-ref.sh > +++ b/t/t1403-show-ref.sh > @@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' ' > > remove_object() { > file=$(sha1_file "$*") && > - test -e "$file" && > + test_path_is_file "$file" && > rm -f "$file" > } && Yup, this makes perfect sense. I would have explained it a bit differently, perhaps like The original uses 'test -e' to ensure that the file exists, but (1) it fails silently if the expectation is not met, and (2) we expect the loose object file not just to exist but to be a file (in other words, the original should have been 'test -f' in the first place). Use test_path_is_file to improve on both points. or something, but the proposed commit log message is sufficiently readable. Will queue. Thanks.
On Tue, Mar 4, 2025 at 11:43 PM Junio C Hamano <gitster@pobox.com> wrote: > > Mahendra Dani <danimahendra0904@gmail.com> writes: > > > Verify that if the path exists then it is a file using test_path_is_file(). > > > > Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com> > > --- > > t/t1403-show-ref.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh > > index 9d698b3cc3..9da3650e91 100755 > > --- a/t/t1403-show-ref.sh > > +++ b/t/t1403-show-ref.sh > > @@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' ' > > > > remove_object() { > > file=$(sha1_file "$*") && > > - test -e "$file" && > > + test_path_is_file "$file" && > > rm -f "$file" > > } && > > Yup, this makes perfect sense. I would have explained it a bit > differently, perhaps like > > The original uses 'test -e' to ensure that the file exists, but > (1) it fails silently if the expectation is not met, and (2) we > expect the loose object file not just to exist but to be a file > (in other words, the original should have been 'test -f' in the > first place). > > Use test_path_is_file to improve on both points. > > or something, but the proposed commit log message is sufficiently > readable. I will take more care while writing commit messages and cover letter from now onwards. > > Will queue. Thanks. > > Thanks. Thanks, Mahendra
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh index 9d698b3cc3..9da3650e91 100755 --- a/t/t1403-show-ref.sh +++ b/t/t1403-show-ref.sh @@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' ' remove_object() { file=$(sha1_file "$*") && - test -e "$file" && + test_path_is_file "$file" && rm -f "$file" } &&
Verify that if the path exists then it is a file using test_path_is_file(). Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com> --- t/t1403-show-ref.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)