Fix use of uninitialized hash algos

Hi,

with c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), we have stopped setting up the default hash function for the_repository. This change was done so that we stop implicitly using SHA1 in places where we don't really intend to. Instead, code where we try to access the_hash_algo without having the_repository properly initialized will now crash hard.

I have found two more cases where this can now be triggered:

- git-patch-id(1) can read diffs from stdin.

- git-hash-object(1) can hash data from stdin.

Both cases can work without a repository, and if they don't have one they will now crash.

I still consider it a good thing that we did the change regardless of those crashes. In the case of git-patch-id(1) I would claim that using the_hash_algo is wrong in the first place, as patch IDs should be stable and are documented to always use SHA1. Thus, patch IDs in SHA256 repos are essentially broken. And in the case of git-hash-object(1), we should expose a command line option to let the user specify the object hash. So both cases demonstrate that there is room for improvement.

If these cases keep on popping up and we don't feel comfortable with it, then we can still decide to drop c8aed5e8da. The remainder of the topic that this commit was part of should in that case stay though, as those are real bug fixes. We could then re-try in a subsequent release cycle. But for now I don't think this would be warranted yet.

This topic depends on js/ps/undecided-is-not-necessarily-sha1 at c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07).

Thanks!

Patrick

Patrick Steinhardt (2):
builtin/patch-id: fix uninitialized hash function builtin/hash-object: fix uninitialized hash function

 builtin/hash-object.c  |  7 +++++++
 builtin/patch-id.c     | 13 +++++++++++++
 t/t1007-hash-object.sh |  6 ++++++
 t/t4204-patch-id.sh    | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+)

2.45.GIT

In c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), we have adapted initialize_repository() to no longer set up a default hash function. As this function is also used to set up the_repository, the consequence is that the_hash_algo will now by default be a NULL pointer unless the hash algorithm was configured properly. This is done as a mechanism to detect cases where we may be using the wrong hash function by accident.

This change now causes git-patch-id(1) to segfault when it's run outside of a repository. As this command can read diffs from stdin, it does not necessarily need a repository, but then relies on the_hash_algo to compute the patch ID itself.

It is somewhat dubious that git-patch-id(1) relies on the_hash_algo in the first place. Quoting its manpage:

A "patch ID" is nothing but a sum of SHA-1 of the file diffs associated with a patch, with line numbers ignored. As such, it’s "reasonably stable", but at the same time also reasonably unique, i.e., two patches that have the same "patch ID" are almost guaranteed to be the same thing.

We explicitly document patch IDs to be using SHA-1. Furthermore, patch IDs are supposed to be stable for most of the part. But even with the same input, the patch IDs will now be different depending on the repo's configured object hash.

Work around the issue by setting up SHA-1 when there was no startup repository for now. This is arguably not the correct fix, but for now we rather want to focus on getting the segfault fixed.

---
 builtin/patch-id.c  | 13 +++++++++++++
 t/t4204-patch-id.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

index 3894d2b970..be5a85e71c 100644
@@ -5,6 +5,7 @@
 #include "hash.h"
 #include "hex.h"
 #include "parse-options.h"
+#include "setup.h"
 
 static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)
 {

@@ -237,6 +238,18 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_patch_id_options,
 			     patch_id_usage, 0);
 
+	/*
+	 * We rely on `the_hash_algo` to compute patch IDs. This is dubious as
+	 * it means that the hash algorithm now depends on the object hash of
+	 * the repository, even though git-patch-id(1) clearly defines that
+	 * patch IDs always use SHA1.
+	 *
+	 * TODO: This hack should be removed in favor of converting the code
+	 *       that computes patch IDs to always use SHA1.
+	 */
+	if (!startup_info->have_repository)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	generate_id_list(opts ? opts > 1 : config.stable,
 			 opts ? opts == 3 : config.verbatim);
 	return 0;

index a7fa94ce0a..605faea0c7 100755
@@ -310,4 +310,38 @@ test_expect_success 'patch-id handles diffs with one line of before/after' '
 	test_config patchid.stable true &&
 	calc_patch_id diffu1stable <diffu1
 '
+
+test_expect_failure 'patch-id computes same ID with different object hashes' '
+	test_when_finished "rm -rf repo-sha1 repo-sha256" &&
+
+	cat >diff <<-\EOF &&
+	diff --git a/bar b/bar
+	index bdaf90f..31051f6 100644
+	--- a/bar
+	+++ b/bar
+	@@ -2 +2,2 @@
+	 b
+	+c
+	EOF
+
+	git init --object-format=sha1 repo-sha1 &&
+	git -C repo-sha1 patch-id <diff >patch-id-sha1 &&
+	git init --object-format=sha256 repo-sha256 &&
+	git -C repo-sha256 patch-id <diff >patch-id-sha256 &&
+	test_cmp patch-id-sha1 patch-id-sha256
+'
+
+test_expect_success 'patch-id without repository' '
+	cat >diff <<-\EOF &&
+	diff --git a/bar b/bar
+	index bdaf90f..31051f6 100644
+	--- a/bar
+	+++ b/bar
+	@@ -2 +2,2 @@
+	 b
+	+c
+	EOF
+	nongit git patch-id <diff
+'
+
 test_done

2.45.GIT

The git-hash-object(1) command allows users to hash an object even without a repository. Starting with c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), this will make us hit an uninitialized hash function, which subsequently leads to a segfault.

Fix this by falling back to SHA-1 explicitly when running outside of a Git repository. Eventually, we should expose this function as a command line option to the users so that they can pick which object hash to use by themselves.

---
 builtin/hash-object.c  | 7 +++++++
 t/t1007-hash-object.sh | 6 ++++++
 2 files changed, 13 insertions(+)

index 82ca6d2bfd..0855f4f8aa 100644
@@ -123,6 +123,13 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
 	else
 		prefix = setup_git_directory_gently(&nongit);
 
+	/*
+	 * TODO: Allow the hash algorithm to be configured by the user via a
+	 *       command line option when not using `-w`.
+	 */
+	if (nongit)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	if (vpath && prefix) {
 		vpath_free = prefix_filename(prefix, vpath);
 		vpath = vpath_free;

index 64aea38486..4c138c6ca4 100755
@@ -260,4 +260,10 @@ test_expect_success '--literally with extra-long type' '
 	echo example | git hash-object -t $t --literally --stdin
 '
 
+test_expect_success '--stdin outside of repository' '
+	nongit git hash-object --stdin <hello >actual &&
+	echo "$(test_oid hello)" >expect &&
+	test_cmp expect actual
+'
+
 test_done

2.45.GIT

Perhaps we should double-check with all commands that are designed to be able to work outside a repository, e.g. "git apply", "git grep --no-index", "git diff --no-index" (tried to be exhausitive without consulting documentation, so the list is not exhausitive at all).

It is good that the topic is kept outside 'master' (and it is in 'next' to give the topic a bit wider exposure than merely in 'seen' and the list archive).

We may want a test file that explicitly make commands that ought to work outside a repository actually run outside a repository, making use of the GIT_CEILING_DIRECTORIES mechanism, something along the lines of the attached.

 t/t1517-outside-repo.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

new file mode 100755
index 0000000000..4c595c2ff7
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='check random commands outside repo'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'set up a non-repo directory and test file' '
+	GIT_CEILING_DIRECTORIES=$(pwd) &&
+	export GIT_CEILING_DIRECTORIES &&
+	mkdir non-repo &&
+	(
+		cd non-repo &&
+		# confirm that git does not find a repo
+		test_must_fail git rev-parse --git-dir
+	) &&
+	test_write_lines one two three four >nums &&
+	git add nums &&
+	cp nums nums.old &&
+	test_write_lines five >>nums &&
+	git diff >sample.patch
+'
+
+test_expect_success 'apply a patch outside repository' '
+	(
+		cd non-repo &&
+		cp ../nums.old nums &&
+		git apply ../sample.patch
+	) &&
+	test_cmp nums non-repo/nums
+'
+
+test_expect_success 'compute a patch-id outside repository' '
+	git patch-id <sample.patch >patch-id.expect &&
+	(
+		cd non-repo &&
+		git patch-id <../sample.patch >../patch-id.actual
+	) &&
+	test_cmp patch-id.expect patch-id.actual
+'
+
+test_done

Another thought. Perhaps we should do, in the meantime, the attached patch?

Without your "fix patch-id" patch, one of the tests in 1517 will fail, and the other fails as there is no fix posted yet, but with the GIT_TEST_DEFAULT_HASH_IS_SHA1 set explicit to 1, they keep passing. This way, when we run out tests, we will always leave the default hash uninitialized to catch more errors, the binary built out of our source by default will crash to help our users catch more errors for us, yet when they really really need to, the users can set and export GIT_TEST_DEFAULT_HASH_IS_SHA1=1 to keep assuming that SHA1 is the hash algorithm when there is no specified.

 repository.c            | 24 ++++++++++++++++++++++++
 t/t1517-outside-repo.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh           |  4 ++++
 3 files changed, 70 insertions(+)

index 15c10015b0..6f67966e35 100644
@@ -26,6 +26,30 @@ void initialize_repository(struct repository *repo)
 	repo->parsed_objects = parsed_object_pool_new();
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
+
+	/*
+	 * Unfortunately, we need to keep this hack around for the time being:
+	 *
+	 *   - Not setting up the hash algorithm for `the_repository` leads to
+	 *     crashes because `the_hash_algo` is a macro that expands to
+	 *     `the_repository->hash_algo`. So if Git commands try to access
+	 *     `the_hash_algo` without a Git directory we crash.
+	 *
+	 *   - Setting up the hash algorithm to be SHA1 by default breaks other
+	 *     commands when running with SHA256.
+	 *
+	 * This is another point in case why having global state is a bad idea.
+	 * Eventually, we should remove this hack and stop setting the hash
+	 * algorithm in this function altogether. Instead, it should only ever
+	 * be set via our repository setup procedures. But that requires more
+	 * work.
+	 *
+	 * As we discover the code paths that need fixing, we may remove this
+	 * code completely, but we are not there yet.
+	 */
+	if (repo == the_repository &&
+	    git_env_bool("GIT_TEST_DEFAULT_HASH_IS_SHA1", 0))
+		repo_set_hash_algo(repo, GIT_HASH_SHA1);
 }
 
 static void expand_base_dir(char **out, const char *in,

index 79d3e0e7d9..36d311fb4a 100644
@@ -542,6 +542,10 @@ export EDITOR
 
 GIT_DEFAULT_HASH="${GIT_TEST_DEFAULT_HASH:-sha1}"
 export GIT_DEFAULT_HASH
+
+GIT_TEST_DEFAULT_HASH_IS_SHA1=0
+export GIT_TEST_DEFAULT_HASH_IS_SHA1
+
 GIT_DEFAULT_REF_FORMAT="${GIT_TEST_DEFAULT_REF_FORMAT:-files}"
 export GIT_DEFAULT_REF_FORMAT
 GIT_TEST_MERGE_ALGORITHM="${GIT_TEST_MERGE_ALGORITHM:-ort}"

The series adds an escape hatch to "the default hash algorithm is no longer SHA-1; it is an error to rely on the default" by introducing an undocumented GIT_TEST_DEFAULT_HASH_IS_SHA1=1 environment variable that users can set after reporting an failure they saw to us.

We can see that Patrick's two fixes to patch-id and hash-object corrects the breakage as the series progresses.

Junio C Hamano (2):
setup: add an escape hatch for "no more default hash algorithm" change
t1517: test commands that are designed to be run outside repository

Patrick Steinhardt (2):
builtin/patch-id: fix uninitialized hash function builtin/hash-object: fix uninitialized hash function

 builtin/hash-object.c   |  7 +++++
 builtin/patch-id.c      | 13 +++++++++
 repository.c            | 24 ++++++++++++++++
 t/t1007-hash-object.sh  |  6 ++++
 t/t1517-outside-repo.sh | 61 +++++++++++++++++++++++++++++++++++++++++
 t/t4204-patch-id.sh     | 34 +++++++++++++++++++++++
 t/test-lib.sh           |  4 +++
 7 files changed, 149 insertions(+)
 create mode 100755 t/t1517-outside-repo.sh

2.45.0-145-g3e4a232f6e

Partially revert c8aed5e8 (repository: stop setting SHA1 as the default object hash, 2024-05-07), to keep end-user systems still broken when we have gap in our test coverage but yet give them an escape hatch to set the GIT_TEST_DEFAULT_HASH_IS_SHA1=1 environment variable to revert to the previous behaviour.

---
 repository.c  | 24 ++++++++++++++++++++++++
 t/test-lib.sh |  4 ++++
 2 files changed, 28 insertions(+)

index 15c10015b0..6f67966e35 100644
@@ -26,6 +26,30 @@ void initialize_repository(struct repository *repo)
 	repo->parsed_objects = parsed_object_pool_new();
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
+
+	/*
+	 * Unfortunately, we need to keep this hack around for the time being:
+	 *
+	 *   - Not setting up the hash algorithm for `the_repository` leads to
+	 *     crashes because `the_hash_algo` is a macro that expands to
+	 *     `the_repository->hash_algo`. So if Git commands try to access
+	 *     `the_hash_algo` without a Git directory we crash.
+	 *
+	 *   - Setting up the hash algorithm to be SHA1 by default breaks other
+	 *     commands when running with SHA256.
+	 *
+	 * This is another point in case why having global state is a bad idea.
+	 * Eventually, we should remove this hack and stop setting the hash
+	 * algorithm in this function altogether. Instead, it should only ever
+	 * be set via our repository setup procedures. But that requires more
+	 * work.
+	 *
+	 * As we discover the code paths that need fixing, we may remove this
+	 * code completely, but we are not there yet.
+	 */
+	if (repo == the_repository &&
+	    git_env_bool("GIT_TEST_DEFAULT_HASH_IS_SHA1", 0))
+		repo_set_hash_algo(repo, GIT_HASH_SHA1);
 }
 
 static void expand_base_dir(char **out, const char *in,

index 79d3e0e7d9..36d311fb4a 100644
@@ -542,6 +542,10 @@ export EDITOR
 
 GIT_DEFAULT_HASH="${GIT_TEST_DEFAULT_HASH:-sha1}"
 export GIT_DEFAULT_HASH
+
+GIT_TEST_DEFAULT_HASH_IS_SHA1=0
+export GIT_TEST_DEFAULT_HASH_IS_SHA1
+
 GIT_DEFAULT_REF_FORMAT="${GIT_TEST_DEFAULT_REF_FORMAT:-files}"
 export GIT_DEFAULT_REF_FORMAT
 GIT_TEST_MERGE_ALGORITHM="${GIT_TEST_MERGE_ALGORITHM:-ort}"

2.45.0-145-g3e4a232f6e

Nice idea, this concept LG.

A few commands, like "git apply" and "git patch-id", have been broken with a recent change to stop setting the default hash algorithm to SHA-1. Test them and fix them in later commits.

---
 t/t1517-outside-repo.sh | 61 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100755 t/t1517-outside-repo.sh

new file mode 100755
index 0000000000..e0fd495ec1
@@ -0,0 +1,61 @@
+#!/bin/sh
+
+test_description='check random commands outside repo'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'set up a non-repo directory and test file' '
+	GIT_CEILING_DIRECTORIES=$(pwd) &&
+	export GIT_CEILING_DIRECTORIES &&
+	mkdir non-repo &&
+	(
+		cd non-repo &&
+		# confirm that git does not find a repo
+		test_must_fail git rev-parse --git-dir
+	) &&
+	test_write_lines one two three four >nums &&
+	git add nums &&
+	cp nums nums.old &&
+	test_write_lines five >>nums &&
+	git diff >sample.patch
+'
+
+test_expect_failure 'compute a patch-id outside repository' '
+	git patch-id <sample.patch >patch-id.expect &&
+	(
+		cd non-repo &&
+		git patch-id <../sample.patch >../patch-id.actual
+	) &&
+	test_cmp patch-id.expect patch-id.actual
+'
+
+test_expect_failure 'hash-object outside repository' '
+	git hash-object --stdin <sample.patch >hash.expect &&
+	(
+		cd non-repo &&
+		git hash-object --stdin <../sample.patch >../hash.actual
+	) &&
+	test_cmp hash.expect hash.actual
+'
+
+test_expect_failure 'apply a patch outside repository' '
+	(
+		cd non-repo &&
+		cp ../nums.old nums &&
+		git apply ../sample.patch
+	) &&
+	test_cmp nums non-repo/nums
+'
+
+test_expect_success 'grep outside repository' '
+	git grep --cached two >expect &&
+	(
+		cd non-repo &&
+		cp ../nums.old nums &&
+		git grep --no-index two >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_done

2.45.0-145-g3e4a232f6e

test_expect_failure is description enough for that purpose.

If a command should NOT work outside the project we will write a test more like so:

test_expect_success 'foo does not work outside' '
... prepare that $cwd is outside ... test_must_fail git foo
'

We say this in t/README:

- test_expect_failure [<prereq>] <message> <script>

This is NOT the opposite of test_expect_success, but is used to mark a test that demonstrates a known breakage. Unlike the usual test_expect_success tests, which say "ok" on success and "FAIL" on failure, this will say "FIXED" on success and "still broken" on failure. Failures from these tests won't cause -i (immediate) to stop.

Which means that when somebody rans out of things to do, grepping for test_expect_failure may give them a good place to start ;-).

Note that there were a few very rare occasions that what was marked as "known breakage" with test_expect_failure turned out to be what was working as intended.

Thanks.

Got it, thanks for explaining. With that, this change looks good to me.

From: Patrick Steinhardt <[email protected]>

In c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), we have adapted initialize_repository() to no longer set up a default hash function. As this function is also used to set up the_repository, the consequence is that the_hash_algo will now by default be a NULL pointer unless the hash algorithm was configured properly. This is done as a mechanism to detect cases where we may be using the wrong hash function by accident.

