{"thread":{"id":"61768","subject":"[PATCH 0/4] use the pager in 'add -p'","startedAt":"2024-07-12T00:57:48Z","lastAt":"2024-07-23T02:31:07Z","messageCount":69,"participants":["Rubén Justo","Dragan Simic","Phillip Wood","Junio C Hamano","phillip.wood123@gmail.com","Eric Sunshine","Kyle Lippincott"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"498557","messageId":"2653fb37-c8a8-49b1-a804-4be6654a2cad@gmail.com","threadId":"61768","inReplyTo":null,"subject":"[PATCH 0/4] use the pager in 'add -p'","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-12T00:57:44Z","receivedAt":"2024-07-12T00:57:48Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"I'm resuming work on introducing a mechanism to use the PAGER to display\nhunks during interactive \"git add -p\" sessions, which will make it\neasier to review large hunks.\n\nThe thread where the previous discussion took place is:\nhttps://lore.kernel.org/git/1d0cb55c-5f32-419a-b593-d5f0969a51fd@gmail.com/\n\nI'm bringing back the proposal to introduce 'P' as a mechanism to\ndisplay the current hunks through the PAGER.\n\nI think it's sensible to exclude from the scope of this series the\noption of a new command '|[cmd]' and other modifications to the original\nproposal that have raised questions that perhaps deserve their own\ndiscussion, outside the scope of this series. Questions like:\n\n - What to do with ANSI codes?\n - How to allow the definition of a default command?\n - How to facilitate the reuse of a command?\n - How to combine a default command with command reuse?\n - What to do if the command fails?\n\nTo mention a few...\n\nI'm also leaving for a future series a possible configuration\n\"interactive.pipeCommand\", \"interactive.pager\" or similar.\n\nI hope this approach makes sense and allows us to move forward, and that\nit doesn't represent a step back.\n\nThanks.\n\nRubén Justo (4):\n add-patch: test for 'p' command\n pager: do not close fd 2 unnecessarily\n pager: introduce wait_for_pager\n add-patch: render hunks through the pager\n\n add-patch.c | 18 ++++++++++--\n pager.c | 45 ++++++++++++++++++++++++----\n pager.h | 1 +\n t/t3701-add-interactive.sh | 60 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 116 insertions(+), 8 deletions(-)\n\n-- \n2.45.1\n"},{"id":"498558","messageId":"3e518245-11f1-413b-a2e6-e3b3efe3d7b9@gmail.com","threadId":"61768","inReplyTo":"2653fb37-c8a8-49b1-a804-4be6654a2cad@gmail.com","subject":"[PATCH 1/4] add-patch: test for 'p' command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-12T01:00:14Z","receivedAt":"2024-07-12T01:00:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Add a test for the 'p' command, which was introduced in 66c14ab592\n(add-patch: introduce 'p' in interactive-patch, 2024-03-29).\n\nSigned-off-by: Rubén Justo \n---\n t/t3701-add-interactive.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 5d78868ac1..6daf3a6be0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -575,6 +575,22 @@ test_expect_success 'navigate to hunk via regex / pattern' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success 'print again the hunk' '\n+\ttest_when_finished \"git reset\" &&\n+\ttr _ \" \" >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n+\t 10\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\tEOF\n+\ttest_write_lines s y g 1 p | git add -p >actual &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.45.1\n"},{"id":"498559","messageId":"fd00e664-93bc-4dc0-b032-f6d9b56a5e44@gmail.com","threadId":"61768","inReplyTo":"2653fb37-c8a8-49b1-a804-4be6654a2cad@gmail.com","subject":"[PATCH 2/4] pager: do not close fd 2 unnecessarily","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-12T01:00:26Z","receivedAt":"2024-07-12T01:00:30Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We send errors to the pager since 61b80509e3 (sending errors to stdout\nunder $PAGER, 2008-02-16).\n\nIn a8335024c2 (pager: do not dup2 stderr if it is already redirected,\n2008-12-15) an exception was introduced to avoid redirecting stderr if\nit is not connected to a terminal.\n\nIn such exceptional cases, the close(STDERR_FILENO) we're doing in\nclose_pager_fds, is unnecessary.\n\nFurthermore, in a subsequent commit we're going to introduce changes\nthat will involve using close_pager_fds multiple times.\n\nWith this in mind, controlling when we want to close stderr, become\nsensible.\n\nLet's close(STDERR_FILENO) only when necessary, and pave the way for the\nupcoming changes.\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex be6f4ee59f..251adfc2ad 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,6 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n+static int close_fd2;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -23,7 +24,8 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tclose(2);\n+\tif (close_fd2)\n+\t\tclose(2);\n }\n \n static void wait_for_pager_atexit(void)\n@@ -141,8 +143,10 @@ void setup_pager(void)\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\n-\tif (isatty(2))\n+\tif (isatty(2)) {\n+\t\tclose_fd2 = 1;\n \t\tdup2(pager_process.in, 2);\n+\t}\n \tclose(pager_process.in);\n \n \t/* this makes sure that the parent terminates after the pager */\n-- \n2.45.1\n"},{"id":"498560","messageId":"4b5d0e7c-9492-4495-9bc1-40ebea850fde@gmail.com","threadId":"61768","inReplyTo":"2653fb37-c8a8-49b1-a804-4be6654a2cad@gmail.com","subject":"[PATCH 3/4] pager: introduce wait_for_pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-12T01:00:37Z","receivedAt":"2024-07-12T01:00:41Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,\n2006-02-28) we have the machinery to send our output to a pager.\n\nThat machinery, once set up, does not allow us to regain the original\nstdio streams.\n\nIn the interactive commands (i.e.: add -p) we want to use the pager for\nsome output, while maintaining the interaction with the user.\n\nModify the pager machinery so that we can use setup_pager and, once\nwe've finished sending the desired output for the pager, wait for the\npager termination using a new function wait_for_pager. Make this\nfunction reset the pager machinery before returning.\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 43 +++++++++++++++++++++++++++++++++++++------\n pager.h | 1 +\n 2 files changed, 38 insertions(+), 6 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 251adfc2ad..bea4345f6f 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,7 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n-static int close_fd2;\n+static int old_fd1 = -1, old_fd2 = -1;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -24,11 +24,11 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tif (close_fd2)\n+\tif (old_fd2 != -1)\n \t\tclose(2);\n }\n \n-static void wait_for_pager_atexit(void)\n+static void finish_pager(void)\n {\n \tfflush(stdout);\n \tfflush(stderr);\n@@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)\n \tfinish_command(&pager_process);\n }\n \n+static void wait_for_pager_atexit(void)\n+{\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n+\tfinish_pager();\n+}\n+\n+void wait_for_pager(void)\n+{\n+\tfinish_pager();\n+\tsigchain_pop_common();\n+\tunsetenv(\"GIT_PAGER_IN_USE\");\n+\tdup2(old_fd1, 1);\n+\tclose(old_fd1);\n+\told_fd1 = -1;\n+\tif (old_fd2 != -1) {\n+\t\tdup2(old_fd2, 2);\n+\t\tclose(old_fd2);\n+\t\told_fd2 = -1;\n+\t}\n+}\n+\n static void wait_for_pager_signal(int signo)\n {\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n \tclose_pager_fds();\n \tfinish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n@@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n \n void setup_pager(void)\n {\n+\tstatic int once = 0;\n \tconst char *pager = git_pager(isatty(1));\n \n \tif (!pager)\n@@ -142,16 +169,20 @@ void setup_pager(void)\n \t\tdie(\"unable to execute pager '%s'\", pager);\n \n \t/* original process continues, but writes to the pipe */\n+\told_fd1 = dup(1);\n \tdup2(pager_process.in, 1);\n \tif (isatty(2)) {\n-\t\tclose_fd2 = 1;\n+\t\told_fd2 = dup(2);\n \t\tdup2(pager_process.in, 2);\n \t}\n \tclose(pager_process.in);\n \n-\t/* this makes sure that the parent terminates after the pager */\n \tsigchain_push_common(wait_for_pager_signal);\n-\tatexit(wait_for_pager_atexit);\n+\n+\tif (!once) {\n+\t\tonce++;\n+\t\tatexit(wait_for_pager_atexit);\n+\t}\n }\n \n int pager_in_use(void)\ndiff --git a/pager.h b/pager.h\nindex b77433026d..103ecac476 100644\n--- a/pager.h\n+++ b/pager.h\n@@ -5,6 +5,7 @@ struct child_process;\n \n const char *git_pager(int stdout_is_tty);\n void setup_pager(void);\n+void wait_for_pager(void);\n int pager_in_use(void);\n int term_columns(void);\n void term_clear_line(void);\n-- \n2.45.1\n"},{"id":"498561","messageId":"5effca4d-536c-4e51-a024-5f1e90583176@gmail.com","threadId":"61768","inReplyTo":"2653fb37-c8a8-49b1-a804-4be6654a2cad@gmail.com","subject":"[PATCH 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-12T01:00:48Z","receivedAt":"2024-07-12T01:00:52Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Make the print command to trigger the pager when invoked using a capital\n'P', to make it easier for the user to review long hunks.\n\nSigned-off-by: Rubén Justo \n---\n add-patch.c | 18 +++++++++++++---\n t/t3701-add-interactive.sh | 44 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 59 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 6e176cd21a..f2c76b7d83 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -7,9 +7,11 @@\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"object-name.h\"\n+#include \"pager.h\"\n #include \"read-cache-ll.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n+#include \"sigchain.h\"\n #include \"run-command.h\"\n #include \"strvec.h\"\n #include \"pathspec.h\"\n@@ -1391,7 +1393,7 @@ N_(\"j - leave this hunk undecided, see next undecided hunk\\n\"\n \"/ - search for a hunk matching the given regex\\n\"\n \"s - split the current hunk into smaller hunks\\n\"\n \"e - manually edit the current hunk\\n\"\n- \"p - print the current hunk\\n\"\n+ \"p - print the current hunk, 'P' to use the pager\\n\"\n \"? - print help\\n\");\n \n static int patch_update_file(struct add_p_state *s,\n@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,\n \tstruct hunk *hunk;\n \tchar ch;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tint colored = !!s->colored.len, quit = 0;\n+\tint colored = !!s->colored.len, quit = 0, use_pager = 0;\n \tenum prompt_mode_type prompt_mode_type;\n \tenum {\n \t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,\n \t\tstrbuf_reset(&s->buf);\n \t\tif (file_diff->hunk_nr) {\n \t\t\tif (rendered_hunk_index != hunk_index) {\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsetup_pager();\n+\t\t\t\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\t\t\t}\n \t\t\t\trender_hunk(s, hunk, 0, colored, &s->buf);\n \t\t\t\tfputs(s->buf.buf, stdout);\n \t\t\t\trendered_hunk_index = hunk_index;\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsigchain_pop(SIGPIPE);\n+\t\t\t\t\twait_for_pager();\n+\t\t\t\t\tuse_pager = 0;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tstrbuf_reset(&s->buf);\n@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\thunk->use = USE_HUNK;\n \t\t\t\tgoto soft_increment;\n \t\t\t}\n-\t\t} else if (s->answer.buf[0] == 'p') {\n+\t\t} else if (ch == 'p') {\n \t\t\trendered_hunk_index = -1;\n+\t\t\tuse_pager = (s->answer.buf[0] == 'P') ? 1 : 0;\n \t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 6daf3a6be0..bf82a9dc35 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -591,6 +591,50 @@ test_expect_success 'print again the hunk' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success TTY 'print again the hunk (PAGER)' '\n+\ttest_when_finished \"git reset\" &&\n+\tcat >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? PAGER @@ -1,2 +1,3 @@\n+\tPAGER 10\n+\tPAGER +15\n+\tPAGER 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? \n+\tEOF\n+\ttest_write_lines s y g 1 P |\n+\t(\n+\t\tGIT_PAGER=\"sed s/^/PAGER\\ /\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p >actual\n+\t) &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n+test_expect_success TTY 'P does not break if pager ends unexpectly' '\n+\ttest_when_finished \"rm -f huge_file; git reset\" &&\n+\tprintf \"%2500000s\" Y >huge_file &&\n+\tgit add -N huge_file &&\n+\tcat >expect <<-EOF &&\n+\t+22\n+\t+23\n+\t+24\n+\t 30\n+\t 40\n+\t 50\n+\t(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? \n+\tEOF\n+\ttest_write_lines P |\n+\t(\n+\t\tGIT_PAGER=\"head -1\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p >actual\n+\t) &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.45.1\n"},{"id":"498573","messageId":"ac4a2d62d9169b2370f6cf40e59f007e@manjaro.org","threadId":"61768","inReplyTo":"2653fb37-c8a8-49b1-a804-4be6654a2cad@gmail.com","subject":"Re: [PATCH 0/4] use the pager in 'add -p'","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-07-12T08:56:31Z","receivedAt":"2024-07-12T08:56:39Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Ruben,\n\nOn 2024-07-12 02:57, Rubén Justo wrote:\n> I'm resuming work on introducing a mechanism to use the PAGER to \n> display\n> hunks during interactive \"git add -p\" sessions, which will make it\n> easier to review large hunks.\n> \n> The thread where the previous discussion took place is:\n> https://lore.kernel.org/git/1d0cb55c-5f32-419a-b593-d5f0969a51fd@gmail.com/\n> \n> I'm bringing back the proposal to introduce 'P' as a mechanism to\n> display the current hunks through the PAGER.\n> \n> I think it's sensible to exclude from the scope of this series the\n> option of a new command '|[cmd]' and other modifications to the \n> original\n> proposal that have raised questions that perhaps deserve their own\n> discussion, outside the scope of this series. Questions like:\n> \n> - What to do with ANSI codes?\n> - How to allow the definition of a default command?\n> - How to facilitate the reuse of a command?\n> - How to combine a default command with command reuse?\n> - What to do if the command fails?\n> \n> To mention a few...\n> \n> I'm also leaving for a future series a possible configuration\n> \"interactive.pipeCommand\", \"interactive.pager\" or similar.\n> \n> I hope this approach makes sense and allows us to move forward, and \n> that\n> it doesn't represent a step back.\n\nI find this approach fine. It would allow us to have this neat feature\navailable in its initial, simplified form, while the future improvements\nwould belong to follow-up discussions and patches.\n\n> Rubén Justo (4):\n> add-patch: test for 'p' command\n> pager: do not close fd 2 unnecessarily\n> pager: introduce wait_for_pager\n> add-patch: render hunks through the pager\n> \n> add-patch.c | 18 ++++++++++--\n> pager.c | 45 ++++++++++++++++++++++++----\n> pager.h | 1 +\n> t/t3701-add-interactive.sh | 60 ++++++++++++++++++++++++++++++++++++++\n> 4 files changed, 116 insertions(+), 8 deletions(-)\n"},{"id":"498574","messageId":"3776d430faaee9b68d488cb11252d6ed@manjaro.org","threadId":"61768","inReplyTo":"5effca4d-536c-4e51-a024-5f1e90583176@gmail.com","subject":"Re: [PATCH 4/4] add-patch: render hunks through the pager","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-07-12T08:58:42Z","receivedAt":"2024-07-12T08:58:44Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-07-12 03:00, Rubén Justo wrote:\n> @@ -1391,7 +1393,7 @@ N_(\"j - leave this hunk undecided, see next\n> undecided hunk\\n\"\n> \"/ - search for a hunk matching the given regex\\n\"\n> \"s - split the current hunk into smaller hunks\\n\"\n> \"e - manually edit the current hunk\\n\"\n> - \"p - print the current hunk\\n\"\n> + \"p - print the current hunk, 'P' to use the pager\\n\"\n> \"? - print help\\n\");\n\nI'm fine with this compact form, even though it diverges from\nthe \"one command per help line\" rule.\n"},{"id":"498576","messageId":"8434fafe-f545-49bc-8cc1-d4e8fb634bec@gmail.com","threadId":"61768","inReplyTo":"4b5d0e7c-9492-4495-9bc1-40ebea850fde@gmail.com","subject":"Re: [PATCH 3/4] pager: introduce wait_for_pager","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-12T13:17:08Z","receivedAt":"2024-07-12T13:17:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 12/07/2024 02:00, Rubén Justo wrote:\n> Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,\n> 2006-02-28) we have the machinery to send our output to a pager.\n> \n> That machinery, once set up, does not allow us to regain the original\n> stdio streams.\n> \n> In the interactive commands (i.e.: add -p) we want to use the pager for\n> some output, while maintaining the interaction with the user.\n> \n> Modify the pager machinery so that we can use setup_pager and, once\n> we've finished sending the desired output for the pager, wait for the\n> pager termination using a new function wait_for_pager. Make this\n> function reset the pager machinery before returning.\n\nThis looks good and addresses my previous comments about leaking file \ndescriptors and restoring signal dispositions. A couple of thoughts \noccurred to me as I was reading it again:\n\n - We ignore any errors when duplicating fds,\n \"git grep '[^a-z_]dup2\\{0,1\\}(' shows that's not unusual in our\n code base, though if we cannot redirect the output to the pager or\n restore stdout when the pager exits that's a problem for \"git add -p\"\n\n - We should perhaps be marking old_fd[12] with O_CLOEXEC to stop them\n being passed to the pager.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Rubén Justo \n> ---\n> pager.c | 43 +++++++++++++++++++++++++++++++++++++------\n> pager.h | 1 +\n> 2 files changed, 38 insertions(+), 6 deletions(-)\n> \n> diff --git a/pager.c b/pager.c\n> index 251adfc2ad..bea4345f6f 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -14,7 +14,7 @@ int pager_use_color = 1;\n> \n> static struct child_process pager_process;\n> static char *pager_program;\n> -static int close_fd2;\n> +static int old_fd1 = -1, old_fd2 = -1;\n> \n> /* Is the value coming back from term_columns() just a guess? */\n> static int term_columns_guessed;\n> @@ -24,11 +24,11 @@ static void close_pager_fds(void)\n> {\n> \t/* signal EOF to pager */\n> \tclose(1);\n> -\tif (close_fd2)\n> +\tif (old_fd2 != -1)\n> \t\tclose(2);\n> }\n> \n> -static void wait_for_pager_atexit(void)\n> +static void finish_pager(void)\n> {\n> \tfflush(stdout);\n> \tfflush(stderr);\n> @@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)\n> \tfinish_command(&pager_process);\n> }\n> \n> +static void wait_for_pager_atexit(void)\n> +{\n> +\tif (old_fd1 == -1)\n> +\t\treturn;\n> +\n> +\tfinish_pager();\n> +}\n> +\n> +void wait_for_pager(void)\n> +{\n> +\tfinish_pager();\n> +\tsigchain_pop_common();\n> +\tunsetenv(\"GIT_PAGER_IN_USE\");\n> +\tdup2(old_fd1, 1);\n> +\tclose(old_fd1);\n> +\told_fd1 = -1;\n> +\tif (old_fd2 != -1) {\n> +\t\tdup2(old_fd2, 2);\n> +\t\tclose(old_fd2);\n> +\t\told_fd2 = -1;\n> +\t}\n> +}\n> +\n> static void wait_for_pager_signal(int signo)\n> {\n> +\tif (old_fd1 == -1)\n> +\t\treturn;\n> +\n> \tclose_pager_fds();\n> \tfinish_command_in_signal(&pager_process);\n> \tsigchain_pop(signo);\n> @@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n> \n> void setup_pager(void)\n> {\n> +\tstatic int once = 0;\n> \tconst char *pager = git_pager(isatty(1));\n> \n> \tif (!pager)\n> @@ -142,16 +169,20 @@ void setup_pager(void)\n> \t\tdie(\"unable to execute pager '%s'\", pager);\n> \n> \t/* original process continues, but writes to the pipe */\n> +\told_fd1 = dup(1);\n> \tdup2(pager_process.in, 1);\n> \tif (isatty(2)) {\n> -\t\tclose_fd2 = 1;\n> +\t\told_fd2 = dup(2);\n> \t\tdup2(pager_process.in, 2);\n> \t}\n> \tclose(pager_process.in);\n> \n> -\t/* this makes sure that the parent terminates after the pager */\n> \tsigchain_push_common(wait_for_pager_signal);\n> -\tatexit(wait_for_pager_atexit);\n> +\n> +\tif (!once) {\n> +\t\tonce++;\n> +\t\tatexit(wait_for_pager_atexit);\n> +\t}\n> }\n> \n> int pager_in_use(void)\n> diff --git a/pager.h b/pager.h\n> index b77433026d..103ecac476 100644\n> --- a/pager.h\n> +++ b/pager.h\n> @@ -5,6 +5,7 @@ struct child_process;\n> \n> const char *git_pager(int stdout_is_tty);\n> void setup_pager(void);\n> +void wait_for_pager(void);\n> int pager_in_use(void);\n> int term_columns(void);\n> void term_clear_line(void);\n\n"},{"id":"498577","messageId":"803b10ed-1cb3-4314-82c9-cf48d5d0bb90@gmail.com","threadId":"61768","inReplyTo":"5effca4d-536c-4e51-a024-5f1e90583176@gmail.com","subject":"Re: [PATCH 4/4] add-patch: render hunks through the pager","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-12T13:26:22Z","receivedAt":"2024-07-12T13:26:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 12/07/2024 02:00, Rubén Justo wrote:\n> Make the print command to trigger the pager when invoked using a capital\n\ns/to//\n\n> 'P', to make it easier for the user to review long hunks.\n> \n> Signed-off-by: Rubén Justo \n\nThanks for working on this. The code changes all look good, I'm a bit \nconfused by this test though\n\n> +test_expect_success TTY 'P does not break if pager ends unexpectly' '\n> +\ttest_when_finished \"rm -f huge_file; git reset\" &&\n> +\tprintf \"%2500000s\" Y >huge_file &&\n> +\tgit add -N huge_file &&\n> +\tcat >expect <<-EOF &&\n> +\t+22\n> +\t+23\n> +\t+24\n> +\t 30\n> +\t 40\n> +\t 50\n> +\t(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? \n> +\tEOF\n> +\ttest_write_lines P |\n> +\t(\n> +\t\tGIT_PAGER=\"head -1\" &&\n> +\t\texport GIT_PAGER &&\n> +\t\ttest_terminal git add -p >actual\n> +\t) &&\n> +\ttail -n 7 actual.trimmed &&\n> +\ttest_cmp expect actual.trimmed\n> +'\n\nWhat is huge_file doing and what happens to the single line of pager output?\n\nThanks\n\nPhillip\n\n> test_expect_success 'split hunk \"add -p (edit)\"' '\n> \t# Split, say Edit and do nothing. Then:\n> \t#\n\n"},{"id":"498588","messageId":"ba8ad59d-d125-41d9-a482-ee8eda187762@gmail.com","threadId":"61768","inReplyTo":"803b10ed-1cb3-4314-82c9-cf48d5d0bb90@gmail.com","subject":"Re: [PATCH 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-12T16:24:32Z","receivedAt":"2024-07-12T16:24:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Jul 12, 2024 at 02:26:22PM +0100, Phillip Wood wrote:\n\n> > +test_expect_success TTY 'P does not break if pager ends unexpectly' '\n> > +\ttest_when_finished \"rm -f huge_file; git reset\" &&\n> > +\tprintf \"%2500000s\" Y >huge_file &&\n> > +\tgit add -N huge_file &&\n> > +\tcat >expect <<-EOF &&\n> > +\t+22\n> > +\t+23\n> > +\t+24\n> > +\t 30\n> > +\t 40\n> > +\t 50\n> > +\t(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? \n> > +\tEOF\n> > +\ttest_write_lines P |\n> > +\t(\n> > +\t\tGIT_PAGER=\"head -1\" &&\n> > +\t\texport GIT_PAGER &&\n> > +\t\ttest_terminal git add -p >actual\n> > +\t) &&\n> > +\ttail -n 7 actual.trimmed &&\n> > +\ttest_cmp expect actual.trimmed\n> > +'\n> \n> What is huge_file doing and what happens to the single line of pager output?\n\nThe huge file is to make sure we are receiving a SIGPIPE. We don't\nreally care about the line \"head -1\" produces, only that we don't\nbreak due to the SIGPIPE that occurs.\n\nMaybe a test like this would be clearer?\n\ntest_expect_success TTY 'P does not break if pager ends unexpectedly' '\n\ttest_when_finished \"rm -f huge_file; git reset\" &&\n\tprintf \"%2500000s\\nfrotz\\n\" Y >huge_file &&\n\tgit add -N huge_file &&\n\tcat >expect <<-EOF &&\n\t+frotz\n\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? @@ -0,0 +1,2 @@\n\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? \n\tEOF\n\ttest_write_lines P q |\n\t(\n\t\tGIT_PAGER=\"head -1\" &&\n\t\texport GIT_PAGER &&\n\t\ttest_terminal git add -p >actual\n\t) &&\n\ttail -n 3 actual.trimmed &&\n\ttest_cmp expect actual.trimmed\n'\n\n> \n> Thanks\n\nThank you.\n"},{"id":"498607","messageId":"91941519-04c0-4306-bc5b-4fa1283d2de6@gmail.com","threadId":"61768","inReplyTo":"ba8ad59d-d125-41d9-a482-ee8eda187762@gmail.com","subject":"Re: [PATCH 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-13T03:23:16Z","receivedAt":"2024-07-13T03:23:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 13/7/24 1:24, Rubén Justo wrote:\n \n> Maybe a test like this would be clearer?\n\ntest_expect_success TTY 'P does not break if pager ends unexpectedly' '\n test_when_finished \"rm -f huge_file; git reset\" &&\n printf \"%2500000s\" Y >huge_file &&\n git add -N huge_file &&\n test_write_lines P q | GIT_PAGER=\"head -c 1\" test_terminal git add -p\n'\n"},{"id":"498608","messageId":"xmqqsewdd5a1.fsf@gitster.g","threadId":"61768","inReplyTo":"ba8ad59d-d125-41d9-a482-ee8eda187762@gmail.com","subject":"Re: [PATCH 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-13T09:12:38Z","receivedAt":"2024-07-13T09:12:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n>> What is huge_file doing and what happens to the single line of pager output?\n>\n> The huge file is to make sure we are receiving a SIGPIPE. We don't\n> really care about the line \"head -1\" produces, only that we don't\n> break due to the SIGPIPE that occurs.\n\nThat deserves to be explained in the proposed log message, I think.\n\nThanks.\n\n"},{"id":"498611","messageId":"9a2feb36-f8c4-4ea6-91a6-a3a24f359a6e@gmail.com","threadId":"61768","inReplyTo":"ba8ad59d-d125-41d9-a482-ee8eda187762@gmail.com","subject":"Re: [PATCH 4/4] add-patch: render hunks through the pager","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-13T13:17:49Z","receivedAt":"2024-07-13T13:17:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/07/2024 17:24, Rubén Justo wrote:\n> On Fri, Jul 12, 2024 at 02:26:22PM +0100, Phillip Wood wrote:\n> \n>>> +test_expect_success TTY 'P does not break if pager ends unexpectly' '\n>>> +\ttest_when_finished \"rm -f huge_file; git reset\" &&\n>>> +\tprintf \"%2500000s\" Y >huge_file &&\n>>> +\tgit add -N huge_file &&\n>>> +\tcat >expect <<-EOF &&\n>>> +\t+22\n>>> +\t+23\n>>> +\t+24\n>>> +\t 30\n>>> +\t 40\n>>> +\t 50\n>>> +\t(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? \n>>> +\tEOF\n>>> +\ttest_write_lines P |\n>>> +\t(\n>>> +\t\tGIT_PAGER=\"head -1\" &&\n>>> +\t\texport GIT_PAGER &&\n>>> +\t\ttest_terminal git add -p >actual\n>>> +\t) &&\n>>> +\ttail -n 7 actual.trimmed &&\n>>> +\ttest_cmp expect actual.trimmed\n>>> +'\n>>\n>> What is huge_file doing and what happens to the single line of pager output?\n> \n> The huge file is to make sure we are receiving a SIGPIPE. We don't\n> really care about the line \"head -1\" produces, only that we don't\n> break due to the SIGPIPE that occurs.\n\nAs Junio said it would help to explain that. I'm still confused why we \ndon't see any output from the pager - shouldn't the pager print the hunk \nheader as it does in the example below?\n\n> Maybe a test like this would be clearer?\n\nI think explaining in the commit message would be best.\n\nThanks\n\nPhillip\n\n> test_expect_success TTY 'P does not break if pager ends unexpectedly' '\n> \ttest_when_finished \"rm -f huge_file; git reset\" &&\n> \tprintf \"%2500000s\\nfrotz\\n\" Y >huge_file &&\n> \tgit add -N huge_file &&\n> \tcat >expect <<-EOF &&\n> \t+frotz\n> \t(1/1) Stage addition [y,n,q,a,d,e,p,?]? @@ -0,0 +1,2 @@\n> \t(1/1) Stage addition [y,n,q,a,d,e,p,?]? \n> \tEOF\n> \ttest_write_lines P q |\n> \t(\n> \t\tGIT_PAGER=\"head -1\" &&\n> \t\texport GIT_PAGER &&\n> \t\ttest_terminal git add -p >actual\n> \t) &&\n> \ttail -n 3 actual.trimmed &&\n> \ttest_cmp expect actual.trimmed\n> '\n> \n>>\n>> Thanks\n> \n> Thank you.\n"},{"id":"498629","messageId":"ebcba08f-3fbb-4130-93eb-d0e62bfe0a8a@gmail.com","threadId":"61768","inReplyTo":"2653fb37-c8a8-49b1-a804-4be6654a2cad@gmail.com","subject":"[PATCH v2 0/4] use the pager in 'add -p'","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-13T16:26:00Z","receivedAt":"2024-07-13T16:26:04Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Rubén Justo (4):\n add-patch: test for 'p' command\n pager: do not close fd 2 unnecessarily\n pager: introduce wait_for_pager\n add-patch: render hunks through the pager\n\n add-patch.c | 18 ++++++++++++---\n pager.c | 45 +++++++++++++++++++++++++++++++++-----\n pager.h | 1 +\n t/t3701-add-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 100 insertions(+), 8 deletions(-)\n\nRange-diff against v1:\n1: 4a5b6e6815 = 1: 6b37507ddd add-patch: test for 'p' command\n2: bf8a68ac37 = 2: 5497fa020b pager: do not close fd 2 unnecessarily\n3: aabd7da4d6 = 3: 30e772cf7c pager: introduce wait_for_pager\n4: ff51cc32bd ! 4: f7cb00b654 add-patch: render hunks through the pager\n @@ Metadata\n ## Commit message ##\n add-patch: render hunks through the pager\n \n - Make the print command to trigger the pager when invoked using a capital\n + Make the print command trigger the pager when invoked using a capital\n 'P', to make it easier for the user to review long hunks.\n \n + Note that if the PAGER ends unexpectedly before we've been able to send\n + the payload, perhaps because the user is not interested in the whole\n + thing, we might receive a SIGPIPE, which would abruptly and unexpectedly\n + terminate the interactive session for the user.\n +\n + Therefore, we need to ignore a possible SIGPIPE signal. Add a test for\n + this, in addition to the test for normal operation.\n +\n + For the SIGPIPE test, we need to make sure that we completely fill the\n + operating system's buffer, otherwise we might not trigger the SIGPIPE\n + signal. The normal size of this buffer in different OSs varies from a\n + few KBs to 1MB. Use a payload large enough to guarantee that we exceed\n + this limit.\n +\n Signed-off-by: Rubén Justo \n \n ## add-patch.c ##\n @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '\n +\ttest_cmp expect actual.trimmed\n +'\n +\n -+test_expect_success TTY 'P does not break if pager ends unexpectly' '\n ++test_expect_success TTY 'P does not break if pager ends unexpectedly' '\n +\ttest_when_finished \"rm -f huge_file; git reset\" &&\n +\tprintf \"%2500000s\" Y >huge_file &&\n +\tgit add -N huge_file &&\n -+\tcat >expect <<-EOF &&\n -+\t+22\n -+\t+23\n -+\t+24\n -+\t 30\n -+\t 40\n -+\t 50\n -+\t(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? \n -+\tEOF\n -+\ttest_write_lines P |\n -+\t(\n -+\t\tGIT_PAGER=\"head -1\" &&\n -+\t\texport GIT_PAGER &&\n -+\t\ttest_terminal git add -p >actual\n -+\t) &&\n -+\ttail -n 7 actual.trimmed &&\n -+\ttest_cmp expect actual.trimmed\n ++\ttest_write_lines P q | GIT_PAGER=\"head -c 1\" test_terminal git add -p >actual\n +'\n +\n test_expect_success 'split hunk \"add -p (edit)\"' '\n-- \n2.45.2.831.g9e4974e3d4\n"},{"id":"498631","messageId":"429f8fda-8f6b-4cef-ab79-27e5d7b56fac@gmail.com","threadId":"61768","inReplyTo":"ebcba08f-3fbb-4130-93eb-d0e62bfe0a8a@gmail.com","subject":"[PATCH v2 1/4] add-patch: test for 'p' command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-13T16:29:00Z","receivedAt":"2024-07-13T16:29:03Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Add a test for the 'p' command, which was introduced in 66c14ab592\n(add-patch: introduce 'p' in interactive-patch, 2024-03-29).\n\nSigned-off-by: Rubén Justo \n---\n t/t3701-add-interactive.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 5d78868ac1..6daf3a6be0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -575,6 +575,22 @@ test_expect_success 'navigate to hunk via regex / pattern' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success 'print again the hunk' '\n+\ttest_when_finished \"git reset\" &&\n+\ttr _ \" \" >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n+\t 10\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\tEOF\n+\ttest_write_lines s y g 1 p | git add -p >actual &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.45.2.831.g9e4974e3d4\n"},{"id":"498632","messageId":"fe7c9555-4147-4649-97de-53ef90b552cb@gmail.com","threadId":"61768","inReplyTo":"ebcba08f-3fbb-4130-93eb-d0e62bfe0a8a@gmail.com","subject":"[PATCH v2 2/4] pager: do not close fd 2 unnecessarily","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-13T16:29:16Z","receivedAt":"2024-07-13T16:29:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We send errors to the pager since 61b80509e3 (sending errors to stdout\nunder $PAGER, 2008-02-16).\n\nIn a8335024c2 (pager: do not dup2 stderr if it is already redirected,\n2008-12-15) an exception was introduced to avoid redirecting stderr if\nit is not connected to a terminal.\n\nIn such exceptional cases, the close(STDERR_FILENO) we're doing in\nclose_pager_fds, is unnecessary.\n\nFurthermore, in a subsequent commit we're going to introduce changes\nthat will involve using close_pager_fds multiple times.\n\nWith this in mind, controlling when we want to close stderr, become\nsensible.\n\nLet's close(STDERR_FILENO) only when necessary, and pave the way for the\nupcoming changes.\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex be6f4ee59f..251adfc2ad 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,6 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n+static int close_fd2;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -23,7 +24,8 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tclose(2);\n+\tif (close_fd2)\n+\t\tclose(2);\n }\n \n static void wait_for_pager_atexit(void)\n@@ -141,8 +143,10 @@ void setup_pager(void)\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\n-\tif (isatty(2))\n+\tif (isatty(2)) {\n+\t\tclose_fd2 = 1;\n \t\tdup2(pager_process.in, 2);\n+\t}\n \tclose(pager_process.in);\n \n \t/* this makes sure that the parent terminates after the pager */\n-- \n2.45.2.831.g9e4974e3d4\n"},{"id":"498633","messageId":"205b0e27-7507-4a95-b239-818bd018c846@gmail.com","threadId":"61768","inReplyTo":"ebcba08f-3fbb-4130-93eb-d0e62bfe0a8a@gmail.com","subject":"[PATCH v2 3/4] pager: introduce wait_for_pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-13T16:29:41Z","receivedAt":"2024-07-13T16:29:44Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,\n2006-02-28) we have the machinery to send our output to a pager.\n\nThat machinery, once set up, does not allow us to regain the original\nstdio streams.\n\nIn the interactive commands (i.e.: add -p) we want to use the pager for\nsome output, while maintaining the interaction with the user.\n\nModify the pager machinery so that we can use setup_pager and, once\nwe've finished sending the desired output for the pager, wait for the\npager termination using a new function wait_for_pager. Make this\nfunction reset the pager machinery before returning.\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 43 +++++++++++++++++++++++++++++++++++++------\n pager.h | 1 +\n 2 files changed, 38 insertions(+), 6 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 251adfc2ad..bea4345f6f 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,7 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n-static int close_fd2;\n+static int old_fd1 = -1, old_fd2 = -1;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -24,11 +24,11 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tif (close_fd2)\n+\tif (old_fd2 != -1)\n \t\tclose(2);\n }\n \n-static void wait_for_pager_atexit(void)\n+static void finish_pager(void)\n {\n \tfflush(stdout);\n \tfflush(stderr);\n@@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)\n \tfinish_command(&pager_process);\n }\n \n+static void wait_for_pager_atexit(void)\n+{\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n+\tfinish_pager();\n+}\n+\n+void wait_for_pager(void)\n+{\n+\tfinish_pager();\n+\tsigchain_pop_common();\n+\tunsetenv(\"GIT_PAGER_IN_USE\");\n+\tdup2(old_fd1, 1);\n+\tclose(old_fd1);\n+\told_fd1 = -1;\n+\tif (old_fd2 != -1) {\n+\t\tdup2(old_fd2, 2);\n+\t\tclose(old_fd2);\n+\t\told_fd2 = -1;\n+\t}\n+}\n+\n static void wait_for_pager_signal(int signo)\n {\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n \tclose_pager_fds();\n \tfinish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n@@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n \n void setup_pager(void)\n {\n+\tstatic int once = 0;\n \tconst char *pager = git_pager(isatty(1));\n \n \tif (!pager)\n@@ -142,16 +169,20 @@ void setup_pager(void)\n \t\tdie(\"unable to execute pager '%s'\", pager);\n \n \t/* original process continues, but writes to the pipe */\n+\told_fd1 = dup(1);\n \tdup2(pager_process.in, 1);\n \tif (isatty(2)) {\n-\t\tclose_fd2 = 1;\n+\t\told_fd2 = dup(2);\n \t\tdup2(pager_process.in, 2);\n \t}\n \tclose(pager_process.in);\n \n-\t/* this makes sure that the parent terminates after the pager */\n \tsigchain_push_common(wait_for_pager_signal);\n-\tatexit(wait_for_pager_atexit);\n+\n+\tif (!once) {\n+\t\tonce++;\n+\t\tatexit(wait_for_pager_atexit);\n+\t}\n }\n \n int pager_in_use(void)\ndiff --git a/pager.h b/pager.h\nindex b77433026d..103ecac476 100644\n--- a/pager.h\n+++ b/pager.h\n@@ -5,6 +5,7 @@ struct child_process;\n \n const char *git_pager(int stdout_is_tty);\n void setup_pager(void);\n+void wait_for_pager(void);\n int pager_in_use(void);\n int term_columns(void);\n void term_clear_line(void);\n-- \n2.45.2.831.g9e4974e3d4\n"},{"id":"498634","messageId":"4556095f-47d7-4849-b6d7-a08cd00ad865@gmail.com","threadId":"61768","inReplyTo":"ebcba08f-3fbb-4130-93eb-d0e62bfe0a8a@gmail.com","subject":"[PATCH v2 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-13T16:30:03Z","receivedAt":"2024-07-13T16:30:07Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Make the print command trigger the pager when invoked using a capital\n'P', to make it easier for the user to review long hunks.\n\nNote that if the PAGER ends unexpectedly before we've been able to send\nthe payload, perhaps because the user is not interested in the whole\nthing, we might receive a SIGPIPE, which would abruptly and unexpectedly\nterminate the interactive session for the user.\n\nTherefore, we need to ignore a possible SIGPIPE signal. Add a test for\nthis, in addition to the test for normal operation.\n\nFor the SIGPIPE test, we need to make sure that we completely fill the\noperating system's buffer, otherwise we might not trigger the SIGPIPE\nsignal. The normal size of this buffer in different OSs varies from a\nfew KBs to 1MB. Use a payload large enough to guarantee that we exceed\nthis limit.\n\nSigned-off-by: Rubén Justo \n---\n add-patch.c | 18 +++++++++++++++---\n t/t3701-add-interactive.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 6e176cd21a..f2c76b7d83 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -7,9 +7,11 @@\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"object-name.h\"\n+#include \"pager.h\"\n #include \"read-cache-ll.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n+#include \"sigchain.h\"\n #include \"run-command.h\"\n #include \"strvec.h\"\n #include \"pathspec.h\"\n@@ -1391,7 +1393,7 @@ N_(\"j - leave this hunk undecided, see next undecided hunk\\n\"\n \"/ - search for a hunk matching the given regex\\n\"\n \"s - split the current hunk into smaller hunks\\n\"\n \"e - manually edit the current hunk\\n\"\n- \"p - print the current hunk\\n\"\n+ \"p - print the current hunk, 'P' to use the pager\\n\"\n \"? - print help\\n\");\n \n static int patch_update_file(struct add_p_state *s,\n@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,\n \tstruct hunk *hunk;\n \tchar ch;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tint colored = !!s->colored.len, quit = 0;\n+\tint colored = !!s->colored.len, quit = 0, use_pager = 0;\n \tenum prompt_mode_type prompt_mode_type;\n \tenum {\n \t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,\n \t\tstrbuf_reset(&s->buf);\n \t\tif (file_diff->hunk_nr) {\n \t\t\tif (rendered_hunk_index != hunk_index) {\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsetup_pager();\n+\t\t\t\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\t\t\t}\n \t\t\t\trender_hunk(s, hunk, 0, colored, &s->buf);\n \t\t\t\tfputs(s->buf.buf, stdout);\n \t\t\t\trendered_hunk_index = hunk_index;\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsigchain_pop(SIGPIPE);\n+\t\t\t\t\twait_for_pager();\n+\t\t\t\t\tuse_pager = 0;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tstrbuf_reset(&s->buf);\n@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\thunk->use = USE_HUNK;\n \t\t\t\tgoto soft_increment;\n \t\t\t}\n-\t\t} else if (s->answer.buf[0] == 'p') {\n+\t\t} else if (ch == 'p') {\n \t\t\trendered_hunk_index = -1;\n+\t\t\tuse_pager = (s->answer.buf[0] == 'P') ? 1 : 0;\n \t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 6daf3a6be0..c89b984751 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -591,6 +591,34 @@ test_expect_success 'print again the hunk' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success TTY 'print again the hunk (PAGER)' '\n+\ttest_when_finished \"git reset\" &&\n+\tcat >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? PAGER @@ -1,2 +1,3 @@\n+\tPAGER 10\n+\tPAGER +15\n+\tPAGER 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? \n+\tEOF\n+\ttest_write_lines s y g 1 P |\n+\t(\n+\t\tGIT_PAGER=\"sed s/^/PAGER\\ /\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p >actual\n+\t) &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n+test_expect_success TTY 'P does not break if pager ends unexpectedly' '\n+\ttest_when_finished \"rm -f huge_file; git reset\" &&\n+\tprintf \"%2500000s\" Y >huge_file &&\n+\tgit add -N huge_file &&\n+\ttest_write_lines P q | GIT_PAGER=\"head -c 1\" test_terminal git add -p >actual\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.45.2.831.g9e4974e3d4\n"},{"id":"498640","messageId":"xmqq7cdpb4op.fsf@gitster.g","threadId":"61768","inReplyTo":"ebcba08f-3fbb-4130-93eb-d0e62bfe0a8a@gmail.com","subject":"Re: [PATCH v2 0/4] use the pager in 'add -p'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-13T17:08:22Z","receivedAt":"2024-07-13T17:08:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> -+\ttest_write_lines P |\n> -+\t(\n> -+\t\tGIT_PAGER=\"head -1\" &&\n> -+\t\texport GIT_PAGER &&\n> -+\t\ttest_terminal git add -p >actual\n> -+\t) &&\n> -+\ttail -n 7 actual.trimmed &&\n> -+\ttest_cmp expect actual.trimmed\n> ++\ttest_write_lines P q | GIT_PAGER=\"head -c 1\" test_terminal git add -p >actual\n> +'\n\n\"make test\" has this to say:\n\nt3701-add-interactive.sh:619: error: head -c is not portable (use test_copy_bytes BYTES out): test_write_lines P q | GIT_PAGER=\"head -c 1\" test_terminal git add -p >actual\ngmake[1]: *** [Makefile:132: test-lint-shell-syntax] Error 1\n"},{"id":"498651","messageId":"2ebcbe5e-a673-40ae-8326-65280dd6b18f@gmail.com","threadId":"61768","inReplyTo":"9a2feb36-f8c4-4ea6-91a6-a3a24f359a6e@gmail.com","subject":"Re: [PATCH 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-13T23:13:13Z","receivedAt":"2024-07-13T23:13:17Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sat, Jul 13, 2024 at 02:17:49PM +0100, phillip.wood123@gmail.com wrote:\n> On 12/07/2024 17:24, Rubén Justo wrote:\n> > On Fri, Jul 12, 2024 at 02:26:22PM +0100, Phillip Wood wrote:\n> > \n> > > > +test_expect_success TTY 'P does not break if pager ends unexpectly' '\n> > > > +\ttest_when_finished \"rm -f huge_file; git reset\" &&\n> > > > +\tprintf \"%2500000s\" Y >huge_file &&\n> > > > +\tgit add -N huge_file &&\n> > > > +\tcat >expect <<-EOF &&\n> > > > +\t+22\n> > > > +\t+23\n> > > > +\t+24\n> > > > +\t 30\n> > > > +\t 40\n> > > > +\t 50\n> > > > +\t(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? \n> > > > +\tEOF\n> > > > +\ttest_write_lines P |\n> > > > +\t(\n> > > > +\t\tGIT_PAGER=\"head -1\" &&\n> > > > +\t\texport GIT_PAGER &&\n> > > > +\t\ttest_terminal git add -p >actual\n> > > > +\t) &&\n> > > > +\ttail -n 7 actual.trimmed &&\n> > > > +\ttest_cmp expect actual.trimmed\n> > > > +'\n> > > \n> > > What is huge_file doing and what happens to the single line of pager output?\n> > \n> [...]\n> \n> I'm still confused why we don't\n> see any output from the pager - shouldn't the pager print the hunk header as\n> it does in the example below?\n\nIn the test above, we would need to use \"tail -n 18\" to capture the\nheader of the hunk.\n\nIn the test below, the \"q\" allows \"tail -n 3\" to capture it. \n\n> > test_expect_success TTY 'P does not break if pager ends unexpectedly' '\n> > \ttest_when_finished \"rm -f huge_file; git reset\" &&\n> > \tprintf \"%2500000s\\nfrotz\\n\" Y >huge_file &&\n> > \tgit add -N huge_file &&\n> > \tcat >expect <<-EOF &&\n> > \t+frotz\n> > \t(1/1) Stage addition [y,n,q,a,d,e,p,?]? @@ -0,0 +1,2 @@\n> > \t(1/1) Stage addition [y,n,q,a,d,e,p,?]? \n> > \tEOF\n> > \ttest_write_lines P q |\n> > \t(\n> > \t\tGIT_PAGER=\"head -1\" &&\n> > \t\texport GIT_PAGER &&\n> > \t\ttest_terminal git add -p >actual\n> > \t) &&\n> > \ttail -n 3 actual.trimmed &&\n> > \ttest_cmp expect actual.trimmed\n> > '\n\nI wonder if the following would be desirable: \n\n--- a/add-patch.c\n+++ b/add-patch.c\n@@@ -1519,8 -1519,8 +1519,10 @@@ static int patch_update_file(struct add\n \t\tif (*s->s.reset_color)\n \t\t\tfputs(s->s.reset_color, stdout);\n \t\tfflush(stdout);\n--\t\tif (read_single_character(s) == EOF)\n++\t\tif (read_single_character(s) == EOF) {\n++\t\t\tquit = 1;\n \t\t\tbreak;\n++\t\t}\n \n \t\tif (!s->answer.len)\n \t\t\tcontinue;\n"},{"id":"498652","messageId":"ec13ed8a-3ad8-45d0-9120-2f5ddabc14fc@gmail.com","threadId":"61768","inReplyTo":"xmqq7cdpb4op.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] use the pager in 'add -p'","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-13T23:21:28Z","receivedAt":"2024-07-13T23:21:31Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sat, Jul 13, 2024 at 10:08:22AM -0700, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > -+\ttest_write_lines P |\n> > -+\t(\n> > -+\t\tGIT_PAGER=\"head -1\" &&\n> > -+\t\texport GIT_PAGER &&\n> > -+\t\ttest_terminal git add -p >actual\n> > -+\t) &&\n> > -+\ttail -n 7 actual.trimmed &&\n> > -+\ttest_cmp expect actual.trimmed\n> > ++\ttest_write_lines P q | GIT_PAGER=\"head -c 1\" test_terminal git add -p >actual\n> > +'\n> \n> \"make test\" has this to say:\n> \n> t3701-add-interactive.sh:619: error: head -c is not portable (use test_copy_bytes BYTES out): test_write_lines P q | GIT_PAGER=\"head -c 1\" test_terminal git add -p >actual\n> gmake[1]: *** [Makefile:132: test-lint-shell-syntax] Error 1\n\nOuch. I'll fix it. I think I'll go back to \"head -1\". But I'll wait to\nhear comments about the change in the message.\n"},{"id":"498654","messageId":"xmqq34ocbwji.fsf@gitster.g","threadId":"61768","inReplyTo":"ec13ed8a-3ad8-45d0-9120-2f5ddabc14fc@gmail.com","subject":"Re: [PATCH v2 0/4] use the pager in 'add -p'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-14T01:18:57Z","receivedAt":"2024-07-14T01:19:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Ouch. I'll fix it. I think I'll go back to \"head -1\".\n\nI think \"head -\" is deprecated, too. \n\nSay \"head -n 1\" probably, if you really wanted to take the first\nline and quit.\n\n\n"},{"id":"498678","messageId":"efa98aec-f117-4cfe-a7c2-e8c0adbdb399@gmail.com","threadId":"61768","inReplyTo":"ebcba08f-3fbb-4130-93eb-d0e62bfe0a8a@gmail.com","subject":"[PATCH v3 0/4] use the pager in 'add -p'","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-14T16:00:28Z","receivedAt":"2024-07-14T16:00:33Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"This iterations fixes this error:\n\nt3701-add-interactive.sh:619: error: head -c is not portable (use test_copy_bytes BYTES out): test_write_lines P q | GIT_PAGER=\"head -c 1\" test_terminal git add -p >actual \ngmake[1]: *** [Makefile:132: test-lint-shell-syntax] Error 1 \n\nRubén Justo (4):\n add-patch: test for 'p' command\n pager: do not close fd 2 unnecessarily\n pager: introduce wait_for_pager\n add-patch: render hunks through the pager\n\n add-patch.c | 18 ++++++++++++---\n pager.c | 45 +++++++++++++++++++++++++++++++++-----\n pager.h | 1 +\n t/t3701-add-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 100 insertions(+), 8 deletions(-)\n\nRange-diff against v3:\n-: ---------- > 1: 6b37507ddd add-patch: test for 'p' command\n-: ---------- > 2: 5497fa020b pager: do not close fd 2 unnecessarily\n-: ---------- > 3: 30e772cf7c pager: introduce wait_for_pager\n1: f7cb00b654 ! 4: 913e7f3d09 add-patch: render hunks through the pager\n @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '\n +\n +test_expect_success TTY 'P does not break if pager ends unexpectedly' '\n +\ttest_when_finished \"rm -f huge_file; git reset\" &&\n -+\tprintf \"%2500000s\" Y >huge_file &&\n ++\tprintf \"\\n%2500000s\" Y >huge_file &&\n +\tgit add -N huge_file &&\n -+\ttest_write_lines P q | GIT_PAGER=\"head -c 1\" test_terminal git add -p >actual\n ++\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p >actual\n +'\n +\n test_expect_success 'split hunk \"add -p (edit)\"' '\n-- \n2.46.0.rc0.4.g913e7f3d09\n"},{"id":"498679","messageId":"73173a13-a537-460a-bca1-501b11728a77@gmail.com","threadId":"61768","inReplyTo":"efa98aec-f117-4cfe-a7c2-e8c0adbdb399@gmail.com","subject":"[PATCH v3 1/4] add-patch: test for 'p' command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-14T16:04:00Z","receivedAt":"2024-07-14T16:04:04Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Add a test for the 'p' command, which was introduced in 66c14ab592\n(add-patch: introduce 'p' in interactive-patch, 2024-03-29).\n\nSigned-off-by: Rubén Justo \n---\n t/t3701-add-interactive.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 5d78868ac1..6daf3a6be0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -575,6 +575,22 @@ test_expect_success 'navigate to hunk via regex / pattern' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success 'print again the hunk' '\n+\ttest_when_finished \"git reset\" &&\n+\ttr _ \" \" >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n+\t 10\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\tEOF\n+\ttest_write_lines s y g 1 p | git add -p >actual &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.46.0.rc0.4.g913e7f3d09\n"},{"id":"498680","messageId":"6649904d-d537-475c-b905-6ec227eb4eb4@gmail.com","threadId":"61768","inReplyTo":"efa98aec-f117-4cfe-a7c2-e8c0adbdb399@gmail.com","subject":"[PATCH v3 2/4] pager: do not close fd 2 unnecessarily","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-14T16:04:10Z","receivedAt":"2024-07-14T16:04:14Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We send errors to the pager since 61b80509e3 (sending errors to stdout\nunder $PAGER, 2008-02-16).\n\nIn a8335024c2 (pager: do not dup2 stderr if it is already redirected,\n2008-12-15) an exception was introduced to avoid redirecting stderr if\nit is not connected to a terminal.\n\nIn such exceptional cases, the close(STDERR_FILENO) we're doing in\nclose_pager_fds, is unnecessary.\n\nFurthermore, in a subsequent commit we're going to introduce changes\nthat will involve using close_pager_fds multiple times.\n\nWith this in mind, controlling when we want to close stderr, become\nsensible.\n\nLet's close(STDERR_FILENO) only when necessary, and pave the way for the\nupcoming changes.\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex be6f4ee59f..251adfc2ad 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,6 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n+static int close_fd2;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -23,7 +24,8 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tclose(2);\n+\tif (close_fd2)\n+\t\tclose(2);\n }\n \n static void wait_for_pager_atexit(void)\n@@ -141,8 +143,10 @@ void setup_pager(void)\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\n-\tif (isatty(2))\n+\tif (isatty(2)) {\n+\t\tclose_fd2 = 1;\n \t\tdup2(pager_process.in, 2);\n+\t}\n \tclose(pager_process.in);\n \n \t/* this makes sure that the parent terminates after the pager */\n-- \n2.46.0.rc0.4.g913e7f3d09\n"},{"id":"498681","messageId":"1dc9ebad-768b-4c1a-8a58-8a7a5d24d49e@gmail.com","threadId":"61768","inReplyTo":"efa98aec-f117-4cfe-a7c2-e8c0adbdb399@gmail.com","subject":"[PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-14T16:04:18Z","receivedAt":"2024-07-14T16:04:23Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Make the print command trigger the pager when invoked using a capital\n'P', to make it easier for the user to review long hunks.\n\nNote that if the PAGER ends unexpectedly before we've been able to send\nthe payload, perhaps because the user is not interested in the whole\nthing, we might receive a SIGPIPE, which would abruptly and unexpectedly\nterminate the interactive session for the user.\n\nTherefore, we need to ignore a possible SIGPIPE signal. Add a test for\nthis, in addition to the test for normal operation.\n\nFor the SIGPIPE test, we need to make sure that we completely fill the\noperating system's buffer, otherwise we might not trigger the SIGPIPE\nsignal. The normal size of this buffer in different OSs varies from a\nfew KBs to 1MB. Use a payload large enough to guarantee that we exceed\nthis limit.\n\nSigned-off-by: Rubén Justo \n---\n add-patch.c | 18 +++++++++++++++---\n t/t3701-add-interactive.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 6e176cd21a..f2c76b7d83 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -7,9 +7,11 @@\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"object-name.h\"\n+#include \"pager.h\"\n #include \"read-cache-ll.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n+#include \"sigchain.h\"\n #include \"run-command.h\"\n #include \"strvec.h\"\n #include \"pathspec.h\"\n@@ -1391,7 +1393,7 @@ N_(\"j - leave this hunk undecided, see next undecided hunk\\n\"\n \"/ - search for a hunk matching the given regex\\n\"\n \"s - split the current hunk into smaller hunks\\n\"\n \"e - manually edit the current hunk\\n\"\n- \"p - print the current hunk\\n\"\n+ \"p - print the current hunk, 'P' to use the pager\\n\"\n \"? - print help\\n\");\n \n static int patch_update_file(struct add_p_state *s,\n@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,\n \tstruct hunk *hunk;\n \tchar ch;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tint colored = !!s->colored.len, quit = 0;\n+\tint colored = !!s->colored.len, quit = 0, use_pager = 0;\n \tenum prompt_mode_type prompt_mode_type;\n \tenum {\n \t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,\n \t\tstrbuf_reset(&s->buf);\n \t\tif (file_diff->hunk_nr) {\n \t\t\tif (rendered_hunk_index != hunk_index) {\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsetup_pager();\n+\t\t\t\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\t\t\t}\n \t\t\t\trender_hunk(s, hunk, 0, colored, &s->buf);\n \t\t\t\tfputs(s->buf.buf, stdout);\n \t\t\t\trendered_hunk_index = hunk_index;\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsigchain_pop(SIGPIPE);\n+\t\t\t\t\twait_for_pager();\n+\t\t\t\t\tuse_pager = 0;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tstrbuf_reset(&s->buf);\n@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\thunk->use = USE_HUNK;\n \t\t\t\tgoto soft_increment;\n \t\t\t}\n-\t\t} else if (s->answer.buf[0] == 'p') {\n+\t\t} else if (ch == 'p') {\n \t\t\trendered_hunk_index = -1;\n+\t\t\tuse_pager = (s->answer.buf[0] == 'P') ? 1 : 0;\n \t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 6daf3a6be0..2ac860cc42 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -591,6 +591,34 @@ test_expect_success 'print again the hunk' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success TTY 'print again the hunk (PAGER)' '\n+\ttest_when_finished \"git reset\" &&\n+\tcat >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? PAGER @@ -1,2 +1,3 @@\n+\tPAGER 10\n+\tPAGER +15\n+\tPAGER 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? \n+\tEOF\n+\ttest_write_lines s y g 1 P |\n+\t(\n+\t\tGIT_PAGER=\"sed s/^/PAGER\\ /\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p >actual\n+\t) &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n+test_expect_success TTY 'P does not break if pager ends unexpectedly' '\n+\ttest_when_finished \"rm -f huge_file; git reset\" &&\n+\tprintf \"\\n%2500000s\" Y >huge_file &&\n+\tgit add -N huge_file &&\n+\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p >actual\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.46.0.rc0.4.g913e7f3d09\n"},{"id":"498682","messageId":"f48ac176-9938-4677-a956-350fb50dbc0f@gmail.com","threadId":"61768","inReplyTo":"efa98aec-f117-4cfe-a7c2-e8c0adbdb399@gmail.com","subject":"[PATCH v3 3/4] pager: introduce wait_for_pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-14T16:04:28Z","receivedAt":"2024-07-14T16:04:32Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,\n2006-02-28) we have the machinery to send our output to a pager.\n\nThat machinery, once set up, does not allow us to regain the original\nstdio streams.\n\nIn the interactive commands (i.e.: add -p) we want to use the pager for\nsome output, while maintaining the interaction with the user.\n\nModify the pager machinery so that we can use setup_pager and, once\nwe've finished sending the desired output for the pager, wait for the\npager termination using a new function wait_for_pager. Make this\nfunction reset the pager machinery before returning.\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 43 +++++++++++++++++++++++++++++++++++++------\n pager.h | 1 +\n 2 files changed, 38 insertions(+), 6 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 251adfc2ad..bea4345f6f 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,7 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n-static int close_fd2;\n+static int old_fd1 = -1, old_fd2 = -1;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -24,11 +24,11 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tif (close_fd2)\n+\tif (old_fd2 != -1)\n \t\tclose(2);\n }\n \n-static void wait_for_pager_atexit(void)\n+static void finish_pager(void)\n {\n \tfflush(stdout);\n \tfflush(stderr);\n@@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)\n \tfinish_command(&pager_process);\n }\n \n+static void wait_for_pager_atexit(void)\n+{\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n+\tfinish_pager();\n+}\n+\n+void wait_for_pager(void)\n+{\n+\tfinish_pager();\n+\tsigchain_pop_common();\n+\tunsetenv(\"GIT_PAGER_IN_USE\");\n+\tdup2(old_fd1, 1);\n+\tclose(old_fd1);\n+\told_fd1 = -1;\n+\tif (old_fd2 != -1) {\n+\t\tdup2(old_fd2, 2);\n+\t\tclose(old_fd2);\n+\t\told_fd2 = -1;\n+\t}\n+}\n+\n static void wait_for_pager_signal(int signo)\n {\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n \tclose_pager_fds();\n \tfinish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n@@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n \n void setup_pager(void)\n {\n+\tstatic int once = 0;\n \tconst char *pager = git_pager(isatty(1));\n \n \tif (!pager)\n@@ -142,16 +169,20 @@ void setup_pager(void)\n \t\tdie(\"unable to execute pager '%s'\", pager);\n \n \t/* original process continues, but writes to the pipe */\n+\told_fd1 = dup(1);\n \tdup2(pager_process.in, 1);\n \tif (isatty(2)) {\n-\t\tclose_fd2 = 1;\n+\t\told_fd2 = dup(2);\n \t\tdup2(pager_process.in, 2);\n \t}\n \tclose(pager_process.in);\n \n-\t/* this makes sure that the parent terminates after the pager */\n \tsigchain_push_common(wait_for_pager_signal);\n-\tatexit(wait_for_pager_atexit);\n+\n+\tif (!once) {\n+\t\tonce++;\n+\t\tatexit(wait_for_pager_atexit);\n+\t}\n }\n \n int pager_in_use(void)\ndiff --git a/pager.h b/pager.h\nindex b77433026d..103ecac476 100644\n--- a/pager.h\n+++ b/pager.h\n@@ -5,6 +5,7 @@ struct child_process;\n \n const char *git_pager(int stdout_is_tty);\n void setup_pager(void);\n+void wait_for_pager(void);\n int pager_in_use(void);\n int term_columns(void);\n void term_clear_line(void);\n-- \n2.46.0.rc0.4.g913e7f3d09\n"},{"id":"498722","messageId":"bb699ae5-deb8-4bd3-ab44-d66f401c7e17@gmail.com","threadId":"61768","inReplyTo":"1dc9ebad-768b-4c1a-8a58-8a7a5d24d49e@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-15T14:10:37Z","receivedAt":"2024-07-15T14:10:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 14/07/2024 17:04, Rubén Justo wrote:\n> Make the print command trigger the pager when invoked using a capital\n> 'P', to make it easier for the user to review long hunks.\n> \n> Note that if the PAGER ends unexpectedly before we've been able to send\n> the payload, perhaps because the user is not interested in the whole\n> thing, we might receive a SIGPIPE, which would abruptly and unexpectedly\n> terminate the interactive session for the user.\n> \n> Therefore, we need to ignore a possible SIGPIPE signal. Add a test for\n> this, in addition to the test for normal operation.\n> \n> For the SIGPIPE test, we need to make sure that we completely fill the\n> operating system's buffer, otherwise we might not trigger the SIGPIPE\n> signal. The normal size of this buffer in different OSs varies from a\n> few KBs to 1MB. Use a payload large enough to guarantee that we exceed\n> this limit.\n\nThanks for updating the commit message to explain the purpose of the \nSIGPIPE test\n\n> Signed-off-by: Rubén Justo \n> ---\n\n> +test_expect_success TTY 'P does not break if pager ends unexpectedly' '\n\nI think it would be helpful to mention SIGPIPE in the title as this test \nis really checking \"we don't die if we receive SIGPIPE\". Maybe\n\n P handles SIGPIPE when writing to pager\n\n> +\ttest_when_finished \"rm -f huge_file; git reset\" &&\n> +\tprintf \"\\n%2500000s\" Y >huge_file &&\n> +\tgit add -N huge_file &&\n> +\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p >actual\n\nIf we're not going to look at the output we don't need to redirect it. \nI'm not sure if there is any benefit to comparing the actual output to \nwhat we expect here.\n\nBest Wishes\n\nPhillip\n\n> +'\n> +\n> test_expect_success 'split hunk \"add -p (edit)\"' '\n> \t# Split, say Edit and do nothing. Then:\n> \t#\n"},{"id":"498723","messageId":"384f0147-d611-493b-a3d4-d83c65bd1114@gmail.com","threadId":"61768","inReplyTo":"f48ac176-9938-4677-a956-350fb50dbc0f@gmail.com","subject":"Re: [PATCH v3 3/4] pager: introduce wait_for_pager","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-15T14:13:09Z","receivedAt":"2024-07-15T14:13:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 14/07/2024 17:04, Rubén Justo wrote:\n> Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,\n> 2006-02-28) we have the machinery to send our output to a pager.\n> \n> That machinery, once set up, does not allow us to regain the original\n> stdio streams.\n> \n> In the interactive commands (i.e.: add -p) we want to use the pager for\n> some output, while maintaining the interaction with the user.\n> \n> Modify the pager machinery so that we can use setup_pager and, once\n> we've finished sending the desired output for the pager, wait for the\n> pager termination using a new function wait_for_pager. Make this\n> function reset the pager machinery before returning.\n\nDo you have any comments on my thoughts in \n<8434fafe-f545-49bc-8cc1-d4e8fb634bec@gmail.com> ?\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Rubén Justo \n> ---\n> pager.c | 43 +++++++++++++++++++++++++++++++++++++------\n> pager.h | 1 +\n> 2 files changed, 38 insertions(+), 6 deletions(-)\n> \n> diff --git a/pager.c b/pager.c\n> index 251adfc2ad..bea4345f6f 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -14,7 +14,7 @@ int pager_use_color = 1;\n> \n> static struct child_process pager_process;\n> static char *pager_program;\n> -static int close_fd2;\n> +static int old_fd1 = -1, old_fd2 = -1;\n> \n> /* Is the value coming back from term_columns() just a guess? */\n> static int term_columns_guessed;\n> @@ -24,11 +24,11 @@ static void close_pager_fds(void)\n> {\n> \t/* signal EOF to pager */\n> \tclose(1);\n> -\tif (close_fd2)\n> +\tif (old_fd2 != -1)\n> \t\tclose(2);\n> }\n> \n> -static void wait_for_pager_atexit(void)\n> +static void finish_pager(void)\n> {\n> \tfflush(stdout);\n> \tfflush(stderr);\n> @@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)\n> \tfinish_command(&pager_process);\n> }\n> \n> +static void wait_for_pager_atexit(void)\n> +{\n> +\tif (old_fd1 == -1)\n> +\t\treturn;\n> +\n> +\tfinish_pager();\n> +}\n> +\n> +void wait_for_pager(void)\n> +{\n> +\tfinish_pager();\n> +\tsigchain_pop_common();\n> +\tunsetenv(\"GIT_PAGER_IN_USE\");\n> +\tdup2(old_fd1, 1);\n> +\tclose(old_fd1);\n> +\told_fd1 = -1;\n> +\tif (old_fd2 != -1) {\n> +\t\tdup2(old_fd2, 2);\n> +\t\tclose(old_fd2);\n> +\t\told_fd2 = -1;\n> +\t}\n> +}\n> +\n> static void wait_for_pager_signal(int signo)\n> {\n> +\tif (old_fd1 == -1)\n> +\t\treturn;\n> +\n> \tclose_pager_fds();\n> \tfinish_command_in_signal(&pager_process);\n> \tsigchain_pop(signo);\n> @@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n> \n> void setup_pager(void)\n> {\n> +\tstatic int once = 0;\n> \tconst char *pager = git_pager(isatty(1));\n> \n> \tif (!pager)\n> @@ -142,16 +169,20 @@ void setup_pager(void)\n> \t\tdie(\"unable to execute pager '%s'\", pager);\n> \n> \t/* original process continues, but writes to the pipe */\n> +\told_fd1 = dup(1);\n> \tdup2(pager_process.in, 1);\n> \tif (isatty(2)) {\n> -\t\tclose_fd2 = 1;\n> +\t\told_fd2 = dup(2);\n> \t\tdup2(pager_process.in, 2);\n> \t}\n> \tclose(pager_process.in);\n> \n> -\t/* this makes sure that the parent terminates after the pager */\n> \tsigchain_push_common(wait_for_pager_signal);\n> -\tatexit(wait_for_pager_atexit);\n> +\n> +\tif (!once) {\n> +\t\tonce++;\n> +\t\tatexit(wait_for_pager_atexit);\n> +\t}\n> }\n> \n> int pager_in_use(void)\n> diff --git a/pager.h b/pager.h\n> index b77433026d..103ecac476 100644\n> --- a/pager.h\n> +++ b/pager.h\n> @@ -5,6 +5,7 @@ struct child_process;\n> \n> const char *git_pager(int stdout_is_tty);\n> void setup_pager(void);\n> +void wait_for_pager(void);\n> int pager_in_use(void);\n> int term_columns(void);\n> void term_clear_line(void);\n"},{"id":"498753","messageId":"1e38a2f0-623c-46cf-b5c5-9e3a4b153cac@gmail.com","threadId":"61768","inReplyTo":"384f0147-d611-493b-a3d4-d83c65bd1114@gmail.com","subject":"Re: [PATCH v3 3/4] pager: introduce wait_for_pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-15T20:04:09Z","receivedAt":"2024-07-15T20:04:14Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Jul 15, 2024 at 03:13:09PM +0100, Phillip Wood wrote:\n> Hi Rubén\n> \n> On 14/07/2024 17:04, Rubén Justo wrote:\n> > Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,\n> > 2006-02-28) we have the machinery to send our output to a pager.\n> > \n> > That machinery, once set up, does not allow us to regain the original\n> > stdio streams.\n> > \n> > In the interactive commands (i.e.: add -p) we want to use the pager for\n> > some output, while maintaining the interaction with the user.\n> > \n> > Modify the pager machinery so that we can use setup_pager and, once\n> > we've finished sending the desired output for the pager, wait for the\n> > pager termination using a new function wait_for_pager. Make this\n> > function reset the pager machinery before returning.\n> \n> Do you have any comments on my thoughts in\n> <8434fafe-f545-49bc-8cc1-d4e8fb634bec@gmail.com> ?\n\nOops! I thought I had responded, but somehow I must not have. \n\nFor reference, these are the points you indicated: \n\n> - We ignore any errors when duplicating fds,\n> \"git grep '[^a-z_]dup2\\{0,1\\}(' shows that's not unusual in our\n> code base, though if we cannot redirect the output to the pager or\n> restore stdout when the pager exits that's a problem for \"git add -p\"\n> \n> - We should perhaps be marking old_fd[12] with O_CLOEXEC to stop them\n> being passed to the pager.\n\nBoth points are interesting and improve resilience to unexpected\nsituations. I remember that the first point was already suggested in\nthe previous thread.\n\nIMHO both points should be considered with a more global perspective\nthan the scope of this series.\n\nAs I said in the first message of this thread, I have left out\ninteresting points that may deserve to be addressed in future series,\nwith the intention of not prolonging the discussion of the current\nchanges too much.\n\nSorry for not responding sooner.\n"},{"id":"498754","messageId":"a70bddd4-ef2d-488e-a2cf-48515f5df357@gmail.com","threadId":"61768","inReplyTo":"efa98aec-f117-4cfe-a7c2-e8c0adbdb399@gmail.com","subject":"[PATCH v4 0/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-15T20:16:55Z","receivedAt":"2024-07-15T20:16:59Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Make the test name more descriptive and avoid unnecessary redirection.\n\nThanks.\n\nRubén Justo (4):\n add-patch: test for 'p' command\n pager: do not close fd 2 unnecessarily\n pager: introduce wait_for_pager\n add-patch: render hunks through the pager\n\n add-patch.c | 18 ++++++++++++---\n pager.c | 45 +++++++++++++++++++++++++++++++++-----\n pager.h | 1 +\n t/t3701-add-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 100 insertions(+), 8 deletions(-)\n\nInterdiff against v3:\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 2ac860cc42..c60589cb94 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -612,11 +612,11 @@ test_expect_success TTY 'print again the hunk (PAGER)' '\n \ttest_cmp expect actual.trimmed\n '\n \n-test_expect_success TTY 'P does not break if pager ends unexpectedly' '\n+test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n \ttest_when_finished \"rm -f huge_file; git reset\" &&\n \tprintf \"\\n%2500000s\" Y >huge_file &&\n \tgit add -N huge_file &&\n-\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p >actual\n+\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n '\n \n test_expect_success 'split hunk \"add -p (edit)\"' '\n-- \n2.46.0.rc0.4.g229d67bbd7\n"},{"id":"498756","messageId":"6b21ace7-2abe-4ec0-9a34-09ec45599575@gmail.com","threadId":"61768","inReplyTo":"a70bddd4-ef2d-488e-a2cf-48515f5df357@gmail.com","subject":"[PATCH v4 1/4] add-patch: test for 'p' command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-15T20:20:58Z","receivedAt":"2024-07-15T20:21:02Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Add a test for the 'p' command, which was introduced in 66c14ab592\n(add-patch: introduce 'p' in interactive-patch, 2024-03-29).\n\nSigned-off-by: Rubén Justo \n---\n t/t3701-add-interactive.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 5d78868ac1..6daf3a6be0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -575,6 +575,22 @@ test_expect_success 'navigate to hunk via regex / pattern' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success 'print again the hunk' '\n+\ttest_when_finished \"git reset\" &&\n+\ttr _ \" \" >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n+\t 10\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\tEOF\n+\ttest_write_lines s y g 1 p | git add -p >actual &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.46.0.rc0.4.g229d67bbd7\n"},{"id":"498757","messageId":"5478131d-ed0c-4a0a-832f-39189db07941@gmail.com","threadId":"61768","inReplyTo":"a70bddd4-ef2d-488e-a2cf-48515f5df357@gmail.com","subject":"[PATCH v4 2/4] pager: do not close fd 2 unnecessarily","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-15T20:21:23Z","receivedAt":"2024-07-15T20:21:27Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We send errors to the pager since 61b80509e3 (sending errors to stdout\nunder $PAGER, 2008-02-16).\n\nIn a8335024c2 (pager: do not dup2 stderr if it is already redirected,\n2008-12-15) an exception was introduced to avoid redirecting stderr if\nit is not connected to a terminal.\n\nIn such exceptional cases, the close(STDERR_FILENO) we're doing in\nclose_pager_fds, is unnecessary.\n\nFurthermore, in a subsequent commit we're going to introduce changes\nthat will involve using close_pager_fds multiple times.\n\nWith this in mind, controlling when we want to close stderr, become\nsensible.\n\nLet's close(STDERR_FILENO) only when necessary, and pave the way for the\nupcoming changes.\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex be6f4ee59f..251adfc2ad 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,6 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n+static int close_fd2;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -23,7 +24,8 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tclose(2);\n+\tif (close_fd2)\n+\t\tclose(2);\n }\n \n static void wait_for_pager_atexit(void)\n@@ -141,8 +143,10 @@ void setup_pager(void)\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\n-\tif (isatty(2))\n+\tif (isatty(2)) {\n+\t\tclose_fd2 = 1;\n \t\tdup2(pager_process.in, 2);\n+\t}\n \tclose(pager_process.in);\n \n \t/* this makes sure that the parent terminates after the pager */\n-- \n2.46.0.rc0.4.g229d67bbd7\n"},{"id":"498758","messageId":"bbdce408-230e-497a-ae05-ad9be8f3e70a@gmail.com","threadId":"61768","inReplyTo":"a70bddd4-ef2d-488e-a2cf-48515f5df357@gmail.com","subject":"[PATCH v4 3/4] pager: introduce wait_for_pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-15T20:21:53Z","receivedAt":"2024-07-15T20:21:57Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,\n2006-02-28) we have the machinery to send our output to a pager.\n\nThat machinery, once set up, does not allow us to regain the original\nstdio streams.\n\nIn the interactive commands (i.e.: add -p) we want to use the pager for\nsome output, while maintaining the interaction with the user.\n\nModify the pager machinery so that we can use setup_pager and, once\nwe've finished sending the desired output for the pager, wait for the\npager termination using a new function wait_for_pager. Make this\nfunction reset the pager machinery before returning.\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 43 +++++++++++++++++++++++++++++++++++++------\n pager.h | 1 +\n 2 files changed, 38 insertions(+), 6 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 251adfc2ad..bea4345f6f 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,7 +14,7 @@ int pager_use_color = 1;\n \n static struct child_process pager_process;\n static char *pager_program;\n-static int close_fd2;\n+static int old_fd1 = -1, old_fd2 = -1;\n \n /* Is the value coming back from term_columns() just a guess? */\n static int term_columns_guessed;\n@@ -24,11 +24,11 @@ static void close_pager_fds(void)\n {\n \t/* signal EOF to pager */\n \tclose(1);\n-\tif (close_fd2)\n+\tif (old_fd2 != -1)\n \t\tclose(2);\n }\n \n-static void wait_for_pager_atexit(void)\n+static void finish_pager(void)\n {\n \tfflush(stdout);\n \tfflush(stderr);\n@@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)\n \tfinish_command(&pager_process);\n }\n \n+static void wait_for_pager_atexit(void)\n+{\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n+\tfinish_pager();\n+}\n+\n+void wait_for_pager(void)\n+{\n+\tfinish_pager();\n+\tsigchain_pop_common();\n+\tunsetenv(\"GIT_PAGER_IN_USE\");\n+\tdup2(old_fd1, 1);\n+\tclose(old_fd1);\n+\told_fd1 = -1;\n+\tif (old_fd2 != -1) {\n+\t\tdup2(old_fd2, 2);\n+\t\tclose(old_fd2);\n+\t\told_fd2 = -1;\n+\t}\n+}\n+\n static void wait_for_pager_signal(int signo)\n {\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n \tclose_pager_fds();\n \tfinish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n@@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n \n void setup_pager(void)\n {\n+\tstatic int once = 0;\n \tconst char *pager = git_pager(isatty(1));\n \n \tif (!pager)\n@@ -142,16 +169,20 @@ void setup_pager(void)\n \t\tdie(\"unable to execute pager '%s'\", pager);\n \n \t/* original process continues, but writes to the pipe */\n+\told_fd1 = dup(1);\n \tdup2(pager_process.in, 1);\n \tif (isatty(2)) {\n-\t\tclose_fd2 = 1;\n+\t\told_fd2 = dup(2);\n \t\tdup2(pager_process.in, 2);\n \t}\n \tclose(pager_process.in);\n \n-\t/* this makes sure that the parent terminates after the pager */\n \tsigchain_push_common(wait_for_pager_signal);\n-\tatexit(wait_for_pager_atexit);\n+\n+\tif (!once) {\n+\t\tonce++;\n+\t\tatexit(wait_for_pager_atexit);\n+\t}\n }\n \n int pager_in_use(void)\ndiff --git a/pager.h b/pager.h\nindex b77433026d..103ecac476 100644\n--- a/pager.h\n+++ b/pager.h\n@@ -5,6 +5,7 @@ struct child_process;\n \n const char *git_pager(int stdout_is_tty);\n void setup_pager(void);\n+void wait_for_pager(void);\n int pager_in_use(void);\n int term_columns(void);\n void term_clear_line(void);\n-- \n2.46.0.rc0.4.g229d67bbd7\n"},{"id":"498759","messageId":"9ad2200b-46b2-40b8-abb6-5bc0c1d1684a@gmail.com","threadId":"61768","inReplyTo":"a70bddd4-ef2d-488e-a2cf-48515f5df357@gmail.com","subject":"[PATCH v4 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-15T20:22:14Z","receivedAt":"2024-07-15T20:22:18Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Make the print command trigger the pager when invoked using a capital\n'P', to make it easier for the user to review long hunks.\n\nNote that if the PAGER ends unexpectedly before we've been able to send\nthe payload, perhaps because the user is not interested in the whole\nthing, we might receive a SIGPIPE, which would abruptly and unexpectedly\nterminate the interactive session for the user.\n\nTherefore, we need to ignore a possible SIGPIPE signal. Add a test for\nthis, in addition to the test for normal operation.\n\nFor the SIGPIPE test, we need to make sure that we completely fill the\noperating system's buffer, otherwise we might not trigger the SIGPIPE\nsignal. The normal size of this buffer in different OSs varies from a\nfew KBs to 1MB. Use a payload large enough to guarantee that we exceed\nthis limit.\n\nSigned-off-by: Rubén Justo \n---\n add-patch.c | 18 +++++++++++++++---\n t/t3701-add-interactive.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 6e176cd21a..f2c76b7d83 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -7,9 +7,11 @@\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"object-name.h\"\n+#include \"pager.h\"\n #include \"read-cache-ll.h\"\n #include \"repository.h\"\n #include \"strbuf.h\"\n+#include \"sigchain.h\"\n #include \"run-command.h\"\n #include \"strvec.h\"\n #include \"pathspec.h\"\n@@ -1391,7 +1393,7 @@ N_(\"j - leave this hunk undecided, see next undecided hunk\\n\"\n \"/ - search for a hunk matching the given regex\\n\"\n \"s - split the current hunk into smaller hunks\\n\"\n \"e - manually edit the current hunk\\n\"\n- \"p - print the current hunk\\n\"\n+ \"p - print the current hunk, 'P' to use the pager\\n\"\n \"? - print help\\n\");\n \n static int patch_update_file(struct add_p_state *s,\n@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,\n \tstruct hunk *hunk;\n \tchar ch;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tint colored = !!s->colored.len, quit = 0;\n+\tint colored = !!s->colored.len, quit = 0, use_pager = 0;\n \tenum prompt_mode_type prompt_mode_type;\n \tenum {\n \t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,\n \t\tstrbuf_reset(&s->buf);\n \t\tif (file_diff->hunk_nr) {\n \t\t\tif (rendered_hunk_index != hunk_index) {\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsetup_pager();\n+\t\t\t\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\t\t\t}\n \t\t\t\trender_hunk(s, hunk, 0, colored, &s->buf);\n \t\t\t\tfputs(s->buf.buf, stdout);\n \t\t\t\trendered_hunk_index = hunk_index;\n+\t\t\t\tif (use_pager) {\n+\t\t\t\t\tsigchain_pop(SIGPIPE);\n+\t\t\t\t\twait_for_pager();\n+\t\t\t\t\tuse_pager = 0;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tstrbuf_reset(&s->buf);\n@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\thunk->use = USE_HUNK;\n \t\t\t\tgoto soft_increment;\n \t\t\t}\n-\t\t} else if (s->answer.buf[0] == 'p') {\n+\t\t} else if (ch == 'p') {\n \t\t\trendered_hunk_index = -1;\n+\t\t\tuse_pager = (s->answer.buf[0] == 'P') ? 1 : 0;\n \t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 6daf3a6be0..c60589cb94 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -591,6 +591,34 @@ test_expect_success 'print again the hunk' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success TTY 'print again the hunk (PAGER)' '\n+\ttest_when_finished \"git reset\" &&\n+\tcat >expect <<-EOF &&\n+\t+15\n+\t 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? PAGER @@ -1,2 +1,3 @@\n+\tPAGER 10\n+\tPAGER +15\n+\tPAGER 20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? \n+\tEOF\n+\ttest_write_lines s y g 1 P |\n+\t(\n+\t\tGIT_PAGER=\"sed s/^/PAGER\\ /\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p >actual\n+\t) &&\n+\ttail -n 7 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n+test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n+\ttest_when_finished \"rm -f huge_file; git reset\" &&\n+\tprintf \"\\n%2500000s\" Y >huge_file &&\n+\tgit add -N huge_file &&\n+\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.46.0.rc0.4.g229d67bbd7\n"},{"id":"498768","messageId":"xmqqttgqyzwa.fsf@gitster.g","threadId":"61768","inReplyTo":"1dc9ebad-768b-4c1a-8a58-8a7a5d24d49e@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-15T23:54:45Z","receivedAt":"2024-07-15T23:54:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> +test_expect_success TTY 'P does not break if pager ends unexpectedly' '\n> +\ttest_when_finished \"rm -f huge_file; git reset\" &&\n> +\tprintf \"\\n%2500000s\" Y >huge_file &&\n> +\tgit add -N huge_file &&\n> +\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p >actual\n> +'\n\nSomehow this is dying with signal 11?\n\n https://github.com/git/git/actions/runs/9944800531/job/27471636926#step:5:1108\n\nI know there is a v4 that has a small update here, but in case that\nthis still is relevant and the removed rediraction to \">actual\" did\nnot magically fixed it...\n\nThanks.\n\n"},{"id":"498833","messageId":"cf999478-b1fd-4c93-a11b-1b34d51767d3@gmail.com","threadId":"61768","inReplyTo":"1e38a2f0-623c-46cf-b5c5-9e3a4b153cac@gmail.com","subject":"Re: [PATCH v3 3/4] pager: introduce wait_for_pager","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-17T14:58:41Z","receivedAt":"2024-07-17T14:58:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 15/07/2024 21:04, Rubén Justo wrote:\n> On Mon, Jul 15, 2024 at 03:13:09PM +0100, Phillip Wood wrote:\n> For reference, these are the points you indicated:\n> \n>> - We ignore any errors when duplicating fds,\n>> \"git grep '[^a-z_]dup2\\{0,1\\}(' shows that's not unusual in our\n>> code base, though if we cannot redirect the output to the pager or\n>> restore stdout when the pager exits that's a problem for \"git add -p\"\n>>\n>> - We should perhaps be marking old_fd[12] with O_CLOEXEC to stop them\n>> being passed to the pager.\n> \n> Both points are interesting and improve resilience to unexpected\n> situations. I remember that the first point was already suggested in\n> the previous thread.\n> \n> IMHO both points should be considered with a more global perspective\n> than the scope of this series.\n\nI'm not sure what you mean by this. It is true that we were ignoring \ndup2() errors in this function before but this patch the O_CLOEXEC issue \nis new.\n\n> As I said in the first message of this thread, I have left out\n> interesting points that may deserve to be addressed in future series,\n> with the intention of not prolonging the discussion of the current\n> changes too much.\n\nI think there is a difference between adding new features which I agree \nshould be left and getting the implementation details of this helper \nfunction right. Having said that ignoring the dup() errors isn't making \nanything worse than it was and the extra fds are for the terminal which \nthe pager is accessing anyway.\n\n> Sorry for not responding sooner.\n\nNo worries, thanks for your reply\n\nPhillip\n"},{"id":"498844","messageId":"2b57479c-29c8-4a6e-b7b0-1309395cfbd9@gmail.com","threadId":"61768","inReplyTo":"xmqqttgqyzwa.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-17T17:20:42Z","receivedAt":"2024-07-17T17:20:45Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Squashing this fixes the test:\n\n--->8---\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex c60589cb94..bb360c92a0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -616,7 +616,12 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n \ttest_when_finished \"rm -f huge_file; git reset\" &&\n \tprintf \"\\n%2500000s\" Y >huge_file &&\n \tgit add -N huge_file &&\n-\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n+\ttest_write_lines P q |\n+\t(\n+\t\tGIT_PAGER=\"head -n 1\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p >actual\n+\t)\n '\n---8<---\n\nHowever, this error has exposed a problem: calling `wait_for_pager` if\n`setup_pager` hasn't worked is an issue that needs to be addressed in this\nseries: `setup_pager` should return a result. I was planning to do that\nin a future series, for the other commented command: `|[cmd]`.\n\nI'm wondering if the best way to proceed here is to revert to: \n\ndiff --git a/pager.c b/pager.c\nindex 5f0c1e9cce..5586e751dc 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)\n\n void wait_for_pager(void)\n {\n+\tif (old_fd1 == -1)\n+\t\treturn;\n \tfinish_pager();\n \tsigchain_pop_common();\n \tunsetenv(\"GIT_PAGER_IN_USE\");\n\nWhich resolves the problem.\n\nThis was a change already commented here:\n\nhttps://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/\n"},{"id":"498851","messageId":"88f9256e-04ba-4799-8048-406863054106@gmail.com","threadId":"61768","inReplyTo":"2b57479c-29c8-4a6e-b7b0-1309395cfbd9@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-17T19:39:12Z","receivedAt":"2024-07-17T19:39:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ruében\n\nThanks for looking into the test failure.\n\nOn 17/07/2024 18:20, Rubén Justo wrote:\n> Squashing this fixes the test:\n> \n> --->8---\n> \n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index c60589cb94..bb360c92a0 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -616,7 +616,12 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n> \ttest_when_finished \"rm -f huge_file; git reset\" &&\n> \tprintf \"\\n%2500000s\" Y >huge_file &&\n> \tgit add -N huge_file &&\n> -\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n> +\ttest_write_lines P q |\n> +\t(\n> +\t\tGIT_PAGER=\"head -n 1\" &&\n> +\t\texport GIT_PAGER &&\n> +\t\ttest_terminal git add -p >actual\n> +\t)\n\nThat's surprising, why does running git in a sub-shell stop it from \nsegfaulting?\n\n> ---8<---\n> \n> However, this error has exposed a problem: calling `wait_for_pager` if\n> `setup_pager` hasn't worked is an issue that needs to be addressed in this\n> series: `setup_pager` should return a result. I was planning to do that\n> in a future series, for the other commented command: `|[cmd]`.\n\nWhat was causing setup pager to fail in this test?\n\n> I'm wondering if the best way to proceed here is to revert to:\n> \n> diff --git a/pager.c b/pager.c\n> index 5f0c1e9cce..5586e751dc 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)\n> \n> void wait_for_pager(void)\n> {\n> +\tif (old_fd1 == -1)\n> +\t\treturn;\n> \tfinish_pager();\n> \tsigchain_pop_common();\n> \tunsetenv(\"GIT_PAGER_IN_USE\");\n> \n> This was a change already commented here:\n> \n> https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/\n\nMy worry was that this would paper over a bug as we shouldn't be calling \nwait_for_pager() without setting up the pager successfully. How easy \nwould it be to fix the source of the problem?\n\nBest Wishes\n\nPhillip\n"},{"id":"498855","messageId":"xmqqfrs723bp.fsf@gitster.g","threadId":"61768","inReplyTo":"88f9256e-04ba-4799-8048-406863054106@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-17T20:03:54Z","receivedAt":"2024-07-17T20:03:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n>> -\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n>> +\ttest_write_lines P q |\n>> +\t(\n>> +\t\tGIT_PAGER=\"head -n 1\" &&\n>> +\t\texport GIT_PAGER &&\n>> +\t\ttest_terminal git add -p >actual\n>> +\t)\n>\n> That's surprising, why does running git in a sub-shell stop it from\n> segfaulting?\n\nYeah, it indeed is curious. \n\nThe rewrite resolves another iffy point in the original---you are\nnot supposed to attempt a one-shot assignment to the environment\nvariable when you are running a shell function, as that is not\nportable. And the above rewrite is a common way to fix that.\n\nBut still, yes, it is curious why the original segfaults. Is there\nsome race there and having a subshell shifts the timing, or\nsomething?\n\n> My worry was that this would paper over a bug as we shouldn't be\n> calling wait_for_pager() without setting up the pager\n> successfully. How easy would it be to fix the source of the problem?\n\n;-) Nice to see people trying to do the right thing.\n\nThanks.\n"},{"id":"498856","messageId":"CAPig+cRyj8J7MZEufu34NUzwOL2n=w35nT1Ug7FGRwMC0=Qpwg@mail.gmail.com","threadId":"61768","inReplyTo":"xmqqfrs723bp.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-07-17T20:09:17Z","receivedAt":"2024-07-17T20:09:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 17, 2024 at 4:04 PM Junio C Hamano wrote:\n> phillip.wood123@gmail.com writes:\n> >> - test_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n> >> + test_write_lines P q |\n> >> + (\n> >> + GIT_PAGER=\"head -n 1\" &&\n> >> + export GIT_PAGER &&\n> >> + test_terminal git add -p >actual\n> >> + )\n> >\n> > That's surprising, why does running git in a sub-shell stop it from\n> > segfaulting?\n>\n> Yeah, it indeed is curious.\n>\n> The rewrite resolves another iffy point in the original---you are\n> not supposed to attempt a one-shot assignment to the environment\n> variable when you are running a shell function, as that is not\n> portable. And the above rewrite is a common way to fix that.\n\nIt's also curious that t/check-non-portable-shell.pl didn't catch this\nuse of one-shot assignment when calling a shell function[*].\n\n[*] a0a630192d (t/check-non-portable-shell: detect \"FOO=bar\nshell_func\", 2018-07-13)\n"},{"id":"498857","messageId":"xmqq8qxz22ii.fsf@gitster.g","threadId":"61768","inReplyTo":"CAPig+cRyj8J7MZEufu34NUzwOL2n=w35nT1Ug7FGRwMC0=Qpwg@mail.gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-17T20:21:25Z","receivedAt":"2024-07-17T20:21:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Jul 17, 2024 at 4:04 PM Junio C Hamano wrote:\n>> phillip.wood123@gmail.com writes:\n>> >> - test_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n>> >> + test_write_lines P q |\n>> >> + (\n>> >> + GIT_PAGER=\"head -n 1\" &&\n>> >> + export GIT_PAGER &&\n>> >> + test_terminal git add -p >actual\n>> >> + )\n>> >\n>> > That's surprising, why does running git in a sub-shell stop it from\n>> > segfaulting?\n>>\n>> Yeah, it indeed is curious.\n>>\n>> The rewrite resolves another iffy point in the original---you are\n>> not supposed to attempt a one-shot assignment to the environment\n>> variable when you are running a shell function, as that is not\n>> portable. And the above rewrite is a common way to fix that.\n>\n> It's also curious that t/check-non-portable-shell.pl didn't catch this\n> use of one-shot assignment when calling a shell function[*].\n\nTrue.\n"},{"id":"498858","messageId":"xmqqy15zzror.fsf@gitster.g","threadId":"61768","inReplyTo":"88f9256e-04ba-4799-8048-406863054106@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-17T20:31:16Z","receivedAt":"2024-07-17T20:31:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n>> -\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n>> +\ttest_write_lines P q |\n>> +\t(\n>> +\t\tGIT_PAGER=\"head -n 1\" &&\n>> +\t\texport GIT_PAGER &&\n>> +\t\ttest_terminal git add -p >actual\n>> +\t)\n>\n> That's surprising, why does running git in a sub-shell stop it from\n> segfaulting?\n\nAnother difference besides the sub-shell is where the output from\ntest_terminal goes. If the above change fixes the issue for Rubén,\nI wonder if it still works if \">actual\" gets removed.\n"},{"id":"498912","messageId":"e2532dee-5d16-484e-ba13-840af7b47c27@gmail.com","threadId":"61768","inReplyTo":"xmqqfrs723bp.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-18T09:48:28Z","receivedAt":"2024-07-18T09:48:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 17/07/2024 21:03, Junio C Hamano wrote:\n> phillip.wood123@gmail.com writes:\n> \n>>> -\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n>>> +\ttest_write_lines P q |\n>>> +\t(\n>>> +\t\tGIT_PAGER=\"head -n 1\" &&\n>>> +\t\texport GIT_PAGER &&\n>>> +\t\ttest_terminal git add -p >actual\n>>> +\t)\n>>\n>> That's surprising, why does running git in a sub-shell stop it from\n>> segfaulting?\n> \n> Yeah, it indeed is curious.\n> \n> The rewrite resolves another iffy point in the original---you are\n> not supposed to attempt a one-shot assignment to the environment\n> variable when you are running a shell function, as that is not\n> portable. And the above rewrite is a common way to fix that.\n\nGood point, I'd not thought of that.\n\nBest Wishes\n\nPhillip\n"},{"id":"498913","messageId":"6317c4a0-06b9-4e2c-9b23-10cb96950617@gmail.com","threadId":"61768","inReplyTo":"xmqqy15zzror.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-18T09:56:19Z","receivedAt":"2024-07-18T09:56:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 17/07/2024 21:31, Junio C Hamano wrote:\n> phillip.wood123@gmail.com writes:\n> \n>>> -\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n>>> +\ttest_write_lines P q |\n>>> +\t(\n>>> +\t\tGIT_PAGER=\"head -n 1\" &&\n>>> +\t\texport GIT_PAGER &&\n>>> +\t\ttest_terminal git add -p >actual\n>>> +\t)\n>>\n>> That's surprising, why does running git in a sub-shell stop it from\n>> segfaulting?\n> \n> Another difference besides the sub-shell is where the output from\n> test_terminal goes. If the above change fixes the issue for Rubén,\n> I wonder if it still works if \">actual\" gets removed.\n\nYes that would be interesting to know. I do think we should be checking \nthe output here to make sure the test is doing what we think it is - all \nwe know at the moment is whether git exits successfully or not. If we \nreceive SIGPIPE then fputs() will see EPIPE which will set ferror() \nwhich we should be clearing and checking that the prompt prints \ncorrectly afterwards. I think it would be helpful to add\n\ntail -n3 actual >actual.trimmed &&\ncat >expect <<\\EOF &&\n\t\\ No newline at end of file\n\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? @@ -0,0 +1,2 @@\n\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?\n\tEOF\ntest_cmp expect actual.trimmed\n\nto the test. That all still leaves me wondering about the segfault though\n\nBest Wishes\n\nPhillip\n"},{"id":"498914","messageId":"f52d5c9c-e96c-430f-a49a-eb2ee6e19d9f@gmail.com","threadId":"61768","inReplyTo":"2b57479c-29c8-4a6e-b7b0-1309395cfbd9@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-18T09:58:36Z","receivedAt":"2024-07-18T09:58:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 17/07/2024 18:20, Rubén Justo wrote:\n> However, this error has exposed a problem: calling `wait_for_pager` if\n> `setup_pager` hasn't worked is an issue that needs to be addressed in this\n> series: `setup_pager` should return a result.\n\nIt already dies if we cannot execute the pager so maybe we should just \ndie on other errors as well?\n\nBest Wishes\n\nPhillip\n"},{"id":"499015","messageId":"a2ea00e2-08e4-4e6b-b81c-ef3ba02b4b1f@gmail.com","threadId":"61768","inReplyTo":"88f9256e-04ba-4799-8048-406863054106@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-20T22:29:44Z","receivedAt":"2024-07-20T22:29:48Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Jul 17, 2024 at 08:39:12PM +0100, phillip.wood123@gmail.com wrote:\n\n> On 17/07/2024 18:20, Rubén Justo wrote:\n> > Squashing this fixes the test:\n> > \n> > --->8---\n> > \n> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> > index c60589cb94..bb360c92a0 100755\n> > --- a/t/t3701-add-interactive.sh\n> > +++ b/t/t3701-add-interactive.sh\n> > @@ -616,7 +616,12 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n> > \ttest_when_finished \"rm -f huge_file; git reset\" &&\n> > \tprintf \"\\n%2500000s\" Y >huge_file &&\n> > \tgit add -N huge_file &&\n> > -\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n> > +\ttest_write_lines P q |\n> > +\t(\n> > +\t\tGIT_PAGER=\"head -n 1\" &&\n> > +\t\texport GIT_PAGER &&\n> > +\t\ttest_terminal git add -p >actual\n> > +\t)\n> \n> That's surprising, why does running git in a sub-shell stop it from\n> segfaulting?\n\nThe fix isn't the sub-shell; it's \"export GIT_PAGER\".\n\n> \n> > ---8<---\n> > \n> > However, this error has exposed a problem: calling `wait_for_pager` if\n> > `setup_pager` hasn't worked is an issue that needs to be addressed in this\n> > series: `setup_pager` should return a result. I was planning to do that\n> > in a future series, for the other commented command: `|[cmd]`.\n> \n> What was causing setup pager to fail in this test?\n\nBecause GIT_PAGER is not being set correctly in the test, \"git add -p\"\ncan use the values defined in the environment where the test is running.\nUsually PAGER is empty or contains \"less\", but in the environment where\nthe fault occurs, it happens to be: \"PAGER=cat\". \n\nSince we have an optimization to avoid forking if the pager is \"cat\",\ncourtesy of caef71a535 (Do not fork PAGER=cat, 2006-04-16), then we fail\nin `wait_for_pager()` because we are calling `finish_command()` with an\nuninitialized `pager_process`.\n\nThat's why I thought, aligned with what we are already doing in\n`wait_for_pager_at_exit()`, that this is a sensible approach: \n\n> > I'm wondering if the best way to proceed here is to revert to:\n> > \n> > diff --git a/pager.c b/pager.c\n> > index 5f0c1e9cce..5586e751dc 100644\n> > --- a/pager.c\n> > +++ b/pager.c\n> > @@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)\n> > \n> > void wait_for_pager(void)\n> > {\n> > +\tif (old_fd1 == -1)\n> > +\t\treturn;\n> > \tfinish_pager();\n> > \tsigchain_pop_common();\n> > \tunsetenv(\"GIT_PAGER_IN_USE\");\n> >\n> > This was a change already commented here:\n> > \n> > https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/\n"},{"id":"499016","messageId":"bc1b9cce-d04d-4a79-8fab-55ec3c8bae30@gmail.com","threadId":"61768","inReplyTo":"CAPig+cRyj8J7MZEufu34NUzwOL2n=w35nT1Ug7FGRwMC0=Qpwg@mail.gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-20T22:37:08Z","receivedAt":"2024-07-20T22:37:11Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Jul 17, 2024 at 04:09:17PM -0400, Eric Sunshine wrote:\n\n> > >> - test_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n\n> It's also curious that t/check-non-portable-shell.pl didn't catch this\n> use of one-shot assignment when calling a shell function[*].\n\nIt would have been great if it had caught that error.\n\nAs a reference:\n\n func () {\n }\n\n VAR=value func # this error is caught\n echo 1 |\n VAR=value func # this one is also caught\n echo 1 | VAR=value func # this one isn't\n\nMaybe, catch this errors expanding the regular expression we have in\n`check-non-portable-shell.pl` isn't the best approach. We might need\nsomething more sophisticated, like what we have in `chainlint.pl`.\n\nPerhaps someone with experience in those scripts could give us this\ncapability :-)\n\nThanks for your message.\n"},{"id":"499017","messageId":"702a80da-e5b2-428a-b91d-8cd1cd897fd7@gmail.com","threadId":"61768","inReplyTo":"xmqqy15zzror.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-20T22:39:19Z","receivedAt":"2024-07-20T22:39:22Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Jul 17, 2024 at 01:31:16PM -0700, Junio C Hamano wrote:\n\n> >> -\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n> >> +\ttest_write_lines P q |\n> >> +\t(\n> >> +\t\tGIT_PAGER=\"head -n 1\" &&\n> >> +\t\texport GIT_PAGER &&\n> >> +\t\ttest_terminal git add -p >actual\n> >> +\t)\n\n> I wonder if it still works if \">actual\" gets removed.\n\nYes, it works. That redirection is just noise from my previous version.\n"},{"id":"499018","messageId":"ba1c3bca-177c-4dae-b4c3-1a4deab27d5e@gmail.com","threadId":"61768","inReplyTo":"f52d5c9c-e96c-430f-a49a-eb2ee6e19d9f@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-20T22:45:17Z","receivedAt":"2024-07-20T22:45:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Thu, Jul 18, 2024 at 10:58:36AM +0100, phillip.wood123@gmail.com wrote:\n\n> > However, this error has exposed a problem: calling `wait_for_pager` if\n> > `setup_pager` hasn't worked is an issue that needs to be addressed in this\n> > series: `setup_pager` should return a result.\n> \n> It already dies if we cannot execute the pager so maybe we should just die\n> on other errors as well?\n\nHonestly, I thought 78f0a5d187 (pager: die when paging to non-existing\ncommand, 2024-06-23) would be sufficient for this series, but I missed\nthe optimization mentioned in my previous message.\n\nThinking in the context of \"add -p\", it might be more sensible not to\ndie but simply show an error, so as not to end the user's interactive\nsession. But it could be a change in a future series and thus avoid\nprolonging this one. \n\nHowever, if you can think of any other cases where we should be stricter\nand die, I'm all ears.\n"},{"id":"499045","messageId":"CAPig+cR3BHMyuGOMYdbRxvMfNzZBGQjgsrJU4MOx-e67DOTsdQ@mail.gmail.com","threadId":"61768","inReplyTo":"bc1b9cce-d04d-4a79-8fab-55ec3c8bae30@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-07-22T07:18:07Z","receivedAt":"2024-07-22T07:18:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jul 20, 2024 at 6:37 PM Rubén Justo wrote:\n> On Wed, Jul 17, 2024 at 04:09:17PM -0400, Eric Sunshine wrote:\n> > > >> - test_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n>\n> > It's also curious that t/check-non-portable-shell.pl didn't catch this\n> > use of one-shot assignment when calling a shell function[*].\n>\n> It would have been great if it had caught that error.\n>\n> As a reference:\n> VAR=value func # this error is caught\n> echo 1 |\n> VAR=value func # this one is also caught\n> echo 1 | VAR=value func # this one isn't\n\nThanks for providing this summary; it saved me the effort of digging\nback through this discussion / patch series.\n\n> Maybe, catch this errors expanding the regular expression we have in\n> `check-non-portable-shell.pl` isn't the best approach. We might need\n> something more sophisticated, like what we have in `chainlint.pl`.\n\nThe idea has been expressed previously of subsuming all the\ncheck-non-portable-shell.pl checks into chainlint.pl some day, thus\nallowing check-non-portable-shell.pl to be retired. In fact, it was\nmentioned again quite recently[1].\n\nHowever, this particular check (detecting `VAR=val shell-func`) poses\nan extra complication which would require some specialized additional\nmechanism in chainlint.pl. In particular, in `VAR=val symbol`, in\norder to distinguish when `symbol` is an external command versus a\nshell-function, it is necessary to scan for function definitions not\njust in the script being checked, but also in all scripts included\n(recursively) by the script being checked. So, it's probably possible\nto do but ought to be done carefully.\n\n> Perhaps someone with experience in those scripts could give us this\n> capability :-)\n\nI posted a series[2] which addresses this shortcoming by enhancing\ncheck-non-portable-shell.pl.\n\n[1]: https://lore.kernel.org/git/CAPig+cTFZuU7zM7poqk4HeK09zn8bFrO37eUZiaGmeJ0yecpiw@mail.gmail.com/\n[2]: https://lore.kernel.org/git/20240722065915.80760-1-ericsunshine@charter.net/T/\n"},{"id":"499052","messageId":"5360ab9d-6d3e-4da0-b1c4-2ff381372c1a@gmail.com","threadId":"61768","inReplyTo":"a2ea00e2-08e4-4e6b-b81c-ef3ba02b4b1f@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-22T10:18:00Z","receivedAt":"2024-07-22T10:18:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 20/07/2024 23:29, Rubén Justo wrote:\n> On Wed, Jul 17, 2024 at 08:39:12PM +0100, phillip.wood123@gmail.com wrote:\n> \n>> On 17/07/2024 18:20, Rubén Justo wrote:\n>>\n>>> However, this error has exposed a problem: calling `wait_for_pager` if\n>>> `setup_pager` hasn't worked is an issue that needs to be addressed in this\n>>> series: `setup_pager` should return a result. I was planning to do that\n>>> in a future series, for the other commented command: `|[cmd]`.\n>>\n>> What was causing setup pager to fail in this test?\n> \n> Because GIT_PAGER is not being set correctly in the test,\n\nOh I see, I had assumed that the shell would report an error if it \ndidn't support setting environment variables like this when running a \nfunction but instead it silently ignored it. This highlights the \nimportance of testing the output of \"git add -p\" in this test so we can \nbe sure the pager is doing what we think it should. Using\n\n\tGIT_PAGER=\"sed -e s/^/PAGER\\ / -e q\"\n\nwould make it clear what the pager is printing while also causing SIGPIPE\n\n> \"git add -p\"\n> can use the values defined in the environment where the test is running.\n> Usually PAGER is empty or contains \"less\", but in the environment where\n> the fault occurs, it happens to be: \"PAGER=cat\".\n> \n> Since we have an optimization to avoid forking if the pager is \"cat\",\n> courtesy of caef71a535 (Do not fork PAGER=cat, 2006-04-16), then we fail\n> in `wait_for_pager()` because we are calling `finish_command()` with an\n> uninitialized `pager_process`.\n> \n> That's why I thought, aligned with what we are already doing in\n> `wait_for_pager_at_exit()`, that this is a sensible approach:\n\nThat extra information is important. When I said [1]\n\n > Isn't it a bug to call this with old_fd1 == -1 or have I missed\n > something?\n\nWhat I'd missed was that we can return early without executing anything. \nWe cannot do\n\n\tif (!git_pager(asatty(1))\n\t\treturn\n\nat the beginning of wait_for_pager() because if we're running a pager \nisatty(1) will return false so I think the old_fd as you suggested is \nthe easiest fix. The existing callers do not need to know if \nsetup_pager() applied the \"cat\" optimization because they only setup the \npager once. For \"add -p\" this no-longer applies so we should think about \nreturning a flag to say \"there was an error\"/\"there is no pager or the \npager is 'cat'\"/\"the pager has been started\"\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com\n\n>>> I'm wondering if the best way to proceed here is to revert to:\n>>>\n>>> diff --git a/pager.c b/pager.c\n>>> index 5f0c1e9cce..5586e751dc 100644\n>>> --- a/pager.c\n>>> +++ b/pager.c\n>>> @@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)\n>>>\n>>> void wait_for_pager(void)\n>>> {\n>>> +\tif (old_fd1 == -1)\n>>> +\t\treturn;\n>>> \tfinish_pager();\n>>> \tsigchain_pop_common();\n>>> \tunsetenv(\"GIT_PAGER_IN_USE\");\n>>>\n>>> This was a change already commented here:\n>>>\n>>> https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/\n\n"},{"id":"499059","messageId":"a0b3fed8-42cf-4e87-bdc2-1f090f3cc2eb@gmail.com","threadId":"61768","inReplyTo":"CAPig+cR3BHMyuGOMYdbRxvMfNzZBGQjgsrJU4MOx-e67DOTsdQ@mail.gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-22T14:53:59Z","receivedAt":"2024-07-22T14:54:02Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Jul 22, 2024 at 03:18:07AM -0400, Eric Sunshine wrote:\n\n> I posted a series[2] which addresses this shortcoming by enhancing\n> check-non-portable-shell.pl.\n\nI tested this series with them, and it correctly detects the error.\n\nThanks for sending the improvement so quick! \n"},{"id":"499064","messageId":"48706007-b387-494f-a104-a8a50128cc67@gmail.com","threadId":"61768","inReplyTo":"5360ab9d-6d3e-4da0-b1c4-2ff381372c1a@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-22T16:45:07Z","receivedAt":"2024-07-22T16:45:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Jul 22, 2024 at 11:18:00AM +0100, Phillip Wood wrote:\n\n> > That's why I thought, aligned with what we are already doing in\n> > `wait_for_pager_at_exit()`, that this is a sensible approach:\n> \n> That extra information is important. When I said [1]\n> \n> > Isn't it a bug to call this with old_fd1 == -1 or have I missed\n> > something?\n> \n> What I'd missed was that we can return early without executing anything.\n\nYep, missing that old optimization is easy ;)\n\n> We cannot do\n> \n> \tif (!git_pager(asatty(1))\n> \t\treturn\n> \n> at the beginning of wait_for_pager() because if we're running a pager\n> isatty(1) will return false so I think the old_fd as you suggested is the\n> easiest fix.\n\nNow that we agree, I'll do it :) \n\n> The existing callers do not need to know if setup_pager()\n> applied the \"cat\" optimization because they only setup the pager once. For\n> \"add -p\" this no-longer applies so we should think about returning a flag to\n> say \"there was an error\"/\"there is no pager or the pager is 'cat'\"/\"the\n> pager has been started\"\n\nI'm not sure it would be valuable for us to make the caller aware that\n\"there is no pager or the pager is 'cat' ... just use stdout\". \n\nHowever, I do agree that probably in the future, if we finally add the\n\"|[cmd]\" command, we'll need to return some kind of error instead of\n`die()`, in setup_pager().\n"},{"id":"499066","messageId":"xmqqv80xcpe5.fsf@gitster.g","threadId":"61768","inReplyTo":"a2ea00e2-08e4-4e6b-b81c-ef3ba02b4b1f@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T17:22:58Z","receivedAt":"2024-07-22T17:23:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n>> > +\ttest_write_lines P q |\n>> > +\t(\n>> > +\t\tGIT_PAGER=\"head -n 1\" &&\n>> > +\t\texport GIT_PAGER &&\n>> > +\t\ttest_terminal git add -p >actual\n>> > +\t)\n>> \n>> That's surprising, why does running git in a sub-shell stop it from\n>> segfaulting?\n>\n> The fix isn't the sub-shell; it's \"export GIT_PAGER\".\n> ...\n> Because GIT_PAGER is not being set correctly in the test, \"git add -p\"\n> can use the values defined in the environment where the test is running.\n> Usually PAGER is empty or contains \"less\", but in the environment where\n> the fault occurs, it happens to be: \"PAGER=cat\". \n>\n> Since we have an optimization to avoid forking if the pager is \"cat\",\n> courtesy of caef71a535 (Do not fork PAGER=cat, 2006-04-16), then we fail\n> in `wait_for_pager()` because we are calling `finish_command()` with an\n> uninitialized `pager_process`.\n\nAttached at the end is a test tweak patch, taking inspirations from\nPhillip's comments, to see what value GIT_PAGER has in the shell\nfunction. I shortened the huge_file a bit so that I do not have to\nhave an infinite scrollback buffer,but otherwise, the test_quirk\nintermediate shell function should work just like the test_terminal\nhelper in the original position would.\n\nAnd I see in the output from \"sh t3701-add-interactive.sh -i -v\":\n\n expecting success of 3701.51 'P handles SIGPIPE when writing to pager': \n test_when_finished \"rm -f huge_file; git reset\" &&\n printf \"\\n%250s\" Y >huge_file &&\n git add -N huge_file &&\n echo \"in env: GIT_PAGER=$(env | grep GIT_PAGER=)\" &&\n test_write_lines P q | GIT_PAGER=\"head -n 1\" test_quirk &&\n echo \"after test_quirk returns: GIT_PAGER=$GIT_PAGER\"\n\n in env: GIT_PAGER=\n in test_quirk: GIT_PAGER=head -n 1\n in env: GIT_PAGER=GIT_PAGER=head -n 1\n In test_terminal: GIT_PAGER=GIT_PAGER=head -n 1\n test-terminal: GIT_PAGER=head -n 1\n diff --git a/huge_file b/huge_file\n new file mode 100644\n index 0000000..d06820d\n --- /dev/null\n +++ b/huge_file\n @@ -0,0 +1,2 @@\n +\n + Y\n \\ No newline at end of file\n (1/1) Stage addition [y,n,q,a,d,e,p,?]? @@ -0,0 +1,2 @@\n (1/1) Stage addition [y,n,q,a,d,e,p,?]? \n after test_quirk returns: GIT_PAGER=\n Unstaged changes after reset:\n M test\n ok 51 - P handles SIGPIPE when writing to pager\n\nSo:\n\n - before the one-shot thing, in the envrionment GIT_PAGER is empty.\n - in the helper function,\n - shell variable GIT_PAGER is set to the expected value.\n - GIT_PAGER env is exported.\n - test-terminal.perl sees $ENV{GIT_PAGER} set to the expected value.\n - after the helper returns GIT_PAGER is empty\n\nIt's a very convincing theory but it does not seem to match my\nobservation. Is there a difference in shells used, or something?\n\n t/lib-terminal.sh | 3 +++\n t/t3701-add-interactive.sh | 15 +++++++++++++--\n t/test-terminal.perl | 2 ++\n 3 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git c/t/lib-terminal.sh w/t/lib-terminal.sh\nindex e3809dcead..558db9aa33 100644\n--- c/t/lib-terminal.sh\n+++ w/t/lib-terminal.sh\n@@ -9,6 +9,9 @@ test_terminal () {\n \t\techo >&4 \"test_terminal: need to declare TTY prerequisite\"\n \t\treturn 127\n \tfi\n+\n+\techo >&4 \"In test_terminal: GIT_PAGER=$(env | grep GIT_PAGER=)\"\n+\n \tperl \"$TEST_DIRECTORY\"/test-terminal.perl \"$@\" 2>&7\n } 7>&2 2>&4\n \ndiff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh\nindex c60589cb94..f7037cbed4 100755\n--- c/t/t3701-add-interactive.sh\n+++ w/t/t3701-add-interactive.sh\n@@ -612,13 +612,24 @@ test_expect_success TTY 'print again the hunk (PAGER)' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_quirk () {\n+\techo \"in test_quirk: GIT_PAGER=$GIT_PAGER\"\n+\techo \"in env: GIT_PAGER=$(env | grep GIT_PAGER=)\"\n+\ttest_terminal git add -p\n+\ttrue\n+}\n+\n test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n \ttest_when_finished \"rm -f huge_file; git reset\" &&\n-\tprintf \"\\n%2500000s\" Y >huge_file &&\n+\tprintf \"\\n%250s\" Y >huge_file &&\n \tgit add -N huge_file &&\n-\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n+\techo \"in env: GIT_PAGER=$(env | grep GIT_PAGER=)\" &&\n+\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_quirk &&\n+\techo \"after test_quirk returns: GIT_PAGER=$GIT_PAGER\"\n '\n \n+exit\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\ndiff --git c/t/test-terminal.perl w/t/test-terminal.perl\nindex b8fd6a4f13..92b1c13675 100755\n--- c/t/test-terminal.perl\n+++ w/t/test-terminal.perl\n@@ -67,6 +67,8 @@ sub copy_stdio {\n if ($#ARGV < 1) {\n \tdie \"usage: test-terminal program args\";\n }\n+print STDERR \"test-terminal: GIT_PAGER=$ENV{GIT_PAGER}\\n\";\n+\n $ENV{TERM} = 'vt100';\n my $parent_out = new IO::Pty;\n my $parent_err = new IO::Pty;\n"},{"id":"499075","messageId":"079901fe-7889-4e1f-bb91-610e1eae25d3@gmail.com","threadId":"61768","inReplyTo":"xmqqv80xcpe5.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-22T19:06:02Z","receivedAt":"2024-07-22T19:06:05Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Jul 22, 2024 at 10:22:58AM -0700, Junio C Hamano wrote:\n \n> Attached at the end is a test tweak patch, taking inspirations from\n> Phillip's comments, to see what value GIT_PAGER has in the shell\n> function. I shortened the huge_file a bit so that I do not have to\n> have an infinite scrollback buffer,but otherwise, the test_quirk\n> intermediate shell function should work just like the test_terminal\n> helper in the original position would.\n> \n> And I see in the output from \"sh t3701-add-interactive.sh -i -v\":\n> \n> expecting success of 3701.51 'P handles SIGPIPE when writing to pager': \n> test_when_finished \"rm -f huge_file; git reset\" &&\n> printf \"\\n%250s\" Y >huge_file &&\n> git add -N huge_file &&\n> echo \"in env: GIT_PAGER=$(env | grep GIT_PAGER=)\" &&\n> test_write_lines P q | GIT_PAGER=\"head -n 1\" test_quirk &&\n> echo \"after test_quirk returns: GIT_PAGER=$GIT_PAGER\"\n> \n> in env: GIT_PAGER=\n> in test_quirk: GIT_PAGER=head -n 1\n> in env: GIT_PAGER=GIT_PAGER=head -n 1\n> In test_terminal: GIT_PAGER=GIT_PAGER=head -n 1\n> test-terminal: GIT_PAGER=head -n 1\n> diff --git a/huge_file b/huge_file\n> new file mode 100644\n> index 0000000..d06820d\n> --- /dev/null\n> +++ b/huge_file\n> @@ -0,0 +1,2 @@\n> +\n> + Y\n> \\ No newline at end of file\n> (1/1) Stage addition [y,n,q,a,d,e,p,?]? @@ -0,0 +1,2 @@\n> (1/1) Stage addition [y,n,q,a,d,e,p,?]? \n> after test_quirk returns: GIT_PAGER=\n> Unstaged changes after reset:\n> M test\n> ok 51 - P handles SIGPIPE when writing to pager\n> \n> So:\n> \n> - before the one-shot thing, in the envrionment GIT_PAGER is empty.\n> - in the helper function,\n> - shell variable GIT_PAGER is set to the expected value.\n> - GIT_PAGER env is exported.\n> - test-terminal.perl sees $ENV{GIT_PAGER} set to the expected value.\n> - after the helper returns GIT_PAGER is empty\n> \n> It's a very convincing theory but it does not seem to match my\n> observation. Is there a difference in shells used, or something?\n\nHave you tried your tweak in the \"linux-gcc (ubuntu-20.04)\" test\nenvironment where the problem was detected? In that environment, the\nvalue of GIT_PAGER is not passed to Git in that test. \n\nTo fix the test, as already said, we need this:\n\n\ttest_write_lines P q |\n\t(\n\t\tGIT_PAGER=\"head -n 1\" &&\n\t\texport GIT_PAGER &&\n\t\ttest_terminal git add -p >actual\n\t)\n\nAnd this series also need the other other change that I'm discussing\nwith Phillip: \n\ndiff --git a/pager.c b/pager.c\nindex 5f0c1e9cce..5586e751dc 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)\n\n void wait_for_pager(void)\n {\n+ if (old_fd1 == -1)\n+ return;\n finish_pager();\n sigchain_pop_common();\n unsetenv(\"GIT_PAGER_IN_USE\");\n\n"},{"id":"499077","messageId":"xmqqa5i9b51m.fsf@gitster.g","threadId":"61768","inReplyTo":"079901fe-7889-4e1f-bb91-610e1eae25d3@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T19:27:49Z","receivedAt":"2024-07-22T19:27:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n>> It's a very convincing theory but it does not seem to match my\n>> observation. Is there a difference in shells used, or something?\n>\n> Have you tried your tweak in the \"linux-gcc (ubuntu-20.04)\" test\n> environment where the problem was detected? In that environment, the\n> value of GIT_PAGER is not passed to Git in that test. \n\nSo, we may have a shell that does not behave like others ;-) Do you\nknow what shell is being used?\n\nThanks.\n"},{"id":"499083","messageId":"xmqqbk2p9lwi.fsf_-_@gitster.g","threadId":"61768","inReplyTo":"xmqqa5i9b51m.fsf@gitster.g","subject":"Re* [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T21:06:37Z","receivedAt":"2024-07-22T21:06:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Rubén Justo writes:\n>\n>>> It's a very convincing theory but it does not seem to match my\n>>> observation. Is there a difference in shells used, or something?\n>>\n>> Have you tried your tweak in the \"linux-gcc (ubuntu-20.04)\" test\n>> environment where the problem was detected? In that environment, the\n>> value of GIT_PAGER is not passed to Git in that test. \n>\n> So, we may have a shell that does not behave like others ;-) Do you\n> know what shell is being used?\n\nSo we have an answer:\n\n https://github.com/git/git/actions/runs/10047627546/job/27769808515\n\ntells us that the problematic shell is used in the job.\n\nIt is\n\nii dash 0.5.10.2-6 amd64 POSIX-compliant shell\n\nrunning on Ubuntu 20.04 that is \"too POSIXly correct\"[*] and behaves\ndifferently from what the tests expect.\n\nSomebody should write this combination down somewhere in the\ndocumentation so that we can answer (better yet, we do not have to\nanswer) when somebody wonders if we know of a version of shell that\nrefuses to do an one-shot export for shell functions as we naïvely\nexpect.\n\n\n[Reference]\n\n * https://lore.kernel.org/git/4B5027B8.2090507@viscovery.net/\n\n\n----- >8 --------- >8 --------- >8 --------- >8 ----\nCodingGuidelines: give an example shell that \"fails\" \"VAR=VAL shell_func\"\n\nOver the years, we accumulated the community wisdom to avoid the\ncommon \"one-short export\" construct for shell functions, but seem to\nhave lost on which exact platform it is known to fail. Now during\nan investigation on a breakage for a recent topic, let's document\none example of failing shell.\n\nThis does *not* mean that we can freely start using the construct\nonce Ubuntu 20.04 is retired. But it does mean that we cannot use\nthe construct until Ubuntu 20.04 is fully retired from the machines\nthat matter.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/CodingGuidelines | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines\nindex 1d92b2da03..a3ecb4ac5a 100644\n--- c/Documentation/CodingGuidelines\n+++ w/Documentation/CodingGuidelines\n@@ -204,6 +204,29 @@ For shell scripts specifically (not exhaustive):\n \tlocal variable=\"$value\"\n \tlocal variable=\"$(command args)\"\n \n+ - The common construct\n+\n+\tVAR=VAL command args\n+\n+ to temporarily set and export environment variable VAR only while\n+ \"command args\" is running is handy, but some versions of dash (like\n+ 0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment\n+ without exporting the variable, when command is *not* an external\n+ command. We often have to resort to subshell with explicit export,\n+ i.e.\n+\n+\t(incorrect)\n+\tVAR=VAL func args\n+\n+\t(correct)\n+\t(\n+\t\tVAR=VAL && export VAR &&\n+\t\tfunc args\n+\t)\n+\n+ but be careful that the effect \"func\" makes to the variables in the\n+ current shell will be lost across the subshell boundary.\n+\n - Use octal escape sequences (e.g. \"\\302\\242\"), not hexadecimal (e.g.\n \"\\xc2\\xa2\") in printf format strings, since hexadecimal escape\n sequences are not portable.\n"},{"id":"499084","messageId":"xmqq7cdd9l0m.fsf@gitster.g","threadId":"61768","inReplyTo":"079901fe-7889-4e1f-bb91-610e1eae25d3@gmail.com","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T21:25:45Z","receivedAt":"2024-07-22T21:25:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> To fix the test, as already said, we need this:\n>\n> \ttest_write_lines P q |\n> \t(\n> \t\tGIT_PAGER=\"head -n 1\" &&\n> \t\texport GIT_PAGER &&\n> \t\ttest_terminal git add -p >actual\n> \t)\n\nThis took sufficiently large amount of collective braincycles, and\nit would be worth documenting as a separate patch, I would suspect.\n\nSomething along the following lines, but please take the authorship\n*and* give it a better explanation.\n\nThanks.\n\n--- >8 ---\nSubject: [PATCH] t3701: avoid one-shot export for shell functions\n\nThe common construct\n\n VAR=VAL command args\n\nto temporarily set and export environment variable VAR only while\n\"command args\" is running is handy, but one of our CI jobs on GitHub\nActions uses Ubuntu 20.04 running dash 0.5.10.2-6 failed with the\nconstruct, making only a temporary assignment without exporting the\nvariable, when command is *not* an external (in this case, a shell\nfunction).\n\nThe \"git add -p\" being tested did not get our custom GIT_PAGER,\nwhich broke the test.\n\nWork it around by explicitly exporting the variable in a subshell.\n\nSigned-off-by: Junio C Hamano \n---\n t/t3701-add-interactive.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex c60589cb94..1b8617e0c1 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -616,7 +616,11 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n \ttest_when_finished \"rm -f huge_file; git reset\" &&\n \tprintf \"\\n%2500000s\" Y >huge_file &&\n \tgit add -N huge_file &&\n-\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n+\ttest_write_lines P q | (\n+\t\tGIT_PAGER=\"head -n 1\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p\n+\t)\n '\n \n test_expect_success 'split hunk \"add -p (edit)\"' '\n-- \n2.46.0-rc1-48-g0900f1888e\n\n"},{"id":"499088","messageId":"0385c0cc-fb3e-4d12-9655-a409aea22c38@gmail.com","threadId":"61768","inReplyTo":"xmqqbk2p9lwi.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-22T22:00:10Z","receivedAt":"2024-07-22T22:00:14Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Jul 22, 2024 at 02:06:37PM -0700, Junio C Hamano wrote:\n\n> So we have an answer:\n> \n> https://github.com/git/git/actions/runs/10047627546/job/27769808515\n> \n> tells us that the problematic shell is used in the job.\n> \n> It is\n> \n> ii dash 0.5.10.2-6 amd64 POSIX-compliant shell\n> \n> running on Ubuntu 20.04 that is \"too POSIXly correct\"[*] and behaves\n> differently from what the tests expect.\n> \n> Somebody should write this combination down somewhere in the\n> documentation so that we can answer (better yet, we do not have to\n> answer) when somebody wonders if we know of a version of shell that\n> refuses to do an one-shot export for shell functions as we naïvely\n> expect.\n> \n> \n> [Reference]\n> \n> * https://lore.kernel.org/git/4B5027B8.2090507@viscovery.net/\n> \n> \n> ----- >8 --------- >8 --------- >8 --------- >8 ----\n> CodingGuidelines: give an example shell that \"fails\" \"VAR=VAL shell_func\"\n> \n> Over the years, we accumulated the community wisdom to avoid the\n> common \"one-short export\" construct for shell functions, but seem to\n> have lost on which exact platform it is known to fail. Now during\n> an investigation on a breakage for a recent topic, let's document\n> one example of failing shell.\n> \n> This does *not* mean that we can freely start using the construct\n> once Ubuntu 20.04 is retired. But it does mean that we cannot use\n> the construct until Ubuntu 20.04 is fully retired from the machines\n> that matter.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/CodingGuidelines | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n> \n> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines\n> index 1d92b2da03..a3ecb4ac5a 100644\n> --- c/Documentation/CodingGuidelines\n> +++ w/Documentation/CodingGuidelines\n> @@ -204,6 +204,29 @@ For shell scripts specifically (not exhaustive):\n> \tlocal variable=\"$value\"\n> \tlocal variable=\"$(command args)\"\n> \n> + - The common construct\n> +\n> +\tVAR=VAL command args\n> +\n> + to temporarily set and export environment variable VAR only while\n> + \"command args\" is running is handy, but some versions of dash (like\n> + 0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment\n> + without exporting the variable, when command is *not* an external\n> + command. We often have to resort to subshell with explicit export,\n> + i.e.\n> +\n> +\t(incorrect)\n> +\tVAR=VAL func args\n> +\n> +\t(correct)\n> +\t(\n> +\t\tVAR=VAL && export VAR &&\n> +\t\tfunc args\n\nJust a small comment; maybe it's worth adding an extra line to make the\nexample clearer: \n\n\t\tVAR=VAL &&\n\t\texport VAR &&\n\t\tfunc args\n\n> +\t)\n> +\n> + but be careful that the effect \"func\" makes to the variables in the\n> + current shell will be lost across the subshell boundary.\n> +\n> - Use octal escape sequences (e.g. \"\\302\\242\"), not hexadecimal (e.g.\n> \"\\xc2\\xa2\") in printf format strings, since hexadecimal escape\n> sequences are not portable.\n\nThank you for digging into the test to find an explanation and for\nadding the comment to the documentation.\n"},{"id":"499096","messageId":"CAO_smVhd4LY_F0Wgt1CfsidFAB1n_8Rv3sXaBCgrCuOVMxS5cw@mail.gmail.com","threadId":"61768","inReplyTo":"xmqqbk2p9lwi.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-07-22T23:12:21Z","receivedAt":"2024-07-22T23:12:36Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Mon, Jul 22, 2024 at 2:06 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Rubén Justo writes:\n> >\n> >>> It's a very convincing theory but it does not seem to match my\n> >>> observation. Is there a difference in shells used, or something?\n> >>\n> >> Have you tried your tweak in the \"linux-gcc (ubuntu-20.04)\" test\n> >> environment where the problem was detected? In that environment, the\n> >> value of GIT_PAGER is not passed to Git in that test.\n> >\n> > So, we may have a shell that does not behave like others ;-) Do you\n> > know what shell is being used?\n>\n> So we have an answer:\n>\n> https://github.com/git/git/actions/runs/10047627546/job/27769808515\n>\n> tells us that the problematic shell is used in the job.\n>\n> It is\n>\n> ii dash 0.5.10.2-6 amd64 POSIX-compliant shell\n>\n> running on Ubuntu 20.04 that is \"too POSIXly correct\"[*] and behaves\n> differently from what the tests expect.\n>\n> Somebody should write this combination down somewhere in the\n> documentation so that we can answer (better yet, we do not have to\n> answer) when somebody wonders if we know of a version of shell that\n> refuses to do an one-shot export for shell functions as we naïvely\n> expect.\n>\n>\n> [Reference]\n>\n> * https://lore.kernel.org/git/4B5027B8.2090507@viscovery.net/\n>\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 ----\n> CodingGuidelines: give an example shell that \"fails\" \"VAR=VAL shell_func\"\n>\n> Over the years, we accumulated the community wisdom to avoid the\n> common \"one-short export\" construct for shell functions, but seem to\n> have lost on which exact platform it is known to fail. Now during\n> an investigation on a breakage for a recent topic, let's document\n> one example of failing shell.\n>\n> This does *not* mean that we can freely start using the construct\n> once Ubuntu 20.04 is retired. But it does mean that we cannot use\n> the construct until Ubuntu 20.04 is fully retired from the machines\n> that matter.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/CodingGuidelines | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n>\n> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines\n> index 1d92b2da03..a3ecb4ac5a 100644\n> --- c/Documentation/CodingGuidelines\n> +++ w/Documentation/CodingGuidelines\n> @@ -204,6 +204,29 @@ For shell scripts specifically (not exhaustive):\n> local variable=\"$value\"\n> local variable=\"$(command args)\"\n>\n> + - The common construct\n> +\n> + VAR=VAL command args\n> +\n> + to temporarily set and export environment variable VAR only while\n> + \"command args\" is running is handy, but some versions of dash (like\n> + 0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment\n\nI was also able to reproduce both aspects of this behavior (doesn't\nexport, value is retained) with ksh (sh (AT&T Research) 93u+m/1.0.8\n2024-01-01), which is the current version on debian testing. So maybe\n\"some versions of ksh (tested: 93u+m/1.0.8 2024-01-01) and dash\n(0.5.10.2-6)\"? Or maybe we move the 'some versions' around, because I\nthink it's probably all versions of ksh :)\n\nI don't know how easily discoverable this is, though. I think I'd\nstill want some linkage between t/check-non-portable-shell.pl and this\nsection of this file? I probably wouldn't think to look here if I\nreceived that error from the check-non-portable-shell.pl linter.\n\nOtherwise, looks good.\n\n> + without exporting the variable, when command is *not* an external\n> + command. We often have to resort to subshell with explicit export,\n> + i.e.\n> +\n> + (incorrect)\n> + VAR=VAL func args\n> +\n> + (correct)\n> + (\n> + VAR=VAL && export VAR &&\n> + func args\n> + )\n> +\n> + but be careful that the effect \"func\" makes to the variables in the\n> + current shell will be lost across the subshell boundary.\n> +\n> - Use octal escape sequences (e.g. \"\\302\\242\"), not hexadecimal (e.g.\n> \"\\xc2\\xa2\") in printf format strings, since hexadecimal escape\n> sequences are not portable.\n"},{"id":"499098","messageId":"43e045e5-4c92-4c5f-b183-d63c5b510023@gmail.com","threadId":"61768","inReplyTo":"xmqq7cdd9l0m.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-22T23:20:56Z","receivedAt":"2024-07-22T23:20:59Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Jul 22, 2024 at 02:25:45PM -0700, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > To fix the test, as already said, we need this:\n> >\n> > \ttest_write_lines P q |\n> > \t(\n> > \t\tGIT_PAGER=\"head -n 1\" &&\n> > \t\texport GIT_PAGER &&\n> > \t\ttest_terminal git add -p >actual\n> > \t)\n> \n> This took sufficiently large amount of collective braincycles, and\n> it would be worth documenting as a separate patch, I would suspect.\n> \n> Something along the following lines, but please take the authorship\n> *and* give it a better explanation.\n\nHere's an attempt. \n\nI'm also adding the change for `wait_for_pager`, which could be squashed\nin b29c59e3d2 (pager: introduce wait_for_pager, 2024-07-16). Although,\nhighlighted I think it's interesting as well. But I don't have a strong\npreference.\n\nThis builds on rj/add-p-pager.\n\nThanks. \n\nRubén Justo (2):\n t3701: avoid one-shot export for shell functions\n pager: make wait_for_pager a no-op for \"cat\"\n\n pager.c | 3 +++\n t/t3701-add-interactive.sh | 6 +++++-\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\n\nbase-commit: 6bc52a5543008bff2c6ec7a0a935c7fc1f79e646\n-- \n2.45.1\n"},{"id":"499099","messageId":"5536b336-5122-47fd-be57-42c299abe60c@gmail.com","threadId":"61768","inReplyTo":"43e045e5-4c92-4c5f-b183-d63c5b510023@gmail.com","subject":"[PATCH 1/2] t3701: avoid one-shot export for shell functions","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-22T23:24:20Z","receivedAt":"2024-07-22T23:24:23Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The common construct:\n\n VAR=VAL command args\n\nit's a common way to define one-shot variables within the scope of\nexecuting a \"command\".\n\nHowever, when \"command\" is a function which in turn executes the\n\"command\", the behavior varies depending on the shell:\n\n ** Bash 5.2.21 **\n\n $ f () { bash -c 'echo A=$A'; }\n $ A=1 f\n A=1\n\n ** dash 0.5.12-9 **\n\n $ f () { bash -c 'echo A=$A'; }\n $ A=1 f\n A=1\n\n ** dash 0.5.10.2-6 **\n\n $ f () { bash -c 'echo A=$A'; }\n $ A=1 f\n A=\n\nOne of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n0.5.10.2-6, so we failed the test t3701:51; the \"git add -p\" being\ntested did not get our custom GIT_PAGER, which broke the test.\n\nWork it around by explicitly exporting the variable in a subshell.\n\nSigned-off-by: Rubén Justo \n---\n t/t3701-add-interactive.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex c60589cb94..1b8617e0c1 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -616,7 +616,11 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n \ttest_when_finished \"rm -f huge_file; git reset\" &&\n \tprintf \"\\n%2500000s\" Y >huge_file &&\n \tgit add -N huge_file &&\n-\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n+\ttest_write_lines P q | (\n+\t\tGIT_PAGER=\"head -n 1\" &&\n+\t\texport GIT_PAGER &&\n+\t\ttest_terminal git add -p\n+\t)\n '\n \n test_expect_success 'split hunk \"add -p (edit)\"' '\n-- \n2.45.1\n"},{"id":"499100","messageId":"c37f0d54-4ead-422c-8193-f0c2ec84ca4a@gmail.com","threadId":"61768","inReplyTo":"43e045e5-4c92-4c5f-b183-d63c5b510023@gmail.com","subject":"[PATCH 2/2] pager: make wait_for_pager a no-op for \"cat\"","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-07-22T23:24:45Z","receivedAt":"2024-07-22T23:24:47Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"If we find that the configured pager is an empty string [*1*] or simply\n\"cat\" [*2*], then we return from `setup_pager()` silently without doing\nanything, allowing the output to go directly to the normal stdout.\n\nLet's make the call to `wait_for_pager()` for these cases, or any other\nfuture optimizations that may occur, also exit silently without doing\nanything.\n\n 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n 2006-04-16)\n\n 2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)\n\nSigned-off-by: Rubén Justo \n---\n pager.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/pager.c b/pager.c\nindex bea4345f6f..896f40fcd2 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -46,6 +46,9 @@ static void wait_for_pager_atexit(void)\n \n void wait_for_pager(void)\n {\n+\tif (old_fd1 == -1)\n+\t\treturn;\n+\n \tfinish_pager();\n \tsigchain_pop_common();\n \tunsetenv(\"GIT_PAGER_IN_USE\");\n-- \n2.45.1\n"},{"id":"499101","messageId":"xmqq8qxt80r4.fsf@gitster.g","threadId":"61768","inReplyTo":"CAO_smVhd4LY_F0Wgt1CfsidFAB1n_8Rv3sXaBCgrCuOVMxS5cw@mail.gmail.com","subject":"Re: Re* [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T23:28:47Z","receivedAt":"2024-07-22T23:28:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott writes:\n\n> I don't know how easily discoverable this is, though. I think I'd\n> still want some linkage between t/check-non-portable-shell.pl and this\n> section of this file?\n\nPatches welcome. I personally think CodingGuidelines would be more\npromiment source of information than a linter script, and comments\nin the lint script cannot grow too elaborate, so that is the reason\nwhy I did this patch first. Once we have something to refer to in a\nsection in an authoritative and canonical document, it should be\neasy to point at the section from other places, like the linter\nscript.\n\nThanks.\n"},{"id":"499103","messageId":"xmqqsew16lg9.fsf@gitster.g","threadId":"61768","inReplyTo":"5536b336-5122-47fd-be57-42c299abe60c@gmail.com","subject":"Re: [PATCH 1/2] t3701: avoid one-shot export for shell functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T23:44:38Z","receivedAt":"2024-07-22T23:44:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> The common construct:\n>\n> VAR=VAL command args\n>\n> it's a common way to define one-shot variables within the scope of\n> executing a \"command\".\n\n\"it's a\" -> \"is a\".\n\"define\" -> \"set and export\".\n\n> However, when \"command\" is a function which in turn executes the\n> \"command\", the behavior varies depending on the shell:\n>\n> ** Bash 5.2.21 **\n>\n> $ f () { bash -c 'echo A=$A'; }\n> $ A=1 f\n> A=1\n>\n> ** dash 0.5.12-9 **\n>\n> $ f () { bash -c 'echo A=$A'; }\n> $ A=1 f\n> A=1\n>\n> ** dash 0.5.10.2-6 **\n>\n> $ f () { bash -c 'echo A=$A'; }\n> $ A=1 f\n> A=\n>\n> One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n> 0.5.10.2-6, so we failed the test t3701:51; the \"git add -p\" being\n> tested did not get our custom GIT_PAGER, which broke the test.\n>\n> Work it around by explicitly exporting the variable in a subshell.\n\nNicely described.\n\n>\n> Signed-off-by: Rubén Justo \n> ---\n> t/t3701-add-interactive.sh | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index c60589cb94..1b8617e0c1 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -616,7 +616,11 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '\n> \ttest_when_finished \"rm -f huge_file; git reset\" &&\n> \tprintf \"\\n%2500000s\" Y >huge_file &&\n> \tgit add -N huge_file &&\n> -\ttest_write_lines P q | GIT_PAGER=\"head -n 1\" test_terminal git add -p\n> +\ttest_write_lines P q | (\n> +\t\tGIT_PAGER=\"head -n 1\" &&\n> +\t\texport GIT_PAGER &&\n> +\t\ttest_terminal git add -p\n> +\t)\n> '\n> \n> test_expect_success 'split hunk \"add -p (edit)\"' '\n"},{"id":"499104","messageId":"xmqqikwx6l01.fsf@gitster.g","threadId":"61768","inReplyTo":"c37f0d54-4ead-422c-8193-f0c2ec84ca4a@gmail.com","subject":"Re: [PATCH 2/2] pager: make wait_for_pager a no-op for \"cat\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T23:54:22Z","receivedAt":"2024-07-22T23:54:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> If we find that the configured pager is an empty string [*1*] or simply\n> \"cat\" [*2*], then we return from `setup_pager()` silently without doing\n> anything, allowing the output to go directly to the normal stdout.\n\nI'm tempted to suggest inserting two extra paragraphs here to avoid\ntoo big a leap in logic flow.\n\n Even though the caller may properly make matching calls to\n setup_pager() and wait_for_pager(), setup_pager() may return early\n without doing much, and the call to wait_for_pager() would segfault.\n\n This condition can be detected by old_fd1 being -1 (not modified in\n setup_pager())\n\n> Let's make the call to `wait_for_pager()` for these cases, or any other\n> future optimizations that may occur, also exit silently without doing\n> anything.\n>\n> 1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,\n> 2006-04-16)\n>\n> 2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)\n>\n> Signed-off-by: Rubén Justo \n> ---\n\nI am not 100% sure about the \"would segfault\", but we'd need to be\nexplicit about what badness it causes to call wait_for_pager()\nwithout starting a pager. Other than that, well explained.\n\nThanks.\n\n> pager.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/pager.c b/pager.c\n> index bea4345f6f..896f40fcd2 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -46,6 +46,9 @@ static void wait_for_pager_atexit(void)\n> \n> void wait_for_pager(void)\n> {\n> +\tif (old_fd1 == -1)\n> +\t\treturn;\n> +\n> \tfinish_pager();\n> \tsigchain_pop_common();\n> \tunsetenv(\"GIT_PAGER_IN_USE\");\n"},{"id":"499105","messageId":"xmqqa5i96kvg.fsf@gitster.g","threadId":"61768","inReplyTo":"xmqqsew16lg9.fsf@gitster.g","subject":"Re: [PATCH 1/2] t3701: avoid one-shot export for shell functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-22T23:57:07Z","receivedAt":"2024-07-22T23:57:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Rubén Justo writes:\n>\n>> The common construct:\n>>\n>> VAR=VAL command args\n>>\n>> it's a common way to define one-shot variables within the scope of\n>> executing a \"command\".\n>\n> \"it's a\" -> \"is a\".\n> \"define\" -> \"set and export\".\n>\n>> However, when \"command\" is a function which in turn executes the\n>> \"command\", the behavior varies depending on the shell:\n>>\n>> ** Bash 5.2.21 **\n>>\n>> $ f () { bash -c 'echo A=$A'; }\n>> $ A=1 f\n>> A=1\n>>\n>> ** dash 0.5.12-9 **\n>>\n>> $ f () { bash -c 'echo A=$A'; }\n>> $ A=1 f\n>> A=1\n>>\n>> ** dash 0.5.10.2-6 **\n>>\n>> $ f () { bash -c 'echo A=$A'; }\n>> $ A=1 f\n>> A=\n\nAnother thing. Let's insert a paragraph perhaps like this here.\n\n Note that POSIX explicitly says the effect of this construct\n used on a shell function is unspecified.\n\n>> One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash\n>> 0.5.10.2-6, so we failed the test t3701:51; the \"git add -p\" being\n>> tested did not get our custom GIT_PAGER, which broke the test.\n>>\n>> Work it around by explicitly exporting the variable in a subshell.\n\nThat way, we won't give a wrong impression that we can safely start\nusing the construct in future once dash 0.5.10.2-6 goes away.\n\n"},{"id":"499119","messageId":"xmqq4j8g6dqv.fsf@gitster.g","threadId":"61768","inReplyTo":"CAO_smVhd4LY_F0Wgt1CfsidFAB1n_8Rv3sXaBCgrCuOVMxS5cw@mail.gmail.com","subject":"Re: Re* [PATCH v3 4/4] add-patch: render hunks through the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-23T02:31:04Z","receivedAt":"2024-07-23T02:31:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott writes:\n\n> I was also able to reproduce both aspects of this behavior (doesn't\n> export, value is retained) with ksh (sh (AT&T Research) 93u+m/1.0.8\n> 2024-01-01), which is the current version on debian testing. So maybe\n> \"some versions of ksh (tested: 93u+m/1.0.8 2024-01-01) and dash\n> (0.5.10.2-6)\"? Or maybe we move the 'some versions' around, because I\n> think it's probably all versions of ksh :)\n\nMakes sense, but I think \"POSIX guarantees that the behaviour is\nsomething you should not rely on by telling us that these are\nunspecified\", which you found, is a much better rationale to\nexplicitly forbid \"VAR=VAL shell_func\" construct.\n\nBesides, as another thread recently discussed, our test scripts,\nwith really heavy uses of \"local\", do not work at all with AT&T ksh\n(other ksh clones are reported to be OK, though). So it may be OK\nto write it off as \"unusuable to run our tests\", at least for now.\n"}]}