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
