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

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

[Rubén Justo]

Invoke the pager when displaying hunks during "add -p" sessions, to make
it easier for the user to review hunks longer than one screen height.

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

 add-patch.c                |  3 +++
 pager.c                    | 41 ++++++++++++++++++++++++++++++++------
 pager.h                    |  1 +
 t/t3701-add-interactive.sh | 37 ++++++++++++++++++++++++++++++++++
 t/test-terminal.perl       | 32 ++++++++++++++++-------------
 5 files changed, 94 insertions(+), 20 deletions(-)

-- 
2.45.1.209.gd5886bf9cd

[PATCH 1/5] 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 28a95a775d..52d7830de2 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -542,6 +542,22 @@ test_expect_success 'goto hunk' '
 	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 'navigate to hunk via regex' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-- 
2.45.1.209.gd5886bf9cd

[PATCH 2/5] 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 might call close_pager_fds multiple times.  With this in mind,
unconditionally closing stderr will become undesirable.

Let's close(STDERR_FILENO) only when necessary, and pave the way for
the coming 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 b8822a9381..3ef6798f7e 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static const char *pager_program;
+static int old_fd2 = -1;
 
 /* 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 (old_fd2 != -1)
+		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)) {
+		old_fd2 = 1;
 		dup2(pager_process.in, 2);
+	}
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
2.45.1.209.gd5886bf9cd

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

[Junio C Hamano]

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

> 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 might call close_pager_fds multiple times.  With this in mind,
> unconditionally closing stderr will become undesirable.

In a new world with such a change, what does it mean to call
close_pager_fds()?  It used to mean "we are really done with the
pager and we no longer need them, ever".

And we still call the helper for that purpose after this change,
from wait_for_pager_atexit() and wait_for_pager_signal().

So no matter what "a subsequent commit" does, it feels conceptually
wrong to call it more than once in the first place.  In other words,
what is wrong is that this function closes stderr, but "a subsequent
commit" calls this function multiple times, no?

>  static struct child_process pager_process;
>  static const char *pager_program;
> +static int old_fd2 = -1;

What does the magic number "-1" mean?  We often use it to signal
"uninitialized", but then what are concrete "initialized" values
mean?  "We dup2()'ed something else to stderr/fd #2 but before doing
so we saved the original fd #2 away to this variable, so that we can
restore fd #2 by another dup2() of the value of this variable when
we declare that we are done with the standard error stream"?

But that does not look like what is happening here.

>  /* 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 (old_fd2 != -1)
> +		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)) {
> +		old_fd2 = 1;

Equally unclear magic number "1" is used here.

This value is different from pager_process.in, and my earlier "we
are saving away" does not apply, either.

>  		dup2(pager_process.in, 2);
> +	}
>  	close(pager_process.in);

Puzzled...

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

[Rubén Justo]

On Mon, May 20, 2024 at 12:14:04PM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > 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 might call close_pager_fds multiple times.  With this in mind,
> > unconditionally closing stderr will become undesirable.
> 
> In a new world with such a change, what does it mean to call
> close_pager_fds()?

There is no change in what calling close_pager_fds() means.

> It used to mean "we are really done with the
> pager and we no longer need them, ever".
> 
> And we still call the helper for that purpose after this change,
> from wait_for_pager_atexit() and wait_for_pager_signal().

Yes, no change here either.  In the next commit, a new client of the
helper is introduced, the new API: wait_for_pager().

> 
> So no matter what "a subsequent commit" does, it feels conceptually
> wrong to call it more than once in the first place.
> In other words,
> what is wrong is that this function closes stderr, but "a subsequent
> commit" calls this function multiple times, no?

This series is trying to allow triggering the pager multiple times.
Reaching to close_pager_fds() multiple times is a consequence of it.

> 
> >  static struct child_process pager_process;
> >  static const char *pager_program;
> > +static int old_fd2 = -1;
> 
> What does the magic number "-1" mean?

Invalid fd.

> We often use it to signal
> "uninitialized", but then what are concrete "initialized" values
> mean?  "We dup2()'ed something else to stderr/fd #2 but before doing
> so we saved the original fd #2 away to this variable, so that we can
> restore fd #2 by another dup2() of the value of this variable when
> we declare that we are done with the standard error stream"?
> 
> But that does not look like what is happening here.
> 
> >  /* 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 (old_fd2 != -1)
> > +		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)) {
> > +		old_fd2 = 1;
> 
> Equally unclear magic number "1" is used here.
> 
> This value is different from pager_process.in, and my earlier "we
> are saving away" does not apply, either.

It applies, in 3/5.

> 
> >  		dup2(pager_process.in, 2);
> > +	}
> >  	close(pager_process.in);
> 
> Puzzled...

Thanks for reading the series.

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

[Junio C Hamano]

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

>> >  static struct child_process pager_process;
>> >  static const char *pager_program;
>> > +static int old_fd2 = -1;
>> 
>> What does the magic number "-1" mean?
>
> Invalid fd.
>
>> We often use it to signal
>> "uninitialized", but then what are concrete "initialized" values
>> mean?  "We dup2()'ed something else to stderr/fd #2 but before doing
>> so we saved the original fd #2 away to this variable, so that we can
>> restore fd #2 by another dup2() of the value of this variable when
>> we declare that we are done with the standard error stream"?
>> 
>> But that does not look like what is happening here.
>>  ....
>> Equally unclear magic number "1" is used here.
>> 
>> This value is different from pager_process.in, and my earlier "we
>> are saving away" does not apply, either.
>
> It applies, in 3/5.

We need to be prepared to see a series chomped at an early stage and
it should still make sense.  If the series does not make sense when
you stop before applying patch 3, it is a strong sign that this step
and the next step can be separated and structured better.

Or perhaps if they are made into a single patch it makes more sense
and becomes easier to explain?

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

[Rubén Justo]

On Tue, May 21, 2024 at 01:57:19PM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> >> >  static struct child_process pager_process;
> >> >  static const char *pager_program;
> >> > +static int old_fd2 = -1;
> >> 
> >> What does the magic number "-1" mean?
> >
> > Invalid fd.
> >
> >> We often use it to signal
> >> "uninitialized", but then what are concrete "initialized" values
> >> mean?  "We dup2()'ed something else to stderr/fd #2 but before doing
> >> so we saved the original fd #2 away to this variable, so that we can
> >> restore fd #2 by another dup2() of the value of this variable when
> >> we declare that we are done with the standard error stream"?
> >> 
> >> But that does not look like what is happening here.
> >>  ....
> >> Equally unclear magic number "1" is used here.
> >> 
> >> This value is different from pager_process.in, and my earlier "we
> >> are saving away" does not apply, either.
> >
> > It applies, in 3/5.
> 
> We need to be prepared to see a series chomped at an early stage and
> it should still make sense.  If the series does not make sense when
> you stop before applying patch 3, it is a strong sign that this step
> and the next step can be separated and structured better.
> 
> Or perhaps if they are made into a single patch it makes more sense
> and becomes easier to explain?
> 

Adding logic to adjust when we close(stderr) in close_pager_fds() makes
sense on its own, I think.

And, the values for the flag "do-we-want-to-close-stderr-at-exit", too,
to me.

I am happy with the series;  the 'P' command introduced in v2 is a good
improvement.  Combining 2/5 and 3/5, I think it is not a good idea.

Therefore, I'm not sure how to alleviate the puzzling.

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

[Junio C Hamano]

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

> Adding logic to adjust when we close(stderr) in close_pager_fds() makes
> sense on its own, I think.

The feature may be.

> And, the values for the flag "do-we-want-to-close-stderr-at-exit", too,
> to me.

But the thing is, the flag is *NOT* named as such, and an
undocumented "value -1 means X, while value 1 means Y", do not make
any sense, either.

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

[Rubén Justo]

On Tue, May 21, 2024 at 03:00:10PM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > Adding logic to adjust when we close(stderr) in close_pager_fds() makes
> > sense on its own, I think.
> 
> The feature may be.
> 
> > And, the values for the flag "do-we-want-to-close-stderr-at-exit", too,
> > to me.
> 
> But the thing is, the flag is *NOT* named as such, and an
> undocumented "value -1 means X, while value 1 means Y", do not make
> any sense, either

Perhaps this makes more sense?:

1:  8fe915a820 ! 1:  70cc34efc4 pager: do not close fd 2 unnecessarily
    @@ pager.c: int pager_use_color = 1;
      
      static struct child_process pager_process;
      static const char *pager_program;
    -+static int old_fd2 = -1;
    ++static int old_fd2;
      
      /* Is the value coming back from term_columns() just a guess? */
      static int term_columns_guessed;
    @@ pager.c: static void close_pager_fds(void)
      	/* signal EOF to pager */
      	close(1);
     -	close(2);
    -+	if (old_fd2 != -1)
    ++	if (old_fd2)
     +		close(2);
      }

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

[Junio C Hamano]

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

> Perhaps this makes more sense?:
>
> 1:  8fe915a820 ! 1:  70cc34efc4 pager: do not close fd 2 unnecessarily
>     @@ pager.c: int pager_use_color = 1;
>       
>       static struct child_process pager_process;
>       static const char *pager_program;
>     -+static int old_fd2 = -1;
>     ++static int old_fd2;
>       
>       /* Is the value coming back from term_columns() just a guess? */
>       static int term_columns_guessed;
>     @@ pager.c: static void close_pager_fds(void)
>       	/* signal EOF to pager */
>       	close(1);
>      -	close(2);
>     -+	if (old_fd2 != -1)
>     ++	if (old_fd2)
>      +		close(2);
>       }

Not really.  The name "old_fd2" strongly implies "where did fd#2
come from?" and it did not come from fd#0, did it?  Until [3/5]
this variable used to mean something different from "this was the
saved fd#2 we can use to restore it later", which is the name
"old_fd2" clearly wants to stand for.

If you really want to have them as two separate patches, I would
expect the proposed log message for the [3/5] step to say something
like

    ... we added variable X to signal if we should close fd#2 in
    function F in the previous step.  As store away the original
    fd#2 with dup(2) to be restored later after we close() it, the
    question the previous step asked, "should we be the one closing
    fd#2?" becomes equivalent to "have we stored away the original
    fd#2 (in which case we close() fd#2 when we are done with the
    pager and restore the original one)?"  Rename X to old_fd2 and
    have it serve both purposes.

or somesuch.

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

[Rubén Justo]

On Wed, May 22, 2024 at 10:40:18AM -0700, Junio C Hamano wrote:

> Not really.  The name "old_fd2" strongly implies "where did fd#2
> come from?" and it did not come from fd#0, did it?

Perhaps "close_fd2" is a better name?:

@@ pager.c: int pager_use_color = 1;
  
  static struct child_process pager_process;
  static const char *pager_program;
-+static int old_fd2 = -1;
++static int close_fd2;
  
  /* Is the value coming back from term_columns() just a guess? */
  static int term_columns_guessed;

@@ pager.c: static void close_pager_fds(void)
  	/* signal EOF to pager */
  	close(1);
 -	close(2);
-+	if (old_fd2 != -1)
++	if (close_fd2)
 +		close(2);
  }

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

[Junio C Hamano]

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

> On Wed, May 22, 2024 at 10:40:18AM -0700, Junio C Hamano wrote:
>
>> Not really.  The name "old_fd2" strongly implies "where did fd#2
>> come from?" and it did not come from fd#0, did it?
>
> Perhaps "close_fd2" is a better name?:
> @@ pager.c: static void close_pager_fds(void)
>   	/* signal EOF to pager */
>   	close(1);
>  -	close(2);
> -+	if (old_fd2 != -1)
> ++	if (close_fd2)
>  +		close(2);

That's a very straight-forward name that says what effect anybody
who assigns to the variable wants to see.

[PATCH 3/5] 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 | 37 +++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 3ef6798f7e..2fa06c43c4 100644
--- a/pager.c
+++ b/pager.c
@@ -14,12 +14,11 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static const char *pager_program;
-static int old_fd2 = -1;
+static int old_fd1 = -1, old_fd2 = -1;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
 
-
 static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
@@ -30,14 +29,35 @@ static void close_pager_fds(void)
 
 static void wait_for_pager_atexit(void)
 {
+	if (old_fd1 == -1)
+		return;
+
 	fflush(stdout);
 	fflush(stderr);
 	close_pager_fds();
 	finish_command(&pager_process);
 }
 
+void wait_for_pager(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	wait_for_pager_atexit();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		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,11 +133,14 @@ 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)
 		return;
 
+	assert(old_fd1 == -1);
+
 	/*
 	 * After we redirect standard output, we won't be able to use an ioctl
 	 * to get the terminal size. Let's grab it now, and then set $COLUMNS
@@ -142,16 +165,18 @@ void setup_pager(void)
 		return;
 
 	/* original process continues, but writes to the pipe */
+	old_fd1 = dup(1);
 	dup2(pager_process.in, 1);
 	if (isatty(2)) {
-		old_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++) {
+		sigchain_push_common(wait_for_pager_signal);
+		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.209.gd5886bf9cd

[PATCH 4/5] test-terminal: introduce --no-stdin-pty

[Rubén Justo]

In 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
2015-08-04), t/test-terminal.perl learned to connect the child process'
stdin to a pty.  It works well for what was intended: satisfying an
`isatty(STDIN_FILENO)` check.

However, the fork introduced, that copies the stdin to the child
process, does not always manage to send all the information.

To illustrate this behavior, we can use a function like this:

    f ()
    {
    	dd if=/dev/zero bs=1 count=10000 status=none |
    	t/test-terminal.perl cat - 2>/dev/null |
    	wc -c;
    }

We do not obtain the expected results when executing this function
100 times:

    $ for i in $(seq 100); do f; done | sort | uniq -c
         36 0
          4 1
         53 4095
          7 4159

If we do the same with a version that does not redirect stdin, a version
prior to 18d8c26930, the expected result is obtained:

    $ git checkout 18d8c26930~1
    $ for i in $(seq 100); do f; done | sort | uniq -c
        100 10000

In a subsequent commit, a new test is going to rely on test-terminate,
and it does not require stdin to be connected to a terminal, but all
piped data needs to be successfully transmitted to the child process.

To make this possible, add a new parameter "--no-stdin-pty" to allow
disabling the stdin redirection though a pty.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/test-terminal.perl | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 3810e9bb43..85edc9e8b9 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -12,10 +12,10 @@ sub start_child {
 	if (not defined $pid) {
 		die "fork failed: $!"
 	} elsif ($pid == 0) {
-		open STDIN, "<&", $in;
+		open STDIN, "<&", $in if $in;
 		open STDOUT, ">&", $out;
 		open STDERR, ">&", $err;
-		close $in;
+		close $in if $in;
 		close $out;
 		exec(@$argv) or die "cannot exec '$argv->[0]': $!"
 	}
@@ -78,28 +78,32 @@ sub copy_stdio {
 }
 
 if ($#ARGV < 1) {
-	die "usage: test-terminal program args";
+	die "usage: test-terminal [--no-stdin-pty] program args";
 }
+my $no_stdin_pty = $ARGV[0] eq '--no-stdin-pty';
+shift @ARGV if $no_stdin_pty;
 $ENV{TERM} = 'vt100';
-my $parent_in = new IO::Pty;
+my $parent_in = $no_stdin_pty ? undef : IO::Pty->new;
 my $parent_out = new IO::Pty;
 my $parent_err = new IO::Pty;
-$parent_in->set_raw();
+$parent_in->set_raw() if $parent_in;
 $parent_out->set_raw();
 $parent_err->set_raw();
-$parent_in->slave->set_raw();
+$parent_in->slave->set_raw() if $parent_in;
 $parent_out->slave->set_raw();
 $parent_err->slave->set_raw();
-my $pid = start_child(\@ARGV, $parent_in->slave, $parent_out->slave, $parent_err->slave);
-close $parent_in->slave;
+my $pid = start_child(\@ARGV,$parent_in ? $parent_in->slave : undef, $parent_out->slave, $parent_err->slave);
+close $parent_in->slave if $parent_in;
 close $parent_out->slave;
 close $parent_err->slave;
-my $in_pid = copy_stdin($parent_in);
+my $in_pid = $no_stdin_pty ? 0 : copy_stdin($parent_in);
 copy_stdio($parent_out, $parent_err);
 my $ret = finish_child($pid);
-# If the child process terminates before our copy_stdin() process is able to
-# write all of its data to $parent_in, the copy_stdin() process could stall.
-# Send SIGTERM to it to ensure it terminates.
-kill 'TERM', $in_pid;
-finish_child($in_pid);
+if ($in_pid) {
+	# If the child process terminates before our copy_stdin() process is able to
+	# write all of its data to $parent_in, the copy_stdin() process could stall.
+	# Send SIGTERM to it to ensure it terminates.
+	kill 'TERM', $in_pid;
+	finish_child($in_pid);
+}
 exit($ret);
-- 
2.45.1.209.gd5886bf9cd

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

[Rubén Justo]

Invoke the pager when displaying hunks during "add -p" sessions, to make
it easier for the user to review hunks longer than one screen height.

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

diff --git a/add-patch.c b/add-patch.c
index 2252895c28..cefa3941a3 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -5,6 +5,7 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "pager.h"
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "strbuf.h"
@@ -1448,9 +1449,11 @@ 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) {
+				setup_pager();
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
+				wait_for_pager();
 			}
 
 			strbuf_reset(&s->buf);
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 52d7830de2..6c4af8904e 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -558,6 +558,27 @@ 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 &&
+	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>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 --no-stdin-pty git add -p >actual
+	) &&
+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
 test_expect_success 'navigate to hunk via regex' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-- 
2.45.1.209.gd5886bf9cd

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

[Junio C Hamano]

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

> Invoke the pager when displaying hunks during "add -p" sessions, to make
> it easier for the user to review hunks longer than one screen height.

If the hunk fits on one screen, it is annoying to see a pager
invoked and then torn down immediately, even with "less -F"
(--quit-if-one-screen).  As we know how much output we are throwing
at the user, we'd want to make this conditional to the size of the
hunk being shown and the terminal height.

Or perhaps show them without such a trick, and add a new variant to
'p' that allows the user to request the output be sent to a pager
(perhaps 'P')?  It would certainly be an alternative with much
smaller damage.  The existing end-user experience would not degrade,
but when the user wants to see a huge hunk, they can send it to the
pager.

Another, ulteriour, motive here behind this suggestion is to
encourage users to work with smaller hunks.  Being able to scroll
around and view lines on demand (i.e. use of pager) is one thing.
Being able to view all relevant lines at once (i.e. not wasting
vertical screen real estate and making things fit on one screen) is
very different and much nicer.

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

[Dragan Simic]

Hello Junio and Ruben,

On 2024-05-20 21:30, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
>> Invoke the pager when displaying hunks during "add -p" sessions, to 
>> make
>> it easier for the user to review hunks longer than one screen height.
> 
> If the hunk fits on one screen, it is annoying to see a pager
> invoked and then torn down immediately, even with "less -F"
> (--quit-if-one-screen).  As we know how much output we are throwing
> at the user, we'd want to make this conditional to the size of the
> hunk being shown and the terminal height.
> 
> Or perhaps show them without such a trick, and add a new variant to
> 'p' that allows the user to request the output be sent to a pager
> (perhaps 'P')?  It would certainly be an alternative with much
> smaller damage.  The existing end-user experience would not degrade,
> but when the user wants to see a huge hunk, they can send it to the
> pager.
> 
> Another, ulteriour, motive here behind this suggestion is to
> encourage users to work with smaller hunks.  Being able to scroll
> around and view lines on demand (i.e. use of pager) is one thing.
> Being able to view all relevant lines at once (i.e. not wasting
> vertical screen real estate and making things fit on one screen) is
> very different and much nicer.

There's another thing to consider, which makes the introduction of
"P" as the new option even more desirable.  Let me explain.

With the upcoming changes to the way less(1) as the pager works,
which was already discussed at length and even required new features
to be implemented in less(1), [1] displaying anything through less(1)
will not leave an accessible scrollback in the terminal emulator.
Only one screen worth of text will be displayed, even after quitting
less(1).  That's what we have to do, to fix age-old issues with the
pager-generated scrollback that easily gets corrupted and actually
becomes misleading.

Thus, if someone wants to have a complete longer-than-one-screen hunk
displayed and use the terminal emulator scrollback to inspect the
hunk in its entirety, passing such (or all) hunks through the pager
would make such inspection impossible.  I'd assume that at least some
Git users already do that (I do, for example), and we surely don't want
to make that no longer possible.  That's why introducing "P" as the
new option would be the desired approach.

[1] 
https://lore.kernel.org/git/8289ef15266172cbfa10bb146afe9797@manjaro.org/

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

[Rubén Justo]

On Mon, May 20, 2024 at 09:45:51PM +0200, Dragan Simic wrote:

> Thus, if someone wants to have a complete longer-than-one-screen hunk
> displayed and use the terminal emulator scrollback to inspect the
> hunk in its entirety, passing such (or all) hunks through the pager
> would make such inspection impossible.  I'd assume that at least some
> Git users already do that (I do, for example), and we surely don't want
> to make that no longer possible.

I'm not sure if I understand the problem... disabling the pager is
still an option, no?

    $ git -P add -p

    $ git -c pager.add=false add -p

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

[Dragan Simic]

On 2024-05-21 00:35, Rubén Justo wrote:
> On Mon, May 20, 2024 at 09:45:51PM +0200, Dragan Simic wrote:
> 
>> Thus, if someone wants to have a complete longer-than-one-screen hunk
>> displayed and use the terminal emulator scrollback to inspect the
>> hunk in its entirety, passing such (or all) hunks through the pager
>> would make such inspection impossible.  I'd assume that at least some
>> Git users already do that (I do, for example), and we surely don't 
>> want
>> to make that no longer possible.
> 
> I'm not sure if I understand the problem... disabling the pager is
> still an option, no?
> 
>     $ git -P add -p
> 
>     $ git -c pager.add=false add -p

