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