{"thread":{"id":"65185","subject":"[PATCH] apply.c: fix -p argument parsing","startedAt":"2026-03-09T23:27:35Z","lastAt":"2026-03-16T19:56:51Z","messageCount":19,"participants":["Mirko Faina","Junio C Hamano","Jeff King","Tian Yuchen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"538336","messageId":"20260309232700.553168-1-mroik@delayed.space","threadId":"65185","inReplyTo":null,"subject":"[PATCH] apply.c: fix -p argument parsing","fromName":"Mirko Faina","fromEmail":"mroik@delayed.space","sentAt":"2026-03-09T23:26:58Z","receivedAt":"2026-03-09T23:27:35Z","isPatch":true,"sender":{"key":"mroik@delayed.space","avatar":"https://avatars.githubusercontent.com/u/25752903?v=4"},"body":"\"git apply\" has an option -p that takes an integer as its argument.\nUnfortunately the function apply_option_parse_p() in charge of parsing\nthis argument uses atoi() to convert from string to integer, which\nallows a non-digit after the number (e.g. \"1q\") to be silently ignored.\nAs a consequence, an argument that does not begin with a digit silently\nbecomes a zero. Despite this command working fine when a non-positive\nargument is passed, it might be useful for the end user to know that\ntheir input contains non-digits that might've been unintended.\n\nReplace atoi() with strtol_i() to catch malformed inputs.\n\nSigned-off-by: Mirko Faina \n---\nUnlike [1], this argument doesn't overwrite an argument initialized to a\ndefault value. Instead state->p_value_known is used instead to see if\nthe p_value should be used.\n\n[1] https://lore.kernel.org/git/xmqq5y181fx0.fsf_-_@gitster.g/\n\n apply.c | 3 ++-\n t/meson.build | 1 +\n t/t4142-apply-args.sh | 31 +++++++++++++++++++++++++++++++\n t/t4142/patch | 16 ++++++++++++++++\n 4 files changed, 50 insertions(+), 1 deletion(-)\n create mode 100755 t/t4142-apply-args.sh\n create mode 100644 t/t4142/patch\n\ndiff --git a/apply.c b/apply.c\nindex b6dd1066a0..b6c9e40700 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tstate->p_value = atoi(arg);\n+\tif (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)\n+\t\tdie(\" has to be non negative an integer\");\n \tstate->p_value_known = 1;\n \treturn 0;\n }\ndiff --git a/t/meson.build b/t/meson.build\nindex 106c68df3d..d26df707cb 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -547,6 +547,7 @@ integration_tests = [\n 't4139-apply-escape.sh',\n 't4140-apply-ita.sh',\n 't4141-apply-too-large.sh',\n+ 't4142-apply-args.sh',\n 't4150-am.sh',\n 't4151-am-abort.sh',\n 't4152-am-subjects.sh',\ndiff --git a/t/t4142-apply-args.sh b/t/t4142-apply-args.sh\nnew file mode 100755\nindex 0000000000..6fe73289f2\n--- /dev/null\n+++ b/t/t4142-apply-args.sh\n@@ -0,0 +1,31 @@\n+#!/bin/bash\n+\n+test_description='git apply test for various malformed arguments\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tgit commit --allow-empty -m \"Initial commit\"\n+'\n+\n+test_expect_success 'git apply -p 1 patch' '\n+\ttest_when_finished \"rm -rf result t\" &&\n+\tgit apply -p 1 $TEST_DIRECTORY/t4142/patch &&\n+\tls -l >result &&\n+\ttest_line_count = 3 result\n+'\n+\n+test_expect_success 'git apply -p malformed patch' '\n+\ttest_must_fail git apply -p malformed $TEST_DIRECTORY/t4142/patch\n+'\n+\n+test_expect_success 'git apply -p 2q patch' '\n+\ttest_must_fail git apply -p 2q $TEST_DIRECTORY/t4142/patch\n+'\n+\n+test_expect_success 'git apply -p -1 patch' '\n+\ttest_must_fail git apply -p -1 $TEST_DIRECTORY/t4142/patch\n+'\n+\n+test_done\ndiff --git a/t/t4142/patch b/t/t4142/patch\nnew file mode 100644\nindex 0000000000..c4511bb708\n--- /dev/null\n+++ b/t/t4142/patch\n@@ -0,0 +1,16 @@\n+From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n+From: Mroik \n+Date: Mon, 9 Mar 2026 23:25:00 +0100\n+Subject: [PATCH] Test\n+\n+---\n+ t/test/test | 0\n+ 1 file changed, 0 insertions(+), 0 deletions(-)\n+ create mode 100644 t/test/test\n+\n+diff --git a/t/test/test b/t/test/test\n+new file mode 100644\n+index 0000000000..e69de29bb2\n+-- \n+2.53.0.851.ga537e3e6e9\n+\n-- \n2.53.0.851.ga537e3e6e9\n\n"},{"id":"538341","messageId":"xmqqv7f4zhzj.fsf@gitster.g","threadId":"65185","inReplyTo":"20260309232700.553168-1-mroik@delayed.space","subject":"Re: [PATCH] apply.c: fix -p argument parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-09T23:43:44Z","receivedAt":"2026-03-09T23:43:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mirko Faina writes:\n\n> \"git apply\" has an option -p that takes an integer as its argument.\n> Unfortunately the function apply_option_parse_p() in charge of parsing\n> this argument uses atoi() to convert from string to integer, which\n> allows a non-digit after the number (e.g. \"1q\") to be silently ignored.\n> As a consequence, an argument that does not begin with a digit silently\n> becomes a zero. Despite this command working fine when a non-positive\n> argument is passed, it might be useful for the end user to know that\n> their input contains non-digits that might've been unintended.\n>\n> Replace atoi() with strtol_i() to catch malformed inputs.\n>\n> Signed-off-by: Mirko Faina \n> ---\n> Unlike [1], this argument doesn't overwrite an argument initialized to a\n> default value. Instead state->p_value_known is used instead to see if\n> the p_value should be used.\n\nThe change to the code looks OK.\n\nI do not think we want to spend a test number with a file only to\nhost just a single small test, though. Can't we roll it into an\nexisting test script instead?\n\nThanks.\n\n>\n> [1] https://lore.kernel.org/git/xmqq5y181fx0.fsf_-_@gitster.g/\n>\n> apply.c | 3 ++-\n> t/meson.build | 1 +\n> t/t4142-apply-args.sh | 31 +++++++++++++++++++++++++++++++\n> t/t4142/patch | 16 ++++++++++++++++\n> 4 files changed, 50 insertions(+), 1 deletion(-)\n> create mode 100755 t/t4142-apply-args.sh\n> create mode 100644 t/t4142/patch\n>\n> diff --git a/apply.c b/apply.c\n> index b6dd1066a0..b6c9e40700 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tstate->p_value = atoi(arg);\n> +\tif (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)\n> +\t\tdie(\" has to be non negative an integer\");\n> \tstate->p_value_known = 1;\n> \treturn 0;\n> }\n> diff --git a/t/meson.build b/t/meson.build\n> index 106c68df3d..d26df707cb 100644\n> --- a/t/meson.build\n> +++ b/t/meson.build\n> @@ -547,6 +547,7 @@ integration_tests = [\n> 't4139-apply-escape.sh',\n> 't4140-apply-ita.sh',\n> 't4141-apply-too-large.sh',\n> + 't4142-apply-args.sh',\n> 't4150-am.sh',\n> 't4151-am-abort.sh',\n> 't4152-am-subjects.sh',\n> diff --git a/t/t4142-apply-args.sh b/t/t4142-apply-args.sh\n> new file mode 100755\n> index 0000000000..6fe73289f2\n> --- /dev/null\n> +++ b/t/t4142-apply-args.sh\n> @@ -0,0 +1,31 @@\n> +#!/bin/bash\n> +\n> +test_description='git apply test for various malformed arguments\n> +'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\tgit commit --allow-empty -m \"Initial commit\"\n> +'\n> +\n> +test_expect_success 'git apply -p 1 patch' '\n> +\ttest_when_finished \"rm -rf result t\" &&\n> +\tgit apply -p 1 $TEST_DIRECTORY/t4142/patch &&\n> +\tls -l >result &&\n> +\ttest_line_count = 3 result\n> +'\n> +\n> +test_expect_success 'git apply -p malformed patch' '\n> +\ttest_must_fail git apply -p malformed $TEST_DIRECTORY/t4142/patch\n> +'\n> +\n> +test_expect_success 'git apply -p 2q patch' '\n> +\ttest_must_fail git apply -p 2q $TEST_DIRECTORY/t4142/patch\n> +'\n> +\n> +test_expect_success 'git apply -p -1 patch' '\n> +\ttest_must_fail git apply -p -1 $TEST_DIRECTORY/t4142/patch\n> +'\n> +\n> +test_done\n> diff --git a/t/t4142/patch b/t/t4142/patch\n> new file mode 100644\n> index 0000000000..c4511bb708\n> --- /dev/null\n> +++ b/t/t4142/patch\n> @@ -0,0 +1,16 @@\n> +From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n> +From: Mroik \n> +Date: Mon, 9 Mar 2026 23:25:00 +0100\n> +Subject: [PATCH] Test\n> +\n> +---\n> + t/test/test | 0\n> + 1 file changed, 0 insertions(+), 0 deletions(-)\n> + create mode 100644 t/test/test\n> +\n> +diff --git a/t/test/test b/t/test/test\n> +new file mode 100644\n> +index 0000000000..e69de29bb2\n> +-- \n> +2.53.0.851.ga537e3e6e9\n> +\n"},{"id":"538343","messageId":"20260310005408.2022216-1-mroik@delayed.space","threadId":"65185","inReplyTo":"20260309232700.553168-1-mroik@delayed.space","subject":"[PATCH v2] apply.c: fix -p argument parsing","fromName":"Mirko Faina","fromEmail":"mroik@delayed.space","sentAt":"2026-03-10T00:54:07Z","receivedAt":"2026-03-10T00:54:26Z","isPatch":true,"sender":{"key":"mroik@delayed.space","avatar":"https://avatars.githubusercontent.com/u/25752903?v=4"},"body":"\"git apply\" has an option -p that takes an integer as its argument.\nUnfortunately the function apply_option_parse_p() in charge of parsing\nthis argument uses atoi() to convert from string to integer, which\nallows a non-digit after the number (e.g. \"1q\") to be silently ignored.\nAs a consequence, an argument that does not begin with a digit silently\nbecomes a zero. Despite this command working fine when a non-positive\nargument is passed, it might be useful for the end user to know that\ntheir input contains non-digits that might've been unintended.\n\nReplace atoi() with strtol_i() to catch malformed inputs.\n\nSigned-off-by: Mirko Faina \n---\n apply.c | 3 ++-\n t/t4103-apply-binary.sh | 19 +++++++++++++++++++\n t/t4103/patch | 16 ++++++++++++++++\n 3 files changed, 37 insertions(+), 1 deletion(-)\n create mode 100644 t/t4103/patch\n\ndiff --git a/apply.c b/apply.c\nindex b6dd1066a0..61df3bdcd0 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tstate->p_value = atoi(arg);\n+\tif (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)\n+\t\tdie(\" has to be a non-negative integer\");\n \tstate->p_value_known = 1;\n \treturn 0;\n }\ndiff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh\nindex 8e302a5a57..d9dc884946 100755\n--- a/t/t4103-apply-binary.sh\n+++ b/t/t4103-apply-binary.sh\n@@ -53,6 +53,25 @@ test_expect_success 'setup' '\n \t)\n '\n \n+test_expect_success 'git apply -p 1 patch' '\n+\ttest_when_finished \"rm -rf result t\" &&\n+\tgit apply -p 1 $TEST_DIRECTORY/t4103/patch &&\n+\tls -l | sed -e \"/[[:space:]]t$/!d\" >result &&\n+\ttest_line_count = 1 result\n+'\n+\n+test_expect_success 'git apply -p malformed patch' '\n+\ttest_must_fail git apply -p malformed $TEST_DIRECTORY/t4103/patch\n+'\n+\n+test_expect_success 'git apply -p 2q patch' '\n+\ttest_must_fail git apply -p 2q $TEST_DIRECTORY/t4103/patch\n+'\n+\n+test_expect_success 'git apply -p -1 patch' '\n+\ttest_must_fail git apply -p -1 $TEST_DIRECTORY/t4103/patch\n+'\n+\n test_expect_success 'stat binary diff -- should not fail.' \\\n \t'git checkout main &&\n \t git apply --stat --summary B.diff'\ndiff --git a/t/t4103/patch b/t/t4103/patch\nnew file mode 100644\nindex 0000000000..c4511bb708\n--- /dev/null\n+++ b/t/t4103/patch\n@@ -0,0 +1,16 @@\n+From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n+From: Mroik \n+Date: Mon, 9 Mar 2026 23:25:00 +0100\n+Subject: [PATCH] Test\n+\n+---\n+ t/test/test | 0\n+ 1 file changed, 0 insertions(+), 0 deletions(-)\n+ create mode 100644 t/test/test\n+\n+diff --git a/t/test/test b/t/test/test\n+new file mode 100644\n+index 0000000000..e69de29bb2\n+-- \n+2.53.0.851.ga537e3e6e9\n+\n-- \n2.53.0.851.ga537e3e6e9\n\n"},{"id":"538351","messageId":"xmqqwlzkxsv5.fsf@gitster.g","threadId":"65185","inReplyTo":"20260310005408.2022216-1-mroik@delayed.space","subject":"Re: [PATCH v2] apply.c: fix -p argument parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-10T03:31:42Z","receivedAt":"2026-03-10T03:31:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mirko Faina writes:\n\n> \"git apply\" has an option -p that takes an integer as its argument.\n> Unfortunately the function apply_option_parse_p() in charge of parsing\n> this argument uses atoi() to convert from string to integer, which\n> allows a non-digit after the number (e.g. \"1q\") to be silently ignored.\n> As a consequence, an argument that does not begin with a digit silently\n> becomes a zero. Despite this command working fine when a non-positive\n> argument is passed, it might be useful for the end user to know that\n> their input contains non-digits that might've been unintended.\n>\n> Replace atoi() with strtol_i() to catch malformed inputs.\n>\n> Signed-off-by: Mirko Faina \n> ---\n\n> apply.c | 3 ++-\n> t/t4103-apply-binary.sh | 19 +++++++++++++++++++\n> t/t4103/patch | 16 ++++++++++++++++\n> 3 files changed, 37 insertions(+), 1 deletion(-)\n> create mode 100644 t/t4103/patch\n\nCurious. It is true that we need to parse the p_value correctly\neven when we are applying a binary patch, but the problem is not\nlimited to binary patches, is it?\n\n> diff --git a/apply.c b/apply.c\n> index b6dd1066a0..61df3bdcd0 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tstate->p_value = atoi(arg);\n> +\tif (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)\n> +\t\tdie(\" has to be a non-negative integer\");\n> \tstate->p_value_known = 1;\n> \treturn 0;\n> }\n\nSounds sensible.\n\nI briefly wondered if it would have negative fallouts to change the\ntype of .p_value member to \"unsigned int\" and use strtol_ui() to\nparse it, but the amount of work this part of the code needs to do\ndoes not change that much, so such a change is of dubious value.\nIt looks like the above draws the line at the right place to stop.\n\nGreat execution.\n\n> diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh\n> index 8e302a5a57..d9dc884946 100755\n> --- a/t/t4103-apply-binary.sh\n> +++ b/t/t4103-apply-binary.sh\n> @@ -53,6 +53,25 @@ test_expect_success 'setup' '\n> \t)\n> '\n> \n> +test_expect_success 'git apply -p 1 patch' '\n> +\ttest_when_finished \"rm -rf result t\" &&\n> +\tgit apply -p 1 $TEST_DIRECTORY/t4103/patch &&\n> +\tls -l | sed -e \"/[[:space:]]t$/!d\" >result &&\n> +\ttest_line_count = 1 result\n> +'\n\nIs this saying \"in the directory there must be only a single file\nwhose name is t?\" Wouldn't it be more readable and direct to do\nsomething like\n\n\ttest_path_is_dir t\n\nor is there something more subtle going on here?\n\n> +test_expect_success 'git apply -p malformed patch' '\n> +\ttest_must_fail git apply -p malformed $TEST_DIRECTORY/t4103/patch\n> +'\n>\n> +test_expect_success 'git apply -p 2q patch' '\n> +\ttest_must_fail git apply -p 2q $TEST_DIRECTORY/t4103/patch\n> +'\n\nIf this did not fail and patch gets applied with some p_value that\nhappens to be used when we fail to parse the number, then ...\n\n> +test_expect_success 'git apply -p -1 patch' '\n> +\ttest_must_fail git apply -p -1 $TEST_DIRECTORY/t4103/patch\n> +'\n\n... it would not be clear why this step fails. Perhaps with that\nsame \"unable to parse\" p_value was used and this tried to create the\nsame file as the previous step already created, or we detected parse\nfailure. We cannot tell.\n\nIt probably is a good idea to prepare for the worst by doing\nsomething silly like\n\n\ttest_when_finished \"rm -f t/test/test test/test test\" &&\n\nat the beginning of each of these tests so that we would clean up\nwhatever we could leave behind? I dunno.\n\n> test_expect_success 'stat binary diff -- should not fail.' \\\n> \t'git checkout main &&\n> \t git apply --stat --summary B.diff'\n> diff --git a/t/t4103/patch b/t/t4103/patch\n> new file mode 100644\n> index 0000000000..c4511bb708\n> --- /dev/null\n> +++ b/t/t4103/patch\n> @@ -0,0 +1,16 @@\n> +From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n> +From: Mroik \n> +Date: Mon, 9 Mar 2026 23:25:00 +0100\n> +Subject: [PATCH] Test\n> +\n> +---\n> + t/test/test | 0\n> + 1 file changed, 0 insertions(+), 0 deletions(-)\n> + create mode 100644 t/test/test\n> +\n> +diff --git a/t/test/test b/t/test/test\n> +new file mode 100644\n> +index 0000000000..e69de29bb2\n> +-- \n> +2.53.0.851.ga537e3e6e9\n> +\n"},{"id":"538355","messageId":"aa-eXgsUnQRV7nvZ@exploit","threadId":"65185","inReplyTo":"xmqqwlzkxsv5.fsf@gitster.g","subject":"Re: [PATCH v2] apply.c: fix -p argument parsing","fromName":"Mirko Faina","fromEmail":"mroik@delayed.space","sentAt":"2026-03-10T04:45:04Z","receivedAt":"2026-03-10T04:45:08Z","isPatch":true,"sender":{"key":"mroik@delayed.space","avatar":"https://avatars.githubusercontent.com/u/25752903?v=4"},"body":"On Mon, Mar 09, 2026 at 08:31:42PM -0700, Junio C Hamano wrote:\n> Curious. It is true that we need to parse the p_value correctly\n> even when we are applying a binary patch, but the problem is not\n> limited to binary patches, is it?\n\nUsing a better regex I now realize t4120 would've been more apropriate.\nI will move the tests.\n\n> Is this saying \"in the directory there must be only a single file\n> whose name is t?\" Wouldn't it be more readable and direct to do\n> something like\n> \n> \ttest_path_is_dir t\n> \n> or is there something more subtle going on here?\n\nSorry, this approach is due to the unfamiliarity of the testing\nframework. I must've missed test_path_is_dir, the README is very dense\nso trying to find things at a glance is not the easiest (in my opinion).\n\nWill rewrite to use test_path_is_dir.\n\n> > +test_expect_success 'git apply -p malformed patch' '\n> > +\ttest_must_fail git apply -p malformed $TEST_DIRECTORY/t4103/patch\n> > +'\n> >\n> > +test_expect_success 'git apply -p 2q patch' '\n> > +\ttest_must_fail git apply -p 2q $TEST_DIRECTORY/t4103/patch\n> > +'\n> \n> If this did not fail and patch gets applied with some p_value that\n> happens to be used when we fail to parse the number, then ...\n> \n> > +test_expect_success 'git apply -p -1 patch' '\n> > +\ttest_must_fail git apply -p -1 $TEST_DIRECTORY/t4103/patch\n> > +'\n> \n> ... it would not be clear why this step fails. Perhaps with that\n> same \"unable to parse\" p_value was used and this tried to create the\n> same file as the previous step already created, or we detected parse\n> failure. We cannot tell.\n> \n> It probably is a good idea to prepare for the worst by doing\n> something silly like\n> \n> \ttest_when_finished \"rm -f t/test/test test/test test\" &&\n> \n> at the beginning of each of these tests so that we would clean up\n> whatever we could leave behind? I dunno.\n\nright, \"rm -rf t test\" should be enough, will add this cleanup code.\n\nThank you for the review :)\n"},{"id":"538359","messageId":"20260310050621.3849719-1-mroik@delayed.space","threadId":"65185","inReplyTo":"20260310005408.2022216-1-mroik@delayed.space","subject":"[PATCH v3] apply.c: fix -p argument parsing","fromName":"Mirko Faina","fromEmail":"mroik@delayed.space","sentAt":"2026-03-10T05:06:15Z","receivedAt":"2026-03-10T05:06:44Z","isPatch":true,"sender":{"key":"mroik@delayed.space","avatar":"https://avatars.githubusercontent.com/u/25752903?v=4"},"body":"\"git apply\" has an option -p that takes an integer as its argument.\nUnfortunately the function apply_option_parse_p() in charge of parsing\nthis argument uses atoi() to convert from string to integer, which\nallows a non-digit after the number (e.g. \"1q\") to be silently ignored.\nAs a consequence, an argument that does not begin with a digit silently\nbecomes a zero. Despite this command working fine when a non-positive\nargument is passed, it might be useful for the end user to know that\ntheir input contains non-digits that might've been unintended.\n\nReplace atoi() with strtol_i() to catch malformed inputs.\n\nSigned-off-by: Mirko Faina \n---\n apply.c | 3 ++-\n t/t4120-apply-popt.sh | 21 +++++++++++++++++++++\n t/t4120/patch | 16 ++++++++++++++++\n 3 files changed, 39 insertions(+), 1 deletion(-)\n create mode 100644 t/t4120/patch\n\ndiff --git a/apply.c b/apply.c\nindex b6dd1066a0..61df3bdcd0 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tstate->p_value = atoi(arg);\n+\tif (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)\n+\t\tdie(\" has to be a non-negative integer\");\n \tstate->p_value_known = 1;\n \treturn 0;\n }\ndiff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh\nindex 697e86c0ff..3fdcfecc52 100755\n--- a/t/t4120-apply-popt.sh\n+++ b/t/t4120-apply-popt.sh\n@@ -23,6 +23,27 @@ test_expect_success setup '\n \trmdir süb\n '\n \n+test_expect_success 'git apply -p 1 patch' '\n+\ttest_when_finished \"rm -rf t\" &&\n+\tgit apply -p 1 $TEST_DIRECTORY/t4120/patch &&\n+\ttest_path_is_dir t\n+'\n+\n+test_expect_success 'apply fails due to non-num -p' '\n+\ttest_when_finished \"rm -rf t test\" &&\n+\ttest_must_fail git apply -p malformed $TEST_DIRECTORY/t4120/patch\n+'\n+\n+test_expect_success 'apply fails due to trailing non-digit in -p' '\n+\ttest_when_finished \"rm -rf t test\" &&\n+\ttest_must_fail git apply -p 2q $TEST_DIRECTORY/t4120/patch\n+'\n+\n+test_expect_success 'apply fails due to negative number in -p' '\n+\ttest_when_finished \"rm -rf t test\" &&\n+\ttest_must_fail git apply -p -1 $TEST_DIRECTORY/t4120/patch\n+'\n+\n test_expect_success 'apply git diff with -p2' '\n \tcp file1.saved file1 &&\n \tgit apply -p2 patch.file\ndiff --git a/t/t4120/patch b/t/t4120/patch\nnew file mode 100644\nindex 0000000000..c4511bb708\n--- /dev/null\n+++ b/t/t4120/patch\n@@ -0,0 +1,16 @@\n+From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n+From: Mroik \n+Date: Mon, 9 Mar 2026 23:25:00 +0100\n+Subject: [PATCH] Test\n+\n+---\n+ t/test/test | 0\n+ 1 file changed, 0 insertions(+), 0 deletions(-)\n+ create mode 100644 t/test/test\n+\n+diff --git a/t/test/test b/t/test/test\n+new file mode 100644\n+index 0000000000..e69de29bb2\n+-- \n+2.53.0.851.ga537e3e6e9\n+\n-- \n2.53.0.316.gd563ecec28\n\n"},{"id":"538417","messageId":"xmqqjyvjygii.fsf@gitster.g","threadId":"65185","inReplyTo":"20260310050621.3849719-1-mroik@delayed.space","subject":"Re: [PATCH v3] apply.c: fix -p argument parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-10T13:13:09Z","receivedAt":"2026-03-10T13:13:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mirko Faina writes:\n\n> apply.c | 3 ++-\n> t/t4120-apply-popt.sh | 21 +++++++++++++++++++++\n> t/t4120/patch | 16 ++++++++++++++++\n> 3 files changed, 39 insertions(+), 1 deletion(-)\n> create mode 100644 t/t4120/patch\n\nYeah, 4120 is about the \"-p\" option, and is much better fit for the\ntests for this input validation feature. Good find.\n\nWill queue. Thanks.\n\n> diff --git a/apply.c b/apply.c\n> index b6dd1066a0..61df3bdcd0 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tstate->p_value = atoi(arg);\n> +\tif (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)\n> +\t\tdie(\" has to be a non-negative integer\");\n> \tstate->p_value_known = 1;\n> \treturn 0;\n> }\n> diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh\n> index 697e86c0ff..3fdcfecc52 100755\n> --- a/t/t4120-apply-popt.sh\n> +++ b/t/t4120-apply-popt.sh\n> @@ -23,6 +23,27 @@ test_expect_success setup '\n> \trmdir süb\n> '\n> \n> +test_expect_success 'git apply -p 1 patch' '\n> +\ttest_when_finished \"rm -rf t\" &&\n> +\tgit apply -p 1 $TEST_DIRECTORY/t4120/patch &&\n> +\ttest_path_is_dir t\n> +'\n> +\n> +test_expect_success 'apply fails due to non-num -p' '\n> +\ttest_when_finished \"rm -rf t test\" &&\n> +\ttest_must_fail git apply -p malformed $TEST_DIRECTORY/t4120/patch\n> +'\n> +\n> +test_expect_success 'apply fails due to trailing non-digit in -p' '\n> +\ttest_when_finished \"rm -rf t test\" &&\n> +\ttest_must_fail git apply -p 2q $TEST_DIRECTORY/t4120/patch\n> +'\n> +\n> +test_expect_success 'apply fails due to negative number in -p' '\n> +\ttest_when_finished \"rm -rf t test\" &&\n> +\ttest_must_fail git apply -p -1 $TEST_DIRECTORY/t4120/patch\n> +'\n> +\n> test_expect_success 'apply git diff with -p2' '\n> \tcp file1.saved file1 &&\n> \tgit apply -p2 patch.file\n> diff --git a/t/t4120/patch b/t/t4120/patch\n> new file mode 100644\n> index 0000000000..c4511bb708\n> --- /dev/null\n> +++ b/t/t4120/patch\n> @@ -0,0 +1,16 @@\n> +From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n> +From: Mroik \n> +Date: Mon, 9 Mar 2026 23:25:00 +0100\n> +Subject: [PATCH] Test\n> +\n> +---\n> + t/test/test | 0\n> + 1 file changed, 0 insertions(+), 0 deletions(-)\n> + create mode 100644 t/test/test\n> +\n> +diff --git a/t/test/test b/t/test/test\n> +new file mode 100644\n> +index 0000000000..e69de29bb2\n> +-- \n> +2.53.0.851.ga537e3e6e9\n> +\n"},{"id":"538825","messageId":"20260313001629.GA3193660@coredump.intra.peff.net","threadId":"65185","inReplyTo":"20260310050621.3849719-1-mroik@delayed.space","subject":"Re: [PATCH v3] apply.c: fix -p argument parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-13T00:16:29Z","receivedAt":"2026-03-13T00:16:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 10, 2026 at 06:06:15AM +0100, Mirko Faina wrote:\n\n> diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh\n> index 697e86c0ff..3fdcfecc52 100755\n> --- a/t/t4120-apply-popt.sh\n> +++ b/t/t4120-apply-popt.sh\n> @@ -23,6 +23,27 @@ test_expect_success setup '\n> \trmdir süb\n> '\n> \n> +test_expect_success 'git apply -p 1 patch' '\n> +\ttest_when_finished \"rm -rf t\" &&\n> +\tgit apply -p 1 $TEST_DIRECTORY/t4120/patch &&\n> +\ttest_path_is_dir t\n> +'\n\nThis test seems to fail on Windows. From CI:\n\n ++ git apply -p 1 /d/a/git/git/t/t4120/patch\n error: git diff header lacks filename information when removing 1 leading pathname component (line 14)\n error: last command exited with $?=128\n\nbut I can't figure out why (and don't have a local Windows machine to\ntest on easily).\n\n> diff --git a/t/t4120/patch b/t/t4120/patch\n\nNot related to the failure, but IMHO we should keep small data like this\nin the script itself, rather than as auxiliary files. The t/ directory\nis already quite crowded, and it is often easier to refer to it when\nit's near the tests themselves.\n\nYou don't even need the full email, just the patch part. (You might even\nbe able to reuse one of the other patches we make in the script, but I\ndidn't check).\n\nSomething like this would work. Note that it also gets rid of bare\nreferences to $TEST_DIRECTORY, which really ought to be quoted (unlike\n$TRASH_DIRECTORY, it does not always have a space, but it depends on\nwhere somebody places their clone of git.git).\n\n\n t/t4120-apply-popt.sh | 16 ++++++++++++----\n t/t4120/patch | 15 ---------------\n 2 files changed, 12 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh\nindex 3fdcfecc52..0531a19ee8 100755\n--- a/t/t4120-apply-popt.sh\n+++ b/t/t4120-apply-popt.sh\n@@ -23,25 +23,33 @@ test_expect_success setup '\n \trmdir süb\n '\n \n+test_expect_success 'setup deep subdir patch' '\n+\tcat >patch.deep <<-\\EOF\n+\tdiff --git a/t/test/test b/t/test/test\n+\tnew file mode 100644\n+\tindex 0000000000..e69de29bb2\n+\tEOF\n+'\n+\n test_expect_success 'git apply -p 1 patch' '\n \ttest_when_finished \"rm -rf t\" &&\n-\tgit apply -p 1 $TEST_DIRECTORY/t4120/patch &&\n+\tgit apply -p 1 patch.deep &&\n \ttest_path_is_dir t\n '\n \n test_expect_success 'apply fails due to non-num -p' '\n \ttest_when_finished \"rm -rf t test\" &&\n-\ttest_must_fail git apply -p malformed $TEST_DIRECTORY/t4120/patch\n+\ttest_must_fail git apply -p malformed patch.deep\n '\n \n test_expect_success 'apply fails due to trailing non-digit in -p' '\n \ttest_when_finished \"rm -rf t test\" &&\n-\ttest_must_fail git apply -p 2q $TEST_DIRECTORY/t4120/patch\n+\ttest_must_fail git apply -p 2q patch.deep\n '\n \n test_expect_success 'apply fails due to negative number in -p' '\n \ttest_when_finished \"rm -rf t test\" &&\n-\ttest_must_fail git apply -p -1 $TEST_DIRECTORY/t4120/patch\n+\ttest_must_fail git apply -p -1 patch.deep\n '\n \n test_expect_success 'apply git diff with -p2' '\ndiff --git a/t/t4120/patch b/t/t4120/patch\ndeleted file mode 100644\nindex bfccc708dd..0000000000\n--- a/t/t4120/patch\n+++ /dev/null\n@@ -1,15 +0,0 @@\n-From 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n-From: Mroik \n-Date: Mon, 9 Mar 2026 23:25:00 +0100\n-Subject: [PATCH] Test\n-\n----\n- t/test/test | 0\n- 1 file changed, 0 insertions(+), 0 deletions(-)\n- create mode 100644 t/test/test\n-\n-diff --git a/t/test/test b/t/test/test\n-new file mode 100644\n-index 0000000000..e69de29bb2\n---\n-2.53.0.851.ga537e3e6e9\n"},{"id":"538828","messageId":"20260313011259.GA3204960@coredump.intra.peff.net","threadId":"65185","inReplyTo":"20260313001629.GA3193660@coredump.intra.peff.net","subject":"Re: [PATCH v3] apply.c: fix -p argument parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-13T01:12:59Z","receivedAt":"2026-03-13T01:13:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 12, 2026 at 08:16:29PM -0400, Jeff King wrote:\n\n> > +test_expect_success 'git apply -p 1 patch' '\n> > +\ttest_when_finished \"rm -rf t\" &&\n> > +\tgit apply -p 1 $TEST_DIRECTORY/t4120/patch &&\n> > +\ttest_path_is_dir t\n> > +'\n> \n> This test seems to fail on Windows. From CI:\n> \n> ++ git apply -p 1 /d/a/git/git/t/t4120/patch\n> error: git diff header lacks filename information when removing 1 leading pathname component (line 14)\n> error: last command exited with $?=128\n> \n> but I can't figure out why (and don't have a local Windows machine to\n> test on easily).\n\nAh, I figured it out. The culprit is CRLF line endings. Naturally. :-/\n\nIt looks like there is an existing bug in apply.c when reading patches\nwith CRLF endings. Because of complicated historical reasons, parsing\nthe:\n\n diff --git a/t/test/test b/t/test/test\n\nline insists that we find the same \"t/test/test\" path at the very end of\nthe line. But instead, we find the extra CR. As a result, we leave\npatch->def_name NULL instead of filling it in with \"t/test/test\".\n\nUsually this is not too big a deal, as we can pick up the name from the\n\"---\" and \"+++\" lines. But in your patch:\n\n diff --git a/t/test/test b/t/test/test\n new file mode 100644\n index 0000000000..e69de29bb2\n\nsince there is no content, we omit them entirely. And so Git has no idea\nwhere to apply the patch (even without \"-p\" at all).\n\nI think the fix is probably:\n\ndiff --git a/apply.c b/apply.c\nindex 61df3bdcd0..62d6a1f8d7 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1295,7 +1295,7 @@ static char *git_header_name(int p_value,\n \t\t\t * (that are separated by one HT or SP we just\n \t\t\t * found) exactly match?\n \t\t\t */\n-\t\t\tif (second[len] == '\\n' && !strncmp(name, second, len))\n+\t\t\tif ((second[len] == '\\n' || second[len] == '\\r') && !strncmp(name, second, len))\n \t\t\t\treturn xmemdupz(name, len);\n \t\t}\n \t}\n\nbut I'm not sure if there are other lurking CRLF issues, or if this\nmight allow malicious input to cause confusion.\n\n\nGetting back to your patch: why is there a CRLF here in the first place?\nBecause on Windows, we check out the whole repo with CRLF conversion,\nexcept for a few known file types listed in .gitattributes. And that\nincludes your t/t4120/patch file.\n\nCoincidentally the style suggestion I made earlier, to just inline it in\nthe t4120 script itself, makes the problem go away. Because we check out\nthose scripts with bare line feeds, per .gitattributes, the file we\ncreate will also have regular line feeds.\n\nSo I would suggest doing that as a workaround. It might be worth\naddressing the CRLF header parsing problem above, too, but I think that\nshould be a separate topic.\n\n-Peff\n"},{"id":"538829","messageId":"20260313012905.GA3749719@coredump.intra.peff.net","threadId":"65185","inReplyTo":"20260313011259.GA3204960@coredump.intra.peff.net","subject":"Re: [PATCH v3] apply.c: fix -p argument parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-13T01:29:05Z","receivedAt":"2026-03-13T01:29:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 12, 2026 at 09:12:59PM -0400, Jeff King wrote:\n\n> Getting back to your patch: why is there a CRLF here in the first place?\n> Because on Windows, we check out the whole repo with CRLF conversion,\n> except for a few known file types listed in .gitattributes. And that\n> includes your t/t4120/patch file.\n> \n> Coincidentally the style suggestion I made earlier, to just inline it in\n> the t4120 script itself, makes the problem go away. Because we check out\n> those scripts with bare line feeds, per .gitattributes, the file we\n> create will also have regular line feeds.\n> \n> So I would suggest doing that as a workaround. It might be worth\n> addressing the CRLF header parsing problem above, too, but I think that\n> should be a separate topic.\n\nIn case we want to pursue the CRLF thing further, you can demonstrate it\non Linux easily with:\n\n {\n printf 'diff --git a/file b/file\\r\\n'\n printf 'old mode 100644'\n printf 'new mode 100755'\n } >patch\n git apply patch\n\nI was surprised that we wouldn't hit this case _somewhere_ in the test\nsuite already, and indeed we do. Even with a separate patch file, like\nyou have! But the tests pass due to 614f4f0f35 (Fix the remaining tests\nthat failed with core.autocrlf=true, 2017-05-09), which explicitly adds\n.gitattributes for \"t/t4101/*\", etc.\n\nSo that's another workaround for your patch: we could mark the directory\nwith .gitattributes in the same way. I still prefer inlining the patch\nin the script for style reasons, though.\n\n-Peff\n"},{"id":"538837","messageId":"20260313031950.1695103-1-mroik@delayed.space","threadId":"65185","inReplyTo":"20260310050621.3849719-1-mroik@delayed.space","subject":"[PATCH v4] apply.c: fix -p argument parsing","fromName":"Mirko Faina","fromEmail":"mroik@delayed.space","sentAt":"2026-03-13T03:19:47Z","receivedAt":"2026-03-13T03:20:01Z","isPatch":true,"sender":{"key":"mroik@delayed.space","avatar":"https://avatars.githubusercontent.com/u/25752903?v=4"},"body":"\"git apply\" has an option -p that takes an integer as its argument.\nUnfortunately the function apply_option_parse_p() in charge of parsing\nthis argument uses atoi() to convert from string to integer, which\nallows a non-digit after the number (e.g. \"1q\") to be silently ignored.\nAs a consequence, an argument that does not begin with a digit silently\nbecomes a zero. Despite this command working fine when a non-positive\nargument is passed, it might be useful for the end user to know that\ntheir input contains non-digits that might've been unintended.\n\nReplace atoi() with strtol_i() to catch malformed inputs.\n\nSigned-off-by: Mirko Faina \n---\nAs Jeff pointed out, the previous patch doesn't pass tests on windows...\nInlined as a workaround and to avoid adding additional folders to the\nexisting test directory.\n\nThank you for the review :)\n\n apply.c | 3 ++-\n t/t4120-apply-popt.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex b6dd1066a0..61df3bdcd0 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tstate->p_value = atoi(arg);\n+\tif (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)\n+\t\tdie(\" has to be a non-negative integer\");\n \tstate->p_value_known = 1;\n \treturn 0;\n }\ndiff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh\nindex 697e86c0ff..3dbccbfc03 100755\n--- a/t/t4120-apply-popt.sh\n+++ b/t/t4120-apply-popt.sh\n@@ -23,6 +23,45 @@ test_expect_success setup '\n \trmdir süb\n '\n \n+test_expect_success 'git apply -p 1 patch' '\n+\tcat >patch <<-\\EOF &&\n+\t\tFrom 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n+\t\tFrom: Mroik \n+\t\tDate: Mon, 9 Mar 2026 23:25:00 +0100\n+\t\tSubject: [PATCH] Test\n+\n+\t\t---\n+\t\t t/test/test | 0\n+\t\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t\t create mode 100644 t/test/test\n+\n+\t\tdiff --git a/t/test/test b/t/test/test\n+\t\tnew file mode 100644\n+\t\tindex 0000000000..e69de29bb2\n+\t\t-- \n+\t\t2.53.0.851.ga537e3e6e9\n+\n+\tEOF\n+\ttest_when_finished \"rm -rf t\" &&\n+\tgit apply -p 1 patch &&\n+\ttest_path_is_dir t\n+'\n+\n+test_expect_success 'apply fails due to non-num -p' '\n+\ttest_when_finished \"rm -rf t test\" &&\n+\ttest_must_fail git apply -p malformed patch\n+'\n+\n+test_expect_success 'apply fails due to trailing non-digit in -p' '\n+\ttest_when_finished \"rm -rf t test\" &&\n+\ttest_must_fail git apply -p 2q patch\n+'\n+\n+test_expect_success 'apply fails due to negative number in -p' '\n+\ttest_when_finished \"rm -rf t test patch\" &&\n+\ttest_must_fail git apply -p -1 patch\n+'\n+\n test_expect_success 'apply git diff with -p2' '\n \tcp file1.saved file1 &&\n \tgit apply -p2 patch.file\n-- \n2.53.0.931.gb56d940889\n\n"},{"id":"538838","messageId":"xmqqcy189x98.fsf@gitster.g","threadId":"65185","inReplyTo":"20260313001629.GA3193660@coredump.intra.peff.net","subject":"Re: [PATCH v3] apply.c: fix -p argument parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-13T04:19:47Z","receivedAt":"2026-03-13T04:19:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> diff --git a/t/t4120/patch b/t/t4120/patch\n>\n> Not related to the failure, but IMHO we should keep small data like this\n> in the script itself, rather than as auxiliary files. The t/ directory\n> is already quite crowded, and it is often easier to refer to it when\n> it's near the tests themselves.\n\nA very good suggestion. I actually did find it a bit annoying to\nsee an extra file there, but somehow failed to mention it in my\nreview. Creating one in a set-up step and reusing it would be just\nas simple as shipping an extra file.\n\nThanks.\n"},{"id":"538840","messageId":"xmqq34249wx3.fsf@gitster.g","threadId":"65185","inReplyTo":"20260313011259.GA3204960@coredump.intra.peff.net","subject":"Re: [PATCH v3] apply.c: fix -p argument parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-13T04:27:04Z","receivedAt":"2026-03-13T04:27:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Getting back to your patch: why is there a CRLF here in the first place?\n> Because on Windows, we check out the whole repo with CRLF conversion,\n> except for a few known file types listed in .gitattributes. And that\n> includes your t/t4120/patch file.\n\nYuck.\n\n> Coincidentally the style suggestion I made earlier, to just inline it in\n> the t4120 script itself, makes the problem go away.\n\nOf course. Joy of stumbling on ^W^Wworking with Windows. Sigh...\n\n> So I would suggest doing that as a workaround. It might be worth\n> addressing the CRLF header parsing problem above, too, but I think that\n> should be a separate topic.\n\n"},{"id":"538842","messageId":"xmqqtsuk8hrg.fsf@gitster.g","threadId":"65185","inReplyTo":"20260313031950.1695103-1-mroik@delayed.space","subject":"Re: [PATCH v4] apply.c: fix -p argument parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-13T04:39:47Z","receivedAt":"2026-03-13T04:39:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mirko Faina writes:\n\n> As Jeff pointed out, the previous patch doesn't pass tests on windows...\n> Inlined as a workaround and to avoid adding additional folders to the\n> existing test directory.\n\nThanks for working very well together.\n> +test_expect_success 'git apply -p 1 patch' '\n> +\tcat >patch <<-\\EOF &&\n> +\t\tFrom 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n> +\t\tFrom: Mroik \n> +\t\tDate: Mon, 9 Mar 2026 23:25:00 +0100\n> +\t\tSubject: [PATCH] Test\n> +\n> +\t\t---\n> +\t\t t/test/test | 0\n> +\t\t 1 file changed, 0 insertions(+), 0 deletions(-)\n> +\t\t create mode 100644 t/test/test\n> +\n> +\t\tdiff --git a/t/test/test b/t/test/test\n> +\t\tnew file mode 100644\n> +\t\tindex 0000000000..e69de29bb2\n> +\t\t-- \n> +\t\t2.53.0.851.ga537e3e6e9\n> +\n> +\tEOF\n\nIt is more customary to indent the here-doc body to the same level\nas surrounding < +\ttest_when_finished \"rm -rf t\" &&\n> +\tgit apply -p 1 patch &&\n> +\ttest_path_is_dir t\n> +'\n> +\n> +test_expect_success 'apply fails due to non-num -p' '\n> +\ttest_when_finished \"rm -rf t test\" &&\n> +\ttest_must_fail git apply -p malformed patch\n> +'\n> +\n> +test_expect_success 'apply fails due to trailing non-digit in -p' '\n> +\ttest_when_finished \"rm -rf t test\" &&\n> +\ttest_must_fail git apply -p 2q patch\n> +'\n> +\n> +test_expect_success 'apply fails due to negative number in -p' '\n> +\ttest_when_finished \"rm -rf t test patch\" &&\n> +\ttest_must_fail git apply -p -1 patch\n> +'\n\nThe test all make sense, but if we know what error message we are\nexpecting, it may not be a bad idea to do something like\n\n\ttest_must_fail git apply -p -1 patch 2>err &&\n\ttest_grep \" has to be a non-negative\" err\n\nto ensure that the command did not fail for a wrong reason.\n\nTHanks.\n\n\n"},{"id":"539042","messageId":"eda5f191-7dfa-4bc0-8ab9-225b20a5e88b@gmail.com","threadId":"65185","inReplyTo":"20260309232700.553168-1-mroik@delayed.space","subject":"Re: [PATCH] apply.c: fix -p argument parsing","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-03-15T17:22:03Z","receivedAt":"2026-03-15T17:22:07Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Mirko,\n\n> +test_expect_success 'git apply -p 1 patch' '\n> +\tcat >patch <<-\\EOF &&\n> +\t\tFrom 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n\n<<-\\EOF should swallow the leading tab, but since you're using spaces \nfor indentation here, that would result in a space at the beginning of \nevery line, right? I think <<\\EOF is correct here.\n\nBut Junio said you don't need to worry about it, it's all good ;)\n\n---\n\nThe rest are just minor flaws (in my opinion) that you can safely ignore:\n\n> +\tif (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)\n> +\t\tdie(\" has to be a non-negative integer\");\n\nI think something like:\n\nif (strtol_i(arg, 10, &state->p_value) || state->p_value < 0)\ndie(_(\"option -p expects a non-negative integer, got '%s'\"), arg);\n\nmight be a bit better;\n\n> +test_expect_success 'apply fails due to trailing non-digit in -p' '\n> +\ttest_when_finished \"rm -rf t test\" &&\n> +\ttest_must_fail git apply -p 2q patch\n> +'\n> +\n> +test_expect_success 'apply fails due to negative number in -p' '\n> +\ttest_when_finished \"rm -rf t test patch\" &&\n> +\ttest_must_fail git apply -p -1 patch\n> +'\n> +\n> test_expect_success 'apply git diff with -p2' '\n> \tcp file1.saved file1 &&\n> \tgit apply -p2 patch.file\n\nThe 'patch' is created in the first test case, will it prevent the \nsubsequent test cases from running on their own?\n\nOverall, the patch itself looks good to me. Thank you!\n\nRegards,\n\nYuchen\n\n\n\n"},{"id":"539043","messageId":"abbv5kG15y7a9wj7@exploit","threadId":"65185","inReplyTo":"eda5f191-7dfa-4bc0-8ab9-225b20a5e88b@gmail.com","subject":"Re: [PATCH] apply.c: fix -p argument parsing","fromName":"Mirko Faina","fromEmail":"mroik@delayed.space","sentAt":"2026-03-15T17:56:31Z","receivedAt":"2026-03-15T17:56:35Z","isPatch":true,"sender":{"key":"mroik@delayed.space","avatar":"https://avatars.githubusercontent.com/u/25752903?v=4"},"body":"On Mon, Mar 16, 2026 at 01:22:03AM +0800, Tian Yuchen wrote:\n> <<-\\EOF should swallow the leading tab, but since you're using spaces for\n> indentation here, that would result in a space at the beginning of every\n> line, right? I think <<\\EOF is correct here.\n> \n> But Junio said you don't need to worry about it, it's all good ;)\n\nI think that's an issue with how the email is rendered. If you view in\nplaintext the raw mailbox file it does uses tabs.\n\n> \n> The rest are just minor flaws (in my opinion) that you can safely ignore:\n> \n> > +\tif (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)\n> > +\t\tdie(\" has to be a non-negative integer\");\n> \n> I think something like:\n> \n> if (strtol_i(arg, 10, &state->p_value) || state->p_value < 0)\n> die(_(\"option -p expects a non-negative integer, got '%s'\"), arg);\n> \n> might be a bit better;\n\nYou're right, users that haven't looked at the help usage in a while\nmight not realize which argument \"\" is, especially if there are\nmultiple.\n\nWill fix this\n\n> > +test_expect_success 'apply fails due to trailing non-digit in -p' '\n> > +\ttest_when_finished \"rm -rf t test\" &&\n> > +\ttest_must_fail git apply -p 2q patch\n> > +'\n> > +\n> > +test_expect_success 'apply fails due to negative number in -p' '\n> > +\ttest_when_finished \"rm -rf t test patch\" &&\n> > +\ttest_must_fail git apply -p -1 patch\n> > +'\n> > +\n> > test_expect_success 'apply git diff with -p2' '\n> > \tcp file1.saved file1 &&\n> > \tgit apply -p2 patch.file\n> \n> The 'patch' is created in the first test case, will it prevent the\n> subsequent test cases from running on their own?\n\nYou are right, the tests that come after that require 'patch' might not\nbe able to run on their own. But it is a common to not clean up files\nthat are required for multiple tests and only do so at the last test.\n\nThat's even the case for files generated in a 'setup' script, they are\nreused for multiple tests.\n\n\nOn another note, you replied to the first version of the patch while\nreferencing the 4th. In this case it was obvious since I used a heredoc\nonly in the last one, but it is not always the case. Just a heads up to\nreply to the correct message-id.\n\nThank you for the review :)\n"},{"id":"539055","messageId":"20260316005120.7079-1-mroik@delayed.space","threadId":"65185","inReplyTo":"20260313031950.1695103-1-mroik@delayed.space","subject":"[PATCH] apply.c: fix -p argument parsing","fromName":"Mirko Faina","fromEmail":"mroik@delayed.space","sentAt":"2026-03-16T00:51:16Z","receivedAt":"2026-03-16T00:51:34Z","isPatch":true,"sender":{"key":"mroik@delayed.space","avatar":"https://avatars.githubusercontent.com/u/25752903?v=4"},"body":"\"git apply\" has an option -p that takes an integer as its argument.\nUnfortunately the function apply_option_parse_p() in charge of parsing\nthis argument uses atoi() to convert from string to integer, which\nallows a non-digit after the number (e.g. \"1q\") to be silently ignored.\nAs a consequence, an argument that does not begin with a digit silently\nbecomes a zero. Despite this command working fine when a non-positive\nargument is passed, it might be useful for the end user to know that\ntheir input contains non-digits that might've been unintended.\n\nReplace atoi() with strtol_i() to catch malformed inputs.\n\nSigned-off-by: Mirko Faina \n---\nSending a new version 'cause Tian pointed out that the die message is\nnot explicit enough, and a user might not understand which option we're\nreferring to if there are multiple.\n\n apply.c | 3 ++-\n t/t4120-apply-popt.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex b6dd1066a0..52cd590bdb 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4981,7 +4981,8 @@ static int apply_option_parse_p(const struct option *opt,\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tstate->p_value = atoi(arg);\n+\tif (strtol_i(arg, 10, &state->p_value) < 0 || state->p_value < 0)\n+\t\tdie(_(\"option -p expects a non-negative integer, got '%s'\"), arg);\n \tstate->p_value_known = 1;\n \treturn 0;\n }\ndiff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh\nindex 697e86c0ff..acb5462a25 100755\n--- a/t/t4120-apply-popt.sh\n+++ b/t/t4120-apply-popt.sh\n@@ -23,6 +23,47 @@ test_expect_success setup '\n \trmdir süb\n '\n \n+test_expect_success 'git apply -p 1 patch' '\n+\tcat >patch <<-\\EOF &&\n+\tFrom 90ad11d5b2d437e82d4d992f72fb44c2227798b5 Mon Sep 17 00:00:00 2001\n+\tFrom: Mroik \n+\tDate: Mon, 9 Mar 2026 23:25:00 +0100\n+\tSubject: [PATCH] Test\n+\n+\t---\n+\t t/test/test | 0\n+\t 1 file changed, 0 insertions(+), 0 deletions(-)\n+\t create mode 100644 t/test/test\n+\n+\tdiff --git a/t/test/test b/t/test/test\n+\tnew file mode 100644\n+\tindex 0000000000..e69de29bb2\n+\t-- \n+\t2.53.0.851.ga537e3e6e9\n+\tEOF\n+\ttest_when_finished \"rm -rf t\" &&\n+\tgit apply -p 1 patch &&\n+\ttest_path_is_dir t\n+'\n+\n+test_expect_success 'apply fails due to non-num -p' '\n+\ttest_when_finished \"rm -rf t test err\" &&\n+\ttest_must_fail git apply -p malformed patch 2>err &&\n+\ttest_grep \"option -p expects a non-negative integer\" err\n+'\n+\n+test_expect_success 'apply fails due to trailing non-digit in -p' '\n+\ttest_when_finished \"rm -rf t test err\" &&\n+\ttest_must_fail git apply -p 2q patch 2>err &&\n+\ttest_grep \"option -p expects a non-negative integer\" err\n+'\n+\n+test_expect_success 'apply fails due to negative number in -p' '\n+\ttest_when_finished \"rm -rf t test err patch\" &&\n+\ttest_must_fail git apply -p -1 patch 2> err &&\n+\ttest_grep \"option -p expects a non-negative integer\" err\n+'\n+\n test_expect_success 'apply git diff with -p2' '\n \tcp file1.saved file1 &&\n \tgit apply -p2 patch.file\n-- \n2.53.0.959.g497ff81fa9\n\n"},{"id":"539056","messageId":"abdUJ4A8GUaRxwPy@exploit","threadId":"65185","inReplyTo":"20260316005120.7079-1-mroik@delayed.space","subject":"Re: [PATCH] apply.c: fix -p argument parsing","fromName":"Mirko Faina","fromEmail":"mroik@delayed.space","sentAt":"2026-03-16T00:52:29Z","receivedAt":"2026-03-16T00:52:32Z","isPatch":true,"sender":{"key":"mroik@delayed.space","avatar":"https://avatars.githubusercontent.com/u/25752903?v=4"},"body":"Sorry, forgot to mark as v5\n"},{"id":"539152","messageId":"xmqqikavo8e7.fsf@gitster.g","threadId":"65185","inReplyTo":"20260316005120.7079-1-mroik@delayed.space","subject":"Re: [PATCH] apply.c: fix -p argument parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-16T19:56:48Z","receivedAt":"2026-03-16T19:56:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mirko Faina writes:\n\n> \"git apply\" has an option -p that takes an integer as its argument.\n> Unfortunately the function apply_option_parse_p() in charge of parsing\n> this argument uses atoi() to convert from string to integer, which\n> allows a non-digit after the number (e.g. \"1q\") to be silently ignored.\n> As a consequence, an argument that does not begin with a digit silently\n> becomes a zero. Despite this command working fine when a non-positive\n> argument is passed, it might be useful for the end user to know that\n> their input contains non-digits that might've been unintended.\n>\n> Replace atoi() with strtol_i() to catch malformed inputs.\n>\n> Signed-off-by: Mirko Faina \n> ---\n> Sending a new version 'cause Tian pointed out that the die message is\n> not explicit enough, and a user might not understand which option we're\n> referring to if there are multiple.\n\nThe updated error message does look more helpful. Will replace.\n\nAlso the post-test clean-up in each test is more thorough, which is\na very good thing to see.\n\nThanks.\n"}]}