[PATCH 2/4] pager: do not close fd 2 unnecessarily

[PATCH 2/4] pager: do not close fd 2 unnecessarily

[Rubén Justo]

We send errors to the pager since 61b80509e3 (sending errors to stdout
under $PAGER, 2008-02-16).

In a8335024c2 (pager: do not dup2 stderr if it is already redirected,
2008-12-15) an exception was introduced to avoid redirecting stderr if
it is not connected to a terminal.

In such exceptional cases, the close(STDERR_FILENO) we're doing in
close_pager_fds, is unnecessary.

Furthermore, in a subsequent commit we're going to introduce changes
that will involve using close_pager_fds multiple times.

With this in mind, controlling when we want to close stderr, become
sensible.

Let's close(STDERR_FILENO) only when necessary, and pave the way for the
upcoming changes.

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

diff --git a/pager.c b/pager.c
index be6f4ee59f..251adfc2ad 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
+static int close_fd2;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -23,7 +24,8 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	close(2);
+	if (close_fd2)
+		close(2);
 }
 
 static void wait_for_pager_atexit(void)
@@ -141,8 +143,10 @@ void setup_pager(void)
 
 	/* original process continues, but writes to the pipe */
 	dup2(pager_process.in, 1);
-	if (isatty(2))
+	if (isatty(2)) {
+		close_fd2 = 1;
 		dup2(pager_process.in, 2);
+	}
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
2.45.1

[PATCH v2 0/2] add-p P fixups

[Rubén Justo]

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)
-- 
2.45.1

[PATCH v2 1/2] t3701: avoid one-shot export for shell functions

[Rubén Justo]

The common construct:

    VAR=VAL command args

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
"command", the behavior varies depending on the shell:

 ** Bash 5.2.21 **

    $ f () { bash -c 'echo A=$A'; }
    $ A=1 f
    A=1

 ** dash 0.5.12-9 **

    $ f () { bash -c 'echo A=$A'; }
    $ A=1 f
    A=1

 ** dash 0.5.10.2-6 **

    $ f () { bash -c 'echo A=$A'; }
    $ 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.

Work it around by explicitly exporting the variable in a subshell.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t3701-add-interactive.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index c60589cb94..1b8617e0c1 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -616,7 +616,11 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
 	test_when_finished "rm -f huge_file; git reset" &&
 	printf "\n%2500000s" Y >huge_file &&
 	git add -N huge_file &&
-	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
+	test_write_lines P q | (
+		GIT_PAGER="head -n 1" &&
+		export GIT_PAGER &&
+		test_terminal git add -p
+	)
 '
 
 test_expect_success 'split hunk "add -p (edit)"' '
-- 
2.45.1

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

[Rubén Justo]

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.

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)

   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(+)

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");
-- 
2.45.1

Re: [PATCH v2 0/2] add-p P fixups

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>     +    Note that POSIX is not specific about this behavior:

This is misleading.  It _specifically_ says that the behaviour is
unspecified.  Here "unspecified" is a term of art with a much less
vague meaning.  With respect to the points described as unspecified,
compliant implementations of shell can behave differently, and
scripts (like our tests) that assume one behaviour cannot complain
if a POSIX compliant shell implements the behaviour differently and
"breaks" them.

So "POSIX says the behaviour is unspecified" would be more
appropriate.

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

It may try to stop one, but because we didn't start one to begin
with, there is nothing to stop.  Then is there any problem and why?

In other words, I was hoping that we can clearly say what the
externally visible breakage was.

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

These two paragraphs are good.

Thanks.

Re: [PATCH v2 0/2] add-p P fixups

[Phillip Wood]

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)

Re: [PATCH v2 0/2] add-p P fixups

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> 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?

Ah, good point.  Throwing around "oops, here is a fixup" makes the
development harder to follow for those watching from the sidelines.
I try to keep what is in my tree on 'seen' fairly complete, but
at some point we need to "sync up".

Thanks.

Re: [PATCH v2 0/2] add-p P fixups

[Rubén Justo]

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)

Re: [PATCH v2 0/2] add-p P fixups

[phillip.wood123]

Hi Rubén

On 23/07/2024 23:08, Rubén Justo wrote:
> On Tue, Jul 23, 2024 at 10:15:03AM +0100, Phillip Wood wrote:
> 
>> 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.

The resulting tree is good, but the history is not bisectable. You 
should squash the fixups locally, updating the message of the fixed up 
commit as needed and submit the result as the final version.

Best Wishes

Phillip

>>
>> 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)

Re: [PATCH v2 0/2] add-p P fixups

[Rubén Justo]

On Wed, Jul 24, 2024 at 04:21:53PM +0100, phillip.wood123@gmail.com wrote:

> > 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.
> 
> The resulting tree is good, but the history is not bisectable. You should
> squash the fixups locally, updating the message of the fixed up commit as
> needed and submit the result as the final version.

That was my initial thought [*1*] when the problem with "dash
0.5.10.2-6" appeared. 

Junio proposed [*2*] documenting the changes to address it as a separate
patch, and I think it makes sense and it is valuable to capture the
situation this way in the history.

Regarding the bisectability, I don't understand what stops from being
bisectable.  Except in a scenario with a shell like "dash 0.5.10.2-6"
there won't be any problem.  And in one with it, which should be
uncommon, the situation is well explained.

So, I dunno.

   1.- 2b57479c-29c8-4a6e-b7b0-1309395cfbd9@gmail.com

   2.- xmqq7cdd9l0m.fsf@gitster.g

