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

From: Rubén Justo <rjusto@gmail.com>
Date: Jul 22, 2024, 19:06 UTC
Message-ID: <079901fe-7889-4e1f-bb91-610e1eae25d3@gmail.com>
In-Reply-To: <xmqqv80xcpe5.fsf@gitster.g>

On Mon, Jul 22, 2024 at 10:22:58AM -0700, Junio C Hamano wrote:

Show 49 quoted lines

> Attached at the end is a test tweak patch, taking inspirations from
> Phillip's comments, to see what value GIT_PAGER has in the shell
> function.  I shortened the huge_file a bit so that I do not have to
> have an infinite scrollback buffer,but otherwise, the test_quirk
> intermediate shell function should work just like the test_terminal
> helper in the original position would.
> 
> And I see in the output from "sh t3701-add-interactive.sh -i -v":
> 
>     expecting success of 3701.51 'P handles SIGPIPE when writing to pager': 
>             test_when_finished "rm -f huge_file; git reset" &&
>             printf "\n%250s" Y >huge_file &&
>             git add -N huge_file &&
>             echo "in env: GIT_PAGER=$(env | grep GIT_PAGER=)" &&
>             test_write_lines P q | GIT_PAGER="head -n 1" test_quirk &&
>             echo "after test_quirk returns: GIT_PAGER=$GIT_PAGER"
> 
>     in env: GIT_PAGER=
>     in test_quirk: GIT_PAGER=head -n 1
>     in env: GIT_PAGER=GIT_PAGER=head -n 1
>     In test_terminal: GIT_PAGER=GIT_PAGER=head -n 1
>     test-terminal: GIT_PAGER=head -n 1
>     diff --git a/huge_file b/huge_file
>     new file mode 100644
>     index 0000000..d06820d
>     --- /dev/null
>     +++ b/huge_file
>     @@ -0,0 +1,2 @@
>     +
>     +                                                                                                                                                                                                                                                         Y
>     \ No newline at end of file
>     (1/1) Stage addition [y,n,q,a,d,e,p,?]? @@ -0,0 +1,2 @@
>     (1/1) Stage addition [y,n,q,a,d,e,p,?]? 
>     after test_quirk returns: GIT_PAGER=
>     Unstaged changes after reset:
>     M       test
>     ok 51 - P handles SIGPIPE when writing to pager
> 
> So:
> 
>  - before the one-shot thing, in the envrionment GIT_PAGER is empty.
>  - in the helper function,
>    - shell variable GIT_PAGER is set to the expected value.
>    - GIT_PAGER env is exported.
>    - test-terminal.perl sees $ENV{GIT_PAGER} set to the expected value.
>  - after the helper returns GIT_PAGER is empty
> 
> It's a very convincing theory but it does not seem to match my
> observation.  Is there a difference in shells used, or something?

Have you tried your tweak in the "linux-gcc (ubuntu-20.04)" test environment where the problem was detected? In that environment, the value of GIT_PAGER is not passed to Git in that test.

To fix the test, as already said, we need this:

	test_write_lines P q |
	(
		GIT_PAGER="head -n 1" &&
		export GIT_PAGER &&
		test_terminal git add -p >actual
	)

And this series also need the other other change that I'm discussing with Phillip:

diff --git a/pager.c b/pager.c
index 5f0c1e9cce..5586e751dc 100644
--- a/pager.c
+++ b/pager.c
@@ -46,6 +46,8 @@ 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");