Re: [PATCH 7/9] http: discard hash in dumb-http http_object_request

[Patrick Steinhardt <ps@pks.im>]

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:
> > 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