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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.
> +	/*
> +	 * 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.