Git development
 help / color / mirror / Atom feed
* Git crashed on `git apply --check`
@ 2026-10-17 15:17 Gingrspacecadet
  2026-10-24  6:10 ` Tanga Joris
  2026-11-07 19:54 ` [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line Tanga Joris
  0 siblings, 2 replies; 12+ messages in thread
From: Gingrspacecadet @ 2026-10-17 15:17 UTC (permalink / raw)
  To: git

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)
Checking the following patch:
diff --git a/tests/constraint_type_signature_lifting.test
b/tests/constraint_type_signature_lifting.test

new file mode 100644
index 0000000..0000000
--- /dev/null
+++ b/tests/constraint_type_signature_lifting.test
@@ -0,0 +1,40 @@
+name: constraint types in signatures are lifted to generic parameters
+expect: pass
+stop: run
+errors: 0
+
+[source]
+constraint Readable = {
+    fn uint64 Readable.read(Readable *self);
+}
+
+type File = struct {
+    uint64 size;
+};
+
+fn uint64 File.read(File *self) {
+    return self.size;
+}
+
+fn Readable identity(Readable value) {
+    return value;
+}
+
+fn uint64 read_size(Readable file) {
+    return file.read();
+}
+
+fn int64 main() {
+    File file = {42};
+    File copy = identity(file);
+    if (copy.size != 42) return 1;
+    if (read_size(file) != 42) return 2;
+    return 0;
+}
+
+[stdout]
+
+[stderr]
+
+[exit]
+0

What did you expect to happen?
The patch check to pass

What happened instead?
"git: apply.c:3856: check_preimage: Assertion `patch->is_new <= 0' failed.i"

[System Info]
git version:
git version 2.54.0
cpu: x86_64
built from commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
rust: enabled
gettext: enabled
libcurl: 8.19.0
OpenSSL: OpenSSL 3.6.2 7 Apr 2026
zlib-ng: 2.3.3
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1
uname: Linux 7.0.10-arch1-1 #1 SMP PREEMPT_DYNAMIC Sat, 23 May 2026
14:21:20 +0000 x86_64
compiler info: gnuc: 15.2
libc info: glibc: 2.43
$SHELL (typically, interactive shell): /usr/bin/bash


[Enabled Hooks]

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: Git crashed on `git apply --check`
  2026-10-17 15:17 Git crashed on `git apply --check` Gingrspacecadet
@ 2026-10-24  6:10 ` Tanga Joris
       [not found]   ` <CALdwsC11JHUBBTt3e_RFixLYSF0VU1C+mG7bQsO0vV=a68XHNw@mail.gmail.com>
  2026-11-07 19:54 ` [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line Tanga Joris
  1 sibling, 1 reply; 12+ messages in thread
From: Tanga Joris @ 2026-10-24  6:10 UTC (permalink / raw)
  To: jackrawlings2011; +Cc: git

Hi,

Thanks for the report. I was unable to reproduce it with the patch as
quoted (the "diff --git" line is wrapped in your mail), but I can
trigger the same assertion on current master, and on 2.43, when the
"--- /dev/null" line comes before "new file mode" in the header:

    diff --git a/n.txt b/n.txt
    --- /dev/null
    +++ b/n.txt
    new file mode 100644
    @@ -0,0 +1 @@
    +hello

Was your patch handwritten or generated by a tool, and could you
please check whether its header lines are in that order? If you can,
please attach the exact file so that we can confirm it is the same
problem.

Thanks,
Tanga

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Git crashed on `git apply --check`
       [not found]   ` <CALdwsC11JHUBBTt3e_RFixLYSF0VU1C+mG7bQsO0vV=a68XHNw@mail.gmail.com>
@ 2026-10-25 22:56     ` Tanga Joris
  0 siblings, 0 replies; 12+ messages in thread
From: Tanga Joris @ 2026-10-25 22:56 UTC (permalink / raw)
  To: Gingrspacecadet, git

Adding the list back

On Sat, 24 Oct 2026 at 20:08, Gingrspacecadet
<jackrawlings2011@gmail.com> wrote:
>
> hi,
>
> I've attached the patch that faulted.
>
> It was generated by an LLM.

Hello,

Thank you for sending the original patch file. The crash was caused
by a blank line after the diff --git line in the new-file section,
and it is reproducible on master. I am currently working on a fix.

Best,
Tanga

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line
  2026-10-17 15:17 Git crashed on `git apply --check` Gingrspacecadet
  2026-10-24  6:10 ` Tanga Joris
@ 2026-11-07 19:54 ` Tanga Joris
  2026-11-07 19:54   ` [Outreachy PATCH 2/2] apply: reject new and deleted files that have both names Tanga Joris
  2026-11-09 10:52   ` [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line Phillip Wood
  1 sibling, 2 replies; 12+ messages in thread
From: Tanga Joris @ 2026-11-07 19:54 UTC (permalink / raw)
  To: git
  Cc: gitster, jackrawlings2011, christian.couder, siddharthasthana31,
	kaartic.sivaraam, usmanakinyemi202, Tanga Joris

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Outreachy PATCH 2/2] apply: reject new and deleted files that have both names
  2026-11-07 19:54 ` [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line Tanga Joris
@ 2026-11-07 19:54   ` Tanga Joris
  2026-11-09 10:52   ` [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line Phillip Wood
  1 sibling, 0 replies; 12+ messages in thread
From: Tanga Joris @ 2026-11-07 19:54 UTC (permalink / raw)
  To: git
  Cc: gitster, jackrawlings2011, christian.couder, siddharthasthana31,
	kaartic.sivaraam, usmanakinyemi202, Tanga Joris

Git writes "new file mode" or "deleted file mode" before the "---"
and "+++" lines. The "new file mode" line sets is_new. Then
gitdiff_oldname() leaves old_name empty for "--- /dev/null".

When the "---" line comes first, is_new is still 0. So
gitdiff_verify_name() takes the name on that line as old_name. For
"--- /dev/null" this gives "dev/null", because one leading directory
is stripped. The "new file mode" line then sets is_new. The result is
a new file that also has an old name. "git apply --check" dies on it,
on the same assertion in check_preimage(). In the same way,
"+++ /dev/null" before "deleted file mode" gives a deleted file with
a new name. "git apply -R" dies on that one.

d70e9c5c8c (apply: check git diffs for mutually exclusive header
lines, 2017-06-27) made apply report other conflicting header lines,
instead of leaving them to an assertion. Do the same here. After the
header is read, reject a new file that has an old name. Reject a
deleted file that has a new name too.

Patches made by Git are therefore not affected. A patch with
"+++ /dev/null" before "deleted file mode" used to apply without -R.
It is now rejected too.

Signed-off-by: Tanga Joris <tangajoris@gmail.com>
---
I chose to reject these headers, like d70e9c5c8c does for other
conflicting lines. Another option would be to accept "--- /dev/null"
before "new file mode" as a new file, since "/dev/null" means no file
there. I'm happy to change it if that is preferred.

 apply.c                    | 20 ++++++++++++++++++++
 t/t4133-apply-filenames.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/apply.c b/apply.c
index b56b363963..76cdacff33 100644
--- a/apply.c
+++ b/apply.c
@@ -1466,6 +1466,26 @@ int parse_git_diff_header(struct strbuf *root,
 			      *linenr);
 		return -128;
 	}
+	if (patch->is_new > 0 && patch->old_name) {
+		if (patch_input_file)
+			error(_("git diff header for new file %s names an old file %s at %s:%d"),
+			      patch->new_name, patch->old_name,
+			      patch_input_file, *linenr);
+		else
+			error(_("git diff header for new file %s names an old file %s (line %d)"),
+			      patch->new_name, patch->old_name, *linenr);
+		return -128;
+	}
+	if (patch->is_delete > 0 && patch->new_name) {
+		if (patch_input_file)
+			error(_("git diff header for deleted file %s names a new file %s at %s:%d"),
+			      patch->old_name, patch->new_name,
+			      patch_input_file, *linenr);
+		else
+			error(_("git diff header for deleted file %s names a new file %s (line %d)"),
+			      patch->old_name, patch->new_name, *linenr);
+		return -128;
+	}
 	patch->is_toplevel_relative = 1;
 	return offset;
 }
diff --git a/t/t4133-apply-filenames.sh b/t/t4133-apply-filenames.sh
index 727c36e7ea..ac9090d903 100755
--- a/t/t4133-apply-filenames.sh
+++ b/t/t4133-apply-filenames.sh
@@ -150,4 +150,35 @@ test_expect_success 'blank line after "diff --git" keeps paths relative to the t
 	test_path_is_missing f
 '
 
+for old in /dev/null a/f
+do
+	test_expect_success "reject \"--- $old\" before \"new file mode\"" '
+		cat >old_before_new.diff <<-EOF &&
+		diff --git a/f b/f
+		--- $old
+		+++ b/f
+		new file mode 100644
+		@@ -0,0 +1 @@
+		+1
+		EOF
+		test_must_fail git apply --check old_before_new.diff 2>err &&
+		test_grep "header for new file f names an old file" err
+	'
+done
+
+test_expect_success 'reject "+++ /dev/null" before "deleted file mode"' '
+	cat >new_before_delete.diff <<-\EOF &&
+	diff --git a/f b/f
+	--- a/f
+	+++ /dev/null
+	deleted file mode 100644
+	@@ -1 +0,0 @@
+	-1
+	EOF
+	test_must_fail git apply --check new_before_delete.diff 2>err &&
+	test_grep "header for deleted file f names a new file" err &&
+	test_must_fail git apply --check -R new_before_delete.diff 2>err &&
+	test_grep "header for deleted file f names a new file" err
+'
+
 test_done
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line
  2026-11-07 19:54 ` [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line Tanga Joris
  2026-11-07 19:54   ` [Outreachy PATCH 2/2] apply: reject new and deleted files that have both names Tanga Joris
@ 2026-11-09 10:52   ` Phillip Wood
  2026-11-09 13:13     ` Tanga Joris
  2026-11-09 19:56     ` Junio C Hamano
  1 sibling, 2 replies; 12+ messages in thread
From: Phillip Wood @ 2026-11-09 10:52 UTC (permalink / raw)
  To: Tanga Joris, git
  Cc: gitster, jackrawlings2011, christian.couder, siddharthasthana31,
	kaartic.sivaraam, usmanakinyemi202

Hi Tanga

On 07/11/2026 19:54, Tanga Joris wrote:
> 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,

These sound like corrupt patches which we should be rejecting with a 
better error message, rather than applying.

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

To me this looks like a corrupt patch because we have a blank line after 
the "diff --git" header followed by valid extended diff headers. Now 
that we accept this do we respect the file mode?

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

This looks like a corrupt patch as well where a space has been 
accidentally introduced at the start of the line.

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

This is the only one that I think it is reasonable to accept - we have 
an empty patch for "f" followed by a valid patch for "g"

Thanks

Phillip


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line
  2026-11-09 10:52   ` [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line Phillip Wood
@ 2026-11-09 13:13     ` Tanga Joris
  2026-11-09 14:31       ` Phillip Wood
  2026-11-09 19:56     ` Junio C Hamano
  1 sibling, 1 reply; 12+ messages in thread
From: Tanga Joris @ 2026-11-09 13:13 UTC (permalink / raw)
  To: phillip.wood
  Cc: git, gitster, jackrawlings2011, christian.couder,
	siddharthasthana31, kaartic.sivaraam, usmanakinyemi202

Hey Phillip,

Thank you for reviewing my patch.

On Mon, 9 Nov 2026 at 13:52, Phillip Wood <phillip.wood123@gmail.com> wrote:
> These sound like corrupt patches which we should be rejecting with a
> better error message, rather than applying.

I now see and agree that accepting invalid patches was the wrong
call. Giving a clearer error seems to be a better direction to take.

> To me this looks like a corrupt patch because we have a blank line after
> the "diff --git" header followed by valid extended diff headers. Now
> that we accept this do we respect the file mode?

No, it does not respect the file mode. "new file mode 100755" is
ignored and the file gets the default mode.

> This looks like a corrupt patch as well where a space has been
> accidentally introduced at the start of the line.

Yes, I will treat this one the same way as the blank line.

> This is the only one that I think it is reasonable to accept - we have
> an empty patch for "f" followed by a valid patch for "g"

Alright, I will keep accepting this one.

For the next version, I plan to die with an error when a "diff --git"
line has no header lines after it, and a "---" and "+++" patch follows
before the next "diff --git" line. The error would name the line of
the "diff --git". An empty section followed by another "diff --git"
line would still be ignored.

One thing I would like to check with you. On master, a blank line
after "diff --git" is accepted in a patch that modifies a file. If
that patch has "old mode" and "new mode" lines, the mode change is
lost. The rule above would reject these patches too. What is your view
on this?

Thanks,
Tanga

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line
  2026-11-09 13:13     ` Tanga Joris
@ 2026-11-09 14:31       ` Phillip Wood
  2026-11-09 20:54         ` Tanga Joris
  0 siblings, 1 reply; 12+ messages in thread
From: Phillip Wood @ 2026-11-09 14:31 UTC (permalink / raw)
  To: Tanga Joris, phillip.wood
  Cc: git, gitster, jackrawlings2011, christian.couder,
	siddharthasthana31, kaartic.sivaraam, usmanakinyemi202

Hi Tanga

On 09/11/2026 13:13, Tanga Joris wrote:
> On Mon, 9 Nov 2026 at 13:52, Phillip Wood <phillip.wood123@gmail.com> wrote:

It would have been helpful to retain a bit more context here, I've 
restored it so anyone reading this could see the patch that I was 
referring to.

>>> diff --git a/f b/f
>>> 
>>> new file mode 100644
>>> index 0000000..d00491f
>>> --- /dev/null
>>> +++ b/f
>>> @@ -0,0 +1 @@
>>> +1 

>> To me this looks like a corrupt patch because we have a blank line after
>> the "diff --git" header followed by valid extended diff headers. Now
>> that we accept this do we respect the file mode?
> 
> No, it does not respect the file mode. "new file mode 100755" is
> ignored and the file gets the default mode.

So that is a good reason to reject this, rather than silently applying a 
corrupt patch.

 >>> diff --git a/f b/f
 >>> diff --git a/g b/g

>> This is the only one that I think it is reasonable to accept - we have
>> an empty patch for "f" followed by a valid patch for "g"
> 
> Alright, I will keep accepting this one.

Do we accept that now, or only with your patch?

> For the next version, I plan to die with an error when a "diff --git"
> line has no header lines after it, and a "---" and "+++" patch follows
> before the next "diff --git" line. The error would name the line of
> the "diff --git". An empty section followed by another "diff --git"
> line would still be ignored.
> 
> One thing I would like to check with you. On master, a blank line
> after "diff --git" is accepted in a patch that modifies a file. If
> that patch has "old mode" and "new mode" lines, the mode change is
> lost. The rule above would reject these patches too. What is your view
> on this?
Oh, so

diff --git a/f b/f

index 573541a..d00491f 100644
--- a/f
+++ b/f
@@ -1 +1 @@
-0
+1

applies. That looks like a corrupt patch as well to me and if we're 
ignoring the mode headers I'd say it is definitely a bug and rejecting 
it is the right thing to do.

Thanks

Phillip

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line
  2026-11-09 10:52   ` [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line Phillip Wood
  2026-11-09 13:13     ` Tanga Joris
@ 2026-11-09 19:56     ` Junio C Hamano
  1 sibling, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2026-11-09 19:56 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Tanga Joris, git, jackrawlings2011, christian.couder,
	siddharthasthana31, kaartic.sivaraam, usmanakinyemi202

Phillip Wood <phillip.wood123@gmail.com> writes:

>> +	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
>
> This is the only one that I think it is reasonable to accept - we have 
> an empty patch for "f" followed by a valid patch for "g"

We shouldn't accept anything that "git format-patch" (or "git diff")
would not produce, I would have to say.

Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line
  2026-11-09 14:31       ` Phillip Wood
@ 2026-11-09 20:54         ` Tanga Joris
  2026-11-09 21:06           ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Tanga Joris @ 2026-11-09 20:54 UTC (permalink / raw)
  To: phillip.wood
  Cc: git, gitster, jackrawlings2011, christian.couder,
	siddharthasthana31, kaartic.sivaraam, usmanakinyemi202

Hey Phillip,

Thanks, and sorry for trimming too much context last time.

On Mon, 9 Nov 2026 at 17:31, Phillip Wood <phillip.wood123@gmail.com> wrote:
>  >>> diff --git a/f b/f
>  >>> diff --git a/g b/g
>
> >> This is the only one that I think it is reasonable to accept - we have
> >> an empty patch for "f" followed by a valid patch for "g"
> >
> > Alright, I will keep accepting this one.
>
> Do we accept that now, or only with your patch?

Only with my patch. On master your example fails with "error: git
apply: bad git-diff - inconsistent old filename", because the names
from the empty "diff --git" line are kept. A new or deleted file
after it fails in a similar way. It does work on master when the next
section names the same file, or is a rename.

Junio wrote in reply to your first mail:

> We shouldn't accept anything that "git format-patch" (or "git diff")
> would not produce, I would have to say.

So I plan to drop this part and leave this case as it is on master.
The next version will only turn the corrupt patches into an error.

One thing I noticed while checking this. "git format-patch" can
produce a "diff --git" line with nothing after it, when a line in the
body of the commit message looks like "diff --git a/x b/x". On
master, "git apply" fails on that patch with the same "inconsistent
old filename" error, unless the line names the same file as the real
patch. This is not new, so I would leave it out of this series,
unless you or Junio think otherwise.

> Oh, so
>
> diff --git a/f b/f
>
> index 573541a..d00491f 100644
> --- a/f
> +++ b/f
> @@ -1 +1 @@
> -0
> +1
>
> applies. That looks like a corrupt patch as well to me and if we're
> ignoring the mode headers I'd say it is definitely a bug and rejecting
> it is the right thing to do.

Yes, it applies on master. Alright, I will reject this one too in the
next version, along with the blank line and the indented "new file
mode" cases.

Thanks,
Tanga

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line
  2026-11-09 20:54         ` Tanga Joris
@ 2026-11-09 21:06           ` Junio C Hamano
  2026-11-09 23:03             ` Tanga Joris
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2026-11-09 21:06 UTC (permalink / raw)
  To: Tanga Joris
  Cc: phillip.wood, git, jackrawlings2011, christian.couder,
	siddharthasthana31, kaartic.sivaraam, usmanakinyemi202

Tanga Joris <tangajoris@gmail.com> writes:

> One thing I noticed while checking this. "git format-patch" can
> produce a "diff --git" line with nothing after it, when a line in the
> body of the commit message looks like "diff --git a/x b/x".

A garbage in the commit log message does not count as coming from
"git diff".  It may take some effort to teach "git am" to avoid
feeding such a false "diff --git" line down to "git apply", but an
important thing to keep in mind is that it is NOT "git apply"'s job
to worry about such a garbage input.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line
  2026-11-09 21:06           ` Junio C Hamano
@ 2026-11-09 23:03             ` Tanga Joris
  0 siblings, 0 replies; 12+ messages in thread
From: Tanga Joris @ 2026-11-09 23:03 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: phillip.wood, git, jackrawlings2011, christian.couder,
	siddharthasthana31, kaartic.sivaraam, usmanakinyemi202

Hello Junio,

On Tue, 10 Nov 2026 at 00:06, Junio C Hamano <gitster@pobox.com> wrote:
> A garbage in the commit log message does not count as coming from
> "git diff".  It may take some effort to teach "git am" to avoid
> feeding such a false "diff --git" line down to "git apply", but an
> important thing to keep in mind is that it is NOT "git apply"'s job
> to worry about such a garbage input.

That makes sense. I had thought of it as something "git format-patch"
produces, but it is only the log message passing through. I will
leave it out, and the next version will only reject the corrupt
headers.

Thanks,
Tanga

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2026-11-09 23:04 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-17 15:17 Git crashed on `git apply --check` Gingrspacecadet
2026-10-24  6:10 ` Tanga Joris
     [not found]   ` <CALdwsC11JHUBBTt3e_RFixLYSF0VU1C+mG7bQsO0vV=a68XHNw@mail.gmail.com>
2026-10-25 22:56     ` Tanga Joris
2026-11-07 19:54 ` [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line Tanga Joris
2026-11-07 19:54   ` [Outreachy PATCH 2/2] apply: reject new and deleted files that have both names Tanga Joris
2026-11-09 10:52   ` [Outreachy PATCH 1/2] apply: do not keep names from an unused "diff --git" line Phillip Wood
2026-11-09 13:13     ` Tanga Joris
2026-11-09 14:31       ` Phillip Wood
2026-11-09 20:54         ` Tanga Joris
2026-11-09 21:06           ` Junio C Hamano
2026-11-09 23:03             ` Tanga Joris
2026-11-09 19:56     ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox