Re: [PATCH] setup: do not change to work tree prematurely

From: Clemens Buchacher <drizzd@aon.at>
Date: May 23, 2010, 09:14 UTC
Message-ID: <20100523091442.GA8128@localhost>
In-Reply-To: <20100523013539.GA14403@progeny.tock>

On Sat, May 22, 2010 at 08:35:39PM -0500, Jonathan Nieder wrote:

Show 12 quoted lines

> Without your patch applied, I get
> 
>  $ cd .git
>  $ git --work-tree=/tmp/git symbolic-ref HEAD
>  refs/heads/hello
> 
> I should have done the following instead:
> 
>  $ worktree=$(pwd)
>  $ cd .git
>  $ git --work-tree="$worktree" symbolic-ref HEAD
>  fatal: ref HEAD is not a symbolic ref

Thanks

> > +char *get_relative_path(char *cwd, const char *dir)
> 
> Looks reasonable.  Should be const char *cwd.

That is not possible, because we return a pointer to cwd plus an offset, which will be subsequently modified by strcat().

Show 15 quoted lines

> > diff --git a/setup.c b/setup.c
> > index 5716d90..67b5122 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -524,14 +524,18 @@ const char *setup_git_directory(void)
> > 	const char *retval = setup_git_directory_gently(NULL);
> >  
> >  	/* If the work tree is not the default one, recompute prefix */
> >  	if (inside_work_tree < 0) {
> 
> If I understand correctly, you made two changes here:
> 
>  - interpret GIT_WORK_TREE and friends relative to .git instead of
>    the original cwd (by not calling chdir(retval) before
>    get_git_work_tree())

Oops

>  - make setup_git_directory stay in the last directory searched for a
>    .git directory instead of chdir-ing into the toplevel of the work tree.
>
> Are these safe changes to make?

Indeed, if you put it that way, this change does not look good. Maybe I'll have a look at Nguyen's patches.

> diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
> index 9df3012..93bf92c 100755
> --- a/t/t1501-worktree.sh
> +++ b/t/t1501-worktree.sh

Thanks