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

[Jeff King <peff@peff.net>]

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