[PATCH 0/4] use the pager in 'add -p'

[PATCH 0/4] use the pager in 'add -p'

[Rubén Justo]

I'm resuming work on introducing a mechanism to use the PAGER to display
hunks during interactive "git add -p" sessions, which will make it
easier to review large hunks.

The thread where the previous discussion took place is:
https://lore.kernel.org/git/1d0cb55c-5f32-419a-b593-d5f0969a51fd@gmail.com/

I'm bringing back the proposal to introduce 'P' as a mechanism to
display the current hunks through the PAGER.

I think it's sensible to exclude from the scope of this series the
option of a new command '|[cmd]' and other modifications to the original
proposal that have raised questions that perhaps deserve their own
discussion, outside the scope of this series.  Questions like:

   - What to do with ANSI codes?
   - How to allow the definition of a default command?
   - How to facilitate the reuse of a command?
   - How to combine a default command with command reuse?
   - What to do if the command fails?

To mention a few...

I'm also leaving for a future series a possible configuration
"interactive.pipeCommand", "interactive.pager" or similar.

I hope this approach makes sense and allows us to move forward, and that
it doesn't represent a step back.

Thanks.

Rubén Justo (4):
  add-patch: test for 'p' command
  pager: do not close fd 2 unnecessarily
  pager: introduce wait_for_pager
  add-patch: render hunks through the pager

 add-patch.c                | 18 ++++++++++--
 pager.c                    | 45 ++++++++++++++++++++++++----
 pager.h                    |  1 +
 t/t3701-add-interactive.sh | 60 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 116 insertions(+), 8 deletions(-)

-- 
2.45.1

[PATCH 1/4] add-patch: test for 'p' command

[Rubén Justo]

Add a test for the 'p' command, which was introduced in 66c14ab592
(add-patch: introduce 'p' in interactive-patch, 2024-03-29).

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t3701-add-interactive.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5d78868ac1..6daf3a6be0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -575,6 +575,22 @@ test_expect_success 'navigate to hunk via regex / pattern' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success 'print again the hunk' '
+	test_when_finished "git reset" &&
+	tr _ " " >expect <<-EOF &&
+	+15
+	 20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	 10
+	+15
+	 20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	EOF
+	test_write_lines s y g 1 p | git add -p >actual &&
+	tail -n 7 <actual >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.45.1

[PATCH 2/4] pager: do not close fd 2 unnecessarily

[Rubén Justo]

We send errors to the pager since 61b80509e3 (sending errors to stdout
under $PAGER, 2008-02-16).

In a8335024c2 (pager: do not dup2 stderr if it is already redirected,
2008-12-15) an exception was introduced to avoid redirecting stderr if
it is not connected to a terminal.

In such exceptional cases, the close(STDERR_FILENO) we're doing in
close_pager_fds, is unnecessary.

Furthermore, in a subsequent commit we're going to introduce changes
that will involve using close_pager_fds multiple times.

With this in mind, controlling when we want to close stderr, become
sensible.

Let's close(STDERR_FILENO) only when necessary, and pave the way for the
upcoming changes.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/pager.c b/pager.c
index be6f4ee59f..251adfc2ad 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
+static int close_fd2;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -23,7 +24,8 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	close(2);
+	if (close_fd2)
+		close(2);
 }
 
 static void wait_for_pager_atexit(void)
@@ -141,8 +143,10 @@ void setup_pager(void)
 
 	/* original process continues, but writes to the pipe */
 	dup2(pager_process.in, 1);
-	if (isatty(2))
+	if (isatty(2)) {
+		close_fd2 = 1;
 		dup2(pager_process.in, 2);
+	}
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
2.45.1

[PATCH 3/4] pager: introduce wait_for_pager

[Rubén Justo]

Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
2006-02-28) we have the machinery to send our output to a pager.

That machinery, once set up, does not allow us to regain the original
stdio streams.

In the interactive commands (i.e.: add -p) we want to use the pager for
some output, while maintaining the interaction with the user.

Modify the pager machinery so that we can use setup_pager and, once
we've finished sending the desired output for the pager, wait for the
pager termination using a new function wait_for_pager.   Make this
function reset the pager machinery before returning.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 43 +++++++++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 251adfc2ad..bea4345f6f 100644
--- a/pager.c
+++ b/pager.c
@@ -14,7 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
-static int close_fd2;
+static int old_fd1 = -1, old_fd2 = -1;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -24,11 +24,11 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	if (close_fd2)
+	if (old_fd2 != -1)
 		close(2);
 }
 
-static void wait_for_pager_atexit(void)
+static void finish_pager(void)
 {
 	fflush(stdout);
 	fflush(stderr);
@@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)
 	finish_command(&pager_process);
 }
 
+static void wait_for_pager_atexit(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	finish_pager();
+}
+
+void wait_for_pager(void)
+{
+	finish_pager();
+	sigchain_pop_common();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	close(old_fd1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		close(old_fd2);
+		old_fd2 = -1;
+	}
+}
+
 static void wait_for_pager_signal(int signo)
 {
+	if (old_fd1 == -1)
+		return;
+
 	close_pager_fds();
 	finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
@@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
 
 void setup_pager(void)
 {
+	static int once = 0;
 	const char *pager = git_pager(isatty(1));
 
 	if (!pager)
@@ -142,16 +169,20 @@ void setup_pager(void)
 		die("unable to execute pager '%s'", pager);
 
 	/* original process continues, but writes to the pipe */
+	old_fd1 = dup(1);
 	dup2(pager_process.in, 1);
 	if (isatty(2)) {
-		close_fd2 = 1;
+		old_fd2 = dup(2);
 		dup2(pager_process.in, 2);
 	}
 	close(pager_process.in);
 
-	/* this makes sure that the parent terminates after the pager */
 	sigchain_push_common(wait_for_pager_signal);
-	atexit(wait_for_pager_atexit);
+
+	if (!once) {
+		once++;
+		atexit(wait_for_pager_atexit);
+	}
 }
 
 int pager_in_use(void)
diff --git a/pager.h b/pager.h
index b77433026d..103ecac476 100644
--- a/pager.h
+++ b/pager.h
@@ -5,6 +5,7 @@ struct child_process;
 
 const char *git_pager(int stdout_is_tty);
 void setup_pager(void);
+void wait_for_pager(void);
 int pager_in_use(void);
 int term_columns(void);
 void term_clear_line(void);
-- 
2.45.1

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

[Rubén Justo]

Make the print command to trigger the pager when invoked using a capital
'P', to make it easier for the user to review long hunks.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c                | 18 +++++++++++++---
 t/t3701-add-interactive.sh | 44 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 6e176cd21a..f2c76b7d83 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -7,9 +7,11 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "pager.h"
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "strbuf.h"
+#include "sigchain.h"
 #include "run-command.h"
 #include "strvec.h"
 #include "pathspec.h"
@@ -1391,7 +1393,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
    "e - manually edit the current hunk\n"
-   "p - print the current hunk\n"
+   "p - print the current hunk, 'P' to use the pager\n"
    "? - print help\n");
 
 static int patch_update_file(struct add_p_state *s,
@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	int colored = !!s->colored.len, quit = 0;
+	int colored = !!s->colored.len, quit = 0, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
 	enum {
 		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
+				if (use_pager) {
+					setup_pager();
+					sigchain_push(SIGPIPE, SIG_IGN);
+				}
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
+				if (use_pager) {
+					sigchain_pop(SIGPIPE);
+					wait_for_pager();
+					use_pager = 0;
+				}
 			}
 
 			strbuf_reset(&s->buf);
@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = USE_HUNK;
 				goto soft_increment;
 			}
-		} else if (s->answer.buf[0] == 'p') {
+		} else if (ch == 'p') {
 			rendered_hunk_index = -1;
+			use_pager = (s->answer.buf[0] == 'P') ? 1 : 0;
 		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 6daf3a6be0..bf82a9dc35 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -591,6 +591,50 @@ test_expect_success 'print again the hunk' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success TTY 'print again the hunk (PAGER)' '
+	test_when_finished "git reset" &&
+	cat >expect <<-EOF &&
+	<GREEN>+<RESET><GREEN>15<RESET>
+	 20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
+	PAGER  10<RESET>
+	PAGER <GREEN>+<RESET><GREEN>15<RESET>
+	PAGER  20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	EOF
+	test_write_lines s y g 1 P |
+	(
+		GIT_PAGER="sed s/^/PAGER\ /" &&
+		export GIT_PAGER &&
+		test_terminal git add -p >actual
+	) &&
+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
+test_expect_success TTY 'P does not break if pager ends unexpectly' '
+	test_when_finished "rm -f huge_file; git reset" &&
+	printf "%2500000s" Y >huge_file &&
+	git add -N huge_file &&
+	cat >expect <<-EOF &&
+	<GREEN>+<RESET><GREEN>22<RESET>
+	<GREEN>+<RESET><GREEN>23<RESET>
+	<GREEN>+<RESET><GREEN>24<RESET>
+	 30<RESET>
+	 40<RESET>
+	 50<RESET>
+	<BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? <RESET>
+	EOF
+	test_write_lines P |
+	(
+		GIT_PAGER="head -1" &&
+		export GIT_PAGER &&
+		test_terminal git add -p >actual
+	) &&
+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.45.1

Re: [PATCH 0/4] use the pager in 'add -p'

[Dragan Simic]

Hello Ruben,

On 2024-07-12 02:57, Rubén Justo wrote:
> I'm resuming work on introducing a mechanism to use the PAGER to 
> display
> hunks during interactive "git add -p" sessions, which will make it
> easier to review large hunks.
> 
> The thread where the previous discussion took place is:
> https://lore.kernel.org/git/1d0cb55c-5f32-419a-b593-d5f0969a51fd@gmail.com/
> 
> I'm bringing back the proposal to introduce 'P' as a mechanism to
> display the current hunks through the PAGER.
> 
> I think it's sensible to exclude from the scope of this series the
> option of a new command '|[cmd]' and other modifications to the 
> original
> proposal that have raised questions that perhaps deserve their own
> discussion, outside the scope of this series.  Questions like:
> 
>    - What to do with ANSI codes?
>    - How to allow the definition of a default command?
>    - How to facilitate the reuse of a command?
>    - How to combine a default command with command reuse?
>    - What to do if the command fails?
> 
> To mention a few...
> 
> I'm also leaving for a future series a possible configuration
> "interactive.pipeCommand", "interactive.pager" or similar.
> 
> I hope this approach makes sense and allows us to move forward, and 
> that
> it doesn't represent a step back.

I find this approach fine.  It would allow us to have this neat feature
available in its initial, simplified form, while the future improvements
would belong to follow-up discussions and patches.

> Rubén Justo (4):
>   add-patch: test for 'p' command
>   pager: do not close fd 2 unnecessarily
>   pager: introduce wait_for_pager
>   add-patch: render hunks through the pager
> 
>  add-patch.c                | 18 ++++++++++--
>  pager.c                    | 45 ++++++++++++++++++++++++----
>  pager.h                    |  1 +
>  t/t3701-add-interactive.sh | 60 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 116 insertions(+), 8 deletions(-)

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

[Dragan Simic]

On 2024-07-12 03:00, Rubén Justo wrote:
> @@ -1391,7 +1393,7 @@ N_("j - leave this hunk undecided, see next
> undecided hunk\n"
>     "/ - search for a hunk matching the given regex\n"
>     "s - split the current hunk into smaller hunks\n"
>     "e - manually edit the current hunk\n"
> -   "p - print the current hunk\n"
> +   "p - print the current hunk, 'P' to use the pager\n"
>     "? - print help\n");

I'm fine with this compact form, even though it diverges from
the "one command per help line" rule.

Re: [PATCH 3/4] pager: introduce wait_for_pager

[Phillip Wood]

Hi Rubén

On 12/07/2024 02:00, Rubén Justo wrote:
> Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
> 2006-02-28) we have the machinery to send our output to a pager.
> 
> That machinery, once set up, does not allow us to regain the original
> stdio streams.
> 
> In the interactive commands (i.e.: add -p) we want to use the pager for
> some output, while maintaining the interaction with the user.
> 
> Modify the pager machinery so that we can use setup_pager and, once
> we've finished sending the desired output for the pager, wait for the
> pager termination using a new function wait_for_pager.   Make this
> function reset the pager machinery before returning.

This looks good and addresses my previous comments about leaking file 
descriptors and restoring signal dispositions. A couple of thoughts 
occurred to me as I was reading it again:

  - We ignore any errors when duplicating fds,
    "git grep '[^a-z_]dup2\{0,1\}(' shows that's not unusual in our
    code base, though if we cannot redirect the output to the pager or
    restore stdout when the pager exits that's a problem for "git add -p"

  - We should perhaps be marking old_fd[12] with O_CLOEXEC to stop them
    being passed to the pager.

Best Wishes

