* [PATCH 0/7] git_hash_*() quality-of-life improvements
@ 2026-07-07 4:55 Jeff King
2026-07-07 5:01 ` [PATCH 1/7] hash: use git_hash_init() consistently Jeff King
` (8 more replies)
0 siblings, 9 replies; 35+ messages in thread
From: Jeff King @ 2026-07-07 4:55 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
This implements the "idempotent git_hash_discard()" discussed in this
subthread:
https://lore.kernel.org/git/20260702080707.GG2029434@coredump.intra.peff.net/
with associated cleanups.
It should be applied on top of jk/hash-algo-leak-fixes.
[1/7]: hash: use git_hash_init() consistently
[2/7]: hash: convert remaining direct function calls
[3/7]: hash: document function pointers and wrappers
[4/7]: hash: make git_hash_discard() idempotent
[5/7]: csum-file: use idempotent git_hash_discard()
[6/7]: http: use idempotent git_hash_discard()
[7/7]: hash: check ctx->active flag in all wrapper functions
builtin/fast-import.c | 4 +--
builtin/index-pack.c | 6 ++--
builtin/patch-id.c | 2 +-
builtin/receive-pack.c | 6 ++--
builtin/submodule--helper.c | 10 +++---
builtin/unpack-objects.c | 4 +--
csum-file.c | 23 +++++---------
diff.c | 4 +--
hash.c | 16 ++++++++++
hash.h | 44 +++++++++++++++++++-------
http-push.c | 2 +-
http.c | 9 ++----
http.h | 1 -
object-file.c | 17 +++++-----
pack-check.c | 2 +-
pack-write.c | 6 ++--
read-cache.c | 6 ++--
rerere.c | 5 +--
t/helper/test-hash-speed.c | 2 +-
t/helper/test-hash.c | 2 +-
t/helper/test-synthesize.c | 33 ++++++++++---------
t/unit-tests/u-hash.c | 2 +-
tools/coccinelle/hash.cocci | 63 +++++++++++++++++++++++++++++++++++++
trace2/tr2_sid.c | 2 +-
24 files changed, 181 insertions(+), 90 deletions(-)
create mode 100644 tools/coccinelle/hash.cocci
-Peff
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/7] hash: use git_hash_init() consistently
2026-07-07 4:55 [PATCH 0/7] git_hash_*() quality-of-life improvements Jeff King
@ 2026-07-07 5:01 ` Jeff King
2026-07-07 14:39 ` Junio C Hamano
2026-07-07 21:25 ` brian m. carlson
2026-07-07 5:04 ` [PATCH 2/7] hash: convert remaining direct function calls Jeff King
` (7 subsequent siblings)
8 siblings, 2 replies; 35+ messages in thread
From: Jeff King @ 2026-07-07 5:01 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
We'd like to add more logic to git_hash_init(), but many callers skip it
and call algop->init_fn() directly. Let's make sure we're consistently
using the wrapper by adding a coccinelle rule.
Besides the coccinelle file itself, this is a purely mechanical
conversion based on the patch it generates. There should be no bare
init_fn() calls left (except for the one in the wrapper).
Signed-off-by: Jeff King <peff@peff.net>
---
It feels like the "expression ALGO" in the rule should be a
"git_hash_algo", but I had trouble getting coccinelle to recognize all
cases when I did that. Probably not worth digging too far into, as
the presence of the git_hash_ctx type means we should never hit any
false positives.
builtin/fast-import.c | 4 ++--
builtin/index-pack.c | 6 +++---
builtin/patch-id.c | 2 +-
builtin/receive-pack.c | 6 +++---
builtin/submodule--helper.c | 2 +-
builtin/unpack-objects.c | 4 ++--
csum-file.c | 6 +++---
diff.c | 4 ++--
http-push.c | 2 +-
http.c | 4 ++--
object-file.c | 17 +++++++++--------
pack-check.c | 2 +-
pack-write.c | 6 +++---
read-cache.c | 6 +++---
rerere.c | 5 +++--
t/helper/test-hash-speed.c | 2 +-
t/helper/test-hash.c | 2 +-
t/helper/test-synthesize.c | 4 ++--
t/unit-tests/u-hash.c | 2 +-
tools/coccinelle/hash.cocci | 10 ++++++++++
trace2/tr2_sid.c | 2 +-
21 files changed, 55 insertions(+), 43 deletions(-)
create mode 100644 tools/coccinelle/hash.cocci
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index f6473dcc8e..6692f7cd81 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -969,7 +969,7 @@ static int store_object(
hdrlen = format_object_header((char *)hdr, sizeof(hdr), type,
dat->len);
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, hdr, hdrlen);
git_hash_update(&c, dat->buf, dat->len);
git_hash_final_oid(&oid, &c);
@@ -1131,7 +1131,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
hdrlen = format_object_header((char *)out_buf, out_sz, OBJ_BLOB, len);
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, out_buf, hdrlen);
crc32_begin(pack_file);
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f396658468..53a8cb9dd7 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -374,7 +374,7 @@ static const char *open_pack_file(const char *pack_name)
output_fd = -1;
nothread_data.pack_fd = input_fd;
}
- the_hash_algo->init_fn(&input_ctx);
+ git_hash_init(&input_ctx, the_hash_algo);
return pack_name;
}
@@ -481,7 +481,7 @@ static void *unpack_entry_data(off_t offset, size_t size,
if (!is_delta_type(type)) {
hdrlen = format_object_header(hdr, sizeof(hdr), type, size);
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, hdr, hdrlen);
} else
oid = NULL;
@@ -1291,7 +1291,7 @@ static void parse_pack_objects(unsigned char *hash)
/* Check pack integrity */
flush();
- the_hash_algo->init_fn(&tmp_ctx);
+ git_hash_init(&tmp_ctx, the_hash_algo);
git_hash_clone(&tmp_ctx, &input_ctx);
git_hash_final(hash, &tmp_ctx);
if (!hasheq(fill(the_hash_algo->rawsz), hash, the_repository->hash_algo))
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 57d9bd4a65..22f36ecf80 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -73,7 +73,7 @@ static size_t get_one_patchid(struct object_id *next_oid, struct object_id *resu
char pre_oid_str[GIT_MAX_HEXSZ + 1], post_oid_str[GIT_MAX_HEXSZ + 1];
struct git_hash_ctx ctx;
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
oidclr(result, the_repository->hash_algo);
while (strbuf_getwholeline(line_buf, stdin, '\n') != EOF) {
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 19eb6a1b61..faf0f120ac 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -615,7 +615,7 @@ static void hmac_hash(unsigned char *out,
/* RFC 2104 2. (1) */
memset(key, '\0', GIT_MAX_BLKSZ);
if (the_hash_algo->blksz < key_len) {
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
git_hash_update(&ctx, key_in, key_len);
git_hash_final(key, &ctx);
} else {
@@ -629,13 +629,13 @@ static void hmac_hash(unsigned char *out,
}
/* RFC 2104 2. (3) & (4) */
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
git_hash_update(&ctx, k_ipad, sizeof(k_ipad));
git_hash_update(&ctx, text, text_len);
git_hash_final(out, &ctx);
/* RFC 2104 2. (6) & (7) */
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
git_hash_update(&ctx, k_opad, sizeof(k_opad));
git_hash_update(&ctx, out, the_hash_algo->rawsz);
git_hash_final(out, &ctx);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1cc82a134d..bf114a7856 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -550,7 +550,7 @@ static void create_default_gitdir_config(const char *submodule_name)
/* Case 2.4: If all the above failed, try a hash of the name as a last resort */
header_len = snprintf(header, sizeof(header), "blob %zu", strlen(submodule_name));
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
the_hash_algo->update_fn(&ctx, header, header_len);
the_hash_algo->update_fn(&ctx, "\0", 1);
the_hash_algo->update_fn(&ctx, submodule_name, strlen(submodule_name));
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index f3849bb654..93a9caa582 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -670,10 +670,10 @@ int cmd_unpack_objects(int argc,
/* We don't take any non-flag arguments now.. Maybe some day */
usage(unpack_usage);
}
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
unpack_all();
git_hash_update(&ctx, buffer, offset);
- the_hash_algo->init_fn(&tmp_ctx);
+ git_hash_init(&tmp_ctx, the_hash_algo);
git_hash_clone(&tmp_ctx, &ctx);
git_hash_final_oid(&oid, &tmp_ctx);
if (strict) {
diff --git a/csum-file.c b/csum-file.c
index b166f89624..7e81391524 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -175,7 +175,7 @@ struct hashfile *hashfd_ext(const struct git_hash_algo *algop,
f->skip_hash = 0;
f->algop = unsafe_hash_algo(algop);
- f->algop->init_fn(&f->ctx);
+ git_hash_init(&f->ctx, f->algop);
f->buffer_len = opts->buffer_len ? opts->buffer_len : DEFAULT_IO_BUFFER_SIZE;
f->buffer = xmalloc(f->buffer_len);
@@ -200,7 +200,7 @@ void hashfile_checkpoint_init(struct hashfile *f,
struct hashfile_checkpoint *checkpoint)
{
memset(checkpoint, 0, sizeof(*checkpoint));
- f->algop->init_fn(&checkpoint->ctx);
+ git_hash_init(&checkpoint->ctx, f->algop);
}
void hashfile_checkpoint(struct hashfile *f, struct hashfile_checkpoint *checkpoint)
@@ -252,7 +252,7 @@ int hashfile_checksum_valid(const struct git_hash_algo *algop,
if (total_len < algop->rawsz)
return 0; /* say "too short"? */
- algop->init_fn(&ctx);
+ git_hash_init(&ctx, algop);
git_hash_update(&ctx, data, data_len);
git_hash_final(got, &ctx);
diff --git a/diff.c b/diff.c
index 1568f0ed9c..589c1969e4 100644
--- a/diff.c
+++ b/diff.c
@@ -6855,7 +6855,7 @@ void flush_one_hunk(struct object_id *result, struct git_hash_ctx *ctx)
int i;
git_hash_final(hash, ctx);
- the_hash_algo->init_fn(ctx);
+ git_hash_init(ctx, the_hash_algo);
/* 20-byte sum, with carry */
for (i = 0; i < the_hash_algo->rawsz; ++i) {
carry += result->hash[i] + hash[i];
@@ -6899,7 +6899,7 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid
struct git_hash_ctx ctx;
struct patch_id_t data;
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
memset(&data, 0, sizeof(struct patch_id_t));
data.ctx = &ctx;
oidclr(oid, the_repository->hash_algo);
diff --git a/http-push.c b/http-push.c
index 3c23cbba27..60f6f8f054 100644
--- a/http-push.c
+++ b/http-push.c
@@ -776,7 +776,7 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)
} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {
lock->token = xstrdup(ctx->cdata);
- the_hash_algo->init_fn(&hash_ctx);
+ git_hash_init(&hash_ctx, the_hash_algo);
git_hash_update(&hash_ctx, lock->token, strlen(lock->token));
git_hash_final(lock_token_hash, &hash_ctx);
diff --git a/http.c b/http.c
index 63abbaae8a..0341de5031 100644
--- a/http.c
+++ b/http.c
@@ -2879,7 +2879,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
git_inflate_init(&freq->stream);
- the_hash_algo->init_fn(&freq->c);
+ git_hash_init(&freq->c, the_hash_algo);
freq->hash_ctx_valid = 1;
freq->url = get_remote_object_url(base_url, hex, 0);
@@ -2916,7 +2916,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
git_inflate_end(&freq->stream);
memset(&freq->stream, 0, sizeof(freq->stream));
git_inflate_init(&freq->stream);
- the_hash_algo->init_fn(&freq->c);
+ git_hash_init(&freq->c, the_hash_algo);
if (prev_posn>0) {
prev_posn = 0;
lseek(freq->localfile, 0, SEEK_SET);
diff --git a/object-file.c b/object-file.c
index e3c68cfb66..f292683c2d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -124,7 +124,7 @@ int stream_object_signature(struct repository *r,
hdrlen = format_object_header(hdr, sizeof(hdr), st->type, st->size);
/* Sha1.. */
- r->hash_algo->init_fn(&c);
+ git_hash_init(&c, r->hash_algo);
git_hash_update(&c, hdr, hdrlen);
for (;;) {
char buf[1024 * 16];
@@ -320,7 +320,7 @@ static void hash_object_body(const struct git_hash_algo *algo, struct git_hash_c
struct object_id *oid,
char *hdr, size_t *hdrlen)
{
- algo->init_fn(c);
+ git_hash_init(c, algo);
git_hash_update(c, hdr, *hdrlen);
git_hash_update(c, buf, len);
git_hash_final_oid(oid, c);
@@ -681,9 +681,10 @@ static int start_loose_object_common(struct odb_source_loose *loose,
git_deflate_init(stream, cfg->zlib_compression_level);
stream->next_out = buf;
stream->avail_out = buflen;
- algo->init_fn(c);
- if (compat && compat_c)
- compat->init_fn(compat_c);
+ git_hash_init(c, algo);
+ if (compat && compat_c) {
+ git_hash_init(compat_c, compat);
+ }
/* Start to feed header to zlib stream */
stream->next_in = (unsigned char *)hdr;
@@ -1141,7 +1142,7 @@ static int hash_blob_stream(struct odb_write_stream *stream,
header_len = format_object_header((char *)buf, sizeof(buf),
OBJ_BLOB, size);
- hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, hash_algo);
git_hash_update(&ctx, buf, header_len);
while (!stream->is_finished) {
@@ -1313,7 +1314,7 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
header_len = format_object_header((char *)obuf, sizeof(obuf),
OBJ_BLOB, size);
- transaction->base.source->odb->repo->hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, transaction->base.source->odb->repo->hash_algo);
git_hash_update(&ctx, obuf, header_len);
/*
@@ -1560,7 +1561,7 @@ static int check_stream_oid(git_zstream *stream,
unsigned long total_read;
int status = Z_OK;
- algop->init_fn(&c);
+ git_hash_init(&c, algop);
git_hash_update(&c, hdr, stream->total_out);
/*
diff --git a/pack-check.c b/pack-check.c
index 5adfb3f272..c3b8db7c5c 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -69,7 +69,7 @@ static int verify_packfile(struct repository *r,
if (!is_pack_valid(p))
return error("packfile %s cannot be accessed", p->pack_name);
- r->hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, r->hash_algo);
do {
unsigned long remaining;
unsigned char *in = use_pack(p, w_curs, offset, &remaining);
diff --git a/pack-write.c b/pack-write.c
index 83eaf88541..24033a9101 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -402,8 +402,8 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
char *buf;
ssize_t read_result;
- hash_algo->init_fn(&old_hash_ctx);
- hash_algo->init_fn(&new_hash_ctx);
+ git_hash_init(&old_hash_ctx, hash_algo);
+ git_hash_init(&new_hash_ctx, hash_algo);
if (lseek(pack_fd, 0, SEEK_SET) != 0)
die_errno("Failed seeking to start of '%s'", pack_name);
@@ -455,7 +455,7 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
* pack, which also means making partial_pack_offset
* big enough not to matter anymore.
*/
- hash_algo->init_fn(&old_hash_ctx);
+ git_hash_init(&old_hash_ctx, hash_algo);
partial_pack_offset = ~partial_pack_offset;
partial_pack_offset -= MSB(partial_pack_offset, 1);
}
diff --git a/read-cache.c b/read-cache.c
index 7c1cdcf696..5fa747e6fc 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1722,7 +1722,7 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)
if (oideq(&oid, null_oid(the_hash_algo)))
return 0;
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, hdr, size - the_hash_algo->rawsz);
git_hash_final(hash, &c);
if (!hasheq(hash, start, the_repository->hash_algo))
@@ -2957,7 +2957,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
*/
if (offset && record_eoie()) {
CALLOC_ARRAY(eoie_c, 1);
- the_hash_algo->init_fn(eoie_c);
+ git_hash_init(eoie_c, the_hash_algo);
}
/*
@@ -3598,7 +3598,7 @@ static size_t read_eoie_extension(const char *mmap, size_t mmap_size)
* "REUC" + <binary representation of M>)
*/
src_offset = offset;
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
while (src_offset < mmap_size - the_hash_algo->rawsz - EOIE_SIZE_WITH_HEADER) {
/* After an array of active_nr index entries,
* there can be arbitrary number of extended
diff --git a/rerere.c b/rerere.c
index 8232542585..2e932439a4 100644
--- a/rerere.c
+++ b/rerere.c
@@ -438,8 +438,9 @@ static int handle_path(unsigned char *hash, struct rerere_io *io, int marker_siz
struct git_hash_ctx ctx;
struct strbuf buf = STRBUF_INIT, out = STRBUF_INIT;
int has_conflicts = 0;
- if (hash)
- the_hash_algo->init_fn(&ctx);
+ if (hash) {
+ git_hash_init(&ctx, the_hash_algo);
+ }
while (!io->getline(&buf, io)) {
if (is_cmarker(buf.buf, '<', marker_size)) {
diff --git a/t/helper/test-hash-speed.c b/t/helper/test-hash-speed.c
index fbf67fe6bd..89b0268011 100644
--- a/t/helper/test-hash-speed.c
+++ b/t/helper/test-hash-speed.c
@@ -5,7 +5,7 @@
static inline void compute_hash(const struct git_hash_algo *algo, struct git_hash_ctx *ctx, uint8_t *final, const void *p, size_t len)
{
- algo->init_fn(ctx);
+ git_hash_init(ctx, algo);
git_hash_update(ctx, p, len);
git_hash_final(final, ctx);
}
diff --git a/t/helper/test-hash.c b/t/helper/test-hash.c
index f0ee61c8b4..1f7163695f 100644
--- a/t/helper/test-hash.c
+++ b/t/helper/test-hash.c
@@ -29,7 +29,7 @@ int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)
die("OOPS");
}
- algop->init_fn(&ctx);
+ git_hash_init(&ctx, algop);
while (1) {
ssize_t sz, this_sz;
diff --git a/t/helper/test-synthesize.c b/t/helper/test-synthesize.c
index 3fa534fbdf..7719fb3a76 100644
--- a/t/helper/test-synthesize.c
+++ b/t/helper/test-synthesize.c
@@ -97,7 +97,7 @@ static void write_pack_object(FILE *f, struct git_hash_ctx *pack_ctx,
/* Write the data as uncompressed zlib */
write_uncompressed_zlib(f, pack_ctx, data, len, algo);
- algo->init_fn(&ctx);
+ git_hash_init(&ctx, algo);
object_header_len = format_object_header(object_header,
sizeof(object_header),
type, len);
@@ -430,7 +430,7 @@ static int generate_pack_with_large_object(const char *path, size_t blob_size,
f = xfopen(path, "wb");
- algo->init_fn(&pack_ctx);
+ git_hash_init(&pack_ctx, algo);
/* Write pack header */
fwrite_or_die(f, &pack_header, sizeof(pack_header));
diff --git a/t/unit-tests/u-hash.c b/t/unit-tests/u-hash.c
index bd4ac6a6e1..19f4efd410 100644
--- a/t/unit-tests/u-hash.c
+++ b/t/unit-tests/u-hash.c
@@ -12,7 +12,7 @@ static void check_hash_data(const void *data, size_t data_length,
unsigned char hash[GIT_MAX_HEXSZ];
const struct git_hash_algo *algop = &hash_algos[i];
- algop->init_fn(&ctx);
+ git_hash_init(&ctx, algop);
git_hash_update(&ctx, data, data_length);
git_hash_final(hash, &ctx);
diff --git a/tools/coccinelle/hash.cocci b/tools/coccinelle/hash.cocci
new file mode 100644
index 0000000000..5a7af6c544
--- /dev/null
+++ b/tools/coccinelle/hash.cocci
@@ -0,0 +1,10 @@
+@@
+identifier f != git_hash_init;
+expression ALGO;
+struct git_hash_ctx *CTX;
+@@
+ f(...) {<...
+- ALGO->init_fn(CTX);
++ git_hash_init(CTX, ALGO);
+ ...>}
+
diff --git a/trace2/tr2_sid.c b/trace2/tr2_sid.c
index 1c1d27b0ee..131b4f5a62 100644
--- a/trace2/tr2_sid.c
+++ b/trace2/tr2_sid.c
@@ -45,7 +45,7 @@ static void tr2_sid_append_my_sid_component(void)
if (xgethostname(hostname, sizeof(hostname)))
strbuf_add(&tr2sid_buf, "Localhost", 9);
else {
- algo->init_fn(&ctx);
+ git_hash_init(&ctx, algo);
git_hash_update(&ctx, hostname, strlen(hostname));
git_hash_final(hash, &ctx);
hash_to_hex_algop_r(hex, hash, algo);
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 2/7] hash: convert remaining direct function calls
2026-07-07 4:55 [PATCH 0/7] git_hash_*() quality-of-life improvements Jeff King
2026-07-07 5:01 ` [PATCH 1/7] hash: use git_hash_init() consistently Jeff King
@ 2026-07-07 5:04 ` Jeff King
2026-07-07 16:15 ` Junio C Hamano
2026-07-07 5:05 ` [PATCH 3/7] hash: document function pointers and wrappers Jeff King
` (6 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Jeff King @ 2026-07-07 5:04 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
The previous patch added a coccinelle rule to make sure callers always
use git_hash_init() rather than direct function pointers from the algo
struct.
Let's do the same for the rest of the git_hash_*() wrappers. I split
these out because they're a bit different: they implicitly use the algop
pointer in the git_hash_ctx. So when we convert:
-algo->update_fn(&ctx, buf, len);
+git_hash_update(&ctx, buf, len);
we drop the reference to algo entirely! But this is always going to be
the right thing. If "algo" does not match what is in ctx.algop, then
we'd already be invoking undefined behavior.
So in addition to making it possible to add more logic to the
git_hash_*() functions, we're avoiding the need to pass around the extra
algo pointer and make sure that it matches what's in "ctx".
The rest of the patch is the mechanical application of that coccinelle
patch, plus a minor cleanup in test-synthesize.c to drop a now-unused
function parameter (since we don't have to pass around the algo
separately anymore).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/submodule--helper.c | 8 +++---
t/helper/test-synthesize.c | 29 ++++++++++----------
tools/coccinelle/hash.cocci | 53 +++++++++++++++++++++++++++++++++++++
3 files changed, 71 insertions(+), 19 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index bf114a7856..510f193a15 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -551,10 +551,10 @@ static void create_default_gitdir_config(const char *submodule_name)
/* Case 2.4: If all the above failed, try a hash of the name as a last resort */
header_len = snprintf(header, sizeof(header), "blob %zu", strlen(submodule_name));
git_hash_init(&ctx, the_hash_algo);
- the_hash_algo->update_fn(&ctx, header, header_len);
- the_hash_algo->update_fn(&ctx, "\0", 1);
- the_hash_algo->update_fn(&ctx, submodule_name, strlen(submodule_name));
- the_hash_algo->final_fn(raw_name_hash, &ctx);
+ git_hash_update(&ctx, header, header_len);
+ git_hash_update(&ctx, "\0", 1);
+ git_hash_update(&ctx, submodule_name, strlen(submodule_name));
+ git_hash_final(raw_name_hash, &ctx);
hash_to_hex_algop_r(hex_name_hash, raw_name_hash, the_hash_algo);
strbuf_reset(&gitdir_path);
repo_git_path_append(the_repository, &gitdir_path, "modules/%s", hex_name_hash);
diff --git a/t/helper/test-synthesize.c b/t/helper/test-synthesize.c
index 7719fb3a76..fd116c87ba 100644
--- a/t/helper/test-synthesize.c
+++ b/t/helper/test-synthesize.c
@@ -25,8 +25,7 @@ static const unsigned char zeros[BLOCK_SIZE];
* Updates the pack checksum context.
*/
static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
- const void *data, size_t len,
- const struct git_hash_algo *algo)
+ const void *data, size_t len)
{
unsigned char zlib_header[2] = { 0x78, 0x01 }; /* CMF, FLG */
unsigned char block_header[5];
@@ -37,7 +36,7 @@ static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
/* Write zlib header */
fwrite_or_die(f, zlib_header, sizeof(zlib_header));
- algo->update_fn(pack_ctx, zlib_header, 2);
+ git_hash_update(pack_ctx, zlib_header, 2);
/* Write uncompressed blocks (max 64KB each) */
do {
@@ -52,11 +51,11 @@ static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
block_header[4] = block_header[2] ^ 0xff;
fwrite_or_die(f, block_header, sizeof(block_header));
- algo->update_fn(pack_ctx, block_header, 5);
+ git_hash_update(pack_ctx, block_header, 5);
if (block_len) {
fwrite_or_die(f, block_data, block_len);
- algo->update_fn(pack_ctx, block_data, block_len);
+ git_hash_update(pack_ctx, block_data, block_len);
adler = adler32(adler, block_data, block_len);
}
@@ -68,7 +67,7 @@ static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
/* Write adler32 checksum */
put_be32(adler_buf, adler);
fwrite_or_die(f, adler_buf, sizeof(adler_buf));
- algo->update_fn(pack_ctx, adler_buf, 4);
+ git_hash_update(pack_ctx, adler_buf, 4);
}
/*
@@ -92,24 +91,24 @@ static void write_pack_object(FILE *f, struct git_hash_ctx *pack_ctx,
sizeof(pack_header),
type, len);
fwrite_or_die(f, pack_header, pack_header_len);
- algo->update_fn(pack_ctx, pack_header, pack_header_len);
+ git_hash_update(pack_ctx, pack_header, pack_header_len);
/* Write the data as uncompressed zlib */
- write_uncompressed_zlib(f, pack_ctx, data, len, algo);
+ write_uncompressed_zlib(f, pack_ctx, data, len);
git_hash_init(&ctx, algo);
object_header_len = format_object_header(object_header,
sizeof(object_header),
type, len);
- algo->update_fn(&ctx, object_header, object_header_len);
+ git_hash_update(&ctx, object_header, object_header_len);
if (data)
- algo->update_fn(&ctx, data, len);
+ git_hash_update(&ctx, data, len);
else {
for (size_t i = len / BLOCK_SIZE; i; i--)
- algo->update_fn(&ctx, zeros, BLOCK_SIZE);
- algo->update_fn(&ctx, zeros, len % BLOCK_SIZE);
+ git_hash_update(&ctx, zeros, BLOCK_SIZE);
+ git_hash_update(&ctx, zeros, len % BLOCK_SIZE);
}
- algo->final_oid_fn(oid, &ctx);
+ git_hash_final_oid(oid, &ctx);
}
/*
@@ -434,7 +433,7 @@ static int generate_pack_with_large_object(const char *path, size_t blob_size,
/* Write pack header */
fwrite_or_die(f, &pack_header, sizeof(pack_header));
- algo->update_fn(&pack_ctx, &pack_header, sizeof(pack_header));
+ git_hash_update(&pack_ctx, &pack_header, sizeof(pack_header));
/* 1. Write the large blob */
write_pack_object(f, &pack_ctx, OBJ_BLOB, NULL, blob_size, &blob_oid, algo);
@@ -472,7 +471,7 @@ static int generate_pack_with_large_object(const char *path, size_t blob_size,
write_pack_object(f, &pack_ctx, OBJ_COMMIT, buf.buf, buf.len, &final_commit_oid, algo);
/* Write pack trailer (checksum) */
- algo->final_fn(pack_hash, &pack_ctx);
+ git_hash_final(pack_hash, &pack_ctx);
fwrite_or_die(f, pack_hash, algo->rawsz);
if (fclose(f))
die_errno(_("could not close '%s'"), path);
diff --git a/tools/coccinelle/hash.cocci b/tools/coccinelle/hash.cocci
index 5a7af6c544..d0e2e5f4b1 100644
--- a/tools/coccinelle/hash.cocci
+++ b/tools/coccinelle/hash.cocci
@@ -8,3 +8,56 @@ struct git_hash_ctx *CTX;
+ git_hash_init(CTX, ALGO);
...>}
+@@
+identifier f != git_hash_clone;
+expression ALGO;
+struct git_hash_ctx *SRC;
+struct git_hash_ctx *DST;
+@@
+ f(...) {<...
+- ALGO->clone_fn(DST, SRC);
++ git_hash_clone(DST, SRC);
+ ...>}
+
+@@
+identifier f != git_hash_update;
+expression ALGO;
+struct git_hash_ctx *CTX;
+expression list ARGS;
+@@
+ f(...) {<...
+- ALGO->update_fn(CTX, ARGS);
++ git_hash_update(CTX, ARGS);
+ ...>}
+
+@@
+identifier f != git_hash_final;
+expression ALGO;
+struct git_hash_ctx *CTX;
+expression list ARGS;
+@@
+ f(...) {<...
+- ALGO->final_fn(ARGS, CTX);
++ git_hash_final(ARGS, CTX);
+ ...>}
+
+@@
+identifier f != git_hash_final_oid;
+expression ALGO;
+struct git_hash_ctx *CTX;
+expression list ARGS;
+@@
+ f(...) {<...
+- ALGO->final_oid_fn(ARGS, CTX);
++ git_hash_final_oid(ARGS, CTX);
+ ...>}
+
+@@
+identifier f != git_hash_discard;
+expression ALGO;
+struct git_hash_ctx *CTX;
+@@
+ f(...) {<...
+- ALGO->discard_fn(CTX);
++ git_hash_discard(CTX);
+ ...>}
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 3/7] hash: document function pointers and wrappers
2026-07-07 4:55 [PATCH 0/7] git_hash_*() quality-of-life improvements Jeff King
2026-07-07 5:01 ` [PATCH 1/7] hash: use git_hash_init() consistently Jeff King
2026-07-07 5:04 ` [PATCH 2/7] hash: convert remaining direct function calls Jeff King
@ 2026-07-07 5:05 ` Jeff King
2026-07-07 14:26 ` Patrick Steinhardt
2026-07-07 5:07 ` [PATCH 4/7] hash: make git_hash_discard() idempotent Jeff King
` (5 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Jeff King @ 2026-07-07 5:05 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
We want people to use the git_hash_*() wrappers rather than the bare
function pointers in the git_hash_algo struct. Let's document them
rather than the bare pointers, and warn people away from the pointers.
Coccinelle will eventually force the use of the wrappers, but it's
helpful to lead readers in the right direction from the start.
While we're here we can document a few other bits of wisdom I've turned
up while working in this area:
- You have to initialize the destination of a git_hash_clone(). This
is something we may eventually change for efficiency, but we should
definitely document the requirement for now.
- You must eventually finalize or discard a hash, since some backends
may allocate resources during initialization.
Signed-off-by: Jeff King <peff@peff.net>
---
hash.h | 43 ++++++++++++++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 11 deletions(-)
diff --git a/hash.h b/hash.h
index 0a23ef4dfd..5686914b71 100644
--- a/hash.h
+++ b/hash.h
@@ -309,22 +309,15 @@ struct git_hash_algo {
/* The block size of the hash. */
size_t blksz;
- /* The hash initialization function. */
+ /*
+ * Low-level implementation hooks. Callers should use the git_hash_*
+ * wrappers below rather than invoking these directly.
+ */
git_hash_init_fn init_fn;
-
- /* The hash context cloning function. */
git_hash_clone_fn clone_fn;
-
- /* The hash update function. */
git_hash_update_fn update_fn;
-
- /* The hash finalization function. */
git_hash_final_fn final_fn;
-
- /* The hash finalization function for object IDs. */
git_hash_final_oid_fn final_oid_fn;
-
- /* Discard an initialized hash without finalizing. */
git_hash_discard_fn discard_fn;
/* The OID of the empty tree. */
@@ -341,12 +334,40 @@ struct git_hash_algo {
};
extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
+/*
+ * Prepare an uninitialized hash context for use. You must eventually release
+ * the context with with git_hash_final() (or final_oid()) or by calling
+ * git_hash_discard().
+ */
void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop);
+
+/*
+ * Clone the state of a hash. Both src and dst must have been initialized with
+ * git_hash_init().
+ */
void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src);
+
+/*
+ * Add more data to an initialized hash context.
+ */
void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len);
+
+/*
+ * Retrieve the final hash value from a context, releasing any resources.
+ */
void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx);
+
+/*
+ * Like git_hash_final(), but write the result into an object_id.
+ */
void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx);
+
+/*
+ * Discard a hash context without computing the final value, but still
+ * releasing any resources.
+ */
void git_hash_discard(struct git_hash_ctx *ctx);
+
const struct git_hash_algo *hash_algo_ptr_by_number(uint32_t algo);
struct git_hash_ctx *git_hash_alloc(void);
void git_hash_free(struct git_hash_ctx *ctx);
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 4/7] hash: make git_hash_discard() idempotent
2026-07-07 4:55 [PATCH 0/7] git_hash_*() quality-of-life improvements Jeff King
` (2 preceding siblings ...)
2026-07-07 5:05 ` [PATCH 3/7] hash: document function pointers and wrappers Jeff King
@ 2026-07-07 5:07 ` Jeff King
2026-07-07 16:22 ` Junio C Hamano
2026-07-07 5:07 ` [PATCH 5/7] csum-file: use idempotent git_hash_discard() Jeff King
` (4 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Jeff King @ 2026-07-07 5:07 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
You must always either finalize or discard a hash context to release any
resources, but you must call only one such function. This creates extra
work for some callers, since their cleanup code paths need to know
whether they got there via their happy path (and the finalization
happened) or due to an error (in which case they need to discard).
Let's add an "active" flag that turns a redundant discard into a noop.
That lets you safely do this:
git_hash_init(&ctx, algo);
...
if (some_error)
goto out;
...
git_hash_final(result, &ctx);
out:
git_hash_discard(&ctx);
This should avoid future errors, and will also let us simplify a few
existing callers (in future patches).
Signed-off-by: Jeff King <peff@peff.net>
---
hash.c | 6 ++++++
hash.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/hash.c b/hash.c
index 55d1d41770..b1296f0018 100644
--- a/hash.c
+++ b/hash.c
@@ -285,6 +285,7 @@ void git_hash_free(struct git_hash_ctx *ctx)
void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop)
{
algop->init_fn(ctx);
+ ctx->active = true;
}
void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src)
@@ -300,16 +301,21 @@ void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len)
void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx)
{
ctx->algop->final_fn(hash, ctx);
+ ctx->active = false;
}
void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
{
ctx->algop->final_oid_fn(oid, ctx);
+ ctx->active = false;
}
void git_hash_discard(struct git_hash_ctx *ctx)
{
+ if (!ctx->active)
+ return;
ctx->algop->discard_fn(ctx);
+ ctx->active = false;
}
uint32_t hash_algo_by_name(const char *name)
diff --git a/hash.h b/hash.h
index 5686914b71..f97f7b9ff4 100644
--- a/hash.h
+++ b/hash.h
@@ -281,6 +281,7 @@ struct git_hash_ctx {
git_SHA_CTX_unsafe sha1_unsafe;
git_SHA256_CTX sha256;
} state;
+ bool active;
};
typedef void (*git_hash_init_fn)(struct git_hash_ctx *ctx);
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 5/7] csum-file: use idempotent git_hash_discard()
2026-07-07 4:55 [PATCH 0/7] git_hash_*() quality-of-life improvements Jeff King
` (3 preceding siblings ...)
2026-07-07 5:07 ` [PATCH 4/7] hash: make git_hash_discard() idempotent Jeff King
@ 2026-07-07 5:07 ` Jeff King
2026-07-07 5:08 ` [PATCH 6/7] http: " Jeff King
` (3 subsequent siblings)
8 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-07 5:07 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
Now that it is safe to call git_hash_discard() even after finalizing it,
we can simplify our cleanup logic a bit. This is mostly undoing a few
bits of 64337aecde (csum-file: always finalize or discard hash,
2026-07-02):
- We no longer need a separate free_hashfile_memory() function for
finalize_hashfile(). It can just call free_hashfile(), which will
now discard (or not) the hash as appropriate.
- When f->skip_hash is set, we don't need to discard; we can rely on
free_hashfile() to do it.
Signed-off-by: Jeff King <peff@peff.net>
---
csum-file.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/csum-file.c b/csum-file.c
index 7e81391524..fe18ee1de3 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -55,32 +55,25 @@ void hashflush(struct hashfile *f)
}
}
-static void free_hashfile_memory(struct hashfile *f)
+void free_hashfile(struct hashfile *f)
{
+ git_hash_discard(&f->ctx);
free(f->buffer);
free(f->check_buffer);
free(f);
}
-void free_hashfile(struct hashfile *f)
-{
- git_hash_discard(&f->ctx);
- free_hashfile_memory(f);
-}
-
int finalize_hashfile(struct hashfile *f, unsigned char *result,
enum fsync_component component, unsigned int flags)
{
int fd;
hashflush(f);
- if (f->skip_hash) {
- git_hash_discard(&f->ctx);
+ if (f->skip_hash)
hashclr(f->buffer, f->algop);
- } else {
+ else
git_hash_final(f->buffer, &f->ctx);
- }
if (result)
hashcpy(result, f->buffer, f->algop);
@@ -105,7 +98,7 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,
if (close(f->check_fd))
die_errno("%s: sha1 file error on close", f->name);
}
- free_hashfile_memory(f);
+ free_hashfile(f);
return fd;
}
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 6/7] http: use idempotent git_hash_discard()
2026-07-07 4:55 [PATCH 0/7] git_hash_*() quality-of-life improvements Jeff King
` (4 preceding siblings ...)
2026-07-07 5:07 ` [PATCH 5/7] csum-file: use idempotent git_hash_discard() Jeff King
@ 2026-07-07 5:08 ` Jeff King
2026-07-07 16:25 ` Junio C Hamano
2026-07-07 5:09 ` [PATCH 7/7] hash: check ctx->active flag in all wrapper functions Jeff King
` (2 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Jeff King @ 2026-07-07 5:08 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
Now that it is OK to call git_hash_discard() even after finalizing the
hash, we no longer need the ctx_valid bool added by a2d8ea5a76 (http:
discard hash in dumb-http http_object_request, 2026-07-02).
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 5 +----
http.h | 1 -
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/http.c b/http.c
index 0341de5031..caccf2108e 100644
--- a/http.c
+++ b/http.c
@@ -2880,7 +2880,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
git_inflate_init(&freq->stream);
git_hash_init(&freq->c, the_hash_algo);
- freq->hash_ctx_valid = 1;
freq->url = get_remote_object_url(base_url, hex, 0);
@@ -2989,7 +2988,6 @@ int finish_http_object_request(struct http_object_request *freq)
}
git_hash_final_oid(&freq->real_oid, &freq->c);
- freq->hash_ctx_valid = 0;
if (freq->zret != Z_STREAM_END) {
unlink_or_warn(freq->tmpfile.buf);
return -1;
@@ -3030,8 +3028,7 @@ void release_http_object_request(struct http_object_request **freq_p)
curl_slist_free_all(freq->headers);
strbuf_release(&freq->tmpfile);
git_inflate_end(&freq->stream);
- if (freq->hash_ctx_valid)
- git_hash_discard(&freq->c);
+ git_hash_discard(&freq->c);
free(freq);
*freq_p = NULL;
diff --git a/http.h b/http.h
index 6b0639150f..729c51904d 100644
--- a/http.h
+++ b/http.h
@@ -255,7 +255,6 @@ struct http_object_request {
struct object_id oid;
struct object_id real_oid;
struct git_hash_ctx c;
- int hash_ctx_valid;
git_zstream stream;
int zret;
int rename;
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 7/7] hash: check ctx->active flag in all wrapper functions
2026-07-07 4:55 [PATCH 0/7] git_hash_*() quality-of-life improvements Jeff King
` (5 preceding siblings ...)
2026-07-07 5:08 ` [PATCH 6/7] http: " Jeff King
@ 2026-07-07 5:09 ` Jeff King
2026-07-07 14:26 ` Patrick Steinhardt
2026-07-07 16:33 ` Junio C Hamano
2026-07-07 14:26 ` [PATCH 0/7] git_hash_*() quality-of-life improvements Patrick Steinhardt
2026-07-08 3:52 ` [PATCH v2 " Jeff King
8 siblings, 2 replies; 35+ messages in thread
From: Jeff King @ 2026-07-07 5:09 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson
It only makes sense to call git_hash_update(), etc, on a hash context
that has been initialized but not yet finalized or discarded. This is an
unlikely error to make, but it's easy for us to catch it and complain.
It's especially important because it would quietly "work" for many hash
backends (like sha1dc, which is just manipulating some bytes) but would
cause undefined behavior with others (like OpenSSL, which puts the
context onto the heap). Checking the flag lets us catch problems
consistently on every build.
Note that we can't do the same for git_init_hash(). Even though it would
cause a leak to call it twice (without an intervening final/discard),
the point of the function is that the contents of the struct are
undefined before the call. But calling it twice is an even less likely
error to make, so not covering it is OK.
Signed-off-by: Jeff King <peff@peff.net>
---
hash.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/hash.c b/hash.c
index b1296f0018..82f7e24404 100644
--- a/hash.c
+++ b/hash.c
@@ -290,22 +290,32 @@ void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop)
void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src)
{
+ if (!src->active)
+ BUG("attempt to copy from an inactive hash context");
+ if (!dst->active)
+ BUG("attempt to copy to an inactive hash context");
src->algop->clone_fn(dst, src);
}
void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len)
{
+ if (!ctx->active)
+ BUG("attempt to update an inactive hash context");
ctx->algop->update_fn(ctx, in, len);
}
void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx)
{
+ if (!ctx->active)
+ BUG("attempt to finalize an inactive hash context");
ctx->algop->final_fn(hash, ctx);
ctx->active = false;
}
void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
{
+ if (!ctx->active)
+ BUG("attempt to finalize an inactive hash context");
ctx->algop->final_oid_fn(oid, ctx);
ctx->active = false;
}
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 3/7] hash: document function pointers and wrappers
2026-07-07 5:05 ` [PATCH 3/7] hash: document function pointers and wrappers Jeff King
@ 2026-07-07 14:26 ` Patrick Steinhardt
2026-07-07 20:05 ` Jeff King
0 siblings, 1 reply; 35+ messages in thread
From: Patrick Steinhardt @ 2026-07-07 14:26 UTC (permalink / raw)
To: Jeff King; +Cc: git, brian m. carlson
On Tue, Jul 07, 2026 at 01:05:57AM -0400, Jeff King wrote:
> diff --git a/hash.h b/hash.h
> index 0a23ef4dfd..5686914b71 100644
> --- a/hash.h
> +++ b/hash.h
> @@ -341,12 +334,40 @@ struct git_hash_algo {
> };
> extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
>
> +/*
> + * Prepare an uninitialized hash context for use. You must eventually release
> + * the context with with git_hash_final() (or final_oid()) or by calling
s/with with/with/
Patrick
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 7/7] hash: check ctx->active flag in all wrapper functions
2026-07-07 5:09 ` [PATCH 7/7] hash: check ctx->active flag in all wrapper functions Jeff King
@ 2026-07-07 14:26 ` Patrick Steinhardt
2026-07-07 16:33 ` Junio C Hamano
1 sibling, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2026-07-07 14:26 UTC (permalink / raw)
To: Jeff King; +Cc: git, brian m. carlson
On Tue, Jul 07, 2026 at 01:09:52AM -0400, Jeff King wrote:
> It only makes sense to call git_hash_update(), etc, on a hash context
> that has been initialized but not yet finalized or discarded. This is an
> unlikely error to make, but it's easy for us to catch it and complain.
>
> It's especially important because it would quietly "work" for many hash
> backends (like sha1dc, which is just manipulating some bytes) but would
> cause undefined behavior with others (like OpenSSL, which puts the
> context onto the heap). Checking the flag lets us catch problems
> consistently on every build.
>
> Note that we can't do the same for git_init_hash(). Even though it would
You probably mean `git_hash_init()`?
> cause a leak to call it twice (without an intervening final/discard),
> the point of the function is that the contents of the struct are
> undefined before the call. But calling it twice is an even less likely
> error to make, so not covering it is OK.
Right. We could of course enforce that the structure must be zeroed
before calling this function. But I agree that this would become quite
awkward.
Patrick
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/7] git_hash_*() quality-of-life improvements
2026-07-07 4:55 [PATCH 0/7] git_hash_*() quality-of-life improvements Jeff King
` (6 preceding siblings ...)
2026-07-07 5:09 ` [PATCH 7/7] hash: check ctx->active flag in all wrapper functions Jeff King
@ 2026-07-07 14:26 ` Patrick Steinhardt
2026-07-08 3:52 ` [PATCH v2 " Jeff King
8 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2026-07-07 14:26 UTC (permalink / raw)
To: Jeff King; +Cc: git, brian m. carlson
On Tue, Jul 07, 2026 at 12:55:56AM -0400, Jeff King wrote:
> This implements the "idempotent git_hash_discard()" discussed in this
> subthread:
>
> https://lore.kernel.org/git/20260702080707.GG2029434@coredump.intra.peff.net/
>
> with associated cleanups.
>
> It should be applied on top of jk/hash-algo-leak-fixes.
Thanks, this was a pleasant read. I have two minor nits, but other than
that I'm happy with this series!
Patrick
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/7] hash: use git_hash_init() consistently
2026-07-07 5:01 ` [PATCH 1/7] hash: use git_hash_init() consistently Jeff King
@ 2026-07-07 14:39 ` Junio C Hamano
2026-07-07 20:13 ` Jeff King
2026-07-07 21:25 ` brian m. carlson
1 sibling, 1 reply; 35+ messages in thread
From: Junio C Hamano @ 2026-07-07 14:39 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt, brian m. carlson
Jeff King <peff@peff.net> writes:
> We'd like to add more logic to git_hash_init(), but many callers skip it
> and call algop->init_fn() directly. Let's make sure we're consistently
> using the wrapper by adding a coccinelle rule.
>
> Besides the coccinelle file itself, this is a purely mechanical
> conversion based on the patch it generates. There should be no bare
> init_fn() calls left (except for the one in the wrapper).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> It feels like the "expression ALGO" in the rule should be a
> "git_hash_algo", but I had trouble getting coccinelle to recognize all
> cases when I did that. Probably not worth digging too far into, as
> the presence of the git_hash_ctx type means we should never hit any
> false positives.
Thanks. May conversions do look simple and straight-forward, but
some look a bit curious.
> diff --git a/object-file.c b/object-file.c
> index e3c68cfb66..f292683c2d 100644
> --- a/object-file.c
> +++ b/object-file.c
> ...
> - algo->init_fn(c);
> - if (compat && compat_c)
> - compat->init_fn(compat_c);
> + git_hash_init(c, algo);
> + if (compat && compat_c) {
> + git_hash_init(compat_c, compat);
> + }
For example, it is a mystery how Coccinelle decided to add a pair of
braces around this single statement. It should be obvious that the
corresponding single statement in the original did not need one.
> diff --git a/rerere.c b/rerere.c
> index 8232542585..2e932439a4 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -438,8 +438,9 @@ static int handle_path(unsigned char *hash, struct rerere_io *io, int marker_siz
> struct git_hash_ctx ctx;
> struct strbuf buf = STRBUF_INIT, out = STRBUF_INIT;
> int has_conflicts = 0;
> - if (hash)
> - the_hash_algo->init_fn(&ctx);
> + if (hash) {
> + git_hash_init(&ctx, the_hash_algo);
> + }
Ditto.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/7] hash: convert remaining direct function calls
2026-07-07 5:04 ` [PATCH 2/7] hash: convert remaining direct function calls Jeff King
@ 2026-07-07 16:15 ` Junio C Hamano
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2026-07-07 16:15 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt, brian m. carlson
Jeff King <peff@peff.net> writes:
> The previous patch added a coccinelle rule to make sure callers always
> use git_hash_init() rather than direct function pointers from the algo
> struct.
>
> Let's do the same for the rest of the git_hash_*() wrappers. I split
> these out because they're a bit different: they implicitly use the algop
> pointer in the git_hash_ctx. So when we convert:
>
> -algo->update_fn(&ctx, buf, len);
> +git_hash_update(&ctx, buf, len);
>
> we drop the reference to algo entirely! But this is always going to be
> the right thing. If "algo" does not match what is in ctx.algop, then
> we'd already be invoking undefined behavior.
>
> So in addition to making it possible to add more logic to the
> git_hash_*() functions, we're avoiding the need to pass around the extra
> algo pointer and make sure that it matches what's in "ctx".
>
> The rest of the patch is the mechanical application of that coccinelle
> patch, plus a minor cleanup in test-synthesize.c to drop a now-unused
> function parameter (since we don't have to pass around the algo
> separately anymore).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/submodule--helper.c | 8 +++---
> t/helper/test-synthesize.c | 29 ++++++++++----------
> tools/coccinelle/hash.cocci | 53 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 71 insertions(+), 19 deletions(-)
Looks very straight-forward.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/7] hash: make git_hash_discard() idempotent
2026-07-07 5:07 ` [PATCH 4/7] hash: make git_hash_discard() idempotent Jeff King
@ 2026-07-07 16:22 ` Junio C Hamano
2026-07-07 20:18 ` Jeff King
0 siblings, 1 reply; 35+ messages in thread
From: Junio C Hamano @ 2026-07-07 16:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt, brian m. carlson
Jeff King <peff@peff.net> writes:
> You must always either finalize or discard a hash context to release any
> resources, but you must call only one such function. This creates extra
> work for some callers, since their cleanup code paths need to know
> whether they got there via their happy path (and the finalization
> happened) or due to an error (in which case they need to discard).
>
> Let's add an "active" flag that turns a redundant discard into a noop.
> That lets you safely do this:
>
> git_hash_init(&ctx, algo);
> ...
> if (some_error)
> goto out;
> ...
> git_hash_final(result, &ctx);
>
> out:
> git_hash_discard(&ctx);
>
> This should avoid future errors, and will also let us simplify a few
> existing callers (in future patches).
Hmph, so is the point of this change to allow _discard() to be
called even after _final() was already called that we do not need an
early return or something before the out: label?
Unlike commit_*() and rollback_*() used in lockfile API, where the
names clearly say which one is for happy and which one is for error
case, the _final() and _discard() pair does not exactly tell me
which is which, but I guess I will get used to it, perhaps.
But the change nevertheless looks mostly good except for one "hmph".
When _init() is called, active gets turned on automatically, and
either _discard() or _final() turns it off. Only _discard() is
protected from getting called multiple times. Is this because
it is already a no-op to call _final() multiple times?
Thanks.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> hash.c | 6 ++++++
> hash.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/hash.c b/hash.c
> index 55d1d41770..b1296f0018 100644
> --- a/hash.c
> +++ b/hash.c
> @@ -285,6 +285,7 @@ void git_hash_free(struct git_hash_ctx *ctx)
> void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop)
> {
> algop->init_fn(ctx);
> + ctx->active = true;
> }
>
> void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src)
> @@ -300,16 +301,21 @@ void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len)
> void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx)
> {
> ctx->algop->final_fn(hash, ctx);
> + ctx->active = false;
> }
>
> void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
> {
> ctx->algop->final_oid_fn(oid, ctx);
> + ctx->active = false;
> }
>
> void git_hash_discard(struct git_hash_ctx *ctx)
> {
> + if (!ctx->active)
> + return;
> ctx->algop->discard_fn(ctx);
> + ctx->active = false;
> }
>
> uint32_t hash_algo_by_name(const char *name)
> diff --git a/hash.h b/hash.h
> index 5686914b71..f97f7b9ff4 100644
> --- a/hash.h
> +++ b/hash.h
> @@ -281,6 +281,7 @@ struct git_hash_ctx {
> git_SHA_CTX_unsafe sha1_unsafe;
> git_SHA256_CTX sha256;
> } state;
> + bool active;
> };
>
> typedef void (*git_hash_init_fn)(struct git_hash_ctx *ctx);
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/7] http: use idempotent git_hash_discard()
2026-07-07 5:08 ` [PATCH 6/7] http: " Jeff King
@ 2026-07-07 16:25 ` Junio C Hamano
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2026-07-07 16:25 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt, brian m. carlson
Jeff King <peff@peff.net> writes:
> Now that it is OK to call git_hash_discard() even after finalizing the
> hash, we no longer need the ctx_valid bool added by a2d8ea5a76 (http:
> discard hash in dumb-http http_object_request, 2026-07-02).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> http.c | 5 +----
> http.h | 1 -
> 2 files changed, 1 insertion(+), 5 deletions(-)
OK, because calling _discard() on an already discarded or finished
hash context is a no-op, we do not have to remember if we finialized
or discarded anymore, allowing us to be extra lazy and safe. Nice.
> diff --git a/http.c b/http.c
> index 0341de5031..caccf2108e 100644
> --- a/http.c
> +++ b/http.c
> @@ -2880,7 +2880,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
> git_inflate_init(&freq->stream);
>
> git_hash_init(&freq->c, the_hash_algo);
> - freq->hash_ctx_valid = 1;
>
> freq->url = get_remote_object_url(base_url, hex, 0);
>
> @@ -2989,7 +2988,6 @@ int finish_http_object_request(struct http_object_request *freq)
> }
>
> git_hash_final_oid(&freq->real_oid, &freq->c);
> - freq->hash_ctx_valid = 0;
> if (freq->zret != Z_STREAM_END) {
> unlink_or_warn(freq->tmpfile.buf);
> return -1;
> @@ -3030,8 +3028,7 @@ void release_http_object_request(struct http_object_request **freq_p)
> curl_slist_free_all(freq->headers);
> strbuf_release(&freq->tmpfile);
> git_inflate_end(&freq->stream);
> - if (freq->hash_ctx_valid)
> - git_hash_discard(&freq->c);
> + git_hash_discard(&freq->c);
>
> free(freq);
> *freq_p = NULL;
> diff --git a/http.h b/http.h
> index 6b0639150f..729c51904d 100644
> --- a/http.h
> +++ b/http.h
> @@ -255,7 +255,6 @@ struct http_object_request {
> struct object_id oid;
> struct object_id real_oid;
> struct git_hash_ctx c;
> - int hash_ctx_valid;
> git_zstream stream;
> int zret;
> int rename;
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 7/7] hash: check ctx->active flag in all wrapper functions
2026-07-07 5:09 ` [PATCH 7/7] hash: check ctx->active flag in all wrapper functions Jeff King
2026-07-07 14:26 ` Patrick Steinhardt
@ 2026-07-07 16:33 ` Junio C Hamano
2026-07-07 20:10 ` Jeff King
1 sibling, 1 reply; 35+ messages in thread
From: Junio C Hamano @ 2026-07-07 16:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt, brian m. carlson
Jeff King <peff@peff.net> writes:
> It only makes sense to call git_hash_update(), etc, on a hash context
> that has been initialized but not yet finalized or discarded. This is an
> unlikely error to make, but it's easy for us to catch it and complain.
>
> It's especially important because it would quietly "work" for many hash
> backends (like sha1dc, which is just manipulating some bytes) but would
> cause undefined behavior with others (like OpenSSL, which puts the
> context onto the heap). Checking the flag lets us catch problems
> consistently on every build.
>
> Note that we can't do the same for git_init_hash(). Even though it would
> cause a leak to call it twice (without an intervening final/discard),
> the point of the function is that the contents of the struct are
> undefined before the call. But calling it twice is an even less likely
> error to make, so not covering it is OK.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> hash.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
Among the four we see here, I agree that calling _clone and _update
on an already discarded or finalized context should be caught as an
error. As I alluded to earlier, though, I am not sure about
_final. The asymmetry in a design that allows _discard after _final
but not _final after _final disturbs me slightly, but perhaps that
is only because my morning caffeine has not yet kicked in.
>
> diff --git a/hash.c b/hash.c
> index b1296f0018..82f7e24404 100644
> --- a/hash.c
> +++ b/hash.c
> @@ -290,22 +290,32 @@ void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop)
>
> void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src)
> {
> + if (!src->active)
> + BUG("attempt to copy from an inactive hash context");
> + if (!dst->active)
> + BUG("attempt to copy to an inactive hash context");
> src->algop->clone_fn(dst, src);
> }
>
> void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len)
> {
> + if (!ctx->active)
> + BUG("attempt to update an inactive hash context");
> ctx->algop->update_fn(ctx, in, len);
> }
>
> void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx)
> {
> + if (!ctx->active)
> + BUG("attempt to finalize an inactive hash context");
> ctx->algop->final_fn(hash, ctx);
> ctx->active = false;
> }
>
> void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
> {
> + if (!ctx->active)
> + BUG("attempt to finalize an inactive hash context");
> ctx->algop->final_oid_fn(oid, ctx);
> ctx->active = false;
> }
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/7] hash: document function pointers and wrappers
2026-07-07 14:26 ` Patrick Steinhardt
@ 2026-07-07 20:05 ` Jeff King
0 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-07 20:05 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, brian m. carlson
On Tue, Jul 07, 2026 at 04:26:36PM +0200, Patrick Steinhardt wrote:
> On Tue, Jul 07, 2026 at 01:05:57AM -0400, Jeff King wrote:
> > diff --git a/hash.h b/hash.h
> > index 0a23ef4dfd..5686914b71 100644
> > --- a/hash.h
> > +++ b/hash.h
> > @@ -341,12 +334,40 @@ struct git_hash_algo {
> > };
> > extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
> >
> > +/*
> > + * Prepare an uninitialized hash context for use. You must eventually release
> > + * the context with with git_hash_final() (or final_oid()) or by calling
>
> s/with with/with/
Thanks, looks like there are a few minor formatting nits, so I'll fix
this in a v2.
-Peff
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 7/7] hash: check ctx->active flag in all wrapper functions
2026-07-07 16:33 ` Junio C Hamano
@ 2026-07-07 20:10 ` Jeff King
0 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-07 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, brian m. carlson
On Tue, Jul 07, 2026 at 09:33:06AM -0700, Junio C Hamano wrote:
> Among the four we see here, I agree that calling _clone and _update
> on an already discarded or finalized context should be caught as an
> error. As I alluded to earlier, though, I am not sure about
> _final. The asymmetry in a design that allows _discard after _final
> but not _final after _final disturbs me slightly, but perhaps that
> is only because my morning caffeine has not yet kicked in.
There was more discussion in the earlier thread:
https://lore.kernel.org/git/20260706000105.GA2301945@coredump.intra.peff.net/
But basically the asymmetry comes from the fact that the finalize is
trying to _do_ something, whereas discard is just, well, discarding.
So what should:
git_hash_discard(&ctx);
git_hash_finalize(result, &ctx);
put into result? It is probably one of:
1. the null hash
2. the hash you get from init() + no updates + final()
3. nothing, BUG() instead
It seems nice at first that (1) or (2) won't cause the program to crash,
but ultimately they are probably the sign of a bug in the program. So
complaining loudly via BUG() is probably our best bet. We could always
loosen it later if somebody actually adds code where another behavior
makes sense (we know there are not such paths now, as they'd segfault
under openssl's heap-based backend).
-Peff
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/7] hash: use git_hash_init() consistently
2026-07-07 14:39 ` Junio C Hamano
@ 2026-07-07 20:13 ` Jeff King
2026-07-07 20:17 ` Junio C Hamano
0 siblings, 1 reply; 35+ messages in thread
From: Jeff King @ 2026-07-07 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, brian m. carlson
On Tue, Jul 07, 2026 at 07:39:24AM -0700, Junio C Hamano wrote:
> > diff --git a/object-file.c b/object-file.c
> > index e3c68cfb66..f292683c2d 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > ...
> > - algo->init_fn(c);
> > - if (compat && compat_c)
> > - compat->init_fn(compat_c);
> > + git_hash_init(c, algo);
> > + if (compat && compat_c) {
> > + git_hash_init(compat_c, compat);
> > + }
>
> For example, it is a mystery how Coccinelle decided to add a pair of
> braces around this single statement. It should be obvious that the
> corresponding single statement in the original did not need one.
Yeah, I noticed that coccinelle was eager to add braces in a few cases,
but I'm not sure why.
I had actually removed them, but either I missed these two, or more
likely I ended up re-applying the semantic patch a final time before
committing (I did a lot of "reset --hard; make hash.cocci.patch && git
apply hash.cocci.patch" while testing various refactors of the patch
itself).
I'll drop them in v2. Thanks for reading carefully.
-Peff
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/7] hash: use git_hash_init() consistently
2026-07-07 20:13 ` Jeff King
@ 2026-07-07 20:17 ` Junio C Hamano
2026-07-07 20:25 ` Jeff King
0 siblings, 1 reply; 35+ messages in thread
From: Junio C Hamano @ 2026-07-07 20:17 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt, brian m. carlson
Jeff King <peff@peff.net> writes:
> On Tue, Jul 07, 2026 at 07:39:24AM -0700, Junio C Hamano wrote:
>
>> > diff --git a/object-file.c b/object-file.c
>> > index e3c68cfb66..f292683c2d 100644
>> > --- a/object-file.c
>> > +++ b/object-file.c
>> > ...
>> > - algo->init_fn(c);
>> > - if (compat && compat_c)
>> > - compat->init_fn(compat_c);
>> > + git_hash_init(c, algo);
>> > + if (compat && compat_c) {
>> > + git_hash_init(compat_c, compat);
>> > + }
>>
>> For example, it is a mystery how Coccinelle decided to add a pair of
>> braces around this single statement. It should be obvious that the
>> corresponding single statement in the original did not need one.
>
> Yeah, I noticed that coccinelle was eager to add braces in a few cases,
> but I'm not sure why.
>
> I had actually removed them, but either I missed these two, or more
> likely I ended up re-applying the semantic patch a final time before
> committing (I did a lot of "reset --hard; make hash.cocci.patch && git
> apply hash.cocci.patch" while testing various refactors of the patch
> itself).
>
> I'll drop them in v2. Thanks for reading carefully.
Thanks.
If we run cocci twice, the second time it should be idempotent,
right? So running it once, fixing these braces and then running it
again would not make us see the extra braces in the result, I guess.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/7] hash: make git_hash_discard() idempotent
2026-07-07 16:22 ` Junio C Hamano
@ 2026-07-07 20:18 ` Jeff King
2026-07-07 21:41 ` brian m. carlson
0 siblings, 1 reply; 35+ messages in thread
From: Jeff King @ 2026-07-07 20:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, brian m. carlson
On Tue, Jul 07, 2026 at 09:22:04AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > You must always either finalize or discard a hash context to release any
> > resources, but you must call only one such function. This creates extra
> > work for some callers, since their cleanup code paths need to know
> > whether they got there via their happy path (and the finalization
> > happened) or due to an error (in which case they need to discard).
> >
> > Let's add an "active" flag that turns a redundant discard into a noop.
> > That lets you safely do this:
> >
> > git_hash_init(&ctx, algo);
> > ...
> > if (some_error)
> > goto out;
> > ...
> > git_hash_final(result, &ctx);
> >
> > out:
> > git_hash_discard(&ctx);
> >
> > This should avoid future errors, and will also let us simplify a few
> > existing callers (in future patches).
>
> Hmph, so is the point of this change to allow _discard() to be
> called even after _final() was already called that we do not need an
> early return or something before the out: label?
Right. Maybe fleshing out this example was not a good idea, as yeah, you
could fix it with an early return. If there were more cleanup in the
"out" label it would be harder. In practice neither of the spots we're
able to clean up look exactly like this. They are split across multiple
functions. So maybe:
/* foo contains a git_hash_ctx and initializes it here */
foo_init(&foo);
if (some_error)
foo_release(&foo);
git_hash_final(&foo.ctx);
foo_release(&foo);
would be more realistic. The problem is that foo_release() doesn't know
if the hash was finalized or not.
> Unlike commit_*() and rollback_*() used in lockfile API, where the
> names clearly say which one is for happy and which one is for error
> case, the _final() and _discard() pair does not exactly tell me
> which is which, but I guess I will get used to it, perhaps.
Hmm, I had hoped that "discard" versus just "release" would communicate
that. "final" is a bit funny, but that is the long-standing name for
that hash operation (both in our code and in libraries).
> But the change nevertheless looks mostly good except for one "hmph".
> When _init() is called, active gets turned on automatically, and
> either _discard() or _final() turns it off. Only _discard() is
> protected from getting called multiple times. Is this because
> it is already a no-op to call _final() multiple times?
No, it's a bug to call _final() multiple times. See my response
elsewhere in the thread.
-Peff
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/7] hash: use git_hash_init() consistently
2026-07-07 20:17 ` Junio C Hamano
@ 2026-07-07 20:25 ` Jeff King
0 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-07 20:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, brian m. carlson
On Tue, Jul 07, 2026 at 01:17:49PM -0700, Junio C Hamano wrote:
> > I had actually removed them, but either I missed these two, or more
> > likely I ended up re-applying the semantic patch a final time before
> > committing (I did a lot of "reset --hard; make hash.cocci.patch && git
> > apply hash.cocci.patch" while testing various refactors of the patch
> > itself).
> >
> > I'll drop them in v2. Thanks for reading carefully.
>
> Thanks.
>
> If we run cocci twice, the second time it should be idempotent,
> right? So running it once, fixing these braces and then running it
> again would not make us see the extra braces in the result, I guess.
Yep, exactly.
If my "re-applying" theory above is correct, that is different because I
was calling "reset --hard" in the middle to test that the patch still
did what it claimed. ;)
I assume this is coccinelle having some kind of "add braces to be
careful in some situations" logic, but I didn't dig into it further.
-Peff
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/7] hash: use git_hash_init() consistently
2026-07-07 5:01 ` [PATCH 1/7] hash: use git_hash_init() consistently Jeff King
2026-07-07 14:39 ` Junio C Hamano
@ 2026-07-07 21:25 ` brian m. carlson
2026-07-08 3:54 ` Jeff King
1 sibling, 1 reply; 35+ messages in thread
From: brian m. carlson @ 2026-07-07 21:25 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt
[-- Attachment #1: Type: text/plain, Size: 981 bytes --]
On 2026-07-07 at 05:01:41, Jeff King wrote:
> We'd like to add more logic to git_hash_init(), but many callers skip it
> and call algop->init_fn() directly. Let's make sure we're consistently
> using the wrapper by adding a coccinelle rule.
>
> Besides the coccinelle file itself, this is a purely mechanical
> conversion based on the patch it generates. There should be no bare
> init_fn() calls left (except for the one in the wrapper).
For context, the reason `git_hash_init` exists is that our Rust code
needs to initialize a hash context but it treats `const struct
git_hash_algo *` as `const void *` and doesn't have any access to the
contents of the structure. We could fix this with `cbindgen` and
`bindgen`, but haven't done so yet.
So that's why everybody has been using `init_fn` instead of
`git_hash_init`. Anyway, I have no objections to making this the
standard interface going forward.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/7] hash: make git_hash_discard() idempotent
2026-07-07 20:18 ` Jeff King
@ 2026-07-07 21:41 ` brian m. carlson
2026-07-07 22:25 ` Junio C Hamano
0 siblings, 1 reply; 35+ messages in thread
From: brian m. carlson @ 2026-07-07 21:41 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Patrick Steinhardt
[-- Attachment #1: Type: text/plain, Size: 2291 bytes --]
On 2026-07-07 at 20:18:08, Jeff King wrote:
> On Tue, Jul 07, 2026 at 09:22:04AM -0700, Junio C Hamano wrote:
> > But the change nevertheless looks mostly good except for one "hmph".
> > When _init() is called, active gets turned on automatically, and
> > either _discard() or _final() turns it off. Only _discard() is
> > protected from getting called multiple times. Is this because
> > it is already a no-op to call _final() multiple times?
>
> No, it's a bug to call _final() multiple times. See my response
> elsewhere in the thread.
This is almost always the case in hash function libraries. Let me
explain why.
A context for SHA-256 contains the 8 32-bit words in the state, a bit or
byte counter (as a 64-bit quantity or two 32-bit quantities), and a
64-byte buffer for unprocessed bytes—and that's it. When finalizing a
hash, you must always pad with a 0x80 byte and then optionally some zero
bytes, plus a 64-bit counter of bits in the message. That may result in
one or two iterations of the hash to process the remaining bytes and the
padding, and that almost always updates the state words in the context
in place. (SHA-1 functions identically but for the state size.)
So if you call the final function multiple times, you're not computing
the final value the second time, but instead trying to re-pad and
re-compute the final hash value, which results in a _different_,
incorrect value. In SHA-256, this is a valid hash value for a different
message (which is the original message with the padding and length
tacked on and is effectively a length-extension attack), but in hashes
that don't allow length-extension attacks, such as SHA-3 and BLAKE2,
what you get is simply corrupt data.
So most hash function libraries that allocate memory are going to free
it in the final function because you can't really call final multiple
times and get a sensible response. If you want to do that, then you
need to clone the context and call final on each context once.
Our Rust code makes calling final a second time impossible because
finalization takes `self`, not `&mut self`, so the object is _moved_
into the final method and you no longer have access to it after that.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/7] hash: make git_hash_discard() idempotent
2026-07-07 21:41 ` brian m. carlson
@ 2026-07-07 22:25 ` Junio C Hamano
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2026-07-07 22:25 UTC (permalink / raw)
To: brian m. carlson; +Cc: Jeff King, git, Patrick Steinhardt
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> Our Rust code makes calling final a second time impossible because
> finalization takes `self`, not `&mut self`, so the object is _moved_
> into the final method and you no longer have access to it after that.
That is a cute trick available to Rust but not many other languages,
I guess ;-).
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 0/7] git_hash_*() quality-of-life improvements
2026-07-07 4:55 [PATCH 0/7] git_hash_*() quality-of-life improvements Jeff King
` (7 preceding siblings ...)
2026-07-07 14:26 ` [PATCH 0/7] git_hash_*() quality-of-life improvements Patrick Steinhardt
@ 2026-07-08 3:52 ` Jeff King
2026-07-08 3:52 ` [PATCH v2 1/7] hash: use git_hash_init() consistently Jeff King
` (7 more replies)
8 siblings, 8 replies; 35+ messages in thread
From: Jeff King @ 2026-07-08 3:52 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson, Junio C Hamano
On Tue, Jul 07, 2026 at 12:55:57AM -0400, Jeff King wrote:
> This implements the "idempotent git_hash_discard()" discussed in this
> subthread:
>
> https://lore.kernel.org/git/20260702080707.GG2029434@coredump.intra.peff.net/
>
> with associated cleanups.
Here's a v2 addressing the comments so far. Mostly minor changes:
- fixed typos noticed by Patrick
- dropped extra braces added by coccinelle
- dropped a trailing blank line from patch 1 (this gets fixed in a
later patch as we add more content after the blank line, but I
noticed "git apply" complaining)
- a bit more explanation in patch 7 about why we don't support
idempotent final() calls
Patch list below, followed by range diff.
[1/7]: hash: use git_hash_init() consistently
[2/7]: hash: convert remaining direct function calls
[3/7]: hash: document function pointers and wrappers
[4/7]: hash: make git_hash_discard() idempotent
[5/7]: csum-file: use idempotent git_hash_discard()
[6/7]: http: use idempotent git_hash_discard()
[7/7]: hash: check ctx->active flag in all wrapper functions
builtin/fast-import.c | 4 +--
builtin/index-pack.c | 6 ++--
builtin/patch-id.c | 2 +-
builtin/receive-pack.c | 6 ++--
builtin/submodule--helper.c | 10 +++---
builtin/unpack-objects.c | 4 +--
csum-file.c | 23 +++++---------
diff.c | 4 +--
hash.c | 16 ++++++++++
hash.h | 44 +++++++++++++++++++-------
http-push.c | 2 +-
http.c | 9 ++----
http.h | 1 -
object-file.c | 14 ++++-----
pack-check.c | 2 +-
pack-write.c | 6 ++--
read-cache.c | 6 ++--
rerere.c | 2 +-
t/helper/test-hash-speed.c | 2 +-
t/helper/test-hash.c | 2 +-
t/helper/test-synthesize.c | 33 ++++++++++---------
t/unit-tests/u-hash.c | 2 +-
tools/coccinelle/hash.cocci | 63 +++++++++++++++++++++++++++++++++++++
trace2/tr2_sid.c | 2 +-
24 files changed, 177 insertions(+), 88 deletions(-)
create mode 100644 tools/coccinelle/hash.cocci
1: 2f1c8cbc98 ! 1: 911cf0dfcd hash: use git_hash_init() consistently
@@ object-file.c: static int start_loose_object_common(struct odb_source_loose *loo
stream->next_out = buf;
stream->avail_out = buflen;
- algo->init_fn(c);
-- if (compat && compat_c)
-- compat->init_fn(compat_c);
+ git_hash_init(c, algo);
-+ if (compat && compat_c) {
+ if (compat && compat_c)
+- compat->init_fn(compat_c);
+ git_hash_init(compat_c, compat);
-+ }
/* Start to feed header to zlib stream */
stream->next_in = (unsigned char *)hdr;
@@ read-cache.c: static size_t read_eoie_extension(const char *mmap, size_t mmap_si
## rerere.c ##
@@ rerere.c: static int handle_path(unsigned char *hash, struct rerere_io *io, int marker_siz
- struct git_hash_ctx ctx;
struct strbuf buf = STRBUF_INIT, out = STRBUF_INIT;
int has_conflicts = 0;
-- if (hash)
+ if (hash)
- the_hash_algo->init_fn(&ctx);
-+ if (hash) {
+ git_hash_init(&ctx, the_hash_algo);
-+ }
while (!io->getline(&buf, io)) {
if (is_cmarker(buf.buf, '<', marker_size)) {
@@ tools/coccinelle/hash.cocci (new)
+- ALGO->init_fn(CTX);
++ git_hash_init(CTX, ALGO);
+ ...>}
-+
## trace2/tr2_sid.c ##
@@ trace2/tr2_sid.c: static void tr2_sid_append_my_sid_component(void)
2: cf88edda3f ! 2: 879962bf47 hash: convert remaining direct function calls
@@ t/helper/test-synthesize.c: static int generate_pack_with_large_object(const cha
## tools/coccinelle/hash.cocci ##
@@ tools/coccinelle/hash.cocci: struct git_hash_ctx *CTX;
+ - ALGO->init_fn(CTX);
+ git_hash_init(CTX, ALGO);
...>}
-
++
+@@
+identifier f != git_hash_clone;
+expression ALGO;
3: 3c302bbe74 ! 3: f06387a467 hash: document function pointers and wrappers
@@ hash.h: struct git_hash_algo {
+/*
+ * Prepare an uninitialized hash context for use. You must eventually release
-+ * the context with with git_hash_final() (or final_oid()) or by calling
++ * the context with git_hash_final() (or final_oid()) or by calling
+ * git_hash_discard().
+ */
void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop);
4: e8b50b164a = 4: 2d876c17b1 hash: make git_hash_discard() idempotent
5: 5488debdae = 5: 3d3d5d2c63 csum-file: use idempotent git_hash_discard()
6: 15fd04f519 = 6: 91bda10e58 http: use idempotent git_hash_discard()
7: 5370cd31a8 ! 7: 2b366bc79b hash: check ctx->active flag in all wrapper functions
@@ Commit message
context onto the heap). Checking the flag lets us catch problems
consistently on every build.
- Note that we can't do the same for git_init_hash(). Even though it would
+ Note that we can't do the same for git_hash_init(). Even though it would
cause a leak to call it twice (without an intervening final/discard),
the point of the function is that the contents of the struct are
undefined before the call. But calling it twice is an even less likely
error to make, so not covering it is OK.
+ We leave git_hash_discard() alone, as its idempotent behavior is
+ convenient for callers. We _could_ try to do something similar for
+ git_hash_final(), allowing:
+
+ git_hash_final(result, &ctx);
+ git_hash_final(other_result, &ctx);
+
+ but it does not make much sense. After the first final() call we have
+ thrown away the state, so we cannot produce the same output. We could
+ come up with some sensible output (the null hash, or the empty hash),
+ but double-calls like this are more likely a bug, so our best bet is to
+ complain loudly (whereas the current code produces either nonsense
+ output or undefined behavior, depending on the backend).
+
Signed-off-by: Jeff King <peff@peff.net>
## hash.c ##
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 1/7] hash: use git_hash_init() consistently
2026-07-08 3:52 ` [PATCH v2 " Jeff King
@ 2026-07-08 3:52 ` Jeff King
2026-07-08 3:52 ` [PATCH v2 2/7] hash: convert remaining direct function calls Jeff King
` (6 subsequent siblings)
7 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-08 3:52 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson, Junio C Hamano
We'd like to add more logic to git_hash_init(), but many callers skip it
and call algop->init_fn() directly. Let's make sure we're consistently
using the wrapper by adding a coccinelle rule.
Besides the coccinelle file itself, this is a purely mechanical
conversion based on the patch it generates. There should be no bare
init_fn() calls left (except for the one in the wrapper).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-import.c | 4 ++--
builtin/index-pack.c | 6 +++---
builtin/patch-id.c | 2 +-
builtin/receive-pack.c | 6 +++---
builtin/submodule--helper.c | 2 +-
builtin/unpack-objects.c | 4 ++--
csum-file.c | 6 +++---
diff.c | 4 ++--
http-push.c | 2 +-
http.c | 4 ++--
object-file.c | 14 +++++++-------
pack-check.c | 2 +-
pack-write.c | 6 +++---
read-cache.c | 6 +++---
rerere.c | 2 +-
t/helper/test-hash-speed.c | 2 +-
t/helper/test-hash.c | 2 +-
t/helper/test-synthesize.c | 4 ++--
t/unit-tests/u-hash.c | 2 +-
tools/coccinelle/hash.cocci | 9 +++++++++
trace2/tr2_sid.c | 2 +-
21 files changed, 50 insertions(+), 41 deletions(-)
create mode 100644 tools/coccinelle/hash.cocci
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index f6473dcc8e..6692f7cd81 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -969,7 +969,7 @@ static int store_object(
hdrlen = format_object_header((char *)hdr, sizeof(hdr), type,
dat->len);
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, hdr, hdrlen);
git_hash_update(&c, dat->buf, dat->len);
git_hash_final_oid(&oid, &c);
@@ -1131,7 +1131,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
hdrlen = format_object_header((char *)out_buf, out_sz, OBJ_BLOB, len);
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, out_buf, hdrlen);
crc32_begin(pack_file);
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f396658468..53a8cb9dd7 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -374,7 +374,7 @@ static const char *open_pack_file(const char *pack_name)
output_fd = -1;
nothread_data.pack_fd = input_fd;
}
- the_hash_algo->init_fn(&input_ctx);
+ git_hash_init(&input_ctx, the_hash_algo);
return pack_name;
}
@@ -481,7 +481,7 @@ static void *unpack_entry_data(off_t offset, size_t size,
if (!is_delta_type(type)) {
hdrlen = format_object_header(hdr, sizeof(hdr), type, size);
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, hdr, hdrlen);
} else
oid = NULL;
@@ -1291,7 +1291,7 @@ static void parse_pack_objects(unsigned char *hash)
/* Check pack integrity */
flush();
- the_hash_algo->init_fn(&tmp_ctx);
+ git_hash_init(&tmp_ctx, the_hash_algo);
git_hash_clone(&tmp_ctx, &input_ctx);
git_hash_final(hash, &tmp_ctx);
if (!hasheq(fill(the_hash_algo->rawsz), hash, the_repository->hash_algo))
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 57d9bd4a65..22f36ecf80 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -73,7 +73,7 @@ static size_t get_one_patchid(struct object_id *next_oid, struct object_id *resu
char pre_oid_str[GIT_MAX_HEXSZ + 1], post_oid_str[GIT_MAX_HEXSZ + 1];
struct git_hash_ctx ctx;
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
oidclr(result, the_repository->hash_algo);
while (strbuf_getwholeline(line_buf, stdin, '\n') != EOF) {
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 19eb6a1b61..faf0f120ac 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -615,7 +615,7 @@ static void hmac_hash(unsigned char *out,
/* RFC 2104 2. (1) */
memset(key, '\0', GIT_MAX_BLKSZ);
if (the_hash_algo->blksz < key_len) {
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
git_hash_update(&ctx, key_in, key_len);
git_hash_final(key, &ctx);
} else {
@@ -629,13 +629,13 @@ static void hmac_hash(unsigned char *out,
}
/* RFC 2104 2. (3) & (4) */
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
git_hash_update(&ctx, k_ipad, sizeof(k_ipad));
git_hash_update(&ctx, text, text_len);
git_hash_final(out, &ctx);
/* RFC 2104 2. (6) & (7) */
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
git_hash_update(&ctx, k_opad, sizeof(k_opad));
git_hash_update(&ctx, out, the_hash_algo->rawsz);
git_hash_final(out, &ctx);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1cc82a134d..bf114a7856 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -550,7 +550,7 @@ static void create_default_gitdir_config(const char *submodule_name)
/* Case 2.4: If all the above failed, try a hash of the name as a last resort */
header_len = snprintf(header, sizeof(header), "blob %zu", strlen(submodule_name));
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
the_hash_algo->update_fn(&ctx, header, header_len);
the_hash_algo->update_fn(&ctx, "\0", 1);
the_hash_algo->update_fn(&ctx, submodule_name, strlen(submodule_name));
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index f3849bb654..93a9caa582 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -670,10 +670,10 @@ int cmd_unpack_objects(int argc,
/* We don't take any non-flag arguments now.. Maybe some day */
usage(unpack_usage);
}
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
unpack_all();
git_hash_update(&ctx, buffer, offset);
- the_hash_algo->init_fn(&tmp_ctx);
+ git_hash_init(&tmp_ctx, the_hash_algo);
git_hash_clone(&tmp_ctx, &ctx);
git_hash_final_oid(&oid, &tmp_ctx);
if (strict) {
diff --git a/csum-file.c b/csum-file.c
index b166f89624..7e81391524 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -175,7 +175,7 @@ struct hashfile *hashfd_ext(const struct git_hash_algo *algop,
f->skip_hash = 0;
f->algop = unsafe_hash_algo(algop);
- f->algop->init_fn(&f->ctx);
+ git_hash_init(&f->ctx, f->algop);
f->buffer_len = opts->buffer_len ? opts->buffer_len : DEFAULT_IO_BUFFER_SIZE;
f->buffer = xmalloc(f->buffer_len);
@@ -200,7 +200,7 @@ void hashfile_checkpoint_init(struct hashfile *f,
struct hashfile_checkpoint *checkpoint)
{
memset(checkpoint, 0, sizeof(*checkpoint));
- f->algop->init_fn(&checkpoint->ctx);
+ git_hash_init(&checkpoint->ctx, f->algop);
}
void hashfile_checkpoint(struct hashfile *f, struct hashfile_checkpoint *checkpoint)
@@ -252,7 +252,7 @@ int hashfile_checksum_valid(const struct git_hash_algo *algop,
if (total_len < algop->rawsz)
return 0; /* say "too short"? */
- algop->init_fn(&ctx);
+ git_hash_init(&ctx, algop);
git_hash_update(&ctx, data, data_len);
git_hash_final(got, &ctx);
diff --git a/diff.c b/diff.c
index 1568f0ed9c..589c1969e4 100644
--- a/diff.c
+++ b/diff.c
@@ -6855,7 +6855,7 @@ void flush_one_hunk(struct object_id *result, struct git_hash_ctx *ctx)
int i;
git_hash_final(hash, ctx);
- the_hash_algo->init_fn(ctx);
+ git_hash_init(ctx, the_hash_algo);
/* 20-byte sum, with carry */
for (i = 0; i < the_hash_algo->rawsz; ++i) {
carry += result->hash[i] + hash[i];
@@ -6899,7 +6899,7 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid
struct git_hash_ctx ctx;
struct patch_id_t data;
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
memset(&data, 0, sizeof(struct patch_id_t));
data.ctx = &ctx;
oidclr(oid, the_repository->hash_algo);
diff --git a/http-push.c b/http-push.c
index 3c23cbba27..60f6f8f054 100644
--- a/http-push.c
+++ b/http-push.c
@@ -776,7 +776,7 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)
} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {
lock->token = xstrdup(ctx->cdata);
- the_hash_algo->init_fn(&hash_ctx);
+ git_hash_init(&hash_ctx, the_hash_algo);
git_hash_update(&hash_ctx, lock->token, strlen(lock->token));
git_hash_final(lock_token_hash, &hash_ctx);
diff --git a/http.c b/http.c
index 63abbaae8a..0341de5031 100644
--- a/http.c
+++ b/http.c
@@ -2879,7 +2879,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
git_inflate_init(&freq->stream);
- the_hash_algo->init_fn(&freq->c);
+ git_hash_init(&freq->c, the_hash_algo);
freq->hash_ctx_valid = 1;
freq->url = get_remote_object_url(base_url, hex, 0);
@@ -2916,7 +2916,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
git_inflate_end(&freq->stream);
memset(&freq->stream, 0, sizeof(freq->stream));
git_inflate_init(&freq->stream);
- the_hash_algo->init_fn(&freq->c);
+ git_hash_init(&freq->c, the_hash_algo);
if (prev_posn>0) {
prev_posn = 0;
lseek(freq->localfile, 0, SEEK_SET);
diff --git a/object-file.c b/object-file.c
index e3c68cfb66..93602f8c50 100644
--- a/object-file.c
+++ b/object-file.c
@@ -124,7 +124,7 @@ int stream_object_signature(struct repository *r,
hdrlen = format_object_header(hdr, sizeof(hdr), st->type, st->size);
/* Sha1.. */
- r->hash_algo->init_fn(&c);
+ git_hash_init(&c, r->hash_algo);
git_hash_update(&c, hdr, hdrlen);
for (;;) {
char buf[1024 * 16];
@@ -320,7 +320,7 @@ static void hash_object_body(const struct git_hash_algo *algo, struct git_hash_c
struct object_id *oid,
char *hdr, size_t *hdrlen)
{
- algo->init_fn(c);
+ git_hash_init(c, algo);
git_hash_update(c, hdr, *hdrlen);
git_hash_update(c, buf, len);
git_hash_final_oid(oid, c);
@@ -681,9 +681,9 @@ static int start_loose_object_common(struct odb_source_loose *loose,
git_deflate_init(stream, cfg->zlib_compression_level);
stream->next_out = buf;
stream->avail_out = buflen;
- algo->init_fn(c);
+ git_hash_init(c, algo);
if (compat && compat_c)
- compat->init_fn(compat_c);
+ git_hash_init(compat_c, compat);
/* Start to feed header to zlib stream */
stream->next_in = (unsigned char *)hdr;
@@ -1141,7 +1141,7 @@ static int hash_blob_stream(struct odb_write_stream *stream,
header_len = format_object_header((char *)buf, sizeof(buf),
OBJ_BLOB, size);
- hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, hash_algo);
git_hash_update(&ctx, buf, header_len);
while (!stream->is_finished) {
@@ -1313,7 +1313,7 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
header_len = format_object_header((char *)obuf, sizeof(obuf),
OBJ_BLOB, size);
- transaction->base.source->odb->repo->hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, transaction->base.source->odb->repo->hash_algo);
git_hash_update(&ctx, obuf, header_len);
/*
@@ -1560,7 +1560,7 @@ static int check_stream_oid(git_zstream *stream,
unsigned long total_read;
int status = Z_OK;
- algop->init_fn(&c);
+ git_hash_init(&c, algop);
git_hash_update(&c, hdr, stream->total_out);
/*
diff --git a/pack-check.c b/pack-check.c
index 5adfb3f272..c3b8db7c5c 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -69,7 +69,7 @@ static int verify_packfile(struct repository *r,
if (!is_pack_valid(p))
return error("packfile %s cannot be accessed", p->pack_name);
- r->hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, r->hash_algo);
do {
unsigned long remaining;
unsigned char *in = use_pack(p, w_curs, offset, &remaining);
diff --git a/pack-write.c b/pack-write.c
index 83eaf88541..24033a9101 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -402,8 +402,8 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
char *buf;
ssize_t read_result;
- hash_algo->init_fn(&old_hash_ctx);
- hash_algo->init_fn(&new_hash_ctx);
+ git_hash_init(&old_hash_ctx, hash_algo);
+ git_hash_init(&new_hash_ctx, hash_algo);
if (lseek(pack_fd, 0, SEEK_SET) != 0)
die_errno("Failed seeking to start of '%s'", pack_name);
@@ -455,7 +455,7 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
* pack, which also means making partial_pack_offset
* big enough not to matter anymore.
*/
- hash_algo->init_fn(&old_hash_ctx);
+ git_hash_init(&old_hash_ctx, hash_algo);
partial_pack_offset = ~partial_pack_offset;
partial_pack_offset -= MSB(partial_pack_offset, 1);
}
diff --git a/read-cache.c b/read-cache.c
index 7c1cdcf696..5fa747e6fc 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1722,7 +1722,7 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size)
if (oideq(&oid, null_oid(the_hash_algo)))
return 0;
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
git_hash_update(&c, hdr, size - the_hash_algo->rawsz);
git_hash_final(hash, &c);
if (!hasheq(hash, start, the_repository->hash_algo))
@@ -2957,7 +2957,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
*/
if (offset && record_eoie()) {
CALLOC_ARRAY(eoie_c, 1);
- the_hash_algo->init_fn(eoie_c);
+ git_hash_init(eoie_c, the_hash_algo);
}
/*
@@ -3598,7 +3598,7 @@ static size_t read_eoie_extension(const char *mmap, size_t mmap_size)
* "REUC" + <binary representation of M>)
*/
src_offset = offset;
- the_hash_algo->init_fn(&c);
+ git_hash_init(&c, the_hash_algo);
while (src_offset < mmap_size - the_hash_algo->rawsz - EOIE_SIZE_WITH_HEADER) {
/* After an array of active_nr index entries,
* there can be arbitrary number of extended
diff --git a/rerere.c b/rerere.c
index 8232542585..216100925a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -439,7 +439,7 @@ static int handle_path(unsigned char *hash, struct rerere_io *io, int marker_siz
struct strbuf buf = STRBUF_INIT, out = STRBUF_INIT;
int has_conflicts = 0;
if (hash)
- the_hash_algo->init_fn(&ctx);
+ git_hash_init(&ctx, the_hash_algo);
while (!io->getline(&buf, io)) {
if (is_cmarker(buf.buf, '<', marker_size)) {
diff --git a/t/helper/test-hash-speed.c b/t/helper/test-hash-speed.c
index fbf67fe6bd..89b0268011 100644
--- a/t/helper/test-hash-speed.c
+++ b/t/helper/test-hash-speed.c
@@ -5,7 +5,7 @@
static inline void compute_hash(const struct git_hash_algo *algo, struct git_hash_ctx *ctx, uint8_t *final, const void *p, size_t len)
{
- algo->init_fn(ctx);
+ git_hash_init(ctx, algo);
git_hash_update(ctx, p, len);
git_hash_final(final, ctx);
}
diff --git a/t/helper/test-hash.c b/t/helper/test-hash.c
index f0ee61c8b4..1f7163695f 100644
--- a/t/helper/test-hash.c
+++ b/t/helper/test-hash.c
@@ -29,7 +29,7 @@ int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)
die("OOPS");
}
- algop->init_fn(&ctx);
+ git_hash_init(&ctx, algop);
while (1) {
ssize_t sz, this_sz;
diff --git a/t/helper/test-synthesize.c b/t/helper/test-synthesize.c
index 3fa534fbdf..7719fb3a76 100644
--- a/t/helper/test-synthesize.c
+++ b/t/helper/test-synthesize.c
@@ -97,7 +97,7 @@ static void write_pack_object(FILE *f, struct git_hash_ctx *pack_ctx,
/* Write the data as uncompressed zlib */
write_uncompressed_zlib(f, pack_ctx, data, len, algo);
- algo->init_fn(&ctx);
+ git_hash_init(&ctx, algo);
object_header_len = format_object_header(object_header,
sizeof(object_header),
type, len);
@@ -430,7 +430,7 @@ static int generate_pack_with_large_object(const char *path, size_t blob_size,
f = xfopen(path, "wb");
- algo->init_fn(&pack_ctx);
+ git_hash_init(&pack_ctx, algo);
/* Write pack header */
fwrite_or_die(f, &pack_header, sizeof(pack_header));
diff --git a/t/unit-tests/u-hash.c b/t/unit-tests/u-hash.c
index bd4ac6a6e1..19f4efd410 100644
--- a/t/unit-tests/u-hash.c
+++ b/t/unit-tests/u-hash.c
@@ -12,7 +12,7 @@ static void check_hash_data(const void *data, size_t data_length,
unsigned char hash[GIT_MAX_HEXSZ];
const struct git_hash_algo *algop = &hash_algos[i];
- algop->init_fn(&ctx);
+ git_hash_init(&ctx, algop);
git_hash_update(&ctx, data, data_length);
git_hash_final(hash, &ctx);
diff --git a/tools/coccinelle/hash.cocci b/tools/coccinelle/hash.cocci
new file mode 100644
index 0000000000..04270ee043
--- /dev/null
+++ b/tools/coccinelle/hash.cocci
@@ -0,0 +1,9 @@
+@@
+identifier f != git_hash_init;
+expression ALGO;
+struct git_hash_ctx *CTX;
+@@
+ f(...) {<...
+- ALGO->init_fn(CTX);
++ git_hash_init(CTX, ALGO);
+ ...>}
diff --git a/trace2/tr2_sid.c b/trace2/tr2_sid.c
index 1c1d27b0ee..131b4f5a62 100644
--- a/trace2/tr2_sid.c
+++ b/trace2/tr2_sid.c
@@ -45,7 +45,7 @@ static void tr2_sid_append_my_sid_component(void)
if (xgethostname(hostname, sizeof(hostname)))
strbuf_add(&tr2sid_buf, "Localhost", 9);
else {
- algo->init_fn(&ctx);
+ git_hash_init(&ctx, algo);
git_hash_update(&ctx, hostname, strlen(hostname));
git_hash_final(hash, &ctx);
hash_to_hex_algop_r(hex, hash, algo);
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 2/7] hash: convert remaining direct function calls
2026-07-08 3:52 ` [PATCH v2 " Jeff King
2026-07-08 3:52 ` [PATCH v2 1/7] hash: use git_hash_init() consistently Jeff King
@ 2026-07-08 3:52 ` Jeff King
2026-07-08 3:52 ` [PATCH v2 3/7] hash: document function pointers and wrappers Jeff King
` (5 subsequent siblings)
7 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-08 3:52 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson, Junio C Hamano
The previous patch added a coccinelle rule to make sure callers always
use git_hash_init() rather than direct function pointers from the algo
struct.
Let's do the same for the rest of the git_hash_*() wrappers. I split
these out because they're a bit different: they implicitly use the algop
pointer in the git_hash_ctx. So when we convert:
-algo->update_fn(&ctx, buf, len);
+git_hash_update(&ctx, buf, len);
we drop the reference to algo entirely! But this is always going to be
the right thing. If "algo" does not match what is in ctx.algop, then
we'd already be invoking undefined behavior.
So in addition to making it possible to add more logic to the
git_hash_*() functions, we're avoiding the need to pass around the extra
algo pointer and make sure that it matches what's in "ctx".
The rest of the patch is the mechanical application of that coccinelle
patch, plus a minor cleanup in test-synthesize.c to drop a now-unused
function parameter (since we don't have to pass around the algo
separately anymore).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/submodule--helper.c | 8 +++---
t/helper/test-synthesize.c | 29 ++++++++++----------
tools/coccinelle/hash.cocci | 54 +++++++++++++++++++++++++++++++++++++
3 files changed, 72 insertions(+), 19 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index bf114a7856..510f193a15 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -551,10 +551,10 @@ static void create_default_gitdir_config(const char *submodule_name)
/* Case 2.4: If all the above failed, try a hash of the name as a last resort */
header_len = snprintf(header, sizeof(header), "blob %zu", strlen(submodule_name));
git_hash_init(&ctx, the_hash_algo);
- the_hash_algo->update_fn(&ctx, header, header_len);
- the_hash_algo->update_fn(&ctx, "\0", 1);
- the_hash_algo->update_fn(&ctx, submodule_name, strlen(submodule_name));
- the_hash_algo->final_fn(raw_name_hash, &ctx);
+ git_hash_update(&ctx, header, header_len);
+ git_hash_update(&ctx, "\0", 1);
+ git_hash_update(&ctx, submodule_name, strlen(submodule_name));
+ git_hash_final(raw_name_hash, &ctx);
hash_to_hex_algop_r(hex_name_hash, raw_name_hash, the_hash_algo);
strbuf_reset(&gitdir_path);
repo_git_path_append(the_repository, &gitdir_path, "modules/%s", hex_name_hash);
diff --git a/t/helper/test-synthesize.c b/t/helper/test-synthesize.c
index 7719fb3a76..fd116c87ba 100644
--- a/t/helper/test-synthesize.c
+++ b/t/helper/test-synthesize.c
@@ -25,8 +25,7 @@ static const unsigned char zeros[BLOCK_SIZE];
* Updates the pack checksum context.
*/
static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
- const void *data, size_t len,
- const struct git_hash_algo *algo)
+ const void *data, size_t len)
{
unsigned char zlib_header[2] = { 0x78, 0x01 }; /* CMF, FLG */
unsigned char block_header[5];
@@ -37,7 +36,7 @@ static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
/* Write zlib header */
fwrite_or_die(f, zlib_header, sizeof(zlib_header));
- algo->update_fn(pack_ctx, zlib_header, 2);
+ git_hash_update(pack_ctx, zlib_header, 2);
/* Write uncompressed blocks (max 64KB each) */
do {
@@ -52,11 +51,11 @@ static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
block_header[4] = block_header[2] ^ 0xff;
fwrite_or_die(f, block_header, sizeof(block_header));
- algo->update_fn(pack_ctx, block_header, 5);
+ git_hash_update(pack_ctx, block_header, 5);
if (block_len) {
fwrite_or_die(f, block_data, block_len);
- algo->update_fn(pack_ctx, block_data, block_len);
+ git_hash_update(pack_ctx, block_data, block_len);
adler = adler32(adler, block_data, block_len);
}
@@ -68,7 +67,7 @@ static void write_uncompressed_zlib(FILE *f, struct git_hash_ctx *pack_ctx,
/* Write adler32 checksum */
put_be32(adler_buf, adler);
fwrite_or_die(f, adler_buf, sizeof(adler_buf));
- algo->update_fn(pack_ctx, adler_buf, 4);
+ git_hash_update(pack_ctx, adler_buf, 4);
}
/*
@@ -92,24 +91,24 @@ static void write_pack_object(FILE *f, struct git_hash_ctx *pack_ctx,
sizeof(pack_header),
type, len);
fwrite_or_die(f, pack_header, pack_header_len);
- algo->update_fn(pack_ctx, pack_header, pack_header_len);
+ git_hash_update(pack_ctx, pack_header, pack_header_len);
/* Write the data as uncompressed zlib */
- write_uncompressed_zlib(f, pack_ctx, data, len, algo);
+ write_uncompressed_zlib(f, pack_ctx, data, len);
git_hash_init(&ctx, algo);
object_header_len = format_object_header(object_header,
sizeof(object_header),
type, len);
- algo->update_fn(&ctx, object_header, object_header_len);
+ git_hash_update(&ctx, object_header, object_header_len);
if (data)
- algo->update_fn(&ctx, data, len);
+ git_hash_update(&ctx, data, len);
else {
for (size_t i = len / BLOCK_SIZE; i; i--)
- algo->update_fn(&ctx, zeros, BLOCK_SIZE);
- algo->update_fn(&ctx, zeros, len % BLOCK_SIZE);
+ git_hash_update(&ctx, zeros, BLOCK_SIZE);
+ git_hash_update(&ctx, zeros, len % BLOCK_SIZE);
}
- algo->final_oid_fn(oid, &ctx);
+ git_hash_final_oid(oid, &ctx);
}
/*
@@ -434,7 +433,7 @@ static int generate_pack_with_large_object(const char *path, size_t blob_size,
/* Write pack header */
fwrite_or_die(f, &pack_header, sizeof(pack_header));
- algo->update_fn(&pack_ctx, &pack_header, sizeof(pack_header));
+ git_hash_update(&pack_ctx, &pack_header, sizeof(pack_header));
/* 1. Write the large blob */
write_pack_object(f, &pack_ctx, OBJ_BLOB, NULL, blob_size, &blob_oid, algo);
@@ -472,7 +471,7 @@ static int generate_pack_with_large_object(const char *path, size_t blob_size,
write_pack_object(f, &pack_ctx, OBJ_COMMIT, buf.buf, buf.len, &final_commit_oid, algo);
/* Write pack trailer (checksum) */
- algo->final_fn(pack_hash, &pack_ctx);
+ git_hash_final(pack_hash, &pack_ctx);
fwrite_or_die(f, pack_hash, algo->rawsz);
if (fclose(f))
die_errno(_("could not close '%s'"), path);
diff --git a/tools/coccinelle/hash.cocci b/tools/coccinelle/hash.cocci
index 04270ee043..d0e2e5f4b1 100644
--- a/tools/coccinelle/hash.cocci
+++ b/tools/coccinelle/hash.cocci
@@ -7,3 +7,57 @@ struct git_hash_ctx *CTX;
- ALGO->init_fn(CTX);
+ git_hash_init(CTX, ALGO);
...>}
+
+@@
+identifier f != git_hash_clone;
+expression ALGO;
+struct git_hash_ctx *SRC;
+struct git_hash_ctx *DST;
+@@
+ f(...) {<...
+- ALGO->clone_fn(DST, SRC);
++ git_hash_clone(DST, SRC);
+ ...>}
+
+@@
+identifier f != git_hash_update;
+expression ALGO;
+struct git_hash_ctx *CTX;
+expression list ARGS;
+@@
+ f(...) {<...
+- ALGO->update_fn(CTX, ARGS);
++ git_hash_update(CTX, ARGS);
+ ...>}
+
+@@
+identifier f != git_hash_final;
+expression ALGO;
+struct git_hash_ctx *CTX;
+expression list ARGS;
+@@
+ f(...) {<...
+- ALGO->final_fn(ARGS, CTX);
++ git_hash_final(ARGS, CTX);
+ ...>}
+
+@@
+identifier f != git_hash_final_oid;
+expression ALGO;
+struct git_hash_ctx *CTX;
+expression list ARGS;
+@@
+ f(...) {<...
+- ALGO->final_oid_fn(ARGS, CTX);
++ git_hash_final_oid(ARGS, CTX);
+ ...>}
+
+@@
+identifier f != git_hash_discard;
+expression ALGO;
+struct git_hash_ctx *CTX;
+@@
+ f(...) {<...
+- ALGO->discard_fn(CTX);
++ git_hash_discard(CTX);
+ ...>}
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 3/7] hash: document function pointers and wrappers
2026-07-08 3:52 ` [PATCH v2 " Jeff King
2026-07-08 3:52 ` [PATCH v2 1/7] hash: use git_hash_init() consistently Jeff King
2026-07-08 3:52 ` [PATCH v2 2/7] hash: convert remaining direct function calls Jeff King
@ 2026-07-08 3:52 ` Jeff King
2026-07-08 3:52 ` [PATCH v2 4/7] hash: make git_hash_discard() idempotent Jeff King
` (4 subsequent siblings)
7 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-08 3:52 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson, Junio C Hamano
We want people to use the git_hash_*() wrappers rather than the bare
function pointers in the git_hash_algo struct. Let's document them
rather than the bare pointers, and warn people away from the pointers.
Coccinelle will eventually force the use of the wrappers, but it's
helpful to lead readers in the right direction from the start.
While we're here we can document a few other bits of wisdom I've turned
up while working in this area:
- You have to initialize the destination of a git_hash_clone(). This
is something we may eventually change for efficiency, but we should
definitely document the requirement for now.
- You must eventually finalize or discard a hash, since some backends
may allocate resources during initialization.
Signed-off-by: Jeff King <peff@peff.net>
---
hash.h | 43 ++++++++++++++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 11 deletions(-)
diff --git a/hash.h b/hash.h
index 0a23ef4dfd..121ecf13aa 100644
--- a/hash.h
+++ b/hash.h
@@ -309,22 +309,15 @@ struct git_hash_algo {
/* The block size of the hash. */
size_t blksz;
- /* The hash initialization function. */
+ /*
+ * Low-level implementation hooks. Callers should use the git_hash_*
+ * wrappers below rather than invoking these directly.
+ */
git_hash_init_fn init_fn;
-
- /* The hash context cloning function. */
git_hash_clone_fn clone_fn;
-
- /* The hash update function. */
git_hash_update_fn update_fn;
-
- /* The hash finalization function. */
git_hash_final_fn final_fn;
-
- /* The hash finalization function for object IDs. */
git_hash_final_oid_fn final_oid_fn;
-
- /* Discard an initialized hash without finalizing. */
git_hash_discard_fn discard_fn;
/* The OID of the empty tree. */
@@ -341,12 +334,40 @@ struct git_hash_algo {
};
extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
+/*
+ * Prepare an uninitialized hash context for use. You must eventually release
+ * the context with git_hash_final() (or final_oid()) or by calling
+ * git_hash_discard().
+ */
void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop);
+
+/*
+ * Clone the state of a hash. Both src and dst must have been initialized with
+ * git_hash_init().
+ */
void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src);
+
+/*
+ * Add more data to an initialized hash context.
+ */
void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len);
+
+/*
+ * Retrieve the final hash value from a context, releasing any resources.
+ */
void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx);
+
+/*
+ * Like git_hash_final(), but write the result into an object_id.
+ */
void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx);
+
+/*
+ * Discard a hash context without computing the final value, but still
+ * releasing any resources.
+ */
void git_hash_discard(struct git_hash_ctx *ctx);
+
const struct git_hash_algo *hash_algo_ptr_by_number(uint32_t algo);
struct git_hash_ctx *git_hash_alloc(void);
void git_hash_free(struct git_hash_ctx *ctx);
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 4/7] hash: make git_hash_discard() idempotent
2026-07-08 3:52 ` [PATCH v2 " Jeff King
` (2 preceding siblings ...)
2026-07-08 3:52 ` [PATCH v2 3/7] hash: document function pointers and wrappers Jeff King
@ 2026-07-08 3:52 ` Jeff King
2026-07-08 3:53 ` [PATCH v2 5/7] csum-file: use idempotent git_hash_discard() Jeff King
` (3 subsequent siblings)
7 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-08 3:52 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson, Junio C Hamano
You must always either finalize or discard a hash context to release any
resources, but you must call only one such function. This creates extra
work for some callers, since their cleanup code paths need to know
whether they got there via their happy path (and the finalization
happened) or due to an error (in which case they need to discard).
Let's add an "active" flag that turns a redundant discard into a noop.
That lets you safely do this:
git_hash_init(&ctx, algo);
...
if (some_error)
goto out;
...
git_hash_final(result, &ctx);
out:
git_hash_discard(&ctx);
This should avoid future errors, and will also let us simplify a few
existing callers (in future patches).
Signed-off-by: Jeff King <peff@peff.net>
---
hash.c | 6 ++++++
hash.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/hash.c b/hash.c
index 55d1d41770..b1296f0018 100644
--- a/hash.c
+++ b/hash.c
@@ -285,6 +285,7 @@ void git_hash_free(struct git_hash_ctx *ctx)
void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop)
{
algop->init_fn(ctx);
+ ctx->active = true;
}
void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src)
@@ -300,16 +301,21 @@ void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len)
void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx)
{
ctx->algop->final_fn(hash, ctx);
+ ctx->active = false;
}
void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
{
ctx->algop->final_oid_fn(oid, ctx);
+ ctx->active = false;
}
void git_hash_discard(struct git_hash_ctx *ctx)
{
+ if (!ctx->active)
+ return;
ctx->algop->discard_fn(ctx);
+ ctx->active = false;
}
uint32_t hash_algo_by_name(const char *name)
diff --git a/hash.h b/hash.h
index 121ecf13aa..cf94ad5700 100644
--- a/hash.h
+++ b/hash.h
@@ -281,6 +281,7 @@ struct git_hash_ctx {
git_SHA_CTX_unsafe sha1_unsafe;
git_SHA256_CTX sha256;
} state;
+ bool active;
};
typedef void (*git_hash_init_fn)(struct git_hash_ctx *ctx);
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 5/7] csum-file: use idempotent git_hash_discard()
2026-07-08 3:52 ` [PATCH v2 " Jeff King
` (3 preceding siblings ...)
2026-07-08 3:52 ` [PATCH v2 4/7] hash: make git_hash_discard() idempotent Jeff King
@ 2026-07-08 3:53 ` Jeff King
2026-07-08 3:53 ` [PATCH v2 6/7] http: " Jeff King
` (2 subsequent siblings)
7 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-08 3:53 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson, Junio C Hamano
Now that it is safe to call git_hash_discard() even after finalizing it,
we can simplify our cleanup logic a bit. This is mostly undoing a few
bits of 64337aecde (csum-file: always finalize or discard hash,
2026-07-02):
- We no longer need a separate free_hashfile_memory() function for
finalize_hashfile(). It can just call free_hashfile(), which will
now discard (or not) the hash as appropriate.
- When f->skip_hash is set, we don't need to discard; we can rely on
free_hashfile() to do it.
Signed-off-by: Jeff King <peff@peff.net>
---
csum-file.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/csum-file.c b/csum-file.c
index 7e81391524..fe18ee1de3 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -55,32 +55,25 @@ void hashflush(struct hashfile *f)
}
}
-static void free_hashfile_memory(struct hashfile *f)
+void free_hashfile(struct hashfile *f)
{
+ git_hash_discard(&f->ctx);
free(f->buffer);
free(f->check_buffer);
free(f);
}
-void free_hashfile(struct hashfile *f)
-{
- git_hash_discard(&f->ctx);
- free_hashfile_memory(f);
-}
-
int finalize_hashfile(struct hashfile *f, unsigned char *result,
enum fsync_component component, unsigned int flags)
{
int fd;
hashflush(f);
- if (f->skip_hash) {
- git_hash_discard(&f->ctx);
+ if (f->skip_hash)
hashclr(f->buffer, f->algop);
- } else {
+ else
git_hash_final(f->buffer, &f->ctx);
- }
if (result)
hashcpy(result, f->buffer, f->algop);
@@ -105,7 +98,7 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,
if (close(f->check_fd))
die_errno("%s: sha1 file error on close", f->name);
}
- free_hashfile_memory(f);
+ free_hashfile(f);
return fd;
}
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 6/7] http: use idempotent git_hash_discard()
2026-07-08 3:52 ` [PATCH v2 " Jeff King
` (4 preceding siblings ...)
2026-07-08 3:53 ` [PATCH v2 5/7] csum-file: use idempotent git_hash_discard() Jeff King
@ 2026-07-08 3:53 ` Jeff King
2026-07-08 3:53 ` [PATCH v2 7/7] hash: check ctx->active flag in all wrapper functions Jeff King
2026-07-08 8:05 ` [PATCH v2 0/7] git_hash_*() quality-of-life improvements Patrick Steinhardt
7 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-08 3:53 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson, Junio C Hamano
Now that it is OK to call git_hash_discard() even after finalizing the
hash, we no longer need the ctx_valid bool added by a2d8ea5a76 (http:
discard hash in dumb-http http_object_request, 2026-07-02).
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 5 +----
http.h | 1 -
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/http.c b/http.c
index 0341de5031..caccf2108e 100644
--- a/http.c
+++ b/http.c
@@ -2880,7 +2880,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
git_inflate_init(&freq->stream);
git_hash_init(&freq->c, the_hash_algo);
- freq->hash_ctx_valid = 1;
freq->url = get_remote_object_url(base_url, hex, 0);
@@ -2989,7 +2988,6 @@ int finish_http_object_request(struct http_object_request *freq)
}
git_hash_final_oid(&freq->real_oid, &freq->c);
- freq->hash_ctx_valid = 0;
if (freq->zret != Z_STREAM_END) {
unlink_or_warn(freq->tmpfile.buf);
return -1;
@@ -3030,8 +3028,7 @@ void release_http_object_request(struct http_object_request **freq_p)
curl_slist_free_all(freq->headers);
strbuf_release(&freq->tmpfile);
git_inflate_end(&freq->stream);
- if (freq->hash_ctx_valid)
- git_hash_discard(&freq->c);
+ git_hash_discard(&freq->c);
free(freq);
*freq_p = NULL;
diff --git a/http.h b/http.h
index 6b0639150f..729c51904d 100644
--- a/http.h
+++ b/http.h
@@ -255,7 +255,6 @@ struct http_object_request {
struct object_id oid;
struct object_id real_oid;
struct git_hash_ctx c;
- int hash_ctx_valid;
git_zstream stream;
int zret;
int rename;
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 7/7] hash: check ctx->active flag in all wrapper functions
2026-07-08 3:52 ` [PATCH v2 " Jeff King
` (5 preceding siblings ...)
2026-07-08 3:53 ` [PATCH v2 6/7] http: " Jeff King
@ 2026-07-08 3:53 ` Jeff King
2026-07-08 8:05 ` [PATCH v2 0/7] git_hash_*() quality-of-life improvements Patrick Steinhardt
7 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-08 3:53 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, brian m. carlson, Junio C Hamano
It only makes sense to call git_hash_update(), etc, on a hash context
that has been initialized but not yet finalized or discarded. This is an
unlikely error to make, but it's easy for us to catch it and complain.
It's especially important because it would quietly "work" for many hash
backends (like sha1dc, which is just manipulating some bytes) but would
cause undefined behavior with others (like OpenSSL, which puts the
context onto the heap). Checking the flag lets us catch problems
consistently on every build.
Note that we can't do the same for git_hash_init(). Even though it would
cause a leak to call it twice (without an intervening final/discard),
the point of the function is that the contents of the struct are
undefined before the call. But calling it twice is an even less likely
error to make, so not covering it is OK.
We leave git_hash_discard() alone, as its idempotent behavior is
convenient for callers. We _could_ try to do something similar for
git_hash_final(), allowing:
git_hash_final(result, &ctx);
git_hash_final(other_result, &ctx);
but it does not make much sense. After the first final() call we have
thrown away the state, so we cannot produce the same output. We could
come up with some sensible output (the null hash, or the empty hash),
but double-calls like this are more likely a bug, so our best bet is to
complain loudly (whereas the current code produces either nonsense
output or undefined behavior, depending on the backend).
Signed-off-by: Jeff King <peff@peff.net>
---
hash.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/hash.c b/hash.c
index b1296f0018..82f7e24404 100644
--- a/hash.c
+++ b/hash.c
@@ -290,22 +290,32 @@ void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop)
void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src)
{
+ if (!src->active)
+ BUG("attempt to copy from an inactive hash context");
+ if (!dst->active)
+ BUG("attempt to copy to an inactive hash context");
src->algop->clone_fn(dst, src);
}
void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len)
{
+ if (!ctx->active)
+ BUG("attempt to update an inactive hash context");
ctx->algop->update_fn(ctx, in, len);
}
void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx)
{
+ if (!ctx->active)
+ BUG("attempt to finalize an inactive hash context");
ctx->algop->final_fn(hash, ctx);
ctx->active = false;
}
void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
{
+ if (!ctx->active)
+ BUG("attempt to finalize an inactive hash context");
ctx->algop->final_oid_fn(oid, ctx);
ctx->active = false;
}
--
2.55.0.459.g1b256877c9
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/7] hash: use git_hash_init() consistently
2026-07-07 21:25 ` brian m. carlson
@ 2026-07-08 3:54 ` Jeff King
0 siblings, 0 replies; 35+ messages in thread
From: Jeff King @ 2026-07-08 3:54 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Patrick Steinhardt
On Tue, Jul 07, 2026 at 09:25:48PM +0000, brian m. carlson wrote:
> On 2026-07-07 at 05:01:41, Jeff King wrote:
> > We'd like to add more logic to git_hash_init(), but many callers skip it
> > and call algop->init_fn() directly. Let's make sure we're consistently
> > using the wrapper by adding a coccinelle rule.
> >
> > Besides the coccinelle file itself, this is a purely mechanical
> > conversion based on the patch it generates. There should be no bare
> > init_fn() calls left (except for the one in the wrapper).
>
> For context, the reason `git_hash_init` exists is that our Rust code
> needs to initialize a hash context but it treats `const struct
> git_hash_algo *` as `const void *` and doesn't have any access to the
> contents of the structure. We could fix this with `cbindgen` and
> `bindgen`, but haven't done so yet.
>
> So that's why everybody has been using `init_fn` instead of
> `git_hash_init`. Anyway, I have no objections to making this the
> standard interface going forward.
Thanks, I remember there being some actual reason but couldn't recall
exactly what it was. The use of bare algo->update_fn(), etc, in two
spots was what really puzzled me. It's not wrong, but just harder to
write than the usual way. ;)
-Peff
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/7] git_hash_*() quality-of-life improvements
2026-07-08 3:52 ` [PATCH v2 " Jeff King
` (6 preceding siblings ...)
2026-07-08 3:53 ` [PATCH v2 7/7] hash: check ctx->active flag in all wrapper functions Jeff King
@ 2026-07-08 8:05 ` Patrick Steinhardt
7 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2026-07-08 8:05 UTC (permalink / raw)
To: Jeff King; +Cc: git, brian m. carlson, Junio C Hamano
On Tue, Jul 07, 2026 at 11:52:35PM -0400, Jeff King wrote:
> On Tue, Jul 07, 2026 at 12:55:57AM -0400, Jeff King wrote:
>
> > This implements the "idempotent git_hash_discard()" discussed in this
> > subthread:
> >
> > https://lore.kernel.org/git/20260702080707.GG2029434@coredump.intra.peff.net/
> >
> > with associated cleanups.
>
> Here's a v2 addressing the comments so far. Mostly minor changes:
>
> - fixed typos noticed by Patrick
>
> - dropped extra braces added by coccinelle
>
> - dropped a trailing blank line from patch 1 (this gets fixed in a
> later patch as we add more content after the blank line, but I
> noticed "git apply" complaining)
>
> - a bit more explanation in patch 7 about why we don't support
> idempotent final() calls
All of these changes look good to me, and the range-diff matches what
you describe here. So this series looks good to me, thanks!
Patrick
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2026-07-08 8:06 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-07 4:55 [PATCH 0/7] git_hash_*() quality-of-life improvements Jeff King
2026-07-07 5:01 ` [PATCH 1/7] hash: use git_hash_init() consistently Jeff King
2026-07-07 14:39 ` Junio C Hamano
2026-07-07 20:13 ` Jeff King
2026-07-07 20:17 ` Junio C Hamano
2026-07-07 20:25 ` Jeff King
2026-07-07 21:25 ` brian m. carlson
2026-07-08 3:54 ` Jeff King
2026-07-07 5:04 ` [PATCH 2/7] hash: convert remaining direct function calls Jeff King
2026-07-07 16:15 ` Junio C Hamano
2026-07-07 5:05 ` [PATCH 3/7] hash: document function pointers and wrappers Jeff King
2026-07-07 14:26 ` Patrick Steinhardt
2026-07-07 20:05 ` Jeff King
2026-07-07 5:07 ` [PATCH 4/7] hash: make git_hash_discard() idempotent Jeff King
2026-07-07 16:22 ` Junio C Hamano
2026-07-07 20:18 ` Jeff King
2026-07-07 21:41 ` brian m. carlson
2026-07-07 22:25 ` Junio C Hamano
2026-07-07 5:07 ` [PATCH 5/7] csum-file: use idempotent git_hash_discard() Jeff King
2026-07-07 5:08 ` [PATCH 6/7] http: " Jeff King
2026-07-07 16:25 ` Junio C Hamano
2026-07-07 5:09 ` [PATCH 7/7] hash: check ctx->active flag in all wrapper functions Jeff King
2026-07-07 14:26 ` Patrick Steinhardt
2026-07-07 16:33 ` Junio C Hamano
2026-07-07 20:10 ` Jeff King
2026-07-07 14:26 ` [PATCH 0/7] git_hash_*() quality-of-life improvements Patrick Steinhardt
2026-07-08 3:52 ` [PATCH v2 " Jeff King
2026-07-08 3:52 ` [PATCH v2 1/7] hash: use git_hash_init() consistently Jeff King
2026-07-08 3:52 ` [PATCH v2 2/7] hash: convert remaining direct function calls Jeff King
2026-07-08 3:52 ` [PATCH v2 3/7] hash: document function pointers and wrappers Jeff King
2026-07-08 3:52 ` [PATCH v2 4/7] hash: make git_hash_discard() idempotent Jeff King
2026-07-08 3:53 ` [PATCH v2 5/7] csum-file: use idempotent git_hash_discard() Jeff King
2026-07-08 3:53 ` [PATCH v2 6/7] http: " Jeff King
2026-07-08 3:53 ` [PATCH v2 7/7] hash: check ctx->active flag in all wrapper functions Jeff King
2026-07-08 8:05 ` [PATCH v2 0/7] git_hash_*() quality-of-life improvements Patrick Steinhardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox