[PATCH] D24167: Moving to GitHub - Unified Proposal

Fri Sep 9 11:39:19 PDT 2016

beanz added a comment.

Lots of inline comments.

================
Comment at: docs/Proposals/GitHubMove.rst:63
@@ +62,3 @@
+
+It seems that Git is new coders first choice nowadays. A lot of them have never
+used SVN, CVS, or anything else. Websites like GitHub have changed the landscape
----------------
The language here is also misleading. Maybe change to something like:

> Many new coders nowadays start with Git, and a lot of people have never used SVN, CVS, or anything else. 

================
Comment at: docs/Proposals/GitHubMove.rst:68
@@ +67,3 @@
+
+Git is also the version control many (most?) LLVM developers use. Despite the
+sources being stored in a SVN server, these developers are already using Git
----------------
I would remove the "(most?)" bit here because it doesn't really add any value. We have no data to support an assertion of "most", and it could be misleading to suggest it.

================
Comment at: docs/Proposals/GitHubMove.rst:79
@@ +78,3 @@
+* Inspect the repository history (blame, log, bisect) without Internet access.
+
+In addition, because Git seems to be replacing many OSS projects' version
----------------
Can we also add this as a point:

> * Maintain remote forks and branches on Git hosting services and easily integrate back to the main repository.

In particular for people that maintain out-of-tree code or forks, the ability to seamlessly merge between repositories is a big win for Git.

================
Comment at: docs/Proposals/GitHubMove.rst:137
@@ +136,3 @@
+Git.
+
+What About Branches and Merges?
----------------
Can we also add something about the more traditional Git approaches to this? Maybe something like:

> Additionally, there are simple Git commands that can also be used to determine the order of commits. For example to answer the question is a bug fixed in <hash-a> fixed in a compiler built at <hash-b> can be answered with the command `git rev-list <hash-a>..<hash-b> --count`. If this prints a number greater than 0, the fix is contained in <hash-b>. Additionally if we were to use Git tags similarly to how we use SVN tags today you would be able to identify which releases contained a fix by running `git describe --contains <hash>`.

================
Comment at: docs/Proposals/GitHubMove.rst:192
@@ +191,3 @@
+more independent from each other, while the second proposal
+encourage better code sharing and refactoring across projects, for example
+reusing a datastructure initially in LLDB by moving it into libSupport. It
----------------
This is a completely subjective statement, and should not be present.

================
Comment at: docs/Proposals/GitHubMove.rst:202
@@ +201,3 @@
+and clang-tools-extra is not useful. With the monorepo, we can move code around
+as we wish and preserve history.
+With the multirepo, moving clang-tools-extra into clang would be
----------------
With git history could be preserved even across repositories. Git subtree merges support this, and while it isn't as simple, it is a one-time cost.

================
Comment at: docs/Proposals/GitHubMove.rst:205
@@ +204,3 @@
+more complicated than a simple `git mv` command, and we would end up losing
+history.
+
----------------
As in my other comment, losing history is not an issue.

================
Comment at: docs/Proposals/GitHubMove.rst:208
@@ +207,3 @@
+Some concerns have been raised that having a single repository would be a burden
+for downstream users that have interest in only a single repository, however
+this is addressed by keeping the single-subproject Git mirrors for each project just as we
----------------
Actually, there were also concerns about the increased burden for *contributors* not just downstream users.

In general I think this entire section is designed to point out supporting arguments for the mono-repo with no recognition of the merits of the multi-repo proposal.

================
Comment at: docs/Proposals/GitHubMove.rst:230
@@ +229,3 @@
+
+Under the multirepo, things are more involved.  We describe here the proposed
+solution.
----------------
This is very slanted wording. From a user perspective the multi-repo solution to this problem is not much more complicate than the mono-repo solution.

================
Comment at: docs/Proposals/GitHubMove.rst:421
@@ +420,3 @@
+
+With the multirepo proposal, the umbrella repository enters the dance. This is
+where the mapping from a single revision number to the individual repositories
----------------
Nit: "enters the dance" implies complexity.

================
Comment at: docs/Proposals/GitHubMove.rst:452
@@ +451,3 @@
+
+Today this is easy for subversion users, and possible but not straighfoward for
+git-svn users. Few Git users try to e.g. update LLD or Clang in the same commit
----------------
Not sure I agree this is easy for svn users. To my knowledge llvm.org doesn't even document how to checkout the SVN repositories in a way to make this possible.

================
Comment at: docs/Proposals/GitHubMove.rst:518
@@ +517,3 @@
+  git checkout AnotherBranch
+
+Bisecting
----------------
Additionally users of the umbrella repo can use `git submodule foreach` to have single command workflows that nearly match the mono-repo proposal.

================
Comment at: docs/Proposals/GitHubMove.rst:570
@@ +569,3 @@
+multirepo situation explained above. The granularity is finer since each
+individual commits in every sub-projects participate in the bisection. The
+bisection script does not need to include the `git submodule update` step.
----------------
This is inaccurate. Even though my rough prototype of the git umbrella repo doesn't have each submodule update being a single commit that was the stated plan for how the umbrella would be updated. That means each umbrella repo commit would represent a single commit to a single subproject, so your bisection granularity is comparable.

================
Comment at: docs/Proposals/GitHubMove.rst:581
@@ +580,3 @@
+* If you were pulling from the SVN repo before the switch to Git. The monorepo
+  will allow you to continue to use SVN. The main caveat is that you'll need to
+  be prepared for a one-time change to the revision numbers. The multirepo 
----------------
Better to say "both proposals will allow you to continue to use SVN". The wording here makes it seem like only the mono-repo has GitHub's SVN support, even though that is later contradicted.

================
Comment at: docs/Proposals/GitHubMove.rst:620
@@ +619,3 @@
+
+Note however that many users of the monorepo would benefit from having all of
+the pieces needed for a full toolchain present in one repository. And for
----------------
This is a subjective statement that I don't believe is factually accurate. We could easily teach the build system to checkout subprojects so that building a full toolchain could be `git clone ... && configure && build` regardless of the repository layout.

================
Comment at: docs/Proposals/GitHubMove.rst:627
@@ +626,3 @@
+they aren't forced to download or check out all of llvm, clang, etc. just to
+make a change to libcxx.)
+
----------------
I'm confused by this. The sub-project mirrors are read-only, so the workflow is either checkout the full mono-repo or use Git-SVN. That doesn't sound unchanged.

================
Comment at: docs/Proposals/GitHubMove.rst:635
@@ +634,3 @@
+
+Example of a working version:
+
----------------
It is worth noting (as I did when I sent this out) that this was a very rough prototype, and it doesn't solve all the problems that we would expect a more permanent solution to solve. For example, the submodule update is periodic, not on a push-based notification, and the scripting around it doesn't do a single commit per update, which was the intended solution.

https://reviews.llvm.org/D24167