[PATCH] apply.c: fix -p argument parsing

[PATCH] apply.c: fix -p argument parsing

[Mirko Faina]

"git apply" has an option -p that takes an integer as its argument.
Unfortunately the function apply_option_parse_p() in charge of parsing
this argument uses atoi() to convert from string to integer, which
allows a non-digit after the number (e.g. "1q") to be silently ignored.
As a consequence, an argument that does not begin with a digit silently
becomes a zero. Despite this command working fine when a non-positive
argument is passed, it might be useful for the end user to know that
their input contains non-digits that might've been unintended.

Replace atoi() with strtol_i() to catch malformed inputs.

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
Unlike [1], this argument doesn't overwrite an argument initialized to a
default value. Instead state->p_value_known is used instead to see if
the p_value should be used.

[1] https://lore.kernel.org/git/xmqq5y181fx0.fsf_-_@gitster.g/

 apply.c               |  3 ++-
 t/meson.build         |  1 +
 t/t4142-apply-args.sh | 31 +++++++++++++++++++++++++++++++
 t/t4142/patch         | 16 ++++++++++++++++
 4 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100755 t/t4142-apply-args.sh
 create mode 100644 t/t4142/patch

diff --git a/apply.c b/apply.c
index b6dd1066a0..b6c9e40700 100644
--- a/apply.c
+++ b/apply.c
@@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,
 
 	BUG_ON_OPT_NEG(unset);
 
-	state->p_value = atoi(arg);
+	if (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)
+		die("<num> has to be non negative an integer");
 	state->p_value_known = 1;
 	return 0;
 }
diff --git a/t/meson.build b/t/meson.build
index 106c68df3d..d26df707cb 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -547,6 +547,7 @@ integration_tests = [
   't4139-apply-escape.sh',
   't4140-apply-ita.sh',
   't4141-apply-too-large.sh',
+  't4142-apply-args.sh',
   't4150-am.sh',
   't4151-am-abort.sh',
   't4152-am-subjects.sh',
diff --git a/t/t4142-apply-args.sh b/t/t4142-apply-args.sh
new file mode 100755
index 0000000000..6fe73289f2
--- /dev/null
+++ b/t/t4142-apply-args.sh
@@ -0,0 +1,31 @@
+#!/bin/bash
+
+test_description='git apply test for various malformed arguments
+'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	git commit --allow-empty -m "Initial commit"
+'
+
+test_expect_success 'git apply -p 1 patch' '
+	test_when_finished "rm -rf result t" &&
+	git apply -p 1 $TEST_DIRECTORY/t4142/patch &&
+	ls -l >result &&
+	test_line_count = 3 result
+'
+
+test_expect_success 'git apply -p malformed patch' '
+	test_must_fail git apply -p malformed $TEST_DIRECTORY/t4142/patch
+'
+
+test_expect_success 'git apply -p 2q patch' '
+	test_must_fail git apply -p 2q $TEST_DIRECTORY/t4142/patch
+'
+
+test_expect_success 'git apply -p -1 patch' '
+	test_must_fail git apply -p -1 $TEST_DIRECTORY/t4142/patch
+'
+
+test_done
diff --git a/t/t4142/patch b/t/t4142/patch
new file mode 100644
index 0000000000..c4511bb708
--- /dev/null
+++ b/t/t4142/patch
@@ -0,0 +1,16 @@
+From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001
+From: Mroik <mroik@delayed.space>
+Date: Mon, 9 Mar 2026 23:25:00 +0100
+Subject: [PATCH] Test
+
+---
+ t/test/test | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+ create mode 100644 t/test/test
+
+diff --git a/t/test/test b/t/test/test
+new file mode 100644
+index 0000000000..e69de29bb2
+-- 
+2.53.0.851.ga537e3e6e9
+
-- 
2.53.0.851.ga537e3e6e9

Re: [PATCH] apply.c: fix -p argument parsing

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

> "git apply" has an option -p that takes an integer as its argument.
> Unfortunately the function apply_option_parse_p() in charge of parsing
> this argument uses atoi() to convert from string to integer, which
> allows a non-digit after the number (e.g. "1q") to be silently ignored.
> As a consequence, an argument that does not begin with a digit silently
> becomes a zero. Despite this command working fine when a non-positive
> argument is passed, it might be useful for the end user to know that
> their input contains non-digits that might've been unintended.
>
> Replace atoi() with strtol_i() to catch malformed inputs.
>
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
> Unlike [1], this argument doesn't overwrite an argument initialized to a
> default value. Instead state->p_value_known is used instead to see if
> the p_value should be used.

The change to the code looks OK.

I do not think we want to spend a test number with a file only to
host just a single small test, though.  Can't we roll it into an
existing test script instead?

Thanks.

>
> [1] https://lore.kernel.org/git/xmqq5y181fx0.fsf_-_@gitster.g/
>
>  apply.c               |  3 ++-
>  t/meson.build         |  1 +
>  t/t4142-apply-args.sh | 31 +++++++++++++++++++++++++++++++
>  t/t4142/patch         | 16 ++++++++++++++++
>  4 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100755 t/t4142-apply-args.sh
>  create mode 100644 t/t4142/patch
>
> diff --git a/apply.c b/apply.c
> index b6dd1066a0..b6c9e40700 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,
>  
>  	BUG_ON_OPT_NEG(unset);
>  
> -	state->p_value = atoi(arg);
> +	if (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)
> +		die("<num> has to be non negative an integer");
>  	state->p_value_known = 1;
>  	return 0;
>  }
> diff --git a/t/meson.build b/t/meson.build
> index 106c68df3d..d26df707cb 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -547,6 +547,7 @@ integration_tests = [
>    't4139-apply-escape.sh',
>    't4140-apply-ita.sh',
>    't4141-apply-too-large.sh',
> +  't4142-apply-args.sh',
>    't4150-am.sh',
>    't4151-am-abort.sh',
>    't4152-am-subjects.sh',
> diff --git a/t/t4142-apply-args.sh b/t/t4142-apply-args.sh
> new file mode 100755
> index 0000000000..6fe73289f2
> --- /dev/null
> +++ b/t/t4142-apply-args.sh
> @@ -0,0 +1,31 @@
> +#!/bin/bash
> +
> +test_description='git apply test for various malformed arguments
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	git commit --allow-empty -m "Initial commit"
> +'
> +
> +test_expect_success 'git apply -p 1 patch' '
> +	test_when_finished "rm -rf result t" &&
> +	git apply -p 1 $TEST_DIRECTORY/t4142/patch &&
> +	ls -l >result &&
> +	test_line_count = 3 result
> +'
> +
> +test_expect_success 'git apply -p malformed patch' '
> +	test_must_fail git apply -p malformed $TEST_DIRECTORY/t4142/patch
> +'
> +
> +test_expect_success 'git apply -p 2q patch' '
> +	test_must_fail git apply -p 2q $TEST_DIRECTORY/t4142/patch
> +'
> +
> +test_expect_success 'git apply -p -1 patch' '
> +	test_must_fail git apply -p -1 $TEST_DIRECTORY/t4142/patch
> +'
> +
> +test_done
> diff --git a/t/t4142/patch b/t/t4142/patch
> new file mode 100644
> index 0000000000..c4511bb708
> --- /dev/null
> +++ b/t/t4142/patch
> @@ -0,0 +1,16 @@
> +From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001
> +From: Mroik <mroik@delayed.space>
> +Date: Mon, 9 Mar 2026 23:25:00 +0100
> +Subject: [PATCH] Test
> +
> +---
> + t/test/test | 0
> + 1 file changed, 0 insertions(+), 0 deletions(-)
> + create mode 100644 t/test/test
> +
> +diff --git a/t/test/test b/t/test/test
> +new file mode 100644
> +index 0000000000..e69de29bb2
> +-- 
> +2.53.0.851.ga537e3e6e9
> +

[PATCH v2] apply.c: fix -p argument parsing

[Mirko Faina]

"git apply" has an option -p that takes an integer as its argument.
Unfortunately the function apply_option_parse_p() in charge of parsing
this argument uses atoi() to convert from string to integer, which
allows a non-digit after the number (e.g. "1q") to be silently ignored.
As a consequence, an argument that does not begin with a digit silently
becomes a zero. Despite this command working fine when a non-positive
argument is passed, it might be useful for the end user to know that
their input contains non-digits that might've been unintended.

Replace atoi() with strtol_i() to catch malformed inputs.

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
 apply.c                 |  3 ++-
 t/t4103-apply-binary.sh | 19 +++++++++++++++++++
 t/t4103/patch           | 16 ++++++++++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 t/t4103/patch

diff --git a/apply.c b/apply.c
index b6dd1066a0..61df3bdcd0 100644
--- a/apply.c
+++ b/apply.c
@@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,
 
 	BUG_ON_OPT_NEG(unset);
 
-	state->p_value = atoi(arg);
+	if (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)
+		die("<num> has to be a non-negative integer");
 	state->p_value_known = 1;
 	return 0;
 }
diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
index 8e302a5a57..d9dc884946 100755
--- a/t/t4103-apply-binary.sh
+++ b/t/t4103-apply-binary.sh
@@ -53,6 +53,25 @@ test_expect_success 'setup' '
 	)
 '
 
+test_expect_success 'git apply -p 1 patch' '
+	test_when_finished "rm -rf result t" &&
+	git apply -p 1 $TEST_DIRECTORY/t4103/patch &&
+	ls -l | sed -e "/[[:space:]]t$/!d" >result &&
+	test_line_count = 1 result
+'
+
+test_expect_success 'git apply -p malformed patch' '
+	test_must_fail git apply -p malformed $TEST_DIRECTORY/t4103/patch
+'
+
+test_expect_success 'git apply -p 2q patch' '
+	test_must_fail git apply -p 2q $TEST_DIRECTORY/t4103/patch
+'
+
+test_expect_success 'git apply -p -1 patch' '
+	test_must_fail git apply -p -1 $TEST_DIRECTORY/t4103/patch
+'
+
 test_expect_success 'stat binary diff -- should not fail.' \
 	'git checkout main &&
 	 git apply --stat --summary B.diff'
diff --git a/t/t4103/patch b/t/t4103/patch
new file mode 100644
index 0000000000..c4511bb708
--- /dev/null
+++ b/t/t4103/patch
@@ -0,0 +1,16 @@
+From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001
+From: Mroik <mroik@delayed.space>
+Date: Mon, 9 Mar 2026 23:25:00 +0100
+Subject: [PATCH] Test
+
+---
+ t/test/test | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+ create mode 100644 t/test/test
+
+diff --git a/t/test/test b/t/test/test
+new file mode 100644
+index 0000000000..e69de29bb2
+-- 
+2.53.0.851.ga537e3e6e9
+
-- 
2.53.0.851.ga537e3e6e9

Re: [PATCH v2] apply.c: fix -p argument parsing

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

> "git apply" has an option -p that takes an integer as its argument.
> Unfortunately the function apply_option_parse_p() in charge of parsing
> this argument uses atoi() to convert from string to integer, which
> allows a non-digit after the number (e.g. "1q") to be silently ignored.
> As a consequence, an argument that does not begin with a digit silently
> becomes a zero. Despite this command working fine when a non-positive
> argument is passed, it might be useful for the end user to know that
> their input contains non-digits that might've been unintended.
>
> Replace atoi() with strtol_i() to catch malformed inputs.
>
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---

