Bug 10610 - sysprefs.sql is really boring to rebase
Summary: sysprefs.sql is really boring to rebase
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-18 13:47 UTC by Jonathan Druart
Modified: 2014-12-07 20:02 UTC (History)
4 users (show)

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


Attachments
Bug XXXXX: sysprefs.sql should be sorted by syspref names (150.59 KB, patch)
2013-07-18 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10610: sysprefs.sql should be sorted by syspref names (150.54 KB, patch)
2013-07-18 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
iBug 10610: sysprefs.sql should be sorted by syspref names (150.39 KB, patch)
2013-08-12 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
iBug 10610: sysprefs.sql should be sorted by syspref names (150.09 KB, patch)
2013-08-14 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
iBug 10610: sysprefs.sql should be sorted by syspref names (150.14 KB, patch)
2013-08-14 23:52 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
iBug 10610: sysprefs.sql should be sorted by syspref names (150.19 KB, patch)
2013-08-16 13:07 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-07-18 13:47:32 UTC
Each time a developer adds a new syspref, he must fill installer/data/mysql/sysprefs.sql. When a patch modifying this file is pushed, the patch does not apply anymore and the developer has to rebase his patch and resubmit it.

If this file is sorted by syspref names, conflicts will appear less often.
Comment 1 Jonathan Druart 2013-07-18 13:48:29 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-07-18 13:48:54 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2013-07-25 04:28:13 UTC
Applying: Bug 10610: sysprefs.sql should be sorted by syspref names
fatal: sha1 information is lacking or useless (installer/data/mysql/sysprefs.sql).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 10610: sysprefs.sql should be sorted by syspref names
Comment 4 Jonathan Druart 2013-08-12 14:11:22 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2013-08-14 01:24:11 UTC
I got 414 with the current script (master a36b3ad43a6a91f800d0d8a148a217d738236d7e), and 416 with the patch. Can you please check, and if you don't get the same I'll dump and compare.
Comment 6 Jonathan Druart 2013-08-14 07:27:05 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2013-08-14 07:28:01 UTC
(In reply to Srdjan Jankovic from comment #5)
> I got 414 with the current script (master
> a36b3ad43a6a91f800d0d8a148a217d738236d7e), and 416 with the patch. Can you
> please check, and if you don't get the same I'll dump and compare.

Oops, I suspect I was not on master when I generate the file.
Comment 8 Srdjan Jankovic 2013-08-14 23:52:45 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2013-08-16 13:07:20 UTC
Created attachment 20408 [details] [review]
iBug 10610: sysprefs.sql should be sorted by syspref names

By sorting the sql syspref file, the developpers will be happy to avoid
conflicts on this file each time a patch modifying it is pushed.

Test plan:
- create a new DB with a new table named systempreferences (with the
  same structure as the one you know).
- insert the current sysprefs.sql file into this table.
- note the number of rows in the systempreferences table.
- apply this patch.
- delete all rows of the systempreferences table.
- insert the new sysprefs.sql file.
- verify the number of rows is the same as the previous.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2013-08-16 13:08:35 UTC
Excellent idea! I wonder if there is any way to add a check for out of order sysprefs in koha-qa.pl, after this patch has been pushed.
Comment 11 Jonathan Druart 2013-08-16 13:26:21 UTC
(In reply to Kyle M Hall from comment #10)
> Excellent idea! I wonder if there is any way to add a check for out of order
> sysprefs in koha-qa.pl, after this patch has been pushed.

Yep, it should certainly be possible to do something.
Comment 12 Galen Charlton 2013-08-16 14:11:31 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Kyle M Hall from comment #10)
> > Excellent idea! I wonder if there is any way to add a check for out of order
> > sysprefs in koha-qa.pl, after this patch has been pushed.
> 
> Yep, it should certainly be possible to do something.

I like the idea of this patch, and will push it shortly unless I find something wrong, but I'm opposed  to putting a sorting requirement into koha-qa.pl.  Sorting the script provides a small convenience by reducing the chance of merge conflicts, but doesn't per se provide a functional benefit.  I'd really hate to see a patch fail QA on account of a new syspref getting inserted in the "wrong" place.
Comment 13 Jonathan Druart 2013-08-16 14:17:38 UTC
(In reply to Galen Charlton from comment #12)
> (In reply to Jonathan Druart from comment #11)
> > (In reply to Kyle M Hall from comment #10)
> > > Excellent idea! I wonder if there is any way to add a check for out of order
> > > sysprefs in koha-qa.pl, after this patch has been pushed.
> > 
> > Yep, it should certainly be possible to do something.
> 
> I like the idea of this patch, and will push it shortly unless I find
> something wrong, but I'm opposed  to putting a sorting requirement into
> koha-qa.pl.  Sorting the script provides a small convenience by reducing the
> chance of merge conflicts, but doesn't per se provide a functional benefit. 
> I'd really hate to see a patch fail QA on account of a new syspref getting
> inserted in the "wrong" place.

I rather imagined that as a warning than a blocker stuff. The change could be made by the QAer with a small followup if the pref is not inserted in the good place.
Comment 14 Kyle M Hall 2013-08-16 16:41:24 UTC
Agreed, it's like horseshoes and hand-grenades. "Close enough" will do just fine.

Kyle

(In reply to Jonathan Druart from comment #13)
> (In reply to Galen Charlton from comment #12)
> > (In reply to Jonathan Druart from comment #11)
> > > (In reply to Kyle M Hall from comment #10)
> > > > Excellent idea! I wonder if there is any way to add a check for out of order
> > > > sysprefs in koha-qa.pl, after this patch has been pushed.
> > > 
> > > Yep, it should certainly be possible to do something.
> > 
> > I like the idea of this patch, and will push it shortly unless I find
> > something wrong, but I'm opposed  to putting a sorting requirement into
> > koha-qa.pl.  Sorting the script provides a small convenience by reducing the
> > chance of merge conflicts, but doesn't per se provide a functional benefit. 
> > I'd really hate to see a patch fail QA on account of a new syspref getting
> > inserted in the "wrong" place.
> 
> I rather imagined that as a warning than a blocker stuff. The change could
> be made by the QAer with a small followup if the pref is not inserted in the
> good place.
Comment 15 Galen Charlton 2013-08-16 18:47:04 UTC
Pushed to master, thanks Jonathan!

I'm OK with adding a warning to qa-tools provided that it never becomes a blocker.