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

2 messages, 2 authors, 2017-08-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2017-08-15 18:23:10

Patryk Obara [off-list ref] writes:

This prevents compilation error if GIT_MAX_RAWSZ is different than 20.

Signed-off-by: Patryk Obara <redacted>
---
 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.

quoted hunk

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

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

From: Stefan Beller <hidden>
Date: 2017-08-15 18:30:00

On Tue, Aug 15, 2017 at 11:23 AM, Junio C Hamano [off-list ref] wrote:

Patryk Obara [off-list ref] writes:

quoted

This prevents compilation error if GIT_MAX_RAWSZ is different than 20.

Signed-off-by: Patryk Obara <redacted>
---
 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.

Once we have 2 hash functions usable in a local Git installation,
this would be wasteful for the smaller hash function (and the
related grafts).

I think Jonathan once envisioned an 'optimized' version as a
second step, maybe this is a good time to discuss how we'd
get the right size for e.g. allocating memory, as _MAX_ seems
to be not the correct solution long term?

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help