[Lldb-commits] [PATCH] D22463: [RFC] Moving to GitHub Proposal: NOT DECISION!

[Renato Golin via lldb-commits]

rengolin added inline comments.

================
Comment at: docs/Proposals/GitHub.rst:68
@@ +67,3 @@
+ * Collaborate with peers directly, even without access to the Internet
+ * Have multiple trees without multiplying disk space, multiple concurrent builds
+
----------------
vsk wrote:
> What do you mean by multiple concurrent builds?
With git worktree you can work on source code and build different things at the same time, but I guess this is a specific use case which is only made "easier" in git. I'll remove this extra comment.

================
Comment at: docs/Proposals/GitHub.rst:78
@@ +77,3 @@
+
+GitHub, like GitLab and BitBucket, provide FREE code hosting for open source
+projects. Essentially, they will completely replace *all* the infrastructure that
----------------
dberris wrote:
> nit: I see you use FREE in caps but this instance isn't *FREE* (as opposed to the first mention above) -- consider making it consistent? Either remove the emphasis (just "free") or emphasise consistently?
good point.

================
Comment at: docs/Proposals/GitHub.rst:86
@@ +85,3 @@
+for example development meetings, sponsoring disadvantaged people to work on
+compilers and foster diversity and quality in our community.
+
----------------
dberris wrote:
> Did you mean "diversity and equality" instead of "diversity and quality" here?
indeed, fixed.

================
Comment at: docs/Proposals/GitHub.rst:102
@@ +101,3 @@
+
+How will the new workflow look like
+===================================
----------------
delcypher wrote:
> s/How will/What will/
ok

================
Comment at: docs/Proposals/GitHub.rst:110-112
@@ +109,5 @@
+
+As with the current SVN and Git repositories, each project will be separate,
+on its own, and people will also be able to check them out in the same way
+they're already doing today.
+
----------------
dberris wrote:
> Consider rewording this sentence -- it's a little too long and is trying to say too many things.
> 
> Perhaps something like:
> 
> "Each LLVM project will continue to be hosted as separate GitHub repositories under a single GitHub organisation. Users can continue to choose to use either SVN or Git to access the repositories to suit their current workflow."
Much better, thanks!

================
Comment at: docs/Proposals/GitHub.rst:136
@@ +135,3 @@
+
+We will need additional server hooks to avoid non-fast-forwards commits (ex.
+merges, forced pushes, etc) in order to keep the linearity of the history.
----------------
delcypher wrote:
> @rengolin : I know GitHub enterprise has a "protected branch" feature to prevent forced pushed ( https://github.com/blog/2051-protected-branches-and-required-status-checks ). You might want to speak to them to see if they can offer us that feature. I think there's a limited support to do a merge as a squash and rebase too ( https://github.com/blog/2141-squash-your-commits )
I'm asking about protected branches (it was proposed earlier, but I can't find any info on it).

But we don't want to squash people's commits. They can do that on their own before commit.

================
Comment at: docs/Proposals/GitHub.rst:185
@@ +184,3 @@
+
+Are there any other upstream systems that could be affected?
+
----------------
dberris wrote:
> vsk wrote:
> > Yes, the `llvmlab bisect` functionality is affected. IMO it is invaluable for bug triage. Could you add some kind of reassurance that initially, updating it for the new VC model will just require pointing it to github's SVN view?
> Probably worth mentioning how Phabricator will need to be updated to integrate with the GitHub repository once the canonical repo is changed.
Adding llvmlab bisect and Phab to the list. Both should be trivial.

================
Comment at: docs/Proposals/GitHub.rst:209
@@ +208,3 @@
+   well as a webhook to update the umbrella project (see below).
+3. Make sure we have an llvm-project (with submodules) setup in the official
+   account, with all necessary hooks (history, update, merges).
----------------
jyknight wrote:
> mehdi_amini wrote:
> > rengolin wrote:
> > > mehdi_amini wrote:
> > > > > Pre-commit hooks
> > > > 
> > > > Won't handle the update of the umbrella.
> > > > 
> > > > > Post-commit hooks
> > > > 
> > > > Can't handle the update of the umbrella *because of GitHub*, this could be possible with our own hosting of git for instance.
> > > > 
> > > Pre-commit hooks are not designed to update the umbrella. Webhooks will be able to update the umbrella with a small external service, as proposed in the IRC.
> > That's why I asked it to be clearly mentioned somewhere else that on top of "hooks" hosted by github, we will need to maintain our own webservice on our own server to answer updates from theses hooks and update the umbrella, because that's a non-negligible drawback in face of the motivation exposed in the "Why move at all?" section. 
> > Right now the document does not acknowledge that AFAICT.
> The maintenance of that service will be negligible compared to running a subversion installation.
> 
> I expect that we could set up the webhook as an AppEngine app, using Github's "Git Data" API to generate the new commit, and then to the first approximation never have to touch it again.
I'm adding a list of needed hooks to the "What changes" section.

================
Comment at: docs/Proposals/GitHub.rst:233
@@ +232,3 @@
+
+10. Collect peoples GitHub account information, adding them to the project.
+11. Switch SVN repository to read-only and allow pushes to the GitHub repository.
----------------
vsk wrote:
> delcypher wrote:
> > GitHub organizations support the notion of teams which can each have different permissions (for example you'll want to make sure only the right people have admin access and give the rest write/read access). You could also make a team per project so that write access in one project does not give write access to another. I think it would be good to decide on how teams will be organized and state this in the document.
> I think this is an important discussion to have once the move is under-way. I don't think finer-grained write privileges should be a part of this proposal since it's (1) a separate issue and (2) not *just* an artifact of llvm-project's svn structure (i.e there are good reasons to keep the current permissions model in place). 
Indeed. We want a flat permission model, in the same way we have today, with the exception of the umbrella project, which no one will have write access to.

If we decide to change things in the future, it'll be a completely different discussion.


https://reviews.llvm.org/D22463