A central piece of modern collaborative software development is done using source control
tools. One of the most prominent being git. While this is by no means the only one, it
is probably the most commonly used one with a cottage industry of tooling around it.
While git is conceptually decentralised, it is mostly used though a central server. Today
this central server is often GitHub, or to some degree GitLab, and even a lesser some other
hosted options. This provides value in that it's easily accessable from a browser (the
ubiquitous way to access information today), and provides discovery facilities.
However, one can argue that the most critical part of collaborative software development is
code review. Some party wants to include changes to the current code base. This is fundamentally
the difference between the current codebase, and the changed code base. We usually call this
a diff(erence) or patch. The rise of git has lead to these differences being seen as so
called Pull Requests (on github) or Merge Requests (on gitlab), which consist of a series of
commits, that--sometimes--can be seen as standing on their own, or explain the progression.
This may or may not add value to the person reviewing the code. If the commits on their own
make sense one could potentially review each on their own. So why is this not just a of patches,
but one patch split into smaller steps? The primary reason I've heard is that they don't make
sense in isolation, and need to be seen as a whole; however the intermediate commits are supposed
to assist in understanding. I do not believe this to be true most of the time. I consider this
an aid for the developer to make incremental progress and keep snapshots during the development
to revert of if things do not turn out the way anticipated.
This brings us to the topic of code review. There are verious ways to do this, and personal preference plays a large role. Some poeple prefer to check out the branch, and review using their editor of choice. Others review online using the tooling provided. I will focus on the latter part. In my opinion what is center to the code review is the actual change (or patch) ontop of what is actually upstream. So integral to a code review is a proper discussion of what this change is supposed to do, and why we are doing it. Thus what really matter for the code review--in my opinion--is the introductory header and explaination what this change does, and the actual change. I don't want to see anything else or be distracted by anything else, these two items should be front and center. Easy to read, easy to reach.
The next fundamental piece is the ability to critique the diff (or even introduction). Thus allowing to put comments onto specific ranges, and potentially even providing easy ways to place suggested code edits is very important.
After a few iterations of code review, you'll usually end up with lots of comments, it should be dead easy to mark items that ar completed and have them be reduced to something almost invisible. Anything else keeps distracting. Keep the display clean and focused.
Changes evolve over time. If I have reviewd a previous patch, allow me an easy way to review the new additional diff (if I so want to), instead of everything. If there is additional generic/global discussion on the code, this should probably go between the introduction and the code, but focus on only the latest changes, filter out anything that is not relevant anymore, and don't clutter it with metadata that's mostly useless, unless I explicilty need to look at it.
Phabricator did get a lot of this right. Not only did it decouple the development of a feature/bugfix/... from the actual patch that was merged in the end; the introduction and explaination was what ended up in the commit log, not some set of intermediate commits. The focus was on writing a coherent motivated message for the change that was supposed to be merged, not on small steps with less discussion. GitLab as well as GitHub, completely ignore the Merge Request / Pull Request description and focus on the actual git commits, this makes the whole MR/PR description lose value.
GitLab and GitHub both focus heavily on the discussion, and less on the actual code. But it's the code that ends up being merged, not the discussion. They also for some reason separate this into two different panes.
One issue that was very contentious with Phabricator was that it focused on Patches, rather than a sequence of commits. I fundamentally beleive this was the right approach, not only did it abstract away different VCS, it also put the focus correctly--in my opinon--on the actual change, not on how we got there. This different mentality was confusing for many, especially when they assumed a git workflow they got accustomed to on GitHub or GitLab. It is however precisely the focus on this workflow of small commits and a focus on how the diff was conceived rahter than a focus on the actual change and a coherent motivation for said change that leads to this difference. And code review suffers for it. The focus should be on the what and why, not on the how.
An ideal solution would allow me to use git trivially a nd not require me to work with arc (Phabricators
Arcanist tool), yet focus heavily on a patch based view, and less on a commit based view. Squash and merge
workflows get us somewhere there, but all the review tooling seems primarily focused on commits, not
patches, apart for the aggressive cluttering of meta information in the process and not putting the actual
change front and center.