Re: [PATCH v2 63/94] builtin/apply: make apply_all_patches() return -1 on error

> On Wed, Jun 8, 2016 at 12:37 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Mon, May 16, 2016 at 5:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, May 11, 2016 at 9:17 AM, Christian Couder
>>> <christian.couder@gmail.com> wrote:
>>>>         if (state->update_index) {
>>>> -               if (write_locked_index(&the_index, state->lock_file, COMMIT_LOCK))
>>>> -                       die(_("Unable to write new index file"));
>>>> +               res = write_locked_index(&the_index, state->lock_file, COMMIT_LOCK);
>>>>                 state->newfd = -1;
>>>
>>> Does write_locked_index() unconditionally close the file descriptor
>>> even when an error occurs? If not, then isn't this potentially leaking
>>> 'newfd'?
>>>
>>> (My very cursory read of write_locked_index() seems to reveal that the
>>> file descriptor may indeed remain open upon index write failure.)
>>
>> You are right, it is leaking newfd if write_locked_index() fails.
>> The solution to that is to call `rollback_lock_file(state->lock_file)`
>> and the following patch was supposed to do that:
>>
>> [PATCH v2 82/94] apply: roll back index lock file in case of error
>>
>> but it would do that only if `state->newfd >= 0` so we should set
>> state->newfd to -1 only if write_locked_index() succeeds.
>>
>> I will fix this.
>>
>> I am also going to add a comment to this patch saying that this patch
>> needs a following patch to call rollback_lock_file(state->lock_file)
>> in case of errors.
>>
>> Or if you prefer, I can squash the patch that call
>> rollback_lock_file(state->lock_file) in case of errors into this
>> patch.
>
> Squashing may indeed be preferable over leaving it in a "broken" state
> until the next patch, though I haven't thought too hard about it.
> Alternately, can the two patches somehow be swapped?