search: add rev: alias and separate validation and transformation
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.