Re: [PATCH 74/83] builtin/apply: make try_create_file() return -1 on error
On Mon, May 2, 2016 at 8:01 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 22 quoted lines
> On Sun, Apr 24, 2016 at 9:34 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> 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 20 quoted lines
>> @@ -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.Ok, I moved up the error cases.
> 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.
Yeah, but I don't see what I could do about that.
Christian Couder <christian.couder@gmail.com>