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

From: Mirko Faina <mroik@delayed.space>
Date: Mar 15, 2026, 17:56 UTC
Message-ID: <abbv5kG15y7a9wj7@exploit>
In-Reply-To: <eda5f191-7dfa-4bc0-8ab9-225b20a5e88b@gmail.com>

On Mon, Mar 16, 2026 at 01:22:03AM +0800, Tian Yuchen wrote:

Show 5 quoted lines

> <<-\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.

Show 12 quoted lines

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

Show 16 quoted lines

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