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

[Martin Ågren]

On 11 October 2017 at 20:36, Kevin Daudt  wrote:
> On Wed, Oct 11, 2017 at 08:12:35PM +0200, Martin Ågren wrote:
>> On 11 October 2017 at 19:23, Kevin Daudt  wrote:
>> 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.
>
> Yeah, I was doubting about that, but wouldn't this amount to testing
> whether git column is working properly, instead of just testing whether
> it's being done at all?

Right, I think you'd need a pretty crazy bug in order to slip through
all tests here.

>> These were just the thoughts that occurred to me, not sure if any of
>> them is particularly significant. Thanks for cleaning up after me.
>>
>
> np. Just as I posted earlier, I think you did not actually cause the bug
> (because this has never worked), it just made it visible to more users.

Well, the general bug/behavior was always there, but I regressed a
particular use-case. In 2.14, you could do `git tag` with column.ui=auto
and it would do the columns-thing. But with ff1e72483, the behavior
changed. To add insult to injury, it might be non-obvious that the pager
is running, since with just a few tags, the pager simply exits silently.
So debugging this could probably be quite frustrating.

Martin

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

[Kevin Daudt]

On Wed, Oct 11, 2017 at 08:12:35PM +0200, Martin Ågren wrote:
> On 11 October 2017 at 19:23, Kevin Daudt  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.

Right, makes sense.
> 
> > Helped-by: Rafael Ascensão 
> > Signed-off-by: Kevin Daudt 
> > ---
> >  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  threefour 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`. :-)

Right, it was a bit of a left-over since I assumed the PAGER='cat 
>paginated.out'
from the beginning of the test was in place and I wasn't getting any
output, but it turned out PAGER wasn't set.

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

Yeah, I was doubting about that, but wouldn't this amount to testing
whether git column is working properly, instead of just testing whether
it's being done at all?

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

Yeah, I actually ran into that, but rm-ing it is better, I agree.

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

np. Just as I posted earlier, I think you did not actually cause the bug
(because this has never worked), it just made it visible to more users.

Kevin

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

[Martin Ågren]

On 11 October 2017 at 19:23, Kevin Daudt  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 
> Signed-off-by: Kevin Daudt 
> ---
>  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  threefour 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