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

[Jeff King <peff@peff.net>]

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:
> > dturner@twopensource.com writes:
> > 
> > > From: David Turner <dturner@twitter.com>
> > >
> > > Signed-off-by: David Turner <dturner@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 <peff@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