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

["Rubén Justo" <rjusto@gmail.com>]

On Wed, Jul 17, 2024 at 08:39:12PM +0100, phillip.wood123@gmail.com wrote:

> On 17/07/2024 18:20, Rubén Justo wrote:
> > Squashing this fixes the test:
> > 
> > --->8---
> > 
> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> > index c60589cb94..bb360c92a0 100755
> > --- a/t/t3701-add-interactive.sh
> > +++ b/t/t3701-add-interactive.sh
> > @@ -616,7 +616,12 @@ 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 >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".

> 
> > ---8<---
> > 
> > However, this error has exposed a problem: calling `wait_for_pager` if
> > `setup_pager` hasn't worked is an issue that needs to be addressed in this
> > series: `setup_pager` should return a result.  I was planning to do that
> > in a future series, for the other commented command: `|[cmd]`.
> 
> What was causing setup pager to fail in this test?

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

That's why I thought, aligned with what we are already doing in
`wait_for_pager_at_exit()`, that this is a sensible approach: 

> > I'm wondering if the best way to proceed here is to revert to:
> > 
> > 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");
> >
> > This was a change already commented here:
> > 
> > https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/