Re: [PATCH v5 6/8] config: correctly read worktree configs in submodules

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Matheus Tavares Bernardino wrote:

> Sorry for the late reply, last week was quite busy.

No problem.  It's an unusual time for everyone.

[...]
> On Wed, Sep 2, 2020 at 5:15 PM Jonathan Nieder <jrnieder@gmail.com> wrote:

>> @@ -1824,27 +1824,33 @@ void read_early_config(config_fn_t cb, void *data)
>>         struct config_options opts = {0};
>>         struct strbuf commondir = STRBUF_INIT;
>>         struct strbuf gitdir = STRBUF_INIT;
>> +       struct repository the_early_repo = {0};
>>
>>         opts.respect_includes = 1;
>>
>>         if (have_git_dir()) {
>> -               opts.commondir = get_git_common_dir();
>> -               opts.git_dir = get_git_dir();
>> +               opts.repo = the_repository;
>
> I'm not very familiar with the code in setup.c so I apologize for the
> noob question: have_git_dir() returns `startup_info->have_repository
> || the_repository->gitdir`; so is it possible that it returns true but
> the_repository->gitdir is not initialized yet? If so, should we also
> check the_repository->gitdir here (before assigning opts.repo), and
> call BUG() when it is NULL, like get_git_dir() does?
>
> Hmm, nevertheless, I see that you already check `opts.repo &&
> opts.repo->gitdir` before trying to use it in
> do_git_config_sequence(). So it should already cover this case, right?

Right --- the main point is that a BUG() call represents "this can't
happen", or in other words, it's an assertion failure.  As a matter of
defensive coding functions like get_git_dir() guard against such cases
to make debugging a little easier and exploitation a little more
difficult when the impossible happens.

[...]
> Thanks a lot for this :) I was thinking of adding it as a preparatory
> patch before the fix itself. May I have your S-o-B as the author?

Sure!

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks,
Jonathan