Junio C Hamano <gitster@xxxxxxxxx> writes: > Junio C Hamano <gitster@xxxxxxxxx> writes: > >> Johannes Schindelin <johannes.schindelin@xxxxxx> 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. We could say "git log --output=foo -3 [-p]" without any of your patches is already broken, and it is a valid excuse to take this change that we are not making things worse with it. It is just 3/9 is a logical first step to correct that exact problem, i.e. some codepaths, even though there is a place that holds the output stream and command line parser does prepare one for "foo" when --output=foo is given, ignore it and send thigns to the standard output stream. You might not have written 3/9 in order to fix that "git log --output=foo" problem, but a fix for it should look exactly like your 3/9, I would think. And it is sad that this step makes that fix impossible. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html