Re: [PATCH v2 0/2] add-p P fixups

[Phillip Wood]

Hi Rubén

On 24/07/2024 17:12, Rubén Justo wrote:
> On Wed, Jul 24, 2024 at 04:21:53PM +0100, phillip.wood123@gmail.com wrote:
> 
> That was my initial thought [*1*] when the problem with "dash
> 0.5.10.2-6" appeared.
> 
> Junio proposed [*2*] documenting the changes to address it as a separate
> patch, and I think it makes sense and it is valuable to capture the
> situation this way in the history.

We normally avoid merging commits that are known to break our ci. We can 
add a comment about the dash problem to the commit message when this 
fixup is squashed. Also the problem is now documented in 
Documentation/CodingGuidelines which is more likely to be read by other 
contributors.

> Regarding the bisectability, I don't understand what stops from being
> bisectable.  Except in a scenario with a shell like "dash 0.5.10.2-6"
> there won't be any problem.

But we know that shell is in use in a popular Linux distribution so it 
is a problem for those users.

Best Wishes

Phillip

> And in one with it, which should be
> uncommon, the situation is well explained.
> 
> So, I dunno.
> 
>     1.- 2b57479c-29c8-4a6e-b7b0-1309395cfbd9@gmail.com
> 
>     2.- xmqq7cdd9l0m.fsf@gitster.g

Re: [PATCH v2 0/2] add-p P fixups

[Rubén Justo]

On Thu, Jul 25, 2024 at 10:45:04AM +0100, Phillip Wood wrote:
> Hi Rubén
> 
> On 24/07/2024 17:12, Rubén Justo wrote:
> > On Wed, Jul 24, 2024 at 04:21:53PM +0100, phillip.wood123@gmail.com wrote:
> > 
> > That was my initial thought [*1*] when the problem with "dash
> > 0.5.10.2-6" appeared.
> > 
> > Junio proposed [*2*] documenting the changes to address it as a separate
> > patch, and I think it makes sense and it is valuable to capture the
> > situation this way in the history.
> 
> We normally avoid merging commits that are known to break our ci.

Fair point.  I'll reroll and let Junio decide, or ignore ;).

> We can add
> a comment about the dash problem to the commit message when this fixup is
> squashed. Also the problem is now documented in
> Documentation/CodingGuidelines which is more likely to be read by other
> contributors.
> 
> > Regarding the bisectability, I don't understand what stops from being
> > bisectable.  Except in a scenario with a shell like "dash 0.5.10.2-6"
> > there won't be any problem.
> 
> But we know that shell is in use in a popular Linux distribution so it is a
> problem for those users.

It's not an excuse; I just wanted to point out that Ubuntu 20.04 was
updated with a version of "dash" that doesn't have the issue we're
seeing in our CI for quite some time now.  The version I mentioned in
message [1/2] of this thread: dash 0.5.12-9.  

> 
> Best Wishes
> 
> Phillip
> 
> > And in one with it, which should be
> > uncommon, the situation is well explained.
> > 
> > So, I dunno.
> > 
> >     1.- 2b57479c-29c8-4a6e-b7b0-1309395cfbd9@gmail.com
> > 
> >     2.- xmqq7cdd9l0m.fsf@gitster.g

Thank you for trying to make Git and its history better.

[PATCH 0/4] squash fixups in rj/add-p-pager

[Rubén Justo]

Here is the series that squashes the fixups in rj/add-p-pager.

I don't have a strong preference for this over what's already in
rj/add-p-pager, but let's go through the changes Phillip has suggested,
even if it's just to archive them in the list.  

Thanks.

Rubén Justo (4):
  add-patch: test for 'p' command
  pager: do not close fd 2 unnecessarily
  pager: introduce wait_for_pager
  add-patch: render hunks through the pager

 add-patch.c                | 18 +++++++++++---
 pager.c                    | 48 ++++++++++++++++++++++++++++++++++----
 pager.h                    |  1 +
 t/t3701-add-interactive.sh | 48 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+), 8 deletions(-)

Range-diff:
1:  5fdfd2f3bd = 1:  9f358c6d69 add-patch: test for 'p' command
2:  506f457e48 = 2:  f45a7ca9b2 pager: do not close fd 2 unnecessarily
3:  b29c59e3d2 ! 3:  9d7a50e531 pager: introduce wait_for_pager
    @@ Commit message
         In the interactive commands (i.e.: add -p) we want to use the pager for
         some output, while maintaining the interaction with the user.
     
    -    Modify the pager machinery so that we can use setup_pager and, once
    +    Modify the pager machinery so that we can use `setup_pager()` and, once
         we've finished sending the desired output for the pager, wait for the
    -    pager termination using a new function wait_for_pager.   Make this
    +    pager termination using a new function `wait_for_pager()`.  Make this
         function reset the pager machinery before returning.
     
    +    One specific point to note is that we avoid forking the pager in
    +    `setup_pager()` if the configured pager is an empty string [*1*] or
    +    simply "cat" [*2*].  In these cases, `setup_pager()` does nothing and
    +    therefore `wait_for_pager()` should not be called.
    +
    +    We could modify `setup_pager()` to return an indication of these
    +    situations, so we could avoid calling `wait_for_pager()`.
    +
    +    However, let's avoid transferring 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 the pager.
    +
    +       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>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## pager.c ##
     @@ pager.c: int pager_use_color = 1;
    @@ pager.c: 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");
