[PATCH] Improved error messages when temporary file creation fails

[PATCH] Improved error messages when temporary file creation fails

[Arnout Engelen]

This patch has been submitted/discussed before, but that version doesn't apply
cleanly to the newest git anymore, so here's an updated version. 

It improves diagnostic error messages when creating a temporary file fails.

Signed-off-by: Arnout Engelen <arnouten@bzzt.net>
---
 Makefile               |    1 +
 t/t0070-fundamental.sh |   13 +++++++++++++
 test-mktemp.c          |   16 ++++++++++++++++
 wrapper.c              |   29 +++++++++++++++++++++++++----
 wrapper.h              |    4 ++++
 5 files changed, 59 insertions(+), 4 deletions(-)
 create mode 100644 test-mktemp.c
 create mode 100644 wrapper.h

diff --git a/Makefile b/Makefile
index 7a5fb69..10cfab2 100644
--- a/Makefile
+++ b/Makefile
@@ -434,6 +434,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool
 TEST_PROGRAMS_NEED_X += test-svn-fe
 TEST_PROGRAMS_NEED_X += test-treap
 TEST_PROGRAMS_NEED_X += test-index-version
+TEST_PROGRAMS_NEED_X += test-mktemp
 
 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 680d7d6..9bee8bf 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -12,4 +12,17 @@ test_expect_success 'character classes (isspace, isalpha etc.)' '
 	test-ctype
 '
 
+test_expect_success 'mktemp to nonexistent directory prints filename' '
+	test_must_fail test-mktemp doesnotexist/testXXXXXX 2>err &&
+	grep "doesnotexist/test" err
+'
+
+test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' '
+	mkdir cannotwrite &&
+	chmod -w cannotwrite &&
+	test_when_finished "chmod +w cannotwrite" &&
+	test_must_fail test-mktemp cannotwrite/testXXXXXX 2>err &&
+	grep "cannotwrite/test" err
+'
+
 test_done
diff --git a/test-mktemp.c b/test-mktemp.c
new file mode 100644
index 0000000..d392fa7
--- /dev/null
+++ b/test-mktemp.c
@@ -0,0 +1,16 @@
+/*
+ * test-mktemp.c: code to exercise the creation of temporary files
+ */
+#include <string.h>
+#include "git-compat-util.h"
+#include "wrapper.h"
+
+int main(int argc, char *argv[])
+{
+	if (argc != 2) {
+		usage("Expected 1 parameter defining the temporary file template");
+	}
+	xmkstemp(strdup(argv[1]));
+
+	return 0;
+}
diff --git a/wrapper.c b/wrapper.c
index 4c1639f..6640c87 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -2,6 +2,7 @@
  * Various trivial helper wrappers around standard functions
  */
 #include "cache.h"
+#include "wrapper.h"
 
 static void do_nothing(size_t size)
 {
@@ -196,10 +197,20 @@ FILE *xfdopen(int fd, const char *mode)
 int xmkstemp(char *template)
 {
 	int fd;
+	char origtemplate[255];
+	strlcpy(origtemplate, template, 255);
 
 	fd = mkstemp(template);
-	if (fd < 0)
-		die_errno("Unable to create temporary file");
+	if (fd < 0) {
+		if (!template[0])
+			template = origtemplate;
+
+		if (is_absolute_path(template))
+			die_errno("Unable to create temporary file '%s'", template);
+		else
+			die_errno("Unable to create temporary file '%s' at %s", 
+				template, getcwd(NULL, 0));
+	}
 	return fd;
 }
 
@@ -319,10 +330,20 @@ int gitmkstemps(char *pattern, int suffix_len)
 int xmkstemp_mode(char *template, int mode)
 {
 	int fd;
+	char origtemplate[255];
+	strlcpy(origtemplate, template, 255);
 
 	fd = git_mkstemp_mode(template, mode);
-	if (fd < 0)
-		die_errno("Unable to create temporary file");
+	if (fd < 0) {
+		if (!template[0])
+			template = origtemplate;
+
+		if (is_absolute_path(template))
+			die_errno("Unable to create temporary file '%s'", template);
+		else
+			die_errno("Unable to create temporary file '%s' at %s", 
+				template, getcwd(NULL, 0));
+	}
 	return fd;
 }
 
diff --git a/wrapper.h b/wrapper.h
new file mode 100644
index 0000000..b06ff0d
--- /dev/null
+++ b/wrapper.h
@@ -0,0 +1,4 @@
+/*
+ * Various trivial helper wrappers around standard functions
+ */
+int xmkstemp(char *template);
-- 
1.7.2.3

Re: [PATCH] Improved error messages when temporary file creation fails

[Jonathan Nieder]

Arnout Engelen wrote:

> +			die_errno("Unable to create temporary file '%s' at %s", 
> +				template, getcwd(NULL, 0));

This is a Linux libc/glibc-specific extension, alas.  On other platforms
it would print "(null)" or segfault.

Here's some other assorted tweaks.  I didn't bother to find your old
patch in the mailing list archive to take a fuller change description
from.

Hope that helps,
Jonathan
---
diff --git a/test-mktemp.c b/test-mktemp.c
index d392fa7..2e3b134 100644
--- a/test-mktemp.c
+++ b/test-mktemp.c
@@ -7,10 +7,9 @@
 
 int main(int argc, char *argv[])
 {
-	if (argc != 2) {
+	if (argc != 2)
 		usage("Expected 1 parameter defining the temporary file template");
-	}
-	xmkstemp(strdup(argv[1]));
+	xmkstemp(xstrdup(argv[1]));
 
 	return 0;
 }
diff --git a/wrapper.c b/wrapper.c
index 6640c87..cb9c9ad 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -197,19 +197,18 @@ FILE *xfdopen(int fd, const char *mode)
 int xmkstemp(char *template)
 {
 	int fd;
-	char origtemplate[255];
-	strlcpy(origtemplate, template, 255);
+	char origtemplate[256];
+	strlcpy(origtemplate, template, sizeof(origtemplate));
 
 	fd = mkstemp(template);
 	if (fd < 0) {
+		int saved_errno = errno;
+
 		if (!template[0])
 			template = origtemplate;
-
-		if (is_absolute_path(template))
-			die_errno("Unable to create temporary file '%s'", template);
-		else
-			die_errno("Unable to create temporary file '%s' at %s", 
-				template, getcwd(NULL, 0));
+		template = make_nonrelative_path(template);
+		errno = saved_errno;
+		die_errno("Unable to create temporary file '%s'", template);
 	}
 	return fd;
 }
@@ -330,19 +329,18 @@ int gitmkstemps(char *pattern, int suffix_len)
 int xmkstemp_mode(char *template, int mode)
 {
 	int fd;
-	char origtemplate[255];
-	strlcpy(origtemplate, template, 255);
+	char origtemplate[256];
+	strlcpy(origtemplate, template, sizeof(origtemplate));
 
 	fd = git_mkstemp_mode(template, mode);
 	if (fd < 0) {
+		int saved_errno = errno;
+
 		if (!template[0])
 			template = origtemplate;
-
-		if (is_absolute_path(template))
-			die_errno("Unable to create temporary file '%s'", template);
-		else
-			die_errno("Unable to create temporary file '%s' at %s", 
-				template, getcwd(NULL, 0));
+		template = make_nonrelative_path(template);
+		errno = saved_errno;
+		die_errno("Unable to create temporary file '%s'", template);
 	}
 	return fd;
 }

Re: [PATCH] Improved error messages when temporary file creation fails

[Junio C Hamano]

Arnout Engelen <arnouten@bzzt.net> writes:

> This patch has been submitted/discussed before, but that version doesn't apply
> cleanly to the newest git anymore, so here's an updated version. 

Thanks, but that comes after "---", no?

> It improves diagnostic error messages when creating a temporary file fails.
>
> Signed-off-by: Arnout Engelen <arnouten@bzzt.net>
> ---

> diff --git a/wrapper.c b/wrapper.c
> index 4c1639f..6640c87 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -2,6 +2,7 @@
>   * Various trivial helper wrappers around standard functions
>   */
>  #include "cache.h"
> +#include "wrapper.h"
>  
>  static void do_nothing(size_t size)
>  {
> @@ -196,10 +197,20 @@ FILE *xfdopen(int fd, const char *mode)
>  int xmkstemp(char *template)
>  {
>  	int fd;
> +	char origtemplate[255];
> +	strlcpy(origtemplate, template, 255);

Why "255"?

It may happen to be sufficiently large for the current callers, but what
provisions if any are made to help the compiler or the runtime protect us
from new and broken callers?  Use of strlcpy() there hides the issue from
the runtime by avoiding segfault, but it actively harms us by making the
code silently behave incorrectly without segfaulting, no?

> diff --git a/wrapper.h b/wrapper.h
> new file mode 100644
> index 0000000..b06ff0d
> --- /dev/null
> +++ b/wrapper.h
> @@ -0,0 +1,4 @@
> +/*
> + * Various trivial helper wrappers around standard functions
> + */
> +int xmkstemp(char *template);

Somewhat questionable...

Why doesn't this say "extern"?
What happend to another copy of this line in git-compat-util.h?  IOW, what
protects this and the other one from drifting apart?
Why doesn't users include wrapper.h but only wrapper.c?
Why yet another header is needed only for this function and nothing else?
Why isn't this protected with the standard #ifndef .../#define .../#endif?

Re: [PATCH] Improved error messages when temporary file creation fails

[Arnout Engelen]

Thanks to you and Jonathan again for the feedback.

On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:
> > +	char origtemplate[255];
> > +	strlcpy(origtemplate, template, 255);
> 
> Why "255"?

Random - 'i had to choose something'.

> It may happen to be sufficiently large for the current callers, but what
> provisions if any are made to help the compiler or the runtime protect us
> from new and broken callers?  Use of strlcpy() there hides the issue from
> the runtime by avoiding segfault, but it actively harms us by making the
> code silently behave incorrectly without segfaulting, no?

Only in a small way: when a bigger template is encountered and the mkstemp 
call succeeds, there is no problem. Only when xmkstemp fails *and* clears the
template, the diagnostic error message shows a truncated version of the 
original.

I *could* dynamically allocate space for the original template string, but that
would mean I'd need to do a malloc() instead of putting the buffer on the stack
like this, and free() it afterwards. I'm not too concerned about the 
performance hit here (presumably the I/O that comes with creating and using 
the temporary file here is orders of magnitude slower than that malloc() 
anyway), but it would also make the code a bit less easy to read.

What do you think would be preferable here, a simple fixed-length buffer on the
stack that might cause a truncated error message or a dynamically-allocated 
one that makes the code somewhat more complicated?

> > +++ b/wrapper.h
> 
> Somewhat questionable...

Agreed, this whole file is unneeded and, well, wrong anyway. 

I'll remove wrapper.h and apply Jonathan's improvements some time this week, 
unless of course someone beats me to it :). 


Kind regards,

Arnout

Re: [PATCH] Improved error messages when temporary file creation fails

[Jakub Narebski]

Arnout Engelen wrote:
> On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:

>>> +   char origtemplate[255];
>>> +   strlcpy(origtemplate, template, 255);
>> 
>> Why "255"?
> 
> Random - 'i had to choose something'.
> 
>> It may happen to be sufficiently large for the current callers, but what
>> provisions if any are made to help the compiler or the runtime protect us
>> from new and broken callers?  Use of strlcpy() there hides the issue from
>> the runtime by avoiding segfault, but it actively harms us by making the
>> code silently behave incorrectly without segfaulting, no?
> 
> Only in a small way: when a bigger template is encountered and the mkstemp 
> call succeeds, there is no problem. Only when xmkstemp fails *and* clears the
> template, the diagnostic error message shows a truncated version of the 
> original.
> 
> I *could* dynamically allocate space for the original template string, but that
> would mean I'd need to do a malloc() instead of putting the buffer on the stack
> like this, and free() it afterwards. I'm not too concerned about the 
> performance hit here (presumably the I/O that comes with creating and using 
> the temporary file here is orders of magnitude slower than that malloc() 
> anyway), but it would also make the code a bit less easy to read.
> 
> What do you think would be preferable here, a simple fixed-length buffer on the
> stack that might cause a truncated error message or a dynamically-allocated 
> one that makes the code somewhat more complicated?

Why not use PATH_MAX instead of 255?

P.S. I'm sory for cutting up CC list...
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] Improved error messages when temporary file creation fails

[Jonathan Nieder]

Jakub Narebski wrote:
> Arnout Engelen wrote:
>> On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:

>>>> +   char origtemplate[255];
>>>> +   strlcpy(origtemplate, template, 255);
>>> 
>>> Why "255"?
[...]
> Why not use PATH_MAX instead of 255?

The advantage I can see to 256 is a small speed-up in the "no errors"
code path.  Since the I/O would tend to be much more costly and PATH_MAX
is self explanatory, using PATH_MAX does seem cleaner.

I know you all are aware of this already; just thought I'd mention it
while forwarding the original message to Arnout. :)

