[GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.

>
> Hello,
>
> I'm currently an undergrad beginning my journey of contributing to the
> Git project. I am seeking feedback on doing "Heed core.bare from
> template config file when no command line override given" described
> here
> https://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/
> by Elijah Newren, as a microproject. I would like to know from the
> community, if the complexity and scope of the project is appropriate
> for a microproject.

> e.g. in builtin/init-db.c :
>
> static int template_bare_config(const char *var, const char *value,
>                      const struct config_context *ctx, void *cb)
> {
>        if(!strcmp(var,"core.bare")) {

>              is_bare_repository_cfg = git_config_bool(var, value);
>        }
>        return 0;
> }
>
> int cmd_init_db(int argc, const char **argv, const char *prefix)
> {
> ...
> ...
>        if(is_bare_repository_cfg==-1) {

>              if(!template_dir)
>                    git_config_get_pathname("init.templateDir",
>                                            &template_dir);
>
>              if(template_dir) {
>                    const char* template_config_path
>                                 = xstrfmt("%s/config",
>                    struct stat st;
>
>                    if(!stat(template_config_path, &st) &&
>                      !S_ISDIR(st.st_mode)) {
>                          git_config_from_file(template_bare_cfg,
>                                         template_config_path, NULL);
>                    }
>              }
> ...
> ...
>        return init_db(git_dir, real_git_dir, template_dir, hash_algo,
>                       initial_branch, init_shared_repository, flags);
> }
>
> I also wanted to know if the global config files should have an effect
> in deciding if the repo is bare or not.
>
> Curious to know your thoughts on, if this is the right approach or
> does it require doing refactoring to bring all the logic in setup.c.
> Based on your feedback, I can quickly send a patch.

> On Tue, Jan 2, 2024 at 11:17 PM Ghanshyam Thakkar
> <shyamthakkar001@gmail.com> wrote:
> >
> > Hello,
> >
> > I'm currently an undergrad beginning my journey of contributing to the
> > Git project. I am seeking feedback on doing "Heed core.bare from
> > template config file when no command line override given" described
> > here
> > https://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/
> > by Elijah Newren, as a microproject. I would like to know from the
> > community, if the complexity and scope of the project is appropriate
> > for a microproject.
>
> Thanks for your interest in the next GSoC!
>
> My opinion is that it's too complex for a micro-project. Now maybe if
> Elijah or others are willing to help you on it, perhaps it will work
> out. I think it's safer to look at simpler micro-projects though.
>

> > e.g. in builtin/init-db.c :
> >
> > static int template_bare_config(const char *var, const char *value,
> >                      const struct config_context *ctx, void *cb)
> > {
> >        if(!strcmp(var,"core.bare")) {
>
> We like to have a space character between "if" and "(" as well as after a ","
>
> >              is_bare_repository_cfg = git_config_bool(var, value);
> >        }
> >        return 0;
> > }
> >
> > int cmd_init_db(int argc, const char **argv, const char *prefix)
> > {
> > ...
> > ...
> >        if(is_bare_repository_cfg==-1) {
>
> We like to have a space character both before and after "==" as well
> as between "if" and "(".
>
> >              if(!template_dir)
> >                    git_config_get_pathname("init.templateDir",
> >                                            &template_dir);
> >
> >              if(template_dir) {
> >                    const char* template_config_path
> >                                 = xstrfmt("%s/config",
> >                    struct stat st;
> >
> >                    if(!stat(template_config_path, &st) &&
> >                      !S_ISDIR(st.st_mode)) {
> >                          git_config_from_file(template_bare_cfg,
> >                                         template_config_path, NULL);
> >                    }
> >              }
> > ...
> > ...
> >        return init_db(git_dir, real_git_dir, template_dir, hash_algo,
> >                       initial_branch, init_shared_repository, flags);
> > }
> >
> > I also wanted to know if the global config files should have an effect
> > in deciding if the repo is bare or not.
> >
> > Curious to know your thoughts on, if this is the right approach or
> > does it require doing refactoring to bring all the logic in setup.c.
> > Based on your feedback, I can quickly send a patch.
>
> I don't know this area of the code well, so I don't think I can help
> you much on this.

>
> On Tue, Jan 2, 2024 at 11:17 PM Ghanshyam Thakkar
> <shyamthakkar001@gmail.com> wrote:
> >
> > Hello,
> >
> > I'm currently an undergrad beginning my journey of contributing to the
> > Git project. I am seeking feedback on doing "Heed core.bare from
> > template config file when no command line override given" described
> > here
> > https://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/
> > by Elijah Newren, as a microproject. I would like to know from the
> > community, if the complexity and scope of the project is appropriate
> > for a microproject.
>
> Thanks for your interest in the next GSoC!
>
> My opinion is that it's too complex for a micro-project. Now maybe if
> Elijah or others are willing to help you on it, perhaps it will work
> out. I think it's safer to look at simpler micro-projects though.

> > e.g. in builtin/init-db.c :
> >
> > static int template_bare_config(const char *var, const char *value,
> >                      const struct config_context *ctx, void *cb)
> > {
> >        if(!strcmp(var,"core.bare")) {
>
> We like to have a space character between "if" and "(" as well as after a ","
>
> >              is_bare_repository_cfg = git_config_bool(var, value);
> >        }
> >        return 0;
> > }
> >
> > int cmd_init_db(int argc, const char **argv, const char *prefix)
> > {
> > ...
> > ...
> >        if(is_bare_repository_cfg==-1) {
>
> We like to have a space character both before and after "==" as well
> as between "if" and "(".
>
> >              if(!template_dir)
> >                    git_config_get_pathname("init.templateDir",
> >                                            &template_dir);
> >
> >              if(template_dir) {
> >                    const char* template_config_path
> >                                 = xstrfmt("%s/config",
> >                    struct stat st;
> >
> >                    if(!stat(template_config_path, &st) &&
> >                      !S_ISDIR(st.st_mode)) {
> >                          git_config_from_file(template_bare_cfg,
> >                                         template_config_path, NULL);
> >                    }
> >              }
> > ...
> > ...
> >        return init_db(git_dir, real_git_dir, template_dir, hash_algo,
> >                       initial_branch, init_shared_repository, flags);
> > }
> >
> > I also wanted to know if the global config files should have an effect
> > in deciding if the repo is bare or not.
> >
> > Curious to know your thoughts on, if this is the right approach or
> > does it require doing refactoring to bring all the logic in setup.c.
> > Based on your feedback, I can quickly send a patch.
>
> I don't know this area of the code well, so I don't think I can help
> you much on this.

> Elijah Newren <newren@gmail.com> 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.

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

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

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

>
> "Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:
>
> >> 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.
>
> I obviously agree with that, after saying that I suspect 0f7443bd
> comes from a misunderstanding ;-).

>  	/*
> -	 * TODO: heed core.bare from config file in templates if no
> -	 *       command-line override given
> +	 * Note: The below line simply checks the presence of worktree (the
> +	 * simplification of which is given after the line) and core.bare from
> +	 * config file is not taken into account when deciding if the worktree
> +	 * should be created or not, even if no command line override given.
> +	 * That is intentional. Therefore, if in future we want to heed
> +	 * core.bare from config file, we should do it before we create any
> +	 * subsequent directories for worktree or repo because until this point
> +	 * they should already be created.
>  	 */
>  	is_bare_repository_cfg = prev_bare_repository || !work_tree;

> -	/* TODO (continued):
> +	/* Note (continued):
>  	 *
> -	 * Unfortunately, the line above is equivalent to
> +	 * The line above is equivalent to
>  	 *    is_bare_repository_cfg = !work_tree;
> -	 * which ignores the config entirely even if no `--[no-]bare`
> -	 * command line option was present.
>  	 *
>  	 * To see why, note that before this function, there was this call:
>  	 *    prev_bare_repository = is_bare_repository()

> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> >  	/*
> > -	 * TODO: heed core.bare from config file in templates if no
> > -	 *       command-line override given
> > +	 * Note: The below line simply checks the presence of worktree (the
> > +	 * simplification of which is given after the line) and core.bare from
> > +	 * config file is not taken into account when deciding if the worktree
> > +	 * should be created or not, even if no command line override given.
> > +	 * That is intentional. Therefore, if in future we want to heed
> > +	 * core.bare from config file, we should do it before we create any
> > +	 * subsequent directories for worktree or repo because until this point
> > +	 * they should already be created.
> >  	 */
> >  	is_bare_repository_cfg = prev_bare_repository || !work_tree;
>
> I do not recall the discussion; others may want to discuss if the
> change above is desirable, before I come back to the topic later.
>
> But I see this long comment totally unnecessary and distracting.
>
> > -	/* TODO (continued):
> > +	/* Note (continued):
> >  	 *
> > -	 * Unfortunately, the line above is equivalent to
> > +	 * The line above is equivalent to
> >  	 *    is_bare_repository_cfg = !work_tree;
> > -	 * which ignores the config entirely even if no `--[no-]bare`
> > -	 * command line option was present.
> >  	 *
> >  	 * To see why, note that before this function, there was this call:
> >  	 *    prev_bare_repository = is_bare_repository()
>
> If it can be proven that the assignment can be simplified to lose
> the "prev_bare_repository ||" part, then the above comment can be
> used as part of the proposed log message for a commit that makes
> such a change.  There is no reason to leave such a long comment to
> leave the more complex "A || B" expression when it can be simplified
> to "B", no?

> However, it was clarified by Junio [1] that such values (including
> core.bare) are ignored intentionally and does not make sense to
> propagate them from template config to repository config. Also, the
> directories for the worktree and repository are already created, and
> therefore the bare/non-bare decision has already been made, by the
> point we reach the codepath where the TODO comment is placed.

> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> index b1eb5c01b8..29cf8a9661 100755
> --- a/t/t1301-shared-repo.sh
> +++ b/t/t1301-shared-repo.sh
> @@ -52,7 +52,7 @@ test_expect_success 'shared=all' '
>  	test 2 = $(git config core.sharedrepository)
>  '
>  
> -test_expect_failure 'template can set core.bare' '
> +test_expect_success 'template cannot set core.bare' '
>  	test_when_finished "rm -rf subdir" &&
>  	test_when_finished "rm -rf templates" &&
>  	test_config core.bare true &&
> @@ -60,18 +60,7 @@ test_expect_failure 'template can set core.bare' '
>  	mkdir -p templates/ &&
>  	cp .git/config templates/config &&
>  	git init --template=templates subdir &&
> -	test_path_exists subdir/HEAD
> +	test_path_is_missing subdir/HEAD
>  '

> -
> -test_expect_success 'template can set core.bare but overridden by command line' '
> -	test_when_finished "rm -rf subdir" &&
> -	test_when_finished "rm -rf templates" &&
> -	test_config core.bare true &&
> -	umask 0022 &&
> -	mkdir -p templates/ &&
> -	cp .git/config templates/config &&
> -	git init --no-bare --template=templates subdir &&
> -	test_path_exists subdir/.git/HEAD
> -'

> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index a400bcca62..e93e0d0cc3 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -120,14 +120,14 @@ test_expect_success 'prefers -c config over --template config' '
>  
>  '
>  
> -test_expect_failure 'prefers --template config even for core.bare' '
> +test_expect_success 'ignore --template config for core.bare' '
>  
>  	template="$TRASH_DIRECTORY/template-with-bare-config" &&
>  	mkdir "$template" &&
>  	git config --file "$template/config" core.bare true &&
>  	git clone "--template=$template" parent clone-bare-config &&
> -	test "$(git -C clone-bare-config config --local core.bare)" = "true" &&
> -	test_path_is_file clone-bare-config/HEAD
> +	test "$(git -C clone-bare-config config --local core.bare)" = "false" &&
> +	test_path_is_missing clone-bare-config/HEAD
>  '

> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> > -
> > -test_expect_success 'template can set core.bare but overridden by command line' '
> > -	test_when_finished "rm -rf subdir" &&
> > -	test_when_finished "rm -rf templates" &&
> > -	test_config core.bare true &&
> > -	umask 0022 &&
> > -	mkdir -p templates/ &&
> > -	cp .git/config templates/config &&
> > -	git init --no-bare --template=templates subdir &&
> > -	test_path_exists subdir/.git/HEAD
> > -'
>
> This removal is a bit unexpected.  Is it because we established with
> the previous test that core.bare in the template should not affect
> the outcome, so this is not worth testing?

> Yes, in the previous testcase we determined that template cannot set
> core.bare. Therefore, this testcase would be like testing
> --bare/--no-bare option, which is already done in 0001-init.sh and
> t5601-clone.sh. However, I don't have strong opinion on this. I can add
> it back if you think it is worth it.