Re: [PATCH 0/2] Simplify "commented" API functions

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 13, 2024, 06:15 UTC
Message-ID: <ZuPYZW3jYas4kJzC@pks.im>
In-Reply-To: <20240912205301.1809355-1-gitster@pobox.com>

On Thu, Sep 12, 2024 at 01:52:59PM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> We [earlier] noticed that a few helper functions that format strings
> into a strbuf, prefixed with an arbitrary comment leading character,
> are forcing their callers to always pass the same argument.  Instead
> of keeping this excess flexibility nobody wants in practice, narrow
> the API so that all callers of these functions will get the same
> comment leading character.
> 
> Superficially it may appear that this goes against one of the recent
> trend, "war on global variables", but I doubt it matters much in the
> longer run.

I'm not quite sure I agree. The comment strings we have are in theory broken, because they can be configured differently per repository. So if you happen to have a Git command that operates on multiple repositories at once and that needs to pay attention to that config then it would now use the same comment character for both repositories, which I'd argue is the wrong thing to do.

The recent patch series that makes "environment.c" aware of `USE_THE_REPOSITORY_VARIABLE` already converted some of the global config variables to be per-repository, because ultimately all of them are broken in the described way. So from my point of view we should aim to convert remaining ones to be per-repository, as well.

Show 15 quoted lines

> These call sites all have already been relying on the global
> "comment_line_str" anyway, and passing the variable from the top of
> arbitrary deep call chain, which may not care specifically about
> this single variable comment_line_str, would not scale as a
> solution.  If we were to make it a convention to pass something from
> the very top of the call chain everywhere, it should not be an
> individual variable that is overly specific, like comment_line_str.
> Rather, it has to be something that allows access to a bag of all
> the global settings (possibly wider than "the_repository" struct).
> We can also limit our reliance to the globals by allowing a single
> global function call to obtaion such a bag of all global settings,
> i.e. "get_the_environment()", and allow everybody, including
> functions at the leaf level, to ask "what is the comment_line_str in
> the environment I am working in?".  As part of the libification, we
> can plug in an appropriate shim for get_the_environment().

This here I can definitely agree with. I think the best fit we have in general is the "repo-settings.c" subsystem, which is also where I've moved some of the previously-global settings into. It still has some downsides in its current format:

  - It depends on a repository, but I'd argue it shouldn't such that we
    can also query configuration in repo-less settings.

  - `prepare_repo_settings()` makes up for a bad calling convention. I
    think that lazy accessors are way easier to use.

But it is a start, and something we can continue to build on.

Patrick