On Wed, Nov 04, 2020 at 12:27:30PM -0500, Eric Sunshine wrote:
> > diff --git a/builtin/log.c b/builtin/log.c
> > @@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > + } else if (rev.diffopt.close_file) {
> > + /*
> > + * The diff code parsed --output; it has already opened the
> > + * file, but but we must instruct it not to close after each
> > + * diff.
> > + */
> > + rev.diffopt.close_file = 0;
> > } else {
>
> The commit message's justification for supporting --output seems
> reasonable. However, my knee-jerk reaction to the implementation was
> that it feels overly magical and a bit too hacky. I can see the logic
> in it but it also leaves a bad taste when the implementation has to
> "undo" a side-effect of some other piece of code, which makes it feel
> unplanned and fragile. The question which popped into my mind
> immediately was "why not handle --output explicitly via
> builtin_format_patch_options[] along with other first-class options?".
> This review comment may or may not be actionable; it's just expressing
> surprise and a bit of nose-wrinkling I experienced.
I agree it's a bit magical. I didn't really consider writing a new
"--output" option for format-patch, because I came at it from the angle
of "let's unbreak the existing diff --output option". Which isn't
necessarily a defense, but just an explanation.
FWIW, that unsetting of rev.diffopt.close_file is exactly how git-log
does it. So while I agree it's a bit ugly, this is the intended way for
--output to be used across multiple diffs, and with log_tree_commit().
I'd prefer to go this way for now, and if somebody wants to make it less
ugly, they can clean up all of the callers in one go.
Amusingly, 6ea57703f6 (log: prepare log/log-tree to reuse the
diffopt.close_file attribute, 2016-06-22) which introduced this code
foresaw the format-patch bug here.
-Peff
