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

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

[Martin Ågren]

The next couple of commits will change how `git config` handles
`pager.config`, similar to how de121ffe5 (tag: respect `pager.tag` in
list-mode only, 2017-08-02) and ff1e72483 (tag: change default of
`pager.tag` to "on", 2017-08-02) changed `git tag`. Similar work has
also been done to `git branch`.

Add tests in this area to make sure that we don't regress and so that
the upcoming commits can be made clearer by adapting the tests. Add some
tests for `--list` and `--get`, one for `--edit`, and one for simple
config-setting.

In particular, use `test_expect_failure` to document that we currently
respect the pager-configuration with `--edit`. The current behavior is
buggy since the pager interferes with the editor and makes the end
result completely broken. See also b3ee740c8 (t7006: add tests for how
git tag paginates, 2017-08-02).

Remove the test added in commit 3ba7e6e29a (config: run
setup_git_directory_gently() sooner, 2010-08-05) since it has some
overlap with these. We could leave it or tweak it, or place new tests
like these next to it, but let's instead make the tests for `git config`
similar to the ones for `git tag` and `git branch`, and place them after
those.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t7006-pager.sh | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f5f46a95b4..5a7b757c94 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -110,13 +110,6 @@ test_expect_success TTY 'configuration can disable pager' '
 	! test -e paginated.out
 '
 
-test_expect_success TTY 'git config uses a pager if configured to' '
-	rm -f paginated.out &&
-	test_config pager.config true &&
-	test_terminal git config --list &&
-	test -e paginated.out
-'
-
 test_expect_success TTY 'configuration can enable pager (from subdir)' '
 	rm -f paginated.out &&
 	mkdir -p subdir &&
@@ -252,6 +245,41 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
 	! test -e paginated.out
 '
 
+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
+'
+
+test_expect_failure TTY 'git config --edit ignores pager.config' '
+	rm -f paginated.out editor.used &&
+	write_script editor <<-\EOF &&
+		touch editor.used
+	EOF
+	EDITOR=./editor test_terminal git -c pager.config config --edit &&
+	! test -e paginated.out &&
+	test -e editor.used
+'
+
+test_expect_success TTY 'git config --get defaults to not paging' '
+	rm -f paginated.out &&
+	test_terminal git config --get foo.bar &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git config --get respects pager.config' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.config config --get foo.bar &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git config --list defaults to not paging' '
+	rm -f paginated.out &&
+	test_terminal git config --list &&
+	! test -e paginated.out
+'
+
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
-- 
2.16.1.72.g5be1f00a9a

[PATCH 2/3] config: respect `pager.config` in list/get-mode only

[Martin Ågren]

Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,
2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect
`pager.config` when we are listing or "get"ing config.

Some getters give at most one line of output, but it is much easier to
document and understand that we page all of --get[-*] and --list, than
to divide the (current and future) getters into "pages" and "doesn't".

This fixes the failing test added in the previous commit. Also adapt the
test for whether `git config foo.bar bar` respects `pager.config`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-config.txt | 5 +++++
 t/t7006-pager.sh             | 6 +++---
 builtin/config.c             | 8 ++++++++
 git.c                        | 2 +-
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 14da5fc157..ccc8f0bcff 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -233,6 +233,11 @@ See also <<FILES>>.
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.
 
+CONFIGURATION
+-------------
+`pager.config` is only respected when listing configuration, i.e., when
+`--list`, `--get` or any of `--get-*` is used.
+
 [[FILES]]
 FILES
 -----
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 5a7b757c94..869a0359a8 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -245,13 +245,13 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
 	! test -e paginated.out
 '
 
-test_expect_success TTY 'git config respects pager.config when setting' '
+test_expect_success TTY 'git config ignores pager.config when setting' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.config config foo.bar bar &&
-	test -e paginated.out
+	! test -e paginated.out
 '
 
-test_expect_failure TTY 'git config --edit ignores pager.config' '
+test_expect_success TTY 'git config --edit ignores pager.config' '
 	rm -f paginated.out editor.used &&
 	write_script editor <<-\EOF &&
 		touch editor.used
diff --git a/builtin/config.c b/builtin/config.c
index ab5f95476e..9a57a0caff 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -48,6 +48,11 @@ static int show_origin;
 #define ACTION_GET_COLORBOOL (1<<14)
 #define ACTION_GET_URLMATCH (1<<15)
 
