4 ms·

What are your thoughts on the "ship, show, ask" workflow? [1] In that workflow, small stuff is simply pushed, which allows PRs to be more single focused and mo

What are your thoughts on the "ship, show, ask" workflow? [1] In that workflow, small stuff is simply pushed, which allows PRs to be more single focused and more atomic. Perhaps your only objection is direct pushes to master? I am really curious if that workflow otherwise addresses all of the downsides you stated while still allowing for all PRs to be uniformly rebase-squash merged. [1] https://martinfowler.com/articles/ship-show-ask.html https://martinfowler.com/articles/ship-show-ask.html

imiric 2y ago
I wasn't aware of this particular workflow, but I've heard of similar ones before. The late Pieter Hintjens, of ZeroMQ fame, advocated for a strategy called "optimistic merging"[1], which essentially abandons the standard code review process in favor of merging changes ASAP, and fixing any issues as they arise later. I'm not a fan of this. It allows contributors to abandon any established coding standards, while placing more burden on maintainers to fix issues later. This in practice never happens, so the quality of the codebase degrades over time. Not to mention that it allows malicious actors to merge changes much more easily. As for "ship, show, ask" specifically, I have similar reservations. I think all changes should go through a review process, even if someone just glances at the changes. It not only gives the opportunity to leave feedback, but also serves as communication so that everyone is aware of the proposed changes. Also, making the decision of whether to choose "ship", "show" or "ask" might only work for senior and well disciplined developers. I think that in most teams you have a mixture of experience, so you'd inevitably end up in situations where a change should've been "show" or "ask", but was "ship", and viceversa. I don't think teams will ever align on a single strategy to make a correct decision, since it's based on subjective opinion to begin with. Always following the same workflow gets rid of these uncertainties. [1]: http://hintjens.com/blog:106 http://hintjens.com/blog:106
- seadan83 2y ago
  After having done a few thousand CRs, I've come to believe that CR is of mixed value. Not always good, nor all good. Removing the bad CRs and doing CR when it is useful IMO helps maximizes productivity. First, I assume a few constraints on "ship, show, ask." First constraint is that all changes go for at least one, if not multiple self reviews. Second, the ability to ship is not outright granted. Until someone has demonstrated they can ship reasonable, working code- they are not given write permissions. They must "ask." Next, ship is not a given. Things that are not interesting are shipped. This enhances the communication part of PRs. If a PR is sent, it is important or wants extra review. That removes the noise of everything being a PR, and removes the noise from PRs where minor changes are smuggled in for the sake of efficiency. This also removes a lot of nitpicking as well. When asked to CR, we often feel we need to CR, and if our only feedback are minor things- then that is the feedback. 'Perfect' becomes the enemy of good enough. Saving time, efficiency are huge. Do CR when it is useful, and only then. Last, post merge reviews can still be done by the maintainers to ensure things are going smoothly. Hence, post merge review is an option for both ship and show. This means the coding lifecycle is blocked in CR only when the CR is valuable. That CR is blocking I feel is a very important drawback. Typos linger because time is finite and only those that do not appreciate that will try to fix everything. The team then gets bogged down spending too much time fixing small things. Leaving the small things is not ideal either. If the overhead to fix small things is tiny, suddenly it takes 5 minutes to do what would otherwise require another person and possibly the next day. For example, A PR preceded by renaming a few files is powerful. That is a game changer compared to a hard to review CR, staged PRs, or simply never doing the renaming because it runs the diff. Blanket CR can also ossify a project. Most projects I've seen are written quickly and in large chunks. If they are reviewed at all, it is a lot of 2000 line "LGTM", zero comment reviews. Then when careful CR is done (often by the "scaling" team), suddenly things that are original become gospel. The CR debates the absolute best naming, original intent, spending magnitudes more time than was originally put into the code in the first place. Meanwhile the code was done quickly, slightly worse than good enough- and now good enough is not even good enough. The CR now demands a far higher quality bar compared to the original code when it was written. Fixing a 50k line code base at 500 lines per day is a way to _not_ fix that codebase. The original bad code is therefor fossilized into the project. Time and effort are not infinite. That turned into a bit of a rant. I wanted to demonstrate that: (A) there are guard rails on SSA so it is not just a free for all. (B) selectively requesting CR when it is valuable amplifies the value of CR. (C) removing the noise of everything is a PR makes PRs stronger as communication tools. (D) post merge review of all code can still meet the goal of reviewing everything. I'll tie this back to git absorb now. Git absorb rebases, which arguably invalidates any CR that was done. Ship-Show-Ask allows for there to be more explicit trust where someone does their git absorb, redoes the self review and then merges. Further, the workflow helps clean up PRs to not be as many commits. Though, I do think the discussion of workflows and CR is really fascinating. Having been essentially just the one senior, or one of two on most all projects I've worked on in the last decade, - I'm certainly (overly) eager to discuss in detail the workflows.
  - imiric 2y ago
    Thanks for your perspective. It's interesting to hear a different take on this. I do think there's no right or wrong answer with these things. Everyone will have different opinions about their ideal workflows, especially senior engineers. The important thing is to find a good balance that everyone on the team is satisfied with, but there will always be concessions, as in any relationship. What I have found, though, is that the more uncertainties a workflow has, the higher the chances of misunderstanding, or someone not following it in the way someone else thinks they should. So with the way "ship, show, ask" is described, and what you mention here, is that there are many constraints, and decisions to be made about what constitutes an "interesting" change, or what is worthy to be shipped without a CR and what isn't. If the team is mostly senior, and there's a lot of trust involved, this workflow might work, and it might reduce the friction of blocking CRs. But there are a lot of variables involved there. OTOH, by always requiring CRs and approvals for shipping, there are far less decisions to be made. The workflow is simple, fixed and known to everyone. Sure, sometimes CRs step into nitpicky territory, but this is usually solved by having some conventions, such as making clear what is a nitpick, what is a minor/major suggestion, and what is a blocker. This way nitpicks and minor suggestions can be safely ignored (as long as they're ackgnowledged in some way), and they're not blockers for merging. In practice I've found that while this workflow does take considerable time and effort, in the vast majority of cases it leads to higher quality code. Even addressing nitpicks and typo fixes is an improvement in most cases. These can be easily automated by suggestions in GitHub, which can be batched and committed directly from the web UI, so it doesn't take much effort at all. > Last, post merge reviews can still be done by the maintainers to ensure things are going smoothly. Sure, but is this diligently done? And how well does it scale if the project has many contributors, but few maintainers? Placing the burden on maintainers to ensure certain quality standards is unrealistic. Moreover, it assumes that maintainers have the final say in these matters, instead of their changes also being worthy of discussion, which happens during a CR. I think that the responsibility for ensuring things are running smoothly should be shared by every member of the team, including external contributors. > Most projects I've seen are written quickly and in large chunks. If they are reviewed at all, it is a lot of 2000 line "LGTM", zero comment reviews. Those are examples of not doing CRs correctly, not a testament that CRs are not worth the time and effort. You can just as well run into this issue with the SSA workflow. As with any issue with team dynamics, this should be resolved by communicating and aligning on core values and practices the team finds important. > The CR debates the absolute best naming, original intent, spending magnitudes more time than was originally put into the code in the first place. Yes, this is to be expected. It takes much more effort to read and understand code than it does to write it. It takes additional time and effort to leave a written comprehensible review, and communicate all of this asynchronously. This scales linearly for every member in the team, and there's no way around it. But the question is whether deciding to invest the time and effort in a well established CR process ultimately results in higher quality software being shipped. In my experience, it does, and it's worth it. That said, CRs are not the only or, even failproof, method of quality assurance. As mentioned in the SSA article, pair programming is another method, but it has its own drawbacks and limitations. All of these things help, and are best used in combination. They can be uncomfortable, tedious and seem pointless at times, but deciding to not do them because of this is doing a disservice to the final product.
keybored 2y ago
Why is the preferred goal to “rebase-squash” (redundant) merge? The implied onus here seems to be for the other party to move towards that strategy. But why this practice should be the goal does not seem to be mentioned. Step one isn’t to find some way to accept squash-merge. Step one is to find some reason for squash-merge.
- seadan83 2y ago
  I think you are coming at this from quite the different angle. To semi quote what I was responding to: > always squashing PRs is not a good strategy [because of the following reason] Hence my question,if workflow X mostly solves the following reasons, then always squashing is potentially a reasonable strategy. I think you are asking, why do that at all? The answer is IMO quite involved, nuanced, and not at all one sized fits all. FWIW: (1) all commits to master become atomic if a passing CI is required before rebase squash-merge. (2) rebase creates a linear history. Non linear history is very difficult to bisect, and can really be complicated. I don't see much positives from that complexity. (3) generally PRs are functional sized pieces of work. (That is ambiguous.) If it helps, PRs are more boulders than pebbles. Having boulders in the history is more useful than every small pebble. Net benefit is the history then becomes a smattering of well marked minor changes and a set of functional changes. Side note, I like anything that helps the workflow of "touch clean code only, touch only the code you need to, clean the code before you touch it". (Clean defined there as "not a pile of illogical, obtusely written crap - riddled with latent bugs). In other words, make the code simple and obvious first, then modify the simple and obvious code. I give little credit to those that spend days studying code to make a one-line "masterful" change. That does not scale, everyone has to do that, and there are thousands of lines. All things in their own context though... (4) utility and communication for commits. IMO ideally a PR is initiated with a single commit that is written "for the history". This commit description is both descriptive for the reviewers, the git history, and is the complete text for the PR. This creates the most value IMO. I view PRs as ephemeral, any extra text written for the sake of PR is overhead. Then, any following commits are pushed to communicate to the reviewers. The audience is no longer a future maintainer, nor someone looking at a PR to get a handle of the in flight changes, but is someone that has looked at the diff. Thus, commit comments like, "address TOCTOU concern", "fix typo", "add null check", "add test case", "address feedback" all then make sense. On squash-merge, those comments are discarded and the merge is just the "commit written for the history". Otherwise, without squash - those commits have a much broader audience and become long lived. I believe this typically creates a much more useful history, eliminates a lot of noise, clarifies the audience of each commit message, and finally removes any overhead where a detailed description is written in a PR but then never captured in the history (wasteful &puts context in a location that is ephemeral and a step removed)

imiric 2y ago

I wasn't aware of this particular workflow, but I've heard of similar ones before. The late Pieter Hintjens, of ZeroMQ fame, advocated for a strategy called "optimistic merging"[1], which essentially abandons the standard code review process in favor of merging changes ASAP, and fixing any issues as they arise later. I'm not a fan of this. It allows contributors to abandon any established coding standards, while placing more burden on maintainers to fix issues later. This in practice never happens, so the quality of the codebase degrades over time. Not to mention that it allows malicious actors to merge changes much more easily. As for "ship, show, ask" specifically, I have similar reservations. I think all changes should go through a review process, even if someone just glances at the changes. It not only gives the opportunity to leave feedback, but also serves as communication so that everyone is aware of the proposed changes. Also, making the decision of whether to choose "ship", "show" or "ask" might only work for senior and well disciplined developers. I think that in most teams you have a mixture of experience, so you'd inevitably end up in situations where a change should've been "show" or "ask", but was "ship", and viceversa. I don't think teams will ever align on a single strategy to make a correct decision, since it's based on subjective opinion to begin with. Always following the same workflow gets rid of these uncertainties. [1]: http://hintjens.com/blog:106 http://hintjens.com/blog:106

seadan83 2y ago
After having done a few thousand CRs, I've come to believe that CR is of mixed value. Not always good, nor all good. Removing the bad CRs and doing CR when it is useful IMO helps maximizes productivity. First, I assume a few constraints on "ship, show, ask." First constraint is that all changes go for at least one, if not multiple self reviews. Second, the ability to ship is not outright granted. Until someone has demonstrated they can ship reasonable, working code- they are not given write permissions. They must "ask." Next, ship is not a given. Things that are not interesting are shipped. This enhances the communication part of PRs. If a PR is sent, it is important or wants extra review. That removes the noise of everything being a PR, and removes the noise from PRs where minor changes are smuggled in for the sake of efficiency. This also removes a lot of nitpicking as well. When asked to CR, we often feel we need to CR, and if our only feedback are minor things- then that is the feedback. 'Perfect' becomes the enemy of good enough. Saving time, efficiency are huge. Do CR when it is useful, and only then. Last, post merge reviews can still be done by the maintainers to ensure things are going smoothly. Hence, post merge review is an option for both ship and show. This means the coding lifecycle is blocked in CR only when the CR is valuable. That CR is blocking I feel is a very important drawback. Typos linger because time is finite and only those that do not appreciate that will try to fix everything. The team then gets bogged down spending too much time fixing small things. Leaving the small things is not ideal either. If the overhead to fix small things is tiny, suddenly it takes 5 minutes to do what would otherwise require another person and possibly the next day. For example, A PR preceded by renaming a few files is powerful. That is a game changer compared to a hard to review CR, staged PRs, or simply never doing the renaming because it runs the diff. Blanket CR can also ossify a project. Most projects I've seen are written quickly and in large chunks. If they are reviewed at all, it is a lot of 2000 line "LGTM", zero comment reviews. Then when careful CR is done (often by the "scaling" team), suddenly things that are original become gospel. The CR debates the absolute best naming, original intent, spending magnitudes more time than was originally put into the code in the first place. Meanwhile the code was done quickly, slightly worse than good enough- and now good enough is not even good enough. The CR now demands a far higher quality bar compared to the original code when it was written. Fixing a 50k line code base at 500 lines per day is a way to _not_ fix that codebase. The original bad code is therefor fossilized into the project. Time and effort are not infinite. That turned into a bit of a rant. I wanted to demonstrate that: (A) there are guard rails on SSA so it is not just a free for all. (B) selectively requesting CR when it is valuable amplifies the value of CR. (C) removing the noise of everything is a PR makes PRs stronger as communication tools. (D) post merge review of all code can still meet the goal of reviewing everything. I'll tie this back to git absorb now. Git absorb rebases, which arguably invalidates any CR that was done. Ship-Show-Ask allows for there to be more explicit trust where someone does their git absorb, redoes the self review and then merges. Further, the workflow helps clean up PRs to not be as many commits. Though, I do think the discussion of workflows and CR is really fascinating. Having been essentially just the one senior, or one of two on most all projects I've worked on in the last decade, - I'm certainly (overly) eager to discuss in detail the workflows.
- imiric 2y ago
  Thanks for your perspective. It's interesting to hear a different take on this. I do think there's no right or wrong answer with these things. Everyone will have different opinions about their ideal workflows, especially senior engineers. The important thing is to find a good balance that everyone on the team is satisfied with, but there will always be concessions, as in any relationship. What I have found, though, is that the more uncertainties a workflow has, the higher the chances of misunderstanding, or someone not following it in the way someone else thinks they should. So with the way "ship, show, ask" is described, and what you mention here, is that there are many constraints, and decisions to be made about what constitutes an "interesting" change, or what is worthy to be shipped without a CR and what isn't. If the team is mostly senior, and there's a lot of trust involved, this workflow might work, and it might reduce the friction of blocking CRs. But there are a lot of variables involved there. OTOH, by always requiring CRs and approvals for shipping, there are far less decisions to be made. The workflow is simple, fixed and known to everyone. Sure, sometimes CRs step into nitpicky territory, but this is usually solved by having some conventions, such as making clear what is a nitpick, what is a minor/major suggestion, and what is a blocker. This way nitpicks and minor suggestions can be safely ignored (as long as they're ackgnowledged in some way), and they're not blockers for merging. In practice I've found that while this workflow does take considerable time and effort, in the vast majority of cases it leads to higher quality code. Even addressing nitpicks and typo fixes is an improvement in most cases. These can be easily automated by suggestions in GitHub, which can be batched and committed directly from the web UI, so it doesn't take much effort at all. > Last, post merge reviews can still be done by the maintainers to ensure things are going smoothly. Sure, but is this diligently done? And how well does it scale if the project has many contributors, but few maintainers? Placing the burden on maintainers to ensure certain quality standards is unrealistic. Moreover, it assumes that maintainers have the final say in these matters, instead of their changes also being worthy of discussion, which happens during a CR. I think that the responsibility for ensuring things are running smoothly should be shared by every member of the team, including external contributors. > Most projects I've seen are written quickly and in large chunks. If they are reviewed at all, it is a lot of 2000 line "LGTM", zero comment reviews. Those are examples of not doing CRs correctly, not a testament that CRs are not worth the time and effort. You can just as well run into this issue with the SSA workflow. As with any issue with team dynamics, this should be resolved by communicating and aligning on core values and practices the team finds important. > The CR debates the absolute best naming, original intent, spending magnitudes more time than was originally put into the code in the first place. Yes, this is to be expected. It takes much more effort to read and understand code than it does to write it. It takes additional time and effort to leave a written comprehensible review, and communicate all of this asynchronously. This scales linearly for every member in the team, and there's no way around it. But the question is whether deciding to invest the time and effort in a well established CR process ultimately results in higher quality software being shipped. In my experience, it does, and it's worth it. That said, CRs are not the only or, even failproof, method of quality assurance. As mentioned in the SSA article, pair programming is another method, but it has its own drawbacks and limitations. All of these things help, and are best used in combination. They can be uncomfortable, tedious and seem pointless at times, but deciding to not do them because of this is doing a disservice to the final product.

keybored 2y ago

Why is the preferred goal to “rebase-squash” (redundant) merge? The implied onus here seems to be for the other party to move towards that strategy. But why this practice should be the goal does not seem to be mentioned. Step one isn’t to find some way to accept squash-merge. Step one is to find some reason for squash-merge.

seadan83 2y ago
I think you are coming at this from quite the different angle. To semi quote what I was responding to: > always squashing PRs is not a good strategy [because of the following reason] Hence my question,if workflow X mostly solves the following reasons, then always squashing is potentially a reasonable strategy. I think you are asking, why do that at all? The answer is IMO quite involved, nuanced, and not at all one sized fits all. FWIW: (1) all commits to master become atomic if a passing CI is required before rebase squash-merge. (2) rebase creates a linear history. Non linear history is very difficult to bisect, and can really be complicated. I don't see much positives from that complexity. (3) generally PRs are functional sized pieces of work. (That is ambiguous.) If it helps, PRs are more boulders than pebbles. Having boulders in the history is more useful than every small pebble. Net benefit is the history then becomes a smattering of well marked minor changes and a set of functional changes. Side note, I like anything that helps the workflow of "touch clean code only, touch only the code you need to, clean the code before you touch it". (Clean defined there as "not a pile of illogical, obtusely written crap - riddled with latent bugs). In other words, make the code simple and obvious first, then modify the simple and obvious code. I give little credit to those that spend days studying code to make a one-line "masterful" change. That does not scale, everyone has to do that, and there are thousands of lines. All things in their own context though... (4) utility and communication for commits. IMO ideally a PR is initiated with a single commit that is written "for the history". This commit description is both descriptive for the reviewers, the git history, and is the complete text for the PR. This creates the most value IMO. I view PRs as ephemeral, any extra text written for the sake of PR is overhead. Then, any following commits are pushed to communicate to the reviewers. The audience is no longer a future maintainer, nor someone looking at a PR to get a handle of the in flight changes, but is someone that has looked at the diff. Thus, commit comments like, "address TOCTOU concern", "fix typo", "add null check", "add test case", "address feedback" all then make sense. On squash-merge, those comments are discarded and the merge is just the "commit written for the history". Otherwise, without squash - those commits have a much broader audience and become long lived. I believe this typically creates a much more useful history, eliminates a lot of noise, clarifies the audience of each commit message, and finally removes any overhead where a detailed description is written in a PR but then never captured in the history (wasteful &puts context in a location that is ephemeral and a step removed)