git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Samuel Abraham <abrahamadekunle50@gmail.com>
Date
Jan 28, 2026, 11:26 UTC
Message-ID
<CADYq+fYeWh0tLEepOGVa=1i9tXZfWaGfyi6H+xUB7rbdQ=t5aQ@mail.gmail.com>
In-Reply-To
<xmqq7bt2g4tl.fsf@gitster.g>
On Tue, Jan 27, 2026 at 9:48 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
>
> 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.

Show 21 quoted lines
>
> > @@ -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.
Show 14 quoted lines
>
> >  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
Show 7 quoted lines
>
> 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.
Show 51 quoted lines
>
> >       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.

Show 34 quoted lines
>
> >                       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.
Show 43 quoted lines
>
> > +                     } 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.
Show 15 quoted lines
>
> >       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
Show 75 quoted lines
>
> > +     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.

Previous: Junio C HamanoNext: Samuel Abraham
Message 8 of 54 in “add-patch: Allow reworking with a file after deciding on its hunks”
  1. 0/1 add-patch: Allow reworking with a file after deciding on its hunksAbraham Samuel Adekunle, Jan 23, 2026
  2. 1/1 add-patch: Allow reworking with a file after deciding on all its hunksAbraham Samuel Adekunle, Jan 23, 2026
  3. Junio C HamanoJan 23, 2026
  4. Samuel AbrahamJan 23, 2026
  5. 0/1 Allow reworking with a file when making hunk decisionsAbraham Samuel Adekunle, Jan 27, 2026
  6. 1/1 Allow reworking with a file after deciding on all its hunksAbraham Samuel Adekunle, Jan 27, 2026
  7. Junio C HamanoJan 27, 2026
  8. Samuel AbrahamJan 28, 2026
  9. Samuel AbrahamJan 30, 2026
  10. Junio C HamanoJan 30, 2026
  11. Samuel AbrahamJan 30, 2026
  12. Junio C HamanoJan 31, 2026
  13. Samuel AbrahamFeb 2, 2026
  14. Junio C HamanoFeb 2, 2026
  15. Samuel AbrahamFeb 3, 2026
  16. Junio C HamanoJan 27, 2026
  17. Samuel AbrahamJan 28, 2026
  18. 0/3 introduce new option `rework-with-file`Abraham Samuel Adekunle, Feb 6, 2026
  19. 1/3 interactive -p: add new `--rework-with-file` flag to interactive machineryAbraham Samuel Adekunle, Feb 6, 2026
  20. Junio C HamanoFeb 6, 2026
  21. Samuel AbrahamFeb 6, 2026
  22. 2/3 add-patch: Allow interfile navigation when selecting hunksAbraham Samuel Adekunle, Feb 6, 2026
  23. Junio C HamanoFeb 6, 2026
  24. Samuel AbrahamFeb 6, 2026
  25. Junio C HamanoFeb 6, 2026
  26. Samuel AbrahamFeb 6, 2026
  27. Junio C HamanoFeb 6, 2026
  28. Samuel AbrahamFeb 6, 2026
  29. Samuel AbrahamFeb 12, 2026
  30. Junio C HamanoFeb 12, 2026
  31. Samuel AbrahamFeb 12, 2026
  32. Junio C HamanoFeb 12, 2026
  33. Samuel AbrahamFeb 12, 2026
  34. 3/3 add-patch: Allow proper 'git apply' when using the --rework-with-file flagAbraham Samuel Adekunle, Feb 6, 2026
  35. Junio C HamanoFeb 6, 2026
  36. Samuel AbrahamFeb 6, 2026
  37. Junio C HamanoFeb 6, 2026
  38. Samuel AbrahamFeb 6, 2026
  39. 0/4 introduce new option `--auto-advance`Abraham Samuel Adekunle, Feb 13, 2026
  40. 1/4 interactive -p: add new `--auto-advance` flagAbraham Samuel Adekunle, Feb 13, 2026
  41. Junio C HamanoFeb 13, 2026
  42. Samuel AbrahamFeb 14, 2026
  43. 2/4 add-patch: modify patch_update_file() signatureAbraham Samuel Adekunle, Feb 13, 2026
  44. Junio C HamanoFeb 13, 2026
  45. Samuel AbrahamFeb 14, 2026
  46. 3/4 add-patch: allow all-or-none application of patchesAbraham Samuel Adekunle, Feb 13, 2026
  47. 4/4 add-patch: allow interfile navigation when selecting hunksAbraham Samuel Adekunle, Feb 13, 2026
  48. 0/4 introduce new option `--auto-advance`Abraham Samuel Adekunle, Feb 14, 2026
  49. 1/4 interactive -p: add new `--auto-advance` flagAbraham Samuel Adekunle, Feb 14, 2026
  50. 2/4 add-patch: modify patch_update_file() signatureAbraham Samuel Adekunle, Feb 14, 2026
  51. 3/4 add-patch: allow all-or-none application of patchesAbraham Samuel Adekunle, Feb 14, 2026
  52. 4/4 add-patch: allow interfile navigation when selecting hunksAbraham Samuel Adekunle, Feb 14, 2026
  53. Junio C HamanoFeb 20, 2026
  54. Samuel AbrahamFeb 21, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.