git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 1/3] t7006: add tests for how git config paginates
@ 2018-02-11 16:40 Martin Ågren
  2018-02-11 16:40 ` [PATCH 2/3] config: respect `pager.config` in list/get-mode only Martin Ågren
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Martin Ågren @ 2018-02-11 16:40 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/3] config: respect `pager.config` in list/get-mode only
  2018-02-11 16:40 [PATCH 1/3] t7006: add tests for how git config paginates Martin Ågren
@ 2018-02-11 16:40 ` Martin Ågren
  2018-02-13 10:25   ` Duy Nguyen
  2018-02-11 16:40 ` [PATCH 3/3] config: change default of `pager.config` to "on" Martin Ågren
  2018-02-12 22:17 ` [PATCH 1/3] t7006: add tests for how git config paginates Junio C Hamano
  2 siblings, 1 reply; 7+ messages in thread
From: Martin Ågren @ 2018-02-11 16:40 UTC (permalink / raw)
  To: git

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 3/3] config: change default of `pager.config` to "on"
  2018-02-11 16:40 [PATCH 1/3] t7006: add tests for how git config paginates Martin Ågren
  2018-02-11 16:40 ` [PATCH 2/3] config: respect `pager.config` in list/get-mode only Martin Ågren
@ 2018-02-11 16:40 ` Martin Ågren
  2018-02-12 22:17 ` [PATCH 1/3] t7006: add tests for how git config paginates Junio C Hamano
  2 siblings, 0 replies; 7+ messages in thread
From: Martin Ågren @ 2018-02-11 16:40 UTC (permalink / raw)
  To: git

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] t7006: add tests for how git config paginates
  2018-02-11 16:40 [PATCH 1/3] t7006: add tests for how git config paginates Martin Ågren
  2018-02-11 16:40 ` [PATCH 2/3] config: respect `pager.config` in list/get-mode only Martin Ågren
  2018-02-11 16:40 ` [PATCH 3/3] config: change default of `pager.config` to "on" Martin Ågren
@ 2018-02-12 22:17 ` Junio C Hamano
  2018-02-12 22:41   ` Martin Ågren
  2 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2018-02-12 22:17 UTC (permalink / raw)
  To: Martin Ågren; +Cc: git, Nguyễn Thái Ngọc Duy

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() {

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] t7006: add tests for how git config paginates
  2018-02-12 22:17 ` [PATCH 1/3] t7006: add tests for how git config paginates Junio C Hamano
@ 2018-02-12 22:41   ` Martin Ågren
  0 siblings, 0 replies; 7+ messages in thread
From: Martin Ågren @ 2018-02-12 22:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, Nguyễn Thái Ngọc Duy

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/3] config: respect `pager.config` in list/get-mode only
  2018-02-11 16:40 ` [PATCH 2/3] config: respect `pager.config` in list/get-mode only Martin Ågren
@ 2018-02-13 10:25   ` Duy Nguyen
  2018-02-13 11:19     ` Martin Ågren
  0 siblings, 1 reply; 7+ messages in thread
From: Duy Nguyen @ 2018-02-13 10:25 UTC (permalink / raw)
  To: Martin Ågren; +Cc: Git Mailing List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/3] config: respect `pager.config` in list/get-mode only
  2018-02-13 10:25   ` Duy Nguyen
@ 2018-02-13 11:19     ` Martin Ågren
  0 siblings, 0 replies; 7+ messages in thread
From: Martin Ågren @ 2018-02-13 11:19 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List

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/

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-11 16:40 [PATCH 1/3] t7006: add tests for how git config paginates Martin Ågren
2018-02-11 16:40 ` [PATCH 2/3] config: respect `pager.config` in list/get-mode only Martin Ågren
2018-02-13 10:25   ` Duy Nguyen
2018-02-13 11:19     ` Martin Ågren
2018-02-11 16:40 ` [PATCH 3/3] config: change default of `pager.config` to "on" Martin Ågren
2018-02-12 22:17 ` [PATCH 1/3] t7006: add tests for how git config paginates Junio C Hamano
2018-02-12 22:41   ` Martin Ågren

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox