Re: [PATCH 3/3] format-patch: support --output option

> > 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.