Patrick Steinhardt <ps@xxxxxx> writes: > 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. > 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. > - 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.
