builtin add -p: fix hunk splitting

From: Phillip Wood <[email protected]>

To determine whether a hunk can be split a counter is incremented each time a context line follows an insertion or deletion. If at the end of the hunk the value of this counter is greater than one then the hunk can be split into that number of smaller hunks. If the last hunk in a file ends with an insertion or deletion then there is no following context line and the counter will not be incremented. This case is already handled at the end of the loop where counter is incremented if the last hunk ended with an insertion or deletion. Unfortunately there is no similar check between files (likely because the perl version only ever parses one diff at a time). Fix this by checking if the last hunk ended with an insertion or deletion when we see the diff header of a new file and extend the existing regression test.

Reproted-by SZEDER Gábor <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
 add-patch.c                |  7 ++++++
 t/t3701-add-interactive.sh | 46 ++++++++++++++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 4 deletions(-)

add-patch.c +0 -0

index 8c41cdfe39b..5cea70666e9 100644

add-patch.c +0 -0

 			ALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,
 				   file_diff_alloc);
 			file_diff = s->file_diff + s->file_diff_nr - 1;

t/t3701-add-interactive.sh +0 -0

index 77de0029ba5..94537a6b40a 100755

From: Phillip Wood <[email protected]>

Clean up some test constructs in preparation for extending the tests in the next commit. There are three small changes, I've grouped them together as they're so small it didn't seem worth creating three separate commits.
1 - "cat file | sed expression" is better written as
"sed expression file".
2 - Follow our usual practice of redirecting the output of git
commands to a file rather than piping it into another command.
3 - Use test_write_lines rather than 'printf "%s\n"'.

From: Phillip Wood <[email protected]>

To determine whether a hunk can be split a counter is incremented each time a context line follows an insertion or deletion. If at the end of the hunk the value of this counter is greater than one then the hunk can be split into that number of smaller hunks. If the last hunk in a file ends with an insertion or deletion then there is no following context line and the counter will not be incremented. This case is already handled at the end of the loop where counter is incremented if the last hunk ended with an insertion or deletion. Unfortunately there is no similar check between files (likely because the perl version only ever parses one diff at a time).

Fix this by checking if the last
hunk ended with an insertion or deletion when we see the diff header of a new file and extend the existing regression test.

Quoted 29 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

To determine whether a hunk can be split a counter is incremented each time a context line follows an insertion or deletion. If at the end of the hunk the value of this counter is greater than one then the hunk can be split into that number of smaller hunks. If the last hunk in a file ends with an insertion or deletion then there is no following context line and the counter will not be incremented. This case is already handled at the end of the loop where counter is incremented if the last hunk ended with an insertion or deletion. Unfortunately there is no similar check between files (likely because the perl version only ever parses one diff at a time). Fix this by checking if the last hunk ended with an insertion or deletion when we see the diff header of a new file and extend the existing regression test.

Reproted-by SZEDER Gábor <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
  add-patch.c                |  7 ++++++
  t/t3701-add-interactive.sh | 46 ++++++++++++++++++++++++++++++++++----
  2 files changed, 49 insertions(+), 4 deletions(-)

add-patch.c +0 -0

index 8c41cdfe39b..5cea70666e9 100644
@@ -472,6 +472,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
  			eol = pend;
 

add-patch.c static int parse_diff(struct add_p_state *s, const struct pathspec *ps) · line 481

Quoted 8 lines

add-patch.c +7 -0

  		if (starts_with(p, "diff ")) {
+			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++;

I wondered if factoring out these several "marker == '-' || marker == '+'" cases in parse_diff() into a "is_plus_minus(marker)" was worth it, but probably not.

'+'"

Quoted 8 lines

add-patch.c +0 -0

  			ALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,
  				   file_diff_alloc);
  			file_diff = s->file_diff + s->file_diff_nr - 1;

t/t3701-add-interactive.sh +0 -0

index 77de0029ba5..94537a6b40a 100755

t/t3701-add-interactive.sh test_expect_success 'correct message when there is nothing to do' ' · line 330

Quoted 6 lines

t/t3701-add-interactive.sh +2 -1

@@ -326,7 +326,9 @@ test_expect_success 'correct message when there is nothing to do' '
  test_expect_success 'setup again' '
  	git reset --hard &&
  	test_chmod +x file &&
-	echo content >>file
+	echo content >>file &&
+	test_write_lines A B C D>file2 &&

