Subject: Re: [PATCH v5 10/15] bugreport: add config values from safelist

From: Emily Shaffer <[email-masked]>
Date: Tue, 4 Feb 2020 18:31:27 -0800

On Fri, Jan 31, 2020 at 10:25:06PM +0100, Martin Ågren wrote:
> On Fri, 24 Jan 2020 at 04:40, <[email-masked]> wrote:
> > Taking the build-time generated array and putting it into a set saves us
> > time - since git_config_bugreport() is called for every option the user
> > has configured, performing option lookup in constant time is a useful
> > optimization.
> 
> I'm sympathetic to your sending out what you have to obtain comments,
> knowing that it's not perfect. It would have saved me some time and
> effort if I'd known that this was the case though. An "[RFC]" tag,
> perhaps. Or at least tweaking the above part of this commit message to
> say that this might be over-engineered, with a reference to [1].
> 
> [1] https://lore.kernel.org/git/[email-masked]/

Yeah, you are right, and thanks for the feedback. I think I had wrapped
up the series before I realized there were those significant outstanding
comments, but I definitely should have said so in this patch.

> 
> > +       int safelist_len = sizeof(bugreport_config_safelist) / sizeof(const char *);
> 
> I was going to suggest ARRAY_SIZE, but then I realized there are some
> outstanding questions around whether you need this stuff in the first
> place. I'd be inclined to guess that the first version of this would be
> "for each safelisted item, obtain it and include", ignoring any "a.*.b"
> business. In which case you wouldn't really need this hashset stuff.

Regardless, I think it's worth doing nicely for now.

It seems to me that supporting wildcarding in the config safelist is a
superset of supporting static strings in that safelist - that is, if I
write it simply to support static strings, a later change to support
wildcards would be welcome and non-breaking.

So I'm going to clean this up without making it more featureful, for
now.

Sorry about the confusion!
 - Emily