Bug 13499 - Auth.pm needs a tidy
Summary: Auth.pm needs a tidy
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 12548 12954 13503 13521
  Show dependency treegraph
 
Reported: 2014-12-30 09:40 UTC by Martin Renvoize
Modified: 2015-12-03 22:02 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13499: Tidy of Auth.pm (108.35 KB, patch)
2014-12-30 09:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 13499: Tidy of Auth.pm (108.55 KB, patch)
2015-01-05 22:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 13499: Tidy of Auth.pm (108.60 KB, patch)
2015-01-18 05:24 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13499: Tidy of Auth.pm (108.70 KB, patch)
2015-01-19 09:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2014-12-30 09:40:41 UTC
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.
Comment 1 Martin Renvoize 2014-12-30 09:52:15 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2015-01-05 18:06:52 UTC
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
Comment 3 Martin Renvoize 2015-01-05 18:53:19 UTC
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
Comment 4 Mark Tompsett 2015-01-05 19:45:17 UTC
(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.
Comment 5 Martin Renvoize 2015-01-05 22:57:38 UTC Comment hidden (obsolete)
Comment 6 Martin Renvoize 2015-01-05 22:58:59 UTC
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
Comment 7 Mark Tompsett 2015-01-06 06:09:03 UTC
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?
Comment 8 Martin Renvoize 2015-01-06 06:38:29 UTC
I've now opened a bug (13521) to record code issues, but haven't yet submitted a patch.
Comment 9 Martin Renvoize 2015-01-06 07:01:03 UTC
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 :)
Comment 10 Chris Cormack 2015-01-18 05:24:04 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2015-01-19 09:03:50 UTC
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>
Comment 12 Marcel de Rooy 2015-01-19 09:05:40 UTC
Looks good to me.
Checked git-blame output.
Running diff -w -B with both versions convinced me :)

Passed QA
Comment 13 Jonathan Druart 2015-01-20 15:49:22 UTC
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.
Comment 14 Tomás Cohen Arazi 2015-01-24 19:28:14 UTC
Enhancement pushed to master. Looking forward for this work on making
C4::Auth better!

Thanks Martin!
Comment 15 Martin Renvoize 2015-01-25 10:32:01 UTC
Thanks for pushing.. should make life a bit easier.

I'll get to re-basing the various bugs as promised this coming week :)
Comment 16 Chris Cormack 2015-02-02 18:45:02 UTC
Pushed to 3.18.x will be in 3.18.4.  Pushing this because I want bug 12954 which depends on this.