Re: [PATCH v3 3/9] bugreport: add version and system information

[Emily Shaffer <emilyshaffer@google.com>]

On Mon, Nov 11, 2019 at 02:48:13PM +0100, Johannes Schindelin wrote:
> > > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > > > index 2ef16440a0..7232d31be7 100644
> > > > --- a/builtin/bugreport.c
> > > > +++ b/builtin/bugreport.c
> > > > @@ -1,4 +1,5 @@
> > > >  #include "builtin.h"
> > > > +#include "bugreport.h"
> > > >  #include "stdio.h"
> > > >  #include "strbuf.h"
> > > >  #include "time.h"
> > > > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +void add_header(FILE *report, const char *title)
> > > > +{
> > > > +	struct strbuf buffer = STRBUF_INIT;
> > > > +	strbuf_addf(&buffer, "\n\n[%s]\n", title);
> > > > +	strbuf_write(&buffer, report);
> > >
> > > This leaks `buffer`. Why not write into `report` via `fprintf()`
> > > directly?
> >
> > Rather, to match the style of the rest of the builtin, modified
> > get_header to add the header to a passed-in strbuf instead of
> > modifying the file directly.
> 
> Hmm. It makes the code less elegant in my opinion. I would rather either
> render the entire bug report into a single `strbuf` and then write it
> via `write_in_full()`, or use `fprintf()` directly.

Yeah, that sounds good. I will do that. :)