Re: [PATCH] add-patch: roll over to next undecided hunk

> René Scharfe <l.s.r@web.de> writes:
> 
>> git add --patch presents diff hunks one after the other, asking whether
>> to add them.  If we mark some as undecided, e.g. with J, then it will
> 
> Perhaps "mark" -> "leave".
> 
> I somehow find it awkward to say "mark as undecided", as I have
> always viewed J/K as a way to skip a hunk, leaving it undecided.
> 
> Besides, "J" lets you revisit a hunk that you earlier have decided
> to use of hold off, and it leaves your last decision on that hunk.
> A statement that implies "J marks as undecided" is misleading.

>> start over after reaching the last hunk.  It always starts over at the
>> very first hunk, though, even if we already decided on it.  Skip
>> decided hunks when rolling over instead.
> 
> Nicely analyzed.
> 
>> Reported-by: Windl, Ulrich <u.windl@ukr.de>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  add-patch.c                |  9 ++++++++-
>>  t/t3701-add-interactive.sh | 20 ++++++++++++++++++++
>>  2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/add-patch.c b/add-patch.c
>> index b0389c5d5b..42a8394c92 100644
>> --- a/add-patch.c
>> +++ b/add-patch.c
>> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,
>>  	render_diff_header(s, file_diff, colored, &s->buf);
>>  	fputs(s->buf.buf, stdout);
>>  	for (;;) {
>> -		if (hunk_index >= file_diff->hunk_nr)
>> +		if (hunk_index >= file_diff->hunk_nr) {
>>  			hunk_index = 0;
>> +			for (i = 0; i < file_diff->hunk_nr; i++) {
>> +				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
>> +					hunk_index = i;
>> +					break;
>> +				}
>> +			}
>> +		}
> 
> OK.
> 
> This is probably a closely related tangent, but last night I was
> looking this function and found that its per-hunk loop does
> completely bogus thing.  For example, find a case where you have
> more than one hunks, among which there are splittable and
> non-splittable hunks (a hunk is splittable if there are context
> lines between an added or a removed line).  Start cycling the hunks
> without making any decisions with "J" or "K".  Once you visited a
> splittable hunk (where you'd see 's' among the possible choices),
> coming back to an unsplittable hunk will now let you split it!  's'
> may not be visible among the choices, but telling it to 's'plit will
> give you "Split into 1", which is a technically correct nonsense.
> 
> This is because the handling of "permitted" in that function only
> adds, without resetting at the end of processing the current hunk.
> Yet it does something like this:
> 
> 	for (;;) {
> 		...
> 		strbuf_reset(&s->buf);
> 		if (file_diff->hunk_nr) {
> 			... add choices to the prompt ...
> 			if (hunk->splittable_into > 1) {
> 				permitted |= ALLOW_SPLIT;
> 				strbuf_addstr(&s->buf, ",s");
> 			}
> 			...
> 		}
> 		...
> 		printf(_(s->mode->prompt_mode[prompt_mode_type]),
> 		       s->buf.buf);
> 		if (*s->s.reset_color_interactive)
> 			fputs(s->s.reset_color_interactive, stdout);
> 		fflush(stdout);
> 		if (read_single_character(s) == EOF)
> 			break;
> 		ch = tolower(s->answer.buf[0]);
> 		... dispatch on the command character ...
> 		if (ch == 'y') {
> 			...
> 		} else if (s->answer.buf[0] == 's') {
> 			size_t splittable_into = hunk->splittable_into;
> 			if (!(permitted & ALLOW_SPLIT)) {
> 				err(s, _("Sorry, cannot split this hunk"));
> 			} else if (!split_hunk(s, file_diff,
> 					     hunk - file_diff->hunk)) {
> 				color_fprintf_ln(stdout, s->s.header_color,
> 						 _("Split into %d hunks."),
> 						 (int)splittable_into);
> 				rendered_hunk_index = -1;
> 			}
> 		...
> 
> Notice that the prompt is built correctly but that information is
> *not* used when deciding if the operation is possible?
> 
> This is another ancient regression that was introduced while
> rewriting this program in C near the end of 2019, I think.  And this
> causes many other bugs in this area, like 'k' at the very first hunk
> gets complaint "No previous hunk" only once (you move to the next
> one with 'j' and come back to the first hunk with 'k', and then 'k'
> no longer complains, even though it is not among the choice).

> With this bug, however, we have gained a bit of useful feature, I
> think.  Even though j/J should not be offered when we are at the
> last hunk for a file, we do wrap-around to the first hunk.  I just
> checked the original code before the C rewrite, and even though it
> were written defensively so that incrementing the current hunk
> number to 5 when you have only 4 hunks would take you back to the
> initial hunk (instead of barfing), because we did not have this
> "permitted is never reset" bug, it actually did not allow you to go
> beyond the end with j/J.  Today's code seems to have inherited this
> defensive adjustment to stay within the available hunks, and with
> the "permitted is never reset" bug, we are taken back to the first
> hunk.