* Video conference libification eng discussion - notes from today
@ 2023-04-20 17:01 Emily Shaffer
2023-04-20 17:08 ` How to discourage introduction of new globals? (was: Video conference libification eng discussion - notes from today) Emily Shaffer
0 siblings, 1 reply; 2+ messages in thread
From: Emily Shaffer @ 2023-04-20 17:01 UTC (permalink / raw
To: Git List
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* How to discourage introduction of new globals? (was: Video conference libification eng discussion - notes from today)
2023-04-20 17:01 Video conference libification eng discussion - notes from today Emily Shaffer
@ 2023-04-20 17:08 ` Emily Shaffer
0 siblings, 0 replies; 2+ messages in thread
From: Emily Shaffer @ 2023-04-20 17:08 UTC (permalink / raw
To: Git List; +Cc: chooglen, newren, cscallsign
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-04-20 17:09 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-20 17:01 Video conference libification eng discussion - notes from today Emily Shaffer
2023-04-20 17:08 ` How to discourage introduction of new globals? (was: Video conference libification eng discussion - notes from today) Emily Shaffer
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).