{"thread":{"id":"63961","subject":"[PATCH] submodule: truncate the oid when fetchig commits","startedAt":"2025-08-14T15:06:35Z","lastAt":"2025-08-19T00:45:48Z","messageCount":6,"participants":["Michael Schroeder","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"524177","messageId":"aJ37eHEGMw6RgmZC@suse.de","threadId":"63961","inReplyTo":null,"subject":"[PATCH] submodule: truncate the oid when fetchig commits","fromName":"Michael Schroeder","fromEmail":"mls@suse.de","sentAt":"2025-08-14T15:06:32Z","receivedAt":"2025-08-14T15:06:35Z","isPatch":true,"sender":{"key":"mls@suse.de","avatar":"https://gravatar.com/avatar/fddde0fb16b27536a511b2be548de141067467ea22fd0a92725fdd384ce32343?d=mp&s=160"},"body":"If a submodule uses a different hash algorithm than used in\nthe main repository, the recorded submodule commit is padded\nwith zeros. This is usually not a problem as the default is to\ndo submodule clones non-shallow and the commit can be found\nin the local objects.\n\nBut this is not true if the --shallow-submodules clone option is\nused (or the --depth option in the submodule update call).\nIn this case, the commit is often not reachable and a fetch of the\nspecific commit is done. But the fetch cannot deal with the zero\npadding and interprets the commit as a name. Because of this,\nthe checkout will fail.\n\nImplement truncation of the recorded commit to the correct size\ncorresponding to the hash algorithm used in the submodule.\n\nSigned-off-by: Michael Schroeder <mls@suse.de>\n---\n builtin/submodule--helper.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 07a1935cbe..ef21eb42b8 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -72,7 +72,7 @@ static char *resolve_relative_url(const char *rel_url, const char *up_path, int\n \treturn resolved_url;\n }\n \n-static int get_default_remote_submodule(const char *module_path, char **default_remote)\n+static int get_default_remote_submodule(const char *module_path, char **default_remote, const struct git_hash_algo **hash_algo)\n {\n \tconst struct submodule *sub;\n \tstruct repository subrepo;\n@@ -106,6 +106,9 @@ static int get_default_remote_submodule(const char *module_path, char **default_\n \n \t*default_remote = xstrdup(remote_name);\n \n+\tif (hash_algo)\n+\t\t*hash_algo = subrepo.hash_algo;\n+\n \trepo_clear(&subrepo);\n \tfree(url);\n \n@@ -1272,7 +1275,7 @@ static void sync_submodule(const char *path, const char *prefix,\n \t\tgoto cleanup;\n \n \tstrbuf_reset(&sb);\n-\tcode = get_default_remote_submodule(path, &default_remote);\n+\tcode = get_default_remote_submodule(path, &default_remote, NULL);\n \tif (code)\n \t\texit(code);\n \n@@ -2319,16 +2322,19 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,\n \tif (depth)\n \t\tstrvec_pushf(&cp.args, \"--depth=%d\", depth);\n \tif (oid) {\n-\t\tchar *hex = oid_to_hex(oid);\n+\t\tchar hexbuffer[GIT_MAX_HEXSZ + 1];\n+\t\tchar *hex = oid_to_hex_r(hexbuffer, oid);\n \t\tchar *remote;\n+\t\tconst struct git_hash_algo *hash_algo = NULL;\n \t\tint code;\n \n-\t\tcode = get_default_remote_submodule(module_path, &remote);\n+\t\tcode = get_default_remote_submodule(module_path, &remote, &hash_algo);\n \t\tif (code) {\n \t\t\tchild_process_clear(&cp);\n \t\t\treturn code;\n \t\t}\n-\n+\t\tif (hash_algo)\n+\t\t\thex[hash_algo->hexsz] = 0;\t/* truncate to correct size */\n \t\tstrvec_pushl(&cp.args, remote, hex, NULL);\n \t\tfree(remote);\n \t}\n@@ -2635,7 +2641,7 @@ static int update_submodule(struct update_data *update_data)\n \t\tchar *remote_ref;\n \t\tint code;\n \n-\t\tcode = get_default_remote_submodule(update_data->sm_path, &remote_name);\n+\t\tcode = get_default_remote_submodule(update_data->sm_path, &remote_name, NULL);\n \t\tif (code)\n \t\t\treturn code;\n \t\tcode = remote_submodule_branch(update_data->sm_path, &branch);\n-- \n2.50.0\n\n"},{"id":"524180","messageId":"xmqqqzxd6haw.fsf@gitster.g","threadId":"63961","inReplyTo":"aJ37eHEGMw6RgmZC@suse.de","subject":"Re: [PATCH] submodule: truncate the oid when fetchig commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-14T15:42:31Z","receivedAt":"2025-08-14T15:42:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Schroeder <mls@suse.de> writes:\n\n> If a submodule uses a different hash algorithm than used in\n> the main repository, the recorded submodule commit is padded\n> with zeros.\n\nHmph.  I suspect that this \"extra zero bits\" would happen when your\nsuperproject uses a longer hash (e.g. SHA-256) than the hash used by\nthe submodule you are fetching?  If the arrangement is reversed,\nwould we see a different and possibly an even severe problem?\n\nI do not mean to say that you need to solve the problem in both\ndirection at all.  But perhaps\n\n    ... uses a hash algorithm with shorter hash length than what is\n    used in the main repository, ...\n\nwould clarify what problem you are solving in this patch better.\n\n> This is usually not a problem as the default is to\n> do submodule clones non-shallow and the commit can be found\n> in the local objects.\n>\n> But this is not true if the --shallow-submodules clone option is\n> used (or the --depth option in the submodule update call).\n> In this case, the commit is often not reachable and a fetch of the\n> specific commit is done. But the fetch cannot deal with the zero\n> padding and interprets the commit as a name. Because of this,\n> the checkout will fail.\n>\n> Implement truncation of the recorded commit to the correct size\n> corresponding to the hash algorithm used in the submodule.\n\nWhen we use the verb \"truncate\" in the context of this project, I\nthink we always use the word to mean making it shorter than its\nnatural length.  But in the above, the word is used to remove excess\nso that we use it at its natural length.  I cannot offhand offer a\nbetter phrase, so I'll let it go for now, but suggestions for better\nwording is very much welcome.\n\n>  builtin/submodule--helper.c | 18 ++++++++++++------\n>  1 file changed, 12 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 07a1935cbe..ef21eb42b8 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -72,7 +72,7 @@ static char *resolve_relative_url(const char *rel_url, const char *up_path, int\n>  \treturn resolved_url;\n>  }\n>  \n> -static int get_default_remote_submodule(const char *module_path, char **default_remote)\n> +static int get_default_remote_submodule(const char *module_path, char **default_remote, const struct git_hash_algo **hash_algo)\n>  {\n>  \tconst struct submodule *sub;\n>  \tstruct repository subrepo;\n> @@ -106,6 +106,9 @@ static int get_default_remote_submodule(const char *module_path, char **default_\n>  \n>  \t*default_remote = xstrdup(remote_name);\n>  \n> +\tif (hash_algo)\n> +\t\t*hash_algo = subrepo.hash_algo;\n> +\n>  \trepo_clear(&subrepo);\n>  \tfree(url);\n>  \n> @@ -1272,7 +1275,7 @@ static void sync_submodule(const char *path, const char *prefix,\n>  \t\tgoto cleanup;\n>  \n>  \tstrbuf_reset(&sb);\n> -\tcode = get_default_remote_submodule(path, &default_remote);\n> +\tcode = get_default_remote_submodule(path, &default_remote, NULL);\n>  \tif (code)\n>  \t\texit(code);\n>  \n> @@ -2319,16 +2322,19 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,\n>  \tif (depth)\n>  \t\tstrvec_pushf(&cp.args, \"--depth=%d\", depth);\n>  \tif (oid) {\n> -\t\tchar *hex = oid_to_hex(oid);\n> +\t\tchar hexbuffer[GIT_MAX_HEXSZ + 1];\n> +\t\tchar *hex = oid_to_hex_r(hexbuffer, oid);\n>  \t\tchar *remote;\n> +\t\tconst struct git_hash_algo *hash_algo = NULL;\n>  \t\tint code;\n>  \n> -\t\tcode = get_default_remote_submodule(module_path, &remote);\n> +\t\tcode = get_default_remote_submodule(module_path, &remote, &hash_algo);\n\nIt feels highly unsatisfying to have oid_to_hex_r() blindly read and\nconvert oid to hex (assuming the hash length of the current\nrepository) without using the knowledge of how much of that object\nname is valid.  We obtain that knowledge shortly after we call\noid_to_hex_r() in hash_algo here.  That is the only reason why we\nmanually have to truncate below, isn't it?\n\nIn other words, shouldn't we be finding out hash_algo first, and\nthen use something like hash_to_hex_algop_r() to convert only the\nvalid bits of the oid into the buffer?.  That way, we do not have to\nmanually add '\\0' in this function below.\n\n>  \t\tif (code) {\n>  \t\t\tchild_process_clear(&cp);\n>  \t\t\treturn code;\n>  \t\t}\n> -\n> +\t\tif (hash_algo)\n> +\t\t\thex[hash_algo->hexsz] = 0;\t/* truncate to correct size */\n>  \t\tstrvec_pushl(&cp.args, remote, hex, NULL);\n>  \t\tfree(remote);\n>  \t}\n> @@ -2635,7 +2641,7 @@ static int update_submodule(struct update_data *update_data)\n>  \t\tchar *remote_ref;\n>  \t\tint code;\n>  \n> -\t\tcode = get_default_remote_submodule(update_data->sm_path, &remote_name);\n> +\t\tcode = get_default_remote_submodule(update_data->sm_path, &remote_name, NULL);\n>  \t\tif (code)\n>  \t\t\treturn code;\n>  \t\tcode = remote_submodule_branch(update_data->sm_path, &branch);\n"},{"id":"524192","messageId":"aJ5gOPQ9oologqj-@fruit.crustytoothpaste.net","threadId":"63961","inReplyTo":"aJ37eHEGMw6RgmZC@suse.de","subject":"Re: [PATCH] submodule: truncate the oid when fetchig commits","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-08-14T22:16:24Z","receivedAt":"2025-08-14T22:16:26Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-08-14 at 15:06:32, Michael Schroeder wrote:\n> If a submodule uses a different hash algorithm than used in\n> the main repository, the recorded submodule commit is padded\n> with zeros. This is usually not a problem as the default is to\n> do submodule clones non-shallow and the commit can be found\n> in the local objects.\n\nThis should not even work at all.  It may currently behave as you\nsuggest when the main repository is SHA-256 and the submodule is SHA-1,\nbut it will corrupt the data if the submodule is SHA-256 and the main\nrepository is SHA-1, since then the data will be truncated.\n\nThe proper way for this to work is that the SHA-1 version of the\nrepository stores submodules in their SHA-1 states and the SHA-256\nversion of the repository stores submodules in their SHA-256 states.\nYes, this means that you have to convert submodules to the same\nalgorithm, but that's required because trees are binary and not text.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"524337","messageId":"aKLvQlwlTpQec_t1@suse.de","threadId":"63961","inReplyTo":"xmqqqzxd6haw.fsf@gitster.g","subject":"Re: [PATCH v2] submodule: truncate the oid when fetchig commits","fromName":"Michael Schroeder","fromEmail":"mls@suse.de","sentAt":"2025-08-18T09:15:46Z","receivedAt":"2025-08-18T09:15:49Z","isPatch":true,"sender":{"key":"mls@suse.de","avatar":"https://gravatar.com/avatar/fddde0fb16b27536a511b2be548de141067467ea22fd0a92725fdd384ce32343?d=mp&s=160"},"body":"\nHi git team,\n\nhere's the new version of the patch based on your comments:\n\nFrom 3962b9cb065504f52875f728f03ecbf8dc1f23e2 Mon Sep 17 00:00:00 2001\nFrom: Michael Schroeder <mls@suse.de>\nDate: Thu, 14 Aug 2025 16:12:53 +0200\nSubject: [PATCH v2] submodule: use subrepo algo when fetchig commits\n\nIf a submodule uses a hash algorithm with shorter hash length than\nwhat is used in the main repository, the recorded submodule commit\nis padded with zeros. This is usually not a problem as the default\nis to do submodule clones non-shallow and the commit can be found\nin the local objects.\n\nBut this is not true if the --shallow-submodules clone option is\nused (or the --depth option in the submodule update call).\nIn this case, the commit is often not reachable and a fetch of the\nspecific commit is done. But the fetch cannot deal with the zero\npadding and interprets the commit as a name. Because of this,\nthe checkout will fail.\n\nUse the subrepo algorithm when converting the stored commit to hex\nso that the result matches the expected oid length of the submodule.\n\nSigned-off-by: Michael Schroeder <mls@suse.de>\n---\n builtin/submodule--helper.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 07a1935cbe..5c2e96b517 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -72,7 +72,7 @@ static char *resolve_relative_url(const char *rel_url, const char *up_path, int\n \treturn resolved_url;\n }\n \n-static int get_default_remote_submodule(const char *module_path, char **default_remote)\n+static int get_default_remote_submodule(const char *module_path, char **default_remote, const struct git_hash_algo **hash_algo)\n {\n \tconst struct submodule *sub;\n \tstruct repository subrepo;\n@@ -106,6 +106,9 @@ static int get_default_remote_submodule(const char *module_path, char **default_\n \n \t*default_remote = xstrdup(remote_name);\n \n+\tif (hash_algo)\n+\t\t*hash_algo = subrepo.hash_algo;\n+\n \trepo_clear(&subrepo);\n \tfree(url);\n \n@@ -1272,7 +1275,7 @@ static void sync_submodule(const char *path, const char *prefix,\n \t\tgoto cleanup;\n \n \tstrbuf_reset(&sb);\n-\tcode = get_default_remote_submodule(path, &default_remote);\n+\tcode = get_default_remote_submodule(path, &default_remote, NULL);\n \tif (code)\n \t\texit(code);\n \n@@ -2319,16 +2322,18 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet,\n \tif (depth)\n \t\tstrvec_pushf(&cp.args, \"--depth=%d\", depth);\n \tif (oid) {\n-\t\tchar *hex = oid_to_hex(oid);\n+\t\tconst struct git_hash_algo *hash_algo = NULL;\n+\t\tchar *hex;\n \t\tchar *remote;\n \t\tint code;\n \n-\t\tcode = get_default_remote_submodule(module_path, &remote);\n+\t\tcode = get_default_remote_submodule(module_path, &remote, &hash_algo);\n \t\tif (code) {\n \t\t\tchild_process_clear(&cp);\n \t\t\treturn code;\n \t\t}\n \n+\t\thex = hash_to_hex_algop(oid->hash, hash_algo);\n \t\tstrvec_pushl(&cp.args, remote, hex, NULL);\n \t\tfree(remote);\n \t}\n@@ -2635,7 +2640,7 @@ static int update_submodule(struct update_data *update_data)\n \t\tchar *remote_ref;\n \t\tint code;\n \n-\t\tcode = get_default_remote_submodule(update_data->sm_path, &remote_name);\n+\t\tcode = get_default_remote_submodule(update_data->sm_path, &remote_name, NULL);\n \t\tif (code)\n \t\t\treturn code;\n \t\tcode = remote_submodule_branch(update_data->sm_path, &branch);\n-- \n2.50.0\n\n"},{"id":"524339","messageId":"aKLyy8nwGQCT-jvB@suse.de","threadId":"63961","inReplyTo":"aJ5gOPQ9oologqj-@fruit.crustytoothpaste.net","subject":"Re: [PATCH] submodule: truncate the oid when fetchig commits","fromName":"Michael Schroeder","fromEmail":"mls@suse.de","sentAt":"2025-08-18T09:30:51Z","receivedAt":"2025-08-18T09:30:55Z","isPatch":true,"sender":{"key":"mls@suse.de","avatar":"https://gravatar.com/avatar/fddde0fb16b27536a511b2be548de141067467ea22fd0a92725fdd384ce32343?d=mp&s=160"},"body":"On Thu, Aug 14, 2025 at 10:16:24PM +0000, brian m. carlson wrote:\n> On 2025-08-14 at 15:06:32, Michael Schroeder wrote:\n> > If a submodule uses a different hash algorithm than used in\n> > the main repository, the recorded submodule commit is padded\n> > with zeros. This is usually not a problem as the default is to\n> > do submodule clones non-shallow and the commit can be found\n> > in the local objects.\n> \n> This should not even work at all.  It may currently behave as you\n> suggest when the main repository is SHA-256 and the submodule is SHA-1,\n> but it will corrupt the data if the submodule is SHA-256 and the main\n> repository is SHA-1, since then the data will be truncated.\n\nBut it works, and I'm pretty sure people already use it. If you\nhave a sha1 main repo and a sha256 submodule, git will truncate\nthe commit when recording the gitlink. The checkout done by\ngit submodule update will work as it does the normal prefix matching.\n\nIf you have a sha256 main repo and a sha submodule, the recorded\ncommit is padded with zero. The checkout will also work as git\nignores the extra data since commit 52fca06db2 (object-names: support\ninput of oids in any supported hash, 2023-10-01).\n\nWhat doesn't work is if a shallow clone is done for the submodule.\nIn that case the commit is not reachable and git tries a direct\nfetch. This fetch can be made to work if the commit was padded with\nzeros. If the commit was truncated, we would probably need some\nprotocol extension to make the server do a prefix match for a\n\"want\" request.\n\n> The proper way for this to work is that the SHA-1 version of the\n> repository stores submodules in their SHA-1 states and the SHA-256\n> version of the repository stores submodules in their SHA-256 states.\n\nYou mean by using \"compatObjectFormat\"? I couldn't make that work,\nbut maybe I missed something. Anyway, I think this also will not\nwork for shallow clones.\n\nCheers,\n  Michael.\n\n-- \nMichael Schroeder          SUSE Software Solutions Germany GmbH\nmls@suse.de               GF: Ivo Totev HRB 36809, AG Nuernberg\nmain(_){while(_=~getchar())putchar(~_-1/(~(_|32)/13*2-11)*13);}\n"},{"id":"524380","messageId":"aKPJNNWMW9gtueEK@fruit.crustytoothpaste.net","threadId":"63961","inReplyTo":"aKLyy8nwGQCT-jvB@suse.de","subject":"Re: [PATCH] submodule: truncate the oid when fetchig commits","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-08-19T00:45:40Z","receivedAt":"2025-08-19T00:45:48Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-08-18 at 09:30:51, Michael Schroeder wrote:\n> On Thu, Aug 14, 2025 at 10:16:24PM +0000, brian m. carlson wrote:\n> > On 2025-08-14 at 15:06:32, Michael Schroeder wrote:\n> > > If a submodule uses a different hash algorithm than used in\n> > > the main repository, the recorded submodule commit is padded\n> > > with zeros. This is usually not a problem as the default is to\n> > > do submodule clones non-shallow and the commit can be found\n> > > in the local objects.\n> > \n> > This should not even work at all.  It may currently behave as you\n> > suggest when the main repository is SHA-256 and the submodule is SHA-1,\n> > but it will corrupt the data if the submodule is SHA-256 and the main\n> > repository is SHA-1, since then the data will be truncated.\n> \n> But it works, and I'm pretty sure people already use it. If you\n> have a sha1 main repo and a sha256 submodule, git will truncate\n> the commit when recording the gitlink. The checkout done by\n> git submodule update will work as it does the normal prefix matching.\n\nUnfortunately, that will break with the interoperability work.  The\nprotocol will learn to convert the object ID on the server side by\nannouncing the mapping and when the object ID doesn't exist, the client\nwill die because it can't remap the object and the process will fail.\nBy doing that, you'll end up with a repository that you can never use\ninteroperability code on, ever, without rewriting history.  There's no\nway around this problem because we don't keep the object format in\ntrees, so we can't distinguish between a SHA-256 submodule that happens\nto end in 24 zeros and a SHA-1 submodule.\n\nThe entire hash function transition has mandated exactly one object\nformat on disk and in data structures from the very beginning:\n\n    This affects both object names and object content -- both the names\n    of objects and all references to other objects within an object are\n    switched to the new hash function.\n\nI apologize that I didn't think about this problem and make the code die\non this case earlier, but it's not a supported configuration and it will\nabsolutely break in the future.  Sorry to be the bearer of bad news.\n\n> > The proper way for this to work is that the SHA-1 version of the\n> > repository stores submodules in their SHA-1 states and the SHA-256\n> > version of the repository stores submodules in their SHA-256 states.\n> \n> You mean by using \"compatObjectFormat\"? I couldn't make that work,\n> but maybe I missed something. Anyway, I think this also will not\n> work for shallow clones.\n\nThere is interoperability code only for loose objects now.  The code\nthat handles packs and interoperability between repositories is in a\nbranch on my remote.  It's work that I'm doing for a talk at Git Merge\nand I will send it upstream when it's ready.\n\nRight now, I'm working on shallow clones at the moment and then\nsubmodules are next.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"}]}