{"thread":{"id":"7411","subject":"[PATCH] sha1_file.c (write_sha1_from_fd): Detect close failure.","startedAt":"2007-03-26T13:57:23Z","lastAt":"2007-03-26T20:44:47Z","messageCount":5,"participants":["Jim Meyering","Junio C Hamano","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"38031","messageId":"874po8umyk.fsf@rho.meyering.net","threadId":"7411","inReplyTo":null,"subject":"[PATCH] sha1_file.c (write_sha1_from_fd): Detect close failure.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-03-26T13:57:23Z","receivedAt":"2007-03-26T13:57:23Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"I stumbled across this in the context of the fchmod 0444 patch.\nAt first, I was going to unlink and call error like the two subsequent\ntests do, but a failed write (above) provokes a \"die\", so I made\nthis do the same. This is testing for a write failure, after all.\n\nSigned-off-by: Jim Meyering \n---\n sha1_file.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 0897b94..42aef33 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2155,7 +2155,8 @@ int write_sha1_from_fd(const unsigned char *sha1, int fd, char *buffer,\n \tinflateEnd(&stream);\n\n \tfchmod(local, 0444);\n-\tclose(local);\n+\tif (close(local) != 0)\n+\t\tdie(\"unable to write sha1 file\");\n \tSHA1_Final(real_sha1, &c);\n \tif (ret != Z_STREAM_END) {\n \t\tunlink(tmpfile);\n--\n1.5.1.rc1.51.gb08b\n"},{"id":"38060","messageId":"7v8xdjwxr4.fsf@assigned-by-dhcp.cox.net","threadId":"7411","inReplyTo":"874po8umyk.fsf@rho.meyering.net","subject":"Re: [PATCH] sha1_file.c (write_sha1_from_fd): Detect close failure.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-26T20:33:35Z","receivedAt":"2007-03-26T20:33:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> I stumbled across this in the context of the fchmod 0444 patch.\n> At first, I was going to unlink and call error like the two subsequent\n> tests do, but a failed write (above) provokes a \"die\", so I made\n> this do the same. This is testing for a write failure, after all.\n>\n> Signed-off-by: Jim Meyering \n> ---\n> sha1_file.c | 3 ++-\n> 1 files changed, 2 insertions(+), 1 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 0897b94..42aef33 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2155,7 +2155,8 @@ int write_sha1_from_fd(const unsigned char *sha1, int fd, char *buffer,\n> \tinflateEnd(&stream);\n>\n> \tfchmod(local, 0444);\n> -\tclose(local);\n> +\tif (close(local) != 0)\n> +\t\tdie(\"unable to write sha1 file\");\n> \tSHA1_Final(real_sha1, &c);\n> \tif (ret != Z_STREAM_END) {\n> \t\tunlink(tmpfile);\n> --\n> 1.5.1.rc1.51.gb08b\n\nHmph. Not catching error from close() is wrong, so this is an\nimprovement, but it still leaves tmpfile on the filesystem,\ndoesn't it?\n\nLooking at write_sha1_file(), which is in a sense more important\nthan this function, it is worse. We should also detect error\nfrom close(), nuke the temporary file and return an error there.\n"},{"id":"38061","messageId":"87fy7ru4fy.fsf@rho.meyering.net","threadId":"7411","inReplyTo":"7v8xdjwxr4.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] sha1_file.c (write_sha1_from_fd): Detect close failure.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-03-26T20:37:21Z","receivedAt":"2007-03-26T20:37:21Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n\n> Jim Meyering writes:\n>\n>> I stumbled across this in the context of the fchmod 0444 patch.\n>> At first, I was going to unlink and call error like the two subsequent\n>> tests do, but a failed write (above) provokes a \"die\", so I made\n>> this do the same. This is testing for a write failure, after all.\n>>\n>> Signed-off-by: Jim Meyering \n>> ---\n>> sha1_file.c | 3 ++-\n>> 1 files changed, 2 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 0897b94..42aef33 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -2155,7 +2155,8 @@ int write_sha1_from_fd(const unsigned char *sha1, int fd, char *buffer,\n>> \tinflateEnd(&stream);\n>>\n>> \tfchmod(local, 0444);\n>> -\tclose(local);\n>> +\tif (close(local) != 0)\n>> +\t\tdie(\"unable to write sha1 file\");\n>> \tSHA1_Final(real_sha1, &c);\n>> \tif (ret != Z_STREAM_END) {\n>> \t\tunlink(tmpfile);\n>> --\n>> 1.5.1.rc1.51.gb08b\n>\n> Hmph. Not catching error from close() is wrong, so this is an\n> improvement, but it still leaves tmpfile on the filesystem,\n> doesn't it?\n>\n> Looking at write_sha1_file(), which is in a sense more important\n> than this function, it is worse. We should also detect error\n> from close(), nuke the temporary file and return an error there.\n\nThat was my thought, too, as I said, above.\nI assumed whoever decided a write failure was worth\ncalling \"die\", also wanted to leave behind the corrupt\ntemporary file. In retrospect, I shouldn't have assumed.\n\nOf course, if that's not the case, they both should unlink\nthe temporary.\n"},{"id":"38063","messageId":"alpine.LFD.0.83.0703261636410.3041@xanadu.home","threadId":"7411","inReplyTo":"7v8xdjwxr4.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] sha1_file.c (write_sha1_from_fd): Detect close failure.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-03-26T20:42:59Z","receivedAt":"2007-03-26T20:42:59Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 26 Mar 2007, Junio C Hamano wrote:\n\n> Hmph. Not catching error from close() is wrong, so this is an\n> improvement, but it still leaves tmpfile on the filesystem,\n> doesn't it?\n> \n> Looking at write_sha1_file(), which is in a sense more important\n> than this function, it is worse. We should also detect error\n> from close(), nuke the temporary file and return an error there.\n\nActually, the temp file is always left there whenever there is an error \nor die() is called or CTRL_C is issued.\n\nI don't think it is worth bothering with the removal of temp files given \nall the cases that would have to be considered, and most probably not \nexercised that often (increasing their likelihood of bing buggy).\n\nInstead, teaching git-purge about those tmp files might be a better \nidea.\n\n\nNicolas\n"},{"id":"38064","messageId":"alpine.LFD.0.83.0703261644200.3041@xanadu.home","threadId":"7411","inReplyTo":"alpine.LFD.0.83.0703261636410.3041@xanadu.home","subject":"Re: [PATCH] sha1_file.c (write_sha1_from_fd): Detect close failure.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-03-26T20:44:47Z","receivedAt":"2007-03-26T20:44:47Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 26 Mar 2007, Nicolas Pitre wrote:\n\n> Instead, teaching git-purge about those tmp files might be a better \n> idea.\n\ngit-prune that is.\n\n\nNicolas\n"}]}