Jonathan

Re: [PATCH] Improved error messages when temporary file creation fails

[Junio C Hamano]

Arnout Engelen <arnouten@bzzt.net> writes:

> Thanks to you and Jonathan again for the feedback.
>
> On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:
>> > +	char origtemplate[255];
>> > +	strlcpy(origtemplate, template, 255);
>> 
>> Why "255"?
>
> Random - 'i had to choose something'.
>
>> It may happen to be sufficiently large for the current callers, but what
>> provisions if any are made to help the compiler or the runtime protect us
>> from new and broken callers?  Use of strlcpy() there hides the issue from
>> the runtime by avoiding segfault, but it actively harms us by making the
>> code silently behave incorrectly without segfaulting, no?
>
> Only in a small way: when a bigger template is encountered and the mkstemp 
> call succeeds, there is no problem. Only when xmkstemp fails *and* clears the
> template, the diagnostic error message shows a truncated version of the 
> original.

Ah, ok, it seems that I misread the patch.  This copy you are making is
not used to actually construct the filename used for creating the
temporary file, so there is no risk the function misbehaving; we would
just give a truncated error report, which is no worse than what we have
been giving the users anyway.

Re: [PATCH] Improved error messages when temporary file creation fails

[Arnout Engelen]

Updated version of the patch, taking into account the feedback from this 
thread. (the use of a fixed-length buffer to temporarily store the template
for the error path is now sort of moot since make_nonrelative_path doesn't 
allow filenames larger than PATH_MAX anyway)

