Re: [PATCH v2 12/12] hash: stop depending on `the_repository` in `null_oid()`

[Patrick Steinhardt <ps@pks.im>]

On Thu, Mar 06, 2025 at 11:14:28AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> > index a4431429b7d..2d2e90bc23a 100644
> > --- a/builtin/ls-files.c
> > +++ b/builtin/ls-files.c
> > @@ -234,7 +234,7 @@ static void show_submodule(struct repository *superproject,
> >  {
> >  	struct repository subrepo;
> >  
> > -	if (repo_submodule_init(&subrepo, superproject, path, null_oid()))
> > +	if (repo_submodule_init(&subrepo, superproject, path, null_oid(the_hash_algo)))
> >  		return;
> >  
> >  	if (repo_read_index(&subrepo) < 0)
> 
> This has an obvious semantic interaction with what is done in
> Usman's series <20250306143629.1267358-7-usmanakinyemi202@gmail.com>
> where builtin/ls-files.c claims that it got rid of its dependence on
> the_repository.
> 
> The resulting ls-files still calls null_oid() here, hence it depends
> on the_hash_algo hence indirectly on the_repository.  When these
> topics are merged together, builtin/ls-files.c again needs to be
> marked that it still needs the_repository variable in order to see
> the_hash_algo.

Ah, thanks for catching this!

> I _think_ the subrepo is not allowed to use different hash from the
> superproject, so we can pass superproject->hash_algo instead in this
> series to make it easier on the other topic?

That's actually a very good question -- I have no idea. My gut says that
it probably won't work, but on a conceptual level I think it would be
nice if it did. Reality lies somewhere between. The following commands
work alright:

    $ git init --object-format=sha256 repo
    $ cd repo
    $ git commit --allow-empty --message initial
    $ git submodule add https://github.com/git/git.git
    $ git -C git rev-parse --show-object-info HEAD
    sha1
    a36e024e989f4d35f35987a60e3af8022cac3420

We end up with a SHA1 submodule in a SHA256 repository. So far so good.
How does it look like?

    $ git diff --cached
    diff --git a/.gitmodules b/.gitmodules
    new file mode 100644
    index 0000000..3594794
    --- /dev/null
    +++ b/.gitmodules
    @@ -0,0 +1,3 @@
    +[submodule "git"]
    +	path = git
    +	url = https://github.com/git/git.git
    diff --git a/git b/git
    new file mode 160000
    index 0000000..a36e024
    --- /dev/null
    +++ b/git
    @@ -0,0 +1 @@
    +Subproject commit a36e024e989f4d35f35987a60e3af8022cac3420000000000000000000000000

Well, now it starts to become weird, as the commit hash is obviously
wrong. Can we update the submodule?

    $ git -C git switch next
    $ git diff
    diff --git a/git b/git
    index a36e024..4cd3354 160000
    --- a/git
    +++ b/git
    @@ -1 +1 @@
    -Subproject commit a36e024e989f4d35f35987a60e3af8022cac3420000000000000000000000000
    +Subproject commit 4cd33545ba4fa82324b454aa5bf2748b40a572fb

Well, that diff at least looks somewhat better than expected? Let's
commit it.

    $ git commit -m update
    $ git diff HEAD~
    diff --git a/git b/git
    index a36e024..4cd3354 160000
    --- a/git
    +++ b/git
    @@ -1 +1 @@
    -Subproject commit a36e024e989f4d35f35987a60e3af8022cac3420000000000000000000000000
    +Subproject commit 4cd33545ba4fa82324b454aa5bf2748b40a572fb000000000000000000000000

Oh, well... so, what happens if we ask git-log(1) to recurse into the
submodule?

    $ git log --submodule=diff --patch
    Segmentation fault (core dumped)

Yeah, no, that's not really working.

So in theory, I don't really think there is anything that keeps us from
mixing object formats, and I even think that this is a necessary feature
to have once people start to migrate their repositories to SHA256. There
are people out there who use submodules, and it may not be feasible for
them to wait until all of their submodules have been converted to SHA256
to migrate their own repositories.

From my perspective, we should thus treat it as a bug that the above
does not work.

> What do you think?

Anyway, that was a bit of a tangent to your initial question: which hash
should we be passing in the first place? As it turns out, we should be
passing the superproject's null OID indeed. The object ID we pass to
`repo_submodule_init()` does not describe any object in the submodule
itself, but rather indicates which tree-ish the submodule information
should be parsed from in the superproject. As such, it refers to an
object in the superproject itself and should thus use its object hash.

I'll include such a change in my patch series to resolve the semantic
conflict.

> Perhaps we should have hidden null_oid() as requiring the_repository
> just like the_hash_algo is guarded like so
> 
>         #ifdef USE_THE_REPOSITORY_VARIABLE
>         # include "repository.h"
>         # define the_hash_algo the_repository->hash_algo
>         #endif
> 
> in <hash.h>.
> 
> In other words, I wish we had the following patch already applied,
> before Usman started working on the other topic.

True. `USE_THE_REPOSITORY_VARIABLE` has always been best effort, and it
was clear from the beginning that it won't catch all functions that
implicitly depend on `the_repository`. The good news is that we uncover
and convert more and more of those implicit dependencies, so as time
progresses it becomes more accurate.

> But with this topic getting solidified, it would become a moot point
> to do that in the longer term.  This series removes null_oid() that
> had the implicit dependency anyway.

Indeed.

Patrick