> > In Github pull requests there is always a git commit that you can just >> feed to the build server. And you can be sure of what really gets merged. >> You review, build and test exactly the change that gets merged afterwards. >> > > How would that be true? Given that upstream keep changing during the > period of review? The commit is going to have to be rebased to head and > that may involve making changes. >Yes, github tells you, that your PR has a merge conflict, that you need to resolve manually. Once you've pushed the conflict resolution to the same PR, it triggers another round of builds and tests. And also potentially another review, depending on what permissions you have and how the project ist set up... -- Best, Christian -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200122/8d8517db/attachment.html>
Christian Kühnel via llvm-dev <llvm-dev at lists.llvm.org> writes:>>> In Github pull requests there is always a git commit that you can just >>> feed to the build server. And you can be sure of what really gets merged. >>> You review, build and test exactly the change that gets merged afterwards. >>> >> >> How would that be true? Given that upstream keep changing during the >> period of review? The commit is going to have to be rebased to head and >> that may involve making changes. >> > > Yes, github tells you, that your PR has a merge conflict, that you need to > resolve manually. Once you've pushed the conflict resolution to the same > PR, it triggers another round of builds and tests. And also potentially > another review, depending on what permissions you have and how the project > ist set up...It's not quite that simple though. In general most PRs will need to be rebased just before merging to maintain linear history. What happens then? Would things need to pass another build before the "final merge?" If so, then something needs to keep a list of pending PRs waiting for "final merge." This quickly gets very hairy as pending PRs have to constantly be rebased and rebuilt as other PRs land. Phab isn't any better in this regard. I'm just pointing out that requiring things to build/test before "final merge" is a hard problem regardless of the code review platform. -David
On Wed, Jan 22, 2020 at 11:24 PM David Greene <greened at obbligato.org> wrote:> Christian Kühnel via llvm-dev <llvm-dev at lists.llvm.org> writes: > > >>> In Github pull requests there is always a git commit that you can just > >>> feed to the build server. And you can be sure of what really gets > merged. > >>> You review, build and test exactly the change that gets merged > afterwards. > >>> > >> > >> How would that be true? Given that upstream keep changing during the > >> period of review? The commit is going to have to be rebased to head and > >> that may involve making changes. > >> > > > > Yes, github tells you, that your PR has a merge conflict, that you need > to > > resolve manually. Once you've pushed the conflict resolution to the same > > PR, it triggers another round of builds and tests. And also potentially > > another review, depending on what permissions you have and how the > project > > ist set up... > > It's not quite that simple though. In general most PRs will need to be > rebased just before merging to maintain linear history.That depends on how frequent merge conflicts are. In my other projects that wasn't really an issue. I rarely had to rebase something manually.> What happens > then? Would things need to pass another build before the "final merge?" >If so, then something needs to keep a list of pending PRs waiting for> "final merge." This quickly gets very hairy as pending PRs have to > constantly be rebased and rebuilt as other PRs land. >Some projects have a flag they set and let some bot do the manual work: Rebase, re-test and then automatically merge. I do not expect this to be perfect, it should rather be much better than today in slipping new bugs into master. -- Best, Christian -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200123/51ab2ff5/attachment-0001.html>