Hi Junio,
On Fri, 24 Jun 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> 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
--
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