It's rather challenging to read and understand what's going on in Auth.pm, as a first stage to some clean up in there, I propose a non-destructive perltidy of the file. By non-destructive, I mean a perltidy with our normal rules minus any line break changes, thus allowing a 'git blame -w' to return the same results before and after the tidy, maintaining history. This should basically reset all indenting, making for an easier to read file.
Created attachment 34825 [details] [review] Bug 13499: Tidy of Auth.pm This tidy should only change whitespace and not line breaks, thus retaining history. There are no code changes, and thus there should be no regressions to test for koha wise. To test the non-destrcutive nature of the patch, run a 'git blame -w' on the file before and after the patch, they should read the same.
You didn't change the status, and I can't successfully repeat your suggested test plan. $ git reset --hard origin/master $ git blame -w C4/Auth.pm | cut -f1 -d' ' > ~/before.txt $ git bz apply 13499 ... $ git blame -w C4/Auth.pm | cut -f1 -d' '> ~/after.txt $ diff ~/before.txt ~/after.txt | less At this point there is a highly repetitive number, let's say a734e040, which corresponds to your applied patch. $ diff ~/before.txt ~/after.txt | grep "[\<\>]" | cut -f2 -d' ' | grep -v a734e040 7535f2bf 493dcede 06d19958
Hmmm, your right.. though it's still non-descructive in that all blames related to my tidy appear to simply be the addition of some blank lines to space the code out a little better. You advice here would be apreciated Mark, would you prefer an updated test plan, i.e. Before patch git blame -w Auth.pm > orig.blame After patch git blame -w Auth.pm > new.blame A diff (I tend to use vimdiff so I can understand what it's telling me) between orig.blame and new.blame should show the addition of a number of additional blank lines atributed to the tidy commit, and a number of whitespace changes on other lines but those changes should still be attributed to their original authors. I can attempt to prevent even these blank lines during the tidy, but I feel this would negate at least some of the point of doing such a tidy to aid in readability. Let me know your thoughts, and thanks for testing
(In reply to Martin Renvoize from comment #3) > Let me know your thoughts, and thanks for testing $ git reset --hard origin/master $ cp C4/Auth.pm ~/before.txt $ git bz apply 13499 ... $ cp C4/Auth.pm ~/after.txt $ diff -w ~/before.txt ~/after.txt ... Adding and removing blank lines shown, so I don't have a problem EXCEPT... you made one code correction on line 669 by removing an extra and out of place semicolon. I don't mind signing off, BUT... I really feel the semicolon fix should be in a patch by itself.
Created attachment 34968 [details] [review] Bug 13499: Tidy of Auth.pm This tidy should only change whitespace and not line breaks, thus retaining history. There are no code changes, and thus there should be no regressions to test for koha wise. To test the non-destrcutive nature of the patch, run a 'git blame -w' on the file before and after the patch. The resulting blame should include a comparabile history of the file, with only some additional blank lines being attributed to this commit. A 'git blame -wM' may also be useful for comparison purposes.
Well spotted, I'de missed the colon change that tidy had also removed. I've removed it from my patch as I agree, such an actualy code change should go in it's own bug. Cheers
And while I am noticing semicolon bugs, (In reply to Martin Renvoize from comment #6) > I've removed it from my patch as I agree, such an actualy code change should > go in it's own bug. Another semi-colon bug while looking through the code: line 353 is missing a semicolon. Did you make a bug report for the semi-colon bugs yet?
I've now opened a bug (13521) to record code issues, but haven't yet submitted a patch.
Now added two tiny patches to the new bug for those semicolon issues you spotted. I'm very impressed you spotted them, nice fine tooth comb aproach :). I intend to add a whole swatch of inline documentation to this script over the next few weeks to aid those attempting to work within it, and then hopefully improve the unit test coverage and clean out superflous code as I'm sure there are unreachable code blocks buried within but at present I can't prove it. Cheers again for looking at this first stage :)
Created attachment 35338 [details] [review] Bug 13499: Tidy of Auth.pm This tidy should only change whitespace and not line breaks, thus retaining history. There are no code changes, and thus there should be no regressions to test for koha wise. To test the non-destrcutive nature of the patch, run a 'git blame -w' on the file before and after the patch. The resulting blame should include a comparabile history of the file, with only some additional blank lines being attributed to this commit. A 'git blame -wM' may also be useful for comparison purposes. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 35363 [details] [review] Bug 13499: Tidy of Auth.pm This tidy should only change whitespace and not line breaks, thus retaining history. There are no code changes, and thus there should be no regressions to test for koha wise. To test the non-destrcutive nature of the patch, run a 'git blame -w' on the file before and after the patch. The resulting blame should include a comparabile history of the file, with only some additional blank lines being attributed to this commit. A 'git blame -wM' may also be useful for comparison purposes. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me. Checked git-blame output. Running diff -w -B with both versions convinced me :) Passed QA
I was a little bit uncertain with this patch, there are something like 25 patches modifying C4/Auth.pm in the bug queue. Most of them are FQA and won't help the rebase. But they are mainly 1 or 2 lines patches, it should not be too difficult to rebase them.
Enhancement pushed to master. Looking forward for this work on making C4::Auth better! Thanks Martin!
Thanks for pushing.. should make life a bit easier. I'll get to re-basing the various bugs as promised this coming week :)
Pushed to 3.18.x will be in 3.18.4. Pushing this because I want bug 12954 which depends on this.