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

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

[Ghanshyam Thakkar]

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.

Approach:
As described by Elijah in commit message that fixing this cannot be
done in the create_default_files() function as it occurs too late.
This is because both clone and init have different checks and steps
for working with bare flag, like clone creates a new directory
[name].git and sets the GIT_DIR_ENVIRONMENT to it (when not provided
with an explicit dir name arg), while init sets the
GIT_DIR_ENVIRONMENT to the current working directory (when not
provided with an dir name arg). Also there are other steps like
setting no_checkout in a bare repository in builtin/clone.c. These are
all command specific steps which occur in builtin/clone.c and
builtin/init-db.c ,before we ever hit the TODO comment via
[builtin/clone.c]cmd_clone()->[setup.c]init_db()->
[setup.c]create_default_files().

Therefore, rather than centralizing the code in setup.c and adding a
bunch of if-else statements to handle different command specific
scenarios related to bare option, I propose to add a check for
template file config just after parsing of the flags and args in
builtin/init-db.c and builtin/clone.c.

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.

p.s.: Apologies for the weird indenting, due to 70 character limit.
consider it just a prototype.

Thanks!

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

[Christian Couder]

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.

Best,
Christian.

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

[Ghanshyam Thakkar]

On Thu Jan 4, 2024 at 3:54 PM IST, Christian Couder wrote:
> 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.
>
Thank you for your feedback. I will wait for Elijah's response on this and 
perhaps look at other microprojects in the meantime. Although, I think I will 
be able to do this with others help.

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

Appreciate your suggestions above. Will keep them in mind while sending the
patch.

> Best,
> Christian.

Thanks,
Ghanshyam

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

[Elijah Newren]

On Thu, Jan 4, 2024 at 2:24 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> 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.

An important part of solving this problem, if you were to do so, would
be adding several testcases (including some showing how it currently
fails).  Simply adding some or all of those testcases would be a good
micro-project.  (If you take this on, it'd probably be worthwhile to
include a reference to any such added tests in the TODO comment here
so that future folks noticing the TODO are aware of them).  Then, if
adding those testcases goes well and you feel ambitious, you can try
to tackle the underlying problem too.

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

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.   If I knew the
correct solution without digging, I probably would have just
implemented it and sent it in as another patch series.  My TODO was
not meant to be a definitive guide about where to make the fixes
(because I don't know yet), but just a helpful guide that the first
spot you'd expect cannot be the correct location (I already tried a
few things there) and that you need to dig further.  Anyway, I'm sure
the correct place to fix can be figured out with a bit of work, but in
this case, figuring out where the changes need to be made is probably
the majority of the effort.

You may well need to just pick an area, start modifying, go through it
in a debugger and with your testcases, etc., and learn whether
modifications in that area even can solve the problem.  You may have
to discard your first or even second attempts, but take what you
learned to guide you on future attempts.

And if you do get it all working, this is a case where it'd likely be
important to comment in the commit message not just why you are making
changes, but why you believe the area you modified is the correct one
(i.e. to mention why the more obvious places to modify don't actually
solve the problem).  And then be prepared for folks on the mailing
list to use the information you provided in the commit message to dive
in and point out some other way to fix the problem that is even better
but requires you to redo it again.  (And I'm not just saying that
because you're new; I would not be surprised if the same happened to
me.  Others are more familiar with various parts of the general setup
and config code than me, and sometimes additional expertise coupled
with a solution of some sort can make it easier to identify an even
better solution.)

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

[Junio C Hamano]

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?

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

[Ghanshyam Thakkar]

On Fri Jan 5, 2024 at 9:29 PM IST, Junio C Hamano wrote:
> 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.

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.

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

[Junio C Hamano]

"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 ;-).

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

Yeah, if this were still in soon after 4f629539 was written, then
such a change might have been a useful feature enhancement, but risk
of breaking people (third-party tools) who use the same template to
initialize both bare and non-bare repositories is there, so...

Thanks.

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

[Elijah Newren]

On Mon, Jan 8, 2024 at 9:32 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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 ;-).

