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

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

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.