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

From: Mahendra Dani <danimahendra0904@gmail.com>
Date: Mar 4, 2025, 17:24 UTC
Message-ID: <CABGrwBAaN3U8WCtPgd+UjVjsoBniCawWV3U+WdKOzBdG5LxmAg@mail.gmail.com>
In-Reply-To: <xmqqldtlt304.fsf@gitster.g>

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/