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

["Martin Ågren" <martin.agren@gmail.com>]

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.

> 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.

> 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