4:  6bc52a5543 ! 4:  6f4990c0d4 add-patch: render hunks through the pager
    @@ Commit message
         this limit.
     
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## add-patch.c ##
     @@
    @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '
     +	test_when_finished "rm -f huge_file; git reset" &&
     +	printf "\n%2500000s" Y >huge_file &&
     +	git add -N huge_file &&
    -+	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
    ++	test_write_lines P q | (
    ++		GIT_PAGER="head -n 1" &&
    ++		export GIT_PAGER &&
    ++		test_terminal git add -p
    ++	)
     +'
     +
      test_expect_success 'split hunk "add -p (edit)"' '
5:  b7637a9f21 < -:  ---------- t3701: avoid one-shot export for shell functions
6:  4b53ff8c0e < -:  ---------- pager: make wait_for_pager a no-op for "cat"

base-commit: a7dae3bdc8b516d36f630b12bb01e853a667e0d9
-- 
2.46.0.rc0.4.g6f4990c0d4

[PATCH 1/4] add-patch: test for 'p' command

[Rubén Justo]

Add a test for the 'p' command, which was introduced in 66c14ab592
(add-patch: introduce 'p' in interactive-patch, 2024-03-29).

Signed-off-by: Rubén Justo <rjusto@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3701-add-interactive.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5d78868ac1..6daf3a6be0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -575,6 +575,22 @@ test_expect_success 'navigate to hunk via regex / pattern' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success 'print again the hunk' '
+	test_when_finished "git reset" &&
+	tr _ " " >expect <<-EOF &&
+	+15
+	 20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	 10
+	+15
+	 20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	EOF
+	test_write_lines s y g 1 p | git add -p >actual &&
+	tail -n 7 <actual >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.46.0.rc0.4.g6f4990c0d4

[PATCH 2/4] pager: do not close fd 2 unnecessarily

[Rubén Justo]

We send errors to the pager since 61b80509e3 (sending errors to stdout
under $PAGER, 2008-02-16).

In a8335024c2 (pager: do not dup2 stderr if it is already redirected,
2008-12-15) an exception was introduced to avoid redirecting stderr if
it is not connected to a terminal.

In such exceptional cases, the close(STDERR_FILENO) we're doing in
close_pager_fds, is unnecessary.

Furthermore, in a subsequent commit we're going to introduce changes
that will involve using close_pager_fds multiple times.

With this in mind, controlling when we want to close stderr, become
sensible.

Let's close(STDERR_FILENO) only when necessary, and pave the way for the
upcoming changes.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pager.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/pager.c b/pager.c
index be6f4ee59f..251adfc2ad 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
+static int close_fd2;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -23,7 +24,8 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	close(2);
+	if (close_fd2)
+		close(2);
 }
 
 static void wait_for_pager_atexit(void)
@@ -141,8 +143,10 @@ void setup_pager(void)
 
 	/* original process continues, but writes to the pipe */
 	dup2(pager_process.in, 1);
-	if (isatty(2))
+	if (isatty(2)) {
+		close_fd2 = 1;
 		dup2(pager_process.in, 2);
+	}
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
2.46.0.rc0.4.g6f4990c0d4

[PATCH 3/4] pager: introduce wait_for_pager

[Rubén Justo]

Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
2006-02-28) we have the machinery to send our output to a pager.

That machinery, once set up, does not allow us to regain the original
stdio streams.

In the interactive commands (i.e.: add -p) we want to use the pager for
some output, while maintaining the interaction with the user.

Modify the pager machinery so that we can use `setup_pager()` and, once
we've finished sending the desired output for the pager, wait for the
pager termination using a new function `wait_for_pager()`.  Make this
function reset the pager machinery before returning.

One specific point to note is that we avoid forking the pager in
`setup_pager()` if the configured pager is an empty string [*1*] or
simply "cat" [*2*].  In these cases, `setup_pager()` does nothing and
therefore `wait_for_pager()` should not be called.

We could modify `setup_pager()` to return an indication of these
situations, so we could avoid calling `wait_for_pager()`.

However, let's avoid transferring 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 the pager.

   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 | 46 ++++++++++++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 251adfc2ad..896f40fcd2 100644
--- a/pager.c
+++ b/pager.c
@@ -14,7 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
-static int close_fd2;
+static int old_fd1 = -1, old_fd2 = -1;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -24,11 +24,11 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	if (close_fd2)
+	if (old_fd2 != -1)
 		close(2);
 }
 
-static void wait_for_pager_atexit(void)
+static void finish_pager(void)
 {
 	fflush(stdout);
 	fflush(stderr);
@@ -36,8 +36,37 @@ static void wait_for_pager_atexit(void)
 	finish_command(&pager_process);
 }
 