Phillip

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   pager.c | 43 +++++++++++++++++++++++++++++++++++++------
>   pager.h |  1 +
>   2 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/pager.c b/pager.c
> index 251adfc2ad..bea4345f6f 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -14,7 +14,7 @@ int pager_use_color = 1;
>   
>   static struct child_process pager_process;
>   static char *pager_program;
> -static int close_fd2;
> +static int old_fd1 = -1, old_fd2 = -1;
>   
>   /* Is the value coming back from term_columns() just a guess? */
>   static int term_columns_guessed;
> @@ -24,11 +24,11 @@ static void close_pager_fds(void)
>   {
>   	/* signal EOF to pager */
>   	close(1);
> -	if (close_fd2)
> +	if (old_fd2 != -1)
>   		close(2);
>   }
>   
> -static void wait_for_pager_atexit(void)
> +static void finish_pager(void)
>   {
>   	fflush(stdout);
>   	fflush(stderr);
> @@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)
>   	finish_command(&pager_process);
>   }
>   
> +static void wait_for_pager_atexit(void)
> +{
> +	if (old_fd1 == -1)
> +		return;
> +
> +	finish_pager();
> +}
> +
> +void wait_for_pager(void)
> +{
> +	finish_pager();
> +	sigchain_pop_common();
> +	unsetenv("GIT_PAGER_IN_USE");
> +	dup2(old_fd1, 1);
> +	close(old_fd1);
> +	old_fd1 = -1;
> +	if (old_fd2 != -1) {
> +		dup2(old_fd2, 2);
> +		close(old_fd2);
> +		old_fd2 = -1;
> +	}
> +}
> +
>   static void wait_for_pager_signal(int signo)
>   {
> +	if (old_fd1 == -1)
> +		return;
> +
>   	close_pager_fds();
>   	finish_command_in_signal(&pager_process);
>   	sigchain_pop(signo);
> @@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
>   
>   void setup_pager(void)
>   {
> +	static int once = 0;
>   	const char *pager = git_pager(isatty(1));
>   
>   	if (!pager)
> @@ -142,16 +169,20 @@ void setup_pager(void)
>   		die("unable to execute pager '%s'", pager);
>   
>   	/* original process continues, but writes to the pipe */
> +	old_fd1 = dup(1);
>   	dup2(pager_process.in, 1);
>   	if (isatty(2)) {
> -		close_fd2 = 1;
> +		old_fd2 = dup(2);
>   		dup2(pager_process.in, 2);
>   	}
>   	close(pager_process.in);
>   
> -	/* this makes sure that the parent terminates after the pager */
>   	sigchain_push_common(wait_for_pager_signal);
> -	atexit(wait_for_pager_atexit);
> +
> +	if (!once) {
> +		once++;
> +		atexit(wait_for_pager_atexit);
> +	}
>   }
>   
>   int pager_in_use(void)
> diff --git a/pager.h b/pager.h
> index b77433026d..103ecac476 100644
> --- a/pager.h
> +++ b/pager.h
> @@ -5,6 +5,7 @@ struct child_process;
>   
>   const char *git_pager(int stdout_is_tty);
>   void setup_pager(void);
> +void wait_for_pager(void);
>   int pager_in_use(void);
>   int term_columns(void);
>   void term_clear_line(void);

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

[Phillip Wood]

Hi Rubén

On 12/07/2024 02:00, Rubén Justo wrote:
> Make the print command to trigger the pager when invoked using a capital

s/to//

> 'P', to make it easier for the user to review long hunks.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>

Thanks for working on this. The code changes all look good, I'm a bit 
confused by this test though

> +test_expect_success TTY 'P does not break if pager ends unexpectly' '
> +	test_when_finished "rm -f huge_file; git reset" &&
> +	printf "%2500000s" Y >huge_file &&
> +	git add -N huge_file &&
> +	cat >expect <<-EOF &&
> +	<GREEN>+<RESET><GREEN>22<RESET>
> +	<GREEN>+<RESET><GREEN>23<RESET>
> +	<GREEN>+<RESET><GREEN>24<RESET>
> +	 30<RESET>
> +	 40<RESET>
> +	 50<RESET>
> +	<BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? <RESET>
> +	EOF
> +	test_write_lines P |
> +	(
> +		GIT_PAGER="head -1" &&
> +		export GIT_PAGER &&
> +		test_terminal git add -p >actual
> +	) &&
> +	tail -n 7 <actual | test_decode_color >actual.trimmed &&
> +	test_cmp expect actual.trimmed
> +'

What is huge_file doing and what happens to the single line of pager output?

Thanks

Phillip

>   test_expect_success 'split hunk "add -p (edit)"' '
>   	# Split, say Edit and do nothing.  Then:
>   	#

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

[Rubén Justo]

On Fri, Jul 12, 2024 at 02:26:22PM +0100, Phillip Wood wrote:

> > +test_expect_success TTY 'P does not break if pager ends unexpectly' '
> > +	test_when_finished "rm -f huge_file; git reset" &&
> > +	printf "%2500000s" Y >huge_file &&
> > +	git add -N huge_file &&
> > +	cat >expect <<-EOF &&
> > +	<GREEN>+<RESET><GREEN>22<RESET>
> > +	<GREEN>+<RESET><GREEN>23<RESET>
> > +	<GREEN>+<RESET><GREEN>24<RESET>
> > +	 30<RESET>
> > +	 40<RESET>
> > +	 50<RESET>
> > +	<BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? <RESET>
> > +	EOF
> > +	test_write_lines P |
> > +	(
> > +		GIT_PAGER="head -1" &&
> > +		export GIT_PAGER &&
> > +		test_terminal git add -p >actual
> > +	) &&
> > +	tail -n 7 <actual | test_decode_color >actual.trimmed &&
> > +	test_cmp expect actual.trimmed
> > +'
> 
> What is huge_file doing and what happens to the single line of pager output?

The huge file is to make sure we are receiving a SIGPIPE.  We don't
really care about the line "head -1" produces, only that we don't
break due to the SIGPIPE that occurs.

Maybe a test like this would be clearer?

test_expect_success TTY 'P does not break if pager ends unexpectedly' '
	test_when_finished "rm -f huge_file; git reset" &&
	printf "%2500000s\nfrotz\n" Y >huge_file &&
	git add -N huge_file &&
	cat >expect <<-EOF &&
	<GREEN>+<RESET><GREEN>frotz<RESET>
	<BOLD;BLUE>(1/1) Stage addition [y,n,q,a,d,e,p,?]? <RESET><CYAN>@@ -0,0 +1,2 @@<RESET>
	<BOLD;BLUE>(1/1) Stage addition [y,n,q,a,d,e,p,?]? <RESET>
	EOF
	test_write_lines P q |
	(
		GIT_PAGER="head -1" &&
		export GIT_PAGER &&
		test_terminal git add -p >actual
	) &&
	tail -n 3 <actual | test_decode_color >actual.trimmed &&
	test_cmp expect actual.trimmed
'

> 
> Thanks

Thank you.

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

[Rubén Justo]

On 13/7/24 1:24, Rubén Justo wrote:
 
> Maybe a test like this would be clearer?

test_expect_success TTY 'P does not break if pager ends unexpectedly' '
       test_when_finished "rm -f huge_file; git reset" &&
       printf "%2500000s" Y >huge_file &&
       git add -N huge_file &&
       test_write_lines P q | GIT_PAGER="head -c 1" test_terminal git add -p
'

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

[Junio C Hamano]

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

>> What is huge_file doing and what happens to the single line of pager output?
>
> The huge file is to make sure we are receiving a SIGPIPE.  We don't
> really care about the line "head -1" produces, only that we don't
> break due to the SIGPIPE that occurs.

That deserves to be explained in the proposed log message, I think.

Thanks.

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

[phillip.wood123]

On 12/07/2024 17:24, Rubén Justo wrote:
> On Fri, Jul 12, 2024 at 02:26:22PM +0100, Phillip Wood wrote:
> 
>>> +test_expect_success TTY 'P does not break if pager ends unexpectly' '
>>> +	test_when_finished "rm -f huge_file; git reset" &&
>>> +	printf "%2500000s" Y >huge_file &&
>>> +	git add -N huge_file &&
>>> +	cat >expect <<-EOF &&
>>> +	<GREEN>+<RESET><GREEN>22<RESET>
>>> +	<GREEN>+<RESET><GREEN>23<RESET>
>>> +	<GREEN>+<RESET><GREEN>24<RESET>
>>> +	 30<RESET>
>>> +	 40<RESET>
>>> +	 50<RESET>
>>> +	<BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? <RESET>
>>> +	EOF
>>> +	test_write_lines P |
>>> +	(
>>> +		GIT_PAGER="head -1" &&
>>> +		export GIT_PAGER &&
>>> +		test_terminal git add -p >actual
>>> +	) &&
>>> +	tail -n 7 <actual | test_decode_color >actual.trimmed &&
>>> +	test_cmp expect actual.trimmed
>>> +'
>>
>> What is huge_file doing and what happens to the single line of pager output?
> 
> The huge file is to make sure we are receiving a SIGPIPE.  We don't
> really care about the line "head -1" produces, only that we don't
> break due to the SIGPIPE that occurs.

As Junio said it would help to explain that. I'm still confused why we 
don't see any output from the pager - shouldn't the pager print the hunk 
header as it does in the example below?

> Maybe a test like this would be clearer?

I think explaining in the commit message would be best.

Thanks

Phillip

> test_expect_success TTY 'P does not break if pager ends unexpectedly' '
> 	test_when_finished "rm -f huge_file; git reset" &&
> 	printf "%2500000s\nfrotz\n" Y >huge_file &&
> 	git add -N huge_file &&
> 	cat >expect <<-EOF &&
> 	<GREEN>+<RESET><GREEN>frotz<RESET>
> 	<BOLD;BLUE>(1/1) Stage addition [y,n,q,a,d,e,p,?]? <RESET><CYAN>@@ -0,0 +1,2 @@<RESET>
> 	<BOLD;BLUE>(1/1) Stage addition [y,n,q,a,d,e,p,?]? <RESET>
> 	EOF
> 	test_write_lines P q |
> 	(
> 		GIT_PAGER="head -1" &&
> 		export GIT_PAGER &&
> 		test_terminal git add -p >actual
> 	) &&
> 	tail -n 3 <actual | test_decode_color >actual.trimmed &&
> 	test_cmp expect actual.trimmed
> '
> 
>>
>> Thanks
> 
> Thank you.

[PATCH v2 0/4] use the pager in 'add -p'

[Rubén Justo]

Rubén Justo (4):
  add-patch: test for 'p' command
  pager: do not close fd 2 unnecessarily
  pager: introduce wait_for_pager
  add-patch: render hunks through the pager

 add-patch.c                | 18 ++++++++++++---
 pager.c                    | 45 +++++++++++++++++++++++++++++++++-----
 pager.h                    |  1 +
 t/t3701-add-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+), 8 deletions(-)

Range-diff against v1:
1:  4a5b6e6815 = 1:  6b37507ddd add-patch: test for 'p' command
2:  bf8a68ac37 = 2:  5497fa020b pager: do not close fd 2 unnecessarily
3:  aabd7da4d6 = 3:  30e772cf7c pager: introduce wait_for_pager
4:  ff51cc32bd ! 4:  f7cb00b654 add-patch: render hunks through the pager
    @@ Metadata
      ## Commit message ##
         add-patch: render hunks through the pager
     
    -    Make the print command to trigger the pager when invoked using a capital
    +    Make the print command trigger the pager when invoked using a capital
         'P', to make it easier for the user to review long hunks.
     
    +    Note that if the PAGER ends unexpectedly before we've been able to send
    +    the payload, perhaps because the user is not interested in the whole
    +    thing, we might receive a SIGPIPE, which would abruptly and unexpectedly
    +    terminate the interactive session for the user.
    +
    +    Therefore, we need to ignore a possible SIGPIPE signal.  Add a test for
    +    this, in addition to the test for normal operation.
    +
    +    For the SIGPIPE test, we need to make sure that we completely fill the
    +    operating system's buffer, otherwise we might not trigger the SIGPIPE
    +    signal.  The normal size of this buffer in different OSs varies from a
    +    few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
    +    this limit.
    +
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
      ## add-patch.c ##
    @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '
     +	test_cmp expect actual.trimmed
     +'
     +
    -+test_expect_success TTY 'P does not break if pager ends unexpectly' '
    ++test_expect_success TTY 'P does not break if pager ends unexpectedly' '
     +	test_when_finished "rm -f huge_file; git reset" &&
     +	printf "%2500000s" Y >huge_file &&
     +	git add -N huge_file &&
    -+	cat >expect <<-EOF &&
    -+	<GREEN>+<RESET><GREEN>22<RESET>
    -+	<GREEN>+<RESET><GREEN>23<RESET>
    -+	<GREEN>+<RESET><GREEN>24<RESET>
    -+	 30<RESET>
    -+	 40<RESET>
    -+	 50<RESET>
    -+	<BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? <RESET>
    -+	EOF
    -+	test_write_lines P |
    -+	(
    -+		GIT_PAGER="head -1" &&
    -+		export GIT_PAGER &&
    -+		test_terminal git add -p >actual
    -+	) &&
    -+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
    -+	test_cmp expect actual.trimmed
    ++	test_write_lines P q | GIT_PAGER="head -c 1" test_terminal git add -p >actual
     +'
     +
      test_expect_success 'split hunk "add -p (edit)"' '
-- 
2.45.2.831.g9e4974e3d4

[PATCH v2 1/4] add-patch: test for 'p' command

[Rubén Justo]

Add a test for the 'p' command, which was introduced in 66c14ab592
(add-patch: introduce 'p' in interactive-patch, 2024-03-29).

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t3701-add-interactive.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5d78868ac1..6daf3a6be0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -575,6 +575,22 @@ test_expect_success 'navigate to hunk via regex / pattern' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success 'print again the hunk' '
+	test_when_finished "git reset" &&
+	tr _ " " >expect <<-EOF &&
+	+15
+	 20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	 10
+	+15
+	 20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	EOF
+	test_write_lines s y g 1 p | git add -p >actual &&
+	tail -n 7 <actual >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.45.2.831.g9e4974e3d4

[PATCH v2 2/4] pager: do not close fd 2 unnecessarily

[Rubén Justo]

We send errors to the pager since 61b80509e3 (sending errors to stdout
under $PAGER, 2008-02-16).

In a8335024c2 (pager: do not dup2 stderr if it is already redirected,
2008-12-15) an exception was introduced to avoid redirecting stderr if
it is not connected to a terminal.

In such exceptional cases, the close(STDERR_FILENO) we're doing in
close_pager_fds, is unnecessary.

Furthermore, in a subsequent commit we're going to introduce changes
that will involve using close_pager_fds multiple times.

With this in mind, controlling when we want to close stderr, become
sensible.

Let's close(STDERR_FILENO) only when necessary, and pave the way for the
upcoming changes.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/pager.c b/pager.c
index be6f4ee59f..251adfc2ad 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
+static int close_fd2;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -23,7 +24,8 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	close(2);
+	if (close_fd2)
+		close(2);
 }
 
 static void wait_for_pager_atexit(void)
@@ -141,8 +143,10 @@ void setup_pager(void)
 
 	/* original process continues, but writes to the pipe */
 	dup2(pager_process.in, 1);
-	if (isatty(2))
+	if (isatty(2)) {
+		close_fd2 = 1;
 		dup2(pager_process.in, 2);
+	}
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
2.45.2.831.g9e4974e3d4

[PATCH v2 3/4] pager: introduce wait_for_pager

[Rubén Justo]

Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
2006-02-28) we have the machinery to send our output to a pager.

That machinery, once set up, does not allow us to regain the original
stdio streams.

In the interactive commands (i.e.: add -p) we want to use the pager for
some output, while maintaining the interaction with the user.

Modify the pager machinery so that we can use setup_pager and, once
we've finished sending the desired output for the pager, wait for the
pager termination using a new function wait_for_pager.   Make this
function reset the pager machinery before returning.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 43 +++++++++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 251adfc2ad..bea4345f6f 100644
--- a/pager.c
+++ b/pager.c
@@ -14,7 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
-static int close_fd2;
+static int old_fd1 = -1, old_fd2 = -1;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -24,11 +24,11 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	if (close_fd2)
+	if (old_fd2 != -1)
 		close(2);
 }
 
-static void wait_for_pager_atexit(void)
+static void finish_pager(void)
 {
 	fflush(stdout);
 	fflush(stderr);
@@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)
 	finish_command(&pager_process);
 }
 
+static void wait_for_pager_atexit(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	finish_pager();
+}
+
+void wait_for_pager(void)
+{
+	finish_pager();
+	sigchain_pop_common();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	close(old_fd1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		close(old_fd2);
+		old_fd2 = -1;
+	}
+}
+
 static void wait_for_pager_signal(int signo)
 {
+	if (old_fd1 == -1)
+		return;
+
 	close_pager_fds();
 	finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
@@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
 
 void setup_pager(void)
 {
+	static int once = 0;
 	const char *pager = git_pager(isatty(1));
 
 	if (!pager)
@@ -142,16 +169,20 @@ void setup_pager(void)
 		die("unable to execute pager '%s'", pager);
 
 	/* original process continues, but writes to the pipe */
+	old_fd1 = dup(1);
 	dup2(pager_process.in, 1);
 	if (isatty(2)) {
-		close_fd2 = 1;
+		old_fd2 = dup(2);
 		dup2(pager_process.in, 2);
 	}
 	close(pager_process.in);
 
-	/* this makes sure that the parent terminates after the pager */
 	sigchain_push_common(wait_for_pager_signal);
-	atexit(wait_for_pager_atexit);
+
+	if (!once) {
+		once++;
+		atexit(wait_for_pager_atexit);
+	}
 }
 
 int pager_in_use(void)
diff --git a/pager.h b/pager.h
index b77433026d..103ecac476 100644
--- a/pager.h
+++ b/pager.h
@@ -5,6 +5,7 @@ struct child_process;
 
 const char *git_pager(int stdout_is_tty);
 void setup_pager(void);
+void wait_for_pager(void);
 int pager_in_use(void);
 int term_columns(void);
 void term_clear_line(void);
-- 
2.45.2.831.g9e4974e3d4

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

[Rubén Justo]

Make the print command trigger the pager when invoked using a capital
'P', to make it easier for the user to review long hunks.

Note that if the PAGER ends unexpectedly before we've been able to send
the payload, perhaps because the user is not interested in the whole
thing, we might receive a SIGPIPE, which would abruptly and unexpectedly
terminate the interactive session for the user.

Therefore, we need to ignore a possible SIGPIPE signal.  Add a test for
this, in addition to the test for normal operation.

For the SIGPIPE test, we need to make sure that we completely fill the
operating system's buffer, otherwise we might not trigger the SIGPIPE
signal.  The normal size of this buffer in different OSs varies from a
few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
this limit.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c                | 18 +++++++++++++++---
 t/t3701-add-interactive.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 6e176cd21a..f2c76b7d83 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -7,9 +7,11 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "pager.h"
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "strbuf.h"
+#include "sigchain.h"
 #include "run-command.h"
 #include "strvec.h"
 #include "pathspec.h"
@@ -1391,7 +1393,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
    "e - manually edit the current hunk\n"
-   "p - print the current hunk\n"
+   "p - print the current hunk, 'P' to use the pager\n"
    "? - print help\n");
 
 static int patch_update_file(struct add_p_state *s,
@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	int colored = !!s->colored.len, quit = 0;
+	int colored = !!s->colored.len, quit = 0, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
 	enum {
 		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
+				if (use_pager) {
+					setup_pager();
+					sigchain_push(SIGPIPE, SIG_IGN);
+				}
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
+				if (use_pager) {
+					sigchain_pop(SIGPIPE);
+					wait_for_pager();
+					use_pager = 0;
+				}
 			}
 
 			strbuf_reset(&s->buf);
@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = USE_HUNK;
 				goto soft_increment;
 			}
-		} else if (s->answer.buf[0] == 'p') {
+		} else if (ch == 'p') {
 			rendered_hunk_index = -1;
+			use_pager = (s->answer.buf[0] == 'P') ? 1 : 0;
 		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 6daf3a6be0..c89b984751 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -591,6 +591,34 @@ test_expect_success 'print again the hunk' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success TTY 'print again the hunk (PAGER)' '
+	test_when_finished "git reset" &&
+	cat >expect <<-EOF &&
+	<GREEN>+<RESET><GREEN>15<RESET>
+	 20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
+	PAGER  10<RESET>
+	PAGER <GREEN>+<RESET><GREEN>15<RESET>
+	PAGER  20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	EOF
+	test_write_lines s y g 1 P |
+	(
+		GIT_PAGER="sed s/^/PAGER\ /" &&
+		export GIT_PAGER &&
+		test_terminal git add -p >actual
+	) &&
+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
+test_expect_success TTY 'P does not break if pager ends unexpectedly' '
+	test_when_finished "rm -f huge_file; git reset" &&
+	printf "%2500000s" Y >huge_file &&
+	git add -N huge_file &&
+	test_write_lines P q | GIT_PAGER="head -c 1" test_terminal git add -p >actual
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.45.2.831.g9e4974e3d4

Re: [PATCH v2 0/4] use the pager in 'add -p'

[Junio C Hamano]

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

>     -+	test_write_lines P |
>     -+	(
>     -+		GIT_PAGER="head -1" &&
>     -+		export GIT_PAGER &&
>     -+		test_terminal git add -p >actual
>     -+	) &&
>     -+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
>     -+	test_cmp expect actual.trimmed
>     ++	test_write_lines P q | GIT_PAGER="head -c 1" test_terminal git add -p >actual
>      +'

"make test" has this to say:

t3701-add-interactive.sh:619: error: head -c is not portable (use test_copy_bytes BYTES <file >out): test_write_lines P q | GIT_PAGER="head -c 1" test_terminal git add -p >actual
gmake[1]: *** [Makefile:132: test-lint-shell-syntax] Error 1

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

[Rubén Justo]

On Sat, Jul 13, 2024 at 02:17:49PM +0100, phillip.wood123@gmail.com wrote:
> On 12/07/2024 17:24, Rubén Justo wrote:
> > On Fri, Jul 12, 2024 at 02:26:22PM +0100, Phillip Wood wrote:
> > 
> > > > +test_expect_success TTY 'P does not break if pager ends unexpectly' '
> > > > +	test_when_finished "rm -f huge_file; git reset" &&
> > > > +	printf "%2500000s" Y >huge_file &&
> > > > +	git add -N huge_file &&
> > > > +	cat >expect <<-EOF &&
> > > > +	<GREEN>+<RESET><GREEN>22<RESET>
> > > > +	<GREEN>+<RESET><GREEN>23<RESET>
> > > > +	<GREEN>+<RESET><GREEN>24<RESET>
> > > > +	 30<RESET>
> > > > +	 40<RESET>
> > > > +	 50<RESET>
> > > > +	<BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? <RESET>
> > > > +	EOF
> > > > +	test_write_lines P |
> > > > +	(
> > > > +		GIT_PAGER="head -1" &&
> > > > +		export GIT_PAGER &&
> > > > +		test_terminal git add -p >actual
> > > > +	) &&
> > > > +	tail -n 7 <actual | test_decode_color >actual.trimmed &&
> > > > +	test_cmp expect actual.trimmed
> > > > +'
> > > 
> > > What is huge_file doing and what happens to the single line of pager output?
> > 
> [...]
> 
> I'm still confused why we don't
> see any output from the pager - shouldn't the pager print the hunk header as
> it does in the example below?

In the test above, we would need to use "tail -n 18" to capture the
header of the hunk.

In the test below, the "q" allows "tail -n 3" to capture it. 

> > test_expect_success TTY 'P does not break if pager ends unexpectedly' '
> > 	test_when_finished "rm -f huge_file; git reset" &&
> > 	printf "%2500000s\nfrotz\n" Y >huge_file &&
> > 	git add -N huge_file &&
> > 	cat >expect <<-EOF &&
> > 	<GREEN>+<RESET><GREEN>frotz<RESET>
> > 	<BOLD;BLUE>(1/1) Stage addition [y,n,q,a,d,e,p,?]? <RESET><CYAN>@@ -0,0 +1,2 @@<RESET>
> > 	<BOLD;BLUE>(1/1) Stage addition [y,n,q,a,d,e,p,?]? <RESET>
> > 	EOF
> > 	test_write_lines P q |
> > 	(
> > 		GIT_PAGER="head -1" &&
> > 		export GIT_PAGER &&
> > 		test_terminal git add -p >actual
> > 	) &&
> > 	tail -n 3 <actual | test_decode_color >actual.trimmed &&
> > 	test_cmp expect actual.trimmed
> > '

I wonder if the following would be desirable: 

--- a/add-patch.c
+++ b/add-patch.c
@@@ -1519,8 -1519,8 +1519,10 @@@ static int patch_update_file(struct add
  		if (*s->s.reset_color)
  			fputs(s->s.reset_color, stdout);
  		fflush(stdout);
--		if (read_single_character(s) == EOF)
++		if (read_single_character(s) == EOF) {
++			quit = 1;
  			break;
++		}
  
  		if (!s->answer.len)
  			continue;

Re: [PATCH v2 0/4] use the pager in 'add -p'

[Rubén Justo]

On Sat, Jul 13, 2024 at 10:08:22AM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> >     -+	test_write_lines P |
> >     -+	(
> >     -+		GIT_PAGER="head -1" &&
> >     -+		export GIT_PAGER &&
> >     -+		test_terminal git add -p >actual
> >     -+	) &&
> >     -+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
> >     -+	test_cmp expect actual.trimmed
> >     ++	test_write_lines P q | GIT_PAGER="head -c 1" test_terminal git add -p >actual
> >      +'
> 
> "make test" has this to say:
> 
> t3701-add-interactive.sh:619: error: head -c is not portable (use test_copy_bytes BYTES <file >out): test_write_lines P q | GIT_PAGER="head -c 1" test_terminal git add -p >actual
> gmake[1]: *** [Makefile:132: test-lint-shell-syntax] Error 1

Ouch. I'll fix it.  I think I'll go back to "head -1".  But I'll wait to
hear comments about the change in the message.

Re: [PATCH v2 0/4] use the pager in 'add -p'

[Junio C Hamano]

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

> Ouch. I'll fix it.  I think I'll go back to "head -1".

I think "head -<n>" is deprecated, too.  

Say "head -n 1" probably, if you really wanted to take the first
line and quit.

[PATCH v3 0/4] use the pager in 'add -p'

[Rubén Justo]

This iterations fixes this error:

t3701-add-interactive.sh:619: error: head -c is not portable (use test_copy_bytes BYTES <file >out): test_write_lines P q | GIT_PAGER="head -c 1" test_terminal git add -p >actual                  
gmake[1]: *** [Makefile:132: test-lint-shell-syntax] Error 1 

Rubén Justo (4):
  add-patch: test for 'p' command
  pager: do not close fd 2 unnecessarily
  pager: introduce wait_for_pager
  add-patch: render hunks through the pager

 add-patch.c                | 18 ++++++++++++---
 pager.c                    | 45 +++++++++++++++++++++++++++++++++-----
 pager.h                    |  1 +
 t/t3701-add-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+), 8 deletions(-)

Range-diff against v3:
-:  ---------- > 1:  6b37507ddd add-patch: test for 'p' command
-:  ---------- > 2:  5497fa020b pager: do not close fd 2 unnecessarily
-:  ---------- > 3:  30e772cf7c pager: introduce wait_for_pager
1:  f7cb00b654 ! 4:  913e7f3d09 add-patch: render hunks through the pager
    @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '
     +
     +test_expect_success TTY 'P does not break if pager ends unexpectedly' '
     +	test_when_finished "rm -f huge_file; git reset" &&
    -+	printf "%2500000s" Y >huge_file &&
    ++	printf "\n%2500000s" Y >huge_file &&
     +	git add -N huge_file &&
    -+	test_write_lines P q | GIT_PAGER="head -c 1" test_terminal git add -p >actual
    ++	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p >actual
     +'
     +
      test_expect_success 'split hunk "add -p (edit)"' '
-- 
2.46.0.rc0.4.g913e7f3d09

[PATCH v3 1/4] add-patch: test for 'p' command

[Rubén Justo]

Add a test for the 'p' command, which was introduced in 66c14ab592
(add-patch: introduce 'p' in interactive-patch, 2024-03-29).

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t3701-add-interactive.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5d78868ac1..6daf3a6be0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -575,6 +575,22 @@ test_expect_success 'navigate to hunk via regex / pattern' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success 'print again the hunk' '
+	test_when_finished "git reset" &&
+	tr _ " " >expect <<-EOF &&
+	+15
+	 20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	 10
+	+15
+	 20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	EOF
+	test_write_lines s y g 1 p | git add -p >actual &&
+	tail -n 7 <actual >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.46.0.rc0.4.g913e7f3d09

[PATCH v3 2/4] pager: do not close fd 2 unnecessarily

[Rubén Justo]

We send errors to the pager since 61b80509e3 (sending errors to stdout
under $PAGER, 2008-02-16).

In a8335024c2 (pager: do not dup2 stderr if it is already redirected,
2008-12-15) an exception was introduced to avoid redirecting stderr if
it is not connected to a terminal.

In such exceptional cases, the close(STDERR_FILENO) we're doing in
close_pager_fds, is unnecessary.

Furthermore, in a subsequent commit we're going to introduce changes
that will involve using close_pager_fds multiple times.

With this in mind, controlling when we want to close stderr, become
sensible.

Let's close(STDERR_FILENO) only when necessary, and pave the way for the
upcoming changes.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/pager.c b/pager.c
index be6f4ee59f..251adfc2ad 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
+static int close_fd2;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -23,7 +24,8 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	close(2);
+	if (close_fd2)
+		close(2);
 }
 
 static void wait_for_pager_atexit(void)
@@ -141,8 +143,10 @@ void setup_pager(void)
 
 	/* original process continues, but writes to the pipe */
 	dup2(pager_process.in, 1);
-	if (isatty(2))
+	if (isatty(2)) {
+		close_fd2 = 1;
 		dup2(pager_process.in, 2);
+	}
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
2.46.0.rc0.4.g913e7f3d09

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

[Rubén Justo]

