On Thu, Oct 21, 2021 at 09:37:07AM -0700, Junio C Hamano wrote: > Taylor Blau <me@xxxxxxxxxxxx> writes: > > > `git repack` invokes a handful of child processes: one to write the > > actual pack, and optionally ones to repack promisor objects and update > > the MIDX. > > > > In none of these cases do we bother to call child_process_clear(), which > > frees the memory associated with each child's arguments and environment. > > > > In order to do so, tweak each function that spawns a child process to > > have a `cleanup` label that we always visit before returning from each > > function. Then, make sure that we call child_process_clear() as a part > > of that label. > > I have a slight aversion against the fact that this patch makes us > call child_process_clear(), when we know we may have called > finish_command(). Callers of finish_command() in other codepaths > rely on the fact that finish_command() calls child_process_clear(), > which means that because of this change, we are now constrained to > keep child_process_clear() idempotent? Good point. In functions besides cmd_repack(), it's easy enough to just call child_process_clear() once before returning instead of using a label. That has the added benefit of side-stepping the variable name question that you and Stolee raised (but I think whether we call it "result", "res", or "ret" is besides the point). In cmd_repack(), I'd like to wrap the other cleanup-related calls underneath a 'cleanup' label. So there we want to track whether or not we have called finish_command(), so that we only call child_process_clear() when we *haven't* already called it via finish_command(). Thanks, Taylor
