{"thread":{"id":"64309","subject":"[PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs","startedAt":"2025-10-13T17:47:13Z","lastAt":"2025-10-15T13:59:22Z","messageCount":9,"participants":["Okhuomon Ajayi","Junio C Hamano","brian m. carlson","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"528646","messageId":"20251013174658.236940-1-okhuomonajayi54@gmail.com","threadId":"64309","inReplyTo":null,"subject":"[PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs","fromName":"Okhuomon Ajayi","fromEmail":"okhuomonajayi54@gmail.com","sentAt":"2025-10-13T17:46:58Z","receivedAt":"2025-10-13T17:47:13Z","isPatch":true,"sender":{"key":"okhuomonajayi54@gmail.com","avatar":null},"body":"Patch IDs in Git must always use SHA1, regardless of the repository's\nobject hash. Previously, the code relied on `the_hash_algo` which could\nvary depending on the repository, and included a NEEDSWORK comment\nsuggesting this should be fixed.\n\nThis patch updates the comment to clearly state that SHA1 is required\nfor patch IDs and sets the hash algorithm to SHA1 if it is not already\nset. This ensures consistent computation of patch IDs in accordance\nwith git-patch-id(1).\n\nNo functional behavior is changed, but misleading comments are removed\nand the code now explicitly enforces correct SHA1 usage for patch IDs.\n\nSigned-off-by: Okhuomon Ajayi \n---\n builtin/patch-id.c | 11 +++--------\n 1 file changed, 3 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex d26e9d0c1e..d47b6f5a3f 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -246,16 +246,11 @@ int cmd_patch_id(int argc,\n \t\t\t patch_id_usage, 0);\n \n \t/*\n-\t * We rely on `the_hash_algo` to compute patch IDs. This is dubious as\n-\t * it means that the hash algorithm now depends on the object hash of\n-\t * the repository, even though git-patch-id(1) clearly defines that\n-\t * patch IDs always use SHA1.\n-\t *\n-\t * NEEDSWORK: This hack should be removed in favor of converting\n-\t * the code that computes patch IDs to always use SHA1.\n+\t * Patch IDs must always use SHA1, regardless of the repository's\n+\t * object hash, See git-patch-id(1) for details. \n \t */\n \tif (!the_hash_algo)\n-\t\trepo_set_hash_algo(the_repository, GIT_HASH_DEFAULT);\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n \n \tgenerate_id_list(opts ? opts > 1 : config.stable,\n \t\t\t opts ? opts == 3 : config.verbatim);\n-- \n2.43.0\n\n"},{"id":"528683","messageId":"xmqqecr6yypu.fsf@gitster.g","threadId":"64309","inReplyTo":"20251013174658.236940-1-okhuomonajayi54@gmail.com","subject":"Re: [PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-14T03:00:13Z","receivedAt":"2025-10-14T03:00:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Okhuomon Ajayi writes:\n\n> Patch IDs in Git must always use SHA1, regardless of the repository's\n> object hash. Previously, the code relied on `the_hash_algo` which could\n> vary depending on the repository, and included a NEEDSWORK comment\n> suggesting this should be fixed.\n\nI do not think that is what the comment suggests to do.\n\nRead it again:\n\n ... should be removed in favor of converting the code that\n computes patch IDs to always use SHA1.\n\nThere are code paths that compute patch IDs elsewhere, and they are\nnot immediately below the NEEDSWORK comment. The code relies on\nthe_hash_algo and that is why the \"hack\" makes repo_set_hash_algo()\ncall. The suggestion is to convert that code that hashes patch to\ncompute patch IDs not to use the_hash_algo that is repository\ndependeant. I think get_one_pathcid() function in the same file is\none of them.\n\n> This patch updates the comment to clearly state that SHA1 is required\n> for patch IDs and sets the hash algorithm to SHA1 if it is not already\n> set. This ensures consistent computation of patch IDs in accordance\n> with git-patch-id(1).\n\nAnd if it is already set? I think what your first paragraph claims\nto be problematic is that case, and the patch does not touch that\ncase at all.\n\nBlindly setting the_hash_algo to SHA-1 may not be the end of the\n\"solution\", so whoever wants to work on this needs to be extra\ncareful. If the code after this point, starting from the call to\ngenerate_id_list() we see in the post context, need to touch any Git\nobjects in the current repository (e.g., to obtain patch text or\nsome configuration data), such accesses need to use the hash that\nthe repository uses. Only the final \"now we have this patch, and we\nlearned what the configuration says how we should compute the\npatch-id. Let's hash the patch text following the specified\nalgorithm\" step should use SHA-1 as the hash algorithm.\n\nPerhaps we are lucky that this program has *no* need to access\nobjects in the repository (my quick scan says this seems to work on\nan external text file and does not generate diffs locally out of\nobjects), and it may not depend on configuration data coming from\nany objects in the repository (there are some configuration variables\nwhose values are blob object names that instructs Git to read such\nan object). In such a case, then the solution may be to always\nmake the code ignore the_hash_algo and unconditionally using SHA1.\n"},{"id":"528689","messageId":"CAFpMFfCAzT0MoVhWmkkY9osSgZtHyb_95j=JOV5f3-y2bE2EPQ@mail.gmail.com","threadId":"64309","inReplyTo":"xmqqecr6yypu.fsf@gitster.g","subject":"Re: [PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs","fromName":"Okhuomon Ajayi","fromEmail":"okhuomonajayi54@gmail.com","sentAt":"2025-10-14T08:04:53Z","receivedAt":"2025-10-14T08:05:06Z","isPatch":true,"sender":{"key":"okhuomonajayi54@gmail.com","avatar":null},"body":"Thanks for the explanation!\n I’ll update the patch so it doesn’t touch the_hash_algo globally, and\ninstead uses SHA1 only for the patch-ID computation itself. I’ll also\ntweak the comment to make it clear that this is just part of the\nbigger work to standardize patch-ID handling across Git\n\nOn Tue, Oct 14, 2025 at 4:00 AM Junio C Hamano wrote:\n>\n> Okhuomon Ajayi writes:\n>\n> > Patch IDs in Git must always use SHA1, regardless of the repository's\n> > object hash. Previously, the code relied on `the_hash_algo` which could\n> > vary depending on the repository, and included a NEEDSWORK comment\n> > suggesting this should be fixed.\n>\n> I do not think that is what the comment suggests to do.\n>\n> Read it again:\n>\n> ... should be removed in favor of converting the code that\n> computes patch IDs to always use SHA1.\n>\n> There are code paths that compute patch IDs elsewhere, and they are\n> not immediately below the NEEDSWORK comment. The code relies on\n> the_hash_algo and that is why the \"hack\" makes repo_set_hash_algo()\n> call. The suggestion is to convert that code that hashes patch to\n> compute patch IDs not to use the_hash_algo that is repository\n> dependeant. I think get_one_pathcid() function in the same file is\n> one of them.\n>\n> > This patch updates the comment to clearly state that SHA1 is required\n> > for patch IDs and sets the hash algorithm to SHA1 if it is not already\n> > set. This ensures consistent computation of patch IDs in accordance\n> > with git-patch-id(1).\n>\n> And if it is already set? I think what your first paragraph claims\n> to be problematic is that case, and the patch does not touch that\n> case at all.\n>\n> Blindly setting the_hash_algo to SHA-1 may not be the end of the\n> \"solution\", so whoever wants to work on this needs to be extra\n> careful. If the code after this point, starting from the call to\n> generate_id_list() we see in the post context, need to touch any Git\n> objects in the current repository (e.g., to obtain patch text or\n> some configuration data), such accesses need to use the hash that\n> the repository uses. Only the final \"now we have this patch, and we\n> learned what the configuration says how we should compute the\n> patch-id. Let's hash the patch text following the specified\n> algorithm\" step should use SHA-1 as the hash algorithm.\n>\n> Perhaps we are lucky that this program has *no* need to access\n> objects in the repository (my quick scan says this seems to work on\n> an external text file and does not generate diffs locally out of\n> objects), and it may not depend on configuration data coming from\n> any objects in the repository (there are some configuration variables\n> whose values are blob object names that instructs Git to read such\n> an object). In such a case, then the solution may be to always\n> make the code ignore the_hash_algo and unconditionally using SHA1.\n"},{"id":"528767","messageId":"aO6-LBqhW87GWD-5@fruit.crustytoothpaste.net","threadId":"64309","inReplyTo":"20251013174658.236940-1-okhuomonajayi54@gmail.com","subject":"Re: [PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-10-14T21:18:36Z","receivedAt":"2025-10-14T21:18:44Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-10-13 at 17:46:58, Okhuomon Ajayi wrote:\n> Patch IDs in Git must always use SHA1, regardless of the repository's\n> object hash. Previously, the code relied on `the_hash_algo` which could\n> vary depending on the repository, and included a NEEDSWORK comment\n> suggesting this should be fixed.\n> \n> This patch updates the comment to clearly state that SHA1 is required\n> for patch IDs and sets the hash algorithm to SHA1 if it is not already\n> set. This ensures consistent computation of patch IDs in accordance\n> with git-patch-id(1).\n> \n> No functional behavior is changed, but misleading comments are removed\n> and the code now explicitly enforces correct SHA1 usage for patch IDs.\n> \n> Signed-off-by: Okhuomon Ajayi \n> ---\n> builtin/patch-id.c | 11 +++--------\n> 1 file changed, 3 insertions(+), 8 deletions(-)\n> \n> diff --git a/builtin/patch-id.c b/builtin/patch-id.c\n> index d26e9d0c1e..d47b6f5a3f 100644\n> --- a/builtin/patch-id.c\n> +++ b/builtin/patch-id.c\n> @@ -246,16 +246,11 @@ int cmd_patch_id(int argc,\n> \t\t\t patch_id_usage, 0);\n> \n> \t/*\n> -\t * We rely on `the_hash_algo` to compute patch IDs. This is dubious as\n> -\t * it means that the hash algorithm now depends on the object hash of\n> -\t * the repository, even though git-patch-id(1) clearly defines that\n> -\t * patch IDs always use SHA1.\n> -\t *\n> -\t * NEEDSWORK: This hack should be removed in favor of converting\n> -\t * the code that computes patch IDs to always use SHA1.\n> +\t * Patch IDs must always use SHA1, regardless of the repository's\n> +\t * object hash, See git-patch-id(1) for details. \n> \t */\n> \tif (!the_hash_algo)\n> -\t\trepo_set_hash_algo(the_repository, GIT_HASH_DEFAULT);\n> +\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n\nHmmm. If I run git patch-id in a SHA-256 repository, then I get a\nSHA-256 output here and it's worked this way since Git 2.29.\n\nI know the comment says what it says, but I personally disagree with\nthis approach. There will be a point in time where SHA-1 is so weak as\nto be useless and people will want to build a Git version without it.\nFor instance, many government agencies around the world have a 2030\ndeadline for completely stopping all use of SHA-1. If we continue to\nuse SHA-1 here, then this will have to change anyway in a few years, so\nwe'd be better off keeping the default algorithm for now and adding an\noption to control which hash is used.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"528769","messageId":"xmqqjz0xw20h.fsf@gitster.g","threadId":"64309","inReplyTo":"aO6-LBqhW87GWD-5@fruit.crustytoothpaste.net","subject":"Re: [PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-14T22:29:34Z","receivedAt":"2025-10-14T22:29:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n>> \tif (!the_hash_algo)\n>> -\t\trepo_set_hash_algo(the_repository, GIT_HASH_DEFAULT);\n>> +\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n>\n> Hmmm. If I run git patch-id in a SHA-256 repository, then I get a\n> SHA-256 output here and it's worked this way since Git 2.29.\n>\n> I know the comment says what it says, but I personally disagree with\n> this approach. There will be a point in time where SHA-1 is so weak as\n> to be useless and people will want to build a Git version without it.\n> For instance, many government agencies around the world have a 2030\n> deadline for completely stopping all use of SHA-1. If we continue to\n> use SHA-1 here, then this will have to change anyway in a few years, so\n> we'd be better off keeping the default algorithm for now and adding an\n> option to control which hash is used.\n\nI do not quite agree with that, as SHA-1 in patch-id is merely used\nas \"a hash function with good distribution that we happened to have\nhandy access to\" without any security requirement. Being able to\ncompare patch IDs computed long ago stored somewhere with patch ID\non a patch that claims to be freshly written and find them the same\nto say \"you know, somebody wrote exactly the same patch 7 years ago\"\nwould be valuable, and we do not want to lose it even when you\nhappen to store your payload in a SHA-256 repository.\n"},{"id":"528770","messageId":"aO7Tgj4OJVLhFASW@fruit.crustytoothpaste.net","threadId":"64309","inReplyTo":"xmqqjz0xw20h.fsf@gitster.g","subject":"Re: [PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-10-14T22:49:38Z","receivedAt":"2025-10-14T22:49:40Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-10-14 at 22:29:34, Junio C Hamano wrote:\n> I do not quite agree with that, as SHA-1 in patch-id is merely used\n> as \"a hash function with good distribution that we happened to have\n> handy access to\" without any security requirement. Being able to\n> compare patch IDs computed long ago stored somewhere with patch ID\n> on a patch that claims to be freshly written and find them the same\n> to say \"you know, somebody wrote exactly the same patch 7 years ago\"\n> would be valuable, and we do not want to lose it even when you\n> happen to store your payload in a SHA-256 repository.\n\nI think that's too late, though. We already use SHA-256 in a SHA-256\nrepository, so people already expect that to work now and in the future.\nThe time to make this decision would have been in 2020 with Git 2.29,\nbut we now have people who will be using SHA-256 patch IDs and we need\nto support them.\n\nWe have also specifically discussed in the past people eventually\nwanting to compile Git without SHA-1 support at some point in the future\nfor regulatory or compliance reasons, so we should full well expect that\nto happen and we'll need to be agile about the algorithm. SHA-1 will\ndefinitely disappear from at least some distributions of Git in the\nfuture.\n\nGiven that context, I think allowing the specification of an algorithm\nwould allow people to say, \"Yes, I am in a SHA-256 repository, but I\nwant SHA-1,\" or vice versa, which would work with your use case better.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"528772","messageId":"CAFpMFfCV0-MHDYDuVz81hdvBN8qoyse=Hie1rF5=qPOigPM67Q@mail.gmail.com","threadId":"64309","inReplyTo":"aO7Tgj4OJVLhFASW@fruit.crustytoothpaste.net","subject":"Re: [PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs","fromName":"Okhuomon Ajayi","fromEmail":"okhuomonajayi54@gmail.com","sentAt":"2025-10-14T23:27:58Z","receivedAt":"2025-10-14T23:28:10Z","isPatch":true,"sender":{"key":"okhuomonajayi54@gmail.com","avatar":null},"body":"Hi Junio, Brian,\n\nThanks a lot for the detailed explanations this gave me a much better\nunderstanding of the history behind patch-id and why the hash choice\nisn’t straightforward.\n\nI see now that just forcing SHA-1 isn’t ideal since patch-id already\nuses the repo’s hash in SHA-256 repos. I’ll take another look at how\nthe computation works in the other paths and think about how to handle\nit better, maybe by adding an option or clarifying the behavior.\n\nReally appreciate you both taking the time to explain I’m learning a\nlot from this.\n\nOn Tue, Oct 14, 2025 at 11:49 PM brian m. carlson\n wrote:\n>\n> On 2025-10-14 at 22:29:34, Junio C Hamano wrote:\n> > I do not quite agree with that, as SHA-1 in patch-id is merely used\n> > as \"a hash function with good distribution that we happened to have\n> > handy access to\" without any security requirement. Being able to\n> > compare patch IDs computed long ago stored somewhere with patch ID\n> > on a patch that claims to be freshly written and find them the same\n> > to say \"you know, somebody wrote exactly the same patch 7 years ago\"\n> > would be valuable, and we do not want to lose it even when you\n> > happen to store your payload in a SHA-256 repository.\n>\n> I think that's too late, though. We already use SHA-256 in a SHA-256\n> repository, so people already expect that to work now and in the future.\n> The time to make this decision would have been in 2020 with Git 2.29,\n> but we now have people who will be using SHA-256 patch IDs and we need\n> to support them.\n>\n> We have also specifically discussed in the past people eventually\n> wanting to compile Git without SHA-1 support at some point in the future\n> for regulatory or compliance reasons, so we should full well expect that\n> to happen and we'll need to be agile about the algorithm. SHA-1 will\n> definitely disappear from at least some distributions of Git in the\n> future.\n>\n> Given that context, I think allowing the specification of an algorithm\n> would allow people to say, \"Yes, I am in a SHA-256 repository, but I\n> want SHA-1,\" or vice versa, which would work with your use case better.\n> --\n> brian m. carlson (they/them)\n> Toronto, Ontario, CA\n"},{"id":"528810","messageId":"xmqqbjm8waki.fsf@gitster.g","threadId":"64309","inReplyTo":"aO7Tgj4OJVLhFASW@fruit.crustytoothpaste.net","subject":"Re: [PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-15T13:37:01Z","receivedAt":"2025-10-15T13:37:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Given that context, I think allowing the specification of an algorithm\n> would allow people to say, \"Yes, I am in a SHA-256 repository, but I\n> want SHA-1,\" or vice versa, which would work with your use case better.\n\nThat is sensible.\n\nIn short, the automatic choice is to use the repository's hash\ninside a repository, or use the then-default algorithm (which comes\nfrom the preimage of the patch we discussed in this thread) outside\na repository. We want a \"Use this hash algorithm, ignoring the\nautomatic choice\" command line option that overrides it.\n\nIf we were to do configuration variables, we may need two. One to\nreplace only the fallback part (i.e. outside a repository, instead\nof using whatever then-current algorithm, use this one), and the\nother to act as if the above command line option is always given.\n\nBut as always, starting with only a command line option would be a\nprudent way forward.\n\nThanks.\n"},{"id":"528812","messageId":"0ebb19d7-b8a5-4792-841f-fa5a6b9c4d63@app.fastmail.com","threadId":"64309","inReplyTo":"xmqqbjm8waki.fsf@gitster.g","subject":"Re: [PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-10-15T13:59:00Z","receivedAt":"2025-10-15T13:59:22Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Oct 15, 2025, at 15:37, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n>\n>> Given that context, I think allowing the specification of an algorithm\n>> would allow people to say, \"Yes, I am in a SHA-256 repository, but I\n>> want SHA-1,\" or vice versa, which would work with your use case better.\n>\n> That is sensible.\n>\n> In short, the automatic choice is to use the repository's hash\n> inside a repository, or use the then-default algorithm (which comes\n> from the preimage of the patch we discussed in this thread) outside\n> a repository. We want a \"Use this hash algorithm, ignoring the\n> automatic choice\" command line option that overrides it.\n>\n> If we were to do configuration variables, we may need two. One to\n> replace only the fallback part (i.e. outside a repository, instead\n> of using whatever then-current algorithm, use this one), and the\n> other to act as if the above command line option is always given.\n\nI’ve been wondering. Why does a command which in my impression is only\nuseful for scripting have configuration variables? Shouldn’t plumbing\ncommands in general avoid that since they can end up relying on\nconfiguration state if they end up in scripts?\n\nI want `--stable` so I always try to use that, not the corresponding\nconfiguration variable.\n\n>\n> But as always, starting with only a command line option would be a\n> prudent way forward.\n>\n> Thanks.\n"}]}