{"thread":{"id":"47807","subject":"[PATCH 1/3] t7006: add tests for how git config paginates","startedAt":"2018-02-11T16:40:39Z","lastAt":"2018-02-21T22:35:42Z","messageCount":12,"participants":["Martin Ågren","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"339020","messageId":"450ff45707b58d6796d55cd594b50686e5d62fc2.1518365675.git.martin.agren@gmail.com","threadId":"47807","inReplyTo":null,"subject":"[PATCH 1/3] t7006: add tests for how git config paginates","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-11T16:40:23Z","receivedAt":"2018-02-11T16:40:39Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The next couple of commits will change how `git config` handles\n`pager.config`, similar to how de121ffe5 (tag: respect `pager.tag` in\nlist-mode only, 2017-08-02) and ff1e72483 (tag: change default of\n`pager.tag` to \"on\", 2017-08-02) changed `git tag`. Similar work has\nalso been done to `git branch`.\n\nAdd tests in this area to make sure that we don't regress and so that\nthe upcoming commits can be made clearer by adapting the tests. Add some\ntests for `--list` and `--get`, one for `--edit`, and one for simple\nconfig-setting.\n\nIn particular, use `test_expect_failure` to document that we currently\nrespect the pager-configuration with `--edit`. The current behavior is\nbuggy since the pager interferes with the editor and makes the end\nresult completely broken. See also b3ee740c8 (t7006: add tests for how\ngit tag paginates, 2017-08-02).\n\nRemove the test added in commit 3ba7e6e29a (config: run\nsetup_git_directory_gently() sooner, 2010-08-05) since it has some\noverlap with these. We could leave it or tweak it, or place new tests\nlike these next to it, but let's instead make the tests for `git config`\nsimilar to the ones for `git tag` and `git branch`, and place them after\nthose.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n t/t7006-pager.sh | 42 +++++++++++++++++++++++++++++++++++-------\n 1 file changed, 35 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex f5f46a95b4..5a7b757c94 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -110,13 +110,6 @@ test_expect_success TTY 'configuration can disable pager' '\n \t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git config uses a pager if configured to' '\n-\trm -f paginated.out &&\n-\ttest_config pager.config true &&\n-\ttest_terminal git config --list &&\n-\ttest -e paginated.out\n-'\n-\n test_expect_success TTY 'configuration can enable pager (from subdir)' '\n \trm -f paginated.out &&\n \tmkdir -p subdir &&\n@@ -252,6 +245,41 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '\n \t! test -e paginated.out\n '\n \n+test_expect_success TTY 'git config respects pager.config when setting' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.config config foo.bar bar &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_failure TTY 'git config --edit ignores pager.config' '\n+\trm -f paginated.out editor.used &&\n+\twrite_script editor <<-\\EOF &&\n+\t\ttouch editor.used\n+\tEOF\n+\tEDITOR=./editor test_terminal git -c pager.config config --edit &&\n+\t! test -e paginated.out &&\n+\ttest -e editor.used\n+'\n+\n+test_expect_success TTY 'git config --get defaults to not paging' '\n+\trm -f paginated.out &&\n+\ttest_terminal git config --get foo.bar &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git config --get respects pager.config' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.config config --get foo.bar &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git config --list defaults to not paging' '\n+\trm -f paginated.out &&\n+\ttest_terminal git config --list &&\n+\t! test -e paginated.out\n+'\n+\n+\n # A colored commit log will begin with an appropriate ANSI escape\n # for the first color; the text \"commit\" comes later.\n colorful() {\n-- \n2.16.1.72.g5be1f00a9a\n\n"},{"id":"339021","messageId":"ad9a12af60cdc6ff747337c4116fe41860f82701.1518365675.git.martin.agren@gmail.com","threadId":"47807","inReplyTo":"450ff45707b58d6796d55cd594b50686e5d62fc2.1518365675.git.martin.agren@gmail.com","subject":"[PATCH 2/3] config: respect `pager.config` in list/get-mode only","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-11T16:40:24Z","receivedAt":"2018-02-11T16:40:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,\n2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect\n`pager.config` when we are listing or \"get\"ing config.\n\nSome getters give at most one line of output, but it is much easier to\ndocument and understand that we page all of --get[-*] and --list, than\nto divide the (current and future) getters into \"pages\" and \"doesn't\".\n\nThis fixes the failing test added in the previous commit. Also adapt the\ntest for whether `git config foo.bar bar` respects `pager.config`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/git-config.txt | 5 +++++\n t/t7006-pager.sh             | 6 +++---\n builtin/config.c             | 8 ++++++++\n git.c                        | 2 +-\n 4 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 14da5fc157..ccc8f0bcff 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -233,6 +233,11 @@ See also <<FILES>>.\n \tusing `--file`, `--global`, etc) and `on` when searching all\n \tconfig files.\n \n+CONFIGURATION\n+-------------\n+`pager.config` is only respected when listing configuration, i.e., when\n+`--list`, `--get` or any of `--get-*` is used.\n+\n [[FILES]]\n FILES\n -----\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 5a7b757c94..869a0359a8 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -245,13 +245,13 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '\n \t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git config respects pager.config when setting' '\n+test_expect_success TTY 'git config ignores pager.config when setting' '\n \trm -f paginated.out &&\n \ttest_terminal git -c pager.config config foo.bar bar &&\n-\ttest -e paginated.out\n+\t! test -e paginated.out\n '\n \n-test_expect_failure TTY 'git config --edit ignores pager.config' '\n+test_expect_success TTY 'git config --edit ignores pager.config' '\n \trm -f paginated.out editor.used &&\n \twrite_script editor <<-\\EOF &&\n \t\ttouch editor.used\ndiff --git a/builtin/config.c b/builtin/config.c\nindex ab5f95476e..9a57a0caff 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -48,6 +48,11 @@ static int show_origin;\n #define ACTION_GET_COLORBOOL (1<<14)\n #define ACTION_GET_URLMATCH (1<<15)\n \n+/* convenience macro for \"ACTION_LIST | ACTION_GET_*\" */\n+#define ACTION_LIST_OR_GET (ACTION_LIST | ACTION_GET | ACTION_GET_ALL | \\\n+\t\t\t    ACTION_GET_REGEXP | ACTION_GET_COLOR | \\\n+\t\t\t    ACTION_GET_COLORBOOL | ACTION_GET_URLMATCH)\n+\n #define TYPE_BOOL (1<<0)\n #define TYPE_INT (1<<1)\n #define TYPE_BOOL_OR_INT (1<<2)\n@@ -594,6 +599,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n \n+\tif (actions & ACTION_LIST_OR_GET)\n+\t\tsetup_auto_pager(\"config\", 0);\n+\n \tif (actions == ACTION_LIST) {\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (config_with_options(show_all_config, NULL,\ndiff --git a/git.c b/git.c\nindex c870b9719c..e5c9b8729d 100644\n--- a/git.c\n+++ b/git.c\n@@ -389,7 +389,7 @@ static struct cmd_struct commands[] = {\n \t{ \"column\", cmd_column, RUN_SETUP_GENTLY },\n \t{ \"commit\", cmd_commit, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"commit-tree\", cmd_commit_tree, RUN_SETUP },\n-\t{ \"config\", cmd_config, RUN_SETUP_GENTLY },\n+\t{ \"config\", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },\n \t{ \"count-objects\", cmd_count_objects, RUN_SETUP },\n \t{ \"credential\", cmd_credential, RUN_SETUP_GENTLY },\n \t{ \"describe\", cmd_describe, RUN_SETUP },\n-- \n2.16.1.72.g5be1f00a9a\n\n"},{"id":"339022","messageId":"cb4440e4f910f7095f16144afcc5963445211482.1518365675.git.martin.agren@gmail.com","threadId":"47807","inReplyTo":"450ff45707b58d6796d55cd594b50686e5d62fc2.1518365675.git.martin.agren@gmail.com","subject":"[PATCH 3/3] config: change default of `pager.config` to \"on\"","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-11T16:40:25Z","receivedAt":"2018-02-11T16:40:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is similar to ff1e72483 (tag: change default of `pager.tag` to\n\"on\", 2017-08-02) and is safe now that we do not consider `pager.config`\nat all when we are not listing or getting configuration. This change\nwill help with listing large configurations, but will not hurt users of\n`git config --edit` as it would have before the previous commit.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/git-config.txt |  2 +-\n t/t7006-pager.sh             | 12 ++++++------\n builtin/config.c             |  2 +-\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex ccc8f0bcff..78074cf3b2 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -236,7 +236,7 @@ See also <<FILES>>.\n CONFIGURATION\n -------------\n `pager.config` is only respected when listing configuration, i.e., when\n-`--list`, `--get` or any of `--get-*` is used.\n+`--list`, `--get` or any of `--get-*` is used. The default is to use a pager.\n \n [[FILES]]\n FILES\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 869a0359a8..47f7830eb1 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -261,22 +261,22 @@ test_expect_success TTY 'git config --edit ignores pager.config' '\n \ttest -e editor.used\n '\n \n-test_expect_success TTY 'git config --get defaults to not paging' '\n+test_expect_success TTY 'git config --get defaults to paging' '\n \trm -f paginated.out &&\n \ttest_terminal git config --get foo.bar &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n test_expect_success TTY 'git config --get respects pager.config' '\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.config config --get foo.bar &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.config=false config --get foo.bar &&\n+\t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git config --list defaults to not paging' '\n+test_expect_success TTY 'git config --list defaults to paging' '\n \trm -f paginated.out &&\n \ttest_terminal git config --list &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n \ndiff --git a/builtin/config.c b/builtin/config.c\nindex 9a57a0caff..61808a93cb 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -600,7 +600,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (actions & ACTION_LIST_OR_GET)\n-\t\tsetup_auto_pager(\"config\", 0);\n+\t\tsetup_auto_pager(\"config\", 1);\n \n \tif (actions == ACTION_LIST) {\n \t\tcheck_argc(argc, 0, 0);\n-- \n2.16.1.72.g5be1f00a9a\n\n"},{"id":"339124","messageId":"xmqq4lmlsvhr.fsf@gitster-ct.c.googlers.com","threadId":"47807","inReplyTo":"450ff45707b58d6796d55cd594b50686e5d62fc2.1518365675.git.martin.agren@gmail.com","subject":"Re: [PATCH 1/3] t7006: add tests for how git config paginates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-12T22:17:52Z","receivedAt":"2018-02-12T22:18:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> +test_expect_success TTY 'git config respects pager.config when setting' '\n> +\trm -f paginated.out &&\n> +\ttest_terminal git -c pager.config config foo.bar bar &&\n> +\ttest -e paginated.out\n> +'\n\nI am debating myself if this test should instead spell out what we\neventually want from the above test and make it expect_failure, just\nlike the next one.\n\nIn addition to setting (which will start ignoring pager in later\nsteps), unsetting, replacing of a variable and renaming/removing a\nsection in a configuration should not page, I would suspect.  Should\nwe test them all?\n\n> +test_expect_failure TTY 'git config --edit ignores pager.config' '\n> +\trm -f paginated.out editor.used &&\n> +\twrite_script editor <<-\\EOF &&\n> +\t\ttouch editor.used\n> +\tEOF\n> +\tEDITOR=./editor test_terminal git -c pager.config config --edit &&\n> +\t! test -e paginated.out &&\n> +\ttest -e editor.used\n> +'\n> +\n> +test_expect_success TTY 'git config --get defaults to not paging' '\n> +\trm -f paginated.out &&\n> +\ttest_terminal git config --get foo.bar &&\n> +\t! test -e paginated.out\n> +'\n> +\n> +test_expect_success TTY 'git config --get respects pager.config' '\n> +\trm -f paginated.out &&\n> +\ttest_terminal git -c pager.config config --get foo.bar &&\n> +\ttest -e paginated.out\n> +'\n> +\n> +test_expect_success TTY 'git config --list defaults to not paging' '\n> +\trm -f paginated.out &&\n> +\ttest_terminal git config --list &&\n> +\t! test -e paginated.out\n> +'\n> +\n> +\n>  # A colored commit log will begin with an appropriate ANSI escape\n>  # for the first color; the text \"commit\" comes later.\n>  colorful() {\n"},{"id":"339125","messageId":"CAN0heSr5bRx7Lrzu4mfeHx3SuZ75bgpXUA75kM1HJ-h=+8dgPQ@mail.gmail.com","threadId":"47807","inReplyTo":"xmqq4lmlsvhr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] t7006: add tests for how git config paginates","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-12T22:41:37Z","receivedAt":"2018-02-12T22:41:43Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 12 February 2018 at 23:17, Junio C Hamano <gitster@pobox.com> wrote:\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n>> +test_expect_success TTY 'git config respects pager.config when setting' '\n>> +     rm -f paginated.out &&\n>> +     test_terminal git -c pager.config config foo.bar bar &&\n>> +     test -e paginated.out\n>> +'\n>\n> I am debating myself if this test should instead spell out what we\n> eventually want from the above test and make it expect_failure, just\n> like the next one.\n\nThat's a valid point. I was coming at it from the point of view of \"the\ncurrent behavior is well-defined and non-broken -- we'll soon change it,\nbut that's more a redefinition, not a bugfix (as with --edit)\". But I\ncould go either way.\n\nThere is some prior art in ma/branch-list-paginate, where I handled `git\nbranch --set-upstream-to` similar to here, cf. d74b541e0b (branch:\nrespect `pager.branch` in list-mode only, 2017-11-19).\n\n> In addition to setting (which will start ignoring pager in later\n> steps), unsetting, replacing of a variable and renaming/removing a\n> section in a configuration should not page, I would suspect.  Should\n> we test them all?\n\nI actually had several more tests in an early draft, including --unset.\nSimilarly, testing all the --get-* would be possible but feels like\noverkill.  From the implementation, it's \"obvious enough\" (famous last\nwords) that there are two classes of arguments, and by testing a few\nfrom each class we should be home free.\n\nThis now comes to `git config` after `git tag` and `git branch`, where\nthe \"complexity\" of the problem has been steadily increasing. (Off the\ntop of my head, tag has two modes, branch has three, config has lots.)\nThat the tests don't get more complex might be bad, or good. But all of\nthese use the same basic API (DELAY_PAGER_CONFIG) in the same rather\nsimple way. I actually almost had the feeling that these tests here were\ntoo much, considering that DELAY_PAGER_CONFIG gets tested quite a few\ntimes by now.\n\nThanks for your comments. I'll ponder this, and see what I come up with.\nMaybe a changed approach, or maybe an extended commit message. Any\nfurther input welcome, as always.\n\nMartin\n"},{"id":"339178","messageId":"CACsJy8BPy_D4pg3n+c6eL_W=9eqeA++V4TOhp3PgOo_++YGhVw@mail.gmail.com","threadId":"47807","inReplyTo":"ad9a12af60cdc6ff747337c4116fe41860f82701.1518365675.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/3] config: respect `pager.config` in list/get-mode only","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-13T10:25:21Z","receivedAt":"2018-02-13T10:26:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 11, 2018 at 11:40 PM, Martin Ågren <martin.agren@gmail.com> wrote:\n> Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,\n> 2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect\n> `pager.config` when we are listing or \"get\"ing config.\n>\n> Some getters give at most one line of output, but it is much easier to\n> document and understand that we page all of --get[-*] and --list, than\n> to divide the (current and future) getters into \"pages\" and \"doesn't\".\n\nI realize modern pagers like 'less' can automatically exit if the\noutput is less than a screen. But are we sure it's true for all\npagers? It would be annoying to have a pager waiting for me to exit\nwhen I only want to check one config item out (which prints one line).\nTrading one-time convenience at reading the manual with constantly\npressing 'q' does not seem justified.\n-- \nDuy\n"},{"id":"339188","messageId":"CAN0heSoDA1bAupV1ZNAF_2K6Op9tM6QGJFCC8RT8WkgQO_EQZg@mail.gmail.com","threadId":"47807","inReplyTo":"CACsJy8BPy_D4pg3n+c6eL_W=9eqeA++V4TOhp3PgOo_++YGhVw@mail.gmail.com","subject":"Re: [PATCH 2/3] config: respect `pager.config` in list/get-mode only","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-13T11:19:21Z","receivedAt":"2018-02-13T11:19:28Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 13 February 2018 at 11:25, Duy Nguyen <pclouds@gmail.com> wrote:\n> On Sun, Feb 11, 2018 at 11:40 PM, Martin Ågren <martin.agren@gmail.com> wrote:\n>> Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,\n>> 2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect\n>> `pager.config` when we are listing or \"get\"ing config.\n>>\n>> Some getters give at most one line of output, but it is much easier to\n>> document and understand that we page all of --get[-*] and --list, than\n>> to divide the (current and future) getters into \"pages\" and \"doesn't\".\n>\n> I realize modern pagers like 'less' can automatically exit if the\n> output is less than a screen. But are we sure it's true for all\n> pagers? It would be annoying to have a pager waiting for me to exit\n> when I only want to check one config item out (which prints one line).\n> Trading one-time convenience at reading the manual with constantly\n> pressing 'q' does not seem justified.\n\nWell, there was one recent instance of a misconfigured LESS causing the\npager not to quit automatically [1]. Your \"Trading\"-sentence does argue\nnicely for rethinking my approach here.\n\nA tweaked behavior could be documented as something like:\n\n    `pager.config` is only respected when listing configuration, i.e.,\n    when using `--list` or any of the `--get-*` which may return\n    multiple results.\n\nMaybe it doesn't look to complicated after all. I'd rather not give any\nideas about how we only page if there *are* more than one line of\nresult, i.e., that we'd examine the result before turning on the pager.\nI think I've avoided that misconception here.\n\nThanks\nMartin\n\n[1] https://public-inbox.org/git/2412A603-4382-4AF5-97D0-D16D5FAAFE28@eluvio.com/\n"},{"id":"339820","messageId":"cover.1519238410.git.martin.agren@gmail.com","threadId":"47807","inReplyTo":"450ff45707b58d6796d55cd594b50686e5d62fc2.1518365675.git.martin.agren@gmail.com","subject":"[PATCH v2 0/3] Re: t7006: add tests for how git config paginates","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-21T18:51:41Z","receivedAt":"2018-02-21T18:52:04Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is v2 of my series to teach `git config` to only respect\n`pager.config` when listing configuration, then changing the default to\n\"on\". Thanks to Duy and Junio for feedback on the first version.\n\nBased on Duy's feeback, I've changed the approach to more carefully\ndivide the various getters into \"may produce multiple lines, so let's\npage\" vs \"may not, so don't\".\n\nJunio hesitated whether we should add tests using `test_expect_success`,\nthen flip the test-definition, or whether we should start with a\n\"failure\" that we then flip to \"success\". I have not done anything about\nthat, except to try and motivate the choice better in the commit message\nof the second patch.\n\nMartin\n\nMartin Ågren (3):\n  t7006: add tests for how git config paginates\n  config: respect `pager.config` in list/get-mode only\n  config: change default of `pager.config` to \"on\"\n\n Documentation/git-config.txt |  6 ++++++\n t/t7006-pager.sh             | 49 +++++++++++++++++++++++++++++++++++++-------\n builtin/config.c             | 10 +++++++++\n git.c                        |  2 +-\n 4 files changed, 59 insertions(+), 8 deletions(-)\n\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"339821","messageId":"e16c55763bb667d0d332f0110a8e31dbc9286b30.1519238410.git.martin.agren@gmail.com","threadId":"47807","inReplyTo":"cover.1519238410.git.martin.agren@gmail.com","subject":"[PATCH v2 1/3] t7006: add tests for how git config paginates","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-21T18:51:42Z","receivedAt":"2018-02-21T18:52:07Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The next couple of commits will change how `git config` handles\n`pager.config`, similar to how de121ffe5 (tag: respect `pager.tag` in\nlist-mode only, 2017-08-02) and ff1e72483 (tag: change default of\n`pager.tag` to \"on\", 2017-08-02) changed `git tag`. Similar work has\nalso been done to `git branch`.\n\nAdd tests in this area to make sure that we don't regress and so that\nthe upcoming commits can be made clearer by adapting the tests. Add\ntests for simple config-setting, `--edit`, `--get`, `--get-urlmatch`,\n`get-all`, and `--list`. Those represent a fair portion of the various\noptions that will be affected by the next two commits.\n\nUse `test_expect_failure` to document that we currently respect the\npager-configuration with `--edit`. The current behavior is buggy since\nthe pager interferes with the editor and makes the end result completely\nbroken. See also b3ee740c8 (t7006: add tests for how git tag paginates,\n2017-08-02).\n\nThe next commit will teach simple config-setting and `--get` to ignore\n`pager.config`. Test the current behavior as \"success\", not \"failure\",\nsince the currently expected behavior according to documentation would\nbe to page. The next commit will change that expectation by updating the\ndocumentation on `git config` and will redefine those successful tests.\n\nRemove the test added in commit 3ba7e6e29a (config: run\nsetup_git_directory_gently() sooner, 2010-08-05) since it has some\noverlap with these. We could leave it or tweak it, or place new tests\nlike these next to it, but let's instead make the tests for `git config`\nas similar as possible to the ones for `git tag` and `git branch`, and\nplace them after those.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n t/t7006-pager.sh | 49 ++++++++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 42 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex f5f46a95b4..a46a079339 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -110,13 +110,6 @@ test_expect_success TTY 'configuration can disable pager' '\n \t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git config uses a pager if configured to' '\n-\trm -f paginated.out &&\n-\ttest_config pager.config true &&\n-\ttest_terminal git config --list &&\n-\ttest -e paginated.out\n-'\n-\n test_expect_success TTY 'configuration can enable pager (from subdir)' '\n \trm -f paginated.out &&\n \tmkdir -p subdir &&\n@@ -252,6 +245,48 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '\n \t! test -e paginated.out\n '\n \n+test_expect_success TTY 'git config respects pager.config when setting' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.config config foo.bar bar &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_failure TTY 'git config --edit ignores pager.config' '\n+\trm -f paginated.out editor.used &&\n+\twrite_script editor <<-\\EOF &&\n+\t\ttouch editor.used\n+\tEOF\n+\tEDITOR=./editor test_terminal git -c pager.config config --edit &&\n+\t! test -e paginated.out &&\n+\ttest -e editor.used\n+'\n+\n+test_expect_success TTY 'git config --get respects pager.config' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.config config --get foo.bar &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git config --get-urlmatch defaults to not paging' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c http.\"https://foo.com/\".bar=foo \\\n+\t\t\t  config --get-urlmatch http https://foo.com &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git config --get-all respects pager.config' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.config config --get-all foo.bar &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git config --list defaults to not paging' '\n+\trm -f paginated.out &&\n+\ttest_terminal git config --list &&\n+\t! test -e paginated.out\n+'\n+\n+\n # A colored commit log will begin with an appropriate ANSI escape\n # for the first color; the text \"commit\" comes later.\n colorful() {\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"339822","messageId":"7ed0f6c3881535c2b87663ccc85705f5503ed8d2.1519238410.git.martin.agren@gmail.com","threadId":"47807","inReplyTo":"cover.1519238410.git.martin.agren@gmail.com","subject":"[PATCH v2 2/3] config: respect `pager.config` in list/get-mode only","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-21T18:51:43Z","receivedAt":"2018-02-21T18:52:11Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,\n2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect\n`pager.config` when we are listing or \"get\"ing config.\n\nWe have several getters and some are guaranteed to give at most one line\nof output. Paging all getters including those could be convenient from a\ndocumentation point-of-view. The downside would be that a misconfigured\nor not so modern pager might wait for user interaction before\nterminating. Let's instead respect the config for precisely those\ngetters which may produce more than one line of output.\n\n`--get-urlmatch` may or may not produce multiple lines of output,\ndepending on the exact usage. Let's not try to recognize the two modes,\nbut instead make `--get-urlmatch` always respect the config. Analyzing\nthe detailed usage might be trivial enough here, but could establish a\nprecedent that we will never be able to enforce throughout the codebase\nand that will just open a can of worms.\n\nThis fixes the failing test added in the previous commit. Also adapt the\ntest for whether `git config foo.bar bar` and `git config --get foo.bar`\nrespects `pager.config`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/git-config.txt |  5 +++++\n t/t7006-pager.sh             | 10 +++++-----\n builtin/config.c             | 10 ++++++++++\n git.c                        |  2 +-\n 4 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 14da5fc157..249090ac84 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -233,6 +233,11 @@ See also <<FILES>>.\n \tusing `--file`, `--global`, etc) and `on` when searching all\n \tconfig files.\n \n+CONFIGURATION\n+-------------\n+`pager.config` is only respected when listing configuration, i.e., when\n+using `--list` or any of the `--get-*` which may return multiple results.\n+\n [[FILES]]\n FILES\n -----\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex a46a079339..95bd26f0b2 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -245,13 +245,13 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '\n \t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git config respects pager.config when setting' '\n+test_expect_success TTY 'git config ignores pager.config when setting' '\n \trm -f paginated.out &&\n \ttest_terminal git -c pager.config config foo.bar bar &&\n-\ttest -e paginated.out\n+\t! test -e paginated.out\n '\n \n-test_expect_failure TTY 'git config --edit ignores pager.config' '\n+test_expect_success TTY 'git config --edit ignores pager.config' '\n \trm -f paginated.out editor.used &&\n \twrite_script editor <<-\\EOF &&\n \t\ttouch editor.used\n@@ -261,10 +261,10 @@ test_expect_failure TTY 'git config --edit ignores pager.config' '\n \ttest -e editor.used\n '\n \n-test_expect_success TTY 'git config --get respects pager.config' '\n+test_expect_success TTY 'git config --get ignores pager.config' '\n \trm -f paginated.out &&\n \ttest_terminal git -c pager.config config --get foo.bar &&\n-\ttest -e paginated.out\n+\t! test -e paginated.out\n '\n \n test_expect_success TTY 'git config --get-urlmatch defaults to not paging' '\ndiff --git a/builtin/config.c b/builtin/config.c\nindex ab5f95476e..a732d9b56c 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -48,6 +48,13 @@ static int show_origin;\n #define ACTION_GET_COLORBOOL (1<<14)\n #define ACTION_GET_URLMATCH (1<<15)\n \n+/*\n+ * The actions \"ACTION_LIST | ACTION_GET_*\" which may produce more than\n+ * one line of output and which should therefore be paged.\n+ */\n+#define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \\\n+\t\t\tACTION_GET_REGEXP | ACTION_GET_URLMATCH)\n+\n #define TYPE_BOOL (1<<0)\n #define TYPE_INT (1<<1)\n #define TYPE_BOOL_OR_INT (1<<2)\n@@ -594,6 +601,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n \n+\tif (actions & PAGING_ACTIONS)\n+\t\tsetup_auto_pager(\"config\", 0);\n+\n \tif (actions == ACTION_LIST) {\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (config_with_options(show_all_config, NULL,\ndiff --git a/git.c b/git.c\nindex c870b9719c..e5c9b8729d 100644\n--- a/git.c\n+++ b/git.c\n@@ -389,7 +389,7 @@ static struct cmd_struct commands[] = {\n \t{ \"column\", cmd_column, RUN_SETUP_GENTLY },\n \t{ \"commit\", cmd_commit, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"commit-tree\", cmd_commit_tree, RUN_SETUP },\n-\t{ \"config\", cmd_config, RUN_SETUP_GENTLY },\n+\t{ \"config\", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },\n \t{ \"count-objects\", cmd_count_objects, RUN_SETUP },\n \t{ \"credential\", cmd_credential, RUN_SETUP_GENTLY },\n \t{ \"describe\", cmd_describe, RUN_SETUP },\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"339824","messageId":"bb68a11f51024980e513c1f001b5759a35829ff1.1519238410.git.martin.agren@gmail.com","threadId":"47807","inReplyTo":"cover.1519238410.git.martin.agren@gmail.com","subject":"[PATCH v2 3/3] config: change default of `pager.config` to \"on\"","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-21T18:51:44Z","receivedAt":"2018-02-21T18:52:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is similar to ff1e72483 (tag: change default of `pager.tag` to\n\"on\", 2017-08-02) and is safe now that we do not consider `pager.config`\nat all when we are not listing or getting configuration. This change\nwill help with listing large configurations, but will not hurt users of\n`git config --edit` as it would have before the previous commit.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/git-config.txt |  1 +\n t/t7006-pager.sh             | 12 ++++++------\n builtin/config.c             |  2 +-\n 3 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 249090ac84..e09ed5d7d5 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -237,6 +237,7 @@ CONFIGURATION\n -------------\n `pager.config` is only respected when listing configuration, i.e., when\n using `--list` or any of the `--get-*` which may return multiple results.\n+The default is to use a pager.\n \n [[FILES]]\n FILES\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 95bd26f0b2..7541ba5edb 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -267,23 +267,23 @@ test_expect_success TTY 'git config --get ignores pager.config' '\n \t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git config --get-urlmatch defaults to not paging' '\n+test_expect_success TTY 'git config --get-urlmatch defaults to paging' '\n \trm -f paginated.out &&\n \ttest_terminal git -c http.\"https://foo.com/\".bar=foo \\\n \t\t\t  config --get-urlmatch http https://foo.com &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n test_expect_success TTY 'git config --get-all respects pager.config' '\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.config config --get-all foo.bar &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.config=false config --get-all foo.bar &&\n+\t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git config --list defaults to not paging' '\n+test_expect_success TTY 'git config --list defaults to paging' '\n \trm -f paginated.out &&\n \ttest_terminal git config --list &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n \ndiff --git a/builtin/config.c b/builtin/config.c\nindex a732d9b56c..01169dd628 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -602,7 +602,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (actions & PAGING_ACTIONS)\n-\t\tsetup_auto_pager(\"config\", 0);\n+\t\tsetup_auto_pager(\"config\", 1);\n \n \tif (actions == ACTION_LIST) {\n \t\tcheck_argc(argc, 0, 0);\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"339848","messageId":"xmqqmv02x96x.fsf@gitster-ct.c.googlers.com","threadId":"47807","inReplyTo":"cover.1519238410.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 0/3] Re: t7006: add tests for how git config paginates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-21T22:35:34Z","receivedAt":"2018-02-21T22:35:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.  The entire thing looked reasonable.\n\nWill replace.\n"}]}