From ea27a18ce2bf5860974745c04c24864231029e1d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 22 Jul 2008 03:14:12 -0400 Subject: spawn pager via run_command interface This has two important effects: 1. The pager is now the _child_ process, instead of the parent. This means that whatever spawned git (e.g., the shell) will see the exit code of the git process, and not the pager. 2. The mingw and regular code are now unified, which makes the setup_pager function much simpler. There are two caveats: 1. We used to call execlp directly on the pager, followed by trying to exec it via the shall. We now just use the shell (which is what mingw has always done). This may have different results for pager names which contain shell metacharacters. It is also slightly less efficient because we unnecessarily run the shell; however, pager spawning is by definition an interactive task, so it shouldn't be a huge problem. 2. The git process will remain in memory while the user looks through the pager. This is potentially wasteful. We could get around this by turning the parent into a meta-process which spawns _both_ git and the pager, collects the exit status from git, waits for both to end, and then exits with git's exit code. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pager.c | 55 +++++++++++-------------------------------------------- 1 file changed, 11 insertions(+), 44 deletions(-) (limited to 'pager.c') diff --git a/pager.c b/pager.c index 6b5c9e44b4..aa0966c9c5 100644 --- a/pager.c +++ b/pager.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "run-command.h" /* * This is split up from the rest of git so that we can do @@ -8,7 +9,7 @@ static int spawned_pager; #ifndef __MINGW32__ -static void run_pager(const char *pager) +static void pager_preexec(void) { /* * Work around bug in "less" by not starting it until we @@ -20,17 +21,13 @@ static void run_pager(const char *pager) FD_SET(0, &in); select(1, &in, NULL, &in, NULL); - execlp(pager, pager, NULL); - execl("/bin/sh", "sh", "-c", pager, NULL); + setenv("LESS", "FRSX", 0); } -#else -#include "run-command.h" +#endif static const char *pager_argv[] = { "sh", "-c", NULL, NULL }; -static struct child_process pager_process = { - .argv = pager_argv, - .in = -1 -}; +static struct child_process pager_process; + static void wait_for_pager(void) { fflush(stdout); @@ -40,14 +37,9 @@ static void wait_for_pager(void) close(2); finish_command(&pager_process); } -#endif void setup_pager(void) { -#ifndef __MINGW32__ - pid_t pid; - int fd[2]; -#endif const char *pager = getenv("GIT_PAGER"); if (!isatty(1)) @@ -66,37 +58,13 @@ void setup_pager(void) spawned_pager = 1; /* means we are emitting to terminal */ -#ifndef __MINGW32__ - if (pipe(fd) < 0) - return; - pid = fork(); - if (pid < 0) { - close(fd[0]); - close(fd[1]); - return; - } - - /* return in the child */ - if (!pid) { - dup2(fd[1], 1); - dup2(fd[1], 2); - close(fd[0]); - close(fd[1]); - return; - } - - /* The original process turns into the PAGER */ - dup2(fd[0], 0); - close(fd[0]); - close(fd[1]); - - setenv("LESS", "FRSX", 0); - run_pager(pager); - die("unable to execute pager '%s'", pager); - exit(255); -#else /* spawn the pager */ pager_argv[2] = pager; + pager_process.argv = pager_argv; + pager_process.in = -1; +#ifndef __MINGW32__ + pager_process.preexec_cb = pager_preexec; +#endif if (start_command(&pager_process)) return; @@ -107,7 +75,6 @@ void setup_pager(void) /* this makes sure that the parent terminates after the pager */ atexit(wait_for_pager); -#endif } int pager_in_use(void) -- cgit v1.2.1 From a8335024c294db470e16e9df3aaa346bfcfbeacb Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 15 Dec 2008 00:33:34 -0800 Subject: pager: do not dup2 stderr if it is already redirected An earlier commit 61b8050 (sending errors to stdout under $PAGER, 2008-02-16) avoided losing the error messages that are sent to the standard error when $PAGER is in effect by dup2'ing fd 2 to the pager. his way, showing a tag object that points to a bad object: $ git show tag-foo would give the error message to the pager. However, it was not quite right if the user did: $ git show 2>error.log tag-foo i.e. use the pager but store the errors in a separate file. Signed-off-by: Junio C Hamano --- pager.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'pager.c') diff --git a/pager.c b/pager.c index 6b5c9e44b4..0b7e55f476 100644 --- a/pager.c +++ b/pager.c @@ -102,7 +102,8 @@ void setup_pager(void) /* original process continues, but writes to the pipe */ dup2(pager_process.in, 1); - dup2(pager_process.in, 2); + if (isatty(2)) + dup2(pager_process.in, 2); close(pager_process.in); /* this makes sure that the parent terminates after the pager */ -- cgit v1.2.1 From a3da8821208d6243dc5530d668f7c8f089814899 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 22 Jan 2009 01:03:28 -0500 Subject: pager: do wait_for_pager on signal death Since ea27a18 (spawn pager via run_command interface), the original git process actually does git work, and the pager is a child process (actually, on Windows it has always been that way, since Windows lacks fork). After spawning the pager, we register an atexit() handler that waits for the pager to finish. Unfortunately, that handler does not always run. In particular, if git is killed by a signal, then we exit immediately. The calling shell then thinks that git is done; however, the pager is still trying to run and impact the terminal. The result can be seen by running a long git process with a pager (e.g., "git log -p") and hitting ^C. Depending on your config, you should see the shell prompt, but pressing a key causes the pager to do any terminal de-initialization sequence. This patch just intercepts any death-dealing signals and waits for the pager before dying. Under typical less configuration, that means hitting ^C will cause git to stop generating output, but the pager will keep running. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pager.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'pager.c') diff --git a/pager.c b/pager.c index f19ddbc87d..4921843577 100644 --- a/pager.c +++ b/pager.c @@ -1,5 +1,6 @@ #include "cache.h" #include "run-command.h" +#include "sigchain.h" /* * This is split up from the rest of git so that we can do @@ -38,6 +39,13 @@ static void wait_for_pager(void) finish_command(&pager_process); } +static void wait_for_pager_signal(int signo) +{ + wait_for_pager(); + sigchain_pop(signo); + raise(signo); +} + void setup_pager(void) { const char *pager = getenv("GIT_PAGER"); @@ -75,6 +83,7 @@ void setup_pager(void) close(pager_process.in); /* this makes sure that the parent terminates after the pager */ + sigchain_push_common(wait_for_pager_signal); atexit(wait_for_pager); } -- cgit v1.2.1 From 25fc1786ab147daa9ffa43ebcbaf5c1cc6e50d4f Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Fri, 11 Sep 2009 19:45:07 +0200 Subject: pager: set LESS=FRSX also on Windows Previously, this environment variable was set in the pager_preexec callback, which is conditionally-compiled only on Unix, because it is not, and cannot be, called on Windows. With this patch the env member of struct child_process is used to set the environment variable, which also works on Windows. Noticed by Alexey Borzenkov. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- pager.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'pager.c') diff --git a/pager.c b/pager.c index 4921843577..f416d38ac2 100644 --- a/pager.c +++ b/pager.c @@ -21,8 +21,6 @@ static void pager_preexec(void) FD_ZERO(&in); FD_SET(0, &in); select(1, &in, NULL, &in, NULL); - - setenv("LESS", "FRSX", 0); } #endif @@ -70,6 +68,10 @@ void setup_pager(void) pager_argv[2] = pager; pager_process.argv = pager_argv; pager_process.in = -1; + if (!getenv("LESS")) { + static const char *env[] = { "LESS=FRSX", NULL }; + pager_process.env = env; + } #ifndef __MINGW32__ pager_process.preexec_cb = pager_preexec; #endif -- cgit v1.2.1 From 71064e3f86fbf2eb5c2b55a3024051f9542ae229 Mon Sep 17 00:00:00 2001 From: Frank Li Date: Wed, 16 Sep 2009 10:20:22 +0200 Subject: Test for WIN32 instead of __MINGW32_ The code which is conditional on MinGW32 is actually conditional on Windows. Use the WIN32 symbol, which is defined by the MINGW32 and MSVC environments, but not by Cygwin. Define SNPRINTF_SIZE_CORR=1 for MSVC too, as its vsnprintf function does not add NUL at the end of the buffer if the result fits the buffer size exactly. Signed-off-by: Frank Li Signed-off-by: Marius Storm-Olsen Acked-by: Johannes Sixt Signed-off-by: Junio C Hamano --- pager.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'pager.c') diff --git a/pager.c b/pager.c index f416d38ac2..86facec7b4 100644 --- a/pager.c +++ b/pager.c @@ -9,7 +9,7 @@ static int spawned_pager; -#ifndef __MINGW32__ +#ifndef WIN32 static void pager_preexec(void) { /* @@ -72,7 +72,7 @@ void setup_pager(void) static const char *env[] = { "LESS=FRSX", NULL }; pager_process.env = env; } -#ifndef __MINGW32__ +#ifndef WIN32 pager_process.preexec_cb = pager_preexec; #endif if (start_command(&pager_process)) -- cgit v1.2.1 From 6361824589bc2d32989a9a33f985d09a368436a3 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Fri, 30 Oct 2009 20:41:27 -0500 Subject: Teach git var about GIT_PAGER Expose the command found by setup_pager() for scripts to use. Scripts can use this to avoid repeating the logic to look for a proper pager in each command. Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- pager.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) (limited to 'pager.c') diff --git a/pager.c b/pager.c index 86facec7b4..0b63d99fe7 100644 --- a/pager.c +++ b/pager.c @@ -44,12 +44,14 @@ static void wait_for_pager_signal(int signo) raise(signo); } -void setup_pager(void) +const char *git_pager(void) { - const char *pager = getenv("GIT_PAGER"); + const char *pager; if (!isatty(1)) - return; + return NULL; + + pager = getenv("GIT_PAGER"); if (!pager) { if (!pager_program) git_config(git_default_config, NULL); @@ -60,6 +62,16 @@ void setup_pager(void) if (!pager) pager = "less"; else if (!*pager || !strcmp(pager, "cat")) + pager = NULL; + + return pager; +} + +void setup_pager(void) +{ + const char *pager = git_pager(); + + if (!pager) return; spawned_pager = 1; /* means we are emitting to terminal */ -- cgit v1.2.1 From a3d023d0a3783612053f2149e784b43befceccad Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 30 Oct 2009 20:45:34 -0500 Subject: Provide a build time default-pager setting Provide a DEFAULT_PAGER knob so packagers can set the fallback pager to something appropriate during the build. Examples: On (old) solaris systems, /usr/bin/less (typically the first less found) doesn't understand the default arguments (FXRS), which forces users to alter their environment (PATH, GIT_PAGER, LESS, etc) or have a local or global gitconfig before paging works as expected. On Debian systems, by policy packages must fall back to the 'pager' command, so that changing the target of the /usr/bin/pager symlink changes the default pager for all packages at once. Signed-off-by: Jonathan Nieder Signed-off-by: Ben Walton Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- pager.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'pager.c') diff --git a/pager.c b/pager.c index 0b63d99fe7..92c03f654a 100644 --- a/pager.c +++ b/pager.c @@ -2,6 +2,10 @@ #include "run-command.h" #include "sigchain.h" +#ifndef DEFAULT_PAGER +#define DEFAULT_PAGER "less" +#endif + /* * This is split up from the rest of git so that we can do * something different on Windows. @@ -60,7 +64,7 @@ const char *git_pager(void) if (!pager) pager = getenv("PAGER"); if (!pager) - pager = "less"; + pager = DEFAULT_PAGER; else if (!*pager || !strcmp(pager, "cat")) pager = NULL; -- cgit v1.2.1 From ac0ba18df0c58decfb128336bab51a172c77abc0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Dec 2009 05:53:57 -0500 Subject: run-command: convert simple callsites to use_shell Now that we have the use_shell feature, these callsites can all be converted with small changes. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pager.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'pager.c') diff --git a/pager.c b/pager.c index 92c03f654a..2c7e8ecb3c 100644 --- a/pager.c +++ b/pager.c @@ -28,7 +28,7 @@ static void pager_preexec(void) } #endif -static const char *pager_argv[] = { "sh", "-c", NULL, NULL }; +static const char *pager_argv[] = { NULL, NULL }; static struct child_process pager_process; static void wait_for_pager(void) @@ -81,7 +81,8 @@ void setup_pager(void) spawned_pager = 1; /* means we are emitting to terminal */ /* spawn the pager */ - pager_argv[2] = pager; + pager_argv[0] = pager; + pager_process.use_shell = 1; pager_process.argv = pager_argv; pager_process.in = -1; if (!getenv("LESS")) { -- cgit v1.2.1