Re: [PATCH 8/8] submodule: fix bug and remove add_submodule_odb()

From: Jonathan Tan <jonathantanmy@google.com>
Date: Feb 10, 2022, 23:04 UTC
Message-ID: <20220210230422.690368-1-jonathantanmy@google.com>
In-Reply-To: <20220210044152.78352-9-chooglen@google.com>

Glen Choo <chooglen@google.com> writes:

Show 29 quoted lines

> 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@google.com>
> ---
> 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().)