[PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Johannes Schindelin]

This supports opening the results of a 'git grep' directly in a pager 
(where the pager can be 'vi', too).

This series is purely about convenience, everything the option does can
be done with a regular script or command line.

But I saw so many people doing their own scripts for that, and in many
cases, they are subtly broken (e.g.

	git grep -z <expr> | xargs -0r vi +/<expr>

would work as long as you do not have to check the exit status of git
grep from another script) that I finally decided to go for it and send
this patch pair.

My most common use case for this is to do something like

	git grep -Ovi SomeJustRenamedFile

to edit all files I might have forgotten to change after a git mv.

(Actually, to be honest, my use case involves -Pvi, but I will have to
retrain my hands.)

Johannes Schindelin (2):
  grep: Add the option '--open-files-in-pager'
  grep -P: allow optional argument specifying the pager (or editor)

 Documentation/git-grep.txt |    8 +++++
 builtin-grep.c             |   74 ++++++++++++++++++++++++++++++++++++++++++++
 git.c                      |    2 +-
 3 files changed, 83 insertions(+), 1 deletions(-)

[PATCH 1/2] grep: Add the option '--open-files-in-pager'

[Johannes Schindelin]

This adds an option to open the matching files in the pager, and if the
pager happens to be "less" (or "vi") and there is only one grep pattern,
it also jumps to the first match right away.

The short option was chose as '-O' to avoid clashes with GNU grep's
options (as suggested by Junio).

So, 'git grep -O abc' is a short form for 'less +/abc $(grep -l abc)'
except that it works also with spaces in file names, and it does not
start the pager if there was no matching file.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-grep.txt |    8 +++++
 builtin-grep.c             |   71 ++++++++++++++++++++++++++++++++++++++++++++
 git.c                      |    2 +-
 3 files changed, 80 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index e019e76..ab3bb77 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -15,6 +15,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]
 	   [-z | --null]
 	   [-c | --count] [--all-match] [-q | --quiet]
 	   [--max-depth <depth>]
@@ -101,6 +102,13 @@ OPTIONS
 	For better compatibility with 'git diff', --name-only is a
 	synonym for --files-with-matches.
 
+-O::
+--open-files-in-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
diff --git a/builtin-grep.c b/builtin-grep.c
index 5d83d9b..233c772 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -14,6 +14,7 @@
 #include "userdiff.h"
 #include "grep.h"
 #include "quote.h"
+#include "string-list.h"
 
 #ifndef NO_PTHREADS
 #include "thread-utils.h"
@@ -517,6 +518,31 @@ 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 && *(char *)data == '\0')
+		return;
+	string_list_append(xstrndup(data, len), path_list);
+}
+
+static void run_pager(struct grep_opt *opt, const char *prefix)
+{
+	struct string_list *path_list = opt->output_priv;
+	char **argv = xmalloc(sizeof(const char *) * (path_list->nr + 1));
+	int i;
+
+	for (i = 0; i < path_list->nr; i++)
+		argv[i] = path_list->items[i].string;
+	argv[path_list->nr] = NULL;
+
+	if (prefix)
+		chdir(prefix);
+	execvp(argv[0], argv);
+	error("Could not run pager %s: %s", argv[0], strerror(errno));
+}
+
 static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
 {
 	int hit = 0;
@@ -727,9 +753,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int cached = 0;
 	int seen_dashdash = 0;
 	int external_grep_allowed__ignored;
+	int show_in_pager = 0;
 	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;
 	struct option options[] = {
@@ -810,6 +838,8 @@ 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(""),
+		OPT_BOOLEAN('O', "open-files-in-pager", &show_in_pager,
+			"show matching files in the 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",
@@ -862,6 +892,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		argc--;
 	}
 
+	if (show_in_pager) {
+		const char *pager = getenv("GIT_PAGER");
+		if (!pager)
+			pager = getenv("PAGER");
+		if (!pager)
+			pager = "less";
+		opt.name_only = 1;
+		opt.null_following_name = 1;
+		opt.output_priv = &path_list;
+		opt.output = append_path;
+		string_list_append(pager, &path_list);
+		use_threads = 0;
+	}
+
 	if (!opt.pattern_list)
 		die("no pattern given.");
 	if (!opt.fixed && opt.ignore_case)
@@ -921,6 +965,29 @@ 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(buf.buf, &path_list);
+			strbuf_detach(&buf, NULL);
+		}
+	}
+
+	if (!show_in_pager)
+		setup_pager();
+
 	if (!list.nr) {
 		if (!cached)
 			setup_work_tree();
@@ -943,6 +1010,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 	if (use_threads)
 		hit |= wait_all();
+
+	if (hit && show_in_pager)
+		run_pager(&opt, prefix);
+
 	free_grep_patterns(&opt);
 	return !hit;
 }
