Re: [PATCH v2 2/9] setup: introduce startup_info->original_cwd
- From
Elijah Newren <newren@gmail.com>
- Date
- Nov 26, 2021, 18:01 UTC
- Message-ID
- <CABPp-BG8narMYNeaBHQfhcOFJ4hHJM+HGoy7S4hyOA-7PbOEPA@mail.gmail.com>
- In-Reply-To
- <xmqqo8678l3x.fsf@gitster.g>
On Thu, Nov 25, 2021 at 10:52 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > diff --git a/git.c b/git.c > > index 5ff21be21f3..2c98ab48936 100644 > > --- a/git.c > > +++ b/git.c > > @@ -866,6 +866,8 @@ int cmd_main(int argc, const char **argv) > > > > trace_command_performance(argv); > > > > + startup_info->original_cwd = xgetcwd(); > > We assume that, unless we are an auxiliary tool like git-remote-http, > we should always have cwd?
Yeah, Ævar caught this mistake too; I should have used strbuf_getcwd(). Will fix.
Show 18 quoted lines
> > diff --git a/setup.c b/setup.c
> > index 347d7181ae9..f30657723ea 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -432,6 +432,54 @@ void setup_work_tree(void)
> > initialized = 1;
> > }
> >
> > +static void setup_original_cwd(void)
> > +{
> > + struct strbuf tmp = STRBUF_INIT;
> > + const char *worktree = NULL;
> > + int offset = -1;
> > +
> > + /*
> > + * startup_info->original_cwd wass set early on in cmd_main(), unless
>
> s/wass/was/;Thanks.
Show 42 quoted lines
> > + * we're an auxiliary tool like git-remote-http or test-tool.
> > + */
> > + if (!startup_info->original_cwd)
> > + return;
> > +
> > + /*
> > + * startup_info->original_cwd points to the current working
> > + * directory we inherited from our parent process, which is a
> > + * directory we want to avoid incidentally removing.
> > + *
> > + * For convience, we would like to have the path relative to the
> > + * worktree instead of an absolute path.
> > + *
> > + * Yes, startup_info->original_cwd is usually the same as 'prefix',
> > + * but differs in two ways:
> > + * - prefix has a trailing '/'
> > + * - if the user passes '-C' to git, that modifies the prefix but
> > + * not startup_info->original_cwd.
> > + */
> > +
> > + /* Normalize the directory */
> > + strbuf_realpath(&tmp, startup_info->original_cwd, 1);
> > + free((char*)startup_info->original_cwd);
> > + startup_info->original_cwd = strbuf_detach(&tmp, NULL);
> > +
> > + /* Find out if this is in the worktree */
> > + worktree = get_git_work_tree();
> > + if (worktree)
> > + offset = dir_inside_of(startup_info->original_cwd, worktree);
> > + if (offset >= 0) {
> > + /*
> > + * original_cwd was inside worktree; precompose it just as
> > + * we do prefix so that built up paths will match
> > + */
> > + startup_info->original_cwd = \
> > + precompose_string_if_needed(startup_info->original_cwd
> > + + offset);
> > + }
>
> I wonder if we want to clear the .original_cwd member, so that the
> "cwd protection" do not have to worry about anything at all when we
> are in a bare repository.Yeah, I wondered about this a bit and whether it'd even matter either way if the original_cwd was something outside the working tree. I can obviously come up with scenarios, but can't see protecting or not protecting such directories as mattering; if the user is in a directory where git internals are stored and deleted, well, they're already off-roading.
But, since you bring it up, I might as well scope this to just protecting a directory within the working tree. Perhaps that'd make Ævar more comfortable with the change too. I'll do that.