Re: [PATCH v2 4/5] add-patch: let options k and K roll over like j and J
- From
René Scharfe <l.s.r@web.de>
- Date
- Oct 6, 2025, 17:18 UTC
- 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:
Show 31 quoted lines
> 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.
Show 11 quoted lines
>> } 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.
Show 16 quoted lines
> 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é