Video conference libification eng discussion - notes from today

Video conference libification eng discussion - notes from today

[Emily Shaffer]

Hi folks, I didn't manage to send the announcement email ahead of
time, but the meeting these notes are from happens weekly on Thursday
at 9:30am Pacific (16:30 UTC for now). Here are the notes from today:

 - What's cooking in libification?
   - Patches we sent regarding libification
- Elijah's series
(https://lore.kernel.org/git/pull.1517.git.1681614205.gitgitgadget@gmail.com/)
 - What happened in the past 1-2 weeks that interested parties or
intermittent contributors need to know?
*   Emily has been working on drafting a style guide for library code,
ETA ~next week
*   Glen is removing global state from config parse, ETA ~this week
    *   When you want metadata about the current value (like scope) we
use globals instead. Working on making those actual context for the
callback
    *   Elijah: glad to see us getting rid of some of that, these
kinds of antipatterns show up all over the place when I'm working on
those refactors
*   How can we avoid introducing new globals?
    *   Elijah: for example, diffcore added a new global for actually no reason
    *   Getting rid of the\_repository is so daunting
    *   Emily: should we introduce some CI rule around new globals in
the diff? Update SubmittingPatches?
    *   Glen: tests which enforce libification also make it difficult
to introduce new globals in libraries. But in some places, like
setting something across the entire process lifetime, we don't have a
better pattern to replace it with, right?
    *   Elijah: in the short term, we do have dozens of globals;
should they go in one place (so we know where to get rid of them)?
Should they go with the file that is using them the most, or
something?
    *   Emily: does putting all the globals in one place make it
easier to tell whether a given path is using a certain global?
    *   Cem: Putting it in one file makes it really easy to tell
people they shouldn't introduce a new global! E.g. adding a comment at
the top of `globals.h` like /\* are you sure you need to add a global
here? \*/
    *   Emily: can coccinelle help?
        *   Glen: probably not, coccinelle runs over all files whether
they're unchanged or not
    *   Emily: does wrapping globals in a getter that traces help?
        *   Elijah: but people will just bypass the getter, unless you
restructure the code so the accessors are the only thing that are
accessible
        *   Cem: This is basically a global to singleton conversion -
but there's no way to guarantee that new globals follow that pattern
either
    *   Emily: I'll spin this out into a separate thread after this meeting
*   Emily: Is it better to add a dependency (e.g. c-tap-harness) or to
roll our own harness in t/helper/, for unit testing libraries?
    *   What else do we have to watch out for when adding a new
dependency to the project? Clone/build times? Licensing?
    *   Glen: Maturity/stability of the library concerns me, do we
need to patch it? Do we need to fix things that are broken there?
    *   Emily: We forked stuff like khash, sha1dc, xdiff, do we
usually send patches back upstream?
        *   Elijah: xdiff died upstream, I've seen people take git's
xdiff and then publish it as standalone xdiff (because we had so many
good patches downstream)
        *   But sha1dc we try to push back upstream, and we do have it
as a submodule as well.
        *   For khash… I think we just one-time imported it
        *   Elijah: We have some other weird deps like libcurl, where
we just use it directly from the package manager (or the Makefile has
to learn about where it comes from) plus a couple more like zlib and
some other one

How to discourage introduction of new globals? (was: Video conference libification eng discussion - notes from today)

[Emily Shaffer]

On Thu, Apr 20, 2023 at 10:01:45AM -0700, Emily Shaffer wrote:
> *   How can we avoid introducing new globals?
>     *   Elijah: for example, diffcore added a new global for actually no reason
>     *   Getting rid of the\_repository is so daunting
>     *   Emily: should we introduce some CI rule around new globals in
> the diff? Update SubmittingPatches?
>     *   Glen: tests which enforce libification also make it difficult
> to introduce new globals in libraries. But in some places, like
> setting something across the entire process lifetime, we don't have a
> better pattern to replace it with, right?
>     *   Elijah: in the short term, we do have dozens of globals;
> should they go in one place (so we know where to get rid of them)?
> Should they go with the file that is using them the most, or
> something?
>     *   Emily: does putting all the globals in one place make it
> easier to tell whether a given path is using a certain global?
>     *   Cem: Putting it in one file makes it really easy to tell
> people they shouldn't introduce a new global! E.g. adding a comment at
> the top of `globals.h` like /\* are you sure you need to add a global
> here? \*/
>     *   Emily: can coccinelle help?
>         *   Glen: probably not, coccinelle runs over all files whether
> they're unchanged or not
>     *   Emily: does wrapping globals in a getter that traces help?
>         *   Elijah: but people will just bypass the getter, unless you
> restructure the code so the accessors are the only thing that are
> accessible
>         *   Cem: This is basically a global to singleton conversion -
> but there's no way to guarantee that new globals follow that pattern
> either
>     *   Emily: I'll spin this out into a separate thread after this meeting

Any thoughts from the rest of the project? To summarize goals:

- The path to library code stays about as hard as it is, doesn't get
  harder by the addition of more globals.
- It's very easy to tell when globals are being used in order to make
  cleaning them up less painful.
- The onus for avoiding new globals doesn't fall entirely on a handful
  of reviewers, who may miss something newly added.