{"thread":{"id":"48464","subject":"[PATCH] tag: fix column output not using all terminal space","startedAt":"2018-05-11T07:56:10Z","lastAt":"2018-05-11T12:16:49Z","messageCount":6,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"347342","messageId":"20180511075602.9182-1-pclouds@gmail.com","threadId":"48464","inReplyTo":null,"subject":"[PATCH] tag: fix column output not using all terminal space","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-11T07:56:02Z","receivedAt":"2018-05-11T07:56:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"git-tag runs a separate git-column command via run_column_filter().\nThis makes the new 'git-column' process fail to pick up the terminal\nwidth for some reason and fall back to default width. Just explicitly\npass terminal width and avoid this terminal width detection business\nin subprocesses.\n\nWhile at there, fix an off-by-one column setting in git-column. We do\nnot want to use up _all_ terminal columns because the last character\nis going hit the border and wrap. Keep it at term_columns() - 1 like\nprint_columns() does. This affects the test in t7004 because effective\ncolumn width before was 40 but now 39 so we need to adjust it back.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/column.c | 2 +-\n column.c         | 2 ++\n t/t7004-tag.sh   | 2 +-\n 3 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/column.c b/builtin/column.c\nindex 0c3223d64b..182c84f778 100644\n--- a/builtin/column.c\n+++ b/builtin/column.c\n@@ -42,7 +42,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n \t\tgit_config(column_config, NULL);\n \n \tmemset(&copts, 0, sizeof(copts));\n-\tcopts.width = term_columns();\n+\tcopts.width = term_columns() - 1;\n \tcopts.padding = 1;\n \targc = parse_options(argc, argv, \"\", options, builtin_column_usage, 0);\n \tif (argc)\ndiff --git a/column.c b/column.c\nindex 49ab85b769..382537b324 100644\n--- a/column.c\n+++ b/column.c\n@@ -381,6 +381,8 @@ int run_column_filter(int colopts, const struct column_options *opts)\n \targv_array_pushf(argv, \"--raw-mode=%d\", colopts);\n \tif (opts && opts->width)\n \t\targv_array_pushf(argv, \"--width=%d\", opts->width);\n+\telse\n+\t\targv_array_pushf(argv, \"--width=%d\", term_columns() - 1);\n \tif (opts && opts->indent)\n \t\targv_array_pushf(argv, \"--indent=%s\", opts->indent);\n \tif (opts && opts->padding)\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e3f1e014aa..d7b319e919 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -363,7 +363,7 @@ test_expect_success 'tag -l <pattern> -l <pattern> works, as our buggy documenta\n '\n \n test_expect_success 'listing tags in column' '\n-\tCOLUMNS=40 git tag -l --column=row >actual &&\n+\tCOLUMNS=41 git tag -l --column=row >actual &&\n \tcat >expected <<\\EOF &&\n a1      aa1     cba     t210    t211\n v0.2.1  v1.0    v1.0.1  v1.1.3\n-- \n2.17.0.705.g3525833791\n\n"},{"id":"347343","messageId":"20180511082840.GA22086@sigill.intra.peff.net","threadId":"48464","inReplyTo":"20180511075602.9182-1-pclouds@gmail.com","subject":"Re: [PATCH] tag: fix column output not using all terminal space","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-11T08:28:40Z","receivedAt":"2018-05-11T08:28:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 11, 2018 at 09:56:02AM +0200, Nguyễn Thái Ngọc Duy wrote:\n\n> git-tag runs a separate git-column command via run_column_filter().\n> This makes the new 'git-column' process fail to pick up the terminal\n> width for some reason and fall back to default width. Just explicitly\n> pass terminal width and avoid this terminal width detection business\n> in subprocesses.\n\nI think \"some reason\" is that we start the pager before running \"git\ncolumn\". Running \"git --no-pager tag --column=row\" seems to fix it.\n\nIt doesn't seem to have anything to do with the pager program itself.\nDoing:\n\n  # use sh to avoid optimizing out pager invocation\n  GIT_PAGER='sh -c cat' git tag --column=row\n\nshows the same problem. It looks like we force term_columns() to run\nbefore invoking the pager in order to cache the value. That makes sense,\nsince TIOCGWINSZ on stdout is not going to be valid after we start\npiping it to the pager. But of course our git-column sub-process won't\nsee that; the value is cached only in memory.\n\nSo I think the approach of communicating the width to the sub-process is\nthe right one. But I think we'd probably want to do so through the\n$COLUMNS variable, rather than passing a command-line option. That would\nfix the same bug for other cases where we might have multiple layers of\nsub-processes (e.g., if we pipe to the pager and then run a hook which\ncolumnizes output).\n\nSomething like this seems to make it work for me:\n\ndiff --git a/pager.c b/pager.c\nindex 92b23e6cd1..c4f3412a84 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -162,8 +162,12 @@ int term_columns(void)\n #ifdef TIOCGWINSZ\n \telse {\n \t\tstruct winsize ws;\n-\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col)\n+\t\tif (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col) {\n+\t\t\tchar buf[64];\n \t\t\tterm_columns_at_startup = ws.ws_col;\n+\t\t\txsnprintf(buf, sizeof(buf), \"%d\", ws.ws_col);\n+\t\t\tsetenv(\"COLUMNS\", buf, 0);\n+\t\t}\n \t}\n #endif\n \n\nthough perhaps that should go into setup_pager(), which is what is\nactually making stdout inaccessible.\n\nAs an aside, I was confused while looking into this because I _thought_\nI had COLUMNS set:\n\n  $ echo $COLUMNS\n  119\n\nBut it turns out that bash sets that by default (if you have the\ncheckwinsize option on) but does not export it. ;)\n\n-Peff\n"},{"id":"347345","messageId":"CACsJy8CE2tnhH6eds6ZePyMxFXGnnyHDkWjSBbJz97BmYFr-gg@mail.gmail.com","threadId":"48464","inReplyTo":"20180511082840.GA22086@sigill.intra.peff.net","subject":"Re: [PATCH] tag: fix column output not using all terminal space","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-11T08:43:45Z","receivedAt":"2018-05-11T08:44:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 11, 2018 at 10:28 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, May 11, 2018 at 09:56:02AM +0200, Nguyễn Thái Ngọc Duy wrote:\n>\n>> git-tag runs a separate git-column command via run_column_filter().\n>> This makes the new 'git-column' process fail to pick up the terminal\n>> width for some reason and fall back to default width. Just explicitly\n>> pass terminal width and avoid this terminal width detection business\n>> in subprocesses.\n>\n> I think \"some reason\" is that we start the pager before running \"git\n> column\". Running \"git --no-pager tag --column=row\" seems to fix it.\n>\n> It doesn't seem to have anything to do with the pager program itself.\n> Doing:\n>\n>   # use sh to avoid optimizing out pager invocation\n>   GIT_PAGER='sh -c cat' git tag --column=row\n>\n> shows the same problem. It looks like we force term_columns() to run\n> before invoking the pager in order to cache the value. That makes sense,\n> since TIOCGWINSZ on stdout is not going to be valid after we start\n> piping it to the pager. But of course our git-column sub-process won't\n> see that; the value is cached only in memory.\n>\n> So I think the approach of communicating the width to the sub-process is\n> the right one. But I think we'd probably want to do so through the\n> $COLUMNS variable, rather than passing a command-line option. That would\n> fix the same bug for other cases where we might have multiple layers of\n> sub-processes (e.g., if we pipe to the pager and then run a hook which\n> columnizes output).\n>\n> Something like this seems to make it work for me:\n>\n> diff --git a/pager.c b/pager.c\n> index 92b23e6cd1..c4f3412a84 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -162,8 +162,12 @@ int term_columns(void)\n>  #ifdef TIOCGWINSZ\n>         else {\n>                 struct winsize ws;\n> -               if (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col)\n> +               if (!ioctl(1, TIOCGWINSZ, &ws) && ws.ws_col) {\n> +                       char buf[64];\n>                         term_columns_at_startup = ws.ws_col;\n> +                       xsnprintf(buf, sizeof(buf), \"%d\", ws.ws_col);\n> +                       setenv(\"COLUMNS\", buf, 0);\n> +               }\n>         }\n>  #endif\n>\n>\n> though perhaps that should go into setup_pager(), which is what is\n> actually making stdout inaccessible.\n>\n> As an aside, I was confused while looking into this because I _thought_\n> I had COLUMNS set:\n>\n>   $ echo $COLUMNS\n>   119\n>\n> But it turns out that bash sets that by default (if you have the\n> checkwinsize option on) but does not export it. ;)\n\nYep. This confused me too and I was \"f*ck it I don't want to deal with\nthis tty crap again\". I'll leave the term_columns() fix to you then,\nand limit this patch to the \"while at there\" part which should be\nfixed anyway.\n-- \nDuy\n"},{"id":"347348","messageId":"20180511092515.GA9567@sigill.intra.peff.net","threadId":"48464","inReplyTo":"CACsJy8CE2tnhH6eds6ZePyMxFXGnnyHDkWjSBbJz97BmYFr-gg@mail.gmail.com","subject":"[PATCH] pager: set COLUMNS to term_columns()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-11T09:25:16Z","receivedAt":"2018-05-11T09:25:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 11, 2018 at 10:43:45AM +0200, Duy Nguyen wrote:\n\n> > As an aside, I was confused while looking into this because I _thought_\n> > I had COLUMNS set:\n> >\n> >   $ echo $COLUMNS\n> >   119\n> >\n> > But it turns out that bash sets that by default (if you have the\n> > checkwinsize option on) but does not export it. ;)\n> \n> Yep. This confused me too and I was \"f*ck it I don't want to deal with\n> this tty crap again\". I'll leave the term_columns() fix to you then,\n> and limit this patch to the \"while at there\" part which should be\n> fixed anyway.\n\nHeh. OK, here it is. I wondered why we didn't notice this earlier and\nelsewhere (like in \"git -p help -a\"). The answer is that git-tag is the\nonly program that uses the column filter. Everybody else does it\nin-process.\n\n-- >8 --\nSubject: [PATCH] pager: set COLUMNS to term_columns()\n\nAfter we invoke the pager, our stdout goes to a pipe, not the\nterminal, meaning we can no longer use an ioctl to get the\nterminal width. For that reason, ad6c3739a3 (pager: find out\nthe terminal width before spawning the pager, 2012-02-12)\nstarted caching the terminal width.\n\nBut that cache is only an in-process variable. Any programs\nwe spawn will also not be able to run that ioctl, but won't\nhave access to our cache. They'll end up falling back to our\n80-column default.\n\nYou can see the problem with:\n\n  git tag --column=row\n\nSince git-tag spawns a pager these days, its spawned\ngit-column helper will see neither the terminal on stdout\nnor a useful COLUMNS value (assuming you do not export it\nfrom your shell already). And you'll end up with 80-column\noutput in the pager, regardless of your terminal size.\n\nWe can fix this by setting COLUMNS right before spawning the\npager. That fixes this case, as well as any more complicated\nones (e.g., a paged program spawns another script which then\ngenerates columnized output).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pager.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 92b23e6cd1..226828f178 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -109,10 +109,15 @@ void setup_pager(void)\n \t\treturn;\n \n \t/*\n-\t * force computing the width of the terminal before we redirect\n-\t * the standard output to the pager.\n+\t * After we redirect standard output, we won't be able to use an ioctl\n+\t * to get the terminal size. Let's grab it now, and then set $COLUMNS\n+\t * to communicate it to any sub-processes.\n \t */\n-\t(void) term_columns();\n+\t{\n+\t\tchar buf[64];\n+\t\txsnprintf(buf, sizeof(buf), \"%d\", term_columns());\n+\t\tsetenv(\"COLUMNS\", buf, 0);\n+\t}\n \n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n-- \n2.17.0.984.g9b00a423a4\n\n"},{"id":"347350","messageId":"20180511121329.16142-1-pclouds@gmail.com","threadId":"48464","inReplyTo":"20180511092515.GA9567@sigill.intra.peff.net","subject":"[PATCH] column: fix off-by-one default width","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-11T12:13:29Z","receivedAt":"2018-05-11T12:13:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"By default we want to fill the whole screen if possible, but we do not\nwant to use up _all_ terminal columns because the last character is\ngoing hit the border, push the cursor over and wrap. Keep it at\ndefault value zero, which will make print_columns() set the width at\nterm_columns() - 1.\n\nThis affects the test in t7004 because effective column width before\nwas 40 but now 39 so we need to compensate it by one or the output at\n39 columns has a different layout.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/column.c | 1 -\n t/t7004-tag.sh   | 2 +-\n 2 files changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/column.c b/builtin/column.c\nindex 0c3223d64b..5228ccf37a 100644\n--- a/builtin/column.c\n+++ b/builtin/column.c\n@@ -42,7 +42,6 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n \t\tgit_config(column_config, NULL);\n \n \tmemset(&copts, 0, sizeof(copts));\n-\tcopts.width = term_columns();\n \tcopts.padding = 1;\n \targc = parse_options(argc, argv, \"\", options, builtin_column_usage, 0);\n \tif (argc)\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e3f1e014aa..d7b319e919 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -363,7 +363,7 @@ test_expect_success 'tag -l <pattern> -l <pattern> works, as our buggy documenta\n '\n \n test_expect_success 'listing tags in column' '\n-\tCOLUMNS=40 git tag -l --column=row >actual &&\n+\tCOLUMNS=41 git tag -l --column=row >actual &&\n \tcat >expected <<\\EOF &&\n a1      aa1     cba     t210    t211\n v0.2.1  v1.0    v1.0.1  v1.1.3\n-- \n2.17.0.705.g3525833791\n\n"},{"id":"347351","messageId":"CACsJy8CdnDYTd5mYe0R5z9cYRcezui-ViVSnvxO4xQLEWi_T5g@mail.gmail.com","threadId":"48464","inReplyTo":"20180511092515.GA9567@sigill.intra.peff.net","subject":"Re: [PATCH] pager: set COLUMNS to term_columns()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-11T12:16:16Z","receivedAt":"2018-05-11T12:16:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 11, 2018 at 11:25 AM, Jeff King <peff@peff.net> wrote:\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -109,10 +109,15 @@ void setup_pager(void)\n>                 return;\n>\n>         /*\n> -        * force computing the width of the terminal before we redirect\n> -        * the standard output to the pager.\n> +        * After we redirect standard output, we won't be able to use an ioctl\n> +        * to get the terminal size. Let's grab it now, and then set $COLUMNS\n> +        * to communicate it to any sub-processes.\n>          */\n> -       (void) term_columns();\n> +       {\n> +               char buf[64];\n> +               xsnprintf(buf, sizeof(buf), \"%d\", term_columns());\n> +               setenv(\"COLUMNS\", buf, 0);\n\nI wonder if this affects bash being a subprocess. E.g. if COLUMNS is\nexported will it still dynamically adjust COLUMNS? A quick test shows\nthat it adjusts $COLUMNS anyway, so even if we need to launch a shell\nwe should be good.\n\n> +       }\n>\n>         setenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n>\n> --\n> 2.17.0.984.g9b00a423a4\n>\n-- \nDuy\n"}]}