On Wed, 5 Feb 2020 at 03:31, Emily Shaffer <emilyshaffer@xxxxxxxxxx> wrote: > > 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. Yeah, agreed. Also, whatever magic the code for gathering the config items knows, the annotations in Documentation/, i.e., the generated list in bugreport-config-safelist.h will be "synced". That is, until we know how to handle "a.*.b" in the safelist, we probably won't be marking "a.*.b" as safe. And even if we have some crazy mixed build, it's a *safe*list, so we should be fine. (Famous last words?) On that topic though, and just so we don't mess up from the beginning, in the documentation, we typically write something like "branch.<name>.merge", i.e., not "branch.*.merge". So I guess the "a.*.b" feature would be more like "look for '.<', something, something, '>.'." Would that be sufficient? Or would we actually want more fine-grained control, i.e., something like regexes? If so, we'd need to provide those regexes somehow, e.g., as part of the asciidoc macro notation. :-/ Do we need to prepare somehow for such a future? I don't think we do. > So I'm going to clean this up without making it more featureful, for > now. > > Sorry about the confusion! > - Emily No worries. :) Martin
