From: Phillip Wood Date: Wed, 27 Mar 2024 11:06:49 GMT Subject: Re: [PATCH v2 2/2] add-patch: do not print hunks repeatedly Message-ID: In-Reply-To: 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