| Message ID | d8530a300b4cf0f854f2b0d03c79876c11d81116.1723013714.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Stop using `the_repository` in "config.c" | expand |
On 24/08/07 08:58AM, Patrick Steinhardt wrote: > When computing branch "includeIf" conditions we use `the_repository` to > obtain the main ref store. We really shouldn't depend on this global > repository though, but should instead use the repository that is being > passed to us via `struct config_include_data`. Otherwise, when parsing > configuration of e.g. submodules, we may end up evaluating the condition > the via the wrong refdb. > > Fix this. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > config.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/config.c b/config.c > index 831c9eacb0..08437f75e5 100644 > --- a/config.c > +++ b/config.c > @@ -300,13 +300,14 @@ static int include_by_gitdir(const struct key_value_info *kvi, > return ret; > } > > -static int include_by_branch(const char *cond, size_t cond_len) > +static int include_by_branch(struct config_include_data *data, > + const char *cond, size_t cond_len) > { > int flags; > int ret; > struct strbuf pattern = STRBUF_INIT; > - const char *refname = !the_repository->gitdir ? > - NULL : refs_resolve_ref_unsafe(get_main_ref_store(the_repository), > + const char *refname = (!data->repo || !data->repo->gitdir) ? > + NULL : refs_resolve_ref_unsafe(get_main_ref_store(data->repo), > "HEAD", 0, NULL, &flags); > const char *shortname; This works the same so long as `config_include_data` always has its repository set. I wonder if for `!data->repo` we should instead signal a BUG? Otherwise we would silently return NULL in such cases. Maybe that would be the desired behavior though? > > @@ -406,7 +407,7 @@ static int include_condition_is_true(const struct key_value_info *kvi, > else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len)) > return include_by_gitdir(kvi, opts, cond, cond_len, 1); > else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len)) > - return include_by_branch(cond, cond_len); > + return include_by_branch(inc, cond, cond_len); > else if (skip_prefix_mem(cond, cond_len, "hasconfig:remote.*.url:", &cond, > &cond_len)) > return include_by_remote_url(inc, cond, cond_len); > -- > 2.46.0.dirty >
On Fri, Aug 09, 2024 at 03:47:50PM -0500, Justin Tobler wrote: > On 24/08/07 08:58AM, Patrick Steinhardt wrote: > > When computing branch "includeIf" conditions we use `the_repository` to > > obtain the main ref store. We really shouldn't depend on this global > > repository though, but should instead use the repository that is being > > passed to us via `struct config_include_data`. Otherwise, when parsing > > configuration of e.g. submodules, we may end up evaluating the condition > > the via the wrong refdb. > > > > Fix this. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > config.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/config.c b/config.c > > index 831c9eacb0..08437f75e5 100644 > > --- a/config.c > > +++ b/config.c > > @@ -300,13 +300,14 @@ static int include_by_gitdir(const struct key_value_info *kvi, > > return ret; > > } > > > > -static int include_by_branch(const char *cond, size_t cond_len) > > +static int include_by_branch(struct config_include_data *data, > > + const char *cond, size_t cond_len) > > { > > int flags; > > int ret; > > struct strbuf pattern = STRBUF_INIT; > > - const char *refname = !the_repository->gitdir ? > > - NULL : refs_resolve_ref_unsafe(get_main_ref_store(the_repository), > > + const char *refname = (!data->repo || !data->repo->gitdir) ? > > + NULL : refs_resolve_ref_unsafe(get_main_ref_store(data->repo), > > "HEAD", 0, NULL, &flags); > > const char *shortname; > > This works the same so long as `config_include_data` always has its > repository set. I wonder if for `!data->repo` we should instead signal a > BUG? Otherwise we would silently return NULL in such cases. Maybe that > would be the desired behavior though? It is expected that the repository may not be set, namely when reading configuration via `read_early_config()` and `read_very_early_config()`. We wouldn't want to hit the refdb there, so that is fine. We also have `read_protected_config()`, which requires a bit more thought. It does respect includes, so you may think we want to read the refdb there. But protected config is defined as having system, global or command scope, which to me means that we shouldn't end up reading config data from the current repository. So not hitting the refdb there also seems like the right thing to do. Patrick
diff --git a/config.c b/config.c index 831c9eacb0..08437f75e5 100644 --- a/config.c +++ b/config.c @@ -300,13 +300,14 @@ static int include_by_gitdir(const struct key_value_info *kvi, return ret; } -static int include_by_branch(const char *cond, size_t cond_len) +static int include_by_branch(struct config_include_data *data, + const char *cond, size_t cond_len) { int flags; int ret; struct strbuf pattern = STRBUF_INIT; - const char *refname = !the_repository->gitdir ? - NULL : refs_resolve_ref_unsafe(get_main_ref_store(the_repository), + const char *refname = (!data->repo || !data->repo->gitdir) ? + NULL : refs_resolve_ref_unsafe(get_main_ref_store(data->repo), "HEAD", 0, NULL, &flags); const char *shortname; @@ -406,7 +407,7 @@ static int include_condition_is_true(const struct key_value_info *kvi, else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len)) return include_by_gitdir(kvi, opts, cond, cond_len, 1); else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len)) - return include_by_branch(cond, cond_len); + return include_by_branch(inc, cond, cond_len); else if (skip_prefix_mem(cond, cond_len, "hasconfig:remote.*.url:", &cond, &cond_len)) return include_by_remote_url(inc, cond, cond_len);
When computing branch "includeIf" conditions we use `the_repository` to obtain the main ref store. We really shouldn't depend on this global repository though, but should instead use the repository that is being passed to us via `struct config_include_data`. Otherwise, when parsing configuration of e.g. submodules, we may end up evaluating the condition the via the wrong refdb. Fix this. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- config.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)