4 ms·
Using email to review patches is completely absurd. The following is a contentious statement but there's no avoiding it: it's done nowadays by people who starte
by da39a3ee 6y ago
Using email to review patches is completely absurd. The following is a contentious statement but there's no avoiding it: it's done nowadays by people who started doing it in the 80s and 90s and are being stubborn.
Here are some reasons email should not be used for review, if any are really needed:
- A "patch" must have a defined parent. Otherwise it is not defined what state of the overall codebase is being reviewed.
- Proposed changes evolve through complex lifecycles of comments/discussions and updates. Some comments are global and some should be attached to specific lines of code as it exists in a specific revision. This complexity is (obviously!) better modeled by a dedicated UI than by shoe-horning it into plain text email.
- It makes sense to attach the dynamic status of test runs to these updates. This can be done in a web UI but cannot be done in an email.
- Commenting on individual lines of code is desirable and (obviously!) better done by attaching a discussion thread to a defined (revision + code coordinates) location in a diff view than by replying inline in an email. The reasons (if they really need stating) include (1) that breaking a diff manually in an email and replying inline makes it hard to assemble the diff in your head, (2) it is not possible to have clearly readable discussion threads involving multiple people with each thread attached to a certain line in the diff, (3) it's certainly not possible to keep that discussion coherent when the underlying patch is evolving.
- Not everyone uses, or wishes to use, plain text emails.
- Not everyone uses or wishes to use a text-based email client that runs on their local machine. Nor do they have, or wish to have, their emails present on their local machine (they want their emails stored online and accessed via a web interface).
(And yes, I am someone who has used mutt, and gnus, and dovecot, and offlineimap.)
- The history/evolution of a codebase can be represented by a graph structure. It makes sense for review discussion to be associated with this graph structure. This is what Bitbucket/Github/Gitlab provide. Email does not provide this. There's a risk with email that it won't even be publicly searchable, but supposing it exists, a web-hosted archive of emails is not an effective way to comprehend the history of a codebase.
As a cultural observation, the people who are still insisting on email are often the sorts of people who insist that email should be plain text and actually look down upon technical people who like web-hosted email clients and HTML emails, and they are sometimes the sorts of people who deride Github as being nonserious -- full of "kids" who carelessly propose ill-thought out change sets and expect them to be incorporated into public projects. Neither of those positions are reasonable or speak highly of the holder.
- lima 6y agoNote that those points are particular disadvantages of the email workflow - all of these things are solved in modern tools like Gerrit.
- da39a3ee 6y agoIf you don't know what the parent is in Gerrit then how can you run CI on the patch? And if you do know what the parent is then how is it different from a "PR"?
- lima 6y agoA PR is roughly the same as a stack of Gerrit CLs. The difference is that Gerrit tracks the evolution of, and reviews, each individual commit. With a PR, you lose history when rebasing and all commits have to be approved as a whole.
- da39a3ee 6y ago> The difference is that Gerrit tracks the evolution of, and reviews, each individual commit. What does it mean for an individual commit to evolve? I think of commits as being the atoms of the evolutionary process; i.e. evolution is a sequence of commits. > With a PR, you lose history when rebasing I don't quite understand that point. (I know what rebasing is.) > and all commits have to be approved as a whole. Hm, so why would one want to approve an individual "commit"? In my world a commit should be a meaningful change but isn't necessarily self-contained and hence not approvable. This sounds like what you're calling commits are in fact independent / self-contained and deployable units of change. So then > A PR is roughly the same as a stack of Gerrit CLs. Does a "CL" contain one or more than one of the entities you refer to as "commits"above? Isn't it the other way around? A Gerrit thing is a stack of mini "PR"s, where each "PR" is a small, but independent / self-contained and deployable, unit of work. In any case, a PR contains many non-self-contained units (commits) so I'm having a hard time equating a PR and a stack of CLs.