Re: [PATCH/RFC 0/9] Setup cleanup, chapter one

[Jonathan Nieder <jrnieder@gmail.com>]

Junio C Hamano wrote:

> By the way, the other day I had an interesting experience after running:
> 
> 	git -p help -m cat-file
> 
> If you try this yourself, you probably need "reset" or "stty sane" to
> recover.
> 
> Does this series address it?

No, it doesn’t.

The bug seems to be timing-related.  The two copies of ‘less’ fight over
initialization of the terminal and nobody wins.

What is the right thing to do in this case?  Can we assume ‘man’ comes
with a pager and suppress our own?  Should we ask ‘man’ to suppress its
pager, losing the formatting and status line in the process, but setting
MANPAGER=cat?  Probably the right thing to do would be to suppress our
pager and set MANPAGER according to git_pager() so ‘man’ can use it.

Maybe something roughly like this (on top of the series)?  This does
not help backends that do not know how to use MANPAGER.

diff --git a/builtin/help.c b/builtin/help.c
index 3182a2b..d9a0307 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -316,6 +316,40 @@ static const char *cmd_to_page(const char *git_cmd)
 		return prepend("git", git_cmd);
 }
 
+static void commit_pager_choice(void) {
+	switch (startup_info->use_pager) {
+	case 0:
+		setenv("GIT_PAGER", "cat", 1);
+		break;
+	case 1:
+		setup_pager();
+		break;
+	default:
+		break;
+	}
+}
+
+static void setup_man_pager(void) {
+	const char *pager;
+
+	switch (startup_info->use_pager) {
+	case 0:
+		setenv("MANPAGER", "cat", 1);
+		break;
+	case 1:
+		pager = git_pager(isatty(1));
+		if (!pager) {
+			setenv("MANPAGER", "cat", 1);
+		} else {
+			setenv("LESS", "FRSX", 0);
+			setenv("MANPAGER", pager, 1);
+		}
+		break;
+	default:
+		break;
+	}
+}
+
 static void setup_man_path(void)
 {
 	struct strbuf new_path = STRBUF_INIT;
@@ -358,6 +392,7 @@ static void show_man_page(const char *git_cmd)
 	const char *fallback = getenv("GIT_MAN_VIEWER");
 
 	setup_man_path();
+	setup_man_pager();
 	for (viewer = man_viewer_list; viewer; viewer = viewer->next)
 	{
 		exec_viewer(viewer->name, page); /* will return when unable */
@@ -423,20 +458,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
-	if (show_all) {
+	if (show_all || !argv[0]) {
+		commit_pager_choice();
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands("git commands", &main_cmds, &other_cmds);
+		if (show_all)
+			list_commands("git commands", &main_cmds, &other_cmds);
+		else
+			list_common_cmds_help();
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
 
-	if (!argv[0]) {
-		printf("usage: %s\n\n", git_usage_string);
-		list_common_cmds_help();
-		printf("\n%s\n", git_more_info_string);
-		return 0;
-	}
-
 	setup_git_directory_gently(&nongit);
 	git_config(git_help_config, NULL);
 
diff --git a/cache.h b/cache.h
index 030ba01..8ba5727 100644
--- a/cache.h
+++ b/cache.h
@@ -1061,6 +1061,7 @@ int split_cmdline(char *cmdline, const char ***argv);
 /* git.c */
 struct startup_info {
 	int have_repository;
+	int use_pager;
 };
 extern struct startup_info *startup_info;
 
diff --git a/git.c b/git.c
index 0a0d9eb..a81726e 100644
--- a/git.c
+++ b/git.c
@@ -14,7 +14,6 @@ const char git_more_info_string[] =
 	"See 'git help COMMAND' for more information on a specific command.";
 
 static struct startup_info git_startup_info;
-static int use_pager = -1;
 struct pager_config {
 	const char *cmd;
 	int val;
@@ -39,7 +38,7 @@ int check_pager_config(const char *cmd)
 }
 
 static void commit_pager_choice(void) {
-	switch (use_pager) {
+	switch (startup_info->use_pager) {
 	case 0:
 		setenv("GIT_PAGER", "cat", 1);
 		break;
@@ -86,9 +85,9 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			puts(system_path(GIT_HTML_PATH));
 			exit(0);
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
-			use_pager = 1;
+			startup_info->use_pager = 1;
 		} else if (!strcmp(cmd, "--no-pager")) {
-			use_pager = 0;
+			startup_info->use_pager = 0;
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--no-replace-objects")) {
@@ -221,13 +220,15 @@ static int handle_alias(int *argcp, const char ***argv)
 const char git_version_string[] = GIT_VERSION;
 
 #define RUN_SETUP		(1<<0)
-#define USE_PAGER		(1<<1)
 /*
  * require working tree to be present -- anything uses this needs
  * RUN_SETUP for reading from the configuration file.
  */
-#define NEED_WORK_TREE		(1<<2)
-#define RUN_SETUP_GENTLY	(1<<3)
+#define NEED_WORK_TREE		(1<<1)
+#define RUN_SETUP_GENTLY	(1<<2)
+#define USE_PAGER		(1<<3)
+/* do not commit_pager_choice() -- the builtin will take care of it */
+#define NO_PAGER_SETUP		(1<<4)
 
 struct cmd_struct {
 	const char *cmd;
@@ -244,6 +245,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	prefix = NULL;
 	help = argc == 2 && !strcmp(argv[1], "-h");
 	if (!help) {
+		int use_pager = startup_info->use_pager;
+
 		if (p->option & RUN_SETUP)
 			prefix = setup_git_directory();
 		if (p->option & RUN_SETUP_GENTLY) {
@@ -255,8 +258,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			use_pager = check_pager_config(p->cmd);
 		if (use_pager == -1 && p->option & USE_PAGER)
 			use_pager = 1;
+		startup_info->use_pager = use_pager;
 	}
-	commit_pager_choice();
+	if (!(p->option & NO_PAGER_SETUP))
+		commit_pager_choice();
 
 	if (!help && p->option & NEED_WORK_TREE)
 		setup_work_tree();
@@ -328,7 +333,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "get-tar-commit-id", cmd_get_tar_commit_id },
 		{ "grep", cmd_grep, USE_PAGER },
 		{ "hash-object", cmd_hash_object },
-		{ "help", cmd_help },
+		{ "help", cmd_help, NO_PAGER_SETUP },
 		{ "index-pack", cmd_index_pack },
 		{ "init", cmd_init_db },
 		{ "init-db", cmd_init_db },
@@ -484,6 +489,7 @@ int main(int argc, const char **argv)
 	const char *cmd;
 
 	memset(&git_startup_info, 0, sizeof(git_startup_info));
+	git_startup_info.use_pager = -1;
 	startup_info = &git_startup_info;
 
 	cmd = git_extract_argv0_path(argv[0]);