Merge branch 'jn/grep-open'

* jn/grep-open:
  t/t7811-grep-open.sh: remove broken/redundant creation of fake "less" script
  t/t7811-grep-open.sh: ensure fake "less" is made executable
  t/lib-pager.sh: remove unnecessary '^' from 'expr' regular expression
  grep -O: allow optional argument specifying the pager (or editor)
  grep: Add the option '--open-files-in-pager'
  Unify code paths of threaded greps
  grep: refactor grep_objects loop into its own function

Conflicts:
	t/t7006-pager.sh

Documentation/git-grep.txt

@@ -14,6 +14,7 @@ SYNOPSIS
 	   [-E | --extended-regexp] [-G | --basic-regexp]
 	   [-F | --fixed-strings] [-n]
 	   [-l | --files-with-matches] [-L | --files-without-match]
+	   [(-O | --open-files-in-pager) [<pager>]]
 	   [-z | --null]
 	   [-c | --count] [--all-match] [-q | --quiet]
 	   [--max-depth <depth>]
@@ -104,6 +105,13 @@ OPTIONS
 	For better compatibility with 'git diff', `--name-only` is a
 	synonym for `--files-with-matches`.
 
+-O [<pager>]::
+--open-files-in-pager [<pager>]::
+	Open the matching files in the pager (not the output of 'grep').
+	If the pager happens to be "less" or "vi", and the user
+	specified only one pattern, the first file is positioned at
+	the first match automatically.
+
 -z::
 --null::
 	Output \0 instead of the character that normally follows a

builtin/grep.c

@@ -11,6 +11,8 @@
 #include "tree-walk.h"
 #include "builtin.h"
 #include "parse-options.h"
+#include "string-list.h"
+#include "run-command.h"
 #include "userdiff.h"
 #include "grep.h"
 #include "quote.h"
@@ -556,6 +558,33 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 	}
 }
 
+static void append_path(struct grep_opt *opt, const void *data, size_t len)
+{
+	struct string_list *path_list = opt->output_priv;
+
+	if (len == 1 && *(const char *)data == '\0')
+		return;
+	string_list_append(path_list, xstrndup(data, len));
+}
+
+static void run_pager(struct grep_opt *opt, const char *prefix)
+{
+	struct string_list *path_list = opt->output_priv;
+	const char **argv = xmalloc(sizeof(const char *) * (path_list->nr + 1));
+	int i, status;
+
+	for (i = 0; i < path_list->nr; i++)
+		argv[i] = path_list->items[i].string;
+	argv[path_list->nr] = NULL;
+
+	if (prefix && chdir(prefix))
+		die("Failed to chdir: %s", prefix);
+	status = run_command_v_opt(argv, RUN_USING_SHELL);
+	if (status)
+		exit(status);
+	free(argv);
+}
+
 static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
 {
 	int hit = 0;
@@ -590,7 +619,6 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
 		if (hit && opt->status_only)
 			break;
 	}
-	free_grep_patterns(opt);
 	return hit;
 }
 
@@ -675,6 +703,25 @@ static int grep_object(struct grep_opt *opt, const char **paths,
 	die("unable to grep from object of type %s", typename(obj->type));
 }
 
+static int grep_objects(struct grep_opt *opt, const char **paths,
+			const struct object_array *list)
+{
+	unsigned int i;
+	int hit = 0;
+	const unsigned int nr = list->nr;
+
+	for (i = 0; i < nr; i++) {
+		struct object *real_obj;
+		real_obj = deref_tag(list->objects[i].item, NULL, 0);
+		if (grep_object(opt, paths, real_obj, list->objects[i].name)) {
+			hit = 1;
+			if (opt->status_only)
+				break;
+		}
+	}
+	return hit;
+}
+
 static int grep_directory(struct grep_opt *opt, const char **paths)
 {
 	struct dir_struct dir;
@@ -689,7 +736,6 @@ static int grep_directory(struct grep_opt *opt, const char **paths)
 		if (hit && opt->status_only)
 			break;
 	}
-	free_grep_patterns(opt);
 	return hit;
 }
 
@@ -786,9 +832,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int cached = 0;
 	int seen_dashdash = 0;
 	int external_grep_allowed__ignored;
+	const char *show_in_pager = NULL, *default_pager = "dummy";
 	struct grep_opt opt;
 	struct object_array list = { 0, 0, NULL };
 	const char **paths = NULL;
+	struct string_list path_list = { NULL, 0, 0, 0 };
 	int i;
 	int dummy;
 	int nongit = 0, use_index = 1;
@@ -872,6 +920,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN(0, "all-match", &opt.all_match,
 			"show only matches from files that match all patterns"),
 		OPT_GROUP(""),
