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

From: Matthew Bystrin <dev.mbstr@gmail.com>
Date: Nov 22, 2024, 18:13 UTC
Message-ID: <D5SWT5JKU6S9.2LH8H7ECRZXXM@gmail.com>
In-Reply-To: <xmqqmshrrxpq.fsf@gitster.g>

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:

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?