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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Tue, 21 Jun 2016, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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.
> 
> 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.

Okay, I ended up seeing that there is no way I can avoid Doing The Right
Thing.

It is not quite as pretty as I hoped it would be (callers of
log_tree_commit() that want to call it in a loop still have to override
close_file manually), but it does produce a nicer story.

Please see the fixes in the latest iteration I just sent out.

Ciao,
Dscho