Please read the thread [1] that I linked in my previous response 
carefully.
I know, there's _a lot_ of text, but I already tried to sum it all up in 
my
previous response.  There's even a video clip [2] in that thread that 
shows
the issue with the corrupted scrollback in a terminal emulator.

Frankly, the "-c pager.add=false" approach is a bit cumbersome.  
Basically,
the new "-P" option would be like "-c pager.add=true", while the already
existing "-p" option would be like "-c pager.add=false".  Though, I 
think
that we don't want to add "pager.add" as a new configuration option, 
because
throwing it into the mix would make the "-p" and "-P" options for "git 
add"
quite confusing.

As another idea, we might also add "p" as another option in the 
"y/n/q/a/d..."
menu when the user decides about each hunk in "git add -p" or "git add 
-P".
When running "git add -p", pressing "p" would display the current hunk 
using
the pager (which would be opposite to the "git add -p"'s behavior of not
using the pager), and when running "git add -P", pressing "p" would 
display
the current hunk by bypassing the pager (again, opposite to the "add 
-P"'s
behavior).  That would allow greatest level of flexibility.

[1] 
https://lore.kernel.org/git/8289ef15266172cbfa10bb146afe9797@manjaro.org/T/#u
[2] https://youtu.be/MsxtQgrKM50

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

[Rubén Justo]

On Tue, May 21, 2024 at 01:54:22AM +0200, Dragan Simic wrote:

> Though, I think that we don't want to add "pager.add" as a new
> configuration option

I have no plans to add a new git-add(1) option or a new configuration
option.  Only a new interactive option 'P'.

I do not see the need for them, but maybe I'm missing some use case.

I'm going send a new iteration, v2;  please, take a look at it.

Thanks.

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

[Jeff King]

On Mon, May 20, 2024 at 09:45:51PM +0200, Dragan Simic wrote:

> > Another, ulteriour, motive here behind this suggestion is to
> > encourage users to work with smaller hunks.  Being able to scroll
> > around and view lines on demand (i.e. use of pager) is one thing.
> > Being able to view all relevant lines at once (i.e. not wasting
> > vertical screen real estate and making things fit on one screen) is
> > very different and much nicer.
> 
> There's another thing to consider, which makes the introduction of
> "P" as the new option even more desirable.  Let me explain.
> 
> With the upcoming changes to the way less(1) as the pager works,
> which was already discussed at length and even required new features
> to be implemented in less(1), [1] displaying anything through less(1)
> will not leave an accessible scrollback in the terminal emulator.
> Only one screen worth of text will be displayed, even after quitting
> less(1).  That's what we have to do, to fix age-old issues with the
> pager-generated scrollback that easily gets corrupted and actually
> becomes misleading.

This feature can be annoying even with current versions of less,
depending on your $LESS variable. If you don't set "F" you'll get a
pager for short inputs, and if you don't set "X" then even small hunks
are cleared from the screen while we ask about them.

So this definitely needs to be configurable, and I'd be tempted to say
it should be off by default, just because we don't know how the user's
pager will behave when invoked for multiple short snippets like this (it
might not even be "less", after all).

I don't think setting pager.add is enough here. You'd also need to set
pager.checkout, pager.reset, and so on, since their interactive modes
all invoke the same code. We'd presumably want a single config option
(and possibly even one that could be set to a distinct pager command for
this context, rather than the usual one).

-Peff

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

[Rubén Justo]

On Tue, May 21, 2024 at 03:07:52AM -0400, Jeff King wrote:
> On Mon, May 20, 2024 at 09:45:51PM +0200, Dragan Simic wrote:
> 
> > > Another, ulteriour, motive here behind this suggestion is to
> > > encourage users to work with smaller hunks.  Being able to scroll
> > > around and view lines on demand (i.e. use of pager) is one thing.
> > > Being able to view all relevant lines at once (i.e. not wasting
> > > vertical screen real estate and making things fit on one screen) is
> > > very different and much nicer.
> > 
> > There's another thing to consider, which makes the introduction of
> > "P" as the new option even more desirable.  Let me explain.
> > 
> > With the upcoming changes to the way less(1) as the pager works,
> > which was already discussed at length and even required new features
> > to be implemented in less(1), [1] displaying anything through less(1)
> > will not leave an accessible scrollback in the terminal emulator.
> > Only one screen worth of text will be displayed, even after quitting
> > less(1).  That's what we have to do, to fix age-old issues with the
> > pager-generated scrollback that easily gets corrupted and actually
> > becomes misleading.
> 
> This feature can be annoying even with current versions of less,

Hopefully, reducing the blast radius to a new 'P' option, will make it
palatable.

> depending on your $LESS variable. If you don't set "F" you'll get a
> pager for short inputs, and if you don't set "X" then even small hunks
> are cleared from the screen while we ask about them.
> 
> So this definitely needs to be configurable, and I'd be tempted to say
> it should be off by default, just because we don't know how the user's
> pager will behave when invoked for multiple short snippets like this (it
> might not even be "less", after all).
> 
> I don't think setting pager.add is enough here. You'd also need to set
> pager.checkout, pager.reset, and so on, since their interactive modes
> all invoke the same code. We'd presumably want a single config option
> (and possibly even one that could be set to a distinct pager command for
> this context, rather than the usual one).
> 
> -Peff

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

[Jeff King]

On Tue, May 21, 2024 at 09:59:55PM +0200, Rubén Justo wrote:

> > This feature can be annoying even with current versions of less,
> 
> Hopefully, reducing the blast radius to a new 'P' option, will make it
> palatable.

Yeah, that would be perfect. I might even use it, then, for the rare
cases when I want to look at a really big hunk. I do still think it
would be useful to be able to configure its pager separately (in my
case, I'd use "less -FX" rather than my default setup, which doesn't use
either of those options). But I'm also OK to leave that for now and let
it be somebody else's itch to scratch later.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do still think it
> would be useful to be able to configure its pager separately (in my
> case, I'd use "less -FX" rather than my default setup, which doesn't use
> either of those options).

Even better.  Allow to optionally have the command after the option,
e.g.,

    (1/1) Use this hunk [y,n,q,j,k,e,p,P] P<RET>
    (1/1) Use this hunk [y,n,q,j,k,e,p,P] Pless -FX<RET>
    (1/1) Use this hunk [y,n,q,j,k,e,p,P] Pcat<RET>

The first one feeds the default program with the hunk via pipe, the
second one instead invokes command you specifed, "less -FX", and
feeds the hunk to it via a pipe.  The last one emulates a plain 'p'
behaviour.

And for usability, perhaps giving a specific command would change
the default program a bare 'P' invokes for the rest of the session
until another specific command overrides.  Another usability hack
may be "[interactive] pipecommand = less -FX" configuration variable
gives the initial default for each session.

At that point, we can explain it as

   p - print the current hunk
   P[<program>] - pipe the current hunk to a program

or even use '|' instead of 'P'.

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

[Dragan Simic]

On 2024-05-23 16:00, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> I do still think it
>> would be useful to be able to configure its pager separately (in my
>> case, I'd use "less -FX" rather than my default setup, which doesn't 
>> use
>> either of those options).
> 
> Even better.  Allow to optionally have the command after the option,
> e.g.,
> 
>     (1/1) Use this hunk [y,n,q,j,k,e,p,P] P<RET>
>     (1/1) Use this hunk [y,n,q,j,k,e,p,P] Pless -FX<RET>
>     (1/1) Use this hunk [y,n,q,j,k,e,p,P] Pcat<RET>

Please note that "-X" will no longer be used as one of the options
passed to less(1) as the pager, in the upcoming resolution of the
age-old pager issues. [1]

In more detail, "-X" is actually an ugly hack that was nothing more
than a stopgap measure, but it has never been resolved properly.  That
is, until recently, when I started to collaborate with the author of
less(1) towards finding and implementing the real solution.

[1] 
https://lore.kernel.org/git/8289ef15266172cbfa10bb146afe9797@manjaro.org/T/#u

> The first one feeds the default program with the hunk via pipe, the
> second one instead invokes command you specifed, "less -FX", and
> feeds the hunk to it via a pipe.  The last one emulates a plain 'p'
> behaviour.

Frankly, that would be a lot of typing, and may even open a path for
some unforeseen security issues.

> And for usability, perhaps giving a specific command would change
> the default program a bare 'P' invokes for the rest of the session
> until another specific command overrides.  Another usability hack
> may be "[interactive] pipecommand = less -FX" configuration variable
> gives the initial default for each session.

I think that would be way too complicated.

> At that point, we can explain it as
> 
>    p - print the current hunk
>    P[<program>] - pipe the current hunk to a program
> 
> or even use '|' instead of 'P'.

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>> And for usability, perhaps giving a specific command would change
>> the default program a bare 'P' invokes for the rest of the session
>> until another specific command overrides.  Another usability hack
>> may be "[interactive] pipecommand = less -FX" configuration variable
>> gives the initial default for each session.
>
> I think that would be way too complicated.

It is modelled after how "less" and "vi" remembers the last pattern
fed to their "/" command.  You once give, say, "/test_<ENTER>" to
find one instance of "test_", then "/<ENTER>" takes to the next
instance.

As I expect our target audiences are used to such a behaviour, I do
not think I agree with your "way too complicated".

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

[Dragan Simic]

On 2024-05-24 01:04, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>>> And for usability, perhaps giving a specific command would change
>>> the default program a bare 'P' invokes for the rest of the session
>>> until another specific command overrides.  Another usability hack
>>> may be "[interactive] pipecommand = less -FX" configuration variable
>>> gives the initial default for each session.
>> 
>> I think that would be way too complicated.
> 
> It is modelled after how "less" and "vi" remembers the last pattern
> fed to their "/" command.  You once give, say, "/test_<ENTER>" to
> find one instance of "test_", then "/<ENTER>" takes to the next
> instance.

Huh, less(1) actually remembers nothing when the secure mode is
turned on.  That's another thing I've collaborated with the author
of less(1), to make it possible to remember the last search term
when running less(1) in secure mode.

> As I expect our target audiences are used to such a behaviour, I do
> not think I agree with your "way too complicated".

Hmm.  Where would that state be stored?

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

[Dragan Simic]

On 2024-05-24 01:28, Dragan Simic wrote:
> On 2024-05-24 01:04, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> 
>>>> And for usability, perhaps giving a specific command would change
>>>> the default program a bare 'P' invokes for the rest of the session
>>>> until another specific command overrides.  Another usability hack
>>>> may be "[interactive] pipecommand = less -FX" configuration variable
>>>> gives the initial default for each session.
>>> 
>>> I think that would be way too complicated.
>> 
>> It is modelled after how "less" and "vi" remembers the last pattern
>> fed to their "/" command.  You once give, say, "/test_<ENTER>" to
>> find one instance of "test_", then "/<ENTER>" takes to the next
>> instance.
> 
> Huh, less(1) actually remembers nothing when the secure mode is
> turned on.  That's another thing I've collaborated with the author
> of less(1), to make it possible to remember the last search term
> when running less(1) in secure mode.
> 
>> As I expect our target audiences are used to such a behaviour, I do
>> not think I agree with your "way too complicated".
> 
> Hmm.  Where would that state be stored?

Ah, sorry, I misread your description a bit.  There's obviously no need
to store any state, because this additional feature doesn't break the
boundaries of a single session.

However, I'd suggest that we leave this additional feature aside for 
now,
until the upcoming pager-related changes and fixes settle down.

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> On 2024-05-24 01:04, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> 
>>>> And for usability, perhaps giving a specific command would change
>>>> the default program a bare 'P' invokes for the rest of the session
>>>> until another specific command overrides.  Another usability hack
>>>> may be "[interactive] pipecommand = less -FX" configuration variable
>>>> gives the initial default for each session.
>>> I think that would be way too complicated.
>> It is modelled after how "less" and "vi" remembers the last pattern
>> fed to their "/" command.  You once give, say, "/test_<ENTER>" to
>> find one instance of "test_", then "/<ENTER>" takes to the next
>> instance.
>
> Huh, less(1) actually remembers nothing when the secure mode is
> turned on.

Are you sure you read me right?  I wasn't talking about storing
anything on disk for the "usability hack", and made it explicitly
clear with "for the rest of the session".

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

[Dragan Simic]

On 2024-05-24 01:54, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> On 2024-05-24 01:04, Junio C Hamano wrote:
>>> Dragan Simic <dsimic@manjaro.org> writes:
>>> 
>>>>> And for usability, perhaps giving a specific command would change
>>>>> the default program a bare 'P' invokes for the rest of the session
>>>>> until another specific command overrides.  Another usability hack
>>>>> may be "[interactive] pipecommand = less -FX" configuration 
>>>>> variable
>>>>> gives the initial default for each session.
>>>> I think that would be way too complicated.
>>> 
>>> It is modelled after how "less" and "vi" remembers the last pattern
>>> fed to their "/" command.  You once give, say, "/test_<ENTER>" to
>>> find one instance of "test_", then "/<ENTER>" takes to the next
>>> instance.
>> 
>> Huh, less(1) actually remembers nothing when the secure mode is
>> turned on.
> 
> Are you sure you read me right?  I wasn't talking about storing
> anything on disk for the "usability hack", and made it explicitly
> clear with "for the rest of the session".

I misread it at first, but I corrected myself. [1]

[1] 
https://lore.kernel.org/git/3391a15907a055c281106c4995fb8272@manjaro.org/

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

[Jeff King]

On Thu, May 23, 2024 at 07:00:47AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I do still think it
> > would be useful to be able to configure its pager separately (in my
> > case, I'd use "less -FX" rather than my default setup, which doesn't use
> > either of those options).
> 
> Even better.  Allow to optionally have the command after the option,
> e.g.,
> 
>     (1/1) Use this hunk [y,n,q,j,k,e,p,P] P<RET>
>     (1/1) Use this hunk [y,n,q,j,k,e,p,P] Pless -FX<RET>
>     (1/1) Use this hunk [y,n,q,j,k,e,p,P] Pcat<RET>
> 
> The first one feeds the default program with the hunk via pipe, the
> second one instead invokes command you specifed, "less -FX", and
> feeds the hunk to it via a pipe.  The last one emulates a plain 'p'
> behaviour.
> 
> And for usability, perhaps giving a specific command would change
> the default program a bare 'P' invokes for the rest of the session
> until another specific command overrides.  Another usability hack
> may be "[interactive] pipecommand = less -FX" configuration variable
> gives the initial default for each session.
> 
> At that point, we can explain it as
> 
>    p - print the current hunk
>    P[<program>] - pipe the current hunk to a program
> 
> or even use '|' instead of 'P'.

Ooh, I like all of that (including "|", which is what triggers the
similar feature in mutt). If interactive.pipeCommand defaults to the
usual pager, then a bare "|" would do what most people want.

-Peff

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

[Rubén Justo]

On Thu, May 23, 2024 at 05:06:01AM -0400, Jeff King wrote:
> On Tue, May 21, 2024 at 09:59:55PM +0200, Rubén Justo wrote:
> 
> > > This feature can be annoying even with current versions of less,
> > 
> > Hopefully, reducing the blast radius to a new 'P' option, will make it
> > palatable.
> 
> Yeah, that would be perfect. I might even use it, then, for the rare
> cases when I want to look at a really big hunk.

In addition to that, I have two use-cases that make sense to me:

  - avoiding a huuge but split-able hunk to go all through my terminal
    before I can say: split, 's'.  For this, perhaps the '-P' suggested
    by Dragan is the way to go.

  - a lot of mid-sized hunks that I only need to see, to decide, the
    result of "head -3".  Here, the pager would be acting as a 'filter'.

Perhaps I am stretching the meaning of 'pager' too far...

> I do still think it would be useful to be able to configure its pager
> separately (in my case, I'd use "less -FX" rather than my default
> setup, which doesn't use either of those options).

A new "interactive.pager" setting?  Perhaps with higher preference than
"add.pager"?  Just questioning, do not take this as an intention of
scratching that itch :-)

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

[Dragan Simic]

On 2024-05-24 00:25, Rubén Justo wrote:
> On Thu, May 23, 2024 at 05:06:01AM -0400, Jeff King wrote:

[...]

> In addition to that, I have two use-cases that make sense to me:
> 
>   - avoiding a huuge but split-able hunk to go all through my terminal
>     before I can say: split, 's'.  For this, perhaps the '-P' suggested
>     by Dragan is the way to go.

A possible UX issue with the "-P" option is that the menu wouldn't
be displayed right after executing "git add -P" if the first displayed
hunk is longer than one screen, leaving the users wondering what
actually happened.  Though, that perhaps could be addressed in the
documentation.

>   - a lot of mid-sized hunks that I only need to see, to decide, the
>     result of "head -3".  Here, the pager would be acting as a 
> 'filter'.
> 
> Perhaps I am stretching the meaning of 'pager' too far...
> 
>> I do still think it would be useful to be able to configure its pager
>> separately (in my case, I'd use "less -FX" rather than my default
>> setup, which doesn't use either of those options).
> 
> A new "interactive.pager" setting?  Perhaps with higher preference than
> "add.pager"?  Just questioning, do not take this as an intention of
> scratching that itch :-)

Huh, I see some possible issues with separate pager configurations,
resulting from the upcoming rework of the default less(1)-as-pager
options, so perhaps it would, in addition, be better to wait until
those changes settle first.

I intend to get into that rather soon, not only for Git, but also
for a few other projects that use less(1) as their pager by default,
such as util-linux.

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

[Rubén Justo]

On Mon, May 20, 2024 at 12:30:50PM -0700, Junio C Hamano wrote:

> > Invoke the pager when displaying hunks during "add -p" sessions, to make
> > it easier for the user to review hunks longer than one screen height.
> 
> If the hunk fits on one screen, it is annoying to see a pager
> invoked and then torn down immediately,

Good point.

> even with "less -F"
> (--quit-if-one-screen).  As we know how much output we are throwing
> at the user, we'd want to make this conditional to the size of the
> hunk being shown and the terminal height.

Are you thinking of something like?:
 
diff --git a/add-patch.c b/add-patch.c
index cefa3941a3..495baad3ac 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1449,11 +1449,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) {
-				setup_pager();
+				int lines = 0;
 				render_hunk(s, hunk, 0, colored, &s->buf);
+				for(int i = 0; i < s->buf.len; i++) {
+					if (s->buf.buf[i] == '\n')
+						lines++;
+				}
+				if (lines > term_columns())
+					setup_pager();
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
-				wait_for_pager();
+				if (lines > term_columns())
+					wait_for_pager();
 			}
 
 			strbuf_reset(&s->buf);

This would significantly reduce the blast radius.

Thanks.

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

[Junio C Hamano]

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

>> even with "less -F"
>> (--quit-if-one-screen).  As we know how much output we are throwing
>> at the user, we'd want to make this conditional to the size of the
>> hunk being shown and the terminal height.
>
> Are you thinking of something like?:

I don't.  

Your hunk may have overly wide lines in which case counting the
number of lines may be insuffucient to measure the necessary display
height.   Besides, comparison with term_columns() is meaningless
unless your window is square ;-)

An explicit 'P' might be palatable, though.

Thanks.

>  
> diff --git a/add-patch.c b/add-patch.c
> index cefa3941a3..495baad3ac 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1449,11 +1449,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) {
> -				setup_pager();
> +				int lines = 0;
>  				render_hunk(s, hunk, 0, colored, &s->buf);
> +				for(int i = 0; i < s->buf.len; i++) {
> +					if (s->buf.buf[i] == '\n')
> +						lines++;
> +				}
> +				if (lines > term_columns())
> +					setup_pager();
>  				fputs(s->buf.buf, stdout);
>  				rendered_hunk_index = hunk_index;
> -				wait_for_pager();
> +				if (lines > term_columns())
> +					wait_for_pager();
>  			}
>  
>  			strbuf_reset(&s->buf);
>
> This would significantly reduce the blast radius.
>
> Thanks.

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

[Rubén Justo]

On Mon, May 20, 2024 at 04:18:33PM -0700, Junio C Hamano wrote:

> Besides, comparison with term_columns() is meaningless
> unless your window is square ;-)

XD

> 
> An explicit 'P' might be palatable, though.

OK.  Thanks.

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

[Rubén Justo]

Invoke the pager when displaying hunks during "add -p" sessions, to make                                                                     
it easier for the user to review hunks longer than one screen height.                                                                        
                                                                                                                                             
This iteration, v2, reduces the use of the pager to the 'print' command
when invoked using a capital 'P'.                 

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

 add-patch.c                | 14 ++++++++++---
 pager.c                    | 41 ++++++++++++++++++++++++++++++++------
 pager.h                    |  1 +
 t/t3701-add-interactive.sh | 37 ++++++++++++++++++++++++++++++++++
 t/test-terminal.perl       | 32 ++++++++++++++++-------------
 5 files changed, 102 insertions(+), 23 deletions(-)

