[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:8-9
@@ +7,4 @@
+
+This is a proposal to move our current revision control system from Subversion
+to GitHub. Below are the financial and technical arguments as to why we need
+such a move and how will people (and validation infrastructure) continue to work
----------------
emaste wrote:
> It seems pedantic, but I think we should try hard to avoid conflating Git and GitHub. What about: "move our revision control system from //self-hosted// Subversion to Git hosted by GitHub."
This is a summary of the whole proposal, which specifically dictates GitHub.

We're not proposing to move to some Git hosting anymore, but exactly GitHub, due to the constraints that we outline below. If we do not move to GitHub specifically, a lot of the assumptions below will be wrong, and this proposal can't be accepted.

There is a paragraph on why git, and another on why GitHub, and both are key points of this proposal.

I'll change "from Subversion" to "from our own hosted Subversion" to make that even more clear.

================
Comment at: docs/Proposals/GitHub.rst:93
@@ +92,3 @@
+Furthermore, GitHub has an *SVN view* (https://github.com/blog/626-announcing-svn-support)
+where people stuck with SVN infrastructure and tooling can slowly migrate or
+even stay working as if it was an SVN repository (including read-write access).
----------------
emaste wrote:
> Replace "stuck" with something neutral. "stuck" implies that everyone will want to move but some may not be able to for technical or other reasons, but some people actually prefer SVN.
good point.

================
Comment at: docs/Proposals/GitHub.rst:122
@@ +121,3 @@
+of understanding the *sequence* in which commits were added by using the
+``git rev-list --count hash`` or ``git describe hash`` commands.
+
----------------
filcab wrote:
> How easy will it be to clone the "aggregated" repo, and then get some (but not all) of the submodules?
Checking out this project:

https://github.com/chapuni/llvm-project-submodule

Will return the references, not the sub modules. You have to "init" each sub-module independently, which achieves the same task as only checking out the SVN repos you need, with the correct numbering.

================
Comment at: docs/Proposals/GitHub.rst:130
@@ +129,3 @@
+* Individual projects' history will be broken (linear, but local), and we need
+  the umbrella project (using submodules) to have the same view as we had in SVN.
+
----------------
filcab wrote:
> I wouldn't call it broken.
> Won't it have the same end result as having a checkout per project and simply updating them close to each other?
> 
> Basically, it won't be "any more broken" than using this method for updating:
> 
> ```
> #!/bin/bash
> for dir in llvm/{,tools/{clang,lld},projects/{libcxx,libcxxabi,compiler-rt}}; do
>   # (cd $dir && svn up) # for SVN
>   (cd $dir && git checkout master && git pull) # for git
> done
> ```
No, it won't.

Checking out LLVM only skips all commits from all other repos. So, for example:

    LNT 123
    Clang 124
    RT 125
    LLVM 126

Then, "svn checkout 126" will be:

    In LNT, 123 as 126
    In Clang, 124 as 126
    In RT, 125 as 126
    In LLVM, 126 as 126

With the new SVN interface, each one of those commits will be referred to, locally, as 123, and 126 will not exist, because the "git rev-list --count" won't get as high as 126.

However, on the umbrella submodule project, because the sequence of the commits is guaranteed, the rev-list count will bring the correct numbering, the same as via the SVN interface, and thus be "just like SVN was".

================
Comment at: docs/Proposals/GitHub.rst:132
@@ +131,3 @@
+
+There is no need to additional tags, flags and properties, nor of external
+services controlling the history, since both SVN and *git rev-list* can already
----------------
winksaville wrote:
> This could be worded a little better, may I suggest something like:
> 
> "There is no need for additional tags, flags, properties, or external ..."
> 
Yup, changing on next round. Thanks!

================
Comment at: docs/Proposals/GitHub.rst:141
@@ +140,3 @@
+has commit access to our current repository. In the future, you only need to
+provide the GitHub user to be granted access.
+
----------------
probinson wrote:
> This reads a little bit like "we will create a GitHub account for you if you don't have one" but I suspect people actually need to create their own GitHub accounts first.  (We're not all on GitHub already!)
well, "you need to provide the GitHub user" should take care of that, but I'll try to re-write this to make it more clear.

> (We're not all on GitHub already!)

Are we not? Egregious! :D

================
Comment at: docs/Proposals/GitHub.rst:180
@@ +179,3 @@
+
+As soon as we decide to move, we'll have to set a date for the process to begin.
+
----------------
emaste wrote:
> This presents it as if the decision is already made, which somewhat defeats the purpose of writing a proposal for the LLVM community to vote on.
> 
> Maybe "If we decide to move"?
I tried very hard to not mean that, but this one may be interpreted in the wrong way... I'll change that.


https://reviews.llvm.org/D22463