diff mbox series

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

Message ID 6bc655ea-783e-477e-84ec-04503fa20799@gmail.com (mailing list archive)
State Superseded
Headers show
Series improve interactive-patch | expand

Commit Message

Rubén Justo March 25, 2024, 9:07 p.m. UTC
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(-)

Comments

Junio C Hamano March 25, 2024, 9:34 p.m. UTC | #1
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.
diff mbox series

Patch

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;