Re: [PATCH 74/83] builtin/apply: make try_create_file() return -1 on error
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- May 2, 2016, 18:01 UTC
- Message-ID
- <CAPig+cQsS7x2qb8SphaAgdfpCLBQ-z=joL3w8RZH0HC9xRG54g@mail.gmail.com>
- In-Reply-To
- <1461504863-15946-75-git-send-email-chriscool@tuxfamily.org>
On Sun, Apr 24, 2016 at 9:34 AM, Christian Couder <christian.couder@gmail.com> wrote:
Show 18 quoted lines
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> diff --git a/builtin/apply.c b/builtin/apply.c
> @@ -4145,28 +4151,32 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
> fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode & 0100) ? 0777 : 0666);
> if (fd < 0)
> - return -1;
> + return 1;
>
> if (convert_to_working_tree(path, buf, size, &nbuf)) {
> size = nbuf.len;
> buf = nbuf.buf;
> }
> - write_or_die(fd, buf, size);
> +
> + if (!write_or_whine_pipe(fd, buf, size, path)) {
> + strbuf_release(&nbuf);
> + return -1;This is leaking 'fd'.
Show 23 quoted lines
> + }
> strbuf_release(&nbuf);
>
> if (close(fd) < 0)
> - die_errno(_("closing file '%s'"), path);
> + return error(_("closing file '%s': %s"), path, strerror(errno));
> return 0;
> }
>
> @@ -4208,12 +4227,15 @@ static void create_one_file(struct apply_state *state,
> for (;;) {
> char newpath[PATH_MAX];
> mksnpath(newpath, sizeof(newpath), "%s~%u", path, nr);
> - if (!try_create_file(newpath, mode, buf, size)) {
> + res = try_create_file(newpath, mode, buf, size);
> + if (!res) {
> if (!rename(newpath, path))
> return;
> unlink_or_warn(newpath);
> break;
> }
> + if (res < 0)
> + exit(1);Two issues:
Getting the error case out of the way early (moving this 'if' just after 'res=...') would make it easier to reason about the remaining logic.
It's already difficult to understand what the below 'errno' check is testing. try_create_file(), rename(), or unlink_or_warn()? Plopping this new error handling conditional in front of it divorces the 'errno' check even further from whatever it is testing.
> if (errno != EEXIST) > break; > ++nr;