threads / patch / 63033

patcht1403: prefer test_path_exists helper function

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

## tl;dr

31 messages between Mar 1, 2025 and Mar 4, 2025. Diffs are folded; open one to read it.

replies: 30people: 4as markdown or json

Mahendra Dani· Mar 1, 2025, 10:58 UTC · lore

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)
Mahendra Dani· Mar 1, 2025, 10:58 UTC · re: Mahendra Dani · lore

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

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(-)
Show changes to t/t1403-show-ref.sh +1 −1
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)
Patrick Steinhardt· Mar 3, 2025, 10:26 UTC · re: Mahendra Dani · lore

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

On Sat, Mar 01, 2025 at 04:28:38PM +0530, Mahendra Dani wrote:
Show 20 quoted lines
> 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
Mahendra Dani· Mar 4, 2025, 02:27 UTC · re: Patrick Steinhardt · lore

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

On Mon, Mar 3, 2025 at 3:56 PM Patrick Steinhardt <ps@pks.im> wrote:
Show 32 quoted lines
>
> 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.

Junio C Hamano· Mar 4, 2025, 12:05 UTC · re: Mahendra Dani · lore

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

Mahendra Dani <danimahendra0904@gmail.com> writes:
Show 16 quoted lines
>> >       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?

Mahendra Dani· Mar 4, 2025, 17:24 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 4, 2025 at 5:35 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
>
> 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/
Junio C Hamano· Mar 4, 2025, 17:26 UTC · re: Mahendra Dani · lore

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

Mahendra Dani <danimahendra0904@gmail.com> writes:
Show 17 quoted lines
> 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?

Mahendra Dani· Mar 4, 2025, 17:35 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 4, 2025 at 10:57 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
>
> 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

Junio C Hamano· Mar 4, 2025, 17:44 UTC · re: Mahendra Dani · lore

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

Mahendra Dani <danimahendra0904@gmail.com> writes:
Show 5 quoted lines
> 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
Mahendra Dani· Mar 4, 2025, 17:49 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 4, 2025 at 11:14 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
>
> 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

Junio C Hamano· Mar 4, 2025, 18:07 UTC · re: Mahendra Dani · lore

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

Mahendra Dani <danimahendra0904@gmail.com> writes:
Show 26 quoted lines
> 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.

Eric Sunshine· Mar 4, 2025, 17:35 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> 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).

Junio C Hamano· Mar 4, 2025, 17:49 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 12 quoted lines
> 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?
Eric Sunshine· Mar 4, 2025, 18:07 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 4, 2025 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> 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.

Eric Sunshine· Mar 4, 2025, 18:28 UTC · re: Eric Sunshine · lore

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

On Tue, Mar 4, 2025 at 1:07 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 13 quoted lines
> > >> >> >       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.

Junio C Hamano· Mar 4, 2025, 18:30 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 15 quoted lines
> 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.
Mahendra Dani· Mar 4, 2025, 09:15 UTC · re: Mahendra Dani · lore

[GSOC][PATCH v2 0/1] t1403: verify 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.

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)
Mahendra Dani· Mar 4, 2025, 09:41 UTC · re: Mahendra Dani · lore

[GSOC][PATCH v3 0/1] t1403: verify 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.

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)
Mahendra Dani· Mar 4, 2025, 09:41 UTC · re: Mahendra Dani · lore

[PATCH v3 1/1] 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().

Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>
---
 t/t1403-show-ref.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to t/t1403-show-ref.sh +2 −1
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)
Junio C Hamano· Mar 4, 2025, 18:05 UTC · re: Mahendra Dani · lore

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

Mahendra Dani <danimahendra0904@gmail.com> writes:
Show 22 quoted lines
> 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.
Junio C Hamano· Mar 4, 2025, 18:06 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 26 quoted lines
> 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 ;-)

Mahendra Dani· Mar 4, 2025, 18:13 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 4, 2025 at 11:36 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 33 quoted lines
>
> 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/
Mahendra Dani· Mar 4, 2025, 11:23 UTC · re: Mahendra Dani · lore

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

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)
Mahendra Dani· Mar 4, 2025, 11:27 UTC · re: Mahendra Dani · lore

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

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)
Mahendra Dani· Mar 4, 2025, 11:27 UTC · re: Mahendra Dani · lore

[PATCH v4 1/1] t1403: verify that path exists and is a 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(-)
Show changes to t/t1403-show-ref.sh +1 −1
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)
Junio C Hamano· Mar 4, 2025, 18:13 UTC · re: Mahendra Dani · lore

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

Mahendra Dani <danimahendra0904@gmail.com> writes:
Show 19 quoted lines
> 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.
Mahendra Dani· Mar 4, 2025, 18:19 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 4, 2025 at 11:43 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 36 quoted lines
>
> 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

Junio C Hamano· Mar 4, 2025, 16:00 UTC · re: Mahendra Dani · lore

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

Mahendra Dani <danimahendra0904@gmail.com> writes:
Show 9 quoted lines
> 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.
Mahendra Dani· Mar 4, 2025, 09:27 UTC · re: Mahendra Dani · lore

[PATCH v2 1/1] 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().

Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com>
---
 t/t1403-show-ref.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to t/t1403-show-ref.sh +2 −1
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)
Patrick Steinhardt· Mar 4, 2025, 11:02 UTC · re: Mahendra Dani · lore

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

On Tue, Mar 04, 2025 at 02:57:22PM +0530, Mahendra Dani wrote:
Show 22 quoted lines
> 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
Mahendra Dani· Mar 4, 2025, 11:15 UTC · re: Patrick Steinhardt · lore

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

On Tue, Mar 4, 2025 at 4:33 PM Patrick Steinhardt <ps@pks.im> wrote:
Show 33 quoted lines
>
> 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.

← back to recent threads