Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 21, 2016, 18:14 UTC
- Message-ID
- <xmqqfus6quii.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <973f9f676225aa98377f607ced1ff474f39b863f.1466505222.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 7 quoted lines
> We are about to teach the log_tree machinery to reuse the diffopt.file > setting to output to a file stream different from stdout. > > This means that builtin am can no longer ask the diff machinery to > close the file when actually calling the log_tree machinery (which > wants to flush the very same file stream that would then already be > closed).
Sorry for being slow, but I am not sure why the first paragraph has to mean the second paragraph. This existing caller opens a new stream, sets .fp to it, and expects that the log_tree_commit() to close it if told by setting .close_file to true, all of which sounds sensible.
If a codepath wants to use the same stream for two or more calls to log_tree by pointing the stream with .fp, it would be of course a problem for the caller to set .close_file to true in its first call, as .fp will be closed and no longer usable for second and subsequent call, and that would be a bug, but for a single-shot call it feels entirely a sensible request to make, no?
Obviously you have looked at the codepaths involved a lot longer than I did, and I do not doubt your conclusion, but I cannot quite convince myself with the above explanation.
The option parser of "git diff" family sets ->close_file to true when the --output option is given.
Wouldn't this patch break "git log --output=foo -3"?
Show 49 quoted lines
> To stave off similar problems in the future, report it as a bug if
> log_tree_commit() is called with a non-zero diffopt.close_file.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/am.c | 6 ++++--
> log-tree.c | 3 +++
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 0e28a62..47d78aa 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1437,6 +1437,7 @@ static int write_commit_patch(const struct am_state *state, struct commit *commi
> {
> struct rev_info rev_info;
> FILE *fp;
> + int res;
>
> fp = fopen(am_path(state, "patch"), "w");
> if (!fp)
> @@ -1453,10 +1454,11 @@ static int write_commit_patch(const struct am_state *state, struct commit *commi
> DIFF_OPT_SET(&rev_info.diffopt, FULL_INDEX);
> rev_info.diffopt.use_color = 0;
> rev_info.diffopt.file = fp;
> - rev_info.diffopt.close_file = 1;
> add_pending_object(&rev_info, &commit->object, "");
> diff_setup_done(&rev_info.diffopt);
> - return log_tree_commit(&rev_info, commit);
> + res = log_tree_commit(&rev_info, commit);
> + fclose(fp);
> + return res;
> }
>
> /**
> diff --git a/log-tree.c b/log-tree.c
> index 78a5381..dc0180d 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -864,6 +864,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
> struct log_info log;
> int shown;
>
> + if (opt->diffopt.close_file)
> + die("BUG: close_file is incompatible with log_tree_commit()");
> +
> log.commit = commit;
> log.parent = NULL;
> opt->loginfo = &log;