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

[Mark Wielaard]

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.

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

> [...]
> > +	    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).

Thanks,

Mark