Re: [PATCH] log which temporary file could not be created

[Arnout Engelen <arnouten@bzzt.net>]

On Mon, Oct 11, 2010 at 08:56:59PM -0700, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
> >> On Sat, Oct 09, 2010 at 09:41:24PM -0500, Jonathan Nieder wrote:
> >>> 	fatal: Unable to create temporary file '.merge_file_Sc7R5c': File exists
> >>> 	fatal: Unable to create temporary file 'newrepo/.git/tOWHcxk': No space left on device
> >
> > Converting the filename to an absolute path with make_absolute_path
> > might be useful, but I am not entirely sure it is worth the
> > complication.
> 
> I am not sure if it is worth the strdup "just in case it fails" either.

I think it's valuable to give better error messages. A strdup/strncpy does not
take much resources (especially compared to creating the temporary file), and 
makes the code only slightly less readable as the change is fairly local. 

Below is an updated patch that:
- copies the original template to the stack to be able to log it in case 
  mkstemp clears it
- on failure, logs the modified template, if any, or the original one
- logs the CWD when the template is relative
- adds a test testing this gives sane output (and doesn't crash, etc)

Signed-off-by: Arnout Engelen <arnouten@bzzt.net>

---
 Makefile          |    1 +
 t/t0007-mktemp.sh |   16 ++++++++++++++++
 test-mktemp.c     |   25 +++++++++++++++++++++++++
 wrapper.c         |   30 ++++++++++++++++++++++++++----
 4 files changed, 68 insertions(+), 4 deletions(-)
 create mode 100755 t/t0007-mktemp.sh
 create mode 100644 test-mktemp.c

diff --git a/Makefile b/Makefile
index 1f1ce04..30aafa2 100644
--- a/Makefile
+++ b/Makefile
@@ -428,6 +428,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/t0007-mktemp.sh b/t/t0007-mktemp.sh
new file mode 100755
index 0000000..7abeeca
--- /dev/null
+++ b/t/t0007-mktemp.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+test_description='test creating temporary files'
+. ./test-lib.sh
+
+check_errormsg() {
+	test_expect_success "$1" "
+		test-mktemp $2 &>actual
+		grep \"$3\" actual
+		"
+}
+
+check_errormsg "Creating temporary file in nonexisting directory" n "Unable to create temporary file '/tmp/does/not/exist/test"
+check_errormsg "Creating temporary file in invalid relative directory" p "at /"
+
+test_done
diff --git a/test-mktemp.c b/test-mktemp.c
new file mode 100644
index 0000000..fad2908
--- /dev/null
+++ b/test-mktemp.c
@@ -0,0 +1,25 @@
+/*
+ * test-mktemp.c: code to exercise the creation of temporary files
+ */
+#include "wrapper.h"
+
+int main(int argc, char *argv[])
+{
+	if (argc != 2) {
+		fprintf(stderr, "Expected 1 parameter defining the situation to test");
+		exit(-1);
+	}
+	switch (argv[1][0]) {
+		case 'n':
+			// temporary file in nonexistent directory
+			xmkstemp(strdup("/tmp/does/not/exist/testXXXXXX"));
+			break;
+		case 'p':
+			// temporary file in directory where we have no write permissions
+			xmkstemp(strdup("../../foo/testXXXXXX"));
+			break;
+		default:
+			fprintf(stderr, "No such test case: %s\n", argv[0]);
+	}
+	return 0;
+}
diff --git a/wrapper.c b/wrapper.c
index fd8ead3..b588e67 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -212,20 +212,42 @@ FILE *xfdopen(int fd, const char *mode)
 int xmkstemp(char *template)
 {
 	int fd;
+	char originalTemplate[255];
+	strncpy(originalTemplate, template, 255);
+	originalTemplate[254] = '\0';
 
 	fd = mkstemp(template);
-	if (fd < 0)
-		die_errno("Unable to create temporary file");
+	if (fd < 0) {
+		if (strlen(template) == 0) {
+			template = originalTemplate;
+		}
+		if (*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;
 }
 
 int xmkstemp_mode(char *template, int mode)
 {
 	int fd;
+	char originalTemplate[255];
+	strncpy(originalTemplate, template, 255);
+	originalTemplate[254] = '\0';
 
 	fd = git_mkstemp_mode(template, mode);
-	if (fd < 0)
-		die_errno("Unable to create temporary file");
+	if (fd < 0) {
+		if (strlen(template) == 0) {
+			template = originalTemplate;
+		}
+		if (*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;
 }
 
-- 
1.7.2.3