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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 2, 2016, 07:18 UTC
Message-ID: <CAPig+cTovkSPi3W2-46fVLOdiYMCCwUkBNKys0C40dJwZ_dL-g@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1605020906400.9313@virtualbox>

On Mon, May 2, 2016 at 3:07 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> apparently this mail never made it to the list???

It made it to the list[1].

[1]: http://git.661346.n2.nabble.com/PATCH-80-83-run-command-make-dup-devnull-non-static-tp7654127p7654233.html

Show 34 quoted lines

> On Mon, 25 Apr 2016, Johannes Schindelin wrote:
>
>> 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).
>>
>> 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%.
>>
>> Thanks,
>> Dscho