* [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 ` (3 more replies) 0 siblings, 4 replies; 12+ 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 related [flat|nested] 12+ 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 ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ 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 related [flat|nested] 12+ 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; 12+ 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] 12+ 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; 12+ 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] 12+ 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 2018-02-21 18:51 ` [PATCH v2 0/3] " Martin Ågren 3 siblings, 0 replies; 12+ 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 related [flat|nested] 12+ 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 2018-02-21 18:51 ` [PATCH v2 0/3] " Martin Ågren 3 siblings, 1 reply; 12+ 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] 12+ 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; 12+ 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] 12+ messages in thread
* [PATCH v2 0/3] Re: 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 ` (2 preceding siblings ...) 2018-02-12 22:17 ` [PATCH 1/3] t7006: add tests for how git config paginates Junio C Hamano @ 2018-02-21 18:51 ` Martin Ågren 2018-02-21 18:51 ` [PATCH v2 1/3] " Martin Ågren ` (3 more replies) 3 siblings, 4 replies; 12+ messages in thread From: Martin Ågren @ 2018-02-21 18:51 UTC (permalink / raw) To: git; +Cc: Duy Nguyen, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/3] t7006: add tests for how git config paginates 2018-02-21 18:51 ` [PATCH v2 0/3] " Martin Ågren @ 2018-02-21 18:51 ` Martin Ågren 2018-02-21 18:51 ` [PATCH v2 2/3] config: respect `pager.config` in list/get-mode only Martin Ågren ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Martin Ågren @ 2018-02-21 18:51 UTC (permalink / raw) To: git; +Cc: Duy Nguyen, Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] config: respect `pager.config` in list/get-mode only 2018-02-21 18:51 ` [PATCH v2 0/3] " Martin Ågren 2018-02-21 18:51 ` [PATCH v2 1/3] " Martin Ågren @ 2018-02-21 18:51 ` Martin Ågren 2018-02-21 18:51 ` [PATCH v2 3/3] config: change default of `pager.config` to "on" Martin Ågren 2018-02-21 22:35 ` [PATCH v2 0/3] Re: t7006: add tests for how git config paginates Junio C Hamano 3 siblings, 0 replies; 12+ messages in thread From: Martin Ågren @ 2018-02-21 18:51 UTC (permalink / raw) To: git; +Cc: Duy Nguyen, Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] config: change default of `pager.config` to "on" 2018-02-21 18:51 ` [PATCH v2 0/3] " Martin Ågren 2018-02-21 18:51 ` [PATCH v2 1/3] " Martin Ågren 2018-02-21 18:51 ` [PATCH v2 2/3] config: respect `pager.config` in list/get-mode only Martin Ågren @ 2018-02-21 18:51 ` Martin Ågren 2018-02-21 22:35 ` [PATCH v2 0/3] Re: t7006: add tests for how git config paginates Junio C Hamano 3 siblings, 0 replies; 12+ messages in thread From: Martin Ågren @ 2018-02-21 18:51 UTC (permalink / raw) To: git; +Cc: Duy Nguyen, Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/3] Re: t7006: add tests for how git config paginates 2018-02-21 18:51 ` [PATCH v2 0/3] " Martin Ågren ` (2 preceding siblings ...) 2018-02-21 18:51 ` [PATCH v2 3/3] config: change default of `pager.config` to "on" Martin Ågren @ 2018-02-21 22:35 ` Junio C Hamano 3 siblings, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2018-02-21 22:35 UTC (permalink / raw) To: Martin Ågren; +Cc: git, Duy Nguyen Thanks. The entire thing looked reasonable. Will replace. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-02-21 22:35 UTC | newest] Thread overview: 12+ 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 2018-02-21 18:51 ` [PATCH v2 0/3] " Martin Ågren 2018-02-21 18:51 ` [PATCH v2 1/3] " Martin Ågren 2018-02-21 18:51 ` [PATCH v2 2/3] config: respect `pager.config` in list/get-mode only Martin Ågren 2018-02-21 18:51 ` [PATCH v2 3/3] config: change default of `pager.config` to "on" Martin Ågren 2018-02-21 22:35 ` [PATCH v2 0/3] Re: t7006: add tests for how git config paginates Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).