[Rpm-maint] Strange test in doPatch/doUntar

[Panu Matilainen]

On 01/28/2015 06:07 PM, Jean Delvare wrote:
> Hi Panu,
>
> On Wed, 28 Jan 2015 17:24:00 +0200, Panu Matilainen wrote:
>> On 01/28/2015 05:08 PM, Jean Delvare wrote:
>>> Hi all,
>>>
>>> While working on the rpmbuild performance patch I send last week, I
>>> noticed something curious in file build/parsePrep.c, functions doPatch
>>> and doUntar. These two functions look quite similar in the way they are
>>> called and what they do. However they have different conditions for
>>> early exit. doPatch has:
>>>
>>>      /* On non-build parse's, file cannot be stat'd or read. */
>>>       if ((spec->flags & RPMSPEC_FORCE) || rpmFileIsCompressed(fn, &compressed) || checkOwners(fn)) goto exit;
>>>
>>> while doUntar has:
>>>
>>>       /* XXX On non-build parse's, file cannot be stat'd or read */
>>>       if (!(spec->flags & RPMSPEC_FORCE) && (rpmFileIsCompressed(fn, &compressed) || checkOwners(fn))) {
>>> 	goto exit;
>>>       }
>>>
>>> While I will gladly admit my ignorance regarding what a "non-build
>>> parse" is, I find the difference in logic rather suspect. Can anyone
>>> more familiar with the code double check if one of these conditions is
>>> incorrect?
>>
>> Non-build parse is a spec query basically, such as 'rpmspec -q foo.spec'
>> or using the API.
>
> Oh, I didn't know about rpmspec, thanks for the pointer.
>
>>> Looking at the git history, I can see that the conditions were similar
>>> up to this commit which changed the condition in doPatch:
>>>
>>> commit 54b027535f95fea9088f2cd12ed765fddb20c5c1
>>> Author: Panu Matilainen
>>> Date:   Wed Mar 18 14:53:34 2009 +0200
>>>
>>>       Don't try to parse %patch on spec query (rhbz#487855)
>>>
>>> So I would bet that the condition in doUntar is incorrect and should be
>>> fixed the same way?
>>
>> This is rpm, forget about logic ;)
>>
>> IIRC the answer is that both cases are behaving incorrectly or at least
>> sub-optimally: you'd want patches and tarballs looked at even on
>> non-build parse IF they exist locally. RPMSPEC_FORCE presence should
>> only cause it to return with non-error if the files are not locally
>> present. The current difference comes from patches being handed to
>> macros but tar-case being handled in C.
>
> OK, if everything is as it should then alright, I can move this concern
> outside of my mind ;)

I didn't exactly say its ok, quite the contrary really. But its more 
subtle and quirky than "the other condition is right and the other one 
wrong".

	- Panu -

> Thanks for your fast answer,
>