+static void wait_for_pager_atexit(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	finish_pager();
+}
+
+void wait_for_pager(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	finish_pager();
+	sigchain_pop_common();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	close(old_fd1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		close(old_fd2);
+		old_fd2 = -1;
+	}
+}
+
 static void wait_for_pager_signal(int signo)
 {
+	if (old_fd1 == -1)
+		return;
+
 	close_pager_fds();
 	finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
@@ -113,6 +142,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
 
 void setup_pager(void)
 {
+	static int once = 0;
 	const char *pager = git_pager(isatty(1));
 
 	if (!pager)
@@ -142,16 +172,20 @@ void setup_pager(void)
 		die("unable to execute pager '%s'", pager);
 
 	/* original process continues, but writes to the pipe */
+	old_fd1 = dup(1);
 	dup2(pager_process.in, 1);
 	if (isatty(2)) {
-		close_fd2 = 1;
+		old_fd2 = dup(2);
 		dup2(pager_process.in, 2);
 	}
 	close(pager_process.in);
 
-	/* this makes sure that the parent terminates after the pager */
 	sigchain_push_common(wait_for_pager_signal);
-	atexit(wait_for_pager_atexit);
+
+	if (!once) {
+		once++;
+		atexit(wait_for_pager_atexit);
+	}
 }
 
 int pager_in_use(void)
diff --git a/pager.h b/pager.h
index b77433026d..103ecac476 100644
--- a/pager.h
+++ b/pager.h
@@ -5,6 +5,7 @@ struct child_process;
 
 const char *git_pager(int stdout_is_tty);
 void setup_pager(void);
+void wait_for_pager(void);
 int pager_in_use(void);
 int term_columns(void);
 void term_clear_line(void);
-- 
2.46.0.rc0.4.g6f4990c0d4

[PATCH 4/4] add-patch: render hunks through the pager

[Rubén Justo]

Make the print command trigger the pager when invoked using a capital
'P', to make it easier for the user to review long hunks.

Note that if the PAGER ends unexpectedly before we've been able to send
the payload, perhaps because the user is not interested in the whole
thing, we might receive a SIGPIPE, which would abruptly and unexpectedly
terminate the interactive session for the user.

Therefore, we need to ignore a possible SIGPIPE signal.  Add a test for
this, in addition to the test for normal operation.

For the SIGPIPE test, we need to make sure that we completely fill the
operating system's buffer, otherwise we might not trigger the SIGPIPE
signal.  The normal size of this buffer in different OSs varies from a
few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
this limit.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c                | 18 +++++++++++++++---
 t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 6e176cd21a..f2c76b7d83 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -7,9 +7,11 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "pager.h"
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "strbuf.h"
+#include "sigchain.h"
 #include "run-command.h"
 #include "strvec.h"
 #include "pathspec.h"
@@ -1391,7 +1393,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
    "e - manually edit the current hunk\n"
-   "p - print the current hunk\n"
+   "p - print the current hunk, 'P' to use the pager\n"
    "? - print help\n");
 
 static int patch_update_file(struct add_p_state *s,
@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	int colored = !!s->colored.len, quit = 0;
+	int colored = !!s->colored.len, quit = 0, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
 	enum {
 		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
+				if (use_pager) {
+					setup_pager();
+					sigchain_push(SIGPIPE, SIG_IGN);
+				}
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
+				if (use_pager) {
+					sigchain_pop(SIGPIPE);
+					wait_for_pager();
+					use_pager = 0;
+				}
 			}
 
 			strbuf_reset(&s->buf);
@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = USE_HUNK;
 				goto soft_increment;
 			}
