[GSOC][PATCH 0/1] t1403: prefer test_path_exists helper function

[GSOC][PATCH 0/1] t1403: prefer test_path_exists helper function

[Mahendra Dani]

test -e does not provide a nice error message when we hit test 
failures, so use test_path_exists instead.

Mahendra Dani (1):
  t1403: prefer test_path_exists helper function

 t/t1403-show-ref.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: cb0ae672aeabefca9704477ea8018ac94f523970
-- 
2.39.2 (Apple Git-143)

[PATCH 1/1] t1403: prefer test_path_exists helper function

[Mahendra Dani]

test -e does not provide a nice error message when
we hit test failures, so use test_path_exists instead.

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..12f7b60024 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_exists "$file" &&
 		rm -f "$file"
 	} &&
 
-- 
2.39.2 (Apple Git-143)

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Patrick Steinhardt]

On Sat, Mar 01, 2025 at 04:28:38PM +0530, Mahendra Dani wrote:
> test -e does not provide a nice error message when
> we hit test failures, so use test_path_exists instead.
> 
> 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..12f7b60024 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_exists "$file" &&
>  		rm -f "$file"
>  	} &&

The refactoring is true to the original spirit of the preimage indeed.
But we could also improve it even further if we verified that the path
not only exists, but exists and is a file via `test_path_is_file()`. If
we decide to do that we should also explain the change in the commit
message.

Thanks!

Patrick

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Mahendra Dani]

On Mon, Mar 3, 2025 at 3:56 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Sat, Mar 01, 2025 at 04:28:38PM +0530, Mahendra Dani wrote:
> > test -e does not provide a nice error message when
> > we hit test failures, so use test_path_exists instead.
> >
> > 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..12f7b60024 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_exists "$file" &&
> >               rm -f "$file"
> >       } &&
>
> The refactoring is true to the original spirit of the preimage indeed.
> But we could also improve it even further if we verified that the path
> not only exists, but exists and is a file via `test_path_is_file()`. If
> we decide to do that we should also explain the change in the commit
> message.
>
> Thanks!
>
> Patrick

Yes, sure.
I will improve it further using the `test_path_is_file()` helper
function and change the commit message in v2 patch.

Thanks,
Mahendra.

[GSOC][PATCH v2 0/1] t1403: verify path exists and is a file

[Mahendra Dani]

test -e does not provide a nice error message when we hit test failures,
so use test_path_exists() instead.

Further, verify that if the path exists, then the path is a file using
test_path_is_file() helper function.

Thanks,
Mahendra

Mahendra Dani (1):
  t1403: verify that path exists and is a file

 t/t1403-show-ref.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Range-diff against v1:
1:  42dd686abe = 1:  42dd686abe t1403: verify that path exists and is a file
-- 
2.39.2 (Apple Git-143)

[PATCH v2 1/1] t1403: verify that path exists and is a file

[Mahendra Dani]

test -e does not provide a nice error message when
we hit test failures, so use test_path_exists() instead
and 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 9d698b3cc3..4afde01a29 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '
 
 	remove_object() {
 		file=$(sha1_file "$*") &&
-		test -e "$file" &&
+		test_path_exists "$file" &&
+		test_path_is_file "$file" &&
 		rm -f "$file"
 	} &&
 
-- 
2.39.2 (Apple Git-143)

[GSOC][PATCH v3 0/1] t1403: verify path exists and is a file

[Mahendra Dani]

test -e does not provide a nice error message when we hit test failures,
so use test_path_exists() instead.

Further, verify that if the path exists, then the path is a file using
test_path_is_file() helper function.

This patch does not change any code in v2, but is rather submitted with proper formatting
which was lacking in v2. 
I apologize for the incorrect patch submission in v2. 

Thanks,
Mahendra

Mahendra Dani (1):
  t1403: verify that path exists and is a file

 t/t1403-show-ref.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Range-diff against v2:
1:  42dd686abe = 1:  42dd686abe t1403: verify that path exists and is a file
-- 
2.39.2 (Apple Git-143)

[PATCH v3 1/1] t1403: verify that path exists and is a file

[Mahendra Dani]

test -e does not provide a nice error message when
we hit test failures, so use test_path_exists() instead
and 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 9d698b3cc3..4afde01a29 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '
 
 	remove_object() {
 		file=$(sha1_file "$*") &&
-		test -e "$file" &&
+		test_path_exists "$file" &&
+		test_path_is_file "$file" &&
 		rm -f "$file"
 	} &&
 
-- 
2.39.2 (Apple Git-143)

Re: [PATCH v2 1/1] t1403: verify that path exists and is a file

[Patrick Steinhardt]

On Tue, Mar 04, 2025 at 02:57:22PM +0530, Mahendra Dani wrote:
> test -e does not provide a nice error message when
> we hit test failures, so use test_path_exists() instead
> and 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
> index 9d698b3cc3..4afde01a29 100755
> --- a/t/t1403-show-ref.sh
> +++ b/t/t1403-show-ref.sh
> @@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '
>  
>  	remove_object() {
>  		file=$(sha1_file "$*") &&
> -		test -e "$file" &&
> +		test_path_exists "$file" &&
> +		test_path_is_file "$file" &&
>  		rm -f "$file"
>  	} &&

There is no need to execute both functions. The underlying
implementation of these functions use `test -e` and `test -f`,
respectively. The former merely checks whether a path exists, whereas
the latter verifies that the path is a file. It follows that when the
path is a file it also has to exist, so using `test -e` (or rather its
wrapper function `test_path_exists`) is redundant.

Patrick

Re: [PATCH v2 1/1] t1403: verify that path exists and is a file

[Mahendra Dani]

On Tue, Mar 4, 2025 at 4:33 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Mar 04, 2025 at 02:57:22PM +0530, Mahendra Dani wrote:
> > test -e does not provide a nice error message when
> > we hit test failures, so use test_path_exists() instead
> > and 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 | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
> > index 9d698b3cc3..4afde01a29 100755
> > --- a/t/t1403-show-ref.sh
> > +++ b/t/t1403-show-ref.sh
> > @@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '
> >
> >       remove_object() {
> >               file=$(sha1_file "$*") &&
> > -             test -e "$file" &&
> > +             test_path_exists "$file" &&
> > +             test_path_is_file "$file" &&
> >               rm -f "$file"
> >       } &&
>
> There is no need to execute both functions. The underlying
> implementation of these functions use `test -e` and `test -f`,
> respectively. The former merely checks whether a path exists, whereas
> the latter verifies that the path is a file. It follows that when the
> path is a file it also has to exist, so using `test -e` (or rather its
> wrapper function `test_path_exists`) is redundant.
>
> Patrick

Sure I will remove that test_path_exists() check in the patch v4.

[PATCH v4 0/1] t1403: verify path exists and is a file

[Mahendra Dani]

Verify that if the path exists, then the path is a file using
test_path_is_file() helper function.

Mahendra Dani (1):
  t1403: verify that path exists and is a file

 t/t1403-show-ref.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Range-diff against v3:
1:  42dd686abe ! 1:  d181f98d1a t1403: verify that path exists and is a file
    @@ Metadata
      ## Commit message ##
         t1403: verify that path exists and is a file
     
    -    test -e does not provide a nice error message when
    -    we hit test failures, so use test_path_exists() instead
    -    and verify that if the path exists then it is a file using test_path_is_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: test_expect_success 'show-ref --verify with dangling ref' '
      	remove_object() {
      		file=$(sha1_file "$*") &&
     -		test -e "$file" &&
    -+		test_path_exists "$file" &&
     +		test_path_is_file "$file" &&
      		rm -f "$file"
      	} &&
-- 
2.39.2 (Apple Git-143)

[PATCH v4 0/1] t1403: verify path exists and is a file

[Mahendra Dani]

Verify that if the path exists, then the path is a file using
test_path_is_file() helper function.

Thanks,
Mahendra

Mahendra Dani (1):
  t1403: verify that path exists and is a file

 t/t1403-show-ref.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Range-diff against v3:
1:  42dd686abe ! 1:  d181f98d1a t1403: verify that path exists and is a file
    @@ Metadata
      ## Commit message ##
         t1403: verify that path exists and is a file
     
    -    test -e does not provide a nice error message when
    -    we hit test failures, so use test_path_exists() instead
    -    and verify that if the path exists then it is a file using test_path_is_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: test_expect_success 'show-ref --verify with dangling ref' '
      	remove_object() {
      		file=$(sha1_file "$*") &&
     -		test -e "$file" &&
    -+		test_path_exists "$file" &&
     +		test_path_is_file "$file" &&
      		rm -f "$file"
      	} &&
-- 
2.39.2 (Apple Git-143)