+		{ OPTION_STRING, 'O', "open-files-in-pager", &show_in_pager,
+			"pager", "show matching files in the pager",
+			PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
 		OPT_BOOLEAN(0, "ext-grep", &external_grep_allowed__ignored,
 			    "allow calling of grep(1) (ignored by this build)"),
 		{ OPTION_CALLBACK, 0, "help-all", &options, NULL, "show usage",
@@ -947,6 +998,17 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		argc--;
 	}
 
+	if (show_in_pager == default_pager)
+		show_in_pager = git_pager(1);
+	if (show_in_pager) {
+		opt.name_only = 1;
+		opt.null_following_name = 1;
+		opt.output_priv = &path_list;
+		opt.output = append_path;
+		string_list_append(&path_list, show_in_pager);
+		use_threads = 0;
+	}
+
 	if (!opt.pattern_list)
 		die("no pattern given.");
 	if (!opt.fixed && opt.ignore_case)
@@ -1003,44 +1065,51 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		paths[1] = NULL;
 	}
 
+	if (show_in_pager && (cached || list.nr))
+		die("--open-files-in-pager only works on the worktree");
+
+	if (show_in_pager && opt.pattern_list && !opt.pattern_list->next) {
+		const char *pager = path_list.items[0].string;
+		int len = strlen(pager);
+
+		if (len > 4 && is_dir_sep(pager[len - 5]))
+			pager += len - 4;
+
+		if (!strcmp("less", pager) || !strcmp("vi", pager)) {
+			struct strbuf buf = STRBUF_INIT;
+			strbuf_addf(&buf, "+/%s%s",
+					strcmp("less", pager) ? "" : "*",
+					opt.pattern_list->pattern);
+			string_list_append(&path_list, buf.buf);
+			strbuf_detach(&buf, NULL);
+		}
+	}
+
+	if (!show_in_pager)
+		setup_pager();
+
+
 	if (!use_index) {
-		int hit;
 		if (cached)
 			die("--cached cannot be used with --no-index.");
 		if (list.nr)
 			die("--no-index cannot be used with revs.");
 		hit = grep_directory(&opt, paths);
-		if (use_threads)
-			hit |= wait_all();
-		return !hit;
-	}
-
-	if (!list.nr) {
-		int hit;
+	} else if (!list.nr) {
 		if (!cached)
 			setup_work_tree();
 
 		hit = grep_cache(&opt, paths, cached);
-		if (use_threads)
-			hit |= wait_all();
-		return !hit;
-	}
-
-	if (cached)
-		die("both --cached and trees are given.");
-
-	for (i = 0; i < list.nr; i++) {
-		struct object *real_obj;
-		real_obj = deref_tag(list.objects[i].item, NULL, 0);
-		if (grep_object(&opt, paths, real_obj, list.objects[i].name)) {
-			hit = 1;
-			if (opt.status_only)
-				break;
-		}
+	} else {
+		if (cached)
+			die("both --cached and trees are given.");
+		hit = grep_objects(&opt, paths, &list);
 	}
 
 	if (use_threads)
 		hit |= wait_all();
+	if (hit && show_in_pager)
+		run_pager(&opt, prefix);
 	free_grep_patterns(&opt);
 	return !hit;
 }

git.c

@@ -329,7 +329,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "fsck-objects", cmd_fsck, RUN_SETUP },
 		{ "gc", cmd_gc, RUN_SETUP },
 		{ "get-tar-commit-id", cmd_get_tar_commit_id },
-		{ "grep", cmd_grep, USE_PAGER },
+		{ "grep", cmd_grep },
 		{ "hash-object", cmd_hash_object },
 		{ "help", cmd_help },
 		{ "index-pack", cmd_index_pack },

t/lib-pager.sh

@@ -0,0 +1,15 @@
+#!/bin/sh
+
+test_expect_success 'determine default pager' '
+	test_might_fail git config --unset core.pager &&
+	less=$(
+		unset PAGER GIT_PAGER;
+		git var GIT_PAGER
+	) &&
+	test -n "$less"
+'
+
+if expr "$less" : '[a-z][a-z]*$' >/dev/null
+then
+	test_set_prereq SIMPLEPAGER
+fi

t/t7006-pager.sh

@@ -3,6 +3,7 @@
 test_description='Test automatic use of a pager.'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-pager.sh
 
 cleanup_fail() {
 	echo >&2 cleanup failed
@@ -158,21 +159,12 @@ test_expect_success 'color when writing to a file intended for a pager' '
 	colorful colorful.log
 '
 
-test_expect_success 'determine default pager' '
-	unset PAGER GIT_PAGER;
-	test_might_fail git config --unset core.pager ||
-	cleanup_fail &&
-
-	less=$(git var GIT_PAGER) &&
-	test -n "$less"
-'
-
-if expr "$less" : '[a-z][a-z]*$' >/dev/null && test_have_prereq TTY
+if test_have_prereq SIMPLEPAGER && test_have_prereq TTY
 then
-	test_set_prereq SIMPLEPAGER
+	test_set_prereq SIMPLEPAGERTTY
 fi
 
-test_expect_success SIMPLEPAGER 'default pager is used by default' '
+test_expect_success SIMPLEPAGERTTY 'default pager is used by default' '
 	unset PAGER GIT_PAGER;
 	test_might_fail git config --unset core.pager &&
 	rm -f default_pager_used ||

t/t7811-grep-open.sh

@@ -0,0 +1,153 @@
+#!/bin/sh
+
+test_description='git grep --open-files-in-pager
+'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-pager.sh
+unset PAGER GIT_PAGER
+
+test_expect_success 'setup' '
+	test_commit initial grep.h "
+enum grep_pat_token {
+	GREP_PATTERN,
+	GREP_PATTERN_HEAD,
+	GREP_PATTERN_BODY,
+	GREP_AND,
+	GREP_OPEN_PAREN,
+	GREP_CLOSE_PAREN,
+	GREP_NOT,
+	GREP_OR,
+};" &&
+
+	test_commit add-user revision.c "
+	}
+	if (seen_dashdash)
+		read_pathspec_from_stdin(revs, &sb, prune);
+	strbuf_release(&sb);
+}
+
+static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
+{
+	append_grep_pattern(&revs->grep_filter, ptn, \"command line\", 0, what);
+" &&
+
+	mkdir subdir &&
+	test_commit subdir subdir/grep.c "enum grep_pat_token" &&
+
+	test_commit uninteresting unrelated "hello, world" &&
+
+	echo GREP_PATTERN >untracked
+'
+
+test_expect_success SIMPLEPAGER 'git grep -O' '
+	cat >$less <<-\EOF &&
+	#!/bin/sh
+	printf "%s\n" "$@" >pager-args
+	EOF
+	chmod +x $less &&
+	cat >expect.less <<-\EOF &&
+	+/*GREP_PATTERN
+	grep.h
+	EOF
+	echo grep.h >expect.notless &&
+	>empty &&
+
+	PATH=.:$PATH git grep -O GREP_PATTERN >out &&
+	{
+		test_cmp expect.less pager-args ||
+		test_cmp expect.notless pager-args
+	} &&
+	test_cmp empty out
+'
+
+test_expect_success 'git grep -O --cached' '
+	test_must_fail git grep --cached -O GREP_PATTERN >out 2>msg &&
+	grep open-files-in-pager msg
+'
+
+test_expect_success 'git grep -O --no-index' '
+	rm -f expect.less pager-args out &&
+	cat >expect <<-\EOF &&
+	grep.h
+	untracked
+	EOF
+	>empty &&
+
+	(
+		GIT_PAGER='\''printf "%s\n" >pager-args'\'' &&
+		export GIT_PAGER &&
+		git grep --no-index -O GREP_PATTERN >out
+	) &&
+	test_cmp expect pager-args &&
+	test_cmp empty out
+'
+
+test_expect_success 'setup: fake "less"' '
+	cat >less <<-\EOF &&
+	#!/bin/sh
+	printf "%s\n" "$@" >actual
+	EOF
+	chmod +x less
+'
+
+test_expect_success 'git grep -O jumps to line in less' '
+	cat >expect <<-\EOF &&
+	+/*GREP_PATTERN
+	grep.h
+	EOF
+	>empty &&
+
+	GIT_PAGER=./less git grep -O GREP_PATTERN >out &&
+	test_cmp expect actual &&
+	test_cmp empty out &&
+
+	git grep -O./less GREP_PATTERN >out2 &&
+	test_cmp expect actual &&
+	test_cmp empty out2
+'
+
+test_expect_success 'modified file' '
+	rm -f actual &&
+	cat >expect <<-\EOF &&
+	+/*enum grep_pat_token
+	grep.h
+	revision.c
+	subdir/grep.c
+	unrelated
+	EOF
+	>empty &&
+
+	echo "enum grep_pat_token" >unrelated &&
+	test_when_finished "git checkout HEAD unrelated" &&
+	GIT_PAGER=./less git grep -F -O "enum grep_pat_token" >out &&
+	test_cmp expect actual &&
+	test_cmp empty out
+'
+
+test_expect_success 'run from subdir' '
+	rm -f actual &&
+	echo grep.c >expect &&
+	>empty &&
+
+	(
+		cd subdir &&
+		export GIT_PAGER &&
+		GIT_PAGER='\''printf "%s\n" >../args'\'' &&
+		git grep -O "enum grep_pat_token" >../out &&
+		git grep -O"pwd >../dir; :" "enum grep_pat_token" >../out2
+	) &&
+	case $(cat dir) in
+	*subdir)
+		: good
+		;;
+	*)
+		false
+		;;
+	esac &&
+	test_cmp expect args &&
+	test_cmp empty out &&
+	test_cmp empty out2
+'
+
+test_done