-		} else if (s->answer.buf[0] == 'p') {
+		} else if (ch == 'p') {
 			rendered_hunk_index = -1;
+			use_pager = (s->answer.buf[0] == 'P') ? 1 : 0;
 		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 6daf3a6be0..1b8617e0c1 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -591,6 +591,38 @@ test_expect_success 'print again the hunk' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success TTY 'print again the hunk (PAGER)' '
+	test_when_finished "git reset" &&
+	cat >expect <<-EOF &&
+	<GREEN>+<RESET><GREEN>15<RESET>
+	 20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
+	PAGER  10<RESET>
+	PAGER <GREEN>+<RESET><GREEN>15<RESET>
+	PAGER  20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	EOF
+	test_write_lines s y g 1 P |
+	(
+		GIT_PAGER="sed s/^/PAGER\ /" &&
+		export GIT_PAGER &&
+		test_terminal git add -p >actual
+	) &&
+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
+test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
+	test_when_finished "rm -f huge_file; git reset" &&
+	printf "\n%2500000s" Y >huge_file &&
+	git add -N huge_file &&
+	test_write_lines P q | (
+		GIT_PAGER="head -n 1" &&
+		export GIT_PAGER &&
+		test_terminal git add -p
+	)
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.46.0.rc0.4.g6f4990c0d4

Re: [PATCH v2 0/2] add-p P fixups

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> ... We
> can add a comment about the dash problem to the commit message when
> this fixup is squashed. Also the problem is now documented in
> Documentation/CodingGuidelines which is more likely to be read by
> other contributors.

That is a good thing to take into consideration, indeed.

Re* [PATCH v2 0/2] add-p P fixups

[Rubén Justo]

On 7/25/24 5:24 PM, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> ... We
>> can add a comment about the dash problem to the commit message when
>> this fixup is squashed. Also the problem is now documented in
>> Documentation/CodingGuidelines which is more likely to be read by
>> other contributors.
> 
> That is a good thing to take into consideration, indeed.

Rubén Justo (2):
  pager: introduce wait_for_pager
  add-patch: render hunks through the pager

 add-patch.c                | 18 ++++++++++++---
 pager.c                    | 46 +++++++++++++++++++++++++++++++++-----
 pager.h                    |  1 +
 t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 9 deletions(-)

Range-diff:
1:  b29c59e3d2 ! 1:  8a6116ef86 pager: introduce wait_for_pager
    @@ Commit message
         In the interactive commands (i.e.: add -p) we want to use the pager for
         some output, while maintaining the interaction with the user.
     
    -    Modify the pager machinery so that we can use setup_pager and, once
    +    Modify the pager machinery so that we can use `setup_pager()` and, once
         we've finished sending the desired output for the pager, wait for the
    -    pager termination using a new function wait_for_pager.   Make this
    +    pager termination using a new function `wait_for_pager()`.  Make this
         function reset the pager machinery before returning.
     
    +    One specific point to note is that we avoid forking the pager in
    +    `setup_pager()` if the configured pager is an empty string [*1*] or
    +    simply "cat" [*2*].  In these cases, `setup_pager()` does nothing and
    +    therefore `wait_for_pager()` should not be called.
    +
    +    We could modify `setup_pager()` to return an indication of these
    +    situations, so we could avoid calling `wait_for_pager()`.
    +
    +    However, let's avoid transferring 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 the pager.
    +
    +       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>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## pager.c ##
     @@ pager.c: int pager_use_color = 1;
    @@ pager.c: 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");
2:  6bc52a5543 ! 2:  980187854a add-patch: render hunks through the pager
    @@ Commit message
         few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
         this limit.
     
    +    For the tests, avoid the common construct to set and export one-shot
    +    variables within the scope of a command:
    +
    +        VAR=VAL command args
    +
    +    It happens that when "command" is a shell function that in turn executes
    +    a "command", the behavior with "VAR" varies depending on the shell:
    +
    +     ** Bash 5.2.21 **
    +
    +        $ f () { bash -c 'echo A=$A'; }
    +        $ A=1 f
    +        A=1
    +
    +     ** dash 0.5.12-9 **
    +
    +        $ f () { bash -c 'echo A=$A'; }
    +        $ A=1 f
    +        A=1
    +
    +     ** dash 0.5.10.2-6 **
    +
    +        $ f () { bash -c 'echo A=$A'; }
    +        $ A=1 f
    +        A=
    +
    +    POSIX explicitly says the effect of this construct is unspecified.
    +
    +    One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash
    +    0.5.10.2-6, so avoid using the construct and use a subshell instead.
    +
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## add-patch.c ##
     @@
    @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '
     +	test_when_finished "rm -f huge_file; git reset" &&
     +	printf "\n%2500000s" Y >huge_file &&
     +	git add -N huge_file &&
    -+	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
    ++	test_write_lines P q | (
    ++		GIT_PAGER="head -n 1" &&
    ++		export GIT_PAGER &&
    ++		test_terminal git add -p
    ++	)
     +'
     +
      test_expect_success 'split hunk "add -p (edit)"' '
3:  b7637a9f21 < -:  ---------- t3701: avoid one-shot export for shell functions
4:  4b53ff8c0e < -:  ---------- pager: make wait_for_pager a no-op for "cat"

base-commit: 506f457e489b2097e2d4fc5ceffd6e242502b2bd
-- 
2.46.0.rc0.4.g6f4990c0d4

[PATCH 1/2] pager: introduce wait_for_pager

[Rubén Justo]

Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
2006-02-28) we have the machinery to send our output to a pager.

That machinery, once set up, does not allow us to regain the original
stdio streams.

In the interactive commands (i.e.: add -p) we want to use the pager for
some output, while maintaining the interaction with the user.

Modify the pager machinery so that we can use `setup_pager()` and, once
we've finished sending the desired output for the pager, wait for the
pager termination using a new function `wait_for_pager()`.  Make this
function reset the pager machinery before returning.

One specific point to note is that we avoid forking the pager in
`setup_pager()` if the configured pager is an empty string [*1*] or
simply "cat" [*2*].  In these cases, `setup_pager()` does nothing and
therefore `wait_for_pager()` should not be called.

We could modify `setup_pager()` to return an indication of these
situations, so we could avoid calling `wait_for_pager()`.

However, let's avoid transferring 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 the pager.

   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 | 46 ++++++++++++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 251adfc2ad..896f40fcd2 100644
--- a/pager.c
+++ b/pager.c
@@ -14,7 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
-static int close_fd2;
+static int old_fd1 = -1, old_fd2 = -1;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -24,11 +24,11 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	if (close_fd2)
+	if (old_fd2 != -1)
 		close(2);
 }
 
-static void wait_for_pager_atexit(void)
+static void finish_pager(void)
 {
 	fflush(stdout);
 	fflush(stderr);
@@ -36,8 +36,37 @@ static void wait_for_pager_atexit(void)
 	finish_command(&pager_process);
 }
 
