* [RFC/PATCHv2 1/5] worktree: provide better prefix to go back to original cwd
2010-10-15 23:26 [RGC/PATCHv2] grep: submodule support Chris Packham
@ 2010-10-15 23:26 ` Chris Packham
2010-10-16 18:42 ` Jonathan Nieder
2010-10-15 23:26 ` [RFC/PATCHv2 2/5] grep: output the path from cwd to worktree Chris Packham
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Chris Packham @ 2010-10-15 23:26 UTC (permalink / raw)
To: git; +Cc: Jens.Lehmann, pclouds, gitster
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
When both GIT_DIR and GIT_WORK_TREE are set, if cwd is outside worktree,
prefix (the one passed to every builtin commands) will be set to NULL,
which means "user stays at worktree topdir".
As a consequence, command line arguments are supposed to be relative
to worktree topdir, not current working directory. Not very intuitive.
Moreover, output from such situation is (again) relative to worktree
topdir. Users are expected to understand that.
This patch allows builtin commands access to original cwd even if it's
outside worktree, via cwd_to_worktree and worktree_to_cwd fields. As
the name implies, if you stay at original cwd, "cd $(cwd_to_worktree)"
would take you to worktree topdir and vice versa.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/rev-parse.c | 10 ++++
cache.h | 2 +
setup.c | 108 ++++++++++++++++++++++++++++++++++++++++++--
t/t1510-worktree-prefix.sh | 52 +++++++++++++++++++++
4 files changed, 168 insertions(+), 4 deletions(-)
create mode 100755 t/t1510-worktree-prefix.sh
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index a5a1c86..525610e 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -623,6 +623,16 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
puts(prefix);
continue;
}
+ if (!strcmp(arg, "--cwd-to-worktree")) {
+ if (startup_info->cwd_to_worktree)
+ puts(startup_info->cwd_to_worktree);
+ continue;
+ }
+ if (!strcmp(arg, "--worktree-to-cwd")) {
+ if (startup_info->worktree_to_cwd)
+ puts(startup_info->worktree_to_cwd);
+ continue;
+ }
if (!strcmp(arg, "--show-cdup")) {
const char *pfx = prefix;
if (!is_inside_work_tree()) {
diff --git a/cache.h b/cache.h
index 33decd9..c001272 100644
--- a/cache.h
+++ b/cache.h
@@ -1117,6 +1117,8 @@ const char *split_cmdline_strerror(int cmdline_errno);
/* git.c */
struct startup_info {
int have_repository;
+ char *cwd_to_worktree; /* chdir("this"); from cwd would return to worktree */
+ char *worktree_to_cwd; /* chdir("this"); from worktree would return to cwd */
};
extern struct startup_info *startup_info;
diff --git a/setup.c b/setup.c
index a3b76de..413703b 100644
--- a/setup.c
+++ b/setup.c
@@ -313,10 +313,109 @@ const char *read_gitfile_gently(const char *path)
return path;
}
+/*
+ * Given "foo/bar" and "hey/hello/world", return "../../hey/hello/world/"
+ * Either path1 or path2 can be NULL
+ */
+static char *make_path_to_path(const char *path1, const char *path2)
+{
+ int nr_back = 0;
+ int i, pathlen = path2 ? strlen(path2) : 0;
+ char *buf, *p;
+
+ if (path1 && *path1) {
+ nr_back = 1;
+ while (*path1) {
+ if (*path1 == '/')
+ nr_back++;
+ path1++;
+ }
+ }
+
+ if (!nr_back && !pathlen)
+ return NULL;
+
+ p = buf = xmalloc(3*nr_back + pathlen + 2); /* "../"^nr_back + path2 + '/' + NULL */
+ for (i = 0; i < nr_back; i++) {
+ memcpy(p, "../", 3);
+ p += 3;
+ }
+ if (pathlen) {
+ memcpy(p, path2, pathlen);
+ p += pathlen;
+ *p++ = '/';
+ }
+ *p = '\0';
+ return buf;
+}
+
+/*
+ * Return a prefix if cwd inside worktree, or NULL otherwise.
+ * Also fill startup_info struct.
+ */
+static const char *setup_prefix(const char *cwd)
+{
+ const char *worktree = get_git_work_tree();
+ int len = 0, cwd_len = strlen(cwd), worktree_len = strlen(worktree);
+
+ while (worktree[len] && worktree[len] == cwd[len])
+ len++;
+
+ if (!worktree[len] && !cwd[len]) {
+ if (startup_info) {
+ startup_info->cwd_to_worktree = NULL;
+ startup_info->worktree_to_cwd = NULL;
+ }
+ return NULL;
+ }
+ /* get /foo/, not /foo/baa if /foo/baa1 and /foo/baa2 are given */
+ else if (worktree[len] && cwd[len]) {
+ while (len && worktree[len] != '/')
+ len--;
+ len++;
+ }
+ else {
+ if (worktree[len]) {
+ if (worktree[len] != '/') {
+ while (len && worktree[len] != '/')
+ len--;
+ }
+ }
+ else {
+ if (cwd[len] != '/') {
+ while (len && cwd[len] != '/')
+ len--;
+ }
+ }
+ len++; /* must be a slash here, skip it */
+ }
+
+ if (len < cwd_len && len < worktree_len) {
+ if (startup_info) {
+ startup_info->cwd_to_worktree = make_path_to_path(cwd+len, worktree+len);
+ startup_info->worktree_to_cwd = make_path_to_path(worktree+len, cwd+len);
+ }
+ return NULL;
+ }
+
+ if (startup_info) {
+ if (len < cwd_len) { /* cwd inside worktree */
+ startup_info->cwd_to_worktree = make_path_to_path(cwd+len, NULL);
+ startup_info->worktree_to_cwd = make_path_to_path(NULL, cwd+len);
+ }
+ else {
+ startup_info->cwd_to_worktree = make_path_to_path(NULL, worktree+len);
+ startup_info->worktree_to_cwd = make_path_to_path(worktree+len, NULL);
+ }
+ }
+
+ return len < cwd_len ? cwd+len : NULL;
+}
+
static const char *setup_explicit_git_dir(const char *gitdirenv,
const char *work_tree_env, int *nongit_ok)
{
- static char buffer[1024 + 1];
+ static char buffer[PATH_MAX];
const char *retval;
if (PATH_MAX - 40 < strlen(gitdirenv))
@@ -337,9 +436,10 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
}
if (check_repository_format_gently(nongit_ok))
return NULL;
- retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
- get_git_work_tree());
- if (!retval || !*retval)
+ if (!getcwd(buffer, sizeof(buffer)))
+ die_errno("can't find the current directory");
+ retval = setup_prefix(buffer);
+ if (!retval)
return NULL;
set_git_dir(make_absolute_path(gitdirenv));
if (chdir(work_tree_env) < 0)
diff --git a/t/t1510-worktree-prefix.sh b/t/t1510-worktree-prefix.sh
new file mode 100755
index 0000000..3839493
--- /dev/null
+++ b/t/t1510-worktree-prefix.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+test_description='test rev-parse --cwd-to-worktree and --worktree-to-cwd'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ mkdir foo bar &&
+ mv .git foo &&
+ mkdir foo/bar &&
+ GIT_DIR=`pwd`/foo/.git &&
+ GIT_WORK_TREE=`pwd`/foo &&
+ export GIT_DIR GIT_WORK_TREE
+'
+
+test_expect_success 'at root' '
+ (
+ cd foo &&
+ git rev-parse --cwd-to-worktree --worktree-to-cwd >result &&
+ : >expected &&
+ test_cmp expected result
+ )
+'
+
+test_expect_success 'cwd inside worktree' '
+ (
+ cd foo/bar &&
+ git rev-parse --cwd-to-worktree --worktree-to-cwd >result &&
+ echo ../ >expected &&
+ echo bar/ >>expected &&
+ test_cmp expected result
+ )
+'
+
+test_expect_success 'cwd outside worktree' '
+ git rev-parse --cwd-to-worktree --worktree-to-cwd >result &&
+ echo foo/ >expected &&
+ echo ../ >>expected &&
+ test_cmp expected result
+'
+
+test_expect_success 'cwd outside worktree (2)' '
+ (
+ cd bar &&
+ git rev-parse --cwd-to-worktree --worktree-to-cwd >result &&
+ echo ../foo/ >expected &&
+ echo ../bar/ >>expected &&
+ test_cmp expected result
+ )
+'
+
+test_done
--
1.7.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [RFC/PATCHv2 1/5] worktree: provide better prefix to go back to original cwd
2010-10-15 23:26 ` [RFC/PATCHv2 1/5] worktree: provide better prefix to go back to original cwd Chris Packham
@ 2010-10-16 18:42 ` Jonathan Nieder
2010-10-17 2:48 ` Chris Packham
0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Nieder @ 2010-10-16 18:42 UTC (permalink / raw)
To: Chris Packham; +Cc: git, Jens.Lehmann, pclouds, gitster
Hi,
Chris Packham wrote:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> When both GIT_DIR and GIT_WORK_TREE are set, if cwd is outside worktree,
> prefix (the one passed to every builtin commands) will be set to NULL,
> which means "user stays at worktree topdir".
>
> As a consequence, command line arguments are supposed to be relative
> to worktree topdir, not current working directory. Not very intuitive.
Thanks. More detailed history for this patch:
- v0: http://thread.gmane.org/gmane.comp.version-control.git/157599/focus=157601
- v1: http://thread.gmane.org/gmane.comp.version-control.git/158287
- v2: http://thread.gmane.org/gmane.comp.version-control.git/158369
Any thoughts about the previous questions?
> --- a/cache.h
> +++ b/cache.h
> @@ -1117,6 +1117,8 @@ const char *split_cmdline_strerror(int cmdline_errno);
> /* git.c */
> struct startup_info {
> int have_repository;
> + char *cwd_to_worktree; /* chdir("this"); from cwd would return to worktree */
> + char *worktree_to_cwd; /* chdir("this"); from worktree would return to cwd */
e.g. I still find these comments hard to understand.
> --- a/setup.c
> +++ b/setup.c
> @@ -313,10 +313,109 @@ const char *read_gitfile_gently(const char *path)
> return path;
> }
>
> +/*
> + * Given "foo/bar" and "hey/hello/world", return "../../hey/hello/world/"
> + * Either path1 or path2 can be NULL
> + */
> +static char *make_path_to_path(const char *path1, const char *path2)
> +{
> + int nr_back = 0;
> + int i, pathlen = path2 ? strlen(path2) : 0;
> + char *buf, *p;
> +
> + if (path1 && *path1) {
> + nr_back = 1;
> + while (*path1) {
> + if (*path1 == '/')
> + nr_back++;
This still assumes Unix-style path separators. Is that okay? (The
answer could be yes; it just seems useful to document the assumptions...)
> +/*
> + * Return a prefix if cwd inside worktree, or NULL otherwise.
> + * Also fill startup_info struct.
> + */
> +static const char *setup_prefix(const char *cwd)
> +{
> + const char *worktree = get_git_work_tree();
> + int len = 0, cwd_len = strlen(cwd), worktree_len = strlen(worktree);
> +
> + while (worktree[len] && worktree[len] == cwd[len])
> + len++;
> +
> + if (!worktree[len] && !cwd[len]) {
> + if (startup_info) {
> + startup_info->cwd_to_worktree = NULL;
> + startup_info->worktree_to_cwd = NULL;
> + }
> + return NULL;
> + }
> + /* get /foo/, not /foo/baa if /foo/baa1 and /foo/baa2 are given */
> + else if (worktree[len] && cwd[len]) {
Style: use cuddled braces.
} else if (worktree[len] && cwd[len]) {
/*
* The result should be /foo/, not /foo/baa, when
* worktree is /foo/baa1 and cwd is /foo/baa2.
*/
> + while (len && worktree[len] != '/')
> + len--;
> + len++;
> + }
> + else {
Likewise.
> + if (worktree[len]) {
> + if (worktree[len] != '/') {
> + while (len && worktree[len] != '/')
> + len--;
> + }
> + }
> + else {
Likewise.
> + if (cwd[len] != '/') {
> + while (len && cwd[len] != '/')
> + len--;
> + }
This is repetitive. Why is the if necessary?
[...]
> static const char *setup_explicit_git_dir(const char *gitdirenv,
> const char *work_tree_env, int *nongit_ok)
> {
> - static char buffer[1024 + 1];
> + static char buffer[PATH_MAX];
Unexplained.
[...]
> --- /dev/null
> +++ b/t/t1510-worktree-prefix.sh
> @@ -0,0 +1,52 @@
[...]
> +test_expect_success 'at root' '
> + (
> + cd foo &&
> + git rev-parse --cwd-to-worktree --worktree-to-cwd >result &&
> + : >expected &&
> + test_cmp expected result
> + )
> +'
It is clearer where a subshell begins and ends if it is indented. The
usual style in git shell scripts is to omit the ":" in
>empty
commands. So maybe:
(
cd foo &&
git rev-parse --cwd-to-worktree --worktree-to-cwd >result
) &&
>expected &&
test_cmp expected foo/result
Sorry, a bit grumpy today. Still, hope that helps,
Jonathan
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [RFC/PATCHv2 1/5] worktree: provide better prefix to go back to original cwd
2010-10-16 18:42 ` Jonathan Nieder
@ 2010-10-17 2:48 ` Chris Packham
2010-10-17 10:01 ` Nguyen Thai Ngoc Duy
0 siblings, 1 reply; 15+ messages in thread
From: Chris Packham @ 2010-10-17 2:48 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jens.Lehmann, pclouds, gitster
On 16/10/10 11:42, Jonathan Nieder wrote:
> Hi,
>
> Chris Packham wrote:
>
>> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>
>> When both GIT_DIR and GIT_WORK_TREE are set, if cwd is outside worktree,
>> prefix (the one passed to every builtin commands) will be set to NULL,
>> which means "user stays at worktree topdir".
>>
>> As a consequence, command line arguments are supposed to be relative
>> to worktree topdir, not current working directory. Not very intuitive.
>
> Thanks. More detailed history for this patch:
>
> - v0: http://thread.gmane.org/gmane.comp.version-control.git/157599/focus=157601
> - v1: http://thread.gmane.org/gmane.comp.version-control.git/158287
> - v2: http://thread.gmane.org/gmane.comp.version-control.git/158369
>
I think I must have missed v2. I was playing around with my gmail
filters around that time so I could have missed them. Actually now I've
found the message it's missing the last 'm' in gmail.com. I'll grab the
latest patch and give it a test when I get a chance.
> Any thoughts about the previous questions?
>
I haven't caught up on the newest thread so no great revelations. Except
that for the grep submodules use-case we can assume that the worktree
will be a subdirectory of the cwd. I don't think we want to limit
ourselves to that one use-case.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCHv2 1/5] worktree: provide better prefix to go back to original cwd
2010-10-17 2:48 ` Chris Packham
@ 2010-10-17 10:01 ` Nguyen Thai Ngoc Duy
2010-10-18 2:05 ` Chris Packham
0 siblings, 1 reply; 15+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-10-17 10:01 UTC (permalink / raw)
To: Chris Packham; +Cc: Jonathan Nieder, git, Jens.Lehmann, gitster
On Sat, Oct 16, 2010 at 07:48:02PM -0700, Chris Packham wrote:
> On 16/10/10 11:42, Jonathan Nieder wrote:
> > Hi,
> >
> > Chris Packham wrote:
> >
> >> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> >>
> >> When both GIT_DIR and GIT_WORK_TREE are set, if cwd is outside worktree,
> >> prefix (the one passed to every builtin commands) will be set to NULL,
> >> which means "user stays at worktree topdir".
> >>
> >> As a consequence, command line arguments are supposed to be relative
> >> to worktree topdir, not current working directory. Not very intuitive.
> >
> > Thanks. More detailed history for this patch:
> >
> > - v0: http://thread.gmane.org/gmane.comp.version-control.git/157599/focus=157601
> > - v1: http://thread.gmane.org/gmane.comp.version-control.git/158287
> > - v2: http://thread.gmane.org/gmane.comp.version-control.git/158369
> >
>
> I think I must have missed v2. I was playing around with my gmail
> filters around that time so I could have missed them. Actually now I've
> found the message it's missing the last 'm' in gmail.com. I'll grab the
> latest patch and give it a test when I get a chance.
I missed the last "m" in your email address. That's why v2 never reached you.
I thought I sent you an email but probably forgot it.
Anyway v2 does not work if worktree and cwd are on different Windows drives.
This on top should fix it:
---8<---
diff --git a/setup.c b/setup.c
index 2389a9e..35d2691 100644
--- a/setup.c
+++ b/setup.c
@@ -371,12 +371,8 @@ static const char *setup_prefix(const char *cwd)
}
/* get /foo/, not /foo/baa if /foo/baa1 and /foo/baa2 are given */
else if (worktree[len] && cwd[len]) {
- while (len && !is_dir_sep(worktree[len]))
- len--;
- len++;
-
/* Worktree and cwd are on different drives? */
- if (len == 3 && has_dos_drive_prefix(cwd)) {
+ if (!len && has_dos_drive_prefix(cwd)) {
if (startup_info) {
/* make_path_to_path will add the trailing slash */
startup_info->cwd_to_worktree = make_path_to_path(NULL, worktree);
@@ -384,6 +380,10 @@ static const char *setup_prefix(const char *cwd)
}
return NULL;
}
+
+ while (len && !is_dir_sep(worktree[len]))
+ len--;
+ len++;
}
else {
if (worktree[len]) {
---8<---
>
> > Any thoughts about the previous questions?
> >
>
> I haven't caught up on the newest thread so no great revelations. Except
> that for the grep submodules use-case we can assume that the worktree
> will be a subdirectory of the cwd. I don't think we want to limit
> ourselves to that one use-case.
While at it, have you thought of support --recursive and
--full-tree [1]? There are issues with --full-tree and prefixes [2],
which is why it is dropped but I think it's a good idea.
--full-tree disregards where you stand and greps in whole repo. In a
repo with submodules, that would mean grep the supermodule and all
submodules regardless where you stand, even if you stand in a
submodule.
[1] http://mid.gmane.org/7vk4xggv27.fsf@alter.siamese.dyndns.org
[2] http://mid.gmane.org/7vskaqptvj.fsf@alter.siamese.dyndns.org
--
Duy
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC/PATCHv2 2/5] grep: output the path from cwd to worktree
2010-10-15 23:26 [RGC/PATCHv2] grep: submodule support Chris Packham
2010-10-15 23:26 ` [RFC/PATCHv2 1/5] worktree: provide better prefix to go back to original cwd Chris Packham
@ 2010-10-15 23:26 ` Chris Packham
2010-10-15 23:26 ` [RFC/PATCHv2 3/5] grep_cache: check pathspec first Chris Packham
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Chris Packham @ 2010-10-15 23:26 UTC (permalink / raw)
To: git; +Cc: Jens.Lehmann, pclouds, gitster, Chris Packham
If we have detected that our worktree is not the cwd.
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
builtin/grep.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index da32f3d..a51eb2c 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -531,6 +531,8 @@ static int grep_file(struct grep_opt *opt, const char *filename)
struct strbuf buf = STRBUF_INIT;
char *name;
+ if (startup_info->cwd_to_worktree)
+ strbuf_addstr(&buf,startup_info->cwd_to_worktree);
if (opt->relative && opt->prefix_length)
quote_path_relative(filename, -1, &buf, opt->prefix);
else
@@ -646,6 +648,8 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
while (tree_entry(tree, &entry)) {
int te_len = tree_entry_len(entry.path, entry.sha1);
pathbuf.len = len;
+ if (startup_info->cwd_to_worktree)
+ strbuf_addstr(&pathbuf,startup_info->cwd_to_worktree);
strbuf_add(&pathbuf, entry.path, te_len);
if (S_ISDIR(entry.mode))
--
1.7.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC/PATCHv2 3/5] grep_cache: check pathspec first
2010-10-15 23:26 [RGC/PATCHv2] grep: submodule support Chris Packham
2010-10-15 23:26 ` [RFC/PATCHv2 1/5] worktree: provide better prefix to go back to original cwd Chris Packham
2010-10-15 23:26 ` [RFC/PATCHv2 2/5] grep: output the path from cwd to worktree Chris Packham
@ 2010-10-15 23:26 ` Chris Packham
2010-10-15 23:26 ` [RFC/PATCHv2 4/5] add test for git grep --recursive Chris Packham
` (2 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Chris Packham @ 2010-10-15 23:26 UTC (permalink / raw)
To: git; +Cc: Jens.Lehmann, pclouds, gitster, Chris Packham
This makes grep_cache consistent with grep_tree.
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
In addition to consistency, moving the pathspec check up allows the
addition of a simple if clause for submodules later.
builtin/grep.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index a51eb2c..251c4e7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -595,10 +595,10 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
for (nr = 0; nr < active_nr; nr++) {
struct cache_entry *ce = active_cache[nr];
- if (!S_ISREG(ce->ce_mode))
- continue;
if (!pathspec_matches(paths, ce->name, opt->max_depth))
continue;
+ if (!S_ISREG(ce->ce_mode))
+ continue;
/*
* If CE_VALID is on, we assume worktree file and its cache entry
* are identical, even if worktree file has been modified, so use
--
1.7.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC/PATCHv2 4/5] add test for git grep --recursive
2010-10-15 23:26 [RGC/PATCHv2] grep: submodule support Chris Packham
` (2 preceding siblings ...)
2010-10-15 23:26 ` [RFC/PATCHv2 3/5] grep_cache: check pathspec first Chris Packham
@ 2010-10-15 23:26 ` Chris Packham
2010-10-15 23:26 ` [RFC/PATCHv2 5/5] grep: add support for grepping in submodules Chris Packham
2010-10-16 15:54 ` [RGC/PATCHv2] grep: submodule support Jens Lehmann
5 siblings, 0 replies; 15+ messages in thread
From: Chris Packham @ 2010-10-15 23:26 UTC (permalink / raw)
To: git; +Cc: Jens.Lehmann, pclouds, gitster, Chris Packham
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
t/t7820-grep-recursive.sh | 183 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 183 insertions(+), 0 deletions(-)
create mode 100644 t/t7820-grep-recursive.sh
diff --git a/t/t7820-grep-recursive.sh b/t/t7820-grep-recursive.sh
new file mode 100644
index 0000000..3c386b4
--- /dev/null
+++ b/t/t7820-grep-recursive.sh
@@ -0,0 +1,183 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Chris Packham
+#
+
+test_description='git grep --recursive test
+
+This test checks the ability of git grep to search within submodules when told
+to do so with the --recursive option'
+
+. ./test-lib.sh
+
+restore_test_defaults()
+{
+ unset GIT_SUPER_REFNAME
+}
+
+test_expect_success 'setup' '
+ cat >t <<-\EOF &&
+ one two three
+ four five six
+ seven eight nine
+ EOF
+ git add t &&
+ git commit -m "initial commit"
+'
+submodurl=$TRASH_DIRECTORY
+
+test_expect_success 'setup submodules' '
+ for mod in submodule1 submodule2 submodule3 submodule4 submodule5; do
+ git submodule add "$submodurl" $mod &&
+ git submodule init $mod
+ done &&
+ git commit -m "setup submodules for test"
+'
+
+test_expect_success 'update data in each submodule' '
+ for n in 1 2 3 4 5; do
+ (cd submodule$n &&
+ sed -i "s/^four.*/& #$n/" t &&
+ git commit -a -m"update") &&
+ git add submodule$n
+ done &&
+ git commit -m "update data in each submodule"
+'
+
+test_expect_success 'non-recursive grep in base' '
+ cat >expected <<-\EOF &&
+ t:four five six
+ EOF
+ git grep "five" >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'submodule-ref option' '
+ cat >expected <<-\EOF &&
+ bar:t:four five six
+ EOF
+ GIT_SUPER_REFNAME=bar &&
+ export GIT_SUPER_REFNAME &&
+ git grep "five" master >actual &&
+ test_cmp expected actual &&
+ restore_test_defaults
+'
+
+test_expect_success 'non-recursive grep in submodule' '
+ (
+ cd submodule1 &&
+ cat >expected <<-\EOF &&
+ t:four five six #1
+ EOF
+ git grep "five" >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success 'recursive grep' '
+ cat >expected <<-\EOF &&
+ submodule1/t:four five six #1
+ submodule2/t:four five six #2
+ submodule3/t:four five six #3
+ submodule4/t:four five six #4
+ submodule5/t:four five six #5
+ t:four five six
+ EOF
+ git grep --recursive "five" >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'recursive grep (with -n)' '
+ cat >expected <<-\EOF &&
+ submodule1/t:2:four five six #1
+ submodule2/t:2:four five six #2
+ submodule3/t:2:four five six #3
+ submodule4/t:2:four five six #4
+ submodule5/t:2:four five six #5
+ t:2:four five six
+ EOF
+ git grep --recursive -n "five" >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'recursive grep (with -l)' '
+ cat >expected <<-\EOF &&
+ submodule1/t
+ submodule2/t
+ submodule3/t
+ submodule4/t
+ submodule5/t
+ t
+ EOF
+ git grep --recursive -l "five" >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'recursive grep (with --or)' '
+ cat >expected <<-\EOF &&
+ submodule1/t:one two three
+ submodule1/t:four five six #1
+ submodule2/t:one two three
+ submodule2/t:four five six #2
+ submodule3/t:one two three
+ submodule3/t:four five six #3
+ submodule4/t:one two three
+ submodule4/t:four five six #4
+ submodule5/t:one two three
+ submodule5/t:four five six #5
+ t:one two three
+ t:four five six
+ EOF
+ git grep --recursive \( -e "five" --or -e "two" \) >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'recursive grep (with --and --not)' '
+ cat >expected <<-\EOF &&
+ submodule2/t:four five six #2
+ submodule3/t:four five six #3
+ submodule4/t:four five six #4
+ submodule5/t:four five six #5
+ t:four five six
+ EOF
+ git grep --recursive \( -e "five" --and --not -e "#1" \) >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'recursive grep with refspec' '
+ cat >expected <<-\EOF &&
+ master:submodule1/t:four five six #1
+ master:submodule2/t:four five six #2
+ master:submodule3/t:four five six #3
+ master:submodule4/t:four five six #4
+ master:submodule5/t:four five six #5
+ master:t:four five six
+ EOF
+ git grep --recursive five master >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'recursive grep with pathspec' '
+ cat >expected <<-\EOF &&
+ submodule2/t:four five six #2
+ EOF
+ git grep --recursive five -- submodule2 >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'recursive grep with pathspec and refspec' '
+ cat >expected <<-\EOF &&
+ master:submodule2/t:four five six #2
+ EOF
+ git grep --recursive five master -- submodule2 >actual &&
+ test_cmp expected actual
+'
+
+test_expect_failure 'recursive grep with --max-depth' '
+ cat >expected <<-\EOF &&
+ t:four five six
+ EOF
+ git grep --recursive --max-depth=1 five >actual &&
+ test_cmp expected actual
+'
+test_done
--
1.7.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC/PATCHv2 5/5] grep: add support for grepping in submodules
2010-10-15 23:26 [RGC/PATCHv2] grep: submodule support Chris Packham
` (3 preceding siblings ...)
2010-10-15 23:26 ` [RFC/PATCHv2 4/5] add test for git grep --recursive Chris Packham
@ 2010-10-15 23:26 ` Chris Packham
2010-10-17 10:28 ` Nguyen Thai Ngoc Duy
2010-10-16 15:54 ` [RGC/PATCHv2] grep: submodule support Jens Lehmann
5 siblings, 1 reply; 15+ messages in thread
From: Chris Packham @ 2010-10-15 23:26 UTC (permalink / raw)
To: git; +Cc: Jens.Lehmann, pclouds, gitster, Chris Packham
When the --recurse-submodules option is given git grep will search in
submodules as they are encountered.
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
The refspec from the super project is passed as an environment variable
along with the GIT_DIR and GIT_WORK_TREE. We re-build a command line for
the sub-process grep which works for the basic cases but I need to add
some more complex tests to ensure everything gets passed through
correctly.
At the moment I don't alter the pathspec or max-depth option but I'll do
so in a future re-roll.
builtin/grep.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
grep.h | 1 +
2 files changed, 102 insertions(+), 2 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 251c4e7..7bcdf05 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -587,6 +587,91 @@ static void run_pager(struct grep_opt *opt, const char *prefix)
free(argv);
}
+static int pattern_list_to_argv(struct grep_opt *opt, const char **argv, int len)
+{
+ int i = 0;
+ struct grep_pat *p = opt->pattern_list;
+ while(p) {
+ if (i > len)
+ die("grep: not enough space for subprocess args");
+ if (p->token == GREP_PATTERN)
+ argv[i++] = "-e";
+ argv[i++] = p->pattern;
+ p = p->next;
+ }
+ return i;
+}
+
+static const char **create_sub_grep_argv(struct grep_opt *opt,
+ const char *path, const char *sha1, const char *tree_name)
+{
+ #define NUM_ARGS 10
+ struct strbuf buf = STRBUF_INIT;
+ const char **argv;
+ int i = 0;
+
+ argv = xcalloc(NUM_ARGS, sizeof(const char *));
+ argv[i++] = "grep";
+
+ if (opt->linenum)
+ argv[i++] = "-n";
+ if (opt->invert)
+ argv[i++] = "-v";
+ if (opt->ignore_case)
+ argv[i++] = "-i";
+ if (opt->count)
+ argv[i++] = "-c";
+ if (opt->name_only)
+ argv[i++] = "-l";
+ if (opt->recurse_submodules)
+ argv[i++] = "--recursive";
+
+ i += pattern_list_to_argv(opt, &argv[i], NUM_ARGS-(i+1));
+ if (sha1) {
+ argv[i++] = sha1;
+ }
+ argv[i++] = NULL;
+
+ strbuf_release(&buf);
+ return argv;
+}
+
+static int grep_submodule(struct grep_opt *opt, const char *path,
+ const char *sha1, const char *tree_name)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct strbuf pre_buf = STRBUF_INIT;
+ struct child_process cp;
+ const char **argv = create_sub_grep_argv(opt, path, sha1, tree_name);
+ const char *git_dir;
+ int hit = 0;
+ memset(&cp, 0, sizeof(cp));
+
+ strbuf_addf(&buf, "%s/.git", path);
+ git_dir = read_gitfile_gently(buf.buf);
+ if (!git_dir)
+ git_dir = buf.buf;
+ if (!is_directory(git_dir))
+ goto out_free;
+
+ setenv("GIT_SUPER_REFNAME", tree_name, 1);
+ setenv(GIT_DIR_ENVIRONMENT, git_dir, 1);
+ setenv(GIT_WORK_TREE_ENVIRONMENT, path, 1);
+ cp.argv = argv;
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ if (run_command(&cp) == 0)
+ hit = 1;
+out_free:
+ unsetenv("GIT_SUPER_REFNAME");
+ unsetenv(GIT_DIR_ENVIRONMENT);
+ unsetenv(GIT_WORK_TREE_ENVIRONMENT);
+ free(argv);
+ strbuf_release(&buf);
+ strbuf_release(&pre_buf);
+ return hit;
+}
+
static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
{
int hit = 0;
@@ -597,6 +682,10 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
struct cache_entry *ce = active_cache[nr];
if (!pathspec_matches(paths, ce->name, opt->max_depth))
continue;
+ if (S_ISGITLINK(ce->ce_mode) && opt->recurse_submodules) {
+ hit |= grep_submodule(opt, ce->name, NULL, NULL);
+ continue;
+ }
if (!S_ISREG(ce->ce_mode))
continue;
/*
@@ -634,11 +723,16 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
char *down;
int tn_len = strlen(tree_name);
struct strbuf pathbuf;
+ const char *refname = getenv("GIT_SUPER_REFNAME");
+ int rn_len = refname ? strlen(refname) : 0;
- strbuf_init(&pathbuf, PATH_MAX + tn_len);
+ strbuf_init(&pathbuf, PATH_MAX + MAX(tn_len, rn_len));
if (tn_len) {
- strbuf_add(&pathbuf, tree_name, tn_len);
+ if (refname)
+ strbuf_add(&pathbuf, refname, rn_len);
+ else
+ strbuf_add(&pathbuf, tree_name, tn_len);
strbuf_addch(&pathbuf, ':');
tn_len = pathbuf.len;
}
@@ -664,6 +758,9 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
;
else if (S_ISREG(entry.mode))
hit |= grep_sha1(opt, entry.sha1, pathbuf.buf, tn_len);
+ else if (S_ISGITLINK(entry.mode) && opt->recurse_submodules)
+ hit |= grep_submodule(opt, entry.path,
+ sha1_to_hex(entry.sha1), tree_name);
else if (S_ISDIR(entry.mode)) {
enum object_type type;
struct tree_desc sub;
@@ -931,6 +1028,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
"allow calling of grep(1) (ignored by this build)"),
{ OPTION_CALLBACK, 0, "help-all", &options, NULL, "show usage",
PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
+ OPT_BOOLEAN(0, "recursive", &opt.recurse_submodules,
+ "recurse into submodules"),
OPT_END()
};
diff --git a/grep.h b/grep.h
index 06621fe..d5e2e11 100644
--- a/grep.h
+++ b/grep.h
@@ -101,6 +101,7 @@ struct grep_opt {
unsigned post_context;
unsigned last_shown;
int show_hunk_mark;
+ int recurse_submodules;
void *priv;
void (*output)(struct grep_opt *opt, const void *data, size_t size);
--
1.7.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [RFC/PATCHv2 5/5] grep: add support for grepping in submodules
2010-10-15 23:26 ` [RFC/PATCHv2 5/5] grep: add support for grepping in submodules Chris Packham
@ 2010-10-17 10:28 ` Nguyen Thai Ngoc Duy
2010-10-18 2:01 ` Chris Packham
0 siblings, 1 reply; 15+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-10-17 10:28 UTC (permalink / raw)
To: Chris Packham; +Cc: git, Jens.Lehmann, gitster
On Sat, Oct 16, 2010 at 6:26 AM, Chris Packham <judge.packham@gmail.com> wrote:
> +static int grep_submodule(struct grep_opt *opt, const char *path,
> + const char *sha1, const char *tree_name)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + struct strbuf pre_buf = STRBUF_INIT;
> + struct child_process cp;
> + const char **argv = create_sub_grep_argv(opt, path, sha1, tree_name);
Can we just save sha1 in a env variable and drop this argv rewrite?
> + const char *git_dir;
> + int hit = 0;
> + memset(&cp, 0, sizeof(cp));
> +
> + strbuf_addf(&buf, "%s/.git", path);
> + git_dir = read_gitfile_gently(buf.buf);
> + if (!git_dir)
> + git_dir = buf.buf;
> + if (!is_directory(git_dir))
> + goto out_free;
> +
> + setenv("GIT_SUPER_REFNAME", tree_name, 1);
> + setenv(GIT_DIR_ENVIRONMENT, git_dir, 1);
> + setenv(GIT_WORK_TREE_ENVIRONMENT, path, 1);
cp.env can be used to set these variables
> + cp.argv = argv;
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
I think you stll need "cp.dir = path;" here because the setup routines
won't do that for you. But I need to check/test that.
> + if (run_command(&cp) == 0)
> + hit = 1;
> +out_free:
> + unsetenv("GIT_SUPER_REFNAME");
> + unsetenv(GIT_DIR_ENVIRONMENT);
> + unsetenv(GIT_WORK_TREE_ENVIRONMENT);
> + free(argv);
> + strbuf_release(&buf);
> + strbuf_release(&pre_buf);
> + return hit;
> +}
> +
> static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
> {
> int hit = 0;
> @@ -597,6 +682,10 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
> struct cache_entry *ce = active_cache[nr];
> if (!pathspec_matches(paths, ce->name, opt->max_depth))
> continue;
> + if (S_ISGITLINK(ce->ce_mode) && opt->recurse_submodules) {
> + hit |= grep_submodule(opt, ce->name, NULL, NULL);
> + continue;
> + }
> if (!S_ISREG(ce->ce_mode))
> continue;
> /*
> @@ -634,11 +723,16 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
> char *down;
> int tn_len = strlen(tree_name);
> struct strbuf pathbuf;
> + const char *refname = getenv("GIT_SUPER_REFNAME");
> + int rn_len = refname ? strlen(refname) : 0;
>
> - strbuf_init(&pathbuf, PATH_MAX + tn_len);
> + strbuf_init(&pathbuf, PATH_MAX + MAX(tn_len, rn_len));
>
> if (tn_len) {
> - strbuf_add(&pathbuf, tree_name, tn_len);
> + if (refname)
> + strbuf_add(&pathbuf, refname, rn_len);
> + else
> + strbuf_add(&pathbuf, tree_name, tn_len);
> strbuf_addch(&pathbuf, ':');
> tn_len = pathbuf.len;
> }
> @@ -664,6 +758,9 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
> ;
> else if (S_ISREG(entry.mode))
> hit |= grep_sha1(opt, entry.sha1, pathbuf.buf, tn_len);
> + else if (S_ISGITLINK(entry.mode) && opt->recurse_submodules)
> + hit |= grep_submodule(opt, entry.path,
> + sha1_to_hex(entry.sha1), tree_name);
> else if (S_ISDIR(entry.mode)) {
> enum object_type type;
> struct tree_desc sub;
> @@ -931,6 +1028,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> "allow calling of grep(1) (ignored by this build)"),
> { OPTION_CALLBACK, 0, "help-all", &options, NULL, "show usage",
> PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
> + OPT_BOOLEAN(0, "recursive", &opt.recurse_submodules,
> + "recurse into submodules"),
> OPT_END()
> };
>
> diff --git a/grep.h b/grep.h
> index 06621fe..d5e2e11 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -101,6 +101,7 @@ struct grep_opt {
> unsigned post_context;
> unsigned last_shown;
> int show_hunk_mark;
> + int recurse_submodules;
> void *priv;
>
> void (*output)(struct grep_opt *opt, const void *data, size_t size);
> --
> 1.7.3.1
>
>
--
Duy
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [RFC/PATCHv2 5/5] grep: add support for grepping in submodules
2010-10-17 10:28 ` Nguyen Thai Ngoc Duy
@ 2010-10-18 2:01 ` Chris Packham
2010-10-18 3:37 ` Nguyen Thai Ngoc Duy
0 siblings, 1 reply; 15+ messages in thread
From: Chris Packham @ 2010-10-18 2:01 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jens.Lehmann, gitster
On 17/10/10 03:28, Nguyen Thai Ngoc Duy wrote:
> On Sat, Oct 16, 2010 at 6:26 AM, Chris Packham <judge.packham@gmail.com> wrote:
>> +static int grep_submodule(struct grep_opt *opt, const char *path,
>> + const char *sha1, const char *tree_name)
>> +{
>> + struct strbuf buf = STRBUF_INIT;
>> + struct strbuf pre_buf = STRBUF_INIT;
>> + struct child_process cp;
>> + const char **argv = create_sub_grep_argv(opt, path, sha1, tree_name);
>
> Can we just save sha1 in a env variable and drop this argv rewrite?
>
The tree_name is a hangover from when I was passing it as a command line
arg, I'll remove it. For now we still need to rewrite argv to modify the
pathspec and max-depth, eventually I'd like to ditch that approach in
favour of using fork() and modifying opts.
>> + const char *git_dir;
>> + int hit = 0;
>> + memset(&cp, 0, sizeof(cp));
>> +
>> + strbuf_addf(&buf, "%s/.git", path);
>> + git_dir = read_gitfile_gently(buf.buf);
>> + if (!git_dir)
>> + git_dir = buf.buf;
>> + if (!is_directory(git_dir))
>> + goto out_free;
>> +
>> + setenv("GIT_SUPER_REFNAME", tree_name, 1);
>> + setenv(GIT_DIR_ENVIRONMENT, git_dir, 1);
>> + setenv(GIT_WORK_TREE_ENVIRONMENT, path, 1);
>
> cp.env can be used to set these variables
>
I was struggling a little with how to do this. The run_command
invocation in submodules.c uses local_repo_env which is protected
against modification. I also don't understand how the actual values are
passed. If someone could point me in the right direction it'd be
appreciated.
>> + cp.argv = argv;
>> + cp.git_cmd = 1;
>> + cp.no_stdin = 1;
>
> I think you stll need "cp.dir = path;" here because the setup routines
> won't do that for you. But I need to check/test that.
>
Originally I did set cp.dir but that is used by run_command (via
start_command) to chdir. Which foils the cwd to worktree detection (at
least in V1 of that patch).
>> + if (run_command(&cp) == 0)
>> + hit = 1;
>> +out_free:
>> + unsetenv("GIT_SUPER_REFNAME");
>> + unsetenv(GIT_DIR_ENVIRONMENT);
>> + unsetenv(GIT_WORK_TREE_ENVIRONMENT);
>> + free(argv);
>> + strbuf_release(&buf);
>> + strbuf_release(&pre_buf);
>> + return hit;
>> +}
>> +
>> static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
>> {
>> int hit = 0;
>> @@ -597,6 +682,10 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
>> struct cache_entry *ce = active_cache[nr];
>> if (!pathspec_matches(paths, ce->name, opt->max_depth))
>> continue;
>> + if (S_ISGITLINK(ce->ce_mode) && opt->recurse_submodules) {
>> + hit |= grep_submodule(opt, ce->name, NULL, NULL);
>> + continue;
>> + }
>> if (!S_ISREG(ce->ce_mode))
>> continue;
>> /*
>> @@ -634,11 +723,16 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
>> char *down;
>> int tn_len = strlen(tree_name);
>> struct strbuf pathbuf;
>> + const char *refname = getenv("GIT_SUPER_REFNAME");
>> + int rn_len = refname ? strlen(refname) : 0;
>>
>> - strbuf_init(&pathbuf, PATH_MAX + tn_len);
>> + strbuf_init(&pathbuf, PATH_MAX + MAX(tn_len, rn_len));
>>
>> if (tn_len) {
>> - strbuf_add(&pathbuf, tree_name, tn_len);
>> + if (refname)
>> + strbuf_add(&pathbuf, refname, rn_len);
>> + else
>> + strbuf_add(&pathbuf, tree_name, tn_len);
>> strbuf_addch(&pathbuf, ':');
>> tn_len = pathbuf.len;
>> }
>> @@ -664,6 +758,9 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
>> ;
>> else if (S_ISREG(entry.mode))
>> hit |= grep_sha1(opt, entry.sha1, pathbuf.buf, tn_len);
>> + else if (S_ISGITLINK(entry.mode) && opt->recurse_submodules)
>> + hit |= grep_submodule(opt, entry.path,
>> + sha1_to_hex(entry.sha1), tree_name);
>> else if (S_ISDIR(entry.mode)) {
>> enum object_type type;
>> struct tree_desc sub;
>> @@ -931,6 +1028,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>> "allow calling of grep(1) (ignored by this build)"),
>> { OPTION_CALLBACK, 0, "help-all", &options, NULL, "show usage",
>> PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
>> + OPT_BOOLEAN(0, "recursive", &opt.recurse_submodules,
>> + "recurse into submodules"),
>> OPT_END()
>> };
>>
>> diff --git a/grep.h b/grep.h
>> index 06621fe..d5e2e11 100644
>> --- a/grep.h
>> +++ b/grep.h
>> @@ -101,6 +101,7 @@ struct grep_opt {
>> unsigned post_context;
>> unsigned last_shown;
>> int show_hunk_mark;
>> + int recurse_submodules;
>> void *priv;
>>
>> void (*output)(struct grep_opt *opt, const void *data, size_t size);
>> --
>> 1.7.3.1
>>
>>
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [RFC/PATCHv2 5/5] grep: add support for grepping in submodules
2010-10-18 2:01 ` Chris Packham
@ 2010-10-18 3:37 ` Nguyen Thai Ngoc Duy
0 siblings, 0 replies; 15+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-10-18 3:37 UTC (permalink / raw)
To: Chris Packham; +Cc: git, Jens.Lehmann, gitster
On Mon, Oct 18, 2010 at 9:01 AM, Chris Packham <judge.packham@gmail.com> wrote:
> On 17/10/10 03:28, Nguyen Thai Ngoc Duy wrote:
>> On Sat, Oct 16, 2010 at 6:26 AM, Chris Packham <judge.packham@gmail.com> wrote:
>>> +static int grep_submodule(struct grep_opt *opt, const char *path,
>>> + const char *sha1, const char *tree_name)
>>> +{
>>> + struct strbuf buf = STRBUF_INIT;
>>> + struct strbuf pre_buf = STRBUF_INIT;
>>> + struct child_process cp;
>>> + const char **argv = create_sub_grep_argv(opt, path, sha1, tree_name);
>>
>> Can we just save sha1 in a env variable and drop this argv rewrite?
>>
> The tree_name is a hangover from when I was passing it as a command line
> arg, I'll remove it. For now we still need to rewrite argv to modify the
> pathspec and max-depth, eventually I'd like to ditch that approach in
> favour of using fork() and modifying opts.
You would also need to undo git repo setup, i.e. resetting static
variables in environment.c and setup.c, maybe some more. Also fork()
is not available on Windows.
Spawning new processes is the only feasible way I can think of now (or
even in the next year).
>>> + const char *git_dir;
>>> + int hit = 0;
>>> + memset(&cp, 0, sizeof(cp));
>>> +
>>> + strbuf_addf(&buf, "%s/.git", path);
>>> + git_dir = read_gitfile_gently(buf.buf);
>>> + if (!git_dir)
>>> + git_dir = buf.buf;
>>> + if (!is_directory(git_dir))
>>> + goto out_free;
>>> +
>>> + setenv("GIT_SUPER_REFNAME", tree_name, 1);
>>> + setenv(GIT_DIR_ENVIRONMENT, git_dir, 1);
>>> + setenv(GIT_WORK_TREE_ENVIRONMENT, path, 1);
>>
>> cp.env can be used to set these variables
>>
> I was struggling a little with how to do this. The run_command
> invocation in submodules.c uses local_repo_env which is protected
> against modification. I also don't understand how the actual values are
> passed. If someone could point me in the right direction it'd be
> appreciated.
From what I read in run-command.c,the current env var set is copied
as-is for new process, then any new variables from cp.env will be
added. Old variables can also be unset by saying "VARIABLE=" in
cp.env. The final env var set will be used for new process. Current
env vars are untouched. Something like this should work:
const char *env[4];
env[0] = malloc_sprintf("GIT_SUPER_REFNAME=%s", tree_name);
env[1] = malloc_sprintf("GIT_DIR_ENVIRONMENT=%s", git_dir);
env[2] = malloc_sprintf("GIT_WORK_TREE+ENVIRONMENT=%s", path);
env[3] = NULL;
cp.env = env;
>>> + cp.argv = argv;
>>> + cp.git_cmd = 1;
>>> + cp.no_stdin = 1;
>>
>> I think you stll need "cp.dir = path;" here because the setup routines
>> won't do that for you. But I need to check/test that.
>>
> Originally I did set cp.dir but that is used by run_command (via
> start_command) to chdir. Which foils the cwd to worktree detection (at
> least in V1 of that patch).
Right. I forgot that. But then when new grep is run, it should move
back to worktree. I believe current grep code assumes that cwd is
worktree root.
--
Duy
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RGC/PATCHv2] grep: submodule support
2010-10-15 23:26 [RGC/PATCHv2] grep: submodule support Chris Packham
` (4 preceding siblings ...)
2010-10-15 23:26 ` [RFC/PATCHv2 5/5] grep: add support for grepping in submodules Chris Packham
@ 2010-10-16 15:54 ` Jens Lehmann
2010-10-17 2:13 ` Chris Packham
5 siblings, 1 reply; 15+ messages in thread
From: Jens Lehmann @ 2010-10-16 15:54 UTC (permalink / raw)
To: Chris Packham; +Cc: git, pclouds, gitster
Am 16.10.2010 01:26, schrieb Chris Packham:
> Here is my updated grep support for submodules series.
Thanks Chris and Duy, I added this series to my git-submod-enhancements
repo on github and will give it some testing next week.
> Chris Packham (4):
> grep: output the path from cwd to worktree
> grep_cache: check pathspec first
> add test for git grep --recursive
Hmm, is it useful to add tests before adding the feature tested? I
thought it makes more sense to add them in the same commit or after
adding what is tested (e.g. to reduce the noise when bisecting
later). Or am I missing something here?
(And by the way: the test script should be executable)
> grep: add support for grepping in submodules
Nit: The commit message (5/5) talks about the old option name
"--recurse-submodules" instead of the "--recursive" the patch
is adding.
> Nguyễn Thái Ngọc Duy (1):
> worktree: provide better prefix to go back to original cwd
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [RGC/PATCHv2] grep: submodule support
2010-10-16 15:54 ` [RGC/PATCHv2] grep: submodule support Jens Lehmann
@ 2010-10-17 2:13 ` Chris Packham
0 siblings, 0 replies; 15+ messages in thread
From: Chris Packham @ 2010-10-17 2:13 UTC (permalink / raw)
To: Jens Lehmann; +Cc: git, pclouds, gitster
On 16/10/10 08:54, Jens Lehmann wrote:
> Am 16.10.2010 01:26, schrieb Chris Packham:
>> Here is my updated grep support for submodules series.
>
> Thanks Chris and Duy, I added this series to my git-submod-enhancements
> repo on github and will give it some testing next week.
>
>> Chris Packham (4):
>> grep: output the path from cwd to worktree
>> grep_cache: check pathspec first
>> add test for git grep --recursive
>
> Hmm, is it useful to add tests before adding the feature tested? I
> thought it makes more sense to add them in the same commit or after
> adding what is tested (e.g. to reduce the noise when bisecting
> later). Or am I missing something here?
That's just reflects my personal preference to write the tests first
(influenced by a former colleague who was keen on extreme
programming/test driven development). Eventually these can be swapped or
squashed as needed, for now it's probably easier to review them separately.
>
> (And by the way: the test script should be executable)
OK will fix.
>
>> grep: add support for grepping in submodules
>
> Nit: The commit message (5/5) talks about the old option name
> "--recurse-submodules" instead of the "--recursive" the patch
> is adding.
Yeah I just noticed that. I'll update the commit message.
>
>> Nguyễn Thái Ngọc Duy (1):
>> worktree: provide better prefix to go back to original cwd
^ permalink raw reply [flat|nested] 15+ messages in thread