Signed-off-by: Arnout Engelen <arnouten@bzzt.net>
---
 Makefile               |    1 +
 t/t0070-fundamental.sh |   13 +++++++++++++
 test-mktemp.c          |   15 +++++++++++++++
 wrapper.c              |   32 ++++++++++++++++++++++++++++----
 4 files changed, 57 insertions(+), 4 deletions(-)
 create mode 100644 test-mktemp.c

diff --git a/Makefile b/Makefile
index 57d9c65..03a51cb 100644
--- a/Makefile
+++ b/Makefile
@@ -434,6 +434,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool
 TEST_PROGRAMS_NEED_X += test-svn-fe
 TEST_PROGRAMS_NEED_X += test-treap
 TEST_PROGRAMS_NEED_X += test-index-version
+TEST_PROGRAMS_NEED_X += test-mktemp
 
 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 680d7d6..9bee8bf 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -12,4 +12,17 @@ test_expect_success 'character classes (isspace, isalpha etc.)' '
 	test-ctype
 '
 
+test_expect_success 'mktemp to nonexistent directory prints filename' '
+	test_must_fail test-mktemp doesnotexist/testXXXXXX 2>err &&
+	grep "doesnotexist/test" err
+'
+
+test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' '
+	mkdir cannotwrite &&
+	chmod -w cannotwrite &&
+	test_when_finished "chmod +w cannotwrite" &&
+	test_must_fail test-mktemp cannotwrite/testXXXXXX 2>err &&
+	grep "cannotwrite/test" err
+'
+
 test_done
diff --git a/test-mktemp.c b/test-mktemp.c
new file mode 100644
index 0000000..00fdd78
--- /dev/null
+++ b/test-mktemp.c
@@ -0,0 +1,15 @@
+/*
+ * test-mktemp.c: code to exercise the creation of temporary files
+ */
+#include <string.h>
+#include "git-compat-util.h"
+
+int main(int argc, char *argv[])
+{
+	if (argc != 2)
+		usage("Expected 1 parameter defining the temporary file template");
+
+	xmkstemp(strdup(argv[1]));
+
+	return 0;
+}
diff --git a/wrapper.c b/wrapper.c
index 4c1639f..9af40ee 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -196,10 +196,22 @@ FILE *xfdopen(int fd, const char *mode)
 int xmkstemp(char *template)
 {
 	int fd;
+	char origtemplate[PATH_MAX];
+	strlcpy(origtemplate, template, sizeof(origtemplate));
 
 	fd = mkstemp(template);
-	if (fd < 0)
-		die_errno("Unable to create temporary file");
+	if (fd < 0) {
+		int saved_errno = errno;
+		const char * nonrelative_template;
+
+		if (!template[0])
+			template = origtemplate;
+
+		nonrelative_template = make_nonrelative_path(template);
+		errno = saved_errno;
+		die_errno("Unable to create temporary file '%s'", 
+			nonrelative_template);
+	}
 	return fd;
 }
 