diff --git a/git.c b/git.c
index 4c3028c..4def718 100644
--- a/git.c
+++ b/git.c
@@ -317,7 +317,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, RUN_SETUP | USE_PAGER },
+		{ "grep", cmd_grep, RUN_SETUP },
 		{ "hash-object", cmd_hash_object },
 		{ "help", cmd_help },
 		{ "index-pack", cmd_index_pack },
-- 
1.6.4.313.g3d9e3

Re: [PATCH 1/2] grep: Add the option '--open-files-in-pager'

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:

> This adds an option to open the matching files in the pager, and if the
> pager happens to be "less" (or "vi") and there is only one grep pattern,
> it also jumps to the first match right away.

Sounds interesting.

> diff --git a/builtin-grep.c b/builtin-grep.c
> index 5d83d9b..233c772 100644

I couldn’t get this to apply to current master or maint, and searching for
blob 5d83d9b to find a more suitable merge base revealed that I don’t
actually have that object in my repo.  Is this patch available already
applied in a public git repo somewhere?

Hopeful,
Jonathan

Re: [PATCH 1/2] grep: Add the option '--open-files-in-pager'

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 516 bytes --]

Hi,

On Sat, 27 Mar 2010, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> 
> > diff --git a/builtin-grep.c b/builtin-grep.c
> > index 5d83d9b..233c772 100644
> 
> I couldn’t get this to apply to current master or maint, and searching 
> for blob 5d83d9b to find a more suitable merge base revealed that I 
> don’t actually have that object in my repo.  Is this patch available 
> already applied in a public git repo somewhere?

Yes, it is in 4msysgit.git's "grep-P" branch (still misnamed).

Ciao,
Dscho

Re: [PATCH 1/2] grep: Add the option '--open-files-in-pager'

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I couldn’t get this to apply to current master or maint, and searching for
> blob 5d83d9b to find a more suitable merge base revealed that I don’t
> actually have that object in my repo.  Is this patch available already
> applied in a public git repo somewhere?

This seems to be based on a fork with a not-very-recent forkpoint.  I
managed to create a topic branch and merged it to 'pu'.

It probably wants to be adjusted to --no-index mode, as that is also about
finding strings in files in the filesystem, but I chose not to do that
myself.  For this topic to go forward I think it needs a rebase to a more
recent 'master'.

[PATCH 2/2] grep -P: allow optional argument specifying the pager (or editor)

[Johannes Schindelin]

Suppose you want to edit all files that contain a specific search term.
Of course, you can do something totally trivial such as

	git grep -z -e <term> | xargs -0r vi +/<term>

but maybe you are happy that the same will be achieved by

	git grep -Ovi <term>

now.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-grep.txt |    6 +++---
 builtin-grep.c             |   21 ++++++++++++---------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index ab3bb77..1b7e32d 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -15,7 +15,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]
+	   [(-O | --open-files-in-pager) [<pager>]]
 	   [-z | --null]
 	   [-c | --count] [--all-match] [-q | --quiet]
 	   [--max-depth <depth>]
@@ -102,8 +102,8 @@ OPTIONS
 	For better compatibility with 'git diff', --name-only is a
 	synonym for --files-with-matches.
 
--O::
---open-files-in-pager::
+-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
diff --git a/builtin-grep.c b/builtin-grep.c
index 233c772..a23f0dc 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -753,7 +753,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int cached = 0;
 	int seen_dashdash = 0;
 	int external_grep_allowed__ignored;
-	int show_in_pager = 0;
+	const char *show_in_pager = NULL, *default_pager = "dummy";
 	struct grep_opt opt;
 	struct object_array list = { 0, 0, NULL };
 	const char **paths = NULL;
