{"thread":{"id":"25403","subject":"[PATCH] log which temporary file could not be created","startedAt":"2010-10-09T20:17:51Z","lastAt":"2010-11-04T00:24:50Z","messageCount":12,"participants":["Arnout Engelen","Jonathan Nieder","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"153079","messageId":"20101009201751.GK9348@bzzt.net","threadId":"25403","inReplyTo":null,"subject":"[PATCH] log which temporary file could not be created","fromName":"Arnout Engelen","fromEmail":"arnouten@bzzt.net","sentAt":"2010-10-09T20:17:51Z","receivedAt":"2010-10-09T20:17:51Z","isPatch":true,"sender":{"key":"arnouten@bzzt.net","avatar":"https://avatars.githubusercontent.com/u/131856?v=4"},"body":"When creating a temporary file, log the template. Useful for making debugging \nproblems like file permission mistakes easier.\n\nSigned-off-by: Arnout Engelen <arnouten@bzzt.net>\n\n---\n wrapper.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex fd8ead3..68053cd 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -215,7 +215,7 @@ int xmkstemp(char *template)\n \n \tfd = mkstemp(template);\n \tif (fd < 0)\n-\t\tdie_errno(\"Unable to create temporary file\");\n+\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n \treturn fd;\n }\n \n@@ -225,7 +225,7 @@ int xmkstemp_mode(char *template, int mode)\n \n \tfd = git_mkstemp_mode(template, mode);\n \tif (fd < 0)\n-\t\tdie_errno(\"Unable to create temporary file\");\n+\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n \treturn fd;\n }\n \n-- \n1.7.2.3\n"},{"id":"153110","messageId":"20101010024124.GA20305@burratino","threadId":"25403","inReplyTo":"20101009201751.GK9348@bzzt.net","subject":"Re: [PATCH] log which temporary file could not be created","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-10T02:41:24Z","receivedAt":"2010-10-10T02:41:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nArnout Engelen wrote:\n\n> When creating a temporary file, log the template.\n\nSince mkstemp modifies its template, these hopefully would print the\nactual filename on errors.  Examples:\n\n\tfatal: Unable to create temporary file '.merge_file_Sc7R5c': File exists\n\tfatal: Unable to create temporary file 'newrepo/.git/tOWHcxk': No space left on device\n\nUnfortunately some mkstemp()s (such as that used to implement\ngit_mkstemp_mode()) clear the template on error, which would result in\n\n\tfatal: Unable to create temporary file '': Permission denied\n\nWhat should git do in this situation?\n\n> Useful for making debugging \n> problems like file permission mistakes easier.\n\nAgreed.  Thanks.\n"},{"id":"153124","messageId":"20101010103327.GO9348@bzzt.net","threadId":"25403","inReplyTo":"20101010024124.GA20305@burratino","subject":"Re: [PATCH] log which temporary file could not be created","fromName":"Arnout Engelen","fromEmail":"arnouten@bzzt.net","sentAt":"2010-10-10T10:33:28Z","receivedAt":"2010-10-10T10:33:28Z","isPatch":true,"sender":{"key":"arnouten@bzzt.net","avatar":"https://avatars.githubusercontent.com/u/131856?v=4"},"body":"On Sat, Oct 09, 2010 at 09:41:24PM -0500, Jonathan Nieder wrote:\n> Arnout Engelen wrote:\n> > When creating a temporary file, log the template.\n> \n> Since mkstemp modifies its template, these hopefully would print the\n> actual filename on errors.  Examples:\n> \n> \tfatal: Unable to create temporary file '.merge_file_Sc7R5c': File exists\n> \tfatal: Unable to create temporary file 'newrepo/.git/tOWHcxk': No space left on device\n\nPerhaps we should also log the current working directory when the temporary \nfilename is relative?\n\n> Unfortunately some mkstemp()s (such as that used to implement\n> git_mkstemp_mode()) clear the template on error, which would result in\n> \n> \tfatal: Unable to create temporary file '': Permission denied\n> \n> What should git do in this situation?\n\nPerhaps we should strdup() the template before mkstemp(), and log the \nstrdup()'ed template when the original has been cleared?\n\n\nArnout\n"},{"id":"153154","messageId":"20101010180909.GA12320@burratino","threadId":"25403","inReplyTo":"20101010103327.GO9348@bzzt.net","subject":"Re: [PATCH] log which temporary file could not be created","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-10T18:09:09Z","receivedAt":"2010-10-10T18:09:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Arnout Engelen wrote:\n> On Sat, Oct 09, 2010 at 09:41:24PM -0500, Jonathan Nieder wrote:\n\n>> \tfatal: Unable to create temporary file '.merge_file_Sc7R5c': File exists\n>> \tfatal: Unable to create temporary file 'newrepo/.git/tOWHcxk': No space left on device\n>\n> Perhaps we should also log the current working directory when the temporary \n> filename is relative?\n\nLet's step back for a moment.  Was there an example that prompted\nthis patch?  Were you aware of where git would be trying to create\nfiles in that example?  (I'm genuinely curious.)\n\nConverting the filename to an absolute path with make_absolute_path\nmight be useful, but I am not entirely sure it is worth the\ncomplication.\n\n>> \tfatal: Unable to create temporary file '': Permission denied\n>>\n>> What should git do in this situation?\n>\n> Perhaps we should strdup() the template before mkstemp(), and log the \n> strdup()'ed template when the original has been cleared?\n\nIs it be preferable for the filename to always have XXXXXX in it?  If\nso, then copying it into a temporary buffer (on-stack, preferably)\ncould be a sane solution, yes.\n\nJonathan\n"},{"id":"153155","messageId":"20101010185622.GX9348@bzzt.net","threadId":"25403","inReplyTo":"20101010180909.GA12320@burratino","subject":"Re: [PATCH] log which temporary file could not be created","fromName":"Arnout Engelen","fromEmail":"arnouten@bzzt.net","sentAt":"2010-10-10T18:56:22Z","receivedAt":"2010-10-10T18:56:22Z","isPatch":true,"sender":{"key":"arnouten@bzzt.net","avatar":"https://avatars.githubusercontent.com/u/131856?v=4"},"body":"On Sun, Oct 10, 2010 at 01:09:09PM -0500, Jonathan Nieder wrote:\n> Arnout Engelen wrote:\n> > On Sat, Oct 09, 2010 at 09:41:24PM -0500, Jonathan Nieder wrote:\n> >> \tfatal: Unable to create temporary file '.merge_file_Sc7R5c': File exists\n> >> \tfatal: Unable to create temporary file 'newrepo/.git/tOWHcxk': No space left on device\n> >\n> > Perhaps we should also log the current working directory when the temporary \n> > filename is relative?\n> \n> Let's step back for a moment.  Was there an example that prompted\n> this patch?  \n\nYes: I was trying to do an initial push to a sf.net git repo, and this gave me:\n\narnouten@bird:~/asdf/notion$ git push sourceforge master\nraboofje@notion.git.sourceforge.net's password: \nCounting objects: 21542, done.\nCompressing objects: 100% (4179/4179), done.\nfatal: Unable to create temporary file: Permission denied\nerror: pack-objects died of signal 13\nerror: pack-objects died with strange error\nerror: failed to push some refs to 'ssh://raboofje@notion.git.sourceforge.net/gitroot/notion/notion'\narnouten@bird:~/asdf/notion$\n\nThis seems to have been an error at the server side. I never did find out what\nexactly went wrong - I just re-initialized the repo using the shell access and\nall was fine after that.\n\n> Were you aware of where git would be trying to create\n> files in that example?  (I'm genuinely curious.)\n\nNo clue - but I'm not very into git 'internals' yet.\n\n> Converting the filename to an absolute path with make_absolute_path\n> might be useful, but I am not entirely sure it is worth the\n> complication.\n> \n> >> \tfatal: Unable to create temporary file '': Permission denied\n> >>\n> >> What should git do in this situation?\n> >\n> > Perhaps we should strdup() the template before mkstemp(), and log the \n> > strdup()'ed template when the original has been cleared?\n> \n> Is it be preferable for the filename to always have XXXXXX in it?  If\n> so, then copying it into a temporary buffer (on-stack, preferably)\n> could be a sane solution, yes.\n\nI'm not sure about 'always', but it'd be nice to have something to fall back \nto when the mkstemp clears the template.\n\n\nKind regards,\n\nArnout\n"},{"id":"153288","messageId":"7v8w24oygk.fsf@alter.siamese.dyndns.org","threadId":"25403","inReplyTo":"20101010180909.GA12320@burratino","subject":"Re: [PATCH] log which temporary file could not be created","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-12T03:56:59Z","receivedAt":"2010-10-12T03:56:59Z","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> Arnout Engelen wrote:\n>> On Sat, Oct 09, 2010 at 09:41:24PM -0500, Jonathan Nieder wrote:\n>\n>>> \tfatal: Unable to create temporary file '.merge_file_Sc7R5c': File exists\n>>> \tfatal: Unable to create temporary file 'newrepo/.git/tOWHcxk': No space left on device\n>>\n>> Perhaps we should also log the current working directory when the temporary \n>> filename is relative?\n>\n> Let's step back for a moment.  Was there an example that prompted\n> this patch?  Were you aware of where git would be trying to create\n> files in that example?  (I'm genuinely curious.)\n>\n> Converting the filename to an absolute path with make_absolute_path\n> might be useful, but I am not entirely sure it is worth the\n> complication.\n\nFor that matter, I am not sure if it is worth the strdup \"just in case it\nfails\" either.\n"},{"id":"153342","messageId":"20101012201946.GA15080@burratino","threadId":"25403","inReplyTo":"20101010185622.GX9348@bzzt.net","subject":"[PATCH] send-pack: avoid redundant \"pack-objects died with strange error\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-12T20:19:46Z","receivedAt":"2010-10-12T20:19:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Arnout Engelen wrote:\n\n> arnouten@bird:~/asdf/notion$ git push sourceforge master\n> raboofje@notion.git.sourceforge.net's password: \n> Counting objects: 21542, done.\n> Compressing objects: 100% (4179/4179), done.\n> fatal: Unable to create temporary file: Permission denied\n> error: pack-objects died of signal 13\n> error: pack-objects died with strange error\n> error: failed to push some refs to 'ssh://raboofje@notion.git.sourceforge.net/gitroot/notion/notion' arnouten@bird:~/asdf/notion$\n\nThat reminds me: the output seems more noisy than it needs to be, no?\n\n-- 8< --\nSubject: send-pack: avoid redundant \"pack-objects died with strange error\"\n\nSaying \"pack-objects died with strange error\" after \"pack-objects\ndied of signal 13\" seems kind of redundant.  The latter was\nintroduced when send-pack switched to the run-command API, which\nreports abnormal exits on behalf of the caller.\n\nNormal exits with nonzero status are not reported by run-command,\nthough.  Be more helpful in reporting them by including the exit\nstatus while at it.\n\nThe result should look something like this:\n\n\t$ git push sf master\n\tCounting objects: 21542, done.\n\tCompressing objects: 100% (4179/4179), done.\n\tfatal: Unable to create temporary file: Permission denied\n\terror: pack-objects died of signal 13\n\terror: failed to push some refs to 'ssh://sf.net/gitroot/project/project'\n\t$\n\nOr in the \"controlled exit\" case:\n\n\t[...]\n\terror: pack-objects died with status 128\n\terror: failed to push some refs to 'ssh://example.com/foo/bar'\n\t$\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n builtin/send-pack.c |    9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 481602d..51b722b 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -50,7 +50,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\tNULL,\n \t};\n \tstruct child_process po;\n-\tint i;\n+\tint i, status;\n \n \ti = 4;\n \tif (args->use_thin_pack)\n@@ -100,9 +100,10 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\tpo.out = -1;\n \t}\n \n-\tif (finish_command(&po))\n-\t\treturn error(\"pack-objects died with strange error\");\n-\treturn 0;\n+\tstatus = finish_command(&po);\n+\tif (status > 0)\n+\t\terror(\"pack-objects died with status %d\", status);\n+\treturn status;\n }\n \n static int receive_status(int in, struct ref *refs)\n-- \n1.7.2.3\n"},{"id":"153618","messageId":"20101016060423.GA26538@burratino","threadId":"25403","inReplyTo":"20101012201946.GA15080@burratino","subject":"[PATCH v2] send-pack: avoid redundant \"pack-objects died with strange error\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-16T06:04:23Z","receivedAt":"2010-10-16T06:04:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Saying \"pack-objects died with strange error\" after \"pack-objects\ndied of signal 13\" seems kind of redundant.  The latter was\nintroduced when send-pack switched to the run-command API, which\nreports abnormal exits on behalf of the caller.\n\nNormal exits with nonzero status are not reported by run-command,\nthough.  Be more helpful in reporting them by including the exit\nstatus while at it (and be sure to continue to return a value\nless than zero from pack_objects() for that case).\n\nThe result should look something like this:\n\n\t$ git push sf master\n\tCounting objects: 21542, done.\n\tCompressing objects: 100% (4179/4179), done.\n\tfatal: Unable to create temporary file: Permission denied\n\terror: pack-objects died of signal 13\n\terror: failed to push some refs to 'ssh://sf.net/gitroot/project/project'\n\t$\n\nOr in the \"controlled exit\" case:\n\n\t[...]\n\terror: pack-objects died with status 128\n\terror: failed to push some refs to 'ssh://example.com/foo/bar'\n\t$\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nJonathan Nieder wrote:\n\n> +++ b/builtin/send-pack.c\n> @@ -100,9 +100,10 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n>  \t\tpo.out = -1;\n>  \t}\n>  \n> -\tif (finish_command(&po))\n> -\t\treturn error(\"pack-objects died with strange error\");\n> +\tstatus = finish_command(&po);\n> +\tif (status > 0)\n> +\t\terror(\"pack-objects died with status %d\", status);\n> +\treturn status;\n\nCaller:\n\n\tif (pack_objects(...) < 0)\n\nSo making pack_objects return >0 where it used to return <0 would be\na regression. :(  An updated patch with the following minimal fix\nsquashed in follows.\n\n\tstatus = finish_command(&po);\n\tif (status > 0)\n -\t\terror(\"pack-objects died with status %d\", status);\n +\t\treturn error(\"pack-objects died with status %d\", status);\n\treturn status;\n  }\n\n builtin/send-pack.c |    9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 481602d..8854748 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -50,7 +50,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\tNULL,\n \t};\n \tstruct child_process po;\n-\tint i;\n+\tint i, status;\n \n \ti = 4;\n \tif (args->use_thin_pack)\n@@ -100,9 +100,10 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t\tpo.out = -1;\n \t}\n \n-\tif (finish_command(&po))\n-\t\treturn error(\"pack-objects died with strange error\");\n-\treturn 0;\n+\tstatus = finish_command(&po);\n+\tif (status > 0)\n+\t\treturn error(\"pack-objects died with status %d\", status);\n+\treturn status;\n }\n \n static int receive_status(int in, struct ref *refs)\n-- \n1.7.2.3\n"},{"id":"153621","messageId":"201010161125.33372.j6t@kdbg.org","threadId":"25403","inReplyTo":"20101016060423.GA26538@burratino","subject":"Re: [PATCH v2] send-pack: avoid redundant \"pack-objects died with strange error\"","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-10-16T09:25:33Z","receivedAt":"2010-10-16T09:25:33Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Samstag, 16. Oktober 2010, Jonathan Nieder wrote:\n> Saying \"pack-objects died with strange error\" after \"pack-objects\n> died of signal 13\" seems kind of redundant.  The latter was\n> introduced when send-pack switched to the run-command API, which\n> reports abnormal exits on behalf of the caller.\n>\n> Normal exits with nonzero status are not reported by run-command,\n> though.\n\nThe rationale for this is the assumption that before a program or script exits \nwith non-zero status, it will have reported an error.\n\n> Or in the \"controlled exit\" case:\n>\n> \t[...]\n> \terror: pack-objects died with status 128\n> \terror: failed to push some refs to 'ssh://example.com/foo/bar'\n\nI sense that this quote is not complete. Did you strip some error message by \nwriting just \"[...]\"?\n\n> -\tif (finish_command(&po))\n> -\t\treturn error(\"pack-objects died with strange error\");\n> -\treturn 0;\n> +\tstatus = finish_command(&po);\n> +\tif (status > 0)\n> +\t\treturn error(\"pack-objects died with status %d\", status);\n> +\treturn status;\n\nIdeally, this should really just be\n\n\tif (finish_command(&po))\n\t\treturn -1;\n\n-- Hannes\n"},{"id":"153630","messageId":"20101016170954.GA29619@burratino","threadId":"25403","inReplyTo":"201010161125.33372.j6t@kdbg.org","subject":"[PATCH v3] send-pack: avoid redundant \"pack-objects died with strange error\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-16T17:09:54Z","receivedAt":"2010-10-16T17:09:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Saying \"pack-objects died with strange error\" after \"pack-objects died\nof signal 13\" seems kind of redundant.  The latter message was\nintroduced when the run-command API changed to report abnormal exits\non behalf of the caller (v1.6.5-rc0~86^2~5, 2009-07-04).\n\nSimilarly, after a controlled pack-objects failure (detectable as a\nnormal exit with nonzero status), a \"died with strange error\" message\nwould be redundant next to the message from pack-objects itself.\n\nSo leave off the \"strange error\" messages.\n\nThe result should look something like this:\n\n\t$ git push sf master\n\tCounting objects: 21542, done.\n\tCompressing objects: 100% (4179/4179), done.\n\tfatal: Unable to create temporary file: Permission denied\n\terror: pack-objects died of signal 13\n\terror: failed to push some refs to 'ssh://sf.net/gitroot/project/project'\n\t$\n\nOr in the \"controlled exit\" case (contrived example):\n\n\t[...]\n\tfatal: delta size changed\n\terror: failed to push some refs to 'ssh://example.com/foo/bar'\n\t$\n\nImproved-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nOn Sat, Oct 16, 2010 at 11:25:33AM +0200, Johannes Sixt wrote:\n> On Samstag, 16. Oktober 2010, Jonathan Nieder wrote:\n\n>> \t[...]\n>> \terror: pack-objects died with status 128\n>> \terror: failed to push some refs to 'ssh://example.com/foo/bar'\n>\n> I sense that this quote is not complete. Did you strip some error message by \n> writing just \"[...]\"?\n\nThat was invented example output.\n\nGoogling for \"pack-objects died with strange error\" gives me [1], with\nv1.6.2:\n\n\t$ git push\n\tCounting objects: 68, done.\n\tDelta compression using 2 threads.\n\tCompressing objects: 100% (50/50), done.\n\tConnection to SERVER closed by remote host.\n\terror: pack-objects died with strange error\n\terror: failed to push some refs to 'SERVER:PROJECT.git'\n\nand [2], with v1.6.4.4:\n\n\tCounting objects: 4364, done.\n\tDelta compression using up to 2 threads.\n\tCompressing objects: 100% (4240/4240), done.\n\terror: pack-objects died with strange error | 44 KiB/s   \n\terror: failed to push some refs to 'mysite:main_site_repo'\n\nand some similar examples.  All involve git versions before\nv1.6.5-rc0~86^2~5 (run_command: report system call errors instead of\nreturning error codes, 2009-07-04) and after a triage turn out to be\nsignals.\n\nLet's go with your fix (thanks!).\n\n[1] http://stackoverflow.com/questions/718962/git-push-error-pack-objects-died-with-strange-error\n[2] http://stackoverflow.com/questions/1781013/git-error-failed-to-push-some-refs-pack-objects-died-with-strange-error\n\n builtin/send-pack.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 481602d..8aa3031 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -101,7 +101,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \t}\n \n \tif (finish_command(&po))\n-\t\treturn error(\"pack-objects died with strange error\");\n+\t\treturn -1;\n \treturn 0;\n }\n \n-- \n1.7.2.3\n"},{"id":"153703","messageId":"20101018092000.GH9348@bzzt.net","threadId":"25403","inReplyTo":"7v8w24oygk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] log which temporary file could not be created","fromName":"Arnout Engelen","fromEmail":"arnouten@bzzt.net","sentAt":"2010-10-18T09:20:00Z","receivedAt":"2010-10-18T09:20:00Z","isPatch":true,"sender":{"key":"arnouten@bzzt.net","avatar":"https://avatars.githubusercontent.com/u/131856?v=4"},"body":"On Mon, Oct 11, 2010 at 08:56:59PM -0700, Junio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n> >> On Sat, Oct 09, 2010 at 09:41:24PM -0500, Jonathan Nieder wrote:\n> >>> \tfatal: Unable to create temporary file '.merge_file_Sc7R5c': File exists\n> >>> \tfatal: Unable to create temporary file 'newrepo/.git/tOWHcxk': No space left on device\n> >\n> > Converting the filename to an absolute path with make_absolute_path\n> > might be useful, but I am not entirely sure it is worth the\n> > complication.\n> \n> I am not sure if it is worth the strdup \"just in case it fails\" either.\n\nI think it's valuable to give better error messages. A strdup/strncpy does not\ntake much resources (especially compared to creating the temporary file), and \nmakes the code only slightly less readable as the change is fairly local. \n\nBelow is an updated patch that:\n- copies the original template to the stack to be able to log it in case \n  mkstemp clears it\n- on failure, logs the modified template, if any, or the original one\n- logs the CWD when the template is relative\n- adds a test testing this gives sane output (and doesn't crash, etc)\n\nSigned-off-by: Arnout Engelen <arnouten@bzzt.net>\n\n---\n Makefile          |    1 +\n t/t0007-mktemp.sh |   16 ++++++++++++++++\n test-mktemp.c     |   25 +++++++++++++++++++++++++\n wrapper.c         |   30 ++++++++++++++++++++++++++----\n 4 files changed, 68 insertions(+), 4 deletions(-)\n create mode 100755 t/t0007-mktemp.sh\n create mode 100644 test-mktemp.c\n\ndiff --git a/Makefile b/Makefile\nindex 1f1ce04..30aafa2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -428,6 +428,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/t0007-mktemp.sh b/t/t0007-mktemp.sh\nnew file mode 100755\nindex 0000000..7abeeca\n--- /dev/null\n+++ b/t/t0007-mktemp.sh\n@@ -0,0 +1,16 @@\n+#!/bin/sh\n+\n+test_description='test creating temporary files'\n+. ./test-lib.sh\n+\n+check_errormsg() {\n+\ttest_expect_success \"$1\" \"\n+\t\ttest-mktemp $2 &>actual\n+\t\tgrep \\\"$3\\\" actual\n+\t\t\"\n+}\n+\n+check_errormsg \"Creating temporary file in nonexisting directory\" n \"Unable to create temporary file '/tmp/does/not/exist/test\"\n+check_errormsg \"Creating temporary file in invalid relative directory\" p \"at /\"\n+\n+test_done\ndiff --git a/test-mktemp.c b/test-mktemp.c\nnew file mode 100644\nindex 0000000..fad2908\n--- /dev/null\n+++ b/test-mktemp.c\n@@ -0,0 +1,25 @@\n+/*\n+ * test-mktemp.c: code to exercise the creation of temporary files\n+ */\n+#include \"wrapper.h\"\n+\n+int main(int argc, char *argv[])\n+{\n+\tif (argc != 2) {\n+\t\tfprintf(stderr, \"Expected 1 parameter defining the situation to test\");\n+\t\texit(-1);\n+\t}\n+\tswitch (argv[1][0]) {\n+\t\tcase 'n':\n+\t\t\t// temporary file in nonexistent directory\n+\t\t\txmkstemp(strdup(\"/tmp/does/not/exist/testXXXXXX\"));\n+\t\t\tbreak;\n+\t\tcase 'p':\n+\t\t\t// temporary file in directory where we have no write permissions\n+\t\t\txmkstemp(strdup(\"../../foo/testXXXXXX\"));\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tfprintf(stderr, \"No such test case: %s\\n\", argv[0]);\n+\t}\n+\treturn 0;\n+}\ndiff --git a/wrapper.c b/wrapper.c\nindex fd8ead3..b588e67 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -212,20 +212,42 @@ FILE *xfdopen(int fd, const char *mode)\n int xmkstemp(char *template)\n {\n \tint fd;\n+\tchar originalTemplate[255];\n+\tstrncpy(originalTemplate, template, 255);\n+\toriginalTemplate[254] = '\\0';\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 (strlen(template) == 0) {\n+\t\t\ttemplate = originalTemplate;\n+\t\t}\n+\t\tif (*template == '/') {\n+\t\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n+\t\t} else {\n+\t\t\tdie_errno(\"Unable to create temporary file '%s' at %s\", template, getcwd(NULL, 0));\n+\t\t}\n+\t}\n \treturn fd;\n }\n \n int xmkstemp_mode(char *template, int mode)\n {\n \tint fd;\n+\tchar originalTemplate[255];\n+\tstrncpy(originalTemplate, template, 255);\n+\toriginalTemplate[254] = '\\0';\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 (strlen(template) == 0) {\n+\t\t\ttemplate = originalTemplate;\n+\t\t}\n+\t\tif (*template == '/') {\n+\t\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n+\t\t} else {\n+\t\t\tdie_errno(\"Unable to create temporary file '%s' at %s\", template, getcwd(NULL, 0));\n+\t\t}\n+\t}\n \treturn fd;\n }\n \n-- \n1.7.2.3\n"},{"id":"155097","messageId":"20101104002450.GQ9348@bzzt.net","threadId":"25403","inReplyTo":"20101021205800.GC12685@burratino","subject":"Re: [PATCH] log which temporary file could not be created","fromName":"Arnout Engelen","fromEmail":"arnouten@bzzt.net","sentAt":"2010-11-04T00:24:50Z","receivedAt":"2010-11-04T00:24:50Z","isPatch":true,"sender":{"key":"arnouten@bzzt.net","avatar":"https://avatars.githubusercontent.com/u/131856?v=4"},"body":"Thanks for your feedback. \n\nModified patch all the way below ;)\n\nOn Thu, Oct 21, 2010 at 03:58:00PM -0500, Jonathan Nieder wrote:\n> A strdup() takes much more than a strncpy() last time I checked.  So I'm\n> glad to see you're using the latter. :)\n\n:)\n\n> Is exit(-1) portable?  I'd suggest using die() or usage().\n\nI think it is, but 'usage' is indeed neater. Done.\n\n> > +\t\t\t// temporary file in nonexistent directory\n> // does not work on C89-based compilers (sadly, there are still many\n> in wide use).  \n\nChanged to /* */\n\n> > +\t\t\txmkstemp(strdup(\"/tmp/does/not/exist/testXXXXXX\"));\n> \n> Probably better to take a parameter with the filename.\n\nDone\n\n> > +++ b/t/t0007-mktemp.sh\n> > +\t\ttest-mktemp $2 &>actual\n> \n> &> does not work on most shells.\n\nChanged to '2>', tested with zsh, ksh and bash.\n\n> Might be clearer to spell these out:\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> This probably would belong in t0070-fundamental.sh.\n\nDone and moved\n\n> > --- a/wrapper.c\n> > +++ b/wrapper.c\n> > @@ -212,20 +212,42 @@ FILE *xfdopen(int fd, const char *mode)\n> >  int xmkstemp(char *template)\n> >  {\n> >  \tint fd;\n> > +\tchar originalTemplate[255];\n> \n> The existing convention is to use lowercase, brief names, with\n> underscores separating words where appropraite.  See \"Chapter 4:\n> Naming\" from Documentation/CodingStyle in linux-2.6 for explanation.\n\nChanged to 'origtemplate'\n\n> > +\tstrncpy(originalTemplate, template, 255);\n> > +\toriginalTemplate[254] = '\\0';\n> \n> Maybe strlcpy() would be simpler?\n\nJep, thanks, didn't know that one. Changed.\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 (strlen(template) == 0) {\n> > +\t\t\ttemplate = originalTemplate;\n> > +\t\t}\n> \n> Useless use of strlen().  You probably wanted 'if (!template[0])'.\n\nI thought strlen() would be a bit neater/more readable - but changed.\n\n> Unnecessary brace clutter (see Documentation/CodingGuidelines for\n> explanation).\n\nChanged\n\n> > +\t\tif (*template == '/') {\n> > +\t\t\tdie_errno(\"Unable to create temporary file '%s'\", template);\n> > +\t\t} else {\n> > +\t\t\tdie_errno(\"Unable to create temporary file '%s' at %s\", template, getcwd(NULL, 0));\n> \n> Okay.  is_absolute_path() might be helpful on Windows.\n\nChanged.\n\nVerified the testcases still succeed. \n\nThe patch below:\n- copies the original template to the stack, to be able to log it in case\n  mkstemp clears it\n- on failure, logs the modified template, if any, or the original one\n- logs the CWD when the template is relative\n- adds a test testing this gives sane output (and doesn't crash, etc)\n\nSigned-off-by: Arnout Engelen <arnouten@bzzt.net>\n---\n Makefile               |    1 +\n t/t0070-fundamental.sh |   13 +++++++++++++\n test-mktemp.c          |   14 ++++++++++++++\n wrapper.c              |   26 ++++++++++++++++++++++----\n 4 files changed, 50 insertions(+), 4 deletions(-)\n create mode 100644 test-mktemp.c\n\ndiff --git a/Makefile b/Makefile\nindex 1f1ce04..30aafa2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -428,6 +428,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..30e266a\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 \"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 fd8ead3..b5f10d1 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -212,20 +212,38 @@ 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\", template, getcwd(NULL, 0));\n+\t}\n \treturn fd;\n }\n \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\", template, getcwd(NULL, 0));\n+\t}\n \treturn fd;\n }\n \n-- \n1.7.2.3\n"}]}