From: Duy Nguyen Date: Wed, 23 Jul 2014 08:02:03 GMT Subject: Re: [PATCH v7 23/31] checkout: clean up half-prepared directories in --to mode Message-ID: In-Reply-To: On Mon, Jul 21, 2014 at 6:55 AM, Eric Sunshine wrote: >> @@ -848,13 +878,21 @@ static int prepare_linked_checkout(const struct checkout_opts *opts, >> strbuf_addf(&sb_repo, "%d", counter); >> } >> name = strrchr(sb_repo.buf, '/') + 1; >> + >> + junk_pid = getpid(); >> + atexit(remove_junk); >> + sigchain_push_common(remove_junk_on_signal); >> + >> if (mkdir(sb_repo.buf, 0777)) >> die_errno(_("could not create directory of '%s'"), sb_repo.buf); >> + junk_git_dir = sb_repo.buf; > > I've managed to convince myself that, although junk_git_dir becomes a > dangling pointer by the end of prepare_linked_checkout(), it should > never afterward be accessed. Perhaps it would make sense to make this > easier to follow by clearing junk_git_dir when is_junk is cleared? You're right. And the dangling junk_git_dir can be accessed if "ret" below is non-zero. Will strdup() both junk_*_dir and free them in "if (!ret)" block. Thanks for catching. >> @@ -879,7 +917,14 @@ static int prepare_linked_checkout(const struct checkout_opts *opts, >> memset(&cp, 0, sizeof(cp)); >> cp.git_cmd = 1; >> cp.argv = opts->saved_argv; >> - return run_command(&cp); >> + ret = run_command(&cp); >> + if (!ret) >> + is_junk = 0; > > Here: perhaps also set is_junk_dir to NULL since it otherwise is about > to become a dangling pointer. > >> + strbuf_release(&sb); >> + strbuf_release(&sb_repo); >> + strbuf_release(&sb_git); >> + return ret; -- Duy