{"thread":{"id":"61463","subject":"[PATCH 0/2] Fix use of uninitialized hash algos","startedAt":"2024-05-13T07:15:10Z","lastAt":"2024-05-22T04:51:52Z","messageCount":62,"participants":["Patrick Steinhardt","Junio C Hamano","Kyle Lippincott"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"494564","messageId":"cover.1715582857.git.ps@pks.im","threadId":"61463","inReplyTo":null,"subject":"[PATCH 0/2] Fix use of uninitialized hash algos","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-13T07:15:04Z","receivedAt":"2024-05-13T07:15:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwith c8aed5e8da (repository: stop setting SHA1 as the default object\nhash, 2024-05-07), we have stopped setting up the default hash function\nfor `the_repository`. This change was done so that we stop implicitly\nusing SHA1 in places where we don't really intend to. Instead, code\nwhere we try to access `the_hash_algo` without having `the_repository`\nproperly initialized will now crash hard.\n\nI have found two more cases where this can now be triggered:\n\n - git-patch-id(1) can read diffs from stdin.\n\n - git-hash-object(1) can hash data from stdin.\n\nBoth cases can work without a repository, and if they don't have one\nthey will now crash.\n\nI still consider it a good thing that we did the change regardless of\nthose crashes. In the case of git-patch-id(1) I would claim that using\n`the_hash_algo` is wrong in the first place, as patch IDs should be\nstable and are documented to always use SHA1. Thus, patch IDs in SHA256\nrepos are essentially broken. And in the case of git-hash-object(1), we\nshould expose a command line option to let the user specify the object\nhash. So both cases demonstrate that there is room for improvement.\n\nIf these cases keep on popping up and we don't feel comfortable with it,\nthen we can still decide to drop c8aed5e8da. The remainder of the topic\nthat this commit was part of should in that case stay though, as those\nare real bug fixes. We could then re-try in a subsequent release cycle.\nBut for now I don't think this would be warranted yet.\n\nThis topic depends on js/ps/undecided-is-not-necessarily-sha1 at\nc8aed5e8da (repository: stop setting SHA1 as the default object hash,\n2024-05-07).\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (2):\n builtin/patch-id: fix uninitialized hash function\n builtin/hash-object: fix uninitialized hash function\n\n builtin/hash-object.c | 7 +++++++\n builtin/patch-id.c | 13 +++++++++++++\n t/t1007-hash-object.sh | 6 ++++++\n t/t4204-patch-id.sh | 34 ++++++++++++++++++++++++++++++++++\n 4 files changed, 60 insertions(+)\n\n-- \n2.45.GIT\n\n"},{"id":"494565","messageId":"3200dcad2372fef3bcb4da599b44043bcf5b3692.1715582857.git.ps@pks.im","threadId":"61463","inReplyTo":"cover.1715582857.git.ps@pks.im","subject":"[PATCH 1/2] builtin/patch-id: fix uninitialized hash function","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-13T07:15:09Z","receivedAt":"2024-05-13T07:15:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In c8aed5e8da (repository: stop setting SHA1 as the default object hash,\n2024-05-07), we have adapted `initialize_repository()` to no longer set\nup a default hash function. As this function is also used to set up\n`the_repository`, the consequence is that `the_hash_algo` will now by\ndefault be a `NULL` pointer unless the hash algorithm was configured\nproperly. This is done as a mechanism to detect cases where we may be\nusing the wrong hash function by accident.\n\nThis change now causes git-patch-id(1) to segfault when it's run outside\nof a repository. As this command can read diffs from stdin, it does not\nnecessarily need a repository, but then relies on `the_hash_algo` to\ncompute the patch ID itself.\n\nIt is somewhat dubious that git-patch-id(1) relies on `the_hash_algo` in\nthe first place. Quoting its manpage:\n\n A \"patch ID\" is nothing but a sum of SHA-1 of the file diffs\n associated with a patch, with line numbers ignored. As such, it’s\n \"reasonably stable\", but at the same time also reasonably unique,\n i.e., two patches that have the same \"patch ID\" are almost\n guaranteed to be the same thing.\n\nWe explicitly document patch IDs to be using SHA-1. Furthermore, patch\nIDs are supposed to be stable for most of the part. But even with the\nsame input, the patch IDs will now be different depending on the repo's\nconfigured object hash.\n\nWork around the issue by setting up SHA-1 when there was no startup\nrepository for now. This is arguably not the correct fix, but for now we\nrather want to focus on getting the segfault fixed.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/patch-id.c | 13 +++++++++++++\n t/t4204-patch-id.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 3894d2b970..be5a85e71c 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -5,6 +5,7 @@\n #include \"hash.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n+#include \"setup.h\"\n \n static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)\n {\n@@ -237,6 +238,18 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_patch_id_options,\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 * TODO: This hack should be removed in favor of converting the code\n+\t * that computes patch IDs to always use SHA1.\n+\t */\n+\tif (!startup_info->have_repository)\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 \treturn 0;\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex a7fa94ce0a..605faea0c7 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -310,4 +310,38 @@ test_expect_success 'patch-id handles diffs with one line of before/after' '\n \ttest_config patchid.stable true &&\n \tcalc_patch_id diffu1stable diff <<-\\EOF &&\n+\tdiff --git a/bar b/bar\n+\tindex bdaf90f..31051f6 100644\n+\t--- a/bar\n+\t+++ b/bar\n+\t@@ -2 +2,2 @@\n+\t b\n+\t+c\n+\tEOF\n+\n+\tgit init --object-format=sha1 repo-sha1 &&\n+\tgit -C repo-sha1 patch-id patch-id-sha1 &&\n+\tgit init --object-format=sha256 repo-sha256 &&\n+\tgit -C repo-sha256 patch-id patch-id-sha256 &&\n+\ttest_cmp patch-id-sha1 patch-id-sha256\n+'\n+\n+test_expect_success 'patch-id without repository' '\n+\tcat >diff <<-\\EOF &&\n+\tdiff --git a/bar b/bar\n+\tindex bdaf90f..31051f6 100644\n+\t--- a/bar\n+\t+++ b/bar\n+\t@@ -2 +2,2 @@\n+\t b\n+\t+c\n+\tEOF\n+\tnongit git patch-id \n---\n builtin/hash-object.c | 7 +++++++\n t/t1007-hash-object.sh | 6 ++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 82ca6d2bfd..0855f4f8aa 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -123,6 +123,13 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \telse\n \t\tprefix = setup_git_directory_gently(&nongit);\n \n+\t/*\n+\t * TODO: Allow the hash algorithm to be configured by the user via a\n+\t * command line option when not using `-w`.\n+\t */\n+\tif (nongit)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tif (vpath && prefix) {\n \t\tvpath_free = prefix_filename(prefix, vpath);\n \t\tvpath = vpath_free;\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex 64aea38486..4c138c6ca4 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -260,4 +260,10 @@ test_expect_success '--literally with extra-long type' '\n \techo example | git hash-object -t $t --literally --stdin\n '\n \n+test_expect_success '--stdin outside of repository' '\n+\tnongit git hash-object --stdin actual &&\n+\techo \"$(test_oid hello)\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.45.GIT\n\n"},{"id":"494636","messageId":"xmqqttj1hfb3.fsf@gitster.g","threadId":"61463","inReplyTo":"cover.1715582857.git.ps@pks.im","subject":"Re: [PATCH 0/2] Fix use of uninitialized hash algos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-13T16:01:04Z","receivedAt":"2024-05-13T16:01:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> with c8aed5e8da (repository: stop setting SHA1 as the default object\n> hash, 2024-05-07), we have stopped setting up the default hash function\n> for `the_repository`. This change was done so that we stop implicitly\n> using SHA1 in places where we don't really intend to. Instead, code\n> where we try to access `the_hash_algo` without having `the_repository`\n> properly initialized will now crash hard.\n>\n> I have found two more cases where this can now be triggered:\n>\n> - git-patch-id(1) can read diffs from stdin.\n>\n> - git-hash-object(1) can hash data from stdin.\n>\n> Both cases can work without a repository, and if they don't have one\n> they will now crash.\n\nPerhaps we should double-check with all commands that are designed\nto be able to work outside a repository, e.g. \"git apply\", \"git grep\n--no-index\", \"git diff --no-index\" (tried to be exhausitive without\nconsulting documentation, so the list is not exhausitive at all).\n\n> I still consider it a good thing that we did the change regardless of\n> those crashes. In the case of git-patch-id(1) I would claim that using\n> `the_hash_algo` is wrong in the first place, as patch IDs should be\n> stable and are documented to always use SHA1. Thus, patch IDs in SHA256\n> repos are essentially broken. And in the case of git-hash-object(1), we\n> should expose a command line option to let the user specify the object\n> hash. So both cases demonstrate that there is room for improvement.\n\nIt is good that the topic is kept outside 'master' (and it is in\n'next' to give the topic a bit wider exposure than merely in 'seen'\nand the list archive).\n\nWe may want a test file that explicitly make commands that ought\nto work outside a repository actually run outside a repository,\nmaking use of the GIT_CEILING_DIRECTORIES mechanism, something along\nthe lines of the attached.\n\n t/t1517-outside-repo.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 42 insertions(+)\n\ndiff --git c/t/t1517-outside-repo.sh w/t/t1517-outside-repo.sh\nnew file mode 100755\nindex 0000000000..4c595c2ff7\n--- /dev/null\n+++ w/t/t1517-outside-repo.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='check random commands outside repo'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'set up a non-repo directory and test file' '\n+\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\texport GIT_CEILING_DIRECTORIES &&\n+\tmkdir non-repo &&\n+\t(\n+\t\tcd non-repo &&\n+\t\t# confirm that git does not find a repo\n+\t\ttest_must_fail git rev-parse --git-dir\n+\t) &&\n+\ttest_write_lines one two three four >nums &&\n+\tgit add nums &&\n+\tcp nums nums.old &&\n+\ttest_write_lines five >>nums &&\n+\tgit diff >sample.patch\n+'\n+\n+test_expect_success 'apply a patch outside repository' '\n+\t(\n+\t\tcd non-repo &&\n+\t\tcp ../nums.old nums &&\n+\t\tgit apply ../sample.patch\n+\t) &&\n+\ttest_cmp nums non-repo/nums\n+'\n+\n+test_expect_success 'compute a patch-id outside repository' '\n+\tgit patch-id patch-id.expect &&\n+\t(\n+\t\tcd non-repo &&\n+\t\tgit patch-id <../sample.patch >../patch-id.actual\n+\t) &&\n+\ttest_cmp patch-id.expect patch-id.actual\n+'\n+\n+test_done\n"},{"id":"494644","messageId":"xmqqcyppftjs.fsf@gitster.g","threadId":"61463","inReplyTo":"xmqqttj1hfb3.fsf@gitster.g","subject":"Re: [PATCH 0/2] Fix use of uninitialized hash algos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-13T18:36:23Z","receivedAt":"2024-05-13T18:36:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> I still consider it a good thing that we did the change regardless of\n>> those crashes. In the case of git-patch-id(1) I would claim that using\n>> `the_hash_algo` is wrong in the first place, as patch IDs should be\n>> stable and are documented to always use SHA1. Thus, patch IDs in SHA256\n>> repos are essentially broken. And in the case of git-hash-object(1), we\n>> should expose a command line option to let the user specify the object\n>> hash. So both cases demonstrate that there is room for improvement.\n>\n> It is good that the topic is kept outside 'master' (and it is in\n> 'next' to give the topic a bit wider exposure than merely in 'seen'\n> and the list archive).\n>\n> We may want a test file that explicitly make commands that ought\n> to work outside a repository actually run outside a repository,\n> making use of the GIT_CEILING_DIRECTORIES mechanism, something along\n> the lines of the attached.\n\nAnother thought. Perhaps we should do, in the meantime, the\nattached patch?\n\nWithout your \"fix patch-id\" patch, one of the tests in 1517 will\nfail, and the other fails as there is no fix posted yet, but with\nthe GIT_TEST_DEFAULT_HASH_IS_SHA1 set explicit to 1, they keep\npassing. This way, when we run out tests, we will always leave the\ndefault hash uninitialized to catch more errors, the binary built\nout of our source by default will crash to help our users catch more\nerrors for us, yet when they really really need to, the users can\nset and export GIT_TEST_DEFAULT_HASH_IS_SHA1=1 to keep assuming that\nSHA1 is the hash algorithm when there is no specified.\n\n\n repository.c | 24 ++++++++++++++++++++++++\n t/t1517-outside-repo.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh | 4 ++++\n 3 files changed, 70 insertions(+)\n\ndiff --git c/repository.c w/repository.c\nindex 15c10015b0..6f67966e35 100644\n--- c/repository.c\n+++ w/repository.c\n@@ -26,6 +26,30 @@ void initialize_repository(struct repository *repo)\n \trepo->parsed_objects = parsed_object_pool_new();\n \tALLOC_ARRAY(repo->index, 1);\n \tindex_state_init(repo->index, repo);\n+\n+\t/*\n+\t * Unfortunately, we need to keep this hack around for the time being:\n+\t *\n+\t * - Not setting up the hash algorithm for `the_repository` leads to\n+\t * crashes because `the_hash_algo` is a macro that expands to\n+\t * `the_repository->hash_algo`. So if Git commands try to access\n+\t * `the_hash_algo` without a Git directory we crash.\n+\t *\n+\t * - Setting up the hash algorithm to be SHA1 by default breaks other\n+\t * commands when running with SHA256.\n+\t *\n+\t * This is another point in case why having global state is a bad idea.\n+\t * Eventually, we should remove this hack and stop setting the hash\n+\t * algorithm in this function altogether. Instead, it should only ever\n+\t * be set via our repository setup procedures. But that requires more\n+\t * work.\n+\t *\n+\t * As we discover the code paths that need fixing, we may remove this\n+\t * code completely, but we are not there yet.\n+\t */\n+\tif (repo == the_repository &&\n+\t git_env_bool(\"GIT_TEST_DEFAULT_HASH_IS_SHA1\", 0))\n+\t\trepo_set_hash_algo(repo, GIT_HASH_SHA1);\n }\n \n static void expand_base_dir(char **out, const char *in,\ndiff --git c/t/t1517-outside-repo.sh w/t/t1517-outside-repo.sh\nnew file mode 100755\nindex 0000000000..4c595c2ff7\n--- /dev/null\n+++ w/t/t1517-outside-repo.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='check random commands outside repo'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'set up a non-repo directory and test file' '\n+\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\texport GIT_CEILING_DIRECTORIES &&\n+\tmkdir non-repo &&\n+\t(\n+\t\tcd non-repo &&\n+\t\t# confirm that git does not find a repo\n+\t\ttest_must_fail git rev-parse --git-dir\n+\t) &&\n+\ttest_write_lines one two three four >nums &&\n+\tgit add nums &&\n+\tcp nums nums.old &&\n+\ttest_write_lines five >>nums &&\n+\tgit diff >sample.patch\n+'\n+\n+test_expect_success 'apply a patch outside repository' '\n+\t(\n+\t\tcd non-repo &&\n+\t\tcp ../nums.old nums &&\n+\t\tgit apply ../sample.patch\n+\t) &&\n+\ttest_cmp nums non-repo/nums\n+'\n+\n+test_expect_success 'compute a patch-id outside repository' '\n+\tgit patch-id patch-id.expect &&\n+\t(\n+\t\tcd non-repo &&\n+\t\tgit patch-id <../sample.patch >../patch-id.actual\n+\t) &&\n+\ttest_cmp patch-id.expect patch-id.actual\n+'\n+\n+test_done\ndiff --git c/t/test-lib.sh w/t/test-lib.sh\nindex 79d3e0e7d9..36d311fb4a 100644\n--- c/t/test-lib.sh\n+++ w/t/test-lib.sh\n@@ -542,6 +542,10 @@ export EDITOR\n \n GIT_DEFAULT_HASH=\"${GIT_TEST_DEFAULT_HASH:-sha1}\"\n export GIT_DEFAULT_HASH\n+\n+GIT_TEST_DEFAULT_HASH_IS_SHA1=0\n+export GIT_TEST_DEFAULT_HASH_IS_SHA1\n+\n GIT_DEFAULT_REF_FORMAT=\"${GIT_TEST_DEFAULT_REF_FORMAT:-files}\"\n export GIT_DEFAULT_REF_FORMAT\n GIT_TEST_MERGE_ALGORITHM=\"${GIT_TEST_MERGE_ALGORITHM:-ort}\"\n"},{"id":"494653","messageId":"20240513192112.866021-1-gitster@pobox.com","threadId":"61463","inReplyTo":"cover.1715582857.git.ps@pks.im","subject":"[PATCH v2 0/4] Fix use of uninitialized hash algorithms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-13T19:21:08Z","receivedAt":"2024-05-13T19:21:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The series adds an escape hatch to \"the default hash algorithm is\nno longer SHA-1; it is an error to rely on the default\" by\nintroducing an undocumented GIT_TEST_DEFAULT_HASH_IS_SHA1=1\nenvironment variable that users can set after reporting an failure\nthey saw to us.\n\nWe can see that Patrick's two fixes to patch-id and hash-object\ncorrects the breakage as the series progresses.\n\nJunio C Hamano (2):\n setup: add an escape hatch for \"no more default hash algorithm\" change\n t1517: test commands that are designed to be run outside repository\n\nPatrick Steinhardt (2):\n builtin/patch-id: fix uninitialized hash function\n builtin/hash-object: fix uninitialized hash function\n\n builtin/hash-object.c | 7 +++++\n builtin/patch-id.c | 13 +++++++++\n repository.c | 24 ++++++++++++++++\n t/t1007-hash-object.sh | 6 ++++\n t/t1517-outside-repo.sh | 61 +++++++++++++++++++++++++++++++++++++++++\n t/t4204-patch-id.sh | 34 +++++++++++++++++++++++\n t/test-lib.sh | 4 +++\n 7 files changed, 149 insertions(+)\n create mode 100755 t/t1517-outside-repo.sh\n\n-- \n2.45.0-145-g3e4a232f6e\n\n"},{"id":"494654","messageId":"20240513192112.866021-2-gitster@pobox.com","threadId":"61463","inReplyTo":"20240513192112.866021-1-gitster@pobox.com","subject":"[PATCH v2 1/4] setup: add an escape hatch for \"no more default hash algorithm\" change","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-13T19:21:09Z","receivedAt":"2024-05-13T19:21:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Partially revert c8aed5e8 (repository: stop setting SHA1 as the\ndefault object hash, 2024-05-07), to keep end-user systems still\nbroken when we have gap in our test coverage but yet give them an\nescape hatch to set the GIT_TEST_DEFAULT_HASH_IS_SHA1=1 environment\nvariable to revert to the previous behaviour.\n\nSigned-off-by: Junio C Hamano \n---\n repository.c | 24 ++++++++++++++++++++++++\n t/test-lib.sh | 4 ++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/repository.c b/repository.c\nindex 15c10015b0..6f67966e35 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -26,6 +26,30 @@ void initialize_repository(struct repository *repo)\n \trepo->parsed_objects = parsed_object_pool_new();\n \tALLOC_ARRAY(repo->index, 1);\n \tindex_state_init(repo->index, repo);\n+\n+\t/*\n+\t * Unfortunately, we need to keep this hack around for the time being:\n+\t *\n+\t * - Not setting up the hash algorithm for `the_repository` leads to\n+\t * crashes because `the_hash_algo` is a macro that expands to\n+\t * `the_repository->hash_algo`. So if Git commands try to access\n+\t * `the_hash_algo` without a Git directory we crash.\n+\t *\n+\t * - Setting up the hash algorithm to be SHA1 by default breaks other\n+\t * commands when running with SHA256.\n+\t *\n+\t * This is another point in case why having global state is a bad idea.\n+\t * Eventually, we should remove this hack and stop setting the hash\n+\t * algorithm in this function altogether. Instead, it should only ever\n+\t * be set via our repository setup procedures. But that requires more\n+\t * work.\n+\t *\n+\t * As we discover the code paths that need fixing, we may remove this\n+\t * code completely, but we are not there yet.\n+\t */\n+\tif (repo == the_repository &&\n+\t git_env_bool(\"GIT_TEST_DEFAULT_HASH_IS_SHA1\", 0))\n+\t\trepo_set_hash_algo(repo, GIT_HASH_SHA1);\n }\n \n static void expand_base_dir(char **out, const char *in,\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 79d3e0e7d9..36d311fb4a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -542,6 +542,10 @@ export EDITOR\n \n GIT_DEFAULT_HASH=\"${GIT_TEST_DEFAULT_HASH:-sha1}\"\n export GIT_DEFAULT_HASH\n+\n+GIT_TEST_DEFAULT_HASH_IS_SHA1=0\n+export GIT_TEST_DEFAULT_HASH_IS_SHA1\n+\n GIT_DEFAULT_REF_FORMAT=\"${GIT_TEST_DEFAULT_REF_FORMAT:-files}\"\n export GIT_DEFAULT_REF_FORMAT\n GIT_TEST_MERGE_ALGORITHM=\"${GIT_TEST_MERGE_ALGORITHM:-ort}\"\n-- \n2.45.0-145-g3e4a232f6e\n\n"},{"id":"494655","messageId":"20240513192112.866021-3-gitster@pobox.com","threadId":"61463","inReplyTo":"20240513192112.866021-1-gitster@pobox.com","subject":"[PATCH v2 2/4] t1517: test commands that are designed to be run outside repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-13T19:21:10Z","receivedAt":"2024-05-13T19:21:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A few commands, like \"git apply\" and \"git patch-id\", have been\nbroken with a recent change to stop setting the default hash\nalgorithm to SHA-1. Test them and fix them in later commits.\n\nSigned-off-by: Junio C Hamano \n---\n t/t1517-outside-repo.sh | 61 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 61 insertions(+)\n create mode 100755 t/t1517-outside-repo.sh\n\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nnew file mode 100755\nindex 0000000000..e0fd495ec1\n--- /dev/null\n+++ b/t/t1517-outside-repo.sh\n@@ -0,0 +1,61 @@\n+#!/bin/sh\n+\n+test_description='check random commands outside repo'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'set up a non-repo directory and test file' '\n+\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\texport GIT_CEILING_DIRECTORIES &&\n+\tmkdir non-repo &&\n+\t(\n+\t\tcd non-repo &&\n+\t\t# confirm that git does not find a repo\n+\t\ttest_must_fail git rev-parse --git-dir\n+\t) &&\n+\ttest_write_lines one two three four >nums &&\n+\tgit add nums &&\n+\tcp nums nums.old &&\n+\ttest_write_lines five >>nums &&\n+\tgit diff >sample.patch\n+'\n+\n+test_expect_failure 'compute a patch-id outside repository' '\n+\tgit patch-id patch-id.expect &&\n+\t(\n+\t\tcd non-repo &&\n+\t\tgit patch-id <../sample.patch >../patch-id.actual\n+\t) &&\n+\ttest_cmp patch-id.expect patch-id.actual\n+'\n+\n+test_expect_failure 'hash-object outside repository' '\n+\tgit hash-object --stdin hash.expect &&\n+\t(\n+\t\tcd non-repo &&\n+\t\tgit hash-object --stdin <../sample.patch >../hash.actual\n+\t) &&\n+\ttest_cmp hash.expect hash.actual\n+'\n+\n+test_expect_failure 'apply a patch outside repository' '\n+\t(\n+\t\tcd non-repo &&\n+\t\tcp ../nums.old nums &&\n+\t\tgit apply ../sample.patch\n+\t) &&\n+\ttest_cmp nums non-repo/nums\n+'\n+\n+test_expect_success 'grep outside repository' '\n+\tgit grep --cached two >expect &&\n+\t(\n+\t\tcd non-repo &&\n+\t\tcp ../nums.old nums &&\n+\t\tgit grep --no-index two >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.45.0-145-g3e4a232f6e\n\n"},{"id":"494656","messageId":"20240513192112.866021-4-gitster@pobox.com","threadId":"61463","inReplyTo":"20240513192112.866021-1-gitster@pobox.com","subject":"[PATCH v2 3/4] builtin/patch-id: fix uninitialized hash function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-13T19:21:11Z","receivedAt":"2024-05-13T19:21:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Patrick Steinhardt \n\nIn c8aed5e8da (repository: stop setting SHA1 as the default object hash,\n2024-05-07), we have adapted `initialize_repository()` to no longer set\nup a default hash function. As this function is also used to set up\n`the_repository`, the consequence is that `the_hash_algo` will now by\ndefault be a `NULL` pointer unless the hash algorithm was configured\nproperly. This is done as a mechanism to detect cases where we may be\nusing the wrong hash function by accident.\n\nThis change now causes git-patch-id(1) to segfault when it's run outside\nof a repository. As this command can read diffs from stdin, it does not\nnecessarily need a repository, but then relies on `the_hash_algo` to\ncompute the patch ID itself.\n\nIt is somewhat dubious that git-patch-id(1) relies on `the_hash_algo` in\nthe first place. Quoting its manpage:\n\n A \"patch ID\" is nothing but a sum of SHA-1 of the file diffs\n associated with a patch, with line numbers ignored. As such, it’s\n \"reasonably stable\", but at the same time also reasonably unique,\n i.e., two patches that have the same \"patch ID\" are almost\n guaranteed to be the same thing.\n\nWe explicitly document patch IDs to be using SHA-1. Furthermore, patch\nIDs are supposed to be stable for most of the part. But even with the\nsame input, the patch IDs will now be different depending on the repo's\nconfigured object hash.\n\nWork around the issue by setting up SHA-1 when there was no startup\nrepository for now. This is arguably not the correct fix, but for now we\nrather want to focus on getting the segfault fixed.\n\nSigned-off-by: Patrick Steinhardt \nSigned-off-by: Junio C Hamano \n---\n builtin/patch-id.c | 13 +++++++++++++\n t/t1517-outside-repo.sh | 2 +-\n t/t4204-patch-id.sh | 34 ++++++++++++++++++++++++++++++++++\n 3 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 3894d2b970..be5a85e71c 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -5,6 +5,7 @@\n #include \"hash.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n+#include \"setup.h\"\n \n static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)\n {\n@@ -237,6 +238,18 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_patch_id_options,\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 * TODO: This hack should be removed in favor of converting the code\n+\t * that computes patch IDs to always use SHA1.\n+\t */\n+\tif (!startup_info->have_repository)\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 \treturn 0;\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex e0fd495ec1..ac5f3191cc 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -21,7 +21,7 @@ test_expect_success 'set up a non-repo directory and test file' '\n \tgit diff >sample.patch\n '\n \n-test_expect_failure 'compute a patch-id outside repository' '\n+test_expect_success 'compute a patch-id outside repository' '\n \tgit patch-id patch-id.expect &&\n \t(\n \t\tcd non-repo &&\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex a7fa94ce0a..605faea0c7 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -310,4 +310,38 @@ test_expect_success 'patch-id handles diffs with one line of before/after' '\n \ttest_config patchid.stable true &&\n \tcalc_patch_id diffu1stable diff <<-\\EOF &&\n+\tdiff --git a/bar b/bar\n+\tindex bdaf90f..31051f6 100644\n+\t--- a/bar\n+\t+++ b/bar\n+\t@@ -2 +2,2 @@\n+\t b\n+\t+c\n+\tEOF\n+\n+\tgit init --object-format=sha1 repo-sha1 &&\n+\tgit -C repo-sha1 patch-id patch-id-sha1 &&\n+\tgit init --object-format=sha256 repo-sha256 &&\n+\tgit -C repo-sha256 patch-id patch-id-sha256 &&\n+\ttest_cmp patch-id-sha1 patch-id-sha256\n+'\n+\n+test_expect_success 'patch-id without repository' '\n+\tcat >diff <<-\\EOF &&\n+\tdiff --git a/bar b/bar\n+\tindex bdaf90f..31051f6 100644\n+\t--- a/bar\n+\t+++ b/bar\n+\t@@ -2 +2,2 @@\n+\t b\n+\t+c\n+\tEOF\n+\tnongit git patch-id \n\nThe git-hash-object(1) command allows users to hash an object even\nwithout a repository. Starting with c8aed5e8da (repository: stop setting\nSHA1 as the default object hash, 2024-05-07), this will make us hit an\nuninitialized hash function, which subsequently leads to a segfault.\n\nFix this by falling back to SHA-1 explicitly when running outside of a\nGit repository. Eventually, we should expose this function as a command\nline option to the users so that they can pick which object hash to use\nby themselves.\n\nSigned-off-by: Patrick Steinhardt \nSigned-off-by: Junio C Hamano \n---\n builtin/hash-object.c | 7 +++++++\n t/t1007-hash-object.sh | 6 ++++++\n t/t1517-outside-repo.sh | 2 +-\n 3 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 82ca6d2bfd..0855f4f8aa 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -123,6 +123,13 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \telse\n \t\tprefix = setup_git_directory_gently(&nongit);\n \n+\t/*\n+\t * TODO: Allow the hash algorithm to be configured by the user via a\n+\t * command line option when not using `-w`.\n+\t */\n+\tif (nongit)\n+\t\trepo_set_hash_algo(the_repository, GIT_HASH_SHA1);\n+\n \tif (vpath && prefix) {\n \t\tvpath_free = prefix_filename(prefix, vpath);\n \t\tvpath = vpath_free;\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex 64aea38486..4c138c6ca4 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -260,4 +260,10 @@ test_expect_success '--literally with extra-long type' '\n \techo example | git hash-object -t $t --literally --stdin\n '\n \n+test_expect_success '--stdin outside of repository' '\n+\tnongit git hash-object --stdin actual &&\n+\techo \"$(test_oid hello)\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex ac5f3191cc..854bb8f343 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -30,7 +30,7 @@ test_expect_success 'compute a patch-id outside repository' '\n \ttest_cmp patch-id.expect patch-id.actual\n '\n \n-test_expect_failure 'hash-object outside repository' '\n+test_expect_success 'hash-object outside repository' '\n \tgit hash-object --stdin hash.expect &&\n \t(\n \t\tcd non-repo &&\n-- \n2.45.0-145-g3e4a232f6e\n\n"},{"id":"494659","messageId":"CAO_smVhxCGUD2D5m9eDj+CBmteauBuGG=6RgjADnuXgBeB5O8A@mail.gmail.com","threadId":"61463","inReplyTo":"20240513192112.866021-2-gitster@pobox.com","subject":"Re: [PATCH v2 1/4] setup: add an escape hatch for \"no more default hash algorithm\" change","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-05-13T19:48:05Z","receivedAt":"2024-05-13T19:48:18Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Mon, May 13, 2024 at 12:21 PM Junio C Hamano wrote:\n>\n> Partially revert c8aed5e8 (repository: stop setting SHA1 as the\n> default object hash, 2024-05-07), to keep end-user systems still\n> broken when we have gap in our test coverage but yet give them an\n> escape hatch to set the GIT_TEST_DEFAULT_HASH_IS_SHA1=1 environment\n> variable to revert to the previous behaviour.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> repository.c | 24 ++++++++++++++++++++++++\n\nNice idea, this concept LG.\n\n> t/test-lib.sh | 4 ++++\n> 2 files changed, 28 insertions(+)\n>\n> diff --git a/repository.c b/repository.c\n> index 15c10015b0..6f67966e35 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -26,6 +26,30 @@ void initialize_repository(struct repository *repo)\n> repo->parsed_objects = parsed_object_pool_new();\n> ALLOC_ARRAY(repo->index, 1);\n> index_state_init(repo->index, repo);\n> +\n> + /*\n> + * Unfortunately, we need to keep this hack around for the time being:\n\nNit: the comment says we need to keep it around, but it's actually\ndefaulted to off. We may want to add clarification of the current\nstatus to the comment (or replace the entire comment with a comment\ndescribing that we added a hack to the hack, and that it should be\nremoved \"soon\").\n\n> + *\n> + * - Not setting up the hash algorithm for `the_repository` leads to\n> + * crashes because `the_hash_algo` is a macro that expands to\n> + * `the_repository->hash_algo`. So if Git commands try to access\n> + * `the_hash_algo` without a Git directory we crash.\n> + *\n> + * - Setting up the hash algorithm to be SHA1 by default breaks other\n> + * commands when running with SHA256.\n> + *\n> + * This is another point in case why having global state is a bad idea.\n> + * Eventually, we should remove this hack and stop setting the hash\n> + * algorithm in this function altogether. Instead, it should only ever\n> + * be set via our repository setup procedures. But that requires more\n> + * work.\n> + *\n> + * As we discover the code paths that need fixing, we may remove this\n> + * code completely, but we are not there yet.\n> + */\n> + if (repo == the_repository &&\n> + git_env_bool(\"GIT_TEST_DEFAULT_HASH_IS_SHA1\", 0))\n> + repo_set_hash_algo(repo, GIT_HASH_SHA1);\n> }\n>\n> static void expand_base_dir(char **out, const char *in,\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 79d3e0e7d9..36d311fb4a 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -542,6 +542,10 @@ export EDITOR\n>\n> GIT_DEFAULT_HASH=\"${GIT_TEST_DEFAULT_HASH:-sha1}\"\n> export GIT_DEFAULT_HASH\n> +\n> +GIT_TEST_DEFAULT_HASH_IS_SHA1=0\n\nThis is the default as being established in this commit, correct?\nShould we add a comment saying this is the default and describe why we\nhave it here anyway?\n\n> +export GIT_TEST_DEFAULT_HASH_IS_SHA1\n> +\n> GIT_DEFAULT_REF_FORMAT=\"${GIT_TEST_DEFAULT_REF_FORMAT:-files}\"\n> export GIT_DEFAULT_REF_FORMAT\n> GIT_TEST_MERGE_ALGORITHM=\"${GIT_TEST_MERGE_ALGORITHM:-ort}\"\n> --\n> 2.45.0-145-g3e4a232f6e\n>\n>\n"},{"id":"494660","messageId":"CAO_smVg7mdQHp+sg4-vVUEOVRBEoZjqXZu=Kk8PuYc9mtYeMXg@mail.gmail.com","threadId":"61463","inReplyTo":"20240513192112.866021-3-gitster@pobox.com","subject":"Re: [PATCH v2 2/4] t1517: test commands that are designed to be run outside repository","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-05-13T19:57:11Z","receivedAt":"2024-05-13T19:57:24Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Mon, May 13, 2024 at 12:21 PM Junio C Hamano wrote:\n>\n> A few commands, like \"git apply\" and \"git patch-id\", have been\n> broken with a recent change to stop setting the default hash\n> algorithm to SHA-1. Test them and fix them in later commits.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> t/t1517-outside-repo.sh | 61 +++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 61 insertions(+)\n> create mode 100755 t/t1517-outside-repo.sh\n>\n> diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\n> new file mode 100755\n> index 0000000000..e0fd495ec1\n> --- /dev/null\n> +++ b/t/t1517-outside-repo.sh\n> @@ -0,0 +1,61 @@\n> +#!/bin/sh\n> +\n> +test_description='check random commands outside repo'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'set up a non-repo directory and test file' '\n> + GIT_CEILING_DIRECTORIES=$(pwd) &&\n> + export GIT_CEILING_DIRECTORIES &&\n> + mkdir non-repo &&\n> + (\n> + cd non-repo &&\n> + # confirm that git does not find a repo\n> + test_must_fail git rev-parse --git-dir\n> + ) &&\n> + test_write_lines one two three four >nums &&\n> + git add nums &&\n> + cp nums nums.old &&\n> + test_write_lines five >>nums &&\n> + git diff >sample.patch\n> +'\n> +\n> +test_expect_failure 'compute a patch-id outside repository' '\n\nDo we only expect failure because of a temporary condition (the bug\nthat is mentioned in the commit message)? If so, we should probably\nadd a TODO, FIXME, or some other similar style of comment that\ndescribes that this should be fixed. This way, if the patch series to\nfix the issue doesn't materialize, people don't read the test file and\nthink that these commands aren't supported outside of a repository.\n\nDo we have a way of catching the specific failure mode? i.e. if it\ncrashes, is there a test_expect_crash? I'm thinking that it might be\nnice to be more specific about what kind of failure we expect, this\nway if it fails in a different way before we convert these to\ntest_expect_success, the test fails (due to the unexpected change).\nI'm assuming that for crashes of this type there's no good/portable\nway of verifying that it's a specific crash, but having the test check\nfor a difference between an exit code that indicates a signal was\nraised and an exit code that indicates that the process returned\n\"naturally\" after an unsuccessful execution might be feasible, if we\nalready have such a mechanism. Adding one just for this series doesn't\nseem justified.\n\n> + git patch-id patch-id.expect &&\n> + (\n> + cd non-repo &&\n> + git patch-id <../sample.patch >../patch-id.actual\n> + ) &&\n> + test_cmp patch-id.expect patch-id.actual\n> +'\n> +\n> +test_expect_failure 'hash-object outside repository' '\n> + git hash-object --stdin hash.expect &&\n> + (\n> + cd non-repo &&\n> + git hash-object --stdin <../sample.patch >../hash.actual\n> + ) &&\n> + test_cmp hash.expect hash.actual\n> +'\n> +\n> +test_expect_failure 'apply a patch outside repository' '\n> + (\n> + cd non-repo &&\n> + cp ../nums.old nums &&\n> + git apply ../sample.patch\n> + ) &&\n> + test_cmp nums non-repo/nums\n> +'\n> +\n> +test_expect_success 'grep outside repository' '\n> + git grep --cached two >expect &&\n> + (\n> + cd non-repo &&\n> + cp ../nums.old nums &&\n> + git grep --no-index two >../actual\n> + ) &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_done\n> --\n> 2.45.0-145-g3e4a232f6e\n>\n>\n"},{"id":"494663","messageId":"xmqqy18de9kq.fsf@gitster.g","threadId":"61463","inReplyTo":"CAO_smVg7mdQHp+sg4-vVUEOVRBEoZjqXZu=Kk8PuYc9mtYeMXg@mail.gmail.com","subject":"Re: [PATCH v2 2/4] t1517: test commands that are designed to be run outside repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-13T20:33:09Z","receivedAt":"2024-05-13T20:33:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott writes:\n\n> Do we only expect failure because of a temporary condition (the bug\n> that is mentioned in the commit message)? If so, we should probably\n> add a TODO, FIXME, or some other similar style of comment that\n> describes that this should be fixed.\n\ntest_expect_failure is description enough for that purpose.\n\nIf a command should NOT work outside the project we will write a\ntest more like so:\n\n\ttest_expect_success 'foo does not work outside' '\n\t\t... prepare that $cwd is outside ...\n\t\ttest_must_fail git foo\n\t'\n"},{"id":"494664","messageId":"xmqqseyle8aw.fsf@gitster.g","threadId":"61463","inReplyTo":"xmqqy18de9kq.fsf@gitster.g","subject":"Re: [PATCH v2 2/4] t1517: test commands that are designed to be run outside repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-13T21:00:39Z","receivedAt":"2024-05-13T21:00:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Kyle Lippincott writes:\n>\n>> Do we only expect failure because of a temporary condition (the bug\n>> that is mentioned in the commit message)? If so, we should probably\n>> add a TODO, FIXME, or some other similar style of comment that\n>> describes that this should be fixed.\n>\n> test_expect_failure is description enough for that purpose.\n\nWe say this in t/README:\n\n - test_expect_failure []