Sounds fine to me.  I have no particular interest in supporting
core.bare from the template; it's just that in order to do other
cleanup, I needed to remove the init_is_bare_repository global
variable (see c2f76965d02 ("init-db: remove unnecessary global
variable", 2023-05-16)).  Attempting to remove that global variable
made it _look_ like I was changing the code behavior and breaking it
in the case when core.bare was true in the template.  I knew possible
code breakage was what code reviewers would ask about.  And my best
reading of the fact that the variable existed plus how the code was
written suggested to me that indeed someone else thought this might be
important to support.  So, I left the TODO behind to document that I
wasn't breaking the code with my changes (or even changing behavior at
all), and left some hints for the next reader who came along about
where they might start looking if they thought it was important to
fix.

[PATCH] setup: clarify TODO comment about ignoring core.bare

[Ghanshyam Thakkar]

The comment suggested to heed core.bare from template config if no
command line override given. However, it was clarified by Junio that
such values are ignored by intention and does not make sense to
propagate it from template config to the repository config[1].

Therefore, remove the TODO tag and update the comment to add
additional context if such functionality is desired in the future.
Also update the relevant test cases to not expect failure and remove
one redundant testcase.

[1]: https://lore.kernel.org/git/xmqqjzonpy9l.fsf@gitster.g/

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 setup.c                  | 16 ++++++++++------
 t/t1301-shared-repo.sh   | 15 ++-------------
 t/t5606-clone-options.sh |  6 +++---
 3 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/setup.c b/setup.c
index b69b1cbc2a..92e0c3a121 100644
--- a/setup.c
+++ b/setup.c
@@ -1997,16 +1997,20 @@ static int create_default_files(const char *template_path,
 	if (init_shared_repository != -1)
 		set_shared_repository(init_shared_repository);
 	/*
-	 * 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()
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_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
+	test_path_is_missing subdir/HEAD
 '
 
 test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository' '
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
 '
 
 test_expect_success 'prefers config "clone.defaultRemoteName" over default' '
-- 
2.44.0

Re: [PATCH] setup: clarify TODO comment about ignoring core.bare

[Junio C Hamano]

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?

Thanks.

Re: [PATCH] setup: clarify TODO comment about ignoring core.bare

[Ghanshyam Thakkar]

On Fri Mar 1, 2024 at 12:45 AM IST, Junio C Hamano wrote:
> 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?

I agree. In fact, we can remove the prev_bare_repository variable altogether
as it was used solely for the "A || B" expression. Initially this
function used to be in builtin/init-db.c and shared with
builtin/clone.c. In moving to setup.c, an unnecessary global variable
equivalent to prev_bare_repository was removed and therefore
prev_bare_repository was intruduced to not hinder the future possibility
of intruducing the (core.bare from config) feature which might have been
the global variables partial intent[1]. Therefore, I kept the original
expression.

However, in the same series that this was introduced, it was acknowledged
by Elijah[2] that create_default_files() would possibly be too late to heed
core.bare. And indeed it is, as the directories for the worktree or repo
are already created by this point. Therefore, prev_bare_repository does
not seem to have any usecase with/without supporting core.bare from
config.

[1]:
https://lore.kernel.org/git/xmqqsf1bf5ew.fsf@gitster.g/T/#m64125dd80d04ae177944434e7092522325b374c9
[2]: 0f7443bdc7 (init-db: document existing bug with core.bare in
template config, 2023-05-16)

Thanks.

[PATCH v2] setup: remove unnecessary variable

[Ghanshyam Thakkar]

The TODO comment suggested to heed core.bare from template config file
if no command line override given. And the prev_bare_repository
variable seems to have been placed for this sole purpose as it is not
used anywhere else.

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.
Therefore, prev_bare_repository does not have a usecase with/without
supporting core.bare from template. And the removal of
prev_bare_repository is safe as proved by the later part of the
comment:

    "Unfortunately, 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()
    expanding the right hand side:
        = is_bare_repository_cfg && !get_git_work_tree()
        = is_bare_repository_cfg && !work_tree
    note that the last simplification above is valid because nothing
    calls repo_init() or set_git_work_tree() between any of the
    relevant calls in the code, and thus the !get_git_work_tree()
    calls will return the same result each time.  So, what we are
    interested in computing is the right hand side of the line of
    code just above this comment:
        prev_bare_repository || !work_tree
        = is_bare_repository_cfg && !work_tree || !work_tree
        = !work_tree
    because "A && !B || !B == !B" for all boolean values of A & B."

Therefore, remove the TODO comment and remove prev_bare_repository
variable. Also, update relevant testcases and remove one redundant
testcase.

[1]: https://lore.kernel.org/git/xmqqjzonpy9l.fsf@gitster.g/

Helped-by: Elijah Newren <newren@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 setup.c                  | 36 +++---------------------------------
 t/t1301-shared-repo.sh   | 15 ++-------------
 t/t5606-clone-options.sh |  6 +++---
 3 files changed, 8 insertions(+), 49 deletions(-)

diff --git a/setup.c b/setup.c
index b69b1cbc2a..81accd1213 100644
--- a/setup.c
+++ b/setup.c
@@ -1961,7 +1961,6 @@ void create_reference_database(unsigned int ref_storage_format,
 static int create_default_files(const char *template_path,
 				const char *original_git_dir,
 				const struct repository_format *fmt,
-				int prev_bare_repository,
 				int init_shared_repository)
 {
 	struct stat st1;
@@ -1996,34 +1995,8 @@ static int create_default_files(const char *template_path,
 	 */
 	if (init_shared_repository != -1)
 		set_shared_repository(init_shared_repository);
-	/*
-	 * TODO: heed core.bare from config file in templates if no
-	 *       command-line override given
-	 */
-	is_bare_repository_cfg = prev_bare_repository || !work_tree;
-	/* TODO (continued):
-	 *
-	 * Unfortunately, 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()
-	 * expanding the right hand side:
-	 *                 = is_bare_repository_cfg && !get_git_work_tree()
-	 *                 = is_bare_repository_cfg && !work_tree
-	 * note that the last simplification above is valid because nothing
-	 * calls repo_init() or set_git_work_tree() between any of the
-	 * relevant calls in the code, and thus the !get_git_work_tree()
-	 * calls will return the same result each time.  So, what we are
-	 * interested in computing is the right hand side of the line of
-	 * code just above this comment:
-	 *     prev_bare_repository || !work_tree
-	 *        = is_bare_repository_cfg && !work_tree || !work_tree
-	 *        = !work_tree
-	 * because "A && !B || !B == !B" for all boolean values of A & B.
-	 */
+
+	is_bare_repository_cfg = !work_tree;
 
 	/*
 	 * We would have created the above under user's umask -- under
@@ -2175,7 +2148,6 @@ int init_db(const char *git_dir, const char *real_git_dir,
 	int exist_ok = flags & INIT_DB_EXIST_OK;
 	char *original_git_dir = real_pathdup(git_dir, 1);
 	struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
-	int prev_bare_repository;
 
 	if (real_git_dir) {
 		struct stat st;
@@ -2201,7 +2173,6 @@ int init_db(const char *git_dir, const char *real_git_dir,
 
 	safe_create_dir(git_dir, 0);
 
-	prev_bare_repository = is_bare_repository();
 
 	/* Check to see if the repository version is right.
 	 * Note that a newly created repository does not have
@@ -2214,8 +2185,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
 	validate_ref_storage_format(&repo_fmt, ref_storage_format);
 
 	reinit = create_default_files(template_dir, original_git_dir,
-				      &repo_fmt, prev_bare_repository,
-				      init_shared_repository);
+				      &repo_fmt, init_shared_repository);
 
 	/*
 	 * Now that we have set up both the hash algorithm and the ref storage
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_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
+	test_path_is_missing subdir/HEAD
 '
 
 test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository' '
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
 '
 
 test_expect_success 'prefers config "clone.defaultRemoteName" over default' '
-- 
2.44.0

Re: [PATCH v2] setup: remove unnecessary variable

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> The TODO comment suggested to heed core.bare from template config file
> if no command line override given. And the prev_bare_repository
> variable seems to have been placed for this sole purpose as it is not
> used anywhere else.

OK.

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

Correct.  Who said it is much less interesting than what was said,
so I would have written the first part of the paragraph more like

	Values including core.bare from the template file are
	ignored on purpose because they may not make sense for the
	repository being created [1].  Also, the directories for ...

but I'll let it pass.

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

So we used to say "subdir should be created as a bare repository but
we fail to do so", but now "subdir should become a non-bare repository
because 'git init' is run without the --bare option".  OK.

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

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

This is in the same spirit as the first change in t1301, which seems
OK.

Thanks.

Re: [PATCH v2] setup: remove unnecessary variable

[Ghanshyam Thakkar]

On Mon Mar 4, 2024 at 11:46 PM IST, Junio C Hamano wrote:
> 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.

Thanks.

Re: [PATCH v2] setup: remove unnecessary variable

[Junio C Hamano]

"Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:

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

I was merely trying to make sure that I understood your motivation
behind the change, which was described at the end of the commit log
message, i.e. "... and remove one redundant testcase.".

Thanks.