Re: [PATCH] tag: fix column output not using all terminal space

[Duy Nguyen <pclouds@gmail.com>]

On Fri, May 11, 2018 at 10:28 AM, Jeff King <peff@peff.net> 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