trigger_files doesn't seem to work when a PR is opened

[camelid]

For example: rust-lang/rust#91833

That PR was force-pushed here: rust-lang/rust#91833 (comment)

Both the old and the new versions modified rustdoc, but only after the force-push did the PR get autolabeled:

@Mark-Simulacrum I suspect there's something wrong in diff_between(). For one, it uses unwrap_or_default for the commit hashes, which seems like it'd introduce weird behavior. If the value isn't there, it'll just use an empty commit hash.

When you get a chance, could you look at the logs for that PR perhaps? Other than that, I don't know how to fix this.

[camelid]

Actually, autolabeling just seems broken altogether; it just added T-rustdoc to rust-lang/rust#91819.

[klensy]

This rust-lang/rust#91825 (comment) can be related?

[camelid]

No, that was fixed already.

[Mark-Simulacrum]

Okay, after some trial and error I think this is fixed on the current deploy; tests with force-pushes worked here rust-lang/rust#72571 (comment)

I haven't tested opening a new PR yet -- but I think we can move ahead on rust-lang/rust#91819 at least.

[camelid]

PR approved :)

[camelid]

I just realized a potential issue that may come up with this feature: If rustbot incorrectly labels a PR with, e.g., T-compiler, then every time the PR is pushed to, the label will have to be manually removed. I imagine it won't be an issue too often in practice though.

[Mark-Simulacrum]

Without storing metadata in the database I'm not sure how much better we can do. It'd theoretically be pretty cheap to add the database state but I'm a little reluctant to introduce that complexity.

That said it would be relatively simple to check just the pre-sync commit, I suppose, to see if that was already enough that the label should have been added..

In general it doesn't seem worth it to me though.

[camelid]

Yeah, I think we should only try to "fix" it if we run into issues.

[camelid]

I haven't tested opening a new PR yet

Ok, I just opened a new PR and the autolabeling didn't work. So I think there's still a bug in that part of the code.

Also, highfive thinks I'm a new contributor for some reason?

[Mark-Simulacrum]

Also, highfive thinks I'm a new contributor for some reason?

IIRC, this is based on the last ~100 PRs or something like that? Not sure.

Ok, I just opened a new PR and the autolabeling didn't work. So I think there's still a bug in that part of the code.

Yeah, seems so. Looking at that particular event, it looks like we .. just didn't deserialize the pull_request field. I think that's another case of serde not having compile-time duplicate detection, pull_request was already alias'd to the issue field. Pushing a fix shortly (done, ~15 minutes till production).

[camelid]

Fix confirmed to work: rust-lang/rust#91876

[Mark-Simulacrum]

rust-lang/rust#89831 (comment) has T-rustdoc being added spuriously(?)

[Mark-Simulacrum]

That generated an event with before/after commits of rust-lang/rust@62e7bcd and rust-lang/rust@314027b, which had an intermediate rebase over a bunch of commits.

Direct comparison rust-lang/rust@62e7bcd...314027b produces a diff that contains rustdoc files, so the bug lies in comparing the two. Not sure if GitHub supports a merge-base comparison...

[Mark-Simulacrum]

I suspect the immediate fix here is to use base/head always, not try to only apply labels on the just-pushed changes. It does mean we get more accidental labeling per #1535 (comment), but my guess is not significantly more, so I'm not too worried by it. Pushed a commit up to #1540 to do this.