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

From: Mirko Faina <mroik@delayed.space>
Date: Mar 10, 2026, 04:45 UTC
Message-ID: <aa-eXgsUnQRV7nvZ@exploit>
In-Reply-To: <xmqqwlzkxsv5.fsf@gitster.g>

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.

Show 7 quoted lines

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

Show 27 quoted lines

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