[PATCH 0/2] improve interactive-patch

[PATCH 0/2] improve interactive-patch

[Rubén Justo]

Let's reduce the verbosity in the interactive-patch process, in order
to make it less confusing.

Rubén Justo (2):
  add-patch: introduce 'p' in interactive-patch
  add-patch: do not print hunks repeatedly

 add-patch.c                | 20 +++++++++++++++-----
 t/t3701-add-interactive.sh | 22 +++++++++++-----------
 2 files changed, 26 insertions(+), 16 deletions(-)

-- 
2.44.0.494.gf640f9da83

[PATCH 1/2] add-patch: introduce 'p' in interactive-patch

[Rubén Justo]

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 */
 		} else {
 			const char *p = _(help_patch_remainder), *eol = p;
 
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
 '
-- 
2.44.0.494.gf640f9da83

Re: [PATCH 1/2] add-patch: introduce 'p' in interactive-patch

[Junio C Hamano]

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.

Thanks.

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

Re: [PATCH 1/2] add-patch: introduce 'p' in interactive-patch

[Rubén Justo]

On Mon, Mar 25, 2024 at 02:38:30PM -0700, Junio C Hamano wrote:
> 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?

Doing nothing here produces, in the current implementation, the intended
printing.  Maybe the message needs to state so?

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

Oh, of course.  I think this is the line needed:

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 14a371fff3..90b47927b2 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 again the current hunk
        ? - print help
 +
 After deciding the fate for all hunks, if there is any hunk

Re: [PATCH 1/2] add-patch: introduce 'p' in interactive-patch

[Junio C Hamano]

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

> Doing nothing here produces, in the current implementation, the intended
> printing.  Maybe the message needs to state so?

Yeah, "/* Nothing special is needed */", or something.

Thanks for a quick response.

[PATCH 2/2] add-patch: do not print hunks repeatedly

[Rubén Justo]

The interactive-patch is a sequential process where, on each step, we
print one hunk from a patch and then ask the user how to proceed.

There is a possibility of repeating a step, for example if the user
enters a non-applicable option, i.e: "s"

    $ 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,?]? s
    Sorry, cannot split this hunk
    @@ -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,?]?

... or an invalid option, i.e: "U"

    $ 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
    ? - 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 52be1ddb15..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))
 				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);
+				prev_hunk_index = -1;
+			}
 		} else if (s->answer.buf[0] == 'e') {
 			if (!(permitted & ALLOW_EDIT))
 				err(s, _("Sorry, cannot edit this hunk"));
@@ -1661,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
 				goto soft_increment;
 			}
 		} else if (s->answer.buf[0] == 'p') {
-			/* nothing to do */
+			prev_hunk_index = -1;
 		} else {
 			const char *p = _(help_patch_remainder), *eol = p;
 
-- 
2.44.0.494.gf640f9da83

Re: [PATCH 2/2] add-patch: do not print hunks repeatedly

[Junio C Hamano]

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

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

Indeed.  The more important part of the message from the command in
these cases tells that the previous input was invalid and why.

Stopping after that without showing the hunk again does make sense.

[PATCH v2 0/2] improve interactive-patch

[Rubén Justo]

Let's reduce the verbosity in the interactive-patch process, in order to
make it less confusing.

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;
-- 
2.44.0.494.gfaa70abb0d

[PATCH v2 1/2] add-patch: introduce 'p' in interactive-patch

[Rubén Justo]

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..90b47927b2 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 again the current hunk
        ? - print help
 +
 After deciding the fate for all hunks, if there is any hunk
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"
    "? - 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 special is needed */
 		} else {
 			const char *p = _(help_patch_remainder), *eol = p;
 
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
 '
-- 
2.44.0.494.gfaa70abb0d

Re: [PATCH v2 1/2] add-patch: introduce 'p' in interactive-patch

[Phillip Wood]

Hi Rubén

On 26/03/2024 00:17, Rubén Justo wrote:
> 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.

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. I also wonder if 'r' for 
"re-display" would better convey the intent of this keybinding.

> 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 (or "re-display the hunk" if 
we go for 'r')

Best Wishes

Phillip

Re: [PATCH v2 1/2] add-patch: introduce 'p' in interactive-patch

[Rubén Justo]

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?

Re: [PATCH v2 1/2] add-patch: introduce 'p' in interactive-patch

[Phillip Wood]

Hi Rubén

On 26/03/2024 18:40, Rubén Justo wrote:
> 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?

If we've printed the hunk followed by the prompt then there is no point 
in offering 'p' because it does not do anything useful for the user. It 
is only useful offer to show the hunk again when we've printed an error 
message that separates the prompt from the hunk. I don't think we should 
make 'p' an error, but it seems like clutter to put it in the prompt 
when it does not offer anything useful to the user.

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

I'm not sure I follow as "re-display" or for that matter "reprint" are 
also verbs. The reason I suggested "re-display" is that it I think the 
name is a more accurate description because we're printing the same hunk 
again.

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

I've no objecting to "current"

Best Wishes

Phillip

[PATCH v2 2/2] add-patch: do not print hunks repeatedly

[Rubén Justo]

The interactive-patch is a sequential process where, on each step, we
print one hunk from a patch and then ask the user how to proceed.

There is a possibility of repeating a step, for example if the user
enters a non-applicable option, i.e: "s"

    $ 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,?]? s
    Sorry, cannot split this hunk
    @@ -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,?]?

... or an invalid option, i.e: "U"

    $ 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))
 				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);
+				prev_hunk_index = -1;
+			}
 		} else if (s->answer.buf[0] == 'e') {
 			if (!(permitted & ALLOW_EDIT))
 				err(s, _("Sorry, cannot edit this hunk"));
@@ -1661,7 +1667,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 */
+			prev_hunk_index = -1;
 		} else {
 			const char *p = _(help_patch_remainder), *eol = p;
 
-- 
2.44.0.494.gfaa70abb0d

Re: [PATCH v2 2/2] add-patch: do not print hunks repeatedly

[Phillip Wood]

Hi Rubén

On 26/03/2024 00:17, Rubén Justo wrote:
>      $ 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.

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

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. Also as it needs to hold a negative value we 
should declare it as ssize_t like the variables on the line below.

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

Best Wishes

Phillip

Re: [PATCH v2 2/2] add-patch: do not print hunks repeatedly

[Rubén Justo]

On Tue, Mar 26, 2024 at 02:39:18PM +0000, Phillip Wood wrote:

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

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.
     (1/4) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? 


I think such a change fits well in this series.  Let's see if it does.

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

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

OK

Re: [PATCH v2 2/2] add-patch: do not print hunks repeatedly

[Phillip Wood]

On 26/03/2024 18:46, Rubén Justo wrote:
> 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.

Yes, I like that (though I'd use the same quotes for both parts of the 
message)

>       (1/4) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?
> 
> 
> I think such a change fits well in this series.  Let's see if it does.

I think it is a good fit with not reprinting the hunk as it reduces the 
number of lines we print after an invalid key which keeps the prompt 
nearer the hunk text.

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

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

Best Wishes

Phillip

Re: [PATCH v2 2/2] add-patch: do not print hunks repeatedly

[Rubén Justo]

On Wed, Mar 27, 2024 at 11:06:49AM +0000, Phillip Wood wrote:

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

Yes, you're right.  Using the same quotes are the correct thing to do.
I don't know how I thought we should print the result of
git_read_line_interactively().  After thinking about it again, I see
that would be misleading, to say the least.

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

I have no strong objection.  I'll reroll leaving that strbuf_reset
untouched.

Re: [PATCH v2 0/2] improve interactive-patch

[Phillip Wood]

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.

Best Wishes

Phillip

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

Re: [PATCH v2 0/2] improve interactive-patch

[Junio C Hamano]

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

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

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.

Re: [PATCH v2 0/2] improve interactive-patch

[Rubén Justo]

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

Thanks.

Re: [PATCH v2 0/2] improve interactive-patch

[Junio C Hamano]

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

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

It is perfectly OK that you are not sure, as I do not think I am,
either.  I am reasonably sure "git diff-files" wouldn't be involved,
though ;-)  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.

In any case, the future enhancement is not within, and we shouldn't
add it to, the scope of this topic.

Re: [PATCH v2 0/2] improve interactive-patch

[Rubén Justo]

On Tue, Mar 26, 2024 at 12:13:46PM -0700, Junio C Hamano wrote:

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

