Re: [PATCH 1/3] repository: move git_*_encoding configs to repo scope

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 17, 2024, 05:03 UTC
Message-ID: <ZxCakURyWqXPyacj@pks.im>
In-Reply-To: <Zw/22XHvtOw6IEHe@five231003>

On Wed, Oct 16, 2024 at 10:54:41PM +0530, Kousik Sanagavarapu wrote:

Show 34 quoted lines

> On Wed, Oct 16, 2024 at 08:15:37AM +0200, Patrick Steinhardt wrote:
> > I guess it would be nice to have a set of convenice functions in our
> > config code that know to handle the case where the passed-in repository
> > is `NULL`. If so, they'd only parse the global- and system-level config.
> > If it is set, it would parse all three of them.
> > 
> > I also kind of agree that it should likely not be put into the `struct
> > repository` in that case. I think where exactly to put things will
> > always depend on the current usecase. I bet that in most cases, we
> > should be able to get away with not storing the value anywhere global at
> > all, which would be the best solution in my opinion:
> > 
> >   - It can either be stored on the stack if we don't have to pass it
> >     around everywhere.
> > 
> >   - It can be passed around in a specific structure if we pass the value
> >     within in a certain subsystem, only.
> > 
> >   - Or we can parse it on an as-needed basis if it happens deep down in
> >     the calling stack when it's used essentially everwhere.
> > 
> > Now there will be situations where we used to store things globally as a
> > caching mechanism, and not caching it may have performance impacts. But
> > I guess that most cases do not fall into this category.
> 
> I like the idea of dynamically fetching the value of the config and not
> caching it somewhere - although in this particular case, ie the
> *_encoding configs I'm guessing it would be better that we cache these
> configs.
> 
> Now the important question is where.  In point 2, you mention about
> having a separate structure - do you mean for all those configs which
> would not be a part of "struct repository"?  Of course in their
> respective subsystems.

I was mostly referring to configs that apply to a specific subsystem. So if it is used in that subsystem only, then it likely is a good idea to put the cached value into a structure that gets passed around. Many subsystems already do have such a structure, so it's only a matter of reading the value once at the entry point into the subsystem.

> Coming to point 3, won't we still have to store it somewhere when we do
> need it deep down the calling stack where it is used everywhere, since
> we need to pass it around?

I was mostly referring to the case where it's only used by a single function deep down in the calling stack. So here it could just be parsed ad-hoc.

In the end we'll probably have to decide on a case by case basis what's best.

Patrick