Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > Glen Choo <chooglen@xxxxxxxxxx> writes: >> add_submodule_odb() is a hack - it adds a submodule's odb as an >> alternate, allowing the submodule's objects to be read via >> the_repository. Its last caller is submodule_has_commits(), which calls >> add_submodule_odb() to prepare for check_has_commit(). This used to be >> necessary because check_has_commit() used the_repository's odb, but this >> is longer true as of 13a2f620b2 (submodule: pass repo to >> check_has_commit(), 2021-10-08). >> >> Removing add_submodule_odb() reveals a bug in check_has_commit(), where >> check_has_commit() will segfault if the submodule is missing (e.g. the >> user has not init-ed the submodule). This happens because the >> submodule's struct repository cannot be initialized, but >> check_has_commit() tries to cleanup the uninitialized struct anyway. >> This was masked by add_submodule_odb(), because add_submodule_odb() >> fails when the submodule is missing, causing the caller to return early >> and avoid calling check_has_commit(). >> >> Fix the bug and remove the call to add_submodule_odb(). Since >> add_submodule_odb() has no more callers, remove it too. >> >> Note that submodule odbs can still by added as alternates via >> add_submodule_odb_by_path(). >> >> Signed-off-by: Glen Choo <chooglen@xxxxxxxxxx> >> --- >> This bug only exists because we can't call repo_clear() twice on the >> same struct repository. So instead of just fixing this site, an >> alternative (and maybe better) fix would be to fix repo_clear(). If >> others think that's a good idea, I'll do that instead. > > Reading the first paragraph of the commit message, I'm given the > impression that this is the last site in which we attempt to add a > submodule ODB as an alternate, but that is not true. This is indeed the > last usage of add_submodule_odb(), but add_submodule_odb_by_path() still > exists. > > I think the primary point of this commit is to fix a latent bug in > check_has_commit(), and add_submodule_odb()'s role here is just hiding > it. Its hacky behavior does not play a role. > > I would write the commit message like this: > > submodule: fix latent check_has_commit() bug > > check_has_commit() will attempt to clear a non-initialized struct > repository if initialization fails. This bug is masked by its only > caller, submodule_has_commits(), first calling add_submodule_odb(). > The latter fails if the repo does not exist, making > submodule_has_commits() exit early and not invoke check_has_commit() > in a situation in which initialization would fail. > > Fix this bug, and because calling add_submodule_odb() is no longer > necessary as of 13a2f620b2 (submodule: pass repo to > check_has_commit(), 2021-10-08), remove that call too. > > This is the last caller of add_submodule_odb(), so remove that > function. (Adding submodule ODBs as alternates is still present in the > form of add_submodule_odb_by_path().) Hm.. that is a good point, the commit message seems to promise more than what it actually delivers. I'll take your suggestion, thanks!
