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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 4, 2025, 18:07 UTC
Message-ID: <CAPig+cS3QZhZ=W2WfM3T3EngaMOSV37KH4Pqp78QHzOAODtenA@mail.gmail.com>
In-Reply-To: <xmqqcyewr8j7.fsf@gitster.g>

On Tue, Mar 4, 2025 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 20 quoted lines

> 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"
> >> >> >       } &&
> > 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.
>
> If sha1_file says the loose object must be at path $file, and the
> call to test -e "$file" returns false, two things happen in this
> function:
>
>  (1) control stops and "rm -f" does not trigger
>  (2) the function returns non-zero status to the caller

True enough. I was misreading `test -e "$file"` as _only_ control flow.

However, it's still not clear to me why this function is making the `test -e "$file"` assertion in the first place or why the enclosing test should care, especially since that assertion is only checking that `git commit` worked correctly, but that's not the intent of this particular test[1]. So, `test -e "$file"` seems pointless or at least misleading.

> If you omit the check and say rm "$file" instead, under the same
> scenario, (1) "rm" is attempted, but there is nothing to remove so
> the command returns non-zero status, and (2) the function returns
> that non-zero status to the caller

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

[1]: d01b8203ec (show-ref: detect dangling refs under --verify as well, 2017-01-23) doesn't explain why `test -e "$file"` was used.