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

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

[Matthew Bystrin]

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

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

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

[Martin Ågren]

Hi Matthew,

On Wed, 20 Nov 2024 at 11:41, Matthew Bystrin <dev.mbstr@gmail.com> wrote:
>
> 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

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

[Junio C Hamano]

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

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

[Matthew Bystrin]

Junio and Martin thanks for your replies!

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

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?

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

[Kristoffer Haugsbakk]

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

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

[Junio C Hamano]

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

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

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

[Martin Ågren]

On Fri, 22 Nov 2024 at 19:13, Matthew Bystrin <dev.mbstr@gmail.com> wrote:
>
> > 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

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

[Matthew Bystrin]

Martin Ågren wrote:
> 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!