patch, 2 partsimprove interactive-patch

> Shortly we're going make interactive-patch stop printing automatically
> the hunk under certain circumstances.
>
> Let's introduce a new option to allow the user to explicitly request
> the printing.

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  add-patch.c                |  4 ++++
>  t/t3701-add-interactive.sh | 22 +++++++++++-----------
>  2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 68f525b35c..52be1ddb15 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1388,6 +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 again the current hunk\n"
>     "? - print help\n");
>  
>  static int patch_update_file(struct add_p_state *s,
> @@ -1480,6 +1481,7 @@ static int patch_update_file(struct add_p_state *s,
>  				permitted |= ALLOW_EDIT;
>  				strbuf_addstr(&s->buf, ",e");
>  			}
> +			strbuf_addstr(&s->buf, ",p");
>  		}
>  		if (file_diff->deleted)
>  			prompt_mode_type = PROMPT_DELETION;
> @@ -1658,6 +1660,8 @@ static int patch_update_file(struct add_p_state *s,
>  				hunk->use = USE_HUNK;
>  				goto soft_increment;
>  			}
> +		} else if (s->answer.buf[0] == 'p') {
> +			/* nothing to do */

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 0b5339ac6c..bc55255b0a 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -325,9 +325,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,?]?
> -	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,?]?
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]?
> +	(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
>  '
> @@ -514,13 +514,13 @@ test_expect_success 'split hunk setup' '
>  test_expect_success 'goto hunk' '
>  	test_when_finished "git reset" &&
>  	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? + 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,?]?_
> +	(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 &&
> @@ -530,11 +530,11 @@ test_expect_success 'goto hunk' '
>  test_expect_success 'navigate to hunk via regex' '
>  	test_when_finished "git reset" &&
>  	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -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,?]?_
> +	(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 &&
> @@ -715,21 +715,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,?]? <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,?]? <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,?]? <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,?]? <RESET>
> +	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
>  	EOF
>  	test_cmp expect actual
>  '