>  apply.c                 |  3 ++-
>  t/t4103-apply-binary.sh | 19 +++++++++++++++++++
>  t/t4103/patch           | 16 ++++++++++++++++
>  3 files changed, 37 insertions(+), 1 deletion(-)
>  create mode 100644 t/t4103/patch

Curious.  It is true that we need to parse the p_value correctly
even when we are applying a binary patch, but the problem is not
limited to binary patches, is it?

> diff --git a/apply.c b/apply.c
> index b6dd1066a0..61df3bdcd0 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,
>  
>  	BUG_ON_OPT_NEG(unset);
>  
> -	state->p_value = atoi(arg);
> +	if (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)
> +		die("<num> has to be a non-negative integer");
>  	state->p_value_known = 1;
>  	return 0;
>  }

Sounds sensible.

I briefly wondered if it would have negative fallouts to change the
type of .p_value member to "unsigned int" and use strtol_ui() to
parse it, but the amount of work this part of the code needs to do
does not change that much, so such a change is of dubious value.
It looks like the above draws the line at the right place to stop.

Great execution.

> diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
> index 8e302a5a57..d9dc884946 100755
> --- a/t/t4103-apply-binary.sh
> +++ b/t/t4103-apply-binary.sh
> @@ -53,6 +53,25 @@ test_expect_success 'setup' '
>  	)
>  '
>  
> +test_expect_success 'git apply -p 1 patch' '
> +	test_when_finished "rm -rf result t" &&
> +	git apply -p 1 $TEST_DIRECTORY/t4103/patch &&
> +	ls -l | sed -e "/[[:space:]]t$/!d" >result &&
> +	test_line_count = 1 result
> +'

Is this saying "in the directory there must be only a single file
whose name is t?"  Wouldn't it be more readable and direct to do
something like

	test_path_is_dir t

or is there something more subtle going on here?

> +test_expect_success 'git apply -p malformed patch' '
> +	test_must_fail git apply -p malformed $TEST_DIRECTORY/t4103/patch
> +'
>
> +test_expect_success 'git apply -p 2q patch' '
> +	test_must_fail git apply -p 2q $TEST_DIRECTORY/t4103/patch
> +'

If this did not fail and patch gets applied with some p_value that
happens to be used when we fail to parse the number, then ...

> +test_expect_success 'git apply -p -1 patch' '
> +	test_must_fail git apply -p -1 $TEST_DIRECTORY/t4103/patch
> +'

... it would not be clear why this step fails.  Perhaps with that
same "unable to parse" p_value was used and this tried to create the
same file as the previous step already created, or we detected parse
failure.  We cannot tell.

It probably is a good idea to prepare for the worst by doing
something silly like

	test_when_finished "rm -f t/test/test test/test test" &&

at the beginning of each of these tests so that we would clean up
whatever we could leave behind?  I dunno.

>  test_expect_success 'stat binary diff -- should not fail.' \
>  	'git checkout main &&
>  	 git apply --stat --summary B.diff'
> diff --git a/t/t4103/patch b/t/t4103/patch
> new file mode 100644
> index 0000000000..c4511bb708
> --- /dev/null
> +++ b/t/t4103/patch
> @@ -0,0 +1,16 @@
> +From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001
> +From: Mroik <mroik@delayed.space>
> +Date: Mon, 9 Mar 2026 23:25:00 +0100
> +Subject: [PATCH] Test
> +
> +---
> + t/test/test | 0
> + 1 file changed, 0 insertions(+), 0 deletions(-)
> + create mode 100644 t/test/test
> +
> +diff --git a/t/test/test b/t/test/test
> +new file mode 100644
> +index 0000000000..e69de29bb2
> +-- 
> +2.53.0.851.ga537e3e6e9
> +

Re: [PATCH v2] apply.c: fix -p argument parsing

[Mirko Faina]

On Mon, Mar 09, 2026 at 08:31:42PM -0700, Junio C Hamano wrote:
> Curious.  It is true that we need to parse the p_value correctly
> even when we are applying a binary patch, but the problem is not
> limited to binary patches, is it?

Using a better regex I now realize t4120 would've been more apropriate.
I will move the tests.

> Is this saying "in the directory there must be only a single file
> whose name is t?"  Wouldn't it be more readable and direct to do
> something like
> 
> 	test_path_is_dir t
> 
> or is there something more subtle going on here?

Sorry, this approach is due to the unfamiliarity of the testing
framework. I must've missed test_path_is_dir, the README is very dense
so trying to find things at a glance is not the easiest (in my opinion).

Will rewrite to use test_path_is_dir.

> > +test_expect_success 'git apply -p malformed patch' '
> > +	test_must_fail git apply -p malformed $TEST_DIRECTORY/t4103/patch
> > +'
> >
> > +test_expect_success 'git apply -p 2q patch' '
> > +	test_must_fail git apply -p 2q $TEST_DIRECTORY/t4103/patch
> > +'
> 
> If this did not fail and patch gets applied with some p_value that
> happens to be used when we fail to parse the number, then ...
> 
> > +test_expect_success 'git apply -p -1 patch' '
> > +	test_must_fail git apply -p -1 $TEST_DIRECTORY/t4103/patch
> > +'
> 
> ... it would not be clear why this step fails.  Perhaps with that
> same "unable to parse" p_value was used and this tried to create the
> same file as the previous step already created, or we detected parse
> failure.  We cannot tell.
> 
> It probably is a good idea to prepare for the worst by doing
> something silly like
> 
> 	test_when_finished "rm -f t/test/test test/test test" &&
> 
> at the beginning of each of these tests so that we would clean up
> whatever we could leave behind?  I dunno.

