Re: [PATCH 4/7] hash: make git_hash_discard() idempotent
From: Junio C Hamano <hidden>
Date: 2026-07-07 16:22:06
Jeff King [off-list ref] 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.
quoted hunk ↗ jump to hunk
Signed-off-by: Jeff King <redacted> --- 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);