[PATCH v4 1/1] t1403: verify that path exists and is a file

[Mahendra Dani]

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"
 	} &&
 
-- 
2.39.2 (Apple Git-143)

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Junio C Hamano]

Mahendra Dani <danimahendra0904@gmail.com> writes:

>> >       remove_object() {
>> >               file=$(sha1_file "$*") &&
>> > -             test -e "$file" &&
>> > +             test_path_exists "$file" &&
>> >               rm -f "$file"
>> >       } &&
>>
>> The refactoring is true to the original spirit of the preimage indeed.
>> But we could also improve it even further if we verified that the path
>> not only exists, but exists and is a file via `test_path_is_file()`. If
>> we decide to do that we should also explain the change in the commit
>> message.
>
> Yes, sure.
> I will improve it further using the `test_path_is_file()` helper
> function and change the commit message in v2 patch.

You may want to think about why there is "-f" there.  If we remove
it, do we still need to have any check there?

Re: [GSOC][PATCH v3 0/1] t1403: verify path exists and is a file

[Junio C Hamano]

Mahendra Dani <danimahendra0904@gmail.com> writes:

> test -e does not provide a nice error message when we hit test failures,
> so use test_path_exists() instead.
>
> Further, verify that if the path exists, then the path is a file using
> test_path_is_file() helper function.
>
> This patch does not change any code in v2, but is rather submitted with proper formatting
> which was lacking in v2. 
> I apologize for the incorrect patch submission in v2. 

My cursory look didn't spot anything iffy in v2 and I found it
nicely described.  I do not see anything, other than the above
3-line paragraph, that is different in v3 from v2, which is a bit
curious.

Thanks.

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Mahendra Dani]

On Tue, Mar 4, 2025 at 5:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Mahendra Dani <danimahendra0904@gmail.com> writes:
>
> >> >       remove_object() {
> >> >               file=$(sha1_file "$*") &&
> >> > -             test -e "$file" &&
> >> > +             test_path_exists "$file" &&
> >> >               rm -f "$file"
> >> >       } &&
> >>
> >> The refactoring is true to the original spirit of the preimage indeed.
> >> But we could also improve it even further if we verified that the path
> >> not only exists, but exists and is a file via `test_path_is_file()`. If
> >> we decide to do that we should also explain the change in the commit
> >> message.
> >
> > Yes, sure.
> > I will improve it further using the `test_path_is_file()` helper
> > function and change the commit message in v2 patch.
>
> You may want to think about why there is "-f" there.  If we remove
> it, do we still need to have any check there?

Here, the "-f" flag in `rm -f "$file"` does not produce an error message even
if the file does not exist [1], thus the `test -e "$file"` check was redundant,
as pointed out by Patrick in [2].

However, switching to `test_path_is_file()` would provide additional safety by
ensuring that we only attempt to remove a regular file and not a directory.

[References]
1. https://man7.org/linux/man-pages/man1/rm.1.html
2. https://lore.kernel.org/git/Z8bd3iHrhXb4WH6A@pks.im/

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Junio C Hamano]

Mahendra Dani <danimahendra0904@gmail.com> writes:

> On Tue, Mar 4, 2025 at 5:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Mahendra Dani <danimahendra0904@gmail.com> writes:
>>
>> >> >       remove_object() {
>> >> >               file=$(sha1_file "$*") &&
>> >> > -             test -e "$file" &&
>> >> > +             test_path_exists "$file" &&
>> >> >               rm -f "$file"
>> >> >       } &&
>>
>> You may want to think about why there is "-f" there.  If we remove
>> it, do we still need to have any check there?
>
> Here, the "-f" flag in `rm -f "$file"` does not produce an error message even
> if the file does not exist [1], thus the `test -e "$file"` check was redundant,
> as pointed out by Patrick in [2].

So what happens if you dropped "-f" as I hinted?  We'll notice the
lack of file and the command exits with non-zero status.  So "test -e"
was not necessary in the first place, was it?

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Mahendra Dani]

On Tue, Mar 4, 2025 at 10:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Mahendra Dani <danimahendra0904@gmail.com> writes:
>
> > On Tue, Mar 4, 2025 at 5:35 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Mahendra Dani <danimahendra0904@gmail.com> writes:
> >>
> >> >> >       remove_object() {
> >> >> >               file=$(sha1_file "$*") &&
> >> >> > -             test -e "$file" &&
> >> >> > +             test_path_exists "$file" &&
> >> >> >               rm -f "$file"
> >> >> >       } &&
> >>
> >> You may want to think about why there is "-f" there.  If we remove
> >> it, do we still need to have any check there?
> >
> > Here, the "-f" flag in `rm -f "$file"` does not produce an error message even
> > if the file does not exist [1], thus the `test -e "$file"` check was redundant,
> > as pointed out by Patrick in [2].
>
> So what happens if you dropped "-f" as I hinted?  We'll notice the
> lack of file and the command exits with non-zero status.  So "test -e"
> was not necessary in the first place, was it?
>

Yes, due to the use of the "-f" flag, it's not necessary to explicitly
check the lack of file using `test -e`.
But if we drop the "-f" flag, we will have to check the lack of file
using `test -e` or
`test_path_is_file()`.

Thanks,
Mahendra

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Eric Sunshine]

On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote:
> Mahendra Dani <danimahendra0904@gmail.com> writes:
> >> >       remove_object() {
> >> >               file=$(sha1_file "$*") &&
> >> > -             test -e "$file" &&
> >> > +             test_path_exists "$file" &&
> >> >               rm -f "$file"
> >> >       } &&
> >>
> >> The refactoring is true to the original spirit of the preimage indeed.
> >> But we could also improve it even further if we verified that the path
> >> not only exists, but exists and is a file via `test_path_is_file()`. If
> >> we decide to do that we should also explain the change in the commit
> >> message.
> >
> > I will improve it further using the `test_path_is_file()` helper
> > function and change the commit message in v2 patch.
>
> You may want to think about why there is "-f" there.  If we remove
> it, do we still need to have any check there?

That's a good question to ask, but isn't the implied suggestion of
dropping "-f" going in the wrong direction? If I'm reading
remove_object() correctly, `test -e` is being used as control flow,
*not* as an assertion that the file exists. That is, the expectation
of the caller is that the file will not exist once the call completes
and that remove_object() will return a success code whether the file
was present before the call or not. By control flow, I mean that the
function, as written, is the same as this more explicit version:

    remove_object() {
        file=$(sha1_file "$*") &&
        if test -e "$file"
        then
            rm -f "$file"
        fi
    } &&

Given this understanding, then it becomes apparent that this GSoC
microproject shouldn't be applying *any* test_path_foo() to this
function. As an alternative, given that `rm -f` returns a success code
whether or not the file exists, the microproject could instead
*remove* the `test -e "$file" &&` line entirely (and the commit
message should explain why doing so is a reasonable thing to do).

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Junio C Hamano]

Mahendra Dani <danimahendra0904@gmail.com> writes:

> Yes, due to the use of the "-f" flag, it's not necessary to explicitly
> check the lack of file using `test -e`.
> But if we drop the "-f" flag, we will have to check the lack of file
> using `test -e` or
> `test_path_is_file()`.

Isn't it the other way around?

    $ rm -f nosuch ; echo $?
    0
    $ rm nosuch ; echo $?
    rm: cannot remove 'nosuch': No such file or directory
    1

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote:
>> Mahendra Dani <danimahendra0904@gmail.com> writes:
>> >> >       remove_object() {
>> >> >               file=$(sha1_file "$*") &&
>> >> > -             test -e "$file" &&
>> >> > +             test_path_exists "$file" &&
>> >> >               rm -f "$file"
>> >> >       } &&
> That's a good question to ask, but isn't the implied suggestion of
> dropping "-f" going in the wrong direction? If I'm reading
> remove_object() correctly, `test -e` is being used as control flow,
> *not* as an assertion that the file exists.

If sha1_file says the loose object must be at path $file, and the
call to test -e "$file" returns false, two things happen in this
function:

 (1) control stops and "rm -f" does not trigger
 (2) the function returns non-zero status to the caller

If you omit the check and say rm "$file" instead, under the same
scenario, (1) "rm" is attempted, but there is nothing to remove so
the command returns non-zero status, and (2) the function returns
that non-zero status to the caller

If the file does exist, both will remove the file, and give the
caller zero status return.

So?

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Mahendra Dani]

On Tue, Mar 4, 2025 at 11:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Mahendra Dani <danimahendra0904@gmail.com> writes:
>
> > Yes, due to the use of the "-f" flag, it's not necessary to explicitly
> > check the lack of file using `test -e`.
> > But if we drop the "-f" flag, we will have to check the lack of file
> > using `test -e` or
> > `test_path_is_file()`.
>
> Isn't it the other way around?
>
>     $ rm -f nosuch ; echo $?
>     0
>     $ rm nosuch ; echo $?
>     rm: cannot remove 'nosuch': No such file or directory
>     1
>

