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

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 25, 2024, 10:55 UTC
Message-ID: <CAN0heSqb+dzGAVnN6=_AkKa-qh4GYo7ku90No=uXBg2+wobL1Q@mail.gmail.com>
In-Reply-To: <D5SWT5JKU6S9.2LH8H7ECRZXXM@gmail.com>

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