On 10/22/08, Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote:
> Hi,
>
> On Tue, 21 Oct 2008, Jeff King wrote:
>
> > On Tue, Oct 21, 2008 at 07:02:48PM +0200, Johannes Schindelin wrote:
> >
> > > So I propose this change in semantics:
> > >
> > > - setup_git_directory_gently(): rename to discover_git_directory(),
> > > and avoid any chdir() at all.
> > > - setup_git_directory(): keep the semantics that it chdir()s to the
> > > worktree, or to the git directory for bare repositories.
> > >
> > > Using _gently() even for RUN_SETUP builtins should solve the long
> > > standing pager problem, too.
> >
> > I'm not sure there aren't hidden problems lurking in that strategy
> > (every time I look at this area of code, something unexpected prevents
> > what I think should Just Work from Just Working), but I think that is a
> > promising direction to go for clearing up some of the long-standing
> > issues.
>
> Same here. I grew a pretty strong opinion about the whole worktree thing,
> but maybe that is only because it was done trying to change as little as
> possible.
I played a bit with code, extracted discover_git_directory() from
setup_git_directory_gently() then made the latter a wrapper of the
former with chdir(). Some more for thoughts from the experiment.
1. Because discover_git_directory() does not do chdir() until later in
setup_git_directory_gently(), setting GIT_DIR to a relative path seems
unsafe (or worse unset at all, in case .git is found in parent
directories). But making GIT_DIR absolute path breaks tests because
some of them expect "git rev-parse --git-dir" to return a relative
path. The approach used in 044bbbc (Make git_dir a path relative to
work_tree in setup_work_tree()) can be reused to performance loss, but
that won't solve the issue.
2. 044bbbc also brings up another issue: code duplication between
setup_git_directory_gently() and setup_work_tree(). The new
setup*gently() can be roughly like this if setup_work_tree() can
calculate prefix too:
const char *setup_git_directory_gently(int *nongit_ok)
{
int nonworktree_ok;
/*
* Let's assume that we are in a git repository.
* If it turns out later that we are somewhere else, the value will be
* updated accordingly.
*/
if (nongit_ok)
*nongit_ok = 0;
if (!discover_git_directory()) {
if (nongit_ok) {
*nongit_ok = 1;
return NULL;
}
die("Not a git repository");
}
return setup_work_tree_gently(&nonworktree_ok); // gentle version
}
So I propose to make setup_work_tree() return a prefix, relative to
current cwd. The setup procedure then would become:
if (discover_git_directory())
die("Git repository needed");
prefix = setup_work_tree(); // die() inside if cannot setup worktree
3. Dealing with cwd outside worktree. If cwd is inside a worktree,
prefix will be calculated correctly. If it is outside, the current
behavior (with both GIT_DIR and GIT_WORK_TREE set) is leave prefix as
NULL. I think that is not right. With a wrong prefix, git commands
will not be able to access on-disk files. I would propose either:
- die() if cwd is outside worktree
- setup*gently() discovers the situation and gives up, then lets git
commands handle themselves. Some commands, like git-archive, don't
care about on-disk files at all, they could just simply ignore the
prefix and keep going. Others may die() or handle it properly.
Again, this breaks things.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
