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

On Tue, Jul 07, 2026 at 09:22:04AM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

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