[PATCH 0/5] Let log-tree and friends respect diffopt's `file` field

[PATCH 0/5] Let log-tree and friends respect diffopt's `file` field

[Johannes Schindelin]

The idea is to allow callers to redirect log-tree's output to a file
without having to freopen() stdout (which would modify global state,
a big no-no-no for library functions).

I reviewed log-tree.c, graph.c, line-log.c, builtin/shortlog.c and
builtin/log.c line by line to ensure that all calls that assumed stdout
previously now use the `file` field instead, of course. I would
welcome additional eyes to go over the code to confirm that I did not
miss anything.

This patch series ends up removing the freopen() call in the
format-patch command, but that is only a by-product. The ulterior motive
behind this series is to allow the sequencer to write a `patch` file as
part of my endeavor to move large chunks of rebase -i into a builtin.

Speaking of said endeavor: it is going a lot slower than I would like,
but I really need rebase -i to be robust. To that end, I not only review
and re-review my patches frequently, I also use a cross-validation
technique inspired by my original ll-merge validation as well as
GitHub's Scientist library: I taught rebase -i to run every interactive
rebase twice, once with the original shell script's code, and once with
the builtin rebase--helper, and then to verify that the end result is as
similar as can be expected (the commit dates will differ most of the
time, of course).

It is a bit tedious, of course, because I have to resolve every merge
conflict twice, I have to reword everything twice (identically!), and I
have to wait longer for the rebase to finish. It is worth it, though,
because I really need rebase -i to be robust, as it is the center piece
of almost all of my workflows.

I organized the patch series into a thicket of branches, to make it
easier to review them; In addition to this here patch series, I plan to
submit 11 separate, partially interdependent series, resubmit another
one, and I already submitted a couple of patches earlier that were
integrated or replaced by superior solutions (thanks, Peff!). Skipping
the temporary patches (e.g. for cross-validation), that leaves me with
99 patches to submit in total (sing with me: "99 patches of code to
submit, 99 patches of code, ...").

I was curious just how much these patches could accelerate rebase -i, so
I disabled the cross-validation temprarily and let Travis measure the
performance improvements via the t/perf test that was already merged to
`master`. Look for "3404.2" in this build's logs:
https://travis-ci.org/git/git/builds/138277774

It looks like a 3x speedup on Linux, a 4x speedup on MacOSX and my local
measurements suggest a >5x speed-up on Windows.


Johannes Schindelin (5):
  log-tree: respect diffopt's configured output file stream
  line-log: respect diffopt's configured output file stream
  graph: respect the diffopt.file setting
  shortlog: support outputting to streams other than stdout
  format-patch: avoid freopen()

 builtin/log.c      | 71 +++++++++++++++++++++++++++---------------------------
 builtin/shortlog.c | 13 ++++++----
 graph.c            | 30 +++++++++++++----------
 line-log.c         | 34 +++++++++++++-------------
 log-tree.c         | 65 +++++++++++++++++++++++++------------------------
 shortlog.h         |  1 +
 6 files changed, 111 insertions(+), 103 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/diffopt.file-v1
-- 
2.9.0.118.gce770ba.dirty

base-commit: 05219a1276341e72d8082d76b7f5ed394b7437a4

[PATCH 1/5] log-tree: respect diffopt's configured output file stream

[Johannes Schindelin]

The diff options already know how to print the output anywhere else
than stdout. The same is needed for log output in general, e.g.
when writing patches to files in `git format-patch`. Let's allow
users to use log_tree_commit() *without* changing global state via
freopen().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 log-tree.c | 65 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 33 insertions(+), 32 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 78a5381..968428a 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -159,12 +159,12 @@ void load_ref_decorations(int flags)
 	}
 }
 
-static void show_parents(struct commit *commit, int abbrev)
+static void show_parents(struct commit *commit, int abbrev, FILE *file)
 {
 	struct commit_list *p;
 	for (p = commit->parents; p ; p = p->next) {
 		struct commit *parent = p->item;
-		printf(" %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
+		fprintf(file, " %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
 	}
 }
 
@@ -172,7 +172,7 @@ static void show_children(struct rev_info *opt, struct commit *commit, int abbre
 {
 	struct commit_list *p = lookup_decoration(&opt->children, &commit->object);
 	for ( ; p; p = p->next) {
-		printf(" %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
+		fprintf(opt->diffopt.file, " %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
 	}
 }
 
@@ -286,11 +286,11 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
 	struct strbuf sb = STRBUF_INIT;
 
 	if (opt->show_source && commit->util)
-		printf("\t%s", (char *) commit->util);
+		fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
 	if (!opt->show_decorations)
 		return;
 	format_decorations(&sb, commit, opt->diffopt.use_color);
-	fputs(sb.buf, stdout);
+	fputs(sb.buf, opt->diffopt.file);
 	strbuf_release(&sb);
 }
 
@@ -364,18 +364,18 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		subject = "Subject: ";
 	}
 
-	printf("From %s Mon Sep 17 00:00:00 2001\n", name);
+	fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);
 	graph_show_oneline(opt->graph);
 	if (opt->message_id) {
-		printf("Message-Id: <%s>\n", opt->message_id);
+		fprintf(opt->diffopt.file, "Message-Id: <%s>\n", opt->message_id);
 		graph_show_oneline(opt->graph);
 	}
 	if (opt->ref_message_ids && opt->ref_message_ids->nr > 0) {
 		int i, n;
 		n = opt->ref_message_ids->nr;
-		printf("In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
+		fprintf(opt->diffopt.file, "In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
 		for (i = 0; i < n; i++)
-			printf("%s<%s>\n", (i > 0 ? "\t" : "References: "),
+			fprintf(opt->diffopt.file, "%s<%s>\n", (i > 0 ? "\t" : "References: "),
 			       opt->ref_message_ids->items[i].string);
 		graph_show_oneline(opt->graph);
 	}
@@ -432,7 +432,7 @@ static void show_sig_lines(struct rev_info *opt, int status, const char *bol)
 	reset = diff_get_color_opt(&opt->diffopt, DIFF_RESET);
 	while (*bol) {
 		eol = strchrnul(bol, '\n');
-		printf("%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
+		fprintf(opt->diffopt.file, "%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
 		       *eol ? "\n" : "");
 		graph_show_oneline(opt->graph);
 		bol = (*eol) ? (eol + 1) : eol;
@@ -553,17 +553,17 @@ void show_log(struct rev_info *opt)
 
 		if (!opt->graph)
 			put_revision_mark(opt, commit);
-		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), stdout);
+		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), opt->diffopt.file);
 		if (opt->print_parents)
-			show_parents(commit, abbrev_commit);
+			show_parents(commit, abbrev_commit, opt->diffopt.file);
 		if (opt->children.name)
 			show_children(opt, commit, abbrev_commit);
 		show_decorations(opt, commit);
 		if (opt->graph && !graph_is_commit_finished(opt->graph)) {
-			putchar('\n');
+			fputc('\n', opt->diffopt.file);
 			graph_show_remainder(opt->graph);
 		}
-		putchar(opt->diffopt.line_termination);
+		fputc(opt->diffopt.line_termination, opt->diffopt.file);
 		return;
 	}
 
@@ -589,7 +589,7 @@ void show_log(struct rev_info *opt)
 		if (opt->diffopt.line_termination == '\n' &&
 		    !opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar(opt->diffopt.line_termination);
+		fputc(opt->diffopt.line_termination, opt->diffopt.file);
 	}
 	opt->shown_one = 1;
 
@@ -607,28 +607,28 @@ void show_log(struct rev_info *opt)
 		log_write_email_headers(opt, commit, &ctx.subject, &extra_headers,
 					&ctx.need_8bit_cte);
 	} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
-		fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), stdout);
+		fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), opt->diffopt.file);
 		if (opt->commit_format != CMIT_FMT_ONELINE)
-			fputs("commit ", stdout);
+			fputs("commit ", opt->diffopt.file);
 
 		if (!opt->graph)
 			put_revision_mark(opt, commit);
 		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit),
-		      stdout);
+		      opt->diffopt.file);
 		if (opt->print_parents)
-			show_parents(commit, abbrev_commit);
+			show_parents(commit, abbrev_commit, opt->diffopt.file);
 		if (opt->children.name)
 			show_children(opt, commit, abbrev_commit);
 		if (parent)
-			printf(" (from %s)",
+			fprintf(opt->diffopt.file, " (from %s)",
 			       find_unique_abbrev(parent->object.oid.hash,
 						  abbrev_commit));
-		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), stdout);
+		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
 		show_decorations(opt, commit);
 		if (opt->commit_format == CMIT_FMT_ONELINE) {
-			putchar(' ');
+			fputc(' ', opt->diffopt.file);
 		} else {
-			putchar('\n');
+			fputc('\n', opt->diffopt.file);
 			graph_show_oneline(opt->graph);
 		}
 		if (opt->reflog_info) {
@@ -702,7 +702,7 @@ void show_log(struct rev_info *opt)
 	}
 
 	if (opt->show_log_size) {
-		printf("log size %i\n", (int)msgbuf.len);
+		fprintf(opt->diffopt.file, "log size %i\n", (int)msgbuf.len);
 		graph_show_oneline(opt->graph);
 	}
 
@@ -718,11 +718,11 @@ void show_log(struct rev_info *opt)
 	if (opt->graph)
 		graph_show_commit_msg(opt->graph, &msgbuf);
 	else
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, opt->diffopt.file);
 	if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar(opt->diffopt.line_termination);
+		fputc(opt->diffopt.line_termination, opt->diffopt.file);
 	}
 
 	strbuf_release(&msgbuf);
@@ -759,7 +759,7 @@ int log_tree_diff_flush(struct rev_info *opt)
 				struct strbuf *msg = NULL;
 				msg = opt->diffopt.output_prefix(&opt->diffopt,
 					opt->diffopt.output_prefix_data);
-				fwrite(msg->buf, msg->len, 1, stdout);
+				fwrite(msg->buf, msg->len, 1, opt->diffopt.file);
 			}
 
 			/*
@@ -774,8 +774,8 @@ int log_tree_diff_flush(struct rev_info *opt)
 			 */
 			if (!opt->shown_dashes &&
 			    (pch & opt->diffopt.output_format) == pch)
-				printf("---");
-			putchar('\n');
+				fprintf(opt->diffopt.file, "---");
+			fputc('\n', opt->diffopt.file);
 		}
 	}
 	diff_flush(&opt->diffopt);
@@ -872,7 +872,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 		return line_log_print(opt, commit);
 
 	if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
-		printf("\n%s\n", opt->break_bar);
+		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	shown = log_tree_diff(opt, commit, &log);
 	if (!shown && opt->loginfo && opt->always_show_header) {
 		log.parent = NULL;
@@ -880,8 +880,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 		shown = 1;
 	}
 	if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
-		printf("\n%s\n", opt->break_bar);
+		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	opt->loginfo = NULL;
-	maybe_flush_or_die(stdout, "stdout");
+	if (opt->diffopt.file == stdout)
+		maybe_flush_or_die(stdout, "stdout");
 	return shown;
 }
-- 
2.9.0.118.gce770ba.dirty

[PATCH 2/5] line-log: respect diffopt's configured output file stream

[Johannes Schindelin]

The diff machinery can optionally output to a file stream other than
stdout, by overriding diffopt.file. In such a case, the rest of the
log tree machinery should also write to that stream.

Currently, there is no user of the line level log that wants to
redirect output to a file. Therefore, one might argue that it is
superfluous to support that now. However, it is better to be
consistent now, rather than to face hard-to-debug problems later.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 line-log.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/line-log.c b/line-log.c
index bbe31ed..c3b8563 100644
--- a/line-log.c
+++ b/line-log.c
@@ -841,7 +841,7 @@ static char *get_nth_line(long line, unsigned long *ends, void *data)
 
 static void print_line(const char *prefix, char first,
 		       long line, unsigned long *ends, void *data,
-		       const char *color, const char *reset)
+		       const char *color, const char *reset, FILE *file)
 {
 	char *begin = get_nth_line(line, ends, data);
 	char *end = get_nth_line(line+1, ends, data);
@@ -852,14 +852,14 @@ static void print_line(const char *prefix, char first,
 		had_nl = 1;
 	}
 
-	fputs(prefix, stdout);
-	fputs(color, stdout);
-	putchar(first);
-	fwrite(begin, 1, end-begin, stdout);
-	fputs(reset, stdout);
-	putchar('\n');
+	fputs(prefix, file);
+	fputs(color, file);
+	fputc(first, file);
+	fwrite(begin, 1, end-begin, file);
+	fputs(reset, file);
+	fputc('\n', file);
 	if (!had_nl)
-		fputs("\\ No newline at end of file\n", stdout);
+		fputs("\\ No newline at end of file\n", file);
 }
 
 static char *output_prefix(struct diff_options *opt)
@@ -898,12 +898,12 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 		fill_line_ends(pair->one, &p_lines, &p_ends);
 	fill_line_ends(pair->two, &t_lines, &t_ends);
 
-	printf("%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
-	printf("%s%s--- %s%s%s\n", prefix, c_meta,
+	fprintf(opt->file, "%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
+	fprintf(opt->file, "%s%s--- %s%s%s\n", prefix, c_meta,
 	       pair->one->sha1_valid ? "a/" : "",
 	       pair->one->sha1_valid ? pair->one->path : "/dev/null",
 	       c_reset);
-	printf("%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
+	fprintf(opt->file, "%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
 	for (i = 0; i < range->ranges.nr; i++) {
 		long p_start, p_end;
 		long t_start = range->ranges.ranges[i].start;
@@ -945,7 +945,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 		}
 
 		/* Now output a diff hunk for this range */
-		printf("%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
+		fprintf(opt->file, "%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
 		       prefix, c_frag,
 		       p_start+1, p_end-p_start, t_start+1, t_end-t_start,
 		       c_reset);
@@ -953,18 +953,18 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 			int k;
 			for (; t_cur < diff->target.ranges[j].start; t_cur++)
 				print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
-					   c_context, c_reset);
+					   c_context, c_reset, opt->file);
 			for (k = diff->parent.ranges[j].start; k < diff->parent.ranges[j].end; k++)
 				print_line(prefix, '-', k, p_ends, pair->one->data,
-					   c_old, c_reset);
+					   c_old, c_reset, opt->file);
 			for (; t_cur < diff->target.ranges[j].end && t_cur < t_end; t_cur++)
 				print_line(prefix, '+', t_cur, t_ends, pair->two->data,
-					   c_new, c_reset);
+					   c_new, c_reset, opt->file);
 			j++;
 		}
 		for (; t_cur < t_end; t_cur++)
 			print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
-				   c_context, c_reset);
+				   c_context, c_reset, opt->file);
 	}
 
 	free(p_ends);
@@ -977,7 +977,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
  */
 static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)
 {
-	puts(output_prefix(&rev->diffopt));
+	fprintf(rev->diffopt.file, "%s\n", output_prefix(&rev->diffopt));
 	while (range) {
 		dump_diff_hacky_one(rev, range);
 		range = range->next;
-- 
2.9.0.118.gce770ba.dirty

[PATCH 3/5] graph: respect the diffopt.file setting

[Johannes Schindelin]

When the caller overrides diffopt.file (which defaults to stdout),
the diff machinery already redirects its output, and the graph display
should also write to that file.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 graph.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/graph.c b/graph.c
index 1350bdd..8ae56bc 100644
--- a/graph.c
+++ b/graph.c
@@ -17,8 +17,8 @@
 static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
 
 /*
- * Print a strbuf to stdout.  If the graph is non-NULL, all lines but the
- * first will be prefixed with the graph output.
+ * Print a strbuf.  If the graph is non-NULL, all lines but the first will be
+ * prefixed with the graph output.
  *
  * If the strbuf ends with a newline, the output will end after this
  * newline.  A new graph line will not be printed after the final newline.
@@ -1193,9 +1193,10 @@ void graph_show_commit(struct git_graph *graph)
 
 	while (!shown_commit_line && !graph_is_commit_finished(graph)) {
 		shown_commit_line = graph_next_line(graph, &msgbuf);
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+			graph->revs->diffopt.file);
 		if (!shown_commit_line)
-			putchar('\n');
+			fputc('\n', graph->revs->diffopt.file);
 		strbuf_setlen(&msgbuf, 0);
 	}
 
@@ -1210,7 +1211,7 @@ void graph_show_oneline(struct git_graph *graph)
 		return;
 
 	graph_next_line(graph, &msgbuf);
-	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
 	strbuf_release(&msgbuf);
 }
 
@@ -1222,7 +1223,7 @@ void graph_show_padding(struct git_graph *graph)
 		return;
 
 	graph_padding_line(graph, &msgbuf);
-	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
 	strbuf_release(&msgbuf);
 }
 
@@ -1239,12 +1240,13 @@ int graph_show_remainder(struct git_graph *graph)
 
 	for (;;) {
 		graph_next_line(graph, &msgbuf);
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+			graph->revs->diffopt.file);
 		strbuf_setlen(&msgbuf, 0);
 		shown = 1;
 
 		if (!graph_is_commit_finished(graph))
-			putchar('\n');
+			fputc('\n', graph->revs->diffopt.file);
 		else
 			break;
 	}
@@ -1259,7 +1261,8 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 	char *p;
 
 	if (!graph) {
-		fwrite(sb->buf, sizeof(char), sb->len, stdout);
+		fwrite(sb->buf, sizeof(char), sb->len,
+			graph->revs->diffopt.file);
 		return;
 	}
 
@@ -1277,7 +1280,7 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 		} else {
 			len = (sb->buf + sb->len) - p;
 		}
-		fwrite(p, sizeof(char), len, stdout);
+		fwrite(p, sizeof(char), len, graph->revs->diffopt.file);
 		if (next_p && *next_p != '\0')
 			graph_show_oneline(graph);
 		p = next_p;
@@ -1297,7 +1300,8 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * CMIT_FMT_USERFORMAT are already missing a terminating
 		 * newline.  All of the other formats should have it.
 		 */
-		fwrite(sb->buf, sizeof(char), sb->len, stdout);
+		fwrite(sb->buf, sizeof(char), sb->len,
+			graph->revs->diffopt.file);
 		return;
 	}
 
@@ -1318,7 +1322,7 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * new line.
 		 */
 		if (!newline_terminated)
-			putchar('\n');
+			fputc('\n', graph->revs->diffopt.file);
 
 		graph_show_remainder(graph);
 
@@ -1326,6 +1330,6 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * If sb ends with a newline, our output should too.
 		 */
 		if (newline_terminated)
-			putchar('\n');
+			fputc('\n', graph->revs->diffopt.file);
 	}
 }
-- 
2.9.0.118.gce770ba.dirty

[PATCH 4/5] shortlog: support outputting to streams other than stdout

[Johannes Schindelin]

This will be needed to avoid freopen() in `git format-patch`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/shortlog.c | 13 ++++++++-----
 shortlog.h         |  1 +
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index bfc082e..4c68ba7 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -229,6 +229,7 @@ void shortlog_init(struct shortlog *log)
 	log->wrap = DEFAULT_WRAPLEN;
 	log->in1 = DEFAULT_INDENT1;
 	log->in2 = DEFAULT_INDENT2;
+	log->file = stdout;
 }
 
 int cmd_shortlog(int argc, const char **argv, const char *prefix)
@@ -310,22 +311,24 @@ void shortlog_output(struct shortlog *log)
 	for (i = 0; i < log->list.nr; i++) {
 		const struct string_list_item *item = &log->list.items[i];
 		if (log->summary) {
-			printf("%6d\t%s\n", (int)UTIL_TO_INT(item), item->string);
+			fprintf(log->file, "%6d\t%s\n",
+				(int)UTIL_TO_INT(item), item->string);
 		} else {
 			struct string_list *onelines = item->util;
-			printf("%s (%d):\n", item->string, onelines->nr);
+			fprintf(log->file, "%s (%d):\n",
+				item->string, onelines->nr);
 			for (j = onelines->nr - 1; j >= 0; j--) {
 				const char *msg = onelines->items[j].string;
 
 				if (log->wrap_lines) {
 					strbuf_reset(&sb);
 					add_wrapped_shortlog_msg(&sb, msg, log);
-					fwrite(sb.buf, sb.len, 1, stdout);
+					fwrite(sb.buf, sb.len, 1, log->file);
 				}
 				else
-					printf("      %s\n", msg);
+					fprintf(log->file, "      %s\n", msg);
 			}
-			putchar('\n');
+			fputc('\n', log->file);
 			onelines->strdup_strings = 1;
 			string_list_clear(onelines, 0);
 			free(onelines);
diff --git a/shortlog.h b/shortlog.h
index de4f86f..5a326c6 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -17,6 +17,7 @@ struct shortlog {
 	char *common_repo_prefix;
 	int email;
 	struct string_list mailmap;
+	FILE *file;
 };
 
 void shortlog_init(struct shortlog *log);
-- 
2.9.0.118.gce770ba.dirty

[PATCH 5/5] format-patch: avoid freopen()

[Johannes Schindelin]

We just taught the relevant functions to respect the diffopt.file field,
to allow writing somewhere else than stdout. Let's make use of it.

Technically, we do not need to avoid that call in a builtin: we assume
that builtins (as opposed to library functions) are stand-alone programs
that may do with their (global) state. Yet, we want to be able to reuse
that code in properly lib-ified code, e.g. when converting scripts into
builtins.

Further, while we did not *have* to touch the cmd_show() and cmd_cherry()
code paths (because they do not want to write anywhere but stdout as of
yet), it just makes sense to be consistent, making it easier and safer to
move the code later.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 71 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 099f4f7..5a889d5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -236,7 +236,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)
 		if (rev->commit_format != CMIT_FMT_ONELINE)
 			putchar(rev->diffopt.line_termination);
 	}
-	printf(_("Final output: %d %s\n"), nr, stage);
+	fprintf(rev->diffopt.file, _("Final output: %d %s\n"), nr, stage);
 }
 
 static struct itimerval early_output_timer;
@@ -445,7 +445,7 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
 	pp.fmt = rev->commit_format;
 	pp.date_mode = rev->date_mode;
 	pp_user_info(&pp, "Tagger", &out, buf, get_log_output_encoding());
-	printf("%s", out.buf);
+	fprintf(rev->diffopt.file, "%s", out.buf);
 	strbuf_release(&out);
 }
 
@@ -456,7 +456,7 @@ static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, con
 	char *buf;
 	unsigned long size;
 
-	fflush(stdout);
+	fflush(rev->diffopt.file);
 	if (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||
 	    !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
 		return stream_blob_to_fd(1, sha1, NULL, 0);
@@ -496,7 +496,7 @@ static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)
 	}
 
 	if (offset < size)
-		fwrite(buf + offset, size - offset, 1, stdout);
+		fwrite(buf + offset, size - offset, 1, rev->diffopt.file);
 	free(buf);
 	return 0;
 }
@@ -505,7 +505,8 @@ static int show_tree_object(const unsigned char *sha1,
 		struct strbuf *base,
 		const char *pathname, unsigned mode, int stage, void *context)
 {
-	printf("%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
+	FILE *file = context;
+	fprintf(file, "%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
 	return 0;
 }
 
@@ -565,7 +566,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 
 			if (rev.shown_one)
 				putchar('\n');
-			printf("%stag %s%s\n",
+			fprintf(rev.diffopt.file, "%stag %s%s\n",
 					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					t->tag,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
@@ -584,12 +585,12 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 		case OBJ_TREE:
 			if (rev.shown_one)
 				putchar('\n');
-			printf("%stree %s%s\n\n",
+			fprintf(rev.diffopt.file, "%stree %s%s\n\n",
 					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					name,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
 			read_tree_recursive((struct tree *)o, "", 0, 0, &match_all,
-					show_tree_object, NULL);
+					show_tree_object, rev.diffopt.file);
 			rev.shown_one = 1;
 			break;
 		case OBJ_COMMIT:
@@ -795,11 +796,10 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	return git_log_config(var, value, cb);
 }
 
-static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
-static int reopen_stdout(struct commit *commit, const char *subject,
+static int open_next_file(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
@@ -821,9 +821,9 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 		fmt_output_subject(&filename, subject, rev);
 
 	if (!quiet)
-		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
+		printf("%s\n", filename.buf + outdir_offset);
 
-	if (freopen(filename.buf, "w", stdout) == NULL)
+	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
 		return error(_("Cannot open patch file %s"), filename.buf);
 
 	strbuf_release(&filename);
@@ -882,15 +882,15 @@ static void gen_message_id(struct rev_info *info, char *base)
 	info->message_id = strbuf_detach(&buf, NULL);
 }
 
-static void print_signature(void)
+static void print_signature(FILE *file)
 {
 	if (!signature || !*signature)
 		return;
 
-	printf("-- \n%s", signature);
+	fprintf(file, "-- \n%s", signature);
 	if (signature[strlen(signature)-1] != '\n')
-		putchar('\n');
-	putchar('\n');
+		fputc('\n', file);
+	fputc('\n', file);
 }
 
 static void add_branch_description(struct strbuf *buf, const char *branch_name)
@@ -959,7 +959,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	committer = git_committer_info(0);
 
 	if (!use_stdout &&
-	    reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
+	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
 		return;
 
 	log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
@@ -982,7 +982,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
 	pp_remainder(&pp, &msg, &sb, 0);
 	add_branch_description(&sb, branch_name);
-	printf("%s\n", sb.buf);
+	fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
 	strbuf_release(&sb);
 
@@ -991,6 +991,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	log.wrap = 72;
 	log.in1 = 2;
 	log.in2 = 4;
+	log.file = rev->diffopt.file;
 	for (i = 0; i < nr; i++)
 		shortlog_add_commit(&log, list[i]);
 
@@ -1013,8 +1014,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	diffcore_std(&opts);
 	diff_flush(&opts);
 
-	printf("\n");
-	print_signature();
+	fprintf(rev->diffopt.file, "\n");
+	print_signature(rev->diffopt.file);
 }
 
 static const char *clean_message_id(const char *msg_id)
@@ -1324,7 +1325,7 @@ static void prepare_bases(struct base_tree_info *bases,
 	}
 }
 
-static void print_bases(struct base_tree_info *bases)
+static void print_bases(struct base_tree_info *bases, FILE *file)
 {
 	int i;
 
@@ -1333,11 +1334,11 @@ static void print_bases(struct base_tree_info *bases)
 		return;
 
 	/* Show the base commit */
-	printf("base-commit: %s\n", oid_to_hex(&bases->base_commit));
+	fprintf(file, "base-commit: %s\n", oid_to_hex(&bases->base_commit));
 
 	/* Show the prerequisite patches */
 	for (i = bases->nr_patch_id - 1; i >= 0; i--)
-		printf("prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
+		fprintf(file, "prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
 
 	free(bases->patch_id);
 	bases->nr_patch_id = 0;
@@ -1569,6 +1570,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		setup_pager();
 
 	if (output_directory) {
+		rev.diffopt.use_color = 0;
 		if (use_stdout)
 			die(_("standard output, or directory, which one?"));
 		if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
@@ -1626,9 +1628,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		get_patch_ids(&rev, &ids);
 	}
 
-	if (!use_stdout)
-		realstdout = xfdopen(xdup(1), "w");
-
 	if (prepare_revision_walk(&rev))
 		die(_("revision walk setup failed"));
 	rev.boundary = 1;
@@ -1693,7 +1692,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout,
 				  origin, nr, list, branch_name, quiet);
-		print_bases(&bases);
+		print_bases(&bases, rev.diffopt.file);
 		total++;
 		start_number--;
 	}
@@ -1739,7 +1738,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!use_stdout &&
-		    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
+		    open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
 			die(_("Failed to create output files"));
 		shown = log_tree_commit(&rev, commit);
 		free_commit_buffer(commit);
@@ -1754,15 +1753,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			rev.shown_one = 0;
 		if (shown) {
 			if (rev.mime_boundary)
-				printf("\n--%s%s--\n\n\n",
+				fprintf(rev.diffopt.file, "\n--%s%s--\n\n\n",
 				       mime_boundary_leader,
 				       rev.mime_boundary);
 			else
-				print_signature();
-			print_bases(&bases);
+				print_signature(rev.diffopt.file);
+			print_bases(&bases, rev.diffopt.file);
 		}
 		if (!use_stdout)
-			fclose(stdout);
+			fclose(rev.diffopt.file);
 	}
 	free(list);
 	free(branch_name);
@@ -1794,15 +1793,15 @@ static const char * const cherry_usage[] = {
 };
 
 static void print_commit(char sign, struct commit *commit, int verbose,
-			 int abbrev)
+			 int abbrev, FILE *file)
 {
 	if (!verbose) {
-		printf("%c %s\n", sign,
+		fprintf(file, "%c %s\n", sign,
 		       find_unique_abbrev(commit->object.oid.hash, abbrev));
 	} else {
 		struct strbuf buf = STRBUF_INIT;
 		pp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);
-		printf("%c %s %s\n", sign,
+		fprintf(file, "%c %s %s\n", sign,
 		       find_unique_abbrev(commit->object.oid.hash, abbrev),
 		       buf.buf);
 		strbuf_release(&buf);
@@ -1883,7 +1882,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 		commit = list->item;
 		if (has_commit_patch_id(commit, &ids))
 			sign = '-';
-		print_commit(sign, commit, verbose, abbrev);
+		print_commit(sign, commit, verbose, abbrev, revs.diffopt.file);
 		list = list->next;
 	}
 
-- 
2.9.0.118.gce770ba.dirty

Re: [PATCH 5/5] format-patch: avoid freopen()

[Eric Sunshine]

On Sat, Jun 18, 2016 at 6:04 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> We just taught the relevant functions to respect the diffopt.file field,
> to allow writing somewhere else than stdout. Let's make use of it.
> [...]
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/builtin/log.c b/builtin/log.c
> @@ -1569,6 +1570,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>                 setup_pager();
>
>         if (output_directory) {
> +               rev.diffopt.use_color = 0;

What is this change about? It doesn't seem to be related to anything
else in the patch.

>                 if (use_stdout)
>                         die(_("standard output, or directory, which one?"));
>                 if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)

Re: [PATCH 5/5] format-patch: avoid freopen()

[Johannes Schindelin]

Hi Eric,

On Sun, 19 Jun 2016, Eric Sunshine wrote:

> On Sat, Jun 18, 2016 at 6:04 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > We just taught the relevant functions to respect the diffopt.file field,
> > to allow writing somewhere else than stdout. Let's make use of it.
> > [...]
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/builtin/log.c b/builtin/log.c
> > @@ -1569,6 +1570,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >                 setup_pager();
> >
> >         if (output_directory) {
> > +               rev.diffopt.use_color = 0;
> 
> What is this change about? It doesn't seem to be related to anything
> else in the patch.

Good point, I completely forgot to mention this is the commit message.

When format-patch calls the diff machinery, want_color() is used to
determine whether to use ANSI color sequences or not. If use_color is not
set explicitly, isatty(1) is used to determine whether or not the user
wants color. When stdout was freopen()ed, this isatty(1) call naturally
looked at the file descriptor that was reopened, and determined correctly
that no color was desired.

With the freopen() call gone, stdout may very well be the terminal. But we
still do not want color because the output is intended to go to a file (at
least if output_directory is set).

So how about this commit message (I inserted the "Note: ..." paragraph)?

-- snipsnap --
format-patch: avoid freopen()

We just taught the relevant functions to respect the diffopt.file field,
to allow writing somewhere else than stdout. Let's make use of it.

Technically, we do not need to avoid that call in a builtin: we assume
that builtins (as opposed to library functions) are stand-alone programs
that may do with their (global) state. Yet, we want to be able to reuse
that code in properly lib-ified code, e.g. when converting scripts into
builtins.

Note: we now have to set use_color = 0 explicitly when writing to files,
as the auto-detection whether to colorify the output *still* looks at
stdout (which is no longer freopen()ed, and therefore might still be
printing to the terminal).

Further, while we did not *have* to touch the cmd_show() and cmd_cherry()
code paths (because they do not want to write anywhere but stdout as of
yet), it just makes sense to be consistent, making it easier and safer to
move the code later.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Re: [PATCH 5/5] format-patch: avoid freopen()

[Eric Sunshine]

On Mon, Jun 20, 2016 at 2:26 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Sun, 19 Jun 2016, Eric Sunshine wrote:
>> On Sat, Jun 18, 2016 at 6:04 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>> >         if (output_directory) {
>> > +               rev.diffopt.use_color = 0;
>>
>> What is this change about? It doesn't seem to be related to anything
>> else in the patch.
>
> Good point, I completely forgot to mention this is the commit message.
>
> When format-patch calls the diff machinery, want_color() is used to
> determine whether to use ANSI color sequences or not. If use_color is not
> set explicitly, isatty(1) is used to determine whether or not the user
> wants color. When stdout was freopen()ed, this isatty(1) call naturally
> looked at the file descriptor that was reopened, and determined correctly
> that no color was desired.
>
> With the freopen() call gone, stdout may very well be the terminal. But we
> still do not want color because the output is intended to go to a file (at
> least if output_directory is set).

Would it make sense to do this as a separate preparatory patch, or is
it just too minor?

> So how about this commit message (I inserted the "Note: ..." paragraph)?
>
> -- snipsnap --
> format-patch: avoid freopen()
> [...]
> Note: we now have to set use_color = 0 explicitly when writing to files,
> as the auto-detection whether to colorify the output *still* looks at
> stdout (which is no longer freopen()ed, and therefore might still be
> printing to the terminal).

Thanks.

Re: [PATCH 5/5] format-patch: avoid freopen()

[Johannes Schindelin]

Hi Eric,

On Mon, 20 Jun 2016, Eric Sunshine wrote:

> On Mon, Jun 20, 2016 at 2:26 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Sun, 19 Jun 2016, Eric Sunshine wrote:
> >> On Sat, Jun 18, 2016 at 6:04 AM, Johannes Schindelin
> >> <johannes.schindelin@gmx.de> wrote:
> >> >         if (output_directory) {
> >> > +               rev.diffopt.use_color = 0;
> >>
> >> What is this change about? It doesn't seem to be related to anything
> >> else in the patch.
> >
> > Good point, I completely forgot to mention this is the commit message.
> >
> > When format-patch calls the diff machinery, want_color() is used to
> > determine whether to use ANSI color sequences or not. If use_color is not
> > set explicitly, isatty(1) is used to determine whether or not the user
> > wants color. When stdout was freopen()ed, this isatty(1) call naturally
> > looked at the file descriptor that was reopened, and determined correctly
> > that no color was desired.
> >
> > With the freopen() call gone, stdout may very well be the terminal. But we
> > still do not want color because the output is intended to go to a file (at
> > least if output_directory is set).
> 
> Would it make sense to do this as a separate preparatory patch, or is
> it just too minor?

That's a good point. It *is* a change in its own right. Will reroll.

Thanks,
Dscho

[PATCH v2 0/7] Let log-tree and friends respect diffopt's `file` field

[Johannes Schindelin]

The idea is to allow callers to redirect log-tree's output to a file
without having to freopen() stdout (which would modify global state,
a big no-no-no for library functions).

I reviewed log-tree.c, graph.c, line-log.c, builtin/shortlog.c and
builtin/log.c line by line to ensure that all calls that assumed stdout
previously now use the `file` field instead, of course. I would
welcome additional eyes to go over the code to confirm that I did not
miss anything.

This patch series ends up removing the freopen() call in the
format-patch command, but that is only a by-product. The ulterior motive
behind this series is to allow the sequencer to write a `patch` file as
part of my endeavor to move large chunks of rebase -i into a builtin.

Speaking of said endeavor: it is going a lot slower than I would like,
but I really need rebase -i to be robust. To that end, I not only review
and re-review my patches frequently, I also use a cross-validation
technique inspired by my original ll-merge validation as well as
GitHub's Scientist library: I taught rebase -i to run every interactive
rebase twice, once with the original shell script's code, and once with
the builtin rebase--helper, and then to verify that the end result is as
similar as can be expected (the commit dates will differ most of the
time, of course).

It is a bit tedious, of course, because I have to resolve every merge
conflict twice, I have to reword everything twice (identically!), and I
have to wait longer for the rebase to finish. It is worth it, though,
because I really need rebase -i to be robust, as it is the center piece
of almost all of my workflows.

I organized the patch series into a thicket of branches, to make it
easier to review them; In addition to this here patch series, I plan to
submit 11 separate, partially interdependent series, resubmit another
one, and I already submitted a couple of patches earlier that were
integrated or replaced by superior solutions (thanks, Peff!). Skipping
the temporary patches (e.g. for cross-validation), that leaves me with
99 patches to submit in total (sing with me: "99 patches of code to
submit, 99 patches of code, ...").

I was curious just how much these patches could accelerate rebase -i, so
I disabled the cross-validation temprarily and let Travis measure the
performance improvements via the t/perf test that was already merged to
`master`. Look for "3404.2" in this build's logs:
https://travis-ci.org/git/git/builds/138277774

It looks like a 3x speedup on Linux, a 4x speedup on MacOSX and my local
measurements suggest a >5x speed-up on Windows.


Johannes Schindelin (7):
  log-tree: respect diffopt's configured output file stream
  line-log: respect diffopt's configured output file stream
  graph: respect the diffopt.file setting
  shortlog: support outputting to streams other than stdout
  format-patch: explicitly switch off color when writing to files
  format-patch: avoid freopen()
  format-patch: use stdout directly

 builtin/log.c      | 71 +++++++++++++++++++++++++++---------------------------
 builtin/shortlog.c | 13 ++++++----
 graph.c            | 30 +++++++++++++----------
 line-log.c         | 34 +++++++++++++-------------
 log-tree.c         | 65 +++++++++++++++++++++++++------------------------
 shortlog.h         |  1 +
 6 files changed, 111 insertions(+), 103 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/diffopt.file-v2
-- 
2.9.0.119.g370c5a9

base-commit: 05219a1276341e72d8082d76b7f5ed394b7437a4

[PATCH v2 1/7] log-tree: respect diffopt's configured output file stream

[Johannes Schindelin]

The diff options already know how to print the output anywhere else
than stdout. The same is needed for log output in general, e.g.
when writing patches to files in `git format-patch`. Let's allow
users to use log_tree_commit() *without* changing global state via
freopen().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 log-tree.c | 65 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 33 insertions(+), 32 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 78a5381..968428a 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -159,12 +159,12 @@ void load_ref_decorations(int flags)
 	}
 }
 
-static void show_parents(struct commit *commit, int abbrev)
+static void show_parents(struct commit *commit, int abbrev, FILE *file)
 {
 	struct commit_list *p;
 	for (p = commit->parents; p ; p = p->next) {
 		struct commit *parent = p->item;
-		printf(" %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
+		fprintf(file, " %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
 	}
 }
 
@@ -172,7 +172,7 @@ static void show_children(struct rev_info *opt, struct commit *commit, int abbre
 {
 	struct commit_list *p = lookup_decoration(&opt->children, &commit->object);
 	for ( ; p; p = p->next) {
-		printf(" %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
+		fprintf(opt->diffopt.file, " %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
 	}
 }
 
@@ -286,11 +286,11 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
 	struct strbuf sb = STRBUF_INIT;
 
 	if (opt->show_source && commit->util)
-		printf("\t%s", (char *) commit->util);
+		fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
 	if (!opt->show_decorations)
 		return;
 	format_decorations(&sb, commit, opt->diffopt.use_color);
-	fputs(sb.buf, stdout);
+	fputs(sb.buf, opt->diffopt.file);
 	strbuf_release(&sb);
 }
 
@@ -364,18 +364,18 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		subject = "Subject: ";
 	}
 
-	printf("From %s Mon Sep 17 00:00:00 2001\n", name);
+	fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);
 	graph_show_oneline(opt->graph);
 	if (opt->message_id) {
-		printf("Message-Id: <%s>\n", opt->message_id);
+		fprintf(opt->diffopt.file, "Message-Id: <%s>\n", opt->message_id);
 		graph_show_oneline(opt->graph);
 	}
 	if (opt->ref_message_ids && opt->ref_message_ids->nr > 0) {
 		int i, n;
 		n = opt->ref_message_ids->nr;
-		printf("In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
+		fprintf(opt->diffopt.file, "In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
 		for (i = 0; i < n; i++)
-			printf("%s<%s>\n", (i > 0 ? "\t" : "References: "),
+			fprintf(opt->diffopt.file, "%s<%s>\n", (i > 0 ? "\t" : "References: "),
 			       opt->ref_message_ids->items[i].string);
 		graph_show_oneline(opt->graph);
 	}
@@ -432,7 +432,7 @@ static void show_sig_lines(struct rev_info *opt, int status, const char *bol)
 	reset = diff_get_color_opt(&opt->diffopt, DIFF_RESET);
 	while (*bol) {
 		eol = strchrnul(bol, '\n');
-		printf("%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
+		fprintf(opt->diffopt.file, "%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
 		       *eol ? "\n" : "");
 		graph_show_oneline(opt->graph);
 		bol = (*eol) ? (eol + 1) : eol;
@@ -553,17 +553,17 @@ void show_log(struct rev_info *opt)
 
 		if (!opt->graph)
 			put_revision_mark(opt, commit);
-		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), stdout);
+		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), opt->diffopt.file);
 		if (opt->print_parents)
-			show_parents(commit, abbrev_commit);
+			show_parents(commit, abbrev_commit, opt->diffopt.file);
 		if (opt->children.name)
 			show_children(opt, commit, abbrev_commit);
 		show_decorations(opt, commit);
 		if (opt->graph && !graph_is_commit_finished(opt->graph)) {
-			putchar('\n');
+			fputc('\n', opt->diffopt.file);
 			graph_show_remainder(opt->graph);
 		}
-		putchar(opt->diffopt.line_termination);
+		fputc(opt->diffopt.line_termination, opt->diffopt.file);
 		return;
 	}
 
@@ -589,7 +589,7 @@ void show_log(struct rev_info *opt)
 		if (opt->diffopt.line_termination == '\n' &&
 		    !opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar(opt->diffopt.line_termination);
+		fputc(opt->diffopt.line_termination, opt->diffopt.file);
 	}
 	opt->shown_one = 1;
 
@@ -607,28 +607,28 @@ void show_log(struct rev_info *opt)
 		log_write_email_headers(opt, commit, &ctx.subject, &extra_headers,
 					&ctx.need_8bit_cte);
 	} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
-		fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), stdout);
+		fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), opt->diffopt.file);
 		if (opt->commit_format != CMIT_FMT_ONELINE)
-			fputs("commit ", stdout);
+			fputs("commit ", opt->diffopt.file);
 
 		if (!opt->graph)
 			put_revision_mark(opt, commit);
 		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit),
-		      stdout);
+		      opt->diffopt.file);
 		if (opt->print_parents)
-			show_parents(commit, abbrev_commit);
+			show_parents(commit, abbrev_commit, opt->diffopt.file);
 		if (opt->children.name)
 			show_children(opt, commit, abbrev_commit);
 		if (parent)
-			printf(" (from %s)",
+			fprintf(opt->diffopt.file, " (from %s)",
 			       find_unique_abbrev(parent->object.oid.hash,
 						  abbrev_commit));
-		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), stdout);
+		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
 		show_decorations(opt, commit);
 		if (opt->commit_format == CMIT_FMT_ONELINE) {
-			putchar(' ');
+			fputc(' ', opt->diffopt.file);
 		} else {
-			putchar('\n');
+			fputc('\n', opt->diffopt.file);
 			graph_show_oneline(opt->graph);
 		}
 		if (opt->reflog_info) {
@@ -702,7 +702,7 @@ void show_log(struct rev_info *opt)
 	}
 
 	if (opt->show_log_size) {
-		printf("log size %i\n", (int)msgbuf.len);
+		fprintf(opt->diffopt.file, "log size %i\n", (int)msgbuf.len);
 		graph_show_oneline(opt->graph);
 	}
 
@@ -718,11 +718,11 @@ void show_log(struct rev_info *opt)
 	if (opt->graph)
 		graph_show_commit_msg(opt->graph, &msgbuf);
 	else
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, opt->diffopt.file);
 	if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar(opt->diffopt.line_termination);
+		fputc(opt->diffopt.line_termination, opt->diffopt.file);
 	}
 
 	strbuf_release(&msgbuf);
@@ -759,7 +759,7 @@ int log_tree_diff_flush(struct rev_info *opt)
 				struct strbuf *msg = NULL;
 				msg = opt->diffopt.output_prefix(&opt->diffopt,
 					opt->diffopt.output_prefix_data);
-				fwrite(msg->buf, msg->len, 1, stdout);
+				fwrite(msg->buf, msg->len, 1, opt->diffopt.file);
 			}
 
 			/*
@@ -774,8 +774,8 @@ int log_tree_diff_flush(struct rev_info *opt)
 			 */
 			if (!opt->shown_dashes &&
 			    (pch & opt->diffopt.output_format) == pch)
-				printf("---");
-			putchar('\n');
+				fprintf(opt->diffopt.file, "---");
+			fputc('\n', opt->diffopt.file);
 		}
 	}
 	diff_flush(&opt->diffopt);
@@ -872,7 +872,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 		return line_log_print(opt, commit);
 
 	if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
-		printf("\n%s\n", opt->break_bar);
+		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	shown = log_tree_diff(opt, commit, &log);
 	if (!shown && opt->loginfo && opt->always_show_header) {
 		log.parent = NULL;
@@ -880,8 +880,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 		shown = 1;
 	}
 	if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
-		printf("\n%s\n", opt->break_bar);
+		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	opt->loginfo = NULL;
-	maybe_flush_or_die(stdout, "stdout");
+	if (opt->diffopt.file == stdout)
+		maybe_flush_or_die(stdout, "stdout");
 	return shown;
 }
-- 
2.9.0.119.g370c5a9

[PATCH v2 2/7] line-log: respect diffopt's configured output file stream

[Johannes Schindelin]

The diff machinery can optionally output to a file stream other than
stdout, by overriding diffopt.file. In such a case, the rest of the
log tree machinery should also write to that stream.

Currently, there is no user of the line level log that wants to
redirect output to a file. Therefore, one might argue that it is
superfluous to support that now. However, it is better to be
consistent now, rather than to face hard-to-debug problems later.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 line-log.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/line-log.c b/line-log.c
index bbe31ed..c3b8563 100644
--- a/line-log.c
+++ b/line-log.c
@@ -841,7 +841,7 @@ static char *get_nth_line(long line, unsigned long *ends, void *data)
 
 static void print_line(const char *prefix, char first,
 		       long line, unsigned long *ends, void *data,
-		       const char *color, const char *reset)
+		       const char *color, const char *reset, FILE *file)
 {
 	char *begin = get_nth_line(line, ends, data);
 	char *end = get_nth_line(line+1, ends, data);
@@ -852,14 +852,14 @@ static void print_line(const char *prefix, char first,
 		had_nl = 1;
 	}
 
-	fputs(prefix, stdout);
-	fputs(color, stdout);
-	putchar(first);
-	fwrite(begin, 1, end-begin, stdout);
-	fputs(reset, stdout);
-	putchar('\n');
+	fputs(prefix, file);
+	fputs(color, file);
+	fputc(first, file);
+	fwrite(begin, 1, end-begin, file);
+	fputs(reset, file);
+	fputc('\n', file);
 	if (!had_nl)
-		fputs("\\ No newline at end of file\n", stdout);
+		fputs("\\ No newline at end of file\n", file);
 }
 
 static char *output_prefix(struct diff_options *opt)
@@ -898,12 +898,12 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 		fill_line_ends(pair->one, &p_lines, &p_ends);
 	fill_line_ends(pair->two, &t_lines, &t_ends);
 
-	printf("%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
-	printf("%s%s--- %s%s%s\n", prefix, c_meta,
+	fprintf(opt->file, "%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
+	fprintf(opt->file, "%s%s--- %s%s%s\n", prefix, c_meta,
 	       pair->one->sha1_valid ? "a/" : "",
 	       pair->one->sha1_valid ? pair->one->path : "/dev/null",
 	       c_reset);
-	printf("%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
+	fprintf(opt->file, "%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
 	for (i = 0; i < range->ranges.nr; i++) {
 		long p_start, p_end;
 		long t_start = range->ranges.ranges[i].start;
@@ -945,7 +945,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 		}
 
 		/* Now output a diff hunk for this range */
-		printf("%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
+		fprintf(opt->file, "%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
 		       prefix, c_frag,
 		       p_start+1, p_end-p_start, t_start+1, t_end-t_start,
 		       c_reset);
@@ -953,18 +953,18 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 			int k;
 			for (; t_cur < diff->target.ranges[j].start; t_cur++)
 				print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
-					   c_context, c_reset);
+					   c_context, c_reset, opt->file);
 			for (k = diff->parent.ranges[j].start; k < diff->parent.ranges[j].end; k++)
 				print_line(prefix, '-', k, p_ends, pair->one->data,
-					   c_old, c_reset);
+					   c_old, c_reset, opt->file);
 			for (; t_cur < diff->target.ranges[j].end && t_cur < t_end; t_cur++)
 				print_line(prefix, '+', t_cur, t_ends, pair->two->data,
-					   c_new, c_reset);
+					   c_new, c_reset, opt->file);
 			j++;
 		}
 		for (; t_cur < t_end; t_cur++)
 			print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
-				   c_context, c_reset);
+				   c_context, c_reset, opt->file);
 	}
 
 	free(p_ends);