@@ -838,8 +838,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(""),
-		OPT_BOOLEAN('O', "open-files-in-pager", &show_in_pager,
-			"show matching files in the pager"),
+		{ 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",
@@ -893,16 +894,18 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	}
 
 	if (show_in_pager) {
-		const char *pager = getenv("GIT_PAGER");
-		if (!pager)
-			pager = getenv("PAGER");
-		if (!pager)
-			pager = "less";
+		if (show_in_pager == default_pager) {
+			show_in_pager = getenv("GIT_PAGER");
+			if (!show_in_pager)
+				show_in_pager = getenv("PAGER");
+			if (!show_in_pager)
+				show_in_pager = "less";
+		}
 		opt.name_only = 1;
 		opt.null_following_name = 1;
 		opt.output_priv = &path_list;
 		opt.output = append_path;
-		string_list_append(pager, &path_list);
+		string_list_append(show_in_pager, &path_list);
 		use_threads = 0;
 	}
 
-- 
1.6.4.313.g3d9e3

Re: [PATCH 2/2] grep -P: allow optional argument specifying the pager (or editor)

[Johannes Schindelin]

Hi,

the subject should read "grep -O...", of course.

Sorry,
Dscho

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Bert Wesarg]

On Fri, Mar 26, 2010 at 11:48, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> This supports opening the results of a 'git grep' directly in a pager
> (where the pager can be 'vi', too).
>
> This series is purely about convenience, everything the option does can
> be done with a regular script or command line.
>
> But I saw so many people doing their own scripts for that, and in many
> cases, they are subtly broken (e.g.
>
>        git grep -z <expr> | xargs -0r vi +/<expr>
>
> would work as long as you do not have to check the exit status of git
> grep from another script) that I finally decided to go for it and send
> this patch pair.
>
> My most common use case for this is to do something like
>
>        git grep -Ovi SomeJustRenamedFile
>
> to edit all files I might have forgotten to change after a git mv.
>
> (Actually, to be honest, my use case involves -Pvi, but I will have to
> retrain my hands.)
>
> Johannes Schindelin (2):
>  grep: Add the option '--open-files-in-pager'
>  grep -P: allow optional argument specifying the pager (or editor)

Your re-training failed miserable ;-)

I like the idea. But could it make sense to get the line number of the
first match to the filename. In the same syntax as on the console
(i.e. <file>:<line>:)?

I have also the feeling that -O potion does not like pager with
arguments, be it from GIT_PAGER, PAGER, or from the command line.

Regards,
Bert

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2004 bytes --]

Hi,

On Fri, 26 Mar 2010, Bert Wesarg wrote:

> On Fri, Mar 26, 2010 at 11:48, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > This supports opening the results of a 'git grep' directly in a pager
> > (where the pager can be 'vi', too).
> >
> > This series is purely about convenience, everything the option does can
> > be done with a regular script or command line.
> >
> > But I saw so many people doing their own scripts for that, and in many
> > cases, they are subtly broken (e.g.
> >
> >        git grep -z <expr> | xargs -0r vi +/<expr>
> >
> > would work as long as you do not have to check the exit status of git
> > grep from another script) that I finally decided to go for it and send
> > this patch pair.
> >
> > My most common use case for this is to do something like
> >
> >        git grep -Ovi SomeJustRenamedFile
> >
> > to edit all files I might have forgotten to change after a git mv.
> >
> > (Actually, to be honest, my use case involves -Pvi, but I will have to
> > retrain my hands.)
> >
> > Johannes Schindelin (2):
> >  grep: Add the option '--open-files-in-pager'
> >  grep -P: allow optional argument specifying the pager (or editor)
> 
> Your re-training failed miserable ;-)

It would have if I said that it is finished.

> I like the idea. But could it make sense to get the line number of the 
> first match to the filename. In the same syntax as on the console (i.e. 
> <file>:<line>:)?

See Peff's mail, and my answer to it for an explanation why it would 
actively _not_ help me that way.

> I have also the feeling that -O potion does not like pager with
> arguments, be it from GIT_PAGER, PAGER, or from the command line.

You are correct. That's why I said "-O[<pager>]" and not 
"-O[<pager-with-arguments-that-cause-whitespace-problems-especially-with-spaces-in-directory-names-so-you-need-to-make-a-script-wrapper-anyway>]" 

Of course, if you would find that useful, nobody stops you from making a 
patch on top of my patch pair.

Ciao,
Dscho

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Bert Wesarg]

Hi,

On Fri, Mar 26, 2010 at 17:18, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>> I have also the feeling that -O potion does not like pager with
>> arguments, be it from GIT_PAGER, PAGER, or from the command line.
>
> You are correct. That's why I said "-O[<pager>]" and not
> "-O[<pager-with-arguments-that-cause-whitespace-problems-especially-with-spaces-in-directory-names-so-you-need-to-make-a-script-wrapper-anyway>]"

I don't know how common it is to have arguments for the pager in
GIT_PAGER or PAGER or whether git actually support this, but your
first patch definitely does not work with this, regardless of your
-O[<pager>] addition of the second patch.

Bert

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Jeff King]

On Fri, Mar 26, 2010 at 11:48:41AM +0100, Johannes Schindelin wrote:

> This supports opening the results of a 'git grep' directly in a pager 
> (where the pager can be 'vi', too).

This is not an argument against your patch, but you may be interested in
an alternate method:

  git grep -n $pattern >grep.out
  vim -q grep.out

The advantage is that the editor understands the output as a "quickfix"
list and lets you cycle through the hits (just like you might with
compiler errors). The disadvantage is that quickfix is a vim extension,
so "less" and stock "vi" can't do this (I imagine emacs has a similar
feature). It's also obviously a little more typing, but you can hide it
inside an alias quite easily.

I use the same trick to look for "^<<<<<<<", and have vim cycle through
merge conflicts (you can also make it quicker by restricitng the grep
only to unmerged files).

-Peff

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Johannes Schindelin]

Hi,

On Fri, 26 Mar 2010, Jeff King wrote:

> On Fri, Mar 26, 2010 at 11:48:41AM +0100, Johannes Schindelin wrote:
> 
> > This supports opening the results of a 'git grep' directly in a pager 
> > (where the pager can be 'vi', too).
> 
> This is not an argument against your patch, but you may be interested in 
> an alternate method:
> 
>   git grep -n $pattern >grep.out vim -q grep.out
> 
> The advantage is that the editor understands the output as a "quickfix" 
> list and lets you cycle through the hits (just like you might with 
> compiler errors). The disadvantage is that quickfix is a vim extension, 
> so "less" and stock "vi" can't do this (I imagine emacs has a similar 
> feature). It's also obviously a little more typing, but you can hide it 
> inside an alias quite easily.

In addition to the disadvantages you are listing, it does not jump 
directly to the word I am looking for. As you know, I am working 
with LaTeX files heavily, where convention dictates that a paragraph is 
represented by a single line in the source code.

So yes, I really need -O, and if a colleague would not have asked me why 
this useful feature is not in upstream Git, I would not even have 
submitted the patch pair.

Ciao,
Dscho

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Jeff King]

On Fri, Mar 26, 2010 at 05:14:46PM +0100, Johannes Schindelin wrote:

> In addition to the disadvantages you are listing, it does not jump 
> directly to the word I am looking for. As you know, I am working 
> with LaTeX files heavily, where convention dictates that a paragraph is 
> represented by a single line in the source code.

Sure, that is a reasonable feature to want that my solution did not have
(though I have never seen LaTeX code with that requirement; I wrap my
lines at a reasonable width and separate paragraphs with a blank line).

> So yes, I really need -O, and if a colleague would not have asked me why 
> this useful feature is not in upstream Git, I would not even have 
> submitted the patch pair.

Yep. That is why I started my mail with "this is not an argument against
your patch". I was just trying to be helpful and offer a possible
alternative.

-Peff

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Mar 26, 2010 at 11:48:41AM +0100, Johannes Schindelin wrote:
>
>> This supports opening the results of a 'git grep' directly in a pager 
>> (where the pager can be 'vi', too).
>
> This is not an argument against your patch, but you may be interested in
> an alternate method:
>
>   git grep -n $pattern >grep.out
>   vim -q grep.out
>
> The advantage is that the editor understands the output as a "quickfix"
> list and lets you cycle through the hits (just like you might with
> compiler errors). The disadvantage is that quickfix is a vim extension,
> so "less" and stock "vi" can't do this (I imagine emacs has a similar
> feature). It's also obviously a little more typing, but you can hide it
> inside an alias quite easily.

"M-x grep-find" mode is designed for this, and I often run "git grep -n"
in that mode..  The output is dumped into a sbuffer that is used as a
"quickfix list" to bring in files with hits and jump around.

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Francis Moreau]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> On Fri, Mar 26, 2010 at 11:48:41AM +0100, Johannes Schindelin wrote:
>>
>>> This supports opening the results of a 'git grep' directly in a pager 
>>> (where the pager can be 'vi', too).
>>
>> This is not an argument against your patch, but you may be interested in
>> an alternate method:
>>
>>   git grep -n $pattern >grep.out
>>   vim -q grep.out
>>
>> The advantage is that the editor understands the output as a "quickfix"
>> list and lets you cycle through the hits (just like you might with
>> compiler errors). The disadvantage is that quickfix is a vim extension,
>> so "less" and stock "vi" can't do this (I imagine emacs has a similar
>> feature). It's also obviously a little more typing, but you can hide it
>> inside an alias quite easily.
>
> "M-x grep-find" mode is designed for this,

Do you mean "M-x grep" ? I don't see the point to use "M-x grep-find".

> and I often run "git grep -n" in that mode..

I always need to pass the '--no-pager' switch: "git --no-pager grep -n"
since git doesn't detect that its output is sent to a 'dumb' terminal.

-- 
Francis

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Junio C Hamano]

Francis Moreau <francis.moro@gmail.com> writes:

>> and I often run "git grep -n" in that mode..
>
> I always need to pass the '--no-pager' switch: "git --no-pager grep -n"
> since git doesn't detect that its output is sent to a 'dumb' terminal.

Sorry, but as any self-respecting Emacs user would have PAGER set to cat
(and EDITOR set to emacsclient), I thought nobody would need --no-pager
;-)

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Francis Moreau]

Junio C Hamano <gitster@pobox.com> writes:

> Francis Moreau <francis.moro@gmail.com> writes:
>
>>> and I often run "git grep -n" in that mode..
>>
>> I always need to pass the '--no-pager' switch: "git --no-pager grep -n"
>> since git doesn't detect that its output is sent to a 'dumb' terminal.
>
> Sorry, but as any self-respecting Emacs user would have PAGER set to cat

Well I do, but only for "M-x shell", probably because I usually start
all commands from that shell.

> (and EDITOR set to emacsclient), I thought nobody would need
> --no-pager ;-)

Indeed, I'll setup PAGER to cat when starting emacs.

That makes me think to one common operation I do which is not really
convenient when PAGER is set to cat: git-log. Piping the output of
git-log to a pager such as less(1) makes git-log to suspend when the
pipe is full. OTOH setting PAGER to cat, git-log never blocks and for
projects with a (very) long history such as git or the linux kernel,
git-log can show a lot of commits making the cpu really busy and the
emacs buffer really huge.

One way to workaround this is to pass -n10 for example to git-log, but
it's rather ugly.

Do you have any tricks for this case ?

Thanks
-- 
Francis

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Junio C Hamano]

Francis Moreau <francis.moro@gmail.com> writes:

> Do you have any tricks for this case ?

I use PAGER=cat only inside Emacs, so I don't see how "log" can be a
problem.  Perhaps that is because I don't use "M-x shell" (I used to but
not anymore).  Of course you have to limit the extent of "log" in "M-x
compile" mode, but that goes without saying.

I may have used some hooks when entering shell mode to tweak the
environment further only under "M-x shell".

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Francis Moreau]

Junio C Hamano <gitster@pobox.com> writes:

> Francis Moreau <francis.moro@gmail.com> writes:
>
>> Do you have any tricks for this case ?
>
> I use PAGER=cat only inside Emacs, so I don't see how "log" can be a
> problem.

what do you mean by this ? Do you mean that you run git-log outside
emacs ?

> Of course you have to limit the extent of "log" in "M-x compile" mode,
> but that goes without saying.

Yes but in this case (log inside emacs) I was actually wondering if you
were using any tricks to avoid limiting _manually_ the extent of
git-log. With PAGER=less for example, the extent is _automatically_
limited by the pipe, which I found convenient.

-- 
Francis

Re: [PATCH 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Jon Seymour]

