Re: [PATCH 1/3] After chdir to run grep, return to old directory

2014-05-07 Thread Junio C Hamano 
David Turner dtur...@twopensource.com writes:

 This causes my test to pass and generally seems correct to me.

Yes, this approach is very sensible, and I'll queue.

But watchman support _should_ be prepared for a program that does
not do this.  Developing your support in on a codebase with this
patch may be sweeping a bug in your code under the rug.

Thanks both for working on your respective changes ;-).


 On Tue, 2014-05-06 at 23:00 -0400, Jeff King wrote:
 ...
 That being said, this really seems like something that the run-command
 interface should be doing, since it can handle the chdir in the forked
 child. And indeed, it seems to support that.
 
 Maybe:
 
 -- 8 --
 Subject: grep: use run-command's dir option for --open-files-in-pager
 
 Git generally changes directory to the repository root on
 startup.  When running grep --open-files-in-pager from a
 subdirectory, we chdir back to the original directory before
 running the pager, so that we can feed the relative
 pathnames to the pager.
 
 We currently do this chdir manually, but we can ask
 run_command to do it for us. This is fewer lines of code,
 and as a bonus, the chdir is limited to the child process,
 which avoids any unexpected surprises for code running after
 the pager (there isn't any currently, but this is
 future-proofing).
 
 Signed-off-by: Jeff King p...@peff.net
 ---
  builtin/grep.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)
 
 diff --git a/builtin/grep.c b/builtin/grep.c
 index 69ac2d8..43af5b7 100644
 --- a/builtin/grep.c
 +++ b/builtin/grep.c
 @@ -361,9 +361,7 @@ static void run_pager(struct grep_opt *opt, const char 
 *prefix)
  argv[i] = path_list-items[i].string;
  argv[path_list-nr] = NULL;
  
 -if (prefix  chdir(prefix))
 -die(_(Failed to chdir: %s), prefix);
 -status = run_command_v_opt(argv, RUN_USING_SHELL);
 +status = run_command_v_opt_cd_env(argv, RUN_USING_SHELL, prefix, NULL);
  if (status)
  exit(status);
  free(argv);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] After chdir to run grep, return to old directory

2014-05-07 Thread David Turner 
On Wed, 2014-05-07 at 10:42 -0700, Junio C Hamano wrote:
 David Turner dtur...@twopensource.com writes:
 
  This causes my test to pass and generally seems correct to me.
 
 Yes, this approach is very sensible, and I'll queue.
 
 But watchman support _should_ be prepared for a program that does
 not do this.  Developing your support in on a codebase with this
 patch may be sweeping a bug in your code under the rug.

I agree that good defensive coding practice would be to not depend on
the cwd.  That's just what everything in environment.c presently does.
I don't want to change the rest of the get_*_file functions, because I
don't know what their callers expect of them.  Here is my patch to do
change just the fs_cache bit:

https://github.com/dturner-tw/git/commit/3fe93aeaee9719ee171a253c49af5126a057c513.patch

(I went ahead and made it part of 
the watchman branch on https://github.com/dturner-tw/git.git )

I wanted to just do the fixup where the path is used, but I couldn't see
a function for that.  If there's no function, that indicates to me that
it's probably a bad idea.  But maybe I'm just missing it.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] After chdir to run grep, return to old directory

2014-05-06 Thread Junio C Hamano 
dtur...@twopensource.com writes:

 From: David Turner dtur...@twitter.com

 Signed-off-by: David Turner dtur...@twitter.com

Ehh, why?

 ---
  builtin/grep.c | 14 --
  1 file changed, 12 insertions(+), 2 deletions(-)

 diff --git a/builtin/grep.c b/builtin/grep.c
 index 69ac2d8..e9fe040 100644
 --- a/builtin/grep.c
 +++ b/builtin/grep.c
 @@ -355,15 +355,25 @@ 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));
 + static char old_directory[PATH_MAX+1];
   int i, status;
  
   for (i = 0; i  path_list-nr; i++)
   argv[i] = path_list-items[i].string;
   argv[path_list-nr] = NULL;
  
 - if (prefix  chdir(prefix))
 - die(_(Failed to chdir: %s), prefix);
 +
 + if (prefix) {
 + if (!getcwd(old_directory, PATH_MAX+1))
 + die(_(Failed to get cwd: %s), prefix);
 + if (chdir(prefix))
 + die(_(Failed to chdir: %s), prefix);
 + }
   status = run_command_v_opt(argv, RUN_USING_SHELL);
 + if (prefix)
 + if (chdir(old_directory))
 + die(_(Failed to chdir: %s), old_directory);
 +
   if (status)
   exit(status);
   free(argv);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] After chdir to run grep, return to old directory

2014-05-06 Thread David Turner 
On Tue, 2014-05-06 at 15:24 -0700, Junio C Hamano wrote:
 dtur...@twopensource.com writes:
 
  From: David Turner dtur...@twitter.com
 
  Signed-off-by: David Turner dtur...@twitter.com
 
 Ehh, why?

Briefly, because otherwise ./t7811-grep-open.sh fails when run under
watchman.

This is actually something that I think I'm doing wrong, but I can't see
what the sensible way to do it is.  When we go to write the fs_cache (in
an atexit hook), we use get_fs_cache_file() from environment.c.  This is
a relative path, because all of the other similar paths are.  So if we
have chdired, then we fail.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] After chdir to run grep, return to old directory