@@ -977,7 +977,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
  */
 static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)
 {
-	puts(output_prefix(&rev->diffopt));
+	fprintf(rev->diffopt.file, "%s\n", output_prefix(&rev->diffopt));
 	while (range) {
 		dump_diff_hacky_one(rev, range);
 		range = range->next;
-- 
2.9.0.119.g370c5a9

[PATCH v2 3/7] graph: respect the diffopt.file setting

[Johannes Schindelin]

When the caller overrides diffopt.file (which defaults to stdout),
the diff machinery already redirects its output, and the graph display
should also write to that file.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 graph.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/graph.c b/graph.c
index 1350bdd..8ae56bc 100644
--- a/graph.c
+++ b/graph.c
@@ -17,8 +17,8 @@
 static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
 
 /*
- * Print a strbuf to stdout.  If the graph is non-NULL, all lines but the
- * first will be prefixed with the graph output.
+ * Print a strbuf.  If the graph is non-NULL, all lines but the first will be
+ * prefixed with the graph output.
  *
  * If the strbuf ends with a newline, the output will end after this
  * newline.  A new graph line will not be printed after the final newline.
@@ -1193,9 +1193,10 @@ void graph_show_commit(struct git_graph *graph)
 
 	while (!shown_commit_line && !graph_is_commit_finished(graph)) {
 		shown_commit_line = graph_next_line(graph, &msgbuf);
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+			graph->revs->diffopt.file);
 		if (!shown_commit_line)
-			putchar('\n');
+			fputc('\n', graph->revs->diffopt.file);
 		strbuf_setlen(&msgbuf, 0);
 	}
 
@@ -1210,7 +1211,7 @@ void graph_show_oneline(struct git_graph *graph)
 		return;
 
 	graph_next_line(graph, &msgbuf);
-	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
 	strbuf_release(&msgbuf);
 }
 
@@ -1222,7 +1223,7 @@ void graph_show_padding(struct git_graph *graph)
 		return;
 
 	graph_padding_line(graph, &msgbuf);
-	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
 	strbuf_release(&msgbuf);
 }
 
@@ -1239,12 +1240,13 @@ int graph_show_remainder(struct git_graph *graph)
 
 	for (;;) {
 		graph_next_line(graph, &msgbuf);
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+			graph->revs->diffopt.file);
 		strbuf_setlen(&msgbuf, 0);
 		shown = 1;
 
 		if (!graph_is_commit_finished(graph))
-			putchar('\n');
+			fputc('\n', graph->revs->diffopt.file);
 		else
 			break;
 	}
@@ -1259,7 +1261,8 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 	char *p;
 
 	if (!graph) {
-		fwrite(sb->buf, sizeof(char), sb->len, stdout);
+		fwrite(sb->buf, sizeof(char), sb->len,
+			graph->revs->diffopt.file);
 		return;
 	}
 
@@ -1277,7 +1280,7 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 		} else {
 			len = (sb->buf + sb->len) - p;
 		}
-		fwrite(p, sizeof(char), len, stdout);
+		fwrite(p, sizeof(char), len, graph->revs->diffopt.file);
 		if (next_p && *next_p != '\0')
 			graph_show_oneline(graph);
 		p = next_p;
@@ -1297,7 +1300,8 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * CMIT_FMT_USERFORMAT are already missing a terminating
 		 * newline.  All of the other formats should have it.
 		 */
-		fwrite(sb->buf, sizeof(char), sb->len, stdout);
+		fwrite(sb->buf, sizeof(char), sb->len,
+			graph->revs->diffopt.file);
 		return;
 	}
 
@@ -1318,7 +1322,7 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * new line.
 		 */
 		if (!newline_terminated)
-			putchar('\n');
+			fputc('\n', graph->revs->diffopt.file);
 
 		graph_show_remainder(graph);
 
@@ -1326,6 +1330,6 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * If sb ends with a newline, our output should too.
 		 */
 		if (newline_terminated)
-			putchar('\n');
+			fputc('\n', graph->revs->diffopt.file);
 	}
 }
-- 
2.9.0.119.g370c5a9

[PATCH v2 4/7] shortlog: support outputting to streams other than stdout

[Johannes Schindelin]

This will be needed to avoid freopen() in `git format-patch`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/shortlog.c | 13 ++++++++-----
 shortlog.h         |  1 +
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index bfc082e..4c68ba7 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -229,6 +229,7 @@ void shortlog_init(struct shortlog *log)
 	log->wrap = DEFAULT_WRAPLEN;
 	log->in1 = DEFAULT_INDENT1;
 	log->in2 = DEFAULT_INDENT2;
+	log->file = stdout;
 }
 
 int cmd_shortlog(int argc, const char **argv, const char *prefix)
@@ -310,22 +311,24 @@ void shortlog_output(struct shortlog *log)
 	for (i = 0; i < log->list.nr; i++) {
 		const struct string_list_item *item = &log->list.items[i];
 		if (log->summary) {
-			printf("%6d\t%s\n", (int)UTIL_TO_INT(item), item->string);
+			fprintf(log->file, "%6d\t%s\n",
+				(int)UTIL_TO_INT(item), item->string);
 		} else {
 			struct string_list *onelines = item->util;
-			printf("%s (%d):\n", item->string, onelines->nr);
+			fprintf(log->file, "%s (%d):\n",
+				item->string, onelines->nr);
 			for (j = onelines->nr - 1; j >= 0; j--) {
 				const char *msg = onelines->items[j].string;
 
 				if (log->wrap_lines) {
 					strbuf_reset(&sb);
 					add_wrapped_shortlog_msg(&sb, msg, log);
-					fwrite(sb.buf, sb.len, 1, stdout);
+					fwrite(sb.buf, sb.len, 1, log->file);
 				}
 				else
-					printf("      %s\n", msg);
+					fprintf(log->file, "      %s\n", msg);
 			}
-			putchar('\n');
+			fputc('\n', log->file);
 			onelines->strdup_strings = 1;
 			string_list_clear(onelines, 0);
 			free(onelines);
diff --git a/shortlog.h b/shortlog.h
index de4f86f..5a326c6 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -17,6 +17,7 @@ struct shortlog {
 	char *common_repo_prefix;
 	int email;
 	struct string_list mailmap;
+	FILE *file;
 };
 
 void shortlog_init(struct shortlog *log);
-- 
2.9.0.119.g370c5a9

[PATCH v2 5/7] format-patch: explicitly switch off color when writing to files

[Johannes Schindelin]

We rely on the auto-detection ("is stdout a terminal?") to determine
whether to use color in the output of format-patch or not. That happens
to work because we freopen() stdout when redirecting the output to files.

However, we are about to fix that work-around, in which case the
auto-detection has no chance to guess whether to use color or not.

But then, we do not need to guess to begin with. We know that we do not
want to use ANSI color sequences in the output files. So let's just be
explicit about our wishes.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/log.c b/builtin/log.c
index 099f4f7..abd889b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1569,6 +1569,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		setup_pager();
 
 	if (output_directory) {
+		rev.diffopt.use_color = 0;
 		if (use_stdout)
 			die(_("standard output, or directory, which one?"));
 		if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
-- 
2.9.0.119.g370c5a9

[PATCH v2 6/7] format-patch: avoid freopen()

[Johannes Schindelin]

We just taught the relevant functions to respect the diffopt.file field,
to allow writing somewhere else than stdout. Let's make use of it.

Technically, we do not need to avoid that call in a builtin: we assume
that builtins (as opposed to library functions) are stand-alone programs
that may do with their (global) state. Yet, we want to be able to reuse
that code in properly lib-ified code, e.g. when converting scripts into
builtins.

Further, while we did not *have* to touch the cmd_show() and cmd_cherry()
code paths (because they do not want to write anywhere but stdout as of
yet), it just makes sense to be consistent, making it easier and safer to
move the code later.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 64 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index abd889b..db034a8 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -236,7 +236,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)
 		if (rev->commit_format != CMIT_FMT_ONELINE)
 			putchar(rev->diffopt.line_termination);
 	}
-	printf(_("Final output: %d %s\n"), nr, stage);
+	fprintf(rev->diffopt.file, _("Final output: %d %s\n"), nr, stage);
 }
 
 static struct itimerval early_output_timer;
@@ -445,7 +445,7 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
 	pp.fmt = rev->commit_format;
 	pp.date_mode = rev->date_mode;
 	pp_user_info(&pp, "Tagger", &out, buf, get_log_output_encoding());
-	printf("%s", out.buf);
+	fprintf(rev->diffopt.file, "%s", out.buf);
 	strbuf_release(&out);
 }
 
@@ -456,7 +456,7 @@ static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, con
 	char *buf;
 	unsigned long size;
 
-	fflush(stdout);
+	fflush(rev->diffopt.file);
 	if (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||
 	    !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
 		return stream_blob_to_fd(1, sha1, NULL, 0);
@@ -496,7 +496,7 @@ static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)
 	}
 
 	if (offset < size)
-		fwrite(buf + offset, size - offset, 1, stdout);
+		fwrite(buf + offset, size - offset, 1, rev->diffopt.file);
 	free(buf);
 	return 0;
 }
@@ -505,7 +505,8 @@ static int show_tree_object(const unsigned char *sha1,
 		struct strbuf *base,
 		const char *pathname, unsigned mode, int stage, void *context)
 {
-	printf("%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
+	FILE *file = context;
+	fprintf(file, "%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
 	return 0;
 }
 
@@ -565,7 +566,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 
 			if (rev.shown_one)
 				putchar('\n');
-			printf("%stag %s%s\n",
+			fprintf(rev.diffopt.file, "%stag %s%s\n",
 					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					t->tag,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
@@ -584,12 +585,12 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 		case OBJ_TREE:
 			if (rev.shown_one)
 				putchar('\n');
-			printf("%stree %s%s\n\n",
+			fprintf(rev.diffopt.file, "%stree %s%s\n\n",
 					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					name,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
 			read_tree_recursive((struct tree *)o, "", 0, 0, &match_all,
-					show_tree_object, NULL);
+					show_tree_object, rev.diffopt.file);
 			rev.shown_one = 1;
 			break;
 		case OBJ_COMMIT:
@@ -799,7 +800,7 @@ static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
-static int reopen_stdout(struct commit *commit, const char *subject,
+static int open_next_file(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
@@ -823,7 +824,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 	if (!quiet)
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
 
-	if (freopen(filename.buf, "w", stdout) == NULL)
+	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
 		return error(_("Cannot open patch file %s"), filename.buf);
 
 	strbuf_release(&filename);
@@ -882,15 +883,15 @@ static void gen_message_id(struct rev_info *info, char *base)
 	info->message_id = strbuf_detach(&buf, NULL);
 }
 
-static void print_signature(void)
+static void print_signature(FILE *file)
 {
 	if (!signature || !*signature)
 		return;
 
-	printf("-- \n%s", signature);
+	fprintf(file, "-- \n%s", signature);
 	if (signature[strlen(signature)-1] != '\n')
-		putchar('\n');
-	putchar('\n');
+		fputc('\n', file);
+	fputc('\n', file);
 }
 
 static void add_branch_description(struct strbuf *buf, const char *branch_name)
@@ -959,7 +960,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	committer = git_committer_info(0);
 
 	if (!use_stdout &&
-	    reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
+	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
 		return;
 
 	log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
@@ -982,7 +983,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
 	pp_remainder(&pp, &msg, &sb, 0);
 	add_branch_description(&sb, branch_name);
-	printf("%s\n", sb.buf);
+	fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
 	strbuf_release(&sb);
 
@@ -991,6 +992,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	log.wrap = 72;
 	log.in1 = 2;
 	log.in2 = 4;
+	log.file = rev->diffopt.file;
 	for (i = 0; i < nr; i++)
 		shortlog_add_commit(&log, list[i]);
 
@@ -1013,8 +1015,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	diffcore_std(&opts);
 	diff_flush(&opts);
 
-	printf("\n");
-	print_signature();
+	fprintf(rev->diffopt.file, "\n");
+	print_signature(rev->diffopt.file);
 }
 
 static const char *clean_message_id(const char *msg_id)
@@ -1324,7 +1326,7 @@ static void prepare_bases(struct base_tree_info *bases,
 	}
 }
 
-static void print_bases(struct base_tree_info *bases)
+static void print_bases(struct base_tree_info *bases, FILE *file)
 {
 	int i;
 
@@ -1333,11 +1335,11 @@ static void print_bases(struct base_tree_info *bases)
 		return;
 
 	/* Show the base commit */
-	printf("base-commit: %s\n", oid_to_hex(&bases->base_commit));
+	fprintf(file, "base-commit: %s\n", oid_to_hex(&bases->base_commit));
 
 	/* Show the prerequisite patches */
 	for (i = bases->nr_patch_id - 1; i >= 0; i--)
-		printf("prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
+		fprintf(file, "prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
 
 	free(bases->patch_id);
 	bases->nr_patch_id = 0;
@@ -1694,7 +1696,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout,
 				  origin, nr, list, branch_name, quiet);
-		print_bases(&bases);
+		print_bases(&bases, rev.diffopt.file);
 		total++;
 		start_number--;
 	}
@@ -1740,7 +1742,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!use_stdout &&
-		    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
+		    open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
 			die(_("Failed to create output files"));
 		shown = log_tree_commit(&rev, commit);
 		free_commit_buffer(commit);
@@ -1755,15 +1757,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			rev.shown_one = 0;
 		if (shown) {
 			if (rev.mime_boundary)
-				printf("\n--%s%s--\n\n\n",
+				fprintf(rev.diffopt.file, "\n--%s%s--\n\n\n",
 				       mime_boundary_leader,
 				       rev.mime_boundary);
 			else
-				print_signature();
-			print_bases(&bases);
+				print_signature(rev.diffopt.file);
+			print_bases(&bases, rev.diffopt.file);
 		}
 		if (!use_stdout)
-			fclose(stdout);
+			fclose(rev.diffopt.file);
 	}
 	free(list);
 	free(branch_name);
@@ -1795,15 +1797,15 @@ static const char * const cherry_usage[] = {
 };
 
 static void print_commit(char sign, struct commit *commit, int verbose,
-			 int abbrev)
+			 int abbrev, FILE *file)
 {
 	if (!verbose) {
-		printf("%c %s\n", sign,
+		fprintf(file, "%c %s\n", sign,
 		       find_unique_abbrev(commit->object.oid.hash, abbrev));
 	} else {
 		struct strbuf buf = STRBUF_INIT;
 		pp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);
-		printf("%c %s %s\n", sign,
+		fprintf(file, "%c %s %s\n", sign,
 		       find_unique_abbrev(commit->object.oid.hash, abbrev),
 		       buf.buf);
 		strbuf_release(&buf);
@@ -1884,7 +1886,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 		commit = list->item;
 		if (has_commit_patch_id(commit, &ids))
 			sign = '-';
-		print_commit(sign, commit, verbose, abbrev);
+		print_commit(sign, commit, verbose, abbrev, revs.diffopt.file);
 		list = list->next;
 	}
 
-- 
2.9.0.119.g370c5a9

[PATCH v2 7/7] format-patch: use stdout directly

[Johannes Schindelin]

Earlier, we freopen()ed stdout in order to write patches to files.
That forced us to duplicate stdout (naming it "realstdout") because we
*still* wanted to be able to report the file names.

As we do not abuse stdout that way anymore, we no longer need to
duplicate stdout, either.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index db034a8..5a889d5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -796,7 +796,6 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	return git_log_config(var, value, cb);
 }
 
-static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
@@ -822,7 +821,7 @@ static int open_next_file(struct commit *commit, const char *subject,
 		fmt_output_subject(&filename, subject, rev);
 
 	if (!quiet)
-		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
+		printf("%s\n", filename.buf + outdir_offset);
 
 	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
 		return error(_("Cannot open patch file %s"), filename.buf);
@@ -1629,9 +1628,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		get_patch_ids(&rev, &ids);
 	}
 
-	if (!use_stdout)
-		realstdout = xfdopen(xdup(1), "w");
-
 	if (prepare_revision_walk(&rev))
 		die(_("revision walk setup failed"));
 	rev.boundary = 1;
-- 
2.9.0.119.g370c5a9

Re: [PATCH v2 0/7] Let log-tree and friends respect diffopt's `file` field

[Johannes Schindelin]

Hi,

[sorry, Eric, forgot to Cc: you on the cover letter...]

On Mon, 20 Jun 2016, Johannes Schindelin wrote:

> Johannes Schindelin (7):
>   log-tree: respect diffopt's configured output file stream
>   line-log: respect diffopt's configured output file stream
>   graph: respect the diffopt.file setting
>   shortlog: support outputting to streams other than stdout
>   format-patch: explicitly switch off color when writing to files
>   format-patch: avoid freopen()
>   format-patch: use stdout directly

The interdiff is empty: all I did was to split out the first and the third
`format-patch:` commit, to leave really only the relevant part in the
"avoid freopen()" commit.

Ciao,
Dscho

Re: [PATCH 5/5] format-patch: avoid freopen()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> When format-patch calls the diff machinery, want_color() is used to
> determine whether to use ANSI color sequences or not. If use_color is not
> set explicitly, isatty(1) is used to determine whether or not the user
> wants color. When stdout was freopen()ed, this isatty(1) call naturally
> looked at the file descriptor that was reopened, and determined correctly
> that no color was desired.
>
> With the freopen() call gone, stdout may very well be the terminal. But we
> still do not want color because the output is intended to go to a file (at
> least if output_directory is set).

How does this interact with --color=... that is given from the
command line at the same time?  Currently --color=always would
give you a coloured output.

I personally do not think of a sensible reason why anybody wants a
colored format-patch output, but Git's userbase has grown large
enough and you can no longer expect that only sensible people use it
;-)

You can probably sell "when giving out put to file, we will never
color the output" as an improved new world order, but if that is
what this change wants to do, it probably deserves a separate patch.

I however think you can avoid breaking expectations by people who
are not so sensible by overriding only when use_color is set to
GIT_COLOR_AUTO, perhaps?

Re: [PATCH v2 1/7] log-tree: respect diffopt's configured output file stream

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> The diff options already know how to print the output anywhere else
> than stdout. The same is needed for log output in general, e.g.
> when writing patches to files in `git format-patch`. Let's allow
> users to use log_tree_commit() *without* changing global state via
> freopen().

I wonder if this change is actually fixing existing bugs.  Are there
cases where diffopt.file is set, i.e. the user expects the output to
be sent elsewhere, but the code here unconditionally emits to the
standard output?  I suspect that such a bug can be demonstratable in
a test or two, if that were the case.

I am sort-of surprised that we didn't do this already even though we
had diffopt.file for a long time since c0c77734 (Write diff output
to a file in struct diff_options, 2008-03-09).

Use of freopen() to always write patches through stdout may have
been done as a lazy workaround of the issue this patch fixes, but
what is surprising to me is that doing it the right way like this
patch does is not that much of work.  Perhaps that was done long
before c0c77734 was done, which would mean doing it the right way
back then when we started using freopen() it would have been a lot
more work and we thought taking a short-cut was warranted.

In any case, this is a change in the good direction.  Thanks for
cleaning things up.

>  		if (opt->children.name)
>  			show_children(opt, commit, abbrev_commit);
>  		show_decorations(opt, commit);
>  		if (opt->graph && !graph_is_commit_finished(opt->graph)) {
> -			putchar('\n');
> +			fputc('\n', opt->diffopt.file);

Hmph.  putc() is the "to the given stream" equivalent of putchar()
in the "send to stdout" world, not fputc().  I do not see a reason
to force the call to go to a function avoiding a possible macro here.

Likewise for all the new fputc() calls in this series that were
originally putchar().

> @@ -880,8 +880,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
>  		shown = 1;
>  	}
>  	if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
> -		printf("\n%s\n", opt->break_bar);
> +		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
>  	opt->loginfo = NULL;
> -	maybe_flush_or_die(stdout, "stdout");
> +	if (opt->diffopt.file == stdout)
> +		maybe_flush_or_die(stdout, "stdout");
>  	return shown;
>  }

This one looks fishy.

Back when we freopen()'ed to write patches only through stdout, we
always called maybe_flush_or_die() to make sure that the output is
flushed correctly after processing each commit.  This change makes
it not to care, which I doubt was what you intended.  Instead, my
suspicion is that you didn't want to say "stdout" when writing into
a file.

But even when writing to on-disk files, the code before your series
would have said "stdout" when it had trouble flushing, so I do not
think this new "if()" condition is making things better.  If "it
said stdout when having trouble flushing to a file" were a problem
to be fixed, "let's not say stdout by not even attempting to flush
and catch errors when writing to a file" would not be the right
solution, no?

Personally, I do not think it hurts if we kept saying 'stdout' here,
even when we flush opt->diffopt.file and found a problem.

Thanks.

Re: [PATCH v2 7/7] format-patch: use stdout directly

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Earlier, we freopen()ed stdout in order to write patches to files.
> That forced us to duplicate stdout (naming it "realstdout") because we
> *still* wanted to be able to report the file names.
>
> As we do not abuse stdout that way anymore, we no longer need to
> duplicate stdout, either.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/log.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Overall, it was a pleasant read, modulo some details that I
mentioned separately elsewhere, i.e.

 - we do not need to lose flush-or-die
 - we do not need to lose --color=always
 - we do not need to lose macro-ness of putchar()

Thanks.

>
> diff --git a/builtin/log.c b/builtin/log.c
> index db034a8..5a889d5 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -796,7 +796,6 @@ static int git_format_config(const char *var, const char *value, void *cb)
>  	return git_log_config(var, value, cb);
>  }
>  
> -static FILE *realstdout = NULL;
>  static const char *output_directory = NULL;
>  static int outdir_offset;
>  
> @@ -822,7 +821,7 @@ static int open_next_file(struct commit *commit, const char *subject,
>  		fmt_output_subject(&filename, subject, rev);
>  
>  	if (!quiet)
> -		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
> +		printf("%s\n", filename.buf + outdir_offset);
>  
>  	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
>  		return error(_("Cannot open patch file %s"), filename.buf);
> @@ -1629,9 +1628,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		get_patch_ids(&rev, &ids);
>  	}
>  
> -	if (!use_stdout)
> -		realstdout = xfdopen(xdup(1), "w");
> -
>  	if (prepare_revision_walk(&rev))
>  		die(_("revision walk setup failed"));
>  	rev.boundary = 1;

Re: [PATCH 5/5] format-patch: avoid freopen()

[Johannes Schindelin]

Hi Junio,

On Mon, 20 Jun 2016, Junio C Hamano wrote:

> You can probably sell "when giving out put to file, we will never color
> the output" as an improved new world order, but if that is what this
> change wants to do, it probably deserves a separate patch.
> 
> I however think you can avoid breaking expectations by people who are
> not so sensible by overriding only when use_color is set to
> GIT_COLOR_AUTO, perhaps?

That is a very convincing argument. So convincing that I wanted to change
the patch to guard behind `diff_use_color_default == GIT_COLOR_AUTO`. But
that is the wrong variable: the variable that *has* that default value is
git_use_color_default, and is private to color.c.

But then I dug further to determine under which circumstances that
variable can be reset to any different value. It turns out that in
format-patch's case, it cannot:

	787570c (format-patch: ignore ui.color, 2011-09-13)

I hope you agree that it will be enough to augment the commit message with
this analysis and keep the patch as-is (as per v2, that is)?

Ciao,
Dscho

Re: [PATCH v2 1/7] log-tree: respect diffopt's configured output file stream

[Johannes Schindelin]

Hi Junio,

On Mon, 20 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > The diff options already know how to print the output anywhere else
> > than stdout. The same is needed for log output in general, e.g.  when
> > writing patches to files in `git format-patch`. Let's allow users to
> > use log_tree_commit() *without* changing global state via freopen().
> 
> I wonder if this change is actually fixing existing bugs.  Are there
> cases where diffopt.file is set, i.e. the user expects the output to be
> sent elsewhere, but the code here unconditionally emits to the standard
> output?  I suspect that such a bug can be demonstratable in a test or
> two, if that were the case.

It is conceivable, but I did not have time to chase those cases down yet.

> I am sort-of surprised that we didn't do this already even though we
> had diffopt.file for a long time since c0c77734 (Write diff output
> to a file in struct diff_options, 2008-03-09).
> 
> Use of freopen() to always write patches through stdout may have
> been done as a lazy workaround of the issue this patch fixes, but
> what is surprising to me is that doing it the right way like this
> patch does is not that much of work.  Perhaps that was done long
> before c0c77734 was done, which would mean doing it the right way
> back then when we started using freopen() it would have been a lot
> more work and we thought taking a short-cut was warranted.

Back when I implemented the feature to write to individual files, I indeed
used freopen() out of laziness: 0377db7 (Teach fmt-patch to write
individual files., 2006-05-05). I could not have used diffopt.file at that
stage, anyway: that field still was almost two years in the future.

> >  		if (opt->children.name)
> >  			show_children(opt, commit, abbrev_commit);
> >  		show_decorations(opt, commit);
> >  		if (opt->graph && !graph_is_commit_finished(opt->graph)) {
> > -			putchar('\n');
> > +			fputc('\n', opt->diffopt.file);
> 
> Hmph.  putc() is the "to the given stream" equivalent of putchar()
> in the "send to stdout" world, not fputc().  I do not see a reason
> to force the call to go to a function avoiding a possible macro here.

TBH I did not even *know* putc(). It is amazing how you can learn new
things about the POSIX API after decades of working with it.

> Likewise for all the new fputc() calls in this series that were
> originally putchar().

Goes without saying.

> > @@ -880,8 +880,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
> >  		shown = 1;
> >  	}
> >  	if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
> > -		printf("\n%s\n", opt->break_bar);
> > +		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
> >  	opt->loginfo = NULL;
> > -	maybe_flush_or_die(stdout, "stdout");
> > +	if (opt->diffopt.file == stdout)
> > +		maybe_flush_or_die(stdout, "stdout");
> >  	return shown;
> >  }
> 
> This one looks fishy.
> 
> Back when we freopen()'ed to write patches only through stdout, we
> always called maybe_flush_or_die() to make sure that the output is
> flushed correctly after processing each commit.  This change makes
> it not to care, which I doubt was what you intended.  Instead, my
> suspicion is that you didn't want to say "stdout" when writing into
> a file.
> 
> But even when writing to on-disk files, the code before your series
> would have said "stdout" when it had trouble flushing, so I do not
> think this new "if()" condition is making things better.  If "it
> said stdout when having trouble flushing to a file" were a problem
> to be fixed, "let's not say stdout by not even attempting to flush
> and catch errors when writing to a file" would not be the right
> solution, no?
> 
> Personally, I do not think it hurts if we kept saying 'stdout' here,
> even when we flush opt->diffopt.file and found a problem.

Okay, I changed it back to be unconditional.

My original thinking was that we will fclose() the file (if it is not
stdout) anyway, which implies flushing. But the more I think about it,
the more I come to the conclusion that this is more of a side effect,
based on deep knowledge of the current code. So I now agree with you that
it would be "too clever".

Expect v3 in a moment.

Ciao,
Dscho

Re: [PATCH v2 1/7] log-tree: respect diffopt's configured output file stream

[Johannes Schindelin]

Hi,

On Tue, 21 Jun 2016, Johannes Schindelin wrote:

> Expect v3 in a moment.

I am sorry. The regression test suite just sounded red alert. So: do not
expect v3 in a moment, but later today.

Ciao,
Dscho

[PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field

[Johannes Schindelin]

The idea is to allow callers to redirect log-tree's output to a file
without having to freopen() stdout (which would modify global state,
a big no-no-no for library functions).

I reviewed log-tree.c, graph.c, line-log.c, builtin/shortlog.c and
builtin/log.c line by line to ensure that all calls that assumed stdout
previously now use the `file` field instead, of course. I would
welcome additional eyes to go over the code to confirm that I did not
miss anything.

This patch series ends up removing the freopen() call in the
format-patch command, but that is only a by-product. The ulterior motive
behind this series is to allow the sequencer to write a `patch` file as
part of my endeavor to move large chunks of rebase -i into a builtin.

As mentioned earlier, the `use_color = 0` setting is still kept
unconditional because format-patch explicitly ignores the ui.color
setting.

This iteration has the following changes with regards to the previous
one:

- putchar() is replaced with putc(), not fputc()

- the maybe_flush_or_die() function is now called by log_tree_commit()
  even if outputting to a file stream different from stdout

- this uncovered a problem with builtin am, where it asked the diff
  machinery to close the file stream, but actually called the log_tree
  machinery (which might mean that this patch series inadvertently fixes
  a bug where `git am --rebasing` would write the commit message to
  stdout instead of the `patch` file when erroring out)

This last point is a bigger issue, actually. There seem to be quite a
few function calls in builtin/am.c whose return values that might
indicate errors are flatly ignored. I see two calls to run_diff_index()
whose return value then goes poof unchecked, and several calls to
write_state_text() and write_state_bool() with the same issue. And I did
not even try to review the code to that end, all I wanted was to verify
that builtin am only has the close_file issue once (it does use it a
second time, but that one is okay because it then calls
run_diff_index(), i.e. the diff machinery).

I am embarrassed to admit that these builtin am problems demonstrate
that I, as a mentor of the builtin am project, failed to help make the
patches as good as I expected myself to do.


Johannes Schindelin (9):
  am: stop ignoring errors reported by log_tree_diff()
  Disallow diffopt.close_file when using the log_tree machinery
  log-tree: respect diffopt's configured output file stream
  line-log: respect diffopt's configured output file stream
  graph: respect the diffopt.file setting
  shortlog: support outputting to streams other than stdout
  format-patch: explicitly switch off color when writing to files
  format-patch: avoid freopen()
  format-patch: use stdout directly

 builtin/am.c       | 18 +++++++++-----
 builtin/log.c      | 71 +++++++++++++++++++++++++++---------------------------
 builtin/shortlog.c | 13 ++++++----
 graph.c            | 30 +++++++++++++----------
 line-log.c         | 34 +++++++++++++-------------
 log-tree.c         | 67 +++++++++++++++++++++++++++------------------------
 shortlog.h         |  1 +
 7 files changed, 125 insertions(+), 109 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/diffopt.file-v3
Interdiff vs v2:

 diff --git a/builtin/am.c b/builtin/am.c
 index 3dfe70b..47d78aa 100644
 --- a/builtin/am.c
 +++ b/builtin/am.c
 @@ -1433,12 +1433,16 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
  /**
   * Writes `commit` as a patch to the state directory's "patch" file.
   */
 -static void write_commit_patch(const struct am_state *state, struct commit *commit)
 +static int write_commit_patch(const struct am_state *state, struct commit *commit)
  {
  	struct rev_info rev_info;
  	FILE *fp;
 +	int res;
  
 -	fp = xfopen(am_path(state, "patch"), "w");
 +	fp = fopen(am_path(state, "patch"), "w");
 +	if (!fp)
 +		return error(_("Could not open %s for writing"),
 +			am_path(state, "patch"));
  	init_revisions(&rev_info, NULL);
  	rev_info.diff = 1;
  	rev_info.abbrev = 0;
 @@ -1450,10 +1454,11 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
  	DIFF_OPT_SET(&rev_info.diffopt, FULL_INDEX);
  	rev_info.diffopt.use_color = 0;
  	rev_info.diffopt.file = fp;
 -	rev_info.diffopt.close_file = 1;
  	add_pending_object(&rev_info, &commit->object, "");
  	diff_setup_done(&rev_info.diffopt);
 -	log_tree_commit(&rev_info, commit);
 +	res = log_tree_commit(&rev_info, commit);
 +	fclose(fp);
 +	return res;
  }
  
  /**
 @@ -1501,13 +1506,14 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
  	unsigned char commit_sha1[GIT_SHA1_RAWSZ];
  
  	if (get_mail_commit_sha1(commit_sha1, mail) < 0)
 -		die(_("could not parse %s"), mail);
 +		return error(_("could not parse %s"), mail);
  
  	commit = lookup_commit_or_die(commit_sha1, mail);
  
  	get_commit_info(state, commit);
  
 -	write_commit_patch(state, commit);
 +	if (write_commit_patch(state, commit) < 0)
 +		return -1;
  
  	hashcpy(state->orig_commit, commit_sha1);
  	write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));
 diff --git a/builtin/log.c b/builtin/log.c
 index 5a889d5..2bfcc43 100644
 --- a/builtin/log.c
 +++ b/builtin/log.c
 @@ -889,8 +889,8 @@ static void print_signature(FILE *file)
  
  	fprintf(file, "-- \n%s", signature);
  	if (signature[strlen(signature)-1] != '\n')
 -		fputc('\n', file);
 -	fputc('\n', file);
 +		putc('\n', file);
 +	putc('\n', file);
  }
  
  static void add_branch_description(struct strbuf *buf, const char *branch_name)
 diff --git a/builtin/shortlog.c b/builtin/shortlog.c
 index 4c68ba7..39d74fe 100644
 --- a/builtin/shortlog.c
 +++ b/builtin/shortlog.c
 @@ -328,7 +328,7 @@ void shortlog_output(struct shortlog *log)
  				else
  					fprintf(log->file, "      %s\n", msg);
  			}
 -			fputc('\n', log->file);
 +			putc('\n', log->file);
  			onelines->strdup_strings = 1;
  			string_list_clear(onelines, 0);
  			free(onelines);
 diff --git a/graph.c b/graph.c
 index 8ae56bc..8ad8ba3 100644
 --- a/graph.c
 +++ b/graph.c
 @@ -1196,7 +1196,7 @@ void graph_show_commit(struct git_graph *graph)
  		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
  			graph->revs->diffopt.file);
  		if (!shown_commit_line)
 -			fputc('\n', graph->revs->diffopt.file);
 +			putc('\n', graph->revs->diffopt.file);
  		strbuf_setlen(&msgbuf, 0);
  	}
  
 @@ -1246,7 +1246,7 @@ int graph_show_remainder(struct git_graph *graph)
  		shown = 1;
  
  		if (!graph_is_commit_finished(graph))
 -			fputc('\n', graph->revs->diffopt.file);
 +			putc('\n', graph->revs->diffopt.file);
  		else
  			break;
  	}
 @@ -1322,7 +1322,7 @@ void graph_show_commit_msg(struct git_graph *graph,
  		 * new line.
  		 */
  		if (!newline_terminated)
 -			fputc('\n', graph->revs->diffopt.file);
 +			putc('\n', graph->revs->diffopt.file);
  
  		graph_show_remainder(graph);
  
 @@ -1330,6 +1330,6 @@ void graph_show_commit_msg(struct git_graph *graph,
  		 * If sb ends with a newline, our output should too.
  		 */
  		if (newline_terminated)
 -			fputc('\n', graph->revs->diffopt.file);
 +			putc('\n', graph->revs->diffopt.file);
  	}
  }
 diff --git a/line-log.c b/line-log.c
 index c3b8563..e62a7f4 100644
 --- a/line-log.c
 +++ b/line-log.c
 @@ -854,10 +854,10 @@ static void print_line(const char *prefix, char first,
  
  	fputs(prefix, file);
  	fputs(color, file);
 -	fputc(first, file);
 +	putc(first, file);
  	fwrite(begin, 1, end-begin, file);
  	fputs(reset, file);
 -	fputc('\n', file);
 +	putc('\n', file);
  	if (!had_nl)
  		fputs("\\ No newline at end of file\n", file);
  }
 diff --git a/log-tree.c b/log-tree.c
 index 968428a..530297d 100644
 --- a/log-tree.c
 +++ b/log-tree.c
 @@ -560,10 +560,10 @@ void show_log(struct rev_info *opt)
  			show_children(opt, commit, abbrev_commit);
  		show_decorations(opt, commit);
  		if (opt->graph && !graph_is_commit_finished(opt->graph)) {
 -			fputc('\n', opt->diffopt.file);
 +			putc('\n', opt->diffopt.file);
  			graph_show_remainder(opt->graph);
  		}
 -		fputc(opt->diffopt.line_termination, opt->diffopt.file);
 +		putc(opt->diffopt.line_termination, opt->diffopt.file);
  		return;
  	}
  
 @@ -589,7 +589,7 @@ void show_log(struct rev_info *opt)
  		if (opt->diffopt.line_termination == '\n' &&
  		    !opt->missing_newline)
  			graph_show_padding(opt->graph);
 -		fputc(opt->diffopt.line_termination, opt->diffopt.file);
 +		putc(opt->diffopt.line_termination, opt->diffopt.file);
  	}
  	opt->shown_one = 1;
  
 @@ -626,9 +626,9 @@ void show_log(struct rev_info *opt)
  		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
  		show_decorations(opt, commit);
  		if (opt->commit_format == CMIT_FMT_ONELINE) {
 -			fputc(' ', opt->diffopt.file);
 +			putc(' ', opt->diffopt.file);
  		} else {
 -			fputc('\n', opt->diffopt.file);
 +			putc('\n', opt->diffopt.file);
  			graph_show_oneline(opt->graph);
  		}
  		if (opt->reflog_info) {
 @@ -722,7 +722,7 @@ void show_log(struct rev_info *opt)
  	if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
  		if (!opt->missing_newline)
  			graph_show_padding(opt->graph);
 -		fputc(opt->diffopt.line_termination, opt->diffopt.file);
 +		putc(opt->diffopt.line_termination, opt->diffopt.file);
  	}
  
  	strbuf_release(&msgbuf);
 @@ -775,7 +775,7 @@ int log_tree_diff_flush(struct rev_info *opt)
  			if (!opt->shown_dashes &&
  			    (pch & opt->diffopt.output_format) == pch)
  				fprintf(opt->diffopt.file, "---");
 -			fputc('\n', opt->diffopt.file);
 +			putc('\n', opt->diffopt.file);
  		}
  	}
  	diff_flush(&opt->diffopt);
 @@ -864,6 +864,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
  	struct log_info log;
  	int shown;
  
 +	if (opt->diffopt.close_file)
 +		die("BUG: close_file is incompatible with log_tree_commit()");
 +
  	log.commit = commit;
  	log.parent = NULL;
  	opt->loginfo = &log;
 @@ -882,7 +885,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
  	if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
  		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
  	opt->loginfo = NULL;
 -	if (opt->diffopt.file == stdout)
 -		maybe_flush_or_die(stdout, "stdout");
 +	maybe_flush_or_die(opt->diffopt.file, "stdout");
  	return shown;
  }

-- 
2.9.0.118.g0e1a633

base-commit: ab7797dbe95fff38d9265869ea367020046db118

[PATCH v3 1/9] am: stop ignoring errors reported by log_tree_diff()

[Johannes Schindelin]

It is never a good idea to ignore errors, so let's handle them.

While at it, handle fopen() errors more gently (i.e. give the caller a
chance to do something about it rather than die()ing).

Note: there are more places in the builtin am code that ignore
errors returned from library functions. Fixing those is outside the
purview of the current patch series, though.

Cc: Paul Tan <pyokagan@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/am.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3dfe70b..0e28a62 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1433,12 +1433,15 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
 /**
  * Writes `commit` as a patch to the state directory's "patch" file.
  */
-static void write_commit_patch(const struct am_state *state, struct commit *commit)
+static int write_commit_patch(const struct am_state *state, struct commit *commit)
 {
 	struct rev_info rev_info;
 	FILE *fp;
 
-	fp = xfopen(am_path(state, "patch"), "w");
+	fp = fopen(am_path(state, "patch"), "w");
+	if (!fp)
+		return error(_("Could not open %s for writing"),
+			am_path(state, "patch"));
 	init_revisions(&rev_info, NULL);
 	rev_info.diff = 1;
 	rev_info.abbrev = 0;
@@ -1453,7 +1456,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
 	rev_info.diffopt.close_file = 1;
 	add_pending_object(&rev_info, &commit->object, "");
 	diff_setup_done(&rev_info.diffopt);
-	log_tree_commit(&rev_info, commit);
+	return log_tree_commit(&rev_info, commit);
 }
 
 /**
@@ -1501,13 +1504,14 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
 	unsigned char commit_sha1[GIT_SHA1_RAWSZ];
 
 	if (get_mail_commit_sha1(commit_sha1, mail) < 0)
-		die(_("could not parse %s"), mail);
+		return error(_("could not parse %s"), mail);
 
 	commit = lookup_commit_or_die(commit_sha1, mail);
 
 	get_commit_info(state, commit);
 
-	write_commit_patch(state, commit);
+	if (write_commit_patch(state, commit) < 0)
+		return -1;
 
 	hashcpy(state->orig_commit, commit_sha1);
 	write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));
-- 
2.9.0.118.g0e1a633

[PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery

[Johannes Schindelin]

We are about to teach the log_tree machinery to reuse the diffopt.file
setting to output to a file stream different from stdout.

This means that builtin am can no longer ask the diff machinery to
close the file when actually calling the log_tree machinery (which
wants to flush the very same file stream that would then already be
closed).

To stave off similar problems in the future, report it as a bug if
log_tree_commit() is called with a non-zero diffopt.close_file.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/am.c | 6 ++++--
 log-tree.c   | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 0e28a62..47d78aa 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1437,6 +1437,7 @@ static int write_commit_patch(const struct am_state *state, struct commit *commi
 {
 	struct rev_info rev_info;
 	FILE *fp;
+	int res;
 
 	fp = fopen(am_path(state, "patch"), "w");
 	if (!fp)
@@ -1453,10 +1454,11 @@ static int write_commit_patch(const struct am_state *state, struct commit *commi
 	DIFF_OPT_SET(&rev_info.diffopt, FULL_INDEX);
 	rev_info.diffopt.use_color = 0;
 	rev_info.diffopt.file = fp;
-	rev_info.diffopt.close_file = 1;
 	add_pending_object(&rev_info, &commit->object, "");
 	diff_setup_done(&rev_info.diffopt);
-	return log_tree_commit(&rev_info, commit);
+	res = log_tree_commit(&rev_info, commit);
+	fclose(fp);
+	return res;
 }
 
 /**
diff --git a/log-tree.c b/log-tree.c
index 78a5381..dc0180d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -864,6 +864,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 	struct log_info log;
 	int shown;
 
+	if (opt->diffopt.close_file)
+		die("BUG: close_file is incompatible with log_tree_commit()");
+
 	log.commit = commit;
 	log.parent = NULL;
 	opt->loginfo = &log;
-- 
2.9.0.118.g0e1a633

[PATCH v3 3/9] log-tree: respect diffopt's configured output file stream

[Johannes Schindelin]

The diff options already know how to print the output anywhere else
than stdout. The same is needed for log output in general, e.g.
when writing patches to files in `git format-patch`. Let's allow
users to use log_tree_commit() *without* changing global state via
freopen().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 log-tree.c | 64 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index dc0180d..530297d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -159,12 +159,12 @@ void load_ref_decorations(int flags)
 	}
 }
 
-static void show_parents(struct commit *commit, int abbrev)
+static void show_parents(struct commit *commit, int abbrev, FILE *file)
 {
 	struct commit_list *p;
 	for (p = commit->parents; p ; p = p->next) {
 		struct commit *parent = p->item;
-		printf(" %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
+		fprintf(file, " %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
 	}
 }
 
@@ -172,7 +172,7 @@ static void show_children(struct rev_info *opt, struct commit *commit, int abbre
 {
 	struct commit_list *p = lookup_decoration(&opt->children, &commit->object);
 	for ( ; p; p = p->next) {
-		printf(" %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
+		fprintf(opt->diffopt.file, " %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
 	}
 }
 
@@ -286,11 +286,11 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
 	struct strbuf sb = STRBUF_INIT;
 
 	if (opt->show_source && commit->util)
-		printf("\t%s", (char *) commit->util);
+		fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
 	if (!opt->show_decorations)
 		return;
 	format_decorations(&sb, commit, opt->diffopt.use_color);
-	fputs(sb.buf, stdout);
+	fputs(sb.buf, opt->diffopt.file);
 	strbuf_release(&sb);
 }
 
@@ -364,18 +364,18 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		subject = "Subject: ";
 	}
 
-	printf("From %s Mon Sep 17 00:00:00 2001\n", name);
+	fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);
 	graph_show_oneline(opt->graph);
 	if (opt->message_id) {
-		printf("Message-Id: <%s>\n", opt->message_id);
+		fprintf(opt->diffopt.file, "Message-Id: <%s>\n", opt->message_id);
 		graph_show_oneline(opt->graph);
 	}
 	if (opt->ref_message_ids && opt->ref_message_ids->nr > 0) {
 		int i, n;
 		n = opt->ref_message_ids->nr;
-		printf("In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
+		fprintf(opt->diffopt.file, "In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
 		for (i = 0; i < n; i++)
-			printf("%s<%s>\n", (i > 0 ? "\t" : "References: "),
+			fprintf(opt->diffopt.file, "%s<%s>\n", (i > 0 ? "\t" : "References: "),
 			       opt->ref_message_ids->items[i].string);
 		graph_show_oneline(opt->graph);
 	}
@@ -432,7 +432,7 @@ static void show_sig_lines(struct rev_info *opt, int status, const char *bol)
 	reset = diff_get_color_opt(&opt->diffopt, DIFF_RESET);
 	while (*bol) {
 		eol = strchrnul(bol, '\n');
-		printf("%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
+		fprintf(opt->diffopt.file, "%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
 		       *eol ? "\n" : "");
 		graph_show_oneline(opt->graph);
 		bol = (*eol) ? (eol + 1) : eol;
@@ -553,17 +553,17 @@ void show_log(struct rev_info *opt)
 
 		if (!opt->graph)
 			put_revision_mark(opt, commit);
-		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), stdout);
+		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), opt->diffopt.file);
 		if (opt->print_parents)
-			show_parents(commit, abbrev_commit);
+			show_parents(commit, abbrev_commit, opt->diffopt.file);
 		if (opt->children.name)
 			show_children(opt, commit, abbrev_commit);
 		show_decorations(opt, commit);
 		if (opt->graph && !graph_is_commit_finished(opt->graph)) {
-			putchar('\n');
+			putc('\n', opt->diffopt.file);
 			graph_show_remainder(opt->graph);
 		}
-		putchar(opt->diffopt.line_termination);
+		putc(opt->diffopt.line_termination, opt->diffopt.file);
 		return;
 	}
 
@@ -589,7 +589,7 @@ void show_log(struct rev_info *opt)
 		if (opt->diffopt.line_termination == '\n' &&
 		    !opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar(opt->diffopt.line_termination);
+		putc(opt->diffopt.line_termination, opt->diffopt.file);
 	}
 	opt->shown_one = 1;
 
@@ -607,28 +607,28 @@ void show_log(struct rev_info *opt)
 		log_write_email_headers(opt, commit, &ctx.subject, &extra_headers,
 					&ctx.need_8bit_cte);
 	} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
-		fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), stdout);
+		fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), opt->diffopt.file);
 		if (opt->commit_format != CMIT_FMT_ONELINE)
-			fputs("commit ", stdout);
+			fputs("commit ", opt->diffopt.file);
 
 		if (!opt->graph)
 			put_revision_mark(opt, commit);
 		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit),
-		      stdout);
+		      opt->diffopt.file);
 		if (opt->print_parents)
-			show_parents(commit, abbrev_commit);
+			show_parents(commit, abbrev_commit, opt->diffopt.file);
 		if (opt->children.name)
 			show_children(opt, commit, abbrev_commit);
 		if (parent)
-			printf(" (from %s)",
+			fprintf(opt->diffopt.file, " (from %s)",
 			       find_unique_abbrev(parent->object.oid.hash,
 						  abbrev_commit));
-		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), stdout);
+		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
 		show_decorations(opt, commit);
 		if (opt->commit_format == CMIT_FMT_ONELINE) {
-			putchar(' ');
+			putc(' ', opt->diffopt.file);
 		} else {
-			putchar('\n');
+			putc('\n', opt->diffopt.file);
 			graph_show_oneline(opt->graph);
 		}
 		if (opt->reflog_info) {
@@ -702,7 +702,7 @@ void show_log(struct rev_info *opt)
 	}
 
 	if (opt->show_log_size) {
-		printf("log size %i\n", (int)msgbuf.len);
+		fprintf(opt->diffopt.file, "log size %i\n", (int)msgbuf.len);
 		graph_show_oneline(opt->graph);
 	}
 
@@ -718,11 +718,11 @@ void show_log(struct rev_info *opt)
 	if (opt->graph)
 		graph_show_commit_msg(opt->graph, &msgbuf);
 	else
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, opt->diffopt.file);
 	if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar(opt->diffopt.line_termination);
+		putc(opt->diffopt.line_termination, opt->diffopt.file);
 	}
 
 	strbuf_release(&msgbuf);
@@ -759,7 +759,7 @@ int log_tree_diff_flush(struct rev_info *opt)
 				struct strbuf *msg = NULL;
 				msg = opt->diffopt.output_prefix(&opt->diffopt,
 					opt->diffopt.output_prefix_data);
-				fwrite(msg->buf, msg->len, 1, stdout);
+				fwrite(msg->buf, msg->len, 1, opt->diffopt.file);
 			}
 
 			/*
@@ -774,8 +774,8 @@ int log_tree_diff_flush(struct rev_info *opt)
 			 */
 			if (!opt->shown_dashes &&
 			    (pch & opt->diffopt.output_format) == pch)
