From: Samuel Abraham Date: Fri, 30 Jan 2026 09:22:25 GMT Subject: Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks Message-ID: In-Reply-To: On Wed, Jan 28, 2026 at 12:26 PM Samuel Abraham wrote: > > On Tue, Jan 27, 2026 at 9:48 PM Junio C Hamano wrote: > > > > Abraham Samuel Adekunle writes: > > > > > diff --git a/add-patch.c b/add-patch.c > > > index 173a53241e..edb2fab3fd 100644 > > > --- a/add-patch.c > > > +++ b/add-patch.c > > > @@ -1418,6 +1418,8 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n" > > > "e - manually edit the current hunk\n" > > > "p - print the current hunk\n" > > > "P - print the current hunk using the pager\n" > > > + "> - go to the next file\n" > > > + "< - go to the previous file\n" > > > "? - print help\n"); > > > > As I said earlier, these may have to be optional. It may give > > existing users a jarring experience to be given a prompt after > > deciding on all the hunks in a file, when they expect to be on > > the next file already. > > Yes I agree. > I will work on making it an optional feature. > > > > > > @@ -1441,6 +1443,17 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx) > > > return false; > > > } > > > > > > +static size_t get_file_diff_index(struct add_p_state *s, struct file_diff *file_diff) { > > > + size_t idx = 0; > > > + for (size_t i = 0; i < s->file_diff_nr; i++) { > > > + if (s->file_diff + i == file_diff) { > > > + idx = i; > > > + break; > > > + } > > > + } > > > + return idx; > > > +} > > > > Yuck. Can't we lose the need for this function if we change the > > interface into patch_update_file so that it takes the index of the > > file (i.e., instead of "&s.file_diff[i]", pass "i")? There is only > > one caller to patch_update_file() which is run_add_p(), so such a > > clean-up should be trivial. > > Ah yes this is definitely a sweet and better option. > > > > > > static int patch_update_file(struct add_p_state *s, > > > struct file_diff *file_diff) > > > { > > > @@ -1448,9 +1461,10 @@ static int patch_update_file(struct add_p_state *s, > > > ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1; > > > struct hunk *hunk; > > > char ch; > > > - struct child_process cp = CHILD_PROCESS_INIT; > > > > This is related to the hoisting of the actual patch application to > > the caller, but it is not explained why such a change is needed, and > > it byitself, even without the "jump to the next file before deciding > > on all the hunks" feature. What problem is it solving??? > > I explained this below > > > > > If it is necessary to move the code to run "git apply" to the > > caller, would it make sense to split this patch into at least two > > patches, one to do such a move, possibly another patch to change the > > function signature of patch_update_file() so that it takes the file > > index instead of file_diff struct, and finally another patch to > > allow jumping around the files? > > Okay yes it would make much sense. > > > > > > int colored = !!s->colored.len, quit = 0, use_pager = 0; > > > enum prompt_mode_type prompt_mode_type; > > > + size_t file_diff_index = get_file_diff_index(s, file_diff); > > > + int all_decided = 0; > > > > > > /* Empty added files have no hunks */ > > > if (!file_diff->hunk_nr && !file_diff->added) > > > @@ -1467,7 +1481,9 @@ static int patch_update_file(struct add_p_state *s, > > > ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3, > > > ALLOW_SEARCH_AND_GOTO = 1 << 4, > > > ALLOW_SPLIT = 1 << 5, > > > - ALLOW_EDIT = 1 << 6 > > > + ALLOW_EDIT = 1 << 6, > > > + ALLOW_GOTO_PREVIOUS_FILE = 1 << 7, > > > + ALLOW_GOTO_NEXT_FILE = 1 << 8 > > > } permitted = 0; > > > > > > if (hunk_index >= file_diff->hunk_nr) > > > @@ -1499,8 +1515,7 @@ static int patch_update_file(struct add_p_state *s, > > > /* Everything decided? */ > > > if (undecided_previous < 0 && undecided_next < 0 && > > > hunk->use != UNDECIDED_HUNK) > > > - break; > > > - > > > + all_decided = 1; > > > strbuf_reset(&s->buf); > > > if (file_diff->hunk_nr) { > > > if (rendered_hunk_index != hunk_index) { > > > @@ -1548,6 +1563,16 @@ static int patch_update_file(struct add_p_state *s, > > > permitted |= ALLOW_EDIT; > > > strbuf_addstr(&s->buf, ",e"); > > > } > > > + if (file_diff_index >= 0 && > > > + file_diff_index < s->file_diff_nr - 1) { > > > + permitted |= ALLOW_GOTO_NEXT_FILE; > > > + strbuf_addstr(&s->buf, ",>"); > > > + } > > > + if (file_diff_index > 0 && > > > + file_diff_index <= s->file_diff_nr - 1) { > > > + permitted |= ALLOW_GOTO_PREVIOUS_FILE; > > > + strbuf_addstr(&s->buf, ",<"); > > > + } > > > > As can be seen in what patch_update_file() does when the user says > > 'J' or 'K', hunks in a file are treated as a ring, and these > > commands are enabled as long as there are more than one hunks. > > > > Perhaps that is more familiar than "when we hit the floor, we cannot > > sink deeper, and when we hit the ceiling, we cannot float more", > > which seems to be what the above implements. > > Yes I understand this now. > It does make sense this way. > > > > > > strbuf_addstr(&s->buf, ",p,P"); > > > } > > > if (file_diff->deleted) > > > @@ -1566,6 +1591,9 @@ static int patch_update_file(struct add_p_state *s, > > > : 1)); > > > printf(_(s->mode->prompt_mode[prompt_mode_type]), > > > s->buf.buf); > > > + if (all_decided) > > > + printf(_("\n%s All hunks decided. What now? "), > > > + s->s.prompt_color); > > > if (*s->s.reset_color_interactive) > > > fputs(s->s.reset_color_interactive, stdout); > > > fflush(stdout); > > > @@ -1618,7 +1646,24 @@ static int patch_update_file(struct add_p_state *s, > > > } else if (ch == 'q') { > > > quit = 1; > > > break; > > > - } else if (s->answer.buf[0] == 'K') { > > > + } else if (s->answer.buf[0] == '>') { > > > + if (permitted & ALLOW_GOTO_NEXT_FILE) { > > > + quit = 0; > > > + break; > > > + } else { > > > + err(s, _("No next file")); > > > + continue; > > > + } > > > + } else if (s->answer.buf[0] == '<') { > > > + if (permitted & ALLOW_GOTO_PREVIOUS_FILE) { > > > + quit = 2; > > > + break; > > > > What's the magic number "2"? Should "quit" become an enum with > > elements that are more meaningfully named? > > Okay, yes an enum would be better. > > > > > > + } else { > > > + err(s, _("No previous file")); > > > + continue; > > > + } > > > + } > > > + else if (s->answer.buf[0] == 'K') { > > > if (permitted & ALLOW_GOTO_PREVIOUS_HUNK) > > > hunk_index = dec_mod(hunk_index, > > > file_diff->hunk_nr); > > > @@ -1775,33 +1820,6 @@ static int patch_update_file(struct add_p_state *s, > > > } > > > } > > > > > > - /* Any hunk to be used? */ > > > - for (i = 0; i < file_diff->hunk_nr; i++) > > > - if (file_diff->hunk[i].use == USE_HUNK) > > > - break; > > > - > > > - if (i < file_diff->hunk_nr || > > > - (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) { > > > - /* At least one hunk selected: apply */ > > > - strbuf_reset(&s->buf); > > > - reassemble_patch(s, file_diff, 0, &s->buf); > > > - > > > - discard_index(s->s.r->index); > > > - if (s->mode->apply_for_checkout) > > > - apply_for_checkout(s, &s->buf, > > > - s->mode->is_reverse); > > > - else { > > > - setup_child_process(s, &cp, "apply", NULL); > > > - strvec_pushv(&cp.args, s->mode->apply_args); > > > - if (pipe_command(&cp, s->buf.buf, s->buf.len, > > > - NULL, 0, NULL, 0)) > > > - error(_("'git apply' failed")); > > > - } > > > - if (repo_read_index(s->s.r) >= 0) > > > - repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0, > > > - 1, NULL, NULL, NULL); > > > - } > > > > It is not obvious why the above code needs to be hoisted to the > > caller. > > I explained this below. > > > > > > putchar('\n'); > > > return quit; > > > } > > > @@ -1813,7 +1831,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode, > > > struct add_p_state s = { > > > { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT > > > }; > > > - size_t i, binary_count = 0; > > > + size_t i, j, binary_count = 0; > > > + size_t patch_update_result = 0; > > > > Hmph, I think patch_update_file() returns "int quit". Why do we > > want overly wide type to store the result, which cannot even express > > negative number to potentially signal a failure? > > Sorry, this is a mistake on my part > > > > > > + struct child_process cp = CHILD_PROCESS_INIT; > > > > > > init_add_i_state(&s.s, r, o); > > > > > > @@ -1852,11 +1872,56 @@ int run_add_p(struct repository *r, enum add_p_mode mode, > > > return -1; > > > } > > > > > > - for (i = 0; i < s.file_diff_nr; i++) > > > - if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) > > > + for (i = 0; i < s.file_diff_nr;) { > > > + if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) { > > > binary_count++; > > > - else if (patch_update_file(&s, s.file_diff + i)) > > > - break; > > > + i++; > > > + continue; > > > + } > > > + else { > > > + patch_update_result = patch_update_file(&s, s.file_diff + i); > > > + if (patch_update_result == 0) { > > > + i++; > > > + continue; > > > + } > > > + if (patch_update_result == 1) > > > + break; > > > + if (patch_update_result == 2) { > > > + i--; > > > + continue; > > > + } > > > + } > > > + } > > > + for (i = 0; i < s.file_diff_nr; i++) { > > > + > > > + /* Any hunk to be used? */ > > > + for (j = 0; j < s.file_diff[i].hunk_nr; j++) > > > + if (s.file_diff[i].hunk[j].use == USE_HUNK) > > > + break; > > > + > > > + if (j < s.file_diff[i].hunk_nr || > > > + (!s.file_diff[i].hunk_nr && s.file_diff[i].head.use == USE_HUNK)) { > > > + /* At least one hunk selected: apply */ > > > + strbuf_reset(&s.buf); > > > + reassemble_patch(&s, s.file_diff + i, 0, &s.buf); > > > + > > > + discard_index(s.s.r->index); > > > + if (s.mode->apply_for_checkout) > > > + apply_for_checkout(&s, &s.buf, > > > + s.mode->is_reverse); > > > + else { > > > + setup_child_process(&s, &cp, "apply", NULL); > > > + strvec_pushv(&cp.args, s.mode->apply_args); > > > + if (pipe_command(&cp, s.buf.buf, s.buf.len, > > > + NULL, 0, NULL, 0)) > > > + error(_("'git apply' failed")); > > > + } > > > + if (repo_read_index(s.s.r) >= 0) > > > + repo_refresh_and_write_index(s.s.r, REFRESH_QUIET, 0, > > > + 1, NULL, NULL, NULL); > > > + } > > > + > > > + } > > > > One upside of having "git apply" at the end of patch_update_file() > > is that you can "^C" out of "git add -p" or your terminal connection > > can be cut off, after dealing with hunks in a few early files, and > > these early part of your work that you have already done are already > > reflected to the working tree files. By hoisting the logic to the > > caller, this is making the update all-or-none, which is good in > > transactional systems, but can make a horrible experience for an > > interactive use where you make progress while thinking. > > > > So I am not yet convinced if this change makes sense---it could be > > because of the lack of justification for this change. > > What I observed after adding the '>' and '<' options is that if a user chooses > to use a hunk A in file 1, and then goes to file 2 with '>', comes back to > file 1 with '<', and decides on hunk A to skip it instead, because > patch_update_file() has > applied the file with the hunk the user initially decided to use > before proceeding to file > 2 with '>', coming back to redecide and say skip does not apply the > latest decision > and when you check the index, the file with the hunks which the user > initially decided to > use but changed to skip is present in the index. > > But if the user initially decided to skip a hunk in a file, goes to > the next file with '>' > and back to the first file, changes the decision on the hunk to use, > it applies the patch > with the hunk because the hunk was not initially selected when the > patch was applied. > But if he now goes away and comes back to the file a third time and > chooses to skip the > hunk, then quits with 'q', because he had selected to use the hunk the > second time, > choosing skip again will not work. > > So basically, initially choosing to use a hunk in a file, going to > another file and coming > back to this file then choosing to skip it does not register the > latest skip decision > on that hunk. > > That was why I decided to do it this way. > I will appreciate a better suggestion from you > > Thanks > Abraham. Hello Junio, thank you for your review. Here I explain my decision to move the "git apply" in patch_update_file() to the caller. Does it sound like a valid reason to make the move? Thanks Abraham