On Sun, Mar 26, 2017 at 03:41:14PM +0200, René Scharfe wrote:
> Am 25.03.2017 um 22:11 schrieb Jeff King:
> > The most correct way is that the caller of log_write_email_headers() and
> > diff_flush() should have a function-local strbuf which holds the data,
> > gets passed to diff_flush() as some kind opaque context, and then is
> > freed afterwards. We don't have such a context, but if we were to abuse
> > diff_options.stat_sep _temporarily_, that would still be a lot cleaner.
> > I.e., something like this:
> >
> > struct strbuf stat_sep = STRBUF_INIT;
> >
> > /* may write into stat_sep, depending on options */
> > log_write_email_headers(..., &stat_sep);
> > opt->diffopt.stat_sep = stat_sep.buf;
> >
> > diff_flush(&opt->diffopt);
> > opt->diffopt.stat_sep = NULL;
> > strbuf_release(&stat_sep);
> >
> > But it's a bit tricky because those two hunks happen in separate
> > functions, which means passing the strbuf around.
>
> You could have a destructor callback, called at the end of diff_flush().
Yeah, though now we have lifetime rules around stat_sep. How long does
it stay good? Which functions decide it's now done? Are we sure they
alweays get called? We're just tacking that onto the end of diff_flush()
because the caller responsibilities are so unclear, but that's making an
assumption that it always gets called.
This case might be simple enough that it's true, but it feels like a
leaky module boundary.
> Hmm. I'm a fan of callbacks, but using them can make the code a bit
> hard to follow. And void context pointers add a type safety hazard.
> Do we need to be this generic? How about switching stat_sep to strbuf?
> fmt_output_commit() requires an allocation anyway, so why not allocate
> stat_sep as well?
Right, I think that is the simplest thing, but it falls afoul of the
lifetime rules above.
If the rule is "the stat_sep gets printed once and then freed", that's
pretty reasonable. But I wonder if there are cases where we might not
print stat_sep (or call diff_flush at all for that matter). I'm not sure
if that's possible with --attach, though, which constrains us to
format-patch.
> if (opt->mime_boundary) {
> - static char buffer[1024];
> struct strbuf filename = STRBUF_INIT;
Actually, I guess the absolute simplest thing would be swap this out for
a static strbuf, and leave the ownership with the function. That's ugly,
but it's how it works now, and lets us drop the fixed buffer.
Another option is to put it in rev_info, and make the rev_info owner
free it (which we know is restricted to cmd_format_patch(), as it is the
only one who uses mime_boundary).
-Peff