Re: [PATCH 1/3] t7006: add tests for how git config paginates

From: Martin Ågren <martin.agren@gmail.com>
Date: Feb 12, 2018, 22:41 UTC
Message-ID: <CAN0heSr5bRx7Lrzu4mfeHx3SuZ75bgpXUA75kM1HJ-h=+8dgPQ@mail.gmail.com>
In-Reply-To: <xmqq4lmlsvhr.fsf@gitster-ct.c.googlers.com>

On 12 February 2018 at 23:17, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> Martin Ågren <martin.agren@gmail.com> writes:
>
>> +test_expect_success TTY 'git config respects pager.config when setting' '
>> +     rm -f paginated.out &&
>> +     test_terminal git -c pager.config config foo.bar bar &&
>> +     test -e paginated.out
>> +'
>
> I am debating myself if this test should instead spell out what we
> eventually want from the above test and make it expect_failure, just
> like the next one.

That's a valid point. I was coming at it from the point of view of "the current behavior is well-defined and non-broken -- we'll soon change it, but that's more a redefinition, not a bugfix (as with --edit)". But I could go either way.

There is some prior art in ma/branch-list-paginate, where I handled `git branch --set-upstream-to` similar to here, cf. d74b541e0b (branch: respect `pager.branch` in list-mode only, 2017-11-19).

> In addition to setting (which will start ignoring pager in later
> steps), unsetting, replacing of a variable and renaming/removing a
> section in a configuration should not page, I would suspect.  Should
> we test them all?

I actually had several more tests in an early draft, including --unset. Similarly, testing all the --get-* would be possible but feels like overkill. From the implementation, it's "obvious enough" (famous last words) that there are two classes of arguments, and by testing a few from each class we should be home free.

This now comes to `git config` after `git tag` and `git branch`, where the "complexity" of the problem has been steadily increasing. (Off the top of my head, tag has two modes, branch has three, config has lots.) That the tests don't get more complex might be bad, or good. But all of these use the same basic API (DELAY_PAGER_CONFIG) in the same rather simple way. I actually almost had the feeling that these tests here were too much, considering that DELAY_PAGER_CONFIG gets tested quite a few times by now.

Thanks for your comments. I'll ponder this, and see what I come up with. Maybe a changed approach, or maybe an extended commit message. Any further input welcome, as always.

Martin