{"thread":{"id":"16324","subject":"[PATCH] sha1_file: make sure correct error is propagated","startedAt":"2008-11-14T07:19:34Z","lastAt":"2008-11-15T06:30:23Z","messageCount":9,"participants":["Sam Vilain","Francis Galiegue","Junio C Hamano","Andreas Ericsson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"95769","messageId":"1226647174-15844-1-git-send-email-sam@vilain.net","threadId":"16324","inReplyTo":null,"subject":"[PATCH] sha1_file: make sure correct error is propagated","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2008-11-14T07:19:34Z","receivedAt":"2008-11-14T07:19:34Z","isPatch":true,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"From: Sam Vilain \n\nIn the case that a object directory exists, but is not writable, the\ncode path that tries to create it is followed and the returned errno\nand path that of the directory tried to be created. The resultant\nerror message is confusing.\n\nSo, if the mkstemp() fails with EPERM, don't try to create the\ndirectory - return straight away.\n\nSigned-off-by: Sam Vilain \n---\n sha1_file.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ab2b520..7662330 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2231,7 +2231,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)\n \tmemcpy(buffer, filename, dirlen);\n \tstrcpy(buffer + dirlen, \"tmp_obj_XXXXXX\");\n \tfd = mkstemp(buffer);\n-\tif (fd < 0 && dirlen) {\n+\tif (fd < 0 && dirlen && (errno != EPERM)) {\n \t\t/* Make sure the directory exists */\n \t\tmemcpy(buffer, filename, dirlen);\n \t\tbuffer[dirlen-1] = 0;\n-- \ndebian.1.5.6.1\n"},{"id":"95770","messageId":"200811140844.58746.fge@one2team.com","threadId":"16324","inReplyTo":"1226647174-15844-1-git-send-email-sam@vilain.net","subject":"Re: [PATCH] sha1_file: make sure correct error is propagated","fromName":"Francis Galiegue","fromEmail":"fge@one2team.com","sentAt":"2008-11-14T07:44:58Z","receivedAt":"2008-11-14T07:44:58Z","isPatch":true,"sender":{"key":"fge@one2team.com","avatar":null},"body":"Le vendredi 14 novembre 2008, Sam Vilain a écrit :\n> From: Sam Vilain \n> \n> In the case that a object directory exists, but is not writable, the\n> code path that tries to create it is followed and the returned errno\n> and path that of the directory tried to be created. The resultant\n> error message is confusing.\n> \n> So, if the mkstemp() fails with EPERM, don't try to create the\n> directory - return straight away.\n> \n\nAre you sure you didn't mean EACCESS?\n\n-- \nFrancis Galiegue\nONE2TEAM\nIngénieur système\nMob : +33 (0) 6 83 87 78 75\nTel : +33 (0) 1 78 94 55 52\nfge@one2team.com\n40 avenue Raymond Poincaré\n75116 Paris\n"},{"id":"95774","messageId":"1226655681.17731.4.camel@maia.lan","threadId":"16324","inReplyTo":"200811140844.58746.fge@one2team.com","subject":"Re: [PATCH] sha1_file: make sure correct error is propagated","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2008-11-14T09:41:21Z","receivedAt":"2008-11-14T09:41:21Z","isPatch":true,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"On Fri, 2008-11-14 at 08:44 +0100, Francis Galiegue wrote:\n> > So, if the mkstemp() fails with EPERM, don't try to create the\n> > directory - return straight away.\n> Are you sure you didn't mean EACCESS?\n\nAh, you're right there. Well, maybe this one should be as well:\n\nSubject: sha1_file: accept EACCESS as equivalent to EPERM\n\nThis was testing for 'Operation not permitted' rather than any kind\nof 'Permission Denied' error; prefer EACCESS.\n \nSigned-off-by: Sam Vilain \n--\n Sorry for the inevitable wrapping/whitespace fail :(\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7662330..cd422e6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2231,7 +2231,7 @@ static int create_tmpfile(char *buffer, size_t\nbufsiz, const char *filename)\n \tmemcpy(buffer, filename, dirlen);\n \tstrcpy(buffer + dirlen, \"tmp_obj_XXXXXX\");\n \tfd = mkstemp(buffer);\n-\tif (fd < 0 && dirlen && (errno != EPERM)) {\n+\tif (fd < 0 && dirlen && (errno != EACCESS)) {\n \t\t/* Make sure the directory exists */\n \t\tmemcpy(buffer, filename, dirlen);\n \t\tbuffer[dirlen-1] = 0;\n@@ -2257,7 +2257,7 @@ static int write_loose_object(const unsigned char\n*sha1, char *hdr, int hdrlen,\n \tfilename = sha1_file_name(sha1);\n \tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n \tif (fd < 0) {\n-\t\tif (errno == EPERM)\n+\t\tif (errno == EACCESS || errno == EPERM)\n \t\t\treturn error(\"insufficient permission for adding an object to\nrepository database %s\\n\", get_object_directory());\n \t\telse\n \t\t\treturn error(\"unable to create temporary sha1 filename %s: %s\\n\",\ntmpfile, strerror(errno));\n"},{"id":"95814","messageId":"7vfxlu9lhs.fsf@gitster.siamese.dyndns.org","threadId":"16324","inReplyTo":"1226655681.17731.4.camel@maia.lan","subject":"Re: [PATCH] sha1_file: make sure correct error is propagated","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-14T19:05:19Z","receivedAt":"2008-11-14T19:05:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sam Vilain writes:\n\n> Subject: sha1_file: accept EACCESS as equivalent to EPERM\n>\n> This was testing for 'Operation not permitted' rather than any kind\n> of 'Permission Denied' error; prefer EACCESS.\n> \n> Signed-off-by: Sam Vilain \n> --\n> Sorry for the inevitable wrapping/whitespace fail :(\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 7662330..cd422e6 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2231,7 +2231,7 @@ static int create_tmpfile(char *buffer, size_t\n> bufsiz, const char *filename)\n> \tmemcpy(buffer, filename, dirlen);\n> \tstrcpy(buffer + dirlen, \"tmp_obj_XXXXXX\");\n> \tfd = mkstemp(buffer);\n> -\tif (fd < 0 && dirlen && (errno != EPERM)) {\n> +\tif (fd < 0 && dirlen && (errno != EACCESS)) {\n\nIs this accepting the two as equivalents???\n"},{"id":"95815","messageId":"200811142009.51803.fg@one2team.com","threadId":"16324","inReplyTo":"7vfxlu9lhs.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] sha1_file: make sure correct error is propagated","fromName":"Francis Galiegue","fromEmail":"fg@one2team.com","sentAt":"2008-11-14T19:09:51Z","receivedAt":"2008-11-14T19:09:51Z","isPatch":true,"sender":{"key":"fg@one2team.com","avatar":null},"body":"Le Friday 14 November 2008 20:05:19 Junio C Hamano, vous avez écrit :\n[...]\n> > \tfd = mkstemp(buffer);\n> > -\tif (fd < 0 && dirlen && (errno != EPERM)) {\n> > +\tif (fd < 0 && dirlen && (errno != EACCESS)) {\n>\n> Is this accepting the two as equivalents???\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\nWell, looking at mkdir(2), it says:\n\n EPERM The file system containing pathname does not support the \ncreation of directories.\n\nHmm, err... git would fail at an earlier point anyway, wouldn't it? Even git \ninit would fail there.\n\n-- \nFrancis Galiegue\nONE2TEAM\nIngénieur système\nMob : +33 (0) 6 83 87 78 75\nTel : +33 (0) 1 78 94 55 52\nfge@one2team.com\n40 avenue Raymond Poincaré\n75116 Paris\n"},{"id":"95820","messageId":"491DD671.8070801@op5.se","threadId":"16324","inReplyTo":"200811142009.51803.fg@one2team.com","subject":"Re: [PATCH] sha1_file: make sure correct error is propagated","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2008-11-14T19:50:09Z","receivedAt":"2008-11-14T19:50:09Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Francis Galiegue wrote:\n> Le Friday 14 November 2008 20:05:19 Junio C Hamano, vous avez écrit :\n> [...]\n>>> \tfd = mkstemp(buffer);\n>>> -\tif (fd < 0 && dirlen && (errno != EPERM)) {\n>>> +\tif (fd < 0 && dirlen && (errno != EACCESS)) {\n>> Is this accepting the two as equivalents???\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n> Well, looking at mkdir(2), it says:\n> \n> EPERM The file system containing pathname does not support the \n> creation of directories.\n> \n> Hmm, err... git would fail at an earlier point anyway, wouldn't it? Even git \n> init would fail there.\n> \n\nNot necessarily. .git could be mounted erroneously from via a networked\nfilesystem but without write permissions. Yes, other things would fail\nthen too, but both EPERM and EACCESS are valid and possible return codes.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n"},{"id":"95824","messageId":"200811142108.46762.fg@one2team.net","threadId":"16324","inReplyTo":"491DD671.8070801@op5.se","subject":"Re: [PATCH] sha1_file: make sure correct error is propagated","fromName":"Francis Galiegue","fromEmail":"fg@one2team.net","sentAt":"2008-11-14T20:08:46Z","receivedAt":"2008-11-14T20:08:46Z","isPatch":true,"sender":{"key":"fg@one2team.net","avatar":null},"body":"Le Friday 14 November 2008 20:50:09 Andreas Ericsson, vous avez écrit :\n> Francis Galiegue wrote:\n> > Le Friday 14 November 2008 20:05:19 Junio C Hamano, vous avez écrit :\n> > [...]\n> >\n> >>> \tfd = mkstemp(buffer);\n> >>> -\tif (fd < 0 && dirlen && (errno != EPERM)) {\n> >>> +\tif (fd < 0 && dirlen && (errno != EACCESS)) {\n> >>\n> >> Is this accepting the two as equivalents???\n> >> --\n> >> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> >> the body of a message to majordomo@vger.kernel.org\n> >> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> >\n> > Well, looking at mkdir(2), it says:\n> >\n> > EPERM The file system containing pathname does not support the\n> > creation of directories.\n> >\n> > Hmm, err... git would fail at an earlier point anyway, wouldn't it? Even\n> > git init would fail there.\n>\n> Not necessarily. .git could be mounted erroneously from via a networked\n> filesystem but without write permissions. \n\nIn which case EACCESS would be returned anyway. There is quite a difference \nbetween EACCESS (Permission denied) and EPERM (operation not permitted).\n\nBasically, my understanding is that mkdir() will only return EPERM if the \nunderlying filesystem can not even CREATE directories on the filesystem. So, \nunless you are doing very bizarre things with your git repository, I cannot \nsee how you can even trigger an EPERM unless you asked for it.\n\n> Yes, other things would fail \n> then too, but both EPERM and EACCESS are valid and possible return codes.\n\nAnd so is ENOSPC, and so is EIO, and so is... It's endless. I think focus \nshould be made on the most common ones, and EACCESS _is_ such one. Others \njust aren't.\n\nThis is why I suggested replacing EPERM with EACCESS in the first place: \nEACCESS is by far the most common error code you will get (even root will get \nthat on a read-only filesystem, not EPERM).\n\n-- \nfge\n"},{"id":"95860","messageId":"7vr65d7dct.fsf@gitster.siamese.dyndns.org","threadId":"16324","inReplyTo":"200811142009.51803.fg@one2team.com","subject":"Re: [PATCH] sha1_file: make sure correct error is propagated","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-15T05:44:02Z","receivedAt":"2008-11-15T05:44:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Francis Galiegue writes:\n\n> Le Friday 14 November 2008 20:05:19 Junio C Hamano, vous avez écrit :\n> [...]\n>> > \tfd = mkstemp(buffer);\n>> > -\tif (fd < 0 && dirlen && (errno != EPERM)) {\n>> > +\tif (fd < 0 && dirlen && (errno != EACCESS)) {\n>>\n>> Is this accepting the two as equivalents???\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at http://vger.kernel.org/majordomo-info.html\n>\n> Well, looking at mkdir(2), it says:\n>\n> EPERM The file system containing pathname does not support the \n> creation of directories.\n>\n> Hmm, err... git would fail at an earlier point anyway, wouldn't it? Even git \n> init would fail there.\n\nActually, POSIX does not even talk about EPERM for mkdir(2), but that was\nnot my point. The code does something different from what the proposed\ncommit log message talks about. That was what bothered me.\n"},{"id":"95863","messageId":"1226730623.26334.2.camel@maia.lan","threadId":"16324","inReplyTo":"7vr65d7dct.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] sha1_file: make sure correct error is propagated","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2008-11-15T06:30:23Z","receivedAt":"2008-11-15T06:30:23Z","isPatch":true,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"On Fri, 2008-11-14 at 21:44 -0800, Junio C Hamano wrote:\n> Actually, POSIX does not even talk about EPERM for mkdir(2), but that was\n> not my point. The code does something different from what the proposed\n> commit log message talks about. That was what bothered me.\n\nMy wording was a little terse and confusing. Here's a new one;\n\nSubject: sha1_file.c: resolve confusion EACCESS vs EPERM\n\nEPERM or 'Operation not permitted' is an unlikely error from\nmkstemp(); test for EACCESS 'Access Denied' instead. Make the\nspecial branch which prints the error to the user nicely also\nunderstand EACCESS.\n"}]}