-				printf("---");
-			putchar('\n');
+				fprintf(opt->diffopt.file, "---");
+			putc('\n', opt->diffopt.file);
 		}
 	}
 	diff_flush(&opt->diffopt);
@@ -875,7 +875,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 		return line_log_print(opt, commit);
 
 	if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
-		printf("\n%s\n", opt->break_bar);
+		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	shown = log_tree_diff(opt, commit, &log);
 	if (!shown && opt->loginfo && opt->always_show_header) {
 		log.parent = NULL;
@@ -883,8 +883,8 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 		shown = 1;
 	}
 	if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
-		printf("\n%s\n", opt->break_bar);
+		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	opt->loginfo = NULL;
-	maybe_flush_or_die(stdout, "stdout");
+	maybe_flush_or_die(opt->diffopt.file, "stdout");
 	return shown;
 }
-- 
2.9.0.118.g0e1a633

[PATCH v3 4/9] line-log: respect diffopt's configured output file stream

[Johannes Schindelin]

The diff machinery can optionally output to a file stream other than
stdout, by overriding diffopt.file. In such a case, the rest of the
log tree machinery should also write to that stream.

Currently, there is no user of the line level log that wants to
redirect output to a file. Therefore, one might argue that it is
superfluous to support that now. However, it is better to be
consistent now, rather than to face hard-to-debug problems later.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 line-log.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/line-log.c b/line-log.c
index bbe31ed..e62a7f4 100644
--- a/line-log.c
+++ b/line-log.c
@@ -841,7 +841,7 @@ static char *get_nth_line(long line, unsigned long *ends, void *data)
 
 static void print_line(const char *prefix, char first,
 		       long line, unsigned long *ends, void *data,
-		       const char *color, const char *reset)
+		       const char *color, const char *reset, FILE *file)
 {
 	char *begin = get_nth_line(line, ends, data);
 	char *end = get_nth_line(line+1, ends, data);
@@ -852,14 +852,14 @@ static void print_line(const char *prefix, char first,
 		had_nl = 1;
 	}
 
-	fputs(prefix, stdout);
-	fputs(color, stdout);
-	putchar(first);
-	fwrite(begin, 1, end-begin, stdout);
-	fputs(reset, stdout);
-	putchar('\n');
+	fputs(prefix, file);
+	fputs(color, file);
+	putc(first, file);
+	fwrite(begin, 1, end-begin, file);
+	fputs(reset, file);
+	putc('\n', file);
 	if (!had_nl)
-		fputs("\\ No newline at end of file\n", stdout);
+		fputs("\\ No newline at end of file\n", file);
 }
 
 static char *output_prefix(struct diff_options *opt)
@@ -898,12 +898,12 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 		fill_line_ends(pair->one, &p_lines, &p_ends);
 	fill_line_ends(pair->two, &t_lines, &t_ends);
 
-	printf("%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
-	printf("%s%s--- %s%s%s\n", prefix, c_meta,
+	fprintf(opt->file, "%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
+	fprintf(opt->file, "%s%s--- %s%s%s\n", prefix, c_meta,
 	       pair->one->sha1_valid ? "a/" : "",
 	       pair->one->sha1_valid ? pair->one->path : "/dev/null",
 	       c_reset);
-	printf("%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
+	fprintf(opt->file, "%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
 	for (i = 0; i < range->ranges.nr; i++) {
 		long p_start, p_end;
 		long t_start = range->ranges.ranges[i].start;
@@ -945,7 +945,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 		}
 
 		/* Now output a diff hunk for this range */
-		printf("%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
+		fprintf(opt->file, "%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
 		       prefix, c_frag,
 		       p_start+1, p_end-p_start, t_start+1, t_end-t_start,
 		       c_reset);
@@ -953,18 +953,18 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 			int k;
 			for (; t_cur < diff->target.ranges[j].start; t_cur++)
 				print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
-					   c_context, c_reset);
+					   c_context, c_reset, opt->file);
 			for (k = diff->parent.ranges[j].start; k < diff->parent.ranges[j].end; k++)
 				print_line(prefix, '-', k, p_ends, pair->one->data,
-					   c_old, c_reset);
+					   c_old, c_reset, opt->file);
 			for (; t_cur < diff->target.ranges[j].end && t_cur < t_end; t_cur++)
 				print_line(prefix, '+', t_cur, t_ends, pair->two->data,
-					   c_new, c_reset);
+					   c_new, c_reset, opt->file);
 			j++;
 		}
 		for (; t_cur < t_end; t_cur++)
 			print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
-				   c_context, c_reset);
+				   c_context, c_reset, opt->file);
 	}
 
 	free(p_ends);
@@ -977,7 +977,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
  */
 static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)
 {
-	puts(output_prefix(&rev->diffopt));
+	fprintf(rev->diffopt.file, "%s\n", output_prefix(&rev->diffopt));
 	while (range) {
 		dump_diff_hacky_one(rev, range);
 		range = range->next;
-- 
2.9.0.118.g0e1a633

[PATCH v3 5/9] graph: respect the diffopt.file setting

[Johannes Schindelin]

When the caller overrides diffopt.file (which defaults to stdout),
the diff machinery already redirects its output, and the graph display
should also write to that file.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 graph.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/graph.c b/graph.c
index 1350bdd..8ad8ba3 100644
--- a/graph.c
+++ b/graph.c
@@ -17,8 +17,8 @@
 static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
 
 /*
- * Print a strbuf to stdout.  If the graph is non-NULL, all lines but the
- * first will be prefixed with the graph output.
+ * Print a strbuf.  If the graph is non-NULL, all lines but the first will be
+ * prefixed with the graph output.
  *
  * If the strbuf ends with a newline, the output will end after this
  * newline.  A new graph line will not be printed after the final newline.
@@ -1193,9 +1193,10 @@ void graph_show_commit(struct git_graph *graph)
 
 	while (!shown_commit_line && !graph_is_commit_finished(graph)) {
 		shown_commit_line = graph_next_line(graph, &msgbuf);
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+			graph->revs->diffopt.file);
 		if (!shown_commit_line)
-			putchar('\n');
+			putc('\n', graph->revs->diffopt.file);
 		strbuf_setlen(&msgbuf, 0);
 	}
 
@@ -1210,7 +1211,7 @@ void graph_show_oneline(struct git_graph *graph)
 		return;
 
 	graph_next_line(graph, &msgbuf);
-	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
 	strbuf_release(&msgbuf);
 }
 
@@ -1222,7 +1223,7 @@ void graph_show_padding(struct git_graph *graph)
 		return;
 
 	graph_padding_line(graph, &msgbuf);
-	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
 	strbuf_release(&msgbuf);
 }
 
@@ -1239,12 +1240,13 @@ int graph_show_remainder(struct git_graph *graph)
 
 	for (;;) {
 		graph_next_line(graph, &msgbuf);
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+			graph->revs->diffopt.file);
 		strbuf_setlen(&msgbuf, 0);
 		shown = 1;
 
 		if (!graph_is_commit_finished(graph))
-			putchar('\n');
+			putc('\n', graph->revs->diffopt.file);
 		else
 			break;
 	}
@@ -1259,7 +1261,8 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 	char *p;
 
 	if (!graph) {
-		fwrite(sb->buf, sizeof(char), sb->len, stdout);
+		fwrite(sb->buf, sizeof(char), sb->len,
+			graph->revs->diffopt.file);
 		return;
 	}
 
@@ -1277,7 +1280,7 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 		} else {
 			len = (sb->buf + sb->len) - p;
 		}
-		fwrite(p, sizeof(char), len, stdout);
+		fwrite(p, sizeof(char), len, graph->revs->diffopt.file);
 		if (next_p && *next_p != '\0')
 			graph_show_oneline(graph);
 		p = next_p;
@@ -1297,7 +1300,8 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * CMIT_FMT_USERFORMAT are already missing a terminating
 		 * newline.  All of the other formats should have it.
 		 */
-		fwrite(sb->buf, sizeof(char), sb->len, stdout);
+		fwrite(sb->buf, sizeof(char), sb->len,
+			graph->revs->diffopt.file);
 		return;
 	}
 
@@ -1318,7 +1322,7 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * new line.
 		 */
 		if (!newline_terminated)
-			putchar('\n');
+			putc('\n', graph->revs->diffopt.file);
 
 		graph_show_remainder(graph);
 
@@ -1326,6 +1330,6 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * If sb ends with a newline, our output should too.
 		 */
 		if (newline_terminated)
-			putchar('\n');
+			putc('\n', graph->revs->diffopt.file);
 	}
 }
-- 
2.9.0.118.g0e1a633

[PATCH v3 6/9] shortlog: support outputting to streams other than stdout

[Johannes Schindelin]

This will be needed to avoid freopen() in `git format-patch`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/shortlog.c | 13 ++++++++-----
 shortlog.h         |  1 +
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index bfc082e..39d74fe 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -229,6 +229,7 @@ void shortlog_init(struct shortlog *log)
 	log->wrap = DEFAULT_WRAPLEN;
 	log->in1 = DEFAULT_INDENT1;
 	log->in2 = DEFAULT_INDENT2;
+	log->file = stdout;
 }
 
 int cmd_shortlog(int argc, const char **argv, const char *prefix)
@@ -310,22 +311,24 @@ void shortlog_output(struct shortlog *log)
 	for (i = 0; i < log->list.nr; i++) {
 		const struct string_list_item *item = &log->list.items[i];
 		if (log->summary) {
-			printf("%6d\t%s\n", (int)UTIL_TO_INT(item), item->string);
+			fprintf(log->file, "%6d\t%s\n",
+				(int)UTIL_TO_INT(item), item->string);
 		} else {
 			struct string_list *onelines = item->util;
-			printf("%s (%d):\n", item->string, onelines->nr);
+			fprintf(log->file, "%s (%d):\n",
+				item->string, onelines->nr);
 			for (j = onelines->nr - 1; j >= 0; j--) {
 				const char *msg = onelines->items[j].string;
 
 				if (log->wrap_lines) {
 					strbuf_reset(&sb);
 					add_wrapped_shortlog_msg(&sb, msg, log);
-					fwrite(sb.buf, sb.len, 1, stdout);
+					fwrite(sb.buf, sb.len, 1, log->file);
 				}
 				else
-					printf("      %s\n", msg);
+					fprintf(log->file, "      %s\n", msg);
 			}
-			putchar('\n');
+			putc('\n', log->file);
 			onelines->strdup_strings = 1;
 			string_list_clear(onelines, 0);
 			free(onelines);
diff --git a/shortlog.h b/shortlog.h
index de4f86f..5a326c6 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -17,6 +17,7 @@ struct shortlog {
 	char *common_repo_prefix;
 	int email;
 	struct string_list mailmap;
+	FILE *file;
 };
 
 void shortlog_init(struct shortlog *log);
-- 
2.9.0.118.g0e1a633

[PATCH v3 7/9] format-patch: explicitly switch off color when writing to files

[Johannes Schindelin]

We rely on the auto-detection ("is stdout a terminal?") to determine
whether to use color in the output of format-patch or not. That happens
to work because we freopen() stdout when redirecting the output to files.

However, we are about to fix that work-around, in which case the
auto-detection has no chance to guess whether to use color or not.

But then, we do not need to guess to begin with. We know that we do not
want to use ANSI color sequences in the output files. So let's just be
explicit about our wishes.

It might be argued that we should only do this when the variable
git_use_color_default still has its default value, GIT_COLOR_AUTO.
As of 7787570c (format-patch: ignore ui.color, 2011-09-13) we
do not allow the ui.color setting to affect format-patch's output,
therefore this will always be the case.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/log.c b/builtin/log.c
index 099f4f7..abd889b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1569,6 +1569,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		setup_pager();
 
 	if (output_directory) {
+		rev.diffopt.use_color = 0;
 		if (use_stdout)
 			die(_("standard output, or directory, which one?"));
 		if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
-- 
2.9.0.118.g0e1a633

[PATCH v3 8/9] format-patch: avoid freopen()

[Johannes Schindelin]

We just taught the relevant functions to respect the diffopt.file field,
to allow writing somewhere else than stdout. Let's make use of it.

Technically, we do not need to avoid that call in a builtin: we assume
that builtins (as opposed to library functions) are stand-alone programs
that may do with their (global) state. Yet, we want to be able to reuse
that code in properly lib-ified code, e.g. when converting scripts into
builtins.

Further, while we did not *have* to touch the cmd_show() and cmd_cherry()
code paths (because they do not want to write anywhere but stdout as of
yet), it just makes sense to be consistent, making it easier and safer to
move the code later.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 64 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index abd889b..8dcf205 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -236,7 +236,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)
 		if (rev->commit_format != CMIT_FMT_ONELINE)
 			putchar(rev->diffopt.line_termination);
 	}
-	printf(_("Final output: %d %s\n"), nr, stage);
+	fprintf(rev->diffopt.file, _("Final output: %d %s\n"), nr, stage);
 }
 
 static struct itimerval early_output_timer;
@@ -445,7 +445,7 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
 	pp.fmt = rev->commit_format;
 	pp.date_mode = rev->date_mode;
 	pp_user_info(&pp, "Tagger", &out, buf, get_log_output_encoding());
-	printf("%s", out.buf);
+	fprintf(rev->diffopt.file, "%s", out.buf);
 	strbuf_release(&out);
 }
 
@@ -456,7 +456,7 @@ static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, con
 	char *buf;
 	unsigned long size;
 
-	fflush(stdout);
+	fflush(rev->diffopt.file);
 	if (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||
 	    !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
 		return stream_blob_to_fd(1, sha1, NULL, 0);
@@ -496,7 +496,7 @@ static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)
 	}
 
 	if (offset < size)
-		fwrite(buf + offset, size - offset, 1, stdout);
+		fwrite(buf + offset, size - offset, 1, rev->diffopt.file);
 	free(buf);
 	return 0;
 }