This change now causes git-patch-id(1) to segfault when it's run outside of a repository. As this command can read diffs from stdin, it does not necessarily need a repository, but then relies on the_hash_algo to compute the patch ID itself.

It is somewhat dubious that git-patch-id(1) relies on the_hash_algo in the first place. Quoting its manpage:

A "patch ID" is nothing but a sum of SHA-1 of the file diffs associated with a patch, with line numbers ignored. As such, it’s "reasonably stable", but at the same time also reasonably unique, i.e., two patches that have the same "patch ID" are almost guaranteed to be the same thing.

We explicitly document patch IDs to be using SHA-1. Furthermore, patch IDs are supposed to be stable for most of the part. But even with the same input, the patch IDs will now be different depending on the repo's configured object hash.

Work around the issue by setting up SHA-1 when there was no startup repository for now. This is arguably not the correct fix, but for now we rather want to focus on getting the segfault fixed.

---
 builtin/patch-id.c      | 13 +++++++++++++
 t/t1517-outside-repo.sh |  2 +-
 t/t4204-patch-id.sh     | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 1 deletion(-)

index 3894d2b970..be5a85e71c 100644
@@ -5,6 +5,7 @@
 #include "hash.h"
 #include "hex.h"
 #include "parse-options.h"
+#include "setup.h"
 
 static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)
 {

@@ -237,6 +238,18 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_patch_id_options,
 			     patch_id_usage, 0);
 
+	/*
+	 * We rely on `the_hash_algo` to compute patch IDs. This is dubious as
+	 * it means that the hash algorithm now depends on the object hash of
+	 * the repository, even though git-patch-id(1) clearly defines that
+	 * patch IDs always use SHA1.
+	 *
+	 * TODO: This hack should be removed in favor of converting the code
+	 *       that computes patch IDs to always use SHA1.
+	 */
+	if (!startup_info->have_repository)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	generate_id_list(opts ? opts > 1 : config.stable,
 			 opts ? opts == 3 : config.verbatim);
 	return 0;

index e0fd495ec1..ac5f3191cc 100755
@@ -21,7 +21,7 @@ test_expect_success 'set up a non-repo directory and test file' '
 	git diff >sample.patch
 '
 
-test_expect_failure 'compute a patch-id outside repository' '
+test_expect_success 'compute a patch-id outside repository' '
 	git patch-id <sample.patch >patch-id.expect &&
 	(
 		cd non-repo &&

index a7fa94ce0a..605faea0c7 100755
@@ -310,4 +310,38 @@ test_expect_success 'patch-id handles diffs with one line of before/after' '
 	test_config patchid.stable true &&
 	calc_patch_id diffu1stable <diffu1
 '
+
+test_expect_failure 'patch-id computes same ID with different object hashes' '
+	test_when_finished "rm -rf repo-sha1 repo-sha256" &&
+
+	cat >diff <<-\EOF &&
+	diff --git a/bar b/bar
+	index bdaf90f..31051f6 100644
+	--- a/bar
+	+++ b/bar
+	@@ -2 +2,2 @@
+	 b
+	+c
+	EOF
+
+	git init --object-format=sha1 repo-sha1 &&
+	git -C repo-sha1 patch-id <diff >patch-id-sha1 &&
+	git init --object-format=sha256 repo-sha256 &&
+	git -C repo-sha256 patch-id <diff >patch-id-sha256 &&
+	test_cmp patch-id-sha1 patch-id-sha256
+'
+
+test_expect_success 'patch-id without repository' '
+	cat >diff <<-\EOF &&
+	diff --git a/bar b/bar
+	index bdaf90f..31051f6 100644
+	--- a/bar
+	+++ b/bar
+	@@ -2 +2,2 @@
+	 b
+	+c
+	EOF
+	nongit git patch-id <diff
+'
+
 test_done

2.45.0-145-g3e4a232f6e

From: Patrick Steinhardt <[email protected]>

The git-hash-object(1) command allows users to hash an object even without a repository. Starting with c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), this will make us hit an uninitialized hash function, which subsequently leads to a segfault.

Fix this by falling back to SHA-1 explicitly when running outside of a Git repository. Eventually, we should expose this function as a command line option to the users so that they can pick which object hash to use by themselves.

---
 builtin/hash-object.c   | 7 +++++++
 t/t1007-hash-object.sh  | 6 ++++++
 t/t1517-outside-repo.sh | 2 +-
 3 files changed, 14 insertions(+), 1 deletion(-)

index 82ca6d2bfd..0855f4f8aa 100644
@@ -123,6 +123,13 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
 	else
 		prefix = setup_git_directory_gently(&nongit);
 
