Re: [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute

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

Hi Junio,

On Fri, 24 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/builtin/log.c b/builtin/log.c
> > index 099f4f7..27bc88d 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -243,9 +243,10 @@ static struct itimerval early_output_timer;
> >  
> >  static void log_show_early(struct rev_info *revs, struct commit_list *list)
> >  {
> > -	int i = revs->early_output;
> > +	int i = revs->early_output, close_file = revs->diffopt.close_file;
> 
> Probably not worth a reroll, but I find this kind of thing easier to
> read as two separate definitions.
> 
> >  	int show_header = 1;
> 
> And this was separate from "int i = ...;" for the same reason.  It
> is OK to write "int i, j, k;" but "show_header" is something that
> keeps track of the more important state during the control flow and
> it is easier to read if we make it stand out.  close_file falls into
> the same category, I would think.

Makes sense. I changed it locally, in case this patch series needs a
re-roll.

> >  		case commit_error:
> > +			if (close_file)
> > +				fclose(revs->diffopt.file);
> 
> I wondered if we want to also clear, i.e. revs->diffopt.file = NULL, 
> but I do not think .close_file does that either, so this is good.

Indeed, the current code in diff_flush() just fclose()s the stream, but
does not reset the "file" field:

	https://github.com/git/git/blob/v2.9.0/diff.c#L4715-L4716

Ciao,
Dscho