From: Junio C Hamano Date: Fri, 17 Jan 2014 19:07:34 GMT Subject: Re: [PATCH 1/2] prefer xwrite instead of write Message-ID: In-Reply-To: <20140117182148.GY18964@google.com> Jonathan Nieder writes: > Hi, > > Erik Faye-Lund wrote: > >> --- a/builtin/merge.c >> +++ b/builtin/merge.c >> @@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead >> sha1_to_hex(commit->object.sha1)); >> pretty_print_commit(&ctx, commit, &out); >> } >> - if (write(fd, out.buf, out.len) < 0) >> + if (xwrite(fd, out.buf, out.len) < 0) >> die_errno(_("Writing SQUASH_MSG")); > > Shouldn't this use write_in_full() to avoid a silently truncated result? (*) Meaning this? If so, I think it makes sense. builtin/merge.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/merge.c b/builtin/merge.c index 6e108d2..a6a38ee 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -367,7 +367,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead sha1_to_hex(commit->object.sha1)); pretty_print_commit(&ctx, commit, &out); } - if (xwrite(fd, out.buf, out.len) < 0) + if (write_in_full(fd, out.buf, out.len) != out.len) die_errno(_("Writing SQUASH_MSG")); if (close(fd)) die_errno(_("Finishing SQUASH_MSG")); > > [...] >> --- a/streaming.c >> +++ b/streaming.c >> @@ -538,7 +538,7 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f >> goto close_and_exit; >> } >> if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 || >> - write(fd, "", 1) != 1)) >> + xwrite(fd, "", 1) != 1)) > > Yeah, if we get EINTR then it's worth retrying. > > [...] >> --- a/transport-helper.c >> +++ b/transport-helper.c >> @@ -1129,9 +1129,8 @@ static int udt_do_write(struct unidirectional_transfer *t) >> return 0; /* Nothing to write. */ >> >> transfer_debug("%s is writable", t->dest_name); >> - bytes = write(t->dest, t->buf, t->bufuse); >> - if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN && >> - errno != EINTR) { >> + bytes = xwrite(t->dest, t->buf, t->bufuse); >> + if (bytes < 0 && errno != EWOULDBLOCK) { > > Here the write is limited by BUFFERSIZE, and returning to the outer > loop to try another read when the write returns EAGAIN, like the > original code does, seems philosophically like the right thing to do. > > Luckily we don't use O_NONBLOCK anywhere, so the change shouldn't > matter in practice. So although it doesn't do any good, using xwrite > here for consistency should be fine. > > So my only worry is the (*) above. With that change, > Reviewed-by: Jonathan Nieder > > -- -- -- *** Please reply-to-all at all times *** *** (do not pretend to know who is subscribed and who is not) *** *** Please avoid top-posting. *** The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free. You received this message because you are subscribed to the Google Groups "msysGit" group. To post to this group, send email to msysgit@googlegroups.com To unsubscribe from this group, send email to msysgit+unsubscribe@googlegroups.com For more options, and view previous threads, visit this group at http://groups.google.com/group/msysgit?hl=en_US?hl=en --- You received this message because you are subscribed to the Google Groups "msysGit" group. To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.