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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 22, 2024, 17:22 UTC
Message-ID: <xmqqv80xcpe5.fsf@gitster.g>
In-Reply-To: <a2ea00e2-08e4-4e6b-b81c-ef3ba02b4b1f@gmail.com>

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

Show 21 quoted lines

>> > +	test_write_lines P q |
>> > +	(
>> > +		GIT_PAGER="head -n 1" &&
>> > +		export GIT_PAGER &&
>> > +		test_terminal git add -p >actual
>> > +	)
>> 
>> That's surprising, why does running git in a sub-shell stop it from
>> segfaulting?
>
> The fix isn't the sub-shell;  it's "export GIT_PAGER".
> ...
> Because GIT_PAGER is not being set correctly in the test, "git add -p"
> can use the values defined in the environment where the test is running.
> Usually PAGER is empty or contains "less", but in the environment where
> the fault occurs, it happens to be: "PAGER=cat". 
>
> Since we have an optimization to avoid forking if the pager is "cat",
> courtesy of caef71a535 (Do not fork PAGER=cat, 2006-04-16), then we fail
> in `wait_for_pager()` because we are calling `finish_command()` with an
> uninitialized `pager_process`.

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?

 t/lib-terminal.sh          |  3 +++
 t/t3701-add-interactive.sh | 15 +++++++++++++--
 t/test-terminal.perl       |  2 ++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git c/t/lib-terminal.sh w/t/lib-terminal.sh
index e3809dcead..558db9aa33 100644
--- c/t/lib-terminal.sh
+++ w/t/lib-terminal.sh
@@ -9,6 +9,9 @@ test_terminal () {
 		echo >&4 "test_terminal: need to declare TTY prerequisite"
 		return 127
 	fi
+
+	echo >&4 "In test_terminal: GIT_PAGER=$(env | grep GIT_PAGER=)"
+
 	perl "$TEST_DIRECTORY"/test-terminal.perl "$@" 2>&7
 } 7>&2 2>&4
 
diff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh
index c60589cb94..f7037cbed4 100755
--- c/t/t3701-add-interactive.sh
+++ w/t/t3701-add-interactive.sh
@@ -612,13 +612,24 @@ test_expect_success TTY 'print again the hunk (PAGER)' '
 	test_cmp expect actual.trimmed
 '
 
+test_quirk () {
+	echo "in test_quirk: GIT_PAGER=$GIT_PAGER"
+	echo "in env: GIT_PAGER=$(env | grep GIT_PAGER=)"
+	test_terminal git add -p
+	true
+}
+
 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 &&
+	printf "\n%250s" Y >huge_file &&
 	git add -N huge_file &&
-	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
+	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"
 '
 
+exit
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
diff --git c/t/test-terminal.perl w/t/test-terminal.perl
index b8fd6a4f13..92b1c13675 100755
--- c/t/test-terminal.perl
+++ w/t/test-terminal.perl
@@ -67,6 +67,8 @@ sub copy_stdio {
 if ($#ARGV < 1) {
 	die "usage: test-terminal program args";
 }
+print STDERR "test-terminal: GIT_PAGER=$ENV{GIT_PAGER}\n";
+
 $ENV{TERM} = 'vt100';
 my $parent_out = new IO::Pty;
 my $parent_err = new IO::Pty;