style nit: "cmd args >file2" not "cmd args>file2"

t/t3701-add-interactive.sh test_expect_success 'setup expected' ' · line 418

Quoted 11 lines

t/t3701-add-interactive.sh +2 -2

@@ -373,8 +411,8 @@ test_expect_success 'setup expected' '
  test_expect_success 'add first line works' '
  	git commit -am "clear local changes" &&
  	git apply patch &&
-	test_write_lines s y y | git add -p file 2>error >raw-output &&
-	sed -n -e "s/^([1-2]\/[1-2]) Stage this hunk[^@]*\(@@ .*\)/\1/" \
+	test_write_lines s y y s y n y | git add -p 2>error >raw-output &&
+	sed -n -e "s/^([1-9]\/[1-9]) Stage this hunk[^@]*\(@@ .*\)/\1/" \
  	       -e "/^[-+@ \\\\]"/p raw-output >output &&
  	test_must_be_empty error &&
  	git diff --cached >diff &&

style/diff nit: maybe worth it to in 1/2 do some version of:

     test_write_lines ... >lines &&
     git ... <lines .. &&
     ...
     sed -n \
     	-e ... \
         -e ... \
         >output

Just to make the diff smaller, i.e. just the "test_write_lines" line would be modified here.

The changes themselves & this series LGTM.

Hi Ævar

Quoted 34 lines · Ævar Arnfjörð Bjarmason

Quoted 32 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

To determine whether a hunk can be split a counter is incremented each time a context line follows an insertion or deletion. If at the end of the hunk the value of this counter is greater than one then the hunk can be split into that number of smaller hunks. If the last hunk in a file ends with an insertion or deletion then there is no following context line and the counter will not be incremented. This case is already handled at the end of the loop where counter is incremented if the last hunk ended with an insertion or deletion. Unfortunately there is no similar check between files (likely because the perl version only ever parses one diff at a time). Fix this by checking if the last hunk ended with an insertion or deletion when we see the diff header of a new file and extend the existing regression test.

Reproted-by SZEDER Gábor <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
  add-patch.c                |  7 ++++++
  t/t3701-add-interactive.sh | 46 ++++++++++++++++++++++++++++++++++----
  2 files changed, 49 insertions(+), 4 deletions(-)

add-patch.c +3 -0

index 8c41cdfe39b..5cea70666e9 100644
@@ -472,6 +472,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
  			eol = pend;
    		if (starts_with(p, "diff ")) {
+			if (marker == '-' || marker == '+')
+				/*
+				 * Last hunk ended in non-context line (i.e. it

Quoted 8 lines

add-patch.c static int parse_diff(struct add_p_state *s, const struct pathspec *ps) · line 481

Quoted 4 lines

add-patch.c +4 -0

+				 * appended lines to the file, so there are no
+				 * trailing context lines).
+				 */
+				hunk->splittable_into++;

I wondered if factoring out these several "marker == '-' || marker ==
'+'" cases in parse_diff() into a "is_plus_minus(marker)" was worth it, but probably not.

Yeah in the end I just factored out this hunk into a new function but I didn't add a function for "marker == '-' || marker ==

Quoted 37 lines

'+'"

Quoted 8 lines

add-patch.c +0 -0

  			ALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,
  				   file_diff_alloc);
  			file_diff = s->file_diff + s->file_diff_nr - 1;

t/t3701-add-interactive.sh +0 -0

index 77de0029ba5..94537a6b40a 100755

t/t3701-add-interactive.sh test_expect_success 'correct message when there is nothing to do' ' · line 330

Quoted 6 lines

t/t3701-add-interactive.sh +2 -1

@@ -326,7 +326,9 @@ test_expect_success 'correct message when there is nothing to do' '
  test_expect_success 'setup again' '
  	git reset --hard &&
  	test_chmod +x file &&
-	echo content >>file
+	echo content >>file &&
+	test_write_lines A B C D>file2 &&

style nit: "cmd args >file2" not "cmd args>file2"

t/t3701-add-interactive.sh test_expect_success 'setup expected' ' · line 418

Quoted 11 lines

t/t3701-add-interactive.sh +2 -2

@@ -373,8 +411,8 @@ test_expect_success 'setup expected' '
  test_expect_success 'add first line works' '
  	git commit -am "clear local changes" &&
  	git apply patch &&
-	test_write_lines s y y | git add -p file 2>error >raw-output &&
-	sed -n -e "s/^([1-2]\/[1-2]) Stage this hunk[^@]*\(@@ .*\)/\1/" \
+	test_write_lines s y y s y n y | git add -p 2>error >raw-output &&
+	sed -n -e "s/^([1-9]\/[1-9]) Stage this hunk[^@]*\(@@ .*\)/\1/" \
  	       -e "/^[-+@ \\\\]"/p raw-output >output &&
  	test_must_be_empty error &&
  	git diff --cached >diff &&

style/diff nit: maybe worth it to in 1/2 do some version of:

     test_write_lines ... >lines &&
     git ... <lines .. &&
     ...
     sed -n \
     	-e ... \
         -e ... \
         >output

Quoted 2 lines

Just to make the diff smaller, i.e. just the "test_write_lines" line would be modified here.

In the end I decided to leave this as is, while refactoring slightly simplifies this patch it makes the previous one bigger and means that would need to be reviewed again.

Quoted 1 line

The changes themselves & this series LGTM.

Thanks

Best Wishes

Phillip

Thanks to Junio and Ævar for their comments on V1. I've updated the commit message and added a helper function as suggested.

Quoted 2 lines · Phillip Wood via GitGitGadget

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.

(quoted diff) +0 -5

@@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 	else if (starts_with(p, "@@ ") ||
 		 (hunk == &file_diff->head &&
 		  (skip_prefix(p, "deleted file", &deleted)))) {
-		if (marker == '-' || marker == '+')
-			/*
-			 * Should not happen; previous hunk did not end
-			 * in a context line? Handle it anyway.
-			 */

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 <[email protected]>
      
       ## 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' '

Thanks to Junio and Ævar for their comments on V1. I've updated the commit message and added a helper function as suggested.

V1 Cover Letter: Fix a small regression in the hunk splitting of the builtin version compared to the perl version. Thanks to Szeder for the easy to follow bug report.

I'm not sure I want to go with your extra changes. I've left some comments on them below

Quoted 13 lines

(quoted diff) +4 -5

@@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 	else if (starts_with(p, "@@ ") ||
 		 (hunk == &file_diff->head &&
 		  (skip_prefix(p, "deleted file", &deleted)))) {
-		if (marker == '-' || marker == '+')
-			/*
-			 * Should not happen; previous hunk did not end
-			 * in a context line? Handle it anyway.
-			 */
+			hunk->splittable_into++;
+		/*
+		 * Should not increment "splittable_into";
+		 * previous hunk did not end in a context

Review static int parse_diff(struct add_p_state *s, const struct pathspec *ps) · line 510

Quoted 5 lines

(quoted diff) +3 -0

+		 * line? Handle it anyway.
+		 */
+		complete_file(marker, &hunk->splittable_into);
  		ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
 			   file_diff->hunk_alloc);

I deliberately left this alone as I think we should probably make this BUG() out instead of silently accepting an invalid diff.

Quoted 16 lines · Phillip Wood

I'm not sure I want to go with your extra changes. I've left some comments on them below

Quoted 13 lines

(quoted diff) +4 -5

@@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
  	else if (starts_with(p, "@@ ") ||
  		 (hunk == &file_diff->head &&
  		  (skip_prefix(p, "deleted file", &deleted)))) {
-		if (marker == '-' || marker == '+')
-			/*
-			 * Should not happen; previous hunk did not end
-			 * in a context line? Handle it anyway.
-			 */
+			hunk->splittable_into++;
+		/*
+		 * Should not increment "splittable_into";
+		 * previous hunk did not end in a context

Quoted 8 lines

Review static int parse_diff(struct add_p_state *s, const struct pathspec *ps) · line 510

Quoted 5 lines

(quoted diff) +3 -0

+		 * line? Handle it anyway.
+		 */
+		complete_file(marker, &hunk->splittable_into);
   		ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
  			   file_diff->hunk_alloc);

I deliberately left this alone as I think we should probably make this BUG() out instead of silently accepting an invalid diff.

As we are reading our own output, I agree that such a data error is a BUG().

In any case, a helper to see if the file ended without post-context is one thing, and a helper that specify what happens after we are done with a single file, before we move on top the next file or after processing the last file, is another thing. The latter may be able to make use of the former, but the latter may want to do more than that in the future.

As complete_file() is about finalizing the processing we have done to the current file, it should be used for that purpose, and nothing else, I think the hunk I see at
https://github.com/git/git/commit/c082176f8c5a1fc1c8b2a93991ca28fd63aae73a
(reproduced below) is simply a nonsense.

Stepping back a bit, though, is this helper really finalizing the current file, or is it finalizing the current hunk? If it were the latter, then its use in the hunk I called "nonsense" above actually makes perfect sense.

There may not be anything other than finalizing
the last hunk when we see the end of a file right now, so we may not need to add a finalize_file() helper right now, and when we need to do something more than finalizing the last hunk, we may need to capture the distinction by adding one.

add-patch.c +2 -2

index 89ffda32b2..6094290c86 100644
@@ -578,8 +578,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
  			    (int)(eol - (plain->buf + file_diff->head.start)),
  			    plain->buf + file_diff->head.start);
 
-		if ((marker == '-' || marker == '+') && *p == ' ')
-			hunk->splittable_into++;
+		if (*p == ' ')
+			complete_file(marker, &hunk->splittable_into);
  		if (marker && *p != '\\')
  			marker = *p;
 

Even if the helper is finalizing the current hunk then I think that "nonsense" hunk would still wrong as it would be calling finalize_hunk() on _every_ context line in the hunk rather than just being called once to finalize the hunk.

Quoted 4 lines

  -		if ((marker == '-' || marker == '+') && *p == ' ')

add-patch.c +2 -1

-			hunk->splittable_into++;
+		if (*p == ' ')
+			complete_file(marker, &hunk->splittable_into);

Quoted 4 lines · Phillip Wood

Even if the helper is finalizing the current hunk then I think that "nonsense" hunk would still wrong as it would be calling finalize_hunk() on _every_ context line in the hunk rather than just being called once to finalize the hunk.

True; this triggers every time we finish reading the common context lines and not at the end of hunk. In any case, I think what we queued looks good for 'next'.

Quoted 4 lines

Quoted 4 lines

  -		if ((marker == '-' || marker == '+') && *p == ' ')

add-patch.c +2 -1

-			hunk->splittable_into++;
+		if (*p == ' ')
+			complete_file(marker, &hunk->splittable_into);

Quoted 10 lines · Junio C Hamano

Quoted 4 lines · Phillip Wood

Even if the helper is finalizing the current hunk then I think that "nonsense" hunk would still wrong as it would be calling finalize_hunk() on _every_ context line in the hunk rather than just being called once to finalize the hunk.

True; this triggers every time we finish reading the common context lines and not at the end of hunk. In any case, I think what we queued looks good for 'next'.

For what it's worth (and as the person who started this side-thread) I agree. This looks good as-is, thanks both!

Quoted 4 lines

Quoted 4 lines

add-patch.c static int parse_diff(struct add_p_state *s, const struct pathspec *ps) · line 582

Quoted 4 lines

  -		if ((marker == '-' || marker == '+') && *p == ' ')

add-patch.c +2 -1

-			hunk->splittable_into++;
+		if (*p == ' ')
+			complete_file(marker, &hunk->splittable_into);

Quoted 19 lines · Junio C Hamano

Quoted 17 lines · Phillip Wood

I'm not sure I want to go with your extra changes. I've left some comments on them below

Quoted 14 lines

(quoted diff) +4 -5

@@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const
 struct pathspec *ps)
   else if (starts_with(p, "@@ ") ||
     (hunk == &file_diff->head &&
  		  (skip_prefix(p, "deleted file", &deleted)))) {
-		if (marker == '-' || marker == '+')
-			/*
-			 * Should not happen; previous hunk did not end
-			 * in a context line? Handle it anyway.
-			 */
+			hunk->splittable_into++;
+		/*
+		 * Should not increment "splittable_into";
+		 * previous hunk did not end in a context

Quoted 8 lines

Quoted 8 lines

Review static int parse_diff(struct add_p_state *s, const

Quoted 5 lines

(quoted diff) +3 -0

+		 * line? Handle it anyway.
+		 */
+		complete_file(marker, &hunk->splittable_into);
     ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
        file_diff->hunk_alloc);

I deliberately left this alone as I think we should probably make this BUG() out instead of silently accepting an invalid diff.

Quoted 2 lines

As we are reading our own output, I agree that such a data error is a BUG().

Quoted 6 lines

In any case, a helper to see if the file ended without post-context is one thing, and a helper that specify what happens after we are done with a single file, before we move on top the next file or after processing the last file, is another thing. The latter may be able to make use of the former, but the latter may want to do more than that in the future.

Quoted 11 lines

As complete_file() is about finalizing the processing we have done to the current file, it should be used for that purpose, and nothing else, I think the hunk I see at
https://github.com/git/git/commit/c082176f8c5a1fc1c8b2a93991ca28fd63aae73a
(reproduced below) is simply a nonsense.

Stepping back a bit, though, is this helper really finalizing the current file, or is it finalizing the current hunk? If it were the latter, then its use in the hunk I called "nonsense" above actually makes perfect sense.

Even if the helper is finalizing the current hunk then I think that "nonsense" hunk would still wrong as it would be calling finalize_hunk() on _every_ context line in the hunk rather than just being called once to finalize the hunk. We could call the function something like update_splittable() but then we'd need to explain why we were calling that function at the start of a diff and at the end of the loop.

Hi Phillip,

first of all: thank you for these patches. I read over them and they have my ACK.

Quoted 21 lines · Junio C Hamano

Quoted 19 lines

Quoted 17 lines · Phillip Wood

I'm not sure I want to go with your extra changes. I've left some comments on them below

Quoted 14 lines

(quoted diff) +4 -5

@@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const
 struct pathspec *ps)
    else if (starts_with(p, "@@ ") ||
      (hunk == &file_diff->head &&
   		  (skip_prefix(p, "deleted file", &deleted)))) {
-		if (marker == '-' || marker == '+')
-			/*
-			 * Should not happen; previous hunk did not end
-			 * in a context line? Handle it anyway.
-			 */
+			hunk->splittable_into++;
+		/*
+		 * Should not increment "splittable_into";
+		 * previous hunk did not end in a context

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review static int parse_diff(struct add_p_state *s, const

Quoted 5 lines

(quoted diff) +3 -0

+		 * line? Handle it anyway.
+		 */
+		complete_file(marker, &hunk->splittable_into);
      ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
         file_diff->hunk_alloc);

I deliberately left this alone as I think we should probably make this BUG() out instead of silently accepting an invalid diff.

FWIW this was overzealous defensive programming on my part. More on that below.

Quoted 2 lines

Quoted 2 lines

As we are reading our own output, I agree that such a data error is a BUG().

Indeed. I was less worried about the output format changing, and more concerned with bugs in my parser ;-)

Although, having said that, I had meant to verify that git add -p cannot be asked to produce and consume diffs with -U0 when I wrote that comment. Now I did that, and I am now confident that there is no way to ask git add -p to generate and use context line-free diffs: we neither add -U<n> in https://github.com/git/git/blob/v2.34.1/add-patch.c#L398-L417
nor do we call the user-facing git diff command that would interpret diff.context, but instead we use git diff-index and git diff-files (which ignore that config setting).

Quoted 6 lines

Quoted 6 lines

In any case, a helper to see if the file ended without post-context is one thing, and a helper that specify what happens after we are done with a single file, before we move on top the next file or after processing the last file, is another thing. The latter may be able to make use of the former, but the latter may want to do more than that in the future.

If you are concerned about the name of the function: maybe a better name would be maybe_increment_splittable_hunk_count(marker).

Quoted 12 lines

Quoted 11 lines

As complete_file() is about finalizing the processing we have done to the current file, it should be used for that purpose, and nothing else, I think the hunk I see at
https://github.com/git/git/commit/c082176f8c5a1fc1c8b2a93991ca28fd63aae73a
(reproduced below) is simply a nonsense.

Stepping back a bit, though, is this helper really finalizing the current file, or is it finalizing the current hunk? If it were the latter, then its use in the hunk I called "nonsense" above actually makes perfect sense.

Quoted 6 lines

Even if the helper is finalizing the current hunk then I think that "nonsense" hunk would still wrong as it would be calling finalize_hunk() on _every_ context line in the hunk rather than just being called once to finalize the hunk. We could call the function something like update_splittable() but then we'd need to explain why we were calling that function at the start of a diff and at the end of the loop.

Right. The point of this check is to see whether we missed counting a splittable hunk. Then it makes more sense to call it at the beginning of a file, at the end of a file _and_ at a context line.

Having said all that, I am really fine with what landed in next.

Thank you,
Dscho