[PATCH] rehabilitate 'git index-pack' inside the object store

[PATCH] rehabilitate 'git index-pack' inside the object store

[Nicolas Pitre]

Before commit d0b92a3f6e it was possible to run 'git index-pack'
directly in the .git/objects/pack/ directory.  Restore that ability.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---

diff --git a/index-pack.c b/index-pack.c
index 0a917d7..79f6fd6 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -878,10 +877,26 @@ int main(int argc, char **argv)
 	char *index_name_buf = NULL, *keep_name_buf = NULL;
 	struct pack_idx_entry **idx_objects;
 	unsigned char pack_sha1[20];
-	int nongit = 0;
 
-	setup_git_directory_gently(&nongit);
-	git_config(git_index_pack_config, NULL);
+	/*
+	 * We wish to read the repository's config file if any, and
+	 * for that it is necessary to call setup_git_directory_gently().
+	 * However if the cwd was inside .git/objects/pack/ then we need
+	 * to go back there or all the pack name arguments will be wrong.
+	 * And in that case we cannot rely on any prefix returned by 
+	 * setup_git_directory_gently() either.
+	 */
+	{
+		char cwd[PATH_MAX+1];
+		int nongit;
+
+		if (!getcwd(cwd, sizeof(cwd)-1))
+			die("Unable to get current working directory");
+		setup_git_directory_gently(&nongit);
+		git_config(git_index_pack_config, NULL);
+		if (chdir(cwd))
+			die("Cannot come back to cwd");
+	}
 
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];

[PATCH] (squash) add index-pack with git-dir test

[Junio C Hamano]

This tries to make sure that the recent fix to d0b92a3 (index-pack: setup
git repository, 2008-08-26) does not introduce regression, and protects
the fix from getting broken again.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * And unfortunately the first one fails with the fix and succeeds
   without; the second one succeeds with your fix and fails without.

 t/t5302-pack-index.sh |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index 6424db1..2edab73 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -177,4 +177,24 @@ test_expect_success \
        ".git/objects/pack/pack-${pack1}.pack" 2>&1) &&
      echo "$err" | grep "CRC mismatch"'
 
