add-patch: Allow reworking with a file after deciding on its hunks

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.

Quoted 4 lines · Abraham Samuel Adekunle

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.

Quoted 2 lines

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

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1452

Quoted 3 lines

add-patch.c +1 -1

-     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?

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1508

Quoted 5 lines

add-patch.c +4 -0

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

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.

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.

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.

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1555

Quoted 5 lines

              strbuf_reset(&s->buf);
              if (file_diff->hunk_nr) {

add-patch.c +1 -1

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

add-patch.c +0 -0

@@ -1441,6 +1443,17 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
 	return false;
 }
 

add-patch.c +6 -3

 	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");
 			}

add-patch.c +5 -1

 			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) {

add-patch.c +6 -16

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

add-patch.c +40 -4

+	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);

add-patch.c +0 -0

 	if (s.file_diff_nr == 0)
 		err(&s, _("No changes."));

add-patch.c N_("j - go to the next undecided hunk, roll over at the bottom\n" · line 1423

Quoted 11 lines · Abraham Samuel Adekunle

add-patch.c +2 -0

index 173a53241e..edb2fab3fd 100644
@@ -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.

Quoted 5 lines

add-patch.c +0 -0

@@ -1441,6 +1443,17 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
      return false;
 }
 

add-patch.c static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx) · line 1455

Quoted 10 lines

add-patch.c +10 -0

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

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1451

Quoted 9 lines

add-patch.c +0 -1

 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???

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?

Quoted 33 lines

add-patch.c +6 -3

      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");
                      }

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1575

Quoted 10 lines

add-patch.c +10 -0

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

Quoted 26 lines

add-patch.c +10 -1

                      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;

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1660

Quoted 5 lines

add-patch.c +5 -0

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

Quoted 35 lines

add-patch.c +6 -21

+                     } 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"));

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1803

Quoted 5 lines

add-patch.c +0 -5

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

add-patch.c int run_add_p(struct repository *r, enum add_p_mode mode, · line 1835

Quoted 11 lines

add-patch.c +2 -1

      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?

Quoted 53 lines

add-patch.c +40 -4

+     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);

add-patch.c int run_add_p(struct repository *r, enum add_p_mode mode, · line 1924

Quoted 10 lines

add-patch.c +10 -0

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

Quoted 19 lines · Junio C Hamano

add-patch.c N_("j - go to the next undecided hunk, roll over at the bottom\n" · line 1423

Quoted 11 lines · Abraham Samuel Adekunle

add-patch.c +2 -0

index 173a53241e..edb2fab3fd 100644
@@ -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 5 lines

Quoted 5 lines

add-patch.c +0 -0

@@ -1441,6 +1443,17 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
      return false;
 }
 

Quoted 16 lines

add-patch.c static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx) · line 1455

Quoted 10 lines

add-patch.c +10 -0

+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 14 lines

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1451

Quoted 9 lines

add-patch.c +0 -1

 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

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

Quoted 48 lines

Quoted 33 lines

add-patch.c +6 -3

      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");
                      }

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1575

Quoted 10 lines

add-patch.c +10 -0

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

Quoted 3 lines

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 26 lines

Quoted 26 lines

add-patch.c +10 -1

                      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;

Quoted 8 lines

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1660

Quoted 5 lines

add-patch.c +5 -0

+                     }
+             } 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 35 lines

Quoted 35 lines

add-patch.c +6 -21

+                     } 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"));

Quoted 8 lines

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1803

Quoted 5 lines

add-patch.c +0 -5

-             }
-             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 15 lines

add-patch.c int run_add_p(struct repository *r, enum add_p_mode mode, · line 1835

Quoted 11 lines

add-patch.c +2 -1

      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 73 lines

Quoted 53 lines

add-patch.c +40 -4

+     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);

add-patch.c int run_add_p(struct repository *r, enum add_p_mode mode, · line 1924

Quoted 10 lines

add-patch.c +10 -0

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

Quoted 2 lines

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?

Quoted 4 lines · Samuel 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.

Quoted 1 line

Does it sound like a valid reason to make the move?

I am not sure, but as long as this is an optional feature, users can choose not to opt in if they do not like the new "all or none" semantics, I guess.

Thanks.

Quoted 11 lines

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.

Quoted 11 lines · Samuel Abraham

Quoted 11 lines

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.

I am not sure if I would like the end result or rather prefer your "all-or-none", so please do not take this as "here is a better way to implement it" suggestion.

But you should be able to keep the current semantics, if you wanted to, even if you apply the chosen hunks when you switch files, like the original code has been doing forever since it was written. You know which hunks you applied, so after applying before moving on to the next file, you can drop these hunks from the list of hunks to be decided for application. When the user comes back to the current file to decide on other hunks, you know that the already used hunks would get in the way, so why keep them?

Having said that, I think the all-or-none mode may be handy if one makes the current working tree dirty with many little unrelated and insignificant changes and the only way to make sense is to see the "git diff --cached" output after adding some and leaving others, at least in the way some people work. I usually am very incremental when doing "git add -p", in that while using the command in one terminal, I run "git diff --cached" to see if I added unwanted things by mistake and "git diff" to see if I left out necessary things, so I would probably not be using the mode. But that is just my hunch without using the new interface long enough.

Quoted 4 lines

I am not sure if I would like the end result or rather prefer your "all-or-none", so please do not take this as "here is a better way to implement it" suggestion.

Quoted 8 lines

But you should be able to keep the current semantics, if you wanted to, even if you apply the chosen hunks when you switch files, like the original code has been doing forever since it was written. You know which hunks you applied, so after applying before moving on to the next file, you can drop these hunks from the list of hunks to be decided for application. When the user comes back to the current file to decide on other hunks, you know that the already used hunks would get in the way, so why keep them?

Yes thank you so much for suggesting this approach.

Quoted 13 lines · Samuel Abraham

Quoted 12 lines

I am not sure if I would like the end result or rather prefer your "all-or-none", so please do not take this as "here is a better way to implement it" suggestion.

But you should be able to keep the current semantics, if you wanted to, even if you apply the chosen hunks when you switch files, like the original code has been doing forever since it was written. You know which hunks you applied, so after applying before moving on to the next file, you can drop these hunks from the list of hunks to be decided for application. When the user comes back to the current file to decide on other hunks, you know that the already used hunks would get in the way, so why keep them?

Quoted 1 line

Yes thank you so much for suggesting this approach.

Not so fast. I explicitly said I am *NOT* suggesting anything.

And thinking about it more, I do not think it makes any sense to do anything other than "all-or-none" when the command is working in your new "you can move to different files before you decide on all hunks in the current file" mode (which I think we agreed to make it an optional mode). Why? After deciding yes, no, no among 5 hunks in the first file (leaving the hunks #4 and #5 undecided), you jump to the second file, do something there, and imagine that you come back. If we drop the alrady applied hunks like the suggestion, which I did not make ;-),

we'd then give you four hunks (as hunk #1
has been already applied), and even though you have already decided not to use hunks #2 and #3, you *can* revisit them with "J" or "K", change your mind and use them if you wanted to. But it is too late for the hunk #1. It looks utterly inconsistent if you cannot change your mind on hunk #1 but can on hunks #2 and #3 and it reduces the usefulness of "you do not have to decide right now and visit other files before you do so" mode.

Thanks.

If there is only one file, neither of the options will be available, if we are in the second of three or more file, both '<' and '>' will be available and if we are at the last file, only '<' will be available.

This will enable simultaneous hunk decisions between between files. After all decisions have been made in a file, a prompt shows which asks "All hunks decided. What now?" that allows reworking with the file, moving to the next or previous file as the case may be.

The decision to use 'q' as a submit is because after some or all the decisions have been made in a file, 'q' submits them as is even though in the help_patch_text it say q will not stage the current hunk and all hunks after it.

Quoted 4 lines · Abraham Samuel Adekunle

If there is only one file, neither of the options will be available, if we are in the second of three or more file, both '<' and '>' will be available and if we are at the last file, only '<' will be available.

An obvious alternative would be to treat the files as a ring, going next from the last one would take you to the first one, etc., but I think what you described is just as good.

Quoted 5 lines

This will enable simultaneous hunk decisions between between files. After all decisions have been made in a file, a prompt shows which asks "All hunks decided. What now?" that allows reworking with the file, moving to the next or previous file as the case may be.

I forgot to mention this in the previous review, but this would be a change that existing users may be surprised by. We _might_ need to introduce a flag to enable this as a new and optional feature.

Quoted 5 lines

The decision to use 'q' as a submit is because after some or all the decisions have been made in a file, 'q' submits them as is even though in the help_patch_text it say q will not stage the current hunk and all hunks after it.

The users do need to _knowingly_ leave some hunks undecided and apply what they already decided to use, and I think 'q' is an appropriate option to use. It is what the current system does, and I do not think it changes with this new feature.

When using the interactive add, reset, stash or checkout machinery, we do not have the option of reworking with a file because the session automatically advances to the next file or ends if we have just one file, immediately all hunks in a file are decided on.

Introduce the flag "--rework-with-file" when interactively selecting patches with the '--patch' option, which does not auto advance, thereby allowing users the option to rework with files.
This ensures the current auto-advance method stays as the default method.

Quoted 4 lines · Abraham Samuel Adekunle

When using the interactive add, reset, stash or checkout machinery, we do not have the option of reworking with a file because the session automatically advances to the next file or ends if we have just one file, immediately all hunks in a file are decided on.

The last part of the sentence after the last comma does not read very well, at least to me.

We recommend to fold lines in such a way that after a few e-mail exchange and quoting it will still stay within 80-column, so a practical fill-column value lies somewhere around ~70. Your lines a slightly longer.

Quoted 5 lines

Introduce the flag "--rework-with-file" when interactively selecting patches with the '--patch' option, which does not auto advance, thereby allowing users the option to rework with files.
This ensures the current auto-advance method stays as the default method.

OK. There may be suggestions for better option names from others; I do not think of any right now.

add-interactive.h line 10

Quoted 11 lines

add-interactive.h +1 -0

index da49502b76..aef2feca56 100644
@@ -6,9 +6,10 @@
 struct add_p_opt {
      int context;
      int interhunkcontext;
+     int no_auto_advance;
 };

We add a new risk of double-negation confusion, e.g.,

if (!opt->no_auto_advance)
... do the auto-advance thing ...

where it may be easier to follow if it were written

if (opt->auto_advance)
... do the auto-advance thing ...

Would it make it harder to arrange the code if we made this member "auto_advance" that defaults to "true"? We have ADD_P_OPT_INIT that everybody is supposed to call already, like this

add-interactive.h line 12

Quoted 2 lines

add-interactive.h +1 -1

-#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1 }
+#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1, .no_auto_advance = 0 }

so I do not imagine it would be too much hassle.

add-interactive.h struct add_i_state { · line 33

Quoted 8 lines

add-interactive.h +1 -1

@@ -28,7 +29,7 @@ struct add_i_state {
 
      int use_single_key;
      char *interactive_diff_filter, *interactive_diff_algorithm;
-     int context, interhunkcontext;
+     int context, interhunkcontext, no_auto_advance;
 };

Likewise.

builtin/add.c static struct option builtin_add_options[] = { · line 260

Quoted 11 lines

builtin/add.c +2 -0

index 32709794b3..408827cf54 100644
@@ -256,6 +256,8 @@ static struct option builtin_add_options[] = {
      OPT_GROUP(""),
      OPT_BOOL('i', "interactive", &add_interactive, N_("interactive picking")),
      OPT_BOOL('p', "patch", &patch_interactive, N_("select hunks interactively")),
+     OPT_BOOL(0, "rework-with-file", &add_p_opt.no_auto_advance,
+              N_("rework with files when selecting hunks interactively")),

Likewise.

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1664

Quoted 9 lines · Abraham Samuel Adekunle

add-patch.c +8 -1

-             } else if (s->answer.buf[0] == 'K') {
+             } else if (s->s.no_auto_advance && s->answer.buf[0] == '>') {
+                     if (permitted & ALLOW_GOTO_NEXT_FILE) {
+                             ret = NEXT_FILE;
+...
+                             continue;
+                     }
+             }
+             else if (s->answer.buf[0] == 'K') {

This funny-looking diff is a sign that the coding guideline was followed in the preimage but not in the postimage, by splitting the "} else if (condition) {" into two lines for 'K'.

add-patch.c +7 -0

+			ret = patch_update_file(&s, s.file_diff + i);
+			if (ret == NEXT_FILE) {
+				if (s.s.no_auto_advance && i == s.file_diff_nr - 1)
+					i = 0;
+				else
+					i++;
+				continue;

add-patch.c int run_add_p(struct repository *r, enum add_p_mode mode, · line 1904

Quoted 6 lines · Abraham Samuel Adekunle

add-patch.c +5 -0

+     for (i = 0; i < s.file_diff_nr;) {
+             if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
                      binary_count++;
+                     i++;
+                     continue;
+             }

This "continue" is a commonly seen good trick to avoid having the nesting go too deep. As we know the case where the condition holds have already been dealt with and moved to the next iteration at this point, we can ...

Quoted 1 line

+ else {

... omit this extra "else" block and write what is inside for everybody (not just "those who did not pass the if condition above", which is what "else" tells us).

Quoted 8 lines

add-patch.c +7 -0

+                     ret = patch_update_file(&s, s.file_diff + i);
+                     if (ret == NEXT_FILE) {
+                             if (s.s.no_auto_advance && i == s.file_diff_nr - 1)
+                                     i = 0;
+                             else
+                                     i++;
+                             continue;

add-patch.c int run_add_p(struct repository *r, enum add_p_mode mode, · line 1922

Quoted 10 lines

add-patch.c +10 -0

+                     }
+                     if (ret == QUIT)
+                             break;
+                     if (s.s.no_auto_advance && ret == PREVIOUS_FILE) {
+                             if (i == 0)
+                                     i = s.file_diff_nr - 1;
+                             else
+                                     i--;
+                             continue;
+                     }

The asymmetry between next/quit and prev feels curious.

The patch_update_file() helper returns QUIT when the user tells us to (regardless of auto-advance setting), PREVIOUS when '<' is given but that is only possible with auto-advance disabled, and NEXT in all other cases. The check inside the NEXT case for auto-advance is to decide if we want to overflow 'i' beyond file_diff_nr to complete the session, or we want to wrap-around back to the first file.

But ret can be PREV only under auto-advance disabled, so the check there feels totally redundant.

And we want to treat the list of files as a ring buffer only when auto-advance is set to false. This may work in practice but the logic feels convoluted.

The patch_update_file() knows how many files there are to decide if we want to offer '<' and '>'. It also knows the file index within the file_diff_nr for the file it is handling. I wonder if it should do a bit more with its return value to help the caller? E.g., perhaps it can return the next 'i' if it wants the caller to advance (and decide to do the ring-buffer if needed), or if it wants to tell the caller that everything is done by returning some sentinel value (e.g., -1)? Then this part of the caller can just be

if ((i = patch_update_file(&s, i)) < 0)
break; /* all done */

perhaps?

By the way, I just noticed that the new local variable you added to patch_update_file() is called "ret" but that is hiding a different variable "int ret" that is used to handle the '/' command. It should be renamed to avoid the name collision.

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1594

Quoted 7 lines · Abraham Samuel Adekunle

add-patch.c +3 -0

@@ -1566,11 +1589,14 @@ static int patch_update_file(struct add_p_state *s,
                                              : 1));
              printf(_(s->mode->prompt_mode[prompt_mode_type]),
                     s->buf.buf);
+             if (s->s.no_auto_advance && all_decided)
+                     printf(_("\n%s All hunks decided. What now? "),
+                             s->s.prompt_color);

This gives an ordinary prompt for the hunk and then another one after it if we notice everything has been decided. I am wondering if it wants to be more like

if (!s->auto_advance && all_decided)
say What now?
else
ask the usual

?

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1594

Quoted 7 lines · Abraham Samuel Adekunle

add-patch.c +3 -0

@@ -1566,11 +1589,14 @@ static int patch_update_file(struct add_p_state *s,
                                              : 1));
              printf(_(s->mode->prompt_mode[prompt_mode_type]),
                     s->buf.buf);
+             if (s->s.no_auto_advance && all_decided)
+                     printf(_("\n%s All hunks decided. What now? "),
+                             s->s.prompt_color);

This gives an ordinary prompt for the hunk and then another one after it if we notice everything has been decided. I am wondering if it wants to be more like

if (!s->auto_advance && all_decided)
say What now?
else
ask the usual

?

Quoted 15 lines · Junio C Hamano

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1594

Quoted 7 lines · Abraham Samuel Adekunle

add-patch.c +3 -0

@@ -1566,11 +1589,14 @@ static int patch_update_file(struct add_p_state *s,
                                              : 1));
              printf(_(s->mode->prompt_mode[prompt_mode_type]),
                     s->buf.buf);
+             if (s->s.no_auto_advance && all_decided)
+                     printf(_("\n%s All hunks decided. What now? "),
+                             s->s.prompt_color);

This gives an ordinary prompt for the hunk and then another one after it if we notice everything has been decided. I am wondering if it wants to be more like

Quoted 6 lines

if (!s->auto_advance && all_decided)
say What now?
else
ask the usual

?

Hello Junio
Please just a small curiosity.

If I do it this way, the user will not be able to see the options available once they have decided on all hunks and want to rework the file. The options for a hunk will not be visible if they navigate with say K or J and want to change decisions on a hunk.
They will always be greeted with What now? without the available options.

Quoted 23 lines · Samuel Abraham

Quoted 15 lines · Junio C Hamano

add-patch.c static int patch_update_file(struct add_p_state *s, · line 1594

Quoted 7 lines · Abraham Samuel Adekunle

add-patch.c +3 -0

@@ -1566,11 +1589,14 @@ static int patch_update_file(struct add_p_state *s,
                                              : 1));
              printf(_(s->mode->prompt_mode[prompt_mode_type]),
                     s->buf.buf);
+             if (s->s.no_auto_advance && all_decided)
+                     printf(_("\n%s All hunks decided. What now? "),
+                             s->s.prompt_color);

This gives an ordinary prompt for the hunk and then another one after it if we notice everything has been decided. I am wondering if it wants to be more like

Quoted 6 lines

if (!s->auto_advance && all_decided)
say What now?
else
ask the usual

?

Quoted 8 lines

Hello Junio
Please just a small curiosity.

If I do it this way, the user will not be able to see the options available once they have decided on all hunks and want to rework the file. The options for a hunk will not be visible if they navigate with say K or J and want to change decisions on a hunk.
They will always be greeted with What now? without the available options.

Ah, OK.

But then after deciding on all hunks and not telling the prompt to move to another file, the user will keep seeing this extra line of prompt?

It somehow smells like a waste of a whole line just to remind the user that all hunks in the file have now been decided.

There was a separate topic that added "(was: [yn])" to the prompt when the prompt asks about a hunk that already has been decided on. As we only need a single bit "all hunks decided", can we do something similar, I wonder? At the beginning of the main prompt, we show which of the N available hunks we are currently at, e.g.,

(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?

Perhaps we can add a third number to indicate how many of the available hunks the user has already decided, or something, that can be used to avoid this wasted line? Or is it a good thing that we are loud in this case using a whole line to remind the user that it may be time to move on? I dunno.

Quoted 6 lines

There was a separate topic that added "(was: [yn])" to the prompt when the prompt asks about a hunk that already has been decided on. As we only need a single bit "all hunks decided", can we do something similar, I wonder? At the beginning of the main prompt, we show which of the N available hunks we are currently at, e.g.,

Quoted 7 lines

(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?

Perhaps we can add a third number to indicate how many of the available hunks the user has already decided, or something, that can be used to avoid this wasted line? Or is it a good thing that we are loud in this case using a whole line to remind the user that it may be time to move on? I dunno.

I thought of a suggestion where after deciding on all hunks in the file, the user
will be able to see the "what now prompt", the options for the current hunk and also the previous decision on the hunk since at this point, all the hunks would have been decided on.

I tried something like

What now? (was: n) [y,n,q,a,d,s,e,>,<,p,P,?]?

This does not show the number of the hunk we are currently at and the "Stage this hunk" since the decision had been made initially but the "whatnow" prompt still provides a chance to change the decision, while showing the previous
decision on the hunk by asking "What now?" instead.
The options have the default [y,n,q,a,d] and the remaining options are populated from the permit set for the hunk. SO the user can still carry out the normal actions on
the hunk.

Quoted 24 lines · Samuel Abraham

Quoted 6 lines

There was a separate topic that added "(was: [yn])" to the prompt when the prompt asks about a hunk that already has been decided on. As we only need a single bit "all hunks decided", can we do something similar, I wonder? At the beginning of the main prompt, we show which of the N available hunks we are currently at, e.g.,

Quoted 7 lines

(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?

Perhaps we can add a third number to indicate how many of the available hunks the user has already decided, or something, that can be used to avoid this wasted line? Or is it a good thing that we are loud in this case using a whole line to remind the user that it may be time to move on? I dunno.

I thought of a suggestion where after deciding on all hunks in the file, the user
will be able to see the "what now prompt", the options for the current hunk and also the previous decision on the hunk since at this point, all the hunks would have been decided on.

I tried something like

What now? (was: n) [y,n,q,a,d,s,e,>,<,p,P,?]?

Quoted 9 lines

This does not show the number of the hunk we are currently at and the "Stage this hunk" since the decision had been made initially but the "whatnow" prompt still provides a chance to change the decision, while showing the previous
decision on the hunk by asking "What now?" instead.
The options have the default [y,n,q,a,d] and the remaining options are populated from the permit set for the hunk. SO the user can still carry out the normal actions on
the hunk.

I like the compactness of that myself, but I have to say that the end-users may feel lost and utterly confused with the distinction, if they are left without being explained why we switch between "Stage this" (which by the way changes phrasing depending on what you are doing) and "What now".

Is it so important to indicate that everything in the hunk has been decided? They'd lose 'j' 'k' when there no longer remains undecided ones, and every hunk they revisit with 'J' or 'K' would say (was: X), which may be a clue enough that they are done with the file, and when they really really wanted to make sure, perhaps they can type '?' and that help can spend a line to say "Out of 8 hunks, you have already decided to use 3 hunks, and skip 5 hunks" or something?

I dunno.

Subject: Re: [PATCH v3 3/3] add-patch: Allow proper 'git apply' when using the --rework-with-file flag

add-patch.c +17 -0

+	for (j = 0; j < file_diff->hunk_nr; j++)
+		if (file_diff->hunk[j].use == USE_HUNK)
+			break;
+
+	if (j < 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);

Quoted 1 line · Abraham Samuel Adekunle

Subject: Re: [PATCH v3 3/3] add-patch: Allow proper 'git apply' when using the --rework-with-file flag

Style. Downcase "Allow". Applies to [2/3].

Avoid "proper" as it is not obvious to everybody what you find proper and why you find it proper. Applies to any value-judgement adjective.

Subject: [PATCH v3 3/3] add-patch: allow all-or-none application of a patch

or something?

add-patch.c N_("j - go to the next undecided hunk, roll over at the bottom\n" · line 1430

Quoted 7 lines

add-patch.c +6 -0

+static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
+{
+     struct child_process cp = CHILD_PROCESS_INIT;
+     size_t j;
+
+             /* Any hunk to be used? */

Funny indentaion?

Quoted 24 lines

add-patch.c +23 -0

+     for (j = 0; j < file_diff->hunk_nr; j++)
+             if (file_diff->hunk[j].use == USE_HUNK)
+                     break;
+
+     if (j < 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,

add-patch.c N_("j - go to the next undecided hunk, roll over at the bottom\n" · line 1457

Quoted 4 lines

add-patch.c +4 -0

+                                                     1, NULL, NULL, NULL);
+     }
+
+}

I suspect that the extraction of this helper function out of its original place in patch_update_file() should be done in its own patch.

Do we need new tests to cover this new feature?

Abraham Samuel Adekunle (3):
interactive -p: add new --rework-with-file flag to interactive
machinery
add-patch: Allow interfile navigation when selecting hunks
add-patch: Allow proper 'git apply' when using the --rework-with-file
flag

Quoted 6 lines · Abraham Samuel Adekunle

Abraham Samuel Adekunle (3):
interactive -p: add new --rework-with-file flag to interactive
machinery
add-patch: Allow interfile navigation when selecting hunks
add-patch: Allow proper 'git apply' when using the --rework-with-file
flag

By the way, this series is conflicting with your other series that has been in 'next' and is ready to graduate. Perhaps it is time to consider rebasing.

When using the interactive add, reset, stash or checkout machinery, we do not have the option of reworking with a file when selecting hunks, because the session automatically advances to the next file or ends if we have just one file.

Introduce the flag --auto-advance which auto advances by default, when interactively selecting patches with the '--patch' option. However, the --no-auto-advance option does not auto advance, thereby allowing users the option to rework with files.

Signed-off-by Abraham Samuel Adekunle <[email protected]>

---
 add-interactive.c     | 4 ++++
 add-interactive.h     | 5 +++--
 builtin/add.c         | 4 ++++
 builtin/checkout.c    | 7 +++++++
 builtin/reset.c       | 4 ++++
 builtin/stash.c       | 8 ++++++++
 t/t9902-completion.sh | 1 +
 7 files changed, 31 insertions(+), 2 deletions(-)

add-interactive.c +1 -0

index 95ec5a89f8..c3a36cd11f 100644
@@ -64,6 +64,7 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
 	s->r = r;
 	s->context = -1;
 	s->interhunkcontext = -1;
+	s->auto_advance = 1;
 
 	s->use_color_interactive = check_color_config(r, "color.interactive");
 

Quoted 34 lines · Abraham Samuel Adekunle

When using the interactive add, reset, stash or checkout machinery, we do not have the option of reworking with a file when selecting hunks, because the session automatically advances to the next file or ends if we have just one file.

Introduce the flag --auto-advance which auto advances by default, when interactively selecting patches with the '--patch' option. However, the --no-auto-advance option does not auto advance, thereby allowing users the option to rework with files.

Signed-off-by Abraham Samuel Adekunle <[email protected]>

---
 add-interactive.c     | 4 ++++
 add-interactive.h     | 5 +++--
 builtin/add.c         | 4 ++++
 builtin/checkout.c    | 7 +++++++
 builtin/reset.c       | 4 ++++
 builtin/stash.c       | 8 ++++++++
 t/t9902-completion.sh | 1 +
 7 files changed, 31 insertions(+), 2 deletions(-)

add-interactive.c +1 -0

index 95ec5a89f8..c3a36cd11f 100644
@@ -64,6 +64,7 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
      s->r = r;
      s->context = -1;
      s->interhunkcontext = -1;
+     s->auto_advance = 1;
 
      s->use_color_interactive = check_color_config(r, "color.interactive");
 

add-interactive.c void init_add_i_state(struct add_i_state *s, struct repository *r, · line 130

Quoted 6 lines

add-interactive.c +2 -0

@@ -124,6 +125,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
                      die(_("%s cannot be negative"), "--inter-hunk-context");
              s->interhunkcontext = add_p_opt->interhunkcontext;
      }
+     if (!add_p_opt->auto_advance)
+             s->auto_advance = 0;
 }

I am confused. Why do we need above two hunks in this function? Wouldn't it suffice to do

        s->auto_advance = add_p_opt->auto_advance;

in the first hunk, instead of assigning 1 to it?

add-interactive.c struct add_i_state { · line 32

Quoted 9 lines

add-interactive.c +1 -1

 struct add_i_state {
      struct repository *r;

@@ -28,7 +29,7 @@ struct add_i_state {
 
      int use_single_key;
      char *interactive_diff_filter, *interactive_diff_algorithm;
-     int context, interhunkcontext;
+     int context, interhunkcontext, auto_advance;

Please don't do this.

The original is already bad to have two members on the same line, but is tolerated as they represent somewhat related concepts. The auto_advance member has nothing to do with these two.

add-patch.c static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx) · line 1444

Quoted 3 lines · Abraham Samuel Adekunle

add-patch.c +1 -2

-static int patch_update_file(struct add_p_state *s,
-                          struct file_diff *file_diff)
+static ssize_t patch_update_file(struct add_p_state *s, size_t idx)

Why ssize_t? Are we going to handle that many hunks that we do not expect to fit in a platform natural "int" type? If we are not doing anything about "idx" being more than half the type, which apparently is the case ...

add-patch.c static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx) · line 1454

Quoted 11 lines

 {
      size_t hunk_index = 0;
      ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
      struct hunk *hunk;
      char ch;
      struct child_process cp = CHILD_PROCESS_INIT;

add-patch.c +3 -1

-     int colored = !!s->colored.len, quit = 0, use_pager = 0;
+     int colored = !!s->colored.len, use_pager = 0;
      enum prompt_mode_type prompt_mode_type;
+     struct file_diff *file_diff = s->file_diff + idx;
+     ssize_t patch_update_resp = (ssize_t)idx;

... with the cast that is not checked here, wouldn't it make sense to just use the platform natural "int" everywhere? Your code is not "safe" either way. I do not think we expect to handle 2 billion hunks, so even on 32-bit platforms, platform natural "int" should be plenty. Instead of religiously using size_t and ssize_t to count things without extra care, I'd rather see us check the error condition for real, if that is what we really care about (and that can still be done while leaving the codebase cleaner by sticking to the platform natural "int").

Enough ranting. Anyway.

If we really are bothered that we cannot handle 3 billion hunks, we could avoid losing half the number range by returning s->file_diff.file_diff_nr (which is one more than there are elements in s->file_diff[] array) or ((size_t)-1). That would allow us to return size_t from here. I care about this a bit more than "why use size_t when int is perfectly fine", because some platforms that are not quite POSIX can have ssize_t that is not as wide as size_t.