From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 23 Jul 2024 09:15:03 GMT
Subject: Re: [PATCH v2 0/2] add-p P fixups
Message-ID: <62af789f-ca19-4f11-9339-a97400f7e70c@gmail.com>
In-Reply-To: <7c9ec43d-f52f-49b7-b1f3-fe3c85554006@gmail.com>

Hi Rubén

These changes themselves look sensible. As rj/add-p-pager is only in 
seen I assume you'll re-roll with these squashed in once everyone is happy?

Best Wishes

Phillip

On 23/07/2024 01:39, Rubén Justo wrote:
> Rubén Justo (1):
>    t3701: avoid one-shot export for shell functions
>    pager: make wait_for_pager a no-op for "cat"
> 
>   pager.c                    | 3 +++
>   t/t3701-add-interactive.sh | 6 +++++-
>   2 files changed, 8 insertions(+), 1 deletion(-)
> 
> Range-diff against v1:
> 1:  c3b8ebbae7 ! 1:  15fbf82fff t3701: avoid one-shot export for shell functions
>      @@ Commit message
>       
>               VAR=VAL command args
>       
>      -    it's a common way to define one-shot variables within the scope of
>      +    is a common way to set and export one-shot variables within the scope of
>           executing a "command".
>       
>           However, when "command" is a function which in turn executes the
>      @@ Commit message
>               $ A=1 f
>               A=
>       
>      +    Note that POSIX is not specific about this behavior:
>      +
>      +    http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01
>      +
>           One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash
>           0.5.10.2-6, so we failed the test t3701:51;  the "git add -p" being
>           tested did not get our custom GIT_PAGER, which broke the test.
> 2:  f45455f1ff ! 2:  b87c3d96e4 pager: make wait_for_pager a no-op for "cat"
>      @@ Commit message
>           "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.
>      +    If `setup_pager()` avoids forking a pager, then when the client calls
>      +    the corresponding `wait_for_pager()`, we might fail trying to terminate
>      +    a process that wasn't started.
>      +
>      +    One solution to avoid this problem could be to make the caller aware
>      +    that `setup_pager()` did nothing, so it could avoid calling
>      +    `wait_for_pager()`.
>      +
>      +    However, let's avoid shifting that responsibility to the caller and
>      +    instead treat the call to `wait_for_pager()` as a no-op when we know we
>      +    haven't forked a pager.
>       
>              1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,
>                              2006-04-16)