+test_expect_success 'running index-pack from a subdirectory' '
+    mkdir subdir &&
+    cp test-1-${pack1}.pack subdir/. &&
+    (
+	cd subdir &&
+	git index-pack test-1-${pack1}.pack
+    ) &&
+    test -f subdir/test-1-${pack1}.idx
+'
+
+test_expect_success 'running index-pack in the object store' '
+    rm -f .git/objects/pack/* &&
+    cp test-1-${pack1}.pack .git/objects/pack/pack-${pack1}.pack &&
+    (
+	cd .git/objects/pack
+	git index-pack pack-${pack1}.pack
+    ) &&
+    test -f .git/objects/pack/pack-${pack1}.idx
+'
+
 test_done
-- 
1.6.0.2.588.g31023

Re: [PATCH] rehabilitate 'git index-pack' inside the object store

[Nguyen Thai Ngoc Duy]

On 10/21/08, Nicolas Pitre <nico@cam.org> wrote:
> Before commit d0b92a3f6e it was possible to run 'git index-pack'
>  directly in the .git/objects/pack/ directory.  Restore that ability.

I am sorry I did not catch this in the first place. While the fix
should be fine for "git index-pack". I wonder what can happen for
other setup_*_gently()'s consumers. Other commands may be affected too
(e.g. running "git apply" or "git bundle" from inside .git). Maybe we
should let setup_*_gently() do read-only stuff only and let its
consumers to handle cwd. I recall Jeff has plan about worktree setup
rework, though could not find the thread. Will think more of it
tomorrow.
-- 
Duy

Re: [PATCH] rehabilitate 'git index-pack' inside the object store

[Jeff King]

On Tue, Oct 21, 2008 at 09:57:04PM +0700, Nguyen Thai Ngoc Duy wrote:

> (e.g. running "git apply" or "git bundle" from inside .git). Maybe we
> should let setup_*_gently() do read-only stuff only and let its
> consumers to handle cwd. I recall Jeff has plan about worktree setup
> rework, though could not find the thread. Will think more of it
> tomorrow.

If by "plan" you mean "intense desire to see it fixed", then yes, I have
a plan. But there isn't a concrete plan yet, and I'm not sure when I'll
get to it. I have a vague assumption that we should move in the
direction of just setting up as much of the environment as possible
(finding git dir, finding work tree, reading config, etc) as soon as
possible, but not producing any error messages or dying as a result.
Then those commands which want to enforce that some setup has occurred
can easily do so.

-Peff

Re: [PATCH] rehabilitate 'git index-pack' inside the object store

[Nicolas Pitre]

On Tue, 21 Oct 2008, Nguyen Thai Ngoc Duy wrote:

> On 10/21/08, Nicolas Pitre <nico@cam.org> wrote:
> > Before commit d0b92a3f6e it was possible to run 'git index-pack'
> >  directly in the .git/objects/pack/ directory.  Restore that ability.
> 
> I am sorry I did not catch this in the first place. While the fix
> should be fine for "git index-pack". I wonder what can happen for
> other setup_*_gently()'s consumers. Other commands may be affected too
> (e.g. running "git apply" or "git bundle" from inside .git).

Normally you should not be running such commands inside the .git 
directory.  The index-pack command is a bit special in that regard.


Nicolas

Re: [PATCH] rehabilitate 'git index-pack' inside the object store

[Johannes Schindelin]

Hi,

On Tue, 21 Oct 2008, Nguyen Thai Ngoc Duy wrote:

> Maybe we should let setup_*_gently() do read-only stuff only and let its 
> consumers to handle cwd.

I think that makes sense.  This would allow setup*gently() to be much 
cleaner, and only setup_git_directory() itself would do the 
chdir(worktree).

However, let's think this really through this time, so that that darned 
worktree stuff is fixed for good.

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.

Ciao,
Dscho

Re: [PATCH] rehabilitate 'git index-pack' inside the object store

[Jeff King]

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.

I think you will need to do something for a few commands which use
_gently() but then, if we _are_ in a git repo, assume we are chdir'd
properly.

We may also be able to just call setup_git_directory_gently() as the
first thing in the wrapper, which should help us more consistently find
config before the 'exec' stage (see 4e10738a for a discussion of some of
the issues).

-Peff

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

[Johannes Schindelin]

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 think you will need to do something for a few commands which use 
> _gently() but then, if we _are_ in a git repo, assume we are chdir'd 
> properly.

Right.

> We may also be able to just call setup_git_directory_gently() as the 
> first thing in the wrapper, which should help us more consistently find 
> config before the 'exec' stage (see 4e10738a for a discussion of some of 
> the issues).

Yep, that's what I referred to as "pager" problems.  Thanks for the 
pointer!  For other people's convenience I quote it here :-)

-- snip --
    Allow per-command pager config

    There is great debate over whether some commands should set
    up a pager automatically. This patch allows individuals to
    set their own pager preferences for each command, overriding
    the default. For example, to disable the pager for git
    status:

      git config pager.status false

    If "--pager" or "--no-pager" is specified on the command
    line, it takes precedence over the config option.

    There are two caveats:

      - you can turn on the pager for plumbing commands.
        Combined with "core.pager = always", this will probably
        break a lot of things. Don't do it.

      - This only works for builtin commands. The reason is
        somewhat complex:

        Calling git_config before we do setup_git_directory
        has bad side effects, because it wants to know where
        the git_dir is to find ".git/config". Unfortunately,
        we cannot call setup_git_directory indiscriminately,
        because some builtins (like "init") break if we do.

        For builtins, this is OK, since we can just wait until
        after we call setup_git_directory. But for aliases, we
        don't know until we expand (recursively) which command
        we're doing. This should not be a huge problem for
        aliases, which can simply use "--pager" or "--no-pager"
        in the alias as appropriate.

        For external commands, however, we don't know we even
        have an external command until we exec it, and by then
        it is too late to check the config.

        An alternative approach would be to have a config mode
        where we don't bother looking at .git/config, but only
        at the user and system config files. This would make the
        behavior consistent across builtins, aliases, and
        external commands, at the cost of not allowing per-repo
        pager config for at all.
-- snap --

Ciao,
Dscho

Re: [PATCH] rehabilitate 'git index-pack' inside the object store

[Nguyen Thai Ngoc Duy]

On 10/22/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

One more thing: "git foo -h" with RUN_SETUP won't run if repository is
not found. Maybe just drop RUN_SETUP and let subcommands call
setup_git_directory()
-- 
Duy

Re: [PATCH] rehabilitate 'git index-pack' inside the object store

[Nguyen Thai Ngoc Duy]

On 10/22/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Also, from [1] may be only set default git_dir in setup_git_env()
after setup_git_directory() has been called and before
setup_work_tree() is called (if any)? If not, die().

[1] http://article.gmane.org/gmane.comp.version-control.git/98849
-- 
Duy

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

[Nguyen Thai Ngoc Duy]

On 10/22/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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