Re: [PATCH 2/5] sha1_file: fix hardcoded size in null_sha1

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 15, 2017, 18:23 UTC
Message-ID: <xmqq1socsnay.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <a21088f049390828cdee957f88503e8466e1d34e.1502796628.git.patryk.obara@gmail.com>

Patryk Obara <patryk.obara@gmail.com> writes:

Show 6 quoted lines

> This prevents compilation error if GIT_MAX_RAWSZ is different than 20.
>
> Signed-off-by: Patryk Obara <patryk.obara@gmail.com>
> ---
>  sha1_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I think this is OK for "null" thing, but in general I feel ambivalent when I see the use of "MAX" thing.

Use of "MAX" here implies that we wish to support multiple hashes at the same time in a single binary, so that longer or shorter hashes fit there. But in such a case, the callers would want a union of some sort so that they can say "I am using SHA2, give me the null thing"?

I said this is OK for "null" because we assume we will use ^\0{len}$ for any hash function we choose as the "impossible" value, and for that particular use pattern, we do not need such a union. Just letting the caller peek at an appropriate number of bytes at the beginning of that NUL buffer for hash the caller wants to use is sufficient.

Thanks.

Show 14 quoted lines

>
> diff --git a/sha1_file.c b/sha1_file.c
> index b60ae15..f5b5bec 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -32,7 +32,7 @@
>  #define SZ_FMT PRIuMAX
>  static inline uintmax_t sz_fmt(size_t s) { return s; }
>  
> -const unsigned char null_sha1[20];
> +const unsigned char null_sha1[GIT_MAX_RAWSZ];
>  const struct object_id null_oid;
>  const struct object_id empty_tree_oid = {
>  	EMPTY_TREE_SHA1_BIN_LITERAL