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

[Elijah Newren <newren@gmail.com>]

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