Re: [PATCH 2/3] help.c: make term_columns() cached and export it

6 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 2/3] help.c: make term_columns() cached and export it

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:03

Zbigniew Jędrzejewski-Szmek [off-list ref] writes:
Junio suggested that "a new file, term.c or something, be a lot more
suitable home for the function you will be reusing from diff and other
parts of the system". Nevertheless, I think that adding two files (.c
and .h) to hold one function isn't worth it. It can live in pager.c.
Terminal size is logically connected to paging after all.
I do not have any objection to the above reasoning.

Given that Nguyen's columns topic hasn't been merged to 'next' and I
expect it will be re-rolled anyway, I would prefer a patch that does the
move from help.c to pager.c that is based directly on v1.7.9, on top of
which your work and the columns topic can both be built independently.

Thanks.

[PATCH 1/2] Save terminal width before setting up pager and export term_columns()

From: Zbigniew Jędrzejewski-Szmek <hidden>
Date: 2016-06-15 22:53:03

term_columns() checks for terminal width via ioctl(2). After
redirecting, stdin is no longer terminal to get terminal width.

Check terminal width and save it before redirecting stdin in
setup_pager() by calling term_columns().

Move term_columns() to pager.c and export it in cache.h.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <redacted>
---
 cache.h |  1 +
 help.c  | 22 -------------------
 pager.c | 45 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 22 deletions(-)

This replaces cb0850f (Save terminal width before setting up pager -
2012-02-04) from Nguyễn Thái Ngọc Duy and my previous patch to export
term_columns().

This is directly on top of v1.7.9 as requested.

I removed Signed-off-by from Nguyễn and Junio because the patch is
substantially changed.
diff --git a/cache.h b/cache.h
index 10afd71..2f30b3a 100644
--- a/cache.h
+++ b/cache.h
@@ -1175,6 +1175,7 @@ extern void setup_pager(void);
 extern const char *pager_program;
 extern int pager_in_use(void);
 extern int pager_use_color;
+extern int term_columns(void);
 
 extern const char *editor_program;
 extern const char *askpass_program;
diff --git a/help.c b/help.c
index cbbe966..14eefc9 100644
--- a/help.c
+++ b/help.c
@@ -5,28 +5,6 @@
 #include "help.h"
 #include "common-cmds.h"
 
-/* most GUI terminals set COLUMNS (although some don't export it) */
-static int term_columns(void)
-{
-	char *col_string = getenv("COLUMNS");
-	int n_cols;
-
-	if (col_string && (n_cols = atoi(col_string)) > 0)
-		return n_cols;
-
-#ifdef TIOCGWINSZ
-	{
-		struct winsize ws;
-		if (!ioctl(1, TIOCGWINSZ, &ws)) {
-			if (ws.ws_col)
-				return ws.ws_col;
-		}
-	}
-#endif
-
-	return 80;
-}
-
 void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
 	struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1);
diff --git a/pager.c b/pager.c
index 975955b..b8049a4 100644
--- a/pager.c
+++ b/pager.c
@@ -76,6 +76,10 @@ void setup_pager(void)
 	if (!pager)
 		return;
 
+	/* prime the term_columns() cache before it is too
+	 * late and stdout is replaced */
+	(void) term_columns();
+
 	setenv("GIT_PAGER_IN_USE", "true", 1);
 
 	/* spawn the pager */
@@ -110,3 +114,44 @@ int pager_in_use(void)
 	env = getenv("GIT_PAGER_IN_USE");
 	return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
 }
+
+/*
+ * Return cached value (if set) or $COLUMNS (if set and positive) or
+ * ioctl(1, TIOCGWINSZ).ws_col (if positive) or 80.
+ *
+ * $COLUMNS even if set, is usually not exported, so
+ * the variable can be used to override autodection.
+ * This behaviour conforms to The Single UNIX Specification, Version 2
+ * (http://pubs.opengroup.org/onlinepubs/7908799/xbd/envvar.html#tag_002_003).
+ */
+int term_columns(void)
+{
+	static int term_columns_cache;
+
+	char *col_string;
+	int n_cols;
+
+	if (term_columns_cache)
+		return term_columns_cache;
+
+	col_string = getenv("COLUMNS");
+	if (col_string && (n_cols = atoi(col_string)) > 0) {
+		term_columns_cache = n_cols;
+		return term_columns_cache;
+	}
+
+#ifdef TIOCGWINSZ
+	{
+		struct winsize ws;
+		if (!ioctl(1, TIOCGWINSZ, &ws)) {
+			if (ws.ws_col) {
+				term_columns_cache = ws.ws_col;
+				return term_columns_cache;
+			}
+		}
+	}
+#endif
+
+	term_columns_cache = 80;
+	return term_columns_cache;
+}
-- 
1.7.9.3.g2429d.dirty

