Re: [PATCH v2 2/2] bugreport: generate config whitelist based on docs

[Emily Shaffer <emilyshaffer@google.com>]

On Sat, Aug 17, 2019 at 10:38:46PM +0200, Martin Ågren wrote:
> On Sat, 17 Aug 2019 at 02:42, Emily Shaffer <emilyshaffer@google.com> wrote:
> >
> > Add a new step to the build to generate a whitelist of git-config
> > variables which are appropriate to include in the output of
> > git-bugreport. New variables can be added to the whitelist by annotating
> > their documentation in Documentation/config with the line
> > "// bugreport-include".
> 
> These "// bugreport-include" show up in the rendered manpages, both with
> AsciiDoc and Asciidoctor. :-(

Hmm, I don't see it in the troff or the HTML with asciidoc using `make`
- but I do see with asciidoctor, or `asciidoc -d html5`. Nice catch,
thanks.

> 
> > --- a/Documentation/config/sendemail.txt
> > +++ b/Documentation/config/sendemail.txt
> > @@ -1,63 +1,63 @@
> > -sendemail.identity::
> > +sendemail.identity:: // bugreport-exclude
> >         A configuration identity. When given, causes values in the
> 
> If I put each comment on a line of its own (after the config option, but
> I suppose before would work the same way), Asciidoctor truly ignores
> them and everything's fine. And AsciiDoc renders this one and others
> like it ok.
> 
> > -sendemail.aliasesFile::
> > -sendemail.aliasFileType::
> > -sendemail.annotate::
> > +sendemail.aliasesFile:: // bugreport-exclude
> > +sendemail.aliasFileType:: // bugreport-exclude
> > +sendemail.annotate:: // bugreport-include
> 
> However, AsciiDoc (version 8.6.10) seems to effectively replace those
> comments with an empty line during processing, and it makes quite the
> difference here. Instead of these appearing in a compact comma-separated
> list, they are treated as individual items in the description list with
> no supporting content.
> 
> FWIW, I like the idea of annotating things here to make it harder to
> forget this whitelisting when adding a new config item.
> 
> Below is what I came up with as an alternative approach. Feel free to
> steal, squash and/or ignore as you see fit.

This is cool - I'll add this to the commit chain and build on top of it.
Thanks!

> 
> BTW, I'm not sure the grep invocation is portable (-R ? -h ? -P ? -o ?).

Yeah, I'll see what I can do about that (recursive, no filename,
Perl-compatible regex, match only) - I can probably replace this with
something like `find | xargs pgrep` - I'll keep digging. Thanks.

> We should probably also do the usual "create a candidate output file,
> then move it into place" dance for robustness.
> 
> I do think we should test the generated whitelist in some minimal way,
> e.g., to check that it does contain something which objectively belongs
> in the whitelist and -- more importantly IMHO -- does *not* contain
> something that shouldn't be there, such as sendemail.smtpPass.

Good point, I'll add a test for this.

Thanks very much for this!
 - Emily