[llvm-dev] Enable Contributions Through Pull-request For LLVM

Thu Nov 7 17:54:20 PST 2019

+1 to the pull request model in general. The pull request model is the more conventional way of using git in my experience and I'm generally in favour of a conventional workflow. I can't really assess the quality of GitHub's code review tools though as I haven't used theirs for several years. I'm willing to try them on a significant patch but I'd like to do that before I +1 for GitHub specifically or decide I prefer to keep Phabricator in the process. That said, I think it's ok to allow them and see if the community shifts to them in the long run. That's pretty much how our usage of Phabricator took off after all.

> On Nov 6, 2019, at 21:32, Mehdi AMINI via llvm-dev <llvm-dev at lists.llvm.org> wrote:
> 
> Hi all,
> 
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step as an experimental channel alongside the existing methods for contributing to LLVM.
> This would allow to find workflow issues and address them, and also LLVM contributors in general to start getting familiar with pull-requests without committing to switching to pull-requests immediately. The community should evaluate after a few months what would the next steps be.
> 
> GitHub pull-requests is the natural way to contribute to project hosted on GitHub: this feature is so core to GitHub that there is no option to disable it! 
> 
> The current proposal is to allow to integrate contributions to the LLVM project directly from pull-requests. In particular the exact setup would be the following:
> 
>   - Contributors should use their own fork and push a branch in their fork.
>   - Reviews can be performed on GitHub. The canonical tools are still the mailing-list and Phabricator: a reviewer can request the review to move to Phabricator.
>   - The only option available will be to “squash and merge”. This mode of review matches the closest our current workflow (both phabricator and mailing-list): conceptually independent contributions belongs to separate review threads, and thus separate pull-requests. 
> This also allow the round of reviews to not force-push the original branch and accumulate commits: this keeps the contextual history of comments and allow to visualize the incremental diff across revision of the pull-request. 

-1 to "squash and merge" being the only option if rebase+push (--ff-only) is possible. I'd rather we use our judgement to decide what's appropriate for the pull request at hand rather than have a blanket rule.

Personally, if I have multiple commits (typically 2-5) in a pull request it's because there's incremental steps that I'd like to preserve. For example:
1. Add assembler support for X
2. Add codegen support for X

1. Move X to libY. NFC
2. Add support for Z
We could do each step in individual pull requests but it's unnecessary overhead.

on the other hand, something like:
1. Add support for X
2. clang-format
3. typo
4. another typo
5. fix the build
6. wip
7. Fix test failure in target Y
should be tidied up before merging (squashed in this case but `rebase -i` is sometimes appropriate).

>   - Upon “merge” of a pull-request: history is linear and a single commit lands in master after review is completed.

Personally, I'd like us to drop the linear history requirement but I know there's some strong opposition there. The main reason I'd like us to accept non-linear history in conjunction with pull requests is that it means that the commits that were tested (whether by the author or by CI) still exist in the repository after the merge. If we rebase+push like we do today or squash+merge as in this proposal, then the information that a particular commit passed testing (or failed a particular test) gets lost.

> As an alternative staging proposal: we could enable pull-requests only for a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with for example) in the repo. In this case, we would propose to begin with the MLIR project (as soon as it gets integrated in the monorepo). This would be a good candidate to be the guinea pig for this process since it does not yet have a wide established community of contributors, and the current contributors are already exclusively using pull-requests <https://github.com/tensorflow/mlir/pulls>.
> 
> Here is a more complete doc on the topic: https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI <https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI>
> 
> Cheers,
> 
> -- 
> Mehdi
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191107/e80ace9f/attachment.html>