On Thu, 23 Apr 2020 at 02:37, Amit Langote <amitlangote09@gmail.com> wrote:
> Regarding the patch, I had been assuming that the "pa" in
> pa_subpaths_valid stands for "parallel append", so it using the
> variable as is in the new code structure would be misleading. Maybe,
> parallel_subpaths_valid?
I started making another pass over this patch again. I did the
renaming you've mentioned plus the other two List variables for the
subpaths.
I did notice that I had forgotten to properly disable parallel append
when it was disallowed by the rel's consider_parallel flag. For us to
have done something wrong, we'd have needed a parallel safe path in
the pathlist and also have needed the rel to be consider_parallel ==
false. It was easy enough to make up a case for that by sticking a
parallel restrict function in the target list. I've fixed that issue
in the attached and also polished up the comments a little and removed
the unused variable that I had forgotten about.
For now. I'd still like to get a bit more confidence that the only
noticeable change in the outcome here is that we're now pulling up
sub-Appends in all cases. I've read the code a number of times and
just can't quite see any room for anything changing. My tests per
Robert's case all matched what the previous version did, but I'm still
only about 93% on this. Given that I'm aiming to fix a bug in master,
v11 and v12 here, I need to get that confidence level up to about the
100% mark.
I've attached the v2 patch.
David