Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting

On Tue, Jan 11 2022, Phillip Wood via GitGitGadget wrote:

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@dunelm.org.uk>
     
      ## 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' '