[PATCH 2/2] Rename lineno_width to decimal_width and export it

From: Zbigniew Jędrzejewski-Szmek <hidden>
Date: 2016-06-15 22:53:03

This function will be used in calculating diff --stat graph width.
The name is changed because the function works for any number.
The function is moved from builtins/blame.c to pager.c because it
will be used not only in builtins/blame.c.
---
 builtin/blame.c | 18 +++---------------
 cache.h         |  1 +
 pager.c         | 12 ++++++++++++
 3 files changed, 16 insertions(+), 15 deletions(-)

This is another function to be exported. I hope it can be exported
together with term_columns(). I'll use it the next version of the diff
--stat patch.
diff --git a/builtin/blame.c b/builtin/blame.c
index 5a67c20..f028e8a 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1829,18 +1829,6 @@ static int read_ancestry(const char *graft_file)
 }
 
 /*
- * How many columns do we need to show line numbers in decimal?
- */
-static int lineno_width(int lines)
-{
-	int i, width;
-
-	for (width = 1, i = 10; i <= lines; width++)
-		i *= 10;
-	return width;
-}
-
-/*
  * How many columns do we need to show line numbers, authors,
  * and filenames?
  */
@@ -1880,9 +1868,9 @@ static void find_alignment(struct scoreboard *sb, int *option)
 		if (largest_score < ent_score(sb, e))
 			largest_score = ent_score(sb, e);
 	}
-	max_orig_digits = lineno_width(longest_src_lines);
-	max_digits = lineno_width(longest_dst_lines);
-	max_score_digits = lineno_width(largest_score);
+	max_orig_digits = decimal_width(longest_src_lines);
+	max_digits = decimal_width(longest_dst_lines);
+	max_score_digits = decimal_width(largest_score);
 }
 
 /*
diff --git a/cache.h b/cache.h
index 2f30b3a..3857dfd 100644
--- a/cache.h
+++ b/cache.h
@@ -1176,6 +1176,7 @@ extern const char *pager_program;
 extern int pager_in_use(void);
 extern int pager_use_color;
 extern int term_columns(void);
+extern int decimal_width(int number);
 
 extern const char *editor_program;
 extern const char *askpass_program;
diff --git a/pager.c b/pager.c
index b8049a4..b6d44ef 100644
--- a/pager.c
+++ b/pager.c
@@ -155,3 +155,15 @@ int term_columns(void)
 	term_columns_cache = 80;
 	return term_columns_cache;
 }
+
+/*
+ * How many columns do we need to show numbers in decimal?
+ */
+int decimal_width(int number)
+{
+	int i, width;
+
+	for (width = 1, i = 10; i <= number; width++)
+		i *= 10;
+	return width;
+}
-- 
1.7.9.3.g2429d.dirty

Re: [PATCH 1/2] Save terminal width before setting up pager and export term_columns()

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:04

Zbigniew Jędrzejewski-Szmek [off-list ref] writes:
term_columns() checks for terminal width via ioctl(2). After
redirecting, stdin is no longer terminal to get terminal width.
s/stdin/stdout/
Check terminal width and save it before redirecting stdin in
setup_pager() by calling term_columns().

Move term_columns() to pager.c and export it in cache.h.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <redacted>
---
Thanks.

It probably is worth mentioning what the end-user visible effect of this
change is somewhere in the log message.

I somehow find "term_columns_cache" a funny name for this variable and
does not describe what it does.  Unlike a real cache, we cannot discard it
and re-read it even if we later wanted to.

I am tempted to rewrite the patch like this to update other minor style
issues.

-- >8 --
From: Zbigniew Jędrzejewski-Szmek <redacted>
Date: Sun, 12 Feb 2012 15:12:32 +0100
Subject: [PATCH] pager: find out the terminal width before spawning the pager

term_columns() checks for terminal width via ioctl(2) on the standard
output, but we spawn the pager too early for this check to be useful.

The effect of this buglet can be observed by opening a wide terminal and
running "git -p help --all", which still shows 80-column output, while
"git help --all" uses the full terminal width. Run the check before we
spawn the pager to fix this.

While at it, move term_columns() to pager.c and export it from cache.h so
that callers other than the help subsystem can use it.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 cache.h |    1 +
 help.c  |   22 ----------------------
 pager.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 22 deletions(-)
