Re: [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 24, 2016, 20:56 UTC
- Message-ID
- <xmqq60syfgqf.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <044f3c6359b63eaa18229cd73e6815e8e15c9778.1466607667.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 12 quoted lines
> 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.
Show 23 quoted lines
> 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.