| Message ID | c123bf09-7f4c-46f5-aa09-48b2816bf85d@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | improve interactive-patch | expand |
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
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
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
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.
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;
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(-)