Optimise manglelist handling (Was: Fails if a Patch contains multiple sections matching different elements on the manglelist!)

[Olf0]

AFAICS, the function mangle_libpath fails if a Patch contains multiple sections matching different elements on the manglelist.conf (which is sourced as MANGLE_CANDIDATES in pm_apply):
The return statement is placed right before the done, hence the for loop is run once at most (i.e., it never loops).

But AFAICS, a Patch might deliberately contain multiple sections matching different elements on the manglelist.conf, thus the for loop should iterate through the elements of MANGLE_CANDIDATES.

Am I missing some aspect, or is this really a flaw worth pondering about (i.e., developing an enhancement)?

[nephros]

I agree that return statement should go.
I'll have to set up a proper test case before I can say for sure whether that's a one-liner or more changes are required. I don´t trust my brain with analyzing loops.

Edit: yeah, no, we need some more involved re-looping if we want to support "mixed" patches, i.e. patches which match more than one "candidate".

[Olf0]

yeah, no, we need some more involved re-looping if we want to support "mixed" patches, i.e. patches which match more than one "candidate".

I will ponder about it and likely come up with a suggestion.
It should not be too hard, rather getting this loop closer to general programming guidelines (i.e., "do avoid jumping out of loops, rather adapt the loop for that to become unnecessary"), which makes it simpler to comprehend.

P.S.: As this is shell code, I should be able to handle that well. What I am rather concerned of, after finding a couple of flaws in these few lines of new code, are the "mangling"-related changes elsewhere, which are way harder for me to understand due to being written in languages foreign to me. I know you partially or mostly authored them, which makes reviewing hard, but maybe after many weeks you can look at these changes / additions as if they were written by someone else; i.e., can you try to review MRs #109 and #140 again (or better, but I fail to find it: whichever MR merged the "patch-bitness" branch into "master").

[Olf0]

^{Missed to mention this:}
Yes, please do create a test case, sometime (not urgent; i.e., take your time).

[Olf0]

yeah, no, we need some more involved re-looping if we want to support "mixed" patches, i.e. patches which match more than one "candidate".

I will ponder about it and likely come up with a suggestion.

• Preparation, with current functionality unaltered (hopefully): MR [pm_apply] Prepare for multiple conversion runs #221
  Please merge and / or test that first!

• Implementing looping over all elements of the candidates: MR [pm_apply] Do convert multiple libpath references #225
  Actually this seemed to be the easier step, after proper preparation.

Please review and test.
Thanks!

P.S.:

I'll have to set up a proper test case before I can say for sure whether that's a one-liner or more changes are required.

Yes, please do create a test case, sometime (not urgent, i.e., take your time).

Are your "test cases" scripts, or performed ad-hoc and manually?
If any of your test cases are fully or mostly programmatically executable, I would appreciate much when you provide them here at GitHub, e.g. in a new test-cases top-level directory.

[nephros]

Yes, please do create a test case, sometime (not urgent, i.e., take your time).

Are your "test cases" scripts, or performed ad-hoc and manually? If any of your test cases are fully or mostly programmatically executable, I would appreciate much when you provide them here at GitHub, e.g. in a new test-cases top-level directory.

Unfortunately not.

I had begun assembling such a set of files (no automation though!) but gave up and deleted them again, thinking it would be too complicated or too error-prone to release.
So I'll have to start from scratch :/

[Olf0]

1. Fixing this issue (#220)

Please review MR #225.
I do not really mind who merges it (although at the start we had the soft convention that usually the original committer does that, which I liked), but please do not squash the two commits.

2. Test cases

Yes, please do create a test case, sometime (not urgent; i.e., take your time).

Are your "test cases" scripts, or performed ad-hoc and manually? If any of your test cases are fully or mostly programmatically executable, I would appreciate much when you provide them here at GitHub, e.g. in a new test-cases top-level directory.

Unfortunately not.
I had begun assembling such a set of files (no automation though!) but gave up and deleted them again, thinking it would be too complicated or too error-prone to release. So I'll have to start from scratch :/

^{Side note:
IMO anything helpful is worth to be published, because any one of us could be ceasing to contribute tomorrow (for a variety of reasons). Thus manually applicable test cases / Test-Patches with rough and terse notes how to use them are better than nothing.
E.g., I still would really like to obtain a clue / hint what the original considerations for PM4 were; maybe @b100dian can try to kindly ask @CODeRUS to provide a little feedback.}
As stated, "take your time" to address this topic.
Although when MR #225 is merged, I would appreciate if you test that somehow, be it fully manually and privately, because after addressing issue #17 (well, a bit more: reworking all strings; I am working on this in a branch at my fork of PM), I would like to start preparing a v3.2.1 release.

3. Re-reviewing the former "patch-bitness" branch

^{As denoted before (in the P.S. there):
What I am rather concerned of, after finding a couple of flaws in these few lines of new [shell] code [in pm_apply], are the "mangling"-related changes elsewhere, which are way harder for me to understand due to being written in a language much more foreign to me (C++).
I know you partially or mostly authored them, which makes reviewing hard, but maybe after many weeks you can look at these changes / additions as if they were written by someone else; i.e., can you try to review MRs #109 and #140 again (or better, but I fail to find it: whichever MR merged the "patch-bitness" branch into "master").}