Add `sqitch grep` command. #913

Ovid · 2025-11-10T15:54:49Z

After a nail-biting five years of anticipation, here's the pull request for sqitch grep.

Full tests and documentation included.

Name
    sqitch-grep - Search change scripts and display results in plan order

Synopsis
      sqitch grep [options] <search-term> [<search-term> ...]
      sqitch grep ALTER TABLE
      sqitch grep --type deploy "CREATE INDEX"
      sqitch grep --insensitive --list "drop table"
      sqitch grep --regex "foo.*bar"

sqitch help grep will give full information.

Disclaimer: while the grep.pm code is based on the code I wrote several years ago, the tests and the documentation have been written by Amazon's Kiro using Claude Sonnet 4.5.

theory · 2025-11-10T16:31:58Z

Thanks for the PR. If you don't mind:

Please squash this down to a single commit
Remove all AI junk: reviews, unreviewed code

Thanks!

Ovid · 2025-11-10T17:43:52Z

Remove all AI junk: reviews, unreviewed code

What does this mean? Do you want me to remove the reviews on this page? Happy to squash to a single commit, but I'm unsure of what you're asking above.

Ovid · 2025-11-10T20:07:51Z

@theory I've gone through the code with a fine-toothed comb. I've made a few minor changes here and there, but largely stylistic (but added some missing POD). I've squashed it all to a single commit.

Is there more you'd like me to take care of?

Ovid · 2025-11-10T20:11:23Z

Also, I listed you as the author on grep.pm. Wasn't sure if there's guidance on that, but happy to have your name there.

theory · 2025-11-11T02:43:31Z

What does this mean? Do you want me to remove the reviews on this page? Happy to squash to a single commit, but I'm unsure of what you're asking above.

I tried to delete the LLM comments, but got an error; I've reported the bug to GitHub. If you can remove them, great! But I assume this PR is stuck with them.

No, what I meant is that all the generated code needs to be exactly like all the other code in the project, and if it's not, should be removed and replaced or refactored, as appropriate. I trust you, not code generators.

Er, and it sounds like you've already done that. 🤦🏻 I'll review sometime this week, I hope.

Don't worry about the CI failures, BTW; something's broken with the Windows builds and the Snowflake builds don't work for PRs from users without the permissions for the Snowflake credentials. Though I would have thought you'd have them TBH. I'll make sure those tests pass at merged time.

theory

Thanks for this. I stopped reviewing the test after around 920 lines because I figure I've left enough comments already to drive someone mad, and a lot of the issues are the same. Sorry about that.

Overall I like the feature, and want to add it, but it needs some more TLC IMO:

Consider greping only files associated with changes and ignore others. You can potentially include other categories of files other than deploy/verify/revert by passing any other subdirectories of the top dir to Change::script_file; this would fork for test scripts, for example.
If you really want to grep all the files in those directories, not just the files associated with changes, maybe document it a bit more and explain the rationale. I suspect it's fairly rare, TBH, but am aware of some cases of \include --- though I typically recommend putting such files in a separate directory.
Take advantage of more of the methods in App::Sqitch::Plan::Change if you can. I has all kinds of stuff for normalized names, file paths (and segments), and more.
A lot of the tests are pretty weak, counting only results rather than inspecting the results, and in some places test nothing or test duplicate code that would not fail if the original code was changed. In other ways it tests more than it needs to! (Honestly just 1-2 tests to show that regular expressions are evaluated at such is enough IMO). I think it needs more massaging to properly evaluate the module.
Please use the output methods for all output and then use MockOutput to test output.

theory · 2026-01-10T21:01:16Z

lib/App/Sqitch/Command/grep.pm

+
+# VERSION
+
+has [qw/type t/] => (


Interesting, I've never seen this syntax before. Too bad it doesn't create just one field and let Getopt::Long map both names to it.

theory · 2026-01-10T21:04:55Z

lib/App/Sqitch/Command/grep.pm

+    }
+    else {


Suggested change

}

else {

} else {

theory · 2026-01-10T21:07:29Z

lib/App/Sqitch/Command/grep.pm

+        return -1 if $a =~ /^$deploy_dir/ and $b !~ /^$deploy_dir/;
+        return -1 if $a =~ /^$verify_dir/ and $b =~ /^$revert_dir/;
+        return 1  if $b =~ /^$deploy_dir/ and $a !~ /^$deploy_dir/;
+        return 1  if $b =~ /^$verify_dir/ and $a =~ /^$revert_dir/;


Should probably escape those $*_dir names in the regular expressions.

theory · 2026-01-10T21:09:29Z

lib/App/Sqitch/Command/grep.pm

+        # figure out the sort order
+        my $dir;
+        foreach ( $deploy_dir, $verify_dir, $revert_dir ) {
+            $dir = $_ if $a =~ /^$_/;


Suggested change

$dir = $_ if $a =~ /^$_/;

$dir = $_ if $a =~ /^\Q$_/;

This will false match on, say deploy/deploy.sql, no?

theory · 2026-01-10T21:12:55Z

lib/App/Sqitch/Command/grep.pm

+A lightweight version of C<grep>, this command allows you to search for files
+in your C<sqitch> directories, but it returns them in the order they were
+defined in your plan (with deploy, verify, and revert directories being sorted
+in that order).


If the goal is to search change scripts in plan order, why not use the plan to figure out which files to search, rather than File::Find::Rule?

theory · 2026-01-10T22:07:47Z

t/grep.t

+        dir(qw(t grep-fixtures deploy)),
+        'CREATE TABLE'
+    );
+    ok scalar(@files) > 0, 'List mode grep should find files with pattern';


Doesn't it also find files when not in list mode? How is this different?

theory · 2026-01-10T22:10:20Z

t/grep.t

+    # Verify the order_by hash has the expected changes
+    ok exists $order_by{roles},   'Plan should include roles change';
+    ok exists $order_by{users},   'Plan should include users change';
+    ok exists $order_by{widgets}, 'Plan should include widgets change';
+    ok exists $order_by{posts},   'Plan should include posts change';
+
+    # Verify the plan order is correct
+    is $order_by{roles},   0, 'roles should be first in plan (index 0)';
+    is $order_by{users},   1, 'users should be second in plan (index 1)';
+    is $order_by{widgets}, 2, 'widgets should be third in plan (index 2)';
+    is $order_by{posts},   3, 'posts should be fourth in plan (index 3)';


These appear to test the result of line 786, not the actual execution done in the module. I understand that the algorithm is the same, but if someone changes it in the module this test will not start failing.

theory · 2026-01-10T22:12:50Z

t/grep.t

+        return 0 if $a eq $b;
+
+        # deploy < verify < revert for different directories
+        return -1 if $a =~ /^$deploy_dir/ and $b !~ /^$deploy_dir/;


This fails on Windows because the names are not quoted. Also, like above, this test, unless I misread it, tests the logic of code duplicated from the module, not its execution in the module.

theory · 2026-01-10T22:13:24Z

t/grep.t

+          'widgets files should appear before posts files (plan order)';
+    }
+
+    # Test that files not in plan are sorted last


Why grep files that don't correspond to plan items?

theory · 2026-01-10T22:26:33Z

lib/App/Sqitch/Command/grep.pm

+        if ( open my $fh, '<:utf8_strict', $file ) {
+            while ( my $line = <$fh> ) {
+                if ( $line =~ /$regex/ ) {
+                    printf "%s:%d: %s" => $file, $., $line;


Please $self->emit or one of the other related output methods that App::Sqitch::Command provides (by delegating to App::Sqitch).

theory · 2026-01-10T22:28:51Z

Also, I listed you as the author on grep.pm. Wasn't sure if there's guidance on that, but happy to have your name there.

Fine with me for you to be listed as author TBH.

Ovid requested review from Copilot and theory November 10, 2025 15:54

This comment was marked as resolved.

Sign in to view

theory self-assigned this Nov 10, 2025

Ovid force-pushed the develop branch 3 times, most recently from 566554b to d157d95 Compare November 10, 2025 18:49

Add 'sqitch grep' command

9dc960c

Ovid force-pushed the develop branch from d157d95 to 9dc960c Compare November 10, 2025 20:06

theory requested changes Jan 10, 2026

View reviewed changes

Add sqitch grep command. #913

Are you sure you want to change the base?

Add sqitch grep command. #913

Uh oh!

Conversation

Ovid commented Nov 10, 2025

Uh oh!

This comment was marked as resolved.

Uh oh!

theory commented Nov 10, 2025

Uh oh!

Ovid commented Nov 10, 2025

Uh oh!

Ovid commented Nov 10, 2025

Uh oh!

Ovid commented Nov 10, 2025

Uh oh!

theory commented Nov 11, 2025

Uh oh!

theory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theory commented Jan 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add `sqitch grep` command. #913

Add `sqitch grep` command. #913