+	/*
+	 * TODO: Allow the hash algorithm to be configured by the user via a
+	 *       command line option when not using `-w`.
+	 */
+	if (nongit)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	if (vpath && prefix) {
 		vpath_free = prefix_filename(prefix, vpath);
 		vpath = vpath_free;

index 64aea38486..4c138c6ca4 100755
@@ -260,4 +260,10 @@ test_expect_success '--literally with extra-long type' '
 	echo example | git hash-object -t $t --literally --stdin
 '
 
+test_expect_success '--stdin outside of repository' '
+	nongit git hash-object --stdin <hello >actual &&
+	echo "$(test_oid hello)" >expect &&
+	test_cmp expect actual
+'
+
 test_done

index ac5f3191cc..854bb8f343 100755
@@ -30,7 +30,7 @@ test_expect_success 'compute a patch-id outside repository' '
 	test_cmp patch-id.expect patch-id.actual
 '
 
-test_expect_failure 'hash-object outside repository' '
+test_expect_success 'hash-object outside repository' '
 	git hash-object --stdin <sample.patch >hash.expect &&
 	(
 		cd non-repo &&

2.45.0-145-g3e4a232f6e

"git apply" can work outside a repository as a better "GNU patch", but when it does so, it still assumed that it can access the_hash_algo, which is no longer true in the new world order.

Make sure we explicitly fall back to SHA-1 algorithm for backward compatibility. Just like we argued for hash-objects, we probably should have a mechanism to specify the hash algorithm used outside a repository.

---

* After finishing these steps, it started to dawn on me that we probably can instead add GIT_DEFAULT_HASH_ALGO environment variable for exactly that purpose. Then the escape hatch I introduced in [1/4] can become just that.

 builtin/apply.c         | 4 ++++
 t/t1517-outside-repo.sh | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

index 861a01910c..e9175f820f 100644
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "gettext.h"
 #include "repository.h"
+#include "hash.h"
 #include "apply.h"
 
 static const char * const apply_usage[] = {

@@ -18,6 +19,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix)
 	if (init_apply_state(&state, the_repository, prefix))
 		exit(128);
 
+	if (!the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	argc = apply_parse_options(argc, argv,
 				   &state, &force_apply, &options,
 				   apply_usage);

index 854bb8f343..6f32a40c6d 100755
@@ -39,7 +39,7 @@ test_expect_success 'hash-object outside repository' '
 	test_cmp hash.expect hash.actual
 '
 
-test_expect_failure 'apply a patch outside repository' '
+test_expect_success 'apply a patch outside repository' '
 	(
 		cd non-repo &&
 		cp ../nums.old nums &&

2.45.0-145-g3e4a232f6e

In this round, the escape hatch is no longer the escape hatch, but also serves as an end-user knob to specify what hash algorithm to use when there is no repository to tell us which one to use.

We can see that Patrick's two fixes to patch-id and hash-object, as well as my fix to apply, correct the breakage as the series progresses.

Junio C Hamano (3):
setup: add an escape hatch for "no more default hash algorithm" change
t1517: test commands that are designed to be run outside repository
apply: fix uninitialized hash function

Patrick Steinhardt (2):
builtin/patch-id: fix uninitialized hash function builtin/hash-object: fix uninitialized hash function

 builtin/apply.c         |  4 +++
 builtin/hash-object.c   |  3 ++
 builtin/patch-id.c      | 13 +++++++++
 repository.c            | 32 +++++++++++++++++++++
 t/t1007-hash-object.sh  |  6 ++++
 t/t1517-outside-repo.sh | 64 +++++++++++++++++++++++++++++++++++++++++
 t/t4204-patch-id.sh     | 34 ++++++++++++++++++++++
 7 files changed, 156 insertions(+)
 create mode 100755 t/t1517-outside-repo.sh

2.45.0-145-g3e4a232f6e

Partially revert c8aed5e8 (repository: stop setting SHA1 as the default object hash, 2024-05-07), to keep end-user systems still broken when we have gap in our test coverage but yet give them an escape hatch to set the GIT_TEST_DEFAULT_HASH_ALGORITHM environment variable to "sha1" in order to revert to the previous behaviour.

---
 repository.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

index 15c10015b0..2001c760dc 100644
@@ -19,6 +19,20 @@
 static struct repository the_repo;
 struct repository *the_repository = &the_repo;
 
+static void set_default_hash_algo(struct repository *repo)
+{
+	const char *hash_name;
+	int algo;
+
+	hash_name = getenv("GIT_DEFAULT_HASH_ALGORITHM");
+	if (!hash_name)
+		return;
+	algo = hash_algo_by_name(hash_name);
+	if (algo == GIT_HASH_UNKNOWN)
+		return;
+	repo_set_hash_algo(repo, algo);
+}
+
 void initialize_repository(struct repository *repo)
 {
 	repo->objects = raw_object_store_new();

@@ -26,6 +40,24 @@ void initialize_repository(struct repository *repo)
 	repo->parsed_objects = parsed_object_pool_new();
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
+
+	/*
+	 * When a command runs inside a repository, it learns what
+	 * hash algorithm is in use from the repository, but some
+	 * commands are designed to work outside a repository, yet
+	 * they want to access the_hash_algo, if only for the length
+	 * of the hashed value to see if their input looks like a
+	 * plausible hash value.
+	 *
+	 * We are in the process of identifying the codepaths and
+	 * giving them an appropriate default individually; any
+	 * unconverted codepath that tries to access the_hash_algo
+	 * will thus fail.  The end-users however have an escape hatch
+	 * to set GIT_DEFAULT_HASH_ALGORITHM environment variable to
+	 * "sha1" get back the old behaviour of defaulting to SHA-1.
+	 */
+	if (repo == the_repository)
+		set_default_hash_algo(repo);
 }
 
 static void expand_base_dir(char **out, const char *in,

2.45.0-145-g3e4a232f6e

A few commands, like "git apply" and "git patch-id", have been broken with a recent change to stop setting the default hash algorithm to SHA-1. Test them and fix them in later commits.

---
 t/t1517-outside-repo.sh | 64 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100755 t/t1517-outside-repo.sh

new file mode 100755
index 0000000000..16d9714c27
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='check random commands outside repo'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+# Do not use the end-user workaround for this test.
+# GIT_DEFAULT_HASH_ALGORITHM=sha1; export GIT_DEFAULT_HASH_ALGORITHM
+
+test_expect_success 'set up a non-repo directory and test file' '
+	GIT_CEILING_DIRECTORIES=$(pwd) &&
+	export GIT_CEILING_DIRECTORIES &&
+	mkdir non-repo &&
+	(
+		cd non-repo &&
+		# confirm that git does not find a repo
+		test_must_fail git rev-parse --git-dir
+	) &&
+	test_write_lines one two three four >nums &&
+	git add nums &&
+	cp nums nums.old &&
+	test_write_lines five >>nums &&
+	git diff >sample.patch
+'
+
+test_expect_failure 'compute a patch-id outside repository' '
+	git patch-id <sample.patch >patch-id.expect &&
+	(
+		cd non-repo &&
+		git patch-id <../sample.patch >../patch-id.actual
+	) &&
+	test_cmp patch-id.expect patch-id.actual
+'
+
+test_expect_failure 'hash-object outside repository' '
+	git hash-object --stdin <sample.patch >hash.expect &&
+	(
+		cd non-repo &&
+		git hash-object --stdin <../sample.patch >../hash.actual
+	) &&
+	test_cmp hash.expect hash.actual
+'
+
+test_expect_failure 'apply a patch outside repository' '
+	(
+		cd non-repo &&
+		cp ../nums.old nums &&
+		git apply ../sample.patch
+	) &&
+	test_cmp nums non-repo/nums
+'
+
+test_expect_success 'grep outside repository' '
+	git grep --cached two >expect &&
+	(
+		cd non-repo &&
+		cp ../nums.old nums &&
+		git grep --no-index two >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_done

2.45.0-145-g3e4a232f6e

From: Patrick Steinhardt <[email protected]>

In c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), we have adapted initialize_repository() to no longer set up a default hash function. As this function is also used to set up the_repository, the consequence is that the_hash_algo will now by default be a NULL pointer unless the hash algorithm was configured properly. This is done as a mechanism to detect cases where we may be using the wrong hash function by accident.

This change now causes git-patch-id(1) to segfault when it's run outside of a repository. As this command can read diffs from stdin, it does not necessarily need a repository, but then relies on the_hash_algo to compute the patch ID itself.

It is somewhat dubious that git-patch-id(1) relies on the_hash_algo in the first place. Quoting its manpage:

A "patch ID" is nothing but a sum of SHA-1 of the file diffs associated with a patch, with line numbers ignored. As such, it’s "reasonably stable", but at the same time also reasonably unique, i.e., two patches that have the same "patch ID" are almost guaranteed to be the same thing.

We explicitly document patch IDs to be using SHA-1. Furthermore, patch IDs are supposed to be stable for most of the part. But even with the same input, the patch IDs will now be different depending on the repo's configured object hash.

Work around the issue by setting up SHA-1 when there was no startup repository for now. This is arguably not the correct fix, but for now we rather want to focus on getting the segfault fixed.

---
 builtin/patch-id.c      | 13 +++++++++++++
 t/t1517-outside-repo.sh |  2 +-
 t/t4204-patch-id.sh     | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 1 deletion(-)

index 3894d2b970..e6ae89beab 100644
@@ -5,6 +5,7 @@
 #include "hash.h"
 #include "hex.h"
 #include "parse-options.h"
+#include "setup.h"
 
 static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)
 {

@@ -237,6 +238,18 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_patch_id_options,
 			     patch_id_usage, 0);
 
+	/*
+	 * We rely on `the_hash_algo` to compute patch IDs. This is dubious as
+	 * it means that the hash algorithm now depends on the object hash of
+	 * the repository, even though git-patch-id(1) clearly defines that
+	 * patch IDs always use SHA1.
+	 *
+	 * NEEDSWORK: This hack should be removed in favor of converting
+	 * the code that computes patch IDs to always use SHA1.
+	 */
+	if (!startup_info->have_repository)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	generate_id_list(opts ? opts > 1 : config.stable,
 			 opts ? opts == 3 : config.verbatim);
 	return 0;

index 16d9714c27..f1fd5c9888 100755
@@ -24,7 +24,7 @@ test_expect_success 'set up a non-repo directory and test file' '
 	git diff >sample.patch
 '
 
-test_expect_failure 'compute a patch-id outside repository' '
+test_expect_success 'compute a patch-id outside repository' '
 	git patch-id <sample.patch >patch-id.expect &&
 	(
 		cd non-repo &&

index a7fa94ce0a..605faea0c7 100755
@@ -310,4 +310,38 @@ test_expect_success 'patch-id handles diffs with one line of before/after' '
 	test_config patchid.stable true &&
 	calc_patch_id diffu1stable <diffu1
 '
+
+test_expect_failure 'patch-id computes same ID with different object hashes' '
+	test_when_finished "rm -rf repo-sha1 repo-sha256" &&
+
+	cat >diff <<-\EOF &&
+	diff --git a/bar b/bar
+	index bdaf90f..31051f6 100644
+	--- a/bar
+	+++ b/bar
+	@@ -2 +2,2 @@
+	 b
+	+c
+	EOF
+
+	git init --object-format=sha1 repo-sha1 &&
+	git -C repo-sha1 patch-id <diff >patch-id-sha1 &&
+	git init --object-format=sha256 repo-sha256 &&
+	git -C repo-sha256 patch-id <diff >patch-id-sha256 &&
+	test_cmp patch-id-sha1 patch-id-sha256
+'
+
+test_expect_success 'patch-id without repository' '
+	cat >diff <<-\EOF &&
+	diff --git a/bar b/bar
+	index bdaf90f..31051f6 100644
+	--- a/bar
+	+++ b/bar
+	@@ -2 +2,2 @@
+	 b
+	+c
+	EOF
+	nongit git patch-id <diff
+'
+
 test_done

2.45.0-145-g3e4a232f6e

I tend to agree that the use of SHA-256 in patch-id computation is a regression when we added the SHA-256 support. Even with the GIT_DEFAULT_HASH_ALGORITHM fallback, we cannot fix it in an initialized SHA-256 repository.

We should fix it but I agree that is probably outside the scope of the "oops, we leave the hash algorithm totally uninitialized" fix.

	if (!the_hash_algo)
		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);

[snip]

To the best of my knowledge there isn't. What I prefer about my approach is that it explicitly points out that this is conditional on whether or not we have a repository. But in the end I don't mind much which of both versions we use.

Patrick

Ah, that makes sense, and it is quite subjective but makes certain sense.

The reason I prefered to check "the_hash_algo" is very much the opposite. In this particular decision to call (or not call) set_hash_algo(), we only care if the_hash_algo is not yet set, and that is why the_hash_algo is checked.

Specifically, we do not care *why* it is still unset; in the current codebase, the most likely reason why we do not have the_hash_algo set might be that we haven't found the repository yet, but we do not have to rely on that assumption to hold true. It would help maintainability into the future where the_hash_algo is set already before we come here when outside a repository, or vice versa.

From: Patrick Steinhardt <[email protected]>

The git-hash-object(1) command allows users to hash an object even without a repository. Starting with c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), this will make us hit an uninitialized hash function, which subsequently leads to a segfault.

Fix this by falling back to SHA-1 explicitly when running outside of a Git repository. Users can use GIT_DEFAULT_HASH_ALGORITHM environment to specify what hash algorithm they want, so arguably this code should not be needed.

---
 builtin/hash-object.c   | 3 +++
 t/t1007-hash-object.sh  | 6 ++++++
 t/t1517-outside-repo.sh | 2 +-
 3 files changed, 10 insertions(+), 1 deletion(-)

index 82ca6d2bfd..c767414a0c 100644
@@ -123,6 +123,9 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
 	else
 		prefix = setup_git_directory_gently(&nongit);
 
+	if (nongit && !the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	if (vpath && prefix) {
 		vpath_free = prefix_filename(prefix, vpath);
 		vpath = vpath_free;

index 64aea38486..4c138c6ca4 100755
@@ -260,4 +260,10 @@ test_expect_success '--literally with extra-long type' '
 	echo example | git hash-object -t $t --literally --stdin
 '
 
+test_expect_success '--stdin outside of repository' '
+	nongit git hash-object --stdin <hello >actual &&
+	echo "$(test_oid hello)" >expect &&
+	test_cmp expect actual
+'
+
 test_done

index f1fd5c9888..80261e43c7 100755
@@ -33,7 +33,7 @@ test_expect_success 'compute a patch-id outside repository' '
 	test_cmp patch-id.expect patch-id.actual
 '
 
-test_expect_failure 'hash-object outside repository' '
+test_expect_success 'hash-object outside repository' '
 	git hash-object --stdin <sample.patch >hash.expect &&
 	(
 		cd non-repo &&

2.45.0-145-g3e4a232f6e

I'd prefer the latter -- always use SHA-1. As you say, it's easier to understand and doesn't create implicit mechanisms that we'll have to maintain going forward. Also, users didn't have a desire yet to pick a different algorithm than SHA-1, which probably also comes from the fact that SHA-256 repositories are still scarce.

Eventually, we should then add a new option --object-hash= to git-hash-object(1) and other commands that may run outside of a Git repository to let the user pick their desired hash.

Patrick

If we were planning to allow using something other than SHA-1 in the future, then I do not think I agree with your argument. We can use the same GIT_DEFAULT_HASH mechanism to specify what hash to use without later adding an extra option just fine.

"git apply" can work outside a repository as a better "GNU patch", but when it does so, it still assumed that it can access the_hash_algo, which is no longer true in the new world order.

Make sure we explicitly fall back to SHA-1 algorithm for backward compatibility.

---
 builtin/apply.c         | 4 ++++
 t/t1517-outside-repo.sh | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

index 861a01910c..e9175f820f 100644
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "gettext.h"
 #include "repository.h"
+#include "hash.h"
 #include "apply.h"
 
 static const char * const apply_usage[] = {

@@ -18,6 +19,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix)
 	if (init_apply_state(&state, the_repository, prefix))
 		exit(128);
 
+	if (!the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	argc = apply_parse_options(argc, argv,
 				   &state, &force_apply, &options,
 				   apply_usage);

index 80261e43c7..89d89cf3f0 100755
@@ -42,7 +42,7 @@ test_expect_success 'hash-object outside repository' '
 	test_cmp hash.expect hash.actual
 '
 
-test_expect_failure 'apply a patch outside repository' '
+test_expect_success 'apply a patch outside repository' '
 	(
 		cd non-repo &&
 		cp ../nums.old nums &&

2.45.0-145-g3e4a232f6e

A change recently merged to 'next' stops us from defaulting to using SHA-1 unless other code (like a logic early in the start-up sequence to see what hash is being used in the repository we are working in) explicitly sets it, leading to a (deliberate) crash of "git" when we forgot to cover certain code paths.

It turns out we have a few. Notable ones are all operations that are designed to work outside a repository. We should go over all such code paths and give them a reasonable default when there is one available (e.g. for historical reasons, patch-id is documented to work with SHA-1 hashes, so arguably it should do so everywhere, not just in SHA-1 repositories, but outside any repository), and when there is none, we can use the existing GIT_DEFAULT_HASH environment variable to let the user control the choice.

In this fourth iteration, I used the existing GIT_DEFAULT_HASH environment variable that has been used to allow users to opt into an early experiment of SHA-256 support. I saw breakages in SHA-256 CI jobs for earlier rounds of this series, but hopefully this is now good to go. Knock wood...

Junio C Hamano (3):
setup: add an escape hatch for "no more default hash algorithm" change
t1517: test commands that are designed to be run outside repository
apply: fix uninitialized hash function

Patrick Steinhardt (2):
builtin/patch-id: fix uninitialized hash function builtin/hash-object: fix uninitialized hash function

 builtin/apply.c         |  4 +++
 builtin/hash-object.c   |  3 ++
 builtin/patch-id.c      | 13 +++++++++
 environment.h           |  1 +
 repository.c            | 40 +++++++++++++++++++++++++++
 setup.c                 |  2 --
 t/t1007-hash-object.sh  |  6 ++++
 t/t1517-outside-repo.sh | 61 +++++++++++++++++++++++++++++++++++++++++
 t/t4204-patch-id.sh     | 34 +++++++++++++++++++++++
 9 files changed, 162 insertions(+), 2 deletions(-)
 create mode 100755 t/t1517-outside-repo.sh

Range-diff against v3:
1: 7adbf0cc5f ! 1: 3038f73e1c setup: add an escape hatch for "no more default hash algorithm" change
@@ Commit message
Partially revert c8aed5e8 (repository: stop setting SHA1 as the
default object hash, 2024-05-07), to keep end-user systems still
broken when we have gap in our test coverage but yet give them an
- escape hatch to set the GIT_TEST_DEFAULT_HASH_ALGORITHM environment
- variable to "sha1" in order to revert to the previous behaviour.
+ escape hatch to set the GIT_DEFAULT_HASH environment variable to
+ "sha1" in order to revert to the previous behaviour. This variable
+ has been in use for using SHA-256 hash by default, and it should be
+ a better fit than inventing a new and test-only knob.

+ ## environment.h ##
+@@ environment.h: const char *getenv_safe(struct strvec *argv, const char *name);
+ #define GIT_OPTIONAL_LOCKS_ENVIRONMENT "GIT_OPTIONAL_LOCKS"
+ #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
+ #define GIT_ATTR_SOURCE_ENVIRONMENT "GIT_ATTR_SOURCE"
++#define GIT_DEFAULT_HASH_ENVIRONMENT "GIT_DEFAULT_HASH"
+
+ /*
+ * Environment variable used in handshaking the wire protocol.
+
## repository.c ##
+@@
+ #include "git-compat-util.h"
+ #include "abspath.h"
++#include "environment.h"
+ #include "repository.h"
+ #include "object-store-ll.h"
+ #include "config.h"
@@
static struct repository the_repo;
struct repository *the_repository = &the_repo;
@@ repository.c
+ const char *hash_name;
+ int algo;
+
-+ hash_name = getenv("GIT_DEFAULT_HASH_ALGORITHM");
++ hash_name = getenv(GIT_DEFAULT_HASH_ENVIRONMENT);
+ if (!hash_name)
+ return;
+ algo = hash_algo_by_name(hash_name);
++
++ /*
++ * NEEDSWORK: after all, falling back to SHA-1 by assigning
++ * GIT_HASH_SHA1 to algo here, instead of returning, may give
++ * us better behaviour.
++ */
+ if (algo == GIT_HASH_UNKNOWN)
+ return;
++
+ repo_set_hash_algo(repo, algo);
+}
+
@@ repository.c: void initialize_repository(struct repository *repo)
+ * giving them an appropriate default individually; any
+ * unconverted codepath that tries to access the_hash_algo
+ * will thus fail. The end-users however have an escape hatch
-+ * to set GIT_DEFAULT_HASH_ALGORITHM environment variable to
-+ * "sha1" get back the old behaviour of defaulting to SHA-1.
++ * to set GIT_DEFAULT_HASH environment variable to "sha1" get
++ * back the old behaviour of defaulting to SHA-1.
+ */
+ if (repo == the_repository)
+ set_default_hash_algo(repo);
}

static void expand_base_dir(char **out, const char *in,
+
+ ## setup.c ##
+@@ setup.c: int daemonize(void)
+ #define TEST_FILEMODE 1
+ #endif
+
+-#define GIT_DEFAULT_HASH_ENVIRONMENT "GIT_DEFAULT_HASH"
+-
+ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
+ DIR *dir)
+ {
2: c40b2bc7b5 ! 2: e56ae68961 t1517: test commands that are designed to be run outside repository

    @@ t/t1517-outside-repo.sh (new)
     +TEST_PASSES_SANITIZE_LEAK=true
     +. ./test-lib.sh
     +
    -+# Do not use the end-user workaround for this test.
    -+# GIT_DEFAULT_HASH_ALGORITHM=sha1; export GIT_DEFAULT_HASH_ALGORITHM
    -+
     +test_expect_success 'set up a non-repo directory and test file' '
     +	GIT_CEILING_DIRECTORIES=$(pwd) &&
     +	export GIT_CEILING_DIRECTORIES &&
3:  48d83fe1fd ! 3:  e1ae0e95fc builtin/patch-id: fix uninitialized hash function
    @@ builtin/patch-id.c: int cmd_patch_id(int argc, const char **argv, const char *pr
     +	 * NEEDSWORK: This hack should be removed in favor of converting
     +	 * the code that computes patch IDs to always use SHA1.
     +	 */
    -+	if (!startup_info->have_repository)
    ++	if (!the_hash_algo)
     +		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
     +
      	generate_id_list(opts ? opts > 1 : config.stable,

4: a5bf0dfb0a ! 4: 1c8ce0d024 builtin/hash-object: fix uninitialized hash function
@@ Commit message
SHA1 as the default object hash, 2024-05-07), this will make us hit an uninitialized hash function, which subsequently leads to a segfault.

- Fix this by falling back to SHA-1 explicitly when running outside of a
- Git repository. Users can use GIT_DEFAULT_HASH_ALGORITHM environment to
- specify what hash algorithm they want, so arguably this code should not
- be needed.
+ Fix this by falling back to SHA-1 explicitly when running outside of
+ a Git repository. Users can use GIT_DEFAULT_HASH environment to
+ specify what hash algorithm they want, so arguably this code should
+ not be needed.

5: 2b9e486c3f = 5: 5b0ec43757 apply: fix uninitialized hash function

Partially revert c8aed5e8 (repository: stop setting SHA1 as the default object hash, 2024-05-07), to keep end-user systems still broken when we have gap in our test coverage but yet give them an escape hatch to set the GIT_DEFAULT_HASH environment variable to "sha1" in order to revert to the previous behaviour. This variable has been in use for using SHA-256 hash by default, and it should be a better fit than inventing a new and test-only knob.

---
 environment.h |  1 +
 repository.c  | 40 ++++++++++++++++++++++++++++++++++++++++
 setup.c       |  2 --
 3 files changed, 41 insertions(+), 2 deletions(-)

index 05fd94d7be..deaa29408f 100644
@@ -56,6 +56,7 @@ const char *getenv_safe(struct strvec *argv, const char *name);
 #define GIT_OPTIONAL_LOCKS_ENVIRONMENT "GIT_OPTIONAL_LOCKS"
 #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
 #define GIT_ATTR_SOURCE_ENVIRONMENT "GIT_ATTR_SOURCE"
+#define GIT_DEFAULT_HASH_ENVIRONMENT "GIT_DEFAULT_HASH"
 
 /*
  * Environment variable used in handshaking the wire protocol.

index 15c10015b0..f912ee9a7c 100644
@@ -1,5 +1,6 @@
 #include "git-compat-util.h"
 #include "abspath.h"
+#include "environment.h"
 #include "repository.h"
 #include "object-store-ll.h"
 #include "config.h"

@@ -19,6 +20,27 @@
 static struct repository the_repo;
 struct repository *the_repository = &the_repo;
 
+static void set_default_hash_algo(struct repository *repo)
+{
+	const char *hash_name;
+	int algo;
+
+	hash_name = getenv(GIT_DEFAULT_HASH_ENVIRONMENT);
+	if (!hash_name)
+		return;
+	algo = hash_algo_by_name(hash_name);
+
+	/*
+	 * NEEDSWORK: after all, falling back to SHA-1 by assigning
+	 * GIT_HASH_SHA1 to algo here, instead of returning, may give
+	 * us better behaviour.
+	 */
+	if (algo == GIT_HASH_UNKNOWN)
+		return;
+
+	repo_set_hash_algo(repo, algo);
+}
+
 void initialize_repository(struct repository *repo)
 {
 	repo->objects = raw_object_store_new();

@@ -26,6 +48,24 @@ void initialize_repository(struct repository *repo)
 	repo->parsed_objects = parsed_object_pool_new();
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
+
+	/*
+	 * When a command runs inside a repository, it learns what
+	 * hash algorithm is in use from the repository, but some
+	 * commands are designed to work outside a repository, yet
+	 * they want to access the_hash_algo, if only for the length
+	 * of the hashed value to see if their input looks like a
+	 * plausible hash value.
+	 *
+	 * We are in the process of identifying the codepaths and
+	 * giving them an appropriate default individually; any
+	 * unconverted codepath that tries to access the_hash_algo
+	 * will thus fail.  The end-users however have an escape hatch
+	 * to set GIT_DEFAULT_HASH environment variable to "sha1" get
+	 * back the old behaviour of defaulting to SHA-1.
+	 */
+	if (repo == the_repository)
+		set_default_hash_algo(repo);
 }
 
 static void expand_base_dir(char **out, const char *in,

index 7c996659bd..d084703465 100644
@@ -1840,8 +1840,6 @@ int daemonize(void)
 #define TEST_FILEMODE 1
 #endif
 
-#define GIT_DEFAULT_HASH_ENVIRONMENT "GIT_DEFAULT_HASH"
-
 static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 			     DIR *dir)
 {

2.45.0-145-g3e4a232f6e

On Mon, May 13, 2024 at 06:14:33PM -0700, Junio C Hamano wrote:
[snip]

I tried the version you posted with only SHA-1 fallback and it failed with SHA256 CI jobs, and eventually came up with using the existing environment variable.

The variable is the perfect fit in the longer term for our purpose. It is what the end-users will set, not for papering over remaining bugs from the "no longer there is a fallback default" change, but for telling Git that they want to use sha256 repositories.

With GIT_DEFAULT_HASH set to sha256, we will be testing a configuration that is very close to those end-users.

There probably are four cases we need to check:

- In a repository, invocations of commands that should honor the hash function that is in use by the repository (i.e. this is true for most commands and their invocations).

- In a repository, invocations of commands that should ignore the repository settings and always use SHA-1 (i.e. proposed fix to patch-id).

- Outside a repository, invocations with GIT_DEFAULT_HASH set should probably parallel the above two, as if GIT_DEFAULT_HASH came from the repository.

- Outside a repository, invocations without GIT_DEFAULT_HASH set should all default to SHA-1???

The last combination cannot be tested UNLESS we unset GIT_DEFAULT_HASH that is given by test-lib.sh, but that can easily be arranged, now we have one primary/central place that tests out-of-repository invocations of various commands in t1517.

And using the existing environment variable has an added benefit that we do not have to add an extra option to random commands.

Having done all of this, I actually am very tempted to add the "always default to SHA-1" back as a fallback position to the set_default_hash_algo() function. We know we are going to get the right hash algorithm when working in the repository, so the only case the default matters in practice is when working outside the repository.

We already have such a custom code for "git diff --no-index", and we are adding a few more back in here, but they can disappear if we had code to set the fallback default when GIT_DEFAULT_HASH does not exist here. The "always use SHA-1 regardless of the hash used by the repository" code like "patch-id" should not depend on such a fallback default but should have its own code to explicitly set it.

As the user can tweak what algorithm they want if the wanted to, it does not sound too bad to have a fallback default when the user did not choose.

I think that this is going into the wrong direction. The patches that we have added surface real issues. If we now unconditionally add back the fallback to SHA-1, then we only punt those issues further down the road to the point in time where we drop the_hash_algo and/or the_repository. All the issues that were surfaced until now are technical debt, and the proposed fixes have been documented with a "TODO" item that they need more work.

In some cases it's as simple as adding in an option to the respective command that lets the user pick the hash algorithm, and I do not think that GIT_DEFAULT_HASH is a proper replacement for that. In other cases like for git-patch-id(1) the issue runs deeper and we need to refactor the whole subsystem to stop relying on the_hash_algo altogether.

Patrick

Not really. It does not add anything to help either real world or our tests. The current test setting is already bad enough in that, unlike in the real world settings, even tests with the SHA-1 algorithm has GIT_DEFAULT_HASH environment variable set, which means that such a "if the environment variable is not set, further fall back to SHA-1" does not do anything.

Unless we change t/test-lib.sh not to set GIT_DEFAULT_HASH tweaking the fallback default in repository.c:set_default_hash_algo() based on GIT_DEFAULT_HASH would not be a workable solution.

I wanted to arrange things so that the end-user exectuion by default has an extra fallback (perhaps to SHA-1, or GIT_DEFAULT_HASH) to avoid disrupting their real-world use, which we can disable in our tests to expose code paths that still rely on the "default" set when in-core repository struct gets initialized, but that is not possible without changing the way t/test-lib.sh uses GIT_DEFAULT_HASH, it seems. So the arrangement unfortunately has to be "we have no default, and bugs will break the real-world uses as well as tests the same way. The real-world users have to export an extra 'workaround' environment variable to force "default" to SHA-1 (or GIT_DEFAULT_HASH) --- which may be "workable" but very far from being intuitive. They can set GIT_DEFAULT_HASH but to make it effective everywhere, including the "default" given by set_default_hash_algo(), they need to set this other "workaround" thing.

While I think a manual setting of the_hash_algo in "diff --no-index" code path should not hardcode "SHA-1" but instead use the hash specified by the GIT_DEFAULT_HASH environment to be consistent with the use of "git" by the same parent process that had that variable exported to the environment, that should not be done globally in repository.c:set_default_hash_algo().

A few commands, like "git apply" and "git patch-id", have been broken with a recent change to stop setting the default hash algorithm to SHA-1. Test them and fix them in later commits.

---
 t/t1517-outside-repo.sh | 61 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100755 t/t1517-outside-repo.sh

new file mode 100755
index 0000000000..e0fd495ec1
@@ -0,0 +1,61 @@
+#!/bin/sh
+
+test_description='check random commands outside repo'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'set up a non-repo directory and test file' '
+	GIT_CEILING_DIRECTORIES=$(pwd) &&
+	export GIT_CEILING_DIRECTORIES &&
+	mkdir non-repo &&
+	(
+		cd non-repo &&
+		# confirm that git does not find a repo
+		test_must_fail git rev-parse --git-dir
+	) &&
+	test_write_lines one two three four >nums &&
+	git add nums &&
+	cp nums nums.old &&
+	test_write_lines five >>nums &&
+	git diff >sample.patch
+'
+
+test_expect_failure 'compute a patch-id outside repository' '
+	git patch-id <sample.patch >patch-id.expect &&
+	(
+		cd non-repo &&
+		git patch-id <../sample.patch >../patch-id.actual
+	) &&
+	test_cmp patch-id.expect patch-id.actual
+'
+
+test_expect_failure 'hash-object outside repository' '
+	git hash-object --stdin <sample.patch >hash.expect &&
+	(
+		cd non-repo &&
+		git hash-object --stdin <../sample.patch >../hash.actual
+	) &&
+	test_cmp hash.expect hash.actual
+'
+
+test_expect_failure 'apply a patch outside repository' '
+	(
+		cd non-repo &&
+		cp ../nums.old nums &&
+		git apply ../sample.patch
+	) &&
+	test_cmp nums non-repo/nums
+'
+
+test_expect_success 'grep outside repository' '
+	git grep --cached two >expect &&
+	(
+		cd non-repo &&
+		cp ../nums.old nums &&
+		git grep --no-index two >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_done

2.45.0-145-g3e4a232f6e

Is there a specific reason why this needs a whole patch suite, as opposed to adding the tests to the respective test suites of the commands?

Yes, testing out-of-repository operations needs certain trick and people forget to write such tests using the GIT_CEILING_DIRECTORIES mechanism. Having one place where we have an enumeration of commands that are designed to be usable outside repository is a handy way to make sure that we have enough test coverage. It would make it easy to control how GIT_DEFAULT_HASH environment is set during these tests to have them in all one place.

We already have the "nogit" command that neatly encapsulates all of this logic, so the trickery is contained in a single spot in practice.

Patrick

Heh, you asked for "a" specific reason, and I listed three. The tests that are spread across many scripts make it harder to see if we have enough coverage for the out-of-repository operations, and the use of "nongit" helper does not change the equation, does it?

Hm. I guess I got thrown off a bit as this was written in a single paragraph, so my mind didn't process it as different reasons :)

Put this way I in fact agree with you.

No, it doesn't really.

Thanks!

Patrick

Of course the other side of the coin is that having all tests about a single command (say, "git mailmap") in the same script for both in-repo and out-of-repo operations is also handy in a different way.

I haven't found a good way to balance the two. Obviously duplicating the same test in two scripts (one collection for the same command, the other collection out-of repo operation of various commands) is something I really do want to avoid, so...

From: Patrick Steinhardt <[email protected]>

In c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), we have adapted initialize_repository() to no longer set up a default hash function. As this function is also used to set up the_repository, the consequence is that the_hash_algo will now by default be a NULL pointer unless the hash algorithm was configured properly. This is done as a mechanism to detect cases where we may be using the wrong hash function by accident.

This change now causes git-patch-id(1) to segfault when it's run outside of a repository. As this command can read diffs from stdin, it does not necessarily need a repository, but then relies on the_hash_algo to compute the patch ID itself.

It is somewhat dubious that git-patch-id(1) relies on the_hash_algo in the first place. Quoting its manpage:

A "patch ID" is nothing but a sum of SHA-1 of the file diffs associated with a patch, with line numbers ignored. As such, it’s "reasonably stable", but at the same time also reasonably unique, i.e., two patches that have the same "patch ID" are almost guaranteed to be the same thing.

We explicitly document patch IDs to be using SHA-1. Furthermore, patch IDs are supposed to be stable for most of the part. But even with the same input, the patch IDs will now be different depending on the repo's configured object hash.

Work around the issue by setting up SHA-1 when there was no startup repository for now. This is arguably not the correct fix, but for now we rather want to focus on getting the segfault fixed.

---
 builtin/patch-id.c      | 13 +++++++++++++
 t/t1517-outside-repo.sh |  2 +-
 t/t4204-patch-id.sh     | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 1 deletion(-)

index 3894d2b970..583099cacf 100644
@@ -5,6 +5,7 @@
 #include "hash.h"
 #include "hex.h"
 #include "parse-options.h"
+#include "setup.h"
 
 static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)
 {

@@ -237,6 +238,18 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_patch_id_options,
 			     patch_id_usage, 0);
 
+	/*
+	 * We rely on `the_hash_algo` to compute patch IDs. This is dubious as
+	 * it means that the hash algorithm now depends on the object hash of
+	 * the repository, even though git-patch-id(1) clearly defines that
+	 * patch IDs always use SHA1.
+	 *
+	 * NEEDSWORK: This hack should be removed in favor of converting
+	 * the code that computes patch IDs to always use SHA1.
+	 */
+	if (!the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	generate_id_list(opts ? opts > 1 : config.stable,
 			 opts ? opts == 3 : config.verbatim);
 	return 0;

index e0fd495ec1..ac5f3191cc 100755
@@ -21,7 +21,7 @@ test_expect_success 'set up a non-repo directory and test file' '
 	git diff >sample.patch
 '
 
-test_expect_failure 'compute a patch-id outside repository' '
+test_expect_success 'compute a patch-id outside repository' '
 	git patch-id <sample.patch >patch-id.expect &&
 	(
 		cd non-repo &&

index a7fa94ce0a..605faea0c7 100755
@@ -310,4 +310,38 @@ test_expect_success 'patch-id handles diffs with one line of before/after' '
 	test_config patchid.stable true &&
 	calc_patch_id diffu1stable <diffu1
 '
+
+test_expect_failure 'patch-id computes same ID with different object hashes' '
+	test_when_finished "rm -rf repo-sha1 repo-sha256" &&
+
+	cat >diff <<-\EOF &&
+	diff --git a/bar b/bar
+	index bdaf90f..31051f6 100644
+	--- a/bar
+	+++ b/bar
+	@@ -2 +2,2 @@
+	 b
+	+c
+	EOF
+
+	git init --object-format=sha1 repo-sha1 &&
+	git -C repo-sha1 patch-id <diff >patch-id-sha1 &&
+	git init --object-format=sha256 repo-sha256 &&
+	git -C repo-sha256 patch-id <diff >patch-id-sha256 &&
+	test_cmp patch-id-sha1 patch-id-sha256
+'
+
+test_expect_success 'patch-id without repository' '
+	cat >diff <<-\EOF &&
+	diff --git a/bar b/bar
+	index bdaf90f..31051f6 100644
+	--- a/bar
+	+++ b/bar
+	@@ -2 +2,2 @@
+	 b
+	+c
+	EOF
+	nongit git patch-id <diff
+'
+
 test_done

2.45.0-145-g3e4a232f6e

From: Patrick Steinhardt <[email protected]>

The git-hash-object(1) command allows users to hash an object even without a repository. Starting with c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), this will make us hit an uninitialized hash function, which subsequently leads to a segfault.

Fix this by falling back to SHA-1 explicitly when running outside of a Git repository. Users can use GIT_DEFAULT_HASH environment to specify what hash algorithm they want, so arguably this code should not be needed.

---
 builtin/hash-object.c   | 3 +++
 t/t1007-hash-object.sh  | 6 ++++++
 t/t1517-outside-repo.sh | 2 +-
 3 files changed, 10 insertions(+), 1 deletion(-)

index 82ca6d2bfd..c767414a0c 100644
@@ -123,6 +123,9 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
 	else
 		prefix = setup_git_directory_gently(&nongit);
 
+	if (nongit && !the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	if (vpath && prefix) {
 		vpath_free = prefix_filename(prefix, vpath);
 		vpath = vpath_free;

index 64aea38486..4c138c6ca4 100755
@@ -260,4 +260,10 @@ test_expect_success '--literally with extra-long type' '
 	echo example | git hash-object -t $t --literally --stdin
 '
 
+test_expect_success '--stdin outside of repository' '
+	nongit git hash-object --stdin <hello >actual &&
+	echo "$(test_oid hello)" >expect &&
+	test_cmp expect actual
+'
+
 test_done

index ac5f3191cc..854bb8f343 100755
@@ -30,7 +30,7 @@ test_expect_success 'compute a patch-id outside repository' '
 	test_cmp patch-id.expect patch-id.actual
 '
 
-test_expect_failure 'hash-object outside repository' '
+test_expect_success 'hash-object outside repository' '
 	git hash-object --stdin <sample.patch >hash.expect &&
 	(
 		cd non-repo &&

2.45.0-145-g3e4a232f6e

By the way, this breaks the expectation of t1007 and t1517 when run with GIT_DEFAULT_HASH=sha256 (I recall that they passed with the earlier iteration of my "fall back to GIT_DEFAULT_HASH" in [1/5], but we decided abusing GIT_DEFAULT_HASH is a bad idea).

https://github.com/git/git/actions/runs/9135149292/job/25122031380

The breakage in 1517 turns out to be a thinko on my part. Outside a repository, we do use SHA-1 with our "if the_hash_algo is not set yet, default to SHA-1" in patch-id and hash-object but the way I prepared the expected output was to use whatever GIT_TEST_DEFAULT_HASH was in use. A fix would be to go outside a repository and force the hash with GIT_DEFAULT_HASH to sha1 when preparing the expected output.

I haven't looked at the breakage in 1007 yet, though.

index 6f32a40c6d..6363c8e3c4 100755
@@ -21,22 +21,24 @@ test_expect_success 'set up a non-repo directory and test file' '
 	git diff >sample.patch
 '
 
-test_expect_success 'compute a patch-id outside repository' '
-	git patch-id <sample.patch >patch-id.expect &&
+test_expect_success 'compute a patch-id outside repository (default to SHA-1)' '
 	(
 		cd non-repo &&
-		git patch-id <../sample.patch >../patch-id.actual
-	) &&
-	test_cmp patch-id.expect patch-id.actual
+		GIT_DEFAULT_HASH=sha1 \
+		git patch-id <../sample.patch >patch-id.expect &&
+		git patch-id <../sample.patch >patch-id.actual &&
+		test_cmp patch-id.expect patch-id.actual
+	)
 '
 
-test_expect_success 'hash-object outside repository' '
-	git hash-object --stdin <sample.patch >hash.expect &&
+test_expect_success 'hash-object outside repository (default to SHA-1)' '
 	(
 		cd non-repo &&
-		git hash-object --stdin <../sample.patch >../hash.actual
-	) &&
-	test_cmp hash.expect hash.actual
+		GIT_DEFAULT_HASH=sha1 \
+		git hash-object --stdin <../sample.patch >hash.expect &&
+		git hash-object --stdin <../sample.patch >hash.actual &&
+		test_cmp hash.expect hash.actual
+	)
 '
 
 test_expect_success 'apply a patch outside repository' '

This turned out to be almost trivial. A fix relative to an earlier version is that the call to test_oid helper needs to explicitly ask for SHA-1 variant, as the command invocation outside a repository uses SHA-1 (not due to falling back to a hardcoded default, but by an explicit fallback in the "git hash-object" itself.

I'll send a v5 of the whole series sometime later (if I have time it may happen today but otherwise tomorrow).

Thanks.

index 64aea38486..d73a5cc237 100755
@@ -260,4 +260,10 @@ test_expect_success '--literally with extra-long type' '
 	echo example | git hash-object -t $t --literally --stdin
 '
 
+test_expect_success '--stdin outside of repository (uses SHA-1)' '
+	nongit git hash-object --stdin <hello >actual &&
+	echo "$(test_oid --hash=sha1 hello)" >expect &&
+	test_cmp expect actual
+'
+
 test_done

index 82ca6d2bfd..c767414a0c 100644
@@ -123,6 +123,9 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
 	else
 		prefix = setup_git_directory_gently(&nongit);
 
+	if (nongit && !the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	if (vpath && prefix) {
 		vpath_free = prefix_filename(prefix, vpath);
 		vpath = vpath_free;

"git apply" can work outside a repository as a better "GNU patch", but when it does so, it still assumed that it can access the_hash_algo, which is no longer true in the new world order.

Make sure we explicitly fall back to SHA-1 algorithm for backward compatibility.

---
 builtin/apply.c         | 4 ++++
 t/t1517-outside-repo.sh | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

index 861a01910c..e9175f820f 100644
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "gettext.h"
 #include "repository.h"
+#include "hash.h"
 #include "apply.h"
 
 static const char * const apply_usage[] = {

@@ -18,6 +19,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix)
 	if (init_apply_state(&state, the_repository, prefix))
 		exit(128);
 
+	if (!the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	argc = apply_parse_options(argc, argv,
 				   &state, &force_apply, &options,
 				   apply_usage);

index 854bb8f343..6f32a40c6d 100755
@@ -39,7 +39,7 @@ test_expect_success 'hash-object outside repository' '
 	test_cmp hash.expect hash.actual
 '
 
-test_expect_failure 'apply a patch outside repository' '
+test_expect_success 'apply a patch outside repository' '
 	(
 		cd non-repo &&
 		cp ../nums.old nums &&

2.45.0-145-g3e4a232f6e

A change recently merged to 'next' stops us from defaulting to using SHA-1 unless other code (like a logic early in the start-up sequence to see what hash is being used in the repository we are working in) explicitly sets it, leading to a (deliberate) crash of "git" when we forgot to cover certain code paths.

It turns out we have a few. Notable ones are all operations that are designed to work outside a repository. We should go over all such code paths and give them a reasonable default when there is one available (e.g. for historical reasons, patch-id is documented to work with SHA-1 hashes, so arguably it, or at least when it is invoked with the "--stable" option, should do so everywhere, not just in SHA-1 repositories, but in SHA-256 repository or outside any repository). In the meantime, if an end-user hits such a "bug" before we can fix it, it would be nice to give them an escape hatch to restore the historical behaviour of falling back to use SHA-1.

These patches are designed to apply on a merge of c8aed5e8 (repository: stop setting SHA1 as the default object hash, 2024-05-07) into 3e4a232f (The third batch, 2024-05-13), which has been the same base throughout the past iterations.

In this fifth iteration:

- The first step no longer falls back to GIT_DEFAULT_HASH; the escape hatch is a dedicated GIT_TEST_DEFAULT_HASH_ALGO environment variable, but hopefully we do not have to advertise it all that often.

- The second step has been simplified somewhat to use the "nongit" helper when we only need to run a single "git" command in t1517. The way the expected output files were prepared in the previous versions did not correctly force use of SHA-1 algorithm, which has been corrected. The third step and fourth step for t1517 continue to be "flip expect_failure to expect_success", but you can see context differences in the range-diff.

- The fourth step also has a fix for t1007 where the previous iterations did not correctly force use of SHA-1 to prepare the expected output.

Otherwise this round should be ready, modulo possible typoes.

Junio C Hamano (3):
setup: add an escape hatch for "no more default hash algorithm" change
t1517: test commands that are designed to be run outside repository
apply: fix uninitialized hash function

Patrick Steinhardt (2):
builtin/patch-id: fix uninitialized hash function builtin/hash-object: fix uninitialized hash function

 builtin/apply.c         |  4 +++
 builtin/hash-object.c   |  3 +++
 builtin/patch-id.c      | 13 +++++++++
 repository.c            | 44 ++++++++++++++++++++++++++++++
 t/t1007-hash-object.sh  |  6 +++++
 t/t1517-outside-repo.sh | 59 +++++++++++++++++++++++++++++++++++++++++
 t/t4204-patch-id.sh     | 34 ++++++++++++++++++++++++
 7 files changed, 163 insertions(+)
 create mode 100755 t/t1517-outside-repo.sh

2.45.1-216-g4365c6fcf9

1:  3038f73e1c ! 1:  f5e6868a61 setup: add an escape hatch for "no more default hash algorithm" change
    @@ Commit message
         Partially revert c8aed5e8 (repository: stop setting SHA1 as the
         default object hash, 2024-05-07), to keep end-user systems still
         broken when we have gap in our test coverage but yet give them an
    -    escape hatch to set the GIT_DEFAULT_HASH environment variable to
    -    "sha1" in order to revert to the previous behaviour.  This variable
    -    has been in use for using SHA-256 hash by default, and it should be
    -    a better fit than inventing a new and test-only knob.
    +    escape hatch to set the GIT_TEST_DEFAULT_HASH_ALGO environment
    +    variable to "sha1" in order to revert to the previous behaviour.
     
    -    Signed-off-by: Junio C Hamano <[email protected]>
    +    Due to the way the end-user facing GIT_DEFAULT_HASH environment
    +    variable is used in our test suite, we unfortunately cannot reuse it
    +    for this purpose.
     
    - ## environment.h ##
    -@@ environment.h: const char *getenv_safe(struct strvec *argv, const char *name);
    - #define GIT_OPTIONAL_LOCKS_ENVIRONMENT "GIT_OPTIONAL_LOCKS"
    - #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
    - #define GIT_ATTR_SOURCE_ENVIRONMENT "GIT_ATTR_SOURCE"
    -+#define GIT_DEFAULT_HASH_ENVIRONMENT "GIT_DEFAULT_HASH"
    - 
    - /*
    -  * Environment variable used in handshaking the wire protocol.
    +    Signed-off-by: Junio C Hamano <[email protected]>
     
      ## repository.c ##
    -@@
    - #include "git-compat-util.h"
    - #include "abspath.h"
    -+#include "environment.h"
    - #include "repository.h"
    - #include "object-store-ll.h"
    - #include "config.h"
     @@
      static struct repository the_repo;
      struct repository *the_repository = &the_repo;
      
    ++/*
    ++ * An escape hatch: if we hit a bug in the production code that fails
    ++ * to set an appropriate hash algorithm (most likely to happen when
    ++ * running outside a repository), we can tell the user who reported
    ++ * the crash to set the environment variable to "sha1" (all lowercase)
    ++ * to revert to the historical behaviour of defaulting to SHA-1.
    ++ */
     +static void set_default_hash_algo(struct repository *repo)
     +{
     +	const char *hash_name;
     +	int algo;
     +
    -+	hash_name = getenv(GIT_DEFAULT_HASH_ENVIRONMENT);
    ++	hash_name = getenv("GIT_TEST_DEFAULT_HASH_ALGO");
     +	if (!hash_name)
     +		return;
     +	algo = hash_algo_by_name(hash_name);
    -+
    -+	/*
    -+	 * NEEDSWORK: after all, falling back to SHA-1 by assigning
    -+	 * GIT_HASH_SHA1 to algo here, instead of returning, may give
    -+	 * us better behaviour.
    -+	 */
     +	if (algo == GIT_HASH_UNKNOWN)
     +		return;
     +
    @@ repository.c: void initialize_repository(struct repository *repo)
     +	 * of the hashed value to see if their input looks like a
     +	 * plausible hash value.
     +	 *
    -+	 * We are in the process of identifying the codepaths and
    ++	 * We are in the process of identifying such code paths and
     +	 * giving them an appropriate default individually; any
    -+	 * unconverted codepath that tries to access the_hash_algo
    ++	 * unconverted code paths that try to access the_hash_algo
     +	 * will thus fail.  The end-users however have an escape hatch
    -+	 * to set GIT_DEFAULT_HASH environment variable to "sha1" get
    -+	 * back the old behaviour of defaulting to SHA-1.
    ++	 * to set GIT_TEST_DEFAULT_HASH_ALGO environment variable to
    ++	 * "sha1" to get back the old behaviour of defaulting to SHA-1.
    ++	 *
    ++	 * This escape hatch is deliberately kept unadvertised, so
    ++	 * that they see crashes and we can get a report before
    ++	 * telling them about it.
     +	 */
     +	if (repo == the_repository)
     +		set_default_hash_algo(repo);
      }
      
      static void expand_base_dir(char **out, const char *in,
    -
    - ## setup.c ##
    -@@ setup.c: int daemonize(void)
    - #define TEST_FILEMODE 1
    - #endif
    - 
    --#define GIT_DEFAULT_HASH_ENVIRONMENT "GIT_DEFAULT_HASH"
    --
    - static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
    - 			     DIR *dir)
    - {
2:  e56ae68961 ! 2:  f3f9bf0480 t1517: test commands that are designed to be run outside repository
    @@ t/t1517-outside-repo.sh (new)
     +	git diff >sample.patch
     +'
     +
    -+test_expect_failure 'compute a patch-id outside repository' '
    -+	git patch-id <sample.patch >patch-id.expect &&
    -+	(
    -+		cd non-repo &&
    -+		git patch-id <../sample.patch >../patch-id.actual
    -+	) &&
    ++test_expect_failure 'compute a patch-id outside repository (uses SHA-1)' '
    ++	nongit env GIT_DEFAULT_HASH=sha1 \
    ++		git patch-id <sample.patch >patch-id.expect &&
    ++	nongit \
    ++		git patch-id <sample.patch >patch-id.actual &&
     +	test_cmp patch-id.expect patch-id.actual
     +'
     +
    -+test_expect_failure 'hash-object outside repository' '
    -+	git hash-object --stdin <sample.patch >hash.expect &&
    -+	(
    -+		cd non-repo &&
    -+		git hash-object --stdin <../sample.patch >../hash.actual
    -+	) &&
    ++test_expect_failure 'hash-object outside repository (uses SHA-1)' '
    ++	nongit env GIT_DEFAULT_HASH=sha1 \
    ++		git hash-object --stdin <sample.patch >hash.expect &&
    ++	nongit \
    ++		git hash-object --stdin <sample.patch >hash.actual &&
     +	test_cmp hash.expect hash.actual
     +'
     +
3:  e1ae0e95fc ! 3:  246cf395d0 builtin/patch-id: fix uninitialized hash function
    @@ t/t1517-outside-repo.sh: test_expect_success 'set up a non-repo directory and te
      	git diff >sample.patch
      '
      
    --test_expect_failure 'compute a patch-id outside repository' '
    -+test_expect_success 'compute a patch-id outside repository' '
    - 	git patch-id <sample.patch >patch-id.expect &&
    - 	(
    - 		cd non-repo &&
    +-test_expect_failure 'compute a patch-id outside repository (uses SHA-1)' '
    ++test_expect_success 'compute a patch-id outside repository (uses SHA-1)' '
    + 	nongit env GIT_DEFAULT_HASH=sha1 \
    + 		git patch-id <sample.patch >patch-id.expect &&
    + 	nongit \
     
      ## t/t4204-patch-id.sh ##
     @@ t/t4204-patch-id.sh: test_expect_success 'patch-id handles diffs with one line of before/after' '
4:  1c8ce0d024 ! 4:  fe14490548 builtin/hash-object: fix uninitialized hash function
    @@ t/t1007-hash-object.sh: test_expect_success '--literally with extra-long type' '
      	echo example | git hash-object -t $t --literally --stdin
      '
      
    -+test_expect_success '--stdin outside of repository' '
    ++test_expect_success '--stdin outside of repository (uses SHA-1)' '
     +	nongit git hash-object --stdin <hello >actual &&
    -+	echo "$(test_oid hello)" >expect &&
    ++	echo "$(test_oid --hash=sha1 hello)" >expect &&
     +	test_cmp expect actual
     +'
     +
      test_done
     
      ## t/t1517-outside-repo.sh ##
    -@@ t/t1517-outside-repo.sh: test_expect_success 'compute a patch-id outside repository' '
    +@@ t/t1517-outside-repo.sh: test_expect_success 'compute a patch-id outside repository (uses SHA-1)' '
      	test_cmp patch-id.expect patch-id.actual
      '
      
    --test_expect_failure 'hash-object outside repository' '
    -+test_expect_success 'hash-object outside repository' '
    - 	git hash-object --stdin <sample.patch >hash.expect &&
    - 	(
    - 		cd non-repo &&
    +-test_expect_failure 'hash-object outside repository (uses SHA-1)' '
    ++test_expect_success 'hash-object outside repository (uses SHA-1)' '
    + 	nongit env GIT_DEFAULT_HASH=sha1 \
    + 		git hash-object --stdin <sample.patch >hash.expect &&
    + 	nongit \
5:  5b0ec43757 ! 5:  c6d5e68374 apply: fix uninitialized hash function
    @@ builtin/apply.c: int cmd_apply(int argc, const char **argv, const char *prefix)
      				   apply_usage);
     
      ## t/t1517-outside-repo.sh ##
    -@@ t/t1517-outside-repo.sh: test_expect_success 'hash-object outside repository' '
    +@@ t/t1517-outside-repo.sh: test_expect_success 'hash-object outside repository (uses SHA-1)' '
      	test_cmp hash.expect hash.actual
      '

Partially revert c8aed5e8 (repository: stop setting SHA1 as the default object hash, 2024-05-07), to keep end-user systems still broken when we have gap in our test coverage but yet give them an escape hatch to set the GIT_TEST_DEFAULT_HASH_ALGO environment variable to "sha1" in order to revert to the previous behaviour.

Due to the way the end-user facing GIT_DEFAULT_HASH environment variable is used in our test suite, we unfortunately cannot reuse it for this purpose.

---
 repository.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

index 15c10015b0..c62e329878 100644
@@ -19,6 +19,28 @@
 static struct repository the_repo;
 struct repository *the_repository = &the_repo;
 
+/*
+ * An escape hatch: if we hit a bug in the production code that fails
+ * to set an appropriate hash algorithm (most likely to happen when
+ * running outside a repository), we can tell the user who reported
+ * the crash to set the environment variable to "sha1" (all lowercase)
+ * to revert to the historical behaviour of defaulting to SHA-1.
+ */
+static void set_default_hash_algo(struct repository *repo)
+{
+	const char *hash_name;
+	int algo;
+
+	hash_name = getenv("GIT_TEST_DEFAULT_HASH_ALGO");
+	if (!hash_name)
+		return;
+	algo = hash_algo_by_name(hash_name);
+	if (algo == GIT_HASH_UNKNOWN)
+		return;
+
+	repo_set_hash_algo(repo, algo);
+}
+
 void initialize_repository(struct repository *repo)
 {
 	repo->objects = raw_object_store_new();

@@ -26,6 +48,28 @@ void initialize_repository(struct repository *repo)
 	repo->parsed_objects = parsed_object_pool_new();
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
+
+	/*
+	 * When a command runs inside a repository, it learns what
+	 * hash algorithm is in use from the repository, but some
+	 * commands are designed to work outside a repository, yet
+	 * they want to access the_hash_algo, if only for the length
+	 * of the hashed value to see if their input looks like a
+	 * plausible hash value.
+	 *
+	 * We are in the process of identifying such code paths and
+	 * giving them an appropriate default individually; any
+	 * unconverted code paths that try to access the_hash_algo
+	 * will thus fail.  The end-users however have an escape hatch
+	 * to set GIT_TEST_DEFAULT_HASH_ALGO environment variable to
+	 * "sha1" to get back the old behaviour of defaulting to SHA-1.
+	 *
+	 * This escape hatch is deliberately kept unadvertised, so
+	 * that they see crashes and we can get a report before
+	 * telling them about it.
+	 */
+	if (repo == the_repository)
+		set_default_hash_algo(repo);
 }
 
 static void expand_base_dir(char **out, const char *in,

2.45.1-216-g4365c6fcf9

Okay, so this now really only is an escape hatch for users as we do not specify it in our tests anymore. It does make me wonder whether we want to name the envvar accordingly, but don't mind it much either way. It is only intended to be a stop gap solution anyway that we can eventually drop once we are sufficiently sure that there is no further breakage.

Patrick

Yes, I think the name of that escape hatch variable should be irrelevant by the time we are reasonably confident that we could remove it ;-)

But the proposed log message should make that intention more clear. How does this read?

... give them an escape hatch ... in order to revert to the previous behaviour, just in case we haven't done a thorough job in fixing the fallout from c8aed5e8. After we build confidence, we should remove the escape hatch support, but we are not there yet after only fixing three commands (hash-object, apply, and patch-id) in this series.

Thanks.

A few commands, like "git apply" and "git patch-id", have been broken with a recent change to stop setting the default hash algorithm to SHA-1. Test them and fix them in later commits.

---
 t/t1517-outside-repo.sh | 59 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100755 t/t1517-outside-repo.sh

new file mode 100755
index 0000000000..389974d9fb
@@ -0,0 +1,59 @@
+#!/bin/sh
+
+test_description='check random commands outside repo'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'set up a non-repo directory and test file' '
+	GIT_CEILING_DIRECTORIES=$(pwd) &&
+	export GIT_CEILING_DIRECTORIES &&
+	mkdir non-repo &&
+	(
+		cd non-repo &&
+		# confirm that git does not find a repo
+		test_must_fail git rev-parse --git-dir
+	) &&
+	test_write_lines one two three four >nums &&
+	git add nums &&
+	cp nums nums.old &&
+	test_write_lines five >>nums &&
+	git diff >sample.patch
+'
+
+test_expect_failure 'compute a patch-id outside repository (uses SHA-1)' '
+	nongit env GIT_DEFAULT_HASH=sha1 \
+		git patch-id <sample.patch >patch-id.expect &&
+	nongit \
+		git patch-id <sample.patch >patch-id.actual &&
+	test_cmp patch-id.expect patch-id.actual
+'
+
+test_expect_failure 'hash-object outside repository (uses SHA-1)' '
+	nongit env GIT_DEFAULT_HASH=sha1 \
+		git hash-object --stdin <sample.patch >hash.expect &&
+	nongit \
+		git hash-object --stdin <sample.patch >hash.actual &&
+	test_cmp hash.expect hash.actual
+'
+
+test_expect_failure 'apply a patch outside repository' '
+	(
+		cd non-repo &&
+		cp ../nums.old nums &&
+		git apply ../sample.patch
+	) &&
+	test_cmp nums non-repo/nums
+'
+
+test_expect_success 'grep outside repository' '
+	git grep --cached two >expect &&
+	(
+		cd non-repo &&
+		cp ../nums.old nums &&
+		git grep --no-index two >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_done

2.45.1-216-g4365c6fcf9

From: Patrick Steinhardt <[email protected]>

In c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), we have adapted initialize_repository() to no longer set up a default hash function. As this function is also used to set up the_repository, the consequence is that the_hash_algo will now by default be a NULL pointer unless the hash algorithm was configured properly. This is done as a mechanism to detect cases where we may be using the wrong hash function by accident.

This change now causes git-patch-id(1) to segfault when it's run outside of a repository. As this command can read diffs from stdin, it does not necessarily need a repository, but then relies on the_hash_algo to compute the patch ID itself.

It is somewhat dubious that git-patch-id(1) relies on the_hash_algo in the first place. Quoting its manpage:

A "patch ID" is nothing but a sum of SHA-1 of the file diffs associated with a patch, with line numbers ignored. As such, it’s "reasonably stable", but at the same time also reasonably unique, i.e., two patches that have the same "patch ID" are almost guaranteed to be the same thing.

We explicitly document patch IDs to be using SHA-1. Furthermore, patch IDs are supposed to be stable for most of the part. But even with the same input, the patch IDs will now be different depending on the repo's configured object hash.

Work around the issue by setting up SHA-1 when there was no startup repository for now. This is arguably not the correct fix, but for now we rather want to focus on getting the segfault fixed.

---
 builtin/patch-id.c      | 13 +++++++++++++
 t/t1517-outside-repo.sh |  2 +-
 t/t4204-patch-id.sh     | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 1 deletion(-)

index 3894d2b970..583099cacf 100644
@@ -5,6 +5,7 @@
 #include "hash.h"
 #include "hex.h"
 #include "parse-options.h"
+#include "setup.h"
 
 static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)
 {

@@ -237,6 +238,18 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_patch_id_options,
 			     patch_id_usage, 0);
 
+	/*
+	 * We rely on `the_hash_algo` to compute patch IDs. This is dubious as
+	 * it means that the hash algorithm now depends on the object hash of
+	 * the repository, even though git-patch-id(1) clearly defines that
+	 * patch IDs always use SHA1.
+	 *
+	 * NEEDSWORK: This hack should be removed in favor of converting
+	 * the code that computes patch IDs to always use SHA1.
+	 */
+	if (!the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	generate_id_list(opts ? opts > 1 : config.stable,
 			 opts ? opts == 3 : config.verbatim);
 	return 0;

index 389974d9fb..278ef57b3a 100755
@@ -21,7 +21,7 @@ test_expect_success 'set up a non-repo directory and test file' '
 	git diff >sample.patch
 '
 
-test_expect_failure 'compute a patch-id outside repository (uses SHA-1)' '
+test_expect_success 'compute a patch-id outside repository (uses SHA-1)' '
 	nongit env GIT_DEFAULT_HASH=sha1 \
 		git patch-id <sample.patch >patch-id.expect &&
 	nongit \

index a7fa94ce0a..605faea0c7 100755
@@ -310,4 +310,38 @@ test_expect_success 'patch-id handles diffs with one line of before/after' '
 	test_config patchid.stable true &&
 	calc_patch_id diffu1stable <diffu1
 '
+
+test_expect_failure 'patch-id computes same ID with different object hashes' '
+	test_when_finished "rm -rf repo-sha1 repo-sha256" &&
+
+	cat >diff <<-\EOF &&
+	diff --git a/bar b/bar
+	index bdaf90f..31051f6 100644
+	--- a/bar
+	+++ b/bar
+	@@ -2 +2,2 @@
+	 b
+	+c
+	EOF
+
+	git init --object-format=sha1 repo-sha1 &&
+	git -C repo-sha1 patch-id <diff >patch-id-sha1 &&
+	git init --object-format=sha256 repo-sha256 &&
+	git -C repo-sha256 patch-id <diff >patch-id-sha256 &&
+	test_cmp patch-id-sha1 patch-id-sha256
+'
+
+test_expect_success 'patch-id without repository' '
+	cat >diff <<-\EOF &&
+	diff --git a/bar b/bar
+	index bdaf90f..31051f6 100644
+	--- a/bar
+	+++ b/bar
+	@@ -2 +2,2 @@
+	 b
+	+c
+	EOF
+	nongit git patch-id <diff
+'
+
 test_done

2.45.1-216-g4365c6fcf9

From: Patrick Steinhardt <[email protected]>

The git-hash-object(1) command allows users to hash an object even without a repository. Starting with c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), this will make us hit an uninitialized hash function, which subsequently leads to a segfault.

Fix this by falling back to SHA-1 explicitly when running outside of a Git repository. Users can use GIT_DEFAULT_HASH environment to specify what hash algorithm they want, so arguably this code should not be needed.

---
 builtin/hash-object.c   | 3 +++
 t/t1007-hash-object.sh  | 6 ++++++
 t/t1517-outside-repo.sh | 2 +-
 3 files changed, 10 insertions(+), 1 deletion(-)

index 82ca6d2bfd..c767414a0c 100644
@@ -123,6 +123,9 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
 	else
 		prefix = setup_git_directory_gently(&nongit);
 
+	if (nongit && !the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	if (vpath && prefix) {
 		vpath_free = prefix_filename(prefix, vpath);
 		vpath = vpath_free;

index 64aea38486..d73a5cc237 100755
@@ -260,4 +260,10 @@ test_expect_success '--literally with extra-long type' '
 	echo example | git hash-object -t $t --literally --stdin
 '
 
+test_expect_success '--stdin outside of repository (uses SHA-1)' '
+	nongit git hash-object --stdin <hello >actual &&
+	echo "$(test_oid --hash=sha1 hello)" >expect &&
+	test_cmp expect actual
+'
+
 test_done

index 278ef57b3a..2d8982d61a 100755
@@ -29,7 +29,7 @@ test_expect_success 'compute a patch-id outside repository (uses SHA-1)' '
 	test_cmp patch-id.expect patch-id.actual
 '
 
-test_expect_failure 'hash-object outside repository (uses SHA-1)' '
+test_expect_success 'hash-object outside repository (uses SHA-1)' '
 	nongit env GIT_DEFAULT_HASH=sha1 \
 		git hash-object --stdin <sample.patch >hash.expect &&
 	nongit \

2.45.1-216-g4365c6fcf9

"git apply" can work outside a repository as a better "GNU patch", but when it does so, it still assumed that it can access the_hash_algo, which is no longer true in the new world order.

Make sure we explicitly fall back to SHA-1 algorithm for backward compatibility.

---
 builtin/apply.c         | 4 ++++
 t/t1517-outside-repo.sh | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

index 861a01910c..e9175f820f 100644
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "gettext.h"
 #include "repository.h"
+#include "hash.h"
 #include "apply.h"
 
 static const char * const apply_usage[] = {

@@ -18,6 +19,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix)
 	if (init_apply_state(&state, the_repository, prefix))
 		exit(128);
 
+	if (!the_hash_algo)
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+
 	argc = apply_parse_options(argc, argv,
 				   &state, &force_apply, &options,
 				   apply_usage);

index 2d8982d61a..557808ffa7 100755
@@ -37,7 +37,7 @@ test_expect_success 'hash-object outside repository (uses SHA-1)' '
 	test_cmp hash.expect hash.actual
 '
 
-test_expect_failure 'apply a patch outside repository' '
+test_expect_success 'apply a patch outside repository' '
 	(
 		cd non-repo &&
 		cp ../nums.old nums &&

2.45.1-216-g4365c6fcf9

We may want a comment here that says

we could to redo the "apply.c" machinery to make this arbitrary fallback unnecessary, but the benefit to do so is dubious and the risk of breaking the code is probably not worth the effort.

When working as "better GNU patch" mode without a repository, we should not and do not use the_hash_algo for the purpose of hashing at all. We do not do the binary diff (because we cannot grab the preimage object out of the object store (that does not exist) to apply the delta to form the postimage, we do not do the 3-way fallback using the preimage blob object names that appear on the "index" lines. As far as I recall, the only thing we use the_hash_algo for is for the max length of the hash to ask "is this hexadecimal string a plausible looking object name? We are parsing a line that started with 'index ' and trying to see if the line syntactically looks like a valid 'index' line" and the like. If we assume SHA-1 and if somebody tries to injest a SHA-256 --full-index patch, that logic may say "nah, we didn't find a valid 'index' header", but I think we'll just leave the fields like old-object blank, which does not affect anything because we won't do "apply --3way" anyway.

So we _could_ identify such places and tell the code "when the_hash_algo is NULL, instead of the_hash_algo->hexsz, use 0 as hexsz" (this example is from apply.c:gitdiff_index()) and it indeed was the approach I tried in my unpublished draft before the first version I posted. It quickly got ugly.

I have two smallish comments, but neither of them really have to be addressed. Overall I very much agree with this iteration and think that it's the right way to go.

Thanks!

Patrick

I've locally done the following locally but it probably does not need to be resent to the list before merging down to 'next'.

1: b23a93597c ! 1: d3b2ff75fd setup: add an escape hatch for "no more default hash algorithm" change
@@ Commit message
default object hash, 2024-05-07), to keep end-user systems still
broken when we have gap in our test coverage but yet give them an
escape hatch to set the GIT_TEST_DEFAULT_HASH_ALGO environment
- variable to "sha1" in order to revert to the previous behaviour.
+ variable to "sha1" in order to revert to the previous behaviour, in
+ case we haven't done a thorough job in fixing the fallout from
+ c8aed5e8. After we build confidence, we should remove the escape
+ hatch support, but we are not there yet after only fixing three
+ commands (hash-object, apply, and patch-id) in this series.

Due to the way the end-user facing GIT_DEFAULT_HASH environment variable is used in our test suite, we unfortunately cannot reuse it

2:  6a20370944 = 2:  abece6e970 t1517: test commands that are designed to be run outside repository
3:  fa258c5d47 = 3:  4a1c95931f builtin/patch-id: fix uninitialized hash function
4:  164d340cbe = 4:  8d058b8024 builtin/hash-object: fix uninitialized hash function
5:  bd0246eb51 ! 5:  4674ab682d apply: fix uninitialized hash function

@@ Commit message
Make sure we explicitly fall back to SHA-1 algorithm for backward compatibility.

+ It is of dubious value to make this configurable to other hash
+ algorithms, as the code does not use the_hash_algo for hashing
+ purposes when working outside a repository (which is how
+ the_hash_algo is left to NULL)---it is only used to learn the max
+ length of the hash when parsing the object names on the "index"
+ line, but failing to parse the "index" line is not a hard failure,
+ and the program does not support operations like applying binary
+ patches and --3way fallback that requires object access outside a
+ repository.
+

## builtin/apply.c ##
@@ builtin/apply.c: int cmd_apply(int argc, const char **argv, const char *prefix)
if (init_apply_state(&state, the_repository, prefix))
exit(128);

++ /*
++ * We could to redo the "apply.c" machinery to make this
++ * arbitrary fallback unnecessary, but it is dubious that it
++ * is worth the effort.
++ * cf. https://lore.kernel.org/git/[email protected]/
++ */
+ if (!the_hash_algo)
+ repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+

Thanks, the diff looks good to me.

Patrick