+/* convenience macro for "ACTION_LIST | ACTION_GET_*" */
+#define ACTION_LIST_OR_GET (ACTION_LIST | ACTION_GET | ACTION_GET_ALL | \
+			    ACTION_GET_REGEXP | ACTION_GET_COLOR | \
+			    ACTION_GET_COLORBOOL | ACTION_GET_URLMATCH)
+
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
 #define TYPE_BOOL_OR_INT (1<<2)
@@ -594,6 +599,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (actions & ACTION_LIST_OR_GET)
+		setup_auto_pager("config", 0);
+
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
 		if (config_with_options(show_all_config, NULL,
diff --git a/git.c b/git.c
index c870b9719c..e5c9b8729d 100644
--- a/git.c
+++ b/git.c
@@ -389,7 +389,7 @@ static struct cmd_struct commands[] = {
 	{ "column", cmd_column, RUN_SETUP_GENTLY },
 	{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
 	{ "commit-tree", cmd_commit_tree, RUN_SETUP },
-	{ "config", cmd_config, RUN_SETUP_GENTLY },
+	{ "config", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },
 	{ "count-objects", cmd_count_objects, RUN_SETUP },
 	{ "credential", cmd_credential, RUN_SETUP_GENTLY },
 	{ "describe", cmd_describe, RUN_SETUP },
-- 
2.16.1.72.g5be1f00a9a

Re: [PATCH 2/3] config: respect `pager.config` in list/get-mode only

[Duy Nguyen]

On Sun, Feb 11, 2018 at 11:40 PM, Martin Ågren <martin.agren@gmail.com> wrote:
> Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,
> 2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect
> `pager.config` when we are listing or "get"ing config.
>
> Some getters give at most one line of output, but it is much easier to
> document and understand that we page all of --get[-*] and --list, than
> to divide the (current and future) getters into "pages" and "doesn't".

I realize modern pagers like 'less' can automatically exit if the
output is less than a screen. But are we sure it's true for all
pagers? It would be annoying to have a pager waiting for me to exit
when I only want to check one config item out (which prints one line).
Trading one-time convenience at reading the manual with constantly
pressing 'q' does not seem justified.
-- 
Duy

Re: [PATCH 2/3] config: respect `pager.config` in list/get-mode only

[Martin Ågren]

On 13 February 2018 at 11:25, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Feb 11, 2018 at 11:40 PM, Martin Ågren <martin.agren@gmail.com> wrote:
>> Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,
>> 2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect
>> `pager.config` when we are listing or "get"ing config.
>>
>> Some getters give at most one line of output, but it is much easier to
>> document and understand that we page all of --get[-*] and --list, than
>> to divide the (current and future) getters into "pages" and "doesn't".
>
> I realize modern pagers like 'less' can automatically exit if the
> output is less than a screen. But are we sure it's true for all
> pagers? It would be annoying to have a pager waiting for me to exit
> when I only want to check one config item out (which prints one line).
> Trading one-time convenience at reading the manual with constantly
> pressing 'q' does not seem justified.

Well, there was one recent instance of a misconfigured LESS causing the
pager not to quit automatically [1]. Your "Trading"-sentence does argue
nicely for rethinking my approach here.

A tweaked behavior could be documented as something like:

    `pager.config` is only respected when listing configuration, i.e.,
    when using `--list` or any of the `--get-*` which may return
    multiple results.

Maybe it doesn't look to complicated after all. I'd rather not give any
ideas about how we only page if there *are* more than one line of
result, i.e., that we'd examine the result before turning on the pager.
I think I've avoided that misconception here.

Thanks
Martin

[1] https://public-inbox.org/git/2412A603-4382-4AF5-97D0-D16D5FAAFE28@eluvio.com/

[PATCH 3/3] config: change default of `pager.config` to "on"

[Martin Ågren]

This is similar to ff1e72483 (tag: change default of `pager.tag` to
"on", 2017-08-02) and is safe now that we do not consider `pager.config`
at all when we are not listing or getting configuration. This change
will help with listing large configurations, but will not hurt users of
`git config --edit` as it would have before the previous commit.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-config.txt |  2 +-
 t/t7006-pager.sh             | 12 ++++++------
 builtin/config.c             |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index ccc8f0bcff..78074cf3b2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -236,7 +236,7 @@ See also <<FILES>>.
 CONFIGURATION
 -------------
 `pager.config` is only respected when listing configuration, i.e., when
-`--list`, `--get` or any of `--get-*` is used.
+`--list`, `--get` or any of `--get-*` is used. The default is to use a pager.
 
 [[FILES]]
 FILES
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 869a0359a8..47f7830eb1 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -261,22 +261,22 @@ test_expect_success TTY 'git config --edit ignores pager.config' '
 	test -e editor.used
 '
 
-test_expect_success TTY 'git config --get defaults to not paging' '
+test_expect_success TTY 'git config --get defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git config --get foo.bar &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 test_expect_success TTY 'git config --get respects pager.config' '
 	rm -f paginated.out &&
-	test_terminal git -c pager.config config --get foo.bar &&
-	test -e paginated.out
+	test_terminal git -c pager.config=false config --get foo.bar &&
+	! test -e paginated.out
 '
 
-test_expect_success TTY 'git config --list defaults to not paging' '
+test_expect_success TTY 'git config --list defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git config --list &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 
diff --git a/builtin/config.c b/builtin/config.c
index 9a57a0caff..61808a93cb 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -600,7 +600,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (actions & ACTION_LIST_OR_GET)
-		setup_auto_pager("config", 0);
+		setup_auto_pager("config", 1);
 
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
-- 
2.16.1.72.g5be1f00a9a

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

[Junio C Hamano]

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.

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?

> +test_expect_failure TTY 'git config --edit ignores pager.config' '
> +	rm -f paginated.out editor.used &&
> +	write_script editor <<-\EOF &&
> +		touch editor.used
> +	EOF
> +	EDITOR=./editor test_terminal git -c pager.config config --edit &&
> +	! test -e paginated.out &&
> +	test -e editor.used
> +'
> +
> +test_expect_success TTY 'git config --get defaults to not paging' '
> +	rm -f paginated.out &&
> +	test_terminal git config --get foo.bar &&
> +	! test -e paginated.out
> +'
> +
> +test_expect_success TTY 'git config --get respects pager.config' '
> +	rm -f paginated.out &&
> +	test_terminal git -c pager.config config --get foo.bar &&
> +	test -e paginated.out
> +'
> +
> +test_expect_success TTY 'git config --list defaults to not paging' '
> +	rm -f paginated.out &&
> +	test_terminal git config --list &&
> +	! test -e paginated.out
> +'
> +
> +
>  # A colored commit log will begin with an appropriate ANSI escape
>  # for the first color; the text "commit" comes later.
>  colorful() {

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

[Martin Ågren]

On 12 February 2018 at 23:17, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Martin Ågren]

This is v2 of my series to teach `git config` to only respect
`pager.config` when listing configuration, then changing the default to
"on". Thanks to Duy and Junio for feedback on the first version.

Based on Duy's feeback, I've changed the approach to more carefully
divide the various getters into "may produce multiple lines, so let's
page" vs "may not, so don't".

Junio hesitated whether we should add tests using `test_expect_success`,
then flip the test-definition, or whether we should start with a
"failure" that we then flip to "success". I have not done anything about
that, except to try and motivate the choice better in the commit message
of the second patch.

Martin

Martin Ågren (3):
  t7006: add tests for how git config paginates
  config: respect `pager.config` in list/get-mode only
  config: change default of `pager.config` to "on"

 Documentation/git-config.txt |  6 ++++++
 t/t7006-pager.sh             | 49 +++++++++++++++++++++++++++++++++++++-------
 builtin/config.c             | 10 +++++++++
 git.c                        |  2 +-
 4 files changed, 59 insertions(+), 8 deletions(-)

-- 
2.16.2.246.ga4ee44448f

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

[Martin Ågren]

The next couple of commits will change how `git config` handles
`pager.config`, similar to how de121ffe5 (tag: respect `pager.tag` in
list-mode only, 2017-08-02) and ff1e72483 (tag: change default of
`pager.tag` to "on", 2017-08-02) changed `git tag`. Similar work has
also been done to `git branch`.

Add tests in this area to make sure that we don't regress and so that
the upcoming commits can be made clearer by adapting the tests. Add
tests for simple config-setting, `--edit`, `--get`, `--get-urlmatch`,
`get-all`, and `--list`. Those represent a fair portion of the various
options that will be affected by the next two commits.

Use `test_expect_failure` to document that we currently respect the
pager-configuration with `--edit`. The current behavior is buggy since
the pager interferes with the editor and makes the end result completely
broken. See also b3ee740c8 (t7006: add tests for how git tag paginates,
2017-08-02).

The next commit will teach simple config-setting and `--get` to ignore
`pager.config`. Test the current behavior as "success", not "failure",
since the currently expected behavior according to documentation would
be to page. The next commit will change that expectation by updating the
documentation on `git config` and will redefine those successful tests.

Remove the test added in commit 3ba7e6e29a (config: run
setup_git_directory_gently() sooner, 2010-08-05) since it has some
overlap with these. We could leave it or tweak it, or place new tests
like these next to it, but let's instead make the tests for `git config`
as similar as possible to the ones for `git tag` and `git branch`, and
place them after those.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t7006-pager.sh | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f5f46a95b4..a46a079339 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -110,13 +110,6 @@ test_expect_success TTY 'configuration can disable pager' '
 	! test -e paginated.out
 '
 
-test_expect_success TTY 'git config uses a pager if configured to' '
-	rm -f paginated.out &&
-	test_config pager.config true &&
-	test_terminal git config --list &&
-	test -e paginated.out
-'
-
 test_expect_success TTY 'configuration can enable pager (from subdir)' '
 	rm -f paginated.out &&
 	mkdir -p subdir &&
@@ -252,6 +245,48 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
 	! test -e paginated.out
 '
 
+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
+'
+
+test_expect_failure TTY 'git config --edit ignores pager.config' '
+	rm -f paginated.out editor.used &&
+	write_script editor <<-\EOF &&
+		touch editor.used
+	EOF
+	EDITOR=./editor test_terminal git -c pager.config config --edit &&
+	! test -e paginated.out &&
+	test -e editor.used
+'
+
+test_expect_success TTY 'git config --get respects pager.config' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.config config --get foo.bar &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git config --get-urlmatch defaults to not paging' '
+	rm -f paginated.out &&
+	test_terminal git -c http."https://foo.com/".bar=foo \
+			  config --get-urlmatch http https://foo.com &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git config --get-all respects pager.config' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.config config --get-all foo.bar &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git config --list defaults to not paging' '
+	rm -f paginated.out &&
+	test_terminal git config --list &&
+	! test -e paginated.out
+'
+
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
-- 
2.16.2.246.ga4ee44448f

[PATCH v2 2/3] config: respect `pager.config` in list/get-mode only

[Martin Ågren]

Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,
2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect
`pager.config` when we are listing or "get"ing config.

We have several getters and some are guaranteed to give at most one line
of output. Paging all getters including those could be convenient from a
documentation point-of-view. The downside would be that a misconfigured
or not so modern pager might wait for user interaction before
terminating. Let's instead respect the config for precisely those
getters which may produce more than one line of output.

`--get-urlmatch` may or may not produce multiple lines of output,
depending on the exact usage. Let's not try to recognize the two modes,
but instead make `--get-urlmatch` always respect the config. Analyzing
the detailed usage might be trivial enough here, but could establish a
precedent that we will never be able to enforce throughout the codebase
and that will just open a can of worms.

This fixes the failing test added in the previous commit. Also adapt the
test for whether `git config foo.bar bar` and `git config --get foo.bar`
respects `pager.config`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-config.txt |  5 +++++
 t/t7006-pager.sh             | 10 +++++-----
 builtin/config.c             | 10 ++++++++++
 git.c                        |  2 +-
 4 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 14da5fc157..249090ac84 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -233,6 +233,11 @@ See also <<FILES>>.
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.
 
+CONFIGURATION
+-------------
+`pager.config` is only respected when listing configuration, i.e., when
+using `--list` or any of the `--get-*` which may return multiple results.
+
 [[FILES]]
 FILES
 -----
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index a46a079339..95bd26f0b2 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -245,13 +245,13 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
 	! test -e paginated.out
 '
 
-test_expect_success TTY 'git config respects pager.config when setting' '
+test_expect_success TTY 'git config ignores pager.config when setting' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.config config foo.bar bar &&
-	test -e paginated.out
+	! test -e paginated.out
 '
 
-test_expect_failure TTY 'git config --edit ignores pager.config' '
+test_expect_success TTY 'git config --edit ignores pager.config' '
 	rm -f paginated.out editor.used &&
 	write_script editor <<-\EOF &&
 		touch editor.used
@@ -261,10 +261,10 @@ test_expect_failure TTY 'git config --edit ignores pager.config' '
 	test -e editor.used
 '
 
-test_expect_success TTY 'git config --get respects pager.config' '
+test_expect_success TTY 'git config --get ignores pager.config' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.config config --get foo.bar &&
-	test -e paginated.out
+	! test -e paginated.out
 '
 
 test_expect_success TTY 'git config --get-urlmatch defaults to not paging' '
diff --git a/builtin/config.c b/builtin/config.c
index ab5f95476e..a732d9b56c 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -48,6 +48,13 @@ static int show_origin;
 #define ACTION_GET_COLORBOOL (1<<14)
 #define ACTION_GET_URLMATCH (1<<15)
 
+/*
+ * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than
+ * one line of output and which should therefore be paged.
+ */
+#define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
+			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
+
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
 #define TYPE_BOOL_OR_INT (1<<2)
@@ -594,6 +601,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (actions & PAGING_ACTIONS)
+		setup_auto_pager("config", 0);
+
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
 		if (config_with_options(show_all_config, NULL,
diff --git a/git.c b/git.c
index c870b9719c..e5c9b8729d 100644
--- a/git.c
+++ b/git.c
@@ -389,7 +389,7 @@ static struct cmd_struct commands[] = {
 	{ "column", cmd_column, RUN_SETUP_GENTLY },
 	{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
 	{ "commit-tree", cmd_commit_tree, RUN_SETUP },
-	{ "config", cmd_config, RUN_SETUP_GENTLY },
+	{ "config", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },
 	{ "count-objects", cmd_count_objects, RUN_SETUP },
 	{ "credential", cmd_credential, RUN_SETUP_GENTLY },
 	{ "describe", cmd_describe, RUN_SETUP },
-- 
2.16.2.246.ga4ee44448f

[PATCH v2 3/3] config: change default of `pager.config` to "on"

[Martin Ågren]

This is similar to ff1e72483 (tag: change default of `pager.tag` to
"on", 2017-08-02) and is safe now that we do not consider `pager.config`
at all when we are not listing or getting configuration. This change
will help with listing large configurations, but will not hurt users of
`git config --edit` as it would have before the previous commit.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-config.txt |  1 +
 t/t7006-pager.sh             | 12 ++++++------
 builtin/config.c             |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 249090ac84..e09ed5d7d5 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -237,6 +237,7 @@ CONFIGURATION
 -------------
 `pager.config` is only respected when listing configuration, i.e., when
 using `--list` or any of the `--get-*` which may return multiple results.
+The default is to use a pager.
 
 [[FILES]]
 FILES
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 95bd26f0b2..7541ba5edb 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -267,23 +267,23 @@ test_expect_success TTY 'git config --get ignores pager.config' '
 	! test -e paginated.out
 '
 
-test_expect_success TTY 'git config --get-urlmatch defaults to not paging' '
+test_expect_success TTY 'git config --get-urlmatch defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git -c http."https://foo.com/".bar=foo \
 			  config --get-urlmatch http https://foo.com &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 test_expect_success TTY 'git config --get-all respects pager.config' '
 	rm -f paginated.out &&
-	test_terminal git -c pager.config config --get-all foo.bar &&
-	test -e paginated.out
+	test_terminal git -c pager.config=false config --get-all foo.bar &&
+	! test -e paginated.out
 '
 
-test_expect_success TTY 'git config --list defaults to not paging' '
+test_expect_success TTY 'git config --list defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git config --list &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 
diff --git a/builtin/config.c b/builtin/config.c
index a732d9b56c..01169dd628 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -602,7 +602,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (actions & PAGING_ACTIONS)
-		setup_auto_pager("config", 0);
+		setup_auto_pager("config", 1);
 
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
-- 
2.16.2.246.ga4ee44448f

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

[Junio C Hamano]

Thanks.  The entire thing looked reasonable.

Will replace.