threads / patch / 62520

patchgit: fix paginate handling for commands with DELAY_PAGER_CONFIG

Subject: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG

## tl;dr

8 messages between Nov 20, 2024 and Nov 28, 2024. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Matthew Bystrin· Nov 20, 2024, 10:17 UTC · lore

Calling commands using editor in terminal with `--paginate` option will break things. For example `git --paginate config --edit`. Add extra check to ignore paginate flag in case command have DELAY_PAGER_CONFIG set.

Relates: cd878a206e8c (t7006: add tests for how git config paginates)
Signed-off-by: Matthew Bystrin <dev.mbstr@gmail.com>
---
Hi!

Some time ago I've sent RFC patch [1], which was not quite ready, and I didn't receive any feedback. Now I'm sending a more complete version of it. Fixing mentioned behaviour of `note` command can be done in separate patch series.

Link: https://lore.kernel.org/git/20241104140536.4970-1-dev.mbstr@gmail.com/

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

Show changes to 2 files +5 −4

git.c, t/t7006-pager.sh

diff --git a/git.c b/git.c
index c2c1b8e22c..2b3b049f4a 100644
--- a/git.c
+++ b/git.c
@@ -464,11 +464,12 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	}
 	assert(!prefix || *prefix);
 	precompose_argv_prefix(argc, argv, NULL);
-	if (use_pager == -1 && run_setup &&
-		!(p->option & DELAY_PAGER_CONFIG))
+	if (use_pager == -1 && run_setup && !(p->option & DELAY_PAGER_CONFIG))
 		use_pager = check_pager_config(p->cmd);
 	if (use_pager == -1 && p->option & USE_PAGER)
 		use_pager = 1;
+	if (use_pager == 1 && (p->option & DELAY_PAGER_CONFIG))
+		use_pager = 0;
 	if (run_setup && startup_info->have_repository)
 		/* get_git_dir() may set up repo, avoid that */
 		trace_repo_setup();
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index a0296d6ca4..218c5093d3 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -188,11 +188,11 @@ test_expect_success TTY 'git tag -a ignores pager.tag' '
 	test_path_is_missing paginated.out
 '
 
-test_expect_success TTY 'git tag -a respects --paginate' '
+test_expect_success TTY 'git tag -a do not respects --paginate' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git --paginate tag -am message newtag &&
-	test_path_is_file paginated.out
+	test_path_is_missing paginated.out
 '
 
 test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '

-- 
2.43.0

Martin Ågren· Nov 21, 2024, 21:50 UTC · re: Matthew Bystrin · lore

Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG

Hi Matthew,

On Wed, 20 Nov 2024 at 11:41, Matthew Bystrin <dev.mbstr@gmail.com> wrote:

Show 9 quoted lines

>
> Calling commands using editor in terminal with `--paginate` option will
> break things. For example `git --paginate config --edit`. Add extra
> check to ignore paginate flag in case command have DELAY_PAGER_CONFIG
> set.
>
> Relates: cd878a206e8c (t7006: add tests for how git config paginates)
> Signed-off-by: Matthew Bystrin <dev.mbstr@gmail.com>
> ---

> Some time ago I've sent RFC patch [1], which was not quite ready, and I didn't
> receive any feedback. Now I'm sending a more complete version of it. Fixing
> mentioned behaviour of `note` command can be done in separate patch series.

Thanks for reposting, and welcome to the list.

> +       if (use_pager == 1 && (p->option & DELAY_PAGER_CONFIG))
> +               use_pager = 0;
>         if (run_setup && startup_info->have_repository)

This flag goes back to c409824cc2a ("git.c: let builtins opt for handling `pager.foo` themselves", 2017-08-02). The original observation was that setting `pager.tag` to true was more or less foolish -- it would help `git tag --list`, sure, but would also break `git tag -a` quite badly along the way since the latter wants to open an editor.

That's why c409824cc2a allowed builtins to basically say "delay pager config handling -- I'll respect it for some options/modes, but not for others". Now this proposed patch wants to extend that handling beyond "delay pager *config*" to even go "for some options/modes, I'm going to completely ignore `--paginate`." ("Delay pager *option*"?)

Actually, no, it's not so much ignoring as *forcing*. Since you force it to 0, doesn't that mean that `--paginate` ends up basically being `--no-pager`? So `git --paginate branch` is now `git --no-pager branch`? That doesn't seem right. An optionless `git branch` would have paginated, so adding `--paginate` shouldn't change anything.

But even if we force it to -1 instead (for "maybe"), I'm not sure I understand why such an undoing of user intention is wanted. If I run `git --paginate tag -a ...`, maybe that's just self-inflicted harm, but are we certain that for all the `git foo --bar` which won't respect `pager.foo`, that it's also completely crazy to provide `--paginate`?

Martin

Junio C Hamano· Nov 22, 2024, 17:21 UTC · re: Martin Ågren · lore

Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG

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

Show 13 quoted lines

> Hi Matthew,
> ...
> Actually, no, it's not so much ignoring as *forcing*. Since you force it
> to 0, doesn't that mean that `--paginate` ends up basically being
> `--no-pager`? So `git --paginate branch` is now `git --no-pager branch`?
> That doesn't seem right. An optionless `git branch` would have
> paginated, so adding `--paginate` shouldn't change anything.
>
> But even if we force it to -1 instead (for "maybe"), I'm not sure I
> understand why such an undoing of user intention is wanted. If I run
> `git --paginate tag -a ...`, maybe that's just self-inflicted harm, but
> are we certain that for all the `git foo --bar` which won't respect
> `pager.foo`, that it's also completely crazy to provide `--paginate`?

The whole thing started with

    Calling commands using editor in terminal with `--paginate`
    option will break things. For example `git --paginate config
    --edit`.

which many of us may respond with "it hurts? do not do it then", so I agree with you that a fallout would be worse than the problem the change is trying to "fix".

Matthew Bystrin· Nov 22, 2024, 18:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG

Junio and Martin thanks for your replies!

Show 25 quoted lines

> Martin Ågren <martin.agren@gmail.com> writes:
>
> > Hi Matthew,
> > ...
> > Actually, no, it's not so much ignoring as *forcing*. Since you force it
> > to 0, doesn't that mean that `--paginate` ends up basically being
> > `--no-pager`? So `git --paginate branch` is now `git --no-pager branch`?
> > That doesn't seem right. An optionless `git branch` would have
> > paginated, so adding `--paginate` shouldn't change anything.
> >
> > But even if we force it to -1 instead (for "maybe"), I'm not sure I
> > understand why such an undoing of user intention is wanted. If I run
> > `git --paginate tag -a ...`, maybe that's just self-inflicted harm, but
> > are we certain that for all the `git foo --bar` which won't respect
> > `pager.foo`, that it's also completely crazy to provide `--paginate`?
>
> The whole thing started with
>
>     Calling commands using editor in terminal with `--paginate`
>     option will break things. For example `git --paginate config
>     --edit`.
>
> which many of us may respond with "it hurts? do not do it then", so
> I agree with you that a fallout would be worse than the problem the
> change is trying to "fix".

I see the point and totally agree with it.

The root of the 'problem' is related with editor, not with commands. So maybe it is a good way to deal with it in editor code? I've quickly come up with something like this:

Show changes to editor.c +4 −0

