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

[Junio C Hamano <gitster@pobox.com>]

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.