Johannes Schindelin <johannes.schindelin@xxxxxx> 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@xxxxxx>
> ---
> 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.
--
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