Merge branch 'jk/trace-fixup'

Various small fixups to the "GIT_TRACE" facility.

* jk/trace-fixup:
  trace: do not fall back to stderr
  write_or_die: drop write_or_whine_pipe()
  trace: disable key after write error
  trace: correct variable name in write() error message
  trace: cosmetic fixes for error messages
  trace: use warning() for printing trace errors
  trace: stop using write_or_whine_pipe()
  trace: handle NULL argument in trace_disable()

cache.h

@@ -1756,7 +1756,6 @@ extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
 
 extern void write_or_die(int fd, const void *buf, size_t count);
-extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);
 extern void fsync_or_die(int fd, const char *);
 
 extern ssize_t read_in_full(int fd, void *buf, size_t count);

trace.c

@@ -25,15 +25,25 @@
 #include "cache.h"
 #include "quote.h"
 
+/*
+ * "Normalize" a key argument by converting NULL to our trace_default,
+ * and otherwise passing through the value. All caller-facing functions
+ * should normalize their inputs in this way, though most get it
+ * for free by calling get_trace_fd() (directly or indirectly).
+ */
+static void normalize_trace_key(struct trace_key **key)
+{
+	static struct trace_key trace_default = { "GIT_TRACE" };
+	if (!*key)
+		*key = &trace_default;
+}
+
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
 {
-	static struct trace_key trace_default = { "GIT_TRACE" };
 	const char *trace;
 
-	/* use default "GIT_TRACE" if NULL */
+	normalize_trace_key(&key);
-	if (!key)
-		key = &trace_default;
 
 	/* don't open twice */
 	if (key->initialized)
@@ -51,22 +61,19 @@ static int get_trace_fd(struct trace_key *key)
 	else if (is_absolute_path(trace)) {
 		int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);
 		if (fd == -1) {
-			fprintf(stderr,
+			warning("could not open '%s' for tracing: %s",
-				"Could not open '%s' for tracing: %s\n"
-				"Defaulting to tracing on stderr...\n",
 				trace, strerror(errno));
-			key->fd = STDERR_FILENO;
+			trace_disable(key);
 		} else {
 			key->fd = fd;
 			key->need_close = 1;
 		}
 	} else {
-		fprintf(stderr, "What does '%s' for %s mean?\n"
+		warning("unknown trace value for '%s': %s\n"
-			"If you want to trace into a file, then please set "
+			"         If you want to trace into a file, then please set %s\n"
-			"%s to an absolute pathname (starting with /).\n"
+			"         to an absolute pathname (starting with /)",
-			"Defaulting to tracing on stderr...\n",
+			key->key, trace, key->key);
-			trace, key->key, key->key);
+		trace_disable(key);
-		key->fd = STDERR_FILENO;
 	}
 
 	key->initialized = 1;
@@ -75,6 +82,8 @@ static int get_trace_fd(struct trace_key *key)
 
 void trace_disable(struct trace_key *key)
 {
+	normalize_trace_key(&key);
+
 	if (key->need_close)
 		close(key->fd);
 	key->fd = 0;
@@ -82,9 +91,6 @@ void trace_disable(struct trace_key *key)
 	key->need_close = 0;
 }
 
-static const char err_msg[] = "Could not trace into fd given by "
-	"GIT_TRACE environment variable";
-
 static int prepare_trace_line(const char *file, int line,
 			      struct trace_key *key, struct strbuf *buf)
 {
@@ -120,18 +126,27 @@ static int prepare_trace_line(const char *file, int line,
 	return 1;
 }
 
+static void trace_write(struct trace_key *key, const void *buf, unsigned len)
+{
+	if (write_in_full(get_trace_fd(key), buf, len) < 0) {
+		normalize_trace_key(&key);
+		warning("unable to write trace for %s: %s",
+			key->key, strerror(errno));
+		trace_disable(key);
+	}
+}
+
 void trace_verbatim(struct trace_key *key, const void *buf, unsigned len)
 {
 	if (!trace_want(key))
 		return;
-	write_or_whine_pipe(get_trace_fd(key), buf, len, err_msg);
+	trace_write(key, buf, len);
 }
 
 static void print_trace_line(struct trace_key *key, struct strbuf *buf)
 {
 	strbuf_complete_line(buf);
-
+	trace_write(key, buf->buf, buf->len);
-	write_or_whine_pipe(get_trace_fd(key), buf->buf, buf->len, err_msg);
 	strbuf_release(buf);
 }
 

write_or_die.c

@@ -82,15 +82,3 @@ void write_or_die(int fd, const void *buf, size_t count)
 		die_errno("write error");
 	}
 }
-
-int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
-{
-	if (write_in_full(fd, buf, count) < 0) {
-		check_pipe(errno);
-		fprintf(stderr, "%s: write error (%s)\n",
-			msg, strerror(errno));
-		return 0;
-	}
-
-	return 1;
-}