@@ -505,7 +505,8 @@ static int show_tree_object(const unsigned char *sha1,
 		struct strbuf *base,
 		const char *pathname, unsigned mode, int stage, void *context)
 {
-	printf("%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
+	FILE *file = context;
+	fprintf(file, "%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
 	return 0;
 }
 
@@ -565,7 +566,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 
 			if (rev.shown_one)
 				putchar('\n');
-			printf("%stag %s%s\n",
+			fprintf(rev.diffopt.file, "%stag %s%s\n",
 					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					t->tag,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
@@ -584,12 +585,12 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 		case OBJ_TREE:
 			if (rev.shown_one)
 				putchar('\n');
-			printf("%stree %s%s\n\n",
+			fprintf(rev.diffopt.file, "%stree %s%s\n\n",
 					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					name,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
 			read_tree_recursive((struct tree *)o, "", 0, 0, &match_all,
-					show_tree_object, NULL);
+					show_tree_object, rev.diffopt.file);
 			rev.shown_one = 1;
 			break;
 		case OBJ_COMMIT:
@@ -799,7 +800,7 @@ static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
-static int reopen_stdout(struct commit *commit, const char *subject,
+static int open_next_file(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
@@ -823,7 +824,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 	if (!quiet)
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
 
-	if (freopen(filename.buf, "w", stdout) == NULL)
+	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
 		return error(_("Cannot open patch file %s"), filename.buf);
 
 	strbuf_release(&filename);
@@ -882,15 +883,15 @@ static void gen_message_id(struct rev_info *info, char *base)
 	info->message_id = strbuf_detach(&buf, NULL);
 }
 
-static void print_signature(void)
+static void print_signature(FILE *file)
 {
 	if (!signature || !*signature)
 		return;
 
-	printf("-- \n%s", signature);
+	fprintf(file, "-- \n%s", signature);
 	if (signature[strlen(signature)-1] != '\n')
-		putchar('\n');
-	putchar('\n');
+		putc('\n', file);
+	putc('\n', file);
 }
 
 static void add_branch_description(struct strbuf *buf, const char *branch_name)
@@ -959,7 +960,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	committer = git_committer_info(0);
 
 	if (!use_stdout &&
-	    reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
+	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
 		return;
 
 	log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
@@ -982,7 +983,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
 	pp_remainder(&pp, &msg, &sb, 0);
 	add_branch_description(&sb, branch_name);
-	printf("%s\n", sb.buf);
+	fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
 	strbuf_release(&sb);
 
@@ -991,6 +992,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	log.wrap = 72;
 	log.in1 = 2;
 	log.in2 = 4;
+	log.file = rev->diffopt.file;
 	for (i = 0; i < nr; i++)
 		shortlog_add_commit(&log, list[i]);
 
@@ -1013,8 +1015,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	diffcore_std(&opts);
 	diff_flush(&opts);
 
-	printf("\n");
-	print_signature();
+	fprintf(rev->diffopt.file, "\n");
+	print_signature(rev->diffopt.file);
 }
 
 static const char *clean_message_id(const char *msg_id)
@@ -1324,7 +1326,7 @@ static void prepare_bases(struct base_tree_info *bases,
 	}
 }
 
-static void print_bases(struct base_tree_info *bases)
+static void print_bases(struct base_tree_info *bases, FILE *file)
 {
 	int i;
 
@@ -1333,11 +1335,11 @@ static void print_bases(struct base_tree_info *bases)
 		return;
 
 	/* Show the base commit */
-	printf("base-commit: %s\n", oid_to_hex(&bases->base_commit));
+	fprintf(file, "base-commit: %s\n", oid_to_hex(&bases->base_commit));
 
 	/* Show the prerequisite patches */
 	for (i = bases->nr_patch_id - 1; i >= 0; i--)
-		printf("prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
+		fprintf(file, "prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
 
 	free(bases->patch_id);
 	bases->nr_patch_id = 0;
@@ -1694,7 +1696,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout,
 				  origin, nr, list, branch_name, quiet);
-		print_bases(&bases);
+		print_bases(&bases, rev.diffopt.file);
 		total++;
 		start_number--;
 	}
@@ -1740,7 +1742,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!use_stdout &&
-		    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
+		    open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
 			die(_("Failed to create output files"));
 		shown = log_tree_commit(&rev, commit);
 		free_commit_buffer(commit);
@@ -1755,15 +1757,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			rev.shown_one = 0;
 		if (shown) {
 			if (rev.mime_boundary)
-				printf("\n--%s%s--\n\n\n",
+				fprintf(rev.diffopt.file, "\n--%s%s--\n\n\n",
 				       mime_boundary_leader,
 				       rev.mime_boundary);
 			else
-				print_signature();
-			print_bases(&bases);
+				print_signature(rev.diffopt.file);
+			print_bases(&bases, rev.diffopt.file);
 		}
 		if (!use_stdout)
-			fclose(stdout);
+			fclose(rev.diffopt.file);
 	}
 	free(list);
 	free(branch_name);
@@ -1795,15 +1797,15 @@ static const char * const cherry_usage[] = {
 };
 
 static void print_commit(char sign, struct commit *commit, int verbose,
-			 int abbrev)
+			 int abbrev, FILE *file)
 {
 	if (!verbose) {
-		printf("%c %s\n", sign,
+		fprintf(file, "%c %s\n", sign,
 		       find_unique_abbrev(commit->object.oid.hash, abbrev));
 	} else {
 		struct strbuf buf = STRBUF_INIT;
 		pp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);
-		printf("%c %s %s\n", sign,
+		fprintf(file, "%c %s %s\n", sign,
 		       find_unique_abbrev(commit->object.oid.hash, abbrev),
 		       buf.buf);
 		strbuf_release(&buf);
@@ -1884,7 +1886,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 		commit = list->item;
 		if (has_commit_patch_id(commit, &ids))
 			sign = '-';
-		print_commit(sign, commit, verbose, abbrev);
+		print_commit(sign, commit, verbose, abbrev, revs.diffopt.file);
 		list = list->next;
 	}
 
-- 
2.9.0.118.g0e1a633

[PATCH v3 9/9] format-patch: use stdout directly

[Johannes Schindelin]

Earlier, we freopen()ed stdout in order to write patches to files.
That forced us to duplicate stdout (naming it "realstdout") because we
*still* wanted to be able to report the file names.

As we do not abuse stdout that way anymore, we no longer need to
duplicate stdout, either.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 8dcf205..2bfcc43 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -796,7 +796,6 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	return git_log_config(var, value, cb);
 }
 
-static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
@@ -822,7 +821,7 @@ static int open_next_file(struct commit *commit, const char *subject,
 		fmt_output_subject(&filename, subject, rev);
 
 	if (!quiet)
-		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
+		printf("%s\n", filename.buf + outdir_offset);
 
 	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
 		return error(_("Cannot open patch file %s"), filename.buf);
@@ -1629,9 +1628,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		get_patch_ids(&rev, &ids);
 	}
 
-	if (!use_stdout)
-		realstdout = xfdopen(xdup(1), "w");
-
 	if (prepare_revision_walk(&rev))
 		die(_("revision walk setup failed"));
 	rev.boundary = 1;
-- 
2.9.0.118.g0e1a633

Re: [PATCH v2 1/7] log-tree: respect diffopt's configured output file stream

[Johannes Schindelin]

Hi,

On Tue, 21 Jun 2016, Johannes Schindelin wrote:

> On Tue, 21 Jun 2016, Johannes Schindelin wrote:
> 
> > Expect v3 in a moment.
> 
> I am sorry. The regression test suite just sounded red alert. So: do not
> expect v3 in a moment, but later today.

So making maybe_flush_or_die() uncovered a problem with the builtin am: it
called log_tree_commit() after setting diffopt.file *and*
diffopt.close_file. The latter forced diff_flush() to close the file, and
log_tree_commit() happily tried to flush() the same file stream. In my
setup, this triggered a segmentation fault which was really hard to debug
because it happened in a Git subprocess that expected input from stdin
(and therefore my go-to debugging method to fire up gdb was a bit
useless).

In the end, it not only added two new patches to the patch series, but
also opened a big rabbit hole: builtin/am.c could use quite some error
checking, I guess... Now I wish I hadn't seen it...

Well, I just sent v3 of the series.

Ciao,
Dscho

Re: [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field

[Paul Tan]

Hi Johannes,

On Tue, Jun 21, 2016 at 6:34 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> - this uncovered a problem with builtin am, where it asked the diff
>   machinery to close the file stream, but actually called the log_tree
>   machinery (which might mean that this patch series inadvertently fixes
>   a bug where `git am --rebasing` would write the commit message to
>   stdout instead of the `patch` file when erroring out)

Please correct me if I'm wrong: looking at log-tree.c, the commit
message will not be printed when no_commit_id = 1, isn't it? This is
because we do not hit the code paths that write to stdout since
show_log() is not called.

Also, the return value of log_tree_commit() is actually a boolean
value, not an error status value, isn't it?

> This last point is a bigger issue, actually. There seem to be quite a
> few function calls in builtin/am.c whose return values that might
> indicate errors are flatly ignored. I see two calls to run_diff_index()
> whose return value then goes poof unchecked,

Thanks, future-proofing the builtin/am.c code is good, in case
run_diff_index() is updated to not call exit(128) on error in the
future.

> and several calls to
> write_state_text() and write_state_bool() with the same issue.

These functions will die() on error, so checking their error code is
not really useful. I'm not opposed to changing them to use the
write_file_gently() version though, although I don't see a need to
unless builtin/am.c is being libified.

> And I did
> not even try to review the code to that end, all I wanted was to verify
> that builtin am only has the close_file issue once (it does use it a
> second time, but that one is okay because it then calls
> run_diff_index(), i.e. the diff machinery).
>
> I am embarrassed to admit that these builtin am problems demonstrate
> that I, as a mentor of the builtin am project, failed to help make the
> patches as good as I expected myself to do.

Sorry to disappoint you :-(

Regards,
Paul

Re: [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field

[Johannes Schindelin]

Hi Paul,

On Tue, 21 Jun 2016, Paul Tan wrote:

> On Tue, Jun 21, 2016 at 6:34 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > - this uncovered a problem with builtin am, where it asked the diff
> >   machinery to close the file stream, but actually called the log_tree
> >   machinery (which might mean that this patch series inadvertently fixes
> >   a bug where `git am --rebasing` would write the commit message to
> >   stdout instead of the `patch` file when erroring out)
> 
> Please correct me if I'm wrong: looking at log-tree.c, the commit
> message will not be printed when no_commit_id = 1, isn't it?
> This is because we do not hit the code paths that write to stdout since
> show_log() is not called.

Why does builtin/am.c use log_tree_commit(), then? Why not simply run
things through the diff machinery?

> Also, the return value of log_tree_commit() is actually a boolean
> value, not an error status value, isn't it?

It is not really a boolean, no. Sure, at the moment, it happens to return
either 0 or 1. You can figure that out by following the call paths all the
way to do_diff_combined() or line_log_print().

The key words are: at the moment.

We do find more and more places where library functions call die() in case
of errors, and it hurts us. Badly. That is why I, among others, try to
remedy the situation by converting these calls to "return error()"
statements.

The log_tree functions are prepared for that: they return non-negative
values in case of success.

The callers are not really prepared for that, hence my complaints.

> > This last point is a bigger issue, actually. There seem to be quite a
> > few function calls in builtin/am.c whose return values that might
> > indicate errors are flatly ignored. I see two calls to run_diff_index()
> > whose return value then goes poof unchecked,
> 
> Thanks, future-proofing the builtin/am.c code is good, in case
> run_diff_index() is updated to not call exit(128) on error in the
> future.

And run_diff_cache(). And read_ref_at(). And rerere(). And
setup_revisions(). And get_sha1().

> > and several calls to write_state_text() and write_state_bool() with
> > the same issue.
> 
> These functions will die() on error

Indeed. And I do not think that is a good practice.

> > And I did not even try to review the code to that end, all I wanted
> > was to verify that builtin am only has the close_file issue once (it
> > does use it a second time, but that one is okay because it then calls
> > run_diff_index(), i.e. the diff machinery).
> >
> > I am embarrassed to admit that these builtin am problems demonstrate
> > that I, as a mentor of the builtin am project, failed to help make the
> > patches as good as I expected myself to do.
> 
> Sorry to disappoint you :-(

You misunderstood. I am not disappointed in you. *I* did a lousy job. Not
only mentoring, but I also obviously failed to make things fun enough for
you.

My apologies,
Dscho

Re: [PATCH 5/5] format-patch: avoid freopen()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> That is a very convincing argument. So convincing that I wanted to change
> the patch to guard behind `diff_use_color_default == GIT_COLOR_AUTO`.

I actually was expecting, instead of your:

 	if (output_directory) {
+		rev.diffopt.use_color = 0;
 		if (use_stdout)
 			die(_("standard output, or directory, which one?"));

an update would say

 	if (output_directory) {
		if (rev.diffopt.use_color == GIT_COLOR_AUTO)
                	rev.diffopt.use_color = 0;
 		if (use_stdout)
 			die(_("standard output, or directory, which one?"));

I didn't expect you to check diff_use_color_default exactly for the
reason why you say "But that is the wrong variable".

Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> We are about to teach the log_tree machinery to reuse the diffopt.file
> setting to output to a file stream different from stdout.
>
> This means that builtin am can no longer ask the diff machinery to
> close the file when actually calling the log_tree machinery (which
> wants to flush the very same file stream that would then already be
> closed).

Sorry for being slow, but I am not sure why the first paragraph has
to mean the second paragraph.  This existing caller opens a new
stream, sets .fp to it, and expects that the log_tree_commit() to
close it if told by setting .close_file to true, all of which sounds
sensible.

If a codepath wants to use the same stream for two or more calls to
log_tree by pointing the stream with .fp, it would be of course a
problem for the caller to set .close_file to true in its first call,
as .fp will be closed and no longer usable for second and subsequent
call, and that would be a bug, but for a single-shot call it feels
entirely a sensible request to make, no?

Obviously you have looked at the codepaths involved a lot longer
than I did, and I do not doubt your conclusion, but I cannot quite
convince myself with the above explanation.

The option parser of "git diff" family sets ->close_file to true
when the --output option is given.

Wouldn't this patch break "git log --output=foo -3"?

> To stave off similar problems in the future, report it as a bug if
> log_tree_commit() is called with a non-zero diffopt.close_file.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/am.c | 6 ++++--
>  log-tree.c   | 3 +++
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 0e28a62..47d78aa 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1437,6 +1437,7 @@ static int write_commit_patch(const struct am_state *state, struct commit *commi
>  {
>  	struct rev_info rev_info;
>  	FILE *fp;
> +	int res;
>  
>  	fp = fopen(am_path(state, "patch"), "w");
>  	if (!fp)
> @@ -1453,10 +1454,11 @@ static int write_commit_patch(const struct am_state *state, struct commit *commi
>  	DIFF_OPT_SET(&rev_info.diffopt, FULL_INDEX);
>  	rev_info.diffopt.use_color = 0;
>  	rev_info.diffopt.file = fp;
> -	rev_info.diffopt.close_file = 1;
>  	add_pending_object(&rev_info, &commit->object, "");
>  	diff_setup_done(&rev_info.diffopt);
> -	return log_tree_commit(&rev_info, commit);
> +	res = log_tree_commit(&rev_info, commit);
> +	fclose(fp);
> +	return res;
>  }
>  
>  /**
> diff --git a/log-tree.c b/log-tree.c
> index 78a5381..dc0180d 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -864,6 +864,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
>  	struct log_info log;
>  	int shown;
>  
> +	if (opt->diffopt.close_file)
> +		die("BUG: close_file is incompatible with log_tree_commit()");
> +
>  	log.commit = commit;
>  	log.parent = NULL;
>  	opt->loginfo = &log;

Re: [PATCH v3 1/9] am: stop ignoring errors reported by log_tree_diff()

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Note: there are more places in the builtin am code that ignore
> errors returned from library functions. Fixing those is outside the
> purview of the current patch series, though.

The caller of parse_mail() and parse_mail_rebase() is not prepared
to see an error code in the returned value from these function,
which are to return a boolean telling the caller to skip or use the
patch file.  At least the caller needs to notice negative return and
made to die/exit(128), instead of silently skipping a corrupt or
unopenable patch, no?  Otherwise this will change the behaviour.

> Cc: Paul Tan <pyokagan@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/am.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 3dfe70b..0e28a62 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1433,12 +1433,15 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
>  /**
>   * Writes `commit` as a patch to the state directory's "patch" file.
>   */
> -static void write_commit_patch(const struct am_state *state, struct commit *commit)
> +static int write_commit_patch(const struct am_state *state, struct commit *commit)
>  {
>  	struct rev_info rev_info;
>  	FILE *fp;
>  
> -	fp = xfopen(am_path(state, "patch"), "w");
> +	fp = fopen(am_path(state, "patch"), "w");
> +	if (!fp)
> +		return error(_("Could not open %s for writing"),
> +			am_path(state, "patch"));
>  	init_revisions(&rev_info, NULL);
>  	rev_info.diff = 1;
>  	rev_info.abbrev = 0;
> @@ -1453,7 +1456,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
>  	rev_info.diffopt.close_file = 1;
>  	add_pending_object(&rev_info, &commit->object, "");
>  	diff_setup_done(&rev_info.diffopt);
> -	log_tree_commit(&rev_info, commit);
> +	return log_tree_commit(&rev_info, commit);
>  }
>  
>  /**
> @@ -1501,13 +1504,14 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
>  	unsigned char commit_sha1[GIT_SHA1_RAWSZ];
>  
>  	if (get_mail_commit_sha1(commit_sha1, mail) < 0)
> -		die(_("could not parse %s"), mail);
> +		return error(_("could not parse %s"), mail);
>  
>  	commit = lookup_commit_or_die(commit_sha1, mail);
>  
>  	get_commit_info(state, commit);
>  
> -	write_commit_patch(state, commit);
> +	if (write_commit_patch(state, commit) < 0)
> +		return -1;
>  
>  	hashcpy(state->orig_commit, commit_sha1);
>  	write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));

Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> We are about to teach the log_tree machinery to reuse the diffopt.file
>> setting to output to a file stream different from stdout.
>>
>> This means that builtin am can no longer ask the diff machinery to
>> close the file when actually calling the log_tree machinery (which
>> wants to flush the very same file stream that would then already be
>> closed).
>
> Sorry for being slow, but I am not sure why the first paragraph has
> to mean the second paragraph.  This existing caller opens a new
> stream, sets .fp to it, and expects that the log_tree_commit() to
> close it if told by setting .close_file to true, all of which sounds
> sensible.
>
> If a codepath wants to use the same stream for two or more calls to
> log_tree by pointing the stream with .fp, it would be of course a
> problem for the caller to set .close_file to true in its first call,
> as .fp will be closed and no longer usable for second and subsequent
> call, and that would be a bug, but for a single-shot call it feels
> entirely a sensible request to make, no?
>
> Obviously you have looked at the codepaths involved a lot longer
> than I did, and I do not doubt your conclusion, but I cannot quite
> convince myself with the above explanation.
>
> The option parser of "git diff" family sets ->close_file to true
> when the --output option is given.
>
> Wouldn't this patch break "git log --output=foo -3"?

I wonder if the right approach is to stop using .close_file
everywhere.

With this "do not set .close_file if you use log_tree_commit()",
"git log --output=/dev/stdout -3" gets broken, but removing that
check is not sufficient to correct the same command with "-p", as
letting .close_file to close the output file after finishing a
single diff would mean that subsequent write to the same file
descriptor will trigger a failure.

Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>> We are about to teach the log_tree machinery to reuse the diffopt.file
>>> setting to output to a file stream different from stdout.
>>>
>>> This means that builtin am can no longer ask the diff machinery to
>>> close the file when actually calling the log_tree machinery (which
>>> wants to flush the very same file stream that would then already be
>>> closed).
>>
>> Sorry for being slow, but I am not sure why the first paragraph has
>> to mean the second paragraph.  This existing caller opens a new
>> stream, sets .fp to it, and expects that the log_tree_commit() to
>> close it if told by setting .close_file to true, all of which sounds
>> sensible.
>>
>> If a codepath wants to use the same stream for two or more calls to
>> log_tree by pointing the stream with .fp, it would be of course a
>> problem for the caller to set .close_file to true in its first call,
>> as .fp will be closed and no longer usable for second and subsequent
>> call, and that would be a bug, but for a single-shot call it feels
>> entirely a sensible request to make, no?
>>
>> Obviously you have looked at the codepaths involved a lot longer
>> than I did, and I do not doubt your conclusion, but I cannot quite
>> convince myself with the above explanation.
>>
>> The option parser of "git diff" family sets ->close_file to true
>> when the --output option is given.
>>
>> Wouldn't this patch break "git log --output=foo -3"?
>
> I wonder if the right approach is to stop using .close_file
> everywhere.
>
> With this "do not set .close_file if you use log_tree_commit()",
> "git log --output=/dev/stdout -3" gets broken, but removing that
> check is not sufficient to correct the same command with "-p", as
> letting .close_file to close the output file after finishing a
> single diff would mean that subsequent write to the same file
> descriptor will trigger a failure.

We could say "git log --output=foo -3 [-p]" without any of your
patches is already broken, and it is a valid excuse to take this
change that we are not making things worse with it.

It is just 3/9 is a logical first step to correct that exact
problem, i.e. some codepaths, even though there is a place that
holds the output stream and command line parser does prepare one for
"foo" when --output=foo is given, ignore it and send thigns to the
standard output stream.  You might not have written 3/9 in order to
fix that "git log --output=foo" problem, but a fix for it should
look exactly like your 3/9, I would think.

And it is sad that this step makes that fix impossible.

Re: [PATCH 5/5] format-patch: avoid freopen()

[Johannes Schindelin]

Hi Junio,

On Tue, 21 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > That is a very convincing argument. So convincing that I wanted to change
> > the patch to guard behind `diff_use_color_default == GIT_COLOR_AUTO`.
> 
> I actually was expecting, instead of your:
> 
>  	if (output_directory) {
> +		rev.diffopt.use_color = 0;
>  		if (use_stdout)
>  			die(_("standard output, or directory, which one?"));
> 
> an update would say
> 
>  	if (output_directory) {
> 		if (rev.diffopt.use_color == GIT_COLOR_AUTO)
>                 	rev.diffopt.use_color = 0;
>  		if (use_stdout)
>  			die(_("standard output, or directory, which one?"));
> 
> I didn't expect you to check diff_use_color_default exactly for the
> reason why you say "But that is the wrong variable".

In diff_setup() (which is called by init_revisions() which in turn is
called by format-patch's cmd_format_patch()), the use_color field is
initialized with the value of diff_use_color_default, though:

	https://github.com/git/git/blob/v2.9.0/diff.c#L3283

Please note that diff_use_color_default is initialized to -1:

	https://github.com/git/git/blob/v2.9.0/diff.c#L32

and set to a different value here:

	https://github.com/git/git/blob/v2.9.0/diff.c#L180

when parsing the diff.color or color.diff config settings, which
however are *not* parsed in format-patch, thanks to:

	https://github.com/git/git/blob/v2.9.0/builtin/log.c#L740-L743

Therefore, use_color is initialized to -1, and in format-patch's case it
remains like this. I was a bit surprised to see that GIT_COLOR_AUTO's
numerical value is *not* -1 (which I would have chosen for the "undecided"
case, but I guess that -1 was assumed to mean the "unspecified" case), but
the numerical value of 2 instead:

	https://github.com/git/git/blob/v2.9.0/color.h#L59

Hence " rev.diffopt.use_color == GIT_COLOR_AUTO" would
evaluate to "-1 == 2" in this context.

Further, I think that the commit message of 7787570c (format-patch: ignore
ui.color, 2011-09-13) makes a pretty eloquent case that we *want* to
switch off color when letting format-patch write to files.

But there's a rub... If you specify --color *explicitly*, use_color is set
to GIT_COLOR_ALWAYS and the file indeed contains ANSI sequences (i.e. my
analysis above left out the command-line part).

In short, I think you're right, I have to guard the assignment, with the
minor adjustment to test use_color != GIT_COLOR_ALWAYS.

Will reroll.

Ciao,
Dscho

Re: [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field

[Paul Tan]

Hi Johannes,

On Tue, Jun 21, 2016 at 10:12 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Paul,
>
> On Tue, 21 Jun 2016, Paul Tan wrote:
>
>> On Tue, Jun 21, 2016 at 6:34 PM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>> > - this uncovered a problem with builtin am, where it asked the diff
>> >   machinery to close the file stream, but actually called the log_tree
>> >   machinery (which might mean that this patch series inadvertently fixes
>> >   a bug where `git am --rebasing` would write the commit message to
>> >   stdout instead of the `patch` file when erroring out)
>>
>> Please correct me if I'm wrong: looking at log-tree.c, the commit
>> message will not be printed when no_commit_id = 1, isn't it?
>> This is because we do not hit the code paths that write to stdout since
>> show_log() is not called.
>
> Why does builtin/am.c use log_tree_commit(), then? Why not simply run
> things through the diff machinery?

It's because git-am.sh called "git diff-tree", which in turn calls
log_tree_commit(), so to be safe I used the same function to ensure
that there were no unintended behavioral changes. e.g. what happened
with 3ecc704 (am --skip/--abort: merge HEAD/ORIG_HEAD tree into index,
2015-08-19)

Of course, it may be true that the diff machinery should be called
directly, although the code is quite involved so I can't really tell
the impact the change will have.

Thanks,
Paul

Re: [PATCH v3 1/9] am: stop ignoring errors reported by log_tree_diff()

[Johannes Schindelin]

Hi Junio,

On Tue, 21 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Note: there are more places in the builtin am code that ignore
> > errors returned from library functions. Fixing those is outside the
> > purview of the current patch series, though.
> 
> The caller of parse_mail() and parse_mail_rebase() is not prepared
> to see an error code in the returned value from these function,
> which are to return a boolean telling the caller to skip or use the
> patch file.  At least the caller needs to notice negative return and
> made to die/exit(128), instead of silently skipping a corrupt or
> unopenable patch, no?  Otherwise this will change the behaviour.

Yeah, that is another rabbit hole I really do not want to dive in.

Will leave it alone,
Dscho

[PATCH v4 00/10] Let log-tree and friends respect diffopt's `file` field

[Johannes Schindelin]

The idea is to allow callers to redirect log-tree's output to a file
without having to freopen() stdout (which would modify global state,
a big no-no-no for library functions).

I reviewed log-tree.c, graph.c, line-log.c, builtin/shortlog.c and
builtin/log.c line by line to ensure that all calls that assumed stdout
previously now use the `file` field instead, of course. I would
welcome additional eyes to go over the code to confirm that I did not
miss anything.

This patch series ends up removing the freopen() call in the
format-patch command, but that is only a by-product. The ulterior motive
behind this series is to allow the sequencer to write a `patch` file as
part of my endeavor to move large chunks of rebase -i into a builtin.

In contrast to the previous iteration of this patch series,

- the use_color = 0 setting was made contingent on use_color != ALWAYS

- close_file = 1 was made to work in more circumstances, most notably
  when calling log_commit_tree() (and in builtin/log.c, where this
  function is called in a loop)

- the changes to builtin/am.c were backed out completely (this is a can
  of worms I am not prepared to open for now)

- I also taught shortlog to respect the --output=<file> option, because
  it was so easy to do

- I added a test case to ensure that `log --output=<file>` works


Johannes Schindelin (10):
  Prepare log/log-tree to reuse the diffopt.close_file attribute
  log-tree: respect diffopt's configured output file stream
  line-log: respect diffopt's configured output file stream
  graph: respect the diffopt.file setting
  shortlog: support outputting to streams other than stdout
  format-patch: explicitly switch off color when writing to files
  format-patch: avoid freopen()
  format-patch: use stdout directly
  shortlog: respect the --output=<file> setting
  Ensure that log respects --output=<file>

 builtin/log.c       | 87 +++++++++++++++++++++++++++++------------------------
 builtin/shortlog.c  | 15 ++++++---
 graph.c             | 30 ++++++++++--------
 line-log.c          | 34 ++++++++++-----------
 log-tree.c          | 69 ++++++++++++++++++++++--------------------
 shortlog.h          |  1 +
 t/t4201-shortlog.sh |  6 ++++
 t/t4211-line-log.sh |  7 +++++
 8 files changed, 142 insertions(+), 107 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/diffopt.file-v4
Interdiff vs v3:

 diff --git a/builtin/am.c b/builtin/am.c
 index 47d78aa..3dfe70b 100644
 --- a/builtin/am.c
 +++ b/builtin/am.c
 @@ -1433,16 +1433,12 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
  /**
   * Writes `commit` as a patch to the state directory's "patch" file.
   */
 -static int write_commit_patch(const struct am_state *state, struct commit *commit)
 +static void write_commit_patch(const struct am_state *state, struct commit *commit)
  {
  	struct rev_info rev_info;
  	FILE *fp;
 -	int res;
  
 -	fp = fopen(am_path(state, "patch"), "w");
 -	if (!fp)
 -		return error(_("Could not open %s for writing"),
 -			am_path(state, "patch"));
 +	fp = xfopen(am_path(state, "patch"), "w");
  	init_revisions(&rev_info, NULL);
  	rev_info.diff = 1;
  	rev_info.abbrev = 0;
 @@ -1454,11 +1450,10 @@ static int write_commit_patch(const struct am_state *state, struct commit *commi
  	DIFF_OPT_SET(&rev_info.diffopt, FULL_INDEX);
  	rev_info.diffopt.use_color = 0;
  	rev_info.diffopt.file = fp;
 +	rev_info.diffopt.close_file = 1;
  	add_pending_object(&rev_info, &commit->object, "");
  	diff_setup_done(&rev_info.diffopt);
 -	res = log_tree_commit(&rev_info, commit);
 -	fclose(fp);
 -	return res;
 +	log_tree_commit(&rev_info, commit);
  }
  
  /**
 @@ -1506,14 +1501,13 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
  	unsigned char commit_sha1[GIT_SHA1_RAWSZ];
  
  	if (get_mail_commit_sha1(commit_sha1, mail) < 0)
 -		return error(_("could not parse %s"), mail);
 +		die(_("could not parse %s"), mail);
  
  	commit = lookup_commit_or_die(commit_sha1, mail);
  
  	get_commit_info(state, commit);
  
 -	if (write_commit_patch(state, commit) < 0)
 -		return -1;
 +	write_commit_patch(state, commit);
  
  	hashcpy(state->orig_commit, commit_sha1);
  	write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));
 diff --git a/builtin/log.c b/builtin/log.c
 index 2bfcc43..2a42216 100644
 --- a/builtin/log.c
 +++ b/builtin/log.c
 @@ -243,9 +243,10 @@ static struct itimerval early_output_timer;
  
  static void log_show_early(struct rev_info *revs, struct commit_list *list)
  {
 -	int i = revs->early_output;
 +	int i = revs->early_output, close_file = revs->diffopt.close_file;
  	int show_header = 1;
  
 +	revs->diffopt.close_file = 0;
  	sort_in_topological_order(&list, revs->sort_order);
  	while (list && i) {
  		struct commit *commit = list->item;
 @@ -262,14 +263,19 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)
  		case commit_ignore:
  			break;
  		case commit_error:
 +			if (close_file)
 +				fclose(revs->diffopt.file);
  			return;
  		}
  		list = list->next;
  	}
  
  	/* Did we already get enough commits for the early output? */
 -	if (!i)
 +	if (!i) {
 +		if (close_file)
 +			fclose(revs->diffopt.file);
  		return;
 +	}
  
  	/*
  	 * ..if no, then repeat it twice a second until we
 @@ -331,7 +337,7 @@ static int cmd_log_walk(struct rev_info *rev)
  {
  	struct commit *commit;
  	int saved_nrl = 0;
 -	int saved_dcctc = 0;
 +	int saved_dcctc = 0, close_file = rev->diffopt.close_file;
  
  	if (rev->early_output)
  		setup_early_output(rev);
 @@ -347,6 +353,7 @@ static int cmd_log_walk(struct rev_info *rev)
  	 * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to
  	 * retain that state information if replacing rev->diffopt in this loop
  	 */
 +	rev->diffopt.close_file = 0;
  	while ((commit = get_revision(rev)) != NULL) {
  		if (!log_tree_commit(rev, commit) && rev->max_count >= 0)
  			/*
 @@ -367,6 +374,8 @@ static int cmd_log_walk(struct rev_info *rev)
  	}
  	rev->diffopt.degraded_cc_to_c = saved_dcctc;
  	rev->diffopt.needed_rename_limit = saved_nrl;
 +	if (close_file)
 +		fclose(rev->diffopt.file);
  
  	if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
  	    DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {
 @@ -1570,7 +1579,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
  		setup_pager();
  
  	if (output_directory) {
 -		rev.diffopt.use_color = 0;
 +		if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
 +			rev.diffopt.use_color = 0;
  		if (use_stdout)
  			die(_("standard output, or directory, which one?"));
  		if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
 diff --git a/builtin/shortlog.c b/builtin/shortlog.c
 index 39d74fe..be80547 100644
 --- a/builtin/shortlog.c
 +++ b/builtin/shortlog.c
 @@ -229,7 +229,6 @@ void shortlog_init(struct shortlog *log)
  	log->wrap = DEFAULT_WRAPLEN;
  	log->in1 = DEFAULT_INDENT1;
  	log->in2 = DEFAULT_INDENT2;
 -	log->file = stdout;
  }
  
  int cmd_shortlog(int argc, const char **argv, const char *prefix)
 @@ -277,6 +276,7 @@ parse_done:
  
  	log.user_format = rev.commit_format == CMIT_FMT_USERFORMAT;
  	log.abbrev = rev.abbrev;
 +	log.file = rev.diffopt.file;
  
  	/* assume HEAD if from a tty */
  	if (!nongit && !rev.pending.nr && isatty(0))
 @@ -290,6 +290,8 @@ parse_done:
  		get_from_rev(&rev, &log);
  
  	shortlog_output(&log);
 +	if (log.file != stdout)
 +		fclose(log.file);
  	return 0;
  }
  
 diff --git a/log-tree.c b/log-tree.c
 index 530297d..cf24027 100644
 --- a/log-tree.c
 +++ b/log-tree.c
 @@ -862,14 +862,12 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
  int log_tree_commit(struct rev_info *opt, struct commit *commit)
  {
  	struct log_info log;
 -	int shown;
 -
 -	if (opt->diffopt.close_file)
 -		die("BUG: close_file is incompatible with log_tree_commit()");
 +	int shown, close_file = opt->diffopt.close_file;
  
  	log.commit = commit;
  	log.parent = NULL;
  	opt->loginfo = &log;
 +	opt->diffopt.close_file = 0;
  
  	if (opt->line_level_traverse)
  		return line_log_print(opt, commit);
 @@ -886,5 +884,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
  		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
  	opt->loginfo = NULL;
  	maybe_flush_or_die(opt->diffopt.file, "stdout");
 +	if (close_file)
 +		fclose(opt->diffopt.file);
  	return shown;
  }
 diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
 index a977365..bd699e1 100755
 --- a/t/t4201-shortlog.sh
 +++ b/t/t4201-shortlog.sh
 @@ -184,4 +184,10 @@ test_expect_success 'shortlog with revision pseudo options' '
  	git shortlog --exclude=refs/heads/m* --all
  '
  
 +test_expect_success 'shortlog with --output=<file>' '
 +	git shortlog --output=shortlog master >output &&
 +	test ! -s output &&
 +	test_line_count = 7 shortlog
 +'
 +
  test_done
 diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
 index 4451127..9d87777 100755
 --- a/t/t4211-line-log.sh
 +++ b/t/t4211-line-log.sh
 @@ -99,4 +99,11 @@ test_expect_success '-L with --first-parent and a merge' '
  	git log --first-parent -L 1,1:b.c
  '
  
 +test_expect_success '-L with --output' '
 +	git checkout parallel-change &&
 +	git log --output=log -L :main:b.c >output &&
 +	test ! -s output &&
 +	test_line_count = 70 log
 +'
 +
  test_done

-- 
2.9.0.118.g0e1a633

base-commit: ab7797dbe95fff38d9265869ea367020046db118

[PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute

[Johannes Schindelin]

We are about to teach the log-tree machinery to reuse the diffopt.file
field to output to a file stream other than stdout, in line with the
diff machinery already writing to diffopt.file.

However, we might want to write something after the diff in
log_tree_commit() (e.g. with the --show-linear-break option), therefore
we must not let the diff machinery close the file (as per
diffopt.close_file.

This means that log_tree_commit() itself must override the
diffopt.close_file flag and close the file, and if log_tree_commit() is
called in a loop, the caller is responsible to do the same.

Note: format-patch has an `--output-directory` option. Due to the fact
that format-patch's options are parsed first, and that the parse-options
machinery accepts uniquely abbreviated options, the diff options
`--output` (and `-o`) are shadowed. Therefore close_file is not set to 1
so that cmd_format_patch() does *not* need to handle the close_file flag
differently, even if it calls log_tree_commit() in a loop.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 15 ++++++++++++---
 log-tree.c    |  5 ++++-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 099f4f7..27bc88d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -243,9 +243,10 @@ static struct itimerval early_output_timer;
 
 static void log_show_early(struct rev_info *revs, struct commit_list *list)
 {
-	int i = revs->early_output;
+	int i = revs->early_output, close_file = revs->diffopt.close_file;
 	int show_header = 1;
 
+	revs->diffopt.close_file = 0;
 	sort_in_topological_order(&list, revs->sort_order);
 	while (list && i) {
 		struct commit *commit = list->item;
@@ -262,14 +263,19 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)
 		case commit_ignore:
 			break;
 		case commit_error:
+			if (close_file)
+				fclose(revs->diffopt.file);
 			return;
 		}
 		list = list->next;
 	}
 
 	/* Did we already get enough commits for the early output? */
-	if (!i)
+	if (!i) {
+		if (close_file)
+			fclose(revs->diffopt.file);
 		return;
+	}
 
 	/*
 	 * ..if no, then repeat it twice a second until we
@@ -331,7 +337,7 @@ static int cmd_log_walk(struct rev_info *rev)
 {
 	struct commit *commit;
 	int saved_nrl = 0;
-	int saved_dcctc = 0;
+	int saved_dcctc = 0, close_file = rev->diffopt.close_file;
 
 	if (rev->early_output)
 		setup_early_output(rev);
@@ -347,6 +353,7 @@ static int cmd_log_walk(struct rev_info *rev)
 	 * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to
 	 * retain that state information if replacing rev->diffopt in this loop
 	 */
+	rev->diffopt.close_file = 0;
 	while ((commit = get_revision(rev)) != NULL) {
 		if (!log_tree_commit(rev, commit) && rev->max_count >= 0)
 			/*
@@ -367,6 +374,8 @@ static int cmd_log_walk(struct rev_info *rev)
 	}
 	rev->diffopt.degraded_cc_to_c = saved_dcctc;
 	rev->diffopt.needed_rename_limit = saved_nrl;
+	if (close_file)
+		fclose(rev->diffopt.file);
 
 	if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
 	    DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {
diff --git a/log-tree.c b/log-tree.c
index 78a5381..456d7e3 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -862,11 +862,12 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
 int log_tree_commit(struct rev_info *opt, struct commit *commit)
 {
 	struct log_info log;
-	int shown;
+	int shown, close_file = opt->diffopt.close_file;
 
 	log.commit = commit;
 	log.parent = NULL;
 	opt->loginfo = &log;
+	opt->diffopt.close_file = 0;
 
 	if (opt->line_level_traverse)
 		return line_log_print(opt, commit);
@@ -883,5 +884,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 		printf("\n%s\n", opt->break_bar);
 	opt->loginfo = NULL;
 	maybe_flush_or_die(stdout, "stdout");
+	if (close_file)
+		fclose(opt->diffopt.file);
 	return shown;
 }
-- 
2.9.0.118.g0e1a633

[PATCH v4 02/10] log-tree: respect diffopt's configured output file stream

[Johannes Schindelin]

The diff options already know how to print the output anywhere else
than stdout. The same is needed for log output in general, e.g.
when writing patches to files in `git format-patch`. Let's allow
users to use log_tree_commit() *without* changing global state via
freopen().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 log-tree.c | 64 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 456d7e3..cf24027 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -159,12 +159,12 @@ void load_ref_decorations(int flags)
 	}
 }
 
-static void show_parents(struct commit *commit, int abbrev)
+static void show_parents(struct commit *commit, int abbrev, FILE *file)
 {
 	struct commit_list *p;
 	for (p = commit->parents; p ; p = p->next) {
 		struct commit *parent = p->item;
-		printf(" %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
+		fprintf(file, " %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
 	}
 }
 
@@ -172,7 +172,7 @@ static void show_children(struct rev_info *opt, struct commit *commit, int abbre
 {
 	struct commit_list *p = lookup_decoration(&opt->children, &commit->object);
 	for ( ; p; p = p->next) {
-		printf(" %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
+		fprintf(opt->diffopt.file, " %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
 	}
 }
 
@@ -286,11 +286,11 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
 	struct strbuf sb = STRBUF_INIT;
 
 	if (opt->show_source && commit->util)
-		printf("\t%s", (char *) commit->util);
+		fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
 	if (!opt->show_decorations)
 		return;
 	format_decorations(&sb, commit, opt->diffopt.use_color);
-	fputs(sb.buf, stdout);
+	fputs(sb.buf, opt->diffopt.file);
 	strbuf_release(&sb);
 }
 
@@ -364,18 +364,18 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		subject = "Subject: ";
 	}
 
-	printf("From %s Mon Sep 17 00:00:00 2001\n", name);
+	fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);
 	graph_show_oneline(opt->graph);
 	if (opt->message_id) {
-		printf("Message-Id: <%s>\n", opt->message_id);
+		fprintf(opt->diffopt.file, "Message-Id: <%s>\n", opt->message_id);
 		graph_show_oneline(opt->graph);
 	}
 	if (opt->ref_message_ids && opt->ref_message_ids->nr > 0) {
 		int i, n;
 		n = opt->ref_message_ids->nr;
-		printf("In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
+		fprintf(opt->diffopt.file, "In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
 		for (i = 0; i < n; i++)
-			printf("%s<%s>\n", (i > 0 ? "\t" : "References: "),
+			fprintf(opt->diffopt.file, "%s<%s>\n", (i > 0 ? "\t" : "References: "),
 			       opt->ref_message_ids->items[i].string);
 		graph_show_oneline(opt->graph);
 	}
@@ -432,7 +432,7 @@ static void show_sig_lines(struct rev_info *opt, int status, const char *bol)
 	reset = diff_get_color_opt(&opt->diffopt, DIFF_RESET);
 	while (*bol) {
 		eol = strchrnul(bol, '\n');
-		printf("%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
+		fprintf(opt->diffopt.file, "%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
 		       *eol ? "\n" : "");
 		graph_show_oneline(opt->graph);
 		bol = (*eol) ? (eol + 1) : eol;
@@ -553,17 +553,17 @@ void show_log(struct rev_info *opt)
 
 		if (!opt->graph)
 			put_revision_mark(opt, commit);
-		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), stdout);
+		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), opt->diffopt.file);
 		if (opt->print_parents)
-			show_parents(commit, abbrev_commit);
+			show_parents(commit, abbrev_commit, opt->diffopt.file);
 		if (opt->children.name)
 			show_children(opt, commit, abbrev_commit);
 		show_decorations(opt, commit);
 		if (opt->graph && !graph_is_commit_finished(opt->graph)) {
-			putchar('\n');
+			putc('\n', opt->diffopt.file);
 			graph_show_remainder(opt->graph);
 		}
-		putchar(opt->diffopt.line_termination);
+		putc(opt->diffopt.line_termination, opt->diffopt.file);
 		return;
 	}
 
@@ -589,7 +589,7 @@ void show_log(struct rev_info *opt)
 		if (opt->diffopt.line_termination == '\n' &&
 		    !opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar(opt->diffopt.line_termination);
+		putc(opt->diffopt.line_termination, opt->diffopt.file);
 	}
 	opt->shown_one = 1;
 
@@ -607,28 +607,28 @@ void show_log(struct rev_info *opt)
 		log_write_email_headers(opt, commit, &ctx.subject, &extra_headers,
 					&ctx.need_8bit_cte);
 	} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
-		fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), stdout);
+		fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), opt->diffopt.file);
 		if (opt->commit_format != CMIT_FMT_ONELINE)
-			fputs("commit ", stdout);
+			fputs("commit ", opt->diffopt.file);
 
 		if (!opt->graph)
 			put_revision_mark(opt, commit);
 		fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit),
-		      stdout);
+		      opt->diffopt.file);
 		if (opt->print_parents)
-			show_parents(commit, abbrev_commit);
+			show_parents(commit, abbrev_commit, opt->diffopt.file);
 		if (opt->children.name)
 			show_children(opt, commit, abbrev_commit);
 		if (parent)
-			printf(" (from %s)",
+			fprintf(opt->diffopt.file, " (from %s)",
 			       find_unique_abbrev(parent->object.oid.hash,
 						  abbrev_commit));
-		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), stdout);
+		fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
 		show_decorations(opt, commit);
 		if (opt->commit_format == CMIT_FMT_ONELINE) {
-			putchar(' ');
+			putc(' ', opt->diffopt.file);
 		} else {
-			putchar('\n');
+			putc('\n', opt->diffopt.file);
 			graph_show_oneline(opt->graph);
 		}
 		if (opt->reflog_info) {
@@ -702,7 +702,7 @@ void show_log(struct rev_info *opt)
 	}
 
 	if (opt->show_log_size) {
-		printf("log size %i\n", (int)msgbuf.len);
+		fprintf(opt->diffopt.file, "log size %i\n", (int)msgbuf.len);
 		graph_show_oneline(opt->graph);
 	}
 
@@ -718,11 +718,11 @@ void show_log(struct rev_info *opt)
 	if (opt->graph)
 		graph_show_commit_msg(opt->graph, &msgbuf);
 	else
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, opt->diffopt.file);
 	if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar(opt->diffopt.line_termination);
+		putc(opt->diffopt.line_termination, opt->diffopt.file);
 	}
 
 	strbuf_release(&msgbuf);
@@ -759,7 +759,7 @@ int log_tree_diff_flush(struct rev_info *opt)
 				struct strbuf *msg = NULL;
 				msg = opt->diffopt.output_prefix(&opt->diffopt,
 					opt->diffopt.output_prefix_data);
-				fwrite(msg->buf, msg->len, 1, stdout);
+				fwrite(msg->buf, msg->len, 1, opt->diffopt.file);
 			}
 
 			/*
@@ -774,8 +774,8 @@ int log_tree_diff_flush(struct rev_info *opt)
 			 */
 			if (!opt->shown_dashes &&
 			    (pch & opt->diffopt.output_format) == pch)
-				printf("---");
-			putchar('\n');
+				fprintf(opt->diffopt.file, "---");
+			putc('\n', opt->diffopt.file);
 		}
 	}
 	diff_flush(&opt->diffopt);
@@ -873,7 +873,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 		return line_log_print(opt, commit);
 
 	if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
-		printf("\n%s\n", opt->break_bar);
+		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	shown = log_tree_diff(opt, commit, &log);
 	if (!shown && opt->loginfo && opt->always_show_header) {
 		log.parent = NULL;
@@ -881,9 +881,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 		shown = 1;
 	}
 	if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
-		printf("\n%s\n", opt->break_bar);
+		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
 	opt->loginfo = NULL;
-	maybe_flush_or_die(stdout, "stdout");
+	maybe_flush_or_die(opt->diffopt.file, "stdout");
 	if (close_file)
 		fclose(opt->diffopt.file);
 	return shown;
-- 
2.9.0.118.g0e1a633

[PATCH v4 03/10] line-log: respect diffopt's configured output file stream

[Johannes Schindelin]

The diff machinery can optionally output to a file stream other than
stdout, by overriding diffopt.file. In such a case, the rest of the
log tree machinery should also write to that stream.

Currently, there is no user of the line level log that wants to
redirect output to a file. Therefore, one might argue that it is
superfluous to support that now. However, it is better to be
consistent now, rather than to face hard-to-debug problems later.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 line-log.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/line-log.c b/line-log.c
index bbe31ed..e62a7f4 100644
--- a/line-log.c
+++ b/line-log.c
@@ -841,7 +841,7 @@ static char *get_nth_line(long line, unsigned long *ends, void *data)
 
 static void print_line(const char *prefix, char first,
 		       long line, unsigned long *ends, void *data,
-		       const char *color, const char *reset)
+		       const char *color, const char *reset, FILE *file)
 {
 	char *begin = get_nth_line(line, ends, data);
 	char *end = get_nth_line(line+1, ends, data);
@@ -852,14 +852,14 @@ static void print_line(const char *prefix, char first,
 		had_nl = 1;
 	}
 
-	fputs(prefix, stdout);
-	fputs(color, stdout);
-	putchar(first);
-	fwrite(begin, 1, end-begin, stdout);
-	fputs(reset, stdout);
-	putchar('\n');
+	fputs(prefix, file);
+	fputs(color, file);
+	putc(first, file);
+	fwrite(begin, 1, end-begin, file);
+	fputs(reset, file);
+	putc('\n', file);
 	if (!had_nl)
-		fputs("\\ No newline at end of file\n", stdout);
+		fputs("\\ No newline at end of file\n", file);
 }
 
 static char *output_prefix(struct diff_options *opt)
@@ -898,12 +898,12 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 		fill_line_ends(pair->one, &p_lines, &p_ends);
 	fill_line_ends(pair->two, &t_lines, &t_ends);
 
-	printf("%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
-	printf("%s%s--- %s%s%s\n", prefix, c_meta,
+	fprintf(opt->file, "%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
+	fprintf(opt->file, "%s%s--- %s%s%s\n", prefix, c_meta,
 	       pair->one->sha1_valid ? "a/" : "",
 	       pair->one->sha1_valid ? pair->one->path : "/dev/null",
 	       c_reset);
-	printf("%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
+	fprintf(opt->file, "%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
 	for (i = 0; i < range->ranges.nr; i++) {
 		long p_start, p_end;
 		long t_start = range->ranges.ranges[i].start;
@@ -945,7 +945,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 		}
 
 		/* Now output a diff hunk for this range */
-		printf("%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
+		fprintf(opt->file, "%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
 		       prefix, c_frag,
 		       p_start+1, p_end-p_start, t_start+1, t_end-t_start,
 		       c_reset);
@@ -953,18 +953,18 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 			int k;
 			for (; t_cur < diff->target.ranges[j].start; t_cur++)
 				print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
-					   c_context, c_reset);
+					   c_context, c_reset, opt->file);
 			for (k = diff->parent.ranges[j].start; k < diff->parent.ranges[j].end; k++)
 				print_line(prefix, '-', k, p_ends, pair->one->data,
-					   c_old, c_reset);
+					   c_old, c_reset, opt->file);
 			for (; t_cur < diff->target.ranges[j].end && t_cur < t_end; t_cur++)
 				print_line(prefix, '+', t_cur, t_ends, pair->two->data,
-					   c_new, c_reset);
+					   c_new, c_reset, opt->file);
 			j++;
 		}
 		for (; t_cur < t_end; t_cur++)
 			print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
-				   c_context, c_reset);
+				   c_context, c_reset, opt->file);
 	}
 
 	free(p_ends);
@@ -977,7 +977,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
  */
 static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)
 {
-	puts(output_prefix(&rev->diffopt));
+	fprintf(rev->diffopt.file, "%s\n", output_prefix(&rev->diffopt));
 	while (range) {
 		dump_diff_hacky_one(rev, range);
 		range = range->next;
-- 
2.9.0.118.g0e1a633

[PATCH v4 04/10] graph: respect the diffopt.file setting

[Johannes Schindelin]

When the caller overrides diffopt.file (which defaults to stdout),
the diff machinery already redirects its output, and the graph display
should also write to that file.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 graph.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/graph.c b/graph.c
index 1350bdd..8ad8ba3 100644
--- a/graph.c
+++ b/graph.c
@@ -17,8 +17,8 @@
 static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
 
 /*
- * Print a strbuf to stdout.  If the graph is non-NULL, all lines but the
- * first will be prefixed with the graph output.
+ * Print a strbuf.  If the graph is non-NULL, all lines but the first will be
+ * prefixed with the graph output.
  *
  * If the strbuf ends with a newline, the output will end after this
  * newline.  A new graph line will not be printed after the final newline.
@@ -1193,9 +1193,10 @@ void graph_show_commit(struct git_graph *graph)
 
 	while (!shown_commit_line && !graph_is_commit_finished(graph)) {
 		shown_commit_line = graph_next_line(graph, &msgbuf);
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+			graph->revs->diffopt.file);
 		if (!shown_commit_line)
-			putchar('\n');
+			putc('\n', graph->revs->diffopt.file);
 		strbuf_setlen(&msgbuf, 0);
 	}
 
@@ -1210,7 +1211,7 @@ void graph_show_oneline(struct git_graph *graph)
 		return;
 
 	graph_next_line(graph, &msgbuf);
-	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
 	strbuf_release(&msgbuf);
 }
 
@@ -1222,7 +1223,7 @@ void graph_show_padding(struct git_graph *graph)
 		return;
 
 	graph_padding_line(graph, &msgbuf);
-	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+	fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
 	strbuf_release(&msgbuf);
 }
 
@@ -1239,12 +1240,13 @@ int graph_show_remainder(struct git_graph *graph)
 
 	for (;;) {
 		graph_next_line(graph, &msgbuf);
-		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+		fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+			graph->revs->diffopt.file);
 		strbuf_setlen(&msgbuf, 0);
 		shown = 1;
 
 		if (!graph_is_commit_finished(graph))
-			putchar('\n');
+			putc('\n', graph->revs->diffopt.file);
 		else
 			break;
 	}
@@ -1259,7 +1261,8 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 	char *p;
 
 	if (!graph) {
-		fwrite(sb->buf, sizeof(char), sb->len, stdout);
+		fwrite(sb->buf, sizeof(char), sb->len,
+			graph->revs->diffopt.file);
 		return;
 	}
 
@@ -1277,7 +1280,7 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 		} else {
 			len = (sb->buf + sb->len) - p;
 		}
-		fwrite(p, sizeof(char), len, stdout);
+		fwrite(p, sizeof(char), len, graph->revs->diffopt.file);
 		if (next_p && *next_p != '\0')
 			graph_show_oneline(graph);
 		p = next_p;
@@ -1297,7 +1300,8 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * CMIT_FMT_USERFORMAT are already missing a terminating
 		 * newline.  All of the other formats should have it.
 		 */
-		fwrite(sb->buf, sizeof(char), sb->len, stdout);
+		fwrite(sb->buf, sizeof(char), sb->len,
+			graph->revs->diffopt.file);
 		return;
 	}
 
@@ -1318,7 +1322,7 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * new line.
 		 */
 		if (!newline_terminated)
-			putchar('\n');
+			putc('\n', graph->revs->diffopt.file);
 
 		graph_show_remainder(graph);
 
@@ -1326,6 +1330,6 @@ void graph_show_commit_msg(struct git_graph *graph,
 		 * If sb ends with a newline, our output should too.
 		 */
 		if (newline_terminated)
-			putchar('\n');
+			putc('\n', graph->revs->diffopt.file);
 	}
 }
-- 
2.9.0.118.g0e1a633

[PATCH v4 05/10] shortlog: support outputting to streams other than stdout

[Johannes Schindelin]

This will be needed to avoid freopen() in `git format-patch`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/shortlog.c | 13 ++++++++-----
 shortlog.h         |  1 +
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index bfc082e..39d74fe 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -229,6 +229,7 @@ void shortlog_init(struct shortlog *log)
 	log->wrap = DEFAULT_WRAPLEN;
 	log->in1 = DEFAULT_INDENT1;
 	log->in2 = DEFAULT_INDENT2;
+	log->file = stdout;
 }
 
 int cmd_shortlog(int argc, const char **argv, const char *prefix)
@@ -310,22 +311,24 @@ void shortlog_output(struct shortlog *log)
 	for (i = 0; i < log->list.nr; i++) {
 		const struct string_list_item *item = &log->list.items[i];
 		if (log->summary) {
-			printf("%6d\t%s\n", (int)UTIL_TO_INT(item), item->string);
+			fprintf(log->file, "%6d\t%s\n",
+				(int)UTIL_TO_INT(item), item->string);
 		} else {
 			struct string_list *onelines = item->util;
-			printf("%s (%d):\n", item->string, onelines->nr);
+			fprintf(log->file, "%s (%d):\n",
+				item->string, onelines->nr);
 			for (j = onelines->nr - 1; j >= 0; j--) {
 				const char *msg = onelines->items[j].string;
 
 				if (log->wrap_lines) {
 					strbuf_reset(&sb);
 					add_wrapped_shortlog_msg(&sb, msg, log);
-					fwrite(sb.buf, sb.len, 1, stdout);
+					fwrite(sb.buf, sb.len, 1, log->file);
 				}
 				else
-					printf("      %s\n", msg);
+					fprintf(log->file, "      %s\n", msg);
 			}
-			putchar('\n');
+			putc('\n', log->file);
 			onelines->strdup_strings = 1;
 			string_list_clear(onelines, 0);
 			free(onelines);
diff --git a/shortlog.h b/shortlog.h
index de4f86f..5a326c6 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -17,6 +17,7 @@ struct shortlog {
 	char *common_repo_prefix;
 	int email;
 	struct string_list mailmap;
+	FILE *file;
 };
 
 void shortlog_init(struct shortlog *log);
-- 
2.9.0.118.g0e1a633

[PATCH v4 06/10] format-patch: explicitly switch off color when writing to files

[Johannes Schindelin]

We rely on the auto-detection ("is stdout a terminal?") to determine
whether to use color in the output of format-patch or not. That happens
to work because we freopen() stdout when redirecting the output to files.

However, we are about to fix that work-around, in which case the
auto-detection has no chance to guess whether to use color or not.

But then, we do not need to guess to begin with. As argued in the commit
message of 7787570c (format-patch: ignore ui.color, 2011-09-13), we do not
allow the ui.color setting to affect format-patch's output. The only time,
therefore, that we allow color sequences to be written to the output files
is when the user specified the --color command-line option explicitly.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index 27bc88d..5683a42 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1578,6 +1578,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		setup_pager();
 
 	if (output_directory) {
+		if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
+			rev.diffopt.use_color = 0;
 		if (use_stdout)
 			die(_("standard output, or directory, which one?"));
 		if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
-- 
2.9.0.118.g0e1a633

[PATCH v4 07/10] format-patch: avoid freopen()

[Johannes Schindelin]

We just taught the relevant functions to respect the diffopt.file field,
to allow writing somewhere else than stdout. Let's make use of it.

Technically, we do not need to avoid that call in a builtin: we assume
that builtins (as opposed to library functions) are stand-alone programs
that may do with their (global) state. Yet, we want to be able to reuse
that code in properly lib-ified code, e.g. when converting scripts into
builtins.

Further, while we did not *have* to touch the cmd_show() and cmd_cherry()
code paths (because they do not want to write anywhere but stdout as of
yet), it just makes sense to be consistent, making it easier and safer to
move the code later.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 64 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 5683a42..869c23b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -236,7 +236,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)
 		if (rev->commit_format != CMIT_FMT_ONELINE)
 			putchar(rev->diffopt.line_termination);
 	}
-	printf(_("Final output: %d %s\n"), nr, stage);
+	fprintf(rev->diffopt.file, _("Final output: %d %s\n"), nr, stage);
 }
 
 static struct itimerval early_output_timer;
@@ -454,7 +454,7 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
 	pp.fmt = rev->commit_format;
 	pp.date_mode = rev->date_mode;
 	pp_user_info(&pp, "Tagger", &out, buf, get_log_output_encoding());
-	printf("%s", out.buf);
+	fprintf(rev->diffopt.file, "%s", out.buf);
 	strbuf_release(&out);
 }
 
@@ -465,7 +465,7 @@ static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, con
 	char *buf;
 	unsigned long size;
 
-	fflush(stdout);
+	fflush(rev->diffopt.file);
 	if (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||
 	    !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
 		return stream_blob_to_fd(1, sha1, NULL, 0);
@@ -505,7 +505,7 @@ static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)
 	}
 
 	if (offset < size)
-		fwrite(buf + offset, size - offset, 1, stdout);
+		fwrite(buf + offset, size - offset, 1, rev->diffopt.file);
 	free(buf);
 	return 0;
 }
@@ -514,7 +514,8 @@ static int show_tree_object(const unsigned char *sha1,
 		struct strbuf *base,
 		const char *pathname, unsigned mode, int stage, void *context)
 {
-	printf("%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
+	FILE *file = context;
+	fprintf(file, "%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
 	return 0;
 }
 
@@ -574,7 +575,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 
 			if (rev.shown_one)
 				putchar('\n');
-			printf("%stag %s%s\n",
+			fprintf(rev.diffopt.file, "%stag %s%s\n",
 					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					t->tag,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
@@ -593,12 +594,12 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 		case OBJ_TREE:
 			if (rev.shown_one)
 				putchar('\n');
-			printf("%stree %s%s\n\n",
+			fprintf(rev.diffopt.file, "%stree %s%s\n\n",
 					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					name,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
 			read_tree_recursive((struct tree *)o, "", 0, 0, &match_all,
-					show_tree_object, NULL);
+					show_tree_object, rev.diffopt.file);
 			rev.shown_one = 1;
 			break;
 		case OBJ_COMMIT:
@@ -808,7 +809,7 @@ static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
-static int reopen_stdout(struct commit *commit, const char *subject,
+static int open_next_file(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
@@ -832,7 +833,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 	if (!quiet)
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
 
-	if (freopen(filename.buf, "w", stdout) == NULL)
+	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
 		return error(_("Cannot open patch file %s"), filename.buf);
 
 	strbuf_release(&filename);
@@ -891,15 +892,15 @@ static void gen_message_id(struct rev_info *info, char *base)
 	info->message_id = strbuf_detach(&buf, NULL);
 }
 
-static void print_signature(void)
+static void print_signature(FILE *file)
 {
 	if (!signature || !*signature)
 		return;
 
-	printf("-- \n%s", signature);
+	fprintf(file, "-- \n%s", signature);
 	if (signature[strlen(signature)-1] != '\n')
-		putchar('\n');
-	putchar('\n');
+		putc('\n', file);
+	putc('\n', file);
 }
 
 static void add_branch_description(struct strbuf *buf, const char *branch_name)
@@ -968,7 +969,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	committer = git_committer_info(0);
 
 	if (!use_stdout &&
-	    reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
+	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
 		return;
 
 	log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
@@ -991,7 +992,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
 	pp_remainder(&pp, &msg, &sb, 0);
 	add_branch_description(&sb, branch_name);
-	printf("%s\n", sb.buf);
+	fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
 	strbuf_release(&sb);
 
@@ -1000,6 +1001,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	log.wrap = 72;
 	log.in1 = 2;
 	log.in2 = 4;
+	log.file = rev->diffopt.file;
 	for (i = 0; i < nr; i++)
 		shortlog_add_commit(&log, list[i]);
 
@@ -1022,8 +1024,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	diffcore_std(&opts);
 	diff_flush(&opts);
 
-	printf("\n");
-	print_signature();
+	fprintf(rev->diffopt.file, "\n");
+	print_signature(rev->diffopt.file);
 }
 
 static const char *clean_message_id(const char *msg_id)
@@ -1333,7 +1335,7 @@ static void prepare_bases(struct base_tree_info *bases,
 	}
 }
 
-static void print_bases(struct base_tree_info *bases)
+static void print_bases(struct base_tree_info *bases, FILE *file)
 {
 	int i;
 
@@ -1342,11 +1344,11 @@ static void print_bases(struct base_tree_info *bases)
 		return;
 
 	/* Show the base commit */
-	printf("base-commit: %s\n", oid_to_hex(&bases->base_commit));
+	fprintf(file, "base-commit: %s\n", oid_to_hex(&bases->base_commit));
 
 	/* Show the prerequisite patches */
 	for (i = bases->nr_patch_id - 1; i >= 0; i--)
-		printf("prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
+		fprintf(file, "prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
 
 	free(bases->patch_id);
 	bases->nr_patch_id = 0;
@@ -1704,7 +1706,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout,
 				  origin, nr, list, branch_name, quiet);
-		print_bases(&bases);
+		print_bases(&bases, rev.diffopt.file);
 		total++;
 		start_number--;
 	}
@@ -1750,7 +1752,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!use_stdout &&
-		    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
+		    open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
 			die(_("Failed to create output files"));
 		shown = log_tree_commit(&rev, commit);
 		free_commit_buffer(commit);
@@ -1765,15 +1767,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			rev.shown_one = 0;
 		if (shown) {
 			if (rev.mime_boundary)
-				printf("\n--%s%s--\n\n\n",
+				fprintf(rev.diffopt.file, "\n--%s%s--\n\n\n",
 				       mime_boundary_leader,
 				       rev.mime_boundary);
 			else
-				print_signature();
-			print_bases(&bases);
+				print_signature(rev.diffopt.file);
+			print_bases(&bases, rev.diffopt.file);
 		}
 		if (!use_stdout)
-			fclose(stdout);
+			fclose(rev.diffopt.file);
 	}
 	free(list);
 	free(branch_name);
@@ -1805,15 +1807,15 @@ static const char * const cherry_usage[] = {
 };
 
 static void print_commit(char sign, struct commit *commit, int verbose,
-			 int abbrev)
+			 int abbrev, FILE *file)
 {
 	if (!verbose) {
-		printf("%c %s\n", sign,
+		fprintf(file, "%c %s\n", sign,
 		       find_unique_abbrev(commit->object.oid.hash, abbrev));
 	} else {
 		struct strbuf buf = STRBUF_INIT;
 		pp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);
-		printf("%c %s %s\n", sign,
+		fprintf(file, "%c %s %s\n", sign,
 		       find_unique_abbrev(commit->object.oid.hash, abbrev),
 		       buf.buf);
 		strbuf_release(&buf);
@@ -1894,7 +1896,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 		commit = list->item;
 		if (has_commit_patch_id(commit, &ids))
 			sign = '-';
-		print_commit(sign, commit, verbose, abbrev);
+		print_commit(sign, commit, verbose, abbrev, revs.diffopt.file);
 		list = list->next;
 	}
 
-- 
2.9.0.118.g0e1a633

[PATCH v4 08/10] format-patch: use stdout directly

[Johannes Schindelin]

Earlier, we freopen()ed stdout in order to write patches to files.
That forced us to duplicate stdout (naming it "realstdout") because we
*still* wanted to be able to report the file names.

As we do not abuse stdout that way anymore, we no longer need to
duplicate stdout, either.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 869c23b..2a42216 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -805,7 +805,6 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	return git_log_config(var, value, cb);
 }
 
-static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
@@ -831,7 +830,7 @@ static int open_next_file(struct commit *commit, const char *subject,
 		fmt_output_subject(&filename, subject, rev);
 
 	if (!quiet)
-		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
+		printf("%s\n", filename.buf + outdir_offset);
 
 	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
 		return error(_("Cannot open patch file %s"), filename.buf);
@@ -1639,9 +1638,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		get_patch_ids(&rev, &ids);
 	}
 
-	if (!use_stdout)
-		realstdout = xfdopen(xdup(1), "w");
-
 	if (prepare_revision_walk(&rev))
 		die(_("revision walk setup failed"));
 	rev.boundary = 1;
-- 
2.9.0.118.g0e1a633

[PATCH v4 09/10] shortlog: respect the --output=<file> setting

[Johannes Schindelin]

Thanks to the diff option parsing, we already know about this option.
We just have to make use of it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/shortlog.c  | 4 +++-
 t/t4201-shortlog.sh | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 39d74fe..be80547 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -229,7 +229,6 @@ void shortlog_init(struct shortlog *log)
 	log->wrap = DEFAULT_WRAPLEN;
 	log->in1 = DEFAULT_INDENT1;
 	log->in2 = DEFAULT_INDENT2;
-	log->file = stdout;
 }
 
 int cmd_shortlog(int argc, const char **argv, const char *prefix)
@@ -277,6 +276,7 @@ parse_done:
 
 	log.user_format = rev.commit_format == CMIT_FMT_USERFORMAT;
 	log.abbrev = rev.abbrev;
+	log.file = rev.diffopt.file;
 
 	/* assume HEAD if from a tty */
 	if (!nongit && !rev.pending.nr && isatty(0))
@@ -290,6 +290,8 @@ parse_done:
 		get_from_rev(&rev, &log);
 
 	shortlog_output(&log);
+	if (log.file != stdout)
+		fclose(log.file);
 	return 0;
 }
 
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index a977365..bd699e1 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -184,4 +184,10 @@ test_expect_success 'shortlog with revision pseudo options' '
 	git shortlog --exclude=refs/heads/m* --all
 '
 
+test_expect_success 'shortlog with --output=<file>' '
+	git shortlog --output=shortlog master >output &&
+	test ! -s output &&
+	test_line_count = 7 shortlog
+'
+
 test_done
-- 
2.9.0.118.g0e1a633

[PATCH v4 10/10] Ensure that log respects --output=<file>

[Johannes Schindelin]

The test script t4202-log.sh is already pretty long, and it is a good
idea to test --output with a more obscure option, anyway. So let's
test it in conjunction with line-log.

The most important part of this test, of course, is to ensure that the
file is not closed after writing the diff, but only at the very end
of the log output. That is the entire reason why the test tries to
generate a log that covers more than one commit.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t4211-line-log.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 4451127..9d87777 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -99,4 +99,11 @@ test_expect_success '-L with --first-parent and a merge' '
 	git log --first-parent -L 1,1:b.c
 '
 
+test_expect_success '-L with --output' '
+	git checkout parallel-change &&
+	git log --output=log -L :main:b.c >output &&
+	test ! -s output &&
+	test_line_count = 70 log
+'
+
 test_done
-- 
2.9.0.118.g0e1a633

Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery

[Johannes Schindelin]

Hi Junio,

On Tue, 21 Jun 2016, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >>
> >>> We are about to teach the log_tree machinery to reuse the diffopt.file
> >>> setting to output to a file stream different from stdout.
> >>>
> >>> This means that builtin am can no longer ask the diff machinery to
> >>> close the file when actually calling the log_tree machinery (which
> >>> wants to flush the very same file stream that would then already be
> >>> closed).
> >>
> >> Sorry for being slow, but I am not sure why the first paragraph has
> >> to mean the second paragraph.  This existing caller opens a new
> >> stream, sets .fp to it, and expects that the log_tree_commit() to
> >> close it if told by setting .close_file to true, all of which sounds
> >> sensible.
> >>
> >> If a codepath wants to use the same stream for two or more calls to
> >> log_tree by pointing the stream with .fp, it would be of course a
> >> problem for the caller to set .close_file to true in its first call,
> >> as .fp will be closed and no longer usable for second and subsequent
> >> call, and that would be a bug, but for a single-shot call it feels
> >> entirely a sensible request to make, no?
> >>
> >> Obviously you have looked at the codepaths involved a lot longer
> >> than I did, and I do not doubt your conclusion, but I cannot quite
> >> convince myself with the above explanation.
> >>
> >> The option parser of "git diff" family sets ->close_file to true
> >> when the --output option is given.
> >>
> >> Wouldn't this patch break "git log --output=foo -3"?
> >
> > I wonder if the right approach is to stop using .close_file
> > everywhere.
> >
> > With this "do not set .close_file if you use log_tree_commit()",
> > "git log --output=/dev/stdout -3" gets broken, but removing that
> > check is not sufficient to correct the same command with "-p", as
> > letting .close_file to close the output file after finishing a
> > single diff would mean that subsequent write to the same file
> > descriptor will trigger a failure.
> 
> We could say "git log --output=foo -3 [-p]" without any of your
> patches is already broken, and it is a valid excuse to take this
> change that we are not making things worse with it.
> 
> It is just 3/9 is a logical first step to correct that exact
> problem, i.e. some codepaths, even though there is a place that
> holds the output stream and command line parser does prepare one for
> "foo" when --output=foo is given, ignore it and send thigns to the
> standard output stream.  You might not have written 3/9 in order to
> fix that "git log --output=foo" problem, but a fix for it should
> look exactly like your 3/9, I would think.
> 
> And it is sad that this step makes that fix impossible.

Okay, I ended up seeing that there is no way I can avoid Doing The Right
Thing.

It is not quite as pretty as I hoped it would be (callers of
log_tree_commit() that want to call it in a loop still have to override
close_file manually), but it does produce a nicer story.

Please see the fixes in the latest iteration I just sent out.

Ciao,
Dscho

Re: [PATCH 5/5] format-patch: avoid freopen()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> But there's a rub... If you specify --color *explicitly*, use_color is set
> to GIT_COLOR_ALWAYS and the file indeed contains ANSI sequences (i.e. my
> analysis above left out the command-line part).

Heh, the command-line is the _ONLY_ thing I raised, as we knew
ui.color is not an issue in this codepath, in $gmane/297757.

Going back to that and reading again, I suggested to check with
GIT_COLOR_AUTO (i.e. if it is left to "auto", disable it) because I
think the former is a much more future-proof way (imagine that we
may add --color=<some new setting> in the future) than checking with
GIT_COLOR_ALWAYS (i.e. if it is not explicitly set to "always",
disable it).

> In short, I think you're right, I have to guard the assignment, with the
> minor adjustment to test use_color != GIT_COLOR_ALWAYS.

So I am not sure if you said "use_color != GIT_COLOR_ALWAYS" only to
be different from what I suggested (you seem to have a tendency to
do so whenever you can), or there was some other reason.

Re: [PATCH 5/5] format-patch: avoid freopen()

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > But there's a rub... If you specify --color *explicitly*, use_color is set
> > to GIT_COLOR_ALWAYS and the file indeed contains ANSI sequences (i.e. my
> > analysis above left out the command-line part).
> 
> Heh, the command-line is the _ONLY_ thing I raised, as we knew
> ui.color is not an issue in this codepath, in $gmane/297757.
> 
> Going back to that and reading again, I suggested to check with
> GIT_COLOR_AUTO (i.e. if it is left to "auto", disable it) because I
> think the former is a much more future-proof way (imagine that we
> may add --color=<some new setting> in the future) than checking with
> GIT_COLOR_ALWAYS (i.e. if it is not explicitly set to "always",
> disable it).

Well, if I change `rev.diffopt.use_color != GIT_COLOR_ALWAYS` to
`rev.diffopt.use_color == GIT_COLOR_AUTO`, then the files will contain
ugly ANSI color sequences if I run `git format-patch -o . -3`.

The reason is, as I tried to explain, that the use_color field is *not*
initialized to GIT_COLOR_AUTO (which is equivalent to 2), but to -1.

So the difference between your version and mine is that mine works ;-)

Ciao,
Dscho

Re: [PATCH 5/5] format-patch: avoid freopen()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Well, if I change `rev.diffopt.use_color != GIT_COLOR_ALWAYS` to
> `rev.diffopt.use_color == GIT_COLOR_AUTO`, then the files will contain
> ugly ANSI color sequences if I run `git format-patch -o . -3`.
>
> The reason is, as I tried to explain, that the use_color field is *not*
> initialized to GIT_COLOR_AUTO (which is equivalent to 2), but to -1.

Ah, OK.

It still feels safer to force no-color only when it is auto
(i.e. the user said --color=auto) or -1 (i.e. the default), rather
than when it is anything other than always, though.

Re: [PATCH 5/5] format-patch: avoid freopen()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Well, if I change `rev.diffopt.use_color != GIT_COLOR_ALWAYS` to
> `rev.diffopt.use_color == GIT_COLOR_AUTO`, then the files will contain
> ugly ANSI color sequences if I run `git format-patch -o . -3`.
>
> The reason is, as I tried to explain, that the use_color field is *not*
> initialized to GIT_COLOR_AUTO (which is equivalent to 2), but to -1.

OK.  I thought forcing no-color only when it is set to COLOR_AUTO or
it is set to -1 (the default) would be safer, but I changed my mind.

"when we add a new --color=<something.we.do.not.know.yet>,
overriding that end-user wish with the unconditional no-color is
likely to be seen as bug." was the implicit bias behind that
suggestion, but that is not substanticated and substatiatable.

If we write

	if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
        	rev.diffopt.use_color = 0;

and if a user of --color=<something.we.do.not.know.yet> wonders why
her output is not colored, it is clear in the code above that we
disable unless it is set with --color=always, so it won't make
fixing such a future breakage harder.  In fact, if we did

	if (rev.diffopt.use_color == GIT_COLOR_AUTO ||
            rev.diffopt.use_color < 0)
        	rev.diffopt.use_color = 0;

it would make it _harder_ to spot where use_color is turned off when
the person who debugs such an issue.

Thanks.

Re: [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> We are about to teach the log-tree machinery to reuse the diffopt.file
> field to output to a file stream other than stdout, in line with the
> diff machinery already writing to diffopt.file.
>
> However, we might want to write something after the diff in
> log_tree_commit() (e.g. with the --show-linear-break option), therefore
> we must not let the diff machinery close the file (as per
> diffopt.close_file.
>
> This means that log_tree_commit() itself must override the
> diffopt.close_file flag and close the file, and if log_tree_commit() is
> called in a loop, the caller is responsible to do the same.

Makes sense.

> Note: format-patch has an `--output-directory` option. Due to the fact
> that format-patch's options are parsed first, and that the parse-options
> machinery accepts uniquely abbreviated options, the diff options
> `--output` (and `-o`) are shadowed. Therefore close_file is not set to 1
> so that cmd_format_patch() does *not* need to handle the close_file flag
> differently, even if it calls log_tree_commit() in a loop.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/log.c | 15 ++++++++++++---
>  log-tree.c    |  5 ++++-
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 099f4f7..27bc88d 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -243,9 +243,10 @@ static struct itimerval early_output_timer;
>  
>  static void log_show_early(struct rev_info *revs, struct commit_list *list)
>  {
> -	int i = revs->early_output;
> +	int i = revs->early_output, close_file = revs->diffopt.close_file;

Probably not worth a reroll, but I find this kind of thing easier to
read as two separate definitions.

>  	int show_header = 1;

And this was separate from "int i = ...;" for the same reason.  It
is OK to write "int i, j, k;" but "show_header" is something that
keeps track of the more important state during the control flow and
it is easier to read if we make it stand out.  close_file falls into
the same category, I would think.

>  		case commit_error:
> +			if (close_file)
> +				fclose(revs->diffopt.file);

I wondered if we want to also clear, i.e. revs->diffopt.file = NULL, 
but I do not think .close_file does that either, so this is good.

Thanks.

Re: [PATCH v4 06/10] format-patch: explicitly switch off color when writing to files

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> We rely on the auto-detection ("is stdout a terminal?") to determine
> whether to use color in the output of format-patch or not. That happens
> to work because we freopen() stdout when redirecting the output to files.
>
> However, we are about to fix that work-around, in which case the
> auto-detection has no chance to guess whether to use color or not.
>
> But then, we do not need to guess to begin with. As argued in the commit
> message of 7787570c (format-patch: ignore ui.color, 2011-09-13), we do not
> allow the ui.color setting to affect format-patch's output. The only time,
> therefore, that we allow color sequences to be written to the output files
> is when the user specified the --color command-line option explicitly.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

The right fix in the longer term (long after this series lands, that
is) is probably to update the world view that the codepath from
want_color_auto() to check_auto_color() has always held.  In their
world view, when they are asked to make --color=auto decision, the
output always goes the standard output, and that is why they
hardcode isatty(1) to decide.  The existing freopen() was a part of
that world view.

We'd need a workaround like this patch if we want to leave the
want_color_auto() as-is, and as a workaround I think this is the
least invasive one, so let's queue it as-is.

If the codepaths that use diffopt.file (not just this one that is
about output directory hence known to be writing to a file, but all
the log/diff family of commands after this series up to 5/10 has
been applied) have a way to tell want_color_auto() that the output
is going to fileno(diffopt.file), and have check_auto_color() use
that fd instead of the hardcoded 1, the problem this step is trying
to address goes away, and I think that would be the longer-term fix.

Thanks.

>  builtin/log.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 27bc88d..5683a42 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1578,6 +1578,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		setup_pager();
>  
>  	if (output_directory) {
> +		if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
> +			rev.diffopt.use_color = 0;
>  		if (use_stdout)
>  			die(_("standard output, or directory, which one?"));
>  		if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)

Re: [PATCH v4 08/10] format-patch: use stdout directly

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Earlier, we freopen()ed stdout in order to write patches to files.
> That forced us to duplicate stdout (naming it "realstdout") because we
> *still* wanted to be able to report the file names.
>
> As we do not abuse stdout that way anymore, we no longer need to
> duplicate stdout, either.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/log.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

The logical consequence of 07/10; very nice.

Re: [PATCH v4 06/10] format-patch: explicitly switch off color when writing to files

[Johannes Schindelin]

Hi Junio,

On Fri, 24 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > We rely on the auto-detection ("is stdout a terminal?") to determine
> > whether to use color in the output of format-patch or not. That
> > happens to work because we freopen() stdout when redirecting the
> > output to files.
> >
> > However, we are about to fix that work-around, in which case the
> > auto-detection has no chance to guess whether to use color or not.
> >
> > But then, we do not need to guess to begin with. As argued in the
> > commit message of 7787570c (format-patch: ignore ui.color,
> > 2011-09-13), we do not allow the ui.color setting to affect
> > format-patch's output. The only time, therefore, that we allow color
> > sequences to be written to the output files is when the user specified
> > the --color command-line option explicitly.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> The right fix in the longer term (long after this series lands, that
> is) is probably to update the world view that the codepath from
> want_color_auto() to check_auto_color() has always held.  In their
> world view, when they are asked to make --color=auto decision, the
> output always goes the standard output, and that is why they
> hardcode isatty(1) to decide.  The existing freopen() was a part of
> that world view.

I agree, and I briefly looked into this. It looks a bit more involved than
I am prepared for, what with my real goal being the rebase--helper, not
clean-ups ;-)

> We'd need a workaround like this patch if we want to leave the
> want_color_auto() as-is, and as a workaround I think this is the
> least invasive one, so let's queue it as-is.

Thanks!
Dscho

Re: [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute

[Johannes Schindelin]

Hi Junio,

On Fri, 24 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/builtin/log.c b/builtin/log.c
> > index 099f4f7..27bc88d 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -243,9 +243,10 @@ static struct itimerval early_output_timer;
> >  
> >  static void log_show_early(struct rev_info *revs, struct commit_list *list)
> >  {
> > -	int i = revs->early_output;
> > +	int i = revs->early_output, close_file = revs->diffopt.close_file;
> 
> Probably not worth a reroll, but I find this kind of thing easier to
> read as two separate definitions.
> 
> >  	int show_header = 1;
> 
> And this was separate from "int i = ...;" for the same reason.  It
> is OK to write "int i, j, k;" but "show_header" is something that
> keeps track of the more important state during the control flow and
> it is easier to read if we make it stand out.  close_file falls into
> the same category, I would think.

Makes sense. I changed it locally, in case this patch series needs a
re-roll.

> >  		case commit_error:
> > +			if (close_file)
> > +				fclose(revs->diffopt.file);
> 
> I wondered if we want to also clear, i.e. revs->diffopt.file = NULL, 
> but I do not think .close_file does that either, so this is good.

Indeed, the current code in diff_flush() just fclose()s the stream, but
does not reset the "file" field:

	https://github.com/git/git/blob/v2.9.0/diff.c#L4715-L4716

Ciao,
Dscho