[Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line

[Tanga Joris <tangajoris@gmail.com>]

When a "diff --git" line is followed by a line that is not a git
header line, such as a blank line or an indented "new file mode"
line, parse_git_diff_header() stops right after the "diff --git"
line. find_header() then treats it as an empty git header and keeps
looking, as it has done since 206de27efa (git-apply: ignore empty git
headers, 2005-06-12). It finds the "--- /dev/null" and "+++ b/<path>"
lines and reads the rest as a traditional patch.

But parse_git_diff_header() has already set old_name and new_name
from the "diff --git" line. These names stay in the patch.
parse_traditional_patch() sets is_new for "--- /dev/null", but it
does not clear old_name. So "git apply --check" dies on the assertion
that 5c47f4c6e7 (builtin-apply: accept patch to an empty file,
2008-05-17) added to check_preimage():

  check_preimage: Assertion `patch->is_new <= 0' failed.

The stale new_name causes the same crash when a deletion with such a
header is applied with "git apply -R". The stale names also break an
empty "diff --git" section that is followed by another section. They
do not match the names in the next section, so "git apply" fails with
a "bad git-diff" error.

Free the names when the "diff --git" line is not used. A new file is
then created, and a deletion can be reversed. An empty section is
ignored, as 206de27efa intended. As with any traditional patch, the
"new file mode" line is not used, so a new file gets the default mode.

parse_git_diff_header() has also set is_toplevel_relative. Keep it,
so that the paths stay relative to the top of the repository, as
before.

Reported-by: Gingrspacecadet <jackrawlings2011@gmail.com>
Signed-off-by: Tanga Joris <tangajoris@gmail.com>
---
This fixes the crash reported in [1]. While looking into it, I found
two more ways to trigger the same assertion: an indented "new file
mode" line, which this patch also fixes, and a "---" line before
"new file mode", which 2/2 handles. Git 2.43 crashes on all three.

I based this on maint (v2.56.0), since the crash is in released
versions. It merges cleanly with master, next and seen.

[1] https://lore.kernel.org/git/CALdwsC1cMKZgRPZA+pBaJe4PPs9q9W0K9CQLmM2bgUCyysTB-w@mail.gmail.com/

 apply.c                    | 10 ++++-
 t/t4133-apply-filenames.sh | 90 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index f00b7ba4d3..b56b363963 100644
--- a/apply.c
+++ b/apply.c
@@ -1651,8 +1651,16 @@ static int find_header(struct apply_state *state,
 								size, patch);
 			if (git_hdr_len < 0)
 				return -128;
-			if (git_hdr_len <= len)
+			if (git_hdr_len <= len) {
+				/*
+				 * Not a git header after all. Do not keep the
+				 * names it took from the "diff --git" line.
+				 */
+				FREE_AND_NULL(patch->def_name);
+				FREE_AND_NULL(patch->old_name);
+				FREE_AND_NULL(patch->new_name);
 				continue;
+			}
 			*hdrsize = git_hdr_len;
 			return offset;
 		}
diff --git a/t/t4133-apply-filenames.sh b/t/t4133-apply-filenames.sh
index 3cab1038cf..727c36e7ea 100755
--- a/t/t4133-apply-filenames.sh
+++ b/t/t4133-apply-filenames.sh
@@ -60,4 +60,94 @@ test_expect_success 'apply diff with old filename missing from headers' '
 	test_grep "lacks filename information" err
 '
 
+test_expect_success 'apply new file with a blank line after "diff --git"' '
+	test_when_finished "rm -f f" &&
+	cat >blank_line.diff <<-\EOF &&
+	diff --git a/f b/f
+
+	new file mode 100644
+	index 0000000..d00491f
+	--- /dev/null
+	+++ b/f
+	@@ -0,0 +1 @@
+	+1
+	EOF
+	git apply --check blank_line.diff &&
+	git apply blank_line.diff &&
+	echo 1 >expect &&
+	test_cmp expect f
+'
+
+test_expect_success 'apply new file with an indented "new file mode" line' '
+	test_when_finished "rm -f f" &&
+	cat >indented_mode.diff <<-\EOF &&
+	diff --git a/f b/f
+	 new file mode 100644
+	index 0000000..d00491f
+	--- /dev/null
+	+++ b/f
+	@@ -0,0 +1 @@
+	+1
+	EOF
+	git apply --check indented_mode.diff &&
+	git apply indented_mode.diff &&
+	echo 1 >expect &&
+	test_cmp expect f
+'
+
+test_expect_success 'reverse a deletion with a blank line after "diff --git"' '
+	test_when_finished "rm -f f" &&
+	cat >blank_line_delete.diff <<-\EOF &&
+	diff --git a/f b/f
+
+	deleted file mode 100644
+	index d00491f..0000000
+	--- a/f
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-1
+	EOF
+	git apply -R --check blank_line_delete.diff &&
+	git apply -R blank_line_delete.diff &&
+	echo 1 >expect &&
+	test_cmp expect f
+'
+
+test_expect_success 'ignore an empty "diff --git" section' '
+	test_when_finished "rm -f g" &&
+	echo 1 >g &&
+	cat >empty_section.diff <<-\EOF &&
+	diff --git a/f b/f
+	diff --git a/g b/g
+	index d00491f..0cfbf08 100644
+	--- a/g
+	+++ b/g
+	@@ -1 +1 @@
+	-1
+	+2
+	EOF
+	git apply --check empty_section.diff &&
+	git apply empty_section.diff &&
+	echo 2 >expect &&
+	test_cmp expect g
+'
+
+test_expect_success 'blank line after "diff --git" keeps paths relative to the top' '
+	test_when_finished "rm -rf sub" &&
+	mkdir sub &&
+	cat >sub/blank_line.diff <<-\EOF &&
+	diff --git a/f b/f
+
+	new file mode 100644
+	index 0000000..d00491f
+	--- /dev/null
+	+++ b/f
+	@@ -0,0 +1 @@
+	+1
+	EOF
+	git -C sub apply blank_line.diff &&
+	test_path_is_missing sub/f &&
+	test_path_is_missing f
+'
+
 test_done

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
-- 
2.43.0