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

From: Junio C Hamano <[email-masked]>
Date: Mon, 25 Nov 2024 11:21:29 +0900

"Matthew Bystrin" <[email-masked]> 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.