fix: handle cargo milestone update for PR from GitHub merge queue #1861
Conversation
| .filter(|commit| | ||
| // Assumptions: | ||
| // * A merge commit always has two parent commits. | ||
| // * Cargo's PR never got merged as fast-forward / rebase / squash merge. | ||
| commit.parents.len() == 2) |
There was a problem hiding this comment.
While the regex below has been proved working, this filter hasn't. This may not be necessary. I added because for preventive purpose.
src/handlers/milestone_prs.rs
Outdated
| commit.commit.message.len() | ||
| ); | ||
|
|
||
| merge_re.captures(&commit.commit.message).map(|cap| { |
There was a problem hiding this comment.
I'm wondering, would it be a good idea to only look at the first line of the commit message? I'm wondering what would happen if there is a stray mention of another issue/PR in the body somewhere, like:
fix(fix): Migrate cargo script manifests across editions (#14864)
This is a multi-line description
that happens to mention an issue (#1234)
somewhere in its text.
There was a problem hiding this comment.
Looking at this closer, this regex doesn't seem to work with a multi-line commit message. For example:
Improve error handling when PathSource is relative (#14854)
When editing dependencies with cargo, if a relative PathSource is
supplied cargo panics with "both paths are absolute". This is the
opposite of what's actually wrong leading to confusion.
Instead, use the same error formatting we use in other diff_paths calls
and return that error instead of panicking.
This is not matching here, since the $ requires the (#14854) to be at the end of the message (since regex does not have mutliline enabled by default).
There was a problem hiding this comment.
Hah! I overlooked that difference. Thanks for investigating I'll update to check the first line only then.
Cargo has migrated to GitHub merge queue: rust-lang/cargo#14718 The regex doesn't work for merge commit made by GitHub merge queue. This PR adds extra regex pattern to capture that. This regex has been proved working locally. See <ehuss/cargo-new-release#2>
weihanglo
force-pushed
the
milestone-cargo
branch
from
b98a33a
to
7cf03cd
Compare
Although this is unlikely (or impossible?) to ever happen in practice, I don't feel comfortable with crashing triagebot if it ever happens.
ehuss
force-pushed
the
milestone-cargo
branch
from
10e5a94
to
6015826
Compare
Cargo has migrated to GitHub merge queue:
rust-lang/cargo#14718
The regex doesn't work for merge commit made by GitHub merge queue.
This PR adds extra regex pattern to capture that.
This regex has been proved working locally.
See ehuss/cargo-new-release#2
cc @ehuss