[Rpm-maint] Strange test in doPatch/doUntar

[Panu Matilainen]

On 01/28/2015 06:26 PM, Panu Matilainen wrote:
> 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".

I thought there was a related ticket on this but rpm.org was down 
yesterday so couldn't get to it, here goes: http://rpm.org/ticket/176

	- Panu -