Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > Agreed. The current usage in worktree.c is a bit too familiar with the > current internal implementation of run_command(). Reinitializing the > child_process struct or using a separate one would be a good cleanup. > >> -- >8 -- >> Subject: [PATCH] worktree: fix leak in check_clean_worktree() >> >> We allocate a child_env strvec but never free its memory. Instead, let's >> just use the strvec that our child_process struct provides, which is >> cleaned up automatically when we run the command. >> >> And while we're moving the initialization of the child_process around, >> let's switch it to use the official init function (zero-initializing it >> works OK, since strvec is happy enough with that, but it sets a bad >> example). > > The various memset()'s in worktree.c seem to have been inherited (and > multiplied) from Duy's original "git checkout --to" implementation > (which later became the basis for "git worktree add" after which it > mutated significantly), and "git checkout --to" predates introduction > of child_process_init(). > >> diff --git a/builtin/worktree.c b/builtin/worktree.c >> @@ -924,7 +924,6 @@ static int move_worktree(int ac, const char **av, const char *prefix) >> - struct strvec child_env = STRVEC_INIT; >> @@ -935,15 +934,14 @@ static void check_clean_worktree(struct worktree *wt, >> - strvec_pushf(&child_env, "%s=%s/.git", >> + child_process_init(&cp); >> + strvec_pushf(&cp.env_array, "%s=%s/.git", >> GIT_DIR_ENVIRONMENT, wt->path); >> - strvec_pushf(&child_env, "%s=%s", >> + strvec_pushf(&cp.env_array, "%s=%s", >> GIT_WORK_TREE_ENVIRONMENT, wt->path); >> - memset(&cp, 0, sizeof(cp)); >> - cp.env = child_env.v; > > Looks good to me. For what it's worth: > > Reviewed-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx> Thanks, both. This looks good.
