Re: [PATCH 4/7] hash: make git_hash_discard() idempotent

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

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);