> Rubén Justo <rjusto@gmail.com> writes:
> 
> > Shortly we're going make interactive-patch stop printing automatically
> > the hunk under certain circumstances.
> >
> > Let's introduce a new option to allow the user to explicitly request
> > the printing.
> 
> That is good, but ...
> 
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> >  add-patch.c                |  4 ++++
> >  t/t3701-add-interactive.sh | 22 +++++++++++-----------
> >  2 files changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/add-patch.c b/add-patch.c
> > index 68f525b35c..52be1ddb15 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -1388,6 +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 again the current hunk\n"
> >     "? - print help\n");
> >  
> >  static int patch_update_file(struct add_p_state *s,
> > @@ -1480,6 +1481,7 @@ static int patch_update_file(struct add_p_state *s,
> >  				permitted |= ALLOW_EDIT;
> >  				strbuf_addstr(&s->buf, ",e");
> >  			}
> > +			strbuf_addstr(&s->buf, ",p");
> >  		}
> >  		if (file_diff->deleted)
> >  			prompt_mode_type = PROMPT_DELETION;
> > @@ -1658,6 +1660,8 @@ static int patch_update_file(struct add_p_state *s,
> >  				hunk->use = USE_HUNK;
> >  				goto soft_increment;
> >  			}
> > +		} else if (s->answer.buf[0] == 'p') {
> > +			/* nothing to do */
> 
> This is not good.  If we are taking a new input, why doesn't the
> code already respond to it?  "Showing it again" should be a separate
> feature even if some other codepaths still do show when [2/2] would
> prevent them to show, no?

> Also, in addition to the changes to the patch to unbreak it, we'd
> need to update the git-add(1) manual page, especially its section on
> the interactive mode.  I think a single-line addition should suffice,
> but it has to be there, added by the same patch as the one that
> starts accepting 'p' and acting on that input.

> ... or an invalid option, i.e: "U"
> ...
>     (1/4) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? U
> ...
>     ? - print help
>     @@ -1394,7 +1394,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
>      static int patch_update_file(struct add_p_state *s,
>      			     struct file_diff *file_diff)
>      {
>     -	size_t hunk_index = 0;
>     +	size_t hunk_index = 0, prev_hunk_index = -1;
>      	ssize_t i, undecided_previous, undecided_next;
>      	struct hunk *hunk;
>      	char ch;
>     (1/4) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?
>
> Printing the chunk again followed by the question can be confusing as
> the user has to pay special attention to notice that the same chunk is
> being reconsidered.
>
> It can also be problematic if the chunk is longer than one screen height
> because the result of the previous iteration is lost off the screen (the
> help guide in the previous example).

> Shortly we're going make interactive-patch stop printing automatically
> the hunk under certain circumstances.
> 
> Let's introduce a new option to allow the user to explicitly request
> the printing.

> diff --git a/add-patch.c b/add-patch.c
> index 68f525b35c..444fd75b2a 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1388,6 +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 again the current hunk\n"

> > Let's introduce a new option to allow the user to explicitly request
> > the printing.
> 
> I wonder if we want to hide this option unless we've skipped rendering the
> hunk in the same way that we hide other options that are not relevant to the
> hunk being displayed.

> 
> > diff --git a/add-patch.c b/add-patch.c
> > index 68f525b35c..444fd75b2a 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -1388,6 +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 again the current hunk\n"
> 
> I think "print the hunk again" is clearer

> On Tue, Mar 26, 2024 at 02:38:02PM +0000, Phillip Wood wrote:
> 
>>> Let's introduce a new option to allow the user to explicitly request
>>> the printing.
>>
>> I wonder if we want to hide this option unless we've skipped rendering the
>> hunk in the same way that we hide other options that are not relevant to the
>> hunk being displayed.
> 
> You've got me scratching my head.  Do you see any cases where we
> shouldn't offer the new option?

>> I also wonder if 'r' for "re-display" would better
>> convey the intent of this keybinding.
> 
> I'm more inclined towards the 'p' because the verb is 'print'.  Does
> this reasoning make sense to you?

>>
>>> diff --git a/add-patch.c b/add-patch.c
>>> index 68f525b35c..444fd75b2a 100644
>>> --- a/add-patch.c
>>> +++ b/add-patch.c
>>> @@ -1388,6 +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 again the current hunk\n"
>>
>> I think "print the hunk again" is clearer
> 
> The word 'current' is in my proposal because IMHO it adds value making
> explicit what we're offering.  Maybe "print the current hunk again"?
> What do you think?

>      $ git add -p
>      diff --git a/add-patch.c b/add-patch.c
>      index 52be1ddb15..8fb75e82e2 100644
>      --- a/add-patch.c
>      +++ b/add-patch.c
>      @@ -1394,7 +1394,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
>       static int patch_update_file(struct add_p_state *s,
>       			     struct file_diff *file_diff)
>       {
>      -	size_t hunk_index = 0;
>      +	size_t hunk_index = 0, prev_hunk_index = -1;
>       	ssize_t i, undecided_previous, undecided_next;
>       	struct hunk *hunk;
>       	char ch;
>      (1/4) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? U
>      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
>      e - manually edit the current hunk
>      p - print again the current hunk
>      ? - print help
>      @@ -1394,7 +1394,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
>       static int patch_update_file(struct add_p_state *s,
>       			     struct file_diff *file_diff)
>       {
>      -	size_t hunk_index = 0;
>      +	size_t hunk_index = 0, prev_hunk_index = -1;
>       	ssize_t i, undecided_previous, undecided_next;
>       	struct hunk *hunk;
>       	char ch;
>      (1/4) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?
> 
> Printing the chunk again followed by the question can be confusing as
> the user has to pay special attention to notice that the same chunk is
> being reconsidered.

> It can also be problematic if the chunk is longer than one screen height
> because the result of the previous iteration is lost off the screen (the
> help guide in the previous example).
> 
> To avoid such problems, stop printing the chunk if the iteration does
> not advance to a different chunk.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   add-patch.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 444fd75b2a..54a7d9c01f 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1394,7 +1394,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
>   static int patch_update_file(struct add_p_state *s,
>   			     struct file_diff *file_diff)
>   {
> -	size_t hunk_index = 0;
> +	size_t hunk_index = 0, prev_hunk_index = -1;

>   	ssize_t i, undecided_previous, undecided_next;
>   	struct hunk *hunk;
>   	char ch;
> @@ -1448,10 +1448,14 @@ static int patch_update_file(struct add_p_state *s,
>   
>   		strbuf_reset(&s->buf);
>   		if (file_diff->hunk_nr) {
> -			render_hunk(s, hunk, 0, colored, &s->buf);
> -			fputs(s->buf.buf, stdout);
> +			if (prev_hunk_index != hunk_index) {
> +				render_hunk(s, hunk, 0, colored, &s->buf);
> +				fputs(s->buf.buf, stdout);
> +				strbuf_reset(&s->buf);
> +
> +				prev_hunk_index = hunk_index;
> +			}
>   
> -			strbuf_reset(&s->buf);

>   			if (undecided_previous >= 0) {
>   				permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
>   				strbuf_addstr(&s->buf, ",k");
> @@ -1649,10 +1653,12 @@ static int patch_update_file(struct add_p_state *s,
>   			if (!(permitted & ALLOW_SPLIT))

> >      $ git add -p
> >      diff --git a/add-patch.c b/add-patch.c
> >      index 52be1ddb15..8fb75e82e2 100644
> >      --- a/add-patch.c
> >      +++ b/add-patch.c
> >      @@ -1394,7 +1394,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
> >       static int patch_update_file(struct add_p_state *s,
> >       			     struct file_diff *file_diff)
> >       {
> >      -	size_t hunk_index = 0;
> >      +	size_t hunk_index = 0, prev_hunk_index = -1;
> >       	ssize_t i, undecided_previous, undecided_next;
> >       	struct hunk *hunk;
> >       	char ch;
> >      (1/4) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? U
> >      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
> >      e - manually edit the current hunk
> >      p - print again the current hunk
> >      ? - print help
> >      @@ -1394,7 +1394,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
> >       static int patch_update_file(struct add_p_state *s,
> >       			     struct file_diff *file_diff)
> >       {
> >      -	size_t hunk_index = 0;
> >      +	size_t hunk_index = 0, prev_hunk_index = -1;
> >       	ssize_t i, undecided_previous, undecided_next;
> >       	struct hunk *hunk;
> >       	char ch;
> >      (1/4) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?
> > 
> > Printing the chunk again followed by the question can be confusing as
> > the user has to pay special attention to notice that the same chunk is
> > being reconsidered.
> 
> As we print a long help message if we don't re-display the hunk it ends up
> being separated from the prompt. Personally I find the help message quite
> annoying when I've fat-fingered the wrong key - I'd prefer a shorter message
> pointing to "?" to display more help. We already do something similar if the
> user presses a key such as "s" that is disabled for the current hunk.

> > -	size_t hunk_index = 0;
> > +	size_t hunk_index = 0, prev_hunk_index = -1;
> 
> I found the name a bit confusing as we have keys for displaying the previous
> hunk and it make me think of that. As it is used to record the index of the
> hunk that we've rendered perhaps "rendered_hunk_index" would be a better
> name.

> 
> >   	ssize_t i, undecided_previous, undecided_next;
> >   	struct hunk *hunk;
> >   	char ch;
> > @@ -1448,10 +1448,14 @@ static int patch_update_file(struct add_p_state *s,
> >   		strbuf_reset(&s->buf);
> >   		if (file_diff->hunk_nr) {
> > -			render_hunk(s, hunk, 0, colored, &s->buf);
> > -			fputs(s->buf.buf, stdout);
> > +			if (prev_hunk_index != hunk_index) {
> > +				render_hunk(s, hunk, 0, colored, &s->buf);
> > +				fputs(s->buf.buf, stdout);
> > +				strbuf_reset(&s->buf);
> > +
> > +				prev_hunk_index = hunk_index;
> > +			}
> > -			strbuf_reset(&s->buf);
> 
> I'd be inclined to leave this line as is to make it clear that the strbuf is
> always cleared before adding the keybindings.

> 
> >   			if (undecided_previous >= 0) {
> >   				permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
> >   				strbuf_addstr(&s->buf, ",k");
> > @@ -1649,10 +1653,12 @@ static int patch_update_file(struct add_p_state *s,
> >   			if (!(permitted & ALLOW_SPLIT))
> 
> style: as you're adding braces to the other clause in this if statement you
> should add them to this clause as well.

> On Tue, Mar 26, 2024 at 02:39:18PM +0000, Phillip Wood wrote:
>>> Printing the chunk again followed by the question can be confusing as
>>> the user has to pay special attention to notice that the same chunk is
>>> being reconsidered.
>>
>> As we print a long help message if we don't re-display the hunk it ends up
>> being separated from the prompt. Personally I find the help message quite
>> annoying when I've fat-fingered the wrong key - I'd prefer a shorter message
>> pointing to "?" to display more help. We already do something similar if the
>> user presses a key such as "s" that is disabled for the current hunk.
> 
> Yeah, I would like that too.  Maybe something like:
> 
>       $ git add -p
>       diff --git a/add-patch.c b/add-patch.c
>       index 52be1ddb15..8fb75e82e2 100644
>       --- a/add-patch.c
>       +++ b/add-patch.c
>       @@ -1394,7 +1394,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
>        static int patch_update_file(struct add_p_state *s,
>        			     struct file_diff *file_diff)
>        {
>       -	size_t hunk_index = 0;
>       +	size_t hunk_index = 0, prev_hunk_index = -1;
>        	ssize_t i, undecided_previous, undecided_next;
>        	struct hunk *hunk;
>        	char ch;
>       (1/4) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? U
>       Unknown option "U".  Use '?' for help.

>>> -	size_t hunk_index = 0;
>>> +	size_t hunk_index = 0, prev_hunk_index = -1;
>>
>> I found the name a bit confusing as we have keys for displaying the previous
>> hunk and it make me think of that. As it is used to record the index of the
>> hunk that we've rendered perhaps "rendered_hunk_index" would be a better
>> name.
> 
> OK.
> 
>> Also as it needs to hold a negative value we should declare it as
>> ssize_t like the variables on the line below.
> 
> Very good point.  OK.
> 
>>
>>>    	ssize_t i, undecided_previous, undecided_next;
>>>    	struct hunk *hunk;
>>>    	char ch;
>>> @@ -1448,10 +1448,14 @@ static int patch_update_file(struct add_p_state *s,
>>>    		strbuf_reset(&s->buf);
>>>    		if (file_diff->hunk_nr) {
>>> -			render_hunk(s, hunk, 0, colored, &s->buf);
>>> -			fputs(s->buf.buf, stdout);
>>> +			if (prev_hunk_index != hunk_index) {
>>> +				render_hunk(s, hunk, 0, colored, &s->buf);
>>> +				fputs(s->buf.buf, stdout);
>>> +				strbuf_reset(&s->buf);
>>> +
>>> +				prev_hunk_index = hunk_index;
>>> +			}
>>> -			strbuf_reset(&s->buf);
>>
>> I'd be inclined to leave this line as is to make it clear that the strbuf is
>> always cleared before adding the keybindings.
> 
> If find having two strbuf_reset()'s in a row confusing.  Maybe it is
> just me not seeing that that second strbuf_reset is "close" to noop.

> >       (1/4) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? U
> >       Unknown option "U".  Use '?' for help.
> 
> Yes, I like that (though I'd use the same quotes for both parts of the
> message)

> > If find having two strbuf_reset()'s in a row confusing.  Maybe it is
> > just me not seeing that that second strbuf_reset is "close" to noop.
> 
> If we don't print the hunk then the second call to strbuf_reset is indeed a
> noop. In our code base it is common to see a call to strbuf_reset()
> immediately before adding new content to the buffer, rather than cleaning up
> ready for reuse after the buffer has been used. If you grep 'strbuf_reset'
> in this file you'll see all the calls come immediately before adding new
> content to the buffer. By moving the call inside the conditional we're
> moving from a pattern of cleaning up before adding new content to a pattern
> of cleaning up afterwards which I think is harder to follow given the way
> the rest of the code uses strbuf_reset()

> Rubén Justo (2):
>    add-patch: introduce 'p' in interactive-patch
>    add-patch: do not print hunks repeatedly
> 
>   Documentation/git-add.txt  |  1 +
>   add-patch.c                | 20 +++++++++++++++-----
>   t/t3701-add-interactive.sh | 22 +++++++++++-----------
>   3 files changed, 27 insertions(+), 16 deletions(-)
> 
> Range-diff against v1:
> 1:  48a2c63b78 ! 1:  5e319f439d add-patch: introduce 'p' in interactive-patch
>      @@ Commit message
>       
>           Signed-off-by: Rubén Justo <rjusto@gmail.com>
>       
>      + ## Documentation/git-add.txt ##
>      +@@ Documentation/git-add.txt: patch::
>      +        K - leave this hunk undecided, see previous hunk
>      +        s - split the current hunk into smaller hunks
>      +        e - manually edit the current hunk
>      ++       p - print again the current hunk
>      +        ? - print help
>      + +
>      + After deciding the fate for all hunks, if there is any hunk
>      +
>        ## add-patch.c ##
>       @@ add-patch.c: N_("j - leave this hunk undecided, see next undecided hunk\n"
>           "/ - search for a hunk matching the given regex\n"
>      @@ add-patch.c: static int patch_update_file(struct add_p_state *s,
>        				goto soft_increment;
>        			}
>       +		} else if (s->answer.buf[0] == 'p') {
>      -+			/* nothing to do */
>      ++			/* nothing special is needed */
>        		} else {
>        			const char *p = _(help_patch_remainder), *eol = p;
>        
> 2:  1730493096 ! 2:  1177bfeae4 add-patch: do not print hunks repeatedly
>      @@ Commit message
>               g - select a hunk to go to
>               / - search for a hunk matching the given regex
>               e - manually edit the current hunk
>      +        p - print again the current hunk
>               ? - print help
>               @@ -1394,7 +1394,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
>                static int patch_update_file(struct add_p_state *s,
>      @@ add-patch.c: static int patch_update_file(struct add_p_state *s,
>        				goto soft_increment;
>        			}
>        		} else if (s->answer.buf[0] == 'p') {
>      --			/* nothing to do */
>      +-			/* nothing special is needed */
>       +			prev_hunk_index = -1;
>        		} else {
>        			const char *p = _(help_patch_remainder), *eol = p;

> Hi Rubén
>
> On 26/03/2024 00:15, Rubén Justo wrote:
>> Let's reduce the verbosity in the interactive-patch process, in order to
>> make it less confusing.
>
> I think this is a good idea, I've left a few comments on the patches.

> On Tue, Mar 26, 2024 at 08:31:41AM -0700, Junio C Hamano wrote:
>
>> 'r'edisplay may work well (and I wonder "r | less" or
>> piping the hunk display to anything in general would be a useful
>> future enhancement).
>
> Yeah.  That's a very good enhancement.
>
> However, I'm not sure how.  Perhaps with the help of git-diff-files(1)?

> > On Tue, Mar 26, 2024 at 08:31:41AM -0700, Junio C Hamano wrote:
> >
> >> 'r'edisplay may work well (and I wonder "r | less" or
> >> piping the hunk display to anything in general would be a useful
> >> future enhancement).
> >
> 
> It would be more like tweaking fputs() of a strbuf that
> was filled by render_hunk() to instead spawn a pager and feed the
> same strbuf to it, or something.  IOW, we already have the payload
> to show.  We just want a pager involved in its showing so that users
> with a huge hunk that does not fit on a page can use "less" on it.

> On Tue, Mar 26, 2024 at 12:13:46PM -0700, Junio C Hamano wrote:
>> > On Tue, Mar 26, 2024 at 08:31:41AM -0700, Junio C Hamano wrote:
>> >
>> >> 'r'edisplay may work well (and I wonder "r | less" or
>> >> piping the hunk display to anything in general would be a useful
>> >> future enhancement).
>> >
>> 
>> It would be more like tweaking fputs() of a strbuf that
>> was filled by render_hunk() to instead spawn a pager and feed the
>> same strbuf to it, or something.  IOW, we already have the payload
>> to show.  We just want a pager involved in its showing so that users
>> with a huge hunk that does not fit on a page can use "less" on it.
> 
> I do not plan to address this in this series, but while the topic is
> warm;  Perhaps?:

> --- >8 ---
> diff --git a/add-patch.c b/add-patch.c
> index 778f168298..cb74fe84f5 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"
> @@ -1450,7 +1451,7 @@ static int patch_update_file(struct add_p_state 
> *s,
>  		if (file_diff->hunk_nr) {
>  			if (rendered_hunk_index != hunk_index) {
>  				render_hunk(s, hunk, 0, colored, &s->buf);
> -				fputs(s->buf.buf, stdout);
> +				fputs_to_pager(s->buf.buf);
>  				rendered_hunk_index = hunk_index;
>  			}
> 
> diff --git a/pager.c b/pager.c
> index b8822a9381..f00fc87a67 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -264,3 +264,30 @@ int check_pager_config(const char *cmd)
>  		pager_program = data.value;
>  	return data.want;
>  }
> +
> +void fputs_to_pager(const char* s)
> +{
> +	struct child_process process;
> +	FILE* pager_stdin;
> +	const char *pager = git_pager(isatty(1));
> +
> +	if (!pager) {
> +		fputs(s, stdout);
> +		return;
> +	}
> +
> +	child_process_init(&process);
> +
> +	prepare_pager_args(&pager_process, pager);
> +	pager_process.in = -1;
> +	strvec_push(&pager_process.env, "GIT_PAGER_IN_USE");
> +	if (start_command(&pager_process))
> +		return;
> +
> +	pager_stdin = fdopen(pager_process.in, "w");
> +	fputs(s, pager_stdin);
> +	fflush(pager_stdin);
> +
> +	close(pager_process.in);
> +	finish_command(&pager_process);
> +}
> diff --git a/pager.h b/pager.h
> index b77433026d..dcccfa632b 100644
> --- a/pager.h
> +++ b/pager.h
> @@ -11,6 +11,7 @@ void term_clear_line(void);
>  int decimal_width(uintmax_t);
>  int check_pager_config(const char *cmd);
>  void prepare_pager_args(struct child_process *, const char *pager);
> +void fputs_to_pager(const char* s);
> 
>  extern int pager_use_color;

> On 2024-03-29 20:26, Rubén Justo wrote:
> > On Tue, Mar 26, 2024 at 12:13:46PM -0700, Junio C Hamano wrote:
> > > > On Tue, Mar 26, 2024 at 08:31:41AM -0700, Junio C Hamano wrote:
> > > >
> > > >> 'r'edisplay may work well (and I wonder "r | less" or
> > > >> piping the hunk display to anything in general would be a useful
> > > >> future enhancement).
> > > >
> > > 
> > > It would be more like tweaking fputs() of a strbuf that
> > > was filled by render_hunk() to instead spawn a pager and feed the
> > > same strbuf to it, or something.  IOW, we already have the payload
> > > to show.  We just want a pager involved in its showing so that users
> > > with a huge hunk that does not fit on a page can use "less" on it.
> > 
> > I do not plan to address this in this series, but while the topic is
> > warm;  Perhaps?:
> 
> As a note, I find that having chunks displayed through the pager
> would be really nice.

> 
> > --- >8 ---
> > diff --git a/add-patch.c b/add-patch.c
> > index 778f168298..cb74fe84f5 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"
> > @@ -1450,7 +1451,7 @@ static int patch_update_file(struct add_p_state
> > *s,
> >  		if (file_diff->hunk_nr) {
> >  			if (rendered_hunk_index != hunk_index) {
> >  				render_hunk(s, hunk, 0, colored, &s->buf);
> > -				fputs(s->buf.buf, stdout);
> > +				fputs_to_pager(s->buf.buf);
> >  				rendered_hunk_index = hunk_index;
> >  			}
> > 
> > diff --git a/pager.c b/pager.c
> > index b8822a9381..f00fc87a67 100644
> > --- a/pager.c
> > +++ b/pager.c
> > @@ -264,3 +264,30 @@ int check_pager_config(const char *cmd)
> >  		pager_program = data.value;
> >  	return data.want;
> >  }
> > +
> > +void fputs_to_pager(const char* s)
> > +{
> > +	struct child_process process;
> > +	FILE* pager_stdin;
> > +	const char *pager = git_pager(isatty(1));
> > +
> > +	if (!pager) {
> > +		fputs(s, stdout);
> > +		return;
> > +	}
> > +
> > +	child_process_init(&process);
> > +
> > +	prepare_pager_args(&pager_process, pager);
> > +	pager_process.in = -1;
> > +	strvec_push(&pager_process.env, "GIT_PAGER_IN_USE");
> > +	if (start_command(&pager_process))
> > +		return;
> > +
> > +	pager_stdin = fdopen(pager_process.in, "w");
> > +	fputs(s, pager_stdin);
> > +	fflush(pager_stdin);
> > +
> > +	close(pager_process.in);
> > +	finish_command(&pager_process);
> > +}
> > diff --git a/pager.h b/pager.h
> > index b77433026d..dcccfa632b 100644
> > --- a/pager.h
> > +++ b/pager.h
> > @@ -11,6 +11,7 @@ void term_clear_line(void);
> >  int decimal_width(uintmax_t);
> >  int check_pager_config(const char *cmd);
> >  void prepare_pager_args(struct child_process *, const char *pager);
> > +void fputs_to_pager(const char* s);
> > 
> >  extern int pager_use_color;

> I do not plan to address this in this series, but while the topic is
> warm;  Perhaps?:
>
> --- >8 ---
> @@ -1450,7 +1451,7 @@ static int patch_update_file(struct add_p_state *s,
>  		if (file_diff->hunk_nr) {
>  			if (rendered_hunk_index != hunk_index) {
>  				render_hunk(s, hunk, 0, colored, &s->buf);
> -				fputs(s->buf.buf, stdout);
> +				fputs_to_pager(s->buf.buf);
>  				rendered_hunk_index = hunk_index;
>  			}

> Phillip Wood <phillip.wood123@gmail.com> writes:
>> I think this is a good idea, I've left a few comments on the patches.
> 
> Thanks for your reviews.  You raised very good points, all of which
> I agree with.  'r'edisplay may work well (and I wonder "r | less" or
> piping the hunk display to anything in general would be a useful
> future enhancement).

>  Response to an unrecognised command should
> probably be a two-step process (a short "'h' is not understood. type
> ? for help" with list of commands upon request).
> 
> I however am unsure about omitting 'p' from the list when we did not
> skip.  We do omit 'k' when we have NO previous hunk to go back to,
> but that is because we cannot do it even if we were asked to and the
> only possible outcome is an error message.  That is quite different
> from 'p' where we can always show the current hunk when asked, even
> if it is just after we have shown it already.

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> ... My
>> reasoning was that 'p' does not do anything useful for the user, if
>> they press it they end up with exactly the same content being printed
>> to the screen ...
> 
> Actually I do not agree that it necessarily is useless that the same
> content is shown.  Especially since we do not page, it is plausible
> for a user, who saw a huge hunk, to want to tweak terminal setting
> and raise scrollbuffer size (which may be set to 0 for usual
> sessions, like me), and say "please show it again".  Or even in a
> more primitive environment, just say "please show it again" and
> immediately type \C-s to stop while the early part of the hunk is
> shown ;-).

> Thinking about the name of the option again, if we are omitting to
> show a hunk in some situations, the request to ask for the current
> hunk to be shown is "please show me", not "please reshow me", so the
> verb 'print' may apply to wider situations than the verb 'reprint',
> strictly speaking.

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > ... My
> > reasoning was that 'p' does not do anything useful for the user, if
> > they press it they end up with exactly the same content being printed
> > to the screen ...
> 
> Actually I do not agree that it necessarily is useless that the same
> content is shown.  Especially since we do not page, it is plausible
> for a user, who saw a huge hunk, to want to tweak terminal setting
> and raise scrollbuffer size (which may be set to 0 for usual
> sessions, like me), and say "please show it again".  Or even in a
> more primitive environment, just say "please show it again" and
> immediately type \C-s to stop while the early part of the hunk is
> shown ;-).
> 
> Thinking about the name of the option again, if we are omitting to
> show a hunk in some situations, the request to ask for the current
> hunk to be shown is "please show me", not "please reshow me", so the
> verb 'print' may apply to wider situations than the verb 'reprint',
> strictly speaking.

> Hi Rubén
> 
> On 26/03/2024 00:15, Rubén Justo wrote:
> > Let's reduce the verbosity in the interactive-patch process, in order to
> > make it less confusing.
> 
> I think this is a good idea, I've left a few comments on the patches.

> Shortly we're going make interactive-patch stop printing automatically
> the hunk under certain circumstances.
>
> Let's introduce a new option to allow the user to explicitly request
> the printing.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  Documentation/git-add.txt  |  1 +
>  add-patch.c                |  4 ++++
>  t/t3701-add-interactive.sh | 22 +++++++++++-----------
>  3 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 14a371fff3..2965cd0fb6 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -348,6 +348,7 @@ patch::
>         K - leave this hunk undecided, see previous hunk
>         s - split the current hunk into smaller hunks
>         e - manually edit the current hunk
> +       p - print the current hunk again

> @@ -1448,10 +1448,15 @@ static int patch_update_file(struct add_p_state *s,
>  
>  		strbuf_reset(&s->buf);
>  		if (file_diff->hunk_nr) {
> -			render_hunk(s, hunk, 0, colored, &s->buf);
> -			fputs(s->buf.buf, stdout);
> +			if (rendered_hunk_index != hunk_index) {

> +				render_hunk(s, hunk, 0, colored, &s->buf);
> +				fputs(s->buf.buf, stdout);
> +
> +				rendered_hunk_index = hunk_index;
> +			}
>  
>  			strbuf_reset(&s->buf);
> +
>  			if (undecided_previous >= 0) {
>  				permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
>  				strbuf_addstr(&s->buf, ",k");
> @@ -1649,10 +1654,12 @@ static int patch_update_file(struct add_p_state *s,
>  			if (!(permitted & ALLOW_SPLIT))
>  				err(s, _("Sorry, cannot split this hunk"));
>  			else if (!split_hunk(s, file_diff,
> -					     hunk - file_diff->hunk))
> +					     hunk - file_diff->hunk)) {
>  				color_fprintf_ln(stdout, s->s.header_color,
>  						 _("Split into %d hunks."),
>  						 (int)splittable_into);
> +				rendered_hunk_index = -1;
> +			}

>  		} else if (s->answer.buf[0] == 'e') {
>  			if (!(permitted & ALLOW_EDIT))
>  				err(s, _("Sorry, cannot edit this hunk"));
> @@ -1661,7 +1668,7 @@ static int patch_update_file(struct add_p_state *s,
>  				goto soft_increment;
>  			}
>  		} else if (s->answer.buf[0] == 'p') {
> -			/* nothing special is needed */
> +			rendered_hunk_index = -1;

> @@ -1448,10 +1448,15 @@ static int patch_update_file(struct add_p_state *s,
>   
>   		strbuf_reset(&s->buf);
>   		if (file_diff->hunk_nr) {
> -			render_hunk(s, hunk, 0, colored, &s->buf);
> -			fputs(s->buf.buf, stdout);
> +			if (rendered_hunk_index != hunk_index) {
> +				render_hunk(s, hunk, 0, colored, &s->buf);
> +				fputs(s->buf.buf, stdout);
> +
> +				rendered_hunk_index = hunk_index;

>   			if (undecided_previous >= 0) {
>   				permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
>   				strbuf_addstr(&s->buf, ",k");
> @@ -1646,13 +1651,15 @@ static int patch_update_file(struct add_p_state *s,
>   			hunk_index = i;
>   		} else if (s->answer.buf[0] == 's') {
>   			size_t splittable_into = hunk->splittable_into;
> -			if (!(permitted & ALLOW_SPLIT))
> +			if (!(permitted & ALLOW_SPLIT)) {
>   				err(s, _("Sorry, cannot split this hunk"));
> -			else if (!split_hunk(s, file_diff,
> -					     hunk - file_diff->hunk))
> +			} else if (!split_hunk(s, file_diff,
> +					     hunk - file_diff->hunk)) {
>   				color_fprintf_ln(stdout, s->s.header_color,
>   						 _("Split into %d hunks."),
>   						 (int)splittable_into);
> +				rendered_hunk_index = -1;
> +			}
>   		} else if (s->answer.buf[0] == 'e') {
>   			if (!(permitted & ALLOW_EDIT))
>   				err(s, _("Sorry, cannot edit this hunk"));
> @@ -1661,7 +1668,7 @@ static int patch_update_file(struct add_p_state *s,
>   				goto soft_increment;
>   			}
>   		} else if (s->answer.buf[0] == 'p') {
> -			/* nothing special is needed */
> +			rendered_hunk_index = -1;
>   		} else {
>   			const char *p = _(help_patch_remainder), *eol = p;
>   

> Hi Rubén
> 
> On 29/03/2024 03:58, Rubén Justo wrote:
> 
> Thanks for re-rolling, this looks pretty good - I've left a couple of small
> comments.
> 
> > @@ -1448,10 +1448,15 @@ static int patch_update_file(struct add_p_state *s,
> >   		strbuf_reset(&s->buf);
> >   		if (file_diff->hunk_nr) {
> > -			render_hunk(s, hunk, 0, colored, &s->buf);
> > -			fputs(s->buf.buf, stdout);
> > +			if (rendered_hunk_index != hunk_index) {
> > +				render_hunk(s, hunk, 0, colored, &s->buf);
> > +				fputs(s->buf.buf, stdout);
> > +
> > +				rendered_hunk_index = hunk_index;
> 
> This line could be grouped with the rest of this block without the blank
> line if you wanted.
> 
> > +			}
> >   			strbuf_reset(&s->buf);
> > +
> 
> I'm not sure what this new blank line is for - previously it was clear that
> the call strbuf_reset() was grouped with the code that then reuses the
> buffer. The rest of the changes look fine