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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 4, 2025, 18:30 UTC
Message-ID: <xmqq1pvcps2f.fsf@gitster.g>
In-Reply-To: <CAPig+cS3QZhZ=W2WfM3T3EngaMOSV37KH4Pqp78QHzOAODtenA@mail.gmail.com>

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.