-
Notifications
You must be signed in to change notification settings - Fork 68
Implement :from filter #1519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Implement :from filter #1519
Conversation
b8ff019 to
d02579b
Compare
1736730 to
9ead7f6
Compare
a371dc6 to
b5f262d
Compare
b5f262d to
1b16b18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| filter_concat = { | ||
| CMD_START ~ "concat" ~ "(" |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter_concat rule incorrectly uses "from" as its keyword instead of "concat". This will cause parsing conflicts since both filter_from and filter_concat would match the same input syntax :from(...). The keyword should be "concat" to match the intended :concat(...) syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already fixed
| ~ (rev ~ filter_spec)? | ||
| ~ (CMD_SEP+ ~ (rev ~ filter_spec))* |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The grammar for filter_from is more permissive than the implementation. The grammar allows optional arguments and multiple comma-separated pairs with (rev ~ filter_spec)? ~ (CMD_SEP+ ~ (rev ~ filter_spec))*, but the parsing code (lines 184-193 in parse.rs) requires exactly 2 elements. The grammar should enforce exactly one rev and one filter_spec by using rev ~ filter_spec without the optional ? and without the repetition *.
| ~ (rev ~ filter_spec)? | |
| ~ (CMD_SEP+ ~ (rev ~ filter_spec))* | |
| ~ rev ~ filter_spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to combine all those similar rules into one eventually so I'd like to keep it the same as the others.
| ~ (rev ~ filter_spec)? | ||
| ~ (CMD_SEP+ ~ (rev ~ filter_spec))* |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The grammar for filter_concat is more permissive than the implementation. The grammar allows optional arguments and multiple comma-separated pairs with (rev ~ filter_spec)? ~ (CMD_SEP+ ~ (rev ~ filter_spec))*, but the parsing code (lines 198-204 in parse.rs) requires exactly 2 elements. The grammar should enforce exactly one rev and one filter_spec by using rev ~ filter_spec without the optional ? and without the repetition *.
| ~ (rev ~ filter_spec)? | |
| ~ (CMD_SEP+ ~ (rev ~ filter_spec))* | |
| ~ rev ~ filter_spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to keep it like that for now to make it easier to see the similarity with other rules.
docs/src/reference/filters.md
Outdated
| Produce a history that keeps the original history leading up to the specified commit `<sha>` unchanged, | ||
| but applies the given `:filter` to all commits from that commit onwards. |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation is ambiguous about whether the specified commit itself is filtered. Based on the test case (concat.t), the specified commit remains unchanged and only commits AFTER it are filtered. The phrase "from that commit onwards" suggests the commit itself is filtered, but the actual behavior keeps it unchanged. Consider rephrasing to: "Produce a history that keeps the original history up to and including the specified commit <sha> unchanged, but applies the given :filter to all commits after that commit."
| Produce a history that keeps the original history leading up to the specified commit `<sha>` unchanged, | |
| but applies the given `:filter` to all commits from that commit onwards. | |
| Produce a history that keeps the original history up to and including the specified commit `<sha>` unchanged, | |
| but applies the given `:filter` to all commits after that commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
4193a7b to
0eed823
Compare
0eed823 to
ffe4854
Compare
Change: from-filter