tag: fix column output not using all terminal space
PATCH 1/1 tag: fix column output not using all terminal space
git-tag runs a separate git-column command via run_column_filter(). This makes the new 'git-column' process fail to pick up the terminal width for some reason and fall back to default width. Just explicitly pass terminal width and avoid this terminal width detection business in subprocesses.
While at there, fix an off-by-one column setting in git-column. We do not want to use up _all_ terminal columns because the last character is going hit the border and wrap. Keep it at term_columns() - 1 like print_columns() does. This affects the test in t7004 because effective column width before was 40 but now 39 so we need to adjust it back.
--- builtin/column.c | 2 +- column.c | 2 ++ t/t7004-tag.sh | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-)
builtin/column.c +1 -1
@@ -42,7 +42,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)git_config (column_config,NULL );memset (&copts,0 ,sizeof (copts)); - copts.width =term_columns (); + copts.width =term_columns () -1 ; copts.padding =1 ; argc =parse_options (argc, argv,"" , options, builtin_column_usage,0 );if (argc)
column.c +2 -0
@@ -381,6 +381,8 @@ int run_column_filter(int colopts, const struct column_options *opts)argv_array_pushf (argv,"--raw-mode=%d" , colopts);if (opts && opts->width)argv_array_pushf (argv,"--width=%d" , opts->width); +else +argv_array_pushf (argv,"--width=%d" ,term_columns () -1 );if (opts && opts->indent)argv_array_pushf (argv,"--indent=%s" , opts->indent);if (opts && opts->padding)
t/t7004-tag.sh +1 -1
@@ -363,7 +363,7 @@ test_expect_success 'tag -l <pattern> -l <pattern> works, as our buggy documenta ' test_expect_success 'listing tags in column' ' - COLUMNS=40 git tag -l --column=row >actual && + COLUMNS=41 git tag -l --column=row >actual && cat >expected <<\EOF && a1 aa1 cba t210 t211 v0.2.1 v1.0 v1.0.1 v1.1.3
Signature · 1 line
2.17.0.705.g3525833791
Quoted 5 lines · Nguyễn Thái Ngọc Duy
git-tag runs a separate git-column command via
run_column_filter(). This makes the new 'git-column' process fail to pick up the terminal width for some reason and fall back to default width. Just explicitly pass terminal width and avoid this terminal width detection business in subprocesses.
I think "some reason" is that we start the pager before running "git column". Running "git --no-pager tag --column=row" seems to fix it.
It doesn't seem to have anything to do with the pager program itself. Doing:
# use sh to avoid optimizing out pager invocation GIT_PAGER='sh -c cat' git tag --column=row
shows the same problem. It looks like we force term_columns() to run before invoking the pager in order to cache the value. That makes sense, since TIOCGWINSZ on stdout is not going to be valid after we start piping it to the pager. But of course our git-column sub-process won't see that; the value is cached only in memory.
So I think the approach of communicating the width to the sub-process is the right one. But I think we'd probably want to do so through the $COLUMNS variable, rather than passing a command-line option. That would fix the same bug for other cases where we might have multiple layers of sub-processes (e.g., if we pipe to the pager and then run a hook which columnizes output).
Something like this seems to make it work for me:
pager.c +5 -1
@@ -162,8 +162,12 @@ int term_columns(void)#ifdef TIOCGWINSZ else {struct winsize ws; -if (!ioctl (1 ,TIOCGWINSZ , &ws) && ws.ws_col) +if (!ioctl (1 ,TIOCGWINSZ , &ws) && ws.ws_col) { +char buf[64 ]; term_columns_at_startup = ws.ws_col; +xsnprintf (buf,sizeof (buf),"%d" , ws.ws_col); +setenv ("COLUMNS" , buf,0 ); + } }#endif
though perhaps that should go into setup_pager(), which is what is actually making stdout inaccessible.
As an aside, I was confused while looking into this because I _thought_ I had COLUMNS set:
$ echo $COLUMNS 119
But it turns out that bash sets that by default (if you have the checkwinsize option on) but does not export it. ;)
-Peff
Quoted 54 lines · Jeff King
Quoted 5 lines · Nguyễn Thái Ngọc Duy
git-tag runs a separate git-column command via
run_column_filter(). This makes the new 'git-column' process fail to pick up the terminal width for some reason and fall back to default width. Just explicitly pass terminal width and avoid this terminal width detection business in subprocesses.I think "some reason" is that we start the pager before running "git column". Running "git --no-pager tag --column=row" seems to fix it.
It doesn't seem to have anything to do with the pager program itself. Doing:
# use sh to avoid optimizing out pager invocation GIT_PAGER='sh -c cat' git tag --column=rowshows the same problem. It looks like we force
term_columns()to run before invoking the pager in order to cache the value. That makes sense, since TIOCGWINSZ on stdout is not going to be valid after we start piping it to the pager. But of course our git-column sub-process won't see that; the value is cached only in memory.So I think the approach of communicating the width to the sub-process is the right one. But I think we'd probably want to do so through the $COLUMNS variable, rather than passing a command-line option. That would fix the same bug for other cases where we might have multiple layers of sub-processes (e.g., if we pipe to the pager and then run a hook which columnizes output).
Something like this seems to make it work for me:
pager.c +5
-1@@ -162,8 +162,12 @@ int term_columns(void)#ifdef TIOCGWINSZ else {struct winsize ws; -if (!ioctl (1 ,TIOCGWINSZ , &ws) && ws.ws_col) +if (!ioctl (1 ,TIOCGWINSZ , &ws) && ws.ws_col) { +char buf[64 ]; term_columns_at_startup = ws.ws_col; +xsnprintf (buf,sizeof (buf),"%d" , ws.ws_col); +setenv ("COLUMNS" , buf,0 ); + } }#endif though perhaps that should go into
setup_pager(), which is what is actually making stdout inaccessible.
Quoted 8 lines
As an aside, I was confused while looking into this because I _thought_ I had COLUMNS set:
$ echo $COLUMNS 119But it turns out that bash sets that by default (if you have the checkwinsize option on) but does not export it. ;)
Yep. This confused me too and I was "f*ck it I don't want to deal with this tty crap again". I'll leave the term_columns() fix to you then, and limit this patch to the "while at there" part which should be fixed anyway.
Signature · 1 line
Duy
PATCH 1/1 pager: set COLUMNS to term_columns()
Quoted 6 lines · Duy Nguyen
Quoted 6 lines
As an aside, I was confused while looking into this because I _thought_ I had COLUMNS set:
$ echo $COLUMNS 119
Quoted 7 lines
Quoted 2 lines
But it turns out that bash sets that by default (if you have the checkwinsize option on) but does not export it. ;)
Yep. This confused me too and I was "f*ck it I don't want to deal with this tty crap again". I'll leave the
term_columns()fix to you then, and limit this patch to the "while at there" part which should be fixed anyway.
Heh. OK, here it is. I wondered why we didn't notice this earlier and elsewhere (like in "git -p help -a"). The answer is that git-tag is the only program that uses the column filter. Everybody else does it in-process.
-- >8 --
Subject: [PATCH] pager: set COLUMNS to term_columns()
After we invoke the pager, our stdout goes to a pipe, not the terminal, meaning we can no longer use an ioctl to get the terminal width. For that reason, ad6c3739a3 (pager: find out the terminal width before spawning the pager, 2012-02-12) started caching the terminal width.
But that cache is only an in-process variable. Any programs we spawn will also not be able to run that ioctl, but won't have access to our cache. They'll end up falling back to our 80-column default.
You can see the problem with:
git tag --column=row
Since git-tag spawns a pager these days, its spawned git-column helper will see neither the terminal on stdout nor a useful COLUMNS value (assuming you do not export it from your shell already). And you'll end up with 80-column output in the pager, regardless of your terminal size.
We can fix this by setting COLUMNS right before spawning the pager. That fixes this case, as well as any more complicated ones (e.g., a paged program spawns another script which then generates columnized output).
--- pager.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
pager.c +8 -3
@@ -109,10 +109,15 @@ void setup_pager(void)return ;/* -* force computing the width of the terminal before we redirect -* the standard output to the pager. +* After we redirect standard output, we won't be able to use an ioctl +* to get the terminal size. Let's grab it now, and then set $COLUMNS +* to communicate it to any sub-processes. */ - (void )term_columns (); + { +char buf[64 ]; +xsnprintf (buf,sizeof (buf),"%d" ,term_columns ()); +setenv ("COLUMNS" , buf,0 ); + }setenv ("GIT_PAGER_IN_USE" ,"true" ,1 );
Signature · 1 line
2.17.0.984.g9b00a423a4
PATCH 1/1 column: fix off-by-one default width
By default we want to fill the whole screen if possible, but we do not want to use up _all_ terminal columns because the last character is going hit the border, push the cursor over and wrap. Keep it at default value zero, which will make print_columns() set the width at term_columns() - 1.
This affects the test in t7004 because effective column width before was 40 but now 39 so we need to compensate it by one or the output at 39 columns has a different layout.
--- builtin/column.c | 1 - t/t7004-tag.sh | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-)
builtin/column.c +0 -1
@@ -42,7 +42,6 @@ int cmd_column(int argc, const char **argv, const char *prefix)git_config (column_config,NULL );memset (&copts,0 ,sizeof (copts)); - copts.width =term_columns (); copts.padding =1 ; argc =parse_options (argc, argv,"" , options, builtin_column_usage,0 );if (argc)
t/t7004-tag.sh +1 -1
@@ -363,7 +363,7 @@ test_expect_success 'tag -l <pattern> -l <pattern> works, as our buggy documenta ' test_expect_success 'listing tags in column' ' - COLUMNS=40 git tag -l --column=row >actual && + COLUMNS=41 git tag -l --column=row >actual && cat >expected <<\EOF && a1 aa1 cba t210 t211 v0.2.1 v1.0 v1.0.1 v1.1.3
Signature · 1 line
2.17.0.705.g3525833791
Re: [PATCH] pager: set COLUMNS to term_columns()
In reply to Jeff KingQuoted 7 lines · Jeff King
pager.c +0
-1@@ -109,10 +109,15 @@ void setup_pager(void)return ;/* -* force computing the width of the terminal before we redirect
pager.c void setup_pager(void) · line 119
Quoted 10 lines
pager.c +7
-2-* the standard output to the pager. +* After we redirect standard output, we won't be able to use an ioctl +* to get the terminal size. Let's grab it now, and then set $COLUMNS +* to communicate it to any sub-processes. */ - (void )term_columns (); + { +char buf[64 ]; +xsnprintf (buf,sizeof (buf),"%d" ,term_columns ()); +setenv ("COLUMNS" , buf,0 );
I wonder if this affects bash being a subprocess. E.g. if COLUMNS is exported will it still dynamically adjust COLUMNS? A quick test shows that it adjusts $COLUMNS anyway, so even if we need to launch a shell we should be good.
Quoted 7 lines
pager.c +1
-1+ }setenv ("GIT_PAGER_IN_USE" ,"true" ,1 ); --2.17.0.984.g9b00a423a4
Signature · 1 line
Duy