{"thread":{"id":"64469","subject":"git fails to checkout SHA1 submodule in SHA256 repo with --depth=1","startedAt":"2025-11-12T12:58:48Z","lastAt":"2025-11-17T08:53:10Z","messageCount":21,"participants":["Martin Wilck","Junio C Hamano","brian m. carlson","Jeff King","Marc Branchaud"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"530586","messageId":"c94a929df63f79e49eeae0cd67c1f59f859e3d62.camel@suse.com","threadId":"64469","inReplyTo":null,"subject":"git fails to checkout SHA1 submodule in SHA256 repo with --depth=1","fromName":"Martin Wilck","fromEmail":"mwilck@suse.com","sentAt":"2025-11-12T12:58:46Z","receivedAt":"2025-11-12T12:58:48Z","isPatch":false,"sender":{"key":"mwilck@suse.com","avatar":null},"body":"> What did you do before the bug happened? (Steps to reproduce your\nissue)\n\n# This is a SHA256 repository with a SHA1 submodule \ngit clone -b next https://src.opensuse.org/mwilck/multipath-tools\n\ncd multipath-tools\ngit submodule init\n\n# Submoudle URL: https://github.com/openSUSE/multipath-tools, \n# branch \"next\ngit submodule update --depth 1\n\n> What did you expect to happen? (Expected behavior)\n\nSuccessful checkout of the submodule.\n\n> What happened instead? (Actual behavior)\n\nThe following error:\n\nfatal: couldn't find remote ref 37f9a4c9c4658da7f9b2b0345836360d2fb119a0000000000000000000000000\nfatal: Fetched in submodule path 'multipath-tools', but it did not contain 37f9a4c9c4658da7f9b2b0345836360d2fb119a0000000000000000000000000. Direct fetching of that commit failed.\n\n> What's different between what you expected and what actually\nhappened?\n\nIt failed.\n\n> Anything else you want to add:\n\n\"git submodule update\" (without --depth 1) succeeds.\n\nThe problem occurs whether or not I use \"git submodule set-branch\" to\nselect the correct remote branch before running \"git submodule update\".\nNeither the \"branch\" setting in .gitmodules nor in .git/config seems to\nmatter.\n\nThe problem seems to be that \"git submodule update --depth 1\" fetches\nthe remote HEAD only, even if submodule..branch is set to\nsomething different (here: \"next\"). (In my case, HEAD was not an\nancestor of the desired branch (\"next\"), nor vice-versa).\n\nI found the following workaround to fetch the desired commit:\n\nSUBMODULE=multipath-tools\nSHA=$(git -C $SUBMODULE ls-remote origin | awk '/refs\\/heads\\/next/ { print $1; }')\ngit -C $SUBMODULE fetch --depth=1 origin next\ngit -C $SUBMODULE reset --hard $SHA\n\nbut that's not the desired solution, because the checkout needs to be\nscripted. Ultimately I want to run\n\"git clone --recurse-submodules --depth 1\", which currently fails as\nwell.\n\nA plain \"git clone\" of the submodule works as expected:\n\ngit clone -b next --depth=1\nhttps://github.com/openSUSE/multipath-tools.git\n\nIn a SHA1 repository, all these operations seem to work as one would\nnaïvely expect. (I was using different repositories though, I currently\ndon't have a SHA1 clone of the SHA256 repo I experimented with).\n\n[System Info]\ngit version:\ngit version 2.52.0.rc1.458.g3549877.dirty\ncpu: x86_64\nbuilt from commit: 3549877a16bc196d0d99bc2f8441eedf0102fcc8\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nrust: disabled\nlibcurl: 8.14.1\nOpenSSL: OpenSSL 3.1.4 24 Oct 2023\nzlib: 1.2.13\nSHA-1: SHA1_DC\nSHA-256: SHA256_BLK\ndefault-ref-format: files\ndefault-hash: sha1\nuname: Linux 6.4.0-150600.23.70-default #1 SMP PREEMPT_DYNAMIC Wed Sep\n10 10:54:24 UTC 2025 (225af75) x86_64\ncompiler info: gnuc: 7.5\nlibc info: glibc: 2.38\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n"},{"id":"530594","messageId":"xmqq7bvvtdoe.fsf@gitster.g","threadId":"64469","inReplyTo":"c94a929df63f79e49eeae0cd67c1f59f859e3d62.camel@suse.com","subject":"Re: git fails to checkout SHA1 submodule in SHA256 repo with --depth=1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-12T16:32:01Z","receivedAt":"2025-11-12T16:32:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Wilck writes:\n\n>> Subject: Re: git fails to checkout SHA1 submodule in SHA256 repo with --depth=1\n\nI think it is not supposed to work to mix repositories like this,\nregardless of any other option like --depth. I think brian gave a\nresponse to that effect in a thread in the past few months.\n\n ... goes and looks ...\n\nhttps://lore.kernel.org/git/aJ5gOPQ9oologqj-@fruit.crustytoothpaste.net/\nhttps://lore.kernel.org/git/aKPJNNWMW9gtueEK@fruit.crustytoothpaste.net/\n"},{"id":"530632","messageId":"aRUaR6IfH9imrF5A@fruit.crustytoothpaste.net","threadId":"64469","inReplyTo":"xmqq7bvvtdoe.fsf@gitster.g","subject":"Re: git fails to checkout SHA1 submodule in SHA256 repo with --depth=1","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-11-12T23:37:43Z","receivedAt":"2025-11-12T23:37:51Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-11-12 at 16:32:01, Junio C Hamano wrote:\n> Martin Wilck writes:\n> \n> >> Subject: Re: git fails to checkout SHA1 submodule in SHA256 repo with --depth=1\n> \n> I think it is not supposed to work to mix repositories like this,\n> regardless of any other option like --depth. I think brian gave a\n> response to that effect in a thread in the past few months.\n> \n> ... goes and looks ...\n> \n> https://lore.kernel.org/git/aJ5gOPQ9oologqj-@fruit.crustytoothpaste.net/\n> https://lore.kernel.org/git/aKPJNNWMW9gtueEK@fruit.crustytoothpaste.net/\n\nYes, that isn't going to work and it never will unless we add some\nextension mechanism for that purpose. The repository in question is\ncorrupt.\n\nI've just written a patch to check for this case and produce an error in\ngit add, which I will send shortly.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"530633","messageId":"20251112235434.1499699-1-sandals@crustytoothpaste.net","threadId":"64469","inReplyTo":"c94a929df63f79e49eeae0cd67c1f59f859e3d62.camel@suse.com","subject":"[PATCH] object-file: disallow adding submodules of different hash algo","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-11-12T23:54:34Z","receivedAt":"2025-11-12T23:54:40Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The design of the hash algorithm transition plan is that objects stored\nmust be entirely in one algorithm since we lack any way to indicate a\nmix of algorithms. This also includes submodules, but we have\ntraditionally not enforced this, which leads to various problems when\ntrying to clone or check out the the submodule from the remote.\n\nSince this cannot work in the general case, restrict adding a submodule\nof a different algorithm to the index. Add tests for git add and git\nsubmodule add that these are rejected.\n\nNote that we cannot check this in git fsck because the malformed\nsubmodule is stored in the tree as an object ID which is either\ntruncated (when a SHA-256 submodule is added to a SHA-1 repository) or\npadded with zeros (when a SHA-1 submodule is added to a SHA-256\nrepository). We cannot detect even the latter case because someone\ncould have an actual submodule that actually ends in 24 zeros, which\nwould be a false positive.\n\nSigned-off-by: brian m. carlson \n---\n object-file.c | 6 +++++-\n t/t3700-add.sh | 27 +++++++++++++++++++++++++++\n t/t7400-submodule-basic.sh | 27 +++++++++++++++++++++++++++\n 3 files changed, 59 insertions(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 4675c8ed6b..8c43c52ed0 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1661,7 +1661,11 @@ int index_path(struct index_state *istate, struct object_id *oid,\n \t\tstrbuf_release(&sb);\n \t\tbreak;\n \tcase S_IFDIR:\n-\t\treturn repo_resolve_gitlink_ref(istate->repo, path, \"HEAD\", oid);\n+\t\tif (repo_resolve_gitlink_ref(istate->repo, path, \"HEAD\", oid))\n+\t\t\treturn -1;\n+\t\tif (&hash_algos[oid->algo] != istate->repo->hash_algo)\n+\t\t\treturn error(_(\"cannot add a submodule of a different hash algorithm\"));\n+\t\tbreak;\n \tdefault:\n \t\treturn error(_(\"%s: unsupported file type\"), path);\n \t}\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex df580a5806..b075eb9b11 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -541,6 +541,33 @@ test_expect_success 'all statuses changed in folder if . is given' '\n \t)\n '\n \n+test_expect_success 'cannot add a submodule of a different algorithm' '\n+\tgit init --object-format=sha256 sha256 &&\n+\t(\n+\t\tcd sha256 &&\n+\t\ttest_commit abc &&\n+\t\tgit init --object-format=sha1 submodule &&\n+\t\t(\n+\t\t\tcd submodule &&\n+\t\t\ttest_commit def\n+\t\t) &&\n+\t\ttest_must_fail git add submodule &&\n+\t\ttest $(git ls-files --stage | grep ^160000 | wc -l) -eq 0\n+\t) &&\n+\tgit init --object-format=sha1 sha1 &&\n+\t(\n+\t\tcd sha1 &&\n+\t\ttest_commit abc &&\n+\t\tgit init --object-format=sha256 submodule &&\n+\t\t(\n+\t\t\tcd submodule &&\n+\t\t\ttest_commit def\n+\t\t) &&\n+\t\ttest_must_fail git add submodule &&\n+\t\ttest $(git ls-files --stage | grep ^160000 | wc -l) -eq 0\n+\t)\n+'\n+\n test_expect_success CASE_INSENSITIVE_FS 'path is case-insensitive' '\n \tpath=\"$(pwd)/BLUB\" &&\n \ttouch \"$path\" &&\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex fd3e7e355e..b190182d62 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -407,6 +407,33 @@ test_expect_success 'submodule add in subdirectory with relative path should fai\n \ttest_grep toplevel output.err\n '\n \n+test_expect_success 'submodule add of a different algorithm fails' '\n+\tgit init --object-format=sha256 sha256 &&\n+\t(\n+\t\tcd sha256 &&\n+\t\ttest_commit abc &&\n+\t\tgit init --object-format=sha1 submodule &&\n+\t\t(\n+\t\t\tcd submodule &&\n+\t\t\ttest_commit def\n+\t\t) &&\n+\t\ttest_must_fail git submodule add \"$submodurl\" submodule &&\n+\t\ttest $(git ls-files --stage | grep ^160000 | wc -l) -eq 0\n+\t) &&\n+\tgit init --object-format=sha1 sha1 &&\n+\t(\n+\t\tcd sha1 &&\n+\t\ttest_commit abc &&\n+\t\tgit init --object-format=sha256 submodule &&\n+\t\t(\n+\t\t\tcd submodule &&\n+\t\t\ttest_commit def\n+\t\t) &&\n+\t\ttest_must_fail git submodule add \"$submodurl\" submodule &&\n+\t\ttest $(git ls-files --stage | grep ^160000 | wc -l) -eq 0\n+\t)\n+'\n+\n test_expect_success 'setup - add an example entry to .gitmodules' '\n \tgit config --file=.gitmodules submodule.example.url git://example.com/init.git\n '\n"},{"id":"530638","messageId":"20251113032619.GA1739649@coredump.intra.peff.net","threadId":"64469","inReplyTo":"20251112235434.1499699-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] object-file: disallow adding submodules of different hash algo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-13T03:26:19Z","receivedAt":"2025-11-13T03:26:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2025 at 11:54:34PM +0000, brian m. carlson wrote:\n\n> Since this cannot work in the general case, restrict adding a submodule\n> of a different algorithm to the index. Add tests for git add and git\n> submodule add that these are rejected.\n\nThis makes sense. I had meant to follow up on our conversation and patch\nfrom last month, but it was still on my todo list. Fortunately that\nearlier attempt gives me something concrete to compare to. ;)\n\n> diff --git a/object-file.c b/object-file.c\n> index 4675c8ed6b..8c43c52ed0 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1661,7 +1661,11 @@ int index_path(struct index_state *istate, struct object_id *oid,\n> \t\tstrbuf_release(&sb);\n> \t\tbreak;\n> \tcase S_IFDIR:\n> -\t\treturn repo_resolve_gitlink_ref(istate->repo, path, \"HEAD\", oid);\n> +\t\tif (repo_resolve_gitlink_ref(istate->repo, path, \"HEAD\", oid))\n> +\t\t\treturn -1;\n> +\t\tif (&hash_algos[oid->algo] != istate->repo->hash_algo)\n> +\t\t\treturn error(_(\"cannot add a submodule of a different hash algorithm\"));\n> +\t\tbreak;\n> \tdefault:\n> \t\treturn error(_(\"%s: unsupported file type\"), path);\n> \t}\n\nOK, you're checking for it here in index_path(), whereas my earlier\nattempt did it in add_to_index(). For the most part, I think your spot\nmakes more sense, as it is at a lower level. add_to_index() eventually\ncalls into index_path(), and so do some other code paths.\n\nThat does leave two interesting oddities:\n\n 1. In add_to_index(), we have this code:\n\n if (S_ISDIR(st_mode)) {\n if (repo_resolve_gitlink_ref(the_repository, path, \"HEAD\", &oid) < 0)\n return error(_(\"'%s' does not have a commit checked out\"), path);\n while (namelen && path[namelen-1] == '/')\n namelen--;\n }\n\n which is run before we hit index_path(). So it may get an oid\n result with an unexpected hash. I think that's OK, because nobody\n ever looks at it (which would be a lot more obvious if we declared\n the variable inside the conditional block here).\n\n This whole lookup does feel a little funny and redundant. It comes\n from f937bc2f86 (add: error appropriately on repository with no\n commits, 2019-04-09), and the main goal is making the error message\n better. But should we just improve the error message from\n index_path() for this case (in which case the resolve call above go\n away)?\n\n I think this is mostly orthogonal to your patch and we can ignore\n it for now. I only bring it up because now it's weird that we are\n trying to catch the hash mismatch, but have this unchecked extra\n resolve.\n\n 2. There are paths in add_to_index() that _don't_ hit index_path(). In\n particular, intent-to-add entries. So with your patch, even though\n a regular \"git add\" is forbidden:\n\n $ git add repo\n error: cannot add a submodule of a different hash algorithm\n error: unable to index file 'repo'\n fatal: updating files failed\n\n I can still do this:\n\n $ git add -N repo\n warning: adding embedded git repository: repo\n $ git ls-files -s\n 160000 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\trepo\n\n which skips the hash check entirely. Which kind of makes sense,\n because the resulting index entry does not have a real oid in it at\n all (it gets the empty blob oid). But it does have a real 160000\n mode.\n\n Can we make things worse from there? If we try to update it, for\n example, that will fail:\n\n $ git add -u\n error: cannot add a submodule of a different hash algorithm\n error: unable to index file 'repo'\n fatal: updating files failed\n\n So...maybe this is OK?\n\n> +test_expect_success 'cannot add a submodule of a different algorithm' '\n> +\tgit init --object-format=sha256 sha256 &&\n> +\t(\n> +\t\tcd sha256 &&\n> +\t\ttest_commit abc &&\n> +\t\tgit init --object-format=sha1 submodule &&\n> +\t\t(\n> +\t\t\tcd submodule &&\n> +\t\t\ttest_commit def\n> +\t\t) &&\n> +\t\ttest_must_fail git add submodule &&\n> +\t\ttest $(git ls-files --stage | grep ^160000 | wc -l) -eq 0\n> +\t) &&\n\nMakes sense. Purists might complain about \"git ls-files\" on the left\nhand side of a pipe, but I think it is OK here. Though you can golf away\na few subprocesses at the same time with:\n\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex b075eb9b11..6a1d4e2659 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -547,12 +547,10 @@ test_expect_success 'cannot add a submodule of a different algorithm' '\n \t\tcd sha256 &&\n \t\ttest_commit abc &&\n \t\tgit init --object-format=sha1 submodule &&\n-\t\t(\n-\t\t\tcd submodule &&\n-\t\t\ttest_commit def\n-\t\t) &&\n+\t\ttest_commit -C submodule def &&\n \t\ttest_must_fail git add submodule &&\n-\t\ttest $(git ls-files --stage | grep ^160000 | wc -l) -eq 0\n+\t\tgit ls-files --stage >entries &&\n+\t\ttest_grep ! ^160000 entries\n \t) &&\n \tgit init --object-format=sha1 sha1 &&\n \t(\n\nbut we are getting into nits there.\n\nIs it worth checking the stderr of the failing \"git add submodule\" call?\nAdding a repo directly via \"git add\" is already something we generate a\nwarning for, and it's possible we might eventually make it an error. In\nwhich case the command would fail without even hitting your new code,\nbut we'd have no idea. Adding in a test_grep for \"cannot add a submodule\nof a different hash algorithm\" would at least make sure we're hitting\nthe error we expect.\n\n-Peff\n"},{"id":"530639","messageId":"20251113035614.GA1758009@coredump.intra.peff.net","threadId":"64469","inReplyTo":"20251113032619.GA1739649@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: disallow adding submodules of different hash algo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-13T03:56:14Z","receivedAt":"2025-11-13T03:56:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 12, 2025 at 10:26:24PM -0500, Jeff King wrote:\n\n> This whole lookup does feel a little funny and redundant. It comes\n> from f937bc2f86 (add: error appropriately on repository with no\n> commits, 2019-04-09), and the main goal is making the error message\n> better. But should we just improve the error message from\n> index_path() for this case (in which case the resolve call above go\n> away)?\n> \n> I think this is mostly orthogonal to your patch and we can ignore\n> it for now. I only bring it up because now it's weird that we are\n> trying to catch the hash mismatch, but have this unchecked extra\n> resolve.\n\nSo this is what I'd propose on top of your patch. I can hold onto it for\nlater if we don't want to muddy up what you're trying to do.\n\n-- >8 --\nSubject: [PATCH] read-cache: drop submodule check from add_to_cache()\n\nIn add_to_cache(), we treat any directories as submodules, and complain\nif we can't resolve their HEAD. This call to resolve_gitlink_ref() was\nadded by f937bc2f86 (add: error appropriately on repository with no\ncommits, 2019-04-09), with the goal of improving the error message for\nempty repositories.\n\nBut we already resolve the submodule HEAD in index_path(), which is\nwhere we find the actual oid we're going to use. Resolving it again here\nintroduces some downsides:\n\n 1. It's more work, since we have to open up the submodule repository's\n files twice.\n\n 2. There are call paths that get to index_path() without going through\n add_to_cache(). For instance, we'd want a similar informative\n message if \"git diff empty\" finds that it can't resolve the\n submodule's HEAD. (In theory we can also get there through\n update-index, but AFAICT it refuses to consider directories as\n submodules at all, and just complains about them).\n\n 3. The resolution in index_path() catches more errors that we don't\n handle here. In particular, it will validate that the object format\n for the submodule matches that of the superproject. This isn't a\n bug, since our call in add_to_cache() throws away the oid it gets\n without looking at it. But it certainly caused confusion for me\n when looking at where the object-format check should go.\n\nSo instead of resolving the submodule HEAD in add_to_cache(), let's just\nteach the call in index_path() to actually produce an error message\n(which it already does for other cases). That's probably what f937bc2f86\nshould have done in the first place, and it gives us a single point of\nresolution when adding a submodule to the index.\n\nThe resulting output is slightly more verbose, as we propagate the error\nup the call stack, but I think that's OK (and again, matches many other\nerrors we get when indexing fails).\n\nI've left the text of the error message as-is, though it is perhaps\noverly specific. There are many reasons that resolving the submodule\nHEAD might fail, though outside of corruption or system errors it is\nprobably most likely that the submodule HEAD is simply on an unborn\nbranch.\n\nSigned-off-by: Jeff King \n---\n object-file.c | 2 +-\n read-cache.c | 3 ---\n t/t3700-add.sh | 1 +\n 3 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 8c43c52ed0..a7438b6205 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1662,7 +1662,7 @@ int index_path(struct index_state *istate, struct object_id *oid,\n \t\tbreak;\n \tcase S_IFDIR:\n \t\tif (repo_resolve_gitlink_ref(istate->repo, path, \"HEAD\", oid))\n-\t\t\treturn -1;\n+\t\t\treturn error(_(\"'%s' does not have a commit checked out\"), path);\n \t\tif (&hash_algos[oid->algo] != istate->repo->hash_algo)\n \t\t\treturn error(_(\"cannot add a submodule of a different hash algorithm\"));\n \t\tbreak;\ndiff --git a/read-cache.c b/read-cache.c\nindex 032480d0c7..990d4ead0d 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -706,7 +706,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \tint add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|\n \t\t\t (intent_only ? ADD_CACHE_NEW_ONLY : 0));\n \tunsigned hash_flags = pretend ? 0 : INDEX_WRITE_OBJECT;\n-\tstruct object_id oid;\n \n \tif (flags & ADD_CACHE_RENORMALIZE)\n \t\thash_flags |= INDEX_RENORMALIZE;\n@@ -716,8 +715,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \n \tnamelen = strlen(path);\n \tif (S_ISDIR(st_mode)) {\n-\t\tif (repo_resolve_gitlink_ref(the_repository, path, \"HEAD\", &oid) < 0)\n-\t\t\treturn error(_(\"'%s' does not have a commit checked out\"), path);\n \t\twhile (namelen && path[namelen-1] == '/')\n \t\t\tnamelen--;\n \t}\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex b075eb9b11..d8cc0e4c66 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -388,6 +388,7 @@ test_expect_success 'error on a repository with no commits' '\n \ttest_must_fail git add empty >actual 2>&1 &&\n \tcat >expect <<-EOF &&\n \terror: '\"'empty/'\"' does not have a commit checked out\n+\terror: unable to index file '\"'empty/'\"'\n \tfatal: adding files failed\n \tEOF\n \ttest_cmp expect actual\n-- \n2.52.0.rc2.237.g020256b90a\n\n"},{"id":"530642","messageId":"a1c0440a6eef8f306f53793b2f96636945d4ced4.camel@suse.com","threadId":"64469","inReplyTo":"aRUaR6IfH9imrF5A@fruit.crustytoothpaste.net","subject":"Re: git fails to checkout SHA1 submodule in SHA256 repo with --depth=1","fromName":"Martin Wilck","fromEmail":"mwilck@suse.com","sentAt":"2025-11-13T10:15:30Z","receivedAt":"2025-11-13T10:15:34Z","isPatch":false,"sender":{"key":"mwilck@suse.com","avatar":null},"body":"On Wed, 2025-11-12 at 23:37 +0000, brian m. carlson wrote:\n> On 2025-11-12 at 16:32:01, Junio C Hamano wrote:\n> > Martin Wilck writes:\n> > \n> > > > Subject: Re: git fails to checkout SHA1 submodule in SHA256\n> > > > repo with --depth=1\n> > \n> > I think it is not supposed to work to mix repositories like this,\n> > regardless of any other option like --depth.  I think brian gave a\n> > response to that effect in a thread in the past few months.\n> > \n> >     ... goes and looks ...\n> > \n> > https://lore.kernel.org/git/aJ5gOPQ9oologqj-@fruit.crustytoothpaste.net/\n> > https://lore.kernel.org/git/aKPJNNWMW9gtueEK@fruit.crustytoothpaste.net/\n> \n> Yes, that isn't going to work and it never will unless we add some\n> extension mechanism for that purpose.  The repository in question is\n> corrupt.\n\nOk, thanks for the clarification.\n\nLet me just explain the use case: The distribution ((open)SUSE) has\nswitched to git for version control of its packages. We have chosen\nSHA256, because we'll need to support the distribution for many years\nto come, much longer than SHA1 is going to be considered good enough.\n\nWe can store the source code of the package e.g. in the form of\ntarballs (and we do). But it's convenient and efficient, and thus\ntempting for developers, to simply link to an existing repository\nhosting the sources, using a submodule. And upstream repos still use\nSHA1. This is what lead us to experiment with this sort of mixed\nrepository.\n\nI get it that the concept is flawed and unsupported. Up to now, that\nwasn't obvious to me.\n\nSo what we can do now is either keep storing tarballs, or wait until\nthere's a full solution for migration between / interoperability of\ndifferent hash algorithms, and until the source code repos we're\ninterested in have been fully migrated to SHA256. In some special\ncases, where (open)SUSE owns the source repositories, we may be able to\nsimply migrate to a SHA256 forge. We can also invent a \"poor man's\nsubmodule\" mechanism to link to sources on some external repository\nfrom ours [1].\n\nDo you see any other approach that I'm overlooking?\n\nAnother question: If I, in the current repo [2], create a commit on top\nremoving the submodule and replacing it by a tarball, would the\nrepository remain broken, as it would still have the deprecated\nSHA256/SHA1 combination in the history? Should I expect errors if run\ne.g. \"git rebase\" or \"git bisect\" in a repository like this? IOW, do I\nneed to rewrite the history of this repo, eliminating all instances of\nsuch mixed-hash submodules, to be on the safe side?\n\nThanks,\nMartin\n\n[1] Such a thing exists already, but to me it feels less clean and\nelegant than using native git functionality.\n[2] https://src.opensuse.org/mwilck/multipath-tools\n"},{"id":"530649","messageId":"xmqqjyztq4kz.fsf@gitster.g","threadId":"64469","inReplyTo":"20251113035614.GA1758009@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: disallow adding submodules of different hash algo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-13T16:29:00Z","receivedAt":"2025-11-13T16:29:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So this is what I'd propose on top of your patch. I can hold onto it for\n> later if we don't want to muddy up what you're trying to do.\n\nI do agree with both of the above. The patch below makes perfect\nsense to me, and it is more about the quality of implementation of\nthis codepath in general, than the primary theme of Brian's changes,\nso there is no strong reason they have to come in a single series.\n\n> -- >8 --\n> Subject: [PATCH] read-cache: drop submodule check from add_to_cache()\n>\n> In add_to_cache(), we treat any directories as submodules, and complain\n> if we can't resolve their HEAD. This call to resolve_gitlink_ref() was\n> added by f937bc2f86 (add: error appropriately on repository with no\n> commits, 2019-04-09), with the goal of improving the error message for\n> empty repositories.\n>\n> But we already resolve the submodule HEAD in index_path(), which is\n> where we find the actual oid we're going to use. Resolving it again here\n> introduces some downsides:\n>\n> 1. It's more work, since we have to open up the submodule repository's\n> files twice.\n>\n> 2. There are call paths that get to index_path() without going through\n> add_to_cache(). For instance, we'd want a similar informative\n> message if \"git diff empty\" finds that it can't resolve the\n> submodule's HEAD. (In theory we can also get there through\n> update-index, but AFAICT it refuses to consider directories as\n> submodules at all, and just complains about them).\n>\n> 3. The resolution in index_path() catches more errors that we don't\n> handle here. In particular, it will validate that the object format\n> for the submodule matches that of the superproject. This isn't a\n> bug, since our call in add_to_cache() throws away the oid it gets\n> without looking at it. But it certainly caused confusion for me\n> when looking at where the object-format check should go.\n>\n> So instead of resolving the submodule HEAD in add_to_cache(), let's just\n> teach the call in index_path() to actually produce an error message\n> (which it already does for other cases). That's probably what f937bc2f86\n> should have done in the first place, and it gives us a single point of\n> resolution when adding a submodule to the index.\n>\n> The resulting output is slightly more verbose, as we propagate the error\n> up the call stack, but I think that's OK (and again, matches many other\n> errors we get when indexing fails).\n>\n> I've left the text of the error message as-is, though it is perhaps\n> overly specific. There are many reasons that resolving the submodule\n> HEAD might fail, though outside of corruption or system errors it is\n> probably most likely that the submodule HEAD is simply on an unborn\n> branch.\n>\n> Signed-off-by: Jeff King \n> ---\n> object-file.c | 2 +-\n> read-cache.c | 3 ---\n> t/t3700-add.sh | 1 +\n> 3 files changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index 8c43c52ed0..a7438b6205 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1662,7 +1662,7 @@ int index_path(struct index_state *istate, struct object_id *oid,\n> \t\tbreak;\n> \tcase S_IFDIR:\n> \t\tif (repo_resolve_gitlink_ref(istate->repo, path, \"HEAD\", oid))\n> -\t\t\treturn -1;\n> +\t\t\treturn error(_(\"'%s' does not have a commit checked out\"), path);\n> \t\tif (&hash_algos[oid->algo] != istate->repo->hash_algo)\n> \t\t\treturn error(_(\"cannot add a submodule of a different hash algorithm\"));\n> \t\tbreak;\n> diff --git a/read-cache.c b/read-cache.c\n> index 032480d0c7..990d4ead0d 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -706,7 +706,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n> \tint add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|\n> \t\t\t (intent_only ? ADD_CACHE_NEW_ONLY : 0));\n> \tunsigned hash_flags = pretend ? 0 : INDEX_WRITE_OBJECT;\n> -\tstruct object_id oid;\n> \n> \tif (flags & ADD_CACHE_RENORMALIZE)\n> \t\thash_flags |= INDEX_RENORMALIZE;\n> @@ -716,8 +715,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n> \n> \tnamelen = strlen(path);\n> \tif (S_ISDIR(st_mode)) {\n> -\t\tif (repo_resolve_gitlink_ref(the_repository, path, \"HEAD\", &oid) < 0)\n> -\t\t\treturn error(_(\"'%s' does not have a commit checked out\"), path);\n> \t\twhile (namelen && path[namelen-1] == '/')\n> \t\t\tnamelen--;\n> \t}\n> diff --git a/t/t3700-add.sh b/t/t3700-add.sh\n> index b075eb9b11..d8cc0e4c66 100755\n> --- a/t/t3700-add.sh\n> +++ b/t/t3700-add.sh\n> @@ -388,6 +388,7 @@ test_expect_success 'error on a repository with no commits' '\n> \ttest_must_fail git add empty >actual 2>&1 &&\n> \tcat >expect <<-EOF &&\n> \terror: '\"'empty/'\"' does not have a commit checked out\n> +\terror: unable to index file '\"'empty/'\"'\n> \tfatal: adding files failed\n> \tEOF\n> \ttest_cmp expect actual\n"},{"id":"530669","messageId":"aRZhAEpm8PbBQcIM@fruit.crustytoothpaste.net","threadId":"64469","inReplyTo":"a1c0440a6eef8f306f53793b2f96636945d4ced4.camel@suse.com","subject":"Re: git fails to checkout SHA1 submodule in SHA256 repo with --depth=1","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-11-13T22:51:44Z","receivedAt":"2025-11-13T22:51:47Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-11-13 at 10:15:30, Martin Wilck wrote:\n> Let me just explain the use case: The distribution ((open)SUSE) has\n> switched to git for version control of its packages. We have chosen\n> SHA256, because we'll need to support the distribution for many years\n> to come, much longer than SHA1 is going to be considered good enough.\n> \n> We can store the source code of the package e.g. in the form of\n> tarballs (and we do). But it's convenient and efficient, and thus\n> tempting for developers, to simply link to an existing repository\n> hosting the sources, using a submodule. And upstream repos still use\n> SHA1. This is what lead us to experiment with this sort of mixed\n> repository.\n> \n> I get it that the concept is flawed and unsupported. Up to now, that\n> wasn't obvious to me.\n\nIt wasn't obvious to a lot of people because the assumption is only\ndocumented in the transition plan. Mixed algorithm submodules are\nsomething we should have thought about earlier on, but we didn't.\n\nThe original transition plan was to have full interoperability support\nearly on, in which case this wouldn't have been a problem, but for\ntechnical reasons it ended up being much easier to have SHA-256-only\nrepositories, so we finished that first. I then lost interest in the\nproject for many years (having kind of burnt out on the work) and, with\nthe exception of one set of patches that were sent in, nobody else\npicked it up either. So this ideally would have been implemented sooner\nunder the original plan, but it wasn't. This is one of the pitfalls of\nopen source projects, as we all know.\n\n> So what we can do now is either keep storing tarballs, or wait until\n> there's a full solution for migration between / interoperability of\n> different hash algorithms, and until the source code repos we're\n> interested in have been fully migrated to SHA256. In some special\n> cases, where (open)SUSE owns the source repositories, we may be able to\n> simply migrate to a SHA256 forge. We can also invent a \"poor man's\n> submodule\" mechanism to link to sources on some external repository\n> from ours [1].\n\nThere is work underway for SHA-1/SHA-256 interoperability. However, it\nis a thing I'm working on primarily in my free time (although also a\nlittle at work) and nobody else has stepped up to contribute, so it will\ntake some time to complete and is not expected to be included in Git\n3.0 without additional assistance.\n\nTo be clear, the existing interoperability code does work for\nrepositories without submodules and you may find it on the\n`sha256-interop` branch at https://github.com/bk2204/git.git, but it\nisn't production ready. I might use it myself, but I would not\nrecommend others do so in its current state.\n\nFor right now, I would say that you should keep storing tarballs.\n\n> Another question: If I, in the current repo [2], create a commit on top\n> removing the submodule and replacing it by a tarball, would the\n> repository remain broken, as it would still have the deprecated\n> SHA256/SHA1 combination in the history? Should I expect errors if run\n> e.g. \"git rebase\" or \"git bisect\" in a repository like this? IOW, do I\n> need to rewrite the history of this repo, eliminating all instances of\n> such mixed-hash submodules, to be on the safe side?\n\nIf the mismatched submodule remains in the history, then it will be\nbroken, so you'll almost certainly want to rewrite the history.\n\nWhen the interoperability work is done and fully functional, a\nrepository in this situation will not work at all with dual algorithms.\nThe reason for that is that the SHA-256 main repository will try to use\nthe submodule value as a SHA-256 submodule (using the incorrect padded\nSHA-1 value as a SHA-256 value) and it won't be able to create the\nappropriate SHA-1/SHA-256 mapping since that object ID doesn't really\nexist in the submodule.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"530670","messageId":"1cf41aad10ab0d32cbb7334cd59144c35629c9b5.camel@suse.com","threadId":"64469","inReplyTo":"aRZhAEpm8PbBQcIM@fruit.crustytoothpaste.net","subject":"Re: git fails to checkout SHA1 submodule in SHA256 repo with --depth=1","fromName":"Martin Wilck","fromEmail":"mwilck@suse.com","sentAt":"2025-11-13T22:57:19Z","receivedAt":"2025-11-13T22:57:22Z","isPatch":false,"sender":{"key":"mwilck@suse.com","avatar":null},"body":"On Thu, 2025-11-13 at 22:51 +0000, brian m. carlson wrote:\n> On 2025-11-13 at 10:15:30, Martin Wilck wrote:\n> > \n> > I get it that the concept is flawed and unsupported. Up to now,\n> > that\n> > wasn't obvious to me.\n> \n> It wasn't obvious to a lot of people because the assumption is only\n> documented in the transition plan.  Mixed algorithm submodules are\n> something we should have thought about earlier on, but we didn't.\n> \n> The original transition plan was to have full interoperability\n> support\n> early on, in which case this wouldn't have been a problem, but for\n> technical reasons it ended up being much easier to have SHA-256-only\n> repositories, so we finished that first.  I then lost interest in the\n> project for many years (having kind of burnt out on the work) and,\n> with\n> the exception of one set of patches that were sent in, nobody else\n> picked it up either.  So this ideally would have been implemented\n> sooner\n> under the original plan, but it wasn't.  This is one of the pitfalls\n> of\n> open source projects, as we all know.\n>\n> [...]\n\nThank you for the detailed response, and good luck finding some support\nfor this very important project.\n\nMartin\n\n"},{"id":"530672","messageId":"aRZmoAI_RjkRgB8l@fruit.crustytoothpaste.net","threadId":"64469","inReplyTo":"20251113032619.GA1739649@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: disallow adding submodules of different hash algo","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-11-13T23:15:44Z","receivedAt":"2025-11-13T23:15:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-11-13 at 03:26:19, Jeff King wrote:\n> This makes sense. I had meant to follow up on our conversation and patch\n> from last month, but it was still on my todo list. Fortunately that\n> earlier attempt gives me something concrete to compare to. ;)\n\nThat's why I CC'd you. I hadn't seen your patch and wasn't sure if I'd\nmissed it or not. I wanted to avoid us doing duplicate work.\n\n> OK, you're checking for it here in index_path(), whereas my earlier\n> attempt did it in add_to_index(). For the most part, I think your spot\n> makes more sense, as it is at a lower level. add_to_index() eventually\n> calls into index_path(), and so do some other code paths.\n\nYeah, I was hoping to catch any code path someone might use that updated\nthe index by path and this seemed like the most likely candidate. I\nthink we'd be okay for `git update-index` or other things that update by\nhash because they always interpret the object ID as the main algorithm.\n\n> That does leave two interesting oddities:\n> \n> 1. In add_to_index(), we have this code:\n> \n> if (S_ISDIR(st_mode)) {\n> if (repo_resolve_gitlink_ref(the_repository, path, \"HEAD\", &oid) < 0)\n> return error(_(\"'%s' does not have a commit checked out\"), path);\n> while (namelen && path[namelen-1] == '/')\n> namelen--;\n> }\n> \n> which is run before we hit index_path(). So it may get an oid\n> result with an unexpected hash. I think that's OK, because nobody\n> ever looks at it (which would be a lot more obvious if we declared\n> the variable inside the conditional block here).\n> \n> This whole lookup does feel a little funny and redundant. It comes\n> from f937bc2f86 (add: error appropriately on repository with no\n> commits, 2019-04-09), and the main goal is making the error message\n> better. But should we just improve the error message from\n> index_path() for this case (in which case the resolve call above go\n> away)?\n> \n> I think this is mostly orthogonal to your patch and we can ignore\n> it for now. I only bring it up because now it's weird that we are\n> trying to catch the hash mismatch, but have this unchecked extra\n> resolve.\n\nYeah, I think we can. I may have touched this case in my interop\nseries, but I think it's just to add another parameter to the function.\nI don't recall anything super interesting about this in terms of hash\ninteroperability here.\n\n> 2. There are paths in add_to_index() that _don't_ hit index_path(). In\n> particular, intent-to-add entries. So with your patch, even though\n> a regular \"git add\" is forbidden:\n> \n> $ git add repo\n> error: cannot add a submodule of a different hash algorithm\n> error: unable to index file 'repo'\n> fatal: updating files failed\n> \n> I can still do this:\n> \n> $ git add -N repo\n> warning: adding embedded git repository: repo\n> $ git ls-files -s\n> 160000 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\trepo\n> \n> which skips the hash check entirely. Which kind of makes sense,\n> because the resulting index entry does not have a real oid in it at\n> all (it gets the empty blob oid). But it does have a real 160000\n> mode.\n\nRight. I at first tried to add the check there and the I realized it\nwas actually storing the empty blob OID and thus the hash algorithm\nwould never mismatch.\n\n> Can we make things worse from there? If we try to update it, for\n> example, that will fail:\n> \n> $ git add -u\n> error: cannot add a submodule of a different hash algorithm\n> error: unable to index file 'repo'\n> fatal: updating files failed\n> \n> So...maybe this is OK?\n\nI think it is, or at least it's the best we can do. If you do a commit\nwith the empty blob OID as your submodule, you already have a corrupt\nrepository, so catching it later on when you do `git add` or `git\nupdate-index` should be okay.\n\n> Makes sense. Purists might complain about \"git ls-files\" on the left\n> hand side of a pipe, but I think it is OK here. Though you can golf away\n> a few subprocesses at the same time with:\n\nI stole that line from elsewhere in t3700 and modified it. I agree that\nit is maybe not the ideal style for our codebase, but I did appreciate\nthe elegance of doing the operation in a single line.\n\n> Is it worth checking the stderr of the failing \"git add submodule\" call?\n> Adding a repo directly via \"git add\" is already something we generate a\n> warning for, and it's possible we might eventually make it an error. In\n> which case the command would fail without even hitting your new code,\n> but we'd have no idea. Adding in a test_grep for \"cannot add a submodule\n> of a different hash algorithm\" would at least make sure we're hitting\n> the error we expect.\n\nI can do a v2 with those changes. I will say that I also added changes\nfor `git submodule add`, which would catch the problem you mentioned,\nbecause I thought that was the code most likely to have a different\nimplementation at some point in the future and I wanted to catch that\nand also anybody doing the sensible thing in terms of adding submodules.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"530718","messageId":"b434bcb1-bf6e-4fe7-af2d-8c33348378fa@xiplink.com","threadId":"64469","inReplyTo":"a1c0440a6eef8f306f53793b2f96636945d4ced4.camel@suse.com","subject":"Re: git fails to checkout SHA1 submodule in SHA256 repo with --depth=1","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2025-11-14T22:55:07Z","receivedAt":"2025-11-14T22:55:15Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"\nOn 2025-11-13 03:15, Martin Wilck wrote:\n> On Wed, 2025-11-12 at 23:37 +0000, brian m. carlson wrote:\n>> On 2025-11-12 at 16:32:01, Junio C Hamano wrote:\n>>> Martin Wilck writes:\n>>>\n>>>>> Subject: Re: git fails to checkout SHA1 submodule in SHA256\n>>>>> repo with --depth=1\n>>>\n>>> I think it is not supposed to work to mix repositories like this,\n>>> regardless of any other option like --depth.  I think brian gave a\n>>> response to that effect in a thread in the past few months.\n>>>\n>>>     ... goes and looks ...\n>>>\n>>> https://lore.kernel.org/git/aJ5gOPQ9oologqj-@fruit.crustytoothpaste.net/\n>>> https://lore.kernel.org/git/aKPJNNWMW9gtueEK@fruit.crustytoothpaste.net/\n>>\n>> Yes, that isn't going to work and it never will unless we add some\n>> extension mechanism for that purpose.  The repository in question is\n>> corrupt.\n> \n> Ok, thanks for the clarification.\n> \n> Let me just explain the use case: The distribution ((open)SUSE) has\n> switched to git for version control of its packages. We have chosen\n> SHA256, because we'll need to support the distribution for many years\n> to come, much longer than SHA1 is going to be considered good enough.\n> \n> We can store the source code of the package e.g. in the form of\n> tarballs (and we do). But it's convenient and efficient, and thus\n> tempting for developers, to simply link to an existing repository\n> hosting the sources, using a submodule. And upstream repos still use\n> SHA1. This is what lead us to experiment with this sort of mixed\n> repository.\n> \n> I get it that the concept is flawed and unsupported. Up to now, that\n> wasn't obvious to me.\n> \n> So what we can do now is either keep storing tarballs, or wait until\n> there's a full solution for migration between / interoperability of\n> different hash algorithms, and until the source code repos we're\n> interested in have been fully migrated to SHA256. In some special\n> cases, where (open)SUSE owns the source repositories, we may be able to\n> simply migrate to a SHA256 forge. We can also invent a \"poor man's\n> submodule\" mechanism to link to sources on some external repository\n> from ours [1].\n> \n> Do you see any other approach that I'm overlooking?\n\nSet up SHA256 mirrors of the SHA1 repos you want to track in submodules?\n\nI'm assuming that it would be easy to keep such mirrors up to date. I'm \nnot familiar enough with Brian's work to know if a SHA256 repo can have\na SHA1 remote (which would mean that updates would just be a simple \"git \nfetch origin\"), but even if that's not possible I'm guessing that \nscripting some kind of regularly-run translation of new commits wouldn't \nbe too hard.\n\n(This feels similar to Git's early years, when people set up Git mirrors \nof repos that used other VCSes like subversion...)\n\nThat might also help encourage your upstreams to switch, if you can give \nthem a ready-to-use SHA256 version of their repos.\n\n\t\tM.\n\n"},{"id":"530720","messageId":"aRe6l5YRCzLAxeuB@fruit.crustytoothpaste.net","threadId":"64469","inReplyTo":"xmqqjyztq4kz.fsf@gitster.g","subject":"Re: [PATCH] object-file: disallow adding submodules of different hash algo","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-11-14T23:26:15Z","receivedAt":"2025-11-14T23:26:23Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-11-13 at 16:29:00, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > So this is what I'd propose on top of your patch. I can hold onto it for\n> > later if we don't want to muddy up what you're trying to do.\n> \n> I do agree with both of the above. The patch below makes perfect\n> sense to me, and it is more about the quality of implementation of\n> this codepath in general, than the primary theme of Brian's changes,\n> so there is no strong reason they have to come in a single series.\n\nThat's certainly true, but it still applies cleanly on top of my revised\npatch and I'll just include it in v2. I'm running tests now and should\nsend out the series later tonight.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"530721","messageId":"20251115005818.2271557-2-sandals@crustytoothpaste.net","threadId":"64469","inReplyTo":"20251115005818.2271557-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 2/2] read-cache: drop submodule check from add_to_cache()","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-11-15T00:58:18Z","receivedAt":"2025-11-15T00:58:21Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: Jeff King \n\nIn add_to_cache(), we treat any directories as submodules, and complain\nif we can't resolve their HEAD. This call to resolve_gitlink_ref() was\nadded by f937bc2f86 (add: error appropriately on repository with no\ncommits, 2019-04-09), with the goal of improving the error message for\nempty repositories.\n\nBut we already resolve the submodule HEAD in index_path(), which is\nwhere we find the actual oid we're going to use. Resolving it again here\nintroduces some downsides:\n\n 1. It's more work, since we have to open up the submodule repository's\n files twice.\n\n 2. There are call paths that get to index_path() without going through\n add_to_cache(). For instance, we'd want a similar informative\n message if \"git diff empty\" finds that it can't resolve the\n submodule's HEAD. (In theory we can also get there through\n update-index, but AFAICT it refuses to consider directories as\n submodules at all, and just complains about them).\n\n 3. The resolution in index_path() catches more errors that we don't\n handle here. In particular, it will validate that the object format\n for the submodule matches that of the superproject. This isn't a\n bug, since our call in add_to_cache() throws away the oid it gets\n without looking at it. But it certainly caused confusion for me\n when looking at where the object-format check should go.\n\nSo instead of resolving the submodule HEAD in add_to_cache(), let's just\nteach the call in index_path() to actually produce an error message\n(which it already does for other cases). That's probably what f937bc2f86\nshould have done in the first place, and it gives us a single point of\nresolution when adding a submodule to the index.\n\nThe resulting output is slightly more verbose, as we propagate the error\nup the call stack, but I think that's OK (and again, matches many other\nerrors we get when indexing fails).\n\nI've left the text of the error message as-is, though it is perhaps\noverly specific. There are many reasons that resolving the submodule\nHEAD might fail, though outside of corruption or system errors it is\nprobably most likely that the submodule HEAD is simply on an unborn\nbranch.\n\nSigned-off-by: Jeff King \n---\n object-file.c | 2 +-\n read-cache.c | 3 ---\n t/t3700-add.sh | 1 +\n 3 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 8c43c52ed0..a7438b6205 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1662,7 +1662,7 @@ int index_path(struct index_state *istate, struct object_id *oid,\n \t\tbreak;\n \tcase S_IFDIR:\n \t\tif (repo_resolve_gitlink_ref(istate->repo, path, \"HEAD\", oid))\n-\t\t\treturn -1;\n+\t\t\treturn error(_(\"'%s' does not have a commit checked out\"), path);\n \t\tif (&hash_algos[oid->algo] != istate->repo->hash_algo)\n \t\t\treturn error(_(\"cannot add a submodule of a different hash algorithm\"));\n \t\tbreak;\ndiff --git a/read-cache.c b/read-cache.c\nindex 032480d0c7..990d4ead0d 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -706,7 +706,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \tint add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|\n \t\t\t (intent_only ? ADD_CACHE_NEW_ONLY : 0));\n \tunsigned hash_flags = pretend ? 0 : INDEX_WRITE_OBJECT;\n-\tstruct object_id oid;\n \n \tif (flags & ADD_CACHE_RENORMALIZE)\n \t\thash_flags |= INDEX_RENORMALIZE;\n@@ -716,8 +715,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \n \tnamelen = strlen(path);\n \tif (S_ISDIR(st_mode)) {\n-\t\tif (repo_resolve_gitlink_ref(the_repository, path, \"HEAD\", &oid) < 0)\n-\t\t\treturn error(_(\"'%s' does not have a commit checked out\"), path);\n \t\twhile (namelen && path[namelen-1] == '/')\n \t\t\tnamelen--;\n \t}\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 9a2c8dbcc2..af93e53c12 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -388,6 +388,7 @@ test_expect_success 'error on a repository with no commits' '\n \ttest_must_fail git add empty >actual 2>&1 &&\n \tcat >expect <<-EOF &&\n \terror: '\"'empty/'\"' does not have a commit checked out\n+\terror: unable to index file '\"'empty/'\"'\n \tfatal: adding files failed\n \tEOF\n \ttest_cmp expect actual\n"},{"id":"530722","messageId":"20251115005818.2271557-1-sandals@crustytoothpaste.net","threadId":"64469","inReplyTo":"20251112235434.1499699-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 1/2] object-file: disallow adding submodules of different hash algo","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-11-15T00:58:17Z","receivedAt":"2025-11-15T00:58:21Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The design of the hash algorithm transition plan is that objects stored\nmust be entirely in one algorithm since we lack any way to indicate a\nmix of algorithms. This also includes submodules, but we have\ntraditionally not enforced this, which leads to various problems when\ntrying to clone or check out the the submodule from the remote.\n\nSince this cannot work in the general case, restrict adding a submodule\nof a different algorithm to the index. Add tests for git add and git\nsubmodule add that these are rejected.\n\nNote that we cannot check this in git fsck because the malformed\nsubmodule is stored in the tree as an object ID which is either\ntruncated (when a SHA-256 submodule is added to a SHA-1 repository) or\npadded with zeros (when a SHA-1 submodule is added to a SHA-256\nrepository). We cannot detect even the latter case because someone\ncould have an actual submodule that actually ends in 24 zeros, which\nwould be a false positive.\n\nSigned-off-by: brian m. carlson \n---\n object-file.c | 6 +++++-\n t/t3700-add.sh | 25 +++++++++++++++++++++++++\n t/t7400-submodule-basic.sh | 25 +++++++++++++++++++++++++\n 3 files changed, 55 insertions(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 4675c8ed6b..8c43c52ed0 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1661,7 +1661,11 @@ int index_path(struct index_state *istate, struct object_id *oid,\n \t\tstrbuf_release(&sb);\n \t\tbreak;\n \tcase S_IFDIR:\n-\t\treturn repo_resolve_gitlink_ref(istate->repo, path, \"HEAD\", oid);\n+\t\tif (repo_resolve_gitlink_ref(istate->repo, path, \"HEAD\", oid))\n+\t\t\treturn -1;\n+\t\tif (&hash_algos[oid->algo] != istate->repo->hash_algo)\n+\t\t\treturn error(_(\"cannot add a submodule of a different hash algorithm\"));\n+\t\tbreak;\n \tdefault:\n \t\treturn error(_(\"%s: unsupported file type\"), path);\n \t}\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex df580a5806..9a2c8dbcc2 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -541,6 +541,31 @@ test_expect_success 'all statuses changed in folder if . is given' '\n \t)\n '\n \n+test_expect_success 'cannot add a submodule of a different algorithm' '\n+\tgit init --object-format=sha256 sha256 &&\n+\t(\n+\t\tcd sha256 &&\n+\t\ttest_commit abc &&\n+\t\tgit init --object-format=sha1 submodule &&\n+\t\ttest_commit -C submodule def &&\n+\t\ttest_must_fail git add submodule 2>err &&\n+\t\ttest_grep \"cannot add a submodule of a different hash algorithm\" err &&\n+\t\tgit ls-files --stage >entries &&\n+\t\ttest_grep ! ^160000 entries\n+\t) &&\n+\tgit init --object-format=sha1 sha1 &&\n+\t(\n+\t\tcd sha1 &&\n+\t\ttest_commit abc &&\n+\t\tgit init --object-format=sha256 submodule &&\n+\t\ttest_commit -C submodule def &&\n+\t\ttest_must_fail git add submodule 2>err &&\n+\t\ttest_grep \"cannot add a submodule of a different hash algorithm\" err &&\n+\t\tgit ls-files --stage >entries &&\n+\t\ttest_grep ! ^160000 entries\n+\t)\n+'\n+\n test_expect_success CASE_INSENSITIVE_FS 'path is case-insensitive' '\n \tpath=\"$(pwd)/BLUB\" &&\n \ttouch \"$path\" &&\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex fd3e7e355e..e6b551daad 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -407,6 +407,31 @@ test_expect_success 'submodule add in subdirectory with relative path should fai\n \ttest_grep toplevel output.err\n '\n \n+test_expect_success 'submodule add of a different algorithm fails' '\n+\tgit init --object-format=sha256 sha256 &&\n+\t(\n+\t\tcd sha256 &&\n+\t\ttest_commit abc &&\n+\t\tgit init --object-format=sha1 submodule &&\n+\t\ttest_commit -C submodule def &&\n+\t\ttest_must_fail git submodule add \"$submodurl\" submodule 2>err &&\n+\t\ttest_grep \"cannot add a submodule of a different hash algorithm\" err &&\n+\t\tgit ls-files --stage >entries &&\n+\t\ttest_grep ! ^160000 entries\n+\t) &&\n+\tgit init --object-format=sha1 sha1 &&\n+\t(\n+\t\tcd sha1 &&\n+\t\ttest_commit abc &&\n+\t\tgit init --object-format=sha256 submodule &&\n+\t\ttest_commit -C submodule def &&\n+\t\ttest_must_fail git submodule add \"$submodurl\" submodule 2>err &&\n+\t\ttest_grep \"cannot add a submodule of a different hash algorithm\" err &&\n+\t\tgit ls-files --stage >entries &&\n+\t\ttest_grep ! ^160000 entries\n+\t)\n+'\n+\n test_expect_success 'setup - add an example entry to .gitmodules' '\n \tgit config --file=.gitmodules submodule.example.url git://example.com/init.git\n '\n"},{"id":"530723","messageId":"20251115015310.GA3499607@coredump.intra.peff.net","threadId":"64469","inReplyTo":"aRe6l5YRCzLAxeuB@fruit.crustytoothpaste.net","subject":"Re: [PATCH] object-file: disallow adding submodules of different hash algo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-15T01:53:10Z","receivedAt":"2025-11-15T01:59:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 14, 2025 at 11:26:15PM +0000, brian m. carlson wrote:\n\n> On 2025-11-13 at 16:29:00, Junio C Hamano wrote:\n> > Jeff King writes:\n> > \n> > > So this is what I'd propose on top of your patch. I can hold onto it for\n> > > later if we don't want to muddy up what you're trying to do.\n> > \n> > I do agree with both of the above. The patch below makes perfect\n> > sense to me, and it is more about the quality of implementation of\n> > this codepath in general, than the primary theme of Brian's changes,\n> > so there is no strong reason they have to come in a single series.\n> \n> That's certainly true, but it still applies cleanly on top of my revised\n> patch and I'll just include it in v2. I'm running tests now and should\n> send out the series later tonight.\n\nThanks, your v2 looks great to me!\n\n-Peff\n"},{"id":"530756","messageId":"xmqqy0o7dqdp.fsf@gitster.g","threadId":"64469","inReplyTo":"20251115005818.2271557-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] object-file: disallow adding submodules of different hash algo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-15T19:53:22Z","receivedAt":"2025-11-15T19:53:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> object-file.c | 6 +++++-\n> t/t3700-add.sh | 25 +++++++++++++++++++++++++\n> t/t7400-submodule-basic.sh | 25 +++++++++++++++++++++++++\n> 3 files changed, 55 insertions(+), 1 deletion(-)\n\nUpdated test reads much nicer. Thanks for updating them.\n"},{"id":"530757","messageId":"xmqqseefdq76.fsf@gitster.g","threadId":"64469","inReplyTo":"20251115005818.2271557-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] read-cache: drop submodule check from add_to_cache()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-15T19:57:17Z","receivedAt":"2025-11-15T19:57:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> From: Jeff King \n>\n> In add_to_cache(), we treat any directories as submodules, and complain\n> if we can't resolve their HEAD. This call to resolve_gitlink_ref() was\n> added by f937bc2f86 (add: error appropriately on repository with no\n> commits, 2019-04-09), with the goal of improving the error message for\n> empty repositories.\n>\n> But we already resolve the submodule HEAD in index_path(), which is\n> where we find the actual oid we're going to use. Resolving it again here\n> introduces some downsides:\n>\n> 1. It's more work, since we have to open up the submodule repository's\n> files twice.\n>\n> 2. There are call paths that get to index_path() without going through\n> add_to_cache(). For instance, we'd want a similar informative\n> message if \"git diff empty\" finds that it can't resolve the\n> submodule's HEAD. (In theory we can also get there through\n> update-index, but AFAICT it refuses to consider directories as\n> submodules at all, and just complains about them).\n>\n> 3. The resolution in index_path() catches more errors that we don't\n> handle here. In particular, it will validate that the object format\n> for the submodule matches that of the superproject. This isn't a\n> bug, since our call in add_to_cache() throws away the oid it gets\n> without looking at it. But it certainly caused confusion for me\n> when looking at where the object-format check should go.\n>\n> So instead of resolving the submodule HEAD in add_to_cache(), let's just\n> teach the call in index_path() to actually produce an error message\n> (which it already does for other cases). That's probably what f937bc2f86\n> should have done in the first place, and it gives us a single point of\n> resolution when adding a submodule to the index.\n>\n> The resulting output is slightly more verbose, as we propagate the error\n> up the call stack, but I think that's OK (and again, matches many other\n> errors we get when indexing fails).\n>\n> I've left the text of the error message as-is, though it is perhaps\n> overly specific. There are many reasons that resolving the submodule\n> HEAD might fail, though outside of corruption or system errors it is\n> probably most likely that the submodule HEAD is simply on an unborn\n> branch.\n>\n> Signed-off-by: Jeff King \n> ---\n\nA tangent.\n\nYou can (!) place your own sign-off after Peff's, as you would want\nto certify\n\nc. The contribution was provided directly to me by some other\n person who certified (a), (b) or (c) and I have not modified\n it.\n\nof the DCO, but it seems that it is optional (which I did not know\nabout---the explanation in SubmittingPatches stops at \"Indeed you\nare encouraged to do so\" without making it a requirement).\n"},{"id":"530759","messageId":"aRjdWunjdMHxpS6Z@fruit.crustytoothpaste.net","threadId":"64469","inReplyTo":"xmqqseefdq76.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] read-cache: drop submodule check from add_to_cache()","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-11-15T20:06:50Z","receivedAt":"2025-11-15T20:06:53Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-11-15 at 19:57:17, Junio C Hamano wrote:\n> A tangent.\n> \n> You can (!) place your own sign-off after Peff's, as you would want\n> to certify\n> \n> c. The contribution was provided directly to me by some other\n> person who certified (a), (b) or (c) and I have not modified\n> it.\n> \n> of the DCO, but it seems that it is optional (which I did not know\n> about---the explanation in SubmittingPatches stops at \"Indeed you\n> are encouraged to do so\" without making it a requirement).\n\nYes, I should have done so; my apologies. Please feel free to add it:\n\nSigned-off-by: brian m. carlson \n\nIf you need, I can send a v3 with that fixed.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"530761","messageId":"aRjfGT_4QT4ANlVx@fruit.crustytoothpaste.net","threadId":"64469","inReplyTo":"b434bcb1-bf6e-4fe7-af2d-8c33348378fa@xiplink.com","subject":"Re: git fails to checkout SHA1 submodule in SHA256 repo with --depth=1","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-11-15T20:14:17Z","receivedAt":"2025-11-15T20:14:19Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-11-14 at 22:55:07, Marc Branchaud wrote:\n> Set up SHA256 mirrors of the SHA1 repos you want to track in submodules?\n> \n> I'm assuming that it would be easy to keep such mirrors up to date. I'm not\n> familiar enough with Brian's work to know if a SHA256 repo can have\n> a SHA1 remote (which would mean that updates would just be a simple \"git\n> fetch origin\"), but even if that's not possible I'm guessing that scripting\n> some kind of regularly-run translation of new commits wouldn't be too hard.\n\nYou can set up a repository which supports both algorithms (for\ninstance, a SHA-256 main algorithm and SHA-1 compatibility) and then\ninteroperate with both algorithms, so yes, this is possible and it\nreally is as easy as `git fetch origin` once set up.\n\nThe current constraint is that the cloned repository must be a full\nclone (not shallow or partial) and it must not have any submodules\nitself.\n\nOf course, that's what the interoperability work does and it's not\nfinished or even mostly upstreamed. As I said, it's mostly functional,\nbut it's not production ready. It also fails many tests because we have\nmany tests in our testsuite that require partial or shallow clones,\nwhose support is in progress.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"530796","messageId":"8263691a45d0a425d24a796c547315932e8c4001.camel@suse.com","threadId":"64469","inReplyTo":"20251115005818.2271557-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] object-file: disallow adding submodules of different hash algo","fromName":"Martin Wilck","fromEmail":"mwilck@suse.com","sentAt":"2025-11-17T08:53:07Z","receivedAt":"2025-11-17T08:53:10Z","isPatch":true,"sender":{"key":"mwilck@suse.com","avatar":null},"body":"On Sat, 2025-11-15 at 00:58 +0000, brian m. carlson wrote:\n> \n> Note that we cannot check this in git fsck because the malformed\n> submodule is stored in the tree as an object ID which is either\n> truncated (when a SHA-256 submodule is added to a SHA-1 repository)\n> or\n> padded with zeros (when a SHA-1 submodule is added to a SHA-256\n> repository).  We cannot detect even the latter case because someone\n> could have an actual submodule that actually ends in 24 zeros, which\n> would be a false positive.\n\nPerhaps a warning could still be printed in this case? After all, the\nprobability for a false positive is about 1:16 million.\n\nThanks\nMartin\n"}]}