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
_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
