On Tue, Jan 27, 2026 at 9:48 PM Junio C Hamano [off-list ref] wrote:

Abraham Samuel Adekunle [off-list ref] writes:

quoted

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.

quoted

@@ -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.

quoted

 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.

quoted

      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.

quoted

                      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.

quoted

+                     } 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.

quoted

      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

quoted

+     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.