public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: "Matthew Bystrin" <dev.mbstr@gmail•com>
Cc: "Martin Ågren" <martin.agren@gmail•com>,
	git@vger•kernel.org,
	"Lessley Dennington" <lessleydennington@gmail•com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail•com>,
	"Elijah Newren" <newren@gmail•com>,
	"Phillip Wood" <phillip.wood123@gmail•com>,
	"idriss fekir" <mcsm224@gmail•com>,
	"Joey Salazar" <jgsal@protonmail•com>
Subject: Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG
Date: Mon, 25 Nov 2024 11:21:29 +0900	[thread overview]
Message-ID: <xmqqa5do834m.fsf@gitster.g> (raw)
In-Reply-To: <D5SWT5JKU6S9.2LH8H7ECRZXXM@gmail.com> (Matthew Bystrin's message of "Fri, 22 Nov 2024 18:13:01 +0000")

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

next prev parent reply	other threads:[~2024-11-25  2:21 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-20 10:17 [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG Matthew Bystrin
2024-11-21 21:50 ` Martin Ågren
2024-11-22 17:21   ` Junio C Hamano
2024-11-22 18:13     ` Matthew Bystrin
2024-11-25  2:21       ` Junio C Hamano [this message]
2024-11-25 10:55       ` Martin Ågren
2024-11-28 22:27         ` Matthew Bystrin
2024-11-22 18:50 ` Kristoffer Haugsbakk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqa5do834m.fsf@gitster.g \
    --to=gitster@pobox$(echo .)com \
    --cc=avarab@gmail$(echo .)com \
    --cc=dev.mbstr@gmail$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=jgsal@protonmail$(echo .)com \
    --cc=lessleydennington@gmail$(echo .)com \
    --cc=martin.agren@gmail$(echo .)com \
    --cc=mcsm224@gmail$(echo .)com \
    --cc=newren@gmail$(echo .)com \
    --cc=phillip.wood123@gmail$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox