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

[Mirko Faina <mroik@delayed.space>]

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