[PATCH] grep: simple test for operation in a bare repository

[PATCH] grep: simple test for operation in a bare repository

[René Scharfe]

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 t/t7002-grep.sh |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/t/t7002-grep.sh b/t/t7002-grep.sh
index bf4d4dc..8cf958d 100755
--- a/t/t7002-grep.sh
+++ b/t/t7002-grep.sh
@@ -45,6 +45,23 @@ test_expect_success 'grep should not segfault with a bad input' '
 	test_must_fail git grep "("
 '
 
+bare_repo=.git/bare_test_repo
+test_expect_success 'setup bare repo' '
+	git clone --bare . $bare_repo
+'
+
+test_expect_success "grep HEAD (t-1), bare repo" '(
+	cd $bare_repo &&
+	echo "HEAD:t/t:1:test" >expected &&
+	git grep -n -e test HEAD >actual &&
+	diff expected actual
+)'
+
+test_expect_success "grep (t-1), bare repo, must fail" '(
+	cd $bare_repo &&
+	test_must_fail git grep -n -e test
+)'
+
 for H in HEAD ''
 do
 	case "$H" in
-- 
1.7.0.rc1

Re: [PATCH] grep: simple test for operation in a bare repository

[René Scharfe]

Am 03.02.2010 19:16, schrieb René Scharfe:
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
>  t/t7002-grep.sh |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)

Err, no, that won't do.  Sorry.

The test script fails to demonstrate the issue I've run into.  It runs
successfully, but running git grep manually fails:

	$ cd t/trash\ directory.t7002-grep/.git/bare_test_repo/
	$ git grep bla HEAD
	fatal: This operation must be run in a work tree

I have to dig a bit deeper and try to come back with a better test script.

René

Re: [PATCH] grep: simple test for operation in a bare repository

[René Scharfe]

Am 04.02.2010 00:50, schrieb René Scharfe:
> Am 03.02.2010 19:16, schrieb René Scharfe:
>> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
>> ---
>>  t/t7002-grep.sh |   17 +++++++++++++++++
>>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> Err, no, that won't do.  Sorry.
> 
> The test script fails to demonstrate the issue I've run into.  It runs
> successfully, but running git grep manually fails:
> 
> 	$ cd t/trash\ directory.t7002-grep/.git/bare_test_repo/
> 	$ git grep bla HEAD
> 	fatal: This operation must be run in a work tree
> 
> I have to dig a bit deeper and try to come back with a better test script.

OK, I have to admit defeat: I can't come up with a test script.  But
the issue is reproducible: git grep in a bare repository fails when
run with a pager.

	$ mkdir /tmp/a
	$ cd /tmp/a
	$ git init
	Initialized empty Git repository in /tmp/a/.git/
	$ echo a >a
	$ git add a
	$ git commit -m.
	[master (root-commit) e11f955] .
	 1 files changed, 1 insertions(+), 0 deletions(-)
	 create mode 100644 a

	$ git clone --bare . ../b
	Initialized empty Git repository in /tmp/b/
	$ cd /tmp/b

	$ git grep a HEAD
	fatal: This operation must be run in a work tree
	$ git grep a HEAD | cat
	HEAD:a:a
	$ git --no-pager grep a HEAD
	HEAD:a:a

Reverting 7e622650 (grep: prepare to run outside of a work tree), or
rather just setting the flag RUN_SETUP for grep in git.c again, makes
the first git grep call succeed, too.

As does the following patch, but I don't know why.  The call chain is
quite deep.  It seems that without the patch the static variable
git_dir in environment.c isn't updated when git finds out that it runs
in a bare repo -- but only if a pager is used.

There are five more sites in git.c, path.c and setup.c where $GIT_DIR
is set directly with setenv().  I wonder if they should better call
set_git_dir() instead, too.


diff --git a/setup.c b/setup.c
index 710e2f3..5fb9b25 100644
--- a/setup.c
+++ b/setup.c
@@ -406,7 +406,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				cwd[offset] = '\0';
 				setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
 			} else
-				setenv(GIT_DIR_ENVIRONMENT, ".", 1);
+				set_git_dir(".");
 			check_repository_format_gently(nongit_ok);
 			return NULL;
 		}

Re: [PATCH] grep: simple test for operation in a bare repository

[Nguyen Thai Ngoc Duy]

