{"thread":{"id":"25985","subject":"[PATCH] Improved error messages when temporary file creation fails","startedAt":"2010-12-07T18:16:33Z","lastAt":"2010-12-18T21:28:00Z","messageCount":12,"participants":["Arnout Engelen","Jonathan Nieder","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"157509","messageId":"20101207181633.GF25767@bzzt.net","threadId":"25985","inReplyTo":null,"subject":"[PATCH] Improved error messages when temporary file creation fails","fromName":"Arnout Engelen","fromEmail":"arnouten@bzzt.net","sentAt":"2010-12-07T18:16:33Z","receivedAt":"2010-12-07T18:16:33Z","isPatch":true,"sender":{"key":"arnouten@bzzt.net","avatar":"https://avatars.githubusercontent.com/u/131856?v=4"},"body":"This patch has been submitted/discussed before, but that version doesn't apply\ncleanly to the newest git anymore, so here's an updated version. \n\nIt improves diagnostic error messages when creating a temporary file fails.\n\nSigned-off-by: Arnout Engelen <arnouten@bzzt.net>\n---\n Makefile               |    1 +\n t/t0070-fundamental.sh |   13 +++++++++++++\n test-mktemp.c          |   16 ++++++++++++++++\n wrapper.c              |   29 +++++++++++++++++++++++++----\n wrapper.h              |    4 ++++\n 5 files changed, 59 insertions(+), 4 deletions(-)\n create mode 100644 test-mktemp.c\n create mode 100644 wrapper.h\n\ndiff --git a/Makefile b/Makefile\nindex 7a5fb69..10cfab2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -434,6 +434,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool\n TEST_PROGRAMS_NEED_X += test-svn-fe\n TEST_PROGRAMS_NEED_X += test-treap\n TEST_PROGRAMS_NEED_X += test-index-version\n+TEST_PROGRAMS_NEED_X += test-mktemp\n \n TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n \ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 680d7d6..9bee8bf 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -12,4 +12,17 @@ test_expect_success 'character classes (isspace, isalpha etc.)' '\n \ttest-ctype\n '\n \n+test_expect_success 'mktemp to nonexistent directory prints filename' '\n+\ttest_must_fail test-mktemp doesnotexist/testXXXXXX 2>err &&\n+\tgrep \"doesnotexist/test\" err\n+'\n+\n+test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' '\n+\tmkdir cannotwrite &&\n+\tchmod -w cannotwrite &&\n+\ttest_when_finished \"chmod +w cannotwrite\" &&\n+\ttest_must_fail test-mktemp cannotwrite/testXXXXXX 2>err &&\n+\tgrep \"cannotwrite/test\" err\n+'\n+\n test_done\ndiff --git a/test-mktemp.c b/test-mktemp.c\nnew file mode 100644\nindex 0000000..d392fa7\n--- /dev/null\n+++ b/test-mktemp.c\n@@ -0,0 +1,16 @@\n+/*\n+ * test-mktemp.c: code to exercise the creation of temporary files\n+ */\n+#include <string.h>\n+#include \"git-compat-util.h\"\n+#include \"wrapper.h\"\n+\n+int main(int argc, char *argv[])\n+{\n+\tif (argc != 2) {\n+\t\tusage(\"Expected 1 parameter defining the temporary file template\");\n+\t}\n+\txmkstemp(strdup(argv[1]));\n+\n+\treturn 0;\n+}\ndiff --git a/wrapper.c b/wrapper.c\nindex 4c1639f..6640c87 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -2,6 +2,7 @@\n  * Various trivial helper wrappers around standard functions\n  */\n #include \"cache.h\"\n+#include \"wrapper.h\"\n \n static void do_nothing(size_t size)\n {\n@@ -196,10 +197,20 @@ FILE *xfdopen(int fd, const char *mode)\n int xmkstemp(char *template)\n {\n \tint fd;\n+\tchar origtemplate[255];\n+\tstrlcpy(origtemplate, template, 255);\n \n \tfd = mkstemp(template);\n-\tif (fd < 0)\n-\t\tdie_errno(\"Unable to create temporary file\");\n+\tif (fd < 0) {\n+\t\tif (!template[0])\n+\t\t\ttemplate = origtemplate;\n+\n+\t\tif (is_absolute_path(template))\n+\t\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n+\t\telse\n+\t\t\tdie_errno(\"Unable to create temporary file '%s' at %s\", \n+\t\t\t\ttemplate, getcwd(NULL, 0));\n+\t}\n \treturn fd;\n }\n \n@@ -319,10 +330,20 @@ int gitmkstemps(char *pattern, int suffix_len)\n int xmkstemp_mode(char *template, int mode)\n {\n \tint fd;\n+\tchar origtemplate[255];\n+\tstrlcpy(origtemplate, template, 255);\n \n \tfd = git_mkstemp_mode(template, mode);\n-\tif (fd < 0)\n-\t\tdie_errno(\"Unable to create temporary file\");\n+\tif (fd < 0) {\n+\t\tif (!template[0])\n+\t\t\ttemplate = origtemplate;\n+\n+\t\tif (is_absolute_path(template))\n+\t\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n+\t\telse\n+\t\t\tdie_errno(\"Unable to create temporary file '%s' at %s\", \n+\t\t\t\ttemplate, getcwd(NULL, 0));\n+\t}\n \treturn fd;\n }\n \ndiff --git a/wrapper.h b/wrapper.h\nnew file mode 100644\nindex 0000000..b06ff0d\n--- /dev/null\n+++ b/wrapper.h\n@@ -0,0 +1,4 @@\n+/*\n+ * Various trivial helper wrappers around standard functions\n+ */\n+int xmkstemp(char *template);\n-- \n1.7.2.3\n"},{"id":"157522","messageId":"20101207190920.GA22587@burratino","threadId":"25985","inReplyTo":"20101207181633.GF25767@bzzt.net","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-07T19:09:20Z","receivedAt":"2010-12-07T19:09:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Arnout Engelen wrote:\n\n> +\t\t\tdie_errno(\"Unable to create temporary file '%s' at %s\", \n> +\t\t\t\ttemplate, getcwd(NULL, 0));\n\nThis is a Linux libc/glibc-specific extension, alas.  On other platforms\nit would print \"(null)\" or segfault.\n\nHere's some other assorted tweaks.  I didn't bother to find your old\npatch in the mailing list archive to take a fuller change description\nfrom.\n\nHope that helps,\nJonathan\n---\ndiff --git a/test-mktemp.c b/test-mktemp.c\nindex d392fa7..2e3b134 100644\n--- a/test-mktemp.c\n+++ b/test-mktemp.c\n@@ -7,10 +7,9 @@\n \n int main(int argc, char *argv[])\n {\n-\tif (argc != 2) {\n+\tif (argc != 2)\n \t\tusage(\"Expected 1 parameter defining the temporary file template\");\n-\t}\n-\txmkstemp(strdup(argv[1]));\n+\txmkstemp(xstrdup(argv[1]));\n \n \treturn 0;\n }\ndiff --git a/wrapper.c b/wrapper.c\nindex 6640c87..cb9c9ad 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -197,19 +197,18 @@ FILE *xfdopen(int fd, const char *mode)\n int xmkstemp(char *template)\n {\n \tint fd;\n-\tchar origtemplate[255];\n-\tstrlcpy(origtemplate, template, 255);\n+\tchar origtemplate[256];\n+\tstrlcpy(origtemplate, template, sizeof(origtemplate));\n \n \tfd = mkstemp(template);\n \tif (fd < 0) {\n+\t\tint saved_errno = errno;\n+\n \t\tif (!template[0])\n \t\t\ttemplate = origtemplate;\n-\n-\t\tif (is_absolute_path(template))\n-\t\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n-\t\telse\n-\t\t\tdie_errno(\"Unable to create temporary file '%s' at %s\", \n-\t\t\t\ttemplate, getcwd(NULL, 0));\n+\t\ttemplate = make_nonrelative_path(template);\n+\t\terrno = saved_errno;\n+\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n \t}\n \treturn fd;\n }\n@@ -330,19 +329,18 @@ int gitmkstemps(char *pattern, int suffix_len)\n int xmkstemp_mode(char *template, int mode)\n {\n \tint fd;\n-\tchar origtemplate[255];\n-\tstrlcpy(origtemplate, template, 255);\n+\tchar origtemplate[256];\n+\tstrlcpy(origtemplate, template, sizeof(origtemplate));\n \n \tfd = git_mkstemp_mode(template, mode);\n \tif (fd < 0) {\n+\t\tint saved_errno = errno;\n+\n \t\tif (!template[0])\n \t\t\ttemplate = origtemplate;\n-\n-\t\tif (is_absolute_path(template))\n-\t\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n-\t\telse\n-\t\t\tdie_errno(\"Unable to create temporary file '%s' at %s\", \n-\t\t\t\ttemplate, getcwd(NULL, 0));\n+\t\ttemplate = make_nonrelative_path(template);\n+\t\terrno = saved_errno;\n+\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n \t}\n \treturn fd;\n }\n"},{"id":"157538","messageId":"7v62v5paj2.fsf@alter.siamese.dyndns.org","threadId":"25985","inReplyTo":"20101207181633.GF25767@bzzt.net","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-07T20:56:17Z","receivedAt":"2010-12-07T20:56:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Arnout Engelen <arnouten@bzzt.net> writes:\n\n> This patch has been submitted/discussed before, but that version doesn't apply\n> cleanly to the newest git anymore, so here's an updated version. \n\nThanks, but that comes after \"---\", no?\n\n> It improves diagnostic error messages when creating a temporary file fails.\n>\n> Signed-off-by: Arnout Engelen <arnouten@bzzt.net>\n> ---\n\n> diff --git a/wrapper.c b/wrapper.c\n> index 4c1639f..6640c87 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -2,6 +2,7 @@\n>   * Various trivial helper wrappers around standard functions\n>   */\n>  #include \"cache.h\"\n> +#include \"wrapper.h\"\n>  \n>  static void do_nothing(size_t size)\n>  {\n> @@ -196,10 +197,20 @@ FILE *xfdopen(int fd, const char *mode)\n>  int xmkstemp(char *template)\n>  {\n>  \tint fd;\n> +\tchar origtemplate[255];\n> +\tstrlcpy(origtemplate, template, 255);\n\nWhy \"255\"?\n\nIt may happen to be sufficiently large for the current callers, but what\nprovisions if any are made to help the compiler or the runtime protect us\nfrom new and broken callers?  Use of strlcpy() there hides the issue from\nthe runtime by avoiding segfault, but it actively harms us by making the\ncode silently behave incorrectly without segfaulting, no?\n\n> diff --git a/wrapper.h b/wrapper.h\n> new file mode 100644\n> index 0000000..b06ff0d\n> --- /dev/null\n> +++ b/wrapper.h\n> @@ -0,0 +1,4 @@\n> +/*\n> + * Various trivial helper wrappers around standard functions\n> + */\n> +int xmkstemp(char *template);\n\nSomewhat questionable...\n\nWhy doesn't this say \"extern\"?\nWhat happend to another copy of this line in git-compat-util.h?  IOW, what\nprotects this and the other one from drifting apart?\nWhy doesn't users include wrapper.h but only wrapper.c?\nWhy yet another header is needed only for this function and nothing else?\nWhy isn't this protected with the standard #ifndef .../#define .../#endif?\n"},{"id":"157545","messageId":"20101207212041.GG25767@bzzt.net","threadId":"25985","inReplyTo":"7v62v5paj2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Arnout Engelen","fromEmail":"arnouten@bzzt.net","sentAt":"2010-12-07T21:20:41Z","receivedAt":"2010-12-07T21:20:41Z","isPatch":true,"sender":{"key":"arnouten@bzzt.net","avatar":"https://avatars.githubusercontent.com/u/131856?v=4"},"body":"Thanks to you and Jonathan again for the feedback.\n\nOn Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:\n> > +\tchar origtemplate[255];\n> > +\tstrlcpy(origtemplate, template, 255);\n> \n> Why \"255\"?\n\nRandom - 'i had to choose something'.\n\n> It may happen to be sufficiently large for the current callers, but what\n> provisions if any are made to help the compiler or the runtime protect us\n> from new and broken callers?  Use of strlcpy() there hides the issue from\n> the runtime by avoiding segfault, but it actively harms us by making the\n> code silently behave incorrectly without segfaulting, no?\n\nOnly in a small way: when a bigger template is encountered and the mkstemp \ncall succeeds, there is no problem. Only when xmkstemp fails *and* clears the\ntemplate, the diagnostic error message shows a truncated version of the \noriginal.\n\nI *could* dynamically allocate space for the original template string, but that\nwould mean I'd need to do a malloc() instead of putting the buffer on the stack\nlike this, and free() it afterwards. I'm not too concerned about the \nperformance hit here (presumably the I/O that comes with creating and using \nthe temporary file here is orders of magnitude slower than that malloc() \nanyway), but it would also make the code a bit less easy to read.\n\nWhat do you think would be preferable here, a simple fixed-length buffer on the\nstack that might cause a truncated error message or a dynamically-allocated \none that makes the code somewhat more complicated?\n\n> > +++ b/wrapper.h\n> \n> Somewhat questionable...\n\nAgreed, this whole file is unneeded and, well, wrong anyway. \n\nI'll remove wrapper.h and apply Jonathan's improvements some time this week, \nunless of course someone beats me to it :). \n\n\nKind regards,\n\nArnout\n"},{"id":"157557","messageId":"idmhjl$n1j$1@dough.gmane.org","threadId":"25985","inReplyTo":"20101207212041.GG25767@bzzt.net","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-07T23:56:39Z","receivedAt":"2010-12-07T23:56:39Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Arnout Engelen wrote:\n> On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:\n\n>>> +   char origtemplate[255];\n>>> +   strlcpy(origtemplate, template, 255);\n>> \n>> Why \"255\"?\n> \n> Random - 'i had to choose something'.\n> \n>> It may happen to be sufficiently large for the current callers, but what\n>> provisions if any are made to help the compiler or the runtime protect us\n>> from new and broken callers?  Use of strlcpy() there hides the issue from\n>> the runtime by avoiding segfault, but it actively harms us by making the\n>> code silently behave incorrectly without segfaulting, no?\n> \n> Only in a small way: when a bigger template is encountered and the mkstemp \n> call succeeds, there is no problem. Only when xmkstemp fails *and* clears the\n> template, the diagnostic error message shows a truncated version of the \n> original.\n> \n> I *could* dynamically allocate space for the original template string, but that\n> would mean I'd need to do a malloc() instead of putting the buffer on the stack\n> like this, and free() it afterwards. I'm not too concerned about the \n> performance hit here (presumably the I/O that comes with creating and using \n> the temporary file here is orders of magnitude slower than that malloc() \n> anyway), but it would also make the code a bit less easy to read.\n> \n> What do you think would be preferable here, a simple fixed-length buffer on the\n> stack that might cause a truncated error message or a dynamically-allocated \n> one that makes the code somewhat more complicated?\n\nWhy not use PATH_MAX instead of 255?\n\nP.S. I'm sory for cutting up CC list...\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"157558","messageId":"20101208001236.GA25527@burratino","threadId":"25985","inReplyTo":"idmhjl$n1j$1@dough.gmane.org","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-08T00:12:36Z","receivedAt":"2010-12-08T00:12:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jakub Narebski wrote:\n> Arnout Engelen wrote:\n>> On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:\n\n>>>> +   char origtemplate[255];\n>>>> +   strlcpy(origtemplate, template, 255);\n>>> \n>>> Why \"255\"?\n[...]\n> Why not use PATH_MAX instead of 255?\n\nThe advantage I can see to 256 is a small speed-up in the \"no errors\"\ncode path.  Since the I/O would tend to be much more costly and PATH_MAX\nis self explanatory, using PATH_MAX does seem cleaner.\n\nI know you all are aware of this already; just thought I'd mention it\nwhile forwarding the original message to Arnout. :)\n\nJonathan\n"},{"id":"157566","messageId":"7vpqtdm3a4.fsf@alter.siamese.dyndns.org","threadId":"25985","inReplyTo":"20101207212041.GG25767@bzzt.net","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-08T02:01:07Z","receivedAt":"2010-12-08T02:01:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Arnout Engelen <arnouten@bzzt.net> writes:\n\n> Thanks to you and Jonathan again for the feedback.\n>\n> On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:\n>> > +\tchar origtemplate[255];\n>> > +\tstrlcpy(origtemplate, template, 255);\n>> \n>> Why \"255\"?\n>\n> Random - 'i had to choose something'.\n>\n>> It may happen to be sufficiently large for the current callers, but what\n>> provisions if any are made to help the compiler or the runtime protect us\n>> from new and broken callers?  Use of strlcpy() there hides the issue from\n>> the runtime by avoiding segfault, but it actively harms us by making the\n>> code silently behave incorrectly without segfaulting, no?\n>\n> Only in a small way: when a bigger template is encountered and the mkstemp \n> call succeeds, there is no problem. Only when xmkstemp fails *and* clears the\n> template, the diagnostic error message shows a truncated version of the \n> original.\n\nAh, ok, it seems that I misread the patch.  This copy you are making is\nnot used to actually construct the filename used for creating the\ntemporary file, so there is no risk the function misbehaving; we would\njust give a truncated error report, which is no worse than what we have\nbeen giving the users anyway.\n"},{"id":"158327","messageId":"20101218165514.GT25767@bzzt.net","threadId":"25985","inReplyTo":"20101207181633.GF25767@bzzt.net","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Arnout Engelen","fromEmail":"arnouten@bzzt.net","sentAt":"2010-12-18T16:55:14Z","receivedAt":"2010-12-18T16:55:14Z","isPatch":true,"sender":{"key":"arnouten@bzzt.net","avatar":"https://avatars.githubusercontent.com/u/131856?v=4"},"body":"Updated version of the patch, taking into account the feedback from this \nthread. (the use of a fixed-length buffer to temporarily store the template\nfor the error path is now sort of moot since make_nonrelative_path doesn't \nallow filenames larger than PATH_MAX anyway)\n\nSigned-off-by: Arnout Engelen <arnouten@bzzt.net>\n---\n Makefile               |    1 +\n t/t0070-fundamental.sh |   13 +++++++++++++\n test-mktemp.c          |   15 +++++++++++++++\n wrapper.c              |   32 ++++++++++++++++++++++++++++----\n 4 files changed, 57 insertions(+), 4 deletions(-)\n create mode 100644 test-mktemp.c\n\ndiff --git a/Makefile b/Makefile\nindex 57d9c65..03a51cb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -434,6 +434,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool\n TEST_PROGRAMS_NEED_X += test-svn-fe\n TEST_PROGRAMS_NEED_X += test-treap\n TEST_PROGRAMS_NEED_X += test-index-version\n+TEST_PROGRAMS_NEED_X += test-mktemp\n \n TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n \ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 680d7d6..9bee8bf 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -12,4 +12,17 @@ test_expect_success 'character classes (isspace, isalpha etc.)' '\n \ttest-ctype\n '\n \n+test_expect_success 'mktemp to nonexistent directory prints filename' '\n+\ttest_must_fail test-mktemp doesnotexist/testXXXXXX 2>err &&\n+\tgrep \"doesnotexist/test\" err\n+'\n+\n+test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' '\n+\tmkdir cannotwrite &&\n+\tchmod -w cannotwrite &&\n+\ttest_when_finished \"chmod +w cannotwrite\" &&\n+\ttest_must_fail test-mktemp cannotwrite/testXXXXXX 2>err &&\n+\tgrep \"cannotwrite/test\" err\n+'\n+\n test_done\ndiff --git a/test-mktemp.c b/test-mktemp.c\nnew file mode 100644\nindex 0000000..00fdd78\n--- /dev/null\n+++ b/test-mktemp.c\n@@ -0,0 +1,15 @@\n+/*\n+ * test-mktemp.c: code to exercise the creation of temporary files\n+ */\n+#include <string.h>\n+#include \"git-compat-util.h\"\n+\n+int main(int argc, char *argv[])\n+{\n+\tif (argc != 2)\n+\t\tusage(\"Expected 1 parameter defining the temporary file template\");\n+\n+\txmkstemp(strdup(argv[1]));\n+\n+\treturn 0;\n+}\ndiff --git a/wrapper.c b/wrapper.c\nindex 4c1639f..9af40ee 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -196,10 +196,22 @@ FILE *xfdopen(int fd, const char *mode)\n int xmkstemp(char *template)\n {\n \tint fd;\n+\tchar origtemplate[PATH_MAX];\n+\tstrlcpy(origtemplate, template, sizeof(origtemplate));\n \n \tfd = mkstemp(template);\n-\tif (fd < 0)\n-\t\tdie_errno(\"Unable to create temporary file\");\n+\tif (fd < 0) {\n+\t\tint saved_errno = errno;\n+\t\tconst char * nonrelative_template;\n+\n+\t\tif (!template[0])\n+\t\t\ttemplate = origtemplate;\n+\n+\t\tnonrelative_template = make_nonrelative_path(template);\n+\t\terrno = saved_errno;\n+\t\tdie_errno(\"Unable to create temporary file '%s'\", \n+\t\t\tnonrelative_template);\n+\t}\n \treturn fd;\n }\n \n@@ -319,10 +331,22 @@ int gitmkstemps(char *pattern, int suffix_len)\n int xmkstemp_mode(char *template, int mode)\n {\n \tint fd;\n+\tchar origtemplate[PATH_MAX];\n+\tstrlcpy(origtemplate, template, sizeof(origtemplate));\n \n \tfd = git_mkstemp_mode(template, mode);\n-\tif (fd < 0)\n-\t\tdie_errno(\"Unable to create temporary file\");\n+\tif (fd < 0) {\n+\t\tint saved_errno = errno;\n+\t\tconst char * nonrelative_template;\n+\n+\t\tif (!template[0])\n+\t\t\ttemplate = origtemplate;\n+\n+\t\tnonrelative_template = make_nonrelative_path(template);\n+\t\terrno = saved_errno;\n+\t\tdie_errno(\"Unable to create temporary file '%s'\", \n+\t\t\tnonrelative_template);\n+\t}\n \treturn fd;\n }\n \n-- \n1.7.2.3\n"},{"id":"158329","messageId":"20101218200516.GA10031@burratino","threadId":"25985","inReplyTo":"20101218165514.GT25767@bzzt.net","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-18T20:05:16Z","receivedAt":"2010-12-18T20:05:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nArnout Engelen wrote:\n\n> Updated version of the patch\n\nAlmost there.  Some remaining nits from my pov:\n\n> , taking into account the feedback from this \n> thread.\n[...]\n\nThis text above the \"---\" becomes part of the log message when a patch\nis committed to git.git, so it is best to make it self-contained.  The\nusual advice is \"describe the current behavior, why the proposed\nbehavior is better, and then how the proposed behavior is achieved.\"\n\n> --- /dev/null\n> +++ b/test-mktemp.c\n> @@ -0,0 +1,15 @@\n> +/*\n> + * test-mktemp.c: code to exercise the creation of temporary files\n> + */\n> +#include <string.h>\n> +#include \"git-compat-util.h\"\n\ngit-compat-util.h takes care of portably including system headers in\nthe right order.  (For example, #include-ing <string.h> before\nsetting _POSIX_SOURCE will cause some symbols not to be defined in\n_other_ headers on some operating systems, iirc.)  I'd suggest\nremoving the redundant #include <string.h>.\n\nSee Documentation/CodingGuidelines.\n\n> +int main(int argc, char *argv[])\n> +{\n> +\tif (argc != 2)\n> +\t\tusage(\"Expected 1 parameter defining the temporary file template\");\n> +\n> +\txmkstemp(strdup(argv[1]));\n\nWhy not xstrdup(), which diagnoses allocation failures?\n\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -196,10 +196,22 @@ FILE *xfdopen(int fd, const char *mode)\n>  int xmkstemp(char *template)\n>  {\n>  \tint fd;\n> +\tchar origtemplate[PATH_MAX];\n> +\tstrlcpy(origtemplate, template, sizeof(origtemplate));\n>  \n>  \tfd = mkstemp(template);\n> -\tif (fd < 0)\n> -\t\tdie_errno(\"Unable to create temporary file\");\n> +\tif (fd < 0) {\n> +\t\tint saved_errno = errno;\n> +\t\tconst char * nonrelative_template;\n\nIt would be more usual not to include a space between the '*' and\n'nonrelative_template'.\n\nThanks for your perseverance.\n"},{"id":"158332","messageId":"7vaak2hkqj.fsf@alter.siamese.dyndns.org","threadId":"25985","inReplyTo":"20101218200516.GA10031@burratino","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-18T20:47:00Z","receivedAt":"2010-12-18T20:47:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Hi again,\n>\n> Arnout Engelen wrote:\n>\n>> Updated version of the patch\n>\n> Almost there.  Some remaining nits from my pov:\n>\n>> , taking into account the feedback from this \n>> thread.\n> [...]\n>\n> This text above the \"---\" becomes part of the log message when a patch\n> is committed to git.git, so it is best to make it self-contained.  The\n> usual advice is \"describe the current behavior, why the proposed\n> behavior is better, and then how the proposed behavior is achieved.\"\n\nThanks, Jonathan.\n\nThe important part of self-containedness is that people who are reading\nthe resulting commit and the history that contains it should not have to\nunnecessarily refer to outside resources, especially the previous rounds\nthat weren't satisfactory.  The comparison with previous rounds however is\na very valuable resource for reviewers on the mailing list, so don't\nremove what you wrote there, but move it below \"---\" lines.\n"},{"id":"158333","messageId":"7v8vzmhkps.fsf@alter.siamese.dyndns.org","threadId":"25985","inReplyTo":"20101218200516.GA10031@burratino","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-18T20:47:27Z","receivedAt":"2010-12-18T20:47:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Hi again,\n>\n> Arnout Engelen wrote:\n>\n>> Updated version of the patch\n>\n> Almost there.  Some remaining nits from my pov:\n>\n>> , taking into account the feedback from this \n>> thread.\n> [...]\n>\n> This text above the \"---\" becomes part of the log message when a patch\n> is committed to git.git, so it is best to make it self-contained.  The\n> usual advice is \"describe the current behavior, why the proposed\n> behavior is better, and then how the proposed behavior is achieved.\"\n\nThanks, Jonathan.\n\nThe important part of self-containedness is that people who are reading\nthe resulting commit and the history that contains it should not have to\nunnecessarily refer to outside resources, especially the previous rounds\nthat weren't satisfactory.  The comparison with previous rounds however is\na very valuable resource for reviewers on the mailing list, so don't\nremove what you wrote there, but move it below \"---\" lines.\n"},{"id":"158338","messageId":"20101218212800.GB29089@bzzt.net","threadId":"25985","inReplyTo":"20101218200516.GA10031@burratino","subject":"Re: [PATCH] Improved error messages when temporary file creation fails","fromName":"Arnout Engelen","fromEmail":"arnouten@bzzt.net","sentAt":"2010-12-18T21:28:00Z","receivedAt":"2010-12-18T21:28:00Z","isPatch":true,"sender":{"key":"arnouten@bzzt.net","avatar":"https://avatars.githubusercontent.com/u/131856?v=4"},"body":"Improve error messages when creating a temporary file fails.\n\nBefore, when creating a temporary file failed, a generic 'Unable to create \ntemporary file' message was printed. In some cases this could lead to \nconfusion as to which directory should be checked for correct permissions etc.\n\nThis patch adds the template for the temporary filename to the error message,\nconverting it to an absolute path if needed. A test verifies that the template\nis indeed printed when pointing to a nonexistent or unwritable directory.\n\n(a copy of the original template is made in case mkstemp clears the template)\n\nSigned-off-by: Arnout Engelen <arnouten@bzzt.net>\n---\nThe use of a fixed-length buffer to temporarily store the template\nfor the error path is now sort of moot since make_nonrelative_path doesn't\nallow filenames larger than PATH_MAX anyway.\n\nSome more tweaks based on Jonathan's feedback: xstrdup, #include of \ngit-compat-util.h in the unittest and for whitespace around the '*' in \npointer declarations.\n\n Makefile               |    1 +\n t/t0070-fundamental.sh |   13 +++++++++++++\n test-mktemp.c          |   14 ++++++++++++++\n wrapper.c              |   32 ++++++++++++++++++++++++++++----\n 4 files changed, 56 insertions(+), 4 deletions(-)\n create mode 100644 test-mktemp.c\n\ndiff --git a/Makefile b/Makefile\nindex 57d9c65..03a51cb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -434,6 +434,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool\n TEST_PROGRAMS_NEED_X += test-svn-fe\n TEST_PROGRAMS_NEED_X += test-treap\n TEST_PROGRAMS_NEED_X += test-index-version\n+TEST_PROGRAMS_NEED_X += test-mktemp\n \n TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n \ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 680d7d6..9bee8bf 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -12,4 +12,17 @@ test_expect_success 'character classes (isspace, isalpha etc.)' '\n \ttest-ctype\n '\n \n+test_expect_success 'mktemp to nonexistent directory prints filename' '\n+\ttest_must_fail test-mktemp doesnotexist/testXXXXXX 2>err &&\n+\tgrep \"doesnotexist/test\" err\n+'\n+\n+test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' '\n+\tmkdir cannotwrite &&\n+\tchmod -w cannotwrite &&\n+\ttest_when_finished \"chmod +w cannotwrite\" &&\n+\ttest_must_fail test-mktemp cannotwrite/testXXXXXX 2>err &&\n+\tgrep \"cannotwrite/test\" err\n+'\n+\n test_done\ndiff --git a/test-mktemp.c b/test-mktemp.c\nnew file mode 100644\nindex 0000000..c8c5421\n--- /dev/null\n+++ b/test-mktemp.c\n@@ -0,0 +1,14 @@\n+/*\n+ * test-mktemp.c: code to exercise the creation of temporary files\n+ */\n+#include \"git-compat-util.h\"\n+\n+int main(int argc, char *argv[])\n+{\n+\tif (argc != 2)\n+\t\tusage(\"Expected 1 parameter defining the temporary file template\");\n+\n+\txmkstemp(xstrdup(argv[1]));\n+\n+\treturn 0;\n+}\ndiff --git a/wrapper.c b/wrapper.c\nindex 4c1639f..0c208c2 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -196,10 +196,22 @@ FILE *xfdopen(int fd, const char *mode)\n int xmkstemp(char *template)\n {\n \tint fd;\n+\tchar origtemplate[PATH_MAX];\n+\tstrlcpy(origtemplate, template, sizeof(origtemplate));\n \n \tfd = mkstemp(template);\n-\tif (fd < 0)\n-\t\tdie_errno(\"Unable to create temporary file\");\n+\tif (fd < 0) {\n+\t\tint saved_errno = errno;\n+\t\tconst char *nonrelative_template;\n+\n+\t\tif (!template[0])\n+\t\t\ttemplate = origtemplate;\n+\n+\t\tnonrelative_template = make_nonrelative_path(template);\n+\t\terrno = saved_errno;\n+\t\tdie_errno(\"Unable to create temporary file '%s'\", \n+\t\t\tnonrelative_template);\n+\t}\n \treturn fd;\n }\n \n@@ -319,10 +331,22 @@ int gitmkstemps(char *pattern, int suffix_len)\n int xmkstemp_mode(char *template, int mode)\n {\n \tint fd;\n+\tchar origtemplate[PATH_MAX];\n+\tstrlcpy(origtemplate, template, sizeof(origtemplate));\n \n \tfd = git_mkstemp_mode(template, mode);\n-\tif (fd < 0)\n-\t\tdie_errno(\"Unable to create temporary file\");\n+\tif (fd < 0) {\n+\t\tint saved_errno = errno;\n+\t\tconst char *nonrelative_template;\n+\n+\t\tif (!template[0])\n+\t\t\ttemplate = origtemplate;\n+\n+\t\tnonrelative_template = make_nonrelative_path(template);\n+\t\terrno = saved_errno;\n+\t\tdie_errno(\"Unable to create temporary file '%s'\", \n+\t\t\tnonrelative_template);\n+\t}\n \treturn fd;\n }\n \n-- \n1.7.2.3\n"}]}