Make the print command trigger the pager when invoked using a capital
'P', to make it easier for the user to review long hunks.

Note that if the PAGER ends unexpectedly before we've been able to send
the payload, perhaps because the user is not interested in the whole
thing, we might receive a SIGPIPE, which would abruptly and unexpectedly
terminate the interactive session for the user.

Therefore, we need to ignore a possible SIGPIPE signal.  Add a test for
this, in addition to the test for normal operation.

For the SIGPIPE test, we need to make sure that we completely fill the
operating system's buffer, otherwise we might not trigger the SIGPIPE
signal.  The normal size of this buffer in different OSs varies from a
few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
this limit.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c                | 18 +++++++++++++++---
 t/t3701-add-interactive.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 6e176cd21a..f2c76b7d83 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -7,9 +7,11 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "pager.h"
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "strbuf.h"
+#include "sigchain.h"
 #include "run-command.h"
 #include "strvec.h"
 #include "pathspec.h"
@@ -1391,7 +1393,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
    "e - manually edit the current hunk\n"
-   "p - print the current hunk\n"
+   "p - print the current hunk, 'P' to use the pager\n"
    "? - print help\n");
 
 static int patch_update_file(struct add_p_state *s,
@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	int colored = !!s->colored.len, quit = 0;
+	int colored = !!s->colored.len, quit = 0, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
 	enum {
 		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
+				if (use_pager) {
+					setup_pager();
+					sigchain_push(SIGPIPE, SIG_IGN);
+				}
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
+				if (use_pager) {
+					sigchain_pop(SIGPIPE);
+					wait_for_pager();
+					use_pager = 0;
+				}
 			}
 
 			strbuf_reset(&s->buf);
@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = USE_HUNK;
 				goto soft_increment;
 			}
-		} else if (s->answer.buf[0] == 'p') {
+		} else if (ch == 'p') {
 			rendered_hunk_index = -1;
+			use_pager = (s->answer.buf[0] == 'P') ? 1 : 0;
 		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 6daf3a6be0..2ac860cc42 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -591,6 +591,34 @@ test_expect_success 'print again the hunk' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success TTY 'print again the hunk (PAGER)' '
+	test_when_finished "git reset" &&
+	cat >expect <<-EOF &&
+	<GREEN>+<RESET><GREEN>15<RESET>
+	 20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
+	PAGER  10<RESET>
+	PAGER <GREEN>+<RESET><GREEN>15<RESET>
+	PAGER  20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	EOF
+	test_write_lines s y g 1 P |
+	(
+		GIT_PAGER="sed s/^/PAGER\ /" &&
+		export GIT_PAGER &&
+		test_terminal git add -p >actual
+	) &&
+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
+test_expect_success TTY 'P does not break if pager ends unexpectedly' '
+	test_when_finished "rm -f huge_file; git reset" &&
+	printf "\n%2500000s" Y >huge_file &&
+	git add -N huge_file &&
+	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p >actual
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.46.0.rc0.4.g913e7f3d09

[PATCH v3 3/4] pager: introduce wait_for_pager

[Rubén Justo]

Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
2006-02-28) we have the machinery to send our output to a pager.

That machinery, once set up, does not allow us to regain the original
stdio streams.

In the interactive commands (i.e.: add -p) we want to use the pager for
some output, while maintaining the interaction with the user.

Modify the pager machinery so that we can use setup_pager and, once
we've finished sending the desired output for the pager, wait for the
pager termination using a new function wait_for_pager.   Make this
function reset the pager machinery before returning.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 43 +++++++++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 251adfc2ad..bea4345f6f 100644
--- a/pager.c
+++ b/pager.c
@@ -14,7 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
-static int close_fd2;
+static int old_fd1 = -1, old_fd2 = -1;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -24,11 +24,11 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	if (close_fd2)
+	if (old_fd2 != -1)
 		close(2);
 }
 
-static void wait_for_pager_atexit(void)
+static void finish_pager(void)
 {
 	fflush(stdout);
 	fflush(stderr);
@@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)
 	finish_command(&pager_process);
 }
 
+static void wait_for_pager_atexit(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	finish_pager();
+}
+
+void wait_for_pager(void)
+{
+	finish_pager();
+	sigchain_pop_common();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	close(old_fd1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		close(old_fd2);
+		old_fd2 = -1;
+	}
+}
+
 static void wait_for_pager_signal(int signo)
 {
+	if (old_fd1 == -1)
+		return;
+
 	close_pager_fds();
 	finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
@@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
 
 void setup_pager(void)
 {
+	static int once = 0;
 	const char *pager = git_pager(isatty(1));
 
 	if (!pager)
@@ -142,16 +169,20 @@ void setup_pager(void)
 		die("unable to execute pager '%s'", pager);
 
 	/* original process continues, but writes to the pipe */
+	old_fd1 = dup(1);
 	dup2(pager_process.in, 1);
 	if (isatty(2)) {
-		close_fd2 = 1;
+		old_fd2 = dup(2);
 		dup2(pager_process.in, 2);
 	}
 	close(pager_process.in);
 
-	/* this makes sure that the parent terminates after the pager */
 	sigchain_push_common(wait_for_pager_signal);
-	atexit(wait_for_pager_atexit);
+
+	if (!once) {
+		once++;
+		atexit(wait_for_pager_atexit);
+	}
 }
 
 int pager_in_use(void)
diff --git a/pager.h b/pager.h
index b77433026d..103ecac476 100644
--- a/pager.h
+++ b/pager.h
@@ -5,6 +5,7 @@ struct child_process;
 
 const char *git_pager(int stdout_is_tty);
 void setup_pager(void);
+void wait_for_pager(void);
 int pager_in_use(void);
 int term_columns(void);
 void term_clear_line(void);
-- 
2.46.0.rc0.4.g913e7f3d09

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

[Phillip Wood]

Hi Rubén

On 14/07/2024 17:04, Rubén Justo wrote:
> Make the print command trigger the pager when invoked using a capital
> 'P', to make it easier for the user to review long hunks.
> 
> Note that if the PAGER ends unexpectedly before we've been able to send
> the payload, perhaps because the user is not interested in the whole
> thing, we might receive a SIGPIPE, which would abruptly and unexpectedly
> terminate the interactive session for the user.
> 
> Therefore, we need to ignore a possible SIGPIPE signal.  Add a test for
> this, in addition to the test for normal operation.
> 
> For the SIGPIPE test, we need to make sure that we completely fill the
> operating system's buffer, otherwise we might not trigger the SIGPIPE
> signal.  The normal size of this buffer in different OSs varies from a
> few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
> this limit.

Thanks for updating the commit message to explain the purpose of the 
SIGPIPE test

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---

> +test_expect_success TTY 'P does not break if pager ends unexpectedly' '

I think it would be helpful to mention SIGPIPE in the title as this test 
is really checking "we don't die if we receive SIGPIPE". Maybe

     P handles SIGPIPE when writing to pager

> +	test_when_finished "rm -f huge_file; git reset" &&
> +	printf "\n%2500000s" Y >huge_file &&
> +	git add -N huge_file &&
> +	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p >actual

If we're not going to look at the output we don't need to redirect it. 
I'm not sure if there is any benefit to comparing the actual output to 
what we expect here.

Best Wishes

Phillip

> +'
> +
>   test_expect_success 'split hunk "add -p (edit)"' '
>   	# Split, say Edit and do nothing.  Then:
>   	#

Re: [PATCH v3 3/4] pager: introduce wait_for_pager

[Phillip Wood]

Hi Rubén

On 14/07/2024 17:04, Rubén Justo wrote:
> Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
> 2006-02-28) we have the machinery to send our output to a pager.
> 
> That machinery, once set up, does not allow us to regain the original
> stdio streams.
> 
> In the interactive commands (i.e.: add -p) we want to use the pager for
> some output, while maintaining the interaction with the user.
> 
> Modify the pager machinery so that we can use setup_pager and, once
> we've finished sending the desired output for the pager, wait for the
> pager termination using a new function wait_for_pager.   Make this
> function reset the pager machinery before returning.

Do you have any comments on my thoughts in 
<8434fafe-f545-49bc-8cc1-d4e8fb634bec@gmail.com> ?

Best Wishes

Phillip

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   pager.c | 43 +++++++++++++++++++++++++++++++++++++------
>   pager.h |  1 +
>   2 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/pager.c b/pager.c
> index 251adfc2ad..bea4345f6f 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -14,7 +14,7 @@ int pager_use_color = 1;
>   
>   static struct child_process pager_process;
>   static char *pager_program;
> -static int close_fd2;
> +static int old_fd1 = -1, old_fd2 = -1;
>   
>   /* Is the value coming back from term_columns() just a guess? */
>   static int term_columns_guessed;
> @@ -24,11 +24,11 @@ static void close_pager_fds(void)
>   {
>   	/* signal EOF to pager */
>   	close(1);
> -	if (close_fd2)
> +	if (old_fd2 != -1)
>   		close(2);
>   }
>   
> -static void wait_for_pager_atexit(void)
> +static void finish_pager(void)
>   {
>   	fflush(stdout);
>   	fflush(stderr);
> @@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)
>   	finish_command(&pager_process);
>   }
>   
> +static void wait_for_pager_atexit(void)
> +{
> +	if (old_fd1 == -1)
> +		return;
> +
> +	finish_pager();
> +}
> +
> +void wait_for_pager(void)
> +{
> +	finish_pager();
> +	sigchain_pop_common();
> +	unsetenv("GIT_PAGER_IN_USE");
> +	dup2(old_fd1, 1);
> +	close(old_fd1);
> +	old_fd1 = -1;
> +	if (old_fd2 != -1) {
> +		dup2(old_fd2, 2);
> +		close(old_fd2);
> +		old_fd2 = -1;
> +	}
> +}
> +
>   static void wait_for_pager_signal(int signo)
>   {
> +	if (old_fd1 == -1)
> +		return;
> +
>   	close_pager_fds();
>   	finish_command_in_signal(&pager_process);
>   	sigchain_pop(signo);
> @@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
>   
>   void setup_pager(void)
>   {
> +	static int once = 0;
>   	const char *pager = git_pager(isatty(1));
>   
>   	if (!pager)
> @@ -142,16 +169,20 @@ void setup_pager(void)
>   		die("unable to execute pager '%s'", pager);
>   
>   	/* original process continues, but writes to the pipe */
> +	old_fd1 = dup(1);
>   	dup2(pager_process.in, 1);
>   	if (isatty(2)) {
> -		close_fd2 = 1;
> +		old_fd2 = dup(2);
>   		dup2(pager_process.in, 2);
>   	}
>   	close(pager_process.in);
>   
> -	/* this makes sure that the parent terminates after the pager */
>   	sigchain_push_common(wait_for_pager_signal);
> -	atexit(wait_for_pager_atexit);
> +
> +	if (!once) {
> +		once++;
> +		atexit(wait_for_pager_atexit);
> +	}
>   }
>   
>   int pager_in_use(void)
> diff --git a/pager.h b/pager.h
> index b77433026d..103ecac476 100644
> --- a/pager.h
> +++ b/pager.h
> @@ -5,6 +5,7 @@ struct child_process;
>   
>   const char *git_pager(int stdout_is_tty);
>   void setup_pager(void);
> +void wait_for_pager(void);
>   int pager_in_use(void);
>   int term_columns(void);
>   void term_clear_line(void);

Re: [PATCH v3 3/4] pager: introduce wait_for_pager

[Rubén Justo]

On Mon, Jul 15, 2024 at 03:13:09PM +0100, Phillip Wood wrote:
> Hi Rubén
> 
> On 14/07/2024 17:04, Rubén Justo wrote:
> > Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
> > 2006-02-28) we have the machinery to send our output to a pager.
> > 
> > That machinery, once set up, does not allow us to regain the original
> > stdio streams.
> > 
> > In the interactive commands (i.e.: add -p) we want to use the pager for
> > some output, while maintaining the interaction with the user.
> > 
> > Modify the pager machinery so that we can use setup_pager and, once
> > we've finished sending the desired output for the pager, wait for the
> > pager termination using a new function wait_for_pager.   Make this
> > function reset the pager machinery before returning.
> 
> Do you have any comments on my thoughts in
> <8434fafe-f545-49bc-8cc1-d4e8fb634bec@gmail.com> ?

Oops! I thought I had responded, but somehow I must not have. 

For reference, these are the points you indicated: 

>  - We ignore any errors when duplicating fds,
>    "git grep '[^a-z_]dup2\{0,1\}(' shows that's not unusual in our
>    code base, though if we cannot redirect the output to the pager or
>    restore stdout when the pager exits that's a problem for "git add -p"
> 
>  - We should perhaps be marking old_fd[12] with O_CLOEXEC to stop them
>    being passed to the pager.

Both points are interesting and improve resilience to unexpected
situations.  I remember that the first point was already suggested in
the previous thread.

IMHO both points should be considered with a more global perspective
than the scope of this series.

As I said in the first message of this thread, I have left out
interesting points that may deserve to be addressed in future series,
with the intention of not prolonging the discussion of the current
changes too much.

Sorry for not responding sooner.

[PATCH v4 0/4] add-patch: render hunks through the pager

[Rubén Justo]

Make the test name more descriptive and avoid unnecessary redirection.

Thanks.

Rubén Justo (4):
  add-patch: test for 'p' command
  pager: do not close fd 2 unnecessarily
  pager: introduce wait_for_pager
  add-patch: render hunks through the pager

 add-patch.c                | 18 ++++++++++++---
 pager.c                    | 45 +++++++++++++++++++++++++++++++++-----
 pager.h                    |  1 +
 t/t3701-add-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+), 8 deletions(-)

Interdiff against v3:
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 2ac860cc42..c60589cb94 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -612,11 +612,11 @@ test_expect_success TTY 'print again the hunk (PAGER)' '
 	test_cmp expect actual.trimmed
 '
 
-test_expect_success TTY 'P does not break if pager ends unexpectedly' '
+test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
 	test_when_finished "rm -f huge_file; git reset" &&
 	printf "\n%2500000s" Y >huge_file &&
 	git add -N huge_file &&
-	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p >actual
+	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
 '
 
 test_expect_success 'split hunk "add -p (edit)"' '
-- 
2.46.0.rc0.4.g229d67bbd7

[PATCH v4 1/4] add-patch: test for 'p' command

[Rubén Justo]

