On Fri Jan 5, 2024 at 9:29 PM IST, Junio C Hamano wrote: > Elijah Newren <newren@xxxxxxxxx> writes: > > > If you look back at the mailing list discussion on the series that > > introduced this TODO comment you are trying to address, you'll note > > that both Glen and I dug into the code and attempted to explain it to > > each other, and we both got it wrong on our first try. > > I think you meant 0f7443bd (init-db: document existing bug with > core.bare in template config, 2023-05-16), where it says: > > The comments in create_default_files() talks about reading config from > the config file in the specified `--templates` directory, which leads to > the question of whether core.bare could be set in such a config file and > thus whether the code is doing the right thing. > > But I suspect the all of the above comes from a misunderstanding. > The comment the above commit log message talks about is: > > /* > * First copy the templates -- we might have the default > * config file there, in which case we would want to read > * from it after installing. > * > * Before reading that config, we also need to clear out any cached > * values (since we've just potentially changed what's available on > * disk). > */ > > This primarily comes from my 4f629539 (init-db: check template and > repository format., 2005-11-25), whose focus was to control the way > HEAD symref is created, but care was taken to avoid propagating > values from the configuration variables in the template that do not > make sense for the repository being initialized. The most important > thing being the repository format version, but the intent to avoid > nonsense combination between the characteristic the new repository > has and the configuration values copied from the template was not > limited to the format version. > > Specifically, the commit that introduced the comment never wanted to > honor core.bare in the template. I do not think I has core.bare in > mind when I wrote the comment, but I would have described it as the > same category as the repository format version, i.e. something you > would not want to copy, if I were pressed to clarify back then. Then I suppose this warrants updating the TODO comment in create_default_files(), which currently can be interpreted as this being a unwanted behavior. And also amending the testcases which currently display this as knwon breakage. > Besides, create_default_files() is way too late, even if we wanted > to create a bare repository when the template config file says > core.bare = true, as the caller would already have created before > passing $DIR (when "git --bare init $DIR" was run) or $DIR/.git > (when "git init $DIR" was run) to the function. > > If somebody wants to always create a bare repository by having > core.bare=true in their template and if we wanted to honor it (which > I am dubious of the value of, by the way), I would think the right > place to do so would be way before create_default_files() is called. > When running "git init [$DIR]", long before calling init_db(), we > decide if we are about to create a bare repository and either create > $DIR or $DIR/.git. What is in the template, if we really wanted to > do so, should be read before that happens, no? That is what I proposed in my original email, after which I had a working solution which passed all the tests. That solution was indeed to check for core.bare in the template before we set GIT_DIR_ENVIRONMENT, which subsequently creates either $DIR or $DIR/.git as you described above. Regardless, I can send the patch with updated comments to clarify that ignoring core.bare from template files is the intended behavior and amend the test_expect_failure testcases, with Elijah's consensus. Thanks.
