Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 21, 2016, 19:05 UTC
- Message-ID
- <xmqqlh1ypdk8.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <xmqqfus6quii.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 31 quoted lines
> 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.