Add a test for the 'p' command, which was introduced in 66c14ab592
(add-patch: introduce 'p' in interactive-patch, 2024-03-29).

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t3701-add-interactive.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5d78868ac1..6daf3a6be0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -575,6 +575,22 @@ test_expect_success 'navigate to hunk via regex / pattern' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success 'print again the hunk' '
+	test_when_finished "git reset" &&
+	tr _ " " >expect <<-EOF &&
+	+15
+	 20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	 10
+	+15
+	 20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	EOF
+	test_write_lines s y g 1 p | git add -p >actual &&
+	tail -n 7 <actual >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.46.0.rc0.4.g229d67bbd7

[PATCH v4 2/4] pager: do not close fd 2 unnecessarily

[Rubén Justo]

We send errors to the pager since 61b80509e3 (sending errors to stdout
under $PAGER, 2008-02-16).

In a8335024c2 (pager: do not dup2 stderr if it is already redirected,
2008-12-15) an exception was introduced to avoid redirecting stderr if
it is not connected to a terminal.

In such exceptional cases, the close(STDERR_FILENO) we're doing in
close_pager_fds, is unnecessary.

Furthermore, in a subsequent commit we're going to introduce changes
that will involve using close_pager_fds multiple times.

With this in mind, controlling when we want to close stderr, become
sensible.

Let's close(STDERR_FILENO) only when necessary, and pave the way for the
upcoming changes.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/pager.c b/pager.c
index be6f4ee59f..251adfc2ad 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
+static int close_fd2;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -23,7 +24,8 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	close(2);
+	if (close_fd2)
+		close(2);
 }
 
 static void wait_for_pager_atexit(void)
@@ -141,8 +143,10 @@ void setup_pager(void)
 
 	/* original process continues, but writes to the pipe */
 	dup2(pager_process.in, 1);
-	if (isatty(2))
+	if (isatty(2)) {
+		close_fd2 = 1;
 		dup2(pager_process.in, 2);
+	}
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
2.46.0.rc0.4.g229d67bbd7

[PATCH v4 3/4] pager: introduce wait_for_pager

[Rubén Justo]

Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
2006-02-28) we have the machinery to send our output to a pager.

That machinery, once set up, does not allow us to regain the original
stdio streams.

In the interactive commands (i.e.: add -p) we want to use the pager for
some output, while maintaining the interaction with the user.

Modify the pager machinery so that we can use setup_pager and, once
we've finished sending the desired output for the pager, wait for the
pager termination using a new function wait_for_pager.   Make this
function reset the pager machinery before returning.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 43 +++++++++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 251adfc2ad..bea4345f6f 100644
--- a/pager.c
+++ b/pager.c
@@ -14,7 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
-static int close_fd2;
+static int old_fd1 = -1, old_fd2 = -1;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -24,11 +24,11 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	if (close_fd2)
+	if (old_fd2 != -1)
 		close(2);
 }
 
-static void wait_for_pager_atexit(void)
+static void finish_pager(void)
 {
 	fflush(stdout);
 	fflush(stderr);
@@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)
 	finish_command(&pager_process);
 }
 
+static void wait_for_pager_atexit(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	finish_pager();
+}
+
+void wait_for_pager(void)
+{
+	finish_pager();
+	sigchain_pop_common();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	close(old_fd1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		close(old_fd2);
+		old_fd2 = -1;
+	}
+}
+
 static void wait_for_pager_signal(int signo)
 {
+	if (old_fd1 == -1)
+		return;
+
 	close_pager_fds();
 	finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
@@ -113,6 +139,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
 
 void setup_pager(void)
 {
+	static int once = 0;
 	const char *pager = git_pager(isatty(1));
 
 	if (!pager)
@@ -142,16 +169,20 @@ void setup_pager(void)
 		die("unable to execute pager '%s'", pager);
 
 	/* original process continues, but writes to the pipe */
+	old_fd1 = dup(1);
 	dup2(pager_process.in, 1);
 	if (isatty(2)) {
-		close_fd2 = 1;
+		old_fd2 = dup(2);
 		dup2(pager_process.in, 2);
 	}
 	close(pager_process.in);
 
-	/* this makes sure that the parent terminates after the pager */
 	sigchain_push_common(wait_for_pager_signal);
-	atexit(wait_for_pager_atexit);
+
+	if (!once) {
+		once++;
+		atexit(wait_for_pager_atexit);
+	}
 }
 
 int pager_in_use(void)
diff --git a/pager.h b/pager.h
index b77433026d..103ecac476 100644
--- a/pager.h
+++ b/pager.h
@@ -5,6 +5,7 @@ struct child_process;
 
 const char *git_pager(int stdout_is_tty);
 void setup_pager(void);
+void wait_for_pager(void);
 int pager_in_use(void);
 int term_columns(void);
 void term_clear_line(void);
-- 
2.46.0.rc0.4.g229d67bbd7

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

[Rubén Justo]

Make the print command trigger the pager when invoked using a capital
'P', to make it easier for the user to review long hunks.

Note that if the PAGER ends unexpectedly before we've been able to send
the payload, perhaps because the user is not interested in the whole
thing, we might receive a SIGPIPE, which would abruptly and unexpectedly
terminate the interactive session for the user.

Therefore, we need to ignore a possible SIGPIPE signal.  Add a test for
this, in addition to the test for normal operation.

For the SIGPIPE test, we need to make sure that we completely fill the
operating system's buffer, otherwise we might not trigger the SIGPIPE
signal.  The normal size of this buffer in different OSs varies from a
few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
this limit.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c                | 18 +++++++++++++++---
 t/t3701-add-interactive.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 6e176cd21a..f2c76b7d83 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -7,9 +7,11 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "pager.h"
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "strbuf.h"
+#include "sigchain.h"
 #include "run-command.h"
 #include "strvec.h"
 #include "pathspec.h"
@@ -1391,7 +1393,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
    "e - manually edit the current hunk\n"
-   "p - print the current hunk\n"
+   "p - print the current hunk, 'P' to use the pager\n"
    "? - print help\n");
 
 static int patch_update_file(struct add_p_state *s,
@@ -1402,7 +1404,7 @@ static int patch_update_file(struct add_p_state *s,
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	int colored = !!s->colored.len, quit = 0;
+	int colored = !!s->colored.len, quit = 0, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
 	enum {
 		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
@@ -1452,9 +1454,18 @@ static int patch_update_file(struct add_p_state *s,
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
+				if (use_pager) {
+					setup_pager();
+					sigchain_push(SIGPIPE, SIG_IGN);
+				}
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
+				if (use_pager) {
+					sigchain_pop(SIGPIPE);
+					wait_for_pager();
+					use_pager = 0;
+				}
 			}
 
 			strbuf_reset(&s->buf);
@@ -1675,8 +1686,9 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = USE_HUNK;
 				goto soft_increment;
 			}
-		} else if (s->answer.buf[0] == 'p') {
+		} else if (ch == 'p') {
 			rendered_hunk_index = -1;
+			use_pager = (s->answer.buf[0] == 'P') ? 1 : 0;
 		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 6daf3a6be0..c60589cb94 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -591,6 +591,34 @@ test_expect_success 'print again the hunk' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success TTY 'print again the hunk (PAGER)' '
+	test_when_finished "git reset" &&
+	cat >expect <<-EOF &&
+	<GREEN>+<RESET><GREEN>15<RESET>
+	 20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
+	PAGER  10<RESET>
+	PAGER <GREEN>+<RESET><GREEN>15<RESET>
+	PAGER  20<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	EOF
+	test_write_lines s y g 1 P |
+	(
+		GIT_PAGER="sed s/^/PAGER\ /" &&
+		export GIT_PAGER &&
+		test_terminal git add -p >actual
+	) &&
+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
+test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
+	test_when_finished "rm -f huge_file; git reset" &&
+	printf "\n%2500000s" Y >huge_file &&
+	git add -N huge_file &&
+	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.46.0.rc0.4.g229d67bbd7

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

[Junio C Hamano]

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

> +test_expect_success TTY 'P does not break if pager ends unexpectedly' '
> +	test_when_finished "rm -f huge_file; git reset" &&
> +	printf "\n%2500000s" Y >huge_file &&
> +	git add -N huge_file &&
> +	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p >actual
> +'

Somehow this is dying with signal 11?

  https://github.com/git/git/actions/runs/9944800531/job/27471636926#step:5:1108

I know there is a v4 that has a small update here, but in case that
this still is relevant and the removed rediraction to ">actual" did
not magically fixed it...

Thanks.

Re: [PATCH v3 3/4] pager: introduce wait_for_pager

[phillip.wood123]

Hi Rubén

On 15/07/2024 21:04, Rubén Justo wrote:
> On Mon, Jul 15, 2024 at 03:13:09PM +0100, Phillip Wood wrote:
> For reference, these are the points you indicated:
> 
>>   - We ignore any errors when duplicating fds,
>>     "git grep '[^a-z_]dup2\{0,1\}(' shows that's not unusual in our
>>     code base, though if we cannot redirect the output to the pager or
>>     restore stdout when the pager exits that's a problem for "git add -p"
>>
>>   - We should perhaps be marking old_fd[12] with O_CLOEXEC to stop them
>>     being passed to the pager.
> 
> Both points are interesting and improve resilience to unexpected
> situations.  I remember that the first point was already suggested in
> the previous thread.
> 
> IMHO both points should be considered with a more global perspective
> than the scope of this series.

I'm not sure what you mean by this. It is true that we were ignoring 
dup2() errors in this function before but this patch the O_CLOEXEC issue 
is new.

> As I said in the first message of this thread, I have left out
> interesting points that may deserve to be addressed in future series,
> with the intention of not prolonging the discussion of the current
> changes too much.

I think there is a difference between adding new features which I agree 
should be left and getting the implementation details of this helper 
function right. Having said that ignoring the dup() errors isn't making 
anything worse than it was and the extra fds are for the terminal which 
the pager is accessing anyway.

> Sorry for not responding sooner.

No worries, thanks for your reply

Phillip

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

[Rubén Justo]

Squashing this fixes the test:

--->8---

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index c60589cb94..bb360c92a0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -616,7 +616,12 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
 	test_when_finished "rm -f huge_file; git reset" &&
 	printf "\n%2500000s" Y >huge_file &&
 	git add -N huge_file &&
-	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
+	test_write_lines P q |
+	(
+		GIT_PAGER="head -n 1" &&
+		export GIT_PAGER &&
+		test_terminal git add -p >actual
+	)
 '
---8<---

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

I'm wondering if the best way to proceed here is to revert to: 

diff --git a/pager.c b/pager.c
index 5f0c1e9cce..5586e751dc 100644
--- a/pager.c
+++ b/pager.c
@@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)

 void wait_for_pager(void)
 {
+	if (old_fd1 == -1)
+		return;
 	finish_pager();
 	sigchain_pop_common();
 	unsetenv("GIT_PAGER_IN_USE");

Which resolves the problem.

This was a change already commented here:

https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/

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

[phillip.wood123]

Hi Ruében

Thanks for looking into the test failure.

On 17/07/2024 18:20, Rubén Justo wrote:
> Squashing this fixes the test:
> 
> --->8---
> 
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index c60589cb94..bb360c92a0 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -616,7 +616,12 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
>   	test_when_finished "rm -f huge_file; git reset" &&
>   	printf "\n%2500000s" Y >huge_file &&
>   	git add -N huge_file &&
> -	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
> +	test_write_lines P q |
> +	(
> +		GIT_PAGER="head -n 1" &&
> +		export GIT_PAGER &&
> +		test_terminal git add -p >actual
> +	)

That's surprising, why does running git in a sub-shell stop it from 
segfaulting?

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

What was causing setup pager to fail in this test?

> I'm wondering if the best way to proceed here is to revert to:
> 
> diff --git a/pager.c b/pager.c
> index 5f0c1e9cce..5586e751dc 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)
> 
>   void wait_for_pager(void)
>   {
> +	if (old_fd1 == -1)
> +		return;
>   	finish_pager();
>   	sigchain_pop_common();
>   	unsetenv("GIT_PAGER_IN_USE");
> 
> This was a change already commented here:
> 
> https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/

My worry was that this would paper over a bug as we shouldn't be calling 
wait_for_pager() without setting up the pager successfully. How easy 
would it be to fix the source of the problem?

Best Wishes

Phillip

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

[Junio C Hamano]

phillip.wood123@gmail.com writes:

>> -	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
>> +	test_write_lines P q |
>> +	(
>> +		GIT_PAGER="head -n 1" &&
>> +		export GIT_PAGER &&
>> +		test_terminal git add -p >actual
>> +	)
>
> That's surprising, why does running git in a sub-shell stop it from
> segfaulting?

Yeah, it indeed is curious.  

The rewrite resolves another iffy point in the original---you are
not supposed to attempt a one-shot assignment to the environment
variable when you are running a shell function, as that is not
portable.  And the above rewrite is a common way to fix that.

But still, yes, it is curious why the original segfaults.  Is there
some race there and having a subshell shifts the timing, or
something?

> My worry was that this would paper over a bug as we shouldn't be
> calling wait_for_pager() without setting up the pager
> successfully. How easy would it be to fix the source of the problem?

;-)  Nice to see people trying to do the right thing.

Thanks.

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

[Eric Sunshine]

On Wed, Jul 17, 2024 at 4:04 PM Junio C Hamano <gitster@pobox.com> wrote:
> phillip.wood123@gmail.com writes:
> >> -    test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
> >> +    test_write_lines P q |
> >> +    (
> >> +            GIT_PAGER="head -n 1" &&
> >> +            export GIT_PAGER &&
> >> +            test_terminal git add -p >actual
> >> +    )
> >
> > That's surprising, why does running git in a sub-shell stop it from
> > segfaulting?
>
> Yeah, it indeed is curious.
>
> The rewrite resolves another iffy point in the original---you are
> not supposed to attempt a one-shot assignment to the environment
> variable when you are running a shell function, as that is not
> portable.  And the above rewrite is a common way to fix that.

It's also curious that t/check-non-portable-shell.pl didn't catch this
use of one-shot assignment when calling a shell function[*].

[*] a0a630192d (t/check-non-portable-shell: detect "FOO=bar
shell_func", 2018-07-13)

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Jul 17, 2024 at 4:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>> phillip.wood123@gmail.com writes:
>> >> -    test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
>> >> +    test_write_lines P q |
>> >> +    (
>> >> +            GIT_PAGER="head -n 1" &&
>> >> +            export GIT_PAGER &&
>> >> +            test_terminal git add -p >actual
>> >> +    )
>> >
>> > That's surprising, why does running git in a sub-shell stop it from
>> > segfaulting?
>>
>> Yeah, it indeed is curious.
>>
>> The rewrite resolves another iffy point in the original---you are
>> not supposed to attempt a one-shot assignment to the environment
>> variable when you are running a shell function, as that is not
>> portable.  And the above rewrite is a common way to fix that.
>
> It's also curious that t/check-non-portable-shell.pl didn't catch this
> use of one-shot assignment when calling a shell function[*].

True.

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

[Junio C Hamano]

phillip.wood123@gmail.com writes:

>> -	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
>> +	test_write_lines P q |
>> +	(
>> +		GIT_PAGER="head -n 1" &&
>> +		export GIT_PAGER &&
>> +		test_terminal git add -p >actual
>> +	)
>
> That's surprising, why does running git in a sub-shell stop it from
> segfaulting?

Another difference besides the sub-shell is where the output from
test_terminal goes.  If the above change fixes the issue for Rubén,
I wonder if it still works if ">actual" gets removed.

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

[phillip.wood123]

On 17/07/2024 21:03, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>>> -	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
>>> +	test_write_lines P q |
>>> +	(
>>> +		GIT_PAGER="head -n 1" &&
>>> +		export GIT_PAGER &&
>>> +		test_terminal git add -p >actual
>>> +	)
>>
>> That's surprising, why does running git in a sub-shell stop it from
>> segfaulting?
> 
> Yeah, it indeed is curious.
> 
> The rewrite resolves another iffy point in the original---you are
> not supposed to attempt a one-shot assignment to the environment
> variable when you are running a shell function, as that is not
> portable.  And the above rewrite is a common way to fix that.

Good point, I'd not thought of that.

Best Wishes

Phillip

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

[phillip.wood123]

On 17/07/2024 21:31, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>>> -	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
>>> +	test_write_lines P q |
>>> +	(
>>> +		GIT_PAGER="head -n 1" &&
>>> +		export GIT_PAGER &&
>>> +		test_terminal git add -p >actual
>>> +	)
>>
>> That's surprising, why does running git in a sub-shell stop it from
>> segfaulting?
> 
> Another difference besides the sub-shell is where the output from
> test_terminal goes.  If the above change fixes the issue for Rubén,
> I wonder if it still works if ">actual" gets removed.

Yes that would be interesting to know. I do think we should be checking 
the output here to make sure the test is doing what we think it is - all 
we know at the moment is whether git exits successfully or not. If we 
receive SIGPIPE then fputs() will see EPIPE which will set ferror() 
which we should be clearing and checking that the prompt prints 
correctly afterwards. I think it would be helpful to add

tail -n3 actual >actual.trimmed &&
cat >expect <<\EOF &&
	\ No newline at end of file
	(1/1) Stage addition [y,n,q,a,d,e,p,?]? @@ -0,0 +1,2 @@
	(1/1) Stage addition [y,n,q,a,d,e,p,?]?
	EOF
test_cmp expect actual.trimmed

to the test. That all still leaves me wondering about the segfault though

Best Wishes

Phillip

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

[phillip.wood123]

Hi Rubén

On 17/07/2024 18:20, Rubén Justo wrote:
> However, this error has exposed a problem: calling `wait_for_pager` if
> `setup_pager` hasn't worked is an issue that needs to be addressed in this
> series: `setup_pager` should return a result.

It already dies if we cannot execute the pager so maybe we should just 
die on other errors as well?

Best Wishes

Phillip

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

[Rubén Justo]

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

> On 17/07/2024 18:20, Rubén Justo wrote:
> > Squashing this fixes the test:
> > 
> > --->8---
> > 
> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> > index c60589cb94..bb360c92a0 100755
> > --- a/t/t3701-add-interactive.sh
> > +++ b/t/t3701-add-interactive.sh
> > @@ -616,7 +616,12 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
> >   	test_when_finished "rm -f huge_file; git reset" &&
> >   	printf "\n%2500000s" Y >huge_file &&
> >   	git add -N huge_file &&
> > -	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
> > +	test_write_lines P q |
> > +	(
> > +		GIT_PAGER="head -n 1" &&
> > +		export GIT_PAGER &&
> > +		test_terminal git add -p >actual
> > +	)
> 
> That's surprising, why does running git in a sub-shell stop it from
> segfaulting?

The fix isn't the sub-shell;  it's "export GIT_PAGER".

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

Because GIT_PAGER is not being set correctly in the test, "git add -p"
can use the values defined in the environment where the test is running.
Usually PAGER is empty or contains "less", but in the environment where
the fault occurs, it happens to be: "PAGER=cat". 

Since we have an optimization to avoid forking if the pager is "cat",
courtesy of caef71a535 (Do not fork PAGER=cat, 2006-04-16), then we fail
in `wait_for_pager()` because we are calling `finish_command()` with an
uninitialized `pager_process`.

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

> > I'm wondering if the best way to proceed here is to revert to:
> > 
> > diff --git a/pager.c b/pager.c
> > index 5f0c1e9cce..5586e751dc 100644
> > --- a/pager.c
> > +++ b/pager.c
> > @@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)
> > 
> >   void wait_for_pager(void)
> >   {
> > +	if (old_fd1 == -1)
> > +		return;
> >   	finish_pager();
> >   	sigchain_pop_common();
> >   	unsetenv("GIT_PAGER_IN_USE");
> >
> > This was a change already commented here:
> > 
> > https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/

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

[Rubén Justo]

On Wed, Jul 17, 2024 at 04:09:17PM -0400, Eric Sunshine wrote:

> > >> -    test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p

> It's also curious that t/check-non-portable-shell.pl didn't catch this
> use of one-shot assignment when calling a shell function[*].

It would have been great if it had caught that error.

As a reference:

    func () {
    }

    VAR=value func           # this error is caught
    echo 1 |
    VAR=value func           # this one is also caught
    echo 1 | VAR=value func  # this one isn't

Maybe, catch this errors expanding the regular expression we have in
`check-non-portable-shell.pl` isn't the best approach.  We might need
something more sophisticated, like what we have in `chainlint.pl`.

Perhaps someone with experience in those scripts could give us this
capability :-)

Thanks for your message.

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

[Rubén Justo]

On Wed, Jul 17, 2024 at 01:31:16PM -0700, Junio C Hamano wrote:

> >> -	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
> >> +	test_write_lines P q |
> >> +	(
> >> +		GIT_PAGER="head -n 1" &&
> >> +		export GIT_PAGER &&
> >> +		test_terminal git add -p >actual
> >> +	)

> I wonder if it still works if ">actual" gets removed.

Yes, it works.  That redirection is just noise from my previous version.

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

[Rubén Justo]

On Thu, Jul 18, 2024 at 10:58:36AM +0100, phillip.wood123@gmail.com wrote:

> > However, this error has exposed a problem: calling `wait_for_pager` if
> > `setup_pager` hasn't worked is an issue that needs to be addressed in this
> > series: `setup_pager` should return a result.
> 
> It already dies if we cannot execute the pager so maybe we should just die
> on other errors as well?

Honestly, I thought 78f0a5d187 (pager: die when paging to non-existing
command, 2024-06-23) would be sufficient for this series, but I missed
the optimization mentioned in my previous message.

Thinking in the context of "add -p", it might be more sensible not to
die but simply show an error, so as not to end the user's interactive
session.  But it could be a change in a future series and thus avoid
prolonging this one. 

However, if you can think of any other cases where we should be stricter
and die, I'm all ears.

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

[Eric Sunshine]

On Sat, Jul 20, 2024 at 6:37 PM Rubén Justo <rjusto@gmail.com> wrote:
> On Wed, Jul 17, 2024 at 04:09:17PM -0400, Eric Sunshine wrote:
> > > >> -    test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
>
> > It's also curious that t/check-non-portable-shell.pl didn't catch this
> > use of one-shot assignment when calling a shell function[*].
>
> It would have been great if it had caught that error.
>
> As a reference:
>     VAR=value func           # this error is caught
>     echo 1 |
>     VAR=value func           # this one is also caught
>     echo 1 | VAR=value func  # this one isn't

Thanks for providing this summary; it saved me the effort of digging
back through this discussion / patch series.

> Maybe, catch this errors expanding the regular expression we have in
> `check-non-portable-shell.pl` isn't the best approach.  We might need
> something more sophisticated, like what we have in `chainlint.pl`.

The idea has been expressed previously of subsuming all the
check-non-portable-shell.pl checks into chainlint.pl some day, thus
allowing check-non-portable-shell.pl to be retired. In fact, it was
mentioned again quite recently[1].

However, this particular check (detecting `VAR=val shell-func`) poses
an extra complication which would require some specialized additional
mechanism in chainlint.pl. In particular, in `VAR=val symbol`, in
order to distinguish when `symbol` is an external command versus a
shell-function, it is necessary to scan for function definitions not
just in the script being checked, but also in all scripts included
(recursively) by the script being checked. So, it's probably possible
to do but ought to be done carefully.

> Perhaps someone with experience in those scripts could give us this
> capability :-)