diff --git a/cache.h b/cache.h
index 79c612f..c7e3b4d 100644
--- a/cache.h
+++ b/cache.h
@@ -1172,6 +1172,7 @@ extern void setup_pager(void);
 extern const char *pager_program;
 extern int pager_in_use(void);
 extern int pager_use_color;
+extern int term_columns(void);
 
 extern const char *editor_program;
 extern const char *askpass_program;
diff --git a/help.c b/help.c
index cbbe966..14eefc9 100644
--- a/help.c
+++ b/help.c
@@ -5,28 +5,6 @@
 #include "help.h"
 #include "common-cmds.h"
 
-/* most GUI terminals set COLUMNS (although some don't export it) */
-static int term_columns(void)
-{
-	char *col_string = getenv("COLUMNS");
-	int n_cols;
-
-	if (col_string && (n_cols = atoi(col_string)) > 0)
-		return n_cols;
-
-#ifdef TIOCGWINSZ
-	{
-		struct winsize ws;
-		if (!ioctl(1, TIOCGWINSZ, &ws)) {
-			if (ws.ws_col)
-				return ws.ws_col;
-		}
-	}
-#endif
-
-	return 80;
-}
-
 void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
 	struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1);
diff --git a/pager.c b/pager.c
index 975955b..e06cfa0 100644
--- a/pager.c
+++ b/pager.c
@@ -76,6 +76,12 @@ void setup_pager(void)
 	if (!pager)
 		return;
 
+	/*
+	 * force computing the width of the terminal before we redirect
+	 * the standard output to the pager.
+	 */
+	(void) term_columns();
+
 	setenv("GIT_PAGER_IN_USE", "true", 1);
 
 	/* spawn the pager */
@@ -110,3 +116,40 @@ int pager_in_use(void)
 	env = getenv("GIT_PAGER_IN_USE");
 	return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
 }
+
+/*
+ * Return cached value (if set) or $COLUMNS (if set and positive) or
+ * ioctl(1, TIOCGWINSZ).ws_col (if positive) or 80.
+ *
+ * $COLUMNS even if set, is usually not exported, so
+ * the variable can be used to override autodection.
+ * This behaviour conforms to The Single UNIX Specification, Version 2
+ * (http://pubs.opengroup.org/onlinepubs/7908799/xbd/envvar.html#tag_002_003).
+ */
+int term_columns(void)
+{
+	static int term_columns_at_startup;
+
+	char *col_string;
+	int n_cols;
+
+	if (term_columns_at_startup)
+		return term_columns_at_startup;
+
+	term_columns_at_startup = 80;
+
+	col_string = getenv("COLUMNS");
+	if (col_string && (n_cols = atoi(col_string)) > 0)
+		term_columns_at_startup = n_cols;
+#ifdef TIOCGWINSZ
+	else {
+		struct winsize ws;
+		if (!ioctl(1, TIOCGWINSZ, &ws)) {
+			if (ws.ws_col) {
+				term_columns_at_startup = ws.ws_col;
+			}
+		}
+	}
+#endif
+	return term_columns_at_startup;
+}
-- 
1.7.9.300.gd47e4

Re: [PATCH 1/2] Save terminal width before setting up pager and export term_columns()

From: Nguyen Thai Ngoc Duy <hidden>
Date: 2016-06-15 22:53:04

2012/2/12 Zbigniew Jędrzejewski-Szmek [off-list ref]:
This replaces cb0850f (Save terminal width before setting up pager -
2012-02-04) from Nguyễn Thái Ngọc Duy and my previous patch to export
term_columns().

This is directly on top of v1.7.9 as requested.

I removed Signed-off-by from Nguyễn and Junio because the patch is
substantially changed.
No problems. I will rebase my series on top of this patch (its final
version, that is).
-- 
Duy

Re: [PATCH 1/2] Save terminal width before setting up pager and export term_columns()

From: Zbigniew Jędrzejewski-Szmek <hidden>
Date: 2016-06-15 22:53:04

On 02/14/2012 12:44 PM, Nguyen Thai Ngoc Duy wrote:
2012/2/12 Zbigniew Jędrzejewski-Szmek[off-list ref]:
quoted
This replaces cb0850f (Save terminal width before setting up pager -
2012-02-04) from Nguyễn Thái Ngọc Duy and my previous patch to export
term_columns().

This is directly on top of v1.7.9 as requested.

I removed Signed-off-by from Nguyễn and Junio because the patch is
substantially changed.
No problems. I will rebase my series on top of this patch (its final
version, that is).
Hi, I think that Junio's will be the final version. I have no objections 
to it.

Zbyszek
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help