From: Martin Ågren Date: Wed, 05 Feb 2020 20:12:42 GMT Subject: Re: [PATCH v5 10/15] bugreport: add config values from safelist Message-ID: In-Reply-To: <20200205023127.GL87163@google.com> On Wed, 5 Feb 2020 at 03:31, Emily Shaffer wrote: > > On Fri, Jan 31, 2020 at 10:25:06PM +0100, Martin Ågren wrote: > > On Fri, 24 Jan 2020 at 04:40, 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@google.com/ > > 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..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