From: Mirko Faina <mroik@delayed.space>
Date: Tue, 10 Mar 2026 04:45:04 GMT
Subject: Re: [PATCH v2] apply.c: fix -p argument parsing
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.

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