OK.  In a superficial review that I made, I was struck by the atexit()
we have in setup_pager().  I suspect we need to deal with that.

> In any case, the future enhancement is not within, and we shouldn't
> add it to, the scope of this topic.

Yeah.  I am quite sure that the other 'Invalid option' thing does not
need to be in this series either.  I'm pretty happy with the current
state of this series and there's plenty of improvement here to have a
future improve-interactive-patch-further series.

I'll wait for Phillip's comments and then I'll reroll a v3.  At least
the ssize_t change deserves to be included.

Re: [PATCH v2 0/2] improve interactive-patch

[Rubén Justo]

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;
 

Re: [PATCH v2 0/2] improve interactive-patch

[Dragan Simic]

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;

Re: [PATCH v2 0/2] improve interactive-patch

[Rubén Justo]

On Fri, Mar 29, 2024 at 08:48:18PM +0100, Dragan Simic wrote:
> 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.

I'm glad you find it useful.

I'm not going to include it in this series.  However, I've been using it
these days, and it's been fulfilling my needs.

I'm not sure of the implementation, though.

For a start, fputs_to_pager_or_stdout may be a better name, but ugly.

And "pager.h" may not be the right place to have it.

Perhaps something based on a strbuf is a better and more usable approach.

To name a few ...

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

Re: [PATCH v2 0/2] improve interactive-patch

[Junio C Hamano]

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

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

For this particular application, such a "I have the whole thing in
core, send it to the pater" API might be sufficient, but it may not
be usable if the existing code sends its output in pieces already.

I was envisioning an API more along the lines of

	/*
	 * pager control data structure
	 */
	struct pager_control;

	/*
	 * Start a pager, from now on, what we write to our fd #0
	 * and stdout are fed to the pager.  The function returns
	 * 0 on success, or -1 on failure.
	 */
	int redirect_fd_to_pager(struct pager_control *);

	/*
	 * We have written everything we want to write to the
	 * pager.  Tell the pager that we are done.  Wait until
	 * the end-user quits the pager and then give us control
	 * back.  The fd #0 and stdout are restored.
	 */
	int wait_for_pager(struct pager_control *);

so that any subpart of existing code can be enclosed inside the two
calls, keep writing to fd#0/stdout as before, and we'd page only
that part of the output from the program without changing anything
else.

Re: [PATCH v2 0/2] improve interactive-patch

[Phillip Wood]

Hi Junio

On 26/03/2024 15:31, Junio C Hamano wrote:
> 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).

It would certainly be nice to have some way of paging the output in the 
future.

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

I agree there is a difference between omitting a key that simply does 
not work because there is no previous hunk to omitting 'p'. 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 
and so I think it clutters up the prompt. I do think though that if the 
user happens to press 'p' when it is not in the prompt then we should 
just re-display the current hunk rather than printing an error.

Best Wishes

Phillip

Re: [PATCH v2 0/2] improve interactive-patch

[Junio C Hamano]

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.

Thanks.

Re: [PATCH v2 0/2] improve interactive-patch

[Phillip Wood]

On 27/03/2024 15:43, Junio C Hamano wrote:
> 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 ;-).

Good point, I'd not thought of that - it is useful after all.

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

We're only omitting to show the hunk if we've already shown it so I 
think of it as showing it again but I don't mind much either way.

Thanks

Phillip

Re: [PATCH v2 0/2] improve interactive-patch

[Rubén Justo]

On Wed, Mar 27, 2024 at 08:43:08AM -0700, Junio C Hamano wrote:
> 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.

Thanks for saying it much better than me.

I would add that I see this series in terms of reducing verbosity rather
than repetition.  I'm not trying to stop showing a hunk because we've
just shown it.  In fact, maybe we can find a way to not show the hunk
until requested.

Re: [PATCH v2 0/2] improve interactive-patch

[Rubén Justo]

On Tue, Mar 26, 2024 at 02:37:50PM +0000, Phillip Wood wrote:
> 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.

I'm glad you think so.  Thank you for reviewing it.

[PATCH v3 0/2] improve interactive-patch

[Rubén Justo]

Let's reduce the verbosity in the interactive-patch process, in order to
make it less confusing.

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                | 19 +++++++++++++++----
 t/t3701-add-interactive.sh | 22 +++++++++++-----------
 3 files changed, 27 insertions(+), 15 deletions(-)

