[PATCH 0/7] tag: more fine-grained pager-configuration

[PATCH 0/7] tag: more fine-grained pager-configuration

[Martin Ågren]

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`.

In 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`.

This 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`.

I 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".

The 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...

A review would be much appreciated. Comments on the way I structured the series would be just as welcome as ones on the final result.

Martin

[1] https://public-inbox.org/git/nrmbrl$hsk$1@blaine.gmane.org/T/

Martin Ågren (7):
  api-builtin.txt: document SUPPORT_SUPER_PREFIX
  git.c: let builtins opt for handling `pager.foo` themselves
  git.c: provide setup_auto_pager()
  t7006: add tests for how git tag paginates
  tag: handle `pager.tag`-configuration within the builtin
  tag: make git tag -l consider new config `pager.tag.list`
  tag: make git tag -l use pager by default

 Documentation/git-tag.txt               |  4 +++
 Documentation/technical/api-builtin.txt | 10 ++++++
 builtin.h                               | 14 +++++++++
 builtin/tag.c                           |  4 +++
 git.c                                   | 44 +++++++++++++++++++++++++--
 t/t7006-pager.sh                        | 54 +++++++++++++++++++++++++++++++++
 6 files changed, 127 insertions(+), 3 deletions(-)

-- 
2.13.2.653.gfb5159d

[PATCH 1/7] api-builtin.txt: document SUPPORT_SUPER_PREFIX

[Martin Ågren]

Commit 74866d75 ("git: make super-prefix option", 2016-10-07) introduced
SUPPORT_SUPER_PREFIX as a builtin flag without documenting it in
api-builtin.txt. The next patch will add another flag, so document
SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with
the available flags.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/technical/api-builtin.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
index 22a39b929..60f442822 100644
--- a/Documentation/technical/api-builtin.txt
+++ b/Documentation/technical/api-builtin.txt
@@ -42,6 +42,10 @@ where options is the bitwise-or of:
 	on bare repositories.
 	This only makes sense when `RUN_SETUP` is also set.
 
+`SUPPORT_SUPER_PREFIX`::
+
+	The builtin supports --super-prefix.
+
 . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
 
 Additionally, if `foo` is a new command, there are 3 more things to do:
-- 
2.13.2.653.gfb5159d

[PATCH 2/7] git.c: let builtins opt for handling `pager.foo` themselves

[Martin Ågren]

Before launching a builtin git foo and unless mechanisms with precedence
are in use, we check for and handle the `pager.foo` config. This is done
without considering exactly how git foo is being used, and indeed, git.c
cannot (and should not) know what the arguments to git foo are supposed
to achieve.

In practice this means that, 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`.

To allow individual builtins to make more informed decisions about when
to respect `pager.foo`, introduce a flag IGNORE_PAGER_CONFIG. If the flag
is set, do not check `pager.foo`. This applies to two code-paths -- one
in run_builtin() and one in execv_dashed_external().

Document the flag in api-builtin.txt.

Don't add any users of IGNORE_PAGER_CONFIG just yet, one will follow in a
later patch.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/technical/api-builtin.txt |  6 ++++++
 git.c                                   | 14 ++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
index 60f442822..61fd8eeb2 100644
--- a/Documentation/technical/api-builtin.txt
+++ b/Documentation/technical/api-builtin.txt
@@ -46,6 +46,12 @@ where options is the bitwise-or of:
 
 	The builtin supports --super-prefix.
 
+`IGNORE_PAGER_CONFIG`::
+
+	Ignore the `pager.<cmd>`-configuration in git.c, instead
+	giving the builtin the chance to handle it and possibly
+	more fine-grained configurations (`pager.<cmd>.<subcmd>`).
+
 . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
 
 Additionally, if `foo` is a new command, there are 3 more things to do:
diff --git a/git.c b/git.c
index 489aab4d8..ae92f8ed5 100644
--- a/git.c
+++ b/git.c
@@ -283,6 +283,12 @@ static int handle_alias(int *argcp, const char ***argv)
  */
 #define NEED_WORK_TREE		(1<<3)
 #define SUPPORT_SUPER_PREFIX	(1<<4)
+/*
+ * Ignore the `pager.<cmd>`-configuration, instead giving the builtin
+ * the chance to handle it and possibly more fine-grained
+ * configurations (`pager.<cmd>.<subcmd>`).
+ */
+#define IGNORE_PAGER_CONFIG	(1<<5)
 
 struct cmd_struct {
 	const char *cmd;
@@ -306,7 +312,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
 
-		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
+		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
+		    !(p->option & IGNORE_PAGER_CONFIG))
 			use_pager = check_pager_config(p->cmd);
 		if (use_pager == -1 && p->option & USE_PAGER)
 			use_pager = 1;
@@ -543,11 +550,14 @@ static void execv_dashed_external(const char **argv)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	int status;
+	struct cmd_struct *builtin;
 
 	if (get_super_prefix())
 		die("%s doesn't support --super-prefix", argv[0]);
 
-	if (use_pager == -1)
+	builtin = get_builtin(argv[0]);
+	if (use_pager == -1 &&
+	    !(builtin && builtin->option & IGNORE_PAGER_CONFIG))
 		use_pager = check_pager_config(argv[0]);
 	commit_pager_choice();
 
-- 
2.13.2.653.gfb5159d

[PATCH 3/7] git.c: provide setup_auto_pager()

[Martin Ågren]

The previous patch introduced a way for builtins to declare that they
will take responsibility for handling the `pager.foo`-config item. (See
the commit message of that patch for why that could be useful.)

Provide setup_auto_pager(), which builtins can call in order to handle
`pager.<cmd>`, including possibly starting the pager. Make this function
don't do anything if a pager has already been started.

When the `cmd` given to setup_auto_pager() contains one or more '.', use
a fallback strategy which checks `pager.foo.bar.baz`, then
`pager.foo.bar`, then `pager.foo`, then resorts to the provided default
value. This ensures a consistent fallback strategy for builtins which
take this type of more fine-grained pager configuration.

An alternative fallback strategy would have been to check for
`pager.foo.bar.baz`, then immediately fall back to `def`. However, that
would have meant that git foo would sometimes completely ignore
`pager.foo`, which seems conceptually wrong. It would also have meant
that builtins that are moved to more fine-grained pager configurations
would have regressed for certain usecases.

Whenever this function is called from a builtin, git.c will already have
called commit_pager_choice(). Since commit_pager_choice() treats the
special value -1 as "punt" or "not yet decided", it is not a problem
that we might end up calling commit_pager_choice() many times. Make the
new function use -1 in the same way and document it as "punt".

Don't add any users of setup_auto_pager just yet, one will follow in
later patches.

setup_auto_pager() is more capable than it needs to be for this patch
series. It would have been sufficient to handle zero or one '.'. We
would probably have wanted some verification+BUG()-patterns or to
define whether we split at the first or last '.', so it seems just as
easy, and certainly more flexible, to handle the more general situation.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 14 ++++++++++++++
 git.c     | 28 ++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/builtin.h b/builtin.h
index 498ac80d0..a6ed6c4ac 100644
--- a/builtin.h
+++ b/builtin.h
@@ -25,6 +25,20 @@ struct fmt_merge_msg_opts {
 extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 			 struct fmt_merge_msg_opts *);
 
+/**
+ * If a builtin has IGNORE_PAGER_CONFIG set, the builtin should call this early
+ * when it wishes to respect the `pager.foo`-config. In the simplest case, the
+ * `cmd` is the name of the builtin, e.g., "foo". If a paging-choice has already
+ * been setup, this does nothing. The default in `def` should be 0 for "pager
+ * off", 1 for "pager on" or -1 for "punt".
+ *
+ * With one or more '.', substrings are tried out from longer to shorter. If no
+ * config is found, uses `def`. For example, with `cmd` as "foo.bar.baz", this
+ * function tries `pager.foo.bar.baz`, `pager.foo.bar` and `pager.foo` in that
+ * order before resorting to `def`.
+ */
+extern void setup_auto_pager(const char *cmd, int def);
+
 extern int is_builtin(const char *s);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index ae92f8ed5..696eaf87a 100644
--- a/git.c
+++ b/git.c
@@ -33,6 +33,34 @@ static void commit_pager_choice(void) {
 	}
 }
 
+void setup_auto_pager(const char *cmd, int def)
+{
+	if (use_pager != -1)
+		return;
+
+	use_pager = check_pager_config(cmd);
+
+	if (use_pager == -1) {
+		struct strbuf buf = STRBUF_INIT;
+		size_t len;
+
+		strbuf_addstr(&buf, cmd);
+		len = buf.len;
+		while (use_pager == -1 && len--) {
+			if (buf.buf[len] == '.') {
+				strbuf_setlen(&buf, len);
+				use_pager = check_pager_config(buf.buf);
+			}
+		}
+		strbuf_release(&buf);
+	}
+
+	if (use_pager == -1)
+		use_pager = def;
+
+	commit_pager_choice();
+}
+
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
 	const char **orig_argv = *argv;
-- 
2.13.2.653.gfb5159d

[PATCH 4/7] t7006: add tests for how git tag paginates

[Martin Ågren]

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`.

Since we're about to add some finer granularity to the configuration,
add tests around how git tag respects `pager.tag` and how that
configuration is ignored if --no-pager or --paginate are used.

Construct tests with two different subcommands: using -l and using -a,
where -a is being used essentially as a representative for "not -l".

Make one of the tests demonstrate the behavior mentioned above, where
`git tag -a` respects `pager.tag`. Actually, the tests use `git tag -a`
with -m, in which case no editor is launched, but that is irrelevant,
since we just want to see whether the pager is used or not. (If `git tag
-am` ever learns to avoid the pager, these tests will need to be
updated and two of them will fail.)

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t7006-pager.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 20b4d83c2..43cce3694 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,6 +134,46 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
 	}
 '
 
+test_expect_success TTY 'git tag -l defaults to not paging' '
+	rm -f paginated.out &&
+	test_terminal git tag -l &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects pager.tag' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag tag -l &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects --no-pager' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag --no-pager tag -l &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a defaults to not paging' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git tag -am message newtag &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a respects pager.tag' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag tag -am message newtag &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a respects --paginate' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag=false --paginate \
+		tag -am message newtag &&
+	test -e paginated.out
+'
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
-- 
2.13.2.653.gfb5159d

[PATCH 5/7] tag: handle `pager.tag`-configuration within the builtin

[Martin Ågren]

Use the mechanisms introduced in two earlier patches to ignore
`pager.tag` in git.c and let the `git tag` builtin handle it on its own.

This is in preparation for the next patch, where we will want to handle
slightly different configuration variables depending on which options
are used with `git tag`. For this reason, place the call to
setup_auto_pager() after the options have been parsed.

No functional change is intended. That said, there is a window between
where the pager is started before and after this patch, and if an error
occurs within this window, as of this patch the error message might not
be paged where it would have been paged before. Since
operation-parsing has to happen inside this window, a difference can be
seen with, e.g., `git -c pager.tag="echo pager is used" tag
--unknown-option`. This change in paging-behavior should be acceptable
since it only affects erroneous usages.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/tag.c | 2 ++
 git.c         | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 01154ea8d..e0f129872 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -446,6 +446,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
+	setup_auto_pager("tag", 0);
+
 	if (keyid) {
 		opt.sign = 1;
 		set_signing_key(keyid);
diff --git a/git.c b/git.c
index 696eaf87a..4d05452a3 100644
--- a/git.c
+++ b/git.c
@@ -489,7 +489,7 @@ static struct cmd_struct commands[] = {
 	{ "stripspace", cmd_stripspace },
 	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},
 	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
-	{ "tag", cmd_tag, RUN_SETUP },
+	{ "tag", cmd_tag, RUN_SETUP | IGNORE_PAGER_CONFIG },
 	{ "unpack-file", cmd_unpack_file, RUN_SETUP },
 	{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
 	{ "update-index", cmd_update_index, RUN_SETUP },
-- 
2.13.2.653.gfb5159d

[PATCH 6/7] tag: make git tag -l consider new config `pager.tag.list`

[Martin Ågren]

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`.

Introduce `pager.tag.list`. Teach git tag to prefer it over `pager.tag`
when running with -l. Update the documentation and add tests. Update an
existing test to use `pager.tag.list` instead of `pager.tag` together
with `git tag -l` since the former is arguably more relevant.

Do not introduce an "else" where we call setup_auto_pager(), although we
could have. Omitting it might keep someone who later implements even
more fine-grained configurations from building a correspondingly
complicated decision tree which carefully ensures that
setup_auto_pager() is called precisely once. A greedy approach such as
the one taken here works just fine.

Noticed-by: Anatoly Borodin <anatoly.borodin@gmail.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  4 ++++
 builtin/tag.c             |  2 ++
 t/t7006-pager.sh          | 16 +++++++++++++++-
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 1eb15afa1..6ad5811a2 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -101,6 +101,10 @@ patterns may be given; if any of them matches, the tag is shown.
 This option is implicitly supplied if any other list-like option such
 as `--contains` is provided. See the documentation for each of those
 options for details.
++
+When determining whether to use a pager, `pager.tag.list` is considered
+before `pager.tag`.
+See linkgit:git-config[1].
 
 --sort=<key>::
 	Sort based on the key given.  Prefix `-` to sort in
diff --git a/builtin/tag.c b/builtin/tag.c
index e0f129872..e96ef7d70 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -446,6 +446,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
+	if (cmdmode == 'l')
+		setup_auto_pager("tag.list", 0);
 	setup_auto_pager("tag", 0);
 
 	if (keyid) {
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 43cce3694..ed34c6734 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -146,9 +146,15 @@ test_expect_success TTY 'git tag -l respects pager.tag' '
 	test -e paginated.out
 '
 
+test_expect_success TTY 'git tag -l respects pager.tag.list' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag=false -c pager.tag.list tag -l &&
+	test -e paginated.out
+'
+
 test_expect_success TTY 'git tag -l respects --no-pager' '
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag --no-pager tag -l &&
+	test_terminal git -c pager.tag.list --no-pager tag -l &&
 	! test -e paginated.out
 '
 
@@ -166,6 +172,14 @@ test_expect_success TTY 'git tag -a respects pager.tag' '
 	test -e paginated.out
 '
 
+test_expect_success TTY 'git tag -a ignores pager.tag.list' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag -c pager.tag.list=false \
+		tag -am message newtag &&
+	test -e paginated.out
+'
+
 test_expect_success TTY 'git tag -a respects --paginate' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
-- 
2.13.2.653.gfb5159d

[PATCH 7/7] tag: make git tag -l use pager by default

[Martin Ågren]

The previous patch introduced `pager.tag.list`. Its default was to use
the value of `pager.tag` or, if that was also not set, to fall back to
"off".

Change that fallback value to "on". Let the default value for
`pager.tag` remain at "off". Update documentation and tests.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  2 +-
 builtin/tag.c             |  2 +-
 t/t7006-pager.sh          | 12 ++++++------
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 6ad5811a2..fbdfb3f59 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -103,7 +103,7 @@ as `--contains` is provided. See the documentation for each of those
 options for details.
 +
 When determining whether to use a pager, `pager.tag.list` is considered
-before `pager.tag`.
+before `pager.tag`. If neither is set, the default is to use a pager.
 See linkgit:git-config[1].
 
 --sort=<key>::
diff --git a/builtin/tag.c b/builtin/tag.c
index e96ef7d70..ec69fca61 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -447,7 +447,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
 	if (cmdmode == 'l')
-		setup_auto_pager("tag.list", 0);
+		setup_auto_pager("tag.list", 1);
 	setup_auto_pager("tag", 0);
 
 	if (keyid) {
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index ed34c6734..94df89a5f 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,22 +134,22 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
 	}
 '
 
-test_expect_success TTY 'git tag -l defaults to not paging' '
+test_expect_success TTY 'git tag -l defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git tag -l &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects pager.tag' '
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag tag -l &&
-	test -e paginated.out
+	test_terminal git -c pager.tag=false tag -l &&
+	! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects pager.tag.list' '
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag=false -c pager.tag.list tag -l &&
-	test -e paginated.out
+	test_terminal git -c pager.tag -c pager.tag.list=false tag -l &&
+	! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects --no-pager' '
-- 
2.13.2.653.gfb5159d

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> 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`.
>
> In 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`.
>
> This 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`.
>
> I 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".

All of the above smells like taking us in a sensible direction.  I
agree with these design decisions described in the above, i.e.
'pager.tag.list falling back to pager.tag', making this an opt-in to
make code transition easier.

Even though it is purely internal thing, IGNORE_PAGER_CONFIG
probably is a bit confusion-inducing name.  After all, the
subcommand specific configuration is not being ignored---we are
merely delaying our reaction to it---instead of acting on it inside
git.c without giving the subcommand a chance to make a decision, we
are still letting (and we do expect) the subcommand to react to it.

But that is a fairly minor thing we can fix.

> A review would be much appreciated. Comments on the way I
> structured the series would be just as welcome as ones on the
> final result.

I see you CC'ed Peff who's passionate in this area, so these patches
are in good hands already ;-) I briefly skimmed your patches myself,
and did not spot anything glaringly wrong.

Thanks.

Re: [PATCH 1/7] api-builtin.txt: document SUPPORT_SUPER_PREFIX

[Brandon Williams]

On 07/10, Martin Ågren wrote:
> Commit 74866d75 ("git: make super-prefix option", 2016-10-07) introduced
> SUPPORT_SUPER_PREFIX as a builtin flag without documenting it in
> api-builtin.txt. The next patch will add another flag, so document
> SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with
> the available flags.
> 
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  Documentation/technical/api-builtin.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
> index 22a39b929..60f442822 100644
> --- a/Documentation/technical/api-builtin.txt
> +++ b/Documentation/technical/api-builtin.txt
> @@ -42,6 +42,10 @@ where options is the bitwise-or of:
>  	on bare repositories.
>  	This only makes sense when `RUN_SETUP` is also set.
>  
> +`SUPPORT_SUPER_PREFIX`::
> +
> +	The builtin supports --super-prefix.
> +
>  . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
>  
>  Additionally, if `foo` is a new command, there are 3 more things to do:

I added SUPER_PREFIX as an implementation detail when trying to recurse
submodules using multiple processes.  Now that we have a 'struct
repository' my plan is to remove SUPER_PREFIX in its entirety.  Since
this won't happen overnight it may still take a bit till its removed so
maybe it makes sense to better document it until that happens?

Either way I think that this sort of Documention better lives in the
code as it is easier to keep up to date.  Last time I tried to look at
stuff in Documentation/technical the files were either place holders or
completely out of date with what the code did.

-- 
Brandon Williams

Re: [PATCH 1/7] api-builtin.txt: document SUPPORT_SUPER_PREFIX

[Martin Ågren]

On 11 July 2017 at 00:50, Brandon Williams <bmwill@google.com> wrote:
> On 07/10, Martin Ågren wrote:
>> Commit 74866d75 ("git: make super-prefix option", 2016-10-07) introduced
>> SUPPORT_SUPER_PREFIX as a builtin flag without documenting it in
>> api-builtin.txt. The next patch will add another flag, so document
>> SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with
>> the available flags.
>>
>> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
>> ---
>>  Documentation/technical/api-builtin.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
>> index 22a39b929..60f442822 100644
>> --- a/Documentation/technical/api-builtin.txt
>> +++ b/Documentation/technical/api-builtin.txt
>> @@ -42,6 +42,10 @@ where options is the bitwise-or of:
>>       on bare repositories.
>>       This only makes sense when `RUN_SETUP` is also set.
>>
>> +`SUPPORT_SUPER_PREFIX`::
>> +
>> +     The builtin supports --super-prefix.
>> +
>>  . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
>>
>>  Additionally, if `foo` is a new command, there are 3 more things to do:
>
> I added SUPER_PREFIX as an implementation detail when trying to recurse
> submodules using multiple processes.  Now that we have a 'struct
> repository' my plan is to remove SUPER_PREFIX in its entirety.  Since
> this won't happen overnight it may still take a bit till its removed so
> maybe it makes sense to better document it until that happens?

I could add something about how this is "temporary" although I have no
idea about the timeframe. ;-)

> Either way I think that this sort of Documention better lives in the
> code as it is easier to keep up to date.

Agreed and I believe that's the long-term goal. I could replace this
preparatory patch with another one where I move api-builtin.txt into
builtin.h or git.c (and document SUPPORT_SUPER_PREFIX if that's
wanted). That's probably better, since right now, patch 2/7 adds
basically the same documentation for the new flag in two places.

Martin

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Jeff King]

On Mon, Jul 10, 2017 at 11:55:13PM +0200, Martin Ågren wrote:

> 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`.

Right, I think we are all agreed that "pager.tag" as it is now is
essentially worthless.

If I understand your series correctly, though, it adds pager.tag.list
but leaves "pager.tag" behaving more or less the same. It's good that we
now have a way for a user to do the thing they actually want, but it
feels like we're leaving pager.tag behind as a booby-trap.

Should we disable it entirely (or only respect it in list mode)?

At which point, I wonder if we actually need pager.tag.list at all.
Slicing up the namespace further would be valuable if there were a
command which had two pager-worthy modes, and somebody might want to
enable the pager for one but not the other. But it seems like most
commands in this boat (e.g., tag, branch, stash) really have two modes:
listing things or creating things.

Would it makes sense to just have git-tag respect pager.tag in listing
mode, and otherwise ignore it completely?

One nice side effect is that it keeps the multi-level pager.X.Y
namespace clear. We've talked about transitioning to allow:

  [pager "foo"]
  enable = true
  command = some-custom-pager

to configure aspects of the pager separately for git-foo. This has two
benefits:

  1. Syntactically, it allows configuration for commands whose names
     aren't valid config keys.

  2. It would allow setting a command with "enable=false", so that "git
     foo" did not paginate, but "git -p foo" paginated with a custom
     command.

Those are admittedly minor features. And assuming we don't go crazy with
the multi-level names, we could have "pager.tag.list" live alongside
"pager.tag.command". So it's not really an objection, so much as wonder
out loud whether we can keep this as simple as possible.

> This 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`.

Yeah, I agree that turning "pager.tag" into a complete noop is probably
a bad idea. But if we made it a silent noop for the non-list cases, that
would be OK (and the hypothetical user who set it to make `git tag -l`
work would see a strict improvement; they'd still get their paging but
not the weird breakage with non-list modes). And I think that applies
whether we have a pager.tag.list in addition or not.

> I 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".

I think this is very sensible. It's the vast minority that would want to
enable this (git-branch is the other obvious one). At some point we may
need a plan to handle non-builtins (like stash), but that's largely
orthogonal to what you're doing here.

> The 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...

I think I covered my thoughts on this part above. :)

> A review would be much appreciated. Comments on the way I structured
> the series would be just as welcome as ones on the final result.

Overall the code looks good, though I'll respond with a few comments.

-Peff

Re: [PATCH 2/7] git.c: let builtins opt for handling `pager.foo` themselves

[Jeff King]

On Mon, Jul 10, 2017 at 11:55:15PM +0200, Martin Ågren wrote:

> To allow individual builtins to make more informed decisions about when
> to respect `pager.foo`, introduce a flag IGNORE_PAGER_CONFIG. If the flag
> is set, do not check `pager.foo`. This applies to two code-paths -- one
> in run_builtin() and one in execv_dashed_external().

Can this ever trigger in execv_dashed_external()? We should only get
there if get_builtin() returned NULL in the first place. Otherwise, we'd
run and exited via handle_builtin().

So I think this hunk:

> @@ -543,11 +550,14 @@ static void execv_dashed_external(const char **argv)
>  {
>  	struct child_process cmd = CHILD_PROCESS_INIT;
>  	int status;
> +	struct cmd_struct *builtin;
>  
>  	if (get_super_prefix())
>  		die("%s doesn't support --super-prefix", argv[0]);
>  
> -	if (use_pager == -1)
> +	builtin = get_builtin(argv[0]);
> +	if (use_pager == -1 &&
> +	    !(builtin && builtin->option & IGNORE_PAGER_CONFIG))
>  		use_pager = check_pager_config(argv[0]);
>  	commit_pager_choice();

...can just go away. And that highlights the issue with externals; we
don't have any internal database that says "these ones handle their own
pager". We don't even have a list of all the possibilities, since users
can drop whatever they like into their $PATH.

So we'd have to make a (backwards-incompatible) decision that pager.*
doesn't work for external commands, and they must manually trigger the
pager themselves. I'm undecided on whether that's reasonable or not
(certainly it's what git-stash would want, but it may be hurting other
commands).

Anyway, I think that's out of scope for your series, which is just
trying to make the builtins work better.

-Peff

Re: [PATCH 3/7] git.c: provide setup_auto_pager()

[Jeff King]

On Mon, Jul 10, 2017 at 11:55:16PM +0200, Martin Ågren wrote:

> +void setup_auto_pager(const char *cmd, int def)
> +{
> +	if (use_pager != -1)
> +		return;

I think you probably also want to return early here if pager_in_use()
is true. If a script runs "git tag -l", you wouldn't want to start a new
pager when the outer script has already been paged (either via config,
or via "git -p").

> +	use_pager = check_pager_config(cmd);
> +
> +	if (use_pager == -1) {
> +		struct strbuf buf = STRBUF_INIT;
> +		size_t len;
> +
> +		strbuf_addstr(&buf, cmd);
> +		len = buf.len;
> +		while (use_pager == -1 && len--) {
> +			if (buf.buf[len] == '.') {
> +				strbuf_setlen(&buf, len);
> +				use_pager = check_pager_config(buf.buf);
> +			}
> +		}
> +		strbuf_release(&buf);
> +	}

This looks good. I wondered if we could fold this all into a single
loop, rather than having the extra check_pager_config() beforehand
(which confused me for a half-second at first). But I tried and I don't
think the result ended up any more readable.

-Peff

Re: [PATCH 5/7] tag: handle `pager.tag`-configuration within the builtin

[Jeff King]

On Mon, Jul 10, 2017 at 11:55:18PM +0200, Martin Ågren wrote:

> Use the mechanisms introduced in two earlier patches to ignore
> `pager.tag` in git.c and let the `git tag` builtin handle it on its own.
> 
> This is in preparation for the next patch, where we will want to handle
> slightly different configuration variables depending on which options
> are used with `git tag`. For this reason, place the call to
> setup_auto_pager() after the options have been parsed.
> 
> No functional change is intended. That said, there is a window between
> where the pager is started before and after this patch, and if an error
> occurs within this window, as of this patch the error message might not
> be paged where it would have been paged before. Since
> operation-parsing has to happen inside this window, a difference can be
> seen with, e.g., `git -c pager.tag="echo pager is used" tag
> --unknown-option`. This change in paging-behavior should be acceptable
> since it only affects erroneous usages.

Thanks for carefully thinking through the details. I agree that it's an
acceptable change of behavior.

-Peff

Re: [PATCH 6/7] tag: make git tag -l consider new config `pager.tag.list`

[Jeff King]

On Mon, Jul 10, 2017 at 11:55:19PM +0200, Martin Ågren wrote:

> diff --git a/builtin/tag.c b/builtin/tag.c
> index e0f129872..e96ef7d70 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -446,6 +446,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  
>  	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
>  
> +	if (cmdmode == 'l')
> +		setup_auto_pager("tag.list", 0);
>  	setup_auto_pager("tag", 0);

Ideally this would kick in whenever we are in list mode, even if the
user didn't say "-l". So:

  $ git tag

should probably respect the list config. Likewise, certain options like
"--contains" trigger list mode. I think the pager setup could just be
bumped a few lines down, past the "if (!cmdmode)" block that sets up
those defaults.

-Peff

Re: [PATCH 7/7] tag: make git tag -l use pager by default

[Jeff King]

On Mon, Jul 10, 2017 at 11:55:20PM +0200, Martin Ågren wrote:

> The previous patch introduced `pager.tag.list`. Its default was to use
> the value of `pager.tag` or, if that was also not set, to fall back to
> "off".
> 
> Change that fallback value to "on". Let the default value for
> `pager.tag` remain at "off". Update documentation and tests.

Thanks for splitting this out. The default setting is definitely
orthogonal to allowing the config.

I've been running with this default for several years now (using the
patch I showed in the earlier thread you linked). It _does_ occasionally
annoy me, but I think overall it's an improvement. So it seems like a
good thing to try, and we can see how people respond as they try it out.

-Peff

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Jeff King]

On Mon, Jul 10, 2017 at 03:42:14PM -0700, Junio C Hamano wrote:

> > A review would be much appreciated. Comments on the way I
> > structured the series would be just as welcome as ones on the
> > final result.
> 
> I see you CC'ed Peff who's passionate in this area, so these patches
> are in good hands already ;-) I briefly skimmed your patches myself,
> and did not spot anything glaringly wrong.

Heh, I don't think of "paging tag output" as one of my passions, but you
may be right. :)

I left a few comments on the code and direction, but I agree that
overall it looks pretty good. And I'm very impressed with the attention
to detail for a first-time contributor.

-Peff

Re: [PATCH 2/7] git.c: let builtins opt for handling `pager.foo` themselves

[Martin Ågren]

On 11 July 2017 at 12:24, Jeff King <peff@peff.net> wrote:
> On Mon, Jul 10, 2017 at 11:55:15PM +0200, Martin Ågren wrote:
>
>> To allow individual builtins to make more informed decisions about when
>> to respect `pager.foo`, introduce a flag IGNORE_PAGER_CONFIG. If the flag
>> is set, do not check `pager.foo`. This applies to two code-paths -- one
>> in run_builtin() and one in execv_dashed_external().
>
> Can this ever trigger in execv_dashed_external()? We should only get
> there if get_builtin() returned NULL in the first place. Otherwise, we'd
> run and exited via handle_builtin().

I can trigger it with this:

$ git -c pager.tag="echo paging" -c pager.tag.list=no -c alias.t=tag t -l

where the alias is what triggers it and the two pager-configurations
demonstrate the effect.

> So I think this hunk:
>
>> @@ -543,11 +550,14 @@ static void execv_dashed_external(const char **argv)
>>  {
>>       struct child_process cmd = CHILD_PROCESS_INIT;
>>       int status;
>> +     struct cmd_struct *builtin;
>>
>>       if (get_super_prefix())
>>               die("%s doesn't support --super-prefix", argv[0]);
>>
>> -     if (use_pager == -1)
>> +     builtin = get_builtin(argv[0]);
>> +     if (use_pager == -1 &&
>> +         !(builtin && builtin->option & IGNORE_PAGER_CONFIG))
>>               use_pager = check_pager_config(argv[0]);
>>       commit_pager_choice();
>
> ...can just go away.

If I remove this, the call I gave above will page although it
shouldn't, and it doesn't if I keep this hunk. There's this in
run_argv: "If we tried alias and futzed with our environment, it no
longer is safe to invoke builtins directly in general.  We have to
spawn them as dashed externals." There's also a NEEDSWORK.

Although, thinking about it, I'm not sure why when I remove this hunk,
the child process doesn't set up the paging correctly. Maybe something
related to my using "-c", or something about the launching of child
processes. Those are both areas where I lack knowledge. Will look into
it.

Martin

Re: [PATCH 3/7] git.c: provide setup_auto_pager()

[Martin Ågren]

On 11 July 2017 at 12:37, Jeff King <peff@peff.net> wrote:
> On Mon, Jul 10, 2017 at 11:55:16PM +0200, Martin Ågren wrote:
>
>> +void setup_auto_pager(const char *cmd, int def)
>> +{
>> +     if (use_pager != -1)
>> +             return;
>
> I think you probably also want to return early here if pager_in_use()
> is true. If a script runs "git tag -l", you wouldn't want to start a new
> pager when the outer script has already been paged (either via config,
> or via "git -p").

Good point. Thanks.

Martin

Re: [PATCH 6/7] tag: make git tag -l consider new config `pager.tag.list`

[Martin Ågren]

On 11 July 2017 at 12:41, Jeff King <peff@peff.net> wrote:
> On Mon, Jul 10, 2017 at 11:55:19PM +0200, Martin Ågren wrote:
>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index e0f129872..e96ef7d70 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -446,6 +446,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>
>>       argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
>>
>> +     if (cmdmode == 'l')
>> +             setup_auto_pager("tag.list", 0);
>>       setup_auto_pager("tag", 0);
>
> Ideally this would kick in whenever we are in list mode, even if the
> user didn't say "-l". So:
>
>   $ git tag
>
> should probably respect the list config. Likewise, certain options like
> "--contains" trigger list mode. I think the pager setup could just be
> bumped a few lines down, past the "if (!cmdmode)" block that sets up
> those defaults.

Oops, that's embarassing. Thanks. That means the paging starts
slightly later, but what happens in those few lines looks fairly
trivial, so there shouldn't be any real difference in behavior. I'll
add one or two tests.

Martin

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Martin Ågren]

On 11 July 2017 at 12:19, Jeff King <peff@peff.net> wrote:
> On Mon, Jul 10, 2017 at 11:55:13PM +0200, Martin Ågren wrote:
>
>> 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`.
>
> Right, I think we are all agreed that "pager.tag" as it is now is
> essentially worthless.
>
> If I understand your series correctly, though, it adds pager.tag.list
> but leaves "pager.tag" behaving more or less the same. It's good that we
> now have a way for a user to do the thing they actually want, but it
> feels like we're leaving pager.tag behind as a booby-trap.
>
> Should we disable it entirely (or only respect it in list mode)?
>
> At which point, I wonder if we actually need pager.tag.list at all.
> Slicing up the namespace further would be valuable if there were a
> command which had two pager-worthy modes, and somebody might want to
> enable the pager for one but not the other. But it seems like most
> commands in this boat (e.g., tag, branch, stash) really have two modes:
> listing things or creating things.
>
> Would it makes sense to just have git-tag respect pager.tag in listing
> mode, and otherwise ignore it completely?

Yes. I doubt anyone would notice, and no-one should mind with the
change (famous last words).

It does mean there's a precedence for individual commands to get to
choose when to honor pager.foo. If more such exceptions are added, at
some point, some command will learn to ignore pager.foo in some
particular situation and someone will consider it a regression.

> One nice side effect is that it keeps the multi-level pager.X.Y
> namespace clear. We've talked about transitioning to allow:
>
>   [pager "foo"]
>   enable = true
>   command = some-custom-pager
>
> to configure aspects of the pager separately for git-foo. This has two
> benefits:
>
>   1. Syntactically, it allows configuration for commands whose names
>      aren't valid config keys.
>
>   2. It would allow setting a command with "enable=false", so that "git
>      foo" did not paginate, but "git -p foo" paginated with a custom
>      command.
>
> Those are admittedly minor features. And assuming we don't go crazy with
> the multi-level names, we could have "pager.tag.list" live alongside
> "pager.tag.command". So it's not really an objection, so much as wonder
> out loud whether we can keep this as simple as possible.

Well, I respect your hunch about .enable and .command and I certainly
don't want to take things in a direction that makes that approach less
clean. You have convinced me that I will instead try to teach git tag
to be more clever about when to use the pager, at least to see what
that looks like.

Let's call such a "git tag" the "future git tag". Just to convince
myself I've thought through the implications -- how would
pager.tag.enable=true affect that future git tag? Would it be fair to
say that enable=false means "definitely don't start the pager (unless
--paginate)" and that enable=true means "feel free to use it (unless
--no-paginate)"? The future git tag would default to using
enable=true. Would --paginate also be "feel free to use it", or rather
"the pager must be used"?

At some point, I thought about "true"/"false"/"maybe", where "maybe"
would be what the future git tag implements. Of course, there's a fair
chance not everyone will agree what exactly should be paged with
"maybe". So it's back to adding various knobs. ;)

Anyway, this is more my thinking out loud. I'll drop pager.tag.list in
v2 and will instead make pager.tag more clever. That should force me
to think through this some more.

>> This 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`.
>
> Yeah, I agree that turning "pager.tag" into a complete noop is probably
> a bad idea. But if we made it a silent noop for the non-list cases, that
> would be OK (and the hypothetical user who set it to make `git tag -l`
> work would see a strict improvement; they'd still get their paging but
> not the weird breakage with non-list modes). And I think that applies
> whether we have a pager.tag.list in addition or not.

Good thinking.

Thanks a lot for your comments. I appreciate it.

Martin

Re: [PATCH 2/7] git.c: let builtins opt for handling `pager.foo` themselves

[Jeff King]

On Tue, Jul 11, 2017 at 03:46:08PM +0200, Martin Ågren wrote:

> > Can this ever trigger in execv_dashed_external()? We should only get
> > there if get_builtin() returned NULL in the first place. Otherwise, we'd
> > run and exited via handle_builtin().
> 
> I can trigger it with this:
> 
> $ git -c pager.tag="echo paging" -c pager.tag.list=no -c alias.t=tag t -l
> 
> where the alias is what triggers it and the two pager-configurations
> demonstrate the effect.

Interesting. As you noted below, I think the dashed external we exec
should be choosing whether to run the pager. I suspect what's happening
is that execv_dashed_external() says "a-ha, we're running 'tag', and I
know how to check its pager config". But IMHO that is wrong in the first
place (but it just never really made a difference until your series).

That's just a guess, though. I didn't dig.

-Peff

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Jeff King]

On Tue, Jul 11, 2017 at 03:50:39PM +0200, Martin Ågren wrote:

> > Would it makes sense to just have git-tag respect pager.tag in listing
> > mode, and otherwise ignore it completely?
> 
> Yes. I doubt anyone would notice, and no-one should mind with the
> change (famous last words).
> 
> It does mean there's a precedence for individual commands to get to
> choose when to honor pager.foo. If more such exceptions are added, at
> some point, some command will learn to ignore pager.foo in some
> particular situation and someone will consider it a regression.

Yes, perhaps. One could even argue that "git tag foo" is OK to page and
somebody would consider it a regression not to respect pager.tag. It
seems useless to me, but at least it's not totally broken like "git tag
-a foo" is.

But I find it pretty unlikely, as it doesn't produce copious output. I'd
worry more about eventually finding a command with two copious-output
modes, and somebody wants to distinguish between them. If we can't come
up with a plausible example now, I'm inclined to deal with it if and
when it ever comes up. Right now I just don't want to paint ourselves
into a corner, and I think we can always add more config later (e.g.,
tag.pageFooCommand=true or something; that's not great, but I'm mostly
betting that it will never come up).

> Well, I respect your hunch about .enable and .command and I certainly
> don't want to take things in a direction that makes that approach less
> clean. You have convinced me that I will instead try to teach git tag
> to be more clever about when to use the pager, at least to see what
> that looks like.

Thanks. I was the original proponent of "pager.tag.list", and only after
reading your series today did I think "gee, this seems unnecessarily
complex". So it's entirely possible I'm missing a corner case that you
may uncover through discussion or experimenting.

> Let's call such a "git tag" the "future git tag". Just to convince
> myself I've thought through the implications -- how would
> pager.tag.enable=true affect that future git tag? Would it be fair to
> say that enable=false means "definitely don't start the pager (unless
> --paginate)" and that enable=true means "feel free to use it (unless
> --no-paginate)"? The future git tag would default to using
> enable=true. Would --paginate also be "feel free to use it", or rather
> "the pager must be used"?

I think the rules would be:

  1. If --paginate is given, do that. Likewise for --no-pager.

  2. Otherwise, respect tag.pager.enable if it is set.

  3. Otherwise, use the built-in default.

  4. Any time the pager is run, respect tag.pager.command if it is set.

And then there are two optional bits:

  2a. If tag.pager is set to a boolean, behave as if tag.pager.enable is
      set to the same boolean. If it's set to a string, behave as if
      tag.pager.enable is set to "true", and tag.pager.command is set to
      the string. That should give us backwards compatibility.

  2b. If tag.pager.command is set but tag.pager.enable isn't, possibly we
      should assume that tag.pager.enable is set to "true". That makes
      the common case of "page and use this command" a single config
      block instead of two. It matters less for "tag" where paging would
      be the default.

So I think that what you asked above, but to be clear on the final
question: --paginate should always be "must be used". And that meshes
nicely with implementing it via the git.c wrapper, where it takes
precedence way before we ever hit the setup_auto_pager() call.

-Peff

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I see you CC'ed Peff who's passionate in this area, so these patches
>> are in good hands already ;-) I briefly skimmed your patches myself,
>> and did not spot anything glaringly wrong.
>
> Heh, I don't think of "paging tag output" as one of my passions, but you
> may be right. :)

Perhaps not "tag output", but the paging is the area you are the
person who spent most time on.

> I left a few comments on the code and direction, but I agree that
> overall it looks pretty good. And I'm very impressed with the attention
> to detail for a first-time contributor.

Yes.

Re: [PATCH 1/7] api-builtin.txt: document SUPPORT_SUPER_PREFIX

[Brandon Williams]

On 07/11, Martin Ågren wrote:
> On 11 July 2017 at 00:50, Brandon Williams <bmwill@google.com> wrote:
> > On 07/10, Martin Ågren wrote:
> >> Commit 74866d75 ("git: make super-prefix option", 2016-10-07) introduced
> >> SUPPORT_SUPER_PREFIX as a builtin flag without documenting it in
> >> api-builtin.txt. The next patch will add another flag, so document
> >> SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to date with
> >> the available flags.
> >>
> >> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> >> ---
> >>  Documentation/technical/api-builtin.txt | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
> >> index 22a39b929..60f442822 100644
> >> --- a/Documentation/technical/api-builtin.txt
> >> +++ b/Documentation/technical/api-builtin.txt
> >> @@ -42,6 +42,10 @@ where options is the bitwise-or of:
> >>       on bare repositories.
> >>       This only makes sense when `RUN_SETUP` is also set.
> >>
> >> +`SUPPORT_SUPER_PREFIX`::
> >> +
> >> +     The builtin supports --super-prefix.
> >> +
> >>  . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
> >>
> >>  Additionally, if `foo` is a new command, there are 3 more things to do:
> >
> > I added SUPER_PREFIX as an implementation detail when trying to recurse
> > submodules using multiple processes.  Now that we have a 'struct
> > repository' my plan is to remove SUPER_PREFIX in its entirety.  Since
> > this won't happen overnight it may still take a bit till its removed so
> > maybe it makes sense to better document it until that happens?
> 
> I could add something about how this is "temporary" although I have no
> idea about the timeframe. ;-)

Its probably better to ensure that things are documented (even if they
are temporary) but you don't need to mark it as such.  And as far as
timeframe, that's my burden to bare ;)

> 
> > Either way I think that this sort of Documention better lives in the
> > code as it is easier to keep up to date.
> 
> Agreed and I believe that's the long-term goal. I could replace this
> preparatory patch with another one where I move api-builtin.txt into
> builtin.h or git.c (and document SUPPORT_SUPER_PREFIX if that's
> wanted). That's probably better, since right now, patch 2/7 adds
> basically the same documentation for the new flag in two places.

I know that sort of migration may be out of the scope of your series
(since its just documentation), though you're more than welcome to do
the work as it would be much appreciated by me and a few other people :)

-- 
Brandon Williams

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

[Martin Ågren]

On 11 July 2017 at 00:42, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>> I 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".
...
> Even though it is purely internal thing, IGNORE_PAGER_CONFIG
> probably is a bit confusion-inducing name.  After all, the
> subcommand specific configuration is not being ignored---we are
> merely delaying our reaction to it---instead of acting on it inside
> git.c without giving the subcommand a chance to make a decision, we
> are still letting (and we do expect) the subcommand to react to it.

Thank you for your comments. I'm working on v2 with the input from
you, Peff and Brandon. I'll make this DELAY_PAGER_CONFIG.

Martin

[PATCH v2 00/10] tag: only respect `pager.tag` in list-mode

[Martin Ågren]

This is the second version of "[PATCH 0/7] tag: more fine-grained
pager-configuration" [1]. That series introduced `pager.tag.list` to
address the fact that `pager.tag` can be useful with `git tag -l` but
actively hostile with `git tag -a`. Thanks to Junio, Peff and Brandon
for helpful feedback.

After that feedback, v2 drops `pager.tag.list` and instead teaches
`git tag` to only consider `pager.tag` in list-mode, as suggested by
Peff.

Patches 1-3/10 replace patch 1/7. They move Documentation/technical/
api-builtin.txt into builtin.h, tweak the formatting and bring it up to
date. I may have gone overboard making this 3 patches...

Patches 4-7/10 correspond to patches 2-5/7. `setup_auto_pager()' is now
much simpler since we do not need to handle "tag.list" with a clever
fallback strategy. IGNORE_PAGER_CONFIG is now called DELAY_PAGER_CONFIG.
I now check with pager_in_use() and I moved the handling of `pager.tag`
a bit further down.

Patches 8-9/10 teach `git tag` to only respect `pager.tag` in list-mode
and flip the default value for that config to "on".

Patch 10/10 is somewhat similar to a hunk in patch 2/7, but is now a
bug-fix instead of a feature. It teaches `execv_dashed_external()` not
to check `pager.foo` when launching `git-foo` where foo is a builtin.
I waffled about where to put this patch. Putting it earlier in the
series as a preparatory step, I couldn't come up with a way of writing a
test. So patch 8/10 introduces a `test_expect_failure` which this patch
then fixes.

Martin

[1] https://public-inbox.org/git/cover.1499723297.git.martin.agren@gmail.com/T/

Martin Ågren (10):
  builtin.h: take over documentation from api-builtin.txt
  builtin.h: format documentation-comment properly
  builtin.h: document SUPPORT_SUPER_PREFIX
  git.c: let builtins opt for handling `pager.foo` themselves
  git.c: provide setup_auto_pager()
  t7006: add tests for how git tag paginates
  tag: handle `pager.tag`-configuration within the builtin
  tag: respect `pager.tag` in list-mode only
  tag: change default of `pager.tag` to "on"
  git.c: ignore pager.* when launching builtin as dashed external

 Documentation/git-tag.txt               |  3 +
 Documentation/technical/api-builtin.txt | 73 -------------------------
 t/t7006-pager.sh                        | 85 +++++++++++++++++++++++++++++
 builtin.h                               | 97 +++++++++++++++++++++++++++++++++
 builtin/tag.c                           |  3 +
 git.c                                   | 18 +++++-
 6 files changed, 203 insertions(+), 76 deletions(-)
 delete mode 100644 Documentation/technical/api-builtin.txt

-- 
2.14.0.rc0

[PATCH v2 01/10] builtin.h: take over documentation from api-builtin.txt

[Martin Ågren]

Delete Documentation/technical/api-builtin.txt and move its content
verbatim into builtin.h. Just wrap it in /* ... */. In order to make
the move obviously correct, do not change any formatting, not even to
format the comment into Git's preferred style. That will be done in a
follow-up patch.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/technical/api-builtin.txt | 73 -------------------------------
 builtin.h                               | 76 +++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 73 deletions(-)
 delete mode 100644 Documentation/technical/api-builtin.txt

diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
deleted file mode 100644
index 22a39b929..000000000
--- a/Documentation/technical/api-builtin.txt
+++ /dev/null
@@ -1,73 +0,0 @@
-builtin API
-===========
-
-Adding a new built-in
----------------------
-
-There are 4 things to do to add a built-in command implementation to
-Git:
-
-. Define the implementation of the built-in command `foo` with
-  signature:
-
-	int cmd_foo(int argc, const char **argv, const char *prefix);
-
-. Add the external declaration for the function to `builtin.h`.
-
-. Add the command to the `commands[]` table defined in `git.c`.
-  The entry should look like:
-
-	{ "foo", cmd_foo, <options> },
-+
-where options is the bitwise-or of:
-
-`RUN_SETUP`::
-	If there is not a Git directory to work on, abort.  If there
-	is a work tree, chdir to the top of it if the command was
-	invoked in a subdirectory.  If there is no work tree, no
-	chdir() is done.
-
-`RUN_SETUP_GENTLY`::
-	If there is a Git directory, chdir as per RUN_SETUP, otherwise,
-	don't chdir anywhere.
-
-`USE_PAGER`::
-
-	If the standard output is connected to a tty, spawn a pager and
-	feed our output to it.
-
-`NEED_WORK_TREE`::
-
-	Make sure there is a work tree, i.e. the command cannot act
-	on bare repositories.
-	This only makes sense when `RUN_SETUP` is also set.
-
-. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
-
-Additionally, if `foo` is a new command, there are 3 more things to do:
-
-. Add tests to `t/` directory.
-
-. Write documentation in `Documentation/git-foo.txt`.
-
-. Add an entry for `git-foo` to `command-list.txt`.
-
-. Add an entry for `/git-foo` to `.gitignore`.
-
-
-How a built-in is called
-------------------------
-
-The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
-and `prefix`.  The first two are similar to what `main()` of a
-standalone command would be called with.
-
-When `RUN_SETUP` is specified in the `commands[]` table, and when you
-were started from a subdirectory of the work tree, `cmd_foo()` is called
-after chdir(2) to the top of the work tree, and `prefix` gets the path
-to the subdirectory the command started from.  This allows you to
-convert a user-supplied pathname (typically relative to that directory)
-to a pathname relative to the top of the work tree.
-
-The return value from `cmd_foo()` becomes the exit status of the
-command.
diff --git a/builtin.h b/builtin.h
index 498ac80d0..51cb0249d 100644
--- a/builtin.h
+++ b/builtin.h
@@ -6,6 +6,82 @@
 #include "cache.h"
 #include "commit.h"
 
+/*
+builtin API
+===========
+
+Adding a new built-in
+---------------------
+
+There are 4 things to do to add a built-in command implementation to
+Git:
+
+. Define the implementation of the built-in command `foo` with
+  signature:
+
+	int cmd_foo(int argc, const char **argv, const char *prefix);
+
+. Add the external declaration for the function to `builtin.h`.
+
+. Add the command to the `commands[]` table defined in `git.c`.
+  The entry should look like:
+
+	{ "foo", cmd_foo, <options> },
++
+where options is the bitwise-or of:
+
+`RUN_SETUP`::
+	If there is not a Git directory to work on, abort.  If there
+	is a work tree, chdir to the top of it if the command was
+	invoked in a subdirectory.  If there is no work tree, no
+	chdir() is done.
+
+`RUN_SETUP_GENTLY`::
+	If there is a Git directory, chdir as per RUN_SETUP, otherwise,
+	don't chdir anywhere.
+
+`USE_PAGER`::
+
+	If the standard output is connected to a tty, spawn a pager and
+	feed our output to it.
+
+`NEED_WORK_TREE`::
+
+	Make sure there is a work tree, i.e. the command cannot act
+	on bare repositories.
+	This only makes sense when `RUN_SETUP` is also set.
+
+. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
+
+Additionally, if `foo` is a new command, there are 3 more things to do:
+
+. Add tests to `t/` directory.
+
+. Write documentation in `Documentation/git-foo.txt`.
+
+. Add an entry for `git-foo` to `command-list.txt`.
+
+. Add an entry for `/git-foo` to `.gitignore`.
+
+
+How a built-in is called
+------------------------
+
+The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
+and `prefix`.  The first two are similar to what `main()` of a
+standalone command would be called with.
+
+When `RUN_SETUP` is specified in the `commands[]` table, and when you
+were started from a subdirectory of the work tree, `cmd_foo()` is called
+after chdir(2) to the top of the work tree, and `prefix` gets the path
+to the subdirectory the command started from.  This allows you to
+convert a user-supplied pathname (typically relative to that directory)
+to a pathname relative to the top of the work tree.
+
+The return value from `cmd_foo()` becomes the exit status of the
+command.
+ */
+
 #define DEFAULT_MERGE_LOG_LEN 20
 
 extern const char git_usage_string[];
-- 
2.14.0.rc0

[PATCH v2 02/10] builtin.h: format documentation-comment properly

[Martin Ågren]

The previous commit moved technical documentation verbatim into
builtin.h. Prefix all the lines with '*' to follow the coding
guidelines.

Remove a '+' which was needed when the information was formatted for
AsciiDoc. Similarly, change "::" to ":".

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 146 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 73 insertions(+), 73 deletions(-)

diff --git a/builtin.h b/builtin.h
index 51cb0249d..62f22b547 100644
--- a/builtin.h
+++ b/builtin.h
@@ -7,79 +7,79 @@
 #include "commit.h"
 
 /*
-builtin API
-===========
-
-Adding a new built-in
----------------------
-
-There are 4 things to do to add a built-in command implementation to
-Git:
-
-. Define the implementation of the built-in command `foo` with
-  signature:
-
-	int cmd_foo(int argc, const char **argv, const char *prefix);
-
-. Add the external declaration for the function to `builtin.h`.
-
-. Add the command to the `commands[]` table defined in `git.c`.
-  The entry should look like:
-
-	{ "foo", cmd_foo, <options> },
-+
-where options is the bitwise-or of:
-
-`RUN_SETUP`::
-	If there is not a Git directory to work on, abort.  If there
-	is a work tree, chdir to the top of it if the command was
-	invoked in a subdirectory.  If there is no work tree, no
-	chdir() is done.
-
-`RUN_SETUP_GENTLY`::
-	If there is a Git directory, chdir as per RUN_SETUP, otherwise,
-	don't chdir anywhere.
-
-`USE_PAGER`::
-
-	If the standard output is connected to a tty, spawn a pager and
-	feed our output to it.
-
-`NEED_WORK_TREE`::
-
-	Make sure there is a work tree, i.e. the command cannot act
-	on bare repositories.
-	This only makes sense when `RUN_SETUP` is also set.
-
-. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
-
-Additionally, if `foo` is a new command, there are 3 more things to do:
-
-. Add tests to `t/` directory.
-
-. Write documentation in `Documentation/git-foo.txt`.
-
-. Add an entry for `git-foo` to `command-list.txt`.
-
-. Add an entry for `/git-foo` to `.gitignore`.
-
-
-How a built-in is called
-------------------------
-
-The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
-and `prefix`.  The first two are similar to what `main()` of a
-standalone command would be called with.
-
-When `RUN_SETUP` is specified in the `commands[]` table, and when you
-were started from a subdirectory of the work tree, `cmd_foo()` is called
-after chdir(2) to the top of the work tree, and `prefix` gets the path
-to the subdirectory the command started from.  This allows you to
-convert a user-supplied pathname (typically relative to that directory)
-to a pathname relative to the top of the work tree.
-
-The return value from `cmd_foo()` becomes the exit status of the
-command.
+ * builtin API
+ * ===========
+ *
+ * Adding a new built-in
+ * ---------------------
+ *
+ * There are 4 things to do to add a built-in command implementation to
+ * Git:
+ *
+ * . Define the implementation of the built-in command `foo` with
+ *   signature:
+ *
+ *	int cmd_foo(int argc, const char **argv, const char *prefix);
+ *
+ * . Add the external declaration for the function to `builtin.h`.
+ *
+ * . Add the command to the `commands[]` table defined in `git.c`.
+ *   The entry should look like:
+ *
+ *	{ "foo", cmd_foo, <options> },
+ *
+ * where options is the bitwise-or of:
+ *
+ * `RUN_SETUP`:
+ *	If there is not a Git directory to work on, abort.  If there
+ *	is a work tree, chdir to the top of it if the command was
+ *	invoked in a subdirectory.  If there is no work tree, no
+ *	chdir() is done.
+ *
+ * `RUN_SETUP_GENTLY`:
+ *	If there is a Git directory, chdir as per RUN_SETUP, otherwise,
+ *	don't chdir anywhere.
+ *
+ * `USE_PAGER`:
+ *
+ *	If the standard output is connected to a tty, spawn a pager and
+ *	feed our output to it.
+ *
+ * `NEED_WORK_TREE`:
+ *
+ *	Make sure there is a work tree, i.e. the command cannot act
+ *	on bare repositories.
+ *	This only makes sense when `RUN_SETUP` is also set.
+ *
+ * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
+ *
+ * Additionally, if `foo` is a new command, there are 3 more things to do:
+ *
+ * . Add tests to `t/` directory.
+ *
+ * . Write documentation in `Documentation/git-foo.txt`.
+ *
+ * . Add an entry for `git-foo` to `command-list.txt`.
+ *
+ * . Add an entry for `/git-foo` to `.gitignore`.
+ *
+ *
+ * How a built-in is called
+ * ------------------------
+ *
+ * The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
+ * and `prefix`.  The first two are similar to what `main()` of a
+ * standalone command would be called with.
+ *
+ * When `RUN_SETUP` is specified in the `commands[]` table, and when you
+ * were started from a subdirectory of the work tree, `cmd_foo()` is called
+ * after chdir(2) to the top of the work tree, and `prefix` gets the path
+ * to the subdirectory the command started from.  This allows you to
+ * convert a user-supplied pathname (typically relative to that directory)
+ * to a pathname relative to the top of the work tree.
+ *
+ * The return value from `cmd_foo()` becomes the exit status of the
+ * command.
  */
 
 #define DEFAULT_MERGE_LOG_LEN 20
-- 
2.14.0.rc0

[PATCH v2 03/10] builtin.h: document SUPPORT_SUPER_PREFIX

[Martin Ågren]

Commit 74866d75 ("git: make super-prefix option", 2016-10-07) introduced
SUPPORT_SUPER_PREFIX as a builtin flag without documenting it. The next
patch will add another flag, so document SUPPORT_SUPER_PREFIX, thereby
bringing the documentation up to date with the available flags.

While at it, correct '3 more things to do' to '4 more things to do'.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin.h b/builtin.h
index 62f22b547..7bcc08456 100644
--- a/builtin.h
+++ b/builtin.h
@@ -51,9 +51,13 @@
  *	on bare repositories.
  *	This only makes sense when `RUN_SETUP` is also set.
  *
+ * `SUPPORT_SUPER_PREFIX`::
+ *
+ *	The builtin supports `--super-prefix`.
+ *
  * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
  *
- * Additionally, if `foo` is a new command, there are 3 more things to do:
+ * Additionally, if `foo` is a new command, there are 4 more things to do:
  *
  * . Add tests to `t/` directory.
  *
-- 
2.14.0.rc0

[PATCH v2 04/10] git.c: let builtins opt for handling `pager.foo` themselves

[Martin Ågren]

Before launching a builtin git foo and unless mechanisms with precedence
are in use, we check for and handle the `pager.foo` config. This is done
without considering exactly how git foo is being used, and indeed, git.c
cannot (and should not) know what the arguments to git foo are supposed
to achieve.

In practice this means that, 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`.

To allow individual builtins to make more informed decisions about when
to respect `pager.foo`, introduce a flag DELAY_PAGER_CONFIG. If the flag
is set, do not check `pager.foo`.

Do not check for DELAY_PAGER_CONFIG in `execv_dashed_external()`. That
call site is arguably wrong, although in a way that is not yet visible,
and will be changed in a slightly different direction in a later patch.

Don't add any users of DELAY_PAGER_CONFIG just yet, one will follow in a
later patch.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 8 ++++++++
 git.c     | 4 +++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin.h b/builtin.h
index 7bcc08456..4186635de 100644
--- a/builtin.h
+++ b/builtin.h
@@ -55,6 +55,14 @@
  *
  *	The builtin supports `--super-prefix`.
  *
+ * `DELAY_PAGER_CONFIG`::
+ *
+ *	If RUN_SETUP or RUN_SETUP_GENTLY is set, git.c normally handles
+ *	the `pager.<cmd>`-configuration. If this flag is used, git.c
+ *	will skip that step, instead allowing the builtin to make a
+ *	more informed decision, e.g., by ignoring `pager.<cmd>` for
+ *	certain subcommands.
+ *
  * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
  *
  * Additionally, if `foo` is a new command, there are 4 more things to do:
diff --git a/git.c b/git.c
index 489aab4d8..79195ebbd 100644
--- a/git.c
+++ b/git.c
@@ -283,6 +283,7 @@ static int handle_alias(int *argcp, const char ***argv)
  */
 #define NEED_WORK_TREE		(1<<3)
 #define SUPPORT_SUPER_PREFIX	(1<<4)
+#define DELAY_PAGER_CONFIG	(1<<5)
 
 struct cmd_struct {
 	const char *cmd;
@@ -306,7 +307,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
 
-		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
+		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
+		    !(p->option & DELAY_PAGER_CONFIG))
 			use_pager = check_pager_config(p->cmd);
 		if (use_pager == -1 && p->option & USE_PAGER)
 			use_pager = 1;
-- 
2.14.0.rc0

[PATCH v2 05/10] git.c: provide setup_auto_pager()

[Martin Ågren]

The previous patch introduced a way for builtins to declare that they
will take responsibility for handling the `pager.foo`-config item. (See
the commit message of that patch for why that could be useful.)

Provide setup_auto_pager(), which builtins can call in order to handle
`pager.<cmd>`, including possibly starting the pager. Make this function
don't do anything if a pager has already been started, as indicated by
use_pager or pager_in_use().

Whenever this function is called from a builtin, git.c will already have
called commit_pager_choice(). Since commit_pager_choice() treats the
special value -1 as "punt" or "not yet decided", it is not a problem
that we might end up calling commit_pager_choice() once in git.c and
once (or more) in the builtin. Make the new function use -1 in the same
way and document it as "punt".

Don't add any users of setup_auto_pager just yet, one will follow in
a later patch.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h |  9 +++++++++
 git.c     | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/builtin.h b/builtin.h
index 4186635de..3ca4a53a8 100644
--- a/builtin.h
+++ b/builtin.h
@@ -113,6 +113,15 @@ struct fmt_merge_msg_opts {
 extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 			 struct fmt_merge_msg_opts *);
 
+/**
+ * If a builtin has DELAY_PAGER_CONFIG set, the builtin should call this early
+ * when it wishes to respect the `pager.foo`-config. The `cmd` is the name of
+ * the builtin, e.g., "foo". If a paging-choice has already been setup, this
+ * does nothing. The default in `def` should be 0 for "pager off", 1 for "pager
+ * on" or -1 for "punt".
+ */
+extern void setup_auto_pager(const char *cmd, int def);
+
 extern int is_builtin(const char *s);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index 79195ebbd..66832f232 100644
--- a/git.c
+++ b/git.c
@@ -33,6 +33,16 @@ static void commit_pager_choice(void) {
 	}
 }
 
+void setup_auto_pager(const char *cmd, int def)
+{
+	if (use_pager != -1 || pager_in_use())
+		return;
+	use_pager = check_pager_config(cmd);
+	if (use_pager == -1)
+		use_pager = def;
+	commit_pager_choice();
+}
+
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
 	const char **orig_argv = *argv;
-- 
2.14.0.rc0

[PATCH v2 06/10] t7006: add tests for how git tag paginates

[Martin Ågren]

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`.

Since we're about to change how `git tag` respects `pager.tag`, add tests
around this, including how the configuration is ignored if --no-pager or
--paginate are used.

Construct tests with a few different subcommands. First, use -l. Second,
use "no arguments" and --contains, since those imply -l. (There are
more arguments which imply -l, but using these two should be enough.)
Third, use -a as a representative for "not -l".

Make one of the tests demonstrate the behavior mentioned above, where
`git tag -a` respects `pager.tag`. Actually, the tests use `git tag -am`
so no editor is launched, but that is irrelevant, since we just want to
see whether the pager is used or not.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t7006-pager.sh | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 20b4d83c2..e7430bc93 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,6 +134,74 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
 	}
 '
 
+test_expect_success TTY 'git tag -l defaults to not paging' '
+	rm -f paginated.out &&
+	test_terminal git tag -l &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects pager.tag' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag tag -l &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects --no-pager' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag --no-pager tag -l &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag with no args defaults to not paging' '
+	# no args implies -l so this should page like -l
+	rm -f paginated.out &&
+	test_terminal git tag &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag with no args respects pager.tag' '
+	# no args implies -l so this should page like -l
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag tag &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git tag --contains defaults to not paging' '
+	# --contains implies -l so this should page like -l
+	rm -f paginated.out &&
+	test_terminal git tag --contains &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag --contains respects pager.tag' '
+	# --contains implies -l so this should page like -l
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag tag --contains &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a defaults to not paging' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git tag -am message newtag &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a respects pager.tag' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag tag -am message newtag &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a respects --paginate' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag=false --paginate \
+		tag -am message newtag &&
+	test -e paginated.out
+'
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
-- 
2.14.0.rc0

[PATCH v2 07/10] tag: handle `pager.tag`-configuration within the builtin

[Martin Ågren]

Use the mechanisms introduced in two earlier patches to ignore
`pager.tag` in git.c and let the `git tag` builtin handle it on its own.

This is in preparation for the next patch, where we will want to ignore
`pager.tag` unless we run in list-mode. For this reason, place the call
to setup_auto_pager() after the options have been parsed and we have
decided whether we are in list-mode.

No functional change is intended. That said, there is a window between
where the pager is started before and after this patch, and if an error
occurs within this window, as of this patch the error message might not
be paged where it would have been paged before. Since
operation-parsing has to happen inside this window, a difference can be
seen with, e.g., `git -c pager.tag="echo pager is used" tag
--unknown-option`. This change in paging-behavior should be acceptable
since it only affects erroneous usages.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/tag.c | 2 ++
 git.c         | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 01154ea8d..9eda434fb 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -461,6 +461,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			cmdmode = 'l';
 	}
 
+	setup_auto_pager("tag", 0);
+
 	if ((create_tag_object || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
 
diff --git a/git.c b/git.c
index 66832f232..82ac2a092 100644
--- a/git.c
+++ b/git.c
@@ -466,7 +466,7 @@ static struct cmd_struct commands[] = {
 	{ "stripspace", cmd_stripspace },
 	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},
 	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
-	{ "tag", cmd_tag, RUN_SETUP },
+	{ "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },
 	{ "unpack-file", cmd_unpack_file, RUN_SETUP },
 	{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
 	{ "update-index", cmd_update_index, RUN_SETUP },
-- 
2.14.0.rc0

[PATCH v2 08/10] tag: respect `pager.tag` in list-mode only

[Martin Ågren]

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`.

Teach git tag to only respect `pager.tag` when running in list-mode.
Update the documentation and update tests.

If an alias is used to run `git tag -a`, then `pager.tag` will still be
respected. Document this known breakage. It will be fixed in a later
commit. Add a similar test for `-l`, which works as it should.

Noticed-by: Anatoly Borodin <anatoly.borodin@gmail.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  3 +++
 t/t7006-pager.sh          | 25 +++++++++++++++++++++----
 builtin/tag.c             |  3 ++-
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 1eb15afa1..875d135e0 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -205,6 +205,9 @@ it in the repository configuration as follows:
     signingKey = <gpg-keyid>
 -------------------------------------
 
+`pager.tag` is only respected when listing tags, i.e., when `-l` is
+used or implied.
+See linkgit:git-config[1].
 
 DISCUSSION
 ----------
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index e7430bc93..a357436e1 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -187,18 +187,35 @@ test_expect_success TTY 'git tag -a defaults to not paging' '
 	! test -e paginated.out
 '
 
-test_expect_success TTY 'git tag -a respects pager.tag' '
+test_expect_success TTY 'git tag -a ignores pager.tag' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag tag -am message newtag &&
-	test -e paginated.out
+	! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -a respects --paginate' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag=false --paginate \
-		tag -am message newtag &&
+	test_terminal git --paginate tag -am message newtag &&
+	test -e paginated.out
+'
+
+test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
+	# git-tag will be launched as a dashed external, which
+	# 1) is the source of a potential bug, and
+	# 2) is why we use test_config and not -c.
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_config pager.tag true &&
+	test_terminal git -c alias.t=tag t -am message newtag &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
+	rm -f paginated.out &&
+	test_config pager.tag true &&
+	test_terminal git -c alias.t=tag t -l &&
 	test -e paginated.out
 '
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 9eda434fb..5ad1af252 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -461,7 +461,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			cmdmode = 'l';
 	}
 
-	setup_auto_pager("tag", 0);
+	if (cmdmode == 'l')
+		setup_auto_pager("tag", 0);
 
 	if ((create_tag_object || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
-- 
2.14.0.rc0

[PATCH v2 09/10] tag: change default of `pager.tag` to "on"

[Martin Ågren]

The previous patch taught `git tag` to only respect `pager.tag` in
list-mode. That patch left the default value of `pager.tag` at "off".

After that patch, it makes sense to let the default value be "on"
instead, since it will help with listing many tags, but will not hurt
users of `git tag -a` as it would have before. Make that change. Update
documentation and tests.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  2 +-
 t/t7006-pager.sh          | 28 ++++++++++++++--------------
 builtin/tag.c             |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 875d135e0..d97aad343 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -206,7 +206,7 @@ it in the repository configuration as follows:
 -------------------------------------
 
 `pager.tag` is only respected when listing tags, i.e., when `-l` is
-used or implied.
+used or implied. The default is to use a pager.
 See linkgit:git-config[1].
 
 DISCUSSION
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index a357436e1..df258c5d4 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,16 +134,16 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
 	}
 '
 
-test_expect_success TTY 'git tag -l defaults to not paging' '
+test_expect_success TTY 'git tag -l defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git tag -l &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects pager.tag' '
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag tag -l &&
-	test -e paginated.out
+	test_terminal git -c pager.tag=false tag -l &&
+	! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects --no-pager' '
@@ -152,32 +152,32 @@ test_expect_success TTY 'git tag -l respects --no-pager' '
 	! test -e paginated.out
 '
 
-test_expect_success TTY 'git tag with no args defaults to not paging' '
+test_expect_success TTY 'git tag with no args defaults to paging' '
 	# no args implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git tag &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 test_expect_success TTY 'git tag with no args respects pager.tag' '
 	# no args implies -l so this should page like -l
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag tag &&
-	test -e paginated.out
+	test_terminal git -c pager.tag=no tag &&
+	! test -e paginated.out
 '
 
-test_expect_success TTY 'git tag --contains defaults to not paging' '
+test_expect_success TTY 'git tag --contains defaults to paging' '
 	# --contains implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git tag --contains &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 test_expect_success TTY 'git tag --contains respects pager.tag' '
 	# --contains implies -l so this should page like -l
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag tag --contains &&
-	test -e paginated.out
+	test_terminal git -c pager.tag=false tag --contains &&
+	! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -a defaults to not paging' '
@@ -214,9 +214,9 @@ test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
 
 test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
 	rm -f paginated.out &&
-	test_config pager.tag true &&
+	test_config pager.tag false &&
 	test_terminal git -c alias.t=tag t -l &&
-	test -e paginated.out
+	! test -e paginated.out
 '
 
 # A colored commit log will begin with an appropriate ANSI escape
diff --git a/builtin/tag.c b/builtin/tag.c
index 5ad1af252..ea83df5e1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -462,7 +462,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 
 	if (cmdmode == 'l')
-		setup_auto_pager("tag", 0);
+		setup_auto_pager("tag", 1);
 
 	if ((create_tag_object || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
-- 
2.14.0.rc0

[PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external

[Martin Ågren]

When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and
execute `git-foo` as a dashed external. This is true even if git foo is
a builtin. That is on purpose, and is motivated in a comment which was
added in commit 441981bc ("git: simplify environment save/restore
logic", 2016-01-26).

Shortly before we launch a dashed external, and unless we have already
found out whether we should use a pager, we check `pager.foo`. This was
added in commit 92058e4d ("support pager.* for external commands",
2011-08-18). If the dashed external is a builtin, this does not match
that commit's intention and is arguably wrong, since it would be cleaner
if we let the "dashed external builtin" handle `pager.foo`.

This has not mattered in practice, but a recent patch taught `git-tag`
to ignore `pager.tag` under certain circumstances. But, when started
using an alias, it doesn't get the chance to do so, as outlined above.
That recent patch added a test to document this breakage.

Do not check `pager.foo` before launching a builtin as a dashed
external, i.e., if we recognize the name of the external as a builtin.
Change the test to use `test_expect_success`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
One could address this in run_argv(), by making the second call to
execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
would be started as "git foo". (Possibly after unrolling and cleaning up
the "while (1)"-loop.) That seems like the wrong fix for this particular
issue, but might be a wanted change on its own -- or maybe not --, since
it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
only for builtins...).

 t/t7006-pager.sh | 2 +-
 git.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index df258c5d4..8b2ffb1aa 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -201,7 +201,7 @@ test_expect_success TTY 'git tag -a respects --paginate' '
 	test -e paginated.out
 '
 
-test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
+test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
 	# git-tag will be launched as a dashed external, which
 	# 1) is the source of a potential bug, and
 	# 2) is why we use test_config and not -c.
diff --git a/git.c b/git.c
index 82ac2a092..6b6d9f68e 100644
--- a/git.c
+++ b/git.c
@@ -559,7 +559,7 @@ static void execv_dashed_external(const char **argv)
 	if (get_super_prefix())
 		die("%s doesn't support --super-prefix", argv[0]);
 
-	if (use_pager == -1)
+	if (use_pager == -1 && !is_builtin(argv[0]))
 		use_pager = check_pager_config(argv[0]);
 	commit_pager_choice();
 
-- 
2.14.0.rc0

Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> After that feedback, v2 drops `pager.tag.list` and instead teaches
> `git tag` to only consider `pager.tag` in list-mode, as suggested by
> Peff.

That does sound like a more sensible and safer approach.  I may have
comments on individual patches, which I will send while I read them.

Thanks.

Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> This is the second version of "[PATCH 0/7] tag: more fine-grained
> pager-configuration" [1]. That series introduced `pager.tag.list` to
> address the fact that `pager.tag` can be useful with `git tag -l` but
> actively hostile with `git tag -a`. Thanks to Junio, Peff and Brandon
> for helpful feedback.
>
> After that feedback, v2 drops `pager.tag.list` and instead teaches
> `git tag` to only consider `pager.tag` in list-mode, as suggested by
> Peff.
>
> Patches 1-3/10 replace patch 1/7. They move Documentation/technical/
> api-builtin.txt into builtin.h, tweak the formatting and bring it up to
> date. I may have gone overboard making this 3 patches...
>
> Patches 4-7/10 correspond to patches 2-5/7. `setup_auto_pager()' is now
> much simpler since we do not need to handle "tag.list" with a clever
> fallback strategy. IGNORE_PAGER_CONFIG is now called DELAY_PAGER_CONFIG.
> I now check with pager_in_use() and I moved the handling of `pager.tag`
> a bit further down.

I tend to agree with you that 1-3/10 may be better off being a
single patch (or 3/10 dropped, as Brandon is working on losing it
nearby).  I would have expected 7-8/10 to be a single patch, as by
the time a reader reaches 07/10, because of the groundwork laid by
04-06/10, it is obvious that the general direction is to allow the
caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
some but not all circumstances, and 07/10 being faithful to the
original behaviour (only to be updated in 08/10) is somewhat counter
intuitive.  It is not wrong per-se; it was just unexpected. 

> Patches 8-9/10 teach `git tag` to only respect `pager.tag` in list-mode
> and flip the default value for that config to "on".
>
> Patch 10/10 is somewhat similar to a hunk in patch 2/7, but is now a
> bug-fix instead of a feature. It teaches `execv_dashed_external()` not
> to check `pager.foo` when launching `git-foo` where foo is a builtin.
> I waffled about where to put this patch. Putting it earlier in the
> series as a preparatory step, I couldn't come up with a way of writing a
> test. So patch 8/10 introduces a `test_expect_failure` which this patch
> then fixes.

I haven't thought about ramifications of 9-10/10 to make a comment
yet, but overall the series was a pleasant read.

Thanks.

Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode

[Martin Ågren]

On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
> I tend to agree with you that 1-3/10 may be better off being a
> single patch (or 3/10 dropped, as Brandon is working on losing it
> nearby).  I would have expected 7-8/10 to be a single patch, as by
> the time a reader reaches 07/10, because of the groundwork laid by
> 04-06/10, it is obvious that the general direction is to allow the
> caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
> some but not all circumstances, and 07/10 being faithful to the
> original behaviour (only to be updated in 08/10) is somewhat counter
> intuitive.  It is not wrong per-se; it was just unexpected.

Thanks for your comments. I will be away for a few days, but once I
get back, I'll try to produce a v3 based on this and any further
feedback.

Martin

Re: [PATCH v2 05/10] git.c: provide setup_auto_pager()

[Jeff King]

On Mon, Jul 17, 2017 at 10:10:47PM +0200, Martin Ågren wrote:

> The previous patch introduced a way for builtins to declare that they
> will take responsibility for handling the `pager.foo`-config item. (See
> the commit message of that patch for why that could be useful.)
> 
> Provide setup_auto_pager(), which builtins can call in order to handle
> `pager.<cmd>`, including possibly starting the pager. Make this function
> don't do anything if a pager has already been started, as indicated by
> use_pager or pager_in_use().
> 
> Whenever this function is called from a builtin, git.c will already have
> called commit_pager_choice(). Since commit_pager_choice() treats the
> special value -1 as "punt" or "not yet decided", it is not a problem
> that we might end up calling commit_pager_choice() once in git.c and
> once (or more) in the builtin. Make the new function use -1 in the same
> way and document it as "punt".

At first I wasn't sure if it would ever make sense to use "-1" here. The
"punt" that happens in earlier calls to commit_pager_choice() is there
because we might adjust our decision later. And this would generally be
the final decision, I would think. So I'd be surprised if we had
anything besides "0" or "1" in the "def" argument.

But thinking on it, the most plausible case is something like:

  setup_auto_pager("foo", -1);
  ...
  /* fallback to some historical compatibility name */
  setup_auto_pager("bar", 0);

And it's important for the "-1" there to be a true punt, and not do
anything in commit_pager_choice(). So it's probably worth documenting
the "-1" behavior as you did here as a possible value for "def".

-Peff

Re: [PATCH v2 06/10] t7006: add tests for how git tag paginates

[Jeff King]

On Mon, Jul 17, 2017 at 10:10:48PM +0200, Martin Ågren wrote:

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index 20b4d83c2..e7430bc93 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -134,6 +134,74 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
>  	}
>  '
>  
> +test_expect_success TTY 'git tag -l defaults to not paging' '
> +	rm -f paginated.out &&
> +	test_terminal git tag -l &&
> +	! test -e paginated.out
> +'

I don't mind an expect_success like this, where it documents a sane and
existing default, even if we're going to flip that default later in the
series.

But here...

> +test_expect_success TTY 'git tag -a respects pager.tag' '
> +	test_when_finished "git tag -d newtag" &&
> +	rm -f paginated.out &&
> +	test_terminal git -c pager.tag tag -am message newtag &&
> +	test -e paginated.out
> +'

I think this behavior is just buggy, and it might be better introduced
as a test_expect_failure on "git tag -a does not respect pager.tag".

Kind of a minor nit, as the series should end up in the right place
either way, but it can be helpful if you end up digging back in history
to the introduction of the test.

-Peff

Re: [PATCH v2 08/10] tag: respect `pager.tag` in list-mode only

[Jeff King]

On Mon, Jul 17, 2017 at 10:10:50PM +0200, Martin Ågren wrote:

>  test_expect_success TTY 'git tag -a respects --paginate' '
>  	test_when_finished "git tag -d newtag" &&
>  	rm -f paginated.out &&
> -	test_terminal git -c pager.tag=false --paginate \
> -		tag -am message newtag &&
> +	test_terminal git --paginate tag -am message newtag &&
> +	test -e paginated.out
> +'

This changes, I guess, because pager.tag should not be having any impact
at all. So it would not hurt to leave it, but the in the final state of
the test what is interesting is that "--paginate" kicks in. I do wonder
if it could just drop the pager.tag bit when it is added in the first
place. We test elsewhere that the command-line overrides the config (for
a case that actually _does_ respect the config).

I'm OK with it either way, though.

-Peff

Re: [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external

[Jeff King]

On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:

> When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and
> execute `git-foo` as a dashed external. This is true even if git foo is
> a builtin. That is on purpose, and is motivated in a comment which was
> added in commit 441981bc ("git: simplify environment save/restore
> logic", 2016-01-26).
> 
> Shortly before we launch a dashed external, and unless we have already
> found out whether we should use a pager, we check `pager.foo`. This was
> added in commit 92058e4d ("support pager.* for external commands",
> 2011-08-18). If the dashed external is a builtin, this does not match
> that commit's intention and is arguably wrong, since it would be cleaner
> if we let the "dashed external builtin" handle `pager.foo`.
> 
> This has not mattered in practice, but a recent patch taught `git-tag`
> to ignore `pager.tag` under certain circumstances. But, when started
> using an alias, it doesn't get the chance to do so, as outlined above.
> That recent patch added a test to document this breakage.
> 
> Do not check `pager.foo` before launching a builtin as a dashed
> external, i.e., if we recognize the name of the external as a builtin.
> Change the test to use `test_expect_success`.

I think floating this change to the end like this has made it much
easier to see why we must do it. The end result looks good to me.

> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> One could address this in run_argv(), by making the second call to
> execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
> would be started as "git foo". (Possibly after unrolling and cleaning up
> the "while (1)"-loop.) That seems like the wrong fix for this particular
> issue, but might be a wanted change on its own -- or maybe not --, since
> it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
> only for builtins...).

We shouldn't need to relay them. They get added to the environment by
the initial "git" invocation, and then are available everywhere (in
fact, it would be wrong to relay them for multi-valued config). Or did
you mean that we could potentially allow:

  [alias]
  foo = "-c baz some-builtin"

That's interesting, but I think the fact that it only works with
builtins makes it a bad idea. And you can always do:

  [alias]
  foo = "!git -c baz some-builtin"

which is equivalent.

-Peff

Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode

[Jeff King]

On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:

> On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
> > I tend to agree with you that 1-3/10 may be better off being a
> > single patch (or 3/10 dropped, as Brandon is working on losing it
> > nearby).  I would have expected 7-8/10 to be a single patch, as by
> > the time a reader reaches 07/10, because of the groundwork laid by
> > 04-06/10, it is obvious that the general direction is to allow the
> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
> > some but not all circumstances, and 07/10 being faithful to the
> > original behaviour (only to be updated in 08/10) is somewhat counter
> > intuitive.  It is not wrong per-se; it was just unexpected.
> 
> Thanks for your comments. I will be away for a few days, but once I
> get back, I'll try to produce a v3 based on this and any further
> feedback.

Overall it looks good to me. I left a few minor comments.

I actually like the split. I found it pretty easy to follow (though
squashing as Junio suggested would be fine with me, too).

Thanks again for working on this.

-Peff

Re: [PATCH v2 05/10] git.c: provide setup_auto_pager()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But thinking on it, the most plausible case is something like:
>
>   setup_auto_pager("foo", -1);
>   ...
>   /* fallback to some historical compatibility name */
>   setup_auto_pager("bar", 0);
>
> And it's important for the "-1" there to be a true punt, and not do
> anything in commit_pager_choice(). So it's probably worth documenting
> the "-1" behavior as you did here as a possible value for "def".

Thanks for reading it over. I agree that the "punt" behaviour is a
sensible one in that use case, and I also agree that it would be
good to explain it.

Re: [PATCH v2 06/10] t7006: add tests for how git tag paginates

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But here...
>
>> +test_expect_success TTY 'git tag -a respects pager.tag' '
>> +	test_when_finished "git tag -d newtag" &&
>> +	rm -f paginated.out &&
>> +	test_terminal git -c pager.tag tag -am message newtag &&
>> +	test -e paginated.out
>> +'
>
> I think this behavior is just buggy, and it might be better introduced
> as a test_expect_failure on "git tag -a does not respect pager.tag".
>
> Kind of a minor nit, as the series should end up in the right place
> either way, but it can be helpful if you end up digging back in history
> to the introduction of the test.

Yes, I think that is essentially the same reaction I had to patches
7 and 8, where it carries the "buggy" behaviour forward and then
fixes it.  The way the series lays groundwork to introduce a
mechanism that can be used to address this behaviour in its earlier
patches strongly suggests to the users that this is considered a bug
by the author of the series to the user from early on, so adding
this as "expect failure" and then flip it to "expect success" when
the bug is fixed would be a more natural sequence of changes.

Thanks.

Re: [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external

[Martin Ågren]

On 31 July 2017 at 05:45, Jeff King <peff@peff.net> wrote:
> On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:
>
>> One could address this in run_argv(), by making the second call to
>> execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
>> would be started as "git foo". (Possibly after unrolling and cleaning up
>> the "while (1)"-loop.) That seems like the wrong fix for this particular
>> issue, but might be a wanted change on its own -- or maybe not --, since
>> it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
>> only for builtins...).
>
> We shouldn't need to relay them. They get added to the environment by
> the initial "git" invocation, and then are available everywhere (in
> fact, it would be wrong to relay them for multi-valued config).

Thanks for explaining. I did some very sloppy reading of the comment
in git.c that we "cannot take flags in between the 'git' and the
'xxxx'" which I somehow misunderstood completely as "we cannot pass
that sort of information to git-xxxx". Silly. Thanks for taking the
time to explain what I should have found out myself...

So yeah, I meant the above and not this:

> Or did
> you mean that we could potentially allow:
>
>   [alias]
>   foo = "-c baz some-builtin"
>
> That's interesting, but I think the fact that it only works with
> builtins makes it a bad idea. And you can always do:
>
>   [alias]
>   foo = "!git -c baz some-builtin"
>
> which is equivalent.

Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode

[Martin Ågren]

On 31 July 2017 at 05:46, Jeff King <peff@peff.net> wrote:
> On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:
>
>> On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
>> > I tend to agree with you that 1-3/10 may be better off being a
>> > single patch (or 3/10 dropped, as Brandon is working on losing it
>> > nearby).  I would have expected 7-8/10 to be a single patch, as by
>> > the time a reader reaches 07/10, because of the groundwork laid by
>> > 04-06/10, it is obvious that the general direction is to allow the
>> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
>> > some but not all circumstances, and 07/10 being faithful to the
>> > original behaviour (only to be updated in 08/10) is somewhat counter
>> > intuitive.  It is not wrong per-se; it was just unexpected.
>>
>> Thanks for your comments. I will be away for a few days, but once I
>> get back, I'll try to produce a v3 based on this and any further
>> feedback.
>
> Overall it looks good to me. I left a few minor comments.
>
> I actually like the split. I found it pretty easy to follow (though
> squashing as Junio suggested would be fine with me, too).

I assume that by "the split" you mean patches 7-8, not 1-3.  Anyway,
I'll squash since you're fine with it and Junio prefers it.

Martin

Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode

[Jeff King]

On Mon, Jul 31, 2017 at 07:44:27PM +0200, Martin Ågren wrote:

> On 31 July 2017 at 05:46, Jeff King <peff@peff.net> wrote:
> > On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:
> >
> >> On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
> >> > I tend to agree with you that 1-3/10 may be better off being a
> >> > single patch (or 3/10 dropped, as Brandon is working on losing it
> >> > nearby).  I would have expected 7-8/10 to be a single patch, as by
> >> > the time a reader reaches 07/10, because of the groundwork laid by
> >> > 04-06/10, it is obvious that the general direction is to allow the
> >> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
> >> > some but not all circumstances, and 07/10 being faithful to the
> >> > original behaviour (only to be updated in 08/10) is somewhat counter
> >> > intuitive.  It is not wrong per-se; it was just unexpected.
> >>
> >> Thanks for your comments. I will be away for a few days, but once I
> >> get back, I'll try to produce a v3 based on this and any further
> >> feedback.
> >
> > Overall it looks good to me. I left a few minor comments.
> >
> > I actually like the split. I found it pretty easy to follow (though
> > squashing as Junio suggested would be fine with me, too).
> 
> I assume that by "the split" you mean patches 7-8, not 1-3.  Anyway,
> I'll squash since you're fine with it and Junio prefers it.

I actually meant both, but as I said, I'm OK with it either way.

-Peff

Re: [PATCH v2 06/10] t7006: add tests for how git tag paginates

[Martin Ågren]

On 31 July 2017 at 18:37, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> But here...
>>
>>> +test_expect_success TTY 'git tag -a respects pager.tag' '
>>> +    test_when_finished "git tag -d newtag" &&
>>> +    rm -f paginated.out &&
>>> +    test_terminal git -c pager.tag tag -am message newtag &&
>>> +    test -e paginated.out
>>> +'
>>
>> I think this behavior is just buggy, and it might be better introduced
>> as a test_expect_failure on "git tag -a does not respect pager.tag".
>>
>> Kind of a minor nit, as the series should end up in the right place
>> either way, but it can be helpful if you end up digging back in history
>> to the introduction of the test.
>
> Yes, I think that is essentially the same reaction I had to patches
> 7 and 8, where it carries the "buggy" behaviour forward and then
> fixes it.  The way the series lays groundwork to introduce a
> mechanism that can be used to address this behaviour in its earlier
> patches strongly suggests to the users that this is considered a bug
> by the author of the series to the user from early on, so adding
> this as "expect failure" and then flip it to "expect success" when
> the bug is fixed would be a more natural sequence of changes.

Thanks both for very helpful comments. I admit I viewed it less as
"fix buggy behavior" and more like "redefine wanted behavior". So I
wanted to postpone the redefinition of the behavior until all the
restructuring was done. Looking at this as a bug-fix does make
carefully moving the bug forward look rather silly.

I haven't responded to each of your suggestions individually where the
answers would have been a mere "thanks, will do". They're still much
appreciated and will help make v3 much better. Thanks.

Martin

Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 31, 2017 at 07:44:27PM +0200, Martin Ågren wrote:
>
>> On 31 July 2017 at 05:46, Jeff King <peff@peff.net> wrote:
>> > On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:
>> >
>> >> On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
>> >> > I tend to agree with you that 1-3/10 may be better off being a
>> >> > single patch (or 3/10 dropped, as Brandon is working on losing it
>> >> > nearby).  I would have expected 7-8/10 to be a single patch, as by
>> >> > the time a reader reaches 07/10, because of the groundwork laid by
>> >> > 04-06/10, it is obvious that the general direction is to allow the
>> >> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
>> >> > some but not all circumstances, and 07/10 being faithful to the
>> >> > original behaviour (only to be updated in 08/10) is somewhat counter
>> >> > intuitive.  It is not wrong per-se; it was just unexpected.
>> >>
>> >> Thanks for your comments. I will be away for a few days, but once I
>> >> get back, I'll try to produce a v3 based on this and any further
>> >> feedback.
>> >
>> > Overall it looks good to me. I left a few minor comments.
>> >
>> > I actually like the split. I found it pretty easy to follow (though
>> > squashing as Junio suggested would be fine with me, too).
>> 
>> I assume that by "the split" you mean patches 7-8, not 1-3.  Anyway,
>> I'll squash since you're fine with it and Junio prefers it.
>
> I actually meant both, but as I said, I'm OK with it either way.

I am OK with it either way, too ;-)

[PATCH v3 0/7] tag: only respect `pager.tag` in list-mode

[Martin Ågren]

This is the third version of my attempt to make `pager tag` useful (v1
at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.

I've squashed patches 01-03/10 and 07-08/10, respectively. The interdiff
is below. I managed to clean up some tests thanks to a drive-by comment
by Peff which cleared up a misunderstanding I had. Some minor changes,
e.g., I write "built-in" instead of "builtin", since that seemed
preferred, at least locally in builtin.h. I documented why a default
value of "punt" could be useful, but also that it's probably not wanted.

Martin

[1] https://public-inbox.org/git/cover.1499723297.git.martin.agren@gmail.com/T/

[2] https://public-inbox.org/git/cover.1500321657.git.martin.agren@gmail.com/T/#u

Martin Ågren (7):
  builtin.h: take over documentation from api-builtin.txt
  git.c: let builtins opt for handling `pager.foo` themselves
  git.c: provide setup_auto_pager()
  t7006: add tests for how git tag paginates
  tag: respect `pager.tag` in list-mode only
  tag: change default of `pager.tag` to "on"
  git.c: ignore pager.* when launching builtin as dashed external

 Documentation/git-tag.txt               |   3 +
 Documentation/technical/api-builtin.txt |  73 -----------------------
 t/t7006-pager.sh                        |  80 +++++++++++++++++++++++++
 builtin.h                               | 100 ++++++++++++++++++++++++++++++++
 builtin/tag.c                           |   3 +
 git.c                                   |  18 +++++-
 6 files changed, 201 insertions(+), 76 deletions(-)
 delete mode 100644 Documentation/technical/api-builtin.txt

Interdiff vs v2:
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 8b2ffb1aa..9128ec5ac 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '
 test_expect_success TTY 'git tag with no args respects pager.tag' '
 	# no args implies -l so this should page like -l
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag=no tag &&
+	test_terminal git -c pager.tag=false tag &&
 	! test -e paginated.out
 '
 
@@ -202,20 +202,15 @@ test_expect_success TTY 'git tag -a respects --paginate' '
 '
 
 test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
-	# git-tag will be launched as a dashed external, which
-	# 1) is the source of a potential bug, and
-	# 2) is why we use test_config and not -c.
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
-	test_config pager.tag true &&
-	test_terminal git -c alias.t=tag t -am message newtag &&
+	test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
 	! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
 	rm -f paginated.out &&
-	test_config pager.tag false &&
-	test_terminal git -c alias.t=tag t -l &&
+	test_terminal git -c pager.tag=false -c alias.t=tag t -l &&
 	! test -e paginated.out
 '
 
diff --git a/builtin.h b/builtin.h
index 3ca4a53a8..42378f3aa 100644
--- a/builtin.h
+++ b/builtin.h
@@ -51,15 +51,15 @@
  *	on bare repositories.
  *	This only makes sense when `RUN_SETUP` is also set.
  *
- * `SUPPORT_SUPER_PREFIX`::
+ * `SUPPORT_SUPER_PREFIX`:
  *
- *	The builtin supports `--super-prefix`.
+ *	The built-in supports `--super-prefix`.
  *
- * `DELAY_PAGER_CONFIG`::
+ * `DELAY_PAGER_CONFIG`:
  *
  *	If RUN_SETUP or RUN_SETUP_GENTLY is set, git.c normally handles
  *	the `pager.<cmd>`-configuration. If this flag is used, git.c
- *	will skip that step, instead allowing the builtin to make a
+ *	will skip that step, instead allowing the built-in to make a
  *	more informed decision, e.g., by ignoring `pager.<cmd>` for
  *	certain subcommands.
  *
@@ -114,11 +114,14 @@ extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 			 struct fmt_merge_msg_opts *);
 
 /**
- * If a builtin has DELAY_PAGER_CONFIG set, the builtin should call this early
+ * If a built-in has DELAY_PAGER_CONFIG set, the built-in should call this early
  * when it wishes to respect the `pager.foo`-config. The `cmd` is the name of
- * the builtin, e.g., "foo". If a paging-choice has already been setup, this
+ * the built-in, e.g., "foo". If a paging-choice has already been setup, this
  * does nothing. The default in `def` should be 0 for "pager off", 1 for "pager
  * on" or -1 for "punt".
+ *
+ * You should most likely use a default of 0 or 1. "Punt" (-1) could be useful
+ * to be able to fall back to some historical compatibility name.
  */
 extern void setup_auto_pager(const char *cmd, int def);
 
-- 
2.14.0.rc1.12.ge2d9c4613

[PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt

[Martin Ågren]

Delete Documentation/technical/api-builtin.txt and move its content
into builtin.h. Format it as a comment. Remove a '+' which was needed
when the information was formatted for AsciiDoc. Similarly, change
"::" to ":".

Document SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to
date with the available flags.

While at it, correct '3 more things to do' to '4 more things to do'.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
I'm still documenting SUPPORT_SUPER_PREFIX. As Junio pointed out, a
nearby patch series is working on losing it (or one user of it). I tried
removing that part, but felt like I was leaving things incomplete.

 Documentation/technical/api-builtin.txt | 73 ------------------------------
 builtin.h                               | 80 +++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 73 deletions(-)
 delete mode 100644 Documentation/technical/api-builtin.txt

diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
deleted file mode 100644
index 22a39b929..000000000
--- a/Documentation/technical/api-builtin.txt
+++ /dev/null
@@ -1,73 +0,0 @@
-builtin API
-===========
-
-Adding a new built-in
----------------------
-
-There are 4 things to do to add a built-in command implementation to
-Git:
-
-. Define the implementation of the built-in command `foo` with
-  signature:
-
-	int cmd_foo(int argc, const char **argv, const char *prefix);
-
-. Add the external declaration for the function to `builtin.h`.
-
-. Add the command to the `commands[]` table defined in `git.c`.
-  The entry should look like:
-
-	{ "foo", cmd_foo, <options> },
-+
-where options is the bitwise-or of:
-
-`RUN_SETUP`::
-	If there is not a Git directory to work on, abort.  If there
-	is a work tree, chdir to the top of it if the command was
-	invoked in a subdirectory.  If there is no work tree, no
-	chdir() is done.
-
-`RUN_SETUP_GENTLY`::
-	If there is a Git directory, chdir as per RUN_SETUP, otherwise,
-	don't chdir anywhere.
-
-`USE_PAGER`::
-
-	If the standard output is connected to a tty, spawn a pager and
-	feed our output to it.
-
-`NEED_WORK_TREE`::
-
-	Make sure there is a work tree, i.e. the command cannot act
-	on bare repositories.
-	This only makes sense when `RUN_SETUP` is also set.
-
-. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
-
-Additionally, if `foo` is a new command, there are 3 more things to do:
-
-. Add tests to `t/` directory.
-
-. Write documentation in `Documentation/git-foo.txt`.
-
-. Add an entry for `git-foo` to `command-list.txt`.
-
-. Add an entry for `/git-foo` to `.gitignore`.
-
-
-How a built-in is called
-------------------------
-
-The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
-and `prefix`.  The first two are similar to what `main()` of a
-standalone command would be called with.
-
-When `RUN_SETUP` is specified in the `commands[]` table, and when you
-were started from a subdirectory of the work tree, `cmd_foo()` is called
-after chdir(2) to the top of the work tree, and `prefix` gets the path
-to the subdirectory the command started from.  This allows you to
-convert a user-supplied pathname (typically relative to that directory)
-to a pathname relative to the top of the work tree.
-
-The return value from `cmd_foo()` becomes the exit status of the
-command.
diff --git a/builtin.h b/builtin.h
index 498ac80d0..8d87d06da 100644
--- a/builtin.h
+++ b/builtin.h
@@ -6,6 +6,86 @@
 #include "cache.h"
 #include "commit.h"
 
+/*
+ * builtin API
+ * ===========
+ *
+ * Adding a new built-in
+ * ---------------------
+ *
+ * There are 4 things to do to add a built-in command implementation to
+ * Git:
+ *
+ * . Define the implementation of the built-in command `foo` with
+ *   signature:
+ *
+ *	int cmd_foo(int argc, const char **argv, const char *prefix);
+ *
+ * . Add the external declaration for the function to `builtin.h`.
+ *
+ * . Add the command to the `commands[]` table defined in `git.c`.
+ *   The entry should look like:
+ *
+ *	{ "foo", cmd_foo, <options> },
+ *
+ * where options is the bitwise-or of:
+ *
+ * `RUN_SETUP`:
+ *	If there is not a Git directory to work on, abort.  If there
+ *	is a work tree, chdir to the top of it if the command was
+ *	invoked in a subdirectory.  If there is no work tree, no
+ *	chdir() is done.
+ *
+ * `RUN_SETUP_GENTLY`:
+ *	If there is a Git directory, chdir as per RUN_SETUP, otherwise,
+ *	don't chdir anywhere.
+ *
+ * `USE_PAGER`:
+ *
+ *	If the standard output is connected to a tty, spawn a pager and
+ *	feed our output to it.
+ *
+ * `NEED_WORK_TREE`:
+ *
+ *	Make sure there is a work tree, i.e. the command cannot act
+ *	on bare repositories.
+ *	This only makes sense when `RUN_SETUP` is also set.
+ *
+ * `SUPPORT_SUPER_PREFIX`:
+ *
+ *	The built-in supports `--super-prefix`.
+ *
+ * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
+ *
+ * Additionally, if `foo` is a new command, there are 4 more things to do:
+ *
+ * . Add tests to `t/` directory.
+ *
+ * . Write documentation in `Documentation/git-foo.txt`.
+ *
+ * . Add an entry for `git-foo` to `command-list.txt`.
+ *
+ * . Add an entry for `/git-foo` to `.gitignore`.
+ *
+ *
+ * How a built-in is called
+ * ------------------------
+ *
+ * The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
+ * and `prefix`.  The first two are similar to what `main()` of a
+ * standalone command would be called with.
+ *
+ * When `RUN_SETUP` is specified in the `commands[]` table, and when you
+ * were started from a subdirectory of the work tree, `cmd_foo()` is called
+ * after chdir(2) to the top of the work tree, and `prefix` gets the path
+ * to the subdirectory the command started from.  This allows you to
+ * convert a user-supplied pathname (typically relative to that directory)
+ * to a pathname relative to the top of the work tree.
+ *
+ * The return value from `cmd_foo()` becomes the exit status of the
+ * command.
+ */
+
 #define DEFAULT_MERGE_LOG_LEN 20
 
 extern const char git_usage_string[];
-- 
2.14.0.rc1.12.ge2d9c4613

[PATCH v3 2/7] git.c: let builtins opt for handling `pager.foo` themselves

[Martin Ågren]

Before launching a builtin git foo and unless mechanisms with precedence
are in use, we check for and handle the `pager.foo` config. This is done
without considering exactly how git foo is being used, and indeed, git.c
cannot (and should not) know what the arguments to git foo are supposed
to achieve.

In practice this means that, 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. Someone who makes use of both `git tag -a` and
`git tag -l` will probably not set `pager.tag`, so that `git tag -a`
will actually work, at the cost of not paging output of `git tag -l`.

To allow individual builtins to make more informed decisions about when
to respect `pager.foo`, introduce a flag DELAY_PAGER_CONFIG. If the flag
is set, do not check `pager.foo`.

Do not check for DELAY_PAGER_CONFIG in `execv_dashed_external()`. That
call site is arguably wrong, although in a way that is not yet visible,
and will be changed in a slightly different direction in a later patch.

Don't add any users of DELAY_PAGER_CONFIG just yet, one will follow in a
later patch.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 8 ++++++++
 git.c     | 4 +++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin.h b/builtin.h
index 8d87d06da..0f3a7b770 100644
--- a/builtin.h
+++ b/builtin.h
@@ -55,6 +55,14 @@
  *
  *	The built-in supports `--super-prefix`.
  *
+ * `DELAY_PAGER_CONFIG`:
+ *
+ *	If RUN_SETUP or RUN_SETUP_GENTLY is set, git.c normally handles
+ *	the `pager.<cmd>`-configuration. If this flag is used, git.c
+ *	will skip that step, instead allowing the built-in to make a
+ *	more informed decision, e.g., by ignoring `pager.<cmd>` for
+ *	certain subcommands.
+ *
  * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
  *
  * Additionally, if `foo` is a new command, there are 4 more things to do:
diff --git a/git.c b/git.c
index 489aab4d8..79195ebbd 100644
--- a/git.c
+++ b/git.c
@@ -283,6 +283,7 @@ static int handle_alias(int *argcp, const char ***argv)
  */
 #define NEED_WORK_TREE		(1<<3)
 #define SUPPORT_SUPER_PREFIX	(1<<4)
+#define DELAY_PAGER_CONFIG	(1<<5)
 
 struct cmd_struct {
 	const char *cmd;
@@ -306,7 +307,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
 
-		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
+		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
+		    !(p->option & DELAY_PAGER_CONFIG))
 			use_pager = check_pager_config(p->cmd);
 		if (use_pager == -1 && p->option & USE_PAGER)
 			use_pager = 1;
-- 
2.14.0.rc1.12.ge2d9c4613

[PATCH v3 3/7] git.c: provide setup_auto_pager()

[Martin Ågren]

The previous patch introduced a way for builtins to declare that they
will take responsibility for handling the `pager.foo`-config item. (See
the commit message of that patch for why that could be useful.)

Provide setup_auto_pager(), which builtins can call in order to handle
`pager.<cmd>`, including possibly starting the pager. Make this function
don't do anything if a pager has already been started, as indicated by
use_pager or pager_in_use().

Whenever this function is called from a builtin, git.c will already have
called commit_pager_choice(). Since commit_pager_choice() treats the
special value -1 as "punt" or "not yet decided", it is not a problem
that we might end up calling commit_pager_choice() once in git.c and
once (or more) in the builtin. Make the new function use -1 in the same
way and document it as "punt".

Don't add any users of setup_auto_pager just yet, one will follow in
a later patch.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin.h | 12 ++++++++++++
 git.c     | 10 ++++++++++
 2 files changed, 22 insertions(+)

diff --git a/builtin.h b/builtin.h
index 0f3a7b770..42378f3aa 100644
--- a/builtin.h
+++ b/builtin.h
@@ -113,6 +113,18 @@ struct fmt_merge_msg_opts {
 extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 			 struct fmt_merge_msg_opts *);
 
+/**
+ * If a built-in has DELAY_PAGER_CONFIG set, the built-in should call this early
+ * when it wishes to respect the `pager.foo`-config. The `cmd` is the name of
+ * the built-in, e.g., "foo". If a paging-choice has already been setup, this
+ * does nothing. The default in `def` should be 0 for "pager off", 1 for "pager
+ * on" or -1 for "punt".
+ *
+ * You should most likely use a default of 0 or 1. "Punt" (-1) could be useful
+ * to be able to fall back to some historical compatibility name.
+ */
+extern void setup_auto_pager(const char *cmd, int def);
+
 extern int is_builtin(const char *s);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index 79195ebbd..66832f232 100644
--- a/git.c
+++ b/git.c
@@ -33,6 +33,16 @@ static void commit_pager_choice(void) {
 	}
 }
 
+void setup_auto_pager(const char *cmd, int def)
+{
+	if (use_pager != -1 || pager_in_use())
+		return;
+	use_pager = check_pager_config(cmd);
+	if (use_pager == -1)
+		use_pager = def;
+	commit_pager_choice();
+}
+
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
 	const char **orig_argv = *argv;
-- 
2.14.0.rc1.12.ge2d9c4613

[PATCH v3 4/7] t7006: add tests for how git tag paginates

[Martin Ågren]

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.
Someone who makes use of both `git tag -a` and `git tag -l` will
probably not set `pager.tag`, so that `git tag -a` will actually work,
at the cost of not paging output of `git tag -l`.

Since we're about to change how `git tag` respects `pager.tag`, add tests
around this, including how the configuration is ignored if --no-pager or
--paginate are used.

Construct tests with a few different subcommands. First, use -l. Second,
use "no arguments" and --contains, since those imply -l. (There are
more arguments which imply -l, but using these two should be enough.)

Third, use -a as a representative for "not -l". Actually, the tests use
`git tag -am` so no editor is launched, but that is irrelevant, since we
just want to see whether the pager is used or not. Make one of the tests
demonstrate the broken behavior mentioned above, where `git tag -a`
respects `pager.tag`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
In one place, I now use test_expect_failure (thanks Peff).

 t/t7006-pager.sh | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 20b4d83c2..b56d4cdd4 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,6 +134,73 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
 	}
 '
 
+test_expect_success TTY 'git tag -l defaults to not paging' '
+	rm -f paginated.out &&
+	test_terminal git tag -l &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects pager.tag' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag tag -l &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects --no-pager' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag --no-pager tag -l &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag with no args defaults to not paging' '
+	# no args implies -l so this should page like -l
+	rm -f paginated.out &&
+	test_terminal git tag &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag with no args respects pager.tag' '
+	# no args implies -l so this should page like -l
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag tag &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git tag --contains defaults to not paging' '
+	# --contains implies -l so this should page like -l
+	rm -f paginated.out &&
+	test_terminal git tag --contains &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag --contains respects pager.tag' '
+	# --contains implies -l so this should page like -l
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag tag --contains &&
+	test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a defaults to not paging' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git tag -am message newtag &&
+	! test -e paginated.out
+'
+
+test_expect_failure TTY 'git tag -a ignores pager.tag' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag tag -am message newtag &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a respects --paginate' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git --paginate tag -am message newtag &&
+	test -e paginated.out
+'
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
-- 
2.14.0.rc1.12.ge2d9c4613

[PATCH v3 5/7] tag: respect `pager.tag` in list-mode only

[Martin Ågren]

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.
Someone who makes use of both `git tag -a` and `git tag -l` will
probably not set `pager.tag`, so that `git tag -a` will actually work,
at the cost of not paging output of `git tag -l`.

Use the mechanisms introduced in two earlier patches to ignore
`pager.tag` in git.c and let the `git tag` builtin handle it on its own.
Only respect `pager.tag` when running in list-mode.

There is a window between where the pager is started before and after
this patch. This means that early errors can behave slightly different
before and after this patch. Since operation-parsing has to happen
inside this window, this can be seen with `git -c pager.tag="echo pager
is used" tag -l --unknown-option`. This change in paging-behavior should
be acceptable since it only affects erroneous usages.

Update the documentation and update tests.

If an alias is used to run `git tag -a`, then `pager.tag` will still be
respected. Document this known breakage. It will be fixed in a later
commit. Add a similar test for `-l`, which works.

Noticed-by: Anatoly Borodin <anatoly.borodin@gmail.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  3 +++
 t/t7006-pager.sh          | 15 ++++++++++++++-
 builtin/tag.c             |  3 +++
 git.c                     |  2 +-
 4 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 1eb15afa1..875d135e0 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -205,6 +205,9 @@ it in the repository configuration as follows:
     signingKey = <gpg-keyid>
 -------------------------------------
 
+`pager.tag` is only respected when listing tags, i.e., when `-l` is
+used or implied.
+See linkgit:git-config[1].
 
 DISCUSSION
 ----------
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index b56d4cdd4..570b2f252 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -187,7 +187,7 @@ test_expect_success TTY 'git tag -a defaults to not paging' '
 	! test -e paginated.out
 '
 
-test_expect_failure TTY 'git tag -a ignores pager.tag' '
+test_expect_success TTY 'git tag -a ignores pager.tag' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag tag -am message newtag &&
@@ -201,6 +201,19 @@ test_expect_success TTY 'git tag -a respects --paginate' '
 	test -e paginated.out
 '
 
+test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
+	test_when_finished "git tag -d newtag" &&
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
+	! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
+	rm -f paginated.out &&
+	test_terminal git -c pager.tag -c alias.t=tag t -l &&
+	test -e paginated.out
+'
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
diff --git a/builtin/tag.c b/builtin/tag.c
index 01154ea8d..5ad1af252 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -461,6 +461,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			cmdmode = 'l';
 	}
 
+	if (cmdmode == 'l')
+		setup_auto_pager("tag", 0);
+
 	if ((create_tag_object || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
 
diff --git a/git.c b/git.c
index 66832f232..82ac2a092 100644
--- a/git.c
+++ b/git.c
@@ -466,7 +466,7 @@ static struct cmd_struct commands[] = {
 	{ "stripspace", cmd_stripspace },
 	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},
 	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
-	{ "tag", cmd_tag, RUN_SETUP },
+	{ "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },
 	{ "unpack-file", cmd_unpack_file, RUN_SETUP },
 	{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
 	{ "update-index", cmd_update_index, RUN_SETUP },
-- 
2.14.0.rc1.12.ge2d9c4613

[PATCH v3 6/7] tag: change default of `pager.tag` to "on"

[Martin Ågren]

The previous patch taught `git tag` to only respect `pager.tag` in
list-mode. That patch left the default value of `pager.tag` at "off".

After that patch, it makes sense to let the default value be "on"
instead, since it will help with listing many tags, but will not hurt
users of `git tag -a` as it would have before. Make that change. Update
documentation and tests.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-tag.txt |  2 +-
 t/t7006-pager.sh          | 28 ++++++++++++++--------------
 builtin/tag.c             |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 875d135e0..d97aad343 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -206,7 +206,7 @@ it in the repository configuration as follows:
 -------------------------------------
 
 `pager.tag` is only respected when listing tags, i.e., when `-l` is
-used or implied.
+used or implied. The default is to use a pager.
 See linkgit:git-config[1].
 
 DISCUSSION
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 570b2f252..afa03f3b6 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,16 +134,16 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
 	}
 '
 
-test_expect_success TTY 'git tag -l defaults to not paging' '
+test_expect_success TTY 'git tag -l defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git tag -l &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects pager.tag' '
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag tag -l &&
-	test -e paginated.out
+	test_terminal git -c pager.tag=false tag -l &&
+	! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -l respects --no-pager' '
@@ -152,32 +152,32 @@ test_expect_success TTY 'git tag -l respects --no-pager' '
 	! test -e paginated.out
 '
 
-test_expect_success TTY 'git tag with no args defaults to not paging' '
+test_expect_success TTY 'git tag with no args defaults to paging' '
 	# no args implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git tag &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 test_expect_success TTY 'git tag with no args respects pager.tag' '
 	# no args implies -l so this should page like -l
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag tag &&
-	test -e paginated.out
+	test_terminal git -c pager.tag=false tag &&
+	! test -e paginated.out
 '
 
-test_expect_success TTY 'git tag --contains defaults to not paging' '
+test_expect_success TTY 'git tag --contains defaults to paging' '
 	# --contains implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git tag --contains &&
-	! test -e paginated.out
+	test -e paginated.out
 '
 
 test_expect_success TTY 'git tag --contains respects pager.tag' '
 	# --contains implies -l so this should page like -l
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag tag --contains &&
-	test -e paginated.out
+	test_terminal git -c pager.tag=false tag --contains &&
+	! test -e paginated.out
 '
 
 test_expect_success TTY 'git tag -a defaults to not paging' '
@@ -210,8 +210,8 @@ test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
 
 test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
 	rm -f paginated.out &&
-	test_terminal git -c pager.tag -c alias.t=tag t -l &&
-	test -e paginated.out
+	test_terminal git -c pager.tag=false -c alias.t=tag t -l &&
+	! test -e paginated.out
 '
 
 # A colored commit log will begin with an appropriate ANSI escape
diff --git a/builtin/tag.c b/builtin/tag.c
index 5ad1af252..ea83df5e1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -462,7 +462,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 
 	if (cmdmode == 'l')
-		setup_auto_pager("tag", 0);
+		setup_auto_pager("tag", 1);
 
 	if ((create_tag_object || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
-- 
2.14.0.rc1.12.ge2d9c4613

[PATCH v3 7/7] git.c: ignore pager.* when launching builtin as dashed external

[Martin Ågren]

When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and
execute `git-foo` as a dashed external. This is true even if git foo is
a builtin. That is on purpose, and is motivated in a comment which was
added in commit 441981bc ("git: simplify environment save/restore
logic", 2016-01-26).

Shortly before we launch a dashed external, and unless we have already
found out whether we should use a pager, we check `pager.foo`. This was
added in commit 92058e4d ("support pager.* for external commands",
2011-08-18). If the dashed external is a builtin, this does not match
that commit's intention and is arguably wrong, since it would be cleaner
if we let the "dashed external builtin" handle `pager.foo`.

This has not mattered in practice, but a recent patch taught `git-tag`
to ignore `pager.tag` under certain circumstances. But, when started
using an alias, it doesn't get the chance to do so, as outlined above.
That recent patch added a test to document this breakage.

Do not check `pager.foo` before launching a builtin as a dashed
external, i.e., if we recognize the name of the external as a builtin.
Change the test to use `test_expect_success`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t7006-pager.sh | 2 +-
 git.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index afa03f3b6..9128ec5ac 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -201,7 +201,7 @@ test_expect_success TTY 'git tag -a respects --paginate' '
 	test -e paginated.out
 '
 
-test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
+test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
diff --git a/git.c b/git.c
index 82ac2a092..6b6d9f68e 100644
--- a/git.c
+++ b/git.c
@@ -559,7 +559,7 @@ static void execv_dashed_external(const char **argv)
 	if (get_super_prefix())
 		die("%s doesn't support --super-prefix", argv[0]);
 
-	if (use_pager == -1)
+	if (use_pager == -1 && !is_builtin(argv[0]))
 		use_pager = check_pager_config(argv[0]);
 	commit_pager_choice();
 
-- 
2.14.0.rc1.12.ge2d9c4613

Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> diff --git a/builtin.h b/builtin.h
> index 498ac80d0..8d87d06da 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -6,6 +6,86 @@
>  #include "cache.h"
>  #include "commit.h"
>  
> +/*
> + * builtin API
> + * ===========
> + *
> + * Adding a new built-in
> + * ---------------------
> + *
> + * There are 4 things to do to add a built-in command implementation to
> + * Git:
> + *
> + * . Define the implementation of the built-in command `foo` with
> + *   signature:
> + *
> + *	int cmd_foo(int argc, const char **argv, const char *prefix);
> + *
> + * . Add the external declaration for the function to `builtin.h`.
> + *
> + * . Add the command to the `commands[]` table defined in `git.c`.
> + *   The entry should look like:
> + *
> + *	{ "foo", cmd_foo, <options> },
> + *
> + * where options is the bitwise-or of:
> + *
> + * `RUN_SETUP`:
> + *	If there is not a Git directory to work on, abort.  If there
> + *	is a work tree, chdir to the top of it if the command was
> + *	invoked in a subdirectory.  If there is no work tree, no
> + *	chdir() is done.
> + *
> + * `RUN_SETUP_GENTLY`:
> + *	If there is a Git directory, chdir as per RUN_SETUP, otherwise,
> + *	don't chdir anywhere.
> + *
> + * `USE_PAGER`:
> + *
> + *	If the standard output is connected to a tty, spawn a pager and
> + *	feed our output to it.
> + *
> + * `NEED_WORK_TREE`:
> + *
> + *	Make sure there is a work tree, i.e. the command cannot act
> + *	on bare repositories.
> + *	This only makes sense when `RUN_SETUP` is also set.
> + *
> + * `SUPPORT_SUPER_PREFIX`:
> + *
> + *	The built-in supports `--super-prefix`.
> + *
> + * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.

Not a new problem but it will become much easier to follow if we
moved this item between the "implement cmd_foo()" and "declare
cmd_foo in builtin.h", like so:

 . Define the implementation of the built-in command `foo` with
   signature:

	int cmd_foo(int argc, const char **argv, const char *prefix);

   in a new file `builtin/foo.c`.

 . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.

Alternatively, we may merge these two into one item (i.e. "in a new
file `builtin/foo.c` and add `builtin/foo.o` to ...").

But of course, this patch 1/7 should not do any of the above.  I am
suggesting a possible future clean-up for anybody on the list
listening from sidelines, and you do not have to be the person who
does it.

Thanks.

Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> This is the third version of my attempt to make `pager tag` useful (v1
> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.
>
> I've squashed patches 01-03/10 and 07-08/10, respectively. The interdiff
> is below. I managed to clean up some tests thanks to a drive-by comment
> by Peff which cleared up a misunderstanding I had. Some minor changes,
> e.g., I write "built-in" instead of "builtin", since that seemed
> preferred, at least locally in builtin.h. I documented why a default
> value of "punt" could be useful, but also that it's probably not wanted.
>
> Martin

Thanks for working on this.  The whole series was well reasoned and
was a very pleasant read.

Will queue.

Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode

[Jeff King]

On Wed, Aug 02, 2017 at 09:40:48PM +0200, Martin Ågren wrote:

> This is the third version of my attempt to make `pager tag` useful (v1
> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.

This looks good to me overall. One minor question from the interdiff:

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index 8b2ffb1aa..9128ec5ac 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '
>  test_expect_success TTY 'git tag with no args respects pager.tag' '
>  	# no args implies -l so this should page like -l
>  	rm -f paginated.out &&
> -	test_terminal git -c pager.tag=no tag &&
> +	test_terminal git -c pager.tag=false tag &&
>  	! test -e paginated.out
>  '

These should behave the same, right? So this is just a style/consistency
fix, not a bugfix?

-Peff

Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt

[Martin Ågren]

On 3 August 2017 at 19:44, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>> + * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
>
> Not a new problem but it will become much easier to follow if we
> moved this item between the "implement cmd_foo()" and "declare
> cmd_foo in builtin.h", like so:
>
>  . Define the implementation of the built-in command `foo` with
>    signature:
>
>         int cmd_foo(int argc, const char **argv, const char *prefix);
>
>    in a new file `builtin/foo.c`.
>
>  . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
>
> Alternatively, we may merge these two into one item (i.e. "in a new
> file `builtin/foo.c` and add `builtin/foo.o` to ...").
>
> But of course, this patch 1/7 should not do any of the above.  I am
> suggesting a possible future clean-up for anybody on the list
> listening from sidelines, and you do not have to be the person who
> does it.

Thank you. If this series needs to be rerolled, I could do it as patch 2.
And if not, I could try to remember to do it once this series has landed.
A that point in time, I'd also like to try changing other commands ("git
branch") similar to "git tag" (although maybe your suggestion above
shouldn't be part of that series, but go on its own).

Since this is my first code contribution to Git, I'll ask about this part of
SubmittingPatches:

"After the list reached a consensus that it is a good idea to apply the
patch, re-send it with "To:" set to the maintainer [*1*] and "cc:" the
list [*2*] for inclusion."

I will boldly assume that I should not be doing this. It seems to me this
doesn't happen very often or not at all -- possibly because you tend to
be involved in virtually all threads anyway, before the list reaches a
consensus.

Which brings me to my final point: Thanks for your very helpful feedback
throughout all three versions and even more thanks for your work on Git.
I find it amazing how much time you are able to constantly spend on all
aspects -- "high and low" -- of Git. It goes a long way to explaining how
Git can be so very useful. Thanks a lot!

Martin

Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode

[Martin Ågren]

On 3 August 2017 at 21:29, Jeff King <peff@peff.net> wrote:
> On Wed, Aug 02, 2017 at 09:40:48PM +0200, Martin Ågren wrote:
>
>> This is the third version of my attempt to make `pager tag` useful (v1
>> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.
>
> This looks good to me overall. One minor question from the interdiff:
>
>> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
>> index 8b2ffb1aa..9128ec5ac 100755
>> --- a/t/t7006-pager.sh
>> +++ b/t/t7006-pager.sh
>> @@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '
>>  test_expect_success TTY 'git tag with no args respects pager.tag' '
>>       # no args implies -l so this should page like -l
>>       rm -f paginated.out &&
>> -     test_terminal git -c pager.tag=no tag &&
>> +     test_terminal git -c pager.tag=false tag &&
>>       ! test -e paginated.out
>>  '
>
> These should behave the same, right? So this is just a style/consistency
> fix, not a bugfix?

Right. I realized I was using "false" everywhere else.  It wouldn't have hurt
to exercise the config-parsing a tiny bit differently, but I assume that's
already being done explicitly in some other test, so I went for consistency.

Thanks for all the feedback and thoughts throughout the different versions
of this series. It changed quite a bit since v1, so thanks a lot.

Martin

Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode

[Jeff King]

On Fri, Aug 04, 2017 at 06:21:47AM +0200, Martin Ågren wrote:

> On 3 August 2017 at 21:29, Jeff King <peff@peff.net> wrote:
> > On Wed, Aug 02, 2017 at 09:40:48PM +0200, Martin Ågren wrote:
> >
> >> This is the third version of my attempt to make `pager tag` useful (v1
> >> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.
> >
> > This looks good to me overall. One minor question from the interdiff:
> >
> >> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> >> index 8b2ffb1aa..9128ec5ac 100755
> >> --- a/t/t7006-pager.sh
> >> +++ b/t/t7006-pager.sh
> >> @@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '
> >>  test_expect_success TTY 'git tag with no args respects pager.tag' '
> >>       # no args implies -l so this should page like -l
> >>       rm -f paginated.out &&
> >> -     test_terminal git -c pager.tag=no tag &&
> >> +     test_terminal git -c pager.tag=false tag &&
> >>       ! test -e paginated.out
> >>  '
> >
> > These should behave the same, right? So this is just a style/consistency
> > fix, not a bugfix?
> 
> Right. I realized I was using "false" everywhere else.  It wouldn't have hurt
> to exercise the config-parsing a tiny bit differently, but I assume that's
> already being done explicitly in some other test, so I went for consistency.

Right, that makes perfect sense. Thanks for confirming.

> Thanks for all the feedback and thoughts throughout the different versions
> of this series. It changed quite a bit since v1, so thanks a lot.

You're very welcome. I hope we see more patches from you in the future.
:)

-Peff

Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> Since this is my first code contribution to Git, I'll ask about this part of
> SubmittingPatches:
>
> "After the list reached a consensus that it is a good idea to apply the
> patch, re-send it with "To:" set to the maintainer [*1*] and "cc:" the
> list [*2*] for inclusion."
>
> I will boldly assume that I should not be doing this. It seems to me this
> doesn't happen very often or not at all -- possibly because you tend to
> be involved in virtually all threads anyway, before the list reaches a
> consensus.

Yeah, that is in the "ideal patch flow" section, isn't it?  We
rarely achieve the "ideal" and often instead go for a more expedited
option, it appears---perhaps I should try to be less involved in
individual patch reviews and place more review burden on other
reviewers ;-)

In any case, it was a pleasure to cheer-lead on the progress of this
series.  Thanks.

Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt

[Martin Ågren]

On 4 August 2017 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>
>> Since this is my first code contribution to Git, I'll ask about this part of
>> SubmittingPatches:
>>
>> "After the list reached a consensus that it is a good idea to apply the
>> patch, re-send it with "To:" set to the maintainer [*1*] and "cc:" the
>> list [*2*] for inclusion."
>>
>> I will boldly assume that I should not be doing this. It seems to me this
>> doesn't happen very often or not at all -- possibly because you tend to
>> be involved in virtually all threads anyway, before the list reaches a
>> consensus.
>
> Yeah, that is in the "ideal patch flow" section, isn't it?

Yes and no. It's in the main part above it, under "(4) Sending your
patches." But you are right that the ideal patch flow section then
says the same thing: "(4) The list forms consensus that the last
round of your patch is good. Send it to the maintainer and cc the
list."

> We
> rarely achieve the "ideal" and often instead go for a more expedited
> option, it appears---perhaps I should try to be less involved in
> individual patch reviews and place more review burden on other
> reviewers ;-)

:-)