Re: [PATCH 81/83] apply: roll back index in case of error

From: Christian Couder <christian.couder@gmail.com>
Date: May 3, 2016, 12:57 UTC
Message-ID: <CAP8UFD0tGruUTrk5aJ7oGM9jj1HzpM2afqEsJfKyuDB+CWc=fQ@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1604251802480.2896@virtualbox>

Hi Dscho,

On Mon, Apr 25, 2016 at 6:06 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 24 quoted lines

> Hi Chris,
>
> On Sun, 24 Apr 2016, Christian Couder wrote:
>
>> @@ -4734,16 +4737,22 @@ int apply_all_patches(struct apply_state *state,
>>               read_stdin = 0;
>>               set_default_whitespace_mode(state);
>>               res = apply_patch(state, fd, arg, options);
>> -             if (res < 0)
>> +             if (res < 0) {
>> +                     if (state->lock_file)
>> +                             rollback_lock_file(state->lock_file);
>>                       return -1;
>> +             }
>>               errs |= res;
>>               close(fd);
>
> In case of error, this leaves fd open, which in the end will prevent the
> "patch" file, and hence the "rebase-apply/" directory from being removed
> on Windows. This triggered a failure of t4014 here (and possibly more, but
> it took me quite a while to track this down, what with builtin/am.c's
> am_destroy() not bothering at all to check the return value of
> remove_dir_recursively(), resulting in the error to be caught only much,
> much later).

Sorry about that and thanks for tracking down the source of the test failure. I fixed this by moving the "close(fd)" call just after the "apply_patch()" call.

> Could you please review all open()/close() and fopen()/fclose() calls in
> your patch series, to make sure that there are no mistakes? A passing test
> suite does not really make me confident here, as our code coverage is not
> quite 100%.

Ok, I will have another look at the 2 other places where there are open()/close() or fopen()/fclose() calls.

Thanks, Christian.