[PATCH] pager: config variable pager.color

[PATCH] pager: config variable pager.color

[Matthias Lederhofer]

enable/disable colored output when the pager is in use

Signed-off-by: Matthias Lederhofer <matled@gmx.net>
---
setup_pager has to be called before git_diff_ui_config because the
latter uses pager_use_color initialized by setup_pager.
---
 Documentation/config.txt |    4 ++++
 builtin-log.c            |    4 +++-
 cache.h                  |    1 +
 diff.c                   |    2 +-
 environment.c            |    1 +
 pager.c                  |    4 ++++
 6 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 96429b6..5e243a8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -121,6 +121,10 @@ pager.program::
 	variable first, then the 'PAGER' environment variable and
 	"less" as fallback.
 
+pager.color::
+	A boolean to enable/disable colored output when the pager is in
+	use (default is true).
+
 diff.color::
 	When true (or `always`), always use colors in patch.
 	When false (or `never`), never.  When set to `auto`, use
diff --git a/builtin-log.c b/builtin-log.c
index 82c69d1..7fdefec 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -34,7 +34,6 @@ static int cmd_log_walk(struct rev_info 
 	struct commit *commit;
 
 	prepare_revision_walk(rev);
-	setup_pager();
 	while ((commit = get_revision(rev)) != NULL) {
 		log_tree_commit(rev, commit);
 		free(commit->buffer);
@@ -49,6 +48,7 @@ int cmd_whatchanged(int argc, const char
 {
 	struct rev_info rev;
 
+	setup_pager();
 	git_config(git_diff_ui_config);
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
@@ -64,6 +64,7 @@ int cmd_show(int argc, const char **argv
 {
 	struct rev_info rev;
 
+	setup_pager();
 	git_config(git_diff_ui_config);
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
@@ -81,6 +82,7 @@ int cmd_log(int argc, const char **argv,
 {
 	struct rev_info rev;
 
+	setup_pager();
 	git_config(git_diff_ui_config);
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
diff --git a/cache.h b/cache.h
index 8891073..913be6a 100644
--- a/cache.h
+++ b/cache.h
@@ -392,6 +392,7 @@ extern int receive_keep_pack(int fd[2], 
 /* pager.c */
 extern void setup_pager(void);
 extern int pager_in_use;
+extern int pager_use_color;
 
 /* base85 */
 int decode_85(char *dst, char *line, int linelen);
diff --git a/diff.c b/diff.c
index 6198a61..16d47bf 100644
--- a/diff.c
+++ b/diff.c
@@ -175,7 +175,7 @@ int git_diff_ui_config(const char *var, 
 			diff_use_color_default = 1; /* bool */
 		else if (!strcasecmp(value, "auto")) {
 			diff_use_color_default = 0;
-			if (isatty(1) || pager_in_use) {
+			if (isatty(1) || (pager_in_use && pager_use_color)) {
 				char *term = getenv("TERM");
 				if (term && strcmp(term, "dumb"))
 					diff_use_color_default = 1;
diff --git a/environment.c b/environment.c
index 558801a..1ce3411 100644
--- a/environment.c
+++ b/environment.c
@@ -23,6 +23,7 @@ int shared_repository = PERM_UMASK;
 const char *apply_default_whitespace = NULL;
 int zlib_compression_level = Z_DEFAULT_COMPRESSION;
 int pager_in_use;
+int pager_use_color = 1;
 
 static int dyn_git_object_dir, dyn_git_index_file, dyn_git_graft_file;
 static char *git_dir, *git_object_dir, *git_index_file, *git_refs_dir,
diff --git a/pager.c b/pager.c
index 3f753f6..afd684e 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,10 @@ static int git_pager_config(const char *
 			pager = strdup(value);
 		return 0;
 	}
+	if (!strcmp(var, "pager.color")) {
+		pager_use_color = git_config_bool(var,value);
+		return 0;
+	}
 	return 0;
 }
 
-- 
1.4.2.rc2.g91b7

Re: [PATCH] pager: config variable pager.color

[Johannes Schindelin]

Hi,

On Sun, 30 Jul 2006, Matthias Lederhofer wrote:

> diff --git a/builtin-log.c b/builtin-log.c
> index 82c69d1..7fdefec 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -34,7 +34,6 @@ static int cmd_log_walk(struct rev_info 
>  	struct commit *commit;
>  
>  	prepare_revision_walk(rev);
> -	setup_pager();
>  	while ((commit = get_revision(rev)) != NULL) {
>  		log_tree_commit(rev, commit);
>  		free(commit->buffer);
> @@ -49,6 +48,7 @@ int cmd_whatchanged(int argc, const char
>  {
>  	struct rev_info rev;
>  
> +	setup_pager();
>  	git_config(git_diff_ui_config);
>  	init_revisions(&rev, prefix);
>  	rev.diff = 1;
> @@ -64,6 +64,7 @@ int cmd_show(int argc, const char **argv
>  {
>  	struct rev_info rev;
>  
> +	setup_pager();
>  	git_config(git_diff_ui_config);
>  	init_revisions(&rev, prefix);
>  	rev.diff = 1;
> @@ -81,6 +82,7 @@ int cmd_log(int argc, const char **argv,
>  {
>  	struct rev_info rev;
>  
> +	setup_pager();
>  	git_config(git_diff_ui_config);
>  	init_revisions(&rev, prefix);
>  	rev.always_show_header = 1;

Why? The three users of cmd_log_walk() need to call setup_pager() 
explicitely, when cmd_log_walk() can do it for them?

Oh, and I do not really understand why you would enable color _at all_ if 
you want to disable it when paging. Do you have many instances when you 
want a color diff which is short enough not to be paged?

Ciao,
Dscho

Re: [PATCH] pager: config variable pager.color

[Matthias Lederhofer]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Why? The three users of cmd_log_walk() need to call setup_pager() 
> explicitely, when cmd_log_walk() can do it for them?

The explanation is below the commit message:
"setup_pager has to be called before git_diff_ui_config because the
latter uses pager_use_color initialized by setup_pager."

> Oh, and I do not really understand why you would enable color _at all_ if 
> you want to disable it when paging. Do you have many instances when you 
> want a color diff which is short enough not to be paged?
When I use a pager that escapes the escape character or highlights the
content itself the output of git diff without the pager should have
colors but not with the pager.  For example using git diff with a
pathspec is quite short most of the time.  For git diff I have to
enable paging manually and run git diff | $PAGER usually but git log
uses the pager automatically and should not use colors with it.

Re: [PATCH] pager: config variable pager.color

[Johannes Schindelin]

Hi,

On Sun, 30 Jul 2006, Matthias Lederhofer wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Why? The three users of cmd_log_walk() need to call setup_pager() 
> > explicitely, when cmd_log_walk() can do it for them?
> 
> The explanation is below the commit message:
> "setup_pager has to be called before git_diff_ui_config because the
> latter uses pager_use_color initialized by setup_pager."
> 
> > Oh, and I do not really understand why you would enable color _at all_ if 
> > you want to disable it when paging. Do you have many instances when you 
> > want a color diff which is short enough not to be paged?

> When I use a pager that escapes the escape character or highlights the
> content itself the output of git diff without the pager should have
> colors but not with the pager.  For example using git diff with a
> pathspec is quite short most of the time.  For git diff I have to
> enable paging manually and run git diff | $PAGER usually but git log
> uses the pager automatically and should not use colors with it.

I understand now. Maybe you want to put such reasoning into the commit 
message next time?

Ciao,
Dscho

Re: [PATCH] pager: config variable pager.color

[Junio C Hamano]

Matthias Lederhofer <matled@gmx.net> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> Why? The three users of cmd_log_walk() need to call setup_pager() 
>> explicitely, when cmd_log_walk() can do it for them?
>
> The explanation is below the commit message:
> "setup_pager has to be called before git_diff_ui_config because the
> latter uses pager_use_color initialized by setup_pager."

If that is the reason, perhaps we could restructure the setting
and use of of diff_use_color_default like the attached, which
would be cleaner.

-- >8 --

diff --git a/cache.h b/cache.h
index 8891073..913be6a 100644
--- a/cache.h
+++ b/cache.h
@@ -392,6 +392,7 @@ extern int receive_keep_pack(int fd[2], 
 /* pager.c */
 extern void setup_pager(void);
 extern int pager_in_use;
+extern int pager_use_color;
 
 /* base85 */
 int decode_85(char *dst, char *line, int linelen);
diff --git a/diff.c b/diff.c
index 6198a61..043ecb1 100644
--- a/diff.c
+++ b/diff.c
@@ -173,14 +173,8 @@ int git_diff_ui_config(const char *var, 
 	if (!strcmp(var, "diff.color")) {
 		if (!value)
 			diff_use_color_default = 1; /* bool */
-		else if (!strcasecmp(value, "auto")) {
-			diff_use_color_default = 0;
-			if (isatty(1) || pager_in_use) {
-				char *term = getenv("TERM");
-				if (term && strcmp(term, "dumb"))
-					diff_use_color_default = 1;
-			}
-		}
+		else if (!strcasecmp(value, "auto"))
+			diff_use_color_default = -1; /* decide in setup */
 		else if (!strcasecmp(value, "never"))
 			diff_use_color_default = 0;
 		else if (!strcasecmp(value, "always"))
@@ -1509,6 +1503,14 @@ void diff_setup(struct diff_options *opt
 
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
+	if (diff_use_color_default < 0) {
+		diff_use_color_default = 0;
+		if (isatty(1) || (pager_in_use && pager_use_color)) {
+			char *term = getenv("TERM");
+			if (term && strcmp(term, "dumb"))
+				diff_use_color_default = 1;
+		}
+	}
 	options->color_diff = diff_use_color_default;
 	options->detect_rename = diff_detect_rename_default;
 }
diff --git a/environment.c b/environment.c
index 558801a..1ce3411 100644
--- a/environment.c
+++ b/environment.c
@@ -23,6 +23,7 @@ int shared_repository = PERM_UMASK;
 const char *apply_default_whitespace = NULL;
 int zlib_compression_level = Z_DEFAULT_COMPRESSION;
 int pager_in_use;
+int pager_use_color = 1;
 
 static int dyn_git_object_dir, dyn_git_index_file, dyn_git_graft_file;
 static char *git_dir, *git_object_dir, *git_index_file, *git_refs_dir,
diff --git a/pager.c b/pager.c
index 3f753f6..e86ea9e 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,10 @@ static int git_pager_config(const char *
 			pager = strdup(value);
 		return 0;
 	}
+	if (!strcmp(var, "pager.color")) {
+		pager_use_color = git_config_bool(var, value);
+		return 0;
+	}
 	return 0;
 }
 

Re: [PATCH] pager: config variable pager.color

[Juergen Ruehle]

Junio C Hamano writes:
 > Matthias Lederhofer <matled@gmx.net> writes:
 > 
 > > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
 > >> Why? The three users of cmd_log_walk() need to call setup_pager() 
 > >> explicitely, when cmd_log_walk() can do it for them?
 > >
 > > The explanation is below the commit message:
 > > "setup_pager has to be called before git_diff_ui_config because the
 > > latter uses pager_use_color initialized by setup_pager."
 > 
 > If that is the reason, perhaps we could restructure the setting
 > and use of of diff_use_color_default like the attached, which
 > would be cleaner.

AFAICS Matthias' patch has the added benefit of moving setup_pager to
before large files (i.e. packs) are mapped. This helps non-COW-fork
(i.e. cygwin) tremendously. Actually with Linus' setup refactoring
this could probably be easily moved to the wrapper, but I'm too stupid
so I just deleted setup_pager from my version:-)

  jr

Re: [PATCH] pager: config variable pager.color

[Junio C Hamano]

Juergen Ruehle <j.ruehle@bmiag.de> writes:

> AFAICS Matthias' patch has the added benefit of moving setup_pager to
> before large files (i.e. packs) are mapped. This helps non-COW-fork
> (i.e. cygwin) tremendously. Actually with Linus' setup refactoring
> this could probably be easily moved to the wrapper,...

Hmph.  Never thought about that.  Something like this?

-- >8 --
Builtins: control the use of pager from the command table.

This moves the built-in "always-use-pager" logic for log family
to the command dispatch table of git wrapper.  This makes it
easier to change the default use of pager, and has an added
benefit that we fork and exec the pager early before packs are
mmapped.

Pointed out by Juergen Ruehle <j.ruehle@bmiag.de>.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
diff --git a/builtin-log.c b/builtin-log.c
index 82c69d1..bba1496 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -34,7 +34,6 @@ static int cmd_log_walk(struct rev_info 
 	struct commit *commit;
 
 	prepare_revision_walk(rev);
-	setup_pager();
 	while ((commit = get_revision(rev)) != NULL) {
 		log_tree_commit(rev, commit);
 		free(commit->buffer);
diff --git a/git.c b/git.c
index 7321d6c..d031eb9 100644
--- a/git.c
+++ b/git.c
@@ -211,6 +211,7 @@ static int handle_alias(int *argcp, cons
 const char git_version_string[] = GIT_VERSION;
 
 #define NEEDS_PREFIX 1
+#define USE_PAGER 2
 
 static void handle_internal_command(int argc, const char **argv, char **envp)
 {
@@ -218,13 +219,13 @@ static void handle_internal_command(int 
 	static struct cmd_struct {
 		const char *cmd;
 		int (*fn)(int, const char **, const char *);
-		int prefix;
+		int option;
 	} commands[] = {
 		{ "version", cmd_version },
 		{ "help", cmd_help },
-		{ "log", cmd_log, NEEDS_PREFIX },
-		{ "whatchanged", cmd_whatchanged, NEEDS_PREFIX },
-		{ "show", cmd_show, NEEDS_PREFIX },
+		{ "log", cmd_log, NEEDS_PREFIX | USE_PAGER },
+		{ "whatchanged", cmd_whatchanged, NEEDS_PREFIX | USE_PAGER },
+		{ "show", cmd_show, NEEDS_PREFIX | USE_PAGER },
 		{ "push", cmd_push },
 		{ "format-patch", cmd_format_patch, NEEDS_PREFIX },
 		{ "count-objects", cmd_count_objects },
@@ -275,8 +276,10 @@ static void handle_internal_command(int 
 			continue;
 
 		prefix = NULL;
-		if (p->prefix)
+		if (p->option & NEEDS_PREFIX)
 			prefix = setup_git_directory();
+		if (p->option & USE_PAGER)
+			setup_pager();
 		if (getenv("GIT_TRACE")) {
 			int i;
 			fprintf(stderr, "trace: built-in: git");

Re: [PATCH] pager: config variable pager.color

[Matthias Lederhofer]

Junio C Hamano <junkio@cox.net> wrote:
> Hmph.  Never thought about that.  Something like this?
Funny, I wrote this patch yesterday too :) Add signed-off-by if you
like.