Yes, you are right.
With the "-f" flag, `rm` returns exit code 0 irrespective of whether
the file is present or not.
Thus, the `test -e` check is _required_ if we drop the "-f" flag to
return the correct exit code.

I apologize for the mistake.

Thanks,
Mahendra

Re: [PATCH v3 1/1] t1403: verify that path exists and is a file

[Junio C Hamano]

Mahendra Dani <danimahendra0904@gmail.com> writes:

> test -e does not provide a nice error message when
> we hit test failures, so use test_path_exists() instead
> and 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
> index 9d698b3cc3..4afde01a29 100755
> --- a/t/t1403-show-ref.sh
> +++ b/t/t1403-show-ref.sh
> @@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '
>  
>  	remove_object() {
>  		file=$(sha1_file "$*") &&
> -		test -e "$file" &&
> +		test_path_exists "$file" &&
> +		test_path_is_file "$file" &&
>  		rm -f "$file"
>  	} &&

Makes sense.  Will queue.

Re: [PATCH v3 1/1] t1403: verify that path exists and is a file

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Mahendra Dani <danimahendra0904@gmail.com> writes:
>
>> test -e does not provide a nice error message when
>> we hit test failures, so use test_path_exists() instead
>> and 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 | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
>> index 9d698b3cc3..4afde01a29 100755
>> --- a/t/t1403-show-ref.sh
>> +++ b/t/t1403-show-ref.sh
>> @@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '
>>  
>>  	remove_object() {
>>  		file=$(sha1_file "$*") &&
>> -		test -e "$file" &&
>> +		test_path_exists "$file" &&
>> +		test_path_is_file "$file" &&
>>  		rm -f "$file"
>>  	} &&
>
> Makes sense.  Will queue.

No, no, no.  test_is_file alone is sufficient---if the thing does
not exist, it would not be a file anyway ;-)

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Eric Sunshine]

On Tue, Mar 4, 2025 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote:
> >> Mahendra Dani <danimahendra0904@gmail.com> writes:
> >> >> >       remove_object() {
> >> >> >               file=$(sha1_file "$*") &&
> >> >> > -             test -e "$file" &&
> >> >> > +             test_path_exists "$file" &&
> >> >> >               rm -f "$file"
> >> >> >       } &&
> > That's a good question to ask, but isn't the implied suggestion of
> > dropping "-f" going in the wrong direction? If I'm reading
> > remove_object() correctly, `test -e` is being used as control flow,
> > *not* as an assertion that the file exists.
>
> If sha1_file says the loose object must be at path $file, and the
> call to test -e "$file" returns false, two things happen in this
> function:
>
>  (1) control stops and "rm -f" does not trigger
>  (2) the function returns non-zero status to the caller

True enough. I was misreading `test -e "$file"` as _only_ control flow.

However, it's still not clear to me why this function is making the
`test -e "$file"` assertion in the first place or why the enclosing
test should care, especially since that assertion is only checking
that `git commit` worked correctly, but that's not the intent of this
particular test[1]. So, `test -e "$file"` seems pointless or at least
misleading.

> If you omit the check and say rm "$file" instead, under the same
> scenario, (1) "rm" is attempted, but there is nothing to remove so
> the command returns non-zero status, and (2) the function returns
> that non-zero status to the caller

Yes, I understood the implication of your suggestion, but as mentioned
above, it's not clear (at least to me) why `test -e "$file"` is there
at all since this test is not about checking functionality of `git
commit`.

[1]: d01b8203ec (show-ref: detect dangling refs under --verify as
well, 2017-01-23) doesn't explain why `test -e "$file"` was used.

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Junio C Hamano]

Mahendra Dani <danimahendra0904@gmail.com> writes:

> On Tue, Mar 4, 2025 at 11:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Mahendra Dani <danimahendra0904@gmail.com> writes:
>>
>> > Yes, due to the use of the "-f" flag, it's not necessary to explicitly
>> > check the lack of file using `test -e`.
>> > But if we drop the "-f" flag, we will have to check the lack of file
>> > using `test -e` or
>> > `test_path_is_file()`.
>>
>> Isn't it the other way around?
>>
>>     $ rm -f nosuch ; echo $?
>>     0
>>     $ rm nosuch ; echo $?
>>     rm: cannot remove 'nosuch': No such file or directory
>>     1
>>
>
> Yes, you are right.
> With the "-f" flag, `rm` returns exit code 0 irrespective of whether
> the file is present or not.
> Thus, the `test -e` check is _required_ if we drop the "-f" flag to
> return the correct exit code.
>
> I apologize for the mistake.

