[PATCH] t7002: test git grep --no-index from a bare repository

[PATCH] t7002: test git grep --no-index from a bare repository

[Nguyễn Thái Ngọc Duy]

From: Jonathan Nieder <jrnieder@gmail.com>

If git grep --no-index is to be a general-purpose replacement for
standard grep, then it should work even for examining the content of a
.git directory.  The current implementation has some problems:

 * .git/info/exclude is honored though it shouldn’t be
 * when run from within a .git directory, grep --no-index searches
   the entire .git directory instead of just directories below
   the current working directory.
 * when run from within a .git directory, prefix will always
   be NULL, so run_pager() will not be able to move back to
   the original cwd.

The last few tests (which demonstrate that grep --no-index looks for
.git/info/exclude even when no repository is involved) are by Duy.
Any bugs in the other tests are my fault.

Also clarify in document what files will be searched in.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 There is an interesting thing about this command. Back in tp/setup
 series, there is a patch that changes the current behavior,
 "calculate prefix even if no worktree is found". grep is interesting
 because it depends on the current behavior, i.e. prefix being NULL
 in bare repo, while it still needs true prefix to do chdir()
 stuff in run_pager().

 So I have a feeling that my patch is the right way to go. But
 I really need to watch all other commands and make sure they don't
 depend on this behavior like grep.

 Documentation/git-grep.txt |    5 +-
 builtin/grep.c             |    2 +

These are my additional changes.

 t/t7810-grep.sh            |  116 ++++++++++++++++++++++++++++++++++++++++++++

This one remains the same from tp/setup.

 3 files changed, 121 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 5474dd7..bb420d9 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -28,8 +28,9 @@ SYNOPSIS
 DESCRIPTION
 -----------
 Look for specified patterns in the tracked files in the work tree, blobs
-registered in the index file, or blobs in given tree objects.
-
+registered in the index file, or blobs in given tree objects. By default
+it will only search tracked files within the current directory (or full
+tree if in bare repository).
 
 OPTIONS
 -------
diff --git a/builtin/grep.c b/builtin/grep.c
index 597f76b..e8abdc7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1109,6 +1109,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 	if (use_threads)
 		hit |= wait_all();
+
+	/* FIXME prefix is NULL in bare repo, no matter where cwd is */
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
 	free_grep_patterns(&opt);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 8a63227..7329433 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -494,6 +494,98 @@ test_expect_success 'inside git repository but with --no-index' '
 	)
 '
 
+test_expect_success 'set up bare repository' '
+
+	rm -fr bare &&
+	mkdir -p bare/basis/sub &&
+	echo hello >bare/basis/file1 &&
+	echo world >bare/basis/sub/file2 &&
+	echo ".*o*" >bare/basis/.gitignore &&
+	(
+		cd bare/basis &&
+		git init &&
+		git ls-files --other | git update-index --add --stdin &&
+		tree=$(git write-tree) &&
+		commit=$(printf "basis\n" | git commit-tree "$tree") &&
+		git update-ref -m init refs/heads/master "$commit" &&
+		git update-index --refresh &&
+		git diff-index --exit-code HEAD
+	) &&
+	git clone --no-checkout --bare bare/basis bare/d.git &&
+	mkdir -p bare/d.git/sub &&
+	echo olleh >bare/d.git/fich1 &&
+	echo dlrow >bare/d.git/sub/file2 &&
+	echo "file2" >bare/d.git/.gitignore &&
+	echo "o*" >>bare/d.git/.gitignore
+	{
+		echo "HEAD:.gitignore:.*o*" &&
+		echo HEAD:file1:hello &&
+		echo HEAD:sub/file2:world
+	} >bare/expect.full &&
+	: >bare/expect.empty &&
+	echo file2:dlrow >bare/expect.sub
+'
+
+test_expect_success 'in bare repo, grep without --no-index or --cached fails' '
+	(
+		cd bare/d.git &&
+		test_must_fail git grep o >../actual.plain 2>../actual.msg &&
+		grep "work tree" ../actual.msg &&
+		test_cmp ../expect.empty ../actual.plain &&
+		cd sub &&
+		test_must_fail git grep o >../../actual.sub 2>../../actual.msg &&
+		test_cmp ../../expect.empty ../../actual.sub &&
+		grep "work tree" ../../actual.msg
+	)
+'
+
+test_expect_success 'in bare repo, --cached and HEAD ignore working dir' '
+
+	(
+		cd bare/d.git &&
+		test_must_fail git grep --cached o >../actual.cached 2>../actual.msg &&
+		test_cmp ../expect.empty ../actual.cached &&
+		! grep fatal ../actual.msg &&
+		git grep -e o HEAD >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+		cd sub &&
+		test_must_fail git grep o >../../actual.sub 2>../../actual.msg &&
+		test_cmp ../../expect.empty ../../actual.sub &&
+		git grep -e o HEAD >../../actual.full-sub &&
+		test_cmp ../../expect.full ../../actual.full-sub
+	)
+'
+
+test_expect_success '--no-index from a bare repository' '
+	rm -f bare/d.git/info/exclude &&
+	(
+		cd bare/d.git &&
+		git grep --no-index o >../actual.noindex &&
+		grep "^fich1:olleh\$" ../actual.noindex &&
+		grep "^.gitignore:o[*]\$" ../actual.noindex &&
+		! grep file2 ../actual.noindex
+	)
+'
+
+test_expect_failure '--no-index from a subdirectory of a bare repository' '
+	(
+		cd bare/d.git/sub &&
+		git grep --no-index o >../../actual.sub &&
+		test_cmp ../../expect.sub ../../actual.sub
+	)
+'
+
+test_expect_failure '--no-index neglects info/exclude in bare repo' '
+	echo "fich1" >bare/d.git/info/exclude &&
+	(
+		cd bare/d.git &&
+		git grep --no-index o >../actual.noindex &&
+		grep "^fich1:olleh\$" ../actual.noindex &&
+		grep "^.gitignore:o[*]\$" ../actual.noindex &&
+		! grep file2 ../actual.noindex
+	)
+'
+
 test_expect_success 'setup double-dash tests' '
 cat >double-dash <<EOF &&
 --
@@ -527,4 +619,28 @@ test_expect_success 'grep -e -- -- path' '
 	test_cmp expected actual
 '
 
+test_expect_success 'Setup fake .git' '
+	cd t &&
+	GIT_CEILING_DIRECTORIES="`pwd`" &&
+	export GIT_CEILING_DIRECTORIES &&
+	cd a &&
+	mkdir -p .git/info &&
+	cd ../..
+
+'
+
+test_expect_failure 'Ignore fake .git/info/exclude' '
+	(
+		cd t/a &&
+		echo v > .git/info/exclude &&
+		git grep --no-index vvv . &&
+		rm .git/info/exclude
+	)
+'
+
+test_expect_success 'Unsetup fake .git' '
+	rm -rf t/a &&
+	unset GIT_CEILING_DIRECTORIES
+'
+
 test_done
-- 
1.7.1.rc1.69.g24c2f7

Re: [PATCH] t7002: test git grep --no-index from a bare repository

[Jonathan Nieder]

Hi Duy,

Nguyễn Thái Ngọc Duy wrote:

> Subject: [PATCH] t7002: test git grep --no-index from a bare repository

It’s t7810 now (to keep t7811 company).

>  There is an interesting thing about this command. Back in tp/setup
>  series, there is a patch that changes the current behavior,
>  "calculate prefix even if no worktree is found". grep is interesting
>  because it depends on the current behavior, i.e. prefix being NULL
>  in bare repo, while it still needs true prefix to do chdir()
>  stuff in run_pager().

Yes, sorry to let this hang for so long.  I liked your setup series
for many reasons and am happy to see pieces of it coming back to life.

> +++ b/Documentation/git-grep.txt
> @@ -28,8 +28,9 @@ SYNOPSIS
>  DESCRIPTION
>  -----------
>  Look for specified patterns in the tracked files in the work tree, blobs
> -registered in the index file, or blobs in given tree objects.
> -
> +registered in the index file, or blobs in given tree objects. By default
> +it will only search tracked files within the current directory (or full
> +tree if in bare repository).

Probably deserves more detail.

	Searches for lines matching the specified patterns in the
	work tree, the index, or a specified tree.

	By default, 'git grep' only examines tracked files in the
	subtree of the work tree rooted at the current working
	directory.  Output consists of matching lines preceded with
	the corresponding filename and a colon.

	With `--cached`, 'git grep' does the same search in the
	version scheduled for the next commit in the index.

	With `--no-index`, 'git grep' pays no mind to the index
	file and reports *all* matching files under the working
	directory.

	Given a commit name, 'git grep' does the same search in that
	historical version.  More generally, given a tree name, 'git
	grep' searches the subtree of that tree object corresponding
	to the path to the current directory from the root of the work
	tree (or the entire tree if there is no work tree, as in a
	bare repository).

>  
>  OPTIONS
>  -------
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 597f76b..e8abdc7 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1109,6 +1109,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  
>  	if (use_threads)
>  		hit |= wait_all();
> +
> +	/* FIXME prefix is NULL in bare repo, no matter where cwd is */
>  	if (hit && show_in_pager)
>  		run_pager(&opt, prefix);

This comment seems kind of unhelpful.  Maybe something like

	/*
	 * NOTE NOTE NOTE: Even in a bare repository, the user
	 * probably expected the command specified with -O to run in
	 * the current directory, but when --no-index is supplied, we
	 * are passing it paths relative to the .git directory.
	 * Until that changes, this needs not to chdir() in that case.
	 */

Do I understand correctly?

-- 8< --
Subject: grep documentation: flesh out description

As Duy noticed, it is not always obvious what directory ‘git grep’
is going to search in.  Add some other details to the description,
too.

Inspired by http://gitster.livejournal.com/27674.html

Cc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-grep.txt |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 5474dd7..d6cfbc6 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -27,9 +27,26 @@ SYNOPSIS
 
 DESCRIPTION
 -----------
