On Fri, Jan 31, 2020 at 10:25:06PM +0100, Martin Ågren wrote: > On Fri, 24 Jan 2020 at 04:40, <emilyshaffer@xxxxxxxxxx> 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/20200124032905.GA37541@xxxxxxxxxx/ 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
