Junio C Hamano <gits...@pobox.com> writes: > Johannes Schindelin <johannes.schinde...@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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
