Die on ambiguous change argument.

Alternate fix to #312. Applies to `revert` and `verify`. When a change is
specified by name only and the change appears in the database more than once
(becuase it has been reworked), instead of picking the first or last instance
(as was done in 806d633), raise an error, instead. When doing so, emit a
message telling the user that the change was ambiguous, and list all the
alternates with proper tag-qualification. This is done in change_id_for(),
which is now documented to rais an error on ambiguous changes. The emitted
name and tag are looked up in the database via the existing
`name_for_change_id` engine method. It looks like this method wasn't actually
being used anywhere, so also modify it to append `@HEAD` when appropriate.

Naturally, revert most of the work that went into 806d633a. Also fix a bug
in `revert` where it returned an error for duplicate change names in the
plan when it found nothing in the database. Done by changing `$plan->get`
to `$plan->find`.

Changes

@@ -26,16 +26,15 @@ Revision history for Perl extension App::Sqitch
      - `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.
-     - Sqitch no longer returns an error when specifying a target name to
-       commands when the default target's plan file does not exist (#324).
+     - The `revert` and `verify` commands will now fail if a change is
+       specified and matches multiple changes. This happens when a change has
+       been reworked and referenced only by its name. In this case, Sqitch
+       will emit an error listing properly tag-qualified changes to use.
+       Suggested by Jay Hannah (#312).
+     - Sqitch no longer returns an error when a target name is passed to a
+       command and the default target's plan file does not exist (#324).
      - Added missing options to the `rework` usage statement. Thanks to Jay
-       Hannah for the PR (#342).
+       Hannahw for the PR (#342).
      - Passing an engine name or plan file as the `<database>` parameter to
        the `log`, `status`, and `upgrade` commands now works correctly,
        matching what the documention has said for some time (#324).

lib/App/Sqitch/Engine.pm

@@ -241,11 +241,10 @@ sub revert {
 
     if (defined $to) {
         my ($change) = $self->_load_changes(
-            # If no tag, default to HEAD to find most recent variant.
-            $self->find_change( tag => 'HEAD', $self->_params_for_key($to) )
+            $self->change_for_key($to)
         ) or do {
             # Not deployed. Is it in the plan?
-            if ( $plan->get($to) ) {
+            if ( $plan->find($to) ) {
                 # Known but not deployed.
                 hurl revert => __x(
                     'Change not deployed: "{change}"',
@@ -259,6 +258,7 @@ sub revert {
             );
         };
 
+        say "Lookup";
         @changes = $self->deployed_changes_since(
             $self->_load_changes($change)
         ) or hurl {
@@ -394,10 +394,8 @@ sub _trim_to {
     my $sqitch = $self->sqitch;
     my $plan   = $self->plan;
 
-    # Find the change in the database.
-    my $to_id =  $self->find_change_id(
-        ($pop ? (tag => 'HEAD') : ()), $self->_params_for_key($key)
-    ) || hurl $ident => (
+    # Find the to change in the database.
+    my $to_id = $self->change_id_for_key( $key ) || hurl $ident => (
         $plan->contains( $key ) ? __x(
             'Change "{change}" has not been deployed',
             change => $key,
@@ -1603,7 +1601,7 @@ matches no changes.
 
 Searches the deployed changes for a change corresponding to the specified key,
 which should be in a format as described in L<sqitchchanges>, and returns the
-change's ID. Throws an exception if the key matches more than one changes.
+change's ID. Throws an exception if the key matches more than one change.
 Returns C<undef> if it matches no changes.
 
 =head3 C<change_for_key>
@@ -1809,7 +1807,8 @@ re-deployed.
 );
 
 Searches the database for the change with the specified name, tag, and offset.
-The parameters are as follows:
+Throws an exception if the key matches more than one changes. Returns C<undef>
+if it matches no changes. The parameters are as follows:
 
 =over
 
@@ -1968,9 +1967,10 @@ should be the same as for those returned by C<deployed_changes()>.
 
   my $change_name = $engine->name_for_change_id($change_id);
 
-Returns the name of the change identified by the ID argument. If a tag was
-applied to a change after that change, the name will be returned with the tag
-qualification, e.g., C<app_user@beta>. This value should be suitable for
+Returns the tag-qualified name of the change identified by the ID. If a tag
+was applied to a change after that change, the name will be returned with the
+tag qualification, e.g., C<app_user@beta>. Otherwise, it will include the
+symbolic tag C<@HEAD. e.g., C<widgets@HEAD>. This value should be suitable for
 uniquely identifying the change, and passing to the C<get> or C<index_of>
 methods of L<App::Sqitch::Plan>.
 

lib/App/Sqitch/Engine/firebird.pm

@@ -563,7 +563,7 @@ sub name_for_change_id {
               JOIN tags ON c2.change_id = tags.change_id
              WHERE c2.committed_at >= c.committed_at
                AND c2.project = c.project
-        ), '')
+        ), '@HEAD')
           FROM changes c
          WHERE change_id = ?
     }, undef, $change_id)->[0];
@@ -675,13 +675,27 @@ sub change_id_for {
         }
 
         # Find earliest by change name.
-        return $dbh->selectcol_arrayref(qq{
+        my $ids = $dbh->selectcol_arrayref(qq{
             SELECT FIRST 1 change_id
               FROM changes
              WHERE project = ?
                AND changes.change  = ?
              ORDER BY changes.committed_at ASC
-        }, undef, $project, $change)->[0];
+        }, undef, $project, $change);
+
+        # Return if 0 or 1 ID.
+        return $ids->[0] if @{ $ids } <= 1;
+
+        # Too many found! Let the user know.
+        $self->sqitch->vent(__x(
+            'Change "{change}" is ambiguous. Please specify a tag-qualified change:',
+            change => $change,
+        ));
+
+        # Lookup, emit reverse-chron list of tag-qualified changes, and die.
+        $self->sqitch->vent( '  * ', $self->name_for_change_id($_) // '' )
+            for reverse @{ $ids };
+        hurl engine => __ 'Change Lookup Failed';
     }
 
     if ( my $tag = $p{tag} ) {

lib/App/Sqitch/Engine/oracle.pm

@@ -335,7 +335,7 @@ sub name_for_change_id {
                    ROW_NUMBER() OVER (partition by project ORDER BY committed_at) AS rnk
               FROM tags
         )
-        SELECT change || COALESCE(t.tag, '')
+        SELECT change || COALESCE(t.tag, '@HEAD')
           FROM changes c
           LEFT JOIN tag t ON c.project = t.project AND t.committed_at >= c.committed_at
          WHERE change_id = ?

lib/App/Sqitch/Engine/vertica.pm

@@ -493,7 +493,7 @@ sub name_for_change_id {
                    ROW_NUMBER() OVER (partition by project ORDER BY committed_at) AS rnk
               FROM tags
         )
-        SELECT change || COALESCE(t.tag, '')
+        SELECT change || COALESCE(t.tag, '@HEAD')
           FROM changes c
           LEFT JOIN tag t ON c.project = t.project AND t.committed_at >= c.committed_at
          WHERE change_id = ?

lib/App/Sqitch/Plan.pm

@@ -729,7 +729,6 @@ sub tag {
         tag => $key
     ) if defined $changes->index_of($key);
 
-
     my $change;
     if (my $spec = $p{change}) {
         $change = $changes->get($spec) or hurl plan => __x(

lib/App/Sqitch/Role/DBIEngine.pm

@@ -618,7 +618,7 @@ sub name_for_change_id {
              WHERE c2.committed_at >= c.committed_at
                AND c2.project = c.project
              LIMIT 1
-        ), '')
+        ), '@HEAD')
           FROM changes c
          WHERE change_id = ?
     }, undef, $change_id)->[0];
@@ -901,14 +901,27 @@ sub change_id_for {
         }
 
         # Find earliest by change name.
-        my $limit = $self->_can_limit ? "\n             LIMIT 1" : '';
-        return $dbh->selectcol_arrayref(qq{
+        my $ids = $dbh->selectcol_arrayref(qq{
             SELECT change_id
               FROM changes
              WHERE project = ?
                AND changes.change  = ?
-             ORDER BY changes.committed_at ASC$limit
-        }, undef, $project, $change)->[0];
+             ORDER BY changes.committed_at ASC
+        }, undef, $project, $change);
+
+        # Return if 0 or 1 ID.
+        return $ids->[0] if @{ $ids } <= 1;
+
+        # Too many found! Let the user know.
+        $self->sqitch->vent(__x(
+            'Change "{change}" is ambiguous. Please specify a tag-qualified change:',
+            change => $change,
+        ));
+
+        # Lookup, emit reverse-chron list of tag-qualified changes, and die.
+        $self->sqitch->vent( '  * ', $self->name_for_change_id($_) // '' )
+            for reverse @{ $ids };
+        hurl engine => __ 'Change Lookup Failed';
     }
 
     if ( my $tag = $p{tag} ) {

t/engine.t

@@ -1766,7 +1766,7 @@ is $@->message, __x(
 is_deeply $engine->seen, [['change_id_for', {
     change_id => undef,
     change  => 'nonexistent',
-    tag     => 'HEAD',
+    tag     => undef,
     project => 'sql',
 }]], 'Should have called change_id_for() with change name';
 is_deeply +MockOutput->get_info, [], 'Nothing should have been output';
@@ -1782,7 +1782,7 @@ is $@->message, __x(
 is_deeply $engine->seen, [['change_id_for', {
     change_id => '8d77c5f588b60bc0f2efcda6369df5cb0177521d',
     change  => undef,
-    tag     => 'HEAD',
+    tag     => undef,
     project => 'sql',
 }]], 'Should have called change_id_for() with change ID';
 is_deeply +MockOutput->get_info, [], 'Nothing should have been output';
@@ -1820,7 +1820,7 @@ is_deeply $engine->seen, [
     [change_id_for => {
         change_id => $changes[0]->id,
         change => undef,
-        tag => 'HEAD',
+        tag => undef,
         project => 'sql',
     }],
     [ change_offset_from_id => [$changes[0]->id, 0] ],
@@ -2629,7 +2629,7 @@ is_deeply $engine->seen, [
         change => $key,
         change_id => undef,
         project => 'sql',
-        tag => 'HEAD',
+        tag => undef,
     } ],
     [ change_id_offset_from_id => [$changes[-1]->id, 0]],
 ], 'It should have passed change -1 ID to change_id_offset_from_id';
@@ -2647,7 +2647,7 @@ is_deeply $engine->seen, [
         change => $key,
         change_id => undef,
         project => 'sql',
-        tag => 'HEAD',
+        tag => undef,
     } ],
     [ change_id_offset_from_id => [$changes[-3]->id, 0]],
 ], 'It should have passed change -3 ID to change_id_offset_from_id';
@@ -2663,7 +2663,7 @@ is_deeply $engine->seen, [
         change => $key,
         change_id => undef,
         project => 'sql',
-        tag => 'HEAD',
+        tag => undef,
     } ],
     [ change_id_offset_from_id => [$changes[-3]->id, -1]],
 ], 'Should pass change -3 ID and offset -1 to change_id_offset_from_id';
@@ -2679,7 +2679,7 @@ is_deeply $engine->seen, [
         change => $key,
         change_id => undef,
         project => 'sql',
-        tag => 'HEAD',
+        tag => undef,
     } ],
     [ change_id_offset_from_id => [$changes[-3]->id, 1]],
 ], 'Should pass change -3 ID and offset 1 to change_id_offset_from_id';

t/lib/DBIEngineTest.pm

@@ -669,8 +669,8 @@ sub run {
         is_deeply all_events($engine), \@event_data,
             'The new change deploy should have been logged';
 
-        is $engine->name_for_change_id($change2->id), 'widgets',
-            'name_for_change_id() should return just the change name';
+        is $engine->name_for_change_id($change2->id), 'widgets@HEAD',
+            'name_for_change_id() should return name with symbolic tag @HEAD';
 
         ok $state = $engine->current_state, 'Get the current state again';
         isa_ok $dt = delete $state->{committed_at}, 'App::Sqitch::DateTime',
@@ -1435,8 +1435,8 @@ sub run {
         # Make sure name_for_change_id() works properly.
         ok $engine->dbh->do(q{DELETE FROM tags WHERE project = 'engine'}),
             'Delete the engine project tags';
-        is $engine->name_for_change_id($change2->id), 'widgets',
-            'name_for_change_id() should return "widgets" for its ID';
+        is $engine->name_for_change_id($change2->id), 'widgets@HEAD',
+            'name_for_change_id() should return "widgets@HEAD" for its ID';
         is $engine->name_for_change_id($ext_change2->id), 'outside_in@meta',
             'name_for_change_id() should return "outside_in@meta" for its ID';
 
@@ -1583,20 +1583,20 @@ sub run {
         $mock_dbh->unmock('do');
 
         ######################################################################
+        # Revert and re-deploy all the changes.
+        my @all_changes  = ($change, $change2, $fred, $barney, $ext_change, $ext_change2, $hyper, $ext_change3);
+        ok $engine->log_revert_change($_),
+            'Revert "' . $_->name . '" change' for reverse @all_changes;
+        ok $engine->log_deploy_change($_),
+            'Deploy "' . $_->name . '" change' for @all_changes;
+
         if ($class eq 'App::Sqitch::Engine::pg') {
             # Test _update_ids by old ID; required only for pg, which was the
             # only engine that existed at the time.
             my @proj_changes = ($change, $change2, $fred, $barney, $hyper);
-            my @all_changes  = ($change, $change2, $fred, $barney, $ext_change, $ext_change2, $hyper, $ext_change3);
             my @proj_tags    = ($change->tags, $beta, $gamma);
             my @all_tags     = (@proj_tags, $ext_tag);
 
-            # Let's just revert and re-deploy them all.
-            ok $engine->log_revert_change($_),
-                'Revert "' . $_->name . '" change' for reverse @all_changes;
-            ok $engine->log_deploy_change($_),
-                'Deploy "' . $_->name . '" change' for @all_changes;
-
             my $upd_change = $engine->dbh->prepare(
                 'UPDATE changes SET change_id = ? WHERE change_id = ?'
             );
@@ -1667,9 +1667,30 @@ sub run {
             $tmp_dir->file( $deploy_file->basename )->copy_to($deploy_file);
         };
 
-        # Make sure that change_id_for() is okay with the dupe.
-        is $engine->change_id_for( change => 'users'), $change->id,
-            'change_id_for() should find the earliest change ID';
+        # Make sure that change_id_for() chokes on the dupe.
+        MOCKVENT: {
+            my $sqitch_mocker = Test::MockModule->new(ref $sqitch);
+            my @args;
+            $sqitch_mocker->mock(vent => sub { shift; push @args => \@_ });
+            throws_ok { $engine->change_id_for( change => 'users') } 'App::Sqitch::X',
+                'Should die on ambiguous change spec';
+            is $@->ident, 'engine', 'Mode should be "engine"';
+            is $@->message, __ 'Change Lookup Failed',
+                'And it should report change lookup failure';
+            is_deeply \@args, [
+                [__x(
+                    'Change "{change}" is ambiguous. Please specify a tag-qualified change:',
+                    change => 'users',
+                )],
+                [ '  * ', $rev_change->format_name . '@HEAD' ],
+                [ '  * ', $change->format_tag_qualified_name ],
+            ], 'Should have vented output for lookup failure';
+        }
+
+        is $engine->change_id_for( change => 'users', tag => 'alpha'), $change->id,
+            'change_id_for() should find the tag-qualified change ID';
+        is $engine->change_id_for( change => 'users', tag => 'HEAD'), $rev_change->id,
+            'change_id_for() should find the reworked change ID @HEAD';
 
         ######################################################################
         # Tag and Rework the change again.
@@ -1692,8 +1713,8 @@ sub run {
         # make sure that change_id_for is still good with things.
         for my $spec (
             [
-                'first instance of change',
-                { change => 'users' },
+                'alpha instance of change',
+                { change => 'users', tag => 'alpha' },
                 $change->id,
             ],
             [