On Fri, Feb 28, 2025 at 11:44 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Phillip Wood <phillip.wood123@xxxxxxxxx> writes: > > > Thanks for working on this, I like the idea but looking at > > read_very_early_config() it sets "opts.ignore_cmdline = 1" which means > > that this will ignore any config options passed with "git -c > > key=value". I think it would be better to call config_with_options() > > with the appropriate options directly. > > hmph. > Hello, > > For this to work all the commands that run outside a repository would > > have to read the config via repo_config(), and take care not to call > > any of the repo_config_get_*() functions. They mostly seem to do that > > but "git for-each-repo" calls repo_config_get_string_multi() - it > > should be easy enough to convert that to a callback when that command > > is updated to stop using "the_repository" > > Well the only reason why we want to allow NULL in repo_config() > calls is to accomodate this pattern, if I am reading the discussion > correctly. > > * A command that wants to run in a repository (i.e. marked with > RUN_SETUP, not RUN_SETUP_GENTLY) is invoked with "-h" and outside > the repository. > > * cmd_foo() is called with "-h" option in argv[]; repo is set to > NULL. > > * The typical start-up sequence for any builtin cmd_foo() is to > read the configuration and then call parseopt, and the latter is > how "git foo -h" is handled. > > * Historically, the "call the configuration" was done by making a > call to git_config(), which used the_repository [*]. But recent > trend is to use the repo passed down to cmd_foo() instead, and > replacing git_config() with repo_config() blindly would break > unless repo_config() is prepared to take NULL. Yeah, these all are true. Also, "git for-each-repo" and many other commands seems not to make use of rep_config() or git_config(). I think this change and functions are good for command marked RUN_SETUP since their behaviour is pretty straight forward. I think, we might want to use another approach for other commands that are not marked RUN_SETUP if we decide to remove the_repository from them. > > So, any command that requires to be in a repository would not go > beyond its call to parse_options() when called with repo==NULL. > Either it would have already died inside getup_git_directory() when > "-h" is not given, or parse_options() would have gave the usage > string and exited. > > For commands like "for-each-repo" that itself wants to be able to > run outside a repository by marking itself as RUN_SETUP_GENTLY, > they do have to update the code after the parse_options() > themselves, of course, but I view it as a separate issue from this > "we make git_config() as the first thing in everything---we want > to replace it with repo_config()" patch. > > Thanks. > > > [Footnote] > > * I personally think that it is an unhealthy fundamentalism to try > eradicating the use of the_repository even from the top-level > calls of cmd_foo() functions, which are like traditional main(). > > They are never meant to be reused as a subroutine that can work > on an arbitrary repository from any arbitrary codepaths. The > special repository instance, the_repository, is set up to be > suitable to work in the current repository, if exists, and gives > a reasonable fallback behaviour when we are not inside a > repository, which behaves exactly how we want it to behave there. > > The more library-ish helper functions (I am talking about > distinction between what is in say builtin/diff.c and diff-lib.c > here and referring to the latter) are totally different matter, > of course. They are meant to be reused and they should be made > to work on an arbitrary repository from arbitrary codepaths, > hence reducing the reliance on the_repository is a good goal for > them to aim at. Yeah, I understand. I believe these change we are trying to make to builtin commands that are marked RUN_SETUP is a good goal due to how they work, what do you think ? Also, about the testing, I was thinking of using the clar framework or the test-tool, do you have any in mind ? Thank you.