2014-05-06 Thread Jeff King 
On Tue, May 06, 2014 at 05:06:51PM -0700, David Turner wrote:

 On Tue, 2014-05-06 at 15:24 -0700, Junio C Hamano wrote:
  dtur...@twopensource.com writes:
  
   From: David Turner dtur...@twitter.com
  
   Signed-off-by: David Turner dtur...@twitter.com
  
  Ehh, why?
 
 Briefly, because otherwise ./t7811-grep-open.sh fails when run under
 watchman.
 
 This is actually something that I think I'm doing wrong, but I can't see
 what the sensible way to do it is.  When we go to write the fs_cache (in
 an atexit hook), we use get_fs_cache_file() from environment.c.  This is
 a relative path, because all of the other similar paths are.  So if we
 have chdired, then we fail.

It sounds like a reasonable code-hygiene thing, too. It looks like we
are only doing the chdir to impact the environment of the pager, and
affecting the global environment of the parent process is a side effect.
It happens not to cause problems now because we exit immediately, but
anybody who adds code after run_pager has to deal with it (and that is
basically what you are doing here). Something like cleaning up tempfiles
would be similarly affected.

That being said, this really seems like something that the run-command
interface should be doing, since it can handle the chdir in the forked
child. And indeed, it seems to support that.

Maybe:

-- 8 --
Subject: grep: use run-command's dir option for --open-files-in-pager

Git generally changes directory to the repository root on
startup.  When running grep --open-files-in-pager from a
subdirectory, we chdir back to the original directory before
running the pager, so that we can feed the relative
pathnames to the pager.

We currently do this chdir manually, but we can ask
run_command to do it for us. This is fewer lines of code,
and as a bonus, the chdir is limited to the child process,
which avoids any unexpected surprises for code running after
the pager (there isn't any currently, but this is
future-proofing).

Signed-off-by: Jeff King p...@peff.net
---
 builtin/grep.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 69ac2d8..43af5b7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -361,9 +361,7 @@ static void run_pager(struct grep_opt *opt, const char 
*prefix)
argv[i] = path_list-items[i].string;
argv[path_list-nr] = NULL;
 
-   if (prefix  chdir(prefix))
-   die(_(Failed to chdir: %s), prefix);
-   status = run_command_v_opt(argv, RUN_USING_SHELL);
+   status = run_command_v_opt_cd_env(argv, RUN_USING_SHELL, prefix, NULL);
if (status)
exit(status);
free(argv);
-- 
2.0.0.rc1.436.g03cb729

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] After chdir to run grep, return to old directory

2014-05-06 Thread David Turner 
This causes my test to pass and generally seems correct to me.

On Tue, 2014-05-06 at 23:00 -0400, Jeff King wrote:
...
 That being said, this really seems like something that the run-command
 interface should be doing, since it can handle the chdir in the forked
 child. And indeed, it seems to support that.
 
 Maybe:
 
 -- 8 --
 Subject: grep: use run-command's dir option for --open-files-in-pager
 
 Git generally changes directory to the repository root on
 startup.  When running grep --open-files-in-pager from a
 subdirectory, we chdir back to the original directory before
 running the pager, so that we can feed the relative
 pathnames to the pager.
 
 We currently do this chdir manually, but we can ask
 run_command to do it for us. This is fewer lines of code,
 and as a bonus, the chdir is limited to the child process,
 which avoids any unexpected surprises for code running after
 the pager (there isn't any currently, but this is
 future-proofing).
 
 Signed-off-by: Jeff King p...@peff.net
 ---
  builtin/grep.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)
 
 diff --git a/builtin/grep.c b/builtin/grep.c
 index 69ac2d8..43af5b7 100644
 --- a/builtin/grep.c
 +++ b/builtin/grep.c
 @@ -361,9 +361,7 @@ static void run_pager(struct grep_opt *opt, const char 
 *prefix)
   argv[i] = path_list-items[i].string;
   argv[path_list-nr] = NULL;
  
 - if (prefix  chdir(prefix))
 - die(_(Failed to chdir: %s), prefix);
 - status = run_command_v_opt(argv, RUN_USING_SHELL);
 + status = run_command_v_opt_cd_env(argv, RUN_USING_SHELL, prefix, NULL);
   if (status)
   exit(status);
   free(argv);


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] After chdir to run grep, return to old directory

2014-05-02 Thread dturner 
From: David Turner dtur...@twitter.com

Signed-off-by: David Turner dtur...@twitter.com
---
 builtin/grep.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 69ac2d8..e9fe040 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -355,15 +355,25 @@ 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));
+   static char old_directory[PATH_MAX+1];
int i, status;
 
for (i = 0; i  path_list-nr; i++)
argv[i] = path_list-items[i].string;
argv[path_list-nr] = NULL;
 
-   if (prefix  chdir(prefix))
-   die(_(Failed to chdir: %s), prefix);
+
+   if (prefix) {
+   if (!getcwd(old_directory, PATH_MAX+1))
+   die(_(Failed to get cwd: %s), prefix);
+   if (chdir(prefix))
+   die(_(Failed to chdir: %s), prefix);
+   }
status = run_command_v_opt(argv, RUN_USING_SHELL);
+   if (prefix)
+   if (chdir(old_directory))
+   die(_(Failed to chdir: %s), old_directory);
+
if (status)
exit(status);
free(argv);
-- 
2.0.0.rc0.31.g69c1a2d

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html