Can you provide some analysis of the problem and your solution? Right now you haven't really provided much, which means that the maintainers are going to have to try to reverse engineer the problem and your solution. Which is fine, but it's going to take us some time to sit down and think about the effects that this change will have.

For example: with this change, now git_mwindow_get_pack locks the mutex twice. This seems a bit odd. Why did you choose to go this route instead of having git_odb_backend_pack lock the mutex, directly or indirectly? (Perhaps moving the current init function to a private one, and then providing a public function that takes the lock that odb_backend_pack calls)?

The problem is that git_mwindow_files_init is not protected. So if the
first libgit operation in a process is two threads opening a
repository for the first time, then the initialization routine gets
called twice, and git__pack_cache can get written twice. The thread
that lost the race to write git__pack_cache will see the
git__pack_cache pointer change from under it. This would lead to
unexpected results, for example here

if (git_strmap_valid_index(git__pack_cache, pos)) {
        // what if git__pack_cache changes here?
    pack = git_strmap_value_at(git__pack_cache, pos);
    git_atomic_inc(&pack->refcount);

The initialization of git__pack_cache does not form a transaction wrt
to the rest git_mwindow_get_pack, so it does not need to run under the
same lock as the git_mwindow_get_pack(), so it seemed logical to not
do so.

I'm not too familiar with the setup of libgit2. I will note the
following:

• Uncontended mutex locks are very cheap.
• git__mwindow_mutex is mostly taken from mwindows.c; taking it in
  odb_pack seemed like an abstraction leak.
• the rest of libgit2 relies on running git_libgit2_init() for thread
  safety. Moving the initialization there seems more consistent with
  the stated policy, but that is a larger refactoring I'd rather leave
  to the maintainers

If you want to resolve this in another way, that is perfectly fine
with me, as long as it fixes the problem.

Thanks for the helpful writeup - this is a lovely analysis.

I agree with your final assessment, that this is very much eligible to simply be created within git_libgit2_init and we need not bother with it again and this would be my preference, if for no other reason than it follows the existing pattern around initialization and we can catch fatal errors earlier.

Can you please sanity check #3879 for me and make sure that it solves your problem appropriately?

Thanks!

Thanks for digging in to this - I hope you don't mind that I went with the more straightforward (I think) strategy of simply running this in git_libgit2_init.