Alternative rollup implementation for the rust branch

[barosl]

This is an alternative implementation of #46.

[frewsxcv]

Can be simplified to

p = -1 if self.batched() else 0

[barosl]

I've applied it, thanks!

[brson]

Why is this cfg.get when the surround code is not?

[brson]

Oh, I guess it's a default value.

[barosl]

I think providing a sane default when the configuration value does not exist is good. (That's what cfg.get(key, default_value) does.) So I think we might even want to replace almost all the occurrences of cfg[key] to cfg.get(key, default_value).

However, it is just a matter of taste. You could also prefer explicit failing if the configuration value is not provided. Which way do you prefer?

[brson]

This is fine as is, thanks.

[brson]

This looks good to me though the logic around self.merge_sha was a bit hard for me to follow, and my python is weak.

Unfortunately I can't test it right now. I may be able to tonight, but if not I will tomorrow.

ISTM that if the rollup fails the failure will be posted to whichever PR triggered the test, but then bors will proceed to the other rollup PRs and do the test again (which will fail again, until all the PRs have triggered a failed rollup). This would be counter-productive.

I'm wondering what the workflow is for rollups that fail. Currently rollups have a PR and if there's a failure, the rollup author fixes it manually and tries again, or removes. Under this scheme I'd imagine someone would figure out which PR caused the failure, remove the 'rollup' comment or 'r-' it, then '@bors: retry' the rollup. Does that sound right?

[brson]

With this arrangement that doesn't create a new PR it won't be possibly to use the hypothetical '@bors: try' to test a rollup before attempting to land. I'm not sure this is a problem, but my hunch is that rollups often fail a number of times since they're so big.

If it becomes desirable to try rollups it might be nice to support tagging rollups independently of r+, like one comment with '@bors: rollup' (though maybe your regexp supports just the word 'rollup'?).

[barosl]

@brson

This looks good to me though the logic around self.merge_sha was a bit hard for me to follow, and my python is weak.

I also think the code around self.merge_sha is somewhat ugly and hacky. In fact, the logic was copied from the master branch and does not fit well with the one-by-one strategy of the rust branch. However I cannot think of a better solution to this...

if the rollup fails the failure will be posted to whichever PR triggered the test, but then bors will proceed to the other rollup PRs and do the test again

This is true, but bors won't proceed to one of the remaining rollup PRs immediately, because they all have lower priority than the normal PRs. (We triggered the rollup by setting p=99.) So while bors trying the normal PRs, we can have time to review the rollup and rearrange them.

remove the 'rollup' comment or 'r-' it, then '@bors: retry' the rollup. Does that sound right?

Yes, that would be a typical approach to a failed rollup.

it might be nice to support tagging rollups independently of r+, like one comment with '@bors: rollup' (though maybe your regexp supports just the word 'rollup'?).

Right, rollup is already independent from r+ by the definition of the regular expression, so it can be freely added or removed regardless of the existence of r+. I think this is reasonable.

[brson]

I've deployed this to production. @barosl will you write up a summary of how to do rollups to discuss.rust-lang.org?

[Gankra]

🎊

[barosl]

@brson Done. Batched merge (“rollup”) feature has landed on bors