[Bug 13499] New: Auth.pm needs a tidy
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Bug ID: 13499 Summary: Auth.pm needs a tidy Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Authentication Assignee: gmcharlt@gmail.com Reporter: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org CC: dpavlin@rot13.org 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 34825 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34825&action=edit 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 are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |12954 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |12548 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13503 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #2 from M. Tompsett <mtompset@hotmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #34825|0 |1 is obsolete| | --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 34968 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34968&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13521 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I've now opened a bug (13521) to record code issues, but haven't yet submitted a patch. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #34968|0 |1 is obsolete| | --- Comment #10 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 35338 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35338&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |martin.renvoize@ptfs-europe | |.com Patch complexity|--- |Trivial patch CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35338|0 |1 is obsolete| | --- Comment #11 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 35363 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35363&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | --- Comment #12 from M. 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #13 from Jonathan Druart <jonathan.druart@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|Passed QA |Pushed to Master --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Enhancement pushed to master. Looking forward for this work on making C4::Auth better! Thanks Martin! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Thanks for pushing.. should make life a bit easier. I'll get to re-basing the various bugs as promised this coming week :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13499 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris@bigballofwax.co.nz Status|Pushed to Master |Pushed to Stable --- Comment #16 from Chris Cormack <chris@bigballofwax.co.nz> --- Pushed to 3.18.x will be in 3.18.4. Pushing this because I want bug 12954 which depends on this. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org