Skip to content

search: add rev: alias and separate validation and transformation

Administrator requested to merge rvt/rev-validation-3 into main

Created by: rvantonder

I started by adding the missing revision alias and felt inspired to rework the rev code into the validation function. The one reason is that previously we weren't checking for things like -rev and how rev can interact with -repo. The other reason is its nice to concentrate all validation in one place and use the existing visitor/mapper stuff for traversals, and don't have to worry about the top-level business. I made a small modification to the mapper interface to remove a node if the callback returns nil to help with this.

The only thing this PR loses is the admittedly nicer error message You have specified multiple revisions (%s) and... and replaces it with rev may not be specified more than once. We can reintroduce the nicer error message if the validation function first collects all the (possibly more than one) revisions and formats the error like before, but I didn't want to get into that.


Other notes on repo:foo repo:bar and -repo:foo@bar syntax (here be dragons):

I was tempted to remove the fact that we can specify repo:foo repo:bar and allow only one repo:foo as this is usually meaningless, but I discovered we actually have the behavior that tries to match repos whose names contain both foo and bar example (previously, my mental model mapped this and operation to evaluating whether both the repos foo and bar exist on an instance). So, to not disrupt whatever existing behavior we have here, I'm just introducing a similar rev validation as before, but taking care not to raise an error if it were something like repo:foo rev:bar -repo:baz.

I also played around with -repo:foo@bar syntax and combinations and this is typically not meaningful. For example, just searching -repo:github.com/sourcegraph/sourcegraph@HEAD I think just excludes all of github.com/sourcegraph/sourcegraph. The point is that that I think the @commit part is insignificant for -repo and thrown away, but I'm not changing the validation for that, because I don't think we have a well-defined behavior and I'm not sure. Further investigating might reveal that we should just alert on -repo:foo@bar and call it invalid.

Merge request reports

Loading