@@ -319,10 +331,22 @@ int gitmkstemps(char *pattern, int suffix_len)
 int xmkstemp_mode(char *template, int mode)
 {
 	int fd;
+	char origtemplate[PATH_MAX];
+	strlcpy(origtemplate, template, sizeof(origtemplate));
 
 	fd = git_mkstemp_mode(template, mode);
-	if (fd < 0)
-		die_errno("Unable to create temporary file");
+	if (fd < 0) {
+		int saved_errno = errno;
+		const char * nonrelative_template;
+
+		if (!template[0])
+			template = origtemplate;
+
+		nonrelative_template = make_nonrelative_path(template);
+		errno = saved_errno;
+		die_errno("Unable to create temporary file '%s'", 
+			nonrelative_template);
+	}
 	return fd;
 }
 
-- 
1.7.2.3

Re: [PATCH] Improved error messages when temporary file creation fails

[Jonathan Nieder]

Hi again,

Arnout Engelen wrote:

> Updated version of the patch

Almost there.  Some remaining nits from my pov:

> , taking into account the feedback from this 
> thread.
[...]

This text above the "---" becomes part of the log message when a patch
is committed to git.git, so it is best to make it self-contained.  The
usual advice is "describe the current behavior, why the proposed
behavior is better, and then how the proposed behavior is achieved."

> --- /dev/null
> +++ b/test-mktemp.c
> @@ -0,0 +1,15 @@
> +/*
> + * test-mktemp.c: code to exercise the creation of temporary files
> + */
> +#include <string.h>
> +#include "git-compat-util.h"

git-compat-util.h takes care of portably including system headers in
the right order.  (For example, #include-ing <string.h> before
setting _POSIX_SOURCE will cause some symbols not to be defined in
_other_ headers on some operating systems, iirc.)  I'd suggest
removing the redundant #include <string.h>.

See Documentation/CodingGuidelines.

> +int main(int argc, char *argv[])
> +{
> +	if (argc != 2)
> +		usage("Expected 1 parameter defining the temporary file template");
> +
> +	xmkstemp(strdup(argv[1]));

Why not xstrdup(), which diagnoses allocation failures?

> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -196,10 +196,22 @@ FILE *xfdopen(int fd, const char *mode)
>  int xmkstemp(char *template)
>  {
>  	int fd;
> +	char origtemplate[PATH_MAX];
> +	strlcpy(origtemplate, template, sizeof(origtemplate));
>  
>  	fd = mkstemp(template);
> -	if (fd < 0)
> -		die_errno("Unable to create temporary file");
> +	if (fd < 0) {
> +		int saved_errno = errno;
> +		const char * nonrelative_template;

It would be more usual not to include a space between the '*' and
'nonrelative_template'.

Thanks for your perseverance.

Re: [PATCH] Improved error messages when temporary file creation fails

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi again,
>
> Arnout Engelen wrote:
>
>> Updated version of the patch
>
> Almost there.  Some remaining nits from my pov:
>
>> , taking into account the feedback from this 
>> thread.
> [...]
>
> This text above the "---" becomes part of the log message when a patch
> is committed to git.git, so it is best to make it self-contained.  The
> usual advice is "describe the current behavior, why the proposed
> behavior is better, and then how the proposed behavior is achieved."

Thanks, Jonathan.

The important part of self-containedness is that people who are reading
the resulting commit and the history that contains it should not have to
unnecessarily refer to outside resources, especially the previous rounds
that weren't satisfactory.  The comparison with previous rounds however is
a very valuable resource for reviewers on the mailing list, so don't
remove what you wrote there, but move it below "---" lines.

Re: [PATCH] Improved error messages when temporary file creation fails

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi again,
>
> Arnout Engelen wrote:
>
>> Updated version of the patch
>
> Almost there.  Some remaining nits from my pov:
>
>> , taking into account the feedback from this 
>> thread.
> [...]
>
> This text above the "---" becomes part of the log message when a patch
> is committed to git.git, so it is best to make it self-contained.  The
> usual advice is "describe the current behavior, why the proposed
> behavior is better, and then how the proposed behavior is achieved."

Thanks, Jonathan.

The important part of self-containedness is that people who are reading
the resulting commit and the history that contains it should not have to
unnecessarily refer to outside resources, especially the previous rounds
that weren't satisfactory.  The comparison with previous rounds however is
a very valuable resource for reviewers on the mailing list, so don't
remove what you wrote there, but move it below "---" lines.

Re: [PATCH] Improved error messages when temporary file creation fails

[Arnout Engelen]

Improve error messages when creating a temporary file fails.

Before, when creating a temporary file failed, a generic 'Unable to create 
temporary file' message was printed. In some cases this could lead to 
confusion as to which directory should be checked for correct permissions etc.

This patch adds the template for the temporary filename to the error message,
converting it to an absolute path if needed. A test verifies that the template
is indeed printed when pointing to a nonexistent or unwritable directory.

(a copy of the original template is made in case mkstemp clears the template)

Signed-off-by: Arnout Engelen <arnouten@bzzt.net>
---
The use of a fixed-length buffer to temporarily store the template
for the error path is now sort of moot since make_nonrelative_path doesn't
allow filenames larger than PATH_MAX anyway.

Some more tweaks based on Jonathan's feedback: xstrdup, #include of 
git-compat-util.h in the unittest and for whitespace around the '*' in 
pointer declarations.

 Makefile               |    1 +
 t/t0070-fundamental.sh |   13 +++++++++++++
 test-mktemp.c          |   14 ++++++++++++++
 wrapper.c              |   32 ++++++++++++++++++++++++++++----
 4 files changed, 56 insertions(+), 4 deletions(-)
 create mode 100644 test-mktemp.c

diff --git a/Makefile b/Makefile
index 57d9c65..03a51cb 100644
--- a/Makefile
+++ b/Makefile
@@ -434,6 +434,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool
 TEST_PROGRAMS_NEED_X += test-svn-fe
 TEST_PROGRAMS_NEED_X += test-treap
 TEST_PROGRAMS_NEED_X += test-index-version
+TEST_PROGRAMS_NEED_X += test-mktemp
 
 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 680d7d6..9bee8bf 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -12,4 +12,17 @@ test_expect_success 'character classes (isspace, isalpha etc.)' '
 	test-ctype
 '
 
+test_expect_success 'mktemp to nonexistent directory prints filename' '
+	test_must_fail test-mktemp doesnotexist/testXXXXXX 2>err &&
+	grep "doesnotexist/test" err
+'
+
+test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' '
+	mkdir cannotwrite &&
+	chmod -w cannotwrite &&
+	test_when_finished "chmod +w cannotwrite" &&
+	test_must_fail test-mktemp cannotwrite/testXXXXXX 2>err &&
+	grep "cannotwrite/test" err
+'
+
 test_done
diff --git a/test-mktemp.c b/test-mktemp.c
new file mode 100644
index 0000000..c8c5421
--- /dev/null
+++ b/test-mktemp.c
@@ -0,0 +1,14 @@
+/*
+ * test-mktemp.c: code to exercise the creation of temporary files
+ */
+#include "git-compat-util.h"
+
+int main(int argc, char *argv[])
+{
+	if (argc != 2)
+		usage("Expected 1 parameter defining the temporary file template");
+
+	xmkstemp(xstrdup(argv[1]));
+
+	return 0;
+}
diff --git a/wrapper.c b/wrapper.c
index 4c1639f..0c208c2 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -196,10 +196,22 @@ FILE *xfdopen(int fd, const char *mode)
 int xmkstemp(char *template)
 {
 	int fd;
+	char origtemplate[PATH_MAX];
+	strlcpy(origtemplate, template, sizeof(origtemplate));
 
 	fd = mkstemp(template);
-	if (fd < 0)
-		die_errno("Unable to create temporary file");
+	if (fd < 0) {
+		int saved_errno = errno;
+		const char *nonrelative_template;
+
+		if (!template[0])
+			template = origtemplate;
+
+		nonrelative_template = make_nonrelative_path(template);
+		errno = saved_errno;
+		die_errno("Unable to create temporary file '%s'", 
+			nonrelative_template);
+	}
 	return fd;
 }
 
@@ -319,10 +331,22 @@ int gitmkstemps(char *pattern, int suffix_len)
 int xmkstemp_mode(char *template, int mode)
 {
 	int fd;
+	char origtemplate[PATH_MAX];
+	strlcpy(origtemplate, template, sizeof(origtemplate));
 
 	fd = git_mkstemp_mode(template, mode);
-	if (fd < 0)
-		die_errno("Unable to create temporary file");
+	if (fd < 0) {
+		int saved_errno = errno;
+		const char *nonrelative_template;
+
+		if (!template[0])
+			template = origtemplate;
+
+		nonrelative_template = make_nonrelative_path(template);
+		errno = saved_errno;
+		die_errno("Unable to create temporary file '%s'", 
+			nonrelative_template);
+	}
 	return fd;
 }
 
-- 
1.7.2.3