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

[Phillip Wood <phillip.wood123@gmail.com>]

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