From: Martin Ågren <hidden> Date: 2018-02-11 16:40:39
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 <redacted>
---
t/t7006-pager.sh | 42 +++++++++++++++++++++++++++++++++++-------
1 file changed, 35 insertions(+), 7 deletions(-)
@@ -110,13 +110,6 @@ test_expect_success TTY 'configuration can disable pager' '!test-epaginated.out'-test_expect_successTTY'git config uses a pager if configured to''-rm-fpaginated.out&&-test_configpager.configtrue&&-test_terminalgitconfig--list&&-test-epaginated.out-'- test_expect_successTTY'configuration can enable pager (from subdir)''rm-fpaginated.out&&mkdir-psubdir&&
@@ -252,6 +245,41 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '!test-epaginated.out'+test_expect_successTTY'git config respects pager.config when setting''+rm-fpaginated.out&&+test_terminalgit-cpager.configconfigfoo.barbar&&+test-epaginated.out+'++test_expect_failureTTY'git config --edit ignores pager.config''+rm-fpaginated.outeditor.used&&+write_scripteditor<<-\EOF&&+toucheditor.used+EOF+EDITOR=./editortest_terminalgit-cpager.configconfig--edit&&+!test-epaginated.out&&+test-eeditor.used+'++test_expect_successTTY'git config --get defaults to not paging''+rm-fpaginated.out&&+test_terminalgitconfig--getfoo.bar&&+!test-epaginated.out+'++test_expect_successTTY'git config --get respects pager.config''+rm-fpaginated.out&&+test_terminalgit-cpager.configconfig--getfoo.bar&&+test-epaginated.out+'++test_expect_successTTY'git config --list defaults to not paging''+rm-fpaginated.out&&+test_terminalgitconfig--list&&+!test-epaginated.out+'++# A colored commit log will begin with an appropriate ANSI escape# for the first color; the text "commit" comes later. colorful(){
From: Martin Ågren <hidden> Date: 2018-02-11 16:40:45
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 <redacted>
---
Documentation/git-config.txt | 5 +++++
t/t7006-pager.sh | 6 +++---
builtin/config.c | 8 ++++++++
git.c | 2 +-
4 files changed, 17 insertions(+), 4 deletions(-)
@@ -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 -----
From: Martin Ågren <hidden> Date: 2018-02-11 16:40:49
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 <redacted>
---
Documentation/git-config.txt | 2 +-
t/t7006-pager.sh | 12 ++++++------
builtin/config.c | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
@@ -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
On Sun, Feb 11, 2018 at 11:40 PM, Martin Ågren [off-list ref] 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
From: Martin Ågren <hidden> Date: 2018-02-13 11:19:28
On 13 February 2018 at 11:25, Duy Nguyen [off-list ref] wrote:
On Sun, Feb 11, 2018 at 11:40 PM, Martin Ågren [off-list ref] wrote:
quoted
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/
From: Martin Ågren <hidden> Date: 2018-02-21 18:52:04
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
From: Martin Ågren <hidden> Date: 2018-02-21 18:52:07
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 <redacted>
---
t/t7006-pager.sh | 49 ++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 42 insertions(+), 7 deletions(-)
@@ -110,13 +110,6 @@ test_expect_success TTY 'configuration can disable pager' '!test-epaginated.out'-test_expect_successTTY'git config uses a pager if configured to''-rm-fpaginated.out&&-test_configpager.configtrue&&-test_terminalgitconfig--list&&-test-epaginated.out-'- test_expect_successTTY'configuration can enable pager (from subdir)''rm-fpaginated.out&&mkdir-psubdir&&
@@ -252,6 +245,48 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '!test-epaginated.out'+test_expect_successTTY'git config respects pager.config when setting''+rm-fpaginated.out&&+test_terminalgit-cpager.configconfigfoo.barbar&&+test-epaginated.out+'++test_expect_failureTTY'git config --edit ignores pager.config''+rm-fpaginated.outeditor.used&&+write_scripteditor<<-\EOF&&+toucheditor.used+EOF+EDITOR=./editortest_terminalgit-cpager.configconfig--edit&&+!test-epaginated.out&&+test-eeditor.used+'++test_expect_successTTY'git config --get respects pager.config''+rm-fpaginated.out&&+test_terminalgit-cpager.configconfig--getfoo.bar&&+test-epaginated.out+'++test_expect_successTTY'git config --get-urlmatch defaults to not paging''+rm-fpaginated.out&&+test_terminalgit-chttp."https://foo.com/".bar=foo\+config--get-urlmatchhttphttps://foo.com&&+!test-epaginated.out+'++test_expect_successTTY'git config --get-all respects pager.config''+rm-fpaginated.out&&+test_terminalgit-cpager.configconfig--get-allfoo.bar&&+test-epaginated.out+'++test_expect_successTTY'git config --list defaults to not paging''+rm-fpaginated.out&&+test_terminalgitconfig--list&&+!test-epaginated.out+'++# A colored commit log will begin with an appropriate ANSI escape# for the first color; the text "commit" comes later. colorful(){
From: Martin Ågren <hidden> Date: 2018-02-21 18:52:11
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 <redacted>
---
Documentation/git-config.txt | 5 +++++
t/t7006-pager.sh | 10 +++++-----
builtin/config.c | 10 ++++++++++
git.c | 2 +-
4 files changed, 21 insertions(+), 6 deletions(-)
@@ -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 -----
From: Martin Ågren <hidden> Date: 2018-02-21 18:52:16
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 <redacted>
---
Documentation/git-config.txt | 1 +
t/t7006-pager.sh | 12 ++++++------
builtin/config.c | 2 +-
3 files changed, 8 insertions(+), 7 deletions(-)
@@ -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