On Fri, Mar 26, 2010 at 11:46 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Mar 26, 2010 at 11:48:41AM +0100, Johannes Schindelin wrote:
>
>> This supports opening the results of a 'git grep' directly in a pager
>> (where the pager can be 'vi', too).
>
> This is not an argument against your patch, but you may be interested in
> an alternate method:
>
>  git grep -n $pattern >grep.out
>  vim -q grep.out

Or, in bash:

    vim -q <(git grep -n $pattern)

though, of course you don't get the exit code from git.

jon.

[PATCH v2 0/2] Teach 'git grep' about --open-files-in-pager=[<pager>]

[Jonathan Nieder]

This series teaches ‘git grep’ to accept

	git grep -Ovi some_function_name

to find all the call sites for some_function_name so they can be
adjusted.  Dscho’s cover letter[1] explains it better.

And in fact, Dscho did all the work here; this iteration just rebases it
on master and adds some tests.

Happy hacking,
Jonathan

Johannes Schindelin (2):
  grep: Add the option '--open-files-in-pager'
  grep -O: allow optional argument specifying the pager (or editor)

 Documentation/git-grep.txt |    8 +
 builtin/grep.c             |   81 +++++++-
 git.c                      |    2 +-
 t/lib-pager.sh             |   15 ++
 t/t7002-grep.sh            |  530 --------------------------------------------
 t/t7006-pager.sh           |   16 +-
 t/t7810-grep.sh            |  530 ++++++++++++++++++++++++++++++++++++++++++++
 t/t7811-grep-open.sh       |  157 +++++++++++++
 8 files changed, 792 insertions(+), 547 deletions(-)
 create mode 100644 t/lib-pager.sh
 delete mode 100755 t/t7002-grep.sh
 create mode 100755 t/t7810-grep.sh
 create mode 100644 t/t7811-grep-open.sh

[1] http://thread.gmane.org/gmane.comp.version-control.git/143219

[PATCH 1/2] grep: Add the option '--open-files-in-pager'

[Jonathan Nieder]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This adds an option to open the matching files in the pager, and if the
pager happens to be "less" (or "vi") and there is only one grep pattern,
it also jumps to the first match right away.

The short option was chose as '-O' to avoid clashes with GNU grep's
options (as suggested by Junio).

So, 'git grep -O abc' is a short form for 'less +/abc $(grep -l abc)'
except that it works also with spaces in file names, and it does not
start the pager if there was no matching file.

[jn: rebased and added tests; with error handling fix from Junio
squashed in]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-grep.txt         |    8 ++
 builtin/grep.c                     |   83 ++++++++++++++++++-
 git.c                              |    2 +-
 t/lib-pager.sh                     |   15 ++++
 t/t7006-pager.sh                   |   16 +---
 t/{t7002-grep.sh => t7810-grep.sh} |    0
 t/t7811-grep-open.sh               |  154 ++++++++++++++++++++++++++++++++++++
 7 files changed, 261 insertions(+), 17 deletions(-)
 create mode 100644 t/lib-pager.sh
 rename t/{t7002-grep.sh => t7810-grep.sh} (100%)
 create mode 100644 t/t7811-grep-open.sh

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 4b32322..8fdd8e1 100644
--- a/Documentation/git-grep.txt
+++ b/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]
 	   [-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::
+--open-files-in-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
diff --git a/builtin/grep.c b/builtin/grep.c
index b194ea3..bc6c2ea 100644
--- a/builtin/grep.c
+++ b/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,7 +558,34 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 	}
 }
 
-static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
+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(xstrndup(data, len), path_list);
+}
+
+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))
+		error("Failed to chdir: %s", prefix);
+	status = run_command_v_opt(argv, RUN_USING_SHELL);
+	if (status)
+		exit(status);
+}
+
+static int grep_cache(struct grep_opt *opt, const char **paths,
+		      int cached, int show_in_pager, const char *prefix)
 {
 	int hit = 0;
 	int nr;
@@ -590,6 +619,8 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
 		if (hit && opt->status_only)
 			break;
 	}
+	if (hit && show_in_pager)
+		run_pager(opt, prefix);
 	free_grep_patterns(opt);
 	return hit;
 }
