Subject: Re: [PATCH v8 24/37] receive-pack: convert push-to-checkout hook to hook.h

From: Junio C Hamano <[email-masked]>
Date: Mon, 29 Mar 2021 17:10:48 -0700

Emily Shaffer <[email-masked]> writes:

>> OK, we opt_init(), futz with opt, call run_hooks() and opt_clear()
>> regardless of the outcome from run_hooks().  Narrow-sighted me
>> wonders if it makes the use of the API easier if run_hooks() did the
>> opt_clear() before it returns, but I haven't yet seen enough use at
>> this point to judge.
>
> Hrm, is that idiomatic? I guess it would be convenient, and as long as
> it doesn't touch explicitly caller-managed context pointer it should be
> safe, but wouldn't it be surprising?

The precedent (at this point, I will not judge if it is a good
pattern to emulate or an anti-pattern to stay away from) I had in
mind was the run_command() which clears child_process structure
as the side effect of internally calling finish_command().

Leaving them separate is of course more flexible, but depending on
how small we can keep down the number of call patterns of this new
API, always having to clear after run might become an unnecessary
source of leaks.  When I gave that comment, I didn't have enough
input to decide, and now it has been so long since I gave my
reviews, I do not quite remember what my impression after reading
all the patches through was X-<.