I posted a series[2] which addresses this shortcoming by enhancing
check-non-portable-shell.pl.

[1]: https://lore.kernel.org/git/CAPig+cTFZuU7zM7poqk4HeK09zn8bFrO37eUZiaGmeJ0yecpiw@mail.gmail.com/
[2]: https://lore.kernel.org/git/20240722065915.80760-1-ericsunshine@charter.net/T/

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

[Phillip Wood]

Hi Rubén

On 20/07/2024 23:29, Rubén Justo wrote:
> On Wed, Jul 17, 2024 at 08:39:12PM +0100, phillip.wood123@gmail.com wrote:
> 
>> On 17/07/2024 18:20, Rubén Justo wrote:
>>
>>> However, this error has exposed a problem: calling `wait_for_pager` if
>>> `setup_pager` hasn't worked is an issue that needs to be addressed in this
>>> series: `setup_pager` should return a result.  I was planning to do that
>>> in a future series, for the other commented command: `|[cmd]`.
>>
>> What was causing setup pager to fail in this test?
> 
> Because GIT_PAGER is not being set correctly in the test,

Oh I see, I had assumed that the shell would report an error if it 
didn't support setting environment variables like this when running a 
function but instead it silently ignored it. This highlights the 
importance of testing the output of "git add -p" in this test so we can 
be sure the pager is doing what we think it should. Using

	GIT_PAGER="sed -e s/^/PAGER\ / -e q"

would make it clear what the pager is printing while also causing SIGPIPE

> "git add -p"
> can use the values defined in the environment where the test is running.
> Usually PAGER is empty or contains "less", but in the environment where
> the fault occurs, it happens to be: "PAGER=cat".
> 
> Since we have an optimization to avoid forking if the pager is "cat",
> courtesy of caef71a535 (Do not fork PAGER=cat, 2006-04-16), then we fail
> in `wait_for_pager()` because we are calling `finish_command()` with an
> uninitialized `pager_process`.
> 
> That's why I thought, aligned with what we are already doing in
> `wait_for_pager_at_exit()`, that this is a sensible approach:

That extra information is important. When I said [1]

 > Isn't it a bug to call this with old_fd1 == -1 or have I missed
 > something?

What I'd missed was that we can return early without executing anything. 
We cannot do

	if (!git_pager(asatty(1))
		return

at the beginning of wait_for_pager() because if we're running a pager 
isatty(1) will return false so I think the old_fd as you suggested is 
the easiest fix. The existing callers do not need to know if 
setup_pager() applied the "cat" optimization because they only setup the 
pager once. For "add -p" this no-longer applies so we should think about 
returning a flag to say "there was an error"/"there is no pager or the 
pager is 'cat'"/"the pager has been started"

Best Wishes

Phillip

[1] 
https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com

>>> I'm wondering if the best way to proceed here is to revert to:
>>>
>>> diff --git a/pager.c b/pager.c
>>> index 5f0c1e9cce..5586e751dc 100644
>>> --- a/pager.c
>>> +++ b/pager.c
>>> @@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)
>>>
>>>    void wait_for_pager(void)
>>>    {
>>> +	if (old_fd1 == -1)
>>> +		return;
>>>    	finish_pager();
>>>    	sigchain_pop_common();
>>>    	unsetenv("GIT_PAGER_IN_USE");
>>>
>>> This was a change already commented here:
>>>
>>> https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/

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

[Rubén Justo]

On Mon, Jul 22, 2024 at 03:18:07AM -0400, Eric Sunshine wrote:

> I posted a series[2] which addresses this shortcoming by enhancing
> check-non-portable-shell.pl.

I tested this series with them, and it correctly detects the error.

Thanks for sending the improvement so quick! 

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

[Rubén Justo]

On Mon, Jul 22, 2024 at 11:18:00AM +0100, Phillip Wood wrote:

> > That's why I thought, aligned with what we are already doing in
> > `wait_for_pager_at_exit()`, that this is a sensible approach:
> 
> That extra information is important. When I said [1]
> 
> > Isn't it a bug to call this with old_fd1 == -1 or have I missed
> > something?
> 
> What I'd missed was that we can return early without executing anything.

Yep, missing that old optimization is easy ;)

> We cannot do
> 
> 	if (!git_pager(asatty(1))
> 		return
> 
> at the beginning of wait_for_pager() because if we're running a pager
> isatty(1) will return false so I think the old_fd as you suggested is the
> easiest fix.

Now that we agree, I'll do it :) 

> The existing callers do not need to know if setup_pager()
> applied the "cat" optimization because they only setup the pager once. For
> "add -p" this no-longer applies so we should think about returning a flag to
> say "there was an error"/"there is no pager or the pager is 'cat'"/"the
> pager has been started"

I'm not sure it would be valuable for us to make the caller aware that
"there is no pager or the pager is 'cat' ... just use stdout". 

However, I do agree that probably in the future, if we finally add the
"|[cmd]" command, we'll need to return some kind of error instead of
`die()`, in setup_pager().

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

[Junio C Hamano]

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

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

Attached at the end is a test tweak patch, taking inspirations from
Phillip's comments, to see what value GIT_PAGER has in the shell
function.  I shortened the huge_file a bit so that I do not have to
have an infinite scrollback buffer,but otherwise, the test_quirk
intermediate shell function should work just like the test_terminal
helper in the original position would.

And I see in the output from "sh t3701-add-interactive.sh -i -v":

    expecting success of 3701.51 'P handles SIGPIPE when writing to pager': 
            test_when_finished "rm -f huge_file; git reset" &&
            printf "\n%250s" Y >huge_file &&
            git add -N huge_file &&
            echo "in env: GIT_PAGER=$(env | grep GIT_PAGER=)" &&
            test_write_lines P q | GIT_PAGER="head -n 1" test_quirk &&
            echo "after test_quirk returns: GIT_PAGER=$GIT_PAGER"

    in env: GIT_PAGER=
    in test_quirk: GIT_PAGER=head -n 1
    in env: GIT_PAGER=GIT_PAGER=head -n 1
    In test_terminal: GIT_PAGER=GIT_PAGER=head -n 1
    test-terminal: GIT_PAGER=head -n 1
    diff --git a/huge_file b/huge_file
    new file mode 100644
    index 0000000..d06820d
    --- /dev/null
    +++ b/huge_file
    @@ -0,0 +1,2 @@
    +
    +                                                                                                                                                                                                                                                         Y
    \ No newline at end of file
    (1/1) Stage addition [y,n,q,a,d,e,p,?]? @@ -0,0 +1,2 @@
    (1/1) Stage addition [y,n,q,a,d,e,p,?]? 
    after test_quirk returns: GIT_PAGER=
    Unstaged changes after reset:
    M       test
    ok 51 - P handles SIGPIPE when writing to pager

So:

 - before the one-shot thing, in the envrionment GIT_PAGER is empty.
 - in the helper function,
   - shell variable GIT_PAGER is set to the expected value.
   - GIT_PAGER env is exported.
   - test-terminal.perl sees $ENV{GIT_PAGER} set to the expected value.
 - after the helper returns GIT_PAGER is empty

It's a very convincing theory but it does not seem to match my
observation.  Is there a difference in shells used, or something?

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

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

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

[Rubén Justo]

On Mon, Jul 22, 2024 at 10:22:58AM -0700, Junio C Hamano wrote:
 
> Attached at the end is a test tweak patch, taking inspirations from
> Phillip's comments, to see what value GIT_PAGER has in the shell
> function.  I shortened the huge_file a bit so that I do not have to
> have an infinite scrollback buffer,but otherwise, the test_quirk
> intermediate shell function should work just like the test_terminal
> helper in the original position would.
> 
> And I see in the output from "sh t3701-add-interactive.sh -i -v":
> 
>     expecting success of 3701.51 'P handles SIGPIPE when writing to pager': 
>             test_when_finished "rm -f huge_file; git reset" &&
>             printf "\n%250s" Y >huge_file &&
>             git add -N huge_file &&
>             echo "in env: GIT_PAGER=$(env | grep GIT_PAGER=)" &&
>             test_write_lines P q | GIT_PAGER="head -n 1" test_quirk &&
>             echo "after test_quirk returns: GIT_PAGER=$GIT_PAGER"
> 
>     in env: GIT_PAGER=
>     in test_quirk: GIT_PAGER=head -n 1
>     in env: GIT_PAGER=GIT_PAGER=head -n 1
>     In test_terminal: GIT_PAGER=GIT_PAGER=head -n 1
>     test-terminal: GIT_PAGER=head -n 1
>     diff --git a/huge_file b/huge_file
>     new file mode 100644
>     index 0000000..d06820d
>     --- /dev/null
>     +++ b/huge_file
>     @@ -0,0 +1,2 @@
>     +
>     +                                                                                                                                                                                                                                                         Y
>     \ No newline at end of file
>     (1/1) Stage addition [y,n,q,a,d,e,p,?]? @@ -0,0 +1,2 @@
>     (1/1) Stage addition [y,n,q,a,d,e,p,?]? 
>     after test_quirk returns: GIT_PAGER=
>     Unstaged changes after reset:
>     M       test
>     ok 51 - P handles SIGPIPE when writing to pager
> 
> So:
> 
>  - before the one-shot thing, in the envrionment GIT_PAGER is empty.
>  - in the helper function,
>    - shell variable GIT_PAGER is set to the expected value.
>    - GIT_PAGER env is exported.
>    - test-terminal.perl sees $ENV{GIT_PAGER} set to the expected value.
>  - after the helper returns GIT_PAGER is empty
> 
> It's a very convincing theory but it does not seem to match my
> observation.  Is there a difference in shells used, or something?

Have you tried your tweak in the "linux-gcc (ubuntu-20.04)" test
environment where the problem was detected?  In that environment, the
value of GIT_PAGER is not passed to Git in that test. 

To fix the test, as already said, we need this:

	test_write_lines P q |
	(
		GIT_PAGER="head -n 1" &&
		export GIT_PAGER &&
		test_terminal git add -p >actual
	)

And this series also need the other other change that I'm discussing
with Phillip: 

diff --git a/pager.c b/pager.c
index 5f0c1e9cce..5586e751dc 100644
--- a/pager.c
+++ b/pager.c
@@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)

 void wait_for_pager(void)
 {
+       if (old_fd1 == -1)
+               return;
        finish_pager();
        sigchain_pop_common();
        unsetenv("GIT_PAGER_IN_USE");

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

[Junio C Hamano]

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

>> It's a very convincing theory but it does not seem to match my
>> observation.  Is there a difference in shells used, or something?
>
> Have you tried your tweak in the "linux-gcc (ubuntu-20.04)" test
> environment where the problem was detected?  In that environment, the
> value of GIT_PAGER is not passed to Git in that test. 

So, we may have a shell that does not behave like others ;-)  Do you
know what shell is being used?

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Rubén Justo <rjusto@gmail.com> writes:
>
>>> It's a very convincing theory but it does not seem to match my
>>> observation.  Is there a difference in shells used, or something?
>>
>> Have you tried your tweak in the "linux-gcc (ubuntu-20.04)" test
>> environment where the problem was detected?  In that environment, the
>> value of GIT_PAGER is not passed to Git in that test. 
>
> So, we may have a shell that does not behave like others ;-)  Do you
> know what shell is being used?

So we have an answer:

  https://github.com/git/git/actions/runs/10047627546/job/27769808515

tells us that the problematic shell is used in the job.

It is

ii  dash           0.5.10.2-6     amd64        POSIX-compliant shell

running on Ubuntu 20.04 that is "too POSIXly correct"[*] and behaves
differently from what the tests expect.

Somebody should write this combination down somewhere in the
documentation so that we can answer (better yet, we do not have to
answer) when somebody wonders if we know of a version of shell that
refuses to do an one-shot export for shell functions as we naïvely
expect.


[Reference]

 * https://lore.kernel.org/git/4B5027B8.2090507@viscovery.net/


----- >8 --------- >8 --------- >8 --------- >8 ----
CodingGuidelines: give an example shell that "fails" "VAR=VAL shell_func"

Over the years, we accumulated the community wisdom to avoid the
common "one-short export" construct for shell functions, but seem to
have lost on which exact platform it is known to fail.  Now during
an investigation on a breakage for a recent topic, let's document
one example of failing shell.

This does *not* mean that we can freely start using the construct
once Ubuntu 20.04 is retired.  But it does mean that we cannot use
the construct until Ubuntu 20.04 is fully retired from the machines
that matter.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
index 1d92b2da03..a3ecb4ac5a 100644
--- c/Documentation/CodingGuidelines
+++ w/Documentation/CodingGuidelines
@@ -204,6 +204,29 @@ For shell scripts specifically (not exhaustive):
 	local variable="$value"
 	local variable="$(command args)"
 
+ - The common construct
+
+	VAR=VAL command args
+
+   to temporarily set and export environment variable VAR only while
+   "command args" is running is handy, but some versions of dash (like
+   0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment
+   without exporting the variable, when command is *not* an external
+   command.  We often have to resort to subshell with explicit export,
+   i.e.
+
+	(incorrect)
+	VAR=VAL func args
+
+	(correct)
+	(
+		VAR=VAL && export VAR &&
+		func args
+	)
+
+   but be careful that the effect "func" makes to the variables in the
+   current shell will be lost across the subshell boundary.
+
  - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g.
    "\xc2\xa2") in printf format strings, since hexadecimal escape
    sequences are not portable.

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

[Junio C Hamano]

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

> To fix the test, as already said, we need this:
>
> 	test_write_lines P q |
> 	(
> 		GIT_PAGER="head -n 1" &&
> 		export GIT_PAGER &&
> 		test_terminal git add -p >actual
> 	)

This took sufficiently large amount of collective braincycles, and
it would be worth documenting as a separate patch, I would suspect.

Something along the following lines, but please take the authorship
*and* give it a better explanation.

Thanks.

--- >8 ---
Subject: [PATCH] t3701: avoid one-shot export for shell functions

The common construct

    VAR=VAL command args

to temporarily set and export environment variable VAR only while
"command args" is running is handy, but one of our CI jobs on GitHub
Actions uses Ubuntu 20.04 running dash 0.5.10.2-6 failed with the
construct, making only a temporary assignment without exporting the
variable, when command is *not* an external (in this case, a shell
function).

The "git add -p" being tested did not get our custom GIT_PAGER,
which broke the test.

Work it around by explicitly exporting the variable in a subshell.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3701-add-interactive.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index c60589cb94..1b8617e0c1 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -616,7 +616,11 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
 	test_when_finished "rm -f huge_file; git reset" &&
 	printf "\n%2500000s" Y >huge_file &&
 	git add -N huge_file &&
-	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
+	test_write_lines P q | (
+		GIT_PAGER="head -n 1" &&
+		export GIT_PAGER &&
+		test_terminal git add -p
+	)
 '
 
 test_expect_success 'split hunk "add -p (edit)"' '
-- 
2.46.0-rc1-48-g0900f1888e

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

[Rubén Justo]

On Mon, Jul 22, 2024 at 02:06:37PM -0700, Junio C Hamano wrote:

> So we have an answer:
> 
>   https://github.com/git/git/actions/runs/10047627546/job/27769808515
> 
> tells us that the problematic shell is used in the job.
> 
> It is
> 
> ii  dash           0.5.10.2-6     amd64        POSIX-compliant shell
> 
> running on Ubuntu 20.04 that is "too POSIXly correct"[*] and behaves
> differently from what the tests expect.
> 
> Somebody should write this combination down somewhere in the
> documentation so that we can answer (better yet, we do not have to
> answer) when somebody wonders if we know of a version of shell that
> refuses to do an one-shot export for shell functions as we naïvely
> expect.
> 
> 
> [Reference]
> 
>  * https://lore.kernel.org/git/4B5027B8.2090507@viscovery.net/
> 
> 
> ----- >8 --------- >8 --------- >8 --------- >8 ----
> CodingGuidelines: give an example shell that "fails" "VAR=VAL shell_func"
> 
> Over the years, we accumulated the community wisdom to avoid the
> common "one-short export" construct for shell functions, but seem to
> have lost on which exact platform it is known to fail.  Now during
> an investigation on a breakage for a recent topic, let's document
> one example of failing shell.
> 
> This does *not* mean that we can freely start using the construct
> once Ubuntu 20.04 is retired.  But it does mean that we cannot use
> the construct until Ubuntu 20.04 is fully retired from the machines
> that matter.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/CodingGuidelines | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
> index 1d92b2da03..a3ecb4ac5a 100644
> --- c/Documentation/CodingGuidelines
> +++ w/Documentation/CodingGuidelines
> @@ -204,6 +204,29 @@ For shell scripts specifically (not exhaustive):
>  	local variable="$value"
>  	local variable="$(command args)"
>  
> + - The common construct
> +
> +	VAR=VAL command args
> +
> +   to temporarily set and export environment variable VAR only while
> +   "command args" is running is handy, but some versions of dash (like
> +   0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment
> +   without exporting the variable, when command is *not* an external
> +   command.  We often have to resort to subshell with explicit export,
> +   i.e.
> +
> +	(incorrect)
> +	VAR=VAL func args
> +
> +	(correct)
> +	(
> +		VAR=VAL && export VAR &&
> +		func args

Just a small comment; maybe it's worth adding an extra line to make the
example clearer:  

		VAR=VAL &&
		export VAR &&
		func args

> +	)
> +
> +   but be careful that the effect "func" makes to the variables in the
> +   current shell will be lost across the subshell boundary.
> +
>   - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g.
>     "\xc2\xa2") in printf format strings, since hexadecimal escape
>     sequences are not portable.

Thank you for digging into the test to find an explanation and for
adding the comment to the documentation.

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

[Kyle Lippincott]

On Mon, Jul 22, 2024 at 2:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Rubén Justo <rjusto@gmail.com> writes:
> >
> >>> It's a very convincing theory but it does not seem to match my
> >>> observation.  Is there a difference in shells used, or something?
> >>
> >> Have you tried your tweak in the "linux-gcc (ubuntu-20.04)" test
> >> environment where the problem was detected?  In that environment, the
> >> value of GIT_PAGER is not passed to Git in that test.
> >
> > So, we may have a shell that does not behave like others ;-)  Do you
> > know what shell is being used?
>
> So we have an answer:
>
>   https://github.com/git/git/actions/runs/10047627546/job/27769808515
>
> tells us that the problematic shell is used in the job.
>
> It is
>
> ii  dash           0.5.10.2-6     amd64        POSIX-compliant shell
>
> running on Ubuntu 20.04 that is "too POSIXly correct"[*] and behaves
> differently from what the tests expect.
>
> Somebody should write this combination down somewhere in the
> documentation so that we can answer (better yet, we do not have to
> answer) when somebody wonders if we know of a version of shell that
> refuses to do an one-shot export for shell functions as we naïvely
> expect.
>
>
> [Reference]
>
>  * https://lore.kernel.org/git/4B5027B8.2090507@viscovery.net/
>
>
> ----- >8 --------- >8 --------- >8 --------- >8 ----
> CodingGuidelines: give an example shell that "fails" "VAR=VAL shell_func"
>
> Over the years, we accumulated the community wisdom to avoid the
> common "one-short export" construct for shell functions, but seem to
> have lost on which exact platform it is known to fail.  Now during
> an investigation on a breakage for a recent topic, let's document
> one example of failing shell.
>
> This does *not* mean that we can freely start using the construct
> once Ubuntu 20.04 is retired.  But it does mean that we cannot use
> the construct until Ubuntu 20.04 is fully retired from the machines
> that matter.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/CodingGuidelines | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
> index 1d92b2da03..a3ecb4ac5a 100644
> --- c/Documentation/CodingGuidelines
> +++ w/Documentation/CodingGuidelines
> @@ -204,6 +204,29 @@ For shell scripts specifically (not exhaustive):
>         local variable="$value"
>         local variable="$(command args)"
>
> + - The common construct
> +
> +       VAR=VAL command args
> +
> +   to temporarily set and export environment variable VAR only while
> +   "command args" is running is handy, but some versions of dash (like
> +   0.5.10.2-6 found on Ubuntu 20.04) makes a temporary assignment

I was also able to reproduce both aspects of this behavior (doesn't
export, value is retained) with ksh (sh (AT&T Research) 93u+m/1.0.8
2024-01-01), which is the current version on debian testing. So maybe
"some versions of ksh (tested: 93u+m/1.0.8 2024-01-01) and dash
(0.5.10.2-6)"? Or maybe we move the 'some versions' around, because I
think it's probably all versions of ksh :)

I don't know how easily discoverable this is, though. I think I'd
still want some linkage between t/check-non-portable-shell.pl and this
section of this file? I probably wouldn't think to look here if I
received that error from the check-non-portable-shell.pl linter.

Otherwise, looks good.

> +   without exporting the variable, when command is *not* an external
> +   command.  We often have to resort to subshell with explicit export,
> +   i.e.
> +
> +       (incorrect)
> +       VAR=VAL func args
> +
> +       (correct)
> +       (
> +               VAR=VAL && export VAR &&
> +               func args
> +       )
> +
> +   but be careful that the effect "func" makes to the variables in the
> +   current shell will be lost across the subshell boundary.
> +
>   - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g.
>     "\xc2\xa2") in printf format strings, since hexadecimal escape
>     sequences are not portable.

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

[Rubén Justo]

On Mon, Jul 22, 2024 at 02:25:45PM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > To fix the test, as already said, we need this:
> >
> > 	test_write_lines P q |
> > 	(
> > 		GIT_PAGER="head -n 1" &&
> > 		export GIT_PAGER &&
> > 		test_terminal git add -p >actual
> > 	)
> 
> This took sufficiently large amount of collective braincycles, and
> it would be worth documenting as a separate patch, I would suspect.
> 
> Something along the following lines, but please take the authorship
> *and* give it a better explanation.

Here's an attempt. 

I'm also adding the change for `wait_for_pager`, which could be squashed
in b29c59e3d2 (pager: introduce wait_for_pager, 2024-07-16).  Although,
highlighted I think it's interesting as well.  But I don't have a strong
preference.

This builds on rj/add-p-pager.

Thanks. 

Rubén Justo (2):
  t3701: avoid one-shot export for shell functions
  pager: make wait_for_pager a no-op for "cat"

 pager.c                    | 3 +++
 t/t3701-add-interactive.sh | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)


base-commit: 6bc52a5543008bff2c6ec7a0a935c7fc1f79e646
-- 
2.45.1

[PATCH 1/2] t3701: avoid one-shot export for shell functions

[Rubén Justo]

The common construct:

    VAR=VAL command args

it's a common way to define one-shot variables within the scope of
executing a "command".

However, when "command" is a function which in turn executes the
"command", the behavior varies depending on the shell:

 ** Bash 5.2.21 **

    $ f () { bash -c 'echo A=$A'; }
    $ A=1 f
    A=1

 ** dash 0.5.12-9 **

    $ f () { bash -c 'echo A=$A'; }
    $ A=1 f
    A=1

 ** dash 0.5.10.2-6 **

    $ f () { bash -c 'echo A=$A'; }
    $ A=1 f
    A=

One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash
0.5.10.2-6, so we failed the test t3701:51;  the "git add -p" being
tested did not get our custom GIT_PAGER, which broke the test.

Work it around by explicitly exporting the variable in a subshell.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t3701-add-interactive.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index c60589cb94..1b8617e0c1 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -616,7 +616,11 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
 	test_when_finished "rm -f huge_file; git reset" &&
 	printf "\n%2500000s" Y >huge_file &&
 	git add -N huge_file &&
-	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
+	test_write_lines P q | (
+		GIT_PAGER="head -n 1" &&
+		export GIT_PAGER &&
+		test_terminal git add -p
+	)
 '
 
 test_expect_success 'split hunk "add -p (edit)"' '
-- 
2.45.1

[PATCH 2/2] pager: make wait_for_pager a no-op for "cat"

[Rubén Justo]

If we find that the configured pager is an empty string [*1*] or simply
"cat" [*2*], then we return from `setup_pager()` silently without doing
anything, allowing the output to go directly to the normal stdout.

Let's make the call to `wait_for_pager()` for these cases, or any other
future optimizations that may occur, also exit silently without doing
anything.

   1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,
                   2006-04-16)

   2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pager.c b/pager.c
index bea4345f6f..896f40fcd2 100644
--- a/pager.c
+++ b/pager.c
@@ -46,6 +46,9 @@ static void wait_for_pager_atexit(void)
 
 void wait_for_pager(void)
 {
+	if (old_fd1 == -1)
+		return;
+
 	finish_pager();
 	sigchain_pop_common();
 	unsetenv("GIT_PAGER_IN_USE");
-- 
2.45.1

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

[Junio C Hamano]

Kyle Lippincott <spectral@google.com> writes:

> I don't know how easily discoverable this is, though. I think I'd
> still want some linkage between t/check-non-portable-shell.pl and this
> section of this file?

Patches welcome.  I personally think CodingGuidelines would be more
promiment source of information than a linter script, and comments
in the lint script cannot grow too elaborate, so that is the reason
why I did this patch first.  Once we have something to refer to in a
section in an authoritative and canonical document, it should be
easy to point at the section from other places, like the linter
script.

Thanks.

Re: [PATCH 1/2] t3701: avoid one-shot export for shell functions

[Junio C Hamano]

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

> The common construct:
>
>     VAR=VAL command args
>
> it's a common way to define one-shot variables within the scope of
> executing a "command".

"it's a" -> "is a".
"define" -> "set and export".

> However, when "command" is a function which in turn executes the
> "command", the behavior varies depending on the shell:
>
>  ** Bash 5.2.21 **
>
>     $ f () { bash -c 'echo A=$A'; }
>     $ A=1 f
>     A=1
>
>  ** dash 0.5.12-9 **
>
>     $ f () { bash -c 'echo A=$A'; }
>     $ A=1 f
>     A=1
>
>  ** dash 0.5.10.2-6 **
>
>     $ f () { bash -c 'echo A=$A'; }
>     $ A=1 f
>     A=
>
> One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash
> 0.5.10.2-6, so we failed the test t3701:51;  the "git add -p" being
> tested did not get our custom GIT_PAGER, which broke the test.
>
> Work it around by explicitly exporting the variable in a subshell.

Nicely described.

>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  t/t3701-add-interactive.sh | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index c60589cb94..1b8617e0c1 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -616,7 +616,11 @@ test_expect_success TTY 'P handles SIGPIPE when writing to pager' '
>  	test_when_finished "rm -f huge_file; git reset" &&
>  	printf "\n%2500000s" Y >huge_file &&
>  	git add -N huge_file &&
> -	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p
> +	test_write_lines P q | (
> +		GIT_PAGER="head -n 1" &&
> +		export GIT_PAGER &&
> +		test_terminal git add -p
> +	)
>  '
>  
>  test_expect_success 'split hunk "add -p (edit)"' '

Re: [PATCH 2/2] pager: make wait_for_pager a no-op for "cat"

[Junio C Hamano]

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

> If we find that the configured pager is an empty string [*1*] or simply
> "cat" [*2*], then we return from `setup_pager()` silently without doing
> anything, allowing the output to go directly to the normal stdout.

I'm tempted to suggest inserting two extra paragraphs here to avoid
too big a leap in logic flow.

    Even though the caller may properly make matching calls to
    setup_pager() and wait_for_pager(), setup_pager() may return early
    without doing much, and the call to wait_for_pager() would segfault.

    This condition can be detected by old_fd1 being -1 (not modified in
    setup_pager())

> Let's make the call to `wait_for_pager()` for these cases, or any other
> future optimizations that may occur, also exit silently without doing
> anything.
>
>    1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,
>                    2006-04-16)
>
>    2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---

I am not 100% sure about the "would segfault", but we'd need to be
explicit about what badness it causes to call wait_for_pager()
without starting a pager.  Other than that, well explained.

Thanks.

>  pager.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/pager.c b/pager.c
> index bea4345f6f..896f40fcd2 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -46,6 +46,9 @@ static void wait_for_pager_atexit(void)
>  
>  void wait_for_pager(void)
>  {
> +	if (old_fd1 == -1)
> +		return;
> +
>  	finish_pager();
>  	sigchain_pop_common();
>  	unsetenv("GIT_PAGER_IN_USE");

Re: [PATCH 1/2] t3701: avoid one-shot export for shell functions

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Rubén Justo <rjusto@gmail.com> writes:
>
>> The common construct:
>>
>>     VAR=VAL command args
>>
>> it's a common way to define one-shot variables within the scope of
>> executing a "command".
>
> "it's a" -> "is a".
> "define" -> "set and export".
>
>> However, when "command" is a function which in turn executes the
>> "command", the behavior varies depending on the shell:
>>
>>  ** Bash 5.2.21 **
>>
>>     $ f () { bash -c 'echo A=$A'; }
>>     $ A=1 f
>>     A=1
>>
>>  ** dash 0.5.12-9 **
>>
>>     $ f () { bash -c 'echo A=$A'; }
>>     $ A=1 f
>>     A=1
>>
>>  ** dash 0.5.10.2-6 **
>>
>>     $ f () { bash -c 'echo A=$A'; }
>>     $ A=1 f
>>     A=

Another thing.  Let's insert a paragraph perhaps like this here.

    Note that POSIX explicitly says the effect of this construct
    used on a shell function is unspecified.

>> One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash
>> 0.5.10.2-6, so we failed the test t3701:51;  the "git add -p" being
>> tested did not get our custom GIT_PAGER, which broke the test.
>>
>> Work it around by explicitly exporting the variable in a subshell.

That way, we won't give a wrong impression that we can safely start
using the construct in future once dash 0.5.10.2-6 goes away.

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

[Junio C Hamano]

Kyle Lippincott <spectral@google.com> writes:

> I was also able to reproduce both aspects of this behavior (doesn't
> export, value is retained) with ksh (sh (AT&T Research) 93u+m/1.0.8
> 2024-01-01), which is the current version on debian testing. So maybe
> "some versions of ksh (tested: 93u+m/1.0.8 2024-01-01) and dash
> (0.5.10.2-6)"? Or maybe we move the 'some versions' around, because I
> think it's probably all versions of ksh :)

Makes sense, but I think "POSIX guarantees that the behaviour is
something you should not rely on by telling us that these are
unspecified", which you found,  is a much better rationale to
explicitly forbid "VAR=VAL shell_func" construct.

Besides, as another thread recently discussed, our test scripts,
with really heavy uses of "local", do not work at all with AT&T ksh
(other ksh clones are reported to be OK, though).  So it may be OK
to write it off as "unusuable to run our tests", at least for now.

[PATCH 3/4] pager: introduce wait_for_pager

[Rubén Justo]

Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
2006-02-28) we have the machinery to send our output to a pager.

That machinery, once set up, does not allow us to regain the original
stdio streams.

In the interactive commands (i.e.: add -p) we want to use the pager for
some output, while maintaining the interaction with the user.

Modify the pager machinery so that we can use `setup_pager()` and, once
we've finished sending the desired output for the pager, wait for the
pager termination using a new function `wait_for_pager()`.  Make this
function reset the pager machinery before returning.

One specific point to note is that we avoid forking the pager in
`setup_pager()` if the configured pager is an empty string [*1*] or
simply "cat" [*2*].  In these cases, `setup_pager()` does nothing and
therefore `wait_for_pager()` should not be called.

We could modify `setup_pager()` to return an indication of these
situations, so we could avoid calling `wait_for_pager()`.

However, let's avoid transferring that responsibility to the caller and
instead treat the call to `wait_for_pager()` as a no-op when we know we
haven't forked the pager.

   1.- 402461aab1 (pager: do not fork a pager if PAGER is set to empty.,
                   2006-04-16)

   2.- caef71a535 (Do not fork PAGER=cat, 2006-04-16)

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 pager.c | 46 ++++++++++++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 251adfc2ad..896f40fcd2 100644
--- a/pager.c
+++ b/pager.c
@@ -14,7 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static char *pager_program;
-static int close_fd2;
+static int old_fd1 = -1, old_fd2 = -1;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
@@ -24,11 +24,11 @@ static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
 	close(1);
-	if (close_fd2)
+	if (old_fd2 != -1)
 		close(2);
 }
 
-static void wait_for_pager_atexit(void)
+static void finish_pager(void)
 {
 	fflush(stdout);
 	fflush(stderr);
@@ -36,8 +36,37 @@ static void wait_for_pager_atexit(void)
 	finish_command(&pager_process);
 }
 
+static void wait_for_pager_atexit(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	finish_pager();
+}
+
+void wait_for_pager(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	finish_pager();
+	sigchain_pop_common();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	close(old_fd1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		close(old_fd2);
+		old_fd2 = -1;
+	}
+}
+
 static void wait_for_pager_signal(int signo)
 {
+	if (old_fd1 == -1)
+		return;
+
 	close_pager_fds();
 	finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
@@ -113,6 +142,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
 
 void setup_pager(void)
 {
+	static int once = 0;
 	const char *pager = git_pager(isatty(1));
 
 	if (!pager)
@@ -142,16 +172,20 @@ void setup_pager(void)
 		die("unable to execute pager '%s'", pager);
 
 	/* original process continues, but writes to the pipe */
+	old_fd1 = dup(1);
 	dup2(pager_process.in, 1);
 	if (isatty(2)) {
-		close_fd2 = 1;
+		old_fd2 = dup(2);
 		dup2(pager_process.in, 2);
 	}
 	close(pager_process.in);
 
-	/* this makes sure that the parent terminates after the pager */
 	sigchain_push_common(wait_for_pager_signal);
-	atexit(wait_for_pager_atexit);
+
+	if (!once) {
+		once++;
+		atexit(wait_for_pager_atexit);
+	}
 }
 
 int pager_in_use(void)
diff --git a/pager.h b/pager.h
index b77433026d..103ecac476 100644
--- a/pager.h
+++ b/pager.h
@@ -5,6 +5,7 @@ struct child_process;
 
 const char *git_pager(int stdout_is_tty);
 void setup_pager(void);
+void wait_for_pager(void);
 int pager_in_use(void);
 int term_columns(void);
 void term_clear_line(void);
-- 
2.46.0.rc0.4.g6f4990c0d4