Hi Rubén
On 20/07/2024 23:29, Rubén Justo wrote:
On Wed, Jul 17, 2024 at 08:39:12PM +0100, phillip.wood123@xxxxxxxxx wrote:
On 17/07/2024 18:20, Rubén Justo wrote:
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,
Oh I see, I had assumed that the shell would report an error if it
didn't support setting environment variables like this when running a
function but instead it silently ignored it. This highlights the
importance of testing the output of "git add -p" in this test so we can
be sure the pager is doing what we think it should. Using
GIT_PAGER="sed -e s/^/PAGER\ / -e q"
would make it clear what the pager is printing while also causing SIGPIPE
"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:
That extra information is important. When I said [1]
> Isn't it a bug to call this with old_fd1 == -1 or have I missed
> something?
What I'd missed was that we can return early without executing anything.
We cannot do
if (!git_pager(asatty(1))
return
at the beginning of wait_for_pager() because if we're running a pager
isatty(1) will return false so I think the old_fd as you suggested is
the easiest fix. The existing callers do not need to know if
setup_pager() applied the "cat" optimization because they only setup the
pager once. For "add -p" this no-longer applies so we should think about
returning a flag to say "there was an error"/"there is no pager or the
pager is 'cat'"/"the pager has been started"
Best Wishes
Phillip
[1]
https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@xxxxxxxxx
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@xxxxxxxxx/
