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

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> We are about to teach the log-tree machinery to reuse the diffopt.file
> field to output to a file stream other than stdout, in line with the
> diff machinery already writing to diffopt.file.
>
> However, we might want to write something after the diff in
> log_tree_commit() (e.g. with the --show-linear-break option), therefore
> we must not let the diff machinery close the file (as per
> diffopt.close_file.
>
> This means that log_tree_commit() itself must override the
> diffopt.close_file flag and close the file, and if log_tree_commit() is
> called in a loop, the caller is responsible to do the same.

Makes sense.

> Note: format-patch has an `--output-directory` option. Due to the fact
> that format-patch's options are parsed first, and that the parse-options
> machinery accepts uniquely abbreviated options, the diff options
> `--output` (and `-o`) are shadowed. Therefore close_file is not set to 1
> so that cmd_format_patch() does *not* need to handle the close_file flag
> differently, even if it calls log_tree_commit() in a loop.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/log.c | 15 ++++++++++++---
>  log-tree.c    |  5 ++++-
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> 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.

>  		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.

Thanks.