Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> On Tue, Mar 4, 2025 at 12:49 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
>> > On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> >> Mahendra Dani <danimahendra0904@xxxxxxxxx> 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.
