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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2024, 17:23 UTC
Message-ID: <xmqqjzffzcvk.fsf@gitster.g>
In-Reply-To: <ZuPYZW3jYas4kJzC@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 6 quoted lines

> 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.

Correct. It needs a move from global to a member in a repository instance, but the same "hey do not keep passing the same parameter" motivation behind these patches applies, as the existing call sites most likely will instead pass "the_repository->comment_line_str" to these two functions. The simplification would move that reference to "the_repository->comment_line_str" down to these two functions.

Show 5 quoted lines

> 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.

Yes, I view the environement change somewhat incomplete and it was annoying to see things other than the_repository itself and those that implicitly refer to the_repository covered by the CPP macro.

But we need to step back a bit in order to make the environment change better. Not everything works inside a repository and you may not even have a repository but want to refer to a comment character (say, "git bugreport" run outside a repository, perhaps, and the bugreport may want to honor end-user configuration for commentChar to mark its various sections). Earlier I said it may make sense to reimplement the global as a member of a repository instance, but that is not entirely true. Such a member in a repository struct may be a good implementation detail for anybody who asks "what comment character should I be using in the context I am called?", and there may be "const char *get_comment_line_str(struct repository *)" that accepts which repository to work with, but such a function would need to be prepared to work without any repository, working out of the system and per-user configuration files.

Show 7 quoted lines

>   - 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.

Yup.