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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 4, 2025, 17:35 UTC
Message-ID: <CAPig+cQ275qBWzaYmMXydiA_8+CTx3FhAAY+o8ro+hD03wzu0g@mail.gmail.com>
In-Reply-To: <xmqqldtlt304.fsf@gitster.g>

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).