Re: [PATCH] tag: fix column output not using all terminal space
On Fri, May 11, 2018 at 10:28 AM, Jeff King wrote:
> On Fri, May 11, 2018 at 09:56:02AM +0200, Nguyễn Thái Ngọc Duy wrote:
>
>> 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:
>
> diff --git a/pager.c b/pager.c
> index 92b23e6cd1..c4f3412a84 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -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. ;)
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.
--
Duy
Re: [PATCH] tag: fix column output not using all terminal space
On Fri, May 11, 2018 at 09:56:02AM +0200, Nguyễn Thái Ngọc Duy wrote:
> 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:
diff --git a/pager.c b/pager.c
index 92b23e6cd1..c4f3412a84 100644
--- a/pager.c
+++ b/pager.c
@@ -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
[PATCH] 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. Signed-off-by: Nguyễn Thái Ngọc Duy --- builtin/column.c | 2 +- column.c | 2 ++ t/t7004-tag.sh | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/column.c b/builtin/column.c index 0c3223d64b..182c84f778 100644 --- a/builtin/column.c +++ b/builtin/column.c @@ -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) diff --git a/column.c b/column.c index 49ab85b769..382537b324 100644 --- a/column.c +++ b/column.c @@ -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) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index e3f1e014aa..d7b319e919 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -363,7 +363,7 @@ test_expect_success 'tag -l -l 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 t210t211 v0.2.1 v1.0v1.0.1 v1.1.3 -- 2.17.0.705.g3525833791
