On Sun, Apr 24, 2016 at 9:34 AM, Christian Couder
<christian.couder@xxxxxxxxx> wrote:
> Signed-off-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> ---
> 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'.
> + }
> 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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html