Re: [PATCH v3 3/5] builtin/patch-id: fix uninitialized hash function

From: Junio C Hamano <gitster@pobox.com>
Date: May 13, 2024, 23:11 UTC
Message-ID: <xmqqbk59cnp6.fsf@gitster.g>
In-Reply-To: <20240513224127.2042052-4-gitster@pobox.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 8 quoted lines

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

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.

Show 11 quoted lines

> +	/*
> +	 * 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);

Hmph, in other places I did

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

to find the case where we need a reasonable default.

Is there a practical difference? If there isn't we should standardise one and use the same test consistently everywhere.

Not that it matters for this particular case, where we in the longer term should be hardcoding the use of SHA-1 even in SHA-256 repository for the pupose of computing the patch-id.

Thanks.