diff --git a/editor.c b/editor.c
index 6b9ce81d5f..04a1f71694 100644
--- a/editor.c
+++ b/editor.c
@@ -13,6 +13,7 @@
 #include "strvec.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "pager.h"
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -60,6 +61,9 @@ const char *git_sequence_editor(void)
 static int launch_specified_editor(const char *editor, const char *path,
 				   struct strbuf *buffer, const char *const *env)
 {
+	if (pager_in_use())
+		wait_for_pager();
+
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");

Brief testing shows what it works, but more complex approach may be needed. What
do you think about that? Should I continue work on that despite the fact it does
not really hurts? If yes, is it better to create new patch or send as an update
of the current?

Junio C Hamano· Nov 25, 2024, 02:21 UTC · re: Matthew Bystrin · lore

Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG

"Matthew Bystrin" <dev.mbstr@gmail.com> writes:

Show 27 quoted lines

> The root of the 'problem' is related with editor, not with commands. So maybe it
> is a good way to deal with it in editor code? I've quickly come up with
> something like this:
>
> diff --git a/editor.c b/editor.c
> index 6b9ce81d5f..04a1f71694 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -13,6 +13,7 @@
>  #include "strvec.h"
>  #include "run-command.h"
>  #include "sigchain.h"
> +#include "pager.h"
>  
>  #ifndef DEFAULT_EDITOR
>  #define DEFAULT_EDITOR "vi"
> @@ -60,6 +61,9 @@ const char *git_sequence_editor(void)
>  static int launch_specified_editor(const char *editor, const char *path,
>  				   struct strbuf *buffer, const char *const *env)
>  {
> +	if (pager_in_use())
> +		wait_for_pager();
> +
>  	if (!editor)
>  		return error("Terminal is dumb, but EDITOR unset");
>
> Brief testing shows what it works, but more complex approach may be needed.

I am not sure what you mean by "it works", and I do not really understand what you meant by "break things" in the original claim in the proposed commit log message, either.

    Calling commands using editor in terminal with `--paginate` option will
    break things. For example `git --paginate config --edit`.

The last sentence is incomplete and it is unclear which part of what `git -p config -e' does (or does not do) is considered "breaks things".

I cannot tell if the simplified change above (which is far nicer than sprinkling code to tweak use_pager variable everywhere) is doing the right thing without knowing what definition of "working" (and hence "breaking") you are using when you report that the above change "works".

Stepping back a bit, whatever code change your next version contains, please make sure that the proposed log message that explains your change tells the readers what you see, and what you would want to see in the ideal world (with a fix), and what the differences between these two are, instead of just saying "currently it breaks, so let's fix it".

Thanks.

Martin Ågren· Nov 25, 2024, 10:55 UTC · re: Matthew Bystrin · lore

Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG

On Fri, 22 Nov 2024 at 19:13, Matthew Bystrin <dev.mbstr@gmail.com> wrote:

Show 43 quoted lines

>
> > The whole thing started with
> >
> >     Calling commands using editor in terminal with `--paginate`
> >     option will break things. For example `git --paginate config
> >     --edit`.
> >
> > which many of us may respond with "it hurts? do not do it then", so
> > I agree with you that a fallout would be worse than the problem the
> > change is trying to "fix".
>
> I see the point and totally agree with it.
>
> The root of the 'problem' is related with editor, not with commands. So maybe it
> is a good way to deal with it in editor code? I've quickly come up with
> something like this:
>
> diff --git a/editor.c b/editor.c
> index 6b9ce81d5f..04a1f71694 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -13,6 +13,7 @@
>  #include "strvec.h"
>  #include "run-command.h"
>  #include "sigchain.h"
> +#include "pager.h"
>
>  #ifndef DEFAULT_EDITOR
>  #define DEFAULT_EDITOR "vi"
> @@ -60,6 +61,9 @@ const char *git_sequence_editor(void)
>  static int launch_specified_editor(const char *editor, const char *path,
>                                    struct strbuf *buffer, const char *const *env)
>  {
> +       if (pager_in_use())
> +               wait_for_pager();
> +
>         if (!editor)
>                 return error("Terminal is dumb, but EDITOR unset");
>
> Brief testing shows what it works, but more complex approach may be needed. What
> do you think about that? Should I continue work on that despite the fact it does
> not really hurts? If yes, is it better to create new patch or send as an update
> of the current?

So the high-level idea here is "if we're going to open an editor, let's first shut down any pager".

This helper is relatively recent, from e8bd8883fe (pager: introduce wait_for_pager, 2024-07-25) and fc87b2f7c1 (add-patch: render hunks through the pager, 2024-07-25). The context there seems to be that a long-running git process wants to open the pager, maybe even more than once. So we need a way to shut down the pager we just opened -- it's done its thing.

What gives me a bit of pause here is that we're now approaching this from a quite different direction: a git process wants to shut down the pager not because it just opened it and the pager has done its thing, but because we want to open an editor and think that any pager might interfere.

If `git add -p` is a long-running process that wants to repeatedly launch a pager, what could the opposite look like? A long-running process that wants to repeatedly launch an editor? While paging?

Maybe `git rebase -i` could be one such example. TBH, running its output through a pager might not look super-pretty today due to it producing temporary output that it then erases, but at least it works. The editor might be graphical (or a script?) that won't actually suffer from the pager running. Killing the pager might be unwanted. So I don't know.

What is the original problem here? Is some kind of tooling issuing a `git -p tag -a` where it's not possible to teach it to drop the `-p`?

Martin

Matthew Bystrin· Nov 28, 2024, 22:27 UTC · re: Martin Ågren · lore

Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG

Martin Ågren wrote:

Show 9 quoted lines

> If `git add -p` is a long-running process that wants to repeatedly
> launch a pager, what could the opposite look like? A long-running
> process that wants to repeatedly launch an editor? While paging?
>
> Maybe `git rebase -i` could be one such example. TBH, running its output
> through a pager might not look super-pretty today due to it producing
> temporary output that it then erases, but at least it works. The editor
> might be graphical (or a script?) that won't actually suffer from the
> pager running. Killing the pager might be unwanted. So I don't know.

Agree, killing might be unwanted. Another approach proposed below.

> What is the original problem here? Is some kind of tooling issuing a
> `git -p tag -a` where it's not possible to teach it to drop the `-p`?

I faced such 'wrong' behaviour by typing commands manually.

It feels like one flag should not break execution of interactive commands like `git -p add -p`, `git -p rebase -i`, `git -p config -e` etc. I took into account Junio's comments, so please let me describe situation in more correct way. The following is related with workflow in terminal.

Currently when some interactive command is called with `--paginate` I see interfered output and cannot proceed to desirable actions. A good analogy is to call `vim | less`.

What I want to see is if we are calling interactive command with `--paginate` (1) it would rather fail explicitly with an error message, or (2) it would close+reopen/bypass pager when some interaction is needed. Bypassing pager is a complicated thing, I'm not sure it can be done (because of how they claim stdin)

Of course we can just simply do not use `-p` flag with interactive commands and provide some info in documentation.

Thanks!

Kristoffer Haugsbakk· Nov 22, 2024, 18:50 UTC · re: Matthew Bystrin · lore

Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG

I was originally not going to nitpick this in case the patch would go through review (not to disrupt).

On Wed, Nov 20, 2024, at 11:17, Matthew Bystrin wrote:

Show 6 quoted lines

> Calling commands using editor in terminal with `--paginate` option will
> break things. For example `git --paginate config --edit`. Add extra
> check to ignore paginate flag in case command have DELAY_PAGER_CONFIG
> set.
>
> Relates: cd878a206e8c (t7006: add tests for how git config paginates)

Trailers in this project usually credit/mention people. Related commits tend to be mentioned in the body (the rest of the body) of the commit message.

> Signed-off-by: Matthew Bystrin <dev.mbstr@gmail.com>

-- 
Kristoffer Haugsbakk