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

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 7, 2025, 09:08 UTC
Message-ID: <Z8q3nzhl3DHETZgf@pks.im>
In-Reply-To: <xmqqr03ageej.fsf@gitster.g>

On Thu, Mar 06, 2025 at 11:14:28AM -0800, Junio C Hamano wrote:

Show 26 quoted lines

> 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.

Show 12 quoted lines

> 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