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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 16, 2016, 03:44 UTC
Message-ID: <CAPig+cQAM8i2sFY9UUYfN23PRGgFacG7KiTD6mZwnm=PgKBL7A@mail.gmail.com>
In-Reply-To: <20160511131745.2914-64-chriscool@tuxfamily.org>

On Wed, May 11, 2016 at 9:17 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 17 quoted lines

> To finish libifying the apply functionality, apply_all_patches() should not
> die() or exit() in case of error, but return -1.
>
> While doing that we must take care that file descriptors are properly closed
> and, if needed, reset a sensible value.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> diff --git a/builtin/apply.c b/builtin/apply.c
> @@ -4613,9 +4613,10 @@ static int apply_all_patches(struct apply_state *state,
>         }
>
>         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.)

Show 5 quoted lines

> +               if (res)
> +                       return error(_("Unable to write new index file"));
>         }
>
>         return !!errs;