On Sun, Jul 05, 2026 at 08:01:05PM -0400, Jeff King wrote:

On Fri, Jul 03, 2026 at 01:27:36PM +0200, Patrick Steinhardt wrote:

quoted

On Thu, Jul 02, 2026 at 04:07:07AM -0400, Jeff King wrote:

[snip]

The second issue is related: what should we do in other functions when
the active flag is not set? For example, what should this do:

  algo->init_fn(&ctx);

  git_hash_update(&ctx, ...);
  git_hash_final(out, &ctx);

  git_hash_update(&ctx, ...);
  git_hash_final(out, &ctx);

In the second git_hash_update() call, there are two obvious options:

  1. It should do nothing; there is no active context to add to.

  2. It should automatically re-init the context (using the algo from
     the previous init) and add the data.

Or 3rd: we `BUG()` when any of the functions is called on an
uninitialized context. That to me feels like the most sensible solution.

The second final() call has the added bonus that it returns data, but I
think there are two matching options:

  1. It should do nothing, and hashclr() the output (leaving it
     uninitialized just seems insane).

  2. It should automatically re-init the context (assuming there was not
     already an update() call that did so). And then I guess return
     whatever hash that particular algo generates for the empty string?

Those all seem reasonable-ish to me and give a defined output at every
moment (which is better than crashing). But it kind of feels like they'd
be papering over potential bugs. Maybe crashing _is_ better (we don't do
so reliably now, but a BUG() could make sense).

Yes, agreed.

And the third is related: do we check the active flag when initializing?
Right now the answer must be "no", because the point of the init
function is that the input is potentially garbage. But that means
something like:

  struct git_hash_ctx ctx;
  algo->init_fn(&ctx);
  algo->init_fn(&ctx);

leaks. That's maybe OK in practice. We could do something more like:

  struct git_hash_ctx = HASH_CTX_INIT;
  git_hash_start(&ctx, algo);

where the INIT step doesn't actually allocate anything, and start() is
the moment where you must promise to call final() or discard(). And then
it would be OK for start() to BUG() when the active flag is already set.

I'd say being as strict as possible is the best way to go until we find
a case where it makes sense to be less strict.

That was maybe more than you wanted to read about the topic. But if the
request is for safer object lifetimes in general, then I think there are
a lot of details about what that means.

If we are going to do anything, I'd be inclined to stop mostly after the
diff I showed above. That's the only thing I've seen that would simplify
existing code. The rest are mostly hypotheticals, but since Rust was
mentioned, I wondered if you're trying to shoot for something safer.

At any rate, I would prefer to do any of this on top of the series I
posted. I took care there to avoid double-calling final()/discard(),
which could now be simplified away. But I think I'd rather see that
simplification its own step.

Fully agreed.

Thanks!

Patrick