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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Feb 5, 2020, 00:30 UTC
Message-ID: <20200205003043.GH87163@google.com>
In-Reply-To: <CAN0heSo2ujJr_Mi5xPv86NneLoXT3DM3dnRmTACs2VQERBq76Q@mail.gmail.com>

On Fri, Jan 31, 2020 at 10:20:05PM +0100, Martin Ågren wrote:

Show 10 quoted lines

> 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.
> 
> "which is recognized by" sounds like it's built-in. Maybe "with a new
> no-op 'bugreport' macro" or something like that.

Sure, I'll wordsmith this a little.

Show 13 quoted lines

> 
> > -sendemail.signedoffcc (deprecated)::
> > +sendemail.signedoffcc (deprecated) bugreport:exclude[x] ::
> >         Deprecated alias for `sendemail.signedoffbycc`.
> >
> > -sendemail.smtpBatchSize::
> > +sendemail.smtpBatchSize bugreport:include[x] ::
> >         Number of messages to be sent per connection, after that a relogin
> >         will happen.  If the value is 0 or undefined, send all messages in
> >         one connection.
> >         See also the `--batch-size` option of linkgit:git-send-email[1].
> 
> Do we really need to list includes *and* excludes?

Need? Nah. The exclude markers are not even acknowledged by the current implementation.

> I could see the point of adding an exclude here and there to signal
> that "this might look innocent enough, but trust me, we really need to
> exclude this" in order to avoid future commits to more or less
> accidentally over-include.

This was more of my line of thinking, along with the thought that some folks may use bug reporting internally, where they have A) more control over which extensions/tools are in use, and B) less concern about sharing things like remote URLs, branch names, etc. and may want to go for a blocklist as opposed to a safelist, to make it easier for their support folks to diagnose.

Show 5 quoted lines

> Should we add some internal documentation and/or a remark in the commit
> message about this? As an example, is "sendemail.signedofcc" sensitive
> enough that we need to explicitly exclude it? If someone wants to come
> along and include it, they don't just need to argue for an inclusion,
> but also for lifting the exclusion. Hmm?

Yeah, I think this is a good point. I'll try to figure out how to reword the commit message, and take another look at the sample include/exclude change I made to see where I can omit entirely.

 - Emily