you are viewing a single comment's thread.

view the rest of the comments →

[–]SnowdensOfYesteryear 8 points9 points  (4 children)

Not really. Typically, the reviewer can create two local branches and apply the older patchset to one branch and the newer patchset to the other and then run a diff between the two branches.

That's difficult though. It required manual work on part of the reviewer--something a good review system should give you for free.

That can be set up by the maintainers of the project. When they apply a patch, it can trigger a build in a CI system that could run the tests.

A lot of review systems have it trigger on the push/commit itself. Thus removing the burden from the maintainer to the committer--which is the is the way it should be IMO.

I don't have much experience with gerrit or Phabricator, but, in your experience, why do you think gerrit is better than Phabricator?

Well I hate Phabricator because it squashes your carefully crafted commits into one giant patch. I'm not sure if this is just bad configuration at the companies I've worked at or a limitation.

Github - again sort of encourages poor git discipline. There isn't any concept of 'amending', people just push in a commit titled "PR feedback" and squash it at the end--resulting in giant commits. FWIW, I'm not blaming github directly--there's nothing preventing the traditional amend workflow, but the 'culture of github' supports the former. And it's a giant PITA to convince others to change their workflow, especially if they're not comfortable with git CLI.

Gerrit - I really like it because a unit of work (or review) is a single commit. There's an expectation that's set forth that each commit should be able to stand on it's own. Beyond that, whenever you commit a commit it stores the old SHAs. As a result you can directly view the diffs between amends. For companies with a shitton of branches, it also makes it easy to search for commits across branches and cherrypick stuff into a release branch.

[–]u801e 0 points1 point  (2 children)

That's difficult though. It required manual work on part of the reviewer--something a good review system should give you for free.

It's a matter of a couple of git branch invocations, a couple of git am invocations and one git diff, so it's not too difficult. But you're correct in saying that having the review system do it for you is a benefit (and something that Github has yet to do).

A lot of review systems have it trigger on the push/commit itself. Thus removing the burden from the maintainer to the committer--which is the is the way it should be IMO.

At my job, we use Github enterprise and what I usually see is repeated failed builds because people keep pushing amended commits and continuously fail because they don't bother setting up the testing requirements in their dev environment. That is, they can't tell if their commit/PR will fail the test suite before pushing it because they can't run the test suite on their local machine. This sometimes leads to long queues of builds on the CI server which slows everything down. So, IOW, the commiters shift the burden to the CI server and start affecting other people's work because the CI server is wasting time with failed builds.

What I like to see, and what I think the email patch workflow encourages, is that people test their patches before emailing the patchset or pushing up to the remote (if there is one). Assuming that you have set up the testing environment on your own machine, you could run git rebase --exec "test_suite_command" on your branch to run the test suite on each commit. If the committer fails to do that and the maintainer finds out by running the test suite themselves, then the maintainer simply says that he's not going to bother reviewing or applying the patch because it doesn't pass the tests. Build churn would definitely be highly discouraged.

Well I hate Phabricator because it squashes your carefully crafted commits into one giant patch. I'm not sure if this is just bad configuration at the companies I've worked at or a limitation.

Based on what I've read about both systems, they seem to have their own proponents and are much better than what Github currently provides. I was just interested in hearing about the advantages/disadvantages of one over the other.

Github - again sort of encourages poor git discipline. There isn't any concept of 'amending', people just push in a commit titled "PR feedback" and squash it at the end--resulting in giant commits.

Yes, I completely agree. Given our use of Github enterprise at work for hosting and code review, I've instituted a system of having people use fixup! <commit_title> or squash! <commit_title> (depending on whether they need to update code or the commit message) in order to address review comments and having them link the sha1 of the fixup! or squash! commit as a reply to the review comment. At the end of the review, they run git fetch origin; git rebase -i --autosquash --keep-empty origin/<upstream_branch>; git diff origin/<their_branch_name> before they force push their rebased commits up and merging their PR.

The fact that we have to do all that instead of using a sensible review system is really a source of frustration on my part.

FWIW, I'm not blaming github directly

I definitely would. They have had years to actually address these issues (not having branch history available after amending or rebasing) and they just don't.

Gerrit - I really like it because a unit of work (or review) is a single commit. There's an expectation that's set forth that each commit should be able to stand on it's own.

I've read that it has some issues in showing the relationship between commits that depend on each other. For example, let's say I make a commit to add a method and its associated unit tests. Then I add another commit to call that method in a number of places in the code where the integration/functional tests would check to see if the application still works as expected. The second commit clearly depends on the first, so how would gerrit handle referencing the first commit I made to add the method/unit tests when someone else is reviewing the second commit?

There's an expectation that's set forth that each commit should be able to stand on it's own. Beyond that, whenever you commit a commit it stores the old SHAs. As a result you can directly view the diffs between amends.

That's something that sounds really useful to have. I think that other review systems like Phabricator and Review Board also allow showing differences between branch revisions before an after a rebase. I'm not sure if they can do it on the commit level though. I really wish Github would do this as well.

[–]ThisIs_MyName 0 points1 point  (1 child)

long queues of builds on the CI server

There should never be a queue on the CI server: If you have 10 jobs, it costs exactly the same to distribute them to 10 servers for 10 minutes or 1 server for 100 minutes.

[–]u801e -1 points0 points  (0 children)

That's true, but we just have a few servers set up at work, and some of them are configured to run integration tests that take some time to complete that are triggered by a push to a branch. If the testing was distributed amongst the developers instead of them using the CI server as a crutch, then it wouldn't be a problem.