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"
> >> >> > } &&
> > 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.
