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

From: Martin Ågren <martin.agren@gmail.com>
Date: Feb 5, 2020, 20:12 UTC
Message-ID: <CAN0heSpkjACrgz8T=-VoQxgMZhMM-oSSpVz4HAPB27b_h3fGpA@mail.gmail.com>
In-Reply-To: <20200205023127.GL87163@google.com>

On Wed, 5 Feb 2020 at 03:31, Emily Shaffer <emilyshaffer@google.com> wrote:

Show 35 quoted lines

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

Show 5 quoted lines

> So I'm going to clean this up without making it more featureful, for
> now.
>
> Sorry about the confusion!
>  - Emily

No worries. :)

Martin