Re: [PATCH v8 23/37] read-cache: convert post-index-change hook to use config

> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > @@ -3070,6 +3071,8 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
> >  				 unsigned flags)
> >  {
> >  	int ret;
> > +	struct run_hooks_opt hook_opt;
> > +	run_hooks_opt_init_async(&hook_opt);
> >  
> 
> Nit. blank line between the last of decls and the first stmt (many
> identical nits exist everywhere in this series).
> 
> >  	/*
> >  	 * TODO trace2: replace "the_repository" with the actual repo instance
> > @@ -3088,9 +3091,13 @@ static int do_write_locked_index(s
> >  	else
> >  		ret = close_lock_file_gently(lock);
> >  
> > -	run_hook_le(NULL, "post-index-change",
> > -			istate->updated_workdir ? "1" : "0",
> > -			istate->updated_skipworktree ? "1" : "0", NULL);
> > +	strvec_pushl(&hook_opt.args,
> > +		     istate->updated_workdir ? "1" : "0",
> > +		     istate->updated_skipworktree ? "1" : "0",
> > +		     NULL);
> > +	run_hooks("post-index-change", &hook_opt);
> > +	run_hooks_opt_clear(&hook_opt);
> 
> There is one early return before the precontext of this hunk that
> bypasses this opt_clear() call.  It is before any member of hook_opt
> structure that was opt_init()'ed gets touched, so with the current
> code, there is no leak, but it probably is laying a landmine for the
> future, where opt_init() may allocate some resource to its member,
> with the expectation that all users of the API would call
> opt_clear() to release.  Or the caller of the API (like this one) may
> start mucking with the opt structure before the existing early return,
> at which point the current assumption that it is safe to return from
> that point without opt_clear() would be broken.
> 
> I saw that there are other early returns in the series that are safe
> right now but may become unsafe when the API implementation gets
> extended that way.  If it does not involve too much code churning,
> we may want to restructure the code to make these early returns into
> "goto"s that jump to a single exit point, so that we can always
> match opt_init() with opt_clear(), like the structure of the
> existing code allowed cmd_rebase() to use the hooks API cleanly in
> [v8 22/37].