@@ -675,7 +706,8 @@ 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_directory(struct grep_opt *opt, const char **paths)
+static int grep_directory(struct grep_opt *opt, const char **paths,
+			  int show_in_pager, const char *prefix)
 {
 	struct dir_struct dir;
 	int i, hit = 0;
@@ -689,6 +721,8 @@ static int grep_directory(struct grep_opt *opt, const char **paths)
 		if (hit && opt->status_only)
 			break;
 	}
+	if (hit && show_in_pager)
+		run_pager(opt, prefix);
 	free_grep_patterns(opt);
 	return hit;
 }
@@ -782,9 +816,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int cached = 0;
 	int seen_dashdash = 0;
 	int external_grep_allowed__ignored;
+	int show_in_pager = 0;
 	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;
@@ -868,6 +904,8 @@ 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(""),
+		OPT_BOOLEAN('O', "open-files-in-pager", &show_in_pager,
+			"show matching files in the 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",
@@ -943,6 +981,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		argc--;
 	}
 
+	if (show_in_pager) {
+		const char *pager = git_pager(1);
+		if (!pager) {
+			show_in_pager = 0;
+		} else {
+			opt.name_only = 1;
+			opt.null_following_name = 1;
+			opt.output_priv = &path_list;
+			opt.output = append_path;
+			string_list_append(pager, &path_list);
+			use_threads = 0;
+		}
+	}
+
 	if (!opt.pattern_list)
 		die("no pattern given.");
 	if (!opt.fixed && opt.ignore_case)
@@ -999,13 +1051,36 @@ 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(buf.buf, &path_list);
+			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);
+		hit = grep_directory(&opt, paths, show_in_pager, prefix);
 		if (use_threads)
 			hit |= wait_all();
 		return !hit;
@@ -1016,7 +1091,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		if (!cached)
 			setup_work_tree();
 
-		hit = grep_cache(&opt, paths, cached);
+		hit = grep_cache(&opt, paths, cached, show_in_pager, prefix);
 		if (use_threads)
 			hit |= wait_all();
 		return !hit;
diff --git a/git.c b/git.c
index 99f0363..265fa09 100644
--- a/git.c
+++ b/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 },
diff --git a/t/lib-pager.sh b/t/lib-pager.sh
new file mode 100644
index 0000000..f8c6025
--- /dev/null
+++ b/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
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 3bc7a2a..fc993fc 100755
--- a/t/t7006-pager.sh
+++ b/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 ||
diff --git a/t/t7002-grep.sh b/t/t7810-grep.sh
similarity index 100%
rename from t/t7002-grep.sh
rename to t/t7810-grep.sh
diff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh
new file mode 100644
index 0000000..72e4023
--- /dev/null
+++ b/t/t7811-grep-open.sh
@@ -0,0 +1,154 @@
+#!/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
+'
+
+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
+'
+
+test_expect_success 'modified file' '
+	rm -f actual &&
+	cat >less <<-\EOF &&
+	#!/bin/sh
+	printf "%s\n" "$@" >actual
+	EOF
+	chmod +x $less &&
+	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_PAGER="pwd >../dir; :" &&
+		git grep -O "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
-- 
1.7.1

Re: [PATCH 1/2] grep: Add the option '--open-files-in-pager'

[Jonathan Nieder]

Jonathan Nieder wrote:

> +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 &&

Ugh.  We need a static analyzer for shell scripts. :)  Short of that,
here’s a fixup to squash in.

Sorry for the trouble.

diff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh
index 72e4023..fcfc56e 100644
--- a/t/t7811-grep-open.sh
+++ b/t/t7811-grep-open.sh
@@ -97,7 +97,7 @@ test_expect_success 'git grep -O jumps to line in less' '
 	EOF
 	>empty &&
 
-	GIT_PAGER=./less git grep -O GREP_PATTERN >out
+	GIT_PAGER=./less git grep -O GREP_PATTERN >out &&
 	test_cmp expect actual &&
 	test_cmp empty out
 '
@@ -134,7 +134,7 @@ test_expect_success 'run from subdir' '
 		cd subdir &&
 		export GIT_PAGER &&
 		GIT_PAGER='\''printf "%s\n" >../args'\'' &&
-		git grep -O "enum grep_pat_token" >../out
+		git grep -O "enum grep_pat_token" >../out &&
 		GIT_PAGER="pwd >../dir; :" &&
 		git grep -O "enum grep_pat_token" >../out2
 	) &&
--