No need for an apology when correcting technical mistakes, which we
all make.

Re: [PATCH v3 1/1] t1403: verify that path exists and is a file

[Mahendra Dani]

On Tue, Mar 4, 2025 at 11:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Mahendra Dani <danimahendra0904@gmail.com> writes:
> >
> >> test -e does not provide a nice error message when
> >> we hit test failures, so use test_path_exists() instead
> >> and 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 | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
> >> index 9d698b3cc3..4afde01a29 100755
> >> --- a/t/t1403-show-ref.sh
> >> +++ b/t/t1403-show-ref.sh
> >> @@ -196,7 +196,8 @@ test_expect_success 'show-ref --verify with dangling ref' '
> >>
> >>      remove_object() {
> >>              file=$(sha1_file "$*") &&
> >> -            test -e "$file" &&
> >> +            test_path_exists "$file" &&
> >> +            test_path_is_file "$file" &&
> >>              rm -f "$file"
> >>      } &&
> >
> > Makes sense.  Will queue.
>
> No, no, no.  test_is_file alone is sufficient---if the thing does
> not exist, it would not be a file anyway ;-)
>

Yes, it was pointed out by Patrick in [1], that `test_path_is_file()`
alone is sufficient. Hence I removed the `test_path_exists()` check in
patch v4 [2].

[References]
1. https://lore.kernel.org/git/Z8bd3iHrhXb4WH6A@pks.im/
2. https://lore.kernel.org/git/20250304112728.41228-2-danimahendra0904@gmail.com/

Re: [PATCH v4 1/1] t1403: verify that path exists and is a file

[Junio C Hamano]

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.

Re: [PATCH v4 1/1] t1403: verify that path exists and is a file

[Mahendra Dani]

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

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Eric Sunshine]

On Tue, Mar 4, 2025 at 1:07 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > >> >> >       remove_object() {
> > >> >> >               file=$(sha1_file "$*") &&
> > >> >> > -             test -e "$file" &&
> > >> >> > +             test_path_exists "$file" &&
> > >> >> >               rm -f "$file"
> > >> >> >       } &&
>
> However, it's still not clear to me why this function is making the
> `test -e "$file"` assertion in the first place or why the enclosing
> test should care, especially since that assertion is only checking
> that `git commit` worked correctly, but that's not the intent of this
> particular test[1]. So, `test -e "$file"` seems pointless or at least
> misleading.

Perhaps, I'm falling into the trap of assuming that a lone `git
commit` in a new repository will unconditionally create a loose
object, and that that will always be the case? If, down the road, `git
commit` no longer works that way, then the assumption about the loose
object becomes invalid, in which case I can see how the `test -e
"$file"` assertion is protecting the test against that future.

Re: [PATCH 1/1] t1403: prefer test_path_exists helper function

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Mar 4, 2025 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote:
>> >> Mahendra Dani <danimahendra0904@gmail.com> writes:
>> >> >> >       remove_object() {
>> >> >> >               file=$(sha1_file "$*") &&
>> >> >> > -             test -e "$file" &&
>> >> >> > +             test_path_exists "$file" &&
>> >> >> >               rm -f "$file"
>> >> >> >       } &&
> ...
> Yes, I understood the implication of your suggestion, but as mentioned
> above, it's not clear (at least to me) why `test -e "$file"` is there
> at all since this test is not about checking functionality of `git
> commit`.

Yup, I do not see much point in "test -e" there in the original, and
it does not change even if it were "test -f".

I would understand if the author wanted to have a "slightly more
intelligent 'rm -f' that knows where a loose object is located, and
removes the named object no matter what", but if the objective were
to ensure the object is missing, I wouldn't have written it to
return non-zero when the object were missing in the first place.

And if the purpose of the function is to catch unexpected cases,
such as "the loose object file should be there but isn't" and "we
located the file but we failed to remove it", then it shouldn't have
the 'test -e' guard and 'rm' shouldn't have been used with '-f'.

So, I agree with you that the original is already iffy.

Thanks.