Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2016, 15:45 UTC
Message-ID: <xmqqr3a45yi7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAFZEwPNFohRLCS4piB1t0LO-=keucwzRKZ2Jyhhf234tnWnVGg@mail.gmail.com>

Pranit Bauva <pranit.bauva@gmail.com> writes:

Show 9 quoted lines

>> Also you do not seem to check the error from the function to smudge
>> the "result" you are returning from this function.
>
> Yes I should combine the results from every removal.
>
>> Isn't unlink_or_warn() more correct helper to use here?
>
> The shell code uses rm -f which is silent and it removes only if
> present.

Isn't that what unlink_or_warn() do? Call unlink() and happily return if unlink() succeeds or errors with ENOENT (i.e. path didn't exist in the first place), but otherwise reports an error (imagine: EPERM).

> So it makes me wonder which would be more appropriate
> unlink_or_warn() or remove_or_warn() or remove_path(). Is
> remove_path() different from its shell equivalent "rm -f"?

Read it again.

Show 8 quoted lines

>>> +     remove_path(git_path_bisect_start());
>>
>> I can see that refs/files-backend.c misuses it already, but
>> remove_path() helper is about removing a path in the working tree,
>> together with any parent directory that becomes empty due to the
>> removal.  You do not expect $GIT_DIR/ to become an empty directory
>> after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even
>> if it becomes empty.  It is a wrong helper function to use here.