Range-diff against v2:
1:  bbb3bbea1b ! 1:  ca2777cb12 add-patch: introduce 'p' in interactive-patch
    @@ 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
    ++       p - print the current hunk again
             ? - print help
      +
      After deciding the fate for all hunks, if there is any hunk
    @@ 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 again the current hunk\n"
    ++   "p - print the current hunk again\n"
         "? - print help\n");
      
      static int patch_update_file(struct add_p_state *s,
2:  4d5626e1c4 ! 2:  d7ad5e5424 add-patch: do not print hunks repeatedly
    @@ Commit message
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
      ## add-patch.c ##
    -@@ add-patch.c: 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,
      			     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;
    + 	size_t hunk_index = 0;
    +-	ssize_t i, undecided_previous, undecided_next;
    ++	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
      	struct hunk *hunk;
      	char ch;
    + 	struct child_process cp = CHILD_PROCESS_INIT;
     @@ add-patch.c: 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) {
    ++			if (rendered_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;
    ++				rendered_hunk_index = hunk_index;
     +			}
      
    --			strbuf_reset(&s->buf);
    + 			strbuf_reset(&s->buf);
    ++
      			if (undecided_previous >= 0) {
      				permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
      				strbuf_addstr(&s->buf, ",k");
    @@ add-patch.c: static int patch_update_file(struct add_p_state *s,
      				color_fprintf_ln(stdout, s->s.header_color,
      						 _("Split into %d hunks."),
      						 (int)splittable_into);
    -+				prev_hunk_index = -1;
    ++				rendered_hunk_index = -1;
     +			}
      		} else if (s->answer.buf[0] == 'e') {
      			if (!(permitted & ALLOW_EDIT))
    @@ add-patch.c: static int patch_update_file(struct add_p_state *s,
      			}
      		} else if (s->answer.buf[0] == 'p') {
     -			/* nothing special is needed */
    -+			prev_hunk_index = -1;
    ++			rendered_hunk_index = -1;
      		} else {
      			const char *p = _(help_patch_remainder), *eol = p;
      
-- 
2.44.0.370.gd7ad5e5424

[PATCH v3 1/2] add-patch: introduce 'p' in interactive-patch

[Rubén Justo]

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
        ? - print help
 +
 After deciding the fate for all hunks, if there is any hunk
diff --git a/add-patch.c b/add-patch.c
index 68f525b35c..922c43378e 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 the current hunk again\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 special is needed */
 		} else {
 			const char *p = _(help_patch_remainder), *eol = p;
 
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
 '
-- 
2.44.0.370.gd7ad5e5424

Re: [PATCH v3 1/2] add-patch: introduce 'p' in interactive-patch

[Junio C Hamano]

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

With the hint of "stop printing under certian circumstances" in the
proposed log message, this makes us anticipate that "again" will be
dropped dynamically when we did skip, and after we printed, "again"
will be kept.

But such a dynamic rewording would be more appropriate for the
interactive command prompt (which is already dynamic).  Perhaps
dropping "again" from this static text would be better.

Let's see what happens in [2/2].  Perhaps we have already code to
dynamically update the help text, in which case the above does not
apply.

[PATCH v3 2/2] add-patch: do not print hunks repeatedly

[Rubén Justo]

The interactive-patch is a sequential process where, on each step, we
print one hunk from a patch and then ask the user how to proceed.

There is a possibility of repeating a step, for example if the user
enters a non-applicable option, i.e: "s"

    $ 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,?]? s
    Sorry, cannot split this hunk
    @@ -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,?]?

... or an invalid option, i.e: "U"

    $ 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 | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 922c43378e..e0f4cd9b9b 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1395,7 +1395,7 @@ static int patch_update_file(struct add_p_state *s,
 			     struct file_diff *file_diff)
 {
 	size_t hunk_index = 0;
-	ssize_t i, undecided_previous, undecided_next;
+	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
@@ -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;
 		} else {
 			const char *p = _(help_patch_remainder), *eol = p;
 
-- 
2.44.0.370.gd7ad5e5424

Re: [PATCH v3 2/2] add-patch: do not print hunks repeatedly

[Junio C Hamano]

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

> @@ -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) {

So, the one previously rendered is compared with the current one,
which raises an obvious question, what happens to the first new hunk
resulting from splitting a hunk?  The answer is below ...

> +				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;
> +			}

