| Message ID | 60c43160-1b60-42f6-9488-4cc332201b7e@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | improve interactive-patch | expand |
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.
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.
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;
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(-)