Re: [PATCH] column: show auto columns when pager is active

From: Martin Ågren <martin.agren@gmail.com>
Date: Oct 11, 2017, 18:12 UTC
Message-ID: <CAN0heSqbD8TJu+_d11gj2eftG3gR+n0j621q_uSnuLQc9t_pbQ@mail.gmail.com>
In-Reply-To: <20171011172310.2932-1-me@ikke.info>

On 11 October 2017 at 19:23, Kevin Daudt <me@ikke.info> wrote:

> finalize_colopts in column.c only checks whether the output is a TTY to
> determine if columns should be enabled with columns set to auto. Also check
> if the pager is active.

Maybe you could say something about the difficulties of writing a test for `git column` proper. Something like this perhaps:

  Adding a test for git column is possible but requires some care to
  work around a race on stdin. See commit 18d8c2693 (test_terminal:
  redirect child process' stdin to a pty, 2015-08-04). Test git tag
  instead, since that does not involve stdin, and since that was the
  original motivation for this patch.

Show 6 quoted lines

> Helped-by: Rafael Ascensão <rafa.almas@gmail.com>
> Signed-off-by: Kevin Daudt <me@ikke.info>
> ---
>  column.c         |  3 ++-
>  t/t7006-pager.sh | 14 ++++++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)

Does the documentation on `column.ui` need to be updated? It talks about "if the output is to the terminal". That's similar to the documentation on the various `color.*`, so we should be fine, and arguably it's even better not to say anything since that makes it consistent.

Show 23 quoted lines

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index f0f1abd1c..44c2ca5d3 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not complain' '
>         test_cmp expect actual
>  '
>
> +test_expect_success TTY 'git tag with auto-columns ' '
> +       test_commit one &&
> +       test_commit two &&
> +       test_commit three &&
> +       test_commit four &&
> +       test_commit five &&
> +       cat >expected <<\EOF &&
> +initial  one      two      three    four     five
> +EOF
> +       test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
> +               git -p -c column.ui=auto tag --sort=authordate &&
> +       test_cmp expected actual.tag
> +'
> +
>  test_done

Since `git tag` pages when it's listing, you don't need the `-p`. But it's not like it hurts to have it. Yeah, I know, you needed it with `git column`. :-)

I wonder if it's useful to set COLUMNS a bit lower so that this has to split across more than one line (but not six), i.e., to do something non-trivial. I suppose that might lower the chances of some weird breakage slipping through.

This test uses "actual.tag" while most (all?) others in this file use "actual". Maybe you worry about checking the "wrong" file, e.g., in case the pager doesn't kick in. You could do `rm -f actual` before the `test_terminal`-invocation to protect against that.

These were just the thoughts that occurred to me, not sure if any of them is particularly significant. Thanks for cleaning up after me.

Martin