+static void wait_for_pager_atexit(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	finish_pager();
+}
+
+void wait_for_pager(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	finish_pager();
+	sigchain_pop_common();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	close(old_fd1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		close(old_fd2);
+		old_fd2 = -1;
+	}
+}
+
 static void wait_for_pager_signal(int signo)
 {
+	if (old_fd1 == -1)
+		return;
+
 	close_pager_fds();
 	finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
@@ -113,6 +142,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
 
 void setup_pager(void)
 {
+	static int once = 0;
 	const char *pager = git_pager(isatty(1));
 
 	if (!pager)
@@ -142,16 +172,20 @@ void setup_pager(void)
 		die("unable to execute pager '%s'", pager);
 
 	/* original process continues, but writes to the pipe */
+	old_fd1 = dup(1);
 	dup2(pager_process.in, 1);
 	if (isatty(2)) {
-		close_fd2 = 1;
+		old_fd2 = dup(2);
 		dup2(pager_process.in, 2);
 	}
 	close(pager_process.in);
 
-	/* this makes sure that the parent terminates after the pager */
 	sigchain_push_common(wait_for_pager_signal);
-	atexit(wait_for_pager_atexit);
+
+	if (!once) {
+		once++;
+		atexit(wait_for_pager_atexit);
+	}
 }
 
 int pager_in_use(void)
diff --git a/pager.h b/pager.h
index b77433026d..103ecac476 100644
--- a/pager.h
+++ b/pager.h
@@ -5,6 +5,7 @@ struct child_process;
 
 const char *git_pager(int stdout_is_tty);
 void setup_pager(void);
+void wait_for_pager(void);
 int pager_in_use(void);
 int term_columns(void);
 void term_clear_line(void);
-- 
2.46.0.rc0.4.g6f4990c0d4

[PATCH 2/2] add-patch: render hunks through the pager

[Rubén Justo]

Make the print command trigger the pager when invoked using a capital
'P', to make it easier for the user to review long hunks.

Note that if the PAGER ends unexpectedly before we've been able to send
the payload, perhaps because the user is not interested in the whole
thing, we might receive a SIGPIPE, which would abruptly and unexpectedly
terminate the interactive session for the user.

Therefore, we need to ignore a possible SIGPIPE signal.  Add a test for
this, in addition to the test for normal operation.

For the SIGPIPE test, we need to make sure that we completely fill the
operating system's buffer, otherwise we might not trigger the SIGPIPE
signal.  The normal size of this buffer in different OSs varies from a
few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
this limit.

For the tests, avoid the common construct to set and export one-shot
variables within the scope of a command:

    VAR=VAL command args

It happens that when "command" is a shell function that in turn executes
a "command", the behavior with "VAR" varies depending on the shell:

 ** Bash 5.2.21 **

    $ f () { bash -c 'echo A=$A'; }
    $ A=1 f
    A=1

 ** dash 0.5.12-9 **

    $ f () { bash -c 'echo A=$A'; }
    $ A=1 f
    A=1

 ** dash 0.5.10.2-6 **

    $ f () { bash -c 'echo A=$A'; }
    $ A=1 f
    A=

POSIX explicitly says the effect of this construct is unspecified.

One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash
0.5.10.2-6, so avoid using the construct and use a subshell instead.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c                | 18 +++++++++++++++---
 t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 6e176cd21a..f2c76b7d83 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -7,9 +7,11 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "pager.h"
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "strbuf.h"
+#include "sigchain.h"
 #include "run-command.h"
 #include "strvec.h"
 #include "pathspec.h"
@@ -1391,7 +1393,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
    "e - manually edit the current hunk\n"
-   "p - print the current hunk\n"
+   "p - print the current hunk, 'P' to use the pager\n"
    "? - print help\n");
 
 static int patch_update_file(struct add_p_state *s,
@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	int colored = !!s->colored.len, quit = 0;
+	int colored = !!s->colored.len, quit = 0, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
 	enum {
 		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
+				if (use_pager) {
+					setup_pager();
+					sigchain_push(SIGPIPE, SIG_IGN);
+				}
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
+				if (use_pager) {
+					sigchain_pop(SIGPIPE);
+					wait_for_pager();
+					use_pager = 0;
+				}
 			}
 
 			strbuf_reset(&s->buf);
@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = USE_HUNK;
 				goto soft_increment;
 			}
