threads / patch / 61825

v2, 2 partsadd-p P fixups

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

## tl;dr

29 messages between Jul 23, 2024 and Jul 29, 2024. Diffs are folded; open one to read it.

replies: 28people: 3as markdown or json

Rubén Justo· Jul 23, 2024, 00:39 UTC · lore
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
Rubén Justo· Jul 23, 2024, 00:42 UTC · re: Rubén Justo · lore

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

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(-)
Show changes to t/t3701-add-interactive.sh +5 −1
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
Rubén Justo· Jul 23, 2024, 00:42 UTC · re: Rubén Justo · lore

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

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(+)
Show changes to pager.c +3 −0
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
Junio C Hamano· Jul 23, 2024, 00:54 UTC · re: Rubén Justo · lore

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

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.

Show 16 quoted lines
>     +    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.

Show 7 quoted lines
>     +    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.
Phillip Wood· Jul 23, 2024, 09:15 UTC · re: Rubén Justo · lore

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

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:
Show 52 quoted lines
> 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)
Junio C Hamano· Jul 23, 2024, 16:52 UTC · re: Phillip Wood · lore

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

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 5 quoted lines
> 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.
Rubén Justo· Jul 23, 2024, 22:08 UTC · re: Phillip Wood · lore

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

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.

Show 52 quoted lines
> 
> 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)
phillip.wood123@gmail.com· Jul 24, 2024, 15:21 UTC · re: Rubén Justo · lore

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

Hi Rubén
On 23/07/2024 23:08, Rubén Justo wrote:
Show 12 quoted lines
> 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
Show 52 quoted lines
>>
>> 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)
Rubén Justo· Jul 24, 2024, 16:12 UTC · re: phillip.wood123@gmail.com · lore

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

On Wed, Jul 24, 2024 at 04:21:53PM +0100, phillip.wood123@gmail.com wrote:
Show 7 quoted lines
> > 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
Phillip Wood· Jul 25, 2024, 09:45 UTC · re: Rubén Justo · lore

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

Hi Rubén
On 24/07/2024 17:12, Rubén Justo wrote:
Show 8 quoted lines
> 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
Show 8 quoted lines
> 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
Rubén Justo· Jul 25, 2024, 12:16 UTC · re: Phillip Wood · lore

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

On Thu, Jul 25, 2024 at 10:45:04AM +0100, Phillip Wood wrote:
Show 13 quoted lines
> 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 ;).
Show 12 quoted lines
> 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.

Show 13 quoted lines
> 
> 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.
Rubén Justo· Jul 25, 2024, 13:42 UTC · re: Rubén Justo · lore

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

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
Rubén Justo· Jul 25, 2024, 13:44 UTC · re: Rubén Justo · lore

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

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(+)
Show changes to t/t3701-add-interactive.sh +16 −0
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
Rubén Justo· Jul 25, 2024, 13:44 UTC · re: Rubén Justo · lore

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

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(-)
Show changes to pager.c +6 −2
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
Rubén Justo· Jul 25, 2024, 13:44 UTC · re: Rubén Justo · lore

[PATCH 3/4] pager: introduce wait_for_pager

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(-)
Show changes to 2 files +41 −6

pager.c, pager.h

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
Rubén Justo· Jul 25, 2024, 13:44 UTC · re: Rubén Justo · lore

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

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(-)
Show changes to 2 files +47 −3

add-patch.c, t/t3701-add-interactive.sh

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
Junio C Hamano· Jul 25, 2024, 15:24 UTC · re: Phillip Wood · lore

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

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 5 quoted lines
> ... 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· Jul 25, 2024, 16:41 UTC · re: Junio C Hamano · lore

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

On 7/25/24 5:24 PM, Junio C Hamano wrote:
Show 9 quoted lines
> 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
Rubén Justo· Jul 25, 2024, 16:43 UTC · re: Rubén Justo · lore

[PATCH 1/2] pager: introduce wait_for_pager

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(-)
Show changes to 2 files +41 −6

pager.c, pager.h

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
Rubén Justo· Jul 25, 2024, 16:43 UTC · re: Rubén Justo · lore

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

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(-)
Show changes to 2 files +47 −3

add-patch.c, t/t3701-add-interactive.sh

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
Junio C Hamano· Jul 26, 2024, 18:36 UTC · re: Rubén Justo · lore

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

Rubén Justo <rjusto@gmail.com> writes:
Show 16 quoted lines
> 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 ...
Show 27 quoted lines
> 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.
Junio C Hamano· Jul 27, 2024, 01:40 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> 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.

Rubén Justo· Jul 27, 2024, 14:33 UTC · re: Junio C Hamano · lore

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

On Fri, Jul 26, 2024 at 06:40:49PM -0700, Junio C Hamano wrote:
Show 26 quoted lines
> 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
Junio C Hamano· Jul 26, 2024, 18:24 UTC · re: Rubén Justo · lore

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

Rubén Justo <rjusto@gmail.com> writes:
Show 14 quoted lines
> 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?

Rubén Justo· Jul 26, 2024, 19:22 UTC · re: Junio C Hamano · lore

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

On Fri, Jul 26, 2024 at 11:24:50AM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> 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.

Junio C Hamano· Jul 26, 2024, 19:48 UTC · re: Rubén Justo · lore

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

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.
Rubén Justo· Jul 26, 2024, 20:16 UTC · re: Junio C Hamano · lore

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

On Fri, Jul 26, 2024 at 12:48:34PM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> 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.
Rubén Justo· Jul 28, 2024, 09:11 UTC · re: Rubén Justo · lore

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

On Sat, Jul 27, 2024 at 04:33:11PM +0200, Rubén Justo wrote:
Show 23 quoted lines
> 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.
Junio C Hamano· Jul 29, 2024, 18:45 UTC · re: Rubén Justo · lore

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

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.

← back to recent threads