{"thread":{"id":"60679","subject":"[GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.","startedAt":"2024-01-02T22:07:23Z","lastAt":"2024-03-04T21:53:53Z","messageCount":15,"participants":["Ghanshyam Thakkar","Christian Couder","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"486220","messageId":"85d4e83c-b6c4-4308-ac8c-a65c911c8a95@gmail.com","threadId":"60679","inReplyTo":null,"subject":"[GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-02T22:07:18Z","receivedAt":"2024-01-02T22:07:23Z","isPatch":false,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Hello,\n\nI'm currently an undergrad beginning my journey of contributing to the\nGit project. I am seeking feedback on doing \"Heed core.bare from\ntemplate config file when no command line override given\" described\nhere \nhttps://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/\nby Elijah Newren, as a microproject. I would like to know from the\ncommunity, if the complexity and scope of the project is appropriate\nfor a microproject.\n\nApproach:\nAs described by Elijah in commit message that fixing this cannot be\ndone in the create_default_files() function as it occurs too late.\nThis is because both clone and init have different checks and steps\nfor working with bare flag, like clone creates a new directory\n[name].git and sets the GIT_DIR_ENVIRONMENT to it (when not provided\nwith an explicit dir name arg), while init sets the\nGIT_DIR_ENVIRONMENT to the current working directory (when not\nprovided with an dir name arg). Also there are other steps like\nsetting no_checkout in a bare repository in builtin/clone.c. These are\nall command specific steps which occur in builtin/clone.c and\nbuiltin/init-db.c ,before we ever hit the TODO comment via\n[builtin/clone.c]cmd_clone()->[setup.c]init_db()->\n[setup.c]create_default_files().\n\nTherefore, rather than centralizing the code in setup.c and adding a\nbunch of if-else statements to handle different command specific\nscenarios related to bare option, I propose to add a check for\ntemplate file config just after parsing of the flags and args in\nbuiltin/init-db.c and builtin/clone.c.\n\ne.g. in builtin/init-db.c :\n\nstatic int template_bare_config(const char *var, const char *value,\n                     const struct config_context *ctx, void *cb)\n{\n       if(!strcmp(var,\"core.bare\")) {\n             is_bare_repository_cfg = git_config_bool(var, value);\n       }\n       return 0;\n}\n\nint cmd_init_db(int argc, const char **argv, const char *prefix)\n{\n...\n...\n       if(is_bare_repository_cfg==-1) {\n             if(!template_dir)\n                   git_config_get_pathname(\"init.templateDir\",\n                                           &template_dir);\n\n             if(template_dir) {\n                   const char* template_config_path\n                                = xstrfmt(\"%s/config\",\n                   struct stat st;\n\n                   if(!stat(template_config_path, &st) &&\n                     !S_ISDIR(st.st_mode)) {\n                         git_config_from_file(template_bare_cfg,\n                                        template_config_path, NULL);\n                   }\n             }\n...\n...\n       return init_db(git_dir, real_git_dir, template_dir, hash_algo,\n                      initial_branch, init_shared_repository, flags);\n}\n\nI also wanted to know if the global config files should have an effect\nin deciding if the repo is bare or not.\n\nCurious to know your thoughts on, if this is the right approach or\ndoes it require doing refactoring to bring all the logic in setup.c.\nBased on your feedback, I can quickly send a patch.\n\np.s.: Apologies for the weird indenting, due to 70 character limit.\nconsider it just a prototype.\n\nThanks!\n"},{"id":"486286","messageId":"CAP8UFD1wMJMY6G4SaPTPwq6b9HbeXG1kB97-RRrL-KGN1wE0rg@mail.gmail.com","threadId":"60679","inReplyTo":"85d4e83c-b6c4-4308-ac8c-a65c911c8a95@gmail.com","subject":"Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-04T10:24:01Z","receivedAt":"2024-01-04T10:24:15Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jan 2, 2024 at 11:17 PM Ghanshyam Thakkar\n<shyamthakkar001@gmail.com> wrote:\n>\n> Hello,\n>\n> I'm currently an undergrad beginning my journey of contributing to the\n> Git project. I am seeking feedback on doing \"Heed core.bare from\n> template config file when no command line override given\" described\n> here\n> https://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/\n> by Elijah Newren, as a microproject. I would like to know from the\n> community, if the complexity and scope of the project is appropriate\n> for a microproject.\n\nThanks for your interest in the next GSoC!\n\nMy opinion is that it's too complex for a micro-project. Now maybe if\nElijah or others are willing to help you on it, perhaps it will work\nout. I think it's safer to look at simpler micro-projects though.\n\n> e.g. in builtin/init-db.c :\n>\n> static int template_bare_config(const char *var, const char *value,\n>                      const struct config_context *ctx, void *cb)\n> {\n>        if(!strcmp(var,\"core.bare\")) {\n\nWe like to have a space character between \"if\" and \"(\" as well as after a \",\"\n\n>              is_bare_repository_cfg = git_config_bool(var, value);\n>        }\n>        return 0;\n> }\n>\n> int cmd_init_db(int argc, const char **argv, const char *prefix)\n> {\n> ...\n> ...\n>        if(is_bare_repository_cfg==-1) {\n\nWe like to have a space character both before and after \"==\" as well\nas between \"if\" and \"(\".\n\n>              if(!template_dir)\n>                    git_config_get_pathname(\"init.templateDir\",\n>                                            &template_dir);\n>\n>              if(template_dir) {\n>                    const char* template_config_path\n>                                 = xstrfmt(\"%s/config\",\n>                    struct stat st;\n>\n>                    if(!stat(template_config_path, &st) &&\n>                      !S_ISDIR(st.st_mode)) {\n>                          git_config_from_file(template_bare_cfg,\n>                                         template_config_path, NULL);\n>                    }\n>              }\n> ...\n> ...\n>        return init_db(git_dir, real_git_dir, template_dir, hash_algo,\n>                       initial_branch, init_shared_repository, flags);\n> }\n>\n> I also wanted to know if the global config files should have an effect\n> in deciding if the repo is bare or not.\n>\n> Curious to know your thoughts on, if this is the right approach or\n> does it require doing refactoring to bring all the logic in setup.c.\n> Based on your feedback, I can quickly send a patch.\n\nI don't know this area of the code well, so I don't think I can help\nyou much on this.\n\nBest,\nChristian.\n"},{"id":"486287","messageId":"CY5UVPHIH8VQ.2I0V9GX4W1A30@gmail.com","threadId":"60679","inReplyTo":"CAP8UFD1wMJMY6G4SaPTPwq6b9HbeXG1kB97-RRrL-KGN1wE0rg@mail.gmail.com","subject":"Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-04T10:39:10Z","receivedAt":"2024-01-04T10:39:15Z","isPatch":false,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Thu Jan 4, 2024 at 3:54 PM IST, Christian Couder wrote:\n> On Tue, Jan 2, 2024 at 11:17 PM Ghanshyam Thakkar\n> <shyamthakkar001@gmail.com> wrote:\n> >\n> > Hello,\n> >\n> > I'm currently an undergrad beginning my journey of contributing to the\n> > Git project. I am seeking feedback on doing \"Heed core.bare from\n> > template config file when no command line override given\" described\n> > here\n> > https://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/\n> > by Elijah Newren, as a microproject. I would like to know from the\n> > community, if the complexity and scope of the project is appropriate\n> > for a microproject.\n>\n> Thanks for your interest in the next GSoC!\n>\n> My opinion is that it's too complex for a micro-project. Now maybe if\n> Elijah or others are willing to help you on it, perhaps it will work\n> out. I think it's safer to look at simpler micro-projects though.\n>\nThank you for your feedback. I will wait for Elijah's response on this and \nperhaps look at other microprojects in the meantime. Although, I think I will \nbe able to do this with others help.\n\n> > e.g. in builtin/init-db.c :\n> >\n> > static int template_bare_config(const char *var, const char *value,\n> >                      const struct config_context *ctx, void *cb)\n> > {\n> >        if(!strcmp(var,\"core.bare\")) {\n>\n> We like to have a space character between \"if\" and \"(\" as well as after a \",\"\n>\n> >              is_bare_repository_cfg = git_config_bool(var, value);\n> >        }\n> >        return 0;\n> > }\n> >\n> > int cmd_init_db(int argc, const char **argv, const char *prefix)\n> > {\n> > ...\n> > ...\n> >        if(is_bare_repository_cfg==-1) {\n>\n> We like to have a space character both before and after \"==\" as well\n> as between \"if\" and \"(\".\n>\n> >              if(!template_dir)\n> >                    git_config_get_pathname(\"init.templateDir\",\n> >                                            &template_dir);\n> >\n> >              if(template_dir) {\n> >                    const char* template_config_path\n> >                                 = xstrfmt(\"%s/config\",\n> >                    struct stat st;\n> >\n> >                    if(!stat(template_config_path, &st) &&\n> >                      !S_ISDIR(st.st_mode)) {\n> >                          git_config_from_file(template_bare_cfg,\n> >                                         template_config_path, NULL);\n> >                    }\n> >              }\n> > ...\n> > ...\n> >        return init_db(git_dir, real_git_dir, template_dir, hash_algo,\n> >                       initial_branch, init_shared_repository, flags);\n> > }\n> >\n> > I also wanted to know if the global config files should have an effect\n> > in deciding if the repo is bare or not.\n> >\n> > Curious to know your thoughts on, if this is the right approach or\n> > does it require doing refactoring to bring all the logic in setup.c.\n> > Based on your feedback, I can quickly send a patch.\n>\n> I don't know this area of the code well, so I don't think I can help\n> you much on this.\n\nAppreciate your suggestions above. Will keep them in mind while sending the\npatch.\n\n> Best,\n> Christian.\n\nThanks,\nGhanshyam\n"},{"id":"486333","messageId":"CABPp-BH+cPdfsctquE60tw_nD6_LCaWf0JwGusuZ0tvQQuWy4w@mail.gmail.com","threadId":"60679","inReplyTo":"CAP8UFD1wMJMY6G4SaPTPwq6b9HbeXG1kB97-RRrL-KGN1wE0rg@mail.gmail.com","subject":"Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-01-05T02:11:00Z","receivedAt":"2024-01-05T02:12:28Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jan 4, 2024 at 2:24 AM Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n> On Tue, Jan 2, 2024 at 11:17 PM Ghanshyam Thakkar\n> <shyamthakkar001@gmail.com> wrote:\n> >\n> > Hello,\n> >\n> > I'm currently an undergrad beginning my journey of contributing to the\n> > Git project. I am seeking feedback on doing \"Heed core.bare from\n> > template config file when no command line override given\" described\n> > here\n> > https://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/\n> > by Elijah Newren, as a microproject. I would like to know from the\n> > community, if the complexity and scope of the project is appropriate\n> > for a microproject.\n>\n> Thanks for your interest in the next GSoC!\n>\n> My opinion is that it's too complex for a micro-project. Now maybe if\n> Elijah or others are willing to help you on it, perhaps it will work\n> out. I think it's safer to look at simpler micro-projects though.\n\nAn important part of solving this problem, if you were to do so, would\nbe adding several testcases (including some showing how it currently\nfails).  Simply adding some or all of those testcases would be a good\nmicro-project.  (If you take this on, it'd probably be worthwhile to\ninclude a reference to any such added tests in the TODO comment here\nso that future folks noticing the TODO are aware of them).  Then, if\nadding those testcases goes well and you feel ambitious, you can try\nto tackle the underlying problem too.\n\n> > e.g. in builtin/init-db.c :\n> >\n> > static int template_bare_config(const char *var, const char *value,\n> >                      const struct config_context *ctx, void *cb)\n> > {\n> >        if(!strcmp(var,\"core.bare\")) {\n>\n> We like to have a space character between \"if\" and \"(\" as well as after a \",\"\n>\n> >              is_bare_repository_cfg = git_config_bool(var, value);\n> >        }\n> >        return 0;\n> > }\n> >\n> > int cmd_init_db(int argc, const char **argv, const char *prefix)\n> > {\n> > ...\n> > ...\n> >        if(is_bare_repository_cfg==-1) {\n>\n> We like to have a space character both before and after \"==\" as well\n> as between \"if\" and \"(\".\n>\n> >              if(!template_dir)\n> >                    git_config_get_pathname(\"init.templateDir\",\n> >                                            &template_dir);\n> >\n> >              if(template_dir) {\n> >                    const char* template_config_path\n> >                                 = xstrfmt(\"%s/config\",\n> >                    struct stat st;\n> >\n> >                    if(!stat(template_config_path, &st) &&\n> >                      !S_ISDIR(st.st_mode)) {\n> >                          git_config_from_file(template_bare_cfg,\n> >                                         template_config_path, NULL);\n> >                    }\n> >              }\n> > ...\n> > ...\n> >        return init_db(git_dir, real_git_dir, template_dir, hash_algo,\n> >                       initial_branch, init_shared_repository, flags);\n> > }\n> >\n> > I also wanted to know if the global config files should have an effect\n> > in deciding if the repo is bare or not.\n> >\n> > Curious to know your thoughts on, if this is the right approach or\n> > does it require doing refactoring to bring all the logic in setup.c.\n> > Based on your feedback, I can quickly send a patch.\n>\n> I don't know this area of the code well, so I don't think I can help\n> you much on this.\n\nIf you look back at the mailing list discussion on the series that\nintroduced this TODO comment you are trying to address, you'll note\nthat both Glen and I dug into the code and attempted to explain it to\neach other, and we both got it wrong on our first try.   If I knew the\ncorrect solution without digging, I probably would have just\nimplemented it and sent it in as another patch series.  My TODO was\nnot meant to be a definitive guide about where to make the fixes\n(because I don't know yet), but just a helpful guide that the first\nspot you'd expect cannot be the correct location (I already tried a\nfew things there) and that you need to dig further.  Anyway, I'm sure\nthe correct place to fix can be figured out with a bit of work, but in\nthis case, figuring out where the changes need to be made is probably\nthe majority of the effort.\n\nYou may well need to just pick an area, start modifying, go through it\nin a debugger and with your testcases, etc., and learn whether\nmodifications in that area even can solve the problem.  You may have\nto discard your first or even second attempts, but take what you\nlearned to guide you on future attempts.\n\nAnd if you do get it all working, this is a case where it'd likely be\nimportant to comment in the commit message not just why you are making\nchanges, but why you believe the area you modified is the correct one\n(i.e. to mention why the more obvious places to modify don't actually\nsolve the problem).  And then be prepared for folks on the mailing\nlist to use the information you provided in the commit message to dive\nin and point out some other way to fix the problem that is even better\nbut requires you to redo it again.  (And I'm not just saying that\nbecause you're new; I would not be surprised if the same happened to\nme.  Others are more familiar with various parts of the general setup\nand config code than me, and sometimes additional expertise coupled\nwith a solution of some sort can make it easier to identify an even\nbetter solution.)\n"},{"id":"486341","messageId":"xmqqjzonpy9l.fsf@gitster.g","threadId":"60679","inReplyTo":"CABPp-BH+cPdfsctquE60tw_nD6_LCaWf0JwGusuZ0tvQQuWy4w@mail.gmail.com","subject":"Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-05T15:59:02Z","receivedAt":"2024-01-05T15:59:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> If you look back at the mailing list discussion on the series that\n> introduced this TODO comment you are trying to address, you'll note\n> that both Glen and I dug into the code and attempted to explain it to\n> each other, and we both got it wrong on our first try.\n\nI think you meant 0f7443bd (init-db: document existing bug with\ncore.bare in template config, 2023-05-16), where it says:\n\n    The comments in create_default_files() talks about reading config from\n    the config file in the specified `--templates` directory, which leads to\n    the question of whether core.bare could be set in such a config file and\n    thus whether the code is doing the right thing.\n\nBut I suspect the all of the above comes from a misunderstanding.\nThe comment the above commit log message talks about is:\n\n /*\n  * First copy the templates -- we might have the default\n  * config file there, in which case we would want to read\n  * from it after installing.\n  *\n  * Before reading that config, we also need to clear out any cached\n  * values (since we've just potentially changed what's available on\n  * disk).\n  */\n\nThis primarily comes from my 4f629539 (init-db: check template and\nrepository format., 2005-11-25), whose focus was to control the way\nHEAD symref is created, but care was taken to avoid propagating\nvalues from the configuration variables in the template that do not\nmake sense for the repository being initialized.  The most important\nthing being the repository format version, but the intent to avoid\nnonsense combination between the characteristic the new repository\nhas and the configuration values copied from the template was not\nlimited to the format version.\n\nSpecifically, the commit that introduced the comment never wanted to\nhonor core.bare in the template.  I do not think I has core.bare in\nmind when I wrote the comment, but I would have described it as the\nsame category as the repository format version, i.e. something you\nwould not want to copy, if I were pressed to clarify back then.\n\nBesides, create_default_files() is way too late, even if we wanted\nto create a bare repository when the template config file says\ncore.bare = true, as the caller would already have created before\npassing $DIR (when \"git --bare init $DIR\" was run) or $DIR/.git\n(when \"git init $DIR\" was run) to the function.\n\nIf somebody wants to always create a bare repository by having\ncore.bare=true in their template and if we wanted to honor it (which\nI am dubious of the value of, by the way), I would think the right\nplace to do so would be way before create_default_files() is called.\nWhen running \"git init [$DIR]\", long before calling init_db(), we\ndecide if we are about to create a bare repository and either create\n$DIR or $DIR/.git.  What is in the template, if we really wanted to\ndo so, should be read before that happens, no?\n"},{"id":"486362","messageId":"CY7M09XT547N.2OOTI5APX9RIX@gmail.com","threadId":"60679","inReplyTo":"xmqqjzonpy9l.fsf@gitster.g","subject":"Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-06T12:07:18Z","receivedAt":"2024-01-06T12:07:23Z","isPatch":false,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Fri Jan 5, 2024 at 9:29 PM IST, Junio C Hamano wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > If you look back at the mailing list discussion on the series that\n> > introduced this TODO comment you are trying to address, you'll note\n> > that both Glen and I dug into the code and attempted to explain it to\n> > each other, and we both got it wrong on our first try.\n>\n> I think you meant 0f7443bd (init-db: document existing bug with\n> core.bare in template config, 2023-05-16), where it says:\n>\n>     The comments in create_default_files() talks about reading config from\n>     the config file in the specified `--templates` directory, which leads to\n>     the question of whether core.bare could be set in such a config file and\n>     thus whether the code is doing the right thing.\n>\n> But I suspect the all of the above comes from a misunderstanding.\n> The comment the above commit log message talks about is:\n>\n>  /*\n>   * First copy the templates -- we might have the default\n>   * config file there, in which case we would want to read\n>   * from it after installing.\n>   *\n>   * Before reading that config, we also need to clear out any cached\n>   * values (since we've just potentially changed what's available on\n>   * disk).\n>   */\n>\n> This primarily comes from my 4f629539 (init-db: check template and\n> repository format., 2005-11-25), whose focus was to control the way\n> HEAD symref is created, but care was taken to avoid propagating\n> values from the configuration variables in the template that do not\n> make sense for the repository being initialized.  The most important\n> thing being the repository format version, but the intent to avoid\n> nonsense combination between the characteristic the new repository\n> has and the configuration values copied from the template was not\n> limited to the format version.\n>\n> Specifically, the commit that introduced the comment never wanted to\n> honor core.bare in the template.  I do not think I has core.bare in\n> mind when I wrote the comment, but I would have described it as the\n> same category as the repository format version, i.e. something you\n> would not want to copy, if I were pressed to clarify back then.\n\nThen I suppose this warrants updating the TODO comment in\ncreate_default_files(), which currently can be interpreted as this \nbeing a unwanted behavior. And also amending the testcases which\ncurrently display this as knwon breakage.\n\n> Besides, create_default_files() is way too late, even if we wanted\n> to create a bare repository when the template config file says\n> core.bare = true, as the caller would already have created before\n> passing $DIR (when \"git --bare init $DIR\" was run) or $DIR/.git\n> (when \"git init $DIR\" was run) to the function.\n>\n> If somebody wants to always create a bare repository by having\n> core.bare=true in their template and if we wanted to honor it (which\n> I am dubious of the value of, by the way), I would think the right\n> place to do so would be way before create_default_files() is called.\n> When running \"git init [$DIR]\", long before calling init_db(), we\n> decide if we are about to create a bare repository and either create\n> $DIR or $DIR/.git.  What is in the template, if we really wanted to\n> do so, should be read before that happens, no?\n\nThat is what I proposed in my original email, after which I had a\nworking solution which passed all the tests. That solution was indeed to\ncheck for core.bare in the template before we set GIT_DIR_ENVIRONMENT, \nwhich subsequently creates either $DIR or $DIR/.git as you described \nabove. \n\nRegardless, I can send the patch with updated comments to clarify\nthat ignoring core.bare from template files is the intended behavior and \namend the test_expect_failure testcases, with Elijah's consensus.\n\nThanks.\n"},{"id":"486398","messageId":"xmqqo7dvloiu.fsf@gitster.g","threadId":"60679","inReplyTo":"CY7M09XT547N.2OOTI5APX9RIX@gmail.com","subject":"Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-08T17:32:09Z","receivedAt":"2024-01-08T17:32:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ghanshyam Thakkar\" <shyamthakkar001@gmail.com> writes:\n\n>> Specifically, the commit that introduced the comment never wanted to\n>> honor core.bare in the template.  I do not think I has core.bare in\n>> mind when I wrote the comment, but I would have described it as the\n>> same category as the repository format version, i.e. something you\n>> would not want to copy, if I were pressed to clarify back then.\n>\n> Then I suppose this warrants updating the TODO comment in\n> create_default_files(), which currently can be interpreted as this \n> being a unwanted behavior. And also amending the testcases which\n> currently display this as knwon breakage.\n\nI obviously agree with that, after saying that I suspect 0f7443bd\ncomes from a misunderstanding ;-).\n\n>> If somebody wants to always create a bare repository by having\n>> core.bare=true in their template and if we wanted to honor it (which\n>> I am dubious of the value of, by the way), I would think the right\n>> place to do so would be way before create_default_files() is called.\n>> When running \"git init [$DIR]\", long before calling init_db(), we\n>> decide if we are about to create a bare repository and either create\n>> $DIR or $DIR/.git.  What is in the template, if we really wanted to\n>> do so, should be read before that happens, no?\n>\n> That is what I proposed in my original email, after which I had a\n> working solution which passed all the tests. That solution was indeed to\n> check for core.bare in the template before we set GIT_DIR_ENVIRONMENT, \n> which subsequently creates either $DIR or $DIR/.git as you described \n> above. \n\nYeah, if this were still in soon after 4f629539 was written, then\nsuch a change might have been a useful feature enhancement, but risk\nof breaking people (third-party tools) who use the same template to\ninitialize both bare and non-bare repositories is there, so...\n\nThanks.\n"},{"id":"487030","messageId":"CABPp-BFSTpe=wT6JM1CCYJCkYptTB_OZSpqOa0Syq3puxNxEPA@mail.gmail.com","threadId":"60679","inReplyTo":"xmqqo7dvloiu.fsf@gitster.g","subject":"Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-01-19T01:43:16Z","receivedAt":"2024-01-19T01:43:30Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jan 8, 2024 at 9:32 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Ghanshyam Thakkar\" <shyamthakkar001@gmail.com> writes:\n>\n> >> Specifically, the commit that introduced the comment never wanted to\n> >> honor core.bare in the template.  I do not think I has core.bare in\n> >> mind when I wrote the comment, but I would have described it as the\n> >> same category as the repository format version, i.e. something you\n> >> would not want to copy, if I were pressed to clarify back then.\n> >\n> > Then I suppose this warrants updating the TODO comment in\n> > create_default_files(), which currently can be interpreted as this\n> > being a unwanted behavior. And also amending the testcases which\n> > currently display this as knwon breakage.\n>\n> I obviously agree with that, after saying that I suspect 0f7443bd\n> comes from a misunderstanding ;-).\n\nSounds fine to me.  I have no particular interest in supporting\ncore.bare from the template; it's just that in order to do other\ncleanup, I needed to remove the init_is_bare_repository global\nvariable (see c2f76965d02 (\"init-db: remove unnecessary global\nvariable\", 2023-05-16)).  Attempting to remove that global variable\nmade it _look_ like I was changing the code behavior and breaking it\nin the case when core.bare was true in the template.  I knew possible\ncode breakage was what code reviewers would ask about.  And my best\nreading of the fact that the variable existed plus how the code was\nwritten suggested to me that indeed someone else thought this might be\nimportant to support.  So, I left the TODO behind to document that I\nwasn't breaking the code with my changes (or even changing behavior at\nall), and left some hints for the next reader who came along about\nwhere they might start looking if they thought it was important to\nfix.\n"},{"id":"489662","messageId":"20240229134114.285393-2-shyamthakkar001@gmail.com","threadId":"60679","inReplyTo":"85d4e83c-b6c4-4308-ac8c-a65c911c8a95@gmail.com","subject":"[PATCH] setup: clarify TODO comment about ignoring core.bare","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-29T13:41:15Z","receivedAt":"2024-02-29T13:43:40Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The comment suggested to heed core.bare from template config if no\ncommand line override given. However, it was clarified by Junio that\nsuch values are ignored by intention and does not make sense to\npropagate it from template config to the repository config[1].\n\nTherefore, remove the TODO tag and update the comment to add\nadditional context if such functionality is desired in the future.\nAlso update the relevant test cases to not expect failure and remove\none redundant testcase.\n\n[1]: https://lore.kernel.org/git/xmqqjzonpy9l.fsf@gitster.g/\n\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n setup.c                  | 16 ++++++++++------\n t/t1301-shared-repo.sh   | 15 ++-------------\n t/t5606-clone-options.sh |  6 +++---\n 3 files changed, 15 insertions(+), 22 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex b69b1cbc2a..92e0c3a121 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1997,16 +1997,20 @@ static int create_default_files(const char *template_path,\n \tif (init_shared_repository != -1)\n \t\tset_shared_repository(init_shared_repository);\n \t/*\n-\t * TODO: heed core.bare from config file in templates if no\n-\t *       command-line override given\n+\t * Note: The below line simply checks the presence of worktree (the\n+\t * simplification of which is given after the line) and core.bare from\n+\t * config file is not taken into account when deciding if the worktree\n+\t * should be created or not, even if no command line override given.\n+\t * That is intentional. Therefore, if in future we want to heed\n+\t * core.bare from config file, we should do it before we create any\n+\t * subsequent directories for worktree or repo because until this point\n+\t * they should already be created.\n \t */\n \tis_bare_repository_cfg = prev_bare_repository || !work_tree;\n-\t/* TODO (continued):\n+\t/* Note (continued):\n \t *\n-\t * Unfortunately, the line above is equivalent to\n+\t * The line above is equivalent to\n \t *    is_bare_repository_cfg = !work_tree;\n-\t * which ignores the config entirely even if no `--[no-]bare`\n-\t * command line option was present.\n \t *\n \t * To see why, note that before this function, there was this call:\n \t *    prev_bare_repository = is_bare_repository()\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex b1eb5c01b8..29cf8a9661 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -52,7 +52,7 @@ test_expect_success 'shared=all' '\n \ttest 2 = $(git config core.sharedrepository)\n '\n \n-test_expect_failure 'template can set core.bare' '\n+test_expect_success 'template cannot set core.bare' '\n \ttest_when_finished \"rm -rf subdir\" &&\n \ttest_when_finished \"rm -rf templates\" &&\n \ttest_config core.bare true &&\n@@ -60,18 +60,7 @@ test_expect_failure 'template can set core.bare' '\n \tmkdir -p templates/ &&\n \tcp .git/config templates/config &&\n \tgit init --template=templates subdir &&\n-\ttest_path_exists subdir/HEAD\n-'\n-\n-test_expect_success 'template can set core.bare but overridden by command line' '\n-\ttest_when_finished \"rm -rf subdir\" &&\n-\ttest_when_finished \"rm -rf templates\" &&\n-\ttest_config core.bare true &&\n-\tumask 0022 &&\n-\tmkdir -p templates/ &&\n-\tcp .git/config templates/config &&\n-\tgit init --no-bare --template=templates subdir &&\n-\ttest_path_exists subdir/.git/HEAD\n+\ttest_path_is_missing subdir/HEAD\n '\n \n test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository' '\ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex a400bcca62..e93e0d0cc3 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -120,14 +120,14 @@ test_expect_success 'prefers -c config over --template config' '\n \n '\n \n-test_expect_failure 'prefers --template config even for core.bare' '\n+test_expect_success 'ignore --template config for core.bare' '\n \n \ttemplate=\"$TRASH_DIRECTORY/template-with-bare-config\" &&\n \tmkdir \"$template\" &&\n \tgit config --file \"$template/config\" core.bare true &&\n \tgit clone \"--template=$template\" parent clone-bare-config &&\n-\ttest \"$(git -C clone-bare-config config --local core.bare)\" = \"true\" &&\n-\ttest_path_is_file clone-bare-config/HEAD\n+\ttest \"$(git -C clone-bare-config config --local core.bare)\" = \"false\" &&\n+\ttest_path_is_missing clone-bare-config/HEAD\n '\n \n test_expect_success 'prefers config \"clone.defaultRemoteName\" over default' '\n-- \n2.44.0\n\n"},{"id":"489680","messageId":"xmqqsf1bf5ew.fsf@gitster.g","threadId":"60679","inReplyTo":"20240229134114.285393-2-shyamthakkar001@gmail.com","subject":"Re: [PATCH] setup: clarify TODO comment about ignoring core.bare","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-29T19:15:35Z","receivedAt":"2024-02-29T19:15:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n>  \t/*\n> -\t * TODO: heed core.bare from config file in templates if no\n> -\t *       command-line override given\n> +\t * Note: The below line simply checks the presence of worktree (the\n> +\t * simplification of which is given after the line) and core.bare from\n> +\t * config file is not taken into account when deciding if the worktree\n> +\t * should be created or not, even if no command line override given.\n> +\t * That is intentional. Therefore, if in future we want to heed\n> +\t * core.bare from config file, we should do it before we create any\n> +\t * subsequent directories for worktree or repo because until this point\n> +\t * they should already be created.\n>  \t */\n>  \tis_bare_repository_cfg = prev_bare_repository || !work_tree;\n\nI do not recall the discussion; others may want to discuss if the\nchange above is desirable, before I come back to the topic later.\n\nBut I see this long comment totally unnecessary and distracting.\n\n> -\t/* TODO (continued):\n> +\t/* Note (continued):\n>  \t *\n> -\t * Unfortunately, the line above is equivalent to\n> +\t * The line above is equivalent to\n>  \t *    is_bare_repository_cfg = !work_tree;\n> -\t * which ignores the config entirely even if no `--[no-]bare`\n> -\t * command line option was present.\n>  \t *\n>  \t * To see why, note that before this function, there was this call:\n>  \t *    prev_bare_repository = is_bare_repository()\n\nIf it can be proven that the assignment can be simplified to lose\nthe \"prev_bare_repository ||\" part, then the above comment can be\nused as part of the proposed log message for a commit that makes\nsuch a change.  There is no reason to leave such a long comment to\nleave the more complex \"A || B\" expression when it can be simplified\nto \"B\", no?\n\nThanks.\n"},{"id":"489690","messageId":"CZHV4DHG59B1.2KMP4AJW9MHJE@gmail.com","threadId":"60679","inReplyTo":"xmqqsf1bf5ew.fsf@gitster.g","subject":"Re: [PATCH] setup: clarify TODO comment about ignoring core.bare","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-29T20:58:27Z","receivedAt":"2024-02-29T20:58:33Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Fri Mar 1, 2024 at 12:45 AM IST, Junio C Hamano wrote:\n> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n>\n> >  \t/*\n> > -\t * TODO: heed core.bare from config file in templates if no\n> > -\t *       command-line override given\n> > +\t * Note: The below line simply checks the presence of worktree (the\n> > +\t * simplification of which is given after the line) and core.bare from\n> > +\t * config file is not taken into account when deciding if the worktree\n> > +\t * should be created or not, even if no command line override given.\n> > +\t * That is intentional. Therefore, if in future we want to heed\n> > +\t * core.bare from config file, we should do it before we create any\n> > +\t * subsequent directories for worktree or repo because until this point\n> > +\t * they should already be created.\n> >  \t */\n> >  \tis_bare_repository_cfg = prev_bare_repository || !work_tree;\n>\n> I do not recall the discussion; others may want to discuss if the\n> change above is desirable, before I come back to the topic later.\n>\n> But I see this long comment totally unnecessary and distracting.\n>\n> > -\t/* TODO (continued):\n> > +\t/* Note (continued):\n> >  \t *\n> > -\t * Unfortunately, the line above is equivalent to\n> > +\t * The line above is equivalent to\n> >  \t *    is_bare_repository_cfg = !work_tree;\n> > -\t * which ignores the config entirely even if no `--[no-]bare`\n> > -\t * command line option was present.\n> >  \t *\n> >  \t * To see why, note that before this function, there was this call:\n> >  \t *    prev_bare_repository = is_bare_repository()\n>\n> If it can be proven that the assignment can be simplified to lose\n> the \"prev_bare_repository ||\" part, then the above comment can be\n> used as part of the proposed log message for a commit that makes\n> such a change.  There is no reason to leave such a long comment to\n> leave the more complex \"A || B\" expression when it can be simplified\n> to \"B\", no?\n\nI agree. In fact, we can remove the prev_bare_repository variable altogether\nas it was used solely for the \"A || B\" expression. Initially this\nfunction used to be in builtin/init-db.c and shared with\nbuiltin/clone.c. In moving to setup.c, an unnecessary global variable\nequivalent to prev_bare_repository was removed and therefore\nprev_bare_repository was intruduced to not hinder the future possibility\nof intruducing the (core.bare from config) feature which might have been\nthe global variables partial intent[1]. Therefore, I kept the original\nexpression.\n\nHowever, in the same series that this was introduced, it was acknowledged\nby Elijah[2] that create_default_files() would possibly be too late to heed\ncore.bare. And indeed it is, as the directories for the worktree or repo\nare already created by this point. Therefore, prev_bare_repository does\nnot seem to have any usecase with/without supporting core.bare from\nconfig.\n\n[1]:\nhttps://lore.kernel.org/git/xmqqsf1bf5ew.fsf@gitster.g/T/#m64125dd80d04ae177944434e7092522325b374c9\n[2]: 0f7443bdc7 (init-db: document existing bug with core.bare in\ntemplate config, 2023-05-16)\n\nThanks.\n"},{"id":"489892","messageId":"20240304151811.511780-1-shyamthakkar001@gmail.com","threadId":"60679","inReplyTo":"20240229134114.285393-2-shyamthakkar001@gmail.com","subject":"[PATCH v2] setup: remove unnecessary variable","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-04T15:18:11Z","receivedAt":"2024-03-04T15:19:12Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The TODO comment suggested to heed core.bare from template config file\nif no command line override given. And the prev_bare_repository\nvariable seems to have been placed for this sole purpose as it is not\nused anywhere else.\n\nHowever, it was clarified by Junio [1] that such values (including\ncore.bare) are ignored intentionally and does not make sense to\npropagate them from template config to repository config. Also, the\ndirectories for the worktree and repository are already created, and\ntherefore the bare/non-bare decision has already been made, by the\npoint we reach the codepath where the TODO comment is placed.\nTherefore, prev_bare_repository does not have a usecase with/without\nsupporting core.bare from template. And the removal of\nprev_bare_repository is safe as proved by the later part of the\ncomment:\n\n    \"Unfortunately, the line above is equivalent to\n        is_bare_repository_cfg = !work_tree;\n    which ignores the config entirely even if no `--[no-]bare`\n    command line option was present.\n\n    To see why, note that before this function, there was this call:\n        prev_bare_repository = is_bare_repository()\n    expanding the right hand side:\n        = is_bare_repository_cfg && !get_git_work_tree()\n        = is_bare_repository_cfg && !work_tree\n    note that the last simplification above is valid because nothing\n    calls repo_init() or set_git_work_tree() between any of the\n    relevant calls in the code, and thus the !get_git_work_tree()\n    calls will return the same result each time.  So, what we are\n    interested in computing is the right hand side of the line of\n    code just above this comment:\n        prev_bare_repository || !work_tree\n        = is_bare_repository_cfg && !work_tree || !work_tree\n        = !work_tree\n    because \"A && !B || !B == !B\" for all boolean values of A & B.\"\n\nTherefore, remove the TODO comment and remove prev_bare_repository\nvariable. Also, update relevant testcases and remove one redundant\ntestcase.\n\n[1]: https://lore.kernel.org/git/xmqqjzonpy9l.fsf@gitster.g/\n\nHelped-by: Elijah Newren <newren@gmail.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n setup.c                  | 36 +++---------------------------------\n t/t1301-shared-repo.sh   | 15 ++-------------\n t/t5606-clone-options.sh |  6 +++---\n 3 files changed, 8 insertions(+), 49 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex b69b1cbc2a..81accd1213 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1961,7 +1961,6 @@ void create_reference_database(unsigned int ref_storage_format,\n static int create_default_files(const char *template_path,\n \t\t\t\tconst char *original_git_dir,\n \t\t\t\tconst struct repository_format *fmt,\n-\t\t\t\tint prev_bare_repository,\n \t\t\t\tint init_shared_repository)\n {\n \tstruct stat st1;\n@@ -1996,34 +1995,8 @@ static int create_default_files(const char *template_path,\n \t */\n \tif (init_shared_repository != -1)\n \t\tset_shared_repository(init_shared_repository);\n-\t/*\n-\t * TODO: heed core.bare from config file in templates if no\n-\t *       command-line override given\n-\t */\n-\tis_bare_repository_cfg = prev_bare_repository || !work_tree;\n-\t/* TODO (continued):\n-\t *\n-\t * Unfortunately, the line above is equivalent to\n-\t *    is_bare_repository_cfg = !work_tree;\n-\t * which ignores the config entirely even if no `--[no-]bare`\n-\t * command line option was present.\n-\t *\n-\t * To see why, note that before this function, there was this call:\n-\t *    prev_bare_repository = is_bare_repository()\n-\t * expanding the right hand side:\n-\t *                 = is_bare_repository_cfg && !get_git_work_tree()\n-\t *                 = is_bare_repository_cfg && !work_tree\n-\t * note that the last simplification above is valid because nothing\n-\t * calls repo_init() or set_git_work_tree() between any of the\n-\t * relevant calls in the code, and thus the !get_git_work_tree()\n-\t * calls will return the same result each time.  So, what we are\n-\t * interested in computing is the right hand side of the line of\n-\t * code just above this comment:\n-\t *     prev_bare_repository || !work_tree\n-\t *        = is_bare_repository_cfg && !work_tree || !work_tree\n-\t *        = !work_tree\n-\t * because \"A && !B || !B == !B\" for all boolean values of A & B.\n-\t */\n+\n+\tis_bare_repository_cfg = !work_tree;\n \n \t/*\n \t * We would have created the above under user's umask -- under\n@@ -2175,7 +2148,6 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \tint exist_ok = flags & INIT_DB_EXIST_OK;\n \tchar *original_git_dir = real_pathdup(git_dir, 1);\n \tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n-\tint prev_bare_repository;\n \n \tif (real_git_dir) {\n \t\tstruct stat st;\n@@ -2201,7 +2173,6 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \n \tsafe_create_dir(git_dir, 0);\n \n-\tprev_bare_repository = is_bare_repository();\n \n \t/* Check to see if the repository version is right.\n \t * Note that a newly created repository does not have\n@@ -2214,8 +2185,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \tvalidate_ref_storage_format(&repo_fmt, ref_storage_format);\n \n \treinit = create_default_files(template_dir, original_git_dir,\n-\t\t\t\t      &repo_fmt, prev_bare_repository,\n-\t\t\t\t      init_shared_repository);\n+\t\t\t\t      &repo_fmt, init_shared_repository);\n \n \t/*\n \t * Now that we have set up both the hash algorithm and the ref storage\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex b1eb5c01b8..29cf8a9661 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -52,7 +52,7 @@ test_expect_success 'shared=all' '\n \ttest 2 = $(git config core.sharedrepository)\n '\n \n-test_expect_failure 'template can set core.bare' '\n+test_expect_success 'template cannot set core.bare' '\n \ttest_when_finished \"rm -rf subdir\" &&\n \ttest_when_finished \"rm -rf templates\" &&\n \ttest_config core.bare true &&\n@@ -60,18 +60,7 @@ test_expect_failure 'template can set core.bare' '\n \tmkdir -p templates/ &&\n \tcp .git/config templates/config &&\n \tgit init --template=templates subdir &&\n-\ttest_path_exists subdir/HEAD\n-'\n-\n-test_expect_success 'template can set core.bare but overridden by command line' '\n-\ttest_when_finished \"rm -rf subdir\" &&\n-\ttest_when_finished \"rm -rf templates\" &&\n-\ttest_config core.bare true &&\n-\tumask 0022 &&\n-\tmkdir -p templates/ &&\n-\tcp .git/config templates/config &&\n-\tgit init --no-bare --template=templates subdir &&\n-\ttest_path_exists subdir/.git/HEAD\n+\ttest_path_is_missing subdir/HEAD\n '\n \n test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository' '\ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex a400bcca62..e93e0d0cc3 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -120,14 +120,14 @@ test_expect_success 'prefers -c config over --template config' '\n \n '\n \n-test_expect_failure 'prefers --template config even for core.bare' '\n+test_expect_success 'ignore --template config for core.bare' '\n \n \ttemplate=\"$TRASH_DIRECTORY/template-with-bare-config\" &&\n \tmkdir \"$template\" &&\n \tgit config --file \"$template/config\" core.bare true &&\n \tgit clone \"--template=$template\" parent clone-bare-config &&\n-\ttest \"$(git -C clone-bare-config config --local core.bare)\" = \"true\" &&\n-\ttest_path_is_file clone-bare-config/HEAD\n+\ttest \"$(git -C clone-bare-config config --local core.bare)\" = \"false\" &&\n+\ttest_path_is_missing clone-bare-config/HEAD\n '\n \n test_expect_success 'prefers config \"clone.defaultRemoteName\" over default' '\n-- \n2.44.0\n\n"},{"id":"489908","messageId":"xmqqjzmhq2vb.fsf@gitster.g","threadId":"60679","inReplyTo":"20240304151811.511780-1-shyamthakkar001@gmail.com","subject":"Re: [PATCH v2] setup: remove unnecessary variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T18:16:24Z","receivedAt":"2024-03-04T18:16:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n> The TODO comment suggested to heed core.bare from template config file\n> if no command line override given. And the prev_bare_repository\n> variable seems to have been placed for this sole purpose as it is not\n> used anywhere else.\n\nOK.\n\n> However, it was clarified by Junio [1] that such values (including\n> core.bare) are ignored intentionally and does not make sense to\n> propagate them from template config to repository config. Also, the\n> directories for the worktree and repository are already created, and\n> therefore the bare/non-bare decision has already been made, by the\n> point we reach the codepath where the TODO comment is placed.\n\nCorrect.  Who said it is much less interesting than what was said,\nso I would have written the first part of the paragraph more like\n\n\tValues including core.bare from the template file are\n\tignored on purpose because they may not make sense for the\n\trepository being created [1].  Also, the directories for ...\n\nbut I'll let it pass.\n\n> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\n> index b1eb5c01b8..29cf8a9661 100755\n> --- a/t/t1301-shared-repo.sh\n> +++ b/t/t1301-shared-repo.sh\n> @@ -52,7 +52,7 @@ test_expect_success 'shared=all' '\n>  \ttest 2 = $(git config core.sharedrepository)\n>  '\n>  \n> -test_expect_failure 'template can set core.bare' '\n> +test_expect_success 'template cannot set core.bare' '\n>  \ttest_when_finished \"rm -rf subdir\" &&\n>  \ttest_when_finished \"rm -rf templates\" &&\n>  \ttest_config core.bare true &&\n> @@ -60,18 +60,7 @@ test_expect_failure 'template can set core.bare' '\n>  \tmkdir -p templates/ &&\n>  \tcp .git/config templates/config &&\n>  \tgit init --template=templates subdir &&\n> -\ttest_path_exists subdir/HEAD\n> +\ttest_path_is_missing subdir/HEAD\n>  '\n\nSo we used to say \"subdir should be created as a bare repository but\nwe fail to do so\", but now \"subdir should become a non-bare repository\nbecause 'git init' is run without the --bare option\".  OK.\n\n> -\n> -test_expect_success 'template can set core.bare but overridden by command line' '\n> -\ttest_when_finished \"rm -rf subdir\" &&\n> -\ttest_when_finished \"rm -rf templates\" &&\n> -\ttest_config core.bare true &&\n> -\tumask 0022 &&\n> -\tmkdir -p templates/ &&\n> -\tcp .git/config templates/config &&\n> -\tgit init --no-bare --template=templates subdir &&\n> -\ttest_path_exists subdir/.git/HEAD\n> -'\n\nThis removal is a bit unexpected.  Is it because we established with\nthe previous test that core.bare in the template should not affect\nthe outcome, so this is not worth testing?\n\n> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\n> index a400bcca62..e93e0d0cc3 100755\n> --- a/t/t5606-clone-options.sh\n> +++ b/t/t5606-clone-options.sh\n> @@ -120,14 +120,14 @@ test_expect_success 'prefers -c config over --template config' '\n>  \n>  '\n>  \n> -test_expect_failure 'prefers --template config even for core.bare' '\n> +test_expect_success 'ignore --template config for core.bare' '\n>  \n>  \ttemplate=\"$TRASH_DIRECTORY/template-with-bare-config\" &&\n>  \tmkdir \"$template\" &&\n>  \tgit config --file \"$template/config\" core.bare true &&\n>  \tgit clone \"--template=$template\" parent clone-bare-config &&\n> -\ttest \"$(git -C clone-bare-config config --local core.bare)\" = \"true\" &&\n> -\ttest_path_is_file clone-bare-config/HEAD\n> +\ttest \"$(git -C clone-bare-config config --local core.bare)\" = \"false\" &&\n> +\ttest_path_is_missing clone-bare-config/HEAD\n>  '\n\nThis is in the same spirit as the first change in t1301, which seems\nOK.\n\nThanks.\n"},{"id":"489918","messageId":"CZLA8TF4XG5S.KU06P62V03TV@gmail.com","threadId":"60679","inReplyTo":"xmqqjzmhq2vb.fsf@gitster.g","subject":"Re: [PATCH v2] setup: remove unnecessary variable","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-04T21:27:32Z","receivedAt":"2024-03-04T21:27:37Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Mon Mar 4, 2024 at 11:46 PM IST, Junio C Hamano wrote:\n> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> > -\n> > -test_expect_success 'template can set core.bare but overridden by command line' '\n> > -\ttest_when_finished \"rm -rf subdir\" &&\n> > -\ttest_when_finished \"rm -rf templates\" &&\n> > -\ttest_config core.bare true &&\n> > -\tumask 0022 &&\n> > -\tmkdir -p templates/ &&\n> > -\tcp .git/config templates/config &&\n> > -\tgit init --no-bare --template=templates subdir &&\n> > -\ttest_path_exists subdir/.git/HEAD\n> > -'\n>\n> This removal is a bit unexpected.  Is it because we established with\n> the previous test that core.bare in the template should not affect\n> the outcome, so this is not worth testing?\n\nYes, in the previous testcase we determined that template cannot set\ncore.bare. Therefore, this testcase would be like testing\n--bare/--no-bare option, which is already done in 0001-init.sh and\nt5601-clone.sh. However, I don't have strong opinion on this. I can add\nit back if you think it is worth it.\n\nThanks.\n"},{"id":"489920","messageId":"xmqq8r2xoe8g.fsf@gitster.g","threadId":"60679","inReplyTo":"CZLA8TF4XG5S.KU06P62V03TV@gmail.com","subject":"Re: [PATCH v2] setup: remove unnecessary variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T21:53:51Z","receivedAt":"2024-03-04T21:53:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ghanshyam Thakkar\" <shyamthakkar001@gmail.com> writes:\n\n> Yes, in the previous testcase we determined that template cannot set\n> core.bare. Therefore, this testcase would be like testing\n> --bare/--no-bare option, which is already done in 0001-init.sh and\n> t5601-clone.sh. However, I don't have strong opinion on this. I can add\n> it back if you think it is worth it.\n\nI was merely trying to make sure that I understood your motivation\nbehind the change, which was described at the end of the commit log\nmessage, i.e. \"... and remove one redundant testcase.\".\n\nThanks.\n"}]}