Re: [PATCH 0/2] Fix use of uninitialized hash algos

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


Patrick Steinhardt <ps@xxxxxx> writes:

> 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.

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).

> 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.

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/ | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git c/t/ w/t/
new file mode 100755
index 0000000000..4c595c2ff7
--- /dev/null
+++ w/t/
@@ -0,0 +1,42 @@
+test_description='check random commands outside repo'
+. ./
+test_expect_success 'set up a non-repo directory and test file' '
+	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

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux