Re: [PATCH v5 09/15] bugreport: generate config safelist based on docs

> On Fri, 24 Jan 2020 at 04:35, <emilyshaffer@google.com> wrote:
> > Add a new step to the build to generate a safelist of git-config
> > variables which are appropriate to include in the output of
> > git-bugreport. New variables can be added to the safelist by annotating
> > their documentation in Documentation/config with the
> > "bugreport" macro, which is recognized by AsciiDoc and
> > AsciiDoctor.
> >
> > Some configs are private in nature, and can contain remote URLs,
> > passwords, or other sensitive information. In the event that a user
> > doesn't notice their information while reviewing a bugreport, that user
> > may leak their credentials to other individuals, mailing lists, or bug
> > tracking tools inadvertently. Heuristic blocklisting of configuration
> > keys is imperfect and prone to false negatives; given the nature of the
> > information which can be leaked, a safelist is more reliable.
> 
> I sort of wonder whether safelist/blocklist is to prefer over
> whitelist/blacklist, or if it's the other way round. The former are new
> to me, whereas the latter are the terms I would have used. But that's
> just me, of course. I was a little surprised, that's all.

> 
> > Implement a new no-op "bugreport" macro for use as
> > "bugreport:include[x]" to annotate the config keys that should be
> > included in the automatically generated safelist. Use "exclude" for the
> > others.
> >
> > With Asciidoctor, it's ok to say "bugreport:include[]", but AsciiDoc
> > seems to want something between the brackets. A bit unfortunate, but
> > not a huge problem -- we'll just provide an "x".
> 
> I recognize this reasoning :-) and I'm not terribly opposed to it, but
> after some nights' sleeping on this, I have to wonder if
> "annotate:bugreport[include]" wouldn't be better than "bugreport[x]"
> with that ugly "x". Maybe this isn't the biggest problem, but if we
> expect this macro to eventually sit right next to ~90% of all our config
> variables...

> 
> > "doc-diff" reports that this macro doesn't render at all. That is,
> > these are both empty after this commit:
> >
> >   cd Documentation
> >   ./doc-diff --asciidoctor :/"bugreport: add tool" HEAD
> >   ./doc-diff --asciidoc    :/"bugreport: add tool" HEAD
> 
> That was true in [1], but alas, no more. In that patch, it's sort of
> obvious from the diff how it adds a "class" which "end"s.
> 
> [1] https://lore.kernel.org/git/20190817203846.31609-1-martin.agren@gmail.com/
> 
> > --- a/Documentation/asciidoctor-extensions.rb
> > +++ b/Documentation/asciidoctor-extensions.rb
> > @@ -37,6 +37,10 @@ module Git
> >            output = output.sub(/<\/refmeta>/, new_tags + "</refmeta>")
> >          end
> >          output
> > +
> > +    class BugReportProcessor < Asciidoctor::Extensions::InlineMacroProcessor
> > +      def process(parent, action, attrs)
> > +        ""
> >        end
> >      end
> >    end
> 
> But this one doesn't add an "end", and Asciidoctor trips up badly.

> 
> > +  # The bugreport macro does nothing as far as rendering is
> > +  # concerned -- we just grep for it in the sources.
> > +  inline_macro Git::Documentation::BugReportProcessor, :bugreport
> 
> (I never much liked this copy-paste comment then, and I still don't, to
> be honest.)