... we explicitly say "we always want to show the current one after
this operation", which makes sense.

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

And that matches what is done for 'p', which is the base case that
wants to say "no matter what, show the current one".  Doubly makes
sense.

>  		} else {
>  			const char *p = _(help_patch_remainder), *eol = p;

Looking good.  As we are not doing anything dynamic to the help
text, I think dropping "again" in [1/2] would make sense.

Will queue.  Thanks.

Re: [PATCH v3 2/2] add-patch: do not print hunks repeatedly

[Rubén Justo]

On Thu, Mar 28, 2024 at 07:46:00AM -0700, Junio C Hamano wrote:

I realized that I missed a minor fix suggested by Phillip in the
previous iteration.

> Looking good.  As we are not doing anything dynamic to the help
> text, I think dropping "again" in [1/2] would make sense.

OK.

I'll reroll with both changes.

[PATCH v4 0/2] improve interactive-patch

[Rubén Justo]

Let's reduce the verbosity in the interactive-patch process, in order to
make it less confusing.

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                | 23 +++++++++++++++++------
 t/t3701-add-interactive.sh | 22 +++++++++++-----------
 3 files changed, 29 insertions(+), 17 deletions(-)

Range-diff against v3:
1:  545ca3f0b3 ! 1:  338f1ebc9e add-patch: introduce 'p' in interactive-patch
    @@ 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 the current hunk again
    ++       p - print the current hunk
             ? - print help
      +
      After deciding the fate for all hunks, if there is any hunk
    @@ 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 again\n"
    ++   "p - print the current hunk\n"
         "? - print help\n");
      
      static int patch_update_file(struct add_p_state *s,
2:  b6b98e5dbb ! 2:  e17b8d70f1 add-patch: do not print hunks repeatedly
    @@ add-patch.c: static int patch_update_file(struct add_p_state *s,
      				permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
      				strbuf_addstr(&s->buf, ",k");
     @@ add-patch.c: static int patch_update_file(struct add_p_state *s,
    - 			if (!(permitted & ALLOW_SPLIT))
    + 			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,
    +-			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."),
-- 
2.44.0.370.ge17b8d70f1

[PATCH v4 1/2] add-patch: introduce 'p' in interactive-patch

[Rubén Justo]

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..aceaa025e3 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
        ? - print help
 +
 After deciding the fate for all hunks, if there is any hunk
diff --git a/add-patch.c b/add-patch.c
index 68f525b35c..b5d3a3f0cc 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 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 special is needed */
 		} else {
 			const char *p = _(help_patch_remainder), *eol = p;
 
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
 '
-- 
2.44.0.370.ge17b8d70f1

[PATCH v4 2/2] add-patch: do not print hunks repeatedly

[Rubén Justo]

The interactive-patch is a sequential process where, on each step, we
print one hunk from a patch and then ask the user how to proceed.

There is a possibility of repeating a step, for example if the user
enters a non-applicable option, i.e: "s"

    $ 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,?]? s
    Sorry, cannot split this hunk
    @@ -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,?]?

... or an invalid option, i.e: "U"

    $ 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 | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index b5d3a3f0cc..5f11692911 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1395,7 +1395,7 @@ static int patch_update_file(struct add_p_state *s,
 			     struct file_diff *file_diff)
 {
 	size_t hunk_index = 0;
-	ssize_t i, undecided_previous, undecided_next;
+	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
@@ -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");
@@ -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;
 
-- 
2.44.0.370.ge17b8d70f1

Re: [PATCH v4 2/2] add-patch: do not print hunks repeatedly

[phillip.wood123]

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

Best Wishes

Phillip

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

Re: [PATCH v4 2/2] add-patch: do not print hunks repeatedly

[Rubén Justo]

On Fri, Mar 29, 2024 at 10:41:05AM +0000, phillip.wood123@gmail.com wrote:
> 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

OK.

Junio has already queued this iteration.  I wonder if we could reduce
the noise in the list by squashing:

--- >8 ---
diff --git a/add-patch.c b/add-patch.c
index 5f11692911..778f168298 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1451,12 +1451,10 @@ static int patch_update_file(struct add_p_state *s,
 			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");