Skip to content

Commit

Permalink
Fix config section case sensistivity.
Browse files Browse the repository at this point in the history
Config::GitLike treats sections case-insensitively, but subsections are
case-sensitive. So if a variable was added for a target named "FooBar", Sqitch
could not find it, becaues the config section was named
`target.FooBar.variables`, but the App::Sqitch::Config treated it as
`target.foobar.variables`. So add the `_skey` function to properly handle the
canonicalization of section keys, and add some tests to make sure it works
properly.

While at it, require Config::GitLike 1.15 on all platforms. It's nearly five
years old, so should be safe to require everywhere, and allows for the removal
of some legacy code and workarounds (some of which were looking for 1.08!).

Finally, fix the display spacing of variable keys and values in the output of
`target show` and `engine show`. No idea why it was looking at the spacing of
values instead of keys, but it resulted in warnings and improperly spaced
values.

Resolves #454.
  • Loading branch information
theory committed May 20, 2019
1 parent 9032bc1 commit 513851b
Show file tree
Hide file tree
Showing 10 changed files with 52 additions and 40 deletions.
8 changes: 8 additions & 0 deletions Changes
Expand Up @@ -41,6 +41,14 @@ Revision history for Perl extension App::Sqitch
- Clarified the role of project URIs for uniqueness: They don't allow
multiple projects with the same name, but do prevent the deployment of
a project with the same name but different URI.
- Fixed an issue where target variables could not be found when a target
name was not lowercase. Thanks to @maximejanssens for the report
(#454).
- Now require Config::GitLike 1.15 or higher.
- Fixed the indentation of variables emitted by the `show` actions of the
`target` and `engine` commands, fixing a "Negative repeat count does
nothing" warning in the process. Thanks to @maximejanssens for the
report (#454).

0.9999 2019-02-01T15:29:40Z
[Bug Fixes]
Expand Down
4 changes: 2 additions & 2 deletions dist/sqitch.spec
Expand Up @@ -14,7 +14,7 @@ BuildRequires: perl(Carp)
BuildRequires: perl(Class::XSAccessor) >= 1.18
BuildRequires: perl(Clone)
BuildRequires: perl(Config)
BuildRequires: perl(Config::GitLike) >= 1.11
BuildRequires: perl(Config::GitLike) >= 1.15
BuildRequires: perl(constant)
BuildRequires: perl(DateTime) >= 1.04
BuildRequires: perl(DateTime::TimeZone)
Expand Down Expand Up @@ -85,7 +85,7 @@ BuildRequires: perl(warnings)
Requires: perl(Class::XSAccessor) >= 1.18
Requires: perl(Clone)
Requires: perl(Config)
Requires: perl(Config::GitLike) >= 1.11
Requires: perl(Config::GitLike) >= 1.15
Requires: perl(constant)
Requires: perl(DateTime) >= 1.04
Requires: perl(DateTime::TimeZone)
Expand Down
1 change: 0 additions & 1 deletion inc/Module/Build/Sqitch.pm
Expand Up @@ -33,7 +33,6 @@ sub new {
$p{requires}{'Win32::Locale'} = 0;
$p{requires}{'Win32::ShellQuote'} = 0;
$p{requires}{'DateTime::TimeZone::Local::Win32'} = 0;
$p{build_requires}{'Config::GitLike'} = '1.15';
}
if (eval { require Hash::Merge; 1 } && $Hash::Merge::VERSION eq '0.298') {
warn join "\n", (
Expand Down
6 changes: 2 additions & 4 deletions lib/App/Sqitch/Command/engine.pm
Expand Up @@ -222,11 +222,9 @@ sub show {
$self->emit(' ', $label_for{verify}, $target->reworked_verify_dir);
my $vars = $target->variables;
if (%{ $vars }) {
my $len = max map { length } values %{ $vars };
$len--;
$_ .= ': ' . ' ' x ($len - length $_) for keys %{ $vars };
my $len = max map { length } keys %{ $vars };
$self->emit(' ', $label_for{variables});
$self->emit(" $_:" . (' ' x ($len - length $_)) . $vars->{$_})
$self->emit(" $_: " . (' ' x ($len - length $_)) . $vars->{$_})
for sort { lc $a cmp lc $b } keys %{ $vars };
} else {
$self->emit(' ', $label_for{no_variables});
Expand Down
6 changes: 2 additions & 4 deletions lib/App/Sqitch/Command/target.pm
Expand Up @@ -213,11 +213,9 @@ sub show {
$self->emit(' ', $label_for{verify}, $target->reworked_verify_dir);
my $vars = $target->variables;
if (%{ $vars }) {
my $len = max map { length } values %{ $vars };
$len--;
$_ .= ': ' . ' ' x ($len - length $_) for keys %{ $vars };
my $len = max map { length } keys %{ $vars };
$self->emit(' ', $label_for{variables});
$self->emit(" $_:" . (' ' x ($len - length $_)) . $vars->{$_})
$self->emit(" $_: " . (' ' x ($len - length $_)) . $vars->{$_})
for sort { lc $a cmp lc $b } keys %{ $vars };
} else {
$self->emit(' ', $label_for{no_variables});
Expand Down
24 changes: 11 additions & 13 deletions lib/App/Sqitch/Config.pm
Expand Up @@ -7,7 +7,7 @@ use warnings;
use Path::Class;
use Locale::TextDomain qw(App-Sqitch);
use App::Sqitch::X qw(hurl);
use Config::GitLike 1.11;
use Config::GitLike 1.15;
use utf8;

extends 'Config::GitLike';
Expand Down Expand Up @@ -54,10 +54,19 @@ sub local_file {

sub dir_file { shift->local_file }

# Section keys always have the top section lowercase, and subsections are
# left as-is.
sub _skey($) {
my $key = shift // return '';
my ($sec, $sub, $name) = Config::GitLike::_split_key($key);
return lc $key unless $sec;
return lc($sec) . '.' . join '.', grep { defined } $sub, $name;
}

sub get_section {
my ( $self, %p ) = @_;
$self->load unless $self->is_loaded;
my $section = lc $p{section} // '';
my $section = _skey $p{section};
my $data = $self->data;
return {
map {
Expand Down Expand Up @@ -181,17 +190,6 @@ Given the lowercase key from the loaded data, this method returns it in its
original case. This is like C<original_key>, only in the case where there are
multiple keys (for multivalue keys), only the first key is returned.
=begin comment
Hide <original_key>: It is defined in Config::GitLike 1.10, and only defined
here for older versions.
=head3 C<original_key>
Only provided if not inherited from Config::GitLike.
=end comment
=head1 See Also
=over
Expand Down
16 changes: 10 additions & 6 deletions t/config.t
Expand Up @@ -476,6 +476,9 @@ engine.pg.target=mydb
engine.sqlite.client=/opt/local/bin/sqlite3
engine.sqlite.registry=meta
engine.sqlite.target=devdb
foo.BAR.baz=hello
guess.Yes.No.calico=false
guess.Yes.No.red=true
revert.count=2
revert.revision=1.1
revert.to=gamma
Expand Down Expand Up @@ -507,6 +510,9 @@ core.pager=less -r
core.top_dir=migrations
core.uri=https://github.com/sqitchers/sqitch/
engine.pg.client=/usr/local/pgsql/bin/psql
foo.BAR.baz=hello
guess.Yes.No.calico=false
guess.Yes.No.red=true
revert.count=2
revert.revision=1.1
revert.to=gamma
Expand Down Expand Up @@ -775,8 +781,7 @@ throws_ok { $cmd->execute('revert.revision') } 'App::Sqitch::X',
is $@->ident, 'config', 'Num int cast exception ident should be "config"';

ok $cmd->execute('bundle.tags_only'), 'Get bundle.tags_only as bool';
is_deeply \@emit, [[$Config::GitLike::VERSION > 1.08 ? 'true' : 1]],
'Should have emitted bundle.tags_only as a bool';
is_deeply \@emit, [['true']], 'Should have emitted bundle.tags_only as a bool';
@emit = ();

# Make sure bool-or-int data type works.
Expand All @@ -797,8 +802,7 @@ is_deeply \@emit, [[1]],
@emit = ();

ok $cmd->execute('bundle.tags_only'), 'Get bundle.tags_only as bool-or-int';
is_deeply \@emit, [[$Config::GitLike::VERSION > 1.08 ? 'true' : 1]],
'Should have emitted bundle.tags_only as a bool';
is_deeply \@emit, [['true']], 'Should have emitted bundle.tags_only as a bool';
@emit = ();

##############################################################################
Expand Down Expand Up @@ -903,7 +907,7 @@ throws_ok { $cmd->execute('revert.revision') } 'App::Sqitch::X',
is $@->ident, 'config', 'Num int cast exception ident should be "config"';

ok $cmd->execute('bundle.tags_only'), 'Get bundle.tags_only as bool';
is_deeply \@emit, [['bundle.tags_only=' . ($Config::GitLike::VERSION > 1.08 ? 'true' : 1)]],
is_deeply \@emit, [['bundle.tags_only=true']],
'Should have emitted bundle.tags_only as a bool';
@emit = ();

Expand All @@ -925,7 +929,7 @@ is_deeply \@emit, [['revert.revision=1']],
@emit = ();

ok $cmd->execute('bundle.tags_only'), 'Get bundle.tags_only as bool-or-int';
is_deeply \@emit, [['bundle.tags_only=' . ($Config::GitLike::VERSION > 1.08 ? 'true' : 1)]],
is_deeply \@emit, [['bundle.tags_only=true']],
'Should have emitted bundle.tags_only as a bool';
@emit = ();

Expand Down
13 changes: 12 additions & 1 deletion t/configuration.t
Expand Up @@ -2,7 +2,7 @@

use strict;
use warnings;
use Test::More tests => 20;
use Test::More tests => 22;
#use Test::More 'no_plan';
use File::Spec;
use Test::Exception;
Expand Down Expand Up @@ -77,3 +77,14 @@ is_deeply $config->get_section(section => 'core'), {
is_deeply $config->get_section(section => 'engine.pg'), {
client => "/usr/local/pgsql/bin/psql",
}, 'get_section("engine.pg") should work';

# Make sure it works with irregular casing.
is_deeply $config->get_section(section => 'foo.BAR'), {
baz => 'hello'
}, 'get_section() whould work with capitalized subsection';

# Should work with multiple subsections and case-preserved keys.
is_deeply $config->get_section(section => 'guess.Yes.No'), {
red => 'true',
Calico => 'false',
}, 'get_section() whould work with mixed case subsections';
8 changes: 0 additions & 8 deletions t/lib/TestConfig.pm
Expand Up @@ -4,14 +4,6 @@ use warnings;
use base 'App::Sqitch::Config';
use Path::Class;

BEGIN {
# Circumvent Config::Gitlike bug on Windows.
# https://rt.cpan.org/Ticket/Display.html?id=96670
if (!$ENV{HOME} && Config::GitLike->VERSION < 1.15) {
$ENV{HOME} = '~';
}
}

# Creates and returns a new TestConfig, which inherits from
# App::Sqitch::Config. Sets nonexistent values for the file locations and
# calls update() on remaining args.
Expand Down
6 changes: 5 additions & 1 deletion t/sqitch.conf
Expand Up @@ -17,4 +17,8 @@
from = gamma
tags_only = true
dest_dir = _build/sql

[foo "BAR"]
baz = hello
[guess "Yes.No"]
red = true
Calico = false

0 comments on commit 513851b

Please sign in to comment.