diff mbox series

[v2,08/21] config: make dependency on repo in `read_early_config()` explicit

Message ID b8aa5dcc0b67e3957dc65f38b7dc02a97cc096a7.1725008898.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series environment: guard reliance on `the_repository` | expand

Commit Message

Patrick Steinhardt Aug. 30, 2024, 9:09 a.m. UTC 
The `read_early_config()` function can be used to read configuration
where a repository has not yet been set up. As such, it is optional
whether or not `the_repository` has already been initialized. If it was
initialized we use its commondir and gitdir. If not, the function will
try to detect the Git directories by itself and, if found, also parse
their config files.

This means that we implicitly rely on `the_repository`. Make this
dependency explicit by passing a `struct repository`. This allows us to
again drop the `USE_THE_REPOSITORY_VARIABLE` define in "config.c".

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 alias.c                |  6 ++++--
 config.c               | 10 ++++------
 config.h               |  2 +-
 help.c                 |  2 +-
 pager.c                |  7 +++++--
 t/helper/test-config.c |  3 ++-
 trace2/tr2_cfg.c       |  4 +++-
 7 files changed, 20 insertions(+), 14 deletions(-)

Comments

James Liu Sept. 4, 2024, 1:46 a.m. UTC | #1 
On Fri Aug 30, 2024 at 7:09 PM AEST, Patrick Steinhardt wrote:
> diff --git a/config.c b/config.c
> index a8357ea9544..043e1c8a078 100644
> --- a/config.c
> +++ b/config.c
> @@ -6,8 +6,6 @@
>   *
>   */
>  
> -#define USE_THE_REPOSITORY_VARIABLE
> -
>  #include "git-compat-util.h"
>  #include "abspath.h"
>  #include "advice.h"
> @@ -2204,7 +2202,7 @@ static void configset_iter(struct config_set *set, config_fn_t fn, void *data)
>  	}
>  }
>  
> -void read_early_config(config_fn_t cb, void *data)
> +void read_early_config(struct repository *repo, config_fn_t cb, void *data)
>  {
>  	struct config_options opts = {0};
>  	struct strbuf commondir = STRBUF_INIT;
> @@ -2212,9 +2210,9 @@ void read_early_config(config_fn_t cb, void *data)
>  
>  	opts.respect_includes = 1;
>  
> -	if (have_git_dir()) {
> -		opts.commondir = repo_get_common_dir(the_repository);
> -		opts.git_dir = repo_get_git_dir(the_repository);
> +	if (repo && repo->gitdir) {
> +		opts.commondir = repo_get_common_dir(repo);
> +		opts.git_dir = repo_get_git_dir(repo);
>  	/*
>  	 * When setup_git_directory() was not yet asked to discover the
>  	 * GIT_DIR, we ask discover_git_directory() to figure out whether there

It doesn't really matter either way since we perform the same checks, but should we do

        if (repo && repo_get_git_dir(repo))

instead of accessing the field directly?
Patrick Steinhardt Sept. 4, 2024, 7:14 a.m. UTC | #2 
On Wed, Sep 04, 2024 at 11:46:47AM +1000, James Liu wrote:
> On Fri Aug 30, 2024 at 7:09 PM AEST, Patrick Steinhardt wrote:
> > diff --git a/config.c b/config.c
> > index a8357ea9544..043e1c8a078 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -6,8 +6,6 @@
> >   *
> >   */
> >  
> > -#define USE_THE_REPOSITORY_VARIABLE
> > -
> >  #include "git-compat-util.h"
> >  #include "abspath.h"
> >  #include "advice.h"
> > @@ -2204,7 +2202,7 @@ static void configset_iter(struct config_set *set, config_fn_t fn, void *data)
> >  	}
> >  }
> >  
> > -void read_early_config(config_fn_t cb, void *data)
> > +void read_early_config(struct repository *repo, config_fn_t cb, void *data)
> >  {
> >  	struct config_options opts = {0};
> >  	struct strbuf commondir = STRBUF_INIT;
> > @@ -2212,9 +2210,9 @@ void read_early_config(config_fn_t cb, void *data)
> >  
> >  	opts.respect_includes = 1;
> >  
> > -	if (have_git_dir()) {
> > -		opts.commondir = repo_get_common_dir(the_repository);
> > -		opts.git_dir = repo_get_git_dir(the_repository);
> > +	if (repo && repo->gitdir) {
> > +		opts.commondir = repo_get_common_dir(repo);
> > +		opts.git_dir = repo_get_git_dir(repo);
> >  	/*
> >  	 * When setup_git_directory() was not yet asked to discover the
> >  	 * GIT_DIR, we ask discover_git_directory() to figure out whether there
> 
> It doesn't really matter either way since we perform the same checks, but should we do
> 
>         if (repo && repo_get_git_dir(repo))
> 
> instead of accessing the field directly?

We in fact can't. `read_early_config()` is typically invoked with the
global `the_repository`. That variable is always set, but its `git_dir`
may not be populated depending on when exactly we call this function and
whether or not we are inside a repository. With `repo_get_git_dir()`
we'd now die in scenarios where it's unset, which we do not want.

Patrick
diff mbox series

Patch

diff --git a/alias.c b/alias.c
index 4daafd9bdae..1a1a141a0ae 100644
--- a/alias.c
+++ b/alias.c
@@ -1,3 +1,5 @@ 
+#define USE_THE_REPOSITORY_VARIABLE
+
 #include "git-compat-util.h"
 #include "alias.h"
 #include "config.h"
@@ -37,7 +39,7 @@  char *alias_lookup(const char *alias)
 {
 	struct config_alias_data data = { alias, NULL };
 
-	read_early_config(config_alias_cb, &data);
+	read_early_config(the_repository, config_alias_cb, &data);
 
 	return data.v;
 }
@@ -46,7 +48,7 @@  void list_aliases(struct string_list *list)
 {
 	struct config_alias_data data = { NULL, NULL, list };
 
-	read_early_config(config_alias_cb, &data);
+	read_early_config(the_repository, config_alias_cb, &data);
 }
 
 void quote_cmdline(struct strbuf *buf, const char **argv)
diff --git a/config.c b/config.c
index a8357ea9544..043e1c8a078 100644
--- a/config.c
+++ b/config.c
@@ -6,8 +6,6 @@ 
  *
  */
 
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "abspath.h"
 #include "advice.h"
@@ -2204,7 +2202,7 @@  static void configset_iter(struct config_set *set, config_fn_t fn, void *data)
 	}
 }
 
-void read_early_config(config_fn_t cb, void *data)
+void read_early_config(struct repository *repo, config_fn_t cb, void *data)
 {
 	struct config_options opts = {0};
 	struct strbuf commondir = STRBUF_INIT;
@@ -2212,9 +2210,9 @@  void read_early_config(config_fn_t cb, void *data)
 
 	opts.respect_includes = 1;
 
-	if (have_git_dir()) {
-		opts.commondir = repo_get_common_dir(the_repository);
-		opts.git_dir = repo_get_git_dir(the_repository);
+	if (repo && repo->gitdir) {
+		opts.commondir = repo_get_common_dir(repo);
+		opts.git_dir = repo_get_git_dir(repo);
 	/*
 	 * When setup_git_directory() was not yet asked to discover the
 	 * GIT_DIR, we ask discover_git_directory() to figure out whether there
diff --git a/config.h b/config.h
index f5fa833cb98..5c730c4f899 100644
--- a/config.h
+++ b/config.h
@@ -198,7 +198,7 @@  int git_config_from_parameters(config_fn_t fn, void *data);
  * `the_repository` has not yet been set up, try to discover the Git
  * directory to read the configuration from.
  */
-void read_early_config(config_fn_t cb, void *data);
+void read_early_config(struct repository *repo, config_fn_t cb, void *data);
 
 /*
  * Read config but only enumerate system and global settings.
diff --git a/help.c b/help.c
index c03863f2265..413c93edaea 100644
--- a/help.c
+++ b/help.c
@@ -618,7 +618,7 @@  const char *help_unknown_cmd(const char *cmd)
 	memset(&other_cmds, 0, sizeof(other_cmds));
 	memset(&aliases, 0, sizeof(aliases));
 
-	read_early_config(git_unknown_cmd_config, NULL);
+	read_early_config(the_repository, git_unknown_cmd_config, NULL);
 
 	/*
 	 * Disable autocorrection prompt in a non-interactive session
diff --git a/pager.c b/pager.c
index 9c24ce62633..40b664f893c 100644
--- a/pager.c
+++ b/pager.c
@@ -1,3 +1,5 @@ 
+#define USE_THE_REPOSITORY_VARIABLE
+
 #include "git-compat-util.h"
 #include "config.h"
 #include "editor.h"
@@ -92,7 +94,8 @@  const char *git_pager(int stdout_is_tty)
 	pager = getenv("GIT_PAGER");
 	if (!pager) {
 		if (!pager_program)
-			read_early_config(core_pager_config, NULL);
+			read_early_config(the_repository,
+					  core_pager_config, NULL);
 		pager = pager_program;
 	}
 	if (!pager)
@@ -298,7 +301,7 @@  int check_pager_config(const char *cmd)
 	data.want = -1;
 	data.value = NULL;
 
-	read_early_config(pager_command_config, &data);
+	read_early_config(the_repository, pager_command_config, &data);
 
 	if (data.value)
 		pager_program = data.value;
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index e193079ed54..33247f0e92e 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -96,7 +96,8 @@  int cmd__config(int argc, const char **argv)
 	struct config_set cs;
 
 	if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
-		read_early_config(early_config_cb, (void *)argv[2]);
+		read_early_config(the_repository, early_config_cb,
+				  (void *)argv[2]);
 		return 0;
 	}
 
diff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c
index d96d908bb9d..22a99a0682a 100644
--- a/trace2/tr2_cfg.c
+++ b/trace2/tr2_cfg.c
@@ -1,3 +1,5 @@ 
+#define USE_THE_REPOSITORY_VARIABLE
+
 #include "git-compat-util.h"
 #include "config.h"
 #include "strbuf.h"
@@ -124,7 +126,7 @@  void tr2_cfg_list_config_fl(const char *file, int line)
 	struct tr2_cfg_data data = { file, line };
 
 	if (tr2_cfg_load_patterns() > 0)
-		read_early_config(tr2_cfg_cb, &data);
+		read_early_config(the_repository, tr2_cfg_cb, &data);
 }
 
 void tr2_list_env_vars_fl(const char *file, int line)