From: René Scharfe <l.s.r@web.de>
Date: Mon, 06 Oct 2025 17:18:04 GMT
Subject: Re: [PATCH v2 4/5] add-patch: let options k and K roll over like j and J
Message-ID: <0ea56923-2041-43bd-8c35-cc93c3c95c70@web.de>
In-Reply-To: <xmqqh5wdrrub.fsf@gitster.g>

On 10/5/25 10:55 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> @@ -1584,7 +1591,8 @@ static int patch_update_file(struct add_p_state *s,
>>  			}
>>  		} else if (s->answer.buf[0] == 'K') {
>>  			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
>> -				hunk_index--;
>> +				hunk_index = dec_mod(hunk_index,
>> +						     file_diff->hunk_nr);
>>  			else
>>  				err(s, _("No previous hunk"));
> 
> I was wondering if we want to always allow J and K; even when you
> have only one hunk, you can still wrap around to come back to the
> current hunk, and that we can do without any extra checking logic.
> 
> But it is also OK to require 2 or more hunks to "switch" to the
> other hunk, which is what you do with
> 
> 			if (file_diff->hunk_nr > 1) {
> 				permitted |= ALLOW_GOTO_PREVIOUS_HUNK;
> 				strbuf_addstr(&s->buf, ",K");
> 			}
> 
> to require more than 1.  But the error message "No previous hunk"
> sounds somewhat awkward.  If user accepts the circular nature of how
> we decide what "previous" is, then when we have a single hunk, the
> current hunk itself _is_ the previous hunk, but because we insist
> that there are at least 2, that interpretation would not work.  With
> "wraparound" semantics, "No other hunk(s)", would be a better way to
> give the error, no?  The same comment applies to 'J'.

OK.
>>  		} else if (s->answer.buf[0] == 'J') {
> 
> This makes perfect sense, but then, after this post-context we have this:
> 
> 			if (permitted & ALLOW_GOTO_NEXT_HUNK)
> 				hunk_index++;
> 			else
> 				err(s, _("No next hunk"));
> 
> and it sticks out that the post-increment of hunk_index here is not
> using inc_mod() for symmetry.

This symmetry _is_ tantalizing.  Had the call originally, removed it
because it was unnecessary and didn't fit the narrative.
> I am wondering if with that updated (I would not say "fixed"), if we
> can lose the "oops we overflowed so let's wrap around" belt-and-suspender
> code at the beginning of the loop, i.e.
> 
> 	for (;;) {
> 		enum {
> 			ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
> 			...
> 			ALLOW_EDIT = 1 << 6
> 		} permitted = 0;
> 
> 		if (hunk_index >= file_diff->hunk_nr)
> 			hunk_index = 0;
> 
> or if there still are other code that rely on this "oops we
> overflowed" adjustment?
Good question, gave me the idea that a and d should roll over as well.

Other than that there's just the so-called soft_increment, which would
need something like this:

diff --git a/add-patch.c b/add-patch.c
index b0389c5d5b..59a9eb586d 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1546,8 +1546,7 @@ static int patch_update_file(struct add_p_state *s,
 		if (ch == 'y') {
 			hunk->use = USE_HUNK;
 soft_increment:
-			hunk_index = undecided_next < 0 ?
-				file_diff->hunk_nr : undecided_next;
+			hunk_index = undecided_next < 0 ? 0 : undecided_next;
 		} else if (ch == 'n') {
 			hunk->use = SKIP_HUNK;
 			goto soft_increment;

Or undecided_next could be set to 0 before the if/else cascade, then
this becomes a simple assignment and we can get rid of the goto.

Couldn't find a way to remove the back-to-square-1 check that would be
significantly better overall and thus worth the hassle, though.

René