Johannes Schindelin <[email protected]> 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 <[email protected]> > --- > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
