On Mon, May 2, 2016 at 8:01 PM, Eric Sunshine <[email protected]> wrote:
> On Sun, Apr 24, 2016 at 9:34 AM, Christian Couder
> <[email protected]> wrote:
>> Signed-off-by: Christian Couder <[email protected]>
>> ---
>> 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'.
Ok, fixed.
>> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
