Re: [PATCH 2/2] config: fix evaluating "onbranch" with nonexistent git dir
- From
shejialuo <shejialuo@gmail.com>
- Date
- Sep 24, 2024, 14:11 UTC
- Message-ID
- <ZvLInrIQRj8xpSgF@ArchLinux>
- In-Reply-To
- <535d0d07506e8248e47f90c1a7581679fc297b3d.1727171197.git.ps@pks.im>
On Tue, Sep 24, 2024 at 12:05:46PM +0200, Patrick Steinhardt wrote:
Show 22 quoted lines
> The `include_by_branch()` function is responsible for evaluating whether > or not a specific include should be pulled in based on the currently > checked out branch. Naturally, his condition can only be evaluated when > we have a properly initialized repository with a ref store in the first > place. This is why the function guards against the case when either > `data->repo` or `data->repo->gitdir` are `NULL` pointers. > > But the second check is insufficient: the `gitdir` may be set even > though the repository has not been initialized. Quoting "setup.c": > > NEEDSWORK: currently we allow bogus GIT_DIR values to be set in some > code paths so we also need to explicitly setup the environment if the > user has set GIT_DIR. It may be beneficial to disallow bogus GIT_DIR > values at some point in the future. > > So when either the GIT_DIR environment variable or the `--git-dir` > global option are set by the user then `the_repository` may end up with > an initialized `gitdir` variable. And this happens even when the dir is > invalid, like for example when it doesn't exist. It follows that only > checking for whether or not `gitdir` is `NULL` is not sufficient for us > to determine whether the repository has been properly initialized. >
When I dive into this bug report, I feel so wired about this behavior. I don't mind whether the code sets "gitdir" field. This is not important. In "setup.c::setup_git_directory_gently", it will set the "the_repository->gitdir" by checking the environment variable "GIT_DIR_ENVIRONMENT". And by using `--git-dir` option, the code will set this environment variable, so these two ways will set the "gitdir" field in the global variable "the_repository".
We actually check the validation of "--gir-dir" option before we set this value. Let me give you an example here:
$ git --git-dir=notexist -c includeIf.onbranch:main.path=any fsck fatal: not a git repository: 'notexist'
I am curious here. And I notice the following difference in "setup.c::setup_explicit_git_dir" function:
if (!is_git_directory(gitdirenv)) {
if (nongit_ok) {
*nongit_ok = 1;
...
return NULL;
}
die(_("not a git repository: '%s'"), gitdirenv);
}Apparently, the above example will execute the "die". For "git-archive(1)", it will simply return NULL. This is because we allow some commands to run outside of the git repo. And we distinguish them by using the ".option" filed:
{ "apply", cmd_apply, RUN_SETUP_GENTLY },
{ "fsck", cmd_fsck, RUN_SETUP },As we can see, the code will check whether the "gitdir" (although it is not set into "the_repository" structure yet) is a valid git repository. We already have this information.
In f7d61c4135 (config: don't depend on `the_repository` with branch conditions, 2024-08-13). "config.c::include_by_branch" drops the global variable "the_repository". This solves the problem reported by Ronan. Because it happens in the set up process, the "data->repo" will be NULL.
config_with_options(cb, data, NULL, NULL, &opts);
int config_with_options(config_fn_t fn, void *data,
const struct git_config_source *config_source,
struct repository *repo,
const struct config_options *opts)
{
struct config_include_data inc = CONFIG_INCLUDE_INIT;...
inc.repo = repo;
}But the problem still exists for "git-archive(1)", in "cmd_archive" function, we will initialize the configurations by using "repo_config". But we are not inside the repo.
I wonder how we use the global variable "the_repository". I think the main problem here is that we use "the_repository" structure outside of the repo where we have already broken the semantics of the "the_repository" variable.
Show 14 quoted lines
> This issue can lead to us triggering a BUG: when using a config with an > "includeIf.onbranch:" condition outside of a repository while using the > `--git-dir` option pointing to an invalid Git directory we may end up > trying to evaluate the condition even though the ref storage format has > not been set up. > > This bisects to 173761e21b (setup: start tracking ref storage format, > 2023-12-29), but that commit really only starts to surface the issue > that has already existed beforehand. The code to check for `gitdir` was > introduced via 85fe0e800c (config: work around bug with > includeif:onbranch and early config, 2019-07-31), which tried to fix > similar issues when we didn't yet have a repository set up. But the fix > was incomplete as it missed the described scenario. >
Yes, exactly. Because before 173761e21b, the code will always find the files backend, it does care about whether we are in the git repo or not.
unsigned int format = REF_STORAGE_FORMAT_FILES;
const struct ref_storage_be *be = find_ref_storage_backend(format);So, it won't complain.
Show 6 quoted lines
> As the quoted comment mentions, we'd ideally refactor the code to not > set up `gitdir` with an invalid value in the first place, but that may > be a bigger undertaking. Instead, refactor the code to use the ref > storage format as an indicator of whether or not the ref store has been > set up to fix the bug. >
Should we? From my above comments, it does no matter whether we set the "gitdir" in the "the_repository". Because we already check this and we have this information. I think the main problem is that we use "the_repository" badly.
We could run git commands inside the repo or outside the repo. If we run git commands outside the repo, should we use the "the_repository" variable? I guess we should not.
Because "git_config", "repo_config" and so on use the global variable "the_repository", so we will encounter the trouble. But if we could use something like "data->repo". We will make everything OK:
1. When running commands inside the git repo, we set "data->repo" to be "the_repository". 2. When ruing commands outside the git repo, we set "data->repo" to be NULL.
Show 49 quoted lines
> Reported-by: Ronan Pigott <ronan@rjp.ie> > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > config.c | 15 +++++++++------ > t/t1305-config-include.sh | 2 +- > 2 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/config.c b/config.c > index 1266eab0860..a11bb85da30 100644 > --- a/config.c > +++ b/config.c > @@ -306,13 +306,16 @@ static int include_by_branch(struct config_include_data *data, > int flags; > int ret; > struct strbuf pattern = STRBUF_INIT; > - 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; > + const char *refname, *shortname; > > - if (!refname || !(flags & REF_ISSYMREF) || > - !skip_prefix(refname, "refs/heads/", &shortname)) > + if (!data->repo || data->repo->ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN) > + return 0; > + > + refname = refs_resolve_ref_unsafe(get_main_ref_store(data->repo), > + "HEAD", 0, NULL, &flags); > + if (!refname || > + !(flags & REF_ISSYMREF) || > + !skip_prefix(refname, "refs/heads/", &shortname)) > return 0; > > strbuf_add(&pattern, cond, cond_len); > diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh > index ad08db72308..517d6c86937 100755 > --- a/t/t1305-config-include.sh > +++ b/t/t1305-config-include.sh > @@ -389,7 +389,7 @@ test_expect_success 'onbranch without repository' ' > test_must_fail nongit git config get foo.bar > ' > > -test_expect_failure 'onbranch without repository but explicit nonexistent Git directory' ' > +test_expect_success 'onbranch without repository but explicit nonexistent Git directory' ' > test_when_finished "rm -f .gitconfig config.inc" && > git config set -f .gitconfig "includeIf.onbranch:**.path" config.inc && > git config set -f config.inc foo.bar baz && > -- > 2.46.0.551.gc5ee8f2d1c.dirty