From: Samuel Abraham Date: Fri, 23 Jan 2026 21:43:09 GMT Subject: Re: [RFC PATCH 1/1] add-patch: Allow reworking with a file after deciding on all its hunks Message-ID: In-Reply-To: On Fri, Jan 23, 2026 at 5:38 PM Junio C Hamano wrote: > > Abraham Samuel Adekunle writes: > > > After deciding on all hunks in a file, the interactive session > > advances automatically to the next file if there is another, > > or the process ends. > > > > Allow for reworking with a file by introducing a what_now prompt which > > allows for navigating with J/K or advancing to the next file if there is one. > > Describe "how" you are allowing these new things that users used not > to be able to do (no, not in the "by adding this variable and > switching on its value" sense, but in the "now deciding on all the > hunks in a file does not automatically advance to the next file, and > the user has to do X to move forward" sense). Thank you for your feedback Junio. Okay, this is noted. > > > - int colored = !!s->colored.len, quit = 0, use_pager = 0; > > + int colored = !!s->colored.len, quit = 0, use_pager = 0, skip_what_now = 0; > > This is getting overly long. Wouldn't it be easier to follow if a > preliminary patch split these existing variables into three > independent definitions, and the main patch adds the fourth one? Yes it will be easier to follow. I will do that. > > > + if (s->file_diff_nr > 1) > > + prompt_whatnow = _("What now? [J,K,q,>]? "); > > + else > > + prompt_whatnow = _("What now? [J,K,q]? "); > > I wonder if ">" has to be made so special. Wouldn't it be easier to > reason about the logic if ">" (and probably "<" to go back by one > file) are added to the prompt in the same logic that decides 'g', > 'k', 's', etc. should be shown using the "permitted" variable? Yes this makes a lot of sense. Thank you for the guidance. > > And when the inter-file navigation is in the permitted set (i.e., > there are multiple files involved), you'd show ">" (or "<", or both > if you are dealing with the second file among three files) and ask, > instead of silently moving to the next one, or something like that. Yes I understand. > > Organizing the logic that way will also allow you to move to the > next file _without_ first having to decide on all hunks in the > current file. Just say ">" to deal with the next file first, and > after you are done, either come back with "<", or the system notices > that there are undecided hunks in the earlier file and takes you > back automatically. This is very insightful. I will work with this design in mind Thank you > > I also have a hunch that with such a code structure you may not even > need skip_what_now flag, but I haven't even written the code in my > head, so if somebody tries to do so, they may discover the reason > why such a flag is still needed. Yes > > > strbuf_reset(&s->buf); > > if (file_diff->hunk_nr) { > > - if (rendered_hunk_index != hunk_index) { > > + if (rendered_hunk_index != hunk_index || skip_what_now == 1) { > > Style (which may become irrelevant, as I just said the variable may > not be needed after all, but anyway). Elsewhere skip_what_now is > used only for "is it zero, or is it not zero?". Comparing > explicitly with 1 only here makes readers suspect if assigning 2 or > 70 to the variable has special meanings and wastes their brain > cycles. I will give more thoughtful efforts into the next versions I will send after your recommendations. Thank you very much Abraham.