Re: [RFC PATCH] config: teach `repo_config()` to allow `repo` to be NULL

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> 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.

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

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.