right, "rm -rf t test" should be enough, will add this cleanup code.

Thank you for the review :)

[PATCH v3] apply.c: fix -p argument parsing

[Mirko Faina]

"git apply" has an option -p that takes an integer as its argument.
Unfortunately the function apply_option_parse_p() in charge of parsing
this argument uses atoi() to convert from string to integer, which
allows a non-digit after the number (e.g. "1q") to be silently ignored.
As a consequence, an argument that does not begin with a digit silently
becomes a zero. Despite this command working fine when a non-positive
argument is passed, it might be useful for the end user to know that
their input contains non-digits that might've been unintended.

Replace atoi() with strtol_i() to catch malformed inputs.

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
 apply.c               |  3 ++-
 t/t4120-apply-popt.sh | 21 +++++++++++++++++++++
 t/t4120/patch         | 16 ++++++++++++++++
 3 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 t/t4120/patch

diff --git a/apply.c b/apply.c
index b6dd1066a0..61df3bdcd0 100644
--- a/apply.c
+++ b/apply.c
@@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,
 
 	BUG_ON_OPT_NEG(unset);
 
-	state->p_value = atoi(arg);
+	if (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)
+		die("<num> has to be a non-negative integer");
 	state->p_value_known = 1;
 	return 0;
 }
diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh
index 697e86c0ff..3fdcfecc52 100755
--- a/t/t4120-apply-popt.sh
+++ b/t/t4120-apply-popt.sh
@@ -23,6 +23,27 @@ test_expect_success setup '
 	rmdir süb
 '
 
+test_expect_success 'git apply -p 1 patch' '
+	test_when_finished "rm -rf t" &&
+	git apply -p 1 $TEST_DIRECTORY/t4120/patch &&
+	test_path_is_dir t
+'
+
+test_expect_success 'apply fails due to non-num -p' '
+	test_when_finished "rm -rf t test" &&
+	test_must_fail git apply -p malformed $TEST_DIRECTORY/t4120/patch
+'
+
+test_expect_success 'apply fails due to trailing non-digit in -p' '
+	test_when_finished "rm -rf t test" &&
+	test_must_fail git apply -p 2q $TEST_DIRECTORY/t4120/patch
+'
+
+test_expect_success 'apply fails due to negative number in -p' '
+	test_when_finished "rm -rf t test" &&
+	test_must_fail git apply -p -1 $TEST_DIRECTORY/t4120/patch
+'
+
 test_expect_success 'apply git diff with -p2' '
 	cp file1.saved file1 &&
 	git apply -p2 patch.file
diff --git a/t/t4120/patch b/t/t4120/patch
new file mode 100644
index 0000000000..c4511bb708
--- /dev/null
+++ b/t/t4120/patch
@@ -0,0 +1,16 @@
+From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001
+From: Mroik <mroik@delayed.space>
+Date: Mon, 9 Mar 2026 23:25:00 +0100
+Subject: [PATCH] Test
+
+---
+ t/test/test | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+ create mode 100644 t/test/test
+
+diff --git a/t/test/test b/t/test/test
+new file mode 100644
+index 0000000000..e69de29bb2
+-- 
+2.53.0.851.ga537e3e6e9
+
-- 
2.53.0.316.gd563ecec28

Re: [PATCH v3] apply.c: fix -p argument parsing

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

>  apply.c               |  3 ++-
>  t/t4120-apply-popt.sh | 21 +++++++++++++++++++++
>  t/t4120/patch         | 16 ++++++++++++++++
>  3 files changed, 39 insertions(+), 1 deletion(-)
>  create mode 100644 t/t4120/patch

Yeah, 4120 is about the "-p" option, and is much better fit for the
tests for this input validation feature.  Good find.

Will queue.  Thanks.

> diff --git a/apply.c b/apply.c
> index b6dd1066a0..61df3bdcd0 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,
>  
>  	BUG_ON_OPT_NEG(unset);
>  
> -	state->p_value = atoi(arg);
> +	if (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)
> +		die("<num> has to be a non-negative integer");
>  	state->p_value_known = 1;
>  	return 0;
>  }
> diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh
> index 697e86c0ff..3fdcfecc52 100755
> --- a/t/t4120-apply-popt.sh
> +++ b/t/t4120-apply-popt.sh
> @@ -23,6 +23,27 @@ test_expect_success setup '
>  	rmdir süb
>  '
>  
> +test_expect_success 'git apply -p 1 patch' '
> +	test_when_finished "rm -rf t" &&
> +	git apply -p 1 $TEST_DIRECTORY/t4120/patch &&
> +	test_path_is_dir t
> +'
> +
> +test_expect_success 'apply fails due to non-num -p' '
> +	test_when_finished "rm -rf t test" &&
> +	test_must_fail git apply -p malformed $TEST_DIRECTORY/t4120/patch
> +'
> +
> +test_expect_success 'apply fails due to trailing non-digit in -p' '
> +	test_when_finished "rm -rf t test" &&
> +	test_must_fail git apply -p 2q $TEST_DIRECTORY/t4120/patch
> +'
> +
> +test_expect_success 'apply fails due to negative number in -p' '
> +	test_when_finished "rm -rf t test" &&
> +	test_must_fail git apply -p -1 $TEST_DIRECTORY/t4120/patch
> +'
> +
>  test_expect_success 'apply git diff with -p2' '
>  	cp file1.saved file1 &&
>  	git apply -p2 patch.file
> diff --git a/t/t4120/patch b/t/t4120/patch
> new file mode 100644
> index 0000000000..c4511bb708
> --- /dev/null
> +++ b/t/t4120/patch
> @@ -0,0 +1,16 @@
> +From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001
> +From: Mroik <mroik@delayed.space>
> +Date: Mon, 9 Mar 2026 23:25:00 +0100
> +Subject: [PATCH] Test
> +
> +---
> + t/test/test | 0
> + 1 file changed, 0 insertions(+), 0 deletions(-)
> + create mode 100644 t/test/test
> +
> +diff --git a/t/test/test b/t/test/test
> +new file mode 100644
> +index 0000000000..e69de29bb2
> +-- 
> +2.53.0.851.ga537e3e6e9
> +