-Look for specified patterns in the tracked files in the work tree, blobs
-registered in the index file, or blobs in given tree objects.
-
+Searches for lines matching the specified patterns in the work
+tree, the index, or the specified tree objects.
+
+By default, 'git grep' only examines tracked files in the subtree
+of the work tree rooted at the current working directory.  Output
+consists of matching lines preceded with the corresponding
+filename and a colon.
+
+With `--cached`, 'git grep' does the same search in the version
+scheduled for the next commit in the index.
+
+With `--no-index`, 'git grep' pays no mind to the index file and
+reports *all* matching files under the working directory.
+
+When passed a commit name, 'git grep' does the same search but in
+that historical version.  More generally, given a tree name,
+'git grep' searches the subtree of that tree object corresponding
+to the path to the current directory from the root of the work tree
+(or the entire tree if there is no work tree, as in a bare
+repository).
 
 OPTIONS
 -------
-- 
1.7.2.rc3

Re: [PATCH] t7002: test git grep --no-index from a bare repository

[Nguyen Thai Ngoc Duy]

2010/7/22 Jonathan Nieder <jrnieder@gmail.com>:
> Hi Duy,
>
> Nguyễn Thái Ngọc Duy wrote:
>
>> Subject: [PATCH] t7002: test git grep --no-index from a bare repository
>
> It’s t7810 now (to keep t7811 company).
>
>>  There is an interesting thing about this command. Back in tp/setup
>>  series, there is a patch that changes the current behavior,
>>  "calculate prefix even if no worktree is found". grep is interesting
>>  because it depends on the current behavior, i.e. prefix being NULL
>>  in bare repo, while it still needs true prefix to do chdir()
>>  stuff in run_pager().
>
> Yes, sorry to let this hang for so long.  I liked your setup series
> for many reasons and am happy to see pieces of it coming back to life.
>
>> +++ b/Documentation/git-grep.txt
>> @@ -28,8 +28,9 @@ SYNOPSIS
>>  DESCRIPTION
>>  -----------
>>  Look for specified patterns in the tracked files in the work tree, blobs
>> -registered in the index file, or blobs in given tree objects.
>> -
>> +registered in the index file, or blobs in given tree objects. By default
>> +it will only search tracked files within the current directory (or full
>> +tree if in bare repository).
>
> Probably deserves more detail.
>
>        Searches for lines matching the specified patterns in the
>        work tree, the index, or a specified tree.
>
>        By default, 'git grep' only examines tracked files in the
>        subtree of the work tree rooted at the current working
>        directory.  Output consists of matching lines preceded with
>        the corresponding filename and a colon.
>
>        With `--cached`, 'git grep' does the same search in the
>        version scheduled for the next commit in the index.
>
>        With `--no-index`, 'git grep' pays no mind to the index
>        file and reports *all* matching files under the working
>        directory.
>
>        Given a commit name, 'git grep' does the same search in that
>        historical version.  More generally, given a tree name, 'git
>        grep' searches the subtree of that tree object corresponding
>        to the path to the current directory from the root of the work
>        tree (or the entire tree if there is no work tree, as in a
>        bare repository).
>

Yeah.. grep looks more complex than I thought :) Documentation patch
should go separately then. Please go ahead and make a patch of this
text. Oh you did.

>>
>>  OPTIONS
>>  -------
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index 597f76b..e8abdc7 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -1109,6 +1109,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>
>>       if (use_threads)
>>               hit |= wait_all();
>> +
>> +     /* FIXME prefix is NULL in bare repo, no matter where cwd is */
>>       if (hit && show_in_pager)
>>               run_pager(&opt, prefix);
>
> This comment seems kind of unhelpful.  Maybe something like
>
>        /*
>         * NOTE NOTE NOTE: Even in a bare repository, the user
>         * probably expected the command specified with -O to run in
>         * the current directory, but when --no-index is supplied, we
>         * are passing it paths relative to the .git directory.
>         * Until that changes, this needs not to chdir() in that case.
>         */
>
> Do I understand correctly?

Hmm.. on second thought, the fault is probably not grep's. These
things must be consistent together:

 - new cwd after setup is at worktree top-level directory (or
undefined if no worktree is found?)
 - prefix must be aligned with new cwd. That is, chdir(prefix) must
give the original cwd (**1**)
 - gitdir is relative to new cwd
 - worktree is relative to new cwd

So probably best to adjust cwd in setup_git_directory_gently() in this
case to align with the NULL prefix, not the other way around.
Something like this (still incorrect):

diff --git a/setup.c b/setup.c
index 87c21f0..b67b3aa 100644
--- a/setup.c
+++ b/setup.c
@@ -412,6 +412,15 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			inside_git_dir = 1;
 			if (!work_tree_env)
 				inside_work_tree = 0;
+
+			/*
+			 * The old behavior is return NULL here.
+			 * Follow it and cwd back to because
+			 * NULL prefix means cwd does not move
+			 */
+			if (chdir(cwd))
+				die_errno("Cannot come back to cwd");
+
 			if (offset != len) {
 				root_len = offset_1st_component(cwd);
 				cwd[offset > root_len ? offset : root_len] = '\0';


(**1**) Not entirely correct, if original cwd is outside worktree,
then prefix is NULL anyway because prefix is not designed to be
"../../../"
-- 
Duy