On Tue, Jul 23, 2024 at 10:15:03AM +0100, Phillip Wood wrote: > Hi Rubén > > These changes themselves look sensible. Glad to hear that. > As rj/add-p-pager is only in seen I > assume you'll re-roll with these squashed in once everyone is happy? Junio has already integrated these changes into the branch he has in his tree, including a small change to the message to adjust it to his comments, which I think is good. I hope that what we already have in Junio's tree is the final iteration of this long series and that we can let it settle before making further changes. > > 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)