Re: [PATCH v3] apply.c: fix -p argument parsing

[Jeff King]

On Tue, Mar 10, 2026 at 06:06:15AM +0100, Mirko Faina wrote:

> diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh
> index 697e86c0ff..3fdcfecc52 100755
> --- a/t/t4120-apply-popt.sh
> +++ b/t/t4120-apply-popt.sh
> @@ -23,6 +23,27 @@ test_expect_success setup '
>  	rmdir süb
>  '
>  
> +test_expect_success 'git apply -p 1 patch' '
> +	test_when_finished "rm -rf t" &&
> +	git apply -p 1 $TEST_DIRECTORY/t4120/patch &&
> +	test_path_is_dir t
> +'

This test seems to fail on Windows. From CI:

    ++ git apply -p 1 /d/a/git/git/t/t4120/patch
    error: git diff header lacks filename information when removing 1 leading pathname component (line 14)
    error: last command exited with $?=128

but I can't figure out why (and don't have a local Windows machine to
test on easily).

> diff --git a/t/t4120/patch b/t/t4120/patch

Not related to the failure, but IMHO we should keep small data like this
in the script itself, rather than as auxiliary files. The t/ directory
is already quite crowded, and it is often easier to refer to it when
it's near the tests themselves.

You don't even need the full email, just the patch part. (You might even
be able to reuse one of the other patches we make in the script, but I
didn't check).

Something like this would work. Note that it also gets rid of bare
references to $TEST_DIRECTORY, which really ought to be quoted (unlike
$TRASH_DIRECTORY, it does not always have a space, but it depends on
where somebody places their clone of git.git).


 t/t4120-apply-popt.sh | 16 ++++++++++++----
 t/t4120/patch         | 15 ---------------
 2 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh
index 3fdcfecc52..0531a19ee8 100755
--- a/t/t4120-apply-popt.sh
+++ b/t/t4120-apply-popt.sh
@@ -23,25 +23,33 @@ test_expect_success setup '
 	rmdir süb
 '
 
+test_expect_success 'setup deep subdir patch' '
+	cat >patch.deep <<-\EOF
+	diff --git a/t/test/test b/t/test/test
+	new file mode 100644
+	index 0000000000..e69de29bb2
+	EOF
+'
+
 test_expect_success 'git apply -p 1 patch' '
 	test_when_finished "rm -rf t" &&
-	git apply -p 1 $TEST_DIRECTORY/t4120/patch &&
+	git apply -p 1 patch.deep &&
 	test_path_is_dir t
 '
 
 test_expect_success 'apply fails due to non-num -p' '
 	test_when_finished "rm -rf t test" &&
-	test_must_fail git apply -p malformed $TEST_DIRECTORY/t4120/patch
+	test_must_fail git apply -p malformed patch.deep
 '
 
 test_expect_success 'apply fails due to trailing non-digit in -p' '
 	test_when_finished "rm -rf t test" &&
-	test_must_fail git apply -p 2q $TEST_DIRECTORY/t4120/patch
+	test_must_fail git apply -p 2q patch.deep
 '
 
 test_expect_success 'apply fails due to negative number in -p' '
 	test_when_finished "rm -rf t test" &&
-	test_must_fail git apply -p -1 $TEST_DIRECTORY/t4120/patch
+	test_must_fail git apply -p -1 patch.deep
 '
 
 test_expect_success 'apply git diff with -p2' '
diff --git a/t/t4120/patch b/t/t4120/patch
deleted file mode 100644
index bfccc708dd..0000000000
--- a/t/t4120/patch
+++ /dev/null
@@ -1,15 +0,0 @@
-From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001
-From: Mroik <mroik@delayed.space>
-Date: Mon, 9 Mar 2026 23:25:00 +0100
-Subject: [PATCH] Test
-
----
- t/test/test | 0
- 1 file changed, 0 insertions(+), 0 deletions(-)
- create mode 100644 t/test/test
-
-diff --git a/t/test/test b/t/test/test
-new file mode 100644
-index 0000000000..e69de29bb2
---
-2.53.0.851.ga537e3e6e9

Re: [PATCH v3] apply.c: fix -p argument parsing

[Jeff King]

On Thu, Mar 12, 2026 at 08:16:29PM -0400, Jeff King wrote:

> > +test_expect_success 'git apply -p 1 patch' '
> > +	test_when_finished "rm -rf t" &&
> > +	git apply -p 1 $TEST_DIRECTORY/t4120/patch &&
> > +	test_path_is_dir t
> > +'
> 
> This test seems to fail on Windows. From CI:
> 
>     ++ git apply -p 1 /d/a/git/git/t/t4120/patch
>     error: git diff header lacks filename information when removing 1 leading pathname component (line 14)
>     error: last command exited with $?=128
> 
> but I can't figure out why (and don't have a local Windows machine to
> test on easily).

Ah, I figured it out. The culprit is CRLF line endings. Naturally. :-/

It looks like there is an existing bug in apply.c when reading patches
with CRLF endings. Because of complicated historical reasons, parsing
the:

  diff --git a/t/test/test b/t/test/test

line insists that we find the same "t/test/test" path at the very end of
the line. But instead, we find the extra CR. As a result, we leave
patch->def_name NULL instead of filling it in with "t/test/test".

Usually this is not too big a deal, as we can pick up the name from the
"---" and "+++" lines. But in your patch:

  diff --git a/t/test/test b/t/test/test
  new file mode 100644
  index 0000000000..e69de29bb2

since there is no content, we omit them entirely. And so Git has no idea
where to apply the patch (even without "-p" at all).

I think the fix is probably:

diff --git a/apply.c b/apply.c
index 61df3bdcd0..62d6a1f8d7 100644
--- a/apply.c
+++ b/apply.c
@@ -1295,7 +1295,7 @@ static char *git_header_name(int p_value,
 			 * (that are separated by one HT or SP we just
 			 * found) exactly match?
 			 */
-			if (second[len] == '\n' && !strncmp(name, second, len))
+			if ((second[len] == '\n' || second[len] == '\r') && !strncmp(name, second, len))
 				return xmemdupz(name, len);
 		}
 	}

but I'm not sure if there are other lurking CRLF issues, or if this
might allow malicious input to cause confusion.


Getting back to your patch: why is there a CRLF here in the first place?
Because on Windows, we check out the whole repo with CRLF conversion,
except for a few known file types listed in .gitattributes. And that
includes your t/t4120/patch file.

Coincidentally the style suggestion I made earlier, to just inline it in
the t4120 script itself, makes the problem go away. Because we check out
those scripts with bare line feeds, per .gitattributes, the file we
create will also have regular line feeds.

So I would suggest doing that as a workaround. It might be worth
addressing the CRLF header parsing problem above, too, but I think that
should be a separate topic.

-Peff

Re: [PATCH v3] apply.c: fix -p argument parsing

[Jeff King]

On Thu, Mar 12, 2026 at 09:12:59PM -0400, Jeff King wrote:

> Getting back to your patch: why is there a CRLF here in the first place?
> Because on Windows, we check out the whole repo with CRLF conversion,
> except for a few known file types listed in .gitattributes. And that
> includes your t/t4120/patch file.
> 
> Coincidentally the style suggestion I made earlier, to just inline it in
> the t4120 script itself, makes the problem go away. Because we check out
> those scripts with bare line feeds, per .gitattributes, the file we
> create will also have regular line feeds.
> 
> So I would suggest doing that as a workaround. It might be worth
> addressing the CRLF header parsing problem above, too, but I think that
> should be a separate topic.

In case we want to pursue the CRLF thing further, you can demonstrate it
on Linux easily with:

  {
    printf 'diff --git a/file b/file\r\n'
    printf 'old mode 100644'
    printf 'new mode 100755'
  } >patch
  git apply patch

I was surprised that we wouldn't hit this case _somewhere_ in the test
suite already, and indeed we do. Even with a separate patch file, like
you have! But the tests pass due to 614f4f0f35 (Fix the remaining tests
that failed with core.autocrlf=true, 2017-05-09), which explicitly adds
.gitattributes for "t/t4101/*", etc.

So that's another workaround for your patch: we could mark the directory
with .gitattributes in the same way. I still prefer inlining the patch
in the script for style reasons, though.

-Peff

Re: [PATCH v3] apply.c: fix -p argument parsing

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Getting back to your patch: why is there a CRLF here in the first place?
> Because on Windows, we check out the whole repo with CRLF conversion,
> except for a few known file types listed in .gitattributes. And that
> includes your t/t4120/patch file.

Yuck.

> Coincidentally the style suggestion I made earlier, to just inline it in
> the t4120 script itself, makes the problem go away.

Of course.  Joy of stumbling on ^W^Wworking with Windows.  Sigh...

> So I would suggest doing that as a workaround. It might be worth
> addressing the CRLF header parsing problem above, too, but I think that
> should be a separate topic.

Re: [PATCH v3] apply.c: fix -p argument parsing

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> diff --git a/t/t4120/patch b/t/t4120/patch
>
> Not related to the failure, but IMHO we should keep small data like this
> in the script itself, rather than as auxiliary files. The t/ directory
> is already quite crowded, and it is often easier to refer to it when
> it's near the tests themselves.

A very good suggestion.  I actually did find it a bit annoying to
see an extra file there, but somehow failed to mention it in my
review.  Creating one in a set-up step and reusing it would be just
as simple as shipping an extra file.

Thanks.

[PATCH v4] apply.c: fix -p argument parsing

[Mirko Faina]

"git apply" has an option -p that takes an integer as its argument.
Unfortunately the function apply_option_parse_p() in charge of parsing
this argument uses atoi() to convert from string to integer, which
allows a non-digit after the number (e.g. "1q") to be silently ignored.
As a consequence, an argument that does not begin with a digit silently
becomes a zero. Despite this command working fine when a non-positive
argument is passed, it might be useful for the end user to know that
their input contains non-digits that might've been unintended.

Replace atoi() with strtol_i() to catch malformed inputs.

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
As Jeff pointed out, the previous patch doesn't pass tests on windows...
Inlined as a workaround and to avoid adding additional folders to the
existing test directory.

Thank you for the review :)

 apply.c               |  3 ++-
 t/t4120-apply-popt.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index b6dd1066a0..61df3bdcd0 100644
--- a/apply.c
+++ b/apply.c
@@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,
 
 	BUG_ON_OPT_NEG(unset);
 
-	state->p_value = atoi(arg);
+	if (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)
+		die("<num> has to be a non-negative integer");
 	state->p_value_known = 1;
 	return 0;
 }
diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh
index 697e86c0ff..3dbccbfc03 100755
--- a/t/t4120-apply-popt.sh
+++ b/t/t4120-apply-popt.sh
@@ -23,6 +23,45 @@ test_expect_success setup '
 	rmdir süb
 '
 
+test_expect_success 'git apply -p 1 patch' '
+	cat >patch <<-\EOF &&
+		From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001
+		From: Mroik <mroik@delayed.space>
+		Date: Mon, 9 Mar 2026 23:25:00 +0100
+		Subject: [PATCH] Test
+
+		---
+		 t/test/test | 0
+		 1 file changed, 0 insertions(+), 0 deletions(-)
+		 create mode 100644 t/test/test
+
+		diff --git a/t/test/test b/t/test/test
+		new file mode 100644
+		index 0000000000..e69de29bb2
+		-- 
+		2.53.0.851.ga537e3e6e9
+
+	EOF
+	test_when_finished "rm -rf t" &&
+	git apply -p 1 patch &&
+	test_path_is_dir t
+'
+
+test_expect_success 'apply fails due to non-num -p' '
+	test_when_finished "rm -rf t test" &&
+	test_must_fail git apply -p malformed patch
+'
+
+test_expect_success 'apply fails due to trailing non-digit in -p' '
+	test_when_finished "rm -rf t test" &&
+	test_must_fail git apply -p 2q patch
+'
+
+test_expect_success 'apply fails due to negative number in -p' '
+	test_when_finished "rm -rf t test patch" &&
+	test_must_fail git apply -p -1 patch
+'
+
 test_expect_success 'apply git diff with -p2' '
 	cp file1.saved file1 &&
 	git apply -p2 patch.file
-- 
2.53.0.931.gb56d940889

Re: [PATCH v4] apply.c: fix -p argument parsing

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

> As Jeff pointed out, the previous patch doesn't pass tests on windows...
> Inlined as a workaround and to avoid adding additional folders to the
> existing test directory.

Thanks for working very well together.
> +test_expect_success 'git apply -p 1 patch' '
> +	cat >patch <<-\EOF &&
> +		From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001
> +		From: Mroik <mroik@delayed.space>
> +		Date: Mon, 9 Mar 2026 23:25:00 +0100
> +		Subject: [PATCH] Test
> +
> +		---
> +		 t/test/test | 0
> +		 1 file changed, 0 insertions(+), 0 deletions(-)
> +		 create mode 100644 t/test/test
> +
> +		diff --git a/t/test/test b/t/test/test
> +		new file mode 100644
> +		index 0000000000..e69de29bb2
> +		-- 
> +		2.53.0.851.ga537e3e6e9
> +
> +	EOF

It is more customary to indent the here-doc body to the same level
as surrounding <<EOF..EOF; no need to resend only to fix this, as I
can easily dedent it by one level.

> +	test_when_finished "rm -rf t" &&
> +	git apply -p 1 patch &&
> +	test_path_is_dir t
> +'
> +
> +test_expect_success 'apply fails due to non-num -p' '
> +	test_when_finished "rm -rf t test" &&
> +	test_must_fail git apply -p malformed patch
> +'
> +
> +test_expect_success 'apply fails due to trailing non-digit in -p' '
> +	test_when_finished "rm -rf t test" &&
> +	test_must_fail git apply -p 2q patch
> +'
> +
> +test_expect_success 'apply fails due to negative number in -p' '
> +	test_when_finished "rm -rf t test patch" &&
> +	test_must_fail git apply -p -1 patch
> +'

The test all make sense, but if we know what error message we are
expecting, it may not be a bad idea to do something like

	test_must_fail git apply -p -1 patch 2>err &&
	test_grep "<num> has to be a non-negative" err

to ensure that the command did not fail for a wrong reason.

THanks.

[PATCH] apply.c: fix -p argument parsing

[Mirko Faina]

"git apply" has an option -p that takes an integer as its argument.
Unfortunately the function apply_option_parse_p() in charge of parsing
this argument uses atoi() to convert from string to integer, which
allows a non-digit after the number (e.g. "1q") to be silently ignored.
As a consequence, an argument that does not begin with a digit silently
becomes a zero. Despite this command working fine when a non-positive
argument is passed, it might be useful for the end user to know that
their input contains non-digits that might've been unintended.

Replace atoi() with strtol_i() to catch malformed inputs.

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
Sending a new version 'cause Tian pointed out that the die message is
not explicit enough, and a user might not understand which option we're
referring to if there are multiple.

 apply.c               |  3 ++-
 t/t4120-apply-popt.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index b6dd1066a0..52cd590bdb 100644
--- a/apply.c
+++ b/apply.c
@@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,
 
 	BUG_ON_OPT_NEG(unset);
 
-	state->p_value = atoi(arg);
+	if (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)
+		die(_("option -p expects a non-negative integer, got '%s'"), arg);
 	state->p_value_known = 1;
 	return 0;
 }
diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh
index 697e86c0ff..acb5462a25 100755
--- a/t/t4120-apply-popt.sh
+++ b/t/t4120-apply-popt.sh
@@ -23,6 +23,47 @@ test_expect_success setup '
 	rmdir süb
 '
 
+test_expect_success 'git apply -p 1 patch' '
+	cat >patch <<-\EOF &&
+	From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001
+	From: Mroik <mroik@delayed.space>
+	Date: Mon, 9 Mar 2026 23:25:00 +0100
+	Subject: [PATCH] Test
+
+	---
+	 t/test/test | 0
+	 1 file changed, 0 insertions(+), 0 deletions(-)
+	 create mode 100644 t/test/test
+
+	diff --git a/t/test/test b/t/test/test
+	new file mode 100644
+	index 0000000000..e69de29bb2
+	-- 
+	2.53.0.851.ga537e3e6e9
+	EOF
+	test_when_finished "rm -rf t" &&
+	git apply -p 1 patch &&
+	test_path_is_dir t
+'
+
+test_expect_success 'apply fails due to non-num -p' '
+	test_when_finished "rm -rf t test err" &&
+	test_must_fail git apply -p malformed patch 2>err &&
+	test_grep "option -p expects a non-negative integer" err
+'
+
+test_expect_success 'apply fails due to trailing non-digit in -p' '
+	test_when_finished "rm -rf t test err" &&
+	test_must_fail git apply -p 2q patch 2>err &&
+	test_grep "option -p expects a non-negative integer" err
+'
+
+test_expect_success 'apply fails due to negative number in -p' '
+	test_when_finished "rm -rf t test err patch" &&
+	test_must_fail git apply -p -1 patch 2> err &&
+	test_grep "option -p expects a non-negative integer" err
+'
+
 test_expect_success 'apply git diff with -p2' '
 	cp file1.saved file1 &&
 	git apply -p2 patch.file
-- 
2.53.0.959.g497ff81fa9

Re: [PATCH] apply.c: fix -p argument parsing

[Mirko Faina]

Sorry, forgot to mark as v5

Re: [PATCH] apply.c: fix -p argument parsing

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

> "git apply" has an option -p that takes an integer as its argument.
> Unfortunately the function apply_option_parse_p() in charge of parsing
> this argument uses atoi() to convert from string to integer, which
> allows a non-digit after the number (e.g. "1q") to be silently ignored.
> As a consequence, an argument that does not begin with a digit silently
> becomes a zero. Despite this command working fine when a non-positive
> argument is passed, it might be useful for the end user to know that
> their input contains non-digits that might've been unintended.
>
> Replace atoi() with strtol_i() to catch malformed inputs.
>
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
> Sending a new version 'cause Tian pointed out that the die message is
> not explicit enough, and a user might not understand which option we're
> referring to if there are multiple.

The updated error message does look more helpful.  Will replace.

Also the post-test clean-up in each test is more thorough, which is
a very good thing to see.

Thanks.

Re: [PATCH] apply.c: fix -p argument parsing

[Tian Yuchen]

Hi Mirko,

> +test_expect_success 'git apply -p 1 patch' '
> +	cat >patch <<-\EOF &&
> +		From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001

<<-\EOF should swallow the leading tab, but since you're using spaces 
for indentation here, that would result in a space at the beginning of 
every line, right? I think <<\EOF is correct here.

But Junio said you don't need to worry about it, it's all good ;)

---

The rest are just minor flaws (in my opinion) that you can safely ignore:

> +	if (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)
> +		die("<num> has to be a non-negative integer");

I think something like:

if (strtol_i(arg, 10, &state->p_value) || state->p_value < 0)
die(_("option -p expects a non-negative integer, got '%s'"), arg);

might be a bit better;

> +test_expect_success 'apply fails due to trailing non-digit in -p' '
> +	test_when_finished "rm -rf t test" &&
> +	test_must_fail git apply -p 2q patch
> +'
> +
> +test_expect_success 'apply fails due to negative number in -p' '
> +	test_when_finished "rm -rf t test patch" &&
> +	test_must_fail git apply -p -1 patch
> +'
> +
>   test_expect_success 'apply git diff with -p2' '
>   	cp file1.saved file1 &&
>   	git apply -p2 patch.file

The 'patch' is created in the first test case, will it prevent the 
subsequent test cases from running on their own?

Overall, the patch itself looks good to me. Thank you!

Regards,

Yuchen

Re: [PATCH] apply.c: fix -p argument parsing

[Mirko Faina]

On Mon, Mar 16, 2026 at 01:22:03AM +0800, Tian Yuchen wrote:
> <<-\EOF should swallow the leading tab, but since you're using spaces for
> indentation here, that would result in a space at the beginning of every
> line, right? I think <<\EOF is correct here.
> 
> But Junio said you don't need to worry about it, it's all good ;)

I think that's an issue with how the email is rendered. If you view in
plaintext the raw mailbox file it does uses tabs.

> 
> The rest are just minor flaws (in my opinion) that you can safely ignore:
> 
> > +	if (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)
> > +		die("<num> has to be a non-negative integer");
> 
> I think something like:
> 
> if (strtol_i(arg, 10, &state->p_value) || state->p_value < 0)
> die(_("option -p expects a non-negative integer, got '%s'"), arg);
> 
> might be a bit better;

You're right, users that haven't looked at the help usage in a while
might not realize which argument "<num>" is, especially if there are
multiple.

Will fix this

> > +test_expect_success 'apply fails due to trailing non-digit in -p' '
> > +	test_when_finished "rm -rf t test" &&
> > +	test_must_fail git apply -p 2q patch
> > +'
> > +
> > +test_expect_success 'apply fails due to negative number in -p' '
> > +	test_when_finished "rm -rf t test patch" &&
> > +	test_must_fail git apply -p -1 patch
> > +'
> > +
> >   test_expect_success 'apply git diff with -p2' '
> >   	cp file1.saved file1 &&
> >   	git apply -p2 patch.file
> 
> The 'patch' is created in the first test case, will it prevent the
> subsequent test cases from running on their own?

You are right, the tests that come after that require 'patch' might not
be able to run on their own. But it is a common to not clean up files
that are required for multiple tests and only do so at the last test.

That's even the case for files generated in a 'setup' script, they are
reused for multiple tests.


On another note, you replied to the first version of the patch while
referencing the 4th. In this case it was obvious since I used a heredoc
only in the last one, but it is not always the case. Just a heads up to
reply to the correct message-id.

Thank you for the review :)