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

Eric Sunshine [off-list ref] writes:

On Tue, Mar 4, 2025 at 12:49 PM Junio C Hamano [off-list ref] wrote:

quoted

Eric Sunshine [off-list ref] writes:

quoted

On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano [off-list ref] wrote:

quoted

Mahendra Dani [off-list ref] writes:

quoted

quoted

quoted

      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.