| Message ID | 1dc9ebad-768b-4c1a-8a58-8a7a5d24d49e@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use the pager in 'add -p' | expand |
Hi Rubén On 14/07/2024 17:04, Rubén Justo wrote: > 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. Thanks for updating the commit message to explain the purpose of the SIGPIPE test > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > +test_expect_success TTY 'P does not break if pager ends unexpectedly' ' I think it would be helpful to mention SIGPIPE in the title as this test is really checking "we don't die if we receive SIGPIPE". Maybe 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 >actual If we're not going to look at the output we don't need to redirect it. I'm not sure if there is any benefit to comparing the actual output to what we expect here. Best Wishes Phillip > +' > + > test_expect_success 'split hunk "add -p (edit)"' ' > # Split, say Edit and do nothing. Then: > #
Rubén Justo <rjusto@gmail.com> writes: > +test_expect_success TTY 'P does not break if pager ends unexpectedly' ' > + 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 >actual > +' Somehow this is dying with signal 11? https://github.com/git/git/actions/runs/9944800531/job/27471636926#step:5:1108 I know there is a v4 that has a small update here, but in case that this still is relevant and the removed rediraction to ">actual" did not magically fixed it... Thanks.
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
+ )
'
---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]`.
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");
Which resolves the problem.
This was a change already commented here:
https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/
Hi Ruében Thanks for looking into the test failure. 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? > ---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? > 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/ My worry was that this would paper over a bug as we shouldn't be calling wait_for_pager() without setting up the pager successfully. How easy would it be to fix the source of the problem? Best Wishes Phillip
phillip.wood123@gmail.com writes: >> - 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? Yeah, it indeed is curious. The rewrite resolves another iffy point in the original---you are not supposed to attempt a one-shot assignment to the environment variable when you are running a shell function, as that is not portable. And the above rewrite is a common way to fix that. But still, yes, it is curious why the original segfaults. Is there some race there and having a subshell shifts the timing, or something? > My worry was that this would paper over a bug as we shouldn't be > calling wait_for_pager() without setting up the pager > successfully. How easy would it be to fix the source of the problem? ;-) Nice to see people trying to do the right thing. Thanks.
On Wed, Jul 17, 2024 at 4:04 PM Junio C Hamano <gitster@pobox.com> wrote: > phillip.wood123@gmail.com writes: > >> - 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? > > Yeah, it indeed is curious. > > The rewrite resolves another iffy point in the original---you are > not supposed to attempt a one-shot assignment to the environment > variable when you are running a shell function, as that is not > portable. And the above rewrite is a common way to fix that. It's also curious that t/check-non-portable-shell.pl didn't catch this use of one-shot assignment when calling a shell function[*]. [*] a0a630192d (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13)
Eric Sunshine <sunshine@sunshineco.com> writes: > On Wed, Jul 17, 2024 at 4:04 PM Junio C Hamano <gitster@pobox.com> wrote: >> phillip.wood123@gmail.com writes: >> >> - 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? >> >> Yeah, it indeed is curious. >> >> The rewrite resolves another iffy point in the original---you are >> not supposed to attempt a one-shot assignment to the environment >> variable when you are running a shell function, as that is not >> portable. And the above rewrite is a common way to fix that. > > It's also curious that t/check-non-portable-shell.pl didn't catch this > use of one-shot assignment when calling a shell function[*]. True.
phillip.wood123@gmail.com writes: >> - 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? Another difference besides the sub-shell is where the output from test_terminal goes. If the above change fixes the issue for Rubén, I wonder if it still works if ">actual" gets removed.
On 17/07/2024 21:03, Junio C Hamano wrote: > phillip.wood123@gmail.com writes: > >>> - 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? > > Yeah, it indeed is curious. > > The rewrite resolves another iffy point in the original---you are > not supposed to attempt a one-shot assignment to the environment > variable when you are running a shell function, as that is not > portable. And the above rewrite is a common way to fix that. Good point, I'd not thought of that. Best Wishes Phillip
On 17/07/2024 21:31, Junio C Hamano wrote: > phillip.wood123@gmail.com writes: > >>> - 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? > > Another difference besides the sub-shell is where the output from > test_terminal goes. If the above change fixes the issue for Rubén, > I wonder if it still works if ">actual" gets removed. Yes that would be interesting to know. I do think we should be checking the output here to make sure the test is doing what we think it is - all we know at the moment is whether git exits successfully or not. If we receive SIGPIPE then fputs() will see EPIPE which will set ferror() which we should be clearing and checking that the prompt prints correctly afterwards. I think it would be helpful to add tail -n3 actual >actual.trimmed && cat >expect <<\EOF && \ 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,?]? EOF test_cmp expect actual.trimmed to the test. That all still leaves me wondering about the segfault though Best Wishes Phillip
Hi Rubén 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. It already dies if we cannot execute the pager so maybe we should just die on other errors as well? Best Wishes Phillip
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/
On Wed, Jul 17, 2024 at 04:09:17PM -0400, Eric Sunshine wrote: > > >> - test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p > It's also curious that t/check-non-portable-shell.pl didn't catch this > use of one-shot assignment when calling a shell function[*]. It would have been great if it had caught that error. As a reference: func () { } VAR=value func # this error is caught echo 1 | VAR=value func # this one is also caught echo 1 | VAR=value func # this one isn't Maybe, catch this errors expanding the regular expression we have in `check-non-portable-shell.pl` isn't the best approach. We might need something more sophisticated, like what we have in `chainlint.pl`. Perhaps someone with experience in those scripts could give us this capability :-) Thanks for your message.
On Wed, Jul 17, 2024 at 01:31:16PM -0700, Junio C Hamano wrote: > >> - 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 > >> + ) > I wonder if it still works if ">actual" gets removed. Yes, it works. That redirection is just noise from my previous version.
On Thu, Jul 18, 2024 at 10:58:36AM +0100, phillip.wood123@gmail.com 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. > > It already dies if we cannot execute the pager so maybe we should just die > on other errors as well? Honestly, I thought 78f0a5d187 (pager: die when paging to non-existing command, 2024-06-23) would be sufficient for this series, but I missed the optimization mentioned in my previous message. Thinking in the context of "add -p", it might be more sensible not to die but simply show an error, so as not to end the user's interactive session. But it could be a change in a future series and thus avoid prolonging this one. However, if you can think of any other cases where we should be stricter and die, I'm all ears.
On Sat, Jul 20, 2024 at 6:37 PM Rubén Justo <rjusto@gmail.com> wrote: > On Wed, Jul 17, 2024 at 04:09:17PM -0400, Eric Sunshine wrote: > > > >> - test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p > > > It's also curious that t/check-non-portable-shell.pl didn't catch this > > use of one-shot assignment when calling a shell function[*]. > > It would have been great if it had caught that error. > > As a reference: > VAR=value func # this error is caught > echo 1 | > VAR=value func # this one is also caught > echo 1 | VAR=value func # this one isn't Thanks for providing this summary; it saved me the effort of digging back through this discussion / patch series. > Maybe, catch this errors expanding the regular expression we have in > `check-non-portable-shell.pl` isn't the best approach. We might need > something more sophisticated, like what we have in `chainlint.pl`. The idea has been expressed previously of subsuming all the check-non-portable-shell.pl checks into chainlint.pl some day, thus allowing check-non-portable-shell.pl to be retired. In fact, it was mentioned again quite recently[1]. However, this particular check (detecting `VAR=val shell-func`) poses an extra complication which would require some specialized additional mechanism in chainlint.pl. In particular, in `VAR=val symbol`, in order to distinguish when `symbol` is an external command versus a shell-function, it is necessary to scan for function definitions not just in the script being checked, but also in all scripts included (recursively) by the script being checked. So, it's probably possible to do but ought to be done carefully. > Perhaps someone with experience in those scripts could give us this > capability :-) I posted a series[2] which addresses this shortcoming by enhancing check-non-portable-shell.pl. [1]: https://lore.kernel.org/git/CAPig+cTFZuU7zM7poqk4HeK09zn8bFrO37eUZiaGmeJ0yecpiw@mail.gmail.com/ [2]: https://lore.kernel.org/git/20240722065915.80760-1-ericsunshine@charter.net/T/
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@gmail.com 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@gmail.com >>> 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/
On Mon, Jul 22, 2024 at 03:18:07AM -0400, Eric Sunshine wrote: > I posted a series[2] which addresses this shortcoming by enhancing > check-non-portable-shell.pl. I tested this series with them, and it correctly detects the error. Thanks for sending the improvement so quick!
On Mon, Jul 22, 2024 at 11:18:00AM +0100, Phillip Wood wrote: > > 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. Yep, missing that old optimization is easy ;) > 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. Now that we agree, I'll do it :) > 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" I'm not sure it would be valuable for us to make the caller aware that "there is no pager or the pager is 'cat' ... just use stdout". However, I do agree that probably in the future, if we finally add the "|[cmd]" command, we'll need to return some kind of error instead of `die()`, in setup_pager().
Rubén Justo <rjusto@gmail.com> writes: >> > + 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;
On Mon, Jul 22, 2024 at 10:22:58AM -0700, Junio C Hamano wrote: > 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");
Rubén Justo <rjusto@gmail.com> writes: >> 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. So, we may have a shell that does not behave like others ;-) Do you know what shell is being used? Thanks.
Rubén Justo <rjusto@gmail.com> writes: > 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 > ) This took sufficiently large amount of collective braincycles, and it would be worth documenting as a separate patch, I would suspect. Something along the following lines, but please take the authorship *and* give it a better explanation. Thanks. --- >8 --- Subject: [PATCH] t3701: avoid one-shot export for shell functions The common construct VAR=VAL command args to temporarily set and export environment variable VAR only while "command args" is running is handy, but one of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash 0.5.10.2-6 failed with the construct, making only a temporary assignment without exporting the variable, when command is *not* an external (in this case, a shell function). 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: Junio C Hamano <gitster@pobox.com> --- t/t3701-add-interactive.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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)"' '
On Mon, Jul 22, 2024 at 02:25:45PM -0700, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > 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 > > ) > > This took sufficiently large amount of collective braincycles, and > it would be worth documenting as a separate patch, I would suspect. > > Something along the following lines, but please take the authorship > *and* give it a better explanation. Here's an attempt. I'm also adding the change for `wait_for_pager`, which could be squashed in b29c59e3d2 (pager: introduce wait_for_pager, 2024-07-16). Although, highlighted I think it's interesting as well. But I don't have a strong preference. This builds on rj/add-p-pager. Thanks. Rubén Justo (2): 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(-) base-commit: 6bc52a5543008bff2c6ec7a0a935c7fc1f79e646
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..2ac860cc42 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -591,6 +591,34 @@ 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 does not break if pager ends unexpectedly' ' + 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 >actual +' + test_expect_success 'split hunk "add -p (edit)"' ' # Split, say Edit and do nothing. Then: #
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 | 28 ++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-)