Linus Torvalds [off-list ref] writes:
On Sat, 13 May 2006, Junio C Hamano wrote:
quoted
* built-in grep (jc/grep)
Ready.
I'm not entirely convinced.
I am not either after seeing your numbers and trying them
myself. I was going to look at it myself over the weekend, but
I ended up spending most of the time migrating my environment,
so no progress on that front yet. Sorry.
On Sun, 14 May 2006, Junio C Hamano wrote:
I am not either after seeing your numbers and trying them
myself. I was going to look at it myself over the weekend, but
I ended up spending most of the time migrating my environment,
so no progress on that front yet. Sorry.
I was looking at it, and I just suspect that "grep" is very optimized.
At the same time, the built-in one had many good features, notably that
you could grep the cached copy and from a specific version. So I think it
makes sense.
I have this suspicion that the best solution is to just handle the "grep
against current tree" separately, and even make that something that only
gets enabled on UNIX - I assume that trying to execve() grep externally
would suck on windows again.
So I would actually assume that the solution is to simply make
grep_cache() have a simple
#ifdef __unix__
if (!cached) {
hit = external_grep(opt, paths, cached);
if (hit >= 0)
return hit;
}
#endif
at the top, so that we'd have the best of both worlds.
The "external_grep()" should look something like this:
#define MAXARGS 1000
static int external_grep(struct grep_opt *opt, const char **paths, int cached)
{
int nr;
char *argv[MAXARGS];
read_cache();
argv[0] = "grep";
argv[1] = "-e";
argv[2] = opt->pattern; /* whatever */
argv[3] = "--";
argc = 4;
for (nr = 0; nr < active_nr; nr++) {
struct cache_entry *ce = active_cache[nr];
if (ce_stage(ce) || !S_ISREG(ntohl(ce->ce_mode)))
continue;
if (!pathspec_matches(paths, ce->name))
continue;
argv[argc++] = ce->name;
if (argc < MAXARGS)
continue;
hit += exec_grep(argv, argc);
argc = 4;
}
if (argc > 4)
hit += exec_grep(argv, argc);
return hit;
}
and you're all done. Except for testing, and fixing my stupid bugs, which
I'm too lazy to do.
The whole "exec_grep()" should basically be the same as "spawn_prog()".
You get the idea.
Anybody?
Linus
On Sun, 14 May 2006, Linus Torvalds wrote:
So I would actually assume that the solution is to simply make
grep_cache() have a simple
#ifdef __unix__
if (!cached) {
hit = external_grep(opt, paths, cached);
if (hit >= 0)
return hit;
}
#endif
at the top, so that we'd have the best of both worlds.
Ok. Here's a slightly tested version, my out-lined thing from the email
was pretty close to working already.
It's not perfect, but it gets the "git grep some-random-string" down to
the good old half-a-second range for the kernel.
It should convert more of the argument flags for "grep", that should be
trivial to expand (I did a few just as an example). It should also bother
to try to return the right "hit" value (which it doesn't, right now - the
code is kind of there, but I didn't actually bother to do it _right_).
Also, right now it _just_ limits by number of arguments, but it should
also strictly speaking limit by total argument size (ie add up the length
of the filenames, and do the "exec_grep()" flush call if it's bigger than
some random value like 32kB).
But I think that it's _conceptually_ doing all the right things, and it
seems to work. So maybe somebody else can do some of the final polish.
Linus
----
diff --git a/builtin-grep.c b/builtin-grep.c
index fead356..14471db 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -12,6 +12,7 @@ #include "tree-walk.h"
#include "builtin.h"
#include <regex.h>
#include <fnmatch.h>
+#include <sys/wait.h>
/*
* git grep pathspecs are somewhat different from diff-tree pathspecs;
@@ -409,12 +410,90 @@ static int grep_file(struct grep_opt *op
return i;
}
+static int exec_grep(int argc, const char **argv)
+{
+ pid_t pid;
+ int status;
+
+ argv[argc] = NULL;
+ pid = fork();
+ if (pid < 0)
+ return pid;
+ if (!pid) {
+ execvp("grep", (char **) argv);
+ exit(255);
+ }
+ while (waitpid(pid, &status, 0) < 0) {
+ if (errno == EINTR)
+ continue;
+ return -1;
+ }
+ if (WIFEXITED(status)) {
+ if (!WEXITSTATUS(status))
+ return 1;
+ return 0;
+ }
+ return -1;
+}
+
+#define MAXARGS 1000
+
+static int external_grep(struct grep_opt *opt, const char **paths, int cached)
+{
+ int i, nr, argc, hit;
+ const char *argv[MAXARGS+1];
+ struct grep_pat *p;
+
+ nr = 0;
+ argv[nr++] = "grep";
+ if (opt->word_regexp)
+ argv[nr++] = "-w";
+ if (opt->name_only)
+ argv[nr++] = "-l";
+ for (p = opt->pattern_list; p; p = p->next) {
+ argv[nr++] = "-e";
+ argv[nr++] = p->pattern;
+ }
+ argv[nr++] = "--";
+
+ hit = 0;
+ argc = nr;
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ if (ce_stage(ce) || !S_ISREG(ntohl(ce->ce_mode)))
+ continue;
+ if (!pathspec_matches(paths, ce->name))
+ continue;
+ argv[argc++] = ce->name;
+ if (argc < MAXARGS)
+ continue;
+ hit += exec_grep(argc, argv);
+ argc = nr;
+ }
+ if (argc > nr)
+ hit += exec_grep(argc, argv);
+ return 0;
+}
+
static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
{
int hit = 0;
int nr;
read_cache();
+#ifdef __unix__
+ /*
+ * Use the external "grep" command for the case where
+ * we grep through the checked-out files. It tends to
+ * be a lot more optimized
+ */
+ if (!cached) {
+ hit = external_grep(opt, paths, cached);
+ if (hit >= 0)
+ return hit;
+ }
+#endif
+
for (nr = 0; nr < active_nr; nr++) {
struct cache_entry *ce = active_cache[nr];
if (ce_stage(ce) || !S_ISREG(ntohl(ce->ce_mode)))