-		} else if (s->answer.buf[0] == 'p') {
+		} else if (ch == 'p') {
 			rendered_hunk_index = -1;
+			use_pager = (s->answer.buf[0] == 'P') ? 1 : 0;
 		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 6daf3a6be0..1b8617e0c1 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -591,6 +591,38 @@ test_expect_success 'print again the hunk' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success TTY 'print again the hunk (PAGER)' '
+	test_when_finished "git reset" &&
+	cat >expect <<-EOF &&
+	<GREEN>+<RESET><GREEN>15<RESET>
+	 20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
+	PAGER  10<RESET>
+	PAGER <GREEN>+<RESET><GREEN>15<RESET>
+	PAGER  20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	EOF
+	test_write_lines s y g 1 P |
+	(
+		GIT_PAGER="sed s/^/PAGER\ /" &&
+		export GIT_PAGER &&
+		test_terminal git add -p >actual
+	) &&
+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
+test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
+	test_when_finished "rm -f huge_file; git reset" &&
+	printf "\n%2500000s" Y >huge_file &&
+	git add -N huge_file &&
+	test_write_lines P q | (
+		GIT_PAGER="head -n 1" &&
+		export GIT_PAGER &&
+		test_terminal git add -p
+	)
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.46.0.rc0.4.g6f4990c0d4

Re: Re* [PATCH v2 0/2] add-p P fixups

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> On 7/25/24 5:24 PM, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>> 
>>> ... We
>>> can add a comment about the dash problem to the commit message when
>>> this fixup is squashed. Also the problem is now documented in
>>> Documentation/CodingGuidelines which is more likely to be read by
>>> other contributors.
>> 
>> That is a good thing to take into consideration, indeed.
>
> Rubén Justo (2):
>   pager: introduce wait_for_pager
>   add-patch: render hunks through the pager

Hmph, what happened to the first two out of the four patch series?
Retracted?  Or you just didn't bother sending the whole thing?

Re: [PATCH 2/2] add-patch: render hunks through the pager

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> Make the print command trigger the pager when invoked using a capital
> 'P', to make it easier for the user to review long hunks.
>
> Note that if the PAGER ends unexpectedly before we've been able to send
> the payload, perhaps because the user is not interested in the whole
> thing, we might receive a SIGPIPE, which would abruptly and unexpectedly
> terminate the interactive session for the user.
>
> Therefore, we need to ignore a possible SIGPIPE signal.  Add a test for
> this, in addition to the test for normal operation.
>
> For the SIGPIPE test, we need to make sure that we completely fill the
> operating system's buffer, otherwise we might not trigger the SIGPIPE
> signal.  The normal size of this buffer in different OSs varies from a
> few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
> this limit.

Up to this point, it is fine.

But ...

> For the tests, avoid the common construct to set and export one-shot
> variables within the scope of a command:
>
>     VAR=VAL command args
>
> It happens that when "command" is a shell function that in turn executes
> a "command", the behavior with "VAR" varies depending on the shell:
>
>  ** Bash 5.2.21 **
>
>     $ f () { bash -c 'echo A=$A'; }
>     $ A=1 f
>     A=1
>
>  ** dash 0.5.12-9 **
>
>     $ f () { bash -c 'echo A=$A'; }
>     $ A=1 f
>     A=1
>
>  ** dash 0.5.10.2-6 **
>
>     $ f () { bash -c 'echo A=$A'; }
>     $ A=1 f
>     A=
>
> POSIX explicitly says the effect of this construct is unspecified.

... unless the patch is about changing an existing

	GIT_PAGER=... test_terminal git add -p

into

	(
		GIT_PAGER=...; export GIT_PAGER
		test_terminal git add -p
	)

then all of the above is irrelevant noise that says "as the coding
guidelines tell us not to use the non-portable 'VAR=VAL shell_func'
construct, we don't."

Always write your proposed log message to those who will read "git
log -p" 6 months down the road.  To them, the trouble we had while
diagnosing the true cause of the breakage in the previous iteration
do not exist.  It is not part of the "log -p" output they see.

> One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash
> 0.5.10.2-6, so avoid using the construct and use a subshell instead.

And it does not matter if all CI platforms are updated.  As long as
our coding guidelines say not to use this construct, we don't.

In any case, that is an appropriate thing to say in a commit that
fixes use of such a construct, but not a commit that uses the right
constuct from the get-go.

I have to say that the [4/4] in the previous round, i.e., fc87b2f7
(add-patch: render hunks through the pager, 2024-07-25) in my tree,
is better than this version.

Thanks.

Re: Re* [PATCH v2 0/2] add-p P fixups

[Rubén Justo]

On Fri, Jul 26, 2024 at 11:24:50AM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > On 7/25/24 5:24 PM, Junio C Hamano wrote:
> >> Phillip Wood <phillip.wood123@gmail.com> writes:
> >> 
> >>> ... We
> >>> can add a comment about the dash problem to the commit message when
> >>> this fixup is squashed. Also the problem is now documented in
> >>> Documentation/CodingGuidelines which is more likely to be read by
> >>> other contributors.
> >> 
> >> That is a good thing to take into consideration, indeed.
> >
> > Rubén Justo (2):
> >   pager: introduce wait_for_pager
> >   add-patch: render hunks through the pager
> 
> Hmph, what happened to the first two out of the four patch series?
> Retracted?  Or you just didn't bother sending the whole thing?
> 

No, I just wanted to modify only the two commits in rj/add-p-pager that
were affected by the fixups. 

I thought it wasn't necessary to modify the first two, which remain
correct, and I didn't want to bring them up again.  Additionally,
keeping the dates of the first two different from the two modified here
could be interesting.

That's the reason.  I didn't want to cause any distraction or add any
inconvenience, if that has been the case.

Re: Re* [PATCH v2 0/2] add-p P fixups

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> I thought it wasn't necessary to modify the first two, which remain
> correct, and I didn't want to bring them up again.  Additionally,
> keeping the dates of the first two different from the two modified here
> could be interesting.

Then at least you should have said so.  From the receiving end, that
and retracting the first two would look the same, so there needs to
be some clue to let the receiver tell which one is the case.

Thanks.

Re: Re* [PATCH v2 0/2] add-p P fixups

[Rubén Justo]

On Fri, Jul 26, 2024 at 12:48:34PM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > I thought it wasn't necessary to modify the first two, which remain
> > correct, and I didn't want to bring them up again.  Additionally,
> > keeping the dates of the first two different from the two modified here
> > could be interesting.
> 
> Then at least you should have said so.  From the receiving end, that
> and retracting the first two would look the same, so there needs to
> be some clue to let the receiver tell which one is the case.

That was my intention with:

base-commit: 506f457e489b2097e2d4fc5ceffd6e242502b2bd

But you're right, I should have made my intentions clearer.

Re: [PATCH 2/2] add-patch: render hunks through the pager

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> In any case, that is an appropriate thing to say in a commit that
> fixes use of such a construct, but not a commit that uses the right
> constuct from the get-go.
>
> I have to say that the [4/4] in the previous round, i.e., fc87b2f7
> (add-patch: render hunks through the pager, 2024-07-25) in my tree,
> is better than this version.

I do recall that you once had a version where the code violates the
guidelines (and breaks dash) in one patch, and gets fixed in another
patch in the same series.  The above material would be a perfect fit
in the proposed log message of the latter step.  If we spent so much
effort and digging to figure out exactly how it breaks with which
shell, a separate patch to fix, primarily to document the fix, would
have made sense.

But the latest squashes the two and avoids making the mistake in the
first place, eliminating the need for a documented fix.  We generally
prefer to do so to avoid breaking bisection (and the recommendation
to keep the fix separate so that we can document it better was made
as an exception), so squashing them into one is fine.  But if we
commit to that approach to pretend that there was no silly mistake,
we should be consistent in pretending that is the case.

Re: [PATCH 2/2] add-patch: render hunks through the pager

[Rubén Justo]

On Fri, Jul 26, 2024 at 06:40:49PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > In any case, that is an appropriate thing to say in a commit that
> > fixes use of such a construct, but not a commit that uses the right
> > constuct from the get-go.
> >
> > I have to say that the [4/4] in the previous round, i.e., fc87b2f7
> > (add-patch: render hunks through the pager, 2024-07-25) in my tree,
> > is better than this version.
> 
> I do recall that you once had a version where the code violates the
> guidelines (and breaks dash) in one patch, and gets fixed in another
> patch in the same series.  The above material would be a perfect fit
> in the proposed log message of the latter step.  If we spent so much
> effort and digging to figure out exactly how it breaks with which
> shell, a separate patch to fix, primarily to document the fix, would
> have made sense.
> 
> But the latest squashes the two and avoids making the mistake in the
> first place, eliminating the need for a documented fix.  We generally
> prefer to do so to avoid breaking bisection (and the recommendation
> to keep the fix separate so that we can document it better was made
> as an exception), so squashing them into one is fine.  But if we
> commit to that approach to pretend that there was no silly mistake,
> we should be consistent in pretending that is the case.
> 

Fixing a problematic change with a new commit isn't the best idea if
we have the opportunity to prevent the problem in the first place, as
Phillip pointed out.  Since rj/add-p-pager is still open, it's
worthwhile to amend the problematic commit.

Of course, we've now updated the documentation [*1*] and reinforced
[*2*] the mechanisms to prevent this from happening again.

However, I think adding a comment about the issue to the amended
commit, which I think it has been suggested at some point, seems to me
like a good addition.  I do not believe that a future reading of the
change will lead to confusion for this reason.  The added comment does
not document a fix, I think, but rather it is an explanation of what
we're doing in the commit.

Furthermore, we capture in the history, IMHO, notes of how things have
happened, which is also why I intend to apply this series on
506f457e489b2097e2d4fc5ceffd6e242502b2bd, to only amend the last two
commits.

   1.- jc/doc-one-shot-export-with-shell-func

   2.- es/shell-check-updates

Re: Re* [PATCH v2 0/2] add-p P fixups

[Rubén Justo]

On Sat, Jul 27, 2024 at 04:33:11PM +0200, Rubén Justo wrote:

> Fixing a problematic change with a new commit isn't the best idea if
> we have the opportunity to prevent the problem in the first place, as
> Phillip pointed out.  Since rj/add-p-pager is still open, it's
> worthwhile to amend the problematic commit.
> 
> Of course, we've now updated the documentation [*1*] and reinforced
> [*2*] the mechanisms to prevent this from happening again.
> 
> However, I think adding a comment about the issue to the amended
> commit, which I think it has been suggested at some point, seems to me
> like a good addition.  I do not believe that a future reading of the
> change will lead to confusion for this reason.  The added comment does
> not document a fix, I think, but rather it is an explanation of what
> we're doing in the commit.
> 
> Furthermore, we capture in the history, IMHO, notes of how things have
> happened, which is also why I intend to apply this series on
> 506f457e489b2097e2d4fc5ceffd6e242502b2bd, to only amend the last two
> commits.
>
>  1.- jc/doc-one-shot-export-with-shell-func
> 
>  2.- es/shell-check-updates

After re-reading the series today, I still believe the change in the
message for [2/2] or rebasing on 506f457e48, add value to the series,
but I also see that it's not a significant improvement.  Besides that
minor detail, IMHO, I think we have consensus on the changes.

I'm not going to send a new iteration, not because I'm against
changing the message, but because I think we are entering, if we
aren't already, the realm of bikeshedding.  

Once the changes settle, I'll send a new series to address the new
"|[cmd]" command.

For reference, this was the first message about the 'P' command:
1d0cb55c-5f32-419a-b593-d5f0969a51fd@gmail.com.  After all, I was only
interested in reviewing hunks longer that one screen height ;)

