Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

>
> On Tue, Jan 27, 2026 at 9:48 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> 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.