On Fri, Feb 5, 2010 at 7:24 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> OK, I have to admit defeat: I can't come up with a test script.  But
> the issue is reproducible: git grep in a bare repository fails when
> run with a pager.
>
>        $ mkdir /tmp/a
>        $ cd /tmp/a
>        $ git init
>        Initialized empty Git repository in /tmp/a/.git/
>        $ echo a >a
>        $ git add a
>        $ git commit -m.
>        [master (root-commit) e11f955] .
>         1 files changed, 1 insertions(+), 0 deletions(-)
>         create mode 100644 a
>
>        $ git clone --bare . ../b
>        Initialized empty Git repository in /tmp/b/
>        $ cd /tmp/b
>
>        $ git grep a HEAD
>        fatal: This operation must be run in a work tree
>        $ git grep a HEAD | cat
>        HEAD:a:a
>        $ git --no-pager grep a HEAD
>        HEAD:a:a
>
> Reverting 7e622650 (grep: prepare to run outside of a work tree), or
> rather just setting the flag RUN_SETUP for grep in git.c again, makes
> the first git grep call succeed, too.
>
> As does the following patch, but I don't know why.  The call chain is
> quite deep.  It seems that without the patch the static variable
> git_dir in environment.c isn't updated when git finds out that it runs
> in a bare repo -- but only if a pager is used.

setup_pager() calls git_config(), which indirectly calls get_git_dir()
and sets git_dir in stone. Changing GIT_DIR environment variable alone
won't work, as you have seen.

When RUN_SETUP is set, setup_git_directory() would be called before
setup_pager() can kick in, so everything is properly set.

> There are five more sites in git.c, path.c and setup.c where $GIT_DIR
> is set directly with setenv().  I wonder if they should better call
> set_git_dir() instead, too.

Yes, they should.
-- 
Duy

Re: [PATCH] grep: simple test for operation in a bare repository

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> There are five more sites in git.c, path.c and setup.c where $GIT_DIR
> is set directly with setenv().  I wonder if they should better call
> set_git_dir() instead, too.
>
>
> diff --git a/setup.c b/setup.c
> index 710e2f3..5fb9b25 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -406,7 +406,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  				cwd[offset] = '\0';
>  				setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
>  			} else
> -				setenv(GIT_DIR_ENVIRONMENT, ".", 1);
> +				set_git_dir(".");
>  			check_repository_format_gently(nongit_ok);
>  			return NULL;
>  		}

Yeah, shouldn't the other setenv() we see in the context need a similar
change as well?

Re: [PATCH] grep: simple test for operation in a bare repository

[René Scharfe]

Am 05.02.2010 03:40, schrieb Nguyen Thai Ngoc Duy:
> setup_pager() calls git_config(), which indirectly calls get_git_dir()
> and sets git_dir in stone. Changing GIT_DIR environment variable alone
> won't work, as you have seen.
> 
> When RUN_SETUP is set, setup_git_directory() would be called before
> setup_pager() can kick in, so everything is properly set.
> 
>> There are five more sites in git.c, path.c and setup.c where $GIT_DIR
>> is set directly with setenv().  I wonder if they should better call
>> set_git_dir() instead, too.
> 
> Yes, they should.

This patch converts the setenv() calls in path.c and setup.c.  After
the call, git grep with a pager works again in bare repos.

It leaves the setenv(GIT_DIR_ENVIRONMENT, ...) calls in git.c alone, as
they respond to command line switches that emulate the effect of setting
the environment variable directly.

The remaining site in environment.c is in set_git_dir() and is left
alone, too, of course.  Finally, builtin-init-db.c is left changed
because the repo is still being carefully constructed when the
environment variable is set.

This fixes git shortlog when run inside a git directory, which had been
broken by abe549e1.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Since it's doesn't fix a regression (abe549e1 was committed in March
2008), this patch doesn't have to go in at this point in the release
cycle.  And perhaps it's even superseded by the more general fix Duy
is working on?

 path.c  |    2 +-
 setup.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/path.c b/path.c
index 79aa104..0005df3 100644
--- a/path.c
+++ b/path.c
@@ -336,7 +336,7 @@ char *enter_repo(char *path, int strict)
  	if (access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
 	    validate_headref("HEAD") == 0) {
-		setenv(GIT_DIR_ENVIRONMENT, ".", 1);
+		set_git_dir(".");
 		check_repository_format();
 		return path;
 	}
diff --git a/setup.c b/setup.c
index 710e2f3..b38cbee 100644
--- a/setup.c
+++ b/setup.c
@@ -404,9 +404,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				inside_work_tree = 0;
 			if (offset != len) {
 				cwd[offset] = '\0';
-				setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
+				set_git_dir(cwd);
 			} else
-				setenv(GIT_DIR_ENVIRONMENT, ".", 1);
+				set_git_dir(".");
 			check_repository_format_gently(nongit_ok);
 			return NULL;
 		}

Re: [PATCH] grep: simple test for operation in a bare repository

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> This patch converts the setenv() calls in path.c and setup.c.  After
> the call, git grep with a pager works again in bare repos.
>
> It leaves the setenv(GIT_DIR_ENVIRONMENT, ...) calls in git.c alone, as
> they respond to command line switches that emulate the effect of setting
> the environment variable directly.
>
> The remaining site in environment.c is in set_git_dir() and is left
> alone, too, of course.  Finally, builtin-init-db.c is left changed
> because the repo is still being carefully constructed when the
> environment variable is set.

Thanks for a thorough analysis.  The patch looks correct and I am very
tempted to put it into 1.7.0, though I'd stop at queuing it to 'next'.