Thanks, all.

Re: Re* [PATCH v2 0/2] add-p P fixups

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> After re-reading the series today, I still believe the change in the
> message for [2/2] or rebasing on 506f457e48, add value to the series,
> but I also see that it's not a significant improvement.  Besides that
> minor detail, IMHO, I think we have consensus on the changes.

I think we have established that the "Don't attempt a one-shot
export with shell functions---it would not work" is not all that
important to stress on *in* *the* *context* *of* *this* *series*.

After all, that is why the latest shape of the series is not to do
the "keep the already known to be bad commit, followed by a fix-up
to illustrate exactly why the first one breaks" pattern, which is
designed to help future developers when the breakage is rather
subtle and we all miss in our initial reviews, but is rather unusual
for a topic that hasn't hit 'next' yet.  Instead we just correct our
mistakes and pretend as if we just got straight to the right
solution.

So, let's just use what we already have queued, without details that
are irrelevant for the final shape of the history that did not have
such a screw-up in the code.  The "Don't attempt a one-shot export
with shell functions" message, as you said, is already captured in a
more relevant place to help developers.  We could expand that part
in the documentation patch, or even a new documentation patch, but
this topic, and especially its test part, is about ensuring that the
pager support added here will correctly deals with a stuck pipe, and
no developers who hit this commit either by browsing "git log" or by
finding it in "git blame" would be seeking an advice on "one-shot
export", as that is not a mistake this series _did_ _not_ commit, at
least to them.