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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 20, 2016, 17:01 UTC
Message-ID: <xmqqwplju74a.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <babf95df5f610feb6c2ae7f2ed3cff98bab47fe2.1466420060.git.johannes.schindelin@gmx.de>

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

Show 5 quoted lines

> 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.

Show 6 quoted lines

>  		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().

Show 12 quoted lines

> @@ -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.