Re: [PATCH] object-file: disallow adding submodules of different hash algo

On Wed, Nov 12, 2025 at 11:54:34PM +0000, brian m. carlson wrote:

This makes sense. I had meant to follow up on our conversation and patch from last month, but it was still on my todo list. Fortunately that earlier attempt gives me something concrete to compare to. ;)

OK, you're checking for it here in index_path(), whereas my earlier attempt did it in add_to_index(). For the most part, I think your spot makes more sense, as it is at a lower level. add_to_index() eventually calls into index_path(), and so do some other code paths.

That does leave two interesting oddities:

  1. In add_to_index(), we have this code:

          if (S_ISDIR(st_mode)) {
                  if (repo_resolve_gitlink_ref(the_repository, path, "HEAD", &oid) < 0)
                          return error(_("'%s' does not have a commit checked out"), path);
                  while (namelen && path[namelen-1] == '/')
                          namelen--;
          }

     which is run before we hit index_path(). So it may get an oid
     result with an unexpected hash. I think that's OK, because nobody
     ever looks at it (which would be a lot more obvious if we declared
     the variable inside the conditional block here).

     This whole lookup does feel a little funny and redundant. It comes
     from f937bc2f86 (add: error appropriately on repository with no
     commits, 2019-04-09), and the main goal is making the error message
     better. But should we just improve the error message from
     index_path() for this case (in which case the resolve call above go
     away)?

     I think this is mostly orthogonal to your patch and we can ignore
     it for now. I only bring it up because now it's weird that we are
     trying to catch the hash mismatch, but have this unchecked extra
     resolve.

  2. There are paths in add_to_index() that _don't_ hit index_path(). In
     particular, intent-to-add entries. So with your patch, even though
     a regular "git add" is forbidden:

       $ git add repo
       error: cannot add a submodule of a different hash algorithm
       error: unable to index file 'repo'
       fatal: updating files failed

     I can still do this:

       $ git add -N repo
       warning: adding embedded git repository: repo
       $ git ls-files -s
       160000 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	repo

     which skips the hash check entirely. Which kind of makes sense,
     because the resulting index entry does not have a real oid in it at
     all (it gets the empty blob oid). But it does have a real 160000
     mode.

     Can we make things worse from there? If we try to update it, for
     example, that will fail:

       $ git add -u
       error: cannot add a submodule of a different hash algorithm
       error: unable to index file 'repo'
       fatal: updating files failed

     So...maybe this is OK?

Makes sense. Purists might complain about "git ls-files" on the left hand side of a pipe, but I think it is OK here. Though you can golf away a few subprocesses at the same time with: