Skip to content

Commit

Permalink
Tweak revert and verify change searches.
Browse files Browse the repository at this point in the history
If change "foo" is reworked, and one says `revert --to foo`, does one want to
revert to the the most recent version of "foo", or to the earlist version of
"foo"? Sqitch has traditionally assumed the latter; this change implements
the former, and does the same for the `--to` option to `verify`.

This might be a bit controversial, however, because some folks might decide
the latter is the more expected case (especially since, if you really want the
latest version you can say `revert --to foo@HEAD`). So this change may well
get reversed pending consensus from the community.

Meanwhile, I also noticed that changes specified as "HEAD" or "ROOT" which are
supposed to be aliases for "@Head" and "@root" since v0.991, does not actually
work. This change fixes this issue, as well. This fix likely won't be
reverted, because frankly it has annoyed me for quite some time.
  • Loading branch information
theory committed Jul 13, 2017
1 parent 532a74a commit 806d633
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 15 deletions.
9 changes: 9 additions & 0 deletions Changes
Expand Up @@ -23,6 +23,15 @@ Revision history for Perl extension App::Sqitch
ignores the target (#281).
- The `--db-*` options are now more consistently applied to a target,
including when the target is specified as a URI (#293).
- `HEAD` and `ROOT` are now properly recognized as aliases for `@HEAD` and
`@ROOT`, when querying the database, too. This was supposedly done in
v0.991, but due to a bug, it wasn't really. Sorry about that.
- The `revert` command will now search for the most recent variant of a
named change , rather than the earliest. This goes along with the
convention of earlier changes generally being referred to as of the tag
that follows them (#312).
- The `verify` command now looks for the most recent variant of a `--to`
change if it's not tag-qualified.

0.9995 2016-07-27T09:23:55Z
- Taught the `add` command not to ignore the `--change` option.
Expand Down
11 changes: 7 additions & 4 deletions lib/App/Sqitch/Engine.pm
Expand Up @@ -241,7 +241,8 @@ sub revert {

if (defined $to) {
my ($change) = $self->_load_changes(
$self->change_for_key($to)
# If no tag, default to HEAD to find most recent variant.
$self->find_change( tag => 'HEAD', $self->_params_for_key($to) )
) or do {
# Not deployed. Is it in the plan?
if ( $plan->get($to) ) {
Expand Down Expand Up @@ -394,7 +395,9 @@ sub _trim_to {
my $plan = $self->plan;

# Find the change in the database.
my $to_id = $self->change_id_for_key( $key ) || hurl $ident => (
my $to_id = $self->find_change_id(
($pop ? (tag => 'HEAD') : ()), $self->_params_for_key($key)
) || hurl $ident => (
$plan->contains( $key ) ? __x(
'Change "{change}" has not been deployed',
change => $key,
Expand All @@ -410,7 +413,7 @@ sub _trim_to {
change => $key,
);

# Pope or shift changes till we find the change we want.
# Pop or shift changes till we find the change we want.
if ($pop) {
pop @{ $changes } while $changes->[-1]->id ne $to_id;
} else {
Expand Down Expand Up @@ -650,7 +653,7 @@ sub _params_for_key {
my @off = ( offset => $offset );
return ( @off, change => $cname, tag => $tag ) if $tag;
return ( @off, change_id => $cname ) if $cname =~ /^[0-9a-f]{40}$/;
return ( @off, tag => '@' . $cname ) if $cname eq 'HEAD' || $cname eq 'ROOT';
return ( @off, tag => $cname ) if $cname eq 'HEAD' || $cname eq 'ROOT';
return ( @off, change => $cname );
}

Expand Down
22 changes: 11 additions & 11 deletions t/engine.t
Expand Up @@ -1766,7 +1766,7 @@ is $@->message, __x(
is_deeply $engine->seen, [['change_id_for', {
change_id => undef,
change => 'nonexistent',
tag => undef,
tag => 'HEAD',
project => 'sql',
}]], 'Should have called change_id_for() with change name';
is_deeply +MockOutput->get_info, [], 'Nothing should have been output';
Expand All @@ -1782,9 +1782,9 @@ is $@->message, __x(
is_deeply $engine->seen, [['change_id_for', {
change_id => '8d77c5f588b60bc0f2efcda6369df5cb0177521d',
change => undef,
tag => undef,
tag => 'HEAD',
project => 'sql',
}]], 'Shoudl have called change_id_for() with change ID';
}]], 'Should have called change_id_for() with change ID';
is_deeply +MockOutput->get_info, [], 'Nothing should have been output';

# Revert an undeployed change.
Expand Down Expand Up @@ -1820,7 +1820,7 @@ is_deeply $engine->seen, [
[change_id_for => {
change_id => $changes[0]->id,
change => undef,
tag => undef,
tag => 'HEAD',
project => 'sql',
}],
[ change_offset_from_id => [$changes[0]->id, 0] ],
Expand Down Expand Up @@ -2542,7 +2542,7 @@ is_deeply $engine->seen, [
project => 'sql',
tag => undef,
} ]
], 'It should have passed the change name to change_id_for';
], 'It should have passed the change name and ROOT tag to change_id_for';

# Should get an error when it's in the plan but not the database.
throws_ok { $engine->_trim_to( 'yep', 'blah', [] ) } 'App::Sqitch::X',
Expand Down Expand Up @@ -2629,7 +2629,7 @@ is_deeply $engine->seen, [
change => $key,
change_id => undef,
project => 'sql',
tag => undef,
tag => 'HEAD',
} ],
[ change_id_offset_from_id => [$changes[-1]->id, 0]],
], 'It should have passed change -1 ID to change_id_offset_from_id';
Expand All @@ -2647,7 +2647,7 @@ is_deeply $engine->seen, [
change => $key,
change_id => undef,
project => 'sql',
tag => undef,
tag => 'HEAD',
} ],
[ change_id_offset_from_id => [$changes[-3]->id, 0]],
], 'It should have passed change -3 ID to change_id_offset_from_id';
Expand All @@ -2663,7 +2663,7 @@ is_deeply $engine->seen, [
change => $key,
change_id => undef,
project => 'sql',
tag => undef,
tag => 'HEAD',
} ],
[ change_id_offset_from_id => [$changes[-3]->id, -1]],
], 'Should pass change -3 ID and offset -1 to change_id_offset_from_id';
Expand All @@ -2679,7 +2679,7 @@ is_deeply $engine->seen, [
change => $key,
change_id => undef,
project => 'sql',
tag => undef,
tag => 'HEAD',
} ],
[ change_id_offset_from_id => [$changes[-3]->id, 1]],
], 'Should pass change -3 ID and offset 1 to change_id_offset_from_id';
Expand Down Expand Up @@ -2714,7 +2714,7 @@ is_deeply $engine->seen, [
change => undef,
change_id => undef,
project => 'sql',
tag => '@HEAD',
tag => 'HEAD',
} ],
[ change_id_offset_from_id => [$changes[2]->id, 0]],
], 'Should pass tag @HEAD to change_id_for';
Expand Down Expand Up @@ -2749,7 +2749,7 @@ is_deeply $engine->seen, [
change => undef,
change_id => undef,
project => 'sql',
tag => '@ROOT',
tag => 'ROOT',
} ],
[ change_id_offset_from_id => [$changes[2]->id, 0]],
], 'Should pass tag @ROOT to change_id_for';
Expand Down

0 comments on commit 806d633

Please sign in to comment.