{"thread":{"id":"46350","subject":"[PATCH 0/7] tag: more fine-grained pager-configuration","startedAt":"2017-07-10T21:56:19Z","lastAt":"2017-08-04T16:42:40Z","messageCount":69,"participants":["Martin Ågren","Junio C Hamano","Brandon Williams","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"324109","messageId":"cover.1499723297.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":null,"subject":"[PATCH 0/7] tag: more fine-grained pager-configuration","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-10T21:55:13Z","receivedAt":"2017-07-10T21:56:19Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as \"Vim: Warning: Output is not to a terminal\" and a garbled terminal. A user who makes use of `git tag -a` and `git tag -l` will probably choose not to configure `pager.tag` or to set it to \"no\", so that `git tag -a` will actually work, at the cost of not getting the pager with `git tag -l`.\n\nIn the discussion at [1], it was brought up that 1) the individual builtins should be in charge of setting up the paging (as opposed to git.c which has no knowledge about what the command is about to do) and that 2) there could then be a configuration `pager.tag.list` to address the specific problem of `git tag`.\n\nThis is an attempt to implement something like that. I decided to let `pager.tag.list` fall back to `pager.tag` before falling back to \"on\". The default for `pager.tag` is still \"off\". I can see how that might seem confusing. However, my argument is that it would be awkward for `git tag -l` to ignore `pager.tag` -- we are after all running a subcommand of `git tag`. Also, this avoids a regression for someone who has set `pager.tag` and uses `git tag -l`.\n\nI am not moving all builtins to handling the pager on their own, but instead introduce a flag IGNORE_PAGER_CONFIG and use it only with the tag builtin. That means there's another flag to reason about, but it avoids moving all builtins to handling the paging themselves just to make one of them do something more \"clever\".\n\nThe discussion mentioned above discusses various approaches. It also notes how the current pager.foo-configuration conflates _whether_ and _how_ to run a pager. Arguably, this series paints ourselves even further into that corner. The idea of `pager.foo.command` and `pager.foo.enabled` has been mentioned and this series might make such an approach slightly less clean, conceptually. We could introduce `paging.foo` as a \"yes\"/\"no\"/\"maybe\" to go alongside `pager.foo` which is then \"less\"/\"cat\"/.... That's definitely outside this series, but should not be prohibited by it...\n\nA review would be much appreciated. Comments on the way I structured the series would be just as welcome as ones on the final result.\n\nMartin\n\n[1] https://public-inbox.org/git/nrmbrl$hsk$1@blaine.gmane.org/T/\n\nMartin Ågren (7):\n api-builtin.txt: document SUPPORT_SUPER_PREFIX\n git.c: let builtins opt for handling `pager.foo` themselves\n git.c: provide setup_auto_pager()\n t7006: add tests for how git tag paginates\n tag: handle `pager.tag`-configuration within the builtin\n tag: make git tag -l consider new config `pager.tag.list`\n tag: make git tag -l use pager by default\n\n Documentation/git-tag.txt | 4 +++\n Documentation/technical/api-builtin.txt | 10 ++++++\n builtin.h | 14 +++++++++\n builtin/tag.c | 4 +++\n git.c | 44 +++++++++++++++++++++++++--\n t/t7006-pager.sh | 54 +++++++++++++++++++++++++++++++++\n 6 files changed, 127 insertions(+), 3 deletions(-)\n\n-- \n2.13.2.653.gfb5159d\n\n"},{"id":"324110","messageId":"1b08c5f568028645325367d6d6b97b9da17894ff.1499723297.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1499723297.git.martin.agren@gmail.com","subject":"[PATCH 1/7] api-builtin.txt: document SUPPORT_SUPER_PREFIX","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-10T21:55:14Z","receivedAt":"2017-07-10T21:56:30Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit 74866d75 (\"git: make super-prefix option\", 2016-10-07) introduced\nSUPPORT_SUPER_PREFIX as a builtin flag without documenting it in\napi-builtin.txt. The next patch will add another flag, so document\nSUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with\nthe available flags.\n\nSigned-off-by: Martin Ågren \n---\n Documentation/technical/api-builtin.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt\nindex 22a39b929..60f442822 100644\n--- a/Documentation/technical/api-builtin.txt\n+++ b/Documentation/technical/api-builtin.txt\n@@ -42,6 +42,10 @@ where options is the bitwise-or of:\n \ton bare repositories.\n \tThis only makes sense when `RUN_SETUP` is also set.\n \n+`SUPPORT_SUPER_PREFIX`::\n+\n+\tThe builtin supports --super-prefix.\n+\n . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n \n Additionally, if `foo` is a new command, there are 3 more things to do:\n-- \n2.13.2.653.gfb5159d\n\n"},{"id":"324111","messageId":"62a5a588d880d1a41d6cdd54cd92ee577a0451fa.1499723297.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1499723297.git.martin.agren@gmail.com","subject":"[PATCH 2/7] git.c: let builtins opt for handling `pager.foo` themselves","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-10T21:55:15Z","receivedAt":"2017-07-10T21:56:32Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Before launching a builtin git foo and unless mechanisms with precedence\nare in use, we check for and handle the `pager.foo` config. This is done\nwithout considering exactly how git foo is being used, and indeed, git.c\ncannot (and should not) know what the arguments to git foo are supposed\nto achieve.\n\nIn practice this means that, e.g., `git -c pager.tag tag -a new-tag`\nresults in errors such as \"Vim: Warning: Output is not to a terminal\"\nand a garbled terminal. A user who makes use of `git tag -a` and `git\ntag -l` will probably choose not to configure `pager.tag` or to set it\nto \"no\", so that `git tag -a` will actually work, at the cost of not\ngetting the pager with `git tag -l`.\n\nTo allow individual builtins to make more informed decisions about when\nto respect `pager.foo`, introduce a flag IGNORE_PAGER_CONFIG. If the flag\nis set, do not check `pager.foo`. This applies to two code-paths -- one\nin run_builtin() and one in execv_dashed_external().\n\nDocument the flag in api-builtin.txt.\n\nDon't add any users of IGNORE_PAGER_CONFIG just yet, one will follow in a\nlater patch.\n\nSuggested-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n Documentation/technical/api-builtin.txt | 6 ++++++\n git.c | 14 ++++++++++++--\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt\nindex 60f442822..61fd8eeb2 100644\n--- a/Documentation/technical/api-builtin.txt\n+++ b/Documentation/technical/api-builtin.txt\n@@ -46,6 +46,12 @@ where options is the bitwise-or of:\n \n \tThe builtin supports --super-prefix.\n \n+`IGNORE_PAGER_CONFIG`::\n+\n+\tIgnore the `pager.`-configuration in git.c, instead\n+\tgiving the builtin the chance to handle it and possibly\n+\tmore fine-grained configurations (`pager..`).\n+\n . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n \n Additionally, if `foo` is a new command, there are 3 more things to do:\ndiff --git a/git.c b/git.c\nindex 489aab4d8..ae92f8ed5 100644\n--- a/git.c\n+++ b/git.c\n@@ -283,6 +283,12 @@ static int handle_alias(int *argcp, const char ***argv)\n */\n #define NEED_WORK_TREE\t\t(1<<3)\n #define SUPPORT_SUPER_PREFIX\t(1<<4)\n+/*\n+ * Ignore the `pager.`-configuration, instead giving the builtin\n+ * the chance to handle it and possibly more fine-grained\n+ * configurations (`pager..`).\n+ */\n+#define IGNORE_PAGER_CONFIG\t(1<<5)\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -306,7 +312,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\t\tprefix = setup_git_directory_gently(&nongit_ok);\n \t\t}\n \n-\t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))\n+\t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n+\t\t !(p->option & IGNORE_PAGER_CONFIG))\n \t\t\tuse_pager = check_pager_config(p->cmd);\n \t\tif (use_pager == -1 && p->option & USE_PAGER)\n \t\t\tuse_pager = 1;\n@@ -543,11 +550,14 @@ static void execv_dashed_external(const char **argv)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint status;\n+\tstruct cmd_struct *builtin;\n \n \tif (get_super_prefix())\n \t\tdie(\"%s doesn't support --super-prefix\", argv[0]);\n \n-\tif (use_pager == -1)\n+\tbuiltin = get_builtin(argv[0]);\n+\tif (use_pager == -1 &&\n+\t !(builtin && builtin->option & IGNORE_PAGER_CONFIG))\n \t\tuse_pager = check_pager_config(argv[0]);\n \tcommit_pager_choice();\n \n-- \n2.13.2.653.gfb5159d\n\n"},{"id":"324112","messageId":"1d639a4c7be5f1c173502f79e85aadd017ef5f7c.1499723297.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1499723297.git.martin.agren@gmail.com","subject":"[PATCH 3/7] git.c: provide setup_auto_pager()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-10T21:55:16Z","receivedAt":"2017-07-10T21:56:37Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The previous patch introduced a way for builtins to declare that they\nwill take responsibility for handling the `pager.foo`-config item. (See\nthe commit message of that patch for why that could be useful.)\n\nProvide setup_auto_pager(), which builtins can call in order to handle\n`pager.`, including possibly starting the pager. Make this function\ndon't do anything if a pager has already been started.\n\nWhen the `cmd` given to setup_auto_pager() contains one or more '.', use\na fallback strategy which checks `pager.foo.bar.baz`, then\n`pager.foo.bar`, then `pager.foo`, then resorts to the provided default\nvalue. This ensures a consistent fallback strategy for builtins which\ntake this type of more fine-grained pager configuration.\n\nAn alternative fallback strategy would have been to check for\n`pager.foo.bar.baz`, then immediately fall back to `def`. However, that\nwould have meant that git foo would sometimes completely ignore\n`pager.foo`, which seems conceptually wrong. It would also have meant\nthat builtins that are moved to more fine-grained pager configurations\nwould have regressed for certain usecases.\n\nWhenever this function is called from a builtin, git.c will already have\ncalled commit_pager_choice(). Since commit_pager_choice() treats the\nspecial value -1 as \"punt\" or \"not yet decided\", it is not a problem\nthat we might end up calling commit_pager_choice() many times. Make the\nnew function use -1 in the same way and document it as \"punt\".\n\nDon't add any users of setup_auto_pager just yet, one will follow in\nlater patches.\n\nsetup_auto_pager() is more capable than it needs to be for this patch\nseries. It would have been sufficient to handle zero or one '.'. We\nwould probably have wanted some verification+BUG()-patterns or to\ndefine whether we split at the first or last '.', so it seems just as\neasy, and certainly more flexible, to handle the more general situation.\n\nSigned-off-by: Martin Ågren \n---\n builtin.h | 14 ++++++++++++++\n git.c | 28 ++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+)\n\ndiff --git a/builtin.h b/builtin.h\nindex 498ac80d0..a6ed6c4ac 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -25,6 +25,20 @@ struct fmt_merge_msg_opts {\n extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \t\t\t struct fmt_merge_msg_opts *);\n \n+/**\n+ * If a builtin has IGNORE_PAGER_CONFIG set, the builtin should call this early\n+ * when it wishes to respect the `pager.foo`-config. In the simplest case, the\n+ * `cmd` is the name of the builtin, e.g., \"foo\". If a paging-choice has already\n+ * been setup, this does nothing. The default in `def` should be 0 for \"pager\n+ * off\", 1 for \"pager on\" or -1 for \"punt\".\n+ *\n+ * With one or more '.', substrings are tried out from longer to shorter. If no\n+ * config is found, uses `def`. For example, with `cmd` as \"foo.bar.baz\", this\n+ * function tries `pager.foo.bar.baz`, `pager.foo.bar` and `pager.foo` in that\n+ * order before resorting to `def`.\n+ */\n+extern void setup_auto_pager(const char *cmd, int def);\n+\n extern int is_builtin(const char *s);\n \n extern int cmd_add(int argc, const char **argv, const char *prefix);\ndiff --git a/git.c b/git.c\nindex ae92f8ed5..696eaf87a 100644\n--- a/git.c\n+++ b/git.c\n@@ -33,6 +33,34 @@ static void commit_pager_choice(void) {\n \t}\n }\n \n+void setup_auto_pager(const char *cmd, int def)\n+{\n+\tif (use_pager != -1)\n+\t\treturn;\n+\n+\tuse_pager = check_pager_config(cmd);\n+\n+\tif (use_pager == -1) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tsize_t len;\n+\n+\t\tstrbuf_addstr(&buf, cmd);\n+\t\tlen = buf.len;\n+\t\twhile (use_pager == -1 && len--) {\n+\t\t\tif (buf.buf[len] == '.') {\n+\t\t\t\tstrbuf_setlen(&buf, len);\n+\t\t\t\tuse_pager = check_pager_config(buf.buf);\n+\t\t\t}\n+\t\t}\n+\t\tstrbuf_release(&buf);\n+\t}\n+\n+\tif (use_pager == -1)\n+\t\tuse_pager = def;\n+\n+\tcommit_pager_choice();\n+}\n+\n static int handle_options(const char ***argv, int *argc, int *envchanged)\n {\n \tconst char **orig_argv = *argv;\n-- \n2.13.2.653.gfb5159d\n\n"},{"id":"324113","messageId":"2615e64041d7918a89143cd724aa8cec5faface3.1499723297.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1499723297.git.martin.agren@gmail.com","subject":"[PATCH 4/7] t7006: add tests for how git tag paginates","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-10T21:55:17Z","receivedAt":"2017-07-10T21:56:38Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as\n\"Vim: Warning: Output is not to a terminal\" and a garbled terminal. A\nuser who makes use of `git tag -a` and `git tag -l` will probably choose\nnot to configure `pager.tag` or to set it to \"no\", so that `git tag -a`\nwill actually work, at the cost of not getting the pager with `git tag\n-l`.\n\nSince we're about to add some finer granularity to the configuration,\nadd tests around how git tag respects `pager.tag` and how that\nconfiguration is ignored if --no-pager or --paginate are used.\n\nConstruct tests with two different subcommands: using -l and using -a,\nwhere -a is being used essentially as a representative for \"not -l\".\n\nMake one of the tests demonstrate the behavior mentioned above, where\n`git tag -a` respects `pager.tag`. Actually, the tests use `git tag -a`\nwith -m, in which case no editor is launched, but that is irrelevant,\nsince we just want to see whether the pager is used or not. (If `git tag\n-am` ever learns to avoid the pager, these tests will need to be\nupdated and two of them will fail.)\n\nSigned-off-by: Martin Ågren \n---\n t/t7006-pager.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 20b4d83c2..43cce3694 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -134,6 +134,46 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '\n \t}\n '\n \n+test_expect_success TTY 'git tag -l defaults to not paging' '\n+\trm -f paginated.out &&\n+\ttest_terminal git tag -l &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -l respects pager.tag' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag tag -l &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -l respects --no-pager' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag --no-pager tag -l &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -a defaults to not paging' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git tag -am message newtag &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -a respects pager.tag' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag tag -am message newtag &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -a respects --paginate' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag=false --paginate \\\n+\t\ttag -am message newtag &&\n+\ttest -e paginated.out\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.13.2.653.gfb5159d\n\n"},{"id":"324114","messageId":"27ca13ea5b520d492f96ea319ae47d6f4b431e94.1499723297.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1499723297.git.martin.agren@gmail.com","subject":"[PATCH 6/7] tag: make git tag -l consider new config `pager.tag.list`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-10T21:55:19Z","receivedAt":"2017-07-10T21:56:40Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as\n\"Vim: Warning: Output is not to a terminal\" and a garbled terminal. A\nuser who makes use of `git tag -a` and `git tag -l` will probably choose\nnot to configure `pager.tag` or to set it to \"no\", so that `git tag -a`\nwill actually work, at the cost of not getting the pager with `git tag\n-l`.\n\nIntroduce `pager.tag.list`. Teach git tag to prefer it over `pager.tag`\nwhen running with -l. Update the documentation and add tests. Update an\nexisting test to use `pager.tag.list` instead of `pager.tag` together\nwith `git tag -l` since the former is arguably more relevant.\n\nDo not introduce an \"else\" where we call setup_auto_pager(), although we\ncould have. Omitting it might keep someone who later implements even\nmore fine-grained configurations from building a correspondingly\ncomplicated decision tree which carefully ensures that\nsetup_auto_pager() is called precisely once. A greedy approach such as\nthe one taken here works just fine.\n\nNoticed-by: Anatoly Borodin \nSuggested-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n Documentation/git-tag.txt | 4 ++++\n builtin/tag.c | 2 ++\n t/t7006-pager.sh | 16 +++++++++++++++-\n 3 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 1eb15afa1..6ad5811a2 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -101,6 +101,10 @@ patterns may be given; if any of them matches, the tag is shown.\n This option is implicitly supplied if any other list-like option such\n as `--contains` is provided. See the documentation for each of those\n options for details.\n++\n+When determining whether to use a pager, `pager.tag.list` is considered\n+before `pager.tag`.\n+See linkgit:git-config[1].\n \n --sort=::\n \tSort based on the key given. Prefix `-` to sort in\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex e0f129872..e96ef7d70 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -446,6 +446,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);\n \n+\tif (cmdmode == 'l')\n+\t\tsetup_auto_pager(\"tag.list\", 0);\n \tsetup_auto_pager(\"tag\", 0);\n \n \tif (keyid) {\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 43cce3694..ed34c6734 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -146,9 +146,15 @@ test_expect_success TTY 'git tag -l respects pager.tag' '\n \ttest -e paginated.out\n '\n \n+test_expect_success TTY 'git tag -l respects pager.tag.list' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag=false -c pager.tag.list tag -l &&\n+\ttest -e paginated.out\n+'\n+\n test_expect_success TTY 'git tag -l respects --no-pager' '\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag --no-pager tag -l &&\n+\ttest_terminal git -c pager.tag.list --no-pager tag -l &&\n \t! test -e paginated.out\n '\n \n@@ -166,6 +172,14 @@ test_expect_success TTY 'git tag -a respects pager.tag' '\n \ttest -e paginated.out\n '\n \n+test_expect_success TTY 'git tag -a ignores pager.tag.list' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag -c pager.tag.list=false \\\n+\t\ttag -am message newtag &&\n+\ttest -e paginated.out\n+'\n+\n test_expect_success TTY 'git tag -a respects --paginate' '\n \ttest_when_finished \"git tag -d newtag\" &&\n \trm -f paginated.out &&\n-- \n2.13.2.653.gfb5159d\n\n"},{"id":"324115","messageId":"c5bb98f94757b94c09d0be27dce0df9d07d9eb6f.1499723297.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1499723297.git.martin.agren@gmail.com","subject":"[PATCH 5/7] tag: handle `pager.tag`-configuration within the builtin","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-10T21:55:18Z","receivedAt":"2017-07-10T21:56:42Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Use the mechanisms introduced in two earlier patches to ignore\n`pager.tag` in git.c and let the `git tag` builtin handle it on its own.\n\nThis is in preparation for the next patch, where we will want to handle\nslightly different configuration variables depending on which options\nare used with `git tag`. For this reason, place the call to\nsetup_auto_pager() after the options have been parsed.\n\nNo functional change is intended. That said, there is a window between\nwhere the pager is started before and after this patch, and if an error\noccurs within this window, as of this patch the error message might not\nbe paged where it would have been paged before. Since\noperation-parsing has to happen inside this window, a difference can be\nseen with, e.g., `git -c pager.tag=\"echo pager is used\" tag\n--unknown-option`. This change in paging-behavior should be acceptable\nsince it only affects erroneous usages.\n\nSigned-off-by: Martin Ågren \n---\n builtin/tag.c | 2 ++\n git.c | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 01154ea8d..e0f129872 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -446,6 +446,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);\n \n+\tsetup_auto_pager(\"tag\", 0);\n+\n \tif (keyid) {\n \t\topt.sign = 1;\n \t\tset_signing_key(keyid);\ndiff --git a/git.c b/git.c\nindex 696eaf87a..4d05452a3 100644\n--- a/git.c\n+++ b/git.c\n@@ -489,7 +489,7 @@ static struct cmd_struct commands[] = {\n \t{ \"stripspace\", cmd_stripspace },\n \t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n-\t{ \"tag\", cmd_tag, RUN_SETUP },\n+\t{ \"tag\", cmd_tag, RUN_SETUP | IGNORE_PAGER_CONFIG },\n \t{ \"unpack-file\", cmd_unpack_file, RUN_SETUP },\n \t{ \"unpack-objects\", cmd_unpack_objects, RUN_SETUP },\n \t{ \"update-index\", cmd_update_index, RUN_SETUP },\n-- \n2.13.2.653.gfb5159d\n\n"},{"id":"324116","messageId":"e893a0b87b14143af6fc162d1acb21012523af69.1499723297.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1499723297.git.martin.agren@gmail.com","subject":"[PATCH 7/7] tag: make git tag -l use pager by default","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-10T21:55:20Z","receivedAt":"2017-07-10T21:56:44Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The previous patch introduced `pager.tag.list`. Its default was to use\nthe value of `pager.tag` or, if that was also not set, to fall back to\n\"off\".\n\nChange that fallback value to \"on\". Let the default value for\n`pager.tag` remain at \"off\". Update documentation and tests.\n\nSigned-off-by: Martin Ågren \n---\n Documentation/git-tag.txt | 2 +-\n builtin/tag.c | 2 +-\n t/t7006-pager.sh | 12 ++++++------\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 6ad5811a2..fbdfb3f59 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -103,7 +103,7 @@ as `--contains` is provided. See the documentation for each of those\n options for details.\n +\n When determining whether to use a pager, `pager.tag.list` is considered\n-before `pager.tag`.\n+before `pager.tag`. If neither is set, the default is to use a pager.\n See linkgit:git-config[1].\n \n --sort=::\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex e96ef7d70..ec69fca61 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -447,7 +447,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);\n \n \tif (cmdmode == 'l')\n-\t\tsetup_auto_pager(\"tag.list\", 0);\n+\t\tsetup_auto_pager(\"tag.list\", 1);\n \tsetup_auto_pager(\"tag\", 0);\n \n \tif (keyid) {\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex ed34c6734..94df89a5f 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -134,22 +134,22 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '\n \t}\n '\n \n-test_expect_success TTY 'git tag -l defaults to not paging' '\n+test_expect_success TTY 'git tag -l defaults to paging' '\n \trm -f paginated.out &&\n \ttest_terminal git tag -l &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n test_expect_success TTY 'git tag -l respects pager.tag' '\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag tag -l &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.tag=false tag -l &&\n+\t! test -e paginated.out\n '\n \n test_expect_success TTY 'git tag -l respects pager.tag.list' '\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag=false -c pager.tag.list tag -l &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.tag -c pager.tag.list=false tag -l &&\n+\t! test -e paginated.out\n '\n \n test_expect_success TTY 'git tag -l respects --no-pager' '\n-- \n2.13.2.653.gfb5159d\n\n"},{"id":"324119","messageId":"xmqqpod752yh.fsf@gitster.mtv.corp.google.com","threadId":"46350","inReplyTo":"cover.1499723297.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/7] tag: more fine-grained pager-configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-10T22:42:14Z","receivedAt":"2017-07-10T22:42:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Using, e.g., `git -c pager.tag tag -a new-tag` results in errors\n> such as \"Vim: Warning: Output is not to a terminal\" and a garbled\n> terminal. A user who makes use of `git tag -a` and `git tag -l`\n> will probably choose not to configure `pager.tag` or to set it to\n> \"no\", so that `git tag -a` will actually work, at the cost of not\n> getting the pager with `git tag -l`.\n>\n> In the discussion at [1], it was brought up that 1) the individual\n> builtins should be in charge of setting up the paging (as opposed\n> to git.c which has no knowledge about what the command is about to\n> do) and that 2) there could then be a configuration\n> `pager.tag.list` to address the specific problem of `git tag`.\n>\n> This is an attempt to implement something like that. I decided to\n> let `pager.tag.list` fall back to `pager.tag` before falling back\n> to \"on\". The default for `pager.tag` is still \"off\". I can see how\n> that might seem confusing. However, my argument is that it would\n> be awkward for `git tag -l` to ignore `pager.tag` -- we are after\n> all running a subcommand of `git tag`. Also, this avoids a\n> regression for someone who has set `pager.tag` and uses `git tag\n> -l`.\n>\n> I am not moving all builtins to handling the pager on their own,\n> but instead introduce a flag IGNORE_PAGER_CONFIG and use it only\n> with the tag builtin. That means there's another flag to reason\n> about, but it avoids moving all builtins to handling the paging\n> themselves just to make one of them do something more \"clever\".\n\nAll of the above smells like taking us in a sensible direction. I\nagree with these design decisions described in the above, i.e.\n'pager.tag.list falling back to pager.tag', making this an opt-in to\nmake code transition easier.\n\nEven though it is purely internal thing, IGNORE_PAGER_CONFIG\nprobably is a bit confusion-inducing name. After all, the\nsubcommand specific configuration is not being ignored---we are\nmerely delaying our reaction to it---instead of acting on it inside\ngit.c without giving the subcommand a chance to make a decision, we\nare still letting (and we do expect) the subcommand to react to it.\n\nBut that is a fairly minor thing we can fix.\n\n> A review would be much appreciated. Comments on the way I\n> structured the series would be just as welcome as ones on the\n> final result.\n\nI see you CC'ed Peff who's passionate in this area, so these patches\nare in good hands already ;-) I briefly skimmed your patches myself,\nand did not spot anything glaringly wrong.\n\nThanks.\n"},{"id":"324122","messageId":"20170710225022.GB161700@google.com","threadId":"46350","inReplyTo":"1b08c5f568028645325367d6d6b97b9da17894ff.1499723297.git.martin.agren@gmail.com","subject":"Re: [PATCH 1/7] api-builtin.txt: document SUPPORT_SUPER_PREFIX","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-10T22:50:22Z","receivedAt":"2017-07-10T22:50:30Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/10, Martin Ågren wrote:\n> Commit 74866d75 (\"git: make super-prefix option\", 2016-10-07) introduced\n> SUPPORT_SUPER_PREFIX as a builtin flag without documenting it in\n> api-builtin.txt. The next patch will add another flag, so document\n> SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with\n> the available flags.\n> \n> Signed-off-by: Martin Ågren \n> ---\n> Documentation/technical/api-builtin.txt | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt\n> index 22a39b929..60f442822 100644\n> --- a/Documentation/technical/api-builtin.txt\n> +++ b/Documentation/technical/api-builtin.txt\n> @@ -42,6 +42,10 @@ where options is the bitwise-or of:\n> \ton bare repositories.\n> \tThis only makes sense when `RUN_SETUP` is also set.\n> \n> +`SUPPORT_SUPER_PREFIX`::\n> +\n> +\tThe builtin supports --super-prefix.\n> +\n> . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n> \n> Additionally, if `foo` is a new command, there are 3 more things to do:\n\nI added SUPER_PREFIX as an implementation detail when trying to recurse\nsubmodules using multiple processes. Now that we have a 'struct\nrepository' my plan is to remove SUPER_PREFIX in its entirety. Since\nthis won't happen overnight it may still take a bit till its removed so\nmaybe it makes sense to better document it until that happens?\n\nEither way I think that this sort of Documention better lives in the\ncode as it is easier to keep up to date. Last time I tried to look at\nstuff in Documentation/technical the files were either place holders or\ncompletely out of date with what the code did.\n\n-- \nBrandon Williams\n"},{"id":"324153","messageId":"CAN0heSq3mxRXdv0WmqqmY1tqPAJH06Eu9xyPWBxY=O1kii1ufw@mail.gmail.com","threadId":"46350","inReplyTo":"20170710225022.GB161700@google.com","subject":"Re: [PATCH 1/7] api-builtin.txt: document SUPPORT_SUPER_PREFIX","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-11T06:32:47Z","receivedAt":"2017-07-11T06:32:52Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 11 July 2017 at 00:50, Brandon Williams wrote:\n> On 07/10, Martin Ågren wrote:\n>> Commit 74866d75 (\"git: make super-prefix option\", 2016-10-07) introduced\n>> SUPPORT_SUPER_PREFIX as a builtin flag without documenting it in\n>> api-builtin.txt. The next patch will add another flag, so document\n>> SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with\n>> the available flags.\n>>\n>> Signed-off-by: Martin Ågren \n>> ---\n>> Documentation/technical/api-builtin.txt | 4 ++++\n>> 1 file changed, 4 insertions(+)\n>>\n>> diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt\n>> index 22a39b929..60f442822 100644\n>> --- a/Documentation/technical/api-builtin.txt\n>> +++ b/Documentation/technical/api-builtin.txt\n>> @@ -42,6 +42,10 @@ where options is the bitwise-or of:\n>> on bare repositories.\n>> This only makes sense when `RUN_SETUP` is also set.\n>>\n>> +`SUPPORT_SUPER_PREFIX`::\n>> +\n>> + The builtin supports --super-prefix.\n>> +\n>> . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n>>\n>> Additionally, if `foo` is a new command, there are 3 more things to do:\n>\n> I added SUPER_PREFIX as an implementation detail when trying to recurse\n> submodules using multiple processes. Now that we have a 'struct\n> repository' my plan is to remove SUPER_PREFIX in its entirety. Since\n> this won't happen overnight it may still take a bit till its removed so\n> maybe it makes sense to better document it until that happens?\n\nI could add something about how this is \"temporary\" although I have no\nidea about the timeframe. ;-)\n\n> Either way I think that this sort of Documention better lives in the\n> code as it is easier to keep up to date.\n\nAgreed and I believe that's the long-term goal. I could replace this\npreparatory patch with another one where I move api-builtin.txt into\nbuiltin.h or git.c (and document SUPPORT_SUPER_PREFIX if that's\nwanted). That's probably better, since right now, patch 2/7 adds\nbasically the same documentation for the new flag in two places.\n\nMartin\n"},{"id":"324172","messageId":"20170711101942.h2uwxtgzvgguzivu@sigill.intra.peff.net","threadId":"46350","inReplyTo":"cover.1499723297.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/7] tag: more fine-grained pager-configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-11T10:19:43Z","receivedAt":"2017-07-11T10:19:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 11:55:13PM +0200, Martin Ågren wrote:\n\n> Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such\n> as \"Vim: Warning: Output is not to a terminal\" and a garbled terminal.\n> A user who makes use of `git tag -a` and `git tag -l` will probably\n> choose not to configure `pager.tag` or to set it to \"no\", so that `git\n> tag -a` will actually work, at the cost of not getting the pager with\n> `git tag -l`.\n\nRight, I think we are all agreed that \"pager.tag\" as it is now is\nessentially worthless.\n\nIf I understand your series correctly, though, it adds pager.tag.list\nbut leaves \"pager.tag\" behaving more or less the same. It's good that we\nnow have a way for a user to do the thing they actually want, but it\nfeels like we're leaving pager.tag behind as a booby-trap.\n\nShould we disable it entirely (or only respect it in list mode)?\n\nAt which point, I wonder if we actually need pager.tag.list at all.\nSlicing up the namespace further would be valuable if there were a\ncommand which had two pager-worthy modes, and somebody might want to\nenable the pager for one but not the other. But it seems like most\ncommands in this boat (e.g., tag, branch, stash) really have two modes:\nlisting things or creating things.\n\nWould it makes sense to just have git-tag respect pager.tag in listing\nmode, and otherwise ignore it completely?\n\nOne nice side effect is that it keeps the multi-level pager.X.Y\nnamespace clear. We've talked about transitioning to allow:\n\n [pager \"foo\"]\n enable = true\n command = some-custom-pager\n\nto configure aspects of the pager separately for git-foo. This has two\nbenefits:\n\n 1. Syntactically, it allows configuration for commands whose names\n aren't valid config keys.\n\n 2. It would allow setting a command with \"enable=false\", so that \"git\n foo\" did not paginate, but \"git -p foo\" paginated with a custom\n command.\n\nThose are admittedly minor features. And assuming we don't go crazy with\nthe multi-level names, we could have \"pager.tag.list\" live alongside\n\"pager.tag.command\". So it's not really an objection, so much as wonder\nout loud whether we can keep this as simple as possible.\n\n> This is an attempt to implement something like that. I decided to let\n> `pager.tag.list` fall back to `pager.tag` before falling back to \"on\".\n> The default for `pager.tag` is still \"off\". I can see how that might\n> seem confusing. However, my argument is that it would be awkward for\n> `git tag -l` to ignore `pager.tag` -- we are after all running a\n> subcommand of `git tag`. Also, this avoids a regression for someone\n> who has set `pager.tag` and uses `git tag -l`.\n\nYeah, I agree that turning \"pager.tag\" into a complete noop is probably\na bad idea. But if we made it a silent noop for the non-list cases, that\nwould be OK (and the hypothetical user who set it to make `git tag -l`\nwork would see a strict improvement; they'd still get their paging but\nnot the weird breakage with non-list modes). And I think that applies\nwhether we have a pager.tag.list in addition or not.\n\n> I am not moving all builtins to handling the pager on their own, but\n> instead introduce a flag IGNORE_PAGER_CONFIG and use it only with the\n> tag builtin. That means there's another flag to reason about, but it\n> avoids moving all builtins to handling the paging themselves just to\n> make one of them do something more \"clever\".\n\nI think this is very sensible. It's the vast minority that would want to\nenable this (git-branch is the other obvious one). At some point we may\nneed a plan to handle non-builtins (like stash), but that's largely\northogonal to what you're doing here.\n\n> The discussion mentioned above discusses various approaches. It also\n> notes how the current pager.foo-configuration conflates _whether_ and\n> _how_ to run a pager. Arguably, this series paints ourselves even\n> further into that corner. The idea of `pager.foo.command` and\n> `pager.foo.enabled` has been mentioned and this series might make such\n> an approach slightly less clean, conceptually. We could introduce\n> `paging.foo` as a \"yes\"/\"no\"/\"maybe\" to go alongside `pager.foo` which\n> is then \"less\"/\"cat\"/.... That's definitely outside this series, but\n> should not be prohibited by it...\n\nI think I covered my thoughts on this part above. :)\n\n> A review would be much appreciated. Comments on the way I structured\n> the series would be just as welcome as ones on the final result.\n\nOverall the code looks good, though I'll respond with a few comments.\n\n-Peff\n"},{"id":"324173","messageId":"20170711102451.27r3yaa2mgjgk4qv@sigill.intra.peff.net","threadId":"46350","inReplyTo":"62a5a588d880d1a41d6cdd54cd92ee577a0451fa.1499723297.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/7] git.c: let builtins opt for handling `pager.foo` themselves","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-11T10:24:51Z","receivedAt":"2017-07-11T10:24:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 11:55:15PM +0200, Martin Ågren wrote:\n\n> To allow individual builtins to make more informed decisions about when\n> to respect `pager.foo`, introduce a flag IGNORE_PAGER_CONFIG. If the flag\n> is set, do not check `pager.foo`. This applies to two code-paths -- one\n> in run_builtin() and one in execv_dashed_external().\n\nCan this ever trigger in execv_dashed_external()? We should only get\nthere if get_builtin() returned NULL in the first place. Otherwise, we'd\nrun and exited via handle_builtin().\n\nSo I think this hunk:\n\n> @@ -543,11 +550,14 @@ static void execv_dashed_external(const char **argv)\n> {\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \tint status;\n> +\tstruct cmd_struct *builtin;\n> \n> \tif (get_super_prefix())\n> \t\tdie(\"%s doesn't support --super-prefix\", argv[0]);\n> \n> -\tif (use_pager == -1)\n> +\tbuiltin = get_builtin(argv[0]);\n> +\tif (use_pager == -1 &&\n> +\t !(builtin && builtin->option & IGNORE_PAGER_CONFIG))\n> \t\tuse_pager = check_pager_config(argv[0]);\n> \tcommit_pager_choice();\n\n...can just go away. And that highlights the issue with externals; we\ndon't have any internal database that says \"these ones handle their own\npager\". We don't even have a list of all the possibilities, since users\ncan drop whatever they like into their $PATH.\n\nSo we'd have to make a (backwards-incompatible) decision that pager.*\ndoesn't work for external commands, and they must manually trigger the\npager themselves. I'm undecided on whether that's reasonable or not\n(certainly it's what git-stash would want, but it may be hurting other\ncommands).\n\nAnyway, I think that's out of scope for your series, which is just\ntrying to make the builtins work better.\n\n-Peff\n"},{"id":"324174","messageId":"20170711103715.a4jfbjop227bfuhh@sigill.intra.peff.net","threadId":"46350","inReplyTo":"1d639a4c7be5f1c173502f79e85aadd017ef5f7c.1499723297.git.martin.agren@gmail.com","subject":"Re: [PATCH 3/7] git.c: provide setup_auto_pager()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-11T10:37:15Z","receivedAt":"2017-07-11T10:37:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 11:55:16PM +0200, Martin Ågren wrote:\n\n> +void setup_auto_pager(const char *cmd, int def)\n> +{\n> +\tif (use_pager != -1)\n> +\t\treturn;\n\nI think you probably also want to return early here if pager_in_use()\nis true. If a script runs \"git tag -l\", you wouldn't want to start a new\npager when the outer script has already been paged (either via config,\nor via \"git -p\").\n\n> +\tuse_pager = check_pager_config(cmd);\n> +\n> +\tif (use_pager == -1) {\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\tsize_t len;\n> +\n> +\t\tstrbuf_addstr(&buf, cmd);\n> +\t\tlen = buf.len;\n> +\t\twhile (use_pager == -1 && len--) {\n> +\t\t\tif (buf.buf[len] == '.') {\n> +\t\t\t\tstrbuf_setlen(&buf, len);\n> +\t\t\t\tuse_pager = check_pager_config(buf.buf);\n> +\t\t\t}\n> +\t\t}\n> +\t\tstrbuf_release(&buf);\n> +\t}\n\nThis looks good. I wondered if we could fold this all into a single\nloop, rather than having the extra check_pager_config() beforehand\n(which confused me for a half-second at first). But I tried and I don't\nthink the result ended up any more readable.\n\n-Peff\n"},{"id":"324175","messageId":"20170711103840.ghdoarxlfibfmg2t@sigill.intra.peff.net","threadId":"46350","inReplyTo":"c5bb98f94757b94c09d0be27dce0df9d07d9eb6f.1499723297.git.martin.agren@gmail.com","subject":"Re: [PATCH 5/7] tag: handle `pager.tag`-configuration within the builtin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-11T10:38:41Z","receivedAt":"2017-07-11T10:38:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 11:55:18PM +0200, Martin Ågren wrote:\n\n> Use the mechanisms introduced in two earlier patches to ignore\n> `pager.tag` in git.c and let the `git tag` builtin handle it on its own.\n> \n> This is in preparation for the next patch, where we will want to handle\n> slightly different configuration variables depending on which options\n> are used with `git tag`. For this reason, place the call to\n> setup_auto_pager() after the options have been parsed.\n> \n> No functional change is intended. That said, there is a window between\n> where the pager is started before and after this patch, and if an error\n> occurs within this window, as of this patch the error message might not\n> be paged where it would have been paged before. Since\n> operation-parsing has to happen inside this window, a difference can be\n> seen with, e.g., `git -c pager.tag=\"echo pager is used\" tag\n> --unknown-option`. This change in paging-behavior should be acceptable\n> since it only affects erroneous usages.\n\nThanks for carefully thinking through the details. I agree that it's an\nacceptable change of behavior.\n\n-Peff\n"},{"id":"324176","messageId":"20170711104108.vfkdo5vfoigcajdo@sigill.intra.peff.net","threadId":"46350","inReplyTo":"27ca13ea5b520d492f96ea319ae47d6f4b431e94.1499723297.git.martin.agren@gmail.com","subject":"Re: [PATCH 6/7] tag: make git tag -l consider new config `pager.tag.list`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-11T10:41:08Z","receivedAt":"2017-07-11T10:41:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 11:55:19PM +0200, Martin Ågren wrote:\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index e0f129872..e96ef7d70 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -446,6 +446,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \n> \targc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);\n> \n> +\tif (cmdmode == 'l')\n> +\t\tsetup_auto_pager(\"tag.list\", 0);\n> \tsetup_auto_pager(\"tag\", 0);\n\nIdeally this would kick in whenever we are in list mode, even if the\nuser didn't say \"-l\". So:\n\n $ git tag\n\nshould probably respect the list config. Likewise, certain options like\n\"--contains\" trigger list mode. I think the pager setup could just be\nbumped a few lines down, past the \"if (!cmdmode)\" block that sets up\nthose defaults.\n\n-Peff\n"},{"id":"324177","messageId":"20170711104404.5shbsptdbzcujwj7@sigill.intra.peff.net","threadId":"46350","inReplyTo":"e893a0b87b14143af6fc162d1acb21012523af69.1499723297.git.martin.agren@gmail.com","subject":"Re: [PATCH 7/7] tag: make git tag -l use pager by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-11T10:44:04Z","receivedAt":"2017-07-11T10:44:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 11:55:20PM +0200, Martin Ågren wrote:\n\n> The previous patch introduced `pager.tag.list`. Its default was to use\n> the value of `pager.tag` or, if that was also not set, to fall back to\n> \"off\".\n> \n> Change that fallback value to \"on\". Let the default value for\n> `pager.tag` remain at \"off\". Update documentation and tests.\n\nThanks for splitting this out. The default setting is definitely\northogonal to allowing the config.\n\nI've been running with this default for several years now (using the\npatch I showed in the earlier thread you linked). It _does_ occasionally\nannoy me, but I think overall it's an improvement. So it seems like a\ngood thing to try, and we can see how people respond as they try it out.\n\n-Peff\n"},{"id":"324178","messageId":"20170711104755.4z5exp43lvwbb5hq@sigill.intra.peff.net","threadId":"46350","inReplyTo":"xmqqpod752yh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/7] tag: more fine-grained pager-configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-11T10:47:56Z","receivedAt":"2017-07-11T10:48:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 03:42:14PM -0700, Junio C Hamano wrote:\n\n> > A review would be much appreciated. Comments on the way I\n> > structured the series would be just as welcome as ones on the\n> > final result.\n> \n> I see you CC'ed Peff who's passionate in this area, so these patches\n> are in good hands already ;-) I briefly skimmed your patches myself,\n> and did not spot anything glaringly wrong.\n\nHeh, I don't think of \"paging tag output\" as one of my passions, but you\nmay be right. :)\n\nI left a few comments on the code and direction, but I agree that\noverall it looks pretty good. And I'm very impressed with the attention\nto detail for a first-time contributor.\n\n-Peff\n"},{"id":"324182","messageId":"CAN0heSqLX1WNk3KC9U3f=qem0SYV-g=-HU+XHepi_CbuHatCOg@mail.gmail.com","threadId":"46350","inReplyTo":"20170711102451.27r3yaa2mgjgk4qv@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] git.c: let builtins opt for handling `pager.foo` themselves","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-11T13:46:08Z","receivedAt":"2017-07-11T13:46:13Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 11 July 2017 at 12:24, Jeff King wrote:\n> On Mon, Jul 10, 2017 at 11:55:15PM +0200, Martin Ågren wrote:\n>\n>> To allow individual builtins to make more informed decisions about when\n>> to respect `pager.foo`, introduce a flag IGNORE_PAGER_CONFIG. If the flag\n>> is set, do not check `pager.foo`. This applies to two code-paths -- one\n>> in run_builtin() and one in execv_dashed_external().\n>\n> Can this ever trigger in execv_dashed_external()? We should only get\n> there if get_builtin() returned NULL in the first place. Otherwise, we'd\n> run and exited via handle_builtin().\n\nI can trigger it with this:\n\n$ git -c pager.tag=\"echo paging\" -c pager.tag.list=no -c alias.t=tag t -l\n\nwhere the alias is what triggers it and the two pager-configurations\ndemonstrate the effect.\n\n> So I think this hunk:\n>\n>> @@ -543,11 +550,14 @@ static void execv_dashed_external(const char **argv)\n>> {\n>> struct child_process cmd = CHILD_PROCESS_INIT;\n>> int status;\n>> + struct cmd_struct *builtin;\n>>\n>> if (get_super_prefix())\n>> die(\"%s doesn't support --super-prefix\", argv[0]);\n>>\n>> - if (use_pager == -1)\n>> + builtin = get_builtin(argv[0]);\n>> + if (use_pager == -1 &&\n>> + !(builtin && builtin->option & IGNORE_PAGER_CONFIG))\n>> use_pager = check_pager_config(argv[0]);\n>> commit_pager_choice();\n>\n> ...can just go away.\n\nIf I remove this, the call I gave above will page although it\nshouldn't, and it doesn't if I keep this hunk. There's this in\nrun_argv: \"If we tried alias and futzed with our environment, it no\nlonger is safe to invoke builtins directly in general. We have to\nspawn them as dashed externals.\" There's also a NEEDSWORK.\n\nAlthough, thinking about it, I'm not sure why when I remove this hunk,\nthe child process doesn't set up the paging correctly. Maybe something\nrelated to my using \"-c\", or something about the launching of child\nprocesses. Those are both areas where I lack knowledge. Will look into\nit.\n\nMartin\n"},{"id":"324183","messageId":"CAN0heSr0czHkoxaYKeR-L-G01PWrHnLF-cArkH3ufpoR1+HZzw@mail.gmail.com","threadId":"46350","inReplyTo":"20170711103715.a4jfbjop227bfuhh@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] git.c: provide setup_auto_pager()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-11T13:47:50Z","receivedAt":"2017-07-11T13:47:56Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 11 July 2017 at 12:37, Jeff King wrote:\n> On Mon, Jul 10, 2017 at 11:55:16PM +0200, Martin Ågren wrote:\n>\n>> +void setup_auto_pager(const char *cmd, int def)\n>> +{\n>> + if (use_pager != -1)\n>> + return;\n>\n> I think you probably also want to return early here if pager_in_use()\n> is true. If a script runs \"git tag -l\", you wouldn't want to start a new\n> pager when the outer script has already been paged (either via config,\n> or via \"git -p\").\n\nGood point. Thanks.\n\nMartin\n"},{"id":"324184","messageId":"CAN0heSqbxf+BNb6PUds364j3h1oJJpMFxXG-raPfFu9XLD5WXQ@mail.gmail.com","threadId":"46350","inReplyTo":"20170711104108.vfkdo5vfoigcajdo@sigill.intra.peff.net","subject":"Re: [PATCH 6/7] tag: make git tag -l consider new config `pager.tag.list`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-11T13:48:29Z","receivedAt":"2017-07-11T13:48:34Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 11 July 2017 at 12:41, Jeff King wrote:\n> On Mon, Jul 10, 2017 at 11:55:19PM +0200, Martin Ågren wrote:\n>\n>> diff --git a/builtin/tag.c b/builtin/tag.c\n>> index e0f129872..e96ef7d70 100644\n>> --- a/builtin/tag.c\n>> +++ b/builtin/tag.c\n>> @@ -446,6 +446,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>>\n>> argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);\n>>\n>> + if (cmdmode == 'l')\n>> + setup_auto_pager(\"tag.list\", 0);\n>> setup_auto_pager(\"tag\", 0);\n>\n> Ideally this would kick in whenever we are in list mode, even if the\n> user didn't say \"-l\". So:\n>\n> $ git tag\n>\n> should probably respect the list config. Likewise, certain options like\n> \"--contains\" trigger list mode. I think the pager setup could just be\n> bumped a few lines down, past the \"if (!cmdmode)\" block that sets up\n> those defaults.\n\nOops, that's embarassing. Thanks. That means the paging starts\nslightly later, but what happens in those few lines looks fairly\ntrivial, so there shouldn't be any real difference in behavior. I'll\nadd one or two tests.\n\nMartin\n"},{"id":"324185","messageId":"CAN0heSqN6SJ_dppbYEDYVs+pSzVdLvLUDL4_VSNn79ro6c_8=g@mail.gmail.com","threadId":"46350","inReplyTo":"20170711101942.h2uwxtgzvgguzivu@sigill.intra.peff.net","subject":"Re: [PATCH 0/7] tag: more fine-grained pager-configuration","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-11T13:50:39Z","receivedAt":"2017-07-11T13:50:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 11 July 2017 at 12:19, Jeff King wrote:\n> On Mon, Jul 10, 2017 at 11:55:13PM +0200, Martin Ågren wrote:\n>\n>> Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such\n>> as \"Vim: Warning: Output is not to a terminal\" and a garbled terminal.\n>> A user who makes use of `git tag -a` and `git tag -l` will probably\n>> choose not to configure `pager.tag` or to set it to \"no\", so that `git\n>> tag -a` will actually work, at the cost of not getting the pager with\n>> `git tag -l`.\n>\n> Right, I think we are all agreed that \"pager.tag\" as it is now is\n> essentially worthless.\n>\n> If I understand your series correctly, though, it adds pager.tag.list\n> but leaves \"pager.tag\" behaving more or less the same. It's good that we\n> now have a way for a user to do the thing they actually want, but it\n> feels like we're leaving pager.tag behind as a booby-trap.\n>\n> Should we disable it entirely (or only respect it in list mode)?\n>\n> At which point, I wonder if we actually need pager.tag.list at all.\n> Slicing up the namespace further would be valuable if there were a\n> command which had two pager-worthy modes, and somebody might want to\n> enable the pager for one but not the other. But it seems like most\n> commands in this boat (e.g., tag, branch, stash) really have two modes:\n> listing things or creating things.\n>\n> Would it makes sense to just have git-tag respect pager.tag in listing\n> mode, and otherwise ignore it completely?\n\nYes. I doubt anyone would notice, and no-one should mind with the\nchange (famous last words).\n\nIt does mean there's a precedence for individual commands to get to\nchoose when to honor pager.foo. If more such exceptions are added, at\nsome point, some command will learn to ignore pager.foo in some\nparticular situation and someone will consider it a regression.\n\n> One nice side effect is that it keeps the multi-level pager.X.Y\n> namespace clear. We've talked about transitioning to allow:\n>\n> [pager \"foo\"]\n> enable = true\n> command = some-custom-pager\n>\n> to configure aspects of the pager separately for git-foo. This has two\n> benefits:\n>\n> 1. Syntactically, it allows configuration for commands whose names\n> aren't valid config keys.\n>\n> 2. It would allow setting a command with \"enable=false\", so that \"git\n> foo\" did not paginate, but \"git -p foo\" paginated with a custom\n> command.\n>\n> Those are admittedly minor features. And assuming we don't go crazy with\n> the multi-level names, we could have \"pager.tag.list\" live alongside\n> \"pager.tag.command\". So it's not really an objection, so much as wonder\n> out loud whether we can keep this as simple as possible.\n\nWell, I respect your hunch about .enable and .command and I certainly\ndon't want to take things in a direction that makes that approach less\nclean. You have convinced me that I will instead try to teach git tag\nto be more clever about when to use the pager, at least to see what\nthat looks like.\n\nLet's call such a \"git tag\" the \"future git tag\". Just to convince\nmyself I've thought through the implications -- how would\npager.tag.enable=true affect that future git tag? Would it be fair to\nsay that enable=false means \"definitely don't start the pager (unless\n--paginate)\" and that enable=true means \"feel free to use it (unless\n--no-paginate)\"? The future git tag would default to using\nenable=true. Would --paginate also be \"feel free to use it\", or rather\n\"the pager must be used\"?\n\nAt some point, I thought about \"true\"/\"false\"/\"maybe\", where \"maybe\"\nwould be what the future git tag implements. Of course, there's a fair\nchance not everyone will agree what exactly should be paged with\n\"maybe\". So it's back to adding various knobs. ;)\n\nAnyway, this is more my thinking out loud. I'll drop pager.tag.list in\nv2 and will instead make pager.tag more clever. That should force me\nto think through this some more.\n\n>> This is an attempt to implement something like that. I decided to let\n>> `pager.tag.list` fall back to `pager.tag` before falling back to \"on\".\n>> The default for `pager.tag` is still \"off\". I can see how that might\n>> seem confusing. However, my argument is that it would be awkward for\n>> `git tag -l` to ignore `pager.tag` -- we are after all running a\n>> subcommand of `git tag`. Also, this avoids a regression for someone\n>> who has set `pager.tag` and uses `git tag -l`.\n>\n> Yeah, I agree that turning \"pager.tag\" into a complete noop is probably\n> a bad idea. But if we made it a silent noop for the non-list cases, that\n> would be OK (and the hypothetical user who set it to make `git tag -l`\n> work would see a strict improvement; they'd still get their paging but\n> not the weird breakage with non-list modes). And I think that applies\n> whether we have a pager.tag.list in addition or not.\n\nGood thinking.\n\nThanks a lot for your comments. I appreciate it.\n\nMartin\n"},{"id":"324186","messageId":"20170711135429.3vq55amejo37mtjy@sigill.intra.peff.net","threadId":"46350","inReplyTo":"CAN0heSqLX1WNk3KC9U3f=qem0SYV-g=-HU+XHepi_CbuHatCOg@mail.gmail.com","subject":"Re: [PATCH 2/7] git.c: let builtins opt for handling `pager.foo` themselves","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-11T13:54:29Z","receivedAt":"2017-07-11T13:54:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 11, 2017 at 03:46:08PM +0200, Martin Ågren wrote:\n\n> > Can this ever trigger in execv_dashed_external()? We should only get\n> > there if get_builtin() returned NULL in the first place. Otherwise, we'd\n> > run and exited via handle_builtin().\n> \n> I can trigger it with this:\n> \n> $ git -c pager.tag=\"echo paging\" -c pager.tag.list=no -c alias.t=tag t -l\n> \n> where the alias is what triggers it and the two pager-configurations\n> demonstrate the effect.\n\nInteresting. As you noted below, I think the dashed external we exec\nshould be choosing whether to run the pager. I suspect what's happening\nis that execv_dashed_external() says \"a-ha, we're running 'tag', and I\nknow how to check its pager config\". But IMHO that is wrong in the first\nplace (but it just never really made a difference until your series).\n\nThat's just a guess, though. I didn't dig.\n\n-Peff\n"},{"id":"324187","messageId":"20170711140801.ocbr44wktjjj44dq@sigill.intra.peff.net","threadId":"46350","inReplyTo":"CAN0heSqN6SJ_dppbYEDYVs+pSzVdLvLUDL4_VSNn79ro6c_8=g@mail.gmail.com","subject":"Re: [PATCH 0/7] tag: more fine-grained pager-configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-11T14:08:01Z","receivedAt":"2017-07-11T14:08:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 11, 2017 at 03:50:39PM +0200, Martin Ågren wrote:\n\n> > Would it makes sense to just have git-tag respect pager.tag in listing\n> > mode, and otherwise ignore it completely?\n> \n> Yes. I doubt anyone would notice, and no-one should mind with the\n> change (famous last words).\n> \n> It does mean there's a precedence for individual commands to get to\n> choose when to honor pager.foo. If more such exceptions are added, at\n> some point, some command will learn to ignore pager.foo in some\n> particular situation and someone will consider it a regression.\n\nYes, perhaps. One could even argue that \"git tag foo\" is OK to page and\nsomebody would consider it a regression not to respect pager.tag. It\nseems useless to me, but at least it's not totally broken like \"git tag\n-a foo\" is.\n\nBut I find it pretty unlikely, as it doesn't produce copious output. I'd\nworry more about eventually finding a command with two copious-output\nmodes, and somebody wants to distinguish between them. If we can't come\nup with a plausible example now, I'm inclined to deal with it if and\nwhen it ever comes up. Right now I just don't want to paint ourselves\ninto a corner, and I think we can always add more config later (e.g.,\ntag.pageFooCommand=true or something; that's not great, but I'm mostly\nbetting that it will never come up).\n\n> Well, I respect your hunch about .enable and .command and I certainly\n> don't want to take things in a direction that makes that approach less\n> clean. You have convinced me that I will instead try to teach git tag\n> to be more clever about when to use the pager, at least to see what\n> that looks like.\n\nThanks. I was the original proponent of \"pager.tag.list\", and only after\nreading your series today did I think \"gee, this seems unnecessarily\ncomplex\". So it's entirely possible I'm missing a corner case that you\nmay uncover through discussion or experimenting.\n\n> Let's call such a \"git tag\" the \"future git tag\". Just to convince\n> myself I've thought through the implications -- how would\n> pager.tag.enable=true affect that future git tag? Would it be fair to\n> say that enable=false means \"definitely don't start the pager (unless\n> --paginate)\" and that enable=true means \"feel free to use it (unless\n> --no-paginate)\"? The future git tag would default to using\n> enable=true. Would --paginate also be \"feel free to use it\", or rather\n> \"the pager must be used\"?\n\nI think the rules would be:\n\n 1. If --paginate is given, do that. Likewise for --no-pager.\n\n 2. Otherwise, respect tag.pager.enable if it is set.\n\n 3. Otherwise, use the built-in default.\n\n 4. Any time the pager is run, respect tag.pager.command if it is set.\n\nAnd then there are two optional bits:\n\n 2a. If tag.pager is set to a boolean, behave as if tag.pager.enable is\n set to the same boolean. If it's set to a string, behave as if\n tag.pager.enable is set to \"true\", and tag.pager.command is set to\n the string. That should give us backwards compatibility.\n\n 2b. If tag.pager.command is set but tag.pager.enable isn't, possibly we\n should assume that tag.pager.enable is set to \"true\". That makes\n the common case of \"page and use this command\" a single config\n block instead of two. It matters less for \"tag\" where paging would\n be the default.\n\nSo I think that what you asked above, but to be clear on the final\nquestion: --paginate should always be \"must be used\". And that meshes\nnicely with implementing it via the git.c wrapper, where it takes\nprecedence way before we ever hit the setup_auto_pager() call.\n\n-Peff\n"},{"id":"324203","messageId":"xmqqlgnv2c2t.fsf@gitster.mtv.corp.google.com","threadId":"46350","inReplyTo":"20170711104755.4z5exp43lvwbb5hq@sigill.intra.peff.net","subject":"Re: [PATCH 0/7] tag: more fine-grained pager-configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-11T16:05:46Z","receivedAt":"2017-07-11T16:05:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> I see you CC'ed Peff who's passionate in this area, so these patches\n>> are in good hands already ;-) I briefly skimmed your patches myself,\n>> and did not spot anything glaringly wrong.\n>\n> Heh, I don't think of \"paging tag output\" as one of my passions, but you\n> may be right. :)\n\nPerhaps not \"tag output\", but the paging is the area you are the\nperson who spent most time on.\n\n> I left a few comments on the code and direction, but I agree that\n> overall it looks pretty good. And I'm very impressed with the attention\n> to detail for a first-time contributor.\n\nYes.\n"},{"id":"324216","messageId":"20170711182355.GH161700@google.com","threadId":"46350","inReplyTo":"CAN0heSq3mxRXdv0WmqqmY1tqPAJH06Eu9xyPWBxY=O1kii1ufw@mail.gmail.com","subject":"Re: [PATCH 1/7] api-builtin.txt: document SUPPORT_SUPER_PREFIX","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-11T18:23:55Z","receivedAt":"2017-07-11T18:24:09Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/11, Martin Ågren wrote:\n> On 11 July 2017 at 00:50, Brandon Williams wrote:\n> > On 07/10, Martin Ågren wrote:\n> >> Commit 74866d75 (\"git: make super-prefix option\", 2016-10-07) introduced\n> >> SUPPORT_SUPER_PREFIX as a builtin flag without documenting it in\n> >> api-builtin.txt. The next patch will add another flag, so document\n> >> SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with\n> >> the available flags.\n> >>\n> >> Signed-off-by: Martin Ågren \n> >> ---\n> >> Documentation/technical/api-builtin.txt | 4 ++++\n> >> 1 file changed, 4 insertions(+)\n> >>\n> >> diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt\n> >> index 22a39b929..60f442822 100644\n> >> --- a/Documentation/technical/api-builtin.txt\n> >> +++ b/Documentation/technical/api-builtin.txt\n> >> @@ -42,6 +42,10 @@ where options is the bitwise-or of:\n> >> on bare repositories.\n> >> This only makes sense when `RUN_SETUP` is also set.\n> >>\n> >> +`SUPPORT_SUPER_PREFIX`::\n> >> +\n> >> + The builtin supports --super-prefix.\n> >> +\n> >> . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n> >>\n> >> Additionally, if `foo` is a new command, there are 3 more things to do:\n> >\n> > I added SUPER_PREFIX as an implementation detail when trying to recurse\n> > submodules using multiple processes. Now that we have a 'struct\n> > repository' my plan is to remove SUPER_PREFIX in its entirety. Since\n> > this won't happen overnight it may still take a bit till its removed so\n> > maybe it makes sense to better document it until that happens?\n> \n> I could add something about how this is \"temporary\" although I have no\n> idea about the timeframe. ;-)\n\nIts probably better to ensure that things are documented (even if they\nare temporary) but you don't need to mark it as such. And as far as\ntimeframe, that's my burden to bare ;)\n\n> \n> > Either way I think that this sort of Documention better lives in the\n> > code as it is easier to keep up to date.\n> \n> Agreed and I believe that's the long-term goal. I could replace this\n> preparatory patch with another one where I move api-builtin.txt into\n> builtin.h or git.c (and document SUPPORT_SUPER_PREFIX if that's\n> wanted). That's probably better, since right now, patch 2/7 adds\n> basically the same documentation for the new flag in two places.\n\nI know that sort of migration may be out of the scope of your series\n(since its just documentation), though you're more than welcome to do\nthe work as it would be much appreciated by me and a few other people :)\n\n-- \nBrandon Williams\n"},{"id":"324255","messageId":"CAN0heSq-HHsW5Cd_VvshNS_qHW3nVz-yzwSfFQin8Dyr2hhksA@mail.gmail.com","threadId":"46350","inReplyTo":"xmqqpod752yh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/7] tag: more fine-grained pager-configuration","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-12T07:29:04Z","receivedAt":"2017-07-12T07:29:11Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 11 July 2017 at 00:42, Junio C Hamano wrote:\n> Martin Ågren writes:\n>> I am not moving all builtins to handling the pager on their own,\n>> but instead introduce a flag IGNORE_PAGER_CONFIG and use it only\n>> with the tag builtin. That means there's another flag to reason\n>> about, but it avoids moving all builtins to handling the paging\n>> themselves just to make one of them do something more \"clever\".\n...\n> Even though it is purely internal thing, IGNORE_PAGER_CONFIG\n> probably is a bit confusion-inducing name. After all, the\n> subcommand specific configuration is not being ignored---we are\n> merely delaying our reaction to it---instead of acting on it inside\n> git.c without giving the subcommand a chance to make a decision, we\n> are still letting (and we do expect) the subcommand to react to it.\n\nThank you for your comments. I'm working on v2 with the input from\nyou, Peff and Brandon. I'll make this DELAY_PAGER_CONFIG.\n\nMartin\n"},{"id":"324669","messageId":"cover.1500321657.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1499723297.git.martin.agren@gmail.com","subject":"[PATCH v2 00/10] tag: only respect `pager.tag` in list-mode","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:42Z","receivedAt":"2017-07-17T20:11:27Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is the second version of \"[PATCH 0/7] tag: more fine-grained\npager-configuration\" [1]. That series introduced `pager.tag.list` to\naddress the fact that `pager.tag` can be useful with `git tag -l` but\nactively hostile with `git tag -a`. Thanks to Junio, Peff and Brandon\nfor helpful feedback.\n\nAfter that feedback, v2 drops `pager.tag.list` and instead teaches\n`git tag` to only consider `pager.tag` in list-mode, as suggested by\nPeff.\n\nPatches 1-3/10 replace patch 1/7. They move Documentation/technical/\napi-builtin.txt into builtin.h, tweak the formatting and bring it up to\ndate. I may have gone overboard making this 3 patches...\n\nPatches 4-7/10 correspond to patches 2-5/7. `setup_auto_pager()' is now\nmuch simpler since we do not need to handle \"tag.list\" with a clever\nfallback strategy. IGNORE_PAGER_CONFIG is now called DELAY_PAGER_CONFIG.\nI now check with pager_in_use() and I moved the handling of `pager.tag`\na bit further down.\n\nPatches 8-9/10 teach `git tag` to only respect `pager.tag` in list-mode\nand flip the default value for that config to \"on\".\n\nPatch 10/10 is somewhat similar to a hunk in patch 2/7, but is now a\nbug-fix instead of a feature. It teaches `execv_dashed_external()` not\nto check `pager.foo` when launching `git-foo` where foo is a builtin.\nI waffled about where to put this patch. Putting it earlier in the\nseries as a preparatory step, I couldn't come up with a way of writing a\ntest. So patch 8/10 introduces a `test_expect_failure` which this patch\nthen fixes.\n\nMartin\n\n[1] https://public-inbox.org/git/cover.1499723297.git.martin.agren@gmail.com/T/\n\nMartin Ågren (10):\n builtin.h: take over documentation from api-builtin.txt\n builtin.h: format documentation-comment properly\n builtin.h: document SUPPORT_SUPER_PREFIX\n git.c: let builtins opt for handling `pager.foo` themselves\n git.c: provide setup_auto_pager()\n t7006: add tests for how git tag paginates\n tag: handle `pager.tag`-configuration within the builtin\n tag: respect `pager.tag` in list-mode only\n tag: change default of `pager.tag` to \"on\"\n git.c: ignore pager.* when launching builtin as dashed external\n\n Documentation/git-tag.txt | 3 +\n Documentation/technical/api-builtin.txt | 73 -------------------------\n t/t7006-pager.sh | 85 +++++++++++++++++++++++++++++\n builtin.h | 97 +++++++++++++++++++++++++++++++++\n builtin/tag.c | 3 +\n git.c | 18 +++++-\n 6 files changed, 203 insertions(+), 76 deletions(-)\n delete mode 100644 Documentation/technical/api-builtin.txt\n\n-- \n2.14.0.rc0\n\n"},{"id":"324670","messageId":"77fdbe70b0807bb9593714f4d43ae96404a73538.1500321657.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v2 01/10] builtin.h: take over documentation from api-builtin.txt","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:43Z","receivedAt":"2017-07-17T20:11:34Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Delete Documentation/technical/api-builtin.txt and move its content\nverbatim into builtin.h. Just wrap it in /* ... */. In order to make\nthe move obviously correct, do not change any formatting, not even to\nformat the comment into Git's preferred style. That will be done in a\nfollow-up patch.\n\nSigned-off-by: Martin Ågren \n---\n Documentation/technical/api-builtin.txt | 73 -------------------------------\n builtin.h | 76 +++++++++++++++++++++++++++++++++\n 2 files changed, 76 insertions(+), 73 deletions(-)\n delete mode 100644 Documentation/technical/api-builtin.txt\n\ndiff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt\ndeleted file mode 100644\nindex 22a39b929..000000000\n--- a/Documentation/technical/api-builtin.txt\n+++ /dev/null\n@@ -1,73 +0,0 @@\n-builtin API\n-===========\n-\n-Adding a new built-in\n----------------------\n-\n-There are 4 things to do to add a built-in command implementation to\n-Git:\n-\n-. Define the implementation of the built-in command `foo` with\n- signature:\n-\n-\tint cmd_foo(int argc, const char **argv, const char *prefix);\n-\n-. Add the external declaration for the function to `builtin.h`.\n-\n-. Add the command to the `commands[]` table defined in `git.c`.\n- The entry should look like:\n-\n-\t{ \"foo\", cmd_foo, },\n-+\n-where options is the bitwise-or of:\n-\n-`RUN_SETUP`::\n-\tIf there is not a Git directory to work on, abort. If there\n-\tis a work tree, chdir to the top of it if the command was\n-\tinvoked in a subdirectory. If there is no work tree, no\n-\tchdir() is done.\n-\n-`RUN_SETUP_GENTLY`::\n-\tIf there is a Git directory, chdir as per RUN_SETUP, otherwise,\n-\tdon't chdir anywhere.\n-\n-`USE_PAGER`::\n-\n-\tIf the standard output is connected to a tty, spawn a pager and\n-\tfeed our output to it.\n-\n-`NEED_WORK_TREE`::\n-\n-\tMake sure there is a work tree, i.e. the command cannot act\n-\ton bare repositories.\n-\tThis only makes sense when `RUN_SETUP` is also set.\n-\n-. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n-\n-Additionally, if `foo` is a new command, there are 3 more things to do:\n-\n-. Add tests to `t/` directory.\n-\n-. Write documentation in `Documentation/git-foo.txt`.\n-\n-. Add an entry for `git-foo` to `command-list.txt`.\n-\n-. Add an entry for `/git-foo` to `.gitignore`.\n-\n-\n-How a built-in is called\n-------------------------\n-\n-The implementation `cmd_foo()` takes three parameters, `argc`, `argv,\n-and `prefix`. The first two are similar to what `main()` of a\n-standalone command would be called with.\n-\n-When `RUN_SETUP` is specified in the `commands[]` table, and when you\n-were started from a subdirectory of the work tree, `cmd_foo()` is called\n-after chdir(2) to the top of the work tree, and `prefix` gets the path\n-to the subdirectory the command started from. This allows you to\n-convert a user-supplied pathname (typically relative to that directory)\n-to a pathname relative to the top of the work tree.\n-\n-The return value from `cmd_foo()` becomes the exit status of the\n-command.\ndiff --git a/builtin.h b/builtin.h\nindex 498ac80d0..51cb0249d 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -6,6 +6,82 @@\n #include \"cache.h\"\n #include \"commit.h\"\n \n+/*\n+builtin API\n+===========\n+\n+Adding a new built-in\n+---------------------\n+\n+There are 4 things to do to add a built-in command implementation to\n+Git:\n+\n+. Define the implementation of the built-in command `foo` with\n+ signature:\n+\n+\tint cmd_foo(int argc, const char **argv, const char *prefix);\n+\n+. Add the external declaration for the function to `builtin.h`.\n+\n+. Add the command to the `commands[]` table defined in `git.c`.\n+ The entry should look like:\n+\n+\t{ \"foo\", cmd_foo, },\n++\n+where options is the bitwise-or of:\n+\n+`RUN_SETUP`::\n+\tIf there is not a Git directory to work on, abort. If there\n+\tis a work tree, chdir to the top of it if the command was\n+\tinvoked in a subdirectory. If there is no work tree, no\n+\tchdir() is done.\n+\n+`RUN_SETUP_GENTLY`::\n+\tIf there is a Git directory, chdir as per RUN_SETUP, otherwise,\n+\tdon't chdir anywhere.\n+\n+`USE_PAGER`::\n+\n+\tIf the standard output is connected to a tty, spawn a pager and\n+\tfeed our output to it.\n+\n+`NEED_WORK_TREE`::\n+\n+\tMake sure there is a work tree, i.e. the command cannot act\n+\ton bare repositories.\n+\tThis only makes sense when `RUN_SETUP` is also set.\n+\n+. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n+\n+Additionally, if `foo` is a new command, there are 3 more things to do:\n+\n+. Add tests to `t/` directory.\n+\n+. Write documentation in `Documentation/git-foo.txt`.\n+\n+. Add an entry for `git-foo` to `command-list.txt`.\n+\n+. Add an entry for `/git-foo` to `.gitignore`.\n+\n+\n+How a built-in is called\n+------------------------\n+\n+The implementation `cmd_foo()` takes three parameters, `argc`, `argv,\n+and `prefix`. The first two are similar to what `main()` of a\n+standalone command would be called with.\n+\n+When `RUN_SETUP` is specified in the `commands[]` table, and when you\n+were started from a subdirectory of the work tree, `cmd_foo()` is called\n+after chdir(2) to the top of the work tree, and `prefix` gets the path\n+to the subdirectory the command started from. This allows you to\n+convert a user-supplied pathname (typically relative to that directory)\n+to a pathname relative to the top of the work tree.\n+\n+The return value from `cmd_foo()` becomes the exit status of the\n+command.\n+ */\n+\n #define DEFAULT_MERGE_LOG_LEN 20\n \n extern const char git_usage_string[];\n-- \n2.14.0.rc0\n\n"},{"id":"324671","messageId":"c0a58c6eacb79bde92a594b501971cb8360e507f.1500321657.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v2 02/10] builtin.h: format documentation-comment properly","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:44Z","receivedAt":"2017-07-17T20:11:37Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The previous commit moved technical documentation verbatim into\nbuiltin.h. Prefix all the lines with '*' to follow the coding\nguidelines.\n\nRemove a '+' which was needed when the information was formatted for\nAsciiDoc. Similarly, change \"::\" to \":\".\n\nSigned-off-by: Martin Ågren \n---\n builtin.h | 146 +++++++++++++++++++++++++++++++-------------------------------\n 1 file changed, 73 insertions(+), 73 deletions(-)\n\ndiff --git a/builtin.h b/builtin.h\nindex 51cb0249d..62f22b547 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -7,79 +7,79 @@\n #include \"commit.h\"\n \n /*\n-builtin API\n-===========\n-\n-Adding a new built-in\n----------------------\n-\n-There are 4 things to do to add a built-in command implementation to\n-Git:\n-\n-. Define the implementation of the built-in command `foo` with\n- signature:\n-\n-\tint cmd_foo(int argc, const char **argv, const char *prefix);\n-\n-. Add the external declaration for the function to `builtin.h`.\n-\n-. Add the command to the `commands[]` table defined in `git.c`.\n- The entry should look like:\n-\n-\t{ \"foo\", cmd_foo, },\n-+\n-where options is the bitwise-or of:\n-\n-`RUN_SETUP`::\n-\tIf there is not a Git directory to work on, abort. If there\n-\tis a work tree, chdir to the top of it if the command was\n-\tinvoked in a subdirectory. If there is no work tree, no\n-\tchdir() is done.\n-\n-`RUN_SETUP_GENTLY`::\n-\tIf there is a Git directory, chdir as per RUN_SETUP, otherwise,\n-\tdon't chdir anywhere.\n-\n-`USE_PAGER`::\n-\n-\tIf the standard output is connected to a tty, spawn a pager and\n-\tfeed our output to it.\n-\n-`NEED_WORK_TREE`::\n-\n-\tMake sure there is a work tree, i.e. the command cannot act\n-\ton bare repositories.\n-\tThis only makes sense when `RUN_SETUP` is also set.\n-\n-. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n-\n-Additionally, if `foo` is a new command, there are 3 more things to do:\n-\n-. Add tests to `t/` directory.\n-\n-. Write documentation in `Documentation/git-foo.txt`.\n-\n-. Add an entry for `git-foo` to `command-list.txt`.\n-\n-. Add an entry for `/git-foo` to `.gitignore`.\n-\n-\n-How a built-in is called\n-------------------------\n-\n-The implementation `cmd_foo()` takes three parameters, `argc`, `argv,\n-and `prefix`. The first two are similar to what `main()` of a\n-standalone command would be called with.\n-\n-When `RUN_SETUP` is specified in the `commands[]` table, and when you\n-were started from a subdirectory of the work tree, `cmd_foo()` is called\n-after chdir(2) to the top of the work tree, and `prefix` gets the path\n-to the subdirectory the command started from. This allows you to\n-convert a user-supplied pathname (typically relative to that directory)\n-to a pathname relative to the top of the work tree.\n-\n-The return value from `cmd_foo()` becomes the exit status of the\n-command.\n+ * builtin API\n+ * ===========\n+ *\n+ * Adding a new built-in\n+ * ---------------------\n+ *\n+ * There are 4 things to do to add a built-in command implementation to\n+ * Git:\n+ *\n+ * . Define the implementation of the built-in command `foo` with\n+ * signature:\n+ *\n+ *\tint cmd_foo(int argc, const char **argv, const char *prefix);\n+ *\n+ * . Add the external declaration for the function to `builtin.h`.\n+ *\n+ * . Add the command to the `commands[]` table defined in `git.c`.\n+ * The entry should look like:\n+ *\n+ *\t{ \"foo\", cmd_foo, },\n+ *\n+ * where options is the bitwise-or of:\n+ *\n+ * `RUN_SETUP`:\n+ *\tIf there is not a Git directory to work on, abort. If there\n+ *\tis a work tree, chdir to the top of it if the command was\n+ *\tinvoked in a subdirectory. If there is no work tree, no\n+ *\tchdir() is done.\n+ *\n+ * `RUN_SETUP_GENTLY`:\n+ *\tIf there is a Git directory, chdir as per RUN_SETUP, otherwise,\n+ *\tdon't chdir anywhere.\n+ *\n+ * `USE_PAGER`:\n+ *\n+ *\tIf the standard output is connected to a tty, spawn a pager and\n+ *\tfeed our output to it.\n+ *\n+ * `NEED_WORK_TREE`:\n+ *\n+ *\tMake sure there is a work tree, i.e. the command cannot act\n+ *\ton bare repositories.\n+ *\tThis only makes sense when `RUN_SETUP` is also set.\n+ *\n+ * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n+ *\n+ * Additionally, if `foo` is a new command, there are 3 more things to do:\n+ *\n+ * . Add tests to `t/` directory.\n+ *\n+ * . Write documentation in `Documentation/git-foo.txt`.\n+ *\n+ * . Add an entry for `git-foo` to `command-list.txt`.\n+ *\n+ * . Add an entry for `/git-foo` to `.gitignore`.\n+ *\n+ *\n+ * How a built-in is called\n+ * ------------------------\n+ *\n+ * The implementation `cmd_foo()` takes three parameters, `argc`, `argv,\n+ * and `prefix`. The first two are similar to what `main()` of a\n+ * standalone command would be called with.\n+ *\n+ * When `RUN_SETUP` is specified in the `commands[]` table, and when you\n+ * were started from a subdirectory of the work tree, `cmd_foo()` is called\n+ * after chdir(2) to the top of the work tree, and `prefix` gets the path\n+ * to the subdirectory the command started from. This allows you to\n+ * convert a user-supplied pathname (typically relative to that directory)\n+ * to a pathname relative to the top of the work tree.\n+ *\n+ * The return value from `cmd_foo()` becomes the exit status of the\n+ * command.\n */\n \n #define DEFAULT_MERGE_LOG_LEN 20\n-- \n2.14.0.rc0\n\n"},{"id":"324672","messageId":"715342d85810ddae1372fb4a1c5e300d323a2b58.1500321657.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v2 03/10] builtin.h: document SUPPORT_SUPER_PREFIX","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:45Z","receivedAt":"2017-07-17T20:11:39Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit 74866d75 (\"git: make super-prefix option\", 2016-10-07) introduced\nSUPPORT_SUPER_PREFIX as a builtin flag without documenting it. The next\npatch will add another flag, so document SUPPORT_SUPER_PREFIX, thereby\nbringing the documentation up to date with the available flags.\n\nWhile at it, correct '3 more things to do' to '4 more things to do'.\n\nSigned-off-by: Martin Ågren \n---\n builtin.h | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin.h b/builtin.h\nindex 62f22b547..7bcc08456 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -51,9 +51,13 @@\n *\ton bare repositories.\n *\tThis only makes sense when `RUN_SETUP` is also set.\n *\n+ * `SUPPORT_SUPER_PREFIX`::\n+ *\n+ *\tThe builtin supports `--super-prefix`.\n+ *\n * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n *\n- * Additionally, if `foo` is a new command, there are 3 more things to do:\n+ * Additionally, if `foo` is a new command, there are 4 more things to do:\n *\n * . Add tests to `t/` directory.\n *\n-- \n2.14.0.rc0\n\n"},{"id":"324673","messageId":"be05f05ba0066cae1f222c90464798fa5013a585.1500321657.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v2 04/10] git.c: let builtins opt for handling `pager.foo` themselves","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:46Z","receivedAt":"2017-07-17T20:11:43Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Before launching a builtin git foo and unless mechanisms with precedence\nare in use, we check for and handle the `pager.foo` config. This is done\nwithout considering exactly how git foo is being used, and indeed, git.c\ncannot (and should not) know what the arguments to git foo are supposed\nto achieve.\n\nIn practice this means that, e.g., `git -c pager.tag tag -a new-tag`\nresults in errors such as \"Vim: Warning: Output is not to a terminal\"\nand a garbled terminal. A user who makes use of `git tag -a` and `git\ntag -l` will probably choose not to configure `pager.tag` or to set it\nto \"no\", so that `git tag -a` will actually work, at the cost of not\ngetting the pager with `git tag -l`.\n\nTo allow individual builtins to make more informed decisions about when\nto respect `pager.foo`, introduce a flag DELAY_PAGER_CONFIG. If the flag\nis set, do not check `pager.foo`.\n\nDo not check for DELAY_PAGER_CONFIG in `execv_dashed_external()`. That\ncall site is arguably wrong, although in a way that is not yet visible,\nand will be changed in a slightly different direction in a later patch.\n\nDon't add any users of DELAY_PAGER_CONFIG just yet, one will follow in a\nlater patch.\n\nSuggested-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n builtin.h | 8 ++++++++\n git.c | 4 +++-\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin.h b/builtin.h\nindex 7bcc08456..4186635de 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -55,6 +55,14 @@\n *\n *\tThe builtin supports `--super-prefix`.\n *\n+ * `DELAY_PAGER_CONFIG`::\n+ *\n+ *\tIf RUN_SETUP or RUN_SETUP_GENTLY is set, git.c normally handles\n+ *\tthe `pager.`-configuration. If this flag is used, git.c\n+ *\twill skip that step, instead allowing the builtin to make a\n+ *\tmore informed decision, e.g., by ignoring `pager.` for\n+ *\tcertain subcommands.\n+ *\n * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n *\n * Additionally, if `foo` is a new command, there are 4 more things to do:\ndiff --git a/git.c b/git.c\nindex 489aab4d8..79195ebbd 100644\n--- a/git.c\n+++ b/git.c\n@@ -283,6 +283,7 @@ static int handle_alias(int *argcp, const char ***argv)\n */\n #define NEED_WORK_TREE\t\t(1<<3)\n #define SUPPORT_SUPER_PREFIX\t(1<<4)\n+#define DELAY_PAGER_CONFIG\t(1<<5)\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -306,7 +307,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\t\tprefix = setup_git_directory_gently(&nongit_ok);\n \t\t}\n \n-\t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))\n+\t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n+\t\t !(p->option & DELAY_PAGER_CONFIG))\n \t\t\tuse_pager = check_pager_config(p->cmd);\n \t\tif (use_pager == -1 && p->option & USE_PAGER)\n \t\t\tuse_pager = 1;\n-- \n2.14.0.rc0\n\n"},{"id":"324674","messageId":"b6ab825d3bda4b40201b0aa8e97c9fd721ed8171.1500321658.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v2 05/10] git.c: provide setup_auto_pager()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:47Z","receivedAt":"2017-07-17T20:11:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The previous patch introduced a way for builtins to declare that they\nwill take responsibility for handling the `pager.foo`-config item. (See\nthe commit message of that patch for why that could be useful.)\n\nProvide setup_auto_pager(), which builtins can call in order to handle\n`pager.`, including possibly starting the pager. Make this function\ndon't do anything if a pager has already been started, as indicated by\nuse_pager or pager_in_use().\n\nWhenever this function is called from a builtin, git.c will already have\ncalled commit_pager_choice(). Since commit_pager_choice() treats the\nspecial value -1 as \"punt\" or \"not yet decided\", it is not a problem\nthat we might end up calling commit_pager_choice() once in git.c and\nonce (or more) in the builtin. Make the new function use -1 in the same\nway and document it as \"punt\".\n\nDon't add any users of setup_auto_pager just yet, one will follow in\na later patch.\n\nSigned-off-by: Martin Ågren \n---\n builtin.h | 9 +++++++++\n git.c | 10 ++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/builtin.h b/builtin.h\nindex 4186635de..3ca4a53a8 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -113,6 +113,15 @@ struct fmt_merge_msg_opts {\n extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \t\t\t struct fmt_merge_msg_opts *);\n \n+/**\n+ * If a builtin has DELAY_PAGER_CONFIG set, the builtin should call this early\n+ * when it wishes to respect the `pager.foo`-config. The `cmd` is the name of\n+ * the builtin, e.g., \"foo\". If a paging-choice has already been setup, this\n+ * does nothing. The default in `def` should be 0 for \"pager off\", 1 for \"pager\n+ * on\" or -1 for \"punt\".\n+ */\n+extern void setup_auto_pager(const char *cmd, int def);\n+\n extern int is_builtin(const char *s);\n \n extern int cmd_add(int argc, const char **argv, const char *prefix);\ndiff --git a/git.c b/git.c\nindex 79195ebbd..66832f232 100644\n--- a/git.c\n+++ b/git.c\n@@ -33,6 +33,16 @@ static void commit_pager_choice(void) {\n \t}\n }\n \n+void setup_auto_pager(const char *cmd, int def)\n+{\n+\tif (use_pager != -1 || pager_in_use())\n+\t\treturn;\n+\tuse_pager = check_pager_config(cmd);\n+\tif (use_pager == -1)\n+\t\tuse_pager = def;\n+\tcommit_pager_choice();\n+}\n+\n static int handle_options(const char ***argv, int *argc, int *envchanged)\n {\n \tconst char **orig_argv = *argv;\n-- \n2.14.0.rc0\n\n"},{"id":"324675","messageId":"f4334503b677bbbeb19660e501ad2d7213428953.1500321658.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v2 06/10] t7006: add tests for how git tag paginates","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:48Z","receivedAt":"2017-07-17T20:11:48Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as\n\"Vim: Warning: Output is not to a terminal\" and a garbled terminal. A\nuser who makes use of `git tag -a` and `git tag -l` will probably choose\nnot to configure `pager.tag` or to set it to \"no\", so that `git tag -a`\nwill actually work, at the cost of not getting the pager with `git tag\n-l`.\n\nSince we're about to change how `git tag` respects `pager.tag`, add tests\naround this, including how the configuration is ignored if --no-pager or\n--paginate are used.\n\nConstruct tests with a few different subcommands. First, use -l. Second,\nuse \"no arguments\" and --contains, since those imply -l. (There are\nmore arguments which imply -l, but using these two should be enough.)\nThird, use -a as a representative for \"not -l\".\n\nMake one of the tests demonstrate the behavior mentioned above, where\n`git tag -a` respects `pager.tag`. Actually, the tests use `git tag -am`\nso no editor is launched, but that is irrelevant, since we just want to\nsee whether the pager is used or not.\n\nSigned-off-by: Martin Ågren \n---\n t/t7006-pager.sh | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 68 insertions(+)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 20b4d83c2..e7430bc93 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -134,6 +134,74 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '\n \t}\n '\n \n+test_expect_success TTY 'git tag -l defaults to not paging' '\n+\trm -f paginated.out &&\n+\ttest_terminal git tag -l &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -l respects pager.tag' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag tag -l &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -l respects --no-pager' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag --no-pager tag -l &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag with no args defaults to not paging' '\n+\t# no args implies -l so this should page like -l\n+\trm -f paginated.out &&\n+\ttest_terminal git tag &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag with no args respects pager.tag' '\n+\t# no args implies -l so this should page like -l\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag tag &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag --contains defaults to not paging' '\n+\t# --contains implies -l so this should page like -l\n+\trm -f paginated.out &&\n+\ttest_terminal git tag --contains &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag --contains respects pager.tag' '\n+\t# --contains implies -l so this should page like -l\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag tag --contains &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -a defaults to not paging' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git tag -am message newtag &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -a respects pager.tag' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag tag -am message newtag &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -a respects --paginate' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag=false --paginate \\\n+\t\ttag -am message newtag &&\n+\ttest -e paginated.out\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.14.0.rc0\n\n"},{"id":"324676","messageId":"37f4efda3f26649bc81c806576d770a2268aa4b2.1500321658.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v2 07/10] tag: handle `pager.tag`-configuration within the builtin","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:49Z","receivedAt":"2017-07-17T20:11:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Use the mechanisms introduced in two earlier patches to ignore\n`pager.tag` in git.c and let the `git tag` builtin handle it on its own.\n\nThis is in preparation for the next patch, where we will want to ignore\n`pager.tag` unless we run in list-mode. For this reason, place the call\nto setup_auto_pager() after the options have been parsed and we have\ndecided whether we are in list-mode.\n\nNo functional change is intended. That said, there is a window between\nwhere the pager is started before and after this patch, and if an error\noccurs within this window, as of this patch the error message might not\nbe paged where it would have been paged before. Since\noperation-parsing has to happen inside this window, a difference can be\nseen with, e.g., `git -c pager.tag=\"echo pager is used\" tag\n--unknown-option`. This change in paging-behavior should be acceptable\nsince it only affects erroneous usages.\n\nSigned-off-by: Martin Ågren \n---\n builtin/tag.c | 2 ++\n git.c | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 01154ea8d..9eda434fb 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -461,6 +461,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcmdmode = 'l';\n \t}\n \n+\tsetup_auto_pager(\"tag\", 0);\n+\n \tif ((create_tag_object || force) && (cmdmode != 0))\n \t\tusage_with_options(git_tag_usage, options);\n \ndiff --git a/git.c b/git.c\nindex 66832f232..82ac2a092 100644\n--- a/git.c\n+++ b/git.c\n@@ -466,7 +466,7 @@ static struct cmd_struct commands[] = {\n \t{ \"stripspace\", cmd_stripspace },\n \t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n-\t{ \"tag\", cmd_tag, RUN_SETUP },\n+\t{ \"tag\", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },\n \t{ \"unpack-file\", cmd_unpack_file, RUN_SETUP },\n \t{ \"unpack-objects\", cmd_unpack_objects, RUN_SETUP },\n \t{ \"update-index\", cmd_update_index, RUN_SETUP },\n-- \n2.14.0.rc0\n\n"},{"id":"324677","messageId":"ca309a7d46027c2804b82813600aaf96f65ca01b.1500321658.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v2 08/10] tag: respect `pager.tag` in list-mode only","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:50Z","receivedAt":"2017-07-17T20:11:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as\n\"Vim: Warning: Output is not to a terminal\" and a garbled terminal. A\nuser who makes use of `git tag -a` and `git tag -l` will probably choose\nnot to configure `pager.tag` or to set it to \"no\", so that `git tag -a`\nwill actually work, at the cost of not getting the pager with `git tag\n-l`.\n\nTeach git tag to only respect `pager.tag` when running in list-mode.\nUpdate the documentation and update tests.\n\nIf an alias is used to run `git tag -a`, then `pager.tag` will still be\nrespected. Document this known breakage. It will be fixed in a later\ncommit. Add a similar test for `-l`, which works as it should.\n\nNoticed-by: Anatoly Borodin \nSuggested-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n Documentation/git-tag.txt | 3 +++\n t/t7006-pager.sh | 25 +++++++++++++++++++++----\n builtin/tag.c | 3 ++-\n 3 files changed, 26 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 1eb15afa1..875d135e0 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -205,6 +205,9 @@ it in the repository configuration as follows:\n signingKey = \n -------------------------------------\n \n+`pager.tag` is only respected when listing tags, i.e., when `-l` is\n+used or implied.\n+See linkgit:git-config[1].\n \n DISCUSSION\n ----------\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex e7430bc93..a357436e1 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -187,18 +187,35 @@ test_expect_success TTY 'git tag -a defaults to not paging' '\n \t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git tag -a respects pager.tag' '\n+test_expect_success TTY 'git tag -a ignores pager.tag' '\n \ttest_when_finished \"git tag -d newtag\" &&\n \trm -f paginated.out &&\n \ttest_terminal git -c pager.tag tag -am message newtag &&\n-\ttest -e paginated.out\n+\t! test -e paginated.out\n '\n \n test_expect_success TTY 'git tag -a respects --paginate' '\n \ttest_when_finished \"git tag -d newtag\" &&\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag=false --paginate \\\n-\t\ttag -am message newtag &&\n+\ttest_terminal git --paginate tag -am message newtag &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '\n+\t# git-tag will be launched as a dashed external, which\n+\t# 1) is the source of a potential bug, and\n+\t# 2) is why we use test_config and not -c.\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_config pager.tag true &&\n+\ttest_terminal git -c alias.t=tag t -am message newtag &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag as alias respects pager.tag with -l' '\n+\trm -f paginated.out &&\n+\ttest_config pager.tag true &&\n+\ttest_terminal git -c alias.t=tag t -l &&\n \ttest -e paginated.out\n '\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 9eda434fb..5ad1af252 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -461,7 +461,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcmdmode = 'l';\n \t}\n \n-\tsetup_auto_pager(\"tag\", 0);\n+\tif (cmdmode == 'l')\n+\t\tsetup_auto_pager(\"tag\", 0);\n \n \tif ((create_tag_object || force) && (cmdmode != 0))\n \t\tusage_with_options(git_tag_usage, options);\n-- \n2.14.0.rc0\n\n"},{"id":"324678","messageId":"86ab09419c951ebeee2435c9c294f4b3f8816775.1500321658.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v2 09/10] tag: change default of `pager.tag` to \"on\"","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:51Z","receivedAt":"2017-07-17T20:11:57Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The previous patch taught `git tag` to only respect `pager.tag` in\nlist-mode. That patch left the default value of `pager.tag` at \"off\".\n\nAfter that patch, it makes sense to let the default value be \"on\"\ninstead, since it will help with listing many tags, but will not hurt\nusers of `git tag -a` as it would have before. Make that change. Update\ndocumentation and tests.\n\nSigned-off-by: Martin Ågren \n---\n Documentation/git-tag.txt | 2 +-\n t/t7006-pager.sh | 28 ++++++++++++++--------------\n builtin/tag.c | 2 +-\n 3 files changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 875d135e0..d97aad343 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -206,7 +206,7 @@ it in the repository configuration as follows:\n -------------------------------------\n \n `pager.tag` is only respected when listing tags, i.e., when `-l` is\n-used or implied.\n+used or implied. The default is to use a pager.\n See linkgit:git-config[1].\n \n DISCUSSION\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex a357436e1..df258c5d4 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -134,16 +134,16 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '\n \t}\n '\n \n-test_expect_success TTY 'git tag -l defaults to not paging' '\n+test_expect_success TTY 'git tag -l defaults to paging' '\n \trm -f paginated.out &&\n \ttest_terminal git tag -l &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n test_expect_success TTY 'git tag -l respects pager.tag' '\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag tag -l &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.tag=false tag -l &&\n+\t! test -e paginated.out\n '\n \n test_expect_success TTY 'git tag -l respects --no-pager' '\n@@ -152,32 +152,32 @@ test_expect_success TTY 'git tag -l respects --no-pager' '\n \t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git tag with no args defaults to not paging' '\n+test_expect_success TTY 'git tag with no args defaults to paging' '\n \t# no args implies -l so this should page like -l\n \trm -f paginated.out &&\n \ttest_terminal git tag &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n test_expect_success TTY 'git tag with no args respects pager.tag' '\n \t# no args implies -l so this should page like -l\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag tag &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.tag=no tag &&\n+\t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git tag --contains defaults to not paging' '\n+test_expect_success TTY 'git tag --contains defaults to paging' '\n \t# --contains implies -l so this should page like -l\n \trm -f paginated.out &&\n \ttest_terminal git tag --contains &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n test_expect_success TTY 'git tag --contains respects pager.tag' '\n \t# --contains implies -l so this should page like -l\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag tag --contains &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.tag=false tag --contains &&\n+\t! test -e paginated.out\n '\n \n test_expect_success TTY 'git tag -a defaults to not paging' '\n@@ -214,9 +214,9 @@ test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '\n \n test_expect_success TTY 'git tag as alias respects pager.tag with -l' '\n \trm -f paginated.out &&\n-\ttest_config pager.tag true &&\n+\ttest_config pager.tag false &&\n \ttest_terminal git -c alias.t=tag t -l &&\n-\ttest -e paginated.out\n+\t! test -e paginated.out\n '\n \n # A colored commit log will begin with an appropriate ANSI escape\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5ad1af252..ea83df5e1 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -462,7 +462,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (cmdmode == 'l')\n-\t\tsetup_auto_pager(\"tag\", 0);\n+\t\tsetup_auto_pager(\"tag\", 1);\n \n \tif ((create_tag_object || force) && (cmdmode != 0))\n \t\tusage_with_options(git_tag_usage, options);\n-- \n2.14.0.rc0\n\n"},{"id":"324679","messageId":"fa91f778e1acff812ddadb313e9979c470edc43b.1500321658.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-17T20:10:52Z","receivedAt":"2017-07-17T20:11:59Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and\nexecute `git-foo` as a dashed external. This is true even if git foo is\na builtin. That is on purpose, and is motivated in a comment which was\nadded in commit 441981bc (\"git: simplify environment save/restore\nlogic\", 2016-01-26).\n\nShortly before we launch a dashed external, and unless we have already\nfound out whether we should use a pager, we check `pager.foo`. This was\nadded in commit 92058e4d (\"support pager.* for external commands\",\n2011-08-18). If the dashed external is a builtin, this does not match\nthat commit's intention and is arguably wrong, since it would be cleaner\nif we let the \"dashed external builtin\" handle `pager.foo`.\n\nThis has not mattered in practice, but a recent patch taught `git-tag`\nto ignore `pager.tag` under certain circumstances. But, when started\nusing an alias, it doesn't get the chance to do so, as outlined above.\nThat recent patch added a test to document this breakage.\n\nDo not check `pager.foo` before launching a builtin as a dashed\nexternal, i.e., if we recognize the name of the external as a builtin.\nChange the test to use `test_expect_success`.\n\nSigned-off-by: Martin Ågren \n---\nOne could address this in run_argv(), by making the second call to\nexecv_dashed_external() conditional on \"!is_builtin()\" whereas a builtin\nwould be started as \"git foo\". (Possibly after unrolling and cleaning up\nthe \"while (1)\"-loop.) That seems like the wrong fix for this particular\nissue, but might be a wanted change on its own -- or maybe not --, since\nit would mean one could relay, e.g., \"-c baz\" to \"git -c baz foo\" (but\nonly for builtins...).\n\n t/t7006-pager.sh | 2 +-\n git.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex df258c5d4..8b2ffb1aa 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -201,7 +201,7 @@ test_expect_success TTY 'git tag -a respects --paginate' '\n \ttest -e paginated.out\n '\n \n-test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '\n+test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '\n \t# git-tag will be launched as a dashed external, which\n \t# 1) is the source of a potential bug, and\n \t# 2) is why we use test_config and not -c.\ndiff --git a/git.c b/git.c\nindex 82ac2a092..6b6d9f68e 100644\n--- a/git.c\n+++ b/git.c\n@@ -559,7 +559,7 @@ static void execv_dashed_external(const char **argv)\n \tif (get_super_prefix())\n \t\tdie(\"%s doesn't support --super-prefix\", argv[0]);\n \n-\tif (use_pager == -1)\n+\tif (use_pager == -1 && !is_builtin(argv[0]))\n \t\tuse_pager = check_pager_config(argv[0]);\n \tcommit_pager_choice();\n \n-- \n2.14.0.rc0\n\n"},{"id":"324727","messageId":"xmqq1spdpni0.fsf@gitster.mtv.corp.google.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-18T19:13:11Z","receivedAt":"2017-07-18T19:13:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> After that feedback, v2 drops `pager.tag.list` and instead teaches\n> `git tag` to only consider `pager.tag` in list-mode, as suggested by\n> Peff.\n\nThat does sound like a more sensible and safer approach. I may have\ncomments on individual patches, which I will send while I read them.\n\nThanks.\n"},{"id":"324847","messageId":"xmqq379qkalj.fsf@gitster.mtv.corp.google.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-20T22:27:36Z","receivedAt":"2017-07-20T22:27:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> This is the second version of \"[PATCH 0/7] tag: more fine-grained\n> pager-configuration\" [1]. That series introduced `pager.tag.list` to\n> address the fact that `pager.tag` can be useful with `git tag -l` but\n> actively hostile with `git tag -a`. Thanks to Junio, Peff and Brandon\n> for helpful feedback.\n>\n> After that feedback, v2 drops `pager.tag.list` and instead teaches\n> `git tag` to only consider `pager.tag` in list-mode, as suggested by\n> Peff.\n>\n> Patches 1-3/10 replace patch 1/7. They move Documentation/technical/\n> api-builtin.txt into builtin.h, tweak the formatting and bring it up to\n> date. I may have gone overboard making this 3 patches...\n>\n> Patches 4-7/10 correspond to patches 2-5/7. `setup_auto_pager()' is now\n> much simpler since we do not need to handle \"tag.list\" with a clever\n> fallback strategy. IGNORE_PAGER_CONFIG is now called DELAY_PAGER_CONFIG.\n> I now check with pager_in_use() and I moved the handling of `pager.tag`\n> a bit further down.\n\nI tend to agree with you that 1-3/10 may be better off being a\nsingle patch (or 3/10 dropped, as Brandon is working on losing it\nnearby). I would have expected 7-8/10 to be a single patch, as by\nthe time a reader reaches 07/10, because of the groundwork laid by\n04-06/10, it is obvious that the general direction is to allow the\ncaller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in\nsome but not all circumstances, and 07/10 being faithful to the\noriginal behaviour (only to be updated in 08/10) is somewhat counter\nintuitive. It is not wrong per-se; it was just unexpected. \n\n> Patches 8-9/10 teach `git tag` to only respect `pager.tag` in list-mode\n> and flip the default value for that config to \"on\".\n>\n> Patch 10/10 is somewhat similar to a hunk in patch 2/7, but is now a\n> bug-fix instead of a feature. It teaches `execv_dashed_external()` not\n> to check `pager.foo` when launching `git-foo` where foo is a builtin.\n> I waffled about where to put this patch. Putting it earlier in the\n> series as a preparatory step, I couldn't come up with a way of writing a\n> test. So patch 8/10 introduces a `test_expect_failure` which this patch\n> then fixes.\n\nI haven't thought about ramifications of 9-10/10 to make a comment\nyet, but overall the series was a pleasant read.\n\nThanks.\n"},{"id":"324917","messageId":"CAN0heSokk9RAYx7pB_qC5dbZ3N1ZJQgKkrmhrPXmyHjFsnJu7Q@mail.gmail.com","threadId":"46350","inReplyTo":"xmqq379qkalj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-23T18:17:42Z","receivedAt":"2017-07-23T18:17:48Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 21 July 2017 at 00:27, Junio C Hamano wrote:\n> I tend to agree with you that 1-3/10 may be better off being a\n> single patch (or 3/10 dropped, as Brandon is working on losing it\n> nearby). I would have expected 7-8/10 to be a single patch, as by\n> the time a reader reaches 07/10, because of the groundwork laid by\n> 04-06/10, it is obvious that the general direction is to allow the\n> caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in\n> some but not all circumstances, and 07/10 being faithful to the\n> original behaviour (only to be updated in 08/10) is somewhat counter\n> intuitive. It is not wrong per-se; it was just unexpected.\n\nThanks for your comments. I will be away for a few days, but once I\nget back, I'll try to produce a v3 based on this and any further\nfeedback.\n\nMartin\n"},{"id":"325289","messageId":"20170731033429.dgftfeum6n3ddxua@sigill.intra.peff.net","threadId":"46350","inReplyTo":"b6ab825d3bda4b40201b0aa8e97c9fd721ed8171.1500321658.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 05/10] git.c: provide setup_auto_pager()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-31T03:34:29Z","receivedAt":"2017-07-31T03:34:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 17, 2017 at 10:10:47PM +0200, Martin Ågren wrote:\n\n> The previous patch introduced a way for builtins to declare that they\n> will take responsibility for handling the `pager.foo`-config item. (See\n> the commit message of that patch for why that could be useful.)\n> \n> Provide setup_auto_pager(), which builtins can call in order to handle\n> `pager.`, including possibly starting the pager. Make this function\n> don't do anything if a pager has already been started, as indicated by\n> use_pager or pager_in_use().\n> \n> Whenever this function is called from a builtin, git.c will already have\n> called commit_pager_choice(). Since commit_pager_choice() treats the\n> special value -1 as \"punt\" or \"not yet decided\", it is not a problem\n> that we might end up calling commit_pager_choice() once in git.c and\n> once (or more) in the builtin. Make the new function use -1 in the same\n> way and document it as \"punt\".\n\nAt first I wasn't sure if it would ever make sense to use \"-1\" here. The\n\"punt\" that happens in earlier calls to commit_pager_choice() is there\nbecause we might adjust our decision later. And this would generally be\nthe final decision, I would think. So I'd be surprised if we had\nanything besides \"0\" or \"1\" in the \"def\" argument.\n\nBut thinking on it, the most plausible case is something like:\n\n setup_auto_pager(\"foo\", -1);\n ...\n /* fallback to some historical compatibility name */\n setup_auto_pager(\"bar\", 0);\n\nAnd it's important for the \"-1\" there to be a true punt, and not do\nanything in commit_pager_choice(). So it's probably worth documenting\nthe \"-1\" behavior as you did here as a possible value for \"def\".\n\n-Peff\n"},{"id":"325290","messageId":"20170731033812.tvclfckx3ascxa2m@sigill.intra.peff.net","threadId":"46350","inReplyTo":"f4334503b677bbbeb19660e501ad2d7213428953.1500321658.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 06/10] t7006: add tests for how git tag paginates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-31T03:38:12Z","receivedAt":"2017-07-31T03:38:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 17, 2017 at 10:10:48PM +0200, Martin Ågren wrote:\n\n> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> index 20b4d83c2..e7430bc93 100755\n> --- a/t/t7006-pager.sh\n> +++ b/t/t7006-pager.sh\n> @@ -134,6 +134,74 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '\n> \t}\n> '\n> \n> +test_expect_success TTY 'git tag -l defaults to not paging' '\n> +\trm -f paginated.out &&\n> +\ttest_terminal git tag -l &&\n> +\t! test -e paginated.out\n> +'\n\nI don't mind an expect_success like this, where it documents a sane and\nexisting default, even if we're going to flip that default later in the\nseries.\n\nBut here...\n\n> +test_expect_success TTY 'git tag -a respects pager.tag' '\n> +\ttest_when_finished \"git tag -d newtag\" &&\n> +\trm -f paginated.out &&\n> +\ttest_terminal git -c pager.tag tag -am message newtag &&\n> +\ttest -e paginated.out\n> +'\n\nI think this behavior is just buggy, and it might be better introduced\nas a test_expect_failure on \"git tag -a does not respect pager.tag\".\n\nKind of a minor nit, as the series should end up in the right place\neither way, but it can be helpful if you end up digging back in history\nto the introduction of the test.\n\n-Peff\n"},{"id":"325291","messageId":"20170731034037.smkfrfd3nuka3zlc@sigill.intra.peff.net","threadId":"46350","inReplyTo":"ca309a7d46027c2804b82813600aaf96f65ca01b.1500321658.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 08/10] tag: respect `pager.tag` in list-mode only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-31T03:40:37Z","receivedAt":"2017-07-31T03:40:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 17, 2017 at 10:10:50PM +0200, Martin Ågren wrote:\n\n> test_expect_success TTY 'git tag -a respects --paginate' '\n> \ttest_when_finished \"git tag -d newtag\" &&\n> \trm -f paginated.out &&\n> -\ttest_terminal git -c pager.tag=false --paginate \\\n> -\t\ttag -am message newtag &&\n> +\ttest_terminal git --paginate tag -am message newtag &&\n> +\ttest -e paginated.out\n> +'\n\nThis changes, I guess, because pager.tag should not be having any impact\nat all. So it would not hurt to leave it, but the in the final state of\nthe test what is interesting is that \"--paginate\" kicks in. I do wonder\nif it could just drop the pager.tag bit when it is added in the first\nplace. We test elsewhere that the command-line overrides the config (for\na case that actually _does_ respect the config).\n\nI'm OK with it either way, though.\n\n-Peff\n"},{"id":"325292","messageId":"20170731034507.2fdjnvgzzt4rgvas@sigill.intra.peff.net","threadId":"46350","inReplyTo":"fa91f778e1acff812ddadb313e9979c470edc43b.1500321658.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-31T03:45:08Z","receivedAt":"2017-07-31T03:45:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:\n\n> When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and\n> execute `git-foo` as a dashed external. This is true even if git foo is\n> a builtin. That is on purpose, and is motivated in a comment which was\n> added in commit 441981bc (\"git: simplify environment save/restore\n> logic\", 2016-01-26).\n> \n> Shortly before we launch a dashed external, and unless we have already\n> found out whether we should use a pager, we check `pager.foo`. This was\n> added in commit 92058e4d (\"support pager.* for external commands\",\n> 2011-08-18). If the dashed external is a builtin, this does not match\n> that commit's intention and is arguably wrong, since it would be cleaner\n> if we let the \"dashed external builtin\" handle `pager.foo`.\n> \n> This has not mattered in practice, but a recent patch taught `git-tag`\n> to ignore `pager.tag` under certain circumstances. But, when started\n> using an alias, it doesn't get the chance to do so, as outlined above.\n> That recent patch added a test to document this breakage.\n> \n> Do not check `pager.foo` before launching a builtin as a dashed\n> external, i.e., if we recognize the name of the external as a builtin.\n> Change the test to use `test_expect_success`.\n\nI think floating this change to the end like this has made it much\neasier to see why we must do it. The end result looks good to me.\n\n> Signed-off-by: Martin Ågren \n> ---\n> One could address this in run_argv(), by making the second call to\n> execv_dashed_external() conditional on \"!is_builtin()\" whereas a builtin\n> would be started as \"git foo\". (Possibly after unrolling and cleaning up\n> the \"while (1)\"-loop.) That seems like the wrong fix for this particular\n> issue, but might be a wanted change on its own -- or maybe not --, since\n> it would mean one could relay, e.g., \"-c baz\" to \"git -c baz foo\" (but\n> only for builtins...).\n\nWe shouldn't need to relay them. They get added to the environment by\nthe initial \"git\" invocation, and then are available everywhere (in\nfact, it would be wrong to relay them for multi-valued config). Or did\nyou mean that we could potentially allow:\n\n [alias]\n foo = \"-c baz some-builtin\"\n\nThat's interesting, but I think the fact that it only works with\nbuiltins makes it a bad idea. And you can always do:\n\n [alias]\n foo = \"!git -c baz some-builtin\"\n\nwhich is equivalent.\n\n-Peff\n"},{"id":"325293","messageId":"20170731034632.r5m3ncgb5scqvltb@sigill.intra.peff.net","threadId":"46350","inReplyTo":"CAN0heSokk9RAYx7pB_qC5dbZ3N1ZJQgKkrmhrPXmyHjFsnJu7Q@mail.gmail.com","subject":"Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-31T03:46:32Z","receivedAt":"2017-07-31T03:46:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:\n\n> On 21 July 2017 at 00:27, Junio C Hamano wrote:\n> > I tend to agree with you that 1-3/10 may be better off being a\n> > single patch (or 3/10 dropped, as Brandon is working on losing it\n> > nearby). I would have expected 7-8/10 to be a single patch, as by\n> > the time a reader reaches 07/10, because of the groundwork laid by\n> > 04-06/10, it is obvious that the general direction is to allow the\n> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in\n> > some but not all circumstances, and 07/10 being faithful to the\n> > original behaviour (only to be updated in 08/10) is somewhat counter\n> > intuitive. It is not wrong per-se; it was just unexpected.\n> \n> Thanks for your comments. I will be away for a few days, but once I\n> get back, I'll try to produce a v3 based on this and any further\n> feedback.\n\nOverall it looks good to me. I left a few minor comments.\n\nI actually like the split. I found it pretty easy to follow (though\nsquashing as Junio suggested would be fine with me, too).\n\nThanks again for working on this.\n\n-Peff\n"},{"id":"325298","messageId":"xmqq7eyok1nw.fsf@gitster.mtv.corp.google.com","threadId":"46350","inReplyTo":"20170731033429.dgftfeum6n3ddxua@sigill.intra.peff.net","subject":"Re: [PATCH v2 05/10] git.c: provide setup_auto_pager()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-31T16:32:19Z","receivedAt":"2017-07-31T16:32:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But thinking on it, the most plausible case is something like:\n>\n> setup_auto_pager(\"foo\", -1);\n> ...\n> /* fallback to some historical compatibility name */\n> setup_auto_pager(\"bar\", 0);\n>\n> And it's important for the \"-1\" there to be a true punt, and not do\n> anything in commit_pager_choice(). So it's probably worth documenting\n> the \"-1\" behavior as you did here as a possible value for \"def\".\n\nThanks for reading it over. I agree that the \"punt\" behaviour is a\nsensible one in that use case, and I also agree that it would be\ngood to explain it.\n"},{"id":"325299","messageId":"xmqq379ck1fd.fsf@gitster.mtv.corp.google.com","threadId":"46350","inReplyTo":"20170731033812.tvclfckx3ascxa2m@sigill.intra.peff.net","subject":"Re: [PATCH v2 06/10] t7006: add tests for how git tag paginates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-31T16:37:26Z","receivedAt":"2017-07-31T16:37:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But here...\n>\n>> +test_expect_success TTY 'git tag -a respects pager.tag' '\n>> +\ttest_when_finished \"git tag -d newtag\" &&\n>> +\trm -f paginated.out &&\n>> +\ttest_terminal git -c pager.tag tag -am message newtag &&\n>> +\ttest -e paginated.out\n>> +'\n>\n> I think this behavior is just buggy, and it might be better introduced\n> as a test_expect_failure on \"git tag -a does not respect pager.tag\".\n>\n> Kind of a minor nit, as the series should end up in the right place\n> either way, but it can be helpful if you end up digging back in history\n> to the introduction of the test.\n\nYes, I think that is essentially the same reaction I had to patches\n7 and 8, where it carries the \"buggy\" behaviour forward and then\nfixes it. The way the series lays groundwork to introduce a\nmechanism that can be used to address this behaviour in its earlier\npatches strongly suggests to the users that this is considered a bug\nby the author of the series to the user from early on, so adding\nthis as \"expect failure\" and then flip it to \"expect success\" when\nthe bug is fixed would be a more natural sequence of changes.\n\nThanks.\n"},{"id":"325305","messageId":"20170731174509.w5hwujpsa4dt3jxv@sigill.intra.peff.net","threadId":"46350","inReplyTo":"CAN0heSonTXWPf-iTbwRbUc8CpYHoX7GBS5uCFFXn3bEPmWtitA@mail.gmail.com","subject":"Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-31T17:45:09Z","receivedAt":"2017-07-31T17:45:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 31, 2017 at 07:44:27PM +0200, Martin Ågren wrote:\n\n> On 31 July 2017 at 05:46, Jeff King wrote:\n> > On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:\n> >\n> >> On 21 July 2017 at 00:27, Junio C Hamano wrote:\n> >> > I tend to agree with you that 1-3/10 may be better off being a\n> >> > single patch (or 3/10 dropped, as Brandon is working on losing it\n> >> > nearby). I would have expected 7-8/10 to be a single patch, as by\n> >> > the time a reader reaches 07/10, because of the groundwork laid by\n> >> > 04-06/10, it is obvious that the general direction is to allow the\n> >> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in\n> >> > some but not all circumstances, and 07/10 being faithful to the\n> >> > original behaviour (only to be updated in 08/10) is somewhat counter\n> >> > intuitive. It is not wrong per-se; it was just unexpected.\n> >>\n> >> Thanks for your comments. I will be away for a few days, but once I\n> >> get back, I'll try to produce a v3 based on this and any further\n> >> feedback.\n> >\n> > Overall it looks good to me. I left a few minor comments.\n> >\n> > I actually like the split. I found it pretty easy to follow (though\n> > squashing as Junio suggested would be fine with me, too).\n> \n> I assume that by \"the split\" you mean patches 7-8, not 1-3. Anyway,\n> I'll squash since you're fine with it and Junio prefers it.\n\nI actually meant both, but as I said, I'm OK with it either way.\n\n-Peff\n"},{"id":"325306","messageId":"CAN0heSonTXWPf-iTbwRbUc8CpYHoX7GBS5uCFFXn3bEPmWtitA@mail.gmail.com","threadId":"46350","inReplyTo":"20170731034632.r5m3ncgb5scqvltb@sigill.intra.peff.net","subject":"Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-31T17:44:27Z","receivedAt":"2017-07-31T17:45:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 31 July 2017 at 05:46, Jeff King wrote:\n> On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:\n>\n>> On 21 July 2017 at 00:27, Junio C Hamano wrote:\n>> > I tend to agree with you that 1-3/10 may be better off being a\n>> > single patch (or 3/10 dropped, as Brandon is working on losing it\n>> > nearby). I would have expected 7-8/10 to be a single patch, as by\n>> > the time a reader reaches 07/10, because of the groundwork laid by\n>> > 04-06/10, it is obvious that the general direction is to allow the\n>> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in\n>> > some but not all circumstances, and 07/10 being faithful to the\n>> > original behaviour (only to be updated in 08/10) is somewhat counter\n>> > intuitive. It is not wrong per-se; it was just unexpected.\n>>\n>> Thanks for your comments. I will be away for a few days, but once I\n>> get back, I'll try to produce a v3 based on this and any further\n>> feedback.\n>\n> Overall it looks good to me. I left a few minor comments.\n>\n> I actually like the split. I found it pretty easy to follow (though\n> squashing as Junio suggested would be fine with me, too).\n\nI assume that by \"the split\" you mean patches 7-8, not 1-3. Anyway,\nI'll squash since you're fine with it and Junio prefers it.\n\nMartin\n"},{"id":"325307","messageId":"CAN0heSr0RFAuRhw9UQaewWWtHv7=EMXqHyMBOD7ARrpM=P1-Qw@mail.gmail.com","threadId":"46350","inReplyTo":"20170731034507.2fdjnvgzzt4rgvas@sigill.intra.peff.net","subject":"Re: [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-31T17:42:59Z","receivedAt":"2017-07-31T17:46:06Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 31 July 2017 at 05:45, Jeff King wrote:\n> On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:\n>\n>> One could address this in run_argv(), by making the second call to\n>> execv_dashed_external() conditional on \"!is_builtin()\" whereas a builtin\n>> would be started as \"git foo\". (Possibly after unrolling and cleaning up\n>> the \"while (1)\"-loop.) That seems like the wrong fix for this particular\n>> issue, but might be a wanted change on its own -- or maybe not --, since\n>> it would mean one could relay, e.g., \"-c baz\" to \"git -c baz foo\" (but\n>> only for builtins...).\n>\n> We shouldn't need to relay them. They get added to the environment by\n> the initial \"git\" invocation, and then are available everywhere (in\n> fact, it would be wrong to relay them for multi-valued config).\n\nThanks for explaining. I did some very sloppy reading of the comment\nin git.c that we \"cannot take flags in between the 'git' and the\n'xxxx'\" which I somehow misunderstood completely as \"we cannot pass\nthat sort of information to git-xxxx\". Silly. Thanks for taking the\ntime to explain what I should have found out myself...\n\nSo yeah, I meant the above and not this:\n\n> Or did\n> you mean that we could potentially allow:\n>\n> [alias]\n> foo = \"-c baz some-builtin\"\n>\n> That's interesting, but I think the fact that it only works with\n> builtins makes it a bad idea. And you can always do:\n>\n> [alias]\n> foo = \"!git -c baz some-builtin\"\n>\n> which is equivalent.\n"},{"id":"325308","messageId":"CAN0heSq2obte0Sp-oYMF+Q-VUZd6_d3BSnAaxwVNC20Fqh6daw@mail.gmail.com","threadId":"46350","inReplyTo":"xmqq379ck1fd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 06/10] t7006: add tests for how git tag paginates","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-07-31T17:50:44Z","receivedAt":"2017-07-31T17:50:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 31 July 2017 at 18:37, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> But here...\n>>\n>>> +test_expect_success TTY 'git tag -a respects pager.tag' '\n>>> + test_when_finished \"git tag -d newtag\" &&\n>>> + rm -f paginated.out &&\n>>> + test_terminal git -c pager.tag tag -am message newtag &&\n>>> + test -e paginated.out\n>>> +'\n>>\n>> I think this behavior is just buggy, and it might be better introduced\n>> as a test_expect_failure on \"git tag -a does not respect pager.tag\".\n>>\n>> Kind of a minor nit, as the series should end up in the right place\n>> either way, but it can be helpful if you end up digging back in history\n>> to the introduction of the test.\n>\n> Yes, I think that is essentially the same reaction I had to patches\n> 7 and 8, where it carries the \"buggy\" behaviour forward and then\n> fixes it. The way the series lays groundwork to introduce a\n> mechanism that can be used to address this behaviour in its earlier\n> patches strongly suggests to the users that this is considered a bug\n> by the author of the series to the user from early on, so adding\n> this as \"expect failure\" and then flip it to \"expect success\" when\n> the bug is fixed would be a more natural sequence of changes.\n\nThanks both for very helpful comments. I admit I viewed it less as\n\"fix buggy behavior\" and more like \"redefine wanted behavior\". So I\nwanted to postpone the redefinition of the behavior until all the\nrestructuring was done. Looking at this as a bug-fix does make\ncarefully moving the bug forward look rather silly.\n\nI haven't responded to each of your suggestions individually where the\nanswers would have been a mere \"thanks, will do\". They're still much\nappreciated and will help make v3 much better. Thanks.\n\nMartin\n"},{"id":"325312","messageId":"xmqqd18giczj.fsf@gitster.mtv.corp.google.com","threadId":"46350","inReplyTo":"20170731174509.w5hwujpsa4dt3jxv@sigill.intra.peff.net","subject":"Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-31T20:10:40Z","receivedAt":"2017-07-31T20:10:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 31, 2017 at 07:44:27PM +0200, Martin Ågren wrote:\n>\n>> On 31 July 2017 at 05:46, Jeff King wrote:\n>> > On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:\n>> >\n>> >> On 21 July 2017 at 00:27, Junio C Hamano wrote:\n>> >> > I tend to agree with you that 1-3/10 may be better off being a\n>> >> > single patch (or 3/10 dropped, as Brandon is working on losing it\n>> >> > nearby). I would have expected 7-8/10 to be a single patch, as by\n>> >> > the time a reader reaches 07/10, because of the groundwork laid by\n>> >> > 04-06/10, it is obvious that the general direction is to allow the\n>> >> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in\n>> >> > some but not all circumstances, and 07/10 being faithful to the\n>> >> > original behaviour (only to be updated in 08/10) is somewhat counter\n>> >> > intuitive. It is not wrong per-se; it was just unexpected.\n>> >>\n>> >> Thanks for your comments. I will be away for a few days, but once I\n>> >> get back, I'll try to produce a v3 based on this and any further\n>> >> feedback.\n>> >\n>> > Overall it looks good to me. I left a few minor comments.\n>> >\n>> > I actually like the split. I found it pretty easy to follow (though\n>> > squashing as Junio suggested would be fine with me, too).\n>> \n>> I assume that by \"the split\" you mean patches 7-8, not 1-3. Anyway,\n>> I'll squash since you're fine with it and Junio prefers it.\n>\n> I actually meant both, but as I said, I'm OK with it either way.\n\nI am OK with it either way, too ;-)\n"},{"id":"325454","messageId":"cover.1501701128.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1500321657.git.martin.agren@gmail.com","subject":"[PATCH v3 0/7] tag: only respect `pager.tag` in list-mode","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-02T19:40:48Z","receivedAt":"2017-08-02T19:41:13Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is the third version of my attempt to make `pager tag` useful (v1\nat [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.\n\nI've squashed patches 01-03/10 and 07-08/10, respectively. The interdiff\nis below. I managed to clean up some tests thanks to a drive-by comment\nby Peff which cleared up a misunderstanding I had. Some minor changes,\ne.g., I write \"built-in\" instead of \"builtin\", since that seemed\npreferred, at least locally in builtin.h. I documented why a default\nvalue of \"punt\" could be useful, but also that it's probably not wanted.\n\nMartin\n\n[1] https://public-inbox.org/git/cover.1499723297.git.martin.agren@gmail.com/T/\n\n[2] https://public-inbox.org/git/cover.1500321657.git.martin.agren@gmail.com/T/#u\n\nMartin Ågren (7):\n builtin.h: take over documentation from api-builtin.txt\n git.c: let builtins opt for handling `pager.foo` themselves\n git.c: provide setup_auto_pager()\n t7006: add tests for how git tag paginates\n tag: respect `pager.tag` in list-mode only\n tag: change default of `pager.tag` to \"on\"\n git.c: ignore pager.* when launching builtin as dashed external\n\n Documentation/git-tag.txt | 3 +\n Documentation/technical/api-builtin.txt | 73 -----------------------\n t/t7006-pager.sh | 80 +++++++++++++++++++++++++\n builtin.h | 100 ++++++++++++++++++++++++++++++++\n builtin/tag.c | 3 +\n git.c | 18 +++++-\n 6 files changed, 201 insertions(+), 76 deletions(-)\n delete mode 100644 Documentation/technical/api-builtin.txt\n\nInterdiff vs v2:\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 8b2ffb1aa..9128ec5ac 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '\n test_expect_success TTY 'git tag with no args respects pager.tag' '\n \t# no args implies -l so this should page like -l\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag=no tag &&\n+\ttest_terminal git -c pager.tag=false tag &&\n \t! test -e paginated.out\n '\n \n@@ -202,20 +202,15 @@ test_expect_success TTY 'git tag -a respects --paginate' '\n '\n \n test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '\n-\t# git-tag will be launched as a dashed external, which\n-\t# 1) is the source of a potential bug, and\n-\t# 2) is why we use test_config and not -c.\n \ttest_when_finished \"git tag -d newtag\" &&\n \trm -f paginated.out &&\n-\ttest_config pager.tag true &&\n-\ttest_terminal git -c alias.t=tag t -am message newtag &&\n+\ttest_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&\n \t! test -e paginated.out\n '\n \n test_expect_success TTY 'git tag as alias respects pager.tag with -l' '\n \trm -f paginated.out &&\n-\ttest_config pager.tag false &&\n-\ttest_terminal git -c alias.t=tag t -l &&\n+\ttest_terminal git -c pager.tag=false -c alias.t=tag t -l &&\n \t! test -e paginated.out\n '\n \ndiff --git a/builtin.h b/builtin.h\nindex 3ca4a53a8..42378f3aa 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -51,15 +51,15 @@\n *\ton bare repositories.\n *\tThis only makes sense when `RUN_SETUP` is also set.\n *\n- * `SUPPORT_SUPER_PREFIX`::\n+ * `SUPPORT_SUPER_PREFIX`:\n *\n- *\tThe builtin supports `--super-prefix`.\n+ *\tThe built-in supports `--super-prefix`.\n *\n- * `DELAY_PAGER_CONFIG`::\n+ * `DELAY_PAGER_CONFIG`:\n *\n *\tIf RUN_SETUP or RUN_SETUP_GENTLY is set, git.c normally handles\n *\tthe `pager.`-configuration. If this flag is used, git.c\n- *\twill skip that step, instead allowing the builtin to make a\n+ *\twill skip that step, instead allowing the built-in to make a\n *\tmore informed decision, e.g., by ignoring `pager.` for\n *\tcertain subcommands.\n *\n@@ -114,11 +114,14 @@ extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \t\t\t struct fmt_merge_msg_opts *);\n \n /**\n- * If a builtin has DELAY_PAGER_CONFIG set, the builtin should call this early\n+ * If a built-in has DELAY_PAGER_CONFIG set, the built-in should call this early\n * when it wishes to respect the `pager.foo`-config. The `cmd` is the name of\n- * the builtin, e.g., \"foo\". If a paging-choice has already been setup, this\n+ * the built-in, e.g., \"foo\". If a paging-choice has already been setup, this\n * does nothing. The default in `def` should be 0 for \"pager off\", 1 for \"pager\n * on\" or -1 for \"punt\".\n+ *\n+ * You should most likely use a default of 0 or 1. \"Punt\" (-1) could be useful\n+ * to be able to fall back to some historical compatibility name.\n */\n extern void setup_auto_pager(const char *cmd, int def);\n \n-- \n2.14.0.rc1.12.ge2d9c4613\n\n"},{"id":"325455","messageId":"e660a9cdaff6d114305a475f9a12876b56b473d1.1501701128.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1501701128.git.martin.agren@gmail.com","subject":"[PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-02T19:40:49Z","receivedAt":"2017-08-02T19:41:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Delete Documentation/technical/api-builtin.txt and move its content\ninto builtin.h. Format it as a comment. Remove a '+' which was needed\nwhen the information was formatted for AsciiDoc. Similarly, change\n\"::\" to \":\".\n\nDocument SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to\ndate with the available flags.\n\nWhile at it, correct '3 more things to do' to '4 more things to do'.\n\nSigned-off-by: Martin Ågren \n---\nI'm still documenting SUPPORT_SUPER_PREFIX. As Junio pointed out, a\nnearby patch series is working on losing it (or one user of it). I tried\nremoving that part, but felt like I was leaving things incomplete.\n\n Documentation/technical/api-builtin.txt | 73 ------------------------------\n builtin.h | 80 +++++++++++++++++++++++++++++++++\n 2 files changed, 80 insertions(+), 73 deletions(-)\n delete mode 100644 Documentation/technical/api-builtin.txt\n\ndiff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt\ndeleted file mode 100644\nindex 22a39b929..000000000\n--- a/Documentation/technical/api-builtin.txt\n+++ /dev/null\n@@ -1,73 +0,0 @@\n-builtin API\n-===========\n-\n-Adding a new built-in\n----------------------\n-\n-There are 4 things to do to add a built-in command implementation to\n-Git:\n-\n-. Define the implementation of the built-in command `foo` with\n- signature:\n-\n-\tint cmd_foo(int argc, const char **argv, const char *prefix);\n-\n-. Add the external declaration for the function to `builtin.h`.\n-\n-. Add the command to the `commands[]` table defined in `git.c`.\n- The entry should look like:\n-\n-\t{ \"foo\", cmd_foo, },\n-+\n-where options is the bitwise-or of:\n-\n-`RUN_SETUP`::\n-\tIf there is not a Git directory to work on, abort. If there\n-\tis a work tree, chdir to the top of it if the command was\n-\tinvoked in a subdirectory. If there is no work tree, no\n-\tchdir() is done.\n-\n-`RUN_SETUP_GENTLY`::\n-\tIf there is a Git directory, chdir as per RUN_SETUP, otherwise,\n-\tdon't chdir anywhere.\n-\n-`USE_PAGER`::\n-\n-\tIf the standard output is connected to a tty, spawn a pager and\n-\tfeed our output to it.\n-\n-`NEED_WORK_TREE`::\n-\n-\tMake sure there is a work tree, i.e. the command cannot act\n-\ton bare repositories.\n-\tThis only makes sense when `RUN_SETUP` is also set.\n-\n-. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n-\n-Additionally, if `foo` is a new command, there are 3 more things to do:\n-\n-. Add tests to `t/` directory.\n-\n-. Write documentation in `Documentation/git-foo.txt`.\n-\n-. Add an entry for `git-foo` to `command-list.txt`.\n-\n-. Add an entry for `/git-foo` to `.gitignore`.\n-\n-\n-How a built-in is called\n-------------------------\n-\n-The implementation `cmd_foo()` takes three parameters, `argc`, `argv,\n-and `prefix`. The first two are similar to what `main()` of a\n-standalone command would be called with.\n-\n-When `RUN_SETUP` is specified in the `commands[]` table, and when you\n-were started from a subdirectory of the work tree, `cmd_foo()` is called\n-after chdir(2) to the top of the work tree, and `prefix` gets the path\n-to the subdirectory the command started from. This allows you to\n-convert a user-supplied pathname (typically relative to that directory)\n-to a pathname relative to the top of the work tree.\n-\n-The return value from `cmd_foo()` becomes the exit status of the\n-command.\ndiff --git a/builtin.h b/builtin.h\nindex 498ac80d0..8d87d06da 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -6,6 +6,86 @@\n #include \"cache.h\"\n #include \"commit.h\"\n \n+/*\n+ * builtin API\n+ * ===========\n+ *\n+ * Adding a new built-in\n+ * ---------------------\n+ *\n+ * There are 4 things to do to add a built-in command implementation to\n+ * Git:\n+ *\n+ * . Define the implementation of the built-in command `foo` with\n+ * signature:\n+ *\n+ *\tint cmd_foo(int argc, const char **argv, const char *prefix);\n+ *\n+ * . Add the external declaration for the function to `builtin.h`.\n+ *\n+ * . Add the command to the `commands[]` table defined in `git.c`.\n+ * The entry should look like:\n+ *\n+ *\t{ \"foo\", cmd_foo, },\n+ *\n+ * where options is the bitwise-or of:\n+ *\n+ * `RUN_SETUP`:\n+ *\tIf there is not a Git directory to work on, abort. If there\n+ *\tis a work tree, chdir to the top of it if the command was\n+ *\tinvoked in a subdirectory. If there is no work tree, no\n+ *\tchdir() is done.\n+ *\n+ * `RUN_SETUP_GENTLY`:\n+ *\tIf there is a Git directory, chdir as per RUN_SETUP, otherwise,\n+ *\tdon't chdir anywhere.\n+ *\n+ * `USE_PAGER`:\n+ *\n+ *\tIf the standard output is connected to a tty, spawn a pager and\n+ *\tfeed our output to it.\n+ *\n+ * `NEED_WORK_TREE`:\n+ *\n+ *\tMake sure there is a work tree, i.e. the command cannot act\n+ *\ton bare repositories.\n+ *\tThis only makes sense when `RUN_SETUP` is also set.\n+ *\n+ * `SUPPORT_SUPER_PREFIX`:\n+ *\n+ *\tThe built-in supports `--super-prefix`.\n+ *\n+ * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n+ *\n+ * Additionally, if `foo` is a new command, there are 4 more things to do:\n+ *\n+ * . Add tests to `t/` directory.\n+ *\n+ * . Write documentation in `Documentation/git-foo.txt`.\n+ *\n+ * . Add an entry for `git-foo` to `command-list.txt`.\n+ *\n+ * . Add an entry for `/git-foo` to `.gitignore`.\n+ *\n+ *\n+ * How a built-in is called\n+ * ------------------------\n+ *\n+ * The implementation `cmd_foo()` takes three parameters, `argc`, `argv,\n+ * and `prefix`. The first two are similar to what `main()` of a\n+ * standalone command would be called with.\n+ *\n+ * When `RUN_SETUP` is specified in the `commands[]` table, and when you\n+ * were started from a subdirectory of the work tree, `cmd_foo()` is called\n+ * after chdir(2) to the top of the work tree, and `prefix` gets the path\n+ * to the subdirectory the command started from. This allows you to\n+ * convert a user-supplied pathname (typically relative to that directory)\n+ * to a pathname relative to the top of the work tree.\n+ *\n+ * The return value from `cmd_foo()` becomes the exit status of the\n+ * command.\n+ */\n+\n #define DEFAULT_MERGE_LOG_LEN 20\n \n extern const char git_usage_string[];\n-- \n2.14.0.rc1.12.ge2d9c4613\n\n"},{"id":"325456","messageId":"721f7b86825ade920b68bd5659cdde19111014ce.1501701128.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1501701128.git.martin.agren@gmail.com","subject":"[PATCH v3 2/7] git.c: let builtins opt for handling `pager.foo` themselves","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-02T19:40:50Z","receivedAt":"2017-08-02T19:41:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Before launching a builtin git foo and unless mechanisms with precedence\nare in use, we check for and handle the `pager.foo` config. This is done\nwithout considering exactly how git foo is being used, and indeed, git.c\ncannot (and should not) know what the arguments to git foo are supposed\nto achieve.\n\nIn practice this means that, e.g., `git -c pager.tag tag -a new-tag`\nresults in errors such as \"Vim: Warning: Output is not to a terminal\"\nand a garbled terminal. Someone who makes use of both `git tag -a` and\n`git tag -l` will probably not set `pager.tag`, so that `git tag -a`\nwill actually work, at the cost of not paging output of `git tag -l`.\n\nTo allow individual builtins to make more informed decisions about when\nto respect `pager.foo`, introduce a flag DELAY_PAGER_CONFIG. If the flag\nis set, do not check `pager.foo`.\n\nDo not check for DELAY_PAGER_CONFIG in `execv_dashed_external()`. That\ncall site is arguably wrong, although in a way that is not yet visible,\nand will be changed in a slightly different direction in a later patch.\n\nDon't add any users of DELAY_PAGER_CONFIG just yet, one will follow in a\nlater patch.\n\nSuggested-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n builtin.h | 8 ++++++++\n git.c | 4 +++-\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin.h b/builtin.h\nindex 8d87d06da..0f3a7b770 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -55,6 +55,14 @@\n *\n *\tThe built-in supports `--super-prefix`.\n *\n+ * `DELAY_PAGER_CONFIG`:\n+ *\n+ *\tIf RUN_SETUP or RUN_SETUP_GENTLY is set, git.c normally handles\n+ *\tthe `pager.`-configuration. If this flag is used, git.c\n+ *\twill skip that step, instead allowing the built-in to make a\n+ *\tmore informed decision, e.g., by ignoring `pager.` for\n+ *\tcertain subcommands.\n+ *\n * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n *\n * Additionally, if `foo` is a new command, there are 4 more things to do:\ndiff --git a/git.c b/git.c\nindex 489aab4d8..79195ebbd 100644\n--- a/git.c\n+++ b/git.c\n@@ -283,6 +283,7 @@ static int handle_alias(int *argcp, const char ***argv)\n */\n #define NEED_WORK_TREE\t\t(1<<3)\n #define SUPPORT_SUPER_PREFIX\t(1<<4)\n+#define DELAY_PAGER_CONFIG\t(1<<5)\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -306,7 +307,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\t\tprefix = setup_git_directory_gently(&nongit_ok);\n \t\t}\n \n-\t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))\n+\t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n+\t\t !(p->option & DELAY_PAGER_CONFIG))\n \t\t\tuse_pager = check_pager_config(p->cmd);\n \t\tif (use_pager == -1 && p->option & USE_PAGER)\n \t\t\tuse_pager = 1;\n-- \n2.14.0.rc1.12.ge2d9c4613\n\n"},{"id":"325457","messageId":"82a3ffd035898992ae328fdb99a04be6ea48eb51.1501701128.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1501701128.git.martin.agren@gmail.com","subject":"[PATCH v3 3/7] git.c: provide setup_auto_pager()","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-02T19:40:51Z","receivedAt":"2017-08-02T19:41:24Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The previous patch introduced a way for builtins to declare that they\nwill take responsibility for handling the `pager.foo`-config item. (See\nthe commit message of that patch for why that could be useful.)\n\nProvide setup_auto_pager(), which builtins can call in order to handle\n`pager.`, including possibly starting the pager. Make this function\ndon't do anything if a pager has already been started, as indicated by\nuse_pager or pager_in_use().\n\nWhenever this function is called from a builtin, git.c will already have\ncalled commit_pager_choice(). Since commit_pager_choice() treats the\nspecial value -1 as \"punt\" or \"not yet decided\", it is not a problem\nthat we might end up calling commit_pager_choice() once in git.c and\nonce (or more) in the builtin. Make the new function use -1 in the same\nway and document it as \"punt\".\n\nDon't add any users of setup_auto_pager just yet, one will follow in\na later patch.\n\nSuggested-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n builtin.h | 12 ++++++++++++\n git.c | 10 ++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/builtin.h b/builtin.h\nindex 0f3a7b770..42378f3aa 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -113,6 +113,18 @@ struct fmt_merge_msg_opts {\n extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \t\t\t struct fmt_merge_msg_opts *);\n \n+/**\n+ * If a built-in has DELAY_PAGER_CONFIG set, the built-in should call this early\n+ * when it wishes to respect the `pager.foo`-config. The `cmd` is the name of\n+ * the built-in, e.g., \"foo\". If a paging-choice has already been setup, this\n+ * does nothing. The default in `def` should be 0 for \"pager off\", 1 for \"pager\n+ * on\" or -1 for \"punt\".\n+ *\n+ * You should most likely use a default of 0 or 1. \"Punt\" (-1) could be useful\n+ * to be able to fall back to some historical compatibility name.\n+ */\n+extern void setup_auto_pager(const char *cmd, int def);\n+\n extern int is_builtin(const char *s);\n \n extern int cmd_add(int argc, const char **argv, const char *prefix);\ndiff --git a/git.c b/git.c\nindex 79195ebbd..66832f232 100644\n--- a/git.c\n+++ b/git.c\n@@ -33,6 +33,16 @@ static void commit_pager_choice(void) {\n \t}\n }\n \n+void setup_auto_pager(const char *cmd, int def)\n+{\n+\tif (use_pager != -1 || pager_in_use())\n+\t\treturn;\n+\tuse_pager = check_pager_config(cmd);\n+\tif (use_pager == -1)\n+\t\tuse_pager = def;\n+\tcommit_pager_choice();\n+}\n+\n static int handle_options(const char ***argv, int *argc, int *envchanged)\n {\n \tconst char **orig_argv = *argv;\n-- \n2.14.0.rc1.12.ge2d9c4613\n\n"},{"id":"325458","messageId":"b55ef829528e744e05510bb3671296d805d1e1d8.1501701128.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1501701128.git.martin.agren@gmail.com","subject":"[PATCH v3 4/7] t7006: add tests for how git tag paginates","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-02T19:40:52Z","receivedAt":"2017-08-02T19:41:27Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as\n\"Vim: Warning: Output is not to a terminal\" and a garbled terminal.\nSomeone who makes use of both `git tag -a` and `git tag -l` will\nprobably not set `pager.tag`, so that `git tag -a` will actually work,\nat the cost of not paging output of `git tag -l`.\n\nSince we're about to change how `git tag` respects `pager.tag`, add tests\naround this, including how the configuration is ignored if --no-pager or\n--paginate are used.\n\nConstruct tests with a few different subcommands. First, use -l. Second,\nuse \"no arguments\" and --contains, since those imply -l. (There are\nmore arguments which imply -l, but using these two should be enough.)\n\nThird, use -a as a representative for \"not -l\". Actually, the tests use\n`git tag -am` so no editor is launched, but that is irrelevant, since we\njust want to see whether the pager is used or not. Make one of the tests\ndemonstrate the broken behavior mentioned above, where `git tag -a`\nrespects `pager.tag`.\n\nSigned-off-by: Martin Ågren \n---\nIn one place, I now use test_expect_failure (thanks Peff).\n\n t/t7006-pager.sh | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 67 insertions(+)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 20b4d83c2..b56d4cdd4 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -134,6 +134,73 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '\n \t}\n '\n \n+test_expect_success TTY 'git tag -l defaults to not paging' '\n+\trm -f paginated.out &&\n+\ttest_terminal git tag -l &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -l respects pager.tag' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag tag -l &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -l respects --no-pager' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag --no-pager tag -l &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag with no args defaults to not paging' '\n+\t# no args implies -l so this should page like -l\n+\trm -f paginated.out &&\n+\ttest_terminal git tag &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag with no args respects pager.tag' '\n+\t# no args implies -l so this should page like -l\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag tag &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag --contains defaults to not paging' '\n+\t# --contains implies -l so this should page like -l\n+\trm -f paginated.out &&\n+\ttest_terminal git tag --contains &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag --contains respects pager.tag' '\n+\t# --contains implies -l so this should page like -l\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag tag --contains &&\n+\ttest -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -a defaults to not paging' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git tag -am message newtag &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_failure TTY 'git tag -a ignores pager.tag' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag tag -am message newtag &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag -a respects --paginate' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git --paginate tag -am message newtag &&\n+\ttest -e paginated.out\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.14.0.rc1.12.ge2d9c4613\n\n"},{"id":"325459","messageId":"c6763c7bc1c02be905c54ed64096ab494cf060c6.1501701128.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1501701128.git.martin.agren@gmail.com","subject":"[PATCH v3 5/7] tag: respect `pager.tag` in list-mode only","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-02T19:40:53Z","receivedAt":"2017-08-02T19:41:29Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as\n\"Vim: Warning: Output is not to a terminal\" and a garbled terminal.\nSomeone who makes use of both `git tag -a` and `git tag -l` will\nprobably not set `pager.tag`, so that `git tag -a` will actually work,\nat the cost of not paging output of `git tag -l`.\n\nUse the mechanisms introduced in two earlier patches to ignore\n`pager.tag` in git.c and let the `git tag` builtin handle it on its own.\nOnly respect `pager.tag` when running in list-mode.\n\nThere is a window between where the pager is started before and after\nthis patch. This means that early errors can behave slightly different\nbefore and after this patch. Since operation-parsing has to happen\ninside this window, this can be seen with `git -c pager.tag=\"echo pager\nis used\" tag -l --unknown-option`. This change in paging-behavior should\nbe acceptable since it only affects erroneous usages.\n\nUpdate the documentation and update tests.\n\nIf an alias is used to run `git tag -a`, then `pager.tag` will still be\nrespected. Document this known breakage. It will be fixed in a later\ncommit. Add a similar test for `-l`, which works.\n\nNoticed-by: Anatoly Borodin \nSuggested-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n Documentation/git-tag.txt | 3 +++\n t/t7006-pager.sh | 15 ++++++++++++++-\n builtin/tag.c | 3 +++\n git.c | 2 +-\n 4 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 1eb15afa1..875d135e0 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -205,6 +205,9 @@ it in the repository configuration as follows:\n signingKey = \n -------------------------------------\n \n+`pager.tag` is only respected when listing tags, i.e., when `-l` is\n+used or implied.\n+See linkgit:git-config[1].\n \n DISCUSSION\n ----------\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex b56d4cdd4..570b2f252 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -187,7 +187,7 @@ test_expect_success TTY 'git tag -a defaults to not paging' '\n \t! test -e paginated.out\n '\n \n-test_expect_failure TTY 'git tag -a ignores pager.tag' '\n+test_expect_success TTY 'git tag -a ignores pager.tag' '\n \ttest_when_finished \"git tag -d newtag\" &&\n \trm -f paginated.out &&\n \ttest_terminal git -c pager.tag tag -am message newtag &&\n@@ -201,6 +201,19 @@ test_expect_success TTY 'git tag -a respects --paginate' '\n \ttest -e paginated.out\n '\n \n+test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '\n+\ttest_when_finished \"git tag -d newtag\" &&\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&\n+\t! test -e paginated.out\n+'\n+\n+test_expect_success TTY 'git tag as alias respects pager.tag with -l' '\n+\trm -f paginated.out &&\n+\ttest_terminal git -c pager.tag -c alias.t=tag t -l &&\n+\ttest -e paginated.out\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() {\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 01154ea8d..5ad1af252 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -461,6 +461,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcmdmode = 'l';\n \t}\n \n+\tif (cmdmode == 'l')\n+\t\tsetup_auto_pager(\"tag\", 0);\n+\n \tif ((create_tag_object || force) && (cmdmode != 0))\n \t\tusage_with_options(git_tag_usage, options);\n \ndiff --git a/git.c b/git.c\nindex 66832f232..82ac2a092 100644\n--- a/git.c\n+++ b/git.c\n@@ -466,7 +466,7 @@ static struct cmd_struct commands[] = {\n \t{ \"stripspace\", cmd_stripspace },\n \t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n-\t{ \"tag\", cmd_tag, RUN_SETUP },\n+\t{ \"tag\", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },\n \t{ \"unpack-file\", cmd_unpack_file, RUN_SETUP },\n \t{ \"unpack-objects\", cmd_unpack_objects, RUN_SETUP },\n \t{ \"update-index\", cmd_update_index, RUN_SETUP },\n-- \n2.14.0.rc1.12.ge2d9c4613\n\n"},{"id":"325460","messageId":"8225436bd8c8eb4af3370867596789ae75729f8d.1501701129.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1501701128.git.martin.agren@gmail.com","subject":"[PATCH v3 6/7] tag: change default of `pager.tag` to \"on\"","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-02T19:40:54Z","receivedAt":"2017-08-02T19:41:34Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The previous patch taught `git tag` to only respect `pager.tag` in\nlist-mode. That patch left the default value of `pager.tag` at \"off\".\n\nAfter that patch, it makes sense to let the default value be \"on\"\ninstead, since it will help with listing many tags, but will not hurt\nusers of `git tag -a` as it would have before. Make that change. Update\ndocumentation and tests.\n\nSigned-off-by: Martin Ågren \n---\n Documentation/git-tag.txt | 2 +-\n t/t7006-pager.sh | 28 ++++++++++++++--------------\n builtin/tag.c | 2 +-\n 3 files changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 875d135e0..d97aad343 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -206,7 +206,7 @@ it in the repository configuration as follows:\n -------------------------------------\n \n `pager.tag` is only respected when listing tags, i.e., when `-l` is\n-used or implied.\n+used or implied. The default is to use a pager.\n See linkgit:git-config[1].\n \n DISCUSSION\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 570b2f252..afa03f3b6 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -134,16 +134,16 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '\n \t}\n '\n \n-test_expect_success TTY 'git tag -l defaults to not paging' '\n+test_expect_success TTY 'git tag -l defaults to paging' '\n \trm -f paginated.out &&\n \ttest_terminal git tag -l &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n test_expect_success TTY 'git tag -l respects pager.tag' '\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag tag -l &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.tag=false tag -l &&\n+\t! test -e paginated.out\n '\n \n test_expect_success TTY 'git tag -l respects --no-pager' '\n@@ -152,32 +152,32 @@ test_expect_success TTY 'git tag -l respects --no-pager' '\n \t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git tag with no args defaults to not paging' '\n+test_expect_success TTY 'git tag with no args defaults to paging' '\n \t# no args implies -l so this should page like -l\n \trm -f paginated.out &&\n \ttest_terminal git tag &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n test_expect_success TTY 'git tag with no args respects pager.tag' '\n \t# no args implies -l so this should page like -l\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag tag &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.tag=false tag &&\n+\t! test -e paginated.out\n '\n \n-test_expect_success TTY 'git tag --contains defaults to not paging' '\n+test_expect_success TTY 'git tag --contains defaults to paging' '\n \t# --contains implies -l so this should page like -l\n \trm -f paginated.out &&\n \ttest_terminal git tag --contains &&\n-\t! test -e paginated.out\n+\ttest -e paginated.out\n '\n \n test_expect_success TTY 'git tag --contains respects pager.tag' '\n \t# --contains implies -l so this should page like -l\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag tag --contains &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.tag=false tag --contains &&\n+\t! test -e paginated.out\n '\n \n test_expect_success TTY 'git tag -a defaults to not paging' '\n@@ -210,8 +210,8 @@ test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '\n \n test_expect_success TTY 'git tag as alias respects pager.tag with -l' '\n \trm -f paginated.out &&\n-\ttest_terminal git -c pager.tag -c alias.t=tag t -l &&\n-\ttest -e paginated.out\n+\ttest_terminal git -c pager.tag=false -c alias.t=tag t -l &&\n+\t! test -e paginated.out\n '\n \n # A colored commit log will begin with an appropriate ANSI escape\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5ad1af252..ea83df5e1 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -462,7 +462,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (cmdmode == 'l')\n-\t\tsetup_auto_pager(\"tag\", 0);\n+\t\tsetup_auto_pager(\"tag\", 1);\n \n \tif ((create_tag_object || force) && (cmdmode != 0))\n \t\tusage_with_options(git_tag_usage, options);\n-- \n2.14.0.rc1.12.ge2d9c4613\n\n"},{"id":"325461","messageId":"68240873fe154bbeda0bf3c4d324f46b6bb800a3.1501701129.git.martin.agren@gmail.com","threadId":"46350","inReplyTo":"cover.1501701128.git.martin.agren@gmail.com","subject":"[PATCH v3 7/7] git.c: ignore pager.* when launching builtin as dashed external","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-02T19:40:55Z","receivedAt":"2017-08-02T19:41:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and\nexecute `git-foo` as a dashed external. This is true even if git foo is\na builtin. That is on purpose, and is motivated in a comment which was\nadded in commit 441981bc (\"git: simplify environment save/restore\nlogic\", 2016-01-26).\n\nShortly before we launch a dashed external, and unless we have already\nfound out whether we should use a pager, we check `pager.foo`. This was\nadded in commit 92058e4d (\"support pager.* for external commands\",\n2011-08-18). If the dashed external is a builtin, this does not match\nthat commit's intention and is arguably wrong, since it would be cleaner\nif we let the \"dashed external builtin\" handle `pager.foo`.\n\nThis has not mattered in practice, but a recent patch taught `git-tag`\nto ignore `pager.tag` under certain circumstances. But, when started\nusing an alias, it doesn't get the chance to do so, as outlined above.\nThat recent patch added a test to document this breakage.\n\nDo not check `pager.foo` before launching a builtin as a dashed\nexternal, i.e., if we recognize the name of the external as a builtin.\nChange the test to use `test_expect_success`.\n\nSigned-off-by: Martin Ågren \n---\n t/t7006-pager.sh | 2 +-\n git.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex afa03f3b6..9128ec5ac 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -201,7 +201,7 @@ test_expect_success TTY 'git tag -a respects --paginate' '\n \ttest -e paginated.out\n '\n \n-test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '\n+test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '\n \ttest_when_finished \"git tag -d newtag\" &&\n \trm -f paginated.out &&\n \ttest_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&\ndiff --git a/git.c b/git.c\nindex 82ac2a092..6b6d9f68e 100644\n--- a/git.c\n+++ b/git.c\n@@ -559,7 +559,7 @@ static void execv_dashed_external(const char **argv)\n \tif (get_super_prefix())\n \t\tdie(\"%s doesn't support --super-prefix\", argv[0]);\n \n-\tif (use_pager == -1)\n+\tif (use_pager == -1 && !is_builtin(argv[0]))\n \t\tuse_pager = check_pager_config(argv[0]);\n \tcommit_pager_choice();\n \n-- \n2.14.0.rc1.12.ge2d9c4613\n\n"},{"id":"325552","messageId":"xmqqpoccpmux.fsf@gitster.mtv.corp.google.com","threadId":"46350","inReplyTo":"e660a9cdaff6d114305a475f9a12876b56b473d1.1501701128.git.martin.agren@gmail.com","subject":"Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-03T17:44:38Z","receivedAt":"2017-08-03T17:44:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> diff --git a/builtin.h b/builtin.h\n> index 498ac80d0..8d87d06da 100644\n> --- a/builtin.h\n> +++ b/builtin.h\n> @@ -6,6 +6,86 @@\n> #include \"cache.h\"\n> #include \"commit.h\"\n> \n> +/*\n> + * builtin API\n> + * ===========\n> + *\n> + * Adding a new built-in\n> + * ---------------------\n> + *\n> + * There are 4 things to do to add a built-in command implementation to\n> + * Git:\n> + *\n> + * . Define the implementation of the built-in command `foo` with\n> + * signature:\n> + *\n> + *\tint cmd_foo(int argc, const char **argv, const char *prefix);\n> + *\n> + * . Add the external declaration for the function to `builtin.h`.\n> + *\n> + * . Add the command to the `commands[]` table defined in `git.c`.\n> + * The entry should look like:\n> + *\n> + *\t{ \"foo\", cmd_foo, },\n> + *\n> + * where options is the bitwise-or of:\n> + *\n> + * `RUN_SETUP`:\n> + *\tIf there is not a Git directory to work on, abort. If there\n> + *\tis a work tree, chdir to the top of it if the command was\n> + *\tinvoked in a subdirectory. If there is no work tree, no\n> + *\tchdir() is done.\n> + *\n> + * `RUN_SETUP_GENTLY`:\n> + *\tIf there is a Git directory, chdir as per RUN_SETUP, otherwise,\n> + *\tdon't chdir anywhere.\n> + *\n> + * `USE_PAGER`:\n> + *\n> + *\tIf the standard output is connected to a tty, spawn a pager and\n> + *\tfeed our output to it.\n> + *\n> + * `NEED_WORK_TREE`:\n> + *\n> + *\tMake sure there is a work tree, i.e. the command cannot act\n> + *\ton bare repositories.\n> + *\tThis only makes sense when `RUN_SETUP` is also set.\n> + *\n> + * `SUPPORT_SUPER_PREFIX`:\n> + *\n> + *\tThe built-in supports `--super-prefix`.\n> + *\n> + * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n\nNot a new problem but it will become much easier to follow if we\nmoved this item between the \"implement cmd_foo()\" and \"declare\ncmd_foo in builtin.h\", like so:\n\n . Define the implementation of the built-in command `foo` with\n signature:\n\n\tint cmd_foo(int argc, const char **argv, const char *prefix);\n\n in a new file `builtin/foo.c`.\n\n . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n\nAlternatively, we may merge these two into one item (i.e. \"in a new\nfile `builtin/foo.c` and add `builtin/foo.o` to ...\").\n\nBut of course, this patch 1/7 should not do any of the above. I am\nsuggesting a possible future clean-up for anybody on the list\nlistening from sidelines, and you do not have to be the person who\ndoes it.\n\nThanks.\n"},{"id":"325567","messageId":"xmqqshh8y0mf.fsf@gitster.mtv.corp.google.com","threadId":"46350","inReplyTo":"cover.1501701128.git.martin.agren@gmail.com","subject":"Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-03T18:20:08Z","receivedAt":"2017-08-03T18:20:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> This is the third version of my attempt to make `pager tag` useful (v1\n> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.\n>\n> I've squashed patches 01-03/10 and 07-08/10, respectively. The interdiff\n> is below. I managed to clean up some tests thanks to a drive-by comment\n> by Peff which cleared up a misunderstanding I had. Some minor changes,\n> e.g., I write \"built-in\" instead of \"builtin\", since that seemed\n> preferred, at least locally in builtin.h. I documented why a default\n> value of \"punt\" could be useful, but also that it's probably not wanted.\n>\n> Martin\n\nThanks for working on this. The whole series was well reasoned and\nwas a very pleasant read.\n\nWill queue.\n"},{"id":"325578","messageId":"20170803192947.7x5tadwb7lxermdk@sigill.intra.peff.net","threadId":"46350","inReplyTo":"cover.1501701128.git.martin.agren@gmail.com","subject":"Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-03T19:29:47Z","receivedAt":"2017-08-03T19:29:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 02, 2017 at 09:40:48PM +0200, Martin Ågren wrote:\n\n> This is the third version of my attempt to make `pager tag` useful (v1\n> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.\n\nThis looks good to me overall. One minor question from the interdiff:\n\n> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> index 8b2ffb1aa..9128ec5ac 100755\n> --- a/t/t7006-pager.sh\n> +++ b/t/t7006-pager.sh\n> @@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '\n> test_expect_success TTY 'git tag with no args respects pager.tag' '\n> \t# no args implies -l so this should page like -l\n> \trm -f paginated.out &&\n> -\ttest_terminal git -c pager.tag=no tag &&\n> +\ttest_terminal git -c pager.tag=false tag &&\n> \t! test -e paginated.out\n> '\n\nThese should behave the same, right? So this is just a style/consistency\nfix, not a bugfix?\n\n-Peff\n"},{"id":"325604","messageId":"CAN0heSrP=HeUcpfwXS9DyzCYCnCCocyV4iCHyAxrYPmW=LgZ+g@mail.gmail.com","threadId":"46350","inReplyTo":"xmqqpoccpmux.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-04T04:18:59Z","receivedAt":"2017-08-04T04:19:05Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 3 August 2017 at 19:44, Junio C Hamano wrote:\n> Martin Ågren writes:\n>> + * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n>\n> Not a new problem but it will become much easier to follow if we\n> moved this item between the \"implement cmd_foo()\" and \"declare\n> cmd_foo in builtin.h\", like so:\n>\n> . Define the implementation of the built-in command `foo` with\n> signature:\n>\n> int cmd_foo(int argc, const char **argv, const char *prefix);\n>\n> in a new file `builtin/foo.c`.\n>\n> . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n>\n> Alternatively, we may merge these two into one item (i.e. \"in a new\n> file `builtin/foo.c` and add `builtin/foo.o` to ...\").\n>\n> But of course, this patch 1/7 should not do any of the above. I am\n> suggesting a possible future clean-up for anybody on the list\n> listening from sidelines, and you do not have to be the person who\n> does it.\n\nThank you. If this series needs to be rerolled, I could do it as patch 2.\nAnd if not, I could try to remember to do it once this series has landed.\nA that point in time, I'd also like to try changing other commands (\"git\nbranch\") similar to \"git tag\" (although maybe your suggestion above\nshouldn't be part of that series, but go on its own).\n\nSince this is my first code contribution to Git, I'll ask about this part of\nSubmittingPatches:\n\n\"After the list reached a consensus that it is a good idea to apply the\npatch, re-send it with \"To:\" set to the maintainer [*1*] and \"cc:\" the\nlist [*2*] for inclusion.\"\n\nI will boldly assume that I should not be doing this. It seems to me this\ndoesn't happen very often or not at all -- possibly because you tend to\nbe involved in virtually all threads anyway, before the list reaches a\nconsensus.\n\nWhich brings me to my final point: Thanks for your very helpful feedback\nthroughout all three versions and even more thanks for your work on Git.\nI find it amazing how much time you are able to constantly spend on all\naspects -- \"high and low\" -- of Git. It goes a long way to explaining how\nGit can be so very useful. Thanks a lot!\n\nMartin\n"},{"id":"325605","messageId":"CAN0heSpzg4jCMetyhdpQn2Zz7nDhv1X14SLQ26WvnRDe9NYCCg@mail.gmail.com","threadId":"46350","inReplyTo":"20170803192947.7x5tadwb7lxermdk@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-04T04:21:47Z","receivedAt":"2017-08-04T04:21:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 3 August 2017 at 21:29, Jeff King wrote:\n> On Wed, Aug 02, 2017 at 09:40:48PM +0200, Martin Ågren wrote:\n>\n>> This is the third version of my attempt to make `pager tag` useful (v1\n>> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.\n>\n> This looks good to me overall. One minor question from the interdiff:\n>\n>> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n>> index 8b2ffb1aa..9128ec5ac 100755\n>> --- a/t/t7006-pager.sh\n>> +++ b/t/t7006-pager.sh\n>> @@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '\n>> test_expect_success TTY 'git tag with no args respects pager.tag' '\n>> # no args implies -l so this should page like -l\n>> rm -f paginated.out &&\n>> - test_terminal git -c pager.tag=no tag &&\n>> + test_terminal git -c pager.tag=false tag &&\n>> ! test -e paginated.out\n>> '\n>\n> These should behave the same, right? So this is just a style/consistency\n> fix, not a bugfix?\n\nRight. I realized I was using \"false\" everywhere else. It wouldn't have hurt\nto exercise the config-parsing a tiny bit differently, but I assume that's\nalready being done explicitly in some other test, so I went for consistency.\n\nThanks for all the feedback and thoughts throughout the different versions\nof this series. It changed quite a bit since v1, so thanks a lot.\n\nMartin\n"},{"id":"325606","messageId":"20170804045911.rffpmrfqmy36r5l5@sigill.intra.peff.net","threadId":"46350","inReplyTo":"CAN0heSpzg4jCMetyhdpQn2Zz7nDhv1X14SLQ26WvnRDe9NYCCg@mail.gmail.com","subject":"Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-04T04:59:11Z","receivedAt":"2017-08-04T04:59:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 04, 2017 at 06:21:47AM +0200, Martin Ågren wrote:\n\n> On 3 August 2017 at 21:29, Jeff King wrote:\n> > On Wed, Aug 02, 2017 at 09:40:48PM +0200, Martin Ågren wrote:\n> >\n> >> This is the third version of my attempt to make `pager tag` useful (v1\n> >> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.\n> >\n> > This looks good to me overall. One minor question from the interdiff:\n> >\n> >> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> >> index 8b2ffb1aa..9128ec5ac 100755\n> >> --- a/t/t7006-pager.sh\n> >> +++ b/t/t7006-pager.sh\n> >> @@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '\n> >> test_expect_success TTY 'git tag with no args respects pager.tag' '\n> >> # no args implies -l so this should page like -l\n> >> rm -f paginated.out &&\n> >> - test_terminal git -c pager.tag=no tag &&\n> >> + test_terminal git -c pager.tag=false tag &&\n> >> ! test -e paginated.out\n> >> '\n> >\n> > These should behave the same, right? So this is just a style/consistency\n> > fix, not a bugfix?\n> \n> Right. I realized I was using \"false\" everywhere else. It wouldn't have hurt\n> to exercise the config-parsing a tiny bit differently, but I assume that's\n> already being done explicitly in some other test, so I went for consistency.\n\nRight, that makes perfect sense. Thanks for confirming.\n\n> Thanks for all the feedback and thoughts throughout the different versions\n> of this series. It changed quite a bit since v1, so thanks a lot.\n\nYou're very welcome. I hope we see more patches from you in the future.\n:)\n\n-Peff\n"},{"id":"325608","messageId":"xmqqzibfuxu9.fsf@gitster.mtv.corp.google.com","threadId":"46350","inReplyTo":"CAN0heSrP=HeUcpfwXS9DyzCYCnCCocyV4iCHyAxrYPmW=LgZ+g@mail.gmail.com","subject":"Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-04T16:00:46Z","receivedAt":"2017-08-04T16:00:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Since this is my first code contribution to Git, I'll ask about this part of\n> SubmittingPatches:\n>\n> \"After the list reached a consensus that it is a good idea to apply the\n> patch, re-send it with \"To:\" set to the maintainer [*1*] and \"cc:\" the\n> list [*2*] for inclusion.\"\n>\n> I will boldly assume that I should not be doing this. It seems to me this\n> doesn't happen very often or not at all -- possibly because you tend to\n> be involved in virtually all threads anyway, before the list reaches a\n> consensus.\n\nYeah, that is in the \"ideal patch flow\" section, isn't it? We\nrarely achieve the \"ideal\" and often instead go for a more expedited\noption, it appears---perhaps I should try to be less involved in\nindividual patch reviews and place more review burden on other\nreviewers ;-)\n\nIn any case, it was a pleasure to cheer-lead on the progress of this\nseries. Thanks.\n"},{"id":"325610","messageId":"CAN0heSq=YE7ep57V6+3M3DKcyamutjug7EkxdXpG5HnSGT-f2A@mail.gmail.com","threadId":"46350","inReplyTo":"xmqqzibfuxu9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-04T16:42:31Z","receivedAt":"2017-08-04T16:42:40Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 4 August 2017 at 18:00, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> Since this is my first code contribution to Git, I'll ask about this part of\n>> SubmittingPatches:\n>>\n>> \"After the list reached a consensus that it is a good idea to apply the\n>> patch, re-send it with \"To:\" set to the maintainer [*1*] and \"cc:\" the\n>> list [*2*] for inclusion.\"\n>>\n>> I will boldly assume that I should not be doing this. It seems to me this\n>> doesn't happen very often or not at all -- possibly because you tend to\n>> be involved in virtually all threads anyway, before the list reaches a\n>> consensus.\n>\n> Yeah, that is in the \"ideal patch flow\" section, isn't it?\n\nYes and no. It's in the main part above it, under \"(4) Sending your\npatches.\" But you are right that the ideal patch flow section then\nsays the same thing: \"(4) The list forms consensus that the last\nround of your patch is good. Send it to the maintainer and cc the\nlist.\"\n\n> We\n> rarely achieve the \"ideal\" and often instead go for a more expedited\n> option, it appears---perhaps I should try to be less involved in\n> individual patch reviews and place more review burden on other\n> reviewers ;-)\n\n:-)\n"}]}