tag: more fine-grained pager-configuration

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/[email protected]/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

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.

---
 Documentation/technical/api-builtin.txt | 4 ++++
 1 file changed, 4 insertions(+)

index 22a39b929..60f442822 100644
@@ -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

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

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

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 ;)

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

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.

---
 Documentation/technical/api-builtin.txt |  6 ++++++
 git.c                                   | 14 ++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

index 60f442822..61fd8eeb2 100644
@@ -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:

index 489aab4d8..ae92f8ed5 100644
@@ -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

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:

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.

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

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

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.

---
 builtin.h | 14 ++++++++++++++
 git.c     | 28 ++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

index 498ac80d0..a6ed6c4ac 100644
@@ -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);

index ae92f8ed5..696eaf87a 100644
@@ -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

Good point. Thanks.

Martin

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

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

index 20b4d83c2..43cce3694 100755
@@ -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

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.

---
 builtin/tag.c | 2 ++
 git.c         | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

index 01154ea8d..e0f129872 100644
@@ -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);

index 696eaf87a..4d05452a3 100644
@@ -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

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

-Peff

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.

---
 Documentation/git-tag.txt |  4 ++++
 builtin/tag.c             |  2 ++
 t/t7006-pager.sh          | 16 +++++++++++++++-
 3 files changed, 21 insertions(+), 1 deletion(-)

index 1eb15afa1..6ad5811a2 100644
@@ -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

index e0f129872..e96ef7d70 100644
@@ -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) {

index 43cce3694..ed34c6734 100755
@@ -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

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

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.

---
 Documentation/git-tag.txt |  2 +-
 builtin/tag.c             |  2 +-
 t/t7006-pager.sh          | 12 ++++++------
 3 files changed, 8 insertions(+), 8 deletions(-)

index 6ad5811a2..fbdfb3f59 100644
@@ -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>::

index e96ef7d70..ec69fca61 100644
@@ -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) {

index ed34c6734..94df89a5f 100755
@@ -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

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

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.

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.

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

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

Yes.

...

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

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.

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

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

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

-Peff

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.

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.

Good thinking.

Thanks a lot for your comments. I appreciate it.

Martin

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

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.

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

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/[email protected]/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

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.

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

deleted file mode 100644
index 22a39b929..000000000
@@ -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.

2.14.0.rc0

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

---
 builtin.h | 146 +++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 73 insertions(+), 73 deletions(-)

index 51cb0249d..62f22b547 100644
@@ -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

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

---
 builtin.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 62f22b547..7bcc08456 100644
@@ -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

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.

---
 builtin.h | 8 ++++++++
 git.c     | 4 +++-
 2 files changed, 11 insertions(+), 1 deletion(-)

index 7bcc08456..4186635de 100644
@@ -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:

index 489aab4d8..79195ebbd 100644
@@ -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

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.

---
 builtin.h |  9 +++++++++
 git.c     | 10 ++++++++++
 2 files changed, 19 insertions(+)

index 4186635de..3ca4a53a8 100644
@@ -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);

index 79195ebbd..66832f232 100644
@@ -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

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

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.

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.

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

index 20b4d83c2..e7430bc93 100755
@@ -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

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.

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

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.

---
 builtin/tag.c | 2 ++
 git.c         | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

index 01154ea8d..9eda434fb 100644
@@ -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);
 

index 66832f232..82ac2a092 100644
@@ -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

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.

---
 Documentation/git-tag.txt |  3 +++
 t/t7006-pager.sh          | 25 +++++++++++++++++++++----
 builtin/tag.c             |  3 ++-
 3 files changed, 26 insertions(+), 5 deletions(-)

index 1eb15afa1..875d135e0 100644
@@ -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
 ----------

index e7430bc93..a357436e1 100755
@@ -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
 '
 

index 9eda434fb..5ad1af252 100644
@@ -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

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.

---
 Documentation/git-tag.txt |  2 +-
 t/t7006-pager.sh          | 28 ++++++++++++++--------------
 builtin/tag.c             |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

index 875d135e0..d97aad343 100644
@@ -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

index a357436e1..df258c5d4 100755
@@ -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

index 5ad1af252..ea83df5e1 100644
@@ -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

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.

---
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(-)

index df258c5d4..8b2ffb1aa 100755
@@ -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.

index 82ac2a092..6b6d9f68e 100644
@@ -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

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.

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

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:

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.

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.

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

Thanks.

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

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

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

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

-Peff

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

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/[email protected]/T/

[2] https://public-inbox.org/git/[email protected]/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:

index 8b2ffb1aa..9128ec5ac 100755
@@ -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
 '
 

index 3ca4a53a8..42378f3aa 100644
@@ -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

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

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

deleted file mode 100644
index 22a39b929..000000000
@@ -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.

2.14.0.rc1.12.ge2d9c4613

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

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.

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

:-)

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.

---
 builtin.h | 8 ++++++++
 git.c     | 4 +++-
 2 files changed, 11 insertions(+), 1 deletion(-)

index 8d87d06da..0f3a7b770 100644
@@ -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:

index 489aab4d8..79195ebbd 100644
@@ -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

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.

---
 builtin.h | 12 ++++++++++++
 git.c     | 10 ++++++++++
 2 files changed, 22 insertions(+)

index 0f3a7b770..42378f3aa 100644
@@ -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);

index 79195ebbd..66832f232 100644
@@ -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

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.

---
In one place, I now use test_expect_failure (thanks Peff).

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

index 20b4d83c2..b56d4cdd4 100755
@@ -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

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.

---
 Documentation/git-tag.txt |  3 +++
 t/t7006-pager.sh          | 15 ++++++++++++++-
 builtin/tag.c             |  3 +++
 git.c                     |  2 +-
 4 files changed, 21 insertions(+), 2 deletions(-)

index 1eb15afa1..875d135e0 100644
@@ -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
 ----------

index b56d4cdd4..570b2f252 100755
@@ -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() {

index 01154ea8d..5ad1af252 100644
@@ -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);
 

index 66832f232..82ac2a092 100644
@@ -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

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.

---
 Documentation/git-tag.txt |  2 +-
 t/t7006-pager.sh          | 28 ++++++++++++++--------------
 builtin/tag.c             |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

index 875d135e0..d97aad343 100644
@@ -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

index 570b2f252..afa03f3b6 100755
@@ -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

index 5ad1af252..ea83df5e1 100644
@@ -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

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.

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

index afa03f3b6..9128ec5ac 100755
@@ -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 &&

index 82ac2a092..6b6d9f68e 100644
@@ -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

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

Will queue.

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

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

Right, that makes perfect sense. Thanks for confirming.

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

-Peff