* [PATCH v2 01/10] builtin.h: take over documentation from api-builtin.txt
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
@ 2017-07-17 20:10 ` Martin Ågren
2017-07-17 20:10 ` [PATCH v2 02/10] builtin.h: format documentation-comment properly Martin Ågren
` (11 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-07-17 20:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams
Delete Documentation/technical/api-builtin.txt and move its content
verbatim into builtin.h. Just wrap it in /* ... */. In order to make
the move obviously correct, do not change any formatting, not even to
format the comment into Git's preferred style. That will be done in a
follow-up patch.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
Documentation/technical/api-builtin.txt | 73 -------------------------------
builtin.h | 76 +++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+), 73 deletions(-)
delete mode 100644 Documentation/technical/api-builtin.txt
diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
deleted file mode 100644
index 22a39b929..000000000
--- a/Documentation/technical/api-builtin.txt
+++ /dev/null
@@ -1,73 +0,0 @@
-builtin API
-===========
-
-Adding a new built-in
----------------------
-
-There are 4 things to do to add a built-in command implementation to
-Git:
-
-. Define the implementation of the built-in command `foo` with
- signature:
-
- int cmd_foo(int argc, const char **argv, const char *prefix);
-
-. Add the external declaration for the function to `builtin.h`.
-
-. Add the command to the `commands[]` table defined in `git.c`.
- The entry should look like:
-
- { "foo", cmd_foo, <options> },
-+
-where options is the bitwise-or of:
-
-`RUN_SETUP`::
- If there is not a Git directory to work on, abort. If there
- is a work tree, chdir to the top of it if the command was
- invoked in a subdirectory. If there is no work tree, no
- chdir() is done.
-
-`RUN_SETUP_GENTLY`::
- If there is a Git directory, chdir as per RUN_SETUP, otherwise,
- don't chdir anywhere.
-
-`USE_PAGER`::
-
- If the standard output is connected to a tty, spawn a pager and
- feed our output to it.
-
-`NEED_WORK_TREE`::
-
- Make sure there is a work tree, i.e. the command cannot act
- on bare repositories.
- This only makes sense when `RUN_SETUP` is also set.
-
-. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
-
-Additionally, if `foo` is a new command, there are 3 more things to do:
-
-. Add tests to `t/` directory.
-
-. Write documentation in `Documentation/git-foo.txt`.
-
-. Add an entry for `git-foo` to `command-list.txt`.
-
-. Add an entry for `/git-foo` to `.gitignore`.
-
-
-How a built-in is called
-------------------------
-
-The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
-and `prefix`. The first two are similar to what `main()` of a
-standalone command would be called with.
-
-When `RUN_SETUP` is specified in the `commands[]` table, and when you
-were started from a subdirectory of the work tree, `cmd_foo()` is called
-after chdir(2) to the top of the work tree, and `prefix` gets the path
-to the subdirectory the command started from. This allows you to
-convert a user-supplied pathname (typically relative to that directory)
-to a pathname relative to the top of the work tree.
-
-The return value from `cmd_foo()` becomes the exit status of the
-command.
diff --git a/builtin.h b/builtin.h
index 498ac80d0..51cb0249d 100644
--- a/builtin.h
+++ b/builtin.h
@@ -6,6 +6,82 @@
#include "cache.h"
#include "commit.h"
+/*
+builtin API
+===========
+
+Adding a new built-in
+---------------------
+
+There are 4 things to do to add a built-in command implementation to
+Git:
+
+. Define the implementation of the built-in command `foo` with
+ signature:
+
+ int cmd_foo(int argc, const char **argv, const char *prefix);
+
+. Add the external declaration for the function to `builtin.h`.
+
+. Add the command to the `commands[]` table defined in `git.c`.
+ The entry should look like:
+
+ { "foo", cmd_foo, <options> },
++
+where options is the bitwise-or of:
+
+`RUN_SETUP`::
+ If there is not a Git directory to work on, abort. If there
+ is a work tree, chdir to the top of it if the command was
+ invoked in a subdirectory. If there is no work tree, no
+ chdir() is done.
+
+`RUN_SETUP_GENTLY`::
+ If there is a Git directory, chdir as per RUN_SETUP, otherwise,
+ don't chdir anywhere.
+
+`USE_PAGER`::
+
+ If the standard output is connected to a tty, spawn a pager and
+ feed our output to it.
+
+`NEED_WORK_TREE`::
+
+ Make sure there is a work tree, i.e. the command cannot act
+ on bare repositories.
+ This only makes sense when `RUN_SETUP` is also set.
+
+. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
+
+Additionally, if `foo` is a new command, there are 3 more things to do:
+
+. Add tests to `t/` directory.
+
+. Write documentation in `Documentation/git-foo.txt`.
+
+. Add an entry for `git-foo` to `command-list.txt`.
+
+. Add an entry for `/git-foo` to `.gitignore`.
+
+
+How a built-in is called
+------------------------
+
+The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
+and `prefix`. The first two are similar to what `main()` of a
+standalone command would be called with.
+
+When `RUN_SETUP` is specified in the `commands[]` table, and when you
+were started from a subdirectory of the work tree, `cmd_foo()` is called
+after chdir(2) to the top of the work tree, and `prefix` gets the path
+to the subdirectory the command started from. This allows you to
+convert a user-supplied pathname (typically relative to that directory)
+to a pathname relative to the top of the work tree.
+
+The return value from `cmd_foo()` becomes the exit status of the
+command.
+ */
+
#define DEFAULT_MERGE_LOG_LEN 20
extern const char git_usage_string[];
--
2.14.0.rc0
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v2 02/10] builtin.h: format documentation-comment properly
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
2017-07-17 20:10 ` [PATCH v2 01/10] builtin.h: take over documentation from api-builtin.txt Martin Ågren
@ 2017-07-17 20:10 ` Martin Ågren
2017-07-17 20:10 ` [PATCH v2 03/10] builtin.h: document SUPPORT_SUPER_PREFIX Martin Ågren
` (10 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-07-17 20:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams
The previous commit moved technical documentation verbatim into
builtin.h. Prefix all the lines with '*' to follow the coding
guidelines.
Remove a '+' which was needed when the information was formatted for
AsciiDoc. Similarly, change "::" to ":".
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
builtin.h | 146 +++++++++++++++++++++++++++++++-------------------------------
1 file changed, 73 insertions(+), 73 deletions(-)
diff --git a/builtin.h b/builtin.h
index 51cb0249d..62f22b547 100644
--- a/builtin.h
+++ b/builtin.h
@@ -7,79 +7,79 @@
#include "commit.h"
/*
-builtin API
-===========
-
-Adding a new built-in
----------------------
-
-There are 4 things to do to add a built-in command implementation to
-Git:
-
-. Define the implementation of the built-in command `foo` with
- signature:
-
- int cmd_foo(int argc, const char **argv, const char *prefix);
-
-. Add the external declaration for the function to `builtin.h`.
-
-. Add the command to the `commands[]` table defined in `git.c`.
- The entry should look like:
-
- { "foo", cmd_foo, <options> },
-+
-where options is the bitwise-or of:
-
-`RUN_SETUP`::
- If there is not a Git directory to work on, abort. If there
- is a work tree, chdir to the top of it if the command was
- invoked in a subdirectory. If there is no work tree, no
- chdir() is done.
-
-`RUN_SETUP_GENTLY`::
- If there is a Git directory, chdir as per RUN_SETUP, otherwise,
- don't chdir anywhere.
-
-`USE_PAGER`::
-
- If the standard output is connected to a tty, spawn a pager and
- feed our output to it.
-
-`NEED_WORK_TREE`::
-
- Make sure there is a work tree, i.e. the command cannot act
- on bare repositories.
- This only makes sense when `RUN_SETUP` is also set.
-
-. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
-
-Additionally, if `foo` is a new command, there are 3 more things to do:
-
-. Add tests to `t/` directory.
-
-. Write documentation in `Documentation/git-foo.txt`.
-
-. Add an entry for `git-foo` to `command-list.txt`.
-
-. Add an entry for `/git-foo` to `.gitignore`.
-
-
-How a built-in is called
-------------------------
-
-The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
-and `prefix`. The first two are similar to what `main()` of a
-standalone command would be called with.
-
-When `RUN_SETUP` is specified in the `commands[]` table, and when you
-were started from a subdirectory of the work tree, `cmd_foo()` is called
-after chdir(2) to the top of the work tree, and `prefix` gets the path
-to the subdirectory the command started from. This allows you to
-convert a user-supplied pathname (typically relative to that directory)
-to a pathname relative to the top of the work tree.
-
-The return value from `cmd_foo()` becomes the exit status of the
-command.
+ * builtin API
+ * ===========
+ *
+ * Adding a new built-in
+ * ---------------------
+ *
+ * There are 4 things to do to add a built-in command implementation to
+ * Git:
+ *
+ * . Define the implementation of the built-in command `foo` with
+ * signature:
+ *
+ * int cmd_foo(int argc, const char **argv, const char *prefix);
+ *
+ * . Add the external declaration for the function to `builtin.h`.
+ *
+ * . Add the command to the `commands[]` table defined in `git.c`.
+ * The entry should look like:
+ *
+ * { "foo", cmd_foo, <options> },
+ *
+ * where options is the bitwise-or of:
+ *
+ * `RUN_SETUP`:
+ * If there is not a Git directory to work on, abort. If there
+ * is a work tree, chdir to the top of it if the command was
+ * invoked in a subdirectory. If there is no work tree, no
+ * chdir() is done.
+ *
+ * `RUN_SETUP_GENTLY`:
+ * If there is a Git directory, chdir as per RUN_SETUP, otherwise,
+ * don't chdir anywhere.
+ *
+ * `USE_PAGER`:
+ *
+ * If the standard output is connected to a tty, spawn a pager and
+ * feed our output to it.
+ *
+ * `NEED_WORK_TREE`:
+ *
+ * Make sure there is a work tree, i.e. the command cannot act
+ * on bare repositories.
+ * This only makes sense when `RUN_SETUP` is also set.
+ *
+ * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
+ *
+ * Additionally, if `foo` is a new command, there are 3 more things to do:
+ *
+ * . Add tests to `t/` directory.
+ *
+ * . Write documentation in `Documentation/git-foo.txt`.
+ *
+ * . Add an entry for `git-foo` to `command-list.txt`.
+ *
+ * . Add an entry for `/git-foo` to `.gitignore`.
+ *
+ *
+ * How a built-in is called
+ * ------------------------
+ *
+ * The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
+ * and `prefix`. The first two are similar to what `main()` of a
+ * standalone command would be called with.
+ *
+ * When `RUN_SETUP` is specified in the `commands[]` table, and when you
+ * were started from a subdirectory of the work tree, `cmd_foo()` is called
+ * after chdir(2) to the top of the work tree, and `prefix` gets the path
+ * to the subdirectory the command started from. This allows you to
+ * convert a user-supplied pathname (typically relative to that directory)
+ * to a pathname relative to the top of the work tree.
+ *
+ * The return value from `cmd_foo()` becomes the exit status of the
+ * command.
*/
#define DEFAULT_MERGE_LOG_LEN 20
--
2.14.0.rc0
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v2 03/10] builtin.h: document SUPPORT_SUPER_PREFIX
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
2017-07-17 20:10 ` [PATCH v2 01/10] builtin.h: take over documentation from api-builtin.txt Martin Ågren
2017-07-17 20:10 ` [PATCH v2 02/10] builtin.h: format documentation-comment properly Martin Ågren
@ 2017-07-17 20:10 ` Martin Ågren
2017-07-17 20:10 ` [PATCH v2 04/10] git.c: let builtins opt for handling `pager.foo` themselves Martin Ågren
` (9 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-07-17 20:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams
Commit 74866d75 ("git: make super-prefix option", 2016-10-07) introduced
SUPPORT_SUPER_PREFIX as a builtin flag without documenting it. The next
patch will add another flag, so document SUPPORT_SUPER_PREFIX, thereby
bringing the documentation up to date with the available flags.
While at it, correct '3 more things to do' to '4 more things to do'.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
builtin.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/builtin.h b/builtin.h
index 62f22b547..7bcc08456 100644
--- a/builtin.h
+++ b/builtin.h
@@ -51,9 +51,13 @@
* on bare repositories.
* This only makes sense when `RUN_SETUP` is also set.
*
+ * `SUPPORT_SUPER_PREFIX`::
+ *
+ * The builtin supports `--super-prefix`.
+ *
* . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
*
- * Additionally, if `foo` is a new command, there are 3 more things to do:
+ * Additionally, if `foo` is a new command, there are 4 more things to do:
*
* . Add tests to `t/` directory.
*
--
2.14.0.rc0
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v2 04/10] git.c: let builtins opt for handling `pager.foo` themselves
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
` (2 preceding siblings ...)
2017-07-17 20:10 ` [PATCH v2 03/10] builtin.h: document SUPPORT_SUPER_PREFIX Martin Ågren
@ 2017-07-17 20:10 ` Martin Ågren
2017-07-17 20:10 ` [PATCH v2 05/10] git.c: provide setup_auto_pager() Martin Ågren
` (8 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-07-17 20:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, 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 DELAY_PAGER_CONFIG. If the flag
is set, do not check `pager.foo`.
Do not check for DELAY_PAGER_CONFIG in `execv_dashed_external()`. That
call site is arguably wrong, although in a way that is not yet visible,
and will be changed in a slightly different direction in a later patch.
Don't add any users of DELAY_PAGER_CONFIG just yet, one will follow in a
later patch.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
builtin.h | 8 ++++++++
git.c | 4 +++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/builtin.h b/builtin.h
index 7bcc08456..4186635de 100644
--- a/builtin.h
+++ b/builtin.h
@@ -55,6 +55,14 @@
*
* The builtin supports `--super-prefix`.
*
+ * `DELAY_PAGER_CONFIG`::
+ *
+ * If RUN_SETUP or RUN_SETUP_GENTLY is set, git.c normally handles
+ * the `pager.<cmd>`-configuration. If this flag is used, git.c
+ * will skip that step, instead allowing the builtin to make a
+ * more informed decision, e.g., by ignoring `pager.<cmd>` for
+ * certain subcommands.
+ *
* . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
*
* Additionally, if `foo` is a new command, there are 4 more things to do:
diff --git a/git.c b/git.c
index 489aab4d8..79195ebbd 100644
--- a/git.c
+++ b/git.c
@@ -283,6 +283,7 @@ static int handle_alias(int *argcp, const char ***argv)
*/
#define NEED_WORK_TREE (1<<3)
#define SUPPORT_SUPER_PREFIX (1<<4)
+#define DELAY_PAGER_CONFIG (1<<5)
struct cmd_struct {
const char *cmd;
@@ -306,7 +307,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
prefix = setup_git_directory_gently(&nongit_ok);
}
- if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
+ if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
+ !(p->option & DELAY_PAGER_CONFIG))
use_pager = check_pager_config(p->cmd);
if (use_pager == -1 && p->option & USE_PAGER)
use_pager = 1;
--
2.14.0.rc0
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v2 05/10] git.c: provide setup_auto_pager()
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
` (3 preceding siblings ...)
2017-07-17 20:10 ` [PATCH v2 04/10] git.c: let builtins opt for handling `pager.foo` themselves Martin Ågren
@ 2017-07-17 20:10 ` Martin Ågren
2017-07-31 3:34 ` Jeff King
2017-07-17 20:10 ` [PATCH v2 06/10] t7006: add tests for how git tag paginates Martin Ågren
` (7 subsequent siblings)
12 siblings, 1 reply; 69+ messages in thread
From: Martin Ågren @ 2017-07-17 20:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams
The previous patch introduced a way for builtins to declare that they
will take responsibility for handling the `pager.foo`-config item. (See
the commit message of that patch for why that could be useful.)
Provide setup_auto_pager(), which builtins can call in order to handle
`pager.<cmd>`, including possibly starting the pager. Make this function
don't do anything if a pager has already been started, as indicated by
use_pager or pager_in_use().
Whenever this function is called from a builtin, git.c will already have
called commit_pager_choice(). Since commit_pager_choice() treats the
special value -1 as "punt" or "not yet decided", it is not a problem
that we might end up calling commit_pager_choice() once in git.c and
once (or more) in the builtin. Make the new function use -1 in the same
way and document it as "punt".
Don't add any users of setup_auto_pager just yet, one will follow in
a later patch.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
builtin.h | 9 +++++++++
git.c | 10 ++++++++++
2 files changed, 19 insertions(+)
diff --git a/builtin.h b/builtin.h
index 4186635de..3ca4a53a8 100644
--- a/builtin.h
+++ b/builtin.h
@@ -113,6 +113,15 @@ struct fmt_merge_msg_opts {
extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
struct fmt_merge_msg_opts *);
+/**
+ * If a builtin has DELAY_PAGER_CONFIG set, the builtin should call this early
+ * when it wishes to respect the `pager.foo`-config. The `cmd` is the name of
+ * the builtin, e.g., "foo". If a paging-choice has already been setup, this
+ * does nothing. The default in `def` should be 0 for "pager off", 1 for "pager
+ * on" or -1 for "punt".
+ */
+extern void setup_auto_pager(const char *cmd, int def);
+
extern int is_builtin(const char *s);
extern int cmd_add(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index 79195ebbd..66832f232 100644
--- a/git.c
+++ b/git.c
@@ -33,6 +33,16 @@ static void commit_pager_choice(void) {
}
}
+void setup_auto_pager(const char *cmd, int def)
+{
+ if (use_pager != -1 || pager_in_use())
+ return;
+ use_pager = check_pager_config(cmd);
+ if (use_pager == -1)
+ use_pager = def;
+ commit_pager_choice();
+}
+
static int handle_options(const char ***argv, int *argc, int *envchanged)
{
const char **orig_argv = *argv;
--
2.14.0.rc0
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v2 05/10] git.c: provide setup_auto_pager()
2017-07-17 20:10 ` [PATCH v2 05/10] git.c: provide setup_auto_pager() Martin Ågren
@ 2017-07-31 3:34 ` Jeff King
2017-07-31 16:32 ` Junio C Hamano
0 siblings, 1 reply; 69+ messages in thread
From: Jeff King @ 2017-07-31 3:34 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Junio C Hamano, Brandon Williams
On Mon, Jul 17, 2017 at 10:10:47PM +0200, Martin Ågren wrote:
> The previous patch introduced a way for builtins to declare that they
> will take responsibility for handling the `pager.foo`-config item. (See
> the commit message of that patch for why that could be useful.)
>
> Provide setup_auto_pager(), which builtins can call in order to handle
> `pager.<cmd>`, including possibly starting the pager. Make this function
> don't do anything if a pager has already been started, as indicated by
> use_pager or pager_in_use().
>
> Whenever this function is called from a builtin, git.c will already have
> called commit_pager_choice(). Since commit_pager_choice() treats the
> special value -1 as "punt" or "not yet decided", it is not a problem
> that we might end up calling commit_pager_choice() once in git.c and
> once (or more) in the builtin. Make the new function use -1 in the same
> way and document it as "punt".
At first I wasn't sure if it would ever make sense to use "-1" here. The
"punt" that happens in earlier calls to commit_pager_choice() is there
because we might adjust our decision later. And this would generally be
the final decision, I would think. So I'd be surprised if we had
anything besides "0" or "1" in the "def" argument.
But thinking on it, the most plausible case is something like:
setup_auto_pager("foo", -1);
...
/* fallback to some historical compatibility name */
setup_auto_pager("bar", 0);
And it's important for the "-1" there to be a true punt, and not do
anything in commit_pager_choice(). So it's probably worth documenting
the "-1" behavior as you did here as a possible value for "def".
-Peff
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 05/10] git.c: provide setup_auto_pager()
2017-07-31 3:34 ` Jeff King
@ 2017-07-31 16:32 ` Junio C Hamano
0 siblings, 0 replies; 69+ messages in thread
From: Junio C Hamano @ 2017-07-31 16:32 UTC (permalink / raw)
To: Jeff King; +Cc: Martin Ågren, git, Brandon Williams
Jeff King <peff@peff.net> writes:
> But thinking on it, the most plausible case is something like:
>
> setup_auto_pager("foo", -1);
> ...
> /* fallback to some historical compatibility name */
> setup_auto_pager("bar", 0);
>
> And it's important for the "-1" there to be a true punt, and not do
> anything in commit_pager_choice(). So it's probably worth documenting
> the "-1" behavior as you did here as a possible value for "def".
Thanks for reading it over. I agree that the "punt" behaviour is a
sensible one in that use case, and I also agree that it would be
good to explain it.
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 06/10] t7006: add tests for how git tag paginates
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
` (4 preceding siblings ...)
2017-07-17 20:10 ` [PATCH v2 05/10] git.c: provide setup_auto_pager() Martin Ågren
@ 2017-07-17 20:10 ` Martin Ågren
2017-07-31 3:38 ` Jeff King
2017-07-17 20:10 ` [PATCH v2 07/10] tag: handle `pager.tag`-configuration within the builtin Martin Ågren
` (6 subsequent siblings)
12 siblings, 1 reply; 69+ messages in thread
From: Martin Ågren @ 2017-07-17 20:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams
Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as
"Vim: Warning: Output is not to a terminal" and a garbled terminal. A
user who makes use of `git tag -a` and `git tag -l` will probably choose
not to configure `pager.tag` or to set it to "no", so that `git tag -a`
will actually work, at the cost of not getting the pager with `git tag
-l`.
Since we're about to change how `git tag` respects `pager.tag`, add tests
around this, including how the configuration is ignored if --no-pager or
--paginate are used.
Construct tests with a few different subcommands. First, use -l. Second,
use "no arguments" and --contains, since those imply -l. (There are
more arguments which imply -l, but using these two should be enough.)
Third, use -a as a representative for "not -l".
Make one of the tests demonstrate the behavior mentioned above, where
`git tag -a` respects `pager.tag`. Actually, the tests use `git tag -am`
so no editor is launched, but that is irrelevant, since we just want to
see whether the pager is used or not.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
| 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 20b4d83c2..e7430bc93 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,6 +134,74 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
}
'
+test_expect_success TTY 'git tag -l defaults to not paging' '
+ rm -f paginated.out &&
+ test_terminal git tag -l &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects pager.tag' '
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag tag -l &&
+ test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects --no-pager' '
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag --no-pager tag -l &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag with no args defaults to not paging' '
+ # no args implies -l so this should page like -l
+ rm -f paginated.out &&
+ test_terminal git tag &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag with no args respects pager.tag' '
+ # no args implies -l so this should page like -l
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag tag &&
+ test -e paginated.out
+'
+
+test_expect_success TTY 'git tag --contains defaults to not paging' '
+ # --contains implies -l so this should page like -l
+ rm -f paginated.out &&
+ test_terminal git tag --contains &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag --contains respects pager.tag' '
+ # --contains implies -l so this should page like -l
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag tag --contains &&
+ test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a defaults to not paging' '
+ test_when_finished "git tag -d newtag" &&
+ rm -f paginated.out &&
+ test_terminal git tag -am message newtag &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a respects pager.tag' '
+ test_when_finished "git tag -d newtag" &&
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag tag -am message newtag &&
+ test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a respects --paginate' '
+ test_when_finished "git tag -d newtag" &&
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag=false --paginate \
+ tag -am message newtag &&
+ test -e paginated.out
+'
+
# A colored commit log will begin with an appropriate ANSI escape
# for the first color; the text "commit" comes later.
colorful() {
--
2.14.0.rc0
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v2 06/10] t7006: add tests for how git tag paginates
2017-07-17 20:10 ` [PATCH v2 06/10] t7006: add tests for how git tag paginates Martin Ågren
@ 2017-07-31 3:38 ` Jeff King
2017-07-31 16:37 ` Junio C Hamano
0 siblings, 1 reply; 69+ messages in thread
From: Jeff King @ 2017-07-31 3:38 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Junio C Hamano, Brandon Williams
On Mon, Jul 17, 2017 at 10:10:48PM +0200, Martin Ågren wrote:
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index 20b4d83c2..e7430bc93 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -134,6 +134,74 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
> }
> '
>
> +test_expect_success TTY 'git tag -l defaults to not paging' '
> + rm -f paginated.out &&
> + test_terminal git tag -l &&
> + ! test -e paginated.out
> +'
I don't mind an expect_success like this, where it documents a sane and
existing default, even if we're going to flip that default later in the
series.
But here...
> +test_expect_success TTY 'git tag -a respects pager.tag' '
> + test_when_finished "git tag -d newtag" &&
> + rm -f paginated.out &&
> + test_terminal git -c pager.tag tag -am message newtag &&
> + test -e paginated.out
> +'
I think this behavior is just buggy, and it might be better introduced
as a test_expect_failure on "git tag -a does not respect pager.tag".
Kind of a minor nit, as the series should end up in the right place
either way, but it can be helpful if you end up digging back in history
to the introduction of the test.
-Peff
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 06/10] t7006: add tests for how git tag paginates
2017-07-31 3:38 ` Jeff King
@ 2017-07-31 16:37 ` Junio C Hamano
2017-07-31 17:50 ` Martin Ågren
0 siblings, 1 reply; 69+ messages in thread
From: Junio C Hamano @ 2017-07-31 16:37 UTC (permalink / raw)
To: Jeff King; +Cc: Martin Ågren, git, Brandon Williams
Jeff King <peff@peff.net> writes:
> But here...
>
>> +test_expect_success TTY 'git tag -a respects pager.tag' '
>> + test_when_finished "git tag -d newtag" &&
>> + rm -f paginated.out &&
>> + test_terminal git -c pager.tag tag -am message newtag &&
>> + test -e paginated.out
>> +'
>
> I think this behavior is just buggy, and it might be better introduced
> as a test_expect_failure on "git tag -a does not respect pager.tag".
>
> Kind of a minor nit, as the series should end up in the right place
> either way, but it can be helpful if you end up digging back in history
> to the introduction of the test.
Yes, I think that is essentially the same reaction I had to patches
7 and 8, where it carries the "buggy" behaviour forward and then
fixes it. The way the series lays groundwork to introduce a
mechanism that can be used to address this behaviour in its earlier
patches strongly suggests to the users that this is considered a bug
by the author of the series to the user from early on, so adding
this as "expect failure" and then flip it to "expect success" when
the bug is fixed would be a more natural sequence of changes.
Thanks.
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 06/10] t7006: add tests for how git tag paginates
2017-07-31 16:37 ` Junio C Hamano
@ 2017-07-31 17:50 ` Martin Ågren
0 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-07-31 17:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Git Mailing List, Brandon Williams
On 31 July 2017 at 18:37, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> But here...
>>
>>> +test_expect_success TTY 'git tag -a respects pager.tag' '
>>> + test_when_finished "git tag -d newtag" &&
>>> + rm -f paginated.out &&
>>> + test_terminal git -c pager.tag tag -am message newtag &&
>>> + test -e paginated.out
>>> +'
>>
>> I think this behavior is just buggy, and it might be better introduced
>> as a test_expect_failure on "git tag -a does not respect pager.tag".
>>
>> Kind of a minor nit, as the series should end up in the right place
>> either way, but it can be helpful if you end up digging back in history
>> to the introduction of the test.
>
> Yes, I think that is essentially the same reaction I had to patches
> 7 and 8, where it carries the "buggy" behaviour forward and then
> fixes it. The way the series lays groundwork to introduce a
> mechanism that can be used to address this behaviour in its earlier
> patches strongly suggests to the users that this is considered a bug
> by the author of the series to the user from early on, so adding
> this as "expect failure" and then flip it to "expect success" when
> the bug is fixed would be a more natural sequence of changes.
Thanks both for very helpful comments. I admit I viewed it less as
"fix buggy behavior" and more like "redefine wanted behavior". So I
wanted to postpone the redefinition of the behavior until all the
restructuring was done. Looking at this as a bug-fix does make
carefully moving the bug forward look rather silly.
I haven't responded to each of your suggestions individually where the
answers would have been a mere "thanks, will do". They're still much
appreciated and will help make v3 much better. Thanks.
Martin
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 07/10] tag: handle `pager.tag`-configuration within the builtin
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
` (5 preceding siblings ...)
2017-07-17 20:10 ` [PATCH v2 06/10] t7006: add tests for how git tag paginates Martin Ågren
@ 2017-07-17 20:10 ` Martin Ågren
2017-07-17 20:10 ` [PATCH v2 08/10] tag: respect `pager.tag` in list-mode only Martin Ågren
` (5 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-07-17 20:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams
Use the mechanisms introduced in two earlier patches to ignore
`pager.tag` in git.c and let the `git tag` builtin handle it on its own.
This is in preparation for the next patch, where we will want to ignore
`pager.tag` unless we run in list-mode. For this reason, place the call
to setup_auto_pager() after the options have been parsed and we have
decided whether we are in list-mode.
No functional change is intended. That said, there is a window between
where the pager is started before and after this patch, and if an error
occurs within this window, as of this patch the error message might not
be paged where it would have been paged before. Since
operation-parsing has to happen inside this window, a difference can be
seen with, e.g., `git -c pager.tag="echo pager is used" tag
--unknown-option`. This change in paging-behavior should be acceptable
since it only affects erroneous usages.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
builtin/tag.c | 2 ++
git.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 01154ea8d..9eda434fb 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -461,6 +461,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
cmdmode = 'l';
}
+ setup_auto_pager("tag", 0);
+
if ((create_tag_object || force) && (cmdmode != 0))
usage_with_options(git_tag_usage, options);
diff --git a/git.c b/git.c
index 66832f232..82ac2a092 100644
--- a/git.c
+++ b/git.c
@@ -466,7 +466,7 @@ static struct cmd_struct commands[] = {
{ "stripspace", cmd_stripspace },
{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
- { "tag", cmd_tag, RUN_SETUP },
+ { "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },
{ "unpack-file", cmd_unpack_file, RUN_SETUP },
{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
{ "update-index", cmd_update_index, RUN_SETUP },
--
2.14.0.rc0
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v2 08/10] tag: respect `pager.tag` in list-mode only
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
` (6 preceding siblings ...)
2017-07-17 20:10 ` [PATCH v2 07/10] tag: handle `pager.tag`-configuration within the builtin Martin Ågren
@ 2017-07-17 20:10 ` Martin Ågren
2017-07-31 3:40 ` Jeff King
2017-07-17 20:10 ` [PATCH v2 09/10] tag: change default of `pager.tag` to "on" Martin Ågren
` (4 subsequent siblings)
12 siblings, 1 reply; 69+ messages in thread
From: Martin Ågren @ 2017-07-17 20:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams
Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as
"Vim: Warning: Output is not to a terminal" and a garbled terminal. A
user who makes use of `git tag -a` and `git tag -l` will probably choose
not to configure `pager.tag` or to set it to "no", so that `git tag -a`
will actually work, at the cost of not getting the pager with `git tag
-l`.
Teach git tag to only respect `pager.tag` when running in list-mode.
Update the documentation and update tests.
If an alias is used to run `git tag -a`, then `pager.tag` will still be
respected. Document this known breakage. It will be fixed in a later
commit. Add a similar test for `-l`, which works as it should.
Noticed-by: Anatoly Borodin <anatoly.borodin@gmail.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
Documentation/git-tag.txt | 3 +++
| 25 +++++++++++++++++++++----
builtin/tag.c | 3 ++-
3 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 1eb15afa1..875d135e0 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -205,6 +205,9 @@ it in the repository configuration as follows:
signingKey = <gpg-keyid>
-------------------------------------
+`pager.tag` is only respected when listing tags, i.e., when `-l` is
+used or implied.
+See linkgit:git-config[1].
DISCUSSION
----------
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index e7430bc93..a357436e1 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -187,18 +187,35 @@ test_expect_success TTY 'git tag -a defaults to not paging' '
! test -e paginated.out
'
-test_expect_success TTY 'git tag -a respects pager.tag' '
+test_expect_success TTY 'git tag -a ignores pager.tag' '
test_when_finished "git tag -d newtag" &&
rm -f paginated.out &&
test_terminal git -c pager.tag tag -am message newtag &&
- test -e paginated.out
+ ! test -e paginated.out
'
test_expect_success TTY 'git tag -a respects --paginate' '
test_when_finished "git tag -d newtag" &&
rm -f paginated.out &&
- test_terminal git -c pager.tag=false --paginate \
- tag -am message newtag &&
+ test_terminal git --paginate tag -am message newtag &&
+ test -e paginated.out
+'
+
+test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
+ # git-tag will be launched as a dashed external, which
+ # 1) is the source of a potential bug, and
+ # 2) is why we use test_config and not -c.
+ test_when_finished "git tag -d newtag" &&
+ rm -f paginated.out &&
+ test_config pager.tag true &&
+ test_terminal git -c alias.t=tag t -am message newtag &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
+ rm -f paginated.out &&
+ test_config pager.tag true &&
+ test_terminal git -c alias.t=tag t -l &&
test -e paginated.out
'
diff --git a/builtin/tag.c b/builtin/tag.c
index 9eda434fb..5ad1af252 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -461,7 +461,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
cmdmode = 'l';
}
- setup_auto_pager("tag", 0);
+ if (cmdmode == 'l')
+ setup_auto_pager("tag", 0);
if ((create_tag_object || force) && (cmdmode != 0))
usage_with_options(git_tag_usage, options);
--
2.14.0.rc0
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v2 08/10] tag: respect `pager.tag` in list-mode only
2017-07-17 20:10 ` [PATCH v2 08/10] tag: respect `pager.tag` in list-mode only Martin Ågren
@ 2017-07-31 3:40 ` Jeff King
0 siblings, 0 replies; 69+ messages in thread
From: Jeff King @ 2017-07-31 3:40 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Junio C Hamano, Brandon Williams
On Mon, Jul 17, 2017 at 10:10:50PM +0200, Martin Ågren wrote:
> test_expect_success TTY 'git tag -a respects --paginate' '
> test_when_finished "git tag -d newtag" &&
> rm -f paginated.out &&
> - test_terminal git -c pager.tag=false --paginate \
> - tag -am message newtag &&
> + test_terminal git --paginate tag -am message newtag &&
> + test -e paginated.out
> +'
This changes, I guess, because pager.tag should not be having any impact
at all. So it would not hurt to leave it, but the in the final state of
the test what is interesting is that "--paginate" kicks in. I do wonder
if it could just drop the pager.tag bit when it is added in the first
place. We test elsewhere that the command-line overrides the config (for
a case that actually _does_ respect the config).
I'm OK with it either way, though.
-Peff
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 09/10] tag: change default of `pager.tag` to "on"
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
` (7 preceding siblings ...)
2017-07-17 20:10 ` [PATCH v2 08/10] tag: respect `pager.tag` in list-mode only Martin Ågren
@ 2017-07-17 20:10 ` Martin Ågren
2017-07-17 20:10 ` [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external Martin Ågren
` (3 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-07-17 20:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams
The previous patch taught `git tag` to only respect `pager.tag` in
list-mode. That patch left the default value of `pager.tag` at "off".
After that patch, it makes sense to let the default value be "on"
instead, since it will help with listing many tags, but will not hurt
users of `git tag -a` as it would have before. Make that change. Update
documentation and tests.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
Documentation/git-tag.txt | 2 +-
| 28 ++++++++++++++--------------
builtin/tag.c | 2 +-
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 875d135e0..d97aad343 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -206,7 +206,7 @@ it in the repository configuration as follows:
-------------------------------------
`pager.tag` is only respected when listing tags, i.e., when `-l` is
-used or implied.
+used or implied. The default is to use a pager.
See linkgit:git-config[1].
DISCUSSION
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index a357436e1..df258c5d4 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,16 +134,16 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
}
'
-test_expect_success TTY 'git tag -l defaults to not paging' '
+test_expect_success TTY 'git tag -l defaults to paging' '
rm -f paginated.out &&
test_terminal git tag -l &&
- ! test -e paginated.out
+ test -e paginated.out
'
test_expect_success TTY 'git tag -l respects pager.tag' '
rm -f paginated.out &&
- test_terminal git -c pager.tag tag -l &&
- test -e paginated.out
+ test_terminal git -c pager.tag=false tag -l &&
+ ! test -e paginated.out
'
test_expect_success TTY 'git tag -l respects --no-pager' '
@@ -152,32 +152,32 @@ test_expect_success TTY 'git tag -l respects --no-pager' '
! test -e paginated.out
'
-test_expect_success TTY 'git tag with no args defaults to not paging' '
+test_expect_success TTY 'git tag with no args defaults to paging' '
# no args implies -l so this should page like -l
rm -f paginated.out &&
test_terminal git tag &&
- ! test -e paginated.out
+ test -e paginated.out
'
test_expect_success TTY 'git tag with no args respects pager.tag' '
# no args implies -l so this should page like -l
rm -f paginated.out &&
- test_terminal git -c pager.tag tag &&
- test -e paginated.out
+ test_terminal git -c pager.tag=no tag &&
+ ! test -e paginated.out
'
-test_expect_success TTY 'git tag --contains defaults to not paging' '
+test_expect_success TTY 'git tag --contains defaults to paging' '
# --contains implies -l so this should page like -l
rm -f paginated.out &&
test_terminal git tag --contains &&
- ! test -e paginated.out
+ test -e paginated.out
'
test_expect_success TTY 'git tag --contains respects pager.tag' '
# --contains implies -l so this should page like -l
rm -f paginated.out &&
- test_terminal git -c pager.tag tag --contains &&
- test -e paginated.out
+ test_terminal git -c pager.tag=false tag --contains &&
+ ! test -e paginated.out
'
test_expect_success TTY 'git tag -a defaults to not paging' '
@@ -214,9 +214,9 @@ test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
rm -f paginated.out &&
- test_config pager.tag true &&
+ test_config pager.tag false &&
test_terminal git -c alias.t=tag t -l &&
- test -e paginated.out
+ ! test -e paginated.out
'
# A colored commit log will begin with an appropriate ANSI escape
diff --git a/builtin/tag.c b/builtin/tag.c
index 5ad1af252..ea83df5e1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -462,7 +462,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
}
if (cmdmode == 'l')
- setup_auto_pager("tag", 0);
+ setup_auto_pager("tag", 1);
if ((create_tag_object || force) && (cmdmode != 0))
usage_with_options(git_tag_usage, options);
--
2.14.0.rc0
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
` (8 preceding siblings ...)
2017-07-17 20:10 ` [PATCH v2 09/10] tag: change default of `pager.tag` to "on" Martin Ågren
@ 2017-07-17 20:10 ` Martin Ågren
2017-07-31 3:45 ` Jeff King
2017-07-18 19:13 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Junio C Hamano
` (2 subsequent siblings)
12 siblings, 1 reply; 69+ messages in thread
From: Martin Ågren @ 2017-07-17 20:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams
When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and
execute `git-foo` as a dashed external. This is true even if git foo is
a builtin. That is on purpose, and is motivated in a comment which was
added in commit 441981bc ("git: simplify environment save/restore
logic", 2016-01-26).
Shortly before we launch a dashed external, and unless we have already
found out whether we should use a pager, we check `pager.foo`. This was
added in commit 92058e4d ("support pager.* for external commands",
2011-08-18). If the dashed external is a builtin, this does not match
that commit's intention and is arguably wrong, since it would be cleaner
if we let the "dashed external builtin" handle `pager.foo`.
This has not mattered in practice, but a recent patch taught `git-tag`
to ignore `pager.tag` under certain circumstances. But, when started
using an alias, it doesn't get the chance to do so, as outlined above.
That recent patch added a test to document this breakage.
Do not check `pager.foo` before launching a builtin as a dashed
external, i.e., if we recognize the name of the external as a builtin.
Change the test to use `test_expect_success`.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
One could address this in run_argv(), by making the second call to
execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
would be started as "git foo". (Possibly after unrolling and cleaning up
the "while (1)"-loop.) That seems like the wrong fix for this particular
issue, but might be a wanted change on its own -- or maybe not --, since
it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
only for builtins...).
| 2 +-
git.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index df258c5d4..8b2ffb1aa 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -201,7 +201,7 @@ test_expect_success TTY 'git tag -a respects --paginate' '
test -e paginated.out
'
-test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
+test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
# git-tag will be launched as a dashed external, which
# 1) is the source of a potential bug, and
# 2) is why we use test_config and not -c.
diff --git a/git.c b/git.c
index 82ac2a092..6b6d9f68e 100644
--- a/git.c
+++ b/git.c
@@ -559,7 +559,7 @@ static void execv_dashed_external(const char **argv)
if (get_super_prefix())
die("%s doesn't support --super-prefix", argv[0]);
- if (use_pager == -1)
+ if (use_pager == -1 && !is_builtin(argv[0]))
use_pager = check_pager_config(argv[0]);
commit_pager_choice();
--
2.14.0.rc0
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external
2017-07-17 20:10 ` [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external Martin Ågren
@ 2017-07-31 3:45 ` Jeff King
2017-07-31 17:42 ` Martin Ågren
0 siblings, 1 reply; 69+ messages in thread
From: Jeff King @ 2017-07-31 3:45 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Junio C Hamano, Brandon Williams
On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:
> When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and
> execute `git-foo` as a dashed external. This is true even if git foo is
> a builtin. That is on purpose, and is motivated in a comment which was
> added in commit 441981bc ("git: simplify environment save/restore
> logic", 2016-01-26).
>
> Shortly before we launch a dashed external, and unless we have already
> found out whether we should use a pager, we check `pager.foo`. This was
> added in commit 92058e4d ("support pager.* for external commands",
> 2011-08-18). If the dashed external is a builtin, this does not match
> that commit's intention and is arguably wrong, since it would be cleaner
> if we let the "dashed external builtin" handle `pager.foo`.
>
> This has not mattered in practice, but a recent patch taught `git-tag`
> to ignore `pager.tag` under certain circumstances. But, when started
> using an alias, it doesn't get the chance to do so, as outlined above.
> That recent patch added a test to document this breakage.
>
> Do not check `pager.foo` before launching a builtin as a dashed
> external, i.e., if we recognize the name of the external as a builtin.
> Change the test to use `test_expect_success`.
I think floating this change to the end like this has made it much
easier to see why we must do it. The end result looks good to me.
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> One could address this in run_argv(), by making the second call to
> execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
> would be started as "git foo". (Possibly after unrolling and cleaning up
> the "while (1)"-loop.) That seems like the wrong fix for this particular
> issue, but might be a wanted change on its own -- or maybe not --, since
> it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
> only for builtins...).
We shouldn't need to relay them. They get added to the environment by
the initial "git" invocation, and then are available everywhere (in
fact, it would be wrong to relay them for multi-valued config). Or did
you mean that we could potentially allow:
[alias]
foo = "-c baz some-builtin"
That's interesting, but I think the fact that it only works with
builtins makes it a bad idea. And you can always do:
[alias]
foo = "!git -c baz some-builtin"
which is equivalent.
-Peff
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external
2017-07-31 3:45 ` Jeff King
@ 2017-07-31 17:42 ` Martin Ågren
0 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-07-31 17:42 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Brandon Williams
On 31 July 2017 at 05:45, Jeff King <peff@peff.net> wrote:
> On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:
>
>> One could address this in run_argv(), by making the second call to
>> execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
>> would be started as "git foo". (Possibly after unrolling and cleaning up
>> the "while (1)"-loop.) That seems like the wrong fix for this particular
>> issue, but might be a wanted change on its own -- or maybe not --, since
>> it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
>> only for builtins...).
>
> We shouldn't need to relay them. They get added to the environment by
> the initial "git" invocation, and then are available everywhere (in
> fact, it would be wrong to relay them for multi-valued config).
Thanks for explaining. I did some very sloppy reading of the comment
in git.c that we "cannot take flags in between the 'git' and the
'xxxx'" which I somehow misunderstood completely as "we cannot pass
that sort of information to git-xxxx". Silly. Thanks for taking the
time to explain what I should have found out myself...
So yeah, I meant the above and not this:
> Or did
> you mean that we could potentially allow:
>
> [alias]
> foo = "-c baz some-builtin"
>
> That's interesting, but I think the fact that it only works with
> builtins makes it a bad idea. And you can always do:
>
> [alias]
> foo = "!git -c baz some-builtin"
>
> which is equivalent.
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
` (9 preceding siblings ...)
2017-07-17 20:10 ` [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external Martin Ågren
@ 2017-07-18 19:13 ` Junio C Hamano
2017-07-20 22:27 ` Junio C Hamano
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
12 siblings, 0 replies; 69+ messages in thread
From: Junio C Hamano @ 2017-07-18 19:13 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Jeff King, Brandon Williams
Martin Ågren <martin.agren@gmail.com> writes:
> After that feedback, v2 drops `pager.tag.list` and instead teaches
> `git tag` to only consider `pager.tag` in list-mode, as suggested by
> Peff.
That does sound like a more sensible and safer approach. I may have
comments on individual patches, which I will send while I read them.
Thanks.
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
` (10 preceding siblings ...)
2017-07-18 19:13 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Junio C Hamano
@ 2017-07-20 22:27 ` Junio C Hamano
2017-07-23 18:17 ` Martin Ågren
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
12 siblings, 1 reply; 69+ messages in thread
From: Junio C Hamano @ 2017-07-20 22:27 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Jeff King, Brandon Williams
Martin Ågren <martin.agren@gmail.com> writes:
> This is the second version of "[PATCH 0/7] tag: more fine-grained
> pager-configuration" [1]. That series introduced `pager.tag.list` to
> address the fact that `pager.tag` can be useful with `git tag -l` but
> actively hostile with `git tag -a`. Thanks to Junio, Peff and Brandon
> for helpful feedback.
>
> After that feedback, v2 drops `pager.tag.list` and instead teaches
> `git tag` to only consider `pager.tag` in list-mode, as suggested by
> Peff.
>
> Patches 1-3/10 replace patch 1/7. They move Documentation/technical/
> api-builtin.txt into builtin.h, tweak the formatting and bring it up to
> date. I may have gone overboard making this 3 patches...
>
> Patches 4-7/10 correspond to patches 2-5/7. `setup_auto_pager()' is now
> much simpler since we do not need to handle "tag.list" with a clever
> fallback strategy. IGNORE_PAGER_CONFIG is now called DELAY_PAGER_CONFIG.
> I now check with pager_in_use() and I moved the handling of `pager.tag`
> a bit further down.
I tend to agree with you that 1-3/10 may be better off being a
single patch (or 3/10 dropped, as Brandon is working on losing it
nearby). I would have expected 7-8/10 to be a single patch, as by
the time a reader reaches 07/10, because of the groundwork laid by
04-06/10, it is obvious that the general direction is to allow the
caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
some but not all circumstances, and 07/10 being faithful to the
original behaviour (only to be updated in 08/10) is somewhat counter
intuitive. It is not wrong per-se; it was just unexpected.
> Patches 8-9/10 teach `git tag` to only respect `pager.tag` in list-mode
> and flip the default value for that config to "on".
>
> Patch 10/10 is somewhat similar to a hunk in patch 2/7, but is now a
> bug-fix instead of a feature. It teaches `execv_dashed_external()` not
> to check `pager.foo` when launching `git-foo` where foo is a builtin.
> I waffled about where to put this patch. Putting it earlier in the
> series as a preparatory step, I couldn't come up with a way of writing a
> test. So patch 8/10 introduces a `test_expect_failure` which this patch
> then fixes.
I haven't thought about ramifications of 9-10/10 to make a comment
yet, but overall the series was a pleasant read.
Thanks.
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode
2017-07-20 22:27 ` Junio C Hamano
@ 2017-07-23 18:17 ` Martin Ågren
2017-07-31 3:46 ` Jeff King
0 siblings, 1 reply; 69+ messages in thread
From: Martin Ågren @ 2017-07-23 18:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Jeff King, Brandon Williams
On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
> I tend to agree with you that 1-3/10 may be better off being a
> single patch (or 3/10 dropped, as Brandon is working on losing it
> nearby). I would have expected 7-8/10 to be a single patch, as by
> the time a reader reaches 07/10, because of the groundwork laid by
> 04-06/10, it is obvious that the general direction is to allow the
> caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
> some but not all circumstances, and 07/10 being faithful to the
> original behaviour (only to be updated in 08/10) is somewhat counter
> intuitive. It is not wrong per-se; it was just unexpected.
Thanks for your comments. I will be away for a few days, but once I
get back, I'll try to produce a v3 based on this and any further
feedback.
Martin
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode
2017-07-23 18:17 ` Martin Ågren
@ 2017-07-31 3:46 ` Jeff King
2017-07-31 17:44 ` Martin Ågren
0 siblings, 1 reply; 69+ messages in thread
From: Jeff King @ 2017-07-31 3:46 UTC (permalink / raw)
To: Martin Ågren; +Cc: Junio C Hamano, Git Mailing List, Brandon Williams
On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:
> On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
> > I tend to agree with you that 1-3/10 may be better off being a
> > single patch (or 3/10 dropped, as Brandon is working on losing it
> > nearby). I would have expected 7-8/10 to be a single patch, as by
> > the time a reader reaches 07/10, because of the groundwork laid by
> > 04-06/10, it is obvious that the general direction is to allow the
> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
> > some but not all circumstances, and 07/10 being faithful to the
> > original behaviour (only to be updated in 08/10) is somewhat counter
> > intuitive. It is not wrong per-se; it was just unexpected.
>
> Thanks for your comments. I will be away for a few days, but once I
> get back, I'll try to produce a v3 based on this and any further
> feedback.
Overall it looks good to me. I left a few minor comments.
I actually like the split. I found it pretty easy to follow (though
squashing as Junio suggested would be fine with me, too).
Thanks again for working on this.
-Peff
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode
2017-07-31 3:46 ` Jeff King
@ 2017-07-31 17:44 ` Martin Ågren
2017-07-31 17:45 ` Jeff King
0 siblings, 1 reply; 69+ messages in thread
From: Martin Ågren @ 2017-07-31 17:44 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git Mailing List, Brandon Williams
On 31 July 2017 at 05:46, Jeff King <peff@peff.net> wrote:
> On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:
>
>> On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
>> > I tend to agree with you that 1-3/10 may be better off being a
>> > single patch (or 3/10 dropped, as Brandon is working on losing it
>> > nearby). I would have expected 7-8/10 to be a single patch, as by
>> > the time a reader reaches 07/10, because of the groundwork laid by
>> > 04-06/10, it is obvious that the general direction is to allow the
>> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
>> > some but not all circumstances, and 07/10 being faithful to the
>> > original behaviour (only to be updated in 08/10) is somewhat counter
>> > intuitive. It is not wrong per-se; it was just unexpected.
>>
>> Thanks for your comments. I will be away for a few days, but once I
>> get back, I'll try to produce a v3 based on this and any further
>> feedback.
>
> Overall it looks good to me. I left a few minor comments.
>
> I actually like the split. I found it pretty easy to follow (though
> squashing as Junio suggested would be fine with me, too).
I assume that by "the split" you mean patches 7-8, not 1-3. Anyway,
I'll squash since you're fine with it and Junio prefers it.
Martin
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode
2017-07-31 17:44 ` Martin Ågren
@ 2017-07-31 17:45 ` Jeff King
2017-07-31 20:10 ` Junio C Hamano
0 siblings, 1 reply; 69+ messages in thread
From: Jeff King @ 2017-07-31 17:45 UTC (permalink / raw)
To: Martin Ågren; +Cc: Junio C Hamano, Git Mailing List, Brandon Williams
On Mon, Jul 31, 2017 at 07:44:27PM +0200, Martin Ågren wrote:
> On 31 July 2017 at 05:46, Jeff King <peff@peff.net> wrote:
> > On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:
> >
> >> On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
> >> > I tend to agree with you that 1-3/10 may be better off being a
> >> > single patch (or 3/10 dropped, as Brandon is working on losing it
> >> > nearby). I would have expected 7-8/10 to be a single patch, as by
> >> > the time a reader reaches 07/10, because of the groundwork laid by
> >> > 04-06/10, it is obvious that the general direction is to allow the
> >> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
> >> > some but not all circumstances, and 07/10 being faithful to the
> >> > original behaviour (only to be updated in 08/10) is somewhat counter
> >> > intuitive. It is not wrong per-se; it was just unexpected.
> >>
> >> Thanks for your comments. I will be away for a few days, but once I
> >> get back, I'll try to produce a v3 based on this and any further
> >> feedback.
> >
> > Overall it looks good to me. I left a few minor comments.
> >
> > I actually like the split. I found it pretty easy to follow (though
> > squashing as Junio suggested would be fine with me, too).
>
> I assume that by "the split" you mean patches 7-8, not 1-3. Anyway,
> I'll squash since you're fine with it and Junio prefers it.
I actually meant both, but as I said, I'm OK with it either way.
-Peff
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode
2017-07-31 17:45 ` Jeff King
@ 2017-07-31 20:10 ` Junio C Hamano
0 siblings, 0 replies; 69+ messages in thread
From: Junio C Hamano @ 2017-07-31 20:10 UTC (permalink / raw)
To: Jeff King; +Cc: Martin Ågren, Git Mailing List, Brandon Williams
Jeff King <peff@peff.net> writes:
> On Mon, Jul 31, 2017 at 07:44:27PM +0200, Martin Ågren wrote:
>
>> On 31 July 2017 at 05:46, Jeff King <peff@peff.net> wrote:
>> > On Sun, Jul 23, 2017 at 08:17:42PM +0200, Martin Ågren wrote:
>> >
>> >> On 21 July 2017 at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
>> >> > I tend to agree with you that 1-3/10 may be better off being a
>> >> > single patch (or 3/10 dropped, as Brandon is working on losing it
>> >> > nearby). I would have expected 7-8/10 to be a single patch, as by
>> >> > the time a reader reaches 07/10, because of the groundwork laid by
>> >> > 04-06/10, it is obvious that the general direction is to allow the
>> >> > caller, i.e. cmd_tag(), to make a call to setup_auto_pager() only in
>> >> > some but not all circumstances, and 07/10 being faithful to the
>> >> > original behaviour (only to be updated in 08/10) is somewhat counter
>> >> > intuitive. It is not wrong per-se; it was just unexpected.
>> >>
>> >> Thanks for your comments. I will be away for a few days, but once I
>> >> get back, I'll try to produce a v3 based on this and any further
>> >> feedback.
>> >
>> > Overall it looks good to me. I left a few minor comments.
>> >
>> > I actually like the split. I found it pretty easy to follow (though
>> > squashing as Junio suggested would be fine with me, too).
>>
>> I assume that by "the split" you mean patches 7-8, not 1-3. Anyway,
>> I'll squash since you're fine with it and Junio prefers it.
>
> I actually meant both, but as I said, I'm OK with it either way.
I am OK with it either way, too ;-)
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode
2017-07-17 20:10 ` [PATCH v2 00/10] tag: only respect `pager.tag` in list-mode Martin Ågren
` (11 preceding siblings ...)
2017-07-20 22:27 ` Junio C Hamano
@ 2017-08-02 19:40 ` Martin Ågren
2017-08-02 19:40 ` [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt Martin Ågren
` (8 more replies)
12 siblings, 9 replies; 69+ messages in thread
From: Martin Ågren @ 2017-08-02 19:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
This is the third version of my attempt to make `pager tag` useful (v1
at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.
I've squashed patches 01-03/10 and 07-08/10, respectively. The interdiff
is below. I managed to clean up some tests thanks to a drive-by comment
by Peff which cleared up a misunderstanding I had. Some minor changes,
e.g., I write "built-in" instead of "builtin", since that seemed
preferred, at least locally in builtin.h. I documented why a default
value of "punt" could be useful, but also that it's probably not wanted.
Martin
[1] https://public-inbox.org/git/cover.1499723297.git.martin.agren@gmail.com/T/
[2] https://public-inbox.org/git/cover.1500321657.git.martin.agren@gmail.com/T/#u
Martin Ågren (7):
builtin.h: take over documentation from api-builtin.txt
git.c: let builtins opt for handling `pager.foo` themselves
git.c: provide setup_auto_pager()
t7006: add tests for how git tag paginates
tag: respect `pager.tag` in list-mode only
tag: change default of `pager.tag` to "on"
git.c: ignore pager.* when launching builtin as dashed external
Documentation/git-tag.txt | 3 +
Documentation/technical/api-builtin.txt | 73 -----------------------
| 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:
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 8b2ffb1aa..9128ec5ac 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '
test_expect_success TTY 'git tag with no args respects pager.tag' '
# no args implies -l so this should page like -l
rm -f paginated.out &&
- test_terminal git -c pager.tag=no tag &&
+ test_terminal git -c pager.tag=false tag &&
! test -e paginated.out
'
@@ -202,20 +202,15 @@ test_expect_success TTY 'git tag -a respects --paginate' '
'
test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
- # git-tag will be launched as a dashed external, which
- # 1) is the source of a potential bug, and
- # 2) is why we use test_config and not -c.
test_when_finished "git tag -d newtag" &&
rm -f paginated.out &&
- test_config pager.tag true &&
- test_terminal git -c alias.t=tag t -am message newtag &&
+ test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
! test -e paginated.out
'
test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
rm -f paginated.out &&
- test_config pager.tag false &&
- test_terminal git -c alias.t=tag t -l &&
+ test_terminal git -c pager.tag=false -c alias.t=tag t -l &&
! test -e paginated.out
'
diff --git a/builtin.h b/builtin.h
index 3ca4a53a8..42378f3aa 100644
--- a/builtin.h
+++ b/builtin.h
@@ -51,15 +51,15 @@
* on bare repositories.
* This only makes sense when `RUN_SETUP` is also set.
*
- * `SUPPORT_SUPER_PREFIX`::
+ * `SUPPORT_SUPER_PREFIX`:
*
- * The builtin supports `--super-prefix`.
+ * The built-in supports `--super-prefix`.
*
- * `DELAY_PAGER_CONFIG`::
+ * `DELAY_PAGER_CONFIG`:
*
* If RUN_SETUP or RUN_SETUP_GENTLY is set, git.c normally handles
* the `pager.<cmd>`-configuration. If this flag is used, git.c
- * will skip that step, instead allowing the builtin to make a
+ * will skip that step, instead allowing the built-in to make a
* more informed decision, e.g., by ignoring `pager.<cmd>` for
* certain subcommands.
*
@@ -114,11 +114,14 @@ extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
struct fmt_merge_msg_opts *);
/**
- * If a builtin has DELAY_PAGER_CONFIG set, the builtin should call this early
+ * If a built-in has DELAY_PAGER_CONFIG set, the built-in should call this early
* when it wishes to respect the `pager.foo`-config. The `cmd` is the name of
- * the builtin, e.g., "foo". If a paging-choice has already been setup, this
+ * the built-in, e.g., "foo". If a paging-choice has already been setup, this
* does nothing. The default in `def` should be 0 for "pager off", 1 for "pager
* on" or -1 for "punt".
+ *
+ * You should most likely use a default of 0 or 1. "Punt" (-1) could be useful
+ * to be able to fall back to some historical compatibility name.
*/
extern void setup_auto_pager(const char *cmd, int def);
--
2.14.0.rc1.12.ge2d9c4613
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
@ 2017-08-02 19:40 ` Martin Ågren
2017-08-03 17:44 ` Junio C Hamano
2017-08-02 19:40 ` [PATCH v3 2/7] git.c: let builtins opt for handling `pager.foo` themselves Martin Ågren
` (7 subsequent siblings)
8 siblings, 1 reply; 69+ messages in thread
From: Martin Ågren @ 2017-08-02 19:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
Delete Documentation/technical/api-builtin.txt and move its content
into builtin.h. Format it as a comment. Remove a '+' which was needed
when the information was formatted for AsciiDoc. Similarly, change
"::" to ":".
Document SUPPORT_SUPER_PREFIX, thereby bringing the documentation up to
date with the available flags.
While at it, correct '3 more things to do' to '4 more things to do'.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
I'm still documenting SUPPORT_SUPER_PREFIX. As Junio pointed out, a
nearby patch series is working on losing it (or one user of it). I tried
removing that part, but felt like I was leaving things incomplete.
Documentation/technical/api-builtin.txt | 73 ------------------------------
builtin.h | 80 +++++++++++++++++++++++++++++++++
2 files changed, 80 insertions(+), 73 deletions(-)
delete mode 100644 Documentation/technical/api-builtin.txt
diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
deleted file mode 100644
index 22a39b929..000000000
--- a/Documentation/technical/api-builtin.txt
+++ /dev/null
@@ -1,73 +0,0 @@
-builtin API
-===========
-
-Adding a new built-in
----------------------
-
-There are 4 things to do to add a built-in command implementation to
-Git:
-
-. Define the implementation of the built-in command `foo` with
- signature:
-
- int cmd_foo(int argc, const char **argv, const char *prefix);
-
-. Add the external declaration for the function to `builtin.h`.
-
-. Add the command to the `commands[]` table defined in `git.c`.
- The entry should look like:
-
- { "foo", cmd_foo, <options> },
-+
-where options is the bitwise-or of:
-
-`RUN_SETUP`::
- If there is not a Git directory to work on, abort. If there
- is a work tree, chdir to the top of it if the command was
- invoked in a subdirectory. If there is no work tree, no
- chdir() is done.
-
-`RUN_SETUP_GENTLY`::
- If there is a Git directory, chdir as per RUN_SETUP, otherwise,
- don't chdir anywhere.
-
-`USE_PAGER`::
-
- If the standard output is connected to a tty, spawn a pager and
- feed our output to it.
-
-`NEED_WORK_TREE`::
-
- Make sure there is a work tree, i.e. the command cannot act
- on bare repositories.
- This only makes sense when `RUN_SETUP` is also set.
-
-. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
-
-Additionally, if `foo` is a new command, there are 3 more things to do:
-
-. Add tests to `t/` directory.
-
-. Write documentation in `Documentation/git-foo.txt`.
-
-. Add an entry for `git-foo` to `command-list.txt`.
-
-. Add an entry for `/git-foo` to `.gitignore`.
-
-
-How a built-in is called
-------------------------
-
-The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
-and `prefix`. The first two are similar to what `main()` of a
-standalone command would be called with.
-
-When `RUN_SETUP` is specified in the `commands[]` table, and when you
-were started from a subdirectory of the work tree, `cmd_foo()` is called
-after chdir(2) to the top of the work tree, and `prefix` gets the path
-to the subdirectory the command started from. This allows you to
-convert a user-supplied pathname (typically relative to that directory)
-to a pathname relative to the top of the work tree.
-
-The return value from `cmd_foo()` becomes the exit status of the
-command.
diff --git a/builtin.h b/builtin.h
index 498ac80d0..8d87d06da 100644
--- a/builtin.h
+++ b/builtin.h
@@ -6,6 +6,86 @@
#include "cache.h"
#include "commit.h"
+/*
+ * builtin API
+ * ===========
+ *
+ * Adding a new built-in
+ * ---------------------
+ *
+ * There are 4 things to do to add a built-in command implementation to
+ * Git:
+ *
+ * . Define the implementation of the built-in command `foo` with
+ * signature:
+ *
+ * int cmd_foo(int argc, const char **argv, const char *prefix);
+ *
+ * . Add the external declaration for the function to `builtin.h`.
+ *
+ * . Add the command to the `commands[]` table defined in `git.c`.
+ * The entry should look like:
+ *
+ * { "foo", cmd_foo, <options> },
+ *
+ * where options is the bitwise-or of:
+ *
+ * `RUN_SETUP`:
+ * If there is not a Git directory to work on, abort. If there
+ * is a work tree, chdir to the top of it if the command was
+ * invoked in a subdirectory. If there is no work tree, no
+ * chdir() is done.
+ *
+ * `RUN_SETUP_GENTLY`:
+ * If there is a Git directory, chdir as per RUN_SETUP, otherwise,
+ * don't chdir anywhere.
+ *
+ * `USE_PAGER`:
+ *
+ * If the standard output is connected to a tty, spawn a pager and
+ * feed our output to it.
+ *
+ * `NEED_WORK_TREE`:
+ *
+ * Make sure there is a work tree, i.e. the command cannot act
+ * on bare repositories.
+ * This only makes sense when `RUN_SETUP` is also set.
+ *
+ * `SUPPORT_SUPER_PREFIX`:
+ *
+ * The built-in supports `--super-prefix`.
+ *
+ * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
+ *
+ * Additionally, if `foo` is a new command, there are 4 more things to do:
+ *
+ * . Add tests to `t/` directory.
+ *
+ * . Write documentation in `Documentation/git-foo.txt`.
+ *
+ * . Add an entry for `git-foo` to `command-list.txt`.
+ *
+ * . Add an entry for `/git-foo` to `.gitignore`.
+ *
+ *
+ * How a built-in is called
+ * ------------------------
+ *
+ * The implementation `cmd_foo()` takes three parameters, `argc`, `argv,
+ * and `prefix`. The first two are similar to what `main()` of a
+ * standalone command would be called with.
+ *
+ * When `RUN_SETUP` is specified in the `commands[]` table, and when you
+ * were started from a subdirectory of the work tree, `cmd_foo()` is called
+ * after chdir(2) to the top of the work tree, and `prefix` gets the path
+ * to the subdirectory the command started from. This allows you to
+ * convert a user-supplied pathname (typically relative to that directory)
+ * to a pathname relative to the top of the work tree.
+ *
+ * The return value from `cmd_foo()` becomes the exit status of the
+ * command.
+ */
+
#define DEFAULT_MERGE_LOG_LEN 20
extern const char git_usage_string[];
--
2.14.0.rc1.12.ge2d9c4613
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt
2017-08-02 19:40 ` [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt Martin Ågren
@ 2017-08-03 17:44 ` Junio C Hamano
2017-08-04 4:18 ` Martin Ågren
0 siblings, 1 reply; 69+ messages in thread
From: Junio C Hamano @ 2017-08-03 17:44 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Jeff King
Martin Ågren <martin.agren@gmail.com> writes:
> diff --git a/builtin.h b/builtin.h
> index 498ac80d0..8d87d06da 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -6,6 +6,86 @@
> #include "cache.h"
> #include "commit.h"
>
> +/*
> + * builtin API
> + * ===========
> + *
> + * Adding a new built-in
> + * ---------------------
> + *
> + * There are 4 things to do to add a built-in command implementation to
> + * Git:
> + *
> + * . Define the implementation of the built-in command `foo` with
> + * signature:
> + *
> + * int cmd_foo(int argc, const char **argv, const char *prefix);
> + *
> + * . Add the external declaration for the function to `builtin.h`.
> + *
> + * . Add the command to the `commands[]` table defined in `git.c`.
> + * The entry should look like:
> + *
> + * { "foo", cmd_foo, <options> },
> + *
> + * where options is the bitwise-or of:
> + *
> + * `RUN_SETUP`:
> + * If there is not a Git directory to work on, abort. If there
> + * is a work tree, chdir to the top of it if the command was
> + * invoked in a subdirectory. If there is no work tree, no
> + * chdir() is done.
> + *
> + * `RUN_SETUP_GENTLY`:
> + * If there is a Git directory, chdir as per RUN_SETUP, otherwise,
> + * don't chdir anywhere.
> + *
> + * `USE_PAGER`:
> + *
> + * If the standard output is connected to a tty, spawn a pager and
> + * feed our output to it.
> + *
> + * `NEED_WORK_TREE`:
> + *
> + * Make sure there is a work tree, i.e. the command cannot act
> + * on bare repositories.
> + * This only makes sense when `RUN_SETUP` is also set.
> + *
> + * `SUPPORT_SUPER_PREFIX`:
> + *
> + * The built-in supports `--super-prefix`.
> + *
> + * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
Not a new problem but it will become much easier to follow if we
moved this item between the "implement cmd_foo()" and "declare
cmd_foo in builtin.h", like so:
. Define the implementation of the built-in command `foo` with
signature:
int cmd_foo(int argc, const char **argv, const char *prefix);
in a new file `builtin/foo.c`.
. Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
Alternatively, we may merge these two into one item (i.e. "in a new
file `builtin/foo.c` and add `builtin/foo.o` to ...").
But of course, this patch 1/7 should not do any of the above. I am
suggesting a possible future clean-up for anybody on the list
listening from sidelines, and you do not have to be the person who
does it.
Thanks.
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt
2017-08-03 17:44 ` Junio C Hamano
@ 2017-08-04 4:18 ` Martin Ågren
2017-08-04 16:00 ` Junio C Hamano
0 siblings, 1 reply; 69+ messages in thread
From: Martin Ågren @ 2017-08-04 4:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Jeff King
On 3 August 2017 at 19:44, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>> + * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
>
> Not a new problem but it will become much easier to follow if we
> moved this item between the "implement cmd_foo()" and "declare
> cmd_foo in builtin.h", like so:
>
> . Define the implementation of the built-in command `foo` with
> signature:
>
> int cmd_foo(int argc, const char **argv, const char *prefix);
>
> in a new file `builtin/foo.c`.
>
> . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
>
> Alternatively, we may merge these two into one item (i.e. "in a new
> file `builtin/foo.c` and add `builtin/foo.o` to ...").
>
> But of course, this patch 1/7 should not do any of the above. I am
> suggesting a possible future clean-up for anybody on the list
> listening from sidelines, and you do not have to be the person who
> does it.
Thank you. If this series needs to be rerolled, I could do it as patch 2.
And if not, I could try to remember to do it once this series has landed.
A that point in time, I'd also like to try changing other commands ("git
branch") similar to "git tag" (although maybe your suggestion above
shouldn't be part of that series, but go on its own).
Since this is my first code contribution to Git, I'll ask about this part of
SubmittingPatches:
"After the list reached a consensus that it is a good idea to apply the
patch, re-send it with "To:" set to the maintainer [*1*] and "cc:" the
list [*2*] for inclusion."
I will boldly assume that I should not be doing this. It seems to me this
doesn't happen very often or not at all -- possibly because you tend to
be involved in virtually all threads anyway, before the list reaches a
consensus.
Which brings me to my final point: Thanks for your very helpful feedback
throughout all three versions and even more thanks for your work on Git.
I find it amazing how much time you are able to constantly spend on all
aspects -- "high and low" -- of Git. It goes a long way to explaining how
Git can be so very useful. Thanks a lot!
Martin
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt
2017-08-04 4:18 ` Martin Ågren
@ 2017-08-04 16:00 ` Junio C Hamano
2017-08-04 16:42 ` Martin Ågren
0 siblings, 1 reply; 69+ messages in thread
From: Junio C Hamano @ 2017-08-04 16:00 UTC (permalink / raw)
To: Martin Ågren; +Cc: Git Mailing List, Jeff King
Martin Ågren <martin.agren@gmail.com> writes:
> Since this is my first code contribution to Git, I'll ask about this part of
> SubmittingPatches:
>
> "After the list reached a consensus that it is a good idea to apply the
> patch, re-send it with "To:" set to the maintainer [*1*] and "cc:" the
> list [*2*] for inclusion."
>
> I will boldly assume that I should not be doing this. It seems to me this
> doesn't happen very often or not at all -- possibly because you tend to
> be involved in virtually all threads anyway, before the list reaches a
> consensus.
Yeah, that is in the "ideal patch flow" section, isn't it? We
rarely achieve the "ideal" and often instead go for a more expedited
option, it appears---perhaps I should try to be less involved in
individual patch reviews and place more review burden on other
reviewers ;-)
In any case, it was a pleasure to cheer-lead on the progress of this
series. Thanks.
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt
2017-08-04 16:00 ` Junio C Hamano
@ 2017-08-04 16:42 ` Martin Ågren
0 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-08-04 16:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Jeff King
On 4 August 2017 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>
>> Since this is my first code contribution to Git, I'll ask about this part of
>> SubmittingPatches:
>>
>> "After the list reached a consensus that it is a good idea to apply the
>> patch, re-send it with "To:" set to the maintainer [*1*] and "cc:" the
>> list [*2*] for inclusion."
>>
>> I will boldly assume that I should not be doing this. It seems to me this
>> doesn't happen very often or not at all -- possibly because you tend to
>> be involved in virtually all threads anyway, before the list reaches a
>> consensus.
>
> Yeah, that is in the "ideal patch flow" section, isn't it?
Yes and no. It's in the main part above it, under "(4) Sending your
patches." But you are right that the ideal patch flow section then
says the same thing: "(4) The list forms consensus that the last
round of your patch is good. Send it to the maintainer and cc the
list."
> We
> rarely achieve the "ideal" and often instead go for a more expedited
> option, it appears---perhaps I should try to be less involved in
> individual patch reviews and place more review burden on other
> reviewers ;-)
:-)
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 2/7] git.c: let builtins opt for handling `pager.foo` themselves
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
2017-08-02 19:40 ` [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt Martin Ågren
@ 2017-08-02 19:40 ` Martin Ågren
2017-08-02 19:40 ` [PATCH v3 3/7] git.c: provide setup_auto_pager() Martin Ågren
` (6 subsequent siblings)
8 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-08-02 19:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
Before launching a builtin git foo and unless mechanisms with precedence
are in use, we check for and handle the `pager.foo` config. This is done
without considering exactly how git foo is being used, and indeed, git.c
cannot (and should not) know what the arguments to git foo are supposed
to achieve.
In practice this means that, e.g., `git -c pager.tag tag -a new-tag`
results in errors such as "Vim: Warning: Output is not to a terminal"
and a garbled terminal. Someone who makes use of both `git tag -a` and
`git tag -l` will probably not set `pager.tag`, so that `git tag -a`
will actually work, at the cost of not paging output of `git tag -l`.
To allow individual builtins to make more informed decisions about when
to respect `pager.foo`, introduce a flag DELAY_PAGER_CONFIG. If the flag
is set, do not check `pager.foo`.
Do not check for DELAY_PAGER_CONFIG in `execv_dashed_external()`. That
call site is arguably wrong, although in a way that is not yet visible,
and will be changed in a slightly different direction in a later patch.
Don't add any users of DELAY_PAGER_CONFIG just yet, one will follow in a
later patch.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
builtin.h | 8 ++++++++
git.c | 4 +++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/builtin.h b/builtin.h
index 8d87d06da..0f3a7b770 100644
--- a/builtin.h
+++ b/builtin.h
@@ -55,6 +55,14 @@
*
* The built-in supports `--super-prefix`.
*
+ * `DELAY_PAGER_CONFIG`:
+ *
+ * If RUN_SETUP or RUN_SETUP_GENTLY is set, git.c normally handles
+ * the `pager.<cmd>`-configuration. If this flag is used, git.c
+ * will skip that step, instead allowing the built-in to make a
+ * more informed decision, e.g., by ignoring `pager.<cmd>` for
+ * certain subcommands.
+ *
* . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
*
* Additionally, if `foo` is a new command, there are 4 more things to do:
diff --git a/git.c b/git.c
index 489aab4d8..79195ebbd 100644
--- a/git.c
+++ b/git.c
@@ -283,6 +283,7 @@ static int handle_alias(int *argcp, const char ***argv)
*/
#define NEED_WORK_TREE (1<<3)
#define SUPPORT_SUPER_PREFIX (1<<4)
+#define DELAY_PAGER_CONFIG (1<<5)
struct cmd_struct {
const char *cmd;
@@ -306,7 +307,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
prefix = setup_git_directory_gently(&nongit_ok);
}
- if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
+ if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
+ !(p->option & DELAY_PAGER_CONFIG))
use_pager = check_pager_config(p->cmd);
if (use_pager == -1 && p->option & USE_PAGER)
use_pager = 1;
--
2.14.0.rc1.12.ge2d9c4613
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v3 3/7] git.c: provide setup_auto_pager()
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
2017-08-02 19:40 ` [PATCH v3 1/7] builtin.h: take over documentation from api-builtin.txt Martin Ågren
2017-08-02 19:40 ` [PATCH v3 2/7] git.c: let builtins opt for handling `pager.foo` themselves Martin Ågren
@ 2017-08-02 19:40 ` Martin Ågren
2017-08-02 19:40 ` [PATCH v3 4/7] t7006: add tests for how git tag paginates Martin Ågren
` (5 subsequent siblings)
8 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-08-02 19:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
The previous patch introduced a way for builtins to declare that they
will take responsibility for handling the `pager.foo`-config item. (See
the commit message of that patch for why that could be useful.)
Provide setup_auto_pager(), which builtins can call in order to handle
`pager.<cmd>`, including possibly starting the pager. Make this function
don't do anything if a pager has already been started, as indicated by
use_pager or pager_in_use().
Whenever this function is called from a builtin, git.c will already have
called commit_pager_choice(). Since commit_pager_choice() treats the
special value -1 as "punt" or "not yet decided", it is not a problem
that we might end up calling commit_pager_choice() once in git.c and
once (or more) in the builtin. Make the new function use -1 in the same
way and document it as "punt".
Don't add any users of setup_auto_pager just yet, one will follow in
a later patch.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
builtin.h | 12 ++++++++++++
git.c | 10 ++++++++++
2 files changed, 22 insertions(+)
diff --git a/builtin.h b/builtin.h
index 0f3a7b770..42378f3aa 100644
--- a/builtin.h
+++ b/builtin.h
@@ -113,6 +113,18 @@ struct fmt_merge_msg_opts {
extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
struct fmt_merge_msg_opts *);
+/**
+ * If a built-in has DELAY_PAGER_CONFIG set, the built-in should call this early
+ * when it wishes to respect the `pager.foo`-config. The `cmd` is the name of
+ * the built-in, e.g., "foo". If a paging-choice has already been setup, this
+ * does nothing. The default in `def` should be 0 for "pager off", 1 for "pager
+ * on" or -1 for "punt".
+ *
+ * You should most likely use a default of 0 or 1. "Punt" (-1) could be useful
+ * to be able to fall back to some historical compatibility name.
+ */
+extern void setup_auto_pager(const char *cmd, int def);
+
extern int is_builtin(const char *s);
extern int cmd_add(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index 79195ebbd..66832f232 100644
--- a/git.c
+++ b/git.c
@@ -33,6 +33,16 @@ static void commit_pager_choice(void) {
}
}
+void setup_auto_pager(const char *cmd, int def)
+{
+ if (use_pager != -1 || pager_in_use())
+ return;
+ use_pager = check_pager_config(cmd);
+ if (use_pager == -1)
+ use_pager = def;
+ commit_pager_choice();
+}
+
static int handle_options(const char ***argv, int *argc, int *envchanged)
{
const char **orig_argv = *argv;
--
2.14.0.rc1.12.ge2d9c4613
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v3 4/7] t7006: add tests for how git tag paginates
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
` (2 preceding siblings ...)
2017-08-02 19:40 ` [PATCH v3 3/7] git.c: provide setup_auto_pager() Martin Ågren
@ 2017-08-02 19:40 ` Martin Ågren
2017-08-02 19:40 ` [PATCH v3 5/7] tag: respect `pager.tag` in list-mode only Martin Ågren
` (4 subsequent siblings)
8 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-08-02 19:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as
"Vim: Warning: Output is not to a terminal" and a garbled terminal.
Someone who makes use of both `git tag -a` and `git tag -l` will
probably not set `pager.tag`, so that `git tag -a` will actually work,
at the cost of not paging output of `git tag -l`.
Since we're about to change how `git tag` respects `pager.tag`, add tests
around this, including how the configuration is ignored if --no-pager or
--paginate are used.
Construct tests with a few different subcommands. First, use -l. Second,
use "no arguments" and --contains, since those imply -l. (There are
more arguments which imply -l, but using these two should be enough.)
Third, use -a as a representative for "not -l". Actually, the tests use
`git tag -am` so no editor is launched, but that is irrelevant, since we
just want to see whether the pager is used or not. Make one of the tests
demonstrate the broken behavior mentioned above, where `git tag -a`
respects `pager.tag`.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
In one place, I now use test_expect_failure (thanks Peff).
| 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 20b4d83c2..b56d4cdd4 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,6 +134,73 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
}
'
+test_expect_success TTY 'git tag -l defaults to not paging' '
+ rm -f paginated.out &&
+ test_terminal git tag -l &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects pager.tag' '
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag tag -l &&
+ test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -l respects --no-pager' '
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag --no-pager tag -l &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag with no args defaults to not paging' '
+ # no args implies -l so this should page like -l
+ rm -f paginated.out &&
+ test_terminal git tag &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag with no args respects pager.tag' '
+ # no args implies -l so this should page like -l
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag tag &&
+ test -e paginated.out
+'
+
+test_expect_success TTY 'git tag --contains defaults to not paging' '
+ # --contains implies -l so this should page like -l
+ rm -f paginated.out &&
+ test_terminal git tag --contains &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag --contains respects pager.tag' '
+ # --contains implies -l so this should page like -l
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag tag --contains &&
+ test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a defaults to not paging' '
+ test_when_finished "git tag -d newtag" &&
+ rm -f paginated.out &&
+ test_terminal git tag -am message newtag &&
+ ! test -e paginated.out
+'
+
+test_expect_failure TTY 'git tag -a ignores pager.tag' '
+ test_when_finished "git tag -d newtag" &&
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag tag -am message newtag &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag -a respects --paginate' '
+ test_when_finished "git tag -d newtag" &&
+ rm -f paginated.out &&
+ test_terminal git --paginate tag -am message newtag &&
+ test -e paginated.out
+'
+
# A colored commit log will begin with an appropriate ANSI escape
# for the first color; the text "commit" comes later.
colorful() {
--
2.14.0.rc1.12.ge2d9c4613
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v3 5/7] tag: respect `pager.tag` in list-mode only
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
` (3 preceding siblings ...)
2017-08-02 19:40 ` [PATCH v3 4/7] t7006: add tests for how git tag paginates Martin Ågren
@ 2017-08-02 19:40 ` Martin Ågren
2017-08-02 19:40 ` [PATCH v3 6/7] tag: change default of `pager.tag` to "on" Martin Ågren
` (3 subsequent siblings)
8 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-08-02 19:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
Using, e.g., `git -c pager.tag tag -a new-tag` results in errors such as
"Vim: Warning: Output is not to a terminal" and a garbled terminal.
Someone who makes use of both `git tag -a` and `git tag -l` will
probably not set `pager.tag`, so that `git tag -a` will actually work,
at the cost of not paging output of `git tag -l`.
Use the mechanisms introduced in two earlier patches to ignore
`pager.tag` in git.c and let the `git tag` builtin handle it on its own.
Only respect `pager.tag` when running in list-mode.
There is a window between where the pager is started before and after
this patch. This means that early errors can behave slightly different
before and after this patch. Since operation-parsing has to happen
inside this window, this can be seen with `git -c pager.tag="echo pager
is used" tag -l --unknown-option`. This change in paging-behavior should
be acceptable since it only affects erroneous usages.
Update the documentation and update tests.
If an alias is used to run `git tag -a`, then `pager.tag` will still be
respected. Document this known breakage. It will be fixed in a later
commit. Add a similar test for `-l`, which works.
Noticed-by: Anatoly Borodin <anatoly.borodin@gmail.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
Documentation/git-tag.txt | 3 +++
| 15 ++++++++++++++-
builtin/tag.c | 3 +++
git.c | 2 +-
4 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 1eb15afa1..875d135e0 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -205,6 +205,9 @@ it in the repository configuration as follows:
signingKey = <gpg-keyid>
-------------------------------------
+`pager.tag` is only respected when listing tags, i.e., when `-l` is
+used or implied.
+See linkgit:git-config[1].
DISCUSSION
----------
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index b56d4cdd4..570b2f252 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -187,7 +187,7 @@ test_expect_success TTY 'git tag -a defaults to not paging' '
! test -e paginated.out
'
-test_expect_failure TTY 'git tag -a ignores pager.tag' '
+test_expect_success TTY 'git tag -a ignores pager.tag' '
test_when_finished "git tag -d newtag" &&
rm -f paginated.out &&
test_terminal git -c pager.tag tag -am message newtag &&
@@ -201,6 +201,19 @@ test_expect_success TTY 'git tag -a respects --paginate' '
test -e paginated.out
'
+test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
+ test_when_finished "git tag -d newtag" &&
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
+ ! test -e paginated.out
+'
+
+test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
+ rm -f paginated.out &&
+ test_terminal git -c pager.tag -c alias.t=tag t -l &&
+ test -e paginated.out
+'
+
# A colored commit log will begin with an appropriate ANSI escape
# for the first color; the text "commit" comes later.
colorful() {
diff --git a/builtin/tag.c b/builtin/tag.c
index 01154ea8d..5ad1af252 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -461,6 +461,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
cmdmode = 'l';
}
+ if (cmdmode == 'l')
+ setup_auto_pager("tag", 0);
+
if ((create_tag_object || force) && (cmdmode != 0))
usage_with_options(git_tag_usage, options);
diff --git a/git.c b/git.c
index 66832f232..82ac2a092 100644
--- a/git.c
+++ b/git.c
@@ -466,7 +466,7 @@ static struct cmd_struct commands[] = {
{ "stripspace", cmd_stripspace },
{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
- { "tag", cmd_tag, RUN_SETUP },
+ { "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },
{ "unpack-file", cmd_unpack_file, RUN_SETUP },
{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
{ "update-index", cmd_update_index, RUN_SETUP },
--
2.14.0.rc1.12.ge2d9c4613
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v3 6/7] tag: change default of `pager.tag` to "on"
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
` (4 preceding siblings ...)
2017-08-02 19:40 ` [PATCH v3 5/7] tag: respect `pager.tag` in list-mode only Martin Ågren
@ 2017-08-02 19:40 ` Martin Ågren
2017-08-02 19:40 ` [PATCH v3 7/7] git.c: ignore pager.* when launching builtin as dashed external Martin Ågren
` (2 subsequent siblings)
8 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-08-02 19:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
The previous patch taught `git tag` to only respect `pager.tag` in
list-mode. That patch left the default value of `pager.tag` at "off".
After that patch, it makes sense to let the default value be "on"
instead, since it will help with listing many tags, but will not hurt
users of `git tag -a` as it would have before. Make that change. Update
documentation and tests.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
Documentation/git-tag.txt | 2 +-
| 28 ++++++++++++++--------------
builtin/tag.c | 2 +-
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 875d135e0..d97aad343 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -206,7 +206,7 @@ it in the repository configuration as follows:
-------------------------------------
`pager.tag` is only respected when listing tags, i.e., when `-l` is
-used or implied.
+used or implied. The default is to use a pager.
See linkgit:git-config[1].
DISCUSSION
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 570b2f252..afa03f3b6 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -134,16 +134,16 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
}
'
-test_expect_success TTY 'git tag -l defaults to not paging' '
+test_expect_success TTY 'git tag -l defaults to paging' '
rm -f paginated.out &&
test_terminal git tag -l &&
- ! test -e paginated.out
+ test -e paginated.out
'
test_expect_success TTY 'git tag -l respects pager.tag' '
rm -f paginated.out &&
- test_terminal git -c pager.tag tag -l &&
- test -e paginated.out
+ test_terminal git -c pager.tag=false tag -l &&
+ ! test -e paginated.out
'
test_expect_success TTY 'git tag -l respects --no-pager' '
@@ -152,32 +152,32 @@ test_expect_success TTY 'git tag -l respects --no-pager' '
! test -e paginated.out
'
-test_expect_success TTY 'git tag with no args defaults to not paging' '
+test_expect_success TTY 'git tag with no args defaults to paging' '
# no args implies -l so this should page like -l
rm -f paginated.out &&
test_terminal git tag &&
- ! test -e paginated.out
+ test -e paginated.out
'
test_expect_success TTY 'git tag with no args respects pager.tag' '
# no args implies -l so this should page like -l
rm -f paginated.out &&
- test_terminal git -c pager.tag tag &&
- test -e paginated.out
+ test_terminal git -c pager.tag=false tag &&
+ ! test -e paginated.out
'
-test_expect_success TTY 'git tag --contains defaults to not paging' '
+test_expect_success TTY 'git tag --contains defaults to paging' '
# --contains implies -l so this should page like -l
rm -f paginated.out &&
test_terminal git tag --contains &&
- ! test -e paginated.out
+ test -e paginated.out
'
test_expect_success TTY 'git tag --contains respects pager.tag' '
# --contains implies -l so this should page like -l
rm -f paginated.out &&
- test_terminal git -c pager.tag tag --contains &&
- test -e paginated.out
+ test_terminal git -c pager.tag=false tag --contains &&
+ ! test -e paginated.out
'
test_expect_success TTY 'git tag -a defaults to not paging' '
@@ -210,8 +210,8 @@ test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
rm -f paginated.out &&
- test_terminal git -c pager.tag -c alias.t=tag t -l &&
- test -e paginated.out
+ test_terminal git -c pager.tag=false -c alias.t=tag t -l &&
+ ! test -e paginated.out
'
# A colored commit log will begin with an appropriate ANSI escape
diff --git a/builtin/tag.c b/builtin/tag.c
index 5ad1af252..ea83df5e1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -462,7 +462,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
}
if (cmdmode == 'l')
- setup_auto_pager("tag", 0);
+ setup_auto_pager("tag", 1);
if ((create_tag_object || force) && (cmdmode != 0))
usage_with_options(git_tag_usage, options);
--
2.14.0.rc1.12.ge2d9c4613
^ permalink raw reply related [flat|nested] 69+ messages in thread* [PATCH v3 7/7] git.c: ignore pager.* when launching builtin as dashed external
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
` (5 preceding siblings ...)
2017-08-02 19:40 ` [PATCH v3 6/7] tag: change default of `pager.tag` to "on" Martin Ågren
@ 2017-08-02 19:40 ` Martin Ågren
2017-08-03 18:20 ` [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode Junio C Hamano
2017-08-03 19:29 ` Jeff King
8 siblings, 0 replies; 69+ messages in thread
From: Martin Ågren @ 2017-08-02 19:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and
execute `git-foo` as a dashed external. This is true even if git foo is
a builtin. That is on purpose, and is motivated in a comment which was
added in commit 441981bc ("git: simplify environment save/restore
logic", 2016-01-26).
Shortly before we launch a dashed external, and unless we have already
found out whether we should use a pager, we check `pager.foo`. This was
added in commit 92058e4d ("support pager.* for external commands",
2011-08-18). If the dashed external is a builtin, this does not match
that commit's intention and is arguably wrong, since it would be cleaner
if we let the "dashed external builtin" handle `pager.foo`.
This has not mattered in practice, but a recent patch taught `git-tag`
to ignore `pager.tag` under certain circumstances. But, when started
using an alias, it doesn't get the chance to do so, as outlined above.
That recent patch added a test to document this breakage.
Do not check `pager.foo` before launching a builtin as a dashed
external, i.e., if we recognize the name of the external as a builtin.
Change the test to use `test_expect_success`.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
| 2 +-
git.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index afa03f3b6..9128ec5ac 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -201,7 +201,7 @@ test_expect_success TTY 'git tag -a respects --paginate' '
test -e paginated.out
'
-test_expect_failure TTY 'git tag as alias ignores pager.tag with -a' '
+test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
test_when_finished "git tag -d newtag" &&
rm -f paginated.out &&
test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
diff --git a/git.c b/git.c
index 82ac2a092..6b6d9f68e 100644
--- a/git.c
+++ b/git.c
@@ -559,7 +559,7 @@ static void execv_dashed_external(const char **argv)
if (get_super_prefix())
die("%s doesn't support --super-prefix", argv[0]);
- if (use_pager == -1)
+ if (use_pager == -1 && !is_builtin(argv[0]))
use_pager = check_pager_config(argv[0]);
commit_pager_choice();
--
2.14.0.rc1.12.ge2d9c4613
^ permalink raw reply related [flat|nested] 69+ messages in thread* Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
` (6 preceding siblings ...)
2017-08-02 19:40 ` [PATCH v3 7/7] git.c: ignore pager.* when launching builtin as dashed external Martin Ågren
@ 2017-08-03 18:20 ` Junio C Hamano
2017-08-03 19:29 ` Jeff King
8 siblings, 0 replies; 69+ messages in thread
From: Junio C Hamano @ 2017-08-03 18:20 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Jeff King
Martin Ågren <martin.agren@gmail.com> writes:
> This is the third version of my attempt to make `pager tag` useful (v1
> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.
>
> I've squashed patches 01-03/10 and 07-08/10, respectively. The interdiff
> is below. I managed to clean up some tests thanks to a drive-by comment
> by Peff which cleared up a misunderstanding I had. Some minor changes,
> e.g., I write "built-in" instead of "builtin", since that seemed
> preferred, at least locally in builtin.h. I documented why a default
> value of "punt" could be useful, but also that it's probably not wanted.
>
> Martin
Thanks for working on this. The whole series was well reasoned and
was a very pleasant read.
Will queue.
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode
2017-08-02 19:40 ` [PATCH v3 0/7] " Martin Ågren
` (7 preceding siblings ...)
2017-08-03 18:20 ` [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode Junio C Hamano
@ 2017-08-03 19:29 ` Jeff King
2017-08-04 4:21 ` Martin Ågren
8 siblings, 1 reply; 69+ messages in thread
From: Jeff King @ 2017-08-03 19:29 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Junio C Hamano
On Wed, Aug 02, 2017 at 09:40:48PM +0200, Martin Ågren wrote:
> This is the third version of my attempt to make `pager tag` useful (v1
> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.
This looks good to me overall. One minor question from the interdiff:
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index 8b2ffb1aa..9128ec5ac 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '
> test_expect_success TTY 'git tag with no args respects pager.tag' '
> # no args implies -l so this should page like -l
> rm -f paginated.out &&
> - test_terminal git -c pager.tag=no tag &&
> + test_terminal git -c pager.tag=false tag &&
> ! test -e paginated.out
> '
These should behave the same, right? So this is just a style/consistency
fix, not a bugfix?
-Peff
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode
2017-08-03 19:29 ` Jeff King
@ 2017-08-04 4:21 ` Martin Ågren
2017-08-04 4:59 ` Jeff King
0 siblings, 1 reply; 69+ messages in thread
From: Martin Ågren @ 2017-08-04 4:21 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Junio C Hamano
On 3 August 2017 at 21:29, Jeff King <peff@peff.net> wrote:
> On Wed, Aug 02, 2017 at 09:40:48PM +0200, Martin Ågren wrote:
>
>> This is the third version of my attempt to make `pager tag` useful (v1
>> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.
>
> This looks good to me overall. One minor question from the interdiff:
>
>> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
>> index 8b2ffb1aa..9128ec5ac 100755
>> --- a/t/t7006-pager.sh
>> +++ b/t/t7006-pager.sh
>> @@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '
>> test_expect_success TTY 'git tag with no args respects pager.tag' '
>> # no args implies -l so this should page like -l
>> rm -f paginated.out &&
>> - test_terminal git -c pager.tag=no tag &&
>> + test_terminal git -c pager.tag=false tag &&
>> ! test -e paginated.out
>> '
>
> These should behave the same, right? So this is just a style/consistency
> fix, not a bugfix?
Right. I realized I was using "false" everywhere else. It wouldn't have hurt
to exercise the config-parsing a tiny bit differently, but I assume that's
already being done explicitly in some other test, so I went for consistency.
Thanks for all the feedback and thoughts throughout the different versions
of this series. It changed quite a bit since v1, so thanks a lot.
Martin
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v3 0/7] tag: only respect `pager.tag` in list-mode
2017-08-04 4:21 ` Martin Ågren
@ 2017-08-04 4:59 ` Jeff King
0 siblings, 0 replies; 69+ messages in thread
From: Jeff King @ 2017-08-04 4:59 UTC (permalink / raw)
To: Martin Ågren; +Cc: Git Mailing List, Junio C Hamano
On Fri, Aug 04, 2017 at 06:21:47AM +0200, Martin Ågren wrote:
> On 3 August 2017 at 21:29, Jeff King <peff@peff.net> wrote:
> > On Wed, Aug 02, 2017 at 09:40:48PM +0200, Martin Ågren wrote:
> >
> >> This is the third version of my attempt to make `pager tag` useful (v1
> >> at [1], v2 at [2]). Thanks to Junio and Peff for comments on v2.
> >
> > This looks good to me overall. One minor question from the interdiff:
> >
> >> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> >> index 8b2ffb1aa..9128ec5ac 100755
> >> --- a/t/t7006-pager.sh
> >> +++ b/t/t7006-pager.sh
> >> @@ -162,7 +162,7 @@ test_expect_success TTY 'git tag with no args defaults to paging' '
> >> test_expect_success TTY 'git tag with no args respects pager.tag' '
> >> # no args implies -l so this should page like -l
> >> rm -f paginated.out &&
> >> - test_terminal git -c pager.tag=no tag &&
> >> + test_terminal git -c pager.tag=false tag &&
> >> ! test -e paginated.out
> >> '
> >
> > These should behave the same, right? So this is just a style/consistency
> > fix, not a bugfix?
>
> Right. I realized I was using "false" everywhere else. It wouldn't have hurt
> to exercise the config-parsing a tiny bit differently, but I assume that's
> already being done explicitly in some other test, so I went for consistency.
Right, that makes perfect sense. Thanks for confirming.
> Thanks for all the feedback and thoughts throughout the different versions
> of this series. It changed quite a bit since v1, so thanks a lot.
You're very welcome. I hope we see more patches from you in the future.
:)
-Peff
^ permalink raw reply [flat|nested] 69+ messages in thread