diff mbox series

[2/2] pager: make wait_for_pager a no-op for "cat"

Message ID c37f0d54-4ead-422c-8193-f0c2ec84ca4a@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/2] t3701: avoid one-shot export for shell functions | expand

Commit Message

Rubén Justo July 22, 2024, 11:24 p.m. UTC
If we find that the configured pager is an empty string [*1*] or simply
"cat" [*2*], then we return from `setup_pager()` silently without doing
anything, allowing the output to go directly to the normal stdout.

Let's make the call to `wait_for_pager()` for these cases, or any other
future optimizations that may occur, also exit silently without doing
anything.

   1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,
                   2006-04-16)

   2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Junio C Hamano July 22, 2024, 11:54 p.m. UTC | #1
Rubén Justo <rjusto@gmail.com> writes:

> If we find that the configured pager is an empty string [*1*] or simply
> "cat" [*2*], then we return from `setup_pager()` silently without doing
> anything, allowing the output to go directly to the normal stdout.

I'm tempted to suggest inserting two extra paragraphs here to avoid
too big a leap in logic flow.

    Even though the caller may properly make matching calls to
    setup_pager() and wait_for_pager(), setup_pager() may return early
    without doing much, and the call to wait_for_pager() would segfault.

    This condition can be detected by old_fd1 being -1 (not modified in
    setup_pager())

> Let's make the call to `wait_for_pager()` for these cases, or any other
> future optimizations that may occur, also exit silently without doing
> anything.
>
>    1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,
>                    2006-04-16)
>
>    2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---

I am not 100% sure about the "would segfault", but we'd need to be
explicit about what badness it causes to call wait_for_pager()
without starting a pager.  Other than that, well explained.

Thanks.

>  pager.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/pager.c b/pager.c
> index bea4345f6f..896f40fcd2 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -46,6 +46,9 @@ static void wait_for_pager_atexit(void)
>  
>  void wait_for_pager(void)
>  {
> +	if (old_fd1 == -1)
> +		return;
> +
>  	finish_pager();
>  	sigchain_pop_common();
>  	unsetenv("GIT_PAGER_IN_USE");
diff mbox series

Patch

diff --git a/pager.c b/pager.c
index bea4345f6f..896f40fcd2 100644
--- a/pager.c
+++ b/pager.c
@@ -46,6 +46,9 @@  static void wait_for_pager_atexit(void)
 
 void wait_for_pager(void)
 {
+	if (old_fd1 == -1)
+		return;
+
 	finish_pager();
 	sigchain_pop_common();
 	unsetenv("GIT_PAGER_IN_USE");