On Tue, Jan 11 2022, Phillip Wood via GitGitGadget wrote: > Thanks to Junio and Ævar for their comments on V1. I've updated the commit > message and added a helper function as suggested. This v2 LGTM as far as the functionality of the end-state is concerned. As a remaining nit the complete_file() helper you introduce in 2/2 changes 2/4 places that increment "hunk->splittable+into". I grabbed this PR and came up with this amendmend to it which adds a 2/3 step that converts 3/3 of them, followed by adding the 4th user in your 2/2 (now patch 3/3): https://github.com/git/git/compare/master...avar:phillipwood-avar/wip/add-p-fix-hunk-splitting-v2.1 It changes nothing as far as the end-state is concerned, but I think it makes this easier to read & follow. The actual behavior change becomes a one-line addition to add-patch.c, instead of being mixed up with the refactoring of adding the new helper. If you'd like to pick that up & run with it as a v3 that's fine by me, and if not that's also fine :) Just a suggestion. A range-diff between your v2 here and that linked-to phillipwood-avar/wip/add-p-fix-hunk-splitting-v2.1: 1: cc8639fc29d = 1: 34392397f04 t3701: clean up hunk splitting tests -: ----------- > 2: c082176f8c5 add-file.c: use static helper to check marker == +|- 2: b698989e265 ! 3: defca0baba4 builtin add -p: fix hunk splitting @@ Commit message Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> ## add-patch.c ## -@@ add-patch.c: static int is_octal(const char *p, size_t len) - return 1; - } - -+static void complete_file(char marker, struct hunk *hunk) -+{ -+ if (marker == '-' || marker == '+') -+ /* -+ * Last hunk ended in non-context line (i.e. it -+ * appended lines to the file, so there are no -+ * trailing context lines). -+ */ -+ hunk->splittable_into++; -+} -+ - static int parse_diff(struct add_p_state *s, const struct pathspec *ps) - { - struct strvec args = STRVEC_INIT; @@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps) eol = pend; if (starts_with(p, "diff ")) { -+ complete_file(marker, hunk); ++ complete_file(marker, &hunk->splittable_into); ALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1, file_diff_alloc); file_diff = s->file_diff + s->file_diff_nr - 1; -@@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps) - file_diff->hunk->colored_end = hunk->colored_end; - } - } -- -- if (marker == '-' || marker == '+') -- /* -- * Last hunk ended in non-context line (i.e. it appended lines -- * to the file, so there are no trailing context lines). -- */ -- hunk->splittable_into++; -+ complete_file(marker, hunk); - - /* non-colored shorter than colored? */ - if (colored_p != colored_pend) { ## t/t3701-add-interactive.sh ## @@ t/t3701-add-interactive.sh: test_expect_success 'correct message when there is nothing to do' '
