[Rpm-maint] [PATCH] Add debugsource recommends to debuginfo packages.

[Panu Matilainen]

On 09/21/2017 05:55 PM, Mark Wielaard wrote:
> On Thu, 2017-09-21 at 10:41 +0300, Panu Matilainen wrote:
>> Moving an entire function while also changing it is a bit of a no-
>> no, because it's makes it hard to see what actually changed. Moving
>> things around also obfuscates git history, which is why I prefer
>> adding a prototype to the top of the file instead. But if you need to
>> move stuff around (such as different file entirely), do so in a
>> separate commit that doesn't change anything and also state this in
>> the commit message.
>>
>> Also please split refactoring changes like this to separate commits:
>> one commit that does the necessary enhancements/changes to
>> addPackageRequires() and updates existing callers, and another commit
>> to add the new functionality. It just makes things the diffs so much
>> more obvious and nicer to review.
> 
> OK. Patch split in two and added an early prototype instead of moving
> the function around.

Thanks, much much nicer this way :)

> 
>>> @@ -2914,6 +2930,8 @@ static void filterDebuginfoPackage(rpmSpec spec, Package pkg,
>>>    	else {
>>>    	    Package dbg = cloneDebuginfoPackage(spec, pkg, maindbg);
>>>    	    dbg->fileList = files;
>>> +	    /* Recommend the debugsource package (or the main debuginfo).  */
>>> +	    addPackageRequiresRecommends(dbg, dbgsrc ?: maindbg, true);
>>   
>> Omitting middle operand of conditional expressions is a gcc
>> extension, those are best avoided in general and especially when
>> there's no actual need to use one.
> 
> I am surprised it is a GNU extension. IMHO it should be standard C. I
> like it, because it is more concise and precise than duplicating the
> condition. But OK. Changed to the long form.


I too had to check whether it actually is part of C99 before 
complaining, but AFAICS not.
https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Conditionals.html#Conditionals

I find the construct entirely alien and incomprehensible. Matter of 
(not) getting used to of course, but since it's an extension...

> 
>> [...]
>>> +	    addPackageRequiresRecommends(pkg, deplink, false);
>>   
>> Hmm, I think this would be both more obvious and generally useful if
>> you just call the function addPackageDeps() and pass the relevant
>> dependency tag instead of true/false for require/recommend, eg
>>   
>> addPackageDeps(pkg, deplink, RPMTAG_REQUIRENAME);
>> addPackageDeps(extradbg, dbgsrcpkg, RPMTAG_RECOMMENDNAME);
>>   
>> vs
>>   
>> addPackageRequiresRecommends(pkg, deplink, false);
>> addPackageRequiresRecommends(extradbg, dbgsrcpkg, true);
>>   
>> That way we wont end up with
>> addPackageRequiresRecommendsConflictsObsoltetes() one day :)
> 
> Makes sense. Changed the function name (and argument).

Revised patches applied, thanks!

	- Panu -

> 
> Thanks,
> 
> Mark
>