Re: new plan for cleaning up the worktree mess, was Re: [PATCH] rehabilitate 'git index-pack' inside the object store

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Oct 28, 2008, 13:52 UTC
Message-ID: <fcaeb9bf0810280652x7f883e35h157f94144263c5fa@mail.gmail.com>
In-Reply-To: <alpine.DEB.1.00.0810211955250.22125@pacific.mpi-cbg.de.mpi-cbg.de>

On 10/22/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 25 quoted lines

> 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