Range-diff against v1:
1:  340b090e64 = 1:  05edb72326 add-patch: test for 'p' command
2:  7cb16742ef = 2:  8fe915a820 pager: do not close fd 2 unnecessarily
3:  207faad1b1 = 3:  fc629a7334 pager: introduce wait_for_pager
4:  a3815e20c3 = 4:  d2bd591e65 test-terminal: introduce --no-stdin-pty
5:  d5886bf9cd ! 5:  d3c11dbb1d add-patch: render hunks through the pager
    @@ Metadata
      ## Commit message ##
         add-patch: render hunks through the pager
     
    -    Invoke the pager when displaying hunks during "add -p" sessions, to make
    -    it easier for the user to review hunks longer than one screen height.
    +    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
      #include "read-cache-ll.h"
      #include "repository.h"
      #include "strbuf.h"
    +@@ add-patch.c: 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,
    +@@ add-patch.c: 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,
     @@ add-patch.c: 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) {
    -+				setup_pager();
    ++				if (use_pager)
    ++					setup_pager();
      				render_hunk(s, hunk, 0, colored, &s->buf);
      				fputs(s->buf.buf, stdout);
      				rendered_hunk_index = hunk_index;
    -+				wait_for_pager();
    ++				if (use_pager) {
    ++					wait_for_pager();
    ++					use_pager = 0;
    ++				}
      			}
      
      			strbuf_reset(&s->buf);
    +@@ add-patch.c: 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;
    + 
     
      ## t/t3701-add-interactive.sh ##
     @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '
    @@ t/t3701-add-interactive.sh: test_expect_success 'print again the hunk' '
     +test_expect_success TTY 'print again the hunk (PAGER)' '
     +	test_when_finished "git reset" &&
     +	cat >expect <<-EOF &&
    -+	PAGER <GREEN>+<RESET><GREEN>15<RESET>
    -+	PAGER  20<RESET>
    ++	<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 |
    ++	test_write_lines s y g 1 P |
     +	(
     +		GIT_PAGER="sed s/^/PAGER\ /" &&
     +		export GIT_PAGER &&
-- 
2.45.1.221.gd3c11dbb1d

[PATCH v2 1/5] 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 28a95a775d..52d7830de2 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -542,6 +542,22 @@ test_expect_success 'goto hunk' '
 	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 'navigate to hunk via regex' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-- 
2.45.1.221.gd3c11dbb1d

[PATCH v2 2/5] 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 might call close_pager_fds multiple times.  With this in mind,
unconditionally closing stderr will become undesirable.

Let's close(STDERR_FILENO) only when necessary, and pave the way for the
comming 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 b8822a9381..3ef6798f7e 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static const char *pager_program;
+static int old_fd2 = -1;
 
 /* 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 (old_fd2 != -1)
+		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)) {
+		old_fd2 = 1;
 		dup2(pager_process.in, 2);
+	}
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
2.45.1.221.gd3c11dbb1d

[PATCH v2 3/5] 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 | 37 +++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 3ef6798f7e..2fa06c43c4 100644
--- a/pager.c
+++ b/pager.c
@@ -14,12 +14,11 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static const char *pager_program;
-static int old_fd2 = -1;
+static int old_fd1 = -1, old_fd2 = -1;
 
 /* Is the value coming back from term_columns() just a guess? */
 static int term_columns_guessed;
 
-
 static void close_pager_fds(void)
 {
 	/* signal EOF to pager */
@@ -30,14 +29,35 @@ static void close_pager_fds(void)
 
 static void wait_for_pager_atexit(void)
 {
+	if (old_fd1 == -1)
+		return;
+
 	fflush(stdout);
 	fflush(stderr);
 	close_pager_fds();
 	finish_command(&pager_process);
 }
 
+void wait_for_pager(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	wait_for_pager_atexit();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		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,11 +133,14 @@ 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)
 		return;
 
+	assert(old_fd1 == -1);
+
 	/*
 	 * After we redirect standard output, we won't be able to use an ioctl
 	 * to get the terminal size. Let's grab it now, and then set $COLUMNS
@@ -142,16 +165,18 @@ void setup_pager(void)
 		return;
 
 	/* original process continues, but writes to the pipe */
+	old_fd1 = dup(1);
 	dup2(pager_process.in, 1);
 	if (isatty(2)) {
-		old_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++) {
+		sigchain_push_common(wait_for_pager_signal);
+		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.221.gd3c11dbb1d

[PATCH v2 4/5] test-terminal: introduce --no-stdin-pty

[Rubén Justo]

In 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
2015-08-04), t/test-terminal.perl learned to connect the child process'
stdin to a pty.  It works well for what was intended: satisfying an
`isatty(STDIN_FILENO)` check.

However, the fork introduced, that copies the stdin to the child
process, does not always manage to send all the information.

To illustrate this behaviour, we can use a function like this:

    f ()
    {
    	dd if=/dev/zero bs=1 count=10000 status=none |
    	t/test-terminal.perl cat - 2>/dev/null |
    	wc -c;
    }

We do not obtain the expected results when executing this function
100 times:

    $ for i in $(seq 100); do f; done | sort | uniq -c
         36 0
          4 1
         53 4095
          7 4159

If we do the same with a version that does not redirect stdin, a version
prior to 18d8c26930, the expected result is obtained:

    $ git checkout 18d8c26930~1
    $ for i in $(seq 100); do f; done | sort | uniq -c
        100 10000

In a subsequent commit, a new test is going to rely on test-terminate,
and it does not require stdin to be connected to a terminal, but all
piped data needs to be successfully transmitted to the child process.

To make this possible, add a new parameter "--no-stdin-pty" to allow
disabling the stdin redirection though a pty.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/test-terminal.perl | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 3810e9bb43..85edc9e8b9 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -12,10 +12,10 @@ sub start_child {
 	if (not defined $pid) {
 		die "fork failed: $!"
 	} elsif ($pid == 0) {
-		open STDIN, "<&", $in;
+		open STDIN, "<&", $in if $in;
 		open STDOUT, ">&", $out;
 		open STDERR, ">&", $err;
-		close $in;
+		close $in if $in;
 		close $out;
 		exec(@$argv) or die "cannot exec '$argv->[0]': $!"
 	}
@@ -78,28 +78,32 @@ sub copy_stdio {
 }
 
 if ($#ARGV < 1) {
-	die "usage: test-terminal program args";
+	die "usage: test-terminal [--no-stdin-pty] program args";
 }
+my $no_stdin_pty = $ARGV[0] eq '--no-stdin-pty';
+shift @ARGV if $no_stdin_pty;
 $ENV{TERM} = 'vt100';
-my $parent_in = new IO::Pty;
+my $parent_in = $no_stdin_pty ? undef : IO::Pty->new;
 my $parent_out = new IO::Pty;
 my $parent_err = new IO::Pty;
-$parent_in->set_raw();
+$parent_in->set_raw() if $parent_in;
 $parent_out->set_raw();
 $parent_err->set_raw();
-$parent_in->slave->set_raw();
+$parent_in->slave->set_raw() if $parent_in;
 $parent_out->slave->set_raw();
 $parent_err->slave->set_raw();
-my $pid = start_child(\@ARGV, $parent_in->slave, $parent_out->slave, $parent_err->slave);
-close $parent_in->slave;
+my $pid = start_child(\@ARGV,$parent_in ? $parent_in->slave : undef, $parent_out->slave, $parent_err->slave);
+close $parent_in->slave if $parent_in;
 close $parent_out->slave;
 close $parent_err->slave;
-my $in_pid = copy_stdin($parent_in);
+my $in_pid = $no_stdin_pty ? 0 : copy_stdin($parent_in);
 copy_stdio($parent_out, $parent_err);
 my $ret = finish_child($pid);
-# If the child process terminates before our copy_stdin() process is able to
-# write all of its data to $parent_in, the copy_stdin() process could stall.
-# Send SIGTERM to it to ensure it terminates.
-kill 'TERM', $in_pid;
-finish_child($in_pid);
+if ($in_pid) {
+	# If the child process terminates before our copy_stdin() process is able to
+	# write all of its data to $parent_in, the copy_stdin() process could stall.
+	# Send SIGTERM to it to ensure it terminates.
+	kill 'TERM', $in_pid;
+	finish_child($in_pid);
+}
 exit($ret);
-- 
2.45.1.221.gd3c11dbb1d

[PATCH v2 5/5] 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                | 14 +++++++++++---
 t/t3701-add-interactive.sh | 21 +++++++++++++++++++++
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 2252895c28..d614536cb2 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -5,6 +5,7 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "pager.h"
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "strbuf.h"
@@ -1387,7 +1388,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,
@@ -1398,7 +1399,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,
@@ -1448,9 +1449,15 @@ 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();
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
+				if (use_pager) {
+					wait_for_pager();
+					use_pager = 0;
+				}
 			}
 
 			strbuf_reset(&s->buf);
@@ -1665,8 +1672,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 52d7830de2..4be7a14419 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -558,6 +558,27 @@ 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 --no-stdin-pty git add -p >actual
+	) &&
+	tail -n 7 <actual | test_decode_color >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
 test_expect_success 'navigate to hunk via regex' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-- 
2.45.1.221.gd3c11dbb1d

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

[Dragan Simic]

Hello Ruben,

On 2024-05-21 22:52, Rubén Justo wrote:
> 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.
> 
> ...
> 
> @@ -1387,7 +1388,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"

I think it would be better to move the description of "P" into
a separate line after the "p" line, perhaps like this:

   "P - use the pager to print the current hunk\n"

I know, we'd sacrifice one line of the valuable vertical space
this way, but I find it more consistent and much harder to miss
the new "P" option.

Overall, I find the introduction of "P" as the new single-character
menu option fine.  Maybe we can later add "-P" as the new command-
line option, if there will be some demand to do that.

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>> @@ -1387,7 +1388,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"
>
> I think it would be better to move the description of "P" into
> a separate line after the "p" line, perhaps like this:
>
>   "P - use the pager to print the current hunk\n"

Sounds good to me, too.

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

[Rubén Justo]

On Wed, May 22, 2024 at 10:09:25AM +0200, Dragan Simic wrote:

> > @@ -1387,7 +1388,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"
> 
> I think it would be better to move the description of "P" into
> a separate line after the "p" line, perhaps like this:
> 
>   "P - use the pager to print the current hunk\n"
> 
> I know, we'd sacrifice one line of the valuable vertical space
> this way, but I find it more consistent and much harder to miss
> the new "P" option.

Making 'P' a /version/ of 'p' allows us to skip adding 'P' to the list
of available options:

    (1/1) Stage this hunk [y,n,q,a,d,j,J,k,K,g,/,s,e,p,P,?]

This is what I though and this long list is what worries me.

But I see your point.  I am not opposed to adding a new line.

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

[Dragan Simic]

On 2024-05-22 23:23, Rubén Justo wrote:
> On Wed, May 22, 2024 at 10:09:25AM +0200, Dragan Simic wrote:
> 
>> > @@ -1387,7 +1388,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"
>> 
>> I think it would be better to move the description of "P" into
>> a separate line after the "p" line, perhaps like this:
>> 
>>   "P - use the pager to print the current hunk\n"
>> 
>> I know, we'd sacrifice one line of the valuable vertical space
>> this way, but I find it more consistent and much harder to miss
>> the new "P" option.
> 
> Making 'P' a /version/ of 'p' allows us to skip adding 'P' to the list
> of available options:
> 
>     (1/1) Stage this hunk [y,n,q,a,d,j,J,k,K,g,/,s,e,p,P,?]
> 
> This is what I though and this long list is what worries me.

Oh, I wouldn't be worried too much about the length of the list of
the options.  It's feature-rich, so the list has to be a bit long. :)

> But I see your point.  I am not opposed to adding a new line.

Thanks.

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

[Rubén Justo]

The main goal in this series is to allow using the pager when displaying
hunks during "add -p" sessions, to make easier for users to review hunks
longer than one screen height.

This iteration, v3, introduces a new command: '|', suggested by Junio,
instead of the 'P' command proposed in the previous iteration.

This allows us to use the pager:

  (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? |

But also to use other programs, like:

  (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | head

Or:

  (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | grep term

Hopefully, we'll find a way to avoid sending ANSI codes, on demand,
without disabling it entirely with color.ui=never or any other global
option.  To make this usable:

  (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | vim -

However, the current functionality meets my current needs, so I'm happy
with it.

This, a new 'interactive.pipeCommand' setting, or a new switch: 'add -P',
are left for discussing in, hopefully, a future series.

One final note;  I preferred to model the help text this way:

    y - stage this hunk
    n - do not stage this hunk
    q - quit; do not stage this hunk or any of the remaining ones
    a - stage this hunk and all later hunks in the file
    d - do not stage this hunk or any of the later hunks in the file
    j - leave this hunk undecided, see next undecided hunk
    J - leave this hunk undecided, see next hunk
    g - select a hunk to go to 
    / - search for a hunk matching the given regex
    s - split the current hunk into smaller hunks
    e - manually edit the current hunk
    p - print the current hunk
    | - pipe the current hunk to the pager, or |<program> to use a program'
    ? - print help

Instead of:

    y - stage this hunk
    n - do not stage this hunk
    q - quit; do not stage this hunk or any of the remaining ones
    a - stage this hunk and all later hunks in the file
    d - do not stage this hunk or any of the later hunks in the file
    j - leave this hunk undecided, see next undecided hunk
    J - leave this hunk undecided, see next hunk
    g - select a hunk to go to
    / - search for a hunk matching the given regex
    s - split the current hunk into smaller hunks
    e - manually edit the current hunk
    p - print the current hunk
    |[program] - pipe the current hunk to a program, the pager if none...
    ? - print help

Because I believe it reads better by maintaining a single character
before the dash.  But I am not opposed to the latter.

The series is now based on jc/add-patch-enforce-single-letter-input,
which has been recently merged to master.

Thanks.

Rubén Justo (6):
  add-patch: test for 'p' command
  pager: do not close fd 2 unnecessarily
  pager: introduce wait_for_pager
  pager: introduce setup_custom_pager
  test-terminal: introduce --no-stdin-pty
  add-patch: introduce the command '|'

 add-patch.c                | 17 ++++++++--
 pager.c                    | 55 ++++++++++++++++++++++++-------
 pager.h                    |  7 +++-
 t/t3701-add-interactive.sh | 67 +++++++++++++++++++++++++++++---------
 t/test-terminal.perl       | 32 ++++++++++--------
 5 files changed, 135 insertions(+), 43 deletions(-)

-- 
2.45.0.97.g9fa538478d

[PATCH v3 1/6] 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 6624a4f7c0..6f6d174687 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -590,6 +590,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.0.97.g9fa538478d

[PATCH v3 2/6] 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 b8822a9381..b786601074 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static const 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.0.97.g9fa538478d

[PATCH v3 3/6] 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 | 36 ++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index b786601074..925f860335 100644
--- a/pager.c
+++ b/pager.c
@@ -14,7 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static const 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,20 +24,41 @@ 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)
 {
+	if (old_fd1 == -1)
+		return;
+
 	fflush(stdout);
 	fflush(stderr);
 	close_pager_fds();
 	finish_command(&pager_process);
 }
 
+void wait_for_pager(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	wait_for_pager_atexit();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		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 +134,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 +164,18 @@ void setup_pager(void)
 		return;
 
 	/* 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++) {
+		sigchain_push_common(wait_for_pager_signal);
+		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.0.97.g9fa538478d

[PATCH v3 4/6] pager: introduce setup_custom_pager

[Rubén Justo]

Introduce a new function setup_custom_pager() to allow setting up our
pager mechanism using a custom pager.  If the custom pager specified is
NULL or an empty string, use the normal pager as setup_pager() currently
does.

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

diff --git a/pager.c b/pager.c
index 925f860335..21a7d9cd60 100644
--- a/pager.c
+++ b/pager.c
@@ -74,14 +74,13 @@ static int core_pager_config(const char *var, const char *value,
 	return 0;
 }
 
-const char *git_pager(int stdout_is_tty)
+static const char *git_pager_custom(int stdout_is_tty, const char* pager)
 {
-	const char *pager;
-
 	if (!stdout_is_tty)
 		return NULL;
 
-	pager = getenv("GIT_PAGER");
+	if (!pager || !*pager)
+		pager = getenv("GIT_PAGER");
 	if (!pager) {
 		if (!pager_program)
 			read_early_config(core_pager_config, NULL);
@@ -97,6 +96,11 @@ const char *git_pager(int stdout_is_tty)
 	return pager;
 }
 
+const char *git_pager(int stdout_is_tty)
+{
+	return git_pager_custom(stdout_is_tty, NULL);
+}
+
 static void setup_pager_env(struct strvec *env)
 {
 	const char **argv;
@@ -132,10 +136,11 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
 	pager_process->trace2_child_class = "pager";
 }
 
-void setup_pager(void)
+void setup_custom_pager(const char* pager)
 {
 	static int once = 0;
-	const char *pager = git_pager(isatty(1));
+
+	pager = git_pager_custom(isatty(1), pager);
 
 	if (!pager)
 		return;
diff --git a/pager.h b/pager.h
index 103ecac476..2166662361 100644
--- a/pager.h
+++ b/pager.h
@@ -4,7 +4,11 @@
 struct child_process;
 
 const char *git_pager(int stdout_is_tty);
-void setup_pager(void);
+void setup_custom_pager(const char*);
+static inline void setup_pager(void)
+{
+	setup_custom_pager(NULL);
+}
 void wait_for_pager(void);
 int pager_in_use(void);
 int term_columns(void);
-- 
2.45.0.97.g9fa538478d

[PATCH v3 5/6] test-terminal: introduce --no-stdin-pty

[Rubén Justo]

In 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
2015-08-04), t/test-terminal.perl learned to connect the child process'
stdin to a pty.  It works well for what was intended: satisfying an
`isatty(STDIN_FILENO)` check.

However, the fork introduced in 18d8c26930, that copies the stdin to the
child process, does not always manage to transmit all the input data.

To illustrate this behavior, we can use a function like this:

    send_data ()
    {
    	dd if=/dev/zero bs=1 count=10000 status=none |
    	t/test-terminal.perl cat - 2>/dev/null |
    	wc -c;
    }

We do not obtain the expected results when executing this function
100 times:

    $ for i in $(seq 100); do send_data; done | sort | uniq -c
         36 0
          4 1
         53 4095
          7 4159

None of the executions have successfully transmitted the full data.

If we do the same with a version of t/test-terminal.perl that does not
redirect stdin, a version prior to 18d8c26930, the expected result is
obtained:

    $ git checkout 18d8c26930~1
    $ for i in $(seq 100); do send_data; done | sort | uniq -c
        100 10000

In a subsequent commit, we'll introduce a new test that depends on
t/test-terminate.perl.  This test does not require stdin to be connected
to a terminal; however, all data piped into the process must be
successfully transmitted to the child process.

To make this possible, add a new parameter "--no-stdin-pty" to allow
disabling the stdin redirection though a pty.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/test-terminal.perl | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 3810e9bb43..85edc9e8b9 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -12,10 +12,10 @@ sub start_child {
 	if (not defined $pid) {
 		die "fork failed: $!"
 	} elsif ($pid == 0) {
-		open STDIN, "<&", $in;
+		open STDIN, "<&", $in if $in;
 		open STDOUT, ">&", $out;
 		open STDERR, ">&", $err;
-		close $in;
+		close $in if $in;
 		close $out;
 		exec(@$argv) or die "cannot exec '$argv->[0]': $!"
 	}
@@ -78,28 +78,32 @@ sub copy_stdio {
 }
 
 if ($#ARGV < 1) {
-	die "usage: test-terminal program args";
+	die "usage: test-terminal [--no-stdin-pty] program args";
 }
+my $no_stdin_pty = $ARGV[0] eq '--no-stdin-pty';
+shift @ARGV if $no_stdin_pty;
 $ENV{TERM} = 'vt100';
-my $parent_in = new IO::Pty;
+my $parent_in = $no_stdin_pty ? undef : IO::Pty->new;
 my $parent_out = new IO::Pty;
 my $parent_err = new IO::Pty;
-$parent_in->set_raw();
+$parent_in->set_raw() if $parent_in;
 $parent_out->set_raw();
 $parent_err->set_raw();
-$parent_in->slave->set_raw();
+$parent_in->slave->set_raw() if $parent_in;
 $parent_out->slave->set_raw();
 $parent_err->slave->set_raw();
-my $pid = start_child(\@ARGV, $parent_in->slave, $parent_out->slave, $parent_err->slave);
-close $parent_in->slave;
+my $pid = start_child(\@ARGV,$parent_in ? $parent_in->slave : undef, $parent_out->slave, $parent_err->slave);
+close $parent_in->slave if $parent_in;
 close $parent_out->slave;
 close $parent_err->slave;
-my $in_pid = copy_stdin($parent_in);
+my $in_pid = $no_stdin_pty ? 0 : copy_stdin($parent_in);
 copy_stdio($parent_out, $parent_err);
 my $ret = finish_child($pid);
-# If the child process terminates before our copy_stdin() process is able to
-# write all of its data to $parent_in, the copy_stdin() process could stall.
-# Send SIGTERM to it to ensure it terminates.
-kill 'TERM', $in_pid;
-finish_child($in_pid);
+if ($in_pid) {
+	# If the child process terminates before our copy_stdin() process is able to
+	# write all of its data to $parent_in, the copy_stdin() process could stall.
+	# Send SIGTERM to it to ensure it terminates.
+	kill 'TERM', $in_pid;
+	finish_child($in_pid);
+}
 exit($ret);
-- 
2.45.0.97.g9fa538478d

[PATCH v3 6/6] add-patch: introduce the command '|'

[Rubén Justo]

Introduce a new command: '|', to send the current hunk to a program.

If no program is specified, default to the pager.

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

diff --git a/add-patch.c b/add-patch.c
index 814de57c4a..5a586d1b9b 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -5,6 +5,7 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "pager.h"
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "strbuf.h"
@@ -1389,6 +1390,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
    "s - split the current hunk into smaller hunks\n"
    "e - manually edit the current hunk\n"
    "p - print the current hunk\n"
+   "| - pipe the current hunk to the pager, or |<program> to use a program'\n"
    "? - print help\n");
 
 static int patch_update_file(struct add_p_state *s,
@@ -1401,6 +1403,7 @@ static int patch_update_file(struct add_p_state *s,
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int colored = !!s->colored.len, quit = 0;
 	enum prompt_mode_type prompt_mode_type;
+	const char* pager = NULL;
 	enum {
 		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
 		ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
@@ -1449,9 +1452,15 @@ 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 (pager)
+					setup_custom_pager(pager);
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
+				if (pager) {
+					wait_for_pager();
+					pager = NULL;
+				}
 			}
 
 			strbuf_reset(&s->buf);
@@ -1485,6 +1494,7 @@ static int patch_update_file(struct add_p_state *s,
 				strbuf_addstr(&s->buf, ",e");
 			}
 			strbuf_addstr(&s->buf, ",p");
+			strbuf_addstr(&s->buf, ",|");
 		}
 		if (file_diff->deleted)
 			prompt_mode_type = PROMPT_DELETION;
@@ -1512,8 +1522,8 @@ static int patch_update_file(struct add_p_state *s,
 			continue;
 		ch = tolower(s->answer.buf[0]);
 
-		/* 'g' takes a hunk number and '/' takes a regexp */
-		if (s->answer.len != 1 && (ch != 'g' && ch != '/')) {
+		/* 'g' takes a hunk number, '/' takes a regexp and '|' takes a program */
+		if (s->answer.len != 1 && (ch != 'g' && ch != '/' && ch != '|')) {
 			err(s, _("Only one letter is expected, got '%s'"), s->answer.buf);
 			continue;
 		}
@@ -1674,6 +1684,9 @@ static int patch_update_file(struct add_p_state *s,
 			}
 		} else if (s->answer.buf[0] == 'p') {
 			rendered_hunk_index = -1;
+		} else if (ch == '|') {
+			rendered_hunk_index = -1;
+			pager = s->answer.buf + 1;
 		} 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 6f6d174687..7b3ebb671d 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -64,8 +64,8 @@ test_expect_success 'unknown command' '
 	git add -N command &&
 	git diff command >expect &&
 	cat >>expect <<-EOF &&
-	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
-	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
+	(1/1) Stage addition [y,n,q,a,d,e,p,|,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
+	(1/1) Stage addition [y,n,q,a,d,e,p,|,?]?$SP
 	EOF
 	git add -p -- command <command >actual 2>&1 &&
 	test_cmp expect actual
@@ -348,9 +348,9 @@ test_expect_success 'different prompts for mode change/deleted' '
 	git -c core.filemode=true add -p >actual &&
 	sed -n "s/^\(([0-9/]*) Stage .*?\).*/\1/p" actual >actual.filtered &&
 	cat >expect <<-\EOF &&
-	(1/1) Stage deletion [y,n,q,a,d,p,?]?
-	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]?
+	(1/1) Stage deletion [y,n,q,a,d,p,|,?]?
+	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,|,?]?
+	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,|,?]?
 	EOF
 	test_cmp expect actual.filtered
 '
@@ -537,13 +537,13 @@ test_expect_success 'split hunk setup' '
 test_expect_success 'goto hunk 1 with "g 1"' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
+	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,|,?]? + 1:  -1,2 +1,3          +15
 	_ 2:  -2,4 +3,8          +21
 	go to which hunk? @@ -1,2 +1,3 @@
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
 	EOF
 	test_write_lines s y g 1 | git add -p >actual &&
 	tail -n 7 <actual >actual.trimmed &&
@@ -556,7 +556,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
 	EOF
 	test_write_lines s y g1 | git add -p >actual &&
 	tail -n 4 <actual >actual.trimmed &&
@@ -566,11 +566,11 @@ test_expect_success 'goto hunk 1 with "g1"' '
 test_expect_success 'navigate to hunk via regex /pattern' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	(2/2) Stage this hunk [y,n,q,a,d,K,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,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
 	EOF
 	test_write_lines s y /1,2 | git add -p >actual &&
 	tail -n 5 <actual >actual.trimmed &&
@@ -583,7 +583,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
 	EOF
 	test_write_lines s y / 1,2 | git add -p >actual &&
 	tail -n 4 <actual >actual.trimmed &&
@@ -595,17 +595,38 @@ test_expect_success 'print again the hunk' '
 	tr _ " " >expect <<-EOF &&
 	+15
 	 20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	(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,?]?_
+	(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 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 \| |
+	(
+		GIT_PAGER="sed s/^/PAGER\ /" &&
+		export GIT_PAGER &&
+		test_terminal --no-stdin-pty 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:
 	#
@@ -780,21 +801,21 @@ test_expect_success 'colors can be overridden' '
 	<BLUE>+<RESET><BLUE>new<RESET>
 	<CYAN> more-context<RESET>
 	<BLUE>+<RESET><BLUE>another-one<RESET>
-	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,|,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
 	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
 	<CYAN> context<RESET>
 	<BOLD>-old<RESET>
 	<BLUE>+<RESET><BLUE>new<RESET>
 	<CYAN> more-context<RESET>
-	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
 	<CYAN> more-context<RESET>
 	<BLUE>+<RESET><BLUE>another-one<RESET>
-	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,|,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
 	<CYAN> context<RESET>
 	<BOLD>-old<RESET>
 	<BLUE>+new<RESET>
 	<CYAN> more-context<RESET>
-	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]? <RESET>
 	EOF
 	test_cmp expect actual
 '
-- 
2.45.0.97.g9fa538478d

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

[Junio C Hamano]

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

> Hopefully, we'll find a way to avoid sending ANSI codes, on demand,
> without disabling it entirely with color.ui=never or any other global
> option.  To make this usable:
>
>   (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | vim -
>
> However, the current functionality meets my current needs, so I'm happy
> with it.

Yup, if it really is needed we could do || or anything "unusual" to
signal the unusual nature of the command.

Or ">" command can send the output to specified file without
coloring, and the user can do whatever they want to it.  

In any case, unlike "Let's not just do pager, but have a facility to
pipe to anything and make the pager a default destination" that was
a natural match to the originally proposed behaviour, these two are
quite different and can be left totally outside the scope of the
topic.

> One final note;  I preferred to model the help text this way:
>
>     y - stage this hunk
>     n - do not stage this hunk
> ...
>     g - select a hunk to go to 
>     / - search for a hunk matching the given regex
> ...
>     | - pipe the current hunk to the pager, or |<program> to use a program'
>     ? - print help

That's fine.

The 'g' and '/' commands take _mandatory_ arguments, but we do not
even mention it in the help text.  But we need to say something for
this new thing, because it is _optional_ and if you do not give a
program, it does not ask.

A possibility is to phrase it like so:

    | - pipe the current hunk to the program (or "%s" by default)

and fill %s with the program you'd use if not given, i.e. initially
the value of the GIT_PAGER but updated to the last used program
after the user uses "|<program>" form to specify one.

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

[Junio C Hamano]

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

> The 'g' and '/' commands take _mandatory_ arguments, but we do not
> even mention it in the help text.  But we need to say something for
> this new thing, because it is _optional_ and if you do not give a
> program, it does not ask.

By the way, although I personally do not have much sympathy to those
who set it, in the presence of interactive.singleKey configuration
variable, a command that takes optional argument may turn out to be
a mistake, as the user cannot give the argument even if they wanted
to, when the configuration variable is set to true.  To help them,
we'd probably need something like the following to allow them to
optionally set their own program, like the following:

 1. read the command, notice that it begins with '|'.

 2. if it has <program> after it, call it <program> and jump to 5.

 3. if it does not have <program> after it, but if single key
    operation is in effect, give the user a chance to give <program>
    by prompting.  Call the answer to the prompt <program>.  If it
    is not an empty string, jump to 5.

 4. at this point, we have <program> that is empty as given by the
    user.  Replace the <program> with the value we remembered from
    step 6. during the last use of the '|' command.

 5. if <program> is all whitespace, replace it with an empty string.

 6. remember the value of <program> (so that you can reuse it next
    time the user says '|' and without <program>), and call the
    "set-custom-pager" thing with <program> (this design assumes
    that "set-custom-pager" thing uses the GIT_PAGER when fed an
    empty string).

 7. spawn the program set by "set-custom-pager", and feed our output
    to it.

So the end-user observable behaviour would become

 * There is the "default" program, initially their pager, but after
   they use the '|' command, we remember the last program they used
   during the session and reuse it when they tell us to do so.

 * For singlekey folks, typing '|' will give them a prompt.  They
   can give an empty string and spawn the "default" thing.  They can
   give " " plus <RET> to reset the "default" to GIT_PAGER and use
   it.  Or they can give <program> plus <RET> to use it and update
   the "default".

 * For the rest of us, typing "|" plus <RET> will spawn the
   "default" thing.  Typing "|<program>" plus <RET> will use the
   <program> and update the "default".  Typing "| " plus <RET> will
   reset the "default" to GIT_PAGER and use it.

which is quite straight-forward and consistent between the two
camps.

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

[Rubén Justo]

On Sun, Jun 02, 2024 at 10:11:49AM -0700, Junio C Hamano wrote:

> By the way, although I personally do not have much sympathy to those
> who set it, in the presence of interactive.singleKey configuration
> variable, a command that takes optional argument may turn out to be
> a mistake, as the user cannot give the argument even if they wanted
> to, when the configuration variable is set to true.

Well spotted.

Perhaps we can do something like:

diff --git a/add-patch.c b/add-patch.c
index 5a586d1b9b..01525214f9 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1685,8 +1685,17 @@ static int patch_update_file(struct add_p_state *s,
 		} else if (s->answer.buf[0] == 'p') {
 			rendered_hunk_index = -1;
 		} else if (ch == '|') {
+			strbuf_remove(&s->answer, 0, 1);
+			if (s->s.use_single_key && !s->answer.len) {
+				printf("%s", _("program? "));
+				fflush(stdout);
+				strbuf_getline(&s->answer, stdin);
+				strbuf_trim_trailing_newline(&s->answer);
+			}
 			rendered_hunk_index = -1;
-			pager = s->answer.buf + 1;
+			pager = s->answer.buf;
 		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
I agree this needs to be addressed in this series.

I'd prefer to leave the feature of reusing the command, to another
series.

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

[Rubén Justo]

On Sun, Jun 02, 2024 at 09:36:35AM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > Hopefully, we'll find a way to avoid sending ANSI codes, on demand,
> > without disabling it entirely with color.ui=never or any other global
> > option.  To make this usable:
> >
> >   (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | vim -
> >
> > However, the current functionality meets my current needs, so I'm happy
> > with it.
> 
> Yup, if it really is needed we could do || or anything "unusual" to
> signal the unusual nature of the command.
> 
> Or ">" command can send the output to specified file without
> coloring, and the user can do whatever they want to it.  

Interesting;  perhaps intuitive and in line with other 'isatty()'
conditions we have.

> 
> In any case, unlike "Let's not just do pager, but have a facility to
> pipe to anything and make the pager a default destination" that was
> a natural match to the originally proposed behaviour, these two are
> quite different and can be left totally outside the scope of the
> topic.
> 
> > One final note;  I preferred to model the help text this way:
> >
> >     y - stage this hunk
> >     n - do not stage this hunk
> > ...
> >     g - select a hunk to go to 
> >     / - search for a hunk matching the given regex
> > ...
> >     | - pipe the current hunk to the pager, or |<program> to use a program'
> >     ? - print help
> 
> That's fine.
> 
> The 'g' and '/' commands take _mandatory_ arguments, but we do not
> even mention it in the help text.  But we need to say something for
> this new thing, because it is _optional_ and if you do not give a
> program, it does not ask.
> 
> A possibility is to phrase it like so:
> 
>     | - pipe the current hunk to the program (or "%s" by default)
> 
> and fill %s with the program you'd use if not given, i.e. initially
> the value of the GIT_PAGER but updated to the last used program
> after the user uses "|<program>" form to specify one.

And this one too;  a nice way to allow reusing the previous value.
Perhaps we'd better find a way to introduce some form of CTRL+P, or
arrow-up...  I dunno.

Interesting ideas.  But my preference is to queue this series as it is,
if no major issue is pointed out.  And leave for future series this
topics or the others mentioned in the thread: the 'interactive.'
setting suggested by Peff, or the '-P' switch, by Dragan.

Let's see what others say.

Thanks.

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

[Dragan Simic]

Hello Junio,

On 2024-06-02 18:36, Junio C Hamano wrote:
> A possibility is to phrase it like so:
> 
>     | - pipe the current hunk to the program (or "%s" by default)
> 
> and fill %s with the program you'd use if not given, i.e. initially
> the value of the GIT_PAGER but updated to the last used program
> after the user uses "|<program>" form to specify one.

The value of GIT_PAGER (or the <program>) can be a rather long string,
so it would have to be stripped down to the base command, but it would
be rather error-prone and the printed information would become much less
informative that way.

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> Hello Junio,
>
> On 2024-06-02 18:36, Junio C Hamano wrote:
>> A possibility is to phrase it like so:
>>     | - pipe the current hunk to the program (or "%s" by default)
>> and fill %s with the program you'd use if not given, i.e. initially
>> the value of the GIT_PAGER but updated to the last used program
>> after the user uses "|<program>" form to specify one.
>
> The value of GIT_PAGER (or the <program>) can be a rather long string,
> so it would have to be stripped down to the base command, but it would
> be rather error-prone and the printed information would become much less
> informative that way.

I'd probably just say $GIT_PAGER instead of its expansion if we were
go that route.

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

[Dragan Simic]

On 2024-06-03 11:03, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>> On 2024-06-02 18:36, Junio C Hamano wrote:
>>> A possibility is to phrase it like so:
>>>     | - pipe the current hunk to the program (or "%s" by default)
>>> and fill %s with the program you'd use if not given, i.e. initially
>>> the value of the GIT_PAGER but updated to the last used program
>>> after the user uses "|<program>" form to specify one.
>> 
>> The value of GIT_PAGER (or the <program>) can be a rather long string,
>> so it would have to be stripped down to the base command, but it would
>> be rather error-prone and the printed information would become much 
>> less
>> informative that way.
> 
> I'd probably just say $GIT_PAGER instead of its expansion if we were
> go that route.

Makes sense to me.  More precisely, the environment should be checked
to see is it "$GIT_PAGER" or "$PAGER" that needs to be printed literally
as part of the help message.

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>> I'd probably just say $GIT_PAGER instead of its expansion if we were
>> go that route.
>
> Makes sense to me.  More precisely, the environment should be checked
> to see is it "$GIT_PAGER" or "$PAGER" that needs to be printed literally
> as part of the help message.

I was sure somebody will split a hair like that.  At that point we
are better off mentioning 'git var' X-<.  Or just 'Your Pager'.

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

[Dragan Simic]

On 2024-06-03 17:28, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>>> I'd probably just say $GIT_PAGER instead of its expansion if we were
>>> go that route.
>> 
>> Makes sense to me.  More precisely, the environment should be checked
>> to see is it "$GIT_PAGER" or "$PAGER" that needs to be printed 
>> literally
>> as part of the help message.
> 
> I was sure somebody will split a hair like that.  At that point we
> are better off mentioning 'git var' X-<.  Or just 'Your Pager'.

I agree that it can be seen as splitting hairs, but the general approach
of Git is accuracy, so using "$GIT_PAGER" regardless of it being used or
not would be simply against the general approach.

Though, I agree that using "pager" or maybe even "your pager" would be
simpler, yet a very good option.

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

[Dragan Simic]

Hello Ruben,

On 2024-06-02 17:38, Rubén Justo wrote:
> This iteration, v3, introduces a new command: '|', suggested by Junio,
> instead of the 'P' command proposed in the previous iteration.
> 
> This allows us to use the pager:
> 
>   (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? |
> 
> But also to use other programs, like:
> 
>   (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | head
> 
> Or:
> 
>   (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | grep term
> 
> Hopefully, we'll find a way to avoid sending ANSI codes, on demand,
> without disabling it entirely with color.ui=never or any other global
> option.  To make this usable:
> 
>   (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,s,e,p,|,?]? | vim -
> 
> However, the current functionality meets my current needs, so I'm happy
> with it.

The way I see it, using "| <program>" should follow the de facto rules
already established by the "--color=auto" command-line option in 
multiple
utilities.  Thus, when piping to a custom program, the escape codes that
perform the coloring should be stripped.

> This, a new 'interactive.pipeCommand' setting, or a new switch: 'add 
> -P',
> are left for discussing in, hopefully, a future series.
> 
> One final note;  I preferred to model the help text this way:
> 
>     y - stage this hunk
>     n - do not stage this hunk
>     q - quit; do not stage this hunk or any of the remaining ones
>     a - stage this hunk and all later hunks in the file
>     d - do not stage this hunk or any of the later hunks in the file
>     j - leave this hunk undecided, see next undecided hunk
>     J - leave this hunk undecided, see next hunk
>     g - select a hunk to go to
>     / - search for a hunk matching the given regex
>     s - split the current hunk into smaller hunks
>     e - manually edit the current hunk
>     p - print the current hunk
>     | - pipe the current hunk to the pager, or |<program> to use a 
> program'
>     ? - print help

I also like this form better, but I think wording could be improved.
I'll think a bit more about it, maybe something like this:

       | - use pager to show the current hunk, or use |<program> to 
customize

Also, what's the single quote doing after "use a program"?

> Instead of:
> 
>     y - stage this hunk
>     n - do not stage this hunk
>     q - quit; do not stage this hunk or any of the remaining ones
>     a - stage this hunk and all later hunks in the file
>     d - do not stage this hunk or any of the later hunks in the file
>     j - leave this hunk undecided, see next undecided hunk
>     J - leave this hunk undecided, see next hunk
>     g - select a hunk to go to
>     / - search for a hunk matching the given regex
>     s - split the current hunk into smaller hunks
>     e - manually edit the current hunk
>     p - print the current hunk
>     |[program] - pipe the current hunk to a program, the pager if 
> none...
>     ? - print help
> 
> Because I believe it reads better by maintaining a single character
> before the dash.  But I am not opposed to the latter.

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> Thus, when piping to a custom program, the escape codes that
> perform the coloring should be stripped.

I tend to agree that if we do not give a way to toggle between
"with" and "without" color when piping to a program, it is safer to
make the default uncolored.

The user's configured pager is expected to deal with colors just
fine (or the user has globally configured colors to be off).  As we
are capable of telling if the user is asking to spawn the default
pager (by not giving a custom command or by clearing the previous
custom command given in the same session) or a custom one, it should
be easily doable to give colored version to the configured pager and
uncolored version to a custom/one-shot command.  Unlike the existing
support for (e)dit command, we do not read back from what the
command does using the hunk and present it again to the user, it
should be a relatively easy and safe thing to do.

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

[Dragan Simic]

On 2024-06-03 18:01, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> Thus, when piping to a custom program, the escape codes that
>> perform the coloring should be stripped.
> 
> I tend to agree that if we do not give a way to toggle between
> "with" and "without" color when piping to a program, it is safer to
> make the default uncolored.

Good point.  The default behavior or "|xyz" should be to strip
the coloring escape sequences, because we don't know is "xyz"
capable of handling those escape sequences properly, and to keep
the coloring with "||xyz" or whatever we come up with for our
equivalent "--color=always", so to speak.

> The user's configured pager is expected to deal with colors just
> fine (or the user has globally configured colors to be off).  As we
> are capable of telling if the user is asking to spawn the default
> pager (by not giving a custom command or by clearing the previous
> custom command given in the same session) or a custom one, it should
> be easily doable to give colored version to the configured pager and
> uncolored version to a custom/one-shot command.  Unlike the existing
> support for (e)dit command, we do not read back from what the
> command does using the hunk and present it again to the user, it
> should be a relatively easy and safe thing to do.

Exactly.

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

[Dragan Simic]

On 2024-06-04 19:41, Dragan Simic wrote:
> On 2024-06-03 18:01, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> 
>>> Thus, when piping to a custom program, the escape codes that
>>> perform the coloring should be stripped.
>> 
>> I tend to agree that if we do not give a way to toggle between
>> "with" and "without" color when piping to a program, it is safer to
>> make the default uncolored.
> 
> Good point.  The default behavior or "|xyz" should be to strip
> the coloring escape sequences, because we don't know is "xyz"
> capable of handling those escape sequences properly, and to keep
> the coloring with "||xyz" or whatever we come up with for our
> equivalent "--color=always", so to speak.

Oops, sorry...  s/equivalent/equivalent of/

>> The user's configured pager is expected to deal with colors just
>> fine (or the user has globally configured colors to be off).  As we
>> are capable of telling if the user is asking to spawn the default
>> pager (by not giving a custom command or by clearing the previous
>> custom command given in the same session) or a custom one, it should
>> be easily doable to give colored version to the configured pager and
>> uncolored version to a custom/one-shot command.  Unlike the existing
>> support for (e)dit command, we do not read back from what the
>> command does using the hunk and present it again to the user, it
>> should be a relatively easy and safe thing to do.
> 
> Exactly.

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

[Rubén Justo]

On Sun, Jun 02, 2024 at 07:36:37PM +0200, Dragan Simic wrote:

> The way I see it, using "| <program>" should follow the de facto rules
> already established by the "--color=auto" command-line option in multiple
> utilities.  Thus, when piping to a custom program, the escape codes that
> perform the coloring should be stripped.

Interesting.  However, I'd like to find a way to keep the escape codes
when using programs like: '|head';  perhaps with the '>' command,
suggested by Junio.

At any rate, I feel we can leave that, perhaps corner-case scenario, for
a future series.  As this series is mainly about the 'pager' machinery.

> 
> > This, a new 'interactive.pipeCommand' setting, or a new switch: 'add
> > -P',
> > are left for discussing in, hopefully, a future series.
> > 
> > One final note;  I preferred to model the help text this way:
> > 
> >     y - stage this hunk
> >     n - do not stage this hunk
> >     q - quit; do not stage this hunk or any of the remaining ones
> >     a - stage this hunk and all later hunks in the file
> >     d - do not stage this hunk or any of the later hunks in the file
> >     j - leave this hunk undecided, see next undecided hunk
> >     J - leave this hunk undecided, see next hunk
> >     g - select a hunk to go to
> >     / - search for a hunk matching the given regex
> >     s - split the current hunk into smaller hunks
> >     e - manually edit the current hunk
> >     p - print the current hunk
> >     | - pipe the current hunk to the pager, or |<program> to use a
> > program'
> >     ? - print help
> 
> I also like this form better, but I think wording could be improved.
> I'll think a bit more about it, maybe something like this:
> 
>       | - use pager to show the current hunk, or use |<program> to customize

Certainly!  It is indeed a sensible idea to improve the wording, avoiding
the word "pipe" :-).  Thank you.

> 
> Also, what's the single quote doing after "use a program"?

Just a typo.  Sorry.

> 
> > Instead of:
> > 
> >     y - stage this hunk
> >     n - do not stage this hunk
> >     q - quit; do not stage this hunk or any of the remaining ones
> >     a - stage this hunk and all later hunks in the file
> >     d - do not stage this hunk or any of the later hunks in the file
> >     j - leave this hunk undecided, see next undecided hunk
> >     J - leave this hunk undecided, see next hunk
> >     g - select a hunk to go to
> >     / - search for a hunk matching the given regex
> >     s - split the current hunk into smaller hunks
> >     e - manually edit the current hunk
> >     p - print the current hunk
> >     |[program] - pipe the current hunk to a program, the pager if
> > none...
> >     ? - print help
> > 
> > Because I believe it reads better by maintaining a single character
> > before the dash.  But I am not opposed to the latter.

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

[Dragan Simic]

On 2024-06-03 22:19, Rubén Justo wrote:
> On Sun, Jun 02, 2024 at 07:36:37PM +0200, Dragan Simic wrote:
> 
>> The way I see it, using "| <program>" should follow the de facto rules
>> already established by the "--color=auto" command-line option in 
>> multiple
>> utilities.  Thus, when piping to a custom program, the escape codes 
>> that
>> perform the coloring should be stripped.
> 
> Interesting.  However, I'd like to find a way to keep the escape codes
> when using programs like: '|head';  perhaps with the '>' command,
> suggested by Junio.
> 
> At any rate, I feel we can leave that, perhaps corner-case scenario, 
> for
> a future series.  As this series is mainly about the 'pager' machinery.

I'd suggest that "| <program>" is made to work as "--color=auto" in
the current series, i.e. with the coloring escape sequences stripped,
which is the safe approach because we don't know is "<program>" capable
of handling those escape sequences, while implementing support for
"--color=always" would be left for a follow-up series.  Using ">" for
that future command makes sense to me, because it suggests an output
redirection in raw form.

Of course, the plain "|" should still leave the coloring escape 
sequences
intact, because the configured pager (or the default pager, configured
by Git internally) is expected to handle them properly.

>> > This, a new 'interactive.pipeCommand' setting, or a new switch: 'add
>> > -P',
>> > are left for discussing in, hopefully, a future series.
>> >
>> > One final note;  I preferred to model the help text this way:
>> >
>> >     y - stage this hunk
>> >     n - do not stage this hunk
>> >     q - quit; do not stage this hunk or any of the remaining ones
>> >     a - stage this hunk and all later hunks in the file
>> >     d - do not stage this hunk or any of the later hunks in the file
>> >     j - leave this hunk undecided, see next undecided hunk
>> >     J - leave this hunk undecided, see next hunk
>> >     g - select a hunk to go to
>> >     / - search for a hunk matching the given regex
>> >     s - split the current hunk into smaller hunks
>> >     e - manually edit the current hunk
>> >     p - print the current hunk
>> >     | - pipe the current hunk to the pager, or |<program> to use a
>> > program'
>> >     ? - print help
>> 
>> I also like this form better, but I think wording could be improved.
>> I'll think a bit more about it, maybe something like this:
>> 
>>       | - use pager to show the current hunk, or use |<program> to 
>> customize
> 
> Certainly!  It is indeed a sensible idea to improve the wording, 
> avoiding
> the word "pipe" :-).  Thank you.

I'm glad that you like it. :)

>> Also, what's the single quote doing after "use a program"?
> 
> Just a typo.  Sorry.

Ah, I see.  It looked a bit strange. :)

>> > Instead of:
>> >
>> >     y - stage this hunk
>> >     n - do not stage this hunk
>> >     q - quit; do not stage this hunk or any of the remaining ones
>> >     a - stage this hunk and all later hunks in the file
>> >     d - do not stage this hunk or any of the later hunks in the file
>> >     j - leave this hunk undecided, see next undecided hunk
>> >     J - leave this hunk undecided, see next hunk
>> >     g - select a hunk to go to
>> >     / - search for a hunk matching the given regex
>> >     s - split the current hunk into smaller hunks
>> >     e - manually edit the current hunk
>> >     p - print the current hunk
>> >     |[program] - pipe the current hunk to a program, the pager if
>> > none...
>> >     ? - print help
>> >
>> > Because I believe it reads better by maintaining a single character
>> > before the dash.  But I am not opposed to the latter.

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

[Rubén Justo]

This iteration is a quick reroll to address the interactive.singleKey
option in the new '|' interactive command.

I've also used the improved wording for the help text, suggested by
Dragan.

The rest of the series remains the same.  Here is the range-diff from
v3:

Range-diff against v3:
-:  ---------- > 1:  49481da8a7 add-patch: test for 'p' command
-:  ---------- > 2:  865bb68508 pager: do not close fd 2 unnecessarily
-:  ---------- > 3:  9fcf244dac pager: introduce wait_for_pager
-:  ---------- > 4:  1e817a9ec0 pager: introduce setup_custom_pager
-:  ---------- > 5:  87dd368346 test-terminal: introduce --no-stdin-pty
1:  9fa538478d ! 6:  b691764a17 add-patch: introduce the command '|'
    @@ add-patch.c: N_("j - leave this hunk undecided, see next undecided hunk\n"
         "s - split the current hunk into smaller hunks\n"
         "e - manually edit the current hunk\n"
         "p - print the current hunk\n"
    -+   "| - pipe the current hunk to the pager, or |<program> to use a program'\n"
    ++   "| - use pager to show the current hunk, or use |<program> to customize\n"
         "? - print help\n");
      
      static int patch_update_file(struct add_p_state *s,
    @@ add-patch.c: static int patch_update_file(struct add_p_state *s,
      		} else if (s->answer.buf[0] == 'p') {
      			rendered_hunk_index = -1;
     +		} else if (ch == '|') {
    ++			strbuf_remove(&s->answer, 0, 1);
    ++			if (s->s.use_single_key && s->answer.len == 0) {
    ++				printf("%s", _("program? "));
    ++				fflush(stdout);
    ++				strbuf_getline(&s->answer, stdin);
    ++				strbuf_trim_trailing_newline(&s->answer);
    ++			}
    ++			strbuf_trim(&s->answer);
    ++			pager = s->answer.buf;
     +			rendered_hunk_index = -1;
    -+			pager = s->answer.buf + 1;
      		} else if (s->answer.buf[0] == '?') {
      			const char *p = _(help_patch_remainder), *eol = p;
      

base-commit: d3f616a4e56f359d84a9d439aa03dca1fe9ac280
-- 
2.45.0.97.gb691764a17

[PATCH v4 1/6] 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 6624a4f7c0..6f6d174687 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -590,6 +590,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.0.97.gb691764a17

[PATCH v4 2/6] 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 b8822a9381..b786601074 100644
--- a/pager.c
+++ b/pager.c
@@ -14,6 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static const 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.0.97.gb691764a17

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

[Junio C Hamano]

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

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

I was wondering how we can test this.

> diff --git a/pager.c b/pager.c
> index b8822a9381..b786601074 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -14,6 +14,7 @@ int pager_use_color = 1;
>  
>  static struct child_process pager_process;
>  static const 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);

At this step, we are assuming that we would start the pager only
once during the whole process, so relying on the 0-initialization of
close_fd2 in the BSS and setting it to 1 as needed when we dup2() is
sufficient, but presumably we would want to explicitly set close_fd2
to 0 when we are not calling dup2() here for completeness, with an
eye to the future where we run the pager multiple time.

Other than that, this looks reasonable to me.

Perhaps we should be checking the return value of our close() system
calls?  We would be getting scolded for closing an invalid file
descriptor, if we are closing something we shouldn't be closing,
right?

Thanks.

[PATCH v4 3/6] 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 | 36 ++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index b786601074..925f860335 100644
--- a/pager.c
+++ b/pager.c
@@ -14,7 +14,7 @@ int pager_use_color = 1;
 
 static struct child_process pager_process;
 static const 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,20 +24,41 @@ 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)
 {
+	if (old_fd1 == -1)
+		return;
+
 	fflush(stdout);
 	fflush(stderr);
 	close_pager_fds();
 	finish_command(&pager_process);
 }
 
+void wait_for_pager(void)
+{
+	if (old_fd1 == -1)
+		return;
+
+	wait_for_pager_atexit();
+	unsetenv("GIT_PAGER_IN_USE");
+	dup2(old_fd1, 1);
+	old_fd1 = -1;
+	if (old_fd2 != -1) {
+		dup2(old_fd2, 2);
+		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 +134,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 +164,18 @@ void setup_pager(void)
 		return;
 
 	/* 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++) {
+		sigchain_push_common(wait_for_pager_signal);
+		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.0.97.gb691764a17

Re: [PATCH v4 3/6] pager: introduce wait_for_pager

[Phillip Wood]

Hi Rubén

On 03/06/2024 21:38, 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 makes sense, I've left a few comments below

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

>   static void wait_for_pager_atexit(void)
>   {
> +	if (old_fd1 == -1)
> +		return;
> +

This is good - we'll return early if we've already cleaned up the pager.

>   	fflush(stdout);
>   	fflush(stderr);
>   	close_pager_fds();
>   	finish_command(&pager_process);
>   }
>   
> +void wait_for_pager(void)
> +{
> +	if (old_fd1 == -1)
> +		return;

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

> +	wait_for_pager_atexit();
> +	unsetenv("GIT_PAGER_IN_USE");
> +	dup2(old_fd1, 1);
> +	old_fd1 = -1;
> +	if (old_fd2 != -1) {
> +		dup2(old_fd2, 2);
> +		old_fd2 = -1;

We're leaking old_fd1 and old_fd2 here. wait_for_pager_atexit() flushes 
stdout and stderr so this switching of fds should play nicely with code 
that uses stdio.

> @@ -113,6 +134,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 +164,18 @@ void setup_pager(void)
>   		return;
>   
>   	/* 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++) {

We only need to increment "once" when we enter this block, not every 
time the code is run.

> +		sigchain_push_common(wait_for_pager_signal);

I think we should be calling this each time we setup the pager and pop 
it in wait_for_pager(). Imagine a caller sets up a signal handler before 
calling setup_pager() and wants to pop it after the pager has finished

	sigchain_push(...)
	setup_pager(...)
	do_something()
	wait_for_pager()
	sigchain_pop(...)

With the changes here it will pop the signal handler added by 
setup_pager() rather than the one it is expecting.

> +		atexit(wait_for_pager_atexit);

It is a bit of a shame we have to leave this function active when the 
pager has finished. We could add a wrapper around atexit() that allows 
us to pop functions we no-longer want to call but I don't think it is 
worth the effort here. wait_for_pager_atexit() is careful to return 
early if it is not needed.


Best Wishes

Phillip

Re: [PATCH v4 3/6] pager: introduce wait_for_pager

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>>   +void wait_for_pager(void)
>> +{
>> +	if (old_fd1 == -1)
>> +		return;
>
> Isn't it a bug to call this with old_fd1 == -1 or have I missed something?

Good point.

>> +	wait_for_pager_atexit();
>> +	unsetenv("GIT_PAGER_IN_USE");
>> +	dup2(old_fd1, 1);
>> +	old_fd1 = -1;
>> +	if (old_fd2 != -1) {
>> +		dup2(old_fd2, 2);
>> +		old_fd2 = -1;
>
> We're leaking old_fd1 and old_fd2 here. wait_for_pager_atexit()

Yeah, that needs fixing.

>> +	if (!once++) {
>
> We only need to increment "once" when we enter this block, not every
> time the code is run.

Running this 4 billion times and we'll be in a trouble ;-).

>> +		sigchain_push_common(wait_for_pager_signal);
>
> I think we should be calling this each time we setup the pager and pop
> it in wait_for_pager().

Good point.

Re: [PATCH v4 3/6] pager: introduce wait_for_pager

[Rubén Justo]

On Tue, Jun 04, 2024 at 11:00:37AM +0100, Phillip Wood wrote:
> Hi Rubén
> 
> On 03/06/2024 21:38, 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 makes sense, I've left a few comments below
> 
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> 
> >   static void wait_for_pager_atexit(void)
> >   {
> > +	if (old_fd1 == -1)
> > +		return;
> > +
> 
> This is good - we'll return early if we've already cleaned up the pager.
> 
> >   	fflush(stdout);
> >   	fflush(stderr);
> >   	close_pager_fds();
> >   	finish_command(&pager_process);
> >   }
> > +void wait_for_pager(void)
> > +{
> > +	if (old_fd1 == -1)
> > +		return;
> 
> Isn't it a bug to call this with old_fd1 == -1 or have I missed something?

It is.  I'll remove it to avoid confusion.

> 
> > +	wait_for_pager_atexit();
> > +	unsetenv("GIT_PAGER_IN_USE");
> > +	dup2(old_fd1, 1);
> > +	old_fd1 = -1;
> > +	if (old_fd2 != -1) {
> > +		dup2(old_fd2, 2);
> > +		old_fd2 = -1;
> 
> We're leaking old_fd1 and old_fd2 here.

Good eyes.  Will fix.  Thanks.

> wait_for_pager_atexit() flushes
> stdout and stderr so this switching of fds should play nicely with code that
> uses stdio.
> 
> > @@ -113,6 +134,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 +164,18 @@ void setup_pager(void)
> >   		return;
> >   	/* 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++) {
> 
> We only need to increment "once" when we enter this block, not every time
> the code is run.

OK. :-)

> 
> > +		sigchain_push_common(wait_for_pager_signal);
> 
> I think we should be calling this each time we setup the pager and pop it in
> wait_for_pager(). Imagine a caller sets up a signal handler before calling
> setup_pager() and wants to pop it after the pager has finished
> 
> 	sigchain_push(...)
> 	setup_pager(...)
> 	do_something()
> 	wait_for_pager()
> 	sigchain_pop(...)
> 
> With the changes here it will pop the signal handler added by setup_pager()
> rather than the one it is expecting.

I hadn't thought about this.  Thank you for pointing it out.

> 
> > +		atexit(wait_for_pager_atexit);
> 
> It is a bit of a shame we have to leave this function active when the pager
> has finished. We could add a wrapper around atexit() that allows us to pop
> functions we no-longer want to call but I don't think it is worth the effort
> here. wait_for_pager_atexit() is careful to return early if it is not
> needed.
> 
> 
> Best Wishes
> 
> Phillip
> 

Thanks!

Re: [PATCH v4 3/6] pager: introduce wait_for_pager

[Junio C Hamano]

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

>  static struct child_process pager_process;
>  static const char *pager_program;
> -static int close_fd2;
> +static int old_fd1 = -1, old_fd2 = -1;

;-)

Presumably when old_fd2 does not have a valid value (i.e. -1) it
means we did not do dup2() to save it away, and we refrain from
closing #2 in that case?

It is curious that #1 did not have similar problem 2/6 addressed,
as we never redirected it with dup() to save it away.  But now we
do for some reason that the proposed log message did not explain.
We should say something like

    We need to take back the standard output and the standard error
    stream after we are done with an invocation of the pager.  For
    that, save away the original file descriptors 1 and 2 when
    spawning the pager, and restore them once the pager exits.  The
    presence of saved fd#2 can be used to replace the "close_fd2"
    flag introduced in the previous patch.

perhaps.

>  /* Is the value coming back from term_columns() just a guess? */
>  static int term_columns_guessed;
> @@ -24,20 +24,41 @@ static void close_pager_fds(void)
>  {
>  	/* signal EOF to pager */
>  	close(1);
> -	if (close_fd2)
> +	if (old_fd2 != -1)
>  		close(2);
>  }

OK.

>  static void wait_for_pager_atexit(void)
>  {
> +	if (old_fd1 == -1)
> +		return;
> +
>  	fflush(stdout);
>  	fflush(stderr);
>  	close_pager_fds();
>  	finish_command(&pager_process);
>  }
>  
> +void wait_for_pager(void)
> +{
> +	if (old_fd1 == -1)
> +		return;
> +
> +	wait_for_pager_atexit();
> +	unsetenv("GIT_PAGER_IN_USE");
> +	dup2(old_fd1, 1);
> +	old_fd1 = -1;
> +	if (old_fd2 != -1) {
> +		dup2(old_fd2, 2);
> +		old_fd2 = -1;
> +	}
> +}

Presumably these use old_fd1's validity as a signal to see if have
pager running that need to be cleaned up?  It feels a bit unnatural
why we do not ask about such a process the structure that is set up
to control it, namely, the pager_process structure, but this is OK
for now.

It is just a naming issue, but it smells strange that the normal
code path (wait_for_pager()) calls a function that is an atexit
handler, which is more specific and only to be called atexit.

I would have made

	static void finish_pager(void)
	{
		fflush(stdout);
		fflush(stderr);
		close_pager_fds();
		finish_command(&pager_process);
	}

and then called it from the atexit handler and wait_for_pager().

> @@ -113,6 +134,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 +164,18 @@ void setup_pager(void)
>  		return;
>  
>  	/* 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);

Can dup(2) fail and return -1?

>  
> -	/* this makes sure that the parent terminates after the pager */
> -	sigchain_push_common(wait_for_pager_signal);
> -	atexit(wait_for_pager_atexit);
> +	if (!once++) {
> +		sigchain_push_common(wait_for_pager_signal);
> +		atexit(wait_for_pager_atexit);
> +	}
>  }

Can we give a name better than "once" to this thing?  

>  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);

Other than that, overall it looks good.

Thanks, will queue.

[PATCH v4 4/6] pager: introduce setup_custom_pager

[Rubén Justo]

Introduce a new function setup_custom_pager() to allow setting up our
pager mechanism using a custom pager.  If the custom pager specified is
NULL or an empty string, use the normal pager as setup_pager() currently
does.

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

diff --git a/pager.c b/pager.c
index 925f860335..21a7d9cd60 100644
--- a/pager.c
+++ b/pager.c
@@ -74,14 +74,13 @@ static int core_pager_config(const char *var, const char *value,
 	return 0;
 }
 
-const char *git_pager(int stdout_is_tty)
+static const char *git_pager_custom(int stdout_is_tty, const char* pager)
 {
-	const char *pager;
-
 	if (!stdout_is_tty)
 		return NULL;
 
-	pager = getenv("GIT_PAGER");
+	if (!pager || !*pager)
+		pager = getenv("GIT_PAGER");
 	if (!pager) {
 		if (!pager_program)
 			read_early_config(core_pager_config, NULL);
@@ -97,6 +96,11 @@ const char *git_pager(int stdout_is_tty)
 	return pager;
 }
 
+const char *git_pager(int stdout_is_tty)
+{
+	return git_pager_custom(stdout_is_tty, NULL);
+}
+
 static void setup_pager_env(struct strvec *env)
 {
 	const char **argv;
@@ -132,10 +136,11 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
 	pager_process->trace2_child_class = "pager";
 }
 
-void setup_pager(void)
+void setup_custom_pager(const char* pager)
 {
 	static int once = 0;
-	const char *pager = git_pager(isatty(1));
+
+	pager = git_pager_custom(isatty(1), pager);
 
 	if (!pager)
 		return;
diff --git a/pager.h b/pager.h
index 103ecac476..2166662361 100644
--- a/pager.h
+++ b/pager.h
@@ -4,7 +4,11 @@
 struct child_process;
 
 const char *git_pager(int stdout_is_tty);
-void setup_pager(void);
+void setup_custom_pager(const char*);
+static inline void setup_pager(void)
+{
+	setup_custom_pager(NULL);
+}
 void wait_for_pager(void);
 int pager_in_use(void);
 int term_columns(void);
-- 
2.45.0.97.gb691764a17

Re: [PATCH v4 4/6] pager: introduce setup_custom_pager

[Junio C Hamano]

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

> Introduce a new function setup_custom_pager() to allow setting up our
> pager mechanism using a custom pager.  If the custom pager specified is
> NULL or an empty string, use the normal pager as setup_pager() currently
> does.

We often see "if the pointer is NULL or points at an empty string"
in code that were originally ported from the scripted Porcelain, but
I doubt we would want to follow that pattern in new code paths.

>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  pager.c | 17 +++++++++++------
>  pager.h |  6 +++++-
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/pager.c b/pager.c
> index 925f860335..21a7d9cd60 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -74,14 +74,13 @@ static int core_pager_config(const char *var, const char *value,
>  	return 0;
>  }
>  
> -const char *git_pager(int stdout_is_tty)
> +static const char *git_pager_custom(int stdout_is_tty, const char* pager)

Call it git_custom_pager(), to be more grammatical and also match
the other one, setup_custom_pager().

The asterisk should stick to "pager", not its type.

>  {
> -	const char *pager;
> -
>  	if (!stdout_is_tty)
>  		return NULL;
>  
> -	pager = getenv("GIT_PAGER");
> +	if (!pager || !*pager)
> +		pager = getenv("GIT_PAGER");

We often see "if the pointer is NULL or points at an empty string"
in code that were originally ported from the scripted Porcelain, but
I doubt we would want to follow that pattern in new code paths.

> @@ -97,6 +96,11 @@ const char *git_pager(int stdout_is_tty)
>  	return pager;
>  }
>  
> +const char *git_pager(int stdout_is_tty)
> +{
> +	return git_pager_custom(stdout_is_tty, NULL);
> +}

OK.

> @@ -132,10 +136,11 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
>  	pager_process->trace2_child_class = "pager";
>  }
>  
> -void setup_pager(void)
> +void setup_custom_pager(const char* pager)

The asterisk should stick to "pager", not its type.

>  {
>  	static int once = 0;
> -	const char *pager = git_pager(isatty(1));
> +
> +	pager = git_pager_custom(isatty(1), pager);

This feels a bit too convoluted.  This thing already knows if it got
a custom one from its caller because "*pager" is _its_ parameter.
Perhaps rip out all the changes before and including the hunk
"@@ -132,10 +136,11 @@" and start it like so, perhaps?

	void setup_custom_pager(const char *pager)
	{
		if (!pager)
			pager = git_pager(isatty(1));
		...

> diff --git a/pager.h b/pager.h
> index 103ecac476..2166662361 100644
> --- a/pager.h
> +++ b/pager.h
> @@ -4,7 +4,11 @@
>  struct child_process;
>  
>  const char *git_pager(int stdout_is_tty);
> -void setup_pager(void);
> +void setup_custom_pager(const char*);
> +static inline void setup_pager(void)
> +{
> +	setup_custom_pager(NULL);
> +}

A good approach to help existing callers---there are more than half
a dozen existing callers to setup_pager() in the code base.  We
could migrate them all away to pass NULL but it would be totally
outside the scope of this topic.

[PATCH v4 5/6] test-terminal: introduce --no-stdin-pty

[Rubén Justo]

In 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
2015-08-04), t/test-terminal.perl learned to connect the child process'
stdin to a pty.  It works well for what was intended: satisfying an
`isatty(STDIN_FILENO)` check.

However, the fork introduced, that copies the stdin to the child
process, does not always manage to send all the information.

To illustrate this behaviour, we can use a function like this:

    f ()
    {
    	dd if=/dev/zero bs=1 count=10000 status=none |
    	t/test-terminal.perl cat - 2>/dev/null |
    	wc -c;
    }

We do not obtain the expected results when executing this function
100 times:

    $ for i in $(seq 100); do f; done | sort | uniq -c
         36 0
          4 1
         53 4095
          7 4159

If we do the same with a version that does not redirect stdin, a version
prior to 18d8c26930, the expected result is obtained:

    $ git checkout 18d8c26930~1
    $ for i in $(seq 100); do f; done | sort | uniq -c
        100 10000

In a subsequent commit, a new test is going to rely on test-terminate,
and it does not require stdin to be connected to a terminal, but all
piped data needs to be successfully transmitted to the child process.

To make this possible, add a new parameter "--no-stdin-pty" to allow
disabling the stdin redirection though a pty.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/test-terminal.perl | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 3810e9bb43..85edc9e8b9 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -12,10 +12,10 @@ sub start_child {
 	if (not defined $pid) {
 		die "fork failed: $!"
 	} elsif ($pid == 0) {
-		open STDIN, "<&", $in;
+		open STDIN, "<&", $in if $in;
 		open STDOUT, ">&", $out;
 		open STDERR, ">&", $err;
-		close $in;
+		close $in if $in;
 		close $out;
 		exec(@$argv) or die "cannot exec '$argv->[0]': $!"
 	}
@@ -78,28 +78,32 @@ sub copy_stdio {
 }
 
 if ($#ARGV < 1) {
-	die "usage: test-terminal program args";
+	die "usage: test-terminal [--no-stdin-pty] program args";
 }
+my $no_stdin_pty = $ARGV[0] eq '--no-stdin-pty';
+shift @ARGV if $no_stdin_pty;
 $ENV{TERM} = 'vt100';
-my $parent_in = new IO::Pty;
+my $parent_in = $no_stdin_pty ? undef : IO::Pty->new;
 my $parent_out = new IO::Pty;
 my $parent_err = new IO::Pty;
-$parent_in->set_raw();
+$parent_in->set_raw() if $parent_in;
 $parent_out->set_raw();
 $parent_err->set_raw();
-$parent_in->slave->set_raw();
+$parent_in->slave->set_raw() if $parent_in;
 $parent_out->slave->set_raw();
 $parent_err->slave->set_raw();
-my $pid = start_child(\@ARGV, $parent_in->slave, $parent_out->slave, $parent_err->slave);
-close $parent_in->slave;
+my $pid = start_child(\@ARGV,$parent_in ? $parent_in->slave : undef, $parent_out->slave, $parent_err->slave);
+close $parent_in->slave if $parent_in;
 close $parent_out->slave;
 close $parent_err->slave;
-my $in_pid = copy_stdin($parent_in);
+my $in_pid = $no_stdin_pty ? 0 : copy_stdin($parent_in);
 copy_stdio($parent_out, $parent_err);
 my $ret = finish_child($pid);
-# If the child process terminates before our copy_stdin() process is able to
-# write all of its data to $parent_in, the copy_stdin() process could stall.
-# Send SIGTERM to it to ensure it terminates.
-kill 'TERM', $in_pid;
-finish_child($in_pid);
+if ($in_pid) {
+	# If the child process terminates before our copy_stdin() process is able to
+	# write all of its data to $parent_in, the copy_stdin() process could stall.
+	# Send SIGTERM to it to ensure it terminates.
+	kill 'TERM', $in_pid;
+	finish_child($in_pid);
+}
 exit($ret);
-- 
2.45.0.97.gb691764a17

Re: [PATCH v4 5/6] test-terminal: introduce --no-stdin-pty

[Phillip Wood]

Hi Rubén

On 03/06/2024 21:38, Rubén Justo wrote:
> In 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
> 2015-08-04), t/test-terminal.perl learned to connect the child process'
> stdin to a pty.  It works well for what was intended: satisfying an
> `isatty(STDIN_FILENO)` check.
> 
> However, the fork introduced, that copies the stdin to the child
> process, does not always manage to send all the information.

I think the problem maybe to do with the use of File::Copy, not with the 
fork. The man page for the copy function says

     Note  that  passing  in  files  as  handles  instead  of  names may
     lead to loss of information on some operating systems; it is
     recommended that you use file names whenever possible.

Rather than adding a new flag to work around a bug in our script it 
might be better to try and fix the bug by using a loop that reads blocks 
of data from the source and writes them to the destination instead of 
calling copy.

Best Wishes

Phillip

> To illustrate this behaviour, we can use a function like this:
> 
>      f ()
>      {
>      	dd if=/dev/zero bs=1 count=10000 status=none |
>      	t/test-terminal.perl cat - 2>/dev/null |
>      	wc -c;
>      }
> 
> We do not obtain the expected results when executing this function
> 100 times:
> 
>      $ for i in $(seq 100); do f; done | sort | uniq -c
>           36 0
>            4 1
>           53 4095
>            7 4159
> 
> If we do the same with a version that does not redirect stdin, a version
> prior to 18d8c26930, the expected result is obtained:
> 
>      $ git checkout 18d8c26930~1
>      $ for i in $(seq 100); do f; done | sort | uniq -c
>          100 10000
> 
> In a subsequent commit, a new test is going to rely on test-terminate,
> and it does not require stdin to be connected to a terminal, but all
> piped data needs to be successfully transmitted to the child process.
> 
> To make this possible, add a new parameter "--no-stdin-pty" to allow
> disabling the stdin redirection though a pty.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   t/test-terminal.perl | 32 ++++++++++++++++++--------------
>   1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/t/test-terminal.perl b/t/test-terminal.perl
> index 3810e9bb43..85edc9e8b9 100755
> --- a/t/test-terminal.perl
> +++ b/t/test-terminal.perl
> @@ -12,10 +12,10 @@ sub start_child {
>   	if (not defined $pid) {
>   		die "fork failed: $!"
>   	} elsif ($pid == 0) {
> -		open STDIN, "<&", $in;
> +		open STDIN, "<&", $in if $in;
>   		open STDOUT, ">&", $out;
>   		open STDERR, ">&", $err;
> -		close $in;
> +		close $in if $in;
>   		close $out;
>   		exec(@$argv) or die "cannot exec '$argv->[0]': $!"
>   	}
> @@ -78,28 +78,32 @@ sub copy_stdio {
>   }
>   
>   if ($#ARGV < 1) {
> -	die "usage: test-terminal program args";
> +	die "usage: test-terminal [--no-stdin-pty] program args";
>   }
> +my $no_stdin_pty = $ARGV[0] eq '--no-stdin-pty';
> +shift @ARGV if $no_stdin_pty;
>   $ENV{TERM} = 'vt100';
> -my $parent_in = new IO::Pty;
> +my $parent_in = $no_stdin_pty ? undef : IO::Pty->new;
>   my $parent_out = new IO::Pty;
>   my $parent_err = new IO::Pty;
> -$parent_in->set_raw();
> +$parent_in->set_raw() if $parent_in;
>   $parent_out->set_raw();
>   $parent_err->set_raw();
> -$parent_in->slave->set_raw();
> +$parent_in->slave->set_raw() if $parent_in;
>   $parent_out->slave->set_raw();
>   $parent_err->slave->set_raw();
> -my $pid = start_child(\@ARGV, $parent_in->slave, $parent_out->slave, $parent_err->slave);
> -close $parent_in->slave;
> +my $pid = start_child(\@ARGV,$parent_in ? $parent_in->slave : undef, $parent_out->slave, $parent_err->slave);
> +close $parent_in->slave if $parent_in;
>   close $parent_out->slave;
>   close $parent_err->slave;
> -my $in_pid = copy_stdin($parent_in);
> +my $in_pid = $no_stdin_pty ? 0 : copy_stdin($parent_in);
>   copy_stdio($parent_out, $parent_err);
>   my $ret = finish_child($pid);
> -# If the child process terminates before our copy_stdin() process is able to
> -# write all of its data to $parent_in, the copy_stdin() process could stall.
> -# Send SIGTERM to it to ensure it terminates.
> -kill 'TERM', $in_pid;
> -finish_child($in_pid);
> +if ($in_pid) {
> +	# If the child process terminates before our copy_stdin() process is able to
> +	# write all of its data to $parent_in, the copy_stdin() process could stall.
> +	# Send SIGTERM to it to ensure it terminates.
> +	kill 'TERM', $in_pid;
> +	finish_child($in_pid);
> +}
>   exit($ret);

Re: [PATCH v4 5/6] test-terminal: introduce --no-stdin-pty

[Jeff King]

On Tue, Jun 04, 2024 at 11:05:15AM +0100, Phillip Wood wrote:

> Hi Rubén
> 
> On 03/06/2024 21:38, Rubén Justo wrote:
> > In 18d8c26930 (test_terminal: redirect child process' stdin to a pty,
> > 2015-08-04), t/test-terminal.perl learned to connect the child process'
> > stdin to a pty.  It works well for what was intended: satisfying an
> > `isatty(STDIN_FILENO)` check.
> > 
> > However, the fork introduced, that copies the stdin to the child
> > process, does not always manage to send all the information.
> 
> I think the problem maybe to do with the use of File::Copy, not with the
> fork. The man page for the copy function says
> 
>     Note  that  passing  in  files  as  handles  instead  of  names may
>     lead to loss of information on some operating systems; it is
>     recommended that you use file names whenever possible.
> 
> Rather than adding a new flag to work around a bug in our script it might be
> better to try and fix the bug by using a loop that reads blocks of data from
> the source and writes them to the destination instead of calling copy.

I don't think I've seen missing data. But do note that the test_terminal
stdin handling is racy:

  https://lore.kernel.org/git/20190520125016.GA13474@sigill.intra.peff.net/

IMHO we should consider getting rid of it entirely. I think the only
thing that uses it is t4153 (AFAICT it is luckily not racy because it
does not actually read stdin, but only checks isatty).

-Peff

Re: [PATCH v4 5/6] test-terminal: introduce --no-stdin-pty

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> IMHO we should consider getting rid of it entirely. I think the only
> thing that uses it is t4153 (AFAICT it is luckily not racy because it
> does not actually read stdin, but only checks isatty).

Sounds like a better approach than piling another workaround on the
test helper.  Reading the old discussion, we seem to have been in
agreement that we generally do not have to insist reading from a tty
and certainly the "add -p" codepath is not one of those "if your
other payload must come from the standard input, your instructions
to specify how to handle that data needs to come from elsewhere, and
that is /dev/tty" cases.

Thanks.

Re: [PATCH v4 5/6] test-terminal: introduce --no-stdin-pty

[Jeff King]

On Wed, Jun 05, 2024 at 08:39:59AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > IMHO we should consider getting rid of it entirely. I think the only
> > thing that uses it is t4153 (AFAICT it is luckily not racy because it
> > does not actually read stdin, but only checks isatty).
> 
> Sounds like a better approach than piling another workaround on the
> test helper.  Reading the old discussion, we seem to have been in
> agreement that we generally do not have to insist reading from a tty
> and certainly the "add -p" codepath is not one of those "if your
> other payload must come from the standard input, your instructions
> to specify how to handle that data needs to come from elsewhere, and
> that is /dev/tty" cases.

I think we got rid of all of the "read interactive input over tty"
cases. The one that I still see is am's heuristic to use isatty() to
decide whether the user might send patches over stdin.

I just sent a separate patch series which I think improves the
situation. And then either Rubén could build on top of that (if we think
it will graduate quickly) or he could do his optional patch, and we
could rip it back out when the two are merged.

-Peff

Re: [PATCH v4 5/6] test-terminal: introduce --no-stdin-pty

[Rubén Justo]

On Tue, Jun 04, 2024 at 11:05:15AM +0100, Phillip Wood wrote:

> Rather than adding a new flag to work around a bug in our script it might be
> better to try and fix the bug by using a loop that reads blocks of data from
> the source and writes them to the destination instead of calling copy.

To be honest, I've tried.  I haven't found a way to fix it properly.

I also thought about removing it.  I agree with Peff, removing the stdin
redirection makes more sense.  IMHO simplifies.

But, perhaps we can happily live another almost-decade with this new
--no-stdin-pty, before finally removing the stdin redirection in this
helper. :-)

Re: [PATCH v4 5/6] test-terminal: introduce --no-stdin-pty

[Jeff King]

On Thu, Jun 06, 2024 at 12:50:39AM +0200, Rubén Justo wrote:

> On Tue, Jun 04, 2024 at 11:05:15AM +0100, Phillip Wood wrote:
> 
> > Rather than adding a new flag to work around a bug in our script it might be
> > better to try and fix the bug by using a loop that reads blocks of data from
> > the source and writes them to the destination instead of calling copy.
> 
> To be honest, I've tried.  I haven't found a way to fix it properly.

I think File::Copy() is not the culprit. The problem is more at the
syscall level. Once test_terminal finishes writing all of the output to
the tty, it closes its end of the descriptor. And now the tty is "gone",
isatty() returns false, and further reads get EIO (even if we didn't
read all of the bytes! They're lost).

So I think the only thing we could do is _not_ actually close the
descriptor. But now we don't have a way to signal EOF to the other side.
Unless perhaps there is some clever way to do so. I'd still favor
ripping it out.

> I also thought about removing it.  I agree with Peff, removing the stdin
> redirection makes more sense.  IMHO simplifies.
> 
> But, perhaps we can happily live another almost-decade with this new
> --no-stdin-pty, before finally removing the stdin redirection in this
> helper. :-)

Hopefully not a decade. ;) I just sent a series, and I think you could
either build on top, or the merge resolution could just drop your new
option.

-Peff

Re: [PATCH v4 5/6] test-terminal: introduce --no-stdin-pty

[Rubén Justo]

On Thu, Jun 06, 2024 at 04:27:48AM -0400, Jeff King wrote:

> I just sent a series, and I think you could either build on top, or
> the merge resolution could just drop your new option.

I've already responded to your series, but just to confirm here.  I'm
glad to drop this step and building on top of jk/am-retry.

Thanks!

[PATCH v4 6/6] add-patch: introduce the command '|'

[Rubén Justo]

Introduce a new command '|' to send the current hunk to a program.  If
no program is specified, use the pager.

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

diff --git a/add-patch.c b/add-patch.c
index 814de57c4a..5d8a2f97f9 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -5,6 +5,7 @@
 #include "environment.h"
 #include "gettext.h"
 #include "object-name.h"
+#include "pager.h"
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "strbuf.h"
@@ -1389,6 +1390,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
    "s - split the current hunk into smaller hunks\n"
    "e - manually edit the current hunk\n"
    "p - print the current hunk\n"
+   "| - use pager to show the current hunk, or use |<program> to customize\n"
    "? - print help\n");
 
 static int patch_update_file(struct add_p_state *s,
@@ -1401,6 +1403,7 @@ static int patch_update_file(struct add_p_state *s,
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int colored = !!s->colored.len, quit = 0;
 	enum prompt_mode_type prompt_mode_type;
+	const char* pager = NULL;
 	enum {
 		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
 		ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
@@ -1449,9 +1452,15 @@ 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 (pager)
+					setup_custom_pager(pager);
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
+				if (pager) {
+					wait_for_pager();
+					pager = NULL;
+				}
 			}
 
 			strbuf_reset(&s->buf);
@@ -1485,6 +1494,7 @@ static int patch_update_file(struct add_p_state *s,
 				strbuf_addstr(&s->buf, ",e");
 			}
 			strbuf_addstr(&s->buf, ",p");
+			strbuf_addstr(&s->buf, ",|");
 		}
 		if (file_diff->deleted)
 			prompt_mode_type = PROMPT_DELETION;
@@ -1512,8 +1522,8 @@ static int patch_update_file(struct add_p_state *s,
 			continue;
 		ch = tolower(s->answer.buf[0]);
 
-		/* 'g' takes a hunk number and '/' takes a regexp */
-		if (s->answer.len != 1 && (ch != 'g' && ch != '/')) {
+		/* 'g' takes a hunk number, '/' takes a regexp and '|' takes a program */
+		if (s->answer.len != 1 && (ch != 'g' && ch != '/' && ch != '|')) {
 			err(s, _("Only one letter is expected, got '%s'"), s->answer.buf);
 			continue;
 		}
@@ -1674,6 +1684,17 @@ static int patch_update_file(struct add_p_state *s,
 			}
 		} else if (s->answer.buf[0] == 'p') {
 			rendered_hunk_index = -1;
+		} else if (ch == '|') {
+			strbuf_remove(&s->answer, 0, 1);
+			if (s->s.use_single_key && s->answer.len == 0) {
+				printf("%s", _("program? "));
+				fflush(stdout);
+				strbuf_getline(&s->answer, stdin);
+				strbuf_trim_trailing_newline(&s->answer);
+			}
+			strbuf_trim(&s->answer);
+			pager = s->answer.buf;
+			rendered_hunk_index = -1;
 		} 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 6f6d174687..7b3ebb671d 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -64,8 +64,8 @@ test_expect_success 'unknown command' '
 	git add -N command &&
 	git diff command >expect &&
 	cat >>expect <<-EOF &&
-	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
-	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
+	(1/1) Stage addition [y,n,q,a,d,e,p,|,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
+	(1/1) Stage addition [y,n,q,a,d,e,p,|,?]?$SP
 	EOF
 	git add -p -- command <command >actual 2>&1 &&
 	test_cmp expect actual
@@ -348,9 +348,9 @@ test_expect_success 'different prompts for mode change/deleted' '
 	git -c core.filemode=true add -p >actual &&
 	sed -n "s/^\(([0-9/]*) Stage .*?\).*/\1/p" actual >actual.filtered &&
 	cat >expect <<-\EOF &&
-	(1/1) Stage deletion [y,n,q,a,d,p,?]?
-	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]?
+	(1/1) Stage deletion [y,n,q,a,d,p,|,?]?
+	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,|,?]?
+	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,|,?]?
 	EOF
 	test_cmp expect actual.filtered
 '
@@ -537,13 +537,13 @@ test_expect_success 'split hunk setup' '
 test_expect_success 'goto hunk 1 with "g 1"' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
+	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,|,?]? + 1:  -1,2 +1,3          +15
 	_ 2:  -2,4 +3,8          +21
 	go to which hunk? @@ -1,2 +1,3 @@
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
 	EOF
 	test_write_lines s y g 1 | git add -p >actual &&
 	tail -n 7 <actual >actual.trimmed &&
@@ -556,7 +556,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
 	EOF
 	test_write_lines s y g1 | git add -p >actual &&
 	tail -n 4 <actual >actual.trimmed &&
@@ -566,11 +566,11 @@ test_expect_success 'goto hunk 1 with "g1"' '
 test_expect_success 'navigate to hunk via regex /pattern' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	(2/2) Stage this hunk [y,n,q,a,d,K,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,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
 	EOF
 	test_write_lines s y /1,2 | git add -p >actual &&
 	tail -n 5 <actual >actual.trimmed &&
@@ -583,7 +583,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
 	EOF
 	test_write_lines s y / 1,2 | git add -p >actual &&
 	tail -n 4 <actual >actual.trimmed &&
@@ -595,17 +595,38 @@ test_expect_success 'print again the hunk' '
 	tr _ " " >expect <<-EOF &&
 	+15
 	 20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	(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,?]?_
+	(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 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 \| |
+	(
+		GIT_PAGER="sed s/^/PAGER\ /" &&
+		export GIT_PAGER &&
+		test_terminal --no-stdin-pty 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:
 	#
@@ -780,21 +801,21 @@ test_expect_success 'colors can be overridden' '
 	<BLUE>+<RESET><BLUE>new<RESET>
 	<CYAN> more-context<RESET>
 	<BLUE>+<RESET><BLUE>another-one<RESET>
-	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,|,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
 	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
 	<CYAN> context<RESET>
 	<BOLD>-old<RESET>
 	<BLUE>+<RESET><BLUE>new<RESET>
 	<CYAN> more-context<RESET>
-	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
 	<CYAN> more-context<RESET>
 	<BLUE>+<RESET><BLUE>another-one<RESET>
-	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,|,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
 	<CYAN> context<RESET>
 	<BOLD>-old<RESET>
 	<BLUE>+new<RESET>
 	<CYAN> more-context<RESET>
-	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]? <RESET>
 	EOF
 	test_cmp expect actual
 '
-- 
2.45.0.97.gb691764a17

Re: [PATCH v4 6/6] add-patch: introduce the command '|'

[Junio C Hamano]

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

> @@ -1389,6 +1390,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
>     "s - split the current hunk into smaller hunks\n"
>     "e - manually edit the current hunk\n"
>     "p - print the current hunk\n"
> +   "| - use pager to show the current hunk, or use |<program> to customize\n"
>     "? - print help\n");

"to customize" strongly hints that the customization will stick, at
least during this session.  Is that what actually happens?

> @@ -1401,6 +1403,7 @@ static int patch_update_file(struct add_p_state *s,
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	int colored = !!s->colored.len, quit = 0;
>  	enum prompt_mode_type prompt_mode_type;
> +	const char* pager = NULL;

The asterisk sticks to "pager", not its type.

>  	enum {
>  		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
>  		ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
> @@ -1449,9 +1452,15 @@ 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 (pager)
> +					setup_custom_pager(pager);
>  				render_hunk(s, hunk, 0, colored, &s->buf);
>  				fputs(s->buf.buf, stdout);
>  				rendered_hunk_index = hunk_index;
> +				if (pager) {
> +					wait_for_pager();
> +					pager = NULL;
> +				}
>  			}
>  
>  			strbuf_reset(&s->buf);
> @@ -1485,6 +1494,7 @@ static int patch_update_file(struct add_p_state *s,
>  				strbuf_addstr(&s->buf, ",e");
>  			}
>  			strbuf_addstr(&s->buf, ",p");
> +			strbuf_addstr(&s->buf, ",|");
>  		}
>  		if (file_diff->deleted)
>  			prompt_mode_type = PROMPT_DELETION;
> @@ -1512,8 +1522,8 @@ static int patch_update_file(struct add_p_state *s,
>  			continue;
>  		ch = tolower(s->answer.buf[0]);
>  
> -		/* 'g' takes a hunk number and '/' takes a regexp */
> -		if (s->answer.len != 1 && (ch != 'g' && ch != '/')) {
> +		/* 'g' takes a hunk number, '/' takes a regexp and '|' takes a program */
> +		if (s->answer.len != 1 && (ch != 'g' && ch != '/' && ch != '|')) {

Not limited to this instance, but a good discipline is to stop and
think twice before adding the third thing to already existing two.

Perhaps

		/*
		 * 'g' takes a hunk number to go to.
		 * '/' takes a regexp to match.
		 * '|' takes a program to pipe to.
		 */
		if (s->answer.len != 1 && !strchr("g/|", ch))

> @@ -1674,6 +1684,17 @@ static int patch_update_file(struct add_p_state *s,
>  			}
>  		} else if (s->answer.buf[0] == 'p') {
>  			rendered_hunk_index = -1;
> +		} else if (ch == '|') {
> +			strbuf_remove(&s->answer, 0, 1);
> +			if (s->s.use_single_key && s->answer.len == 0) {

If you check .use_single_key, you do not need to check answer.len,
do you?  Can it ever be anything other than 0 here in the single-key
mode?

> +				printf("%s", _("program? "));
> +				fflush(stdout);
> +				strbuf_getline(&s->answer, stdin);
> +				strbuf_trim_trailing_newline(&s->answer);
> +			}
> +			strbuf_trim(&s->answer);
> +			pager = s->answer.buf;

Is it safe to peek into s->answer.buf and expect it to be live until
we have to use the pager like this?

By the way, it should be trivial to make the "custom" pager more sticky.

		} else if (ch == '|') {
			if (s->s.use_single_key) {
				... read into s->answer ...
			} else {
				strbuf_remove(&s->answer, 0, 1);
			}
			strbuf_trim_trailing_newline(&s->answer);

			/*
                         * If it is completely empty, use the last
                         * one, if it is semi-empty, reset to the default.
                         */
			if (!s->answer.len) {
				;
			} else {
				FREE_AND_NULL(pager);
				strbuf_trim(&s->answer);
                                if (!s->answer.len)
                                	pager = xstrdup(s->answer.buf);
			}

Even better, we can lift the scope of "pager" one level up, define
it as an on-stack variable in run_add_p(), add a new parameter of
type "const char **" to patch_update_file(), and use it throughout
patch_update_file(), so that a "custom" pager set here will be
carried across file boundaries.

> +			rendered_hunk_index = -1;
>  		} 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 6f6d174687..7b3ebb671d 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -64,8 +64,8 @@ test_expect_success 'unknown command' '
>  	git add -N command &&
>  	git diff command >expect &&
>  	cat >>expect <<-EOF &&
> -	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> -	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
> +	(1/1) Stage addition [y,n,q,a,d,e,p,|,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> +	(1/1) Stage addition [y,n,q,a,d,e,p,|,?]?$SP
>  	EOF
>  	git add -p -- command <command >actual 2>&1 &&
>  	test_cmp expect actual
> @@ -348,9 +348,9 @@ test_expect_success 'different prompts for mode change/deleted' '
>  	git -c core.filemode=true add -p >actual &&
>  	sed -n "s/^\(([0-9/]*) Stage .*?\).*/\1/p" actual >actual.filtered &&
>  	cat >expect <<-\EOF &&
> -	(1/1) Stage deletion [y,n,q,a,d,p,?]?
> -	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]?
> +	(1/1) Stage deletion [y,n,q,a,d,p,|,?]?
> +	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,|,?]?
> +	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,|,?]?
>  	EOF
>  	test_cmp expect actual.filtered
>  '
> @@ -537,13 +537,13 @@ test_expect_success 'split hunk setup' '
>  test_expect_success 'goto hunk 1 with "g 1"' '
>  	test_when_finished "git reset" &&
>  	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
> +	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,|,?]? + 1:  -1,2 +1,3          +15
>  	_ 2:  -2,4 +3,8          +21
>  	go to which hunk? @@ -1,2 +1,3 @@
>  	_10
>  	+15
>  	_20
> -	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
> +	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
>  	EOF
>  	test_write_lines s y g 1 | git add -p >actual &&
>  	tail -n 7 <actual >actual.trimmed &&
> @@ -556,7 +556,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
>  	_10
>  	+15
>  	_20
> -	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
> +	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
>  	EOF
>  	test_write_lines s y g1 | git add -p >actual &&
>  	tail -n 4 <actual >actual.trimmed &&
> @@ -566,11 +566,11 @@ test_expect_success 'goto hunk 1 with "g1"' '
>  test_expect_success 'navigate to hunk via regex /pattern' '
>  	test_when_finished "git reset" &&
>  	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@
> +	(2/2) Stage this hunk [y,n,q,a,d,K,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,?]?_
> +	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
>  	EOF
>  	test_write_lines s y /1,2 | git add -p >actual &&
>  	tail -n 5 <actual >actual.trimmed &&
> @@ -583,7 +583,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '
>  	_10
>  	+15
>  	_20
> -	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
> +	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
>  	EOF
>  	test_write_lines s y / 1,2 | git add -p >actual &&
>  	tail -n 4 <actual >actual.trimmed &&
> @@ -595,17 +595,38 @@ test_expect_success 'print again the hunk' '
>  	tr _ " " >expect <<-EOF &&
>  	+15
>  	 20
> -	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
> +	(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,?]?_
> +	(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 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 \| |
> +	(
> +		GIT_PAGER="sed s/^/PAGER\ /" &&
> +		export GIT_PAGER &&
> +		test_terminal --no-stdin-pty 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:
>  	#
> @@ -780,21 +801,21 @@ test_expect_success 'colors can be overridden' '
>  	<BLUE>+<RESET><BLUE>new<RESET>
>  	<CYAN> more-context<RESET>
>  	<BLUE>+<RESET><BLUE>another-one<RESET>
> -	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
> +	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,|,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
>  	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
>  	<CYAN> context<RESET>
>  	<BOLD>-old<RESET>
>  	<BLUE>+<RESET><BLUE>new<RESET>
>  	<CYAN> more-context<RESET>
> -	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
> +	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
>  	<CYAN> more-context<RESET>
>  	<BLUE>+<RESET><BLUE>another-one<RESET>
> -	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
> +	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,|,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
>  	<CYAN> context<RESET>
>  	<BOLD>-old<RESET>
>  	<BLUE>+new<RESET>
>  	<CYAN> more-context<RESET>
> -	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
> +	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]? <RESET>
>  	EOF
>  	test_cmp expect actual
>  '

Re: [PATCH v4 6/6] add-patch: introduce the command '|'

[Dragan Simic]

On 2024-06-04 19:12, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
>> @@ -1389,6 +1390,7 @@ N_("j - leave this hunk undecided, see next 
>> undecided hunk\n"
>>     "s - split the current hunk into smaller hunks\n"
>>     "e - manually edit the current hunk\n"
>>     "p - print the current hunk\n"
>> +   "| - use pager to show the current hunk, or use |<program> to 
>> customize\n"
>>     "? - print help\n");
> 
> "to customize" strongly hints that the customization will stick, at
> least during this session.  Is that what actually happens?

Good point.  I used "customize" in the proposed wording improvement
because we previously (kind of) agreed about making "| <program>",
once selected, stick to the end of the current session.

Re: [PATCH v4 6/6] add-patch: introduce the command '|'

[Junio C Hamano]

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

> By the way, it should be trivial to make the "custom" pager more sticky.

Here is what you can squash into this step.  I gave many other
pieces of style and design advices in other messages, which are not
covered by this patch but the necessary fixes should be obvious.

This message is only about making the custom pager stick during a
session.  It does not adjust the command help to give the last pager
command (or literally "your pager"), either.

---- >8 ----
Subject: [PATCH] add-p: make custom pager sticky during a session

The original design kept resetting the choice of the custom pager
every time the '|' command is used.  This was way cumbersome to use.

Keep track of the last choice in the add_p_state.custom_pager
member.  This value can stick across calls to patch_update_file()
function, so a custom pager used for choosing hunks in one file
can be carried over to the view hunks in the next file.

As we make the custom pager stick, we need a way to reset it back to
the default value (which we use NULL for, as set_custom_pager()
takes the value to mean "use the default one").

As there is no value that can say "pager is not used" suitable for
the custom_pager member to take, we need a separate "use_pager" flag
so that the fact that '|' command was used can be propagated to the
next iteration of the loop, independent from what custom pager is
used.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 add-patch.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index da13e267db..71ee7f9a94 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -272,6 +272,7 @@ struct add_p_state {
 	/* patch mode */
 	struct patch_mode *mode;
 	const char *revision;
+	char *custom_pager;
 };
 
 static void add_p_state_clear(struct add_p_state *s)
@@ -285,6 +286,7 @@ static void add_p_state_clear(struct add_p_state *s)
 	for (i = 0; i < s->file_diff_nr; i++)
 		free(s->file_diff[i].hunk);
 	free(s->file_diff);
+	free(s->custom_pager);
 	clear_add_i_state(&s->s);
 }
 
@@ -1403,7 +1405,7 @@ static int patch_update_file(struct add_p_state *s,
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int colored = !!s->colored.len, quit = 0;
 	enum prompt_mode_type prompt_mode_type;
-	const char* pager = NULL;
+	int use_pager = 0;
 	enum {
 		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
 		ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
@@ -1452,14 +1454,14 @@ 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 (pager)
-					setup_custom_pager(pager);
+				if (use_pager)
+					setup_custom_pager(s->custom_pager);
 				render_hunk(s, hunk, 0, colored, &s->buf);
 				fputs(s->buf.buf, stdout);
 				rendered_hunk_index = hunk_index;
-				if (pager) {
+				if (use_pager) {
 					wait_for_pager();
-					pager = NULL;
+					use_pager = 0;
 				}
 			}
 
@@ -1685,15 +1687,26 @@ static int patch_update_file(struct add_p_state *s,
 		} else if (s->answer.buf[0] == 'p') {
 			rendered_hunk_index = -1;
 		} else if (ch == '|') {
-			strbuf_remove(&s->answer, 0, 1);
-			if (s->s.use_single_key && s->answer.len == 0) {
+			if (!s->s.use_single_key) {
+				strbuf_remove(&s->answer, 0, 1);
+			} else {
 				printf("%s", _("program? "));
 				fflush(stdout);
 				strbuf_getline(&s->answer, stdin);
-				strbuf_trim_trailing_newline(&s->answer);
 			}
-			strbuf_trim(&s->answer);
-			pager = s->answer.buf;
+			strbuf_trim_trailing_newline(&s->answer);
+
+			if (!s->answer.len)
+				; /* empty input - reuse the previous */
+			else {
+				strbuf_trim(&s->answer);
+				FREE_AND_NULL(s->custom_pager);
+				if (!s->answer.len)
+					; /* semi-empty - use your pager */
+				else
+					s->custom_pager = xstrdup(s->answer.buf);
+			}
+			use_pager = 1;
 			rendered_hunk_index = -1;
 		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
-- 
2.45.2-409-g7b0defb391

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

[Jeff King]

On Sun, Jun 02, 2024 at 05:38:42PM +0200, Rubén Justo wrote:

> However, the current functionality meets my current needs, so I'm happy
> with it.
> 
> This, a new 'interactive.pipeCommand' setting, or a new switch: 'add -P',
> are left for discussing in, hopefully, a future series.

Earlier I suggested that I'd set such a config variable to something
like:

  diff-highlight | less -FX

But after playing with your patch, I realized that:

  - there's no need to pipe through diff-highlight; it already happened
    as part of interactive.diffFilter!

  - since it's triggered manually now, there's no need to add in -FX

So I am perfectly happy for you to stop where you did. Possibly
interactive.pipeCommand could be useful in a more general sense, but we
can wait until somebody encounters that itch.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   diff-highlight | less -FX
>
> But after playing with your patch, I realized that:
>
>   - there's no need to pipe through diff-highlight; it already happened
>     as part of interactive.diffFilter!

;-)

>   - since it's triggered manually now, there's no need to add in -FX

I do not know about -X, but yeah, -F is of dubious value in this
particular context.  You explicitly told us that you want to page,
somehow knowing that the hunk needs paging.

> So I am perfectly happy for you to stop where you did. Possibly
> interactive.pipeCommand could be useful in a more general sense, but we
> can wait until somebody encounters that itch.

It makes it sound like if somebody has a use case already we
shouldn't stop here ;-)

The default that colors the output is something we might later
regret.  Those who want colored output can always use the
interactive.diffFilter configuration, but I am not sure if going
the other direction to strip coloring is just as easy.  But other
than that, I think we are at an OK place to stop.

Thanks.

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

[Jeff King]

On Tue, Jun 04, 2024 at 08:32:04AM -0700, Junio C Hamano wrote:

> The default that colors the output is something we might later
> regret.  Those who want colored output can always use the
> interactive.diffFilter configuration, but I am not sure if going
> the other direction to strip coloring is just as easy.  But other
> than that, I think we are at an OK place to stop.

I don't think diffFilter is a great solution there. In my experience,
that is augmenting the existing coloring done by "diff --color" itself
(and of course many people do not use a separate filter in the first
place, and just see the normal color output). So there's not an easy way
to add the color back to a stripped version.

The interactive-patch code is literally holding the two color/non-color
variants in memory, splitting the hunks based on line counts, and then
showing you one and applying the other. Even if you wanted to run "git
diff --color" yourself in the "|" command, it would be a lot of work to
pick out the hunk of interest.

Given that the main use case for "|" is for human viewing through a
pager, I think the colorful, filtered version meant for users is the
best default. And then the "bare" version can come from an alternate
command or a knob.

Just to note some prior art, mutt's "<pipe-message>" faces a similar
problem. You might want the raw message (if you're going to poke at
headers, MIME parts, etc yourself) or you may want a decoded one (if you
just care about body text and don't want to deal with base64, qp, etc,
yourself). They provide a stateful config knob, but then you end up with
horrible macros that toggle the knob, like:

  :set pipe_decode=yes<enter><pipe-message>my-script<enter>:set pipe_decode=no<enter>

I think having two separate commands for the two modes would be less
confusing.

-Peff

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

[Phillip Wood]

On 05/06/2024 10:09, Jeff King wrote:
> On Tue, Jun 04, 2024 at 08:32:04AM -0700, Junio C Hamano wrote:
> 
> Given that the main use case for "|" is for human viewing through a
> pager, I think the colorful, filtered version meant for users is the
> best default. And then the "bare" version can come from an alternate
> command or a knob.

I think that's a very good point. It is hard to see what "|" can be used 
for other than viewing the hunk as (a) git does not read the output so 
it cannot be used to filter or edit the hunk that is applied and (b) we 
pass an isolated hunk so the post-image offset in the hunk header is 
likely to be wrong and there is no indication as to which file it comes 
from so the program being run cannot apply the hunk itself. Having the 
escape codes does make it harder to filter the hunk. For example to just 
look at the post-image as one needs to do something like

	grep '^[^-+ @]*[+ @]'

instead of just using '^[+ @]' as the pattern but the bonus is that the 
output is colored.

> Just to note some prior art, mutt's "<pipe-message>" faces a similar
> problem. You might want the raw message (if you're going to poke at
> headers, MIME parts, etc yourself) or you may want a decoded one (if you
> just care about body text and don't want to deal with base64, qp, etc,
> yourself). They provide a stateful config knob, but then you end up with
> horrible macros that toggle the knob, like:
> 
>    :set pipe_decode=yes<enter><pipe-message>my-script<enter>:set pipe_decode=no<enter>
> 
> I think having two separate commands for the two modes would be less
> confusing.

That does sound simpler

Best Wishes

Phillip

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

[Dragan Simic]

On 2024-06-05 15:21, Phillip Wood wrote:
> On 05/06/2024 10:09, Jeff King wrote:
>> On Tue, Jun 04, 2024 at 08:32:04AM -0700, Junio C Hamano wrote:
>> 
>> Given that the main use case for "|" is for human viewing through a
>> pager, I think the colorful, filtered version meant for users is the
>> best default. And then the "bare" version can come from an alternate
>> command or a knob.
> 
> I think that's a very good point. It is hard to see what "|" can be
> used for other than viewing the hunk as (a) git does not read the
> output so it cannot be used to filter or edit the hunk that is applied
> and (b) we pass an isolated hunk so the post-image offset in the hunk
> header is likely to be wrong and there is no indication as to which
> file it comes from so the program being run cannot apply the hunk
> itself. Having the escape codes does make it harder to filter the
> hunk. For example to just look at the post-image as one needs to do
> something like
> 
> 	grep '^[^-+ @]*[+ @]'
> 
> instead of just using '^[+ @]' as the pattern but the bonus is that
> the output is colored.

Agreed, but as I already explained, [1] only when using the bare "|"
command.  When "|xyz" is used instead, the version of the hunk with
no coloring escape sequences should be piped to xyz.

[1] 
https://lore.kernel.org/git/844704794168f9fcb85c75014c84cde0@manjaro.org/

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

[Rubén Justo]

On Sat, Jun 08, 2024 at 07:54:34AM +0200, Dragan Simic wrote:

> When "|xyz" is used instead, the version of the hunk with no coloring
> escape sequences should be piped to xyz.

That is a sane and conservative approach, and I'm not opposed.  However,
giving the colorful version though a custom pager is a good thing to
have, I think, i.e: allowing a simple "head" without losing the
coloring.

Let's recap a bit.

Initially, this series aimed to enable sending chunks to the pager
during "add -p" sessions.

To reduce the blast radius of spawning a pager for each chunk, we
introduced a new command "P".

Junio suggested opening up the command to allow specifying a custom
pager, in the form of "P<program>".

The "P" command started to resemble a lot to the common pipe operator.
Thus, we shifted to "|<program>".

Some concerns were raised about controlling when to send coloring escape
sequences.  Several ideas were discussed to address this, including
introducing a new command ">", a modifier for "|": "||", and others.
Alternatively, we could leave it up to the user to filter as needed.
Or, simply, do not send escape codes at all.

So, looking back at the ideas discussed in the thread, perhaps a
reasonable next step might be to reintroduce the 'P<program>' command
and let '|<program>' be the way to send raw, uncolored, chunks.

This approach makes sense to me.  I'll wait a bit before sending a
reroll to gather feedback, though.

Thanks.

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

[Dragan Simic]

Hello Ruben,

On 2024-06-09 09:44, Rubén Justo wrote:
> Some concerns were raised about controlling when to send coloring 
> escape
> sequences.  Several ideas were discussed to address this, including
> introducing a new command ">", a modifier for "|": "||", and others.
> Alternatively, we could leave it up to the user to filter as needed.
> Or, simply, do not send escape codes at all.
> 
> So, looking back at the ideas discussed in the thread, perhaps a
> reasonable next step might be to reintroduce the 'P<program>' command
> and let '|<program>' be the way to send raw, uncolored, chunks.

Actually, it would be better to re-introduce the "P" option, without
any parameters, which would display the current hunk through the
already configured pager, and let "|<program>" be the new option
that pipes hunks _without_ coloring escape sequences to "<program>".

As a follow-up, after the dust settles, we could add "><program>" as
another new option that pipes hunks _with_ the coloring escape sequences
to "<program>".  That would be a rather clean approach, which would
also reserve the "weird-looking" commands with additional parameters
(i.e. "|<program>" and "><program>") for the more advanced operations,
while the "normal-looking" command (i.e. "P") would be assigned to
the, presumably, most commonly used new hunk operation.

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

[Rubén Justo]

On Sun, Jun 09, 2024 at 09:57:20AM +0200, Dragan Simic wrote:
> Hello Ruben,
> 
> On 2024-06-09 09:44, Rubén Justo wrote:
> > Some concerns were raised about controlling when to send coloring escape
> > sequences.  Several ideas were discussed to address this, including
> > introducing a new command ">", a modifier for "|": "||", and others.
> > Alternatively, we could leave it up to the user to filter as needed.
> > Or, simply, do not send escape codes at all.
> > 
> > So, looking back at the ideas discussed in the thread, perhaps a
> > reasonable next step might be to reintroduce the 'P<program>' command
> > and let '|<program>' be the way to send raw, uncolored, chunks.
> 
> Actually, it would be better to re-introduce the "P" option, without
> any parameters, which would display the current hunk through the
> already configured pager

I'm sorry, but why limit the "P" command now?  

I understand the caution expressed in another message of this thread
about playing it safe, but I think the user won't be surprised if we
respect here the "color.diff" setting, just like we do with "p", and
...

> and let "|<program>" be the new option
> that pipes hunks _without_ coloring escape sequences to "<program>".

... we'll offer the command "|" to allow the user to process the raw
chunk.

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

[Dragan Simic]

Hello all,

On 2024-06-10 21:09, Rubén Justo wrote:
> On Sun, Jun 09, 2024 at 09:57:20AM +0200, Dragan Simic wrote:
>> On 2024-06-09 09:44, Rubén Justo wrote:
>> > Some concerns were raised about controlling when to send coloring escape
>> > sequences.  Several ideas were discussed to address this, including
>> > introducing a new command ">", a modifier for "|": "||", and others.
>> > Alternatively, we could leave it up to the user to filter as needed.
>> > Or, simply, do not send escape codes at all.
>> >
>> > So, looking back at the ideas discussed in the thread, perhaps a
>> > reasonable next step might be to reintroduce the 'P<program>' command
>> > and let '|<program>' be the way to send raw, uncolored, chunks.
>> 
>> Actually, it would be better to re-introduce the "P" option, without
>> any parameters, which would display the current hunk through the
>> already configured pager
> 
> I'm sorry, but why limit the "P" command now?

I'll explain below, and provide an alternate approach.

> I understand the caution expressed in another message of this thread
> about playing it safe, but I think the user won't be surprised if we
> respect here the "color.diff" setting, just like we do with "p", and
> ...

Let me provide a quotation from git-config(1):

   color.diff
            Whether to use ANSI escape sequences to add color to
            patches. If this is set to always, git-diff(1), git-log(1),
            and git-show(1) will use color for all patches. If it
            is set to true or auto, those commands will only use color
            when output is to the terminal. If unset, then the value
            of color.ui is used (auto by default).

The key takeaway there is "when output is to the terminal", but when
you think about it, the description is a bit vague, because the output
to the terminal is also piped through the default pager when running
git-log(1), for example.  Perhaps the more accurate wording could be
"when the known destination of the output is the terminal", but that
might also be a bit confusing.

Thus, if we want to honor the "color.diff" setting, which I think we
should, then we can do everything with only one new command for "git
add -p", which would be "P<program>".  It would be seen as just another
"channel" for the "output to the terminal" category, and it would honor
"color.diff" just like git-log(1) does, for example, both when received
as "P" and as "Pxyz" from the user.

As a result, there would be no way to pipe a hunk to "xyz" with the
coloring escape sequences removed, unless "color.diff" is set to false.
That would be the most consistent approach, because "P<program>" would
be just another "channel" to reaching the terminal with the output.

Also, we'd need to update the documentation for "color.diff" to include
a description of the new "git add -p" command.

>> and let "|<program>" be the new option
>> that pipes hunks _without_ coloring escape sequences to "<program>".
> 
> ... we'll offer the command "|" to allow the user to process the raw
> chunk.

Sorry, but now I have to ask why do we want that?  If the goal is to
open the hunk in an editor, there's already a command in "git add -p"
for that purpose.

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

[Phillip Wood]

Hi Rubén

On 09/06/2024 08:44, Rubén Justo wrote:
> On Sat, Jun 08, 2024 at 07:54:34AM +0200, Dragan Simic wrote:
> 
>> When "|xyz" is used instead, the version of the hunk with no coloring
>> escape sequences should be piped to xyz.
> 
> That is a sane and conservative approach, and I'm not opposed.  However,
> giving the colorful version though a custom pager is a good thing to
> have, I think, i.e: allowing a simple "head" without losing the
> coloring.
> 
> Let's recap a bit.
> 
> Initially, this series aimed to enable sending chunks to the pager
> during "add -p" sessions.
> 
> To reduce the blast radius of spawning a pager for each chunk, we
> introduced a new command "P".
> 
> Junio suggested opening up the command to allow specifying a custom
> pager, in the form of "P<program>".
> 
> The "P" command started to resemble a lot to the common pipe operator.
> Thus, we shifted to "|<program>".
> 
> Some concerns were raised about controlling when to send coloring escape
> sequences.

I'm still not really convinced that the escape sequences are a problem. 
As Peff has pointed out [1] this new command exists primarily to display 
the current hunk. I've asked for concrete examples of programs that it 
would be useful to run from "git add -p" where the escape codes are a 
problem [2,3]. Sadly the replies talked in generic terms about an 
imaginary program without any reference to displaying or processing a 
hunk from "git add -p". Without a clear use case for stripping the 
escape codes I think we should add a single command that pipes the 
colored output to a user specified program. We can make it clear in the 
documentation that the input to the user's command will contain escape 
sequences unless they pass "-c color.diff=false" when starting "git add 
-p". If it becomes clear that there is a use for the plain output we can 
add that at a later stage.

Best Wishes

Phillip

[1] 
https://lore.kernel.org/git/20240605090935.GF2345232@coredump.intra.peff.net
[2] 
https://lore.kernel.org/git/a2a59f5e-fd55-41d3-8472-b99256e1f428@gmail.com>
[3] 
https://lore.kernel.org/git/1ae0715d-df76-4019-995e-f00f3506f2ac@gmail.com

> Several ideas were discussed to address this, including
> introducing a new command ">", a modifier for "|": "||", and others.
> Alternatively, we could leave it up to the user to filter as needed.
> Or, simply, do not send escape codes at all.
> 
> So, looking back at the ideas discussed in the thread, perhaps a
> reasonable next step might be to reintroduce the 'P<program>' command
> and let '|<program>' be the way to send raw, uncolored, chunks.
> 
> This approach makes sense to me.  I'll wait a bit before sending a
> reroll to gather feedback, though.
> 
> Thanks.

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> ... We can make it
> clear in the documentation that the input to the user's command will
> contain escape sequences unless they pass "-c color.diff=false" when
> starting "git add -p". If it becomes clear that there is a use for the
> plain output we can add that at a later stage.

It is a reasonable stance to take, I would think.  I do not _mind_ a
non-colored mode existing once we identify a use case that needs it.
Those who want colored output already are known (i.e. users of color
capable pagers with customization).  Once we give output that are
colored only, those who want plain output can make complain with
concrete use cases they have.

Thanks.

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

[Rubén Justo]

On Mon, Jun 10, 2024 at 03:09:48PM +0100, Phillip Wood wrote:

> I'm still not really convinced that the escape sequences are a problem.

A concern about the escape sequences was mentioned in this message:
https://lore.kernel.org/git/b7e24b08-40a1-4b18-89f6-e25ab96facaf@gmail.com/

It arose when exploring the possibility of using the new command "|"
with tools that do not support escape sequences, e.g.: "| vim -", or
"| clip.exe" to send the hunk to the clipboard, on Windows.

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

[Junio C Hamano]

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

> On Mon, Jun 10, 2024 at 03:09:48PM +0100, Phillip Wood wrote:
>
>> I'm still not really convinced that the escape sequences are a problem.
>
> A concern about the escape sequences was mentioned in this message:
> https://lore.kernel.org/git/b7e24b08-40a1-4b18-89f6-e25ab96facaf@gmail.com/
>
> It arose when exploring the possibility of using the new command "|"
> with tools that do not support escape sequences, e.g.: "| vim -", or
> "| clip.exe" to send the hunk to the clipboard, on Windows.

Let's not bikeshed this for too long.

Pipe "|" stuff that is suitable for pager in the context of Git
(i.e. if color.ui does not forbid coloring with "no", their usual
pager is capable of color, so we send colored output to the pipe),
and if somebody wants uncolored output later, they can use "P" to
pipe or ">" to file or whatever other enhancement they want, but
let's leave them out of this round.

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

[Dragan Simic]

On 2024-06-10 21:14, Rubén Justo wrote:
> On Mon, Jun 10, 2024 at 03:09:48PM +0100, Phillip Wood wrote:
> 
>> I'm still not really convinced that the escape sequences are a 
>> problem.
> 
> A concern about the escape sequences was mentioned in this message:
> https://lore.kernel.org/git/b7e24b08-40a1-4b18-89f6-e25ab96facaf@gmail.com/
> 
> It arose when exploring the possibility of using the new command "|"
> with tools that do not support escape sequences, e.g.: "| vim -", or
> "| clip.exe" to send the hunk to the clipboard, on Windows.

Hmm, I asked [1] why do we need "|xyz" and escape-sequence-free hunks,
and the answer is above.  Another example would be "|xclip" on Linux.

With that in mind, let's have "P" and "Pxyz" that both honor 
"color.diff",
as already proposed, [1] and "|xyz" that strips the coloring escape
sequences.

[1] 
https://lore.kernel.org/git/3268c498c9ee60803384db6db2d0e94b@manjaro.org/

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

[Dragan Simic]

Hello Phillip,

On 2024-06-10 16:09, Phillip Wood wrote:
> On 09/06/2024 08:44, Rubén Justo wrote:
>> On Sat, Jun 08, 2024 at 07:54:34AM +0200, Dragan Simic wrote:
>> 
>>> When "|xyz" is used instead, the version of the hunk with no coloring
>>> escape sequences should be piped to xyz.
>> 
>> That is a sane and conservative approach, and I'm not opposed.  
>> However,
>> giving the colorful version though a custom pager is a good thing to
>> have, I think, i.e: allowing a simple "head" without losing the
>> coloring.
>> 
>> Let's recap a bit.
>> 
>> Initially, this series aimed to enable sending chunks to the pager
>> during "add -p" sessions.
>> 
>> To reduce the blast radius of spawning a pager for each chunk, we
>> introduced a new command "P".
>> 
>> Junio suggested opening up the command to allow specifying a custom
>> pager, in the form of "P<program>".
>> 
>> The "P" command started to resemble a lot to the common pipe operator.
>> Thus, we shifted to "|<program>".
>> 
>> Some concerns were raised about controlling when to send coloring 
>> escape
>> sequences.
> 
> I'm still not really convinced that the escape sequences are a
> problem. As Peff has pointed out [1] this new command exists primarily
> to display the current hunk. I've asked for concrete examples of
> programs that it would be useful to run from "git add -p" where the
> escape codes are a problem [2,3]. Sadly the replies talked in generic
> terms about an imaginary program without any reference to displaying
> or processing a hunk from "git add -p". Without a clear use case for
> stripping the escape codes I think we should add a single command that
> pipes the colored output to a user specified program. We can make it
> clear in the documentation that the input to the user's command will
> contain escape sequences unless they pass "-c color.diff=false" when
> starting "git add -p". If it becomes clear that there is a use for the
> plain output we can add that at a later stage.

Regarding examples, just have a look at less(1), [4] which is probably
the most commonly used pager.  It requires "-R" to be specified as one
of its command-line parameters for the coloring escape sequences to be
processed properly.  The presence of "-R" is taken as granted these 
days,
but it hasn't always been the case.

If you want to see an example of the garbled output, as a result of the
coloring escape sequences not being processed correctly, just "cd" into
your favorite Git repository and run the following commands (assuming
you're using bash, if not, please adjust the first command):

   export GIT_PAGER='less'
   git log --patch

I hope that will make more clear why I'm "advocating" for three separate
new commands for "git add -p", i.e. "P", "|xyz" and ">xyz".

> [1] 
> https://lore.kernel.org/git/20240605090935.GF2345232@coredump.intra.peff.net
> [2] 
> https://lore.kernel.org/git/a2a59f5e-fd55-41d3-8472-b99256e1f428@gmail.com>
> [3] 
> https://lore.kernel.org/git/1ae0715d-df76-4019-995e-f00f3506f2ac@gmail.com
[4] https://man.archlinux.org/man/less.1.en

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> If you want to see an example of the garbled output, as a result of the
> coloring escape sequences not being processed correctly, just "cd" into
> your favorite Git repository and run the following commands (assuming
> you're using bash, if not, please adjust the first command):
>
>   export GIT_PAGER='less'
>   git log --patch
>
> I hope that will make more clear why I'm "advocating" for three separate
> new commands for "git add -p", i.e. "P", "|xyz" and ">xyz".

We are talking about folks who use a pager in the context of Git,
and to them, your example is totally made-up and irrelevant one.

Either they use a color-enabled pager (like "less -R") or they have
configured color.ui to disable coloring.  So I do not see this quite
as a safety issue.

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

[Dragan Simic]

On 2024-06-10 22:08, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> If you want to see an example of the garbled output, as a result of 
>> the
>> coloring escape sequences not being processed correctly, just "cd" 
>> into
>> your favorite Git repository and run the following commands (assuming
>> you're using bash, if not, please adjust the first command):
>> 
>>   export GIT_PAGER='less'
>>   git log --patch
>> 
>> I hope that will make more clear why I'm "advocating" for three 
>> separate
>> new commands for "git add -p", i.e. "P", "|xyz" and ">xyz".
> 
> We are talking about folks who use a pager in the context of Git,
> and to them, your example is totally made-up and irrelevant one.

Ah, that wasn't my intention.  I just wanted to provide an example
that shows what a garbled output looks like, because I was under
impression that Phillip asked for such an example.

> Either they use a color-enabled pager (like "less -R") or they have
> configured color.ui to disable coloring.  So I do not see this quite
> as a safety issue.

Agreed, but only if we end up honoring "color.ui" and "color.diff"
in the new "git add -p" command(s).

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

[phillip.wood123]

Hi Dragan

On 08/06/2024 06:54, Dragan Simic wrote:
> On 2024-06-05 15:21, Phillip Wood wrote:
>> On 05/06/2024 10:09, Jeff King wrote:
>>> On Tue, Jun 04, 2024 at 08:32:04AM -0700, Junio C Hamano wrote:
>>>
>>> Given that the main use case for "|" is for human viewing through a
>>> pager, I think the colorful, filtered version meant for users is the
>>> best default. And then the "bare" version can come from an alternate
>>> command or a knob.
>>
>> I think that's a very good point. It is hard to see what "|" can be
>> used for other than viewing the hunk as (a) git does not read the
>> output so it cannot be used to filter or edit the hunk that is applied
>> and (b) we pass an isolated hunk so the post-image offset in the hunk
>> header is likely to be wrong and there is no indication as to which
>> file it comes from so the program being run cannot apply the hunk
>> itself. Having the escape codes does make it harder to filter the
>> hunk. For example to just look at the post-image as one needs to do
>> something like
>>
>>     grep '^[^-+ @]*[+ @]'
>>
>> instead of just using '^[+ @]' as the pattern but the bonus is that
>> the output is colored.
> 
> Agreed, but as I already explained, [1] only when using the bare "|"
> command.  When "|xyz" is used instead, the version of the hunk with
> no coloring escape sequences should be piped to xyz.

Having read the message you referenced I'm struggling to understand the 
use-case for stripping escape codes - what do you want to do with the 
hunk that means you want to remove the color?

Best Wishes

Phillip

> 
> [1] 
> https://lore.kernel.org/git/844704794168f9fcb85c75014c84cde0@manjaro.org/

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

[Dragan Simic]

Hello Phillip,

On 2024-06-09 16:29, phillip.wood123@gmail.com wrote:
> On 08/06/2024 06:54, Dragan Simic wrote:
>> On 2024-06-05 15:21, Phillip Wood wrote:
>>> On 05/06/2024 10:09, Jeff King wrote:
>>>> On Tue, Jun 04, 2024 at 08:32:04AM -0700, Junio C Hamano wrote:
>>>> 
>>>> Given that the main use case for "|" is for human viewing through a
>>>> pager, I think the colorful, filtered version meant for users is the
>>>> best default. And then the "bare" version can come from an alternate
>>>> command or a knob.
>>> 
>>> I think that's a very good point. It is hard to see what "|" can be
>>> used for other than viewing the hunk as (a) git does not read the
>>> output so it cannot be used to filter or edit the hunk that is 
>>> applied
>>> and (b) we pass an isolated hunk so the post-image offset in the hunk
>>> header is likely to be wrong and there is no indication as to which
>>> file it comes from so the program being run cannot apply the hunk
>>> itself. Having the escape codes does make it harder to filter the
>>> hunk. For example to just look at the post-image as one needs to do
>>> something like
>>> 
>>>     grep '^[^-+ @]*[+ @]'
>>> 
>>> instead of just using '^[+ @]' as the pattern but the bonus is that
>>> the output is colored.
>> 
>> Agreed, but as I already explained, [1] only when using the bare "|"
>> command.  When "|xyz" is used instead, the version of the hunk with
>> no coloring escape sequences should be piped to xyz.
> 
> Having read the message you referenced I'm struggling to understand
> the use-case for stripping escape codes - what do you want to do with
> the hunk that means you want to remove the color?

Let me recap, please.  Basically, when an output of some command is
piped into another command, e.g. by running "grep -r abc . | grep def",
the command that produces the piped output doesn't put the coloring
escape codes into the produced output, because it's unknown can the
command that receives it handle those escape codes properly.  That's
become some kind of de facto standard embodied into the "--color=auto"
command-line option for various utilities.

In the example above, one can have "grep -n --color=auto" defined as
their alias for "grep", which is what I use, and the "grep -r abc"
produces the output with no coloring escape sequences, which gets
piped into "grep def" that does produce coloring escape codes, because
its output goes to the terminal emulator, which is expected to handle
those escape codes properly.

In our use case, Git becomes what produces a hunk as the output that
gets piped into some program "xyz" by receiving "|xyz" as the command
while "git add -p" is executed, but it isn't known can "xyz" handle
the coloring escape sequences properly, so they should not be included
in the piped output, i.e. in the produced hunk.

As discussed later, [2] we could introduce ">xyz" as another command
for "git add -p", so receiving ">xyz" from the user would pipe the
hunk to "xyz" with the coloring escape sequences included, because
the user already knows that "xyz" can handle them.  Separating this
into two commands is a safe approach.

I hope this makes the whole thing more clear.

[1] 
https://lore.kernel.org/git/844704794168f9fcb85c75014c84cde0@manjaro.org/
[2] 
https://lore.kernel.org/git/7937845d7cb7ae0179c4922ed154c5c7@manjaro.org/

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

[Phillip Wood]

Hi Dragan

On 09/06/2024 18:20, Dragan Simic wrote:
> On 2024-06-09 16:29, phillip.wood123@gmail.com wrote:
>
>> Having read the message you referenced I'm struggling to understand
>> the use-case for stripping escape codes - what do you want to do with
>> the hunk that means you want to remove the color?
> 
> Let me recap, please.
> [...]
> I hope this makes the whole thing more clear.

It is very clear _how_ you think it should work and I agree that makes 
sense in the context of a generic shell pipeline. What's not clear to me 
is _why_ that is useful in the context of displaying hunks in "git add 
-p". The purpose of "git add -p" is to allow the user to interactively 
stage individual hunks. The "|" command allows the user to display the 
hunk in a way that helps them decide whether to stage that particular 
hunk. Are you able to give a specific example of a command that would 
help you decide whether to stage a particular hunk where you would not 
want to keep the escape codes?

Best Wishes

Phillip

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

[Dragan Simic]

Hello Phillip,

On 2024-06-10 10:27, Phillip Wood wrote:
> On 09/06/2024 18:20, Dragan Simic wrote:
>> On 2024-06-09 16:29, phillip.wood123@gmail.com wrote:
>> 
>>> Having read the message you referenced I'm struggling to understand
>>> the use-case for stripping escape codes - what do you want to do with
>>> the hunk that means you want to remove the color?
>> 
>> Let me recap, please.
>> [...]
>> I hope this makes the whole thing more clear.
> 
> It is very clear _how_ you think it should work and I agree that makes
> sense in the context of a generic shell pipeline. What's not clear to
> me is _why_ that is useful in the context of displaying hunks in "git
> add -p". The purpose of "git add -p" is to allow the user to
> interactively stage individual hunks. The "|" command allows the user
> to display the hunk in a way that helps them decide whether to stage
> that particular hunk. Are you able to give a specific example of a
> command that would help you decide whether to stage a particular hunk
> where you would not want to keep the escape codes?

Well, it isn't about not _wanting_ to keep the coloring escape
sequences, but about the _need_ to play it safe, so to speak.
In other words, we can't know that a random "xyz", as in "|xyz",
can actually handle the coloring escape sequences, so we need
to be on the safe side and protect the users from being hit by
garbled outputs.

That's why it would be the best to have "P", "|xyz" and ">xyz"
as the different "git add -p" options (piped to the configured
pager with preserved escape sequences, piped to "xyz" with
stripped escape sequences, and piped to "xyz" with preserved
escape sequences, respectively), as I proposed earlier.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Just to note some prior art, mutt's "<pipe-message>" faces a similar
> problem. You might want the raw message (if you're going to poke at
> headers, MIME parts, etc yourself) or you may want a decoded one (if you
> just care about body text and don't want to deal with base64, qp, etc,
> yourself). They provide a stateful config knob, but then you end up with
> horrible macros that toggle the knob, like:
>
>   :set pipe_decode=yes<enter><pipe-message>my-script<enter>:set pipe_decode=no<enter>
>
> I think having two separate commands for the two modes would be less
> confusing.

Yup, I have been suffering from its equivalent in Gnus/Emacs every
day, where decoded version of multipart/signed messages look
different enough from valid RFC2822 messages and break application
with "git am", and there of course is "feed the undecoded original
to pipe" option.

But the need for both is pretty much known, and the real issue is
that what the default should be.  The default destination of the
pipe being a pager, making the colored output the default is
consistent with that, I would think.

Thanks.