Why do you claim that PR should be done by a single person?

[gquemener]

Hello,

I've been fond of TBD for quite some time, but never really took the time to read the introduction of your (rich) website!

I was a bit surprised to read that "Pull-Request workflow (should be) the product of a single person" (source).
Am I wrong to interpret that PR workflow should not be performed in a pair/mob programming setup? I fail to see why not.

Would you mind explaining the underlying reasoning behind this sentence, please?

EDIT: I see why it is unecessary in organization where pair/mob programming is considered as continuous reviewing, thus removing the need for a PR in the first place. However, some context (especially when learning TBD) requires to keep a (fast) PR review process, even with pair/mob programming, in my experience.

[paul-hammant]

Or a pair in the pair-programming style of eXtreme Programming (one of the Agile methodologies). Could be a four-day-to-implement PR, could have some pair changes before the end, so more than two in that case. Before the Zoom era (or https://pop.com/screenhero) it was easier to keep that collaborative work on one workstation and pair-swap in any way. Of course the commit/push is always that of the one logged in to that workstation, so you'd have a convention to credit pairs in the comment for the commits.

Mob, if at one workstation, would be the same.

I'll update it to say the same. I've never done a week of mobbing myself, just one offs. I've paired on and off since 2002 :)

[gquemener]

IMO, the most important aspect, that you already underline, is that any kind of feature branch should be short-lived (no matter how many devs are working on it!).

My suggestion would be to rephrase so that the mention to a "single person" would disappear.

Pairing (2 or more devs) has the advantage of keeping old habits (working in parallel to trunk), while ensuring short-lived branches (branches are reviewed continuously, so it can be merged directly after final commit). It provides a transition path.

[bradapp]

Note it doesnt have to be a "pair" in the pair-programming sense. It could be simply a double-teaming "duo" [like a mini-(2 person)-team], or even an [agile] triad (a.k.a. "three amigos"): in the former case, the duo isnt necessarily pairing, but using a "buddy system" for frequent reviews: - They are *double-teaming* the same feature/story/enhancement and they sync-up frequently (typically every 1-2 hours) - This includes "reviewing" what each other has done since their last sync-up. (Typically well under 100 lines of changed/added code, ideally no more than 50). - They don't necessarily *need *a PR for each of these reviews (other than possibly to create a "diff/review" package), but they may often use one simply as evidence that such a review took place (and with whom)! Everything is still synchronous (there is no async waiting/blocking of any kind) A similar scenario can occur with a typical "agile triad" (a.k.a. "three amigos"): - It's not akin to "triple teaming" because each is playing a separate role (e.g., BA, Dev, Tester) - But it's a very common way to use BDD or ATDD, along with "executable specification" (e.g. Cucumber's "Feature files') or even just SBE (Specification by Example) - It may not even be "test-first" but may still be concurrent/parallel dev+test (after theyve concurred with BA on the "spec" and 'interface under test') - But they should still be syncing up frequently enough (i.e. behavior/functional-test by behavioral/functional-test) - NOTE: HERE they use the PR not so much for code-review, but rather as evidence of 'approval' for ready-to-integrate - (Because that's what PR's really are! They arent really about reviewing code -- and arent needed for that; they're really about getting approval/signoff to integrate)

…

On Tue, Apr 1, 2025 at 8:40 AM Paul Hammant ***@***.***> wrote: Or a pair in the pair-programming style of eXtreme Programming (one of the Agile methodologies). Could be a four-day-to-implement PR, could have some pair changes before the end, so more than two in that case. Before the Zoom era (or https://pop.com/screenhero) it was easier to keep that collaborative work on one workstation and pair-swap in any way. Of course the commit/push is always that of the one logged in to that workstation, so you'd have a convention to credit pairs in the comment for the commits. Mob, if at one workstation, would be the same. I'll update it to say the same. I've never done a week of mobbing myself, just one offs. I've paired on and off since 2002 :) — Reply to this email directly, view it on GitHub <#122 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADKOCOJB5CFLQGI3RSKFJFD2XKJMHAVCNFSM6AAAAAB2G5QCWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRZGQYDANBXGU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***> [image: paul-hammant]*paul-hammant* left a comment (paul-hammant/tbd#122) <#122 (comment)> Or a pair in the pair-programming style of eXtreme Programming (one of the Agile methodologies). Could be a four-day-to-implement PR, could have some pair changes before the end, so more than two in that case. Before the Zoom era (or https://pop.com/screenhero) it was easier to keep that collaborative work on one workstation and pair-swap in any way. Of course the commit/push is always that of the one logged in to that workstation, so you'd have a convention to credit pairs in the comment for the commits. Mob, if at one workstation, would be the same. I'll update it to say the same. I've never done a week of mobbing myself, just one offs. I've paired on and off since 2002 :) — Reply to this email directly, view it on GitHub <#122 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADKOCOJB5CFLQGI3RSKFJFD2XKJMHAVCNFSM6AAAAAB2G5QCWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRZGQYDANBXGU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[paul-hammant]

I've experienced BA and QE rolling into a pair (and out) for things around the story that they're expert on. Same for experts on on particular technology (DB, CSS) that the pair thinks they're NOT expert on. Good thought, Brad.

And yes, some teams trying to get as close to the intentions of CI are reviewing code at a desk, without that being a PR as such. That's not quite the "desk check" where a product owner might walk to a developer workstation, see the app instantiated locally pre-commit and judge the correctness of it before the commit (and push these days)...

... There was a horrible period of time where product owner checking of correctness would be in a QA environment that was deployed to but otherwise shared. And if the PO (or BA, or user) didn't like it in QA, it would be worked on some more and deployed again. Brittle and slow loops, leading to a long term suspect quality QA env. Un-agile, un-CI and un-CD. I digress.

I'm not sure, but you're saying you've seen committers live the ideal of the short-lived-feature branch cooperating on a story. Say one pair does backend (biz logic, SQL/DML/DDL) commits to the branch, and another pair does frontend commits (JavaScript, HTML, CSS) to the same branch. Perhaps with lots of chat on the lew key evolution of that and pull/push before the final declaration of "we're finished".

[bradapp]

Hmmn - youre using some terms differently - let's get those out of the way first. *"Desk Check"* -- in the world of peer-review & code-inspection, this has nothing to do with product-owners or business-users validating/signing-off anything. It refers to a review/inspection where the changes are small enough to not require a 'formal review' of 4 or more people, but can instead be done as a "desk check: with just one other person. This is the origin of the term "desk check" when it comes to code-reviews and PRs - It's not a "desk-check" if there are no artifact-changes being reviewed/inspected - PO-signoff and/or Business-User "acceptance" is typically called either [business-]acceptance, or [customer-]validation: It's part of the outer-loop <https://antonymarcano.com/blog/2014/02/old-favourite-radio-by-example/> (or "outer" test-loop <http://www.growing-object-oriented-software.com/figures/tdd-with-acceptance-tests.svg>) in the GOOS book <https://www.amazon.com/Growing-Object-Oriented-Software-Guided-Tests/dp/0321503627> that describes TDD as covering both acceptance [functional] test *and* unit [programmer] tests. The main inspection/review "methodologies" that were used when this started was typically one of (in order of popularity/fame): Fagan Inspection <https://en.wikipedia.org/wiki/Fagan_inspection>; Gilb-Graham <https://www.amazon.com/Software-Inspection-Author-Gilb-October/dp/B006V38ZR0/> (Tom Gilb & Dorothy Graham); and Ebenau-Strauss <https://www.amazon.com/Software-Inspection-Process-Systems-Implementation/dp/0070621667>. All three of these acknowledged that, if the changes ate small enough, it is sufficient to do an 'informal;' desk-check rather than a formal-inspection. For source-code in particular, it was generally (often) the case that 25lines or less of added/changed code, and only a desk-check was needed. HOWEVER, the suggested that data-should still be collected regarding: - defects traced back to "desk-checked" code + defect-density (per "module", where a module could be a "component", or a source-file). - If the "defect-density" for a component was low-enough, some organizations could often go as high as 50 changed-lines for a desk-check. - whereas others might require anything >10 lines of changes to be formally inspected if the defect-density was not low-enough * [Paul] >>* "*you're saying you've seen committers live the ideal of the short-lived-feature branch cooperating on a story." * Yes. But what you write next is something different. * [Paul]>> *"Say one pair does backend commits to the branch [...] and another pair does frontend commits [...] to the same branch" In my example, I wasnt talking about "two pairs" of (i.e. 4) programmers. I was talking about 2 (and only 2) programmers: - they work together on the same "story" (or "job", or whatever you call the low-level backlog-item) - they DO work together on a *shared* [feature/story] branch, in a *shared* workspace/sandbox env - they do NOT do *proper* pair programming (i.e., not always both looking & working at the same screen in the same session the whole team) - Instead, it's more like a ... *Pattern: 2-person "mini" feature-team *(or story-team). 1. they initially chat to talk about who's going to do what next (i.e. for the next hour or two). The even do some shared brainstorming/planning/etc. 2. then, for the next 1-2 hours (or less), each is at their own desk/screen, working in the same branch & sandbox, doing whatever they each agreed to work-on 3. after 1hr or so (possibly less), they sync-up again and review what each has done so for (any changes, tests, etc.) 4. They may (or may not) choose to integrate their current "checkpoint" of what theyve done to the mainline (i.e., "mainline their changes) 5. Then they rinse, lather & repeat. They might noty mainline EVERY time thru this micro-cycle. But they should be doing it at least daily (ideally at least a couple-times a day, if not more) *Pattern:* *"Buddy System"* The OTHER EXAMPLE I gave was what was often called the "Buddy system". 1. Each programmer on the team was assigned their own separate story/task to work on 2. BUT they were required to do no more than 25 or so lines of change at a time (based on what their "desk-check" threshhold-limit was) 3. They might use a timer to go off every 20, 30 or 60min (or they might do it without a "timer") After that period of time (or ~25lines of change -- which ever came first) it was sort-of like a "buddy check" when you were swimming at summer-camp: - You had to "find your buddy" ASAP - You reviewed each other's code-changes (desk-check) - you each made your changes, rebuild + re-test and then "commit" your checkpoint (each to your own branch) Again, they might not each integrate after EVERY "buddy-checked" checkpoint (they might alternate, so not everyone was trying to build at the same time during the same integration-window). But they'd integrate to shared main it at LEAST daily (preferably at least 2x per day). Both of these were exceedingly common in ClearCase, Perforce, even CVS and Subversion back in the 90s and early 2000s. (along with frequent/incremental integration - even if it wasnt always 'continuous') *Patterns: Shared Branch, Shared Workspace/SandboxView/Dev-Env* But also "shared branch" and "shared view/sandbox" were almost equally common (in ClearCase/Perforce/AccurRev it was a "shared view"; in CVS & SVN is was a "shared sandbox"; in Accu-Rev it was a "shared stream" instead of a "shared branch"). *Pattern variation*: Shared Branch+Sanddox In this case a "*subteam*" or "*mini-team*" as we called it back then would work closely together (in the same branch + workspace/env) doing what we today would most likely call 'swarming' or 'mobbing' 1. either the whole (small) team, or a subteam/mini-team (2-4 devs) would work together similar to what I called a "2-person feature-team" above 2. But every 30-60 min or so, instead of finding your "review partner" for a desk-check, they could instead "congregate" in a team-room/team-area (sometimes even a foos-ball table :-) 3. If "sub-teams" then each subteam would review the other subteams-changes; if small "whole team:" they would split into subteams (or 2-3) to review each-others changes. Each (sub)team would commit changes to it's shared-branch in its shared-view, and least daily (preferably at least 2x/day) they would integrate to main ("a.k.k. "mainline" their changes). *HOWTO Before Git & PRs* In the days before Git or PRs, if you wanted a tool to help you do code review/inspections, there was ReviewBoard <https://www.reviewboard.org/>, FishEye (Cenqua), Collaborator (SmartBear), Crucible (Atlassian), Reitveld (predecessor of Gerrit), and more. And if you were using a centralized VCS, they all pretty much had some kind of 'pre' integration "hook" to enforce these basic rules: - you had to have pulled (or rebased) with the latest changes on mainline before attempting to merge to main - you could also check for a CODE-REVIEWED meta-data tag/attribute of some kind (or even in a comment) associated with the branch (or the commit) *HOWTO With Git* All these same patterns translated directly to using Git, where a "shared branch" meant the same thing as above, and a "shared workspace/sandbox env" translated to the same local (git) repo. *If using "Gerrit"*: - there was no such thing as PRs. They were "code-reviews", and they were less about getting signoff and more about getting code reviews - Gerrit required review PER Commit to be merged (so you might "squash" some to collapse them into the same review) - Gerrit also had an configurable signoff "scoring" system to make it easy to auto-signofff (i.e. synchronous, instead of async/blocking) - Gerrit also had configurable review-to-merge workflow states (this was less well known), which also made it easier to automate streamline reviews/signoffs *When using Gitolite, or the equivalent *(i.,e., no formal code-review mechanism) : - team-members working on a* shared-branch* from a *private (non-shared) local-repo* would simply to a pull-then-push to create a "diff/review package" in the shared repository so others could see/view it - (like a "pushmi-pullyu", from Dr. Doolittle [the book, or the Rex Harrison movie] -- only it was really the other order "pull-you ==> push-me) *Facebook's "Phabricator <https://en.wikipedia.org/wiki/Phabricator>" toolset *made this easiest (see intro/overview <https://secure.phabricator.com/book/phabricator/article/introduction/> -- it is called Graphite <https://graphite.dev/> these days) - it gave a way to do the code-review in local (or central) repo BEFORE "submitting" for integration - using pre-integration code-reviews, eliminated handoff/waiting (the 'primary' dev was responsible for making sure any necessary code-review was logged BEFORE attempting to integrate - at integration-time, when initiating the merge to mainline, it would check to see if any required code-review was logged/stamped - if not, there was no queuing/async handoff, it just aborted, telling you to try again AFTER code-review (i.e. "go away and dont come back until you have the broom of the wicked witch of the west") I believe *NetFlix *created/used a similar suite, until they settled on GitHub and its PRs. *RhodeCode *also facilitated this (without requiring PRs) because you could do code-reviews of *commits* (without creating a PR). [You could also do PRs if you wanted to] *With PRs:* The rest of us that were stuck using systems that had PRs (GitHub, GitLab, Bitbucket, etc.), if we wanted to do TRUE Continuous Integration (or even just frequent incremental integration) would automate this ourselves with scripting (perl, python, ruby, groovy) to "*Hijack the PR mechanism*"! 1. First off, for any non-negligible-sized codebase (or well populated Git-Server), you did NOT want to do "polling" on the integration branch the way your typical CruiseControl/Hudson/Jenkins setup did it. This was a performance/resource hog - particularly if every team (much less every repo) using that GitHub/GitLab/Bitbucket "instance" was going to "poll" all their integration branches 2. Instead, you would follow the guidance from the "Continuous Integration" book (2007, Duvall. et.al.) and implement the *"Integration Button" pattern*. - You could use pre-commit hooks for the central-repo to do this - And/Or you could (literally) implement an "integrate" (or "mainline") command (and menu-item in UI) -- but then it had to be accessible from every dev's workstation (possibly as part of an 'extended' local Git install) - And/Or you could "hijack" the implementation of PRs to get your own code to execute per/post "PR" creation, as well as to automate (or nearly automate) signoff/approval - All three of those options were actually a LOT more common than you would have guessed!!!

…

-- Brad Appleton (he/they) ***@***.****> * "And miles to go before I sleep" --Robert Frost

On Wed, Apr 2, 2025 at 10:00 AM Paul Hammant ***@***.***> wrote: I've experienced BA and QE rolling into a pair (and out) for things around the story that they're expert on. Same for experts on on particular technology (DB, CSS) that the pair thinks they're NOT expert on. Good thought, Brad. And yes, some teams trying to get as close to the intentions of CI are reviewing code at a desk, without that being a PR as such. That's not quite the "desk check" where a product owner might walk to a developer workstation, see the app instantiated locally pre-commit and judge the correctness of it before the commit (and push these days)... ... There was a horrible period of time where product owner checking of correctness would be in a QA environment that was deployed to but otherwise shared. And if the PO (or BA, or user) didn't like it in QA, it would be worked on some more and deployed again. Brittle and slow loops, leading to a long term suspect quality QA env. Un-agile, un-CI and un-CD. I digress. I'm not sure, but you're saying you've seen committers live the ideal of the short-lived-feature branch cooperating on a story. Say one pair does backend (biz logic, SQL/DML/DDL) commits to the branch, and another pair does frontend commits (JavaScript, HTML, CSS) to the same branch. Perhaps with lots of chat on the lew key evolution of that and pull/push before the final declaration of "we're finished". — Reply to this email directly, view it on GitHub <#122 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADKOCOMN5RM453OTAV5O2DD2XP3P5AVCNFSM6AAAAAB2G5QCWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONZSHA3DINJXGE> . You are receiving this because you commented.Message ID: ***@***.***> [image: paul-hammant]*paul-hammant* left a comment (paul-hammant/tbd#122) <#122 (comment)> I've experienced BA and QE rolling into a pair (and out) for things around the story that they're expert on. Same for experts on on particular technology (DB, CSS) that the pair thinks they're NOT expert on. Good thought, Brad. And yes, some teams trying to get as close to the intentions of CI are reviewing code at a desk, without that being a PR as such. That's not quite the "desk check" where a product owner might walk to a developer workstation, see the app instantiated locally pre-commit and judge the correctness of it before the commit (and push these days)... ... There was a horrible period of time where product owner checking of correctness would be in a QA environment that was deployed to but otherwise shared. And if the PO (or BA, or user) didn't like it in QA, it would be worked on some more and deployed again. Brittle and slow loops, leading to a long term suspect quality QA env. Un-agile, un-CI and un-CD. I digress. I'm not sure, but you're saying you've seen committers live the ideal of the short-lived-feature branch cooperating on a story. Say one pair does backend (biz logic, SQL/DML/DDL) commits to the branch, and another pair does frontend commits (JavaScript, HTML, CSS) to the same branch. Perhaps with lots of chat on the lew key evolution of that and pull/push before the final declaration of "we're finished". — Reply to this email directly, view it on GitHub <#122 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADKOCOMN5RM453OTAV5O2DD2XP3P5AVCNFSM6AAAAAB2G5QCWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONZSHA3DINJXGE> . You are receiving this because you commented.Message ID: ***@***.***>

[paul-hammant]

^^ Hey friends of TBD, please review change just made ^^

[tdpauw]

Done 😉
f04867e#commitcomment-156383449

[bradapp]

Sorry for the late followup ... I wonder if there might be a more fundamental (mis?)understanding going on here, that of ... === *"Push" integration model versus "Pull" integration model* === These 'models" and there name "precede" git, and are still in common use today, but Git's implementation of the commands named "push" and "pull" are *not *consistent with these models, and tend to cause a lot of misinformation & misunderstanding! A) *git push* is strictly a transport command: it "transports" a change from the local repo to the remote (central) repo [between branches of the same name]. Ideally it just syncs/mirrors from local to remote, but it will attempt to merge, if necessary. B) But the *"Push" integration model* means *"developer push" *as in the *developer* is supposed to merge their own change(s) to the target integration branch. (If we assume that is "*main*" then the developer takes responsibility for "mainlining" their changes) C) And the *"Pull" integration model* means *"integrator pull"*, as in someone other than the developer is responsible for merging of developer changes to the target integration branch (e.g. '*main*') D) However both '*git pull*' and '*git merge*' *always assume* the "current branch" is ALSO the target merge/integration branch! (which is a really bad assumption) E) There's actually *a 3rd model* that is a *compromise *between "*developer push*" and "*integrator pull*": it's called "*merge token*" (a.k.a. "*merge permission*") ==> The "developer" gets to merge their own changes ONLY AFTER someone else (e.g. a integrator/build/CM person) gives them explicit permission to do so (i.e. a "merge token") Git seems to go out of its way to make it extra difficult to do true "developer push" integration from any branch that is not the integration branch (if someone would 'fix' fix 'git merge' to allow explicitly specifying the "to" branch - that would be a big help). PRs (in GitHub, Bitbucket, etc.) and MRs (in GitLab) make it even harder to attempt true "developer push" integration (which is what you want for true CI/CD). ==> PRs practically force you to do either "*integrator pull*" (non-author merges) or else *merge-token* (both with a blocking 'wait' for signoff/approval) ==> This typically makes PRs be more about *requesting* merge-approval much more so than reviewing/improving code-quality And when requesting approval, there is typically just a single annointed authority required to "bless" the given change to be be merged to the "blessed" (integration) branch of the "blessed" (central) repository [also called the "single wringable neck" in IT mgmt literature) But for those really concerned with reviewing/inspecting the code for "code quality" (including coding stds/conventions) it's quite common to want MULTIPLE reviewers for a code-change (especially for medium & large sized changes)! - In fact, the major code review/inspection methods usually call for anywhere from 2-4 reviewers (besides the author) for a given change - and for large changes (e.g. 100 added/changed lines or more) you probably should have >1 reviewer - But for smaller changes (e.g, <=25lines, *maybe* upto 50) it's usually sufficient to have a SINGLE reviewer (and much more expedient). [ ==> [This is what is known as "deskcheck" review in peer code review/inspection literature] ==> [This *should not be confused* with the kind of deskcheck validation/acceptance done by a PO for an entire story (vs for a single change)] This is the REAL intent behind the claim that a "PR should be done by a single person" - it's talking about the review/approval of a change (which might be just one incremental piece of a story's implementation) - and because PR's often require blocking wait-time, the fewer 'approvers' the better/shorter the wait-time - which also encourages making small (incremental) changes, small enough for a single 'approver' to review quickly yet effectively Again, all this because PRs (and git) make it harder to do true "developer push" integration, and tend to turn what should be real code reviews/inspections into merge-approvals instead. (i.e. where the developer is responsible for getting their changes reviewed *before* attempting to submit their change for integration)

…

-- Brad Appleton (he/they) ***@***.****> * "And miles to go before I sleep" --Robert Frost On Mon, Apr 14, 2025 at 5:36 PM Brad Appleton ***@***.***> wrote:

Hmmn - youre using some terms differently - let's get those out of the way first. *"Desk Check"* -- in the world of peer-review & code-inspection, this has nothing to do with product-owners or business-users validating/signing-off anything. It refers to a review/inspection where the changes are small enough to not require a 'formal review' of 4 or more people, but can instead be done as a "desk check: with just one other person. This is the origin of the term "desk check" when it comes to code-reviews and PRs - It's not a "desk-check" if there are no artifact-changes being reviewed/inspected - PO-signoff and/or Business-User "acceptance" is typically called either [business-]acceptance, or [customer-]validation: It's part of the outer-loop <https://antonymarcano.com/blog/2014/02/old-favourite-radio-by-example/> (or "outer" test-loop <http://www.growing-object-oriented-software.com/figures/tdd-with-acceptance-tests.svg>) in the GOOS book <https://www.amazon.com/Growing-Object-Oriented-Software-Guided-Tests/dp/0321503627> that describes TDD as covering both acceptance [functional] test *and* unit [programmer] tests. The main inspection/review "methodologies" that were used when this started was typically one of (in order of popularity/fame): Fagan Inspection <https://en.wikipedia.org/wiki/Fagan_inspection>; Gilb-Graham <https://www.amazon.com/Software-Inspection-Author-Gilb-October/dp/B006V38ZR0/> (Tom Gilb & Dorothy Graham); and Ebenau-Strauss <https://www.amazon.com/Software-Inspection-Process-Systems-Implementation/dp/0070621667>. All three of these acknowledged that, if the changes ate small enough, it is sufficient to do an 'informal;' desk-check rather than a formal-inspection. For source-code in particular, it was generally (often) the case that 25lines or less of added/changed code, and only a desk-check was needed. HOWEVER, the suggested that data-should still be collected regarding: - defects traced back to "desk-checked" code + defect-density (per "module", where a module could be a "component", or a source-file). - If the "defect-density" for a component was low-enough, some organizations could often go as high as 50 changed-lines for a desk-check. - whereas others might require anything >10 lines of changes to be formally inspected if the defect-density was not low-enough * [Paul] >>* "*you're saying you've seen committers live the ideal of the short-lived-feature branch cooperating on a story." * Yes. But what you write next is something different. * [Paul]>> *"Say one pair does backend commits to the branch [...] and another pair does frontend commits [...] to the same branch" In my example, I wasnt talking about "two pairs" of (i.e. 4) programmers. I was talking about 2 (and only 2) programmers: - they work together on the same "story" (or "job", or whatever you call the low-level backlog-item) - they DO work together on a *shared* [feature/story] branch, in a *shared* workspace/sandbox env - they do NOT do *proper* pair programming (i.e., not always both looking & working at the same screen in the same session the whole team) - Instead, it's more like a ... *Pattern: 2-person "mini" feature-team *(or story-team). 1. they initially chat to talk about who's going to do what next (i.e. for the next hour or two). The even do some shared brainstorming/planning/etc. 2. then, for the next 1-2 hours (or less), each is at their own desk/screen, working in the same branch & sandbox, doing whatever they each agreed to work-on 3. after 1hr or so (possibly less), they sync-up again and review what each has done so for (any changes, tests, etc.) 4. They may (or may not) choose to integrate their current "checkpoint" of what theyve done to the mainline (i.e., "mainline their changes) 5. Then they rinse, lather & repeat. They might noty mainline EVERY time thru this micro-cycle. But they should be doing it at least daily (ideally at least a couple-times a day, if not more) *Pattern:* *"Buddy System"* The OTHER EXAMPLE I gave was what was often called the "Buddy system". 1. Each programmer on the team was assigned their own separate story/task to work on 2. BUT they were required to do no more than 25 or so lines of change at a time (based on what their "desk-check" threshhold-limit was) 3. They might use a timer to go off every 20, 30 or 60min (or they might do it without a "timer") After that period of time (or ~25lines of change -- which ever came first) it was sort-of like a "buddy check" when you were swimming at summer-camp: - You had to "find your buddy" ASAP - You reviewed each other's code-changes (desk-check) - you each made your changes, rebuild + re-test and then "commit" your checkpoint (each to your own branch) Again, they might not each integrate after EVERY "buddy-checked" checkpoint (they might alternate, so not everyone was trying to build at the same time during the same integration-window). But they'd integrate to shared main it at LEAST daily (preferably at least 2x per day). Both of these were exceedingly common in ClearCase, Perforce, even CVS and Subversion back in the 90s and early 2000s. (along with frequent/incremental integration - even if it wasnt always 'continuous') *Patterns: Shared Branch, Shared Workspace/SandboxView/Dev-Env* But also "shared branch" and "shared view/sandbox" were almost equally common (in ClearCase/Perforce/AccurRev it was a "shared view"; in CVS & SVN is was a "shared sandbox"; in Accu-Rev it was a "shared stream" instead of a "shared branch"). *Pattern variation*: Shared Branch+Sanddox In this case a "*subteam*" or "*mini-team*" as we called it back then would work closely together (in the same branch + workspace/env) doing what we today would most likely call 'swarming' or 'mobbing' 1. either the whole (small) team, or a subteam/mini-team (2-4 devs) would work together similar to what I called a "2-person feature-team" above 2. But every 30-60 min or so, instead of finding your "review partner" for a desk-check, they could instead "congregate" in a team-room/team-area (sometimes even a foos-ball table :-) 3. If "sub-teams" then each subteam would review the other subteams-changes; if small "whole team:" they would split into subteams (or 2-3) to review each-others changes. Each (sub)team would commit changes to it's shared-branch in its shared-view, and least daily (preferably at least 2x/day) they would integrate to main ("a.k.k. "mainline" their changes). *HOWTO Before Git & PRs* In the days before Git or PRs, if you wanted a tool to help you do code review/inspections, there was ReviewBoard <https://www.reviewboard.org/>, FishEye (Cenqua), Collaborator (SmartBear), Crucible (Atlassian), Reitveld (predecessor of Gerrit), and more. And if you were using a centralized VCS, they all pretty much had some kind of 'pre' integration "hook" to enforce these basic rules: - you had to have pulled (or rebased) with the latest changes on mainline before attempting to merge to main - you could also check for a CODE-REVIEWED meta-data tag/attribute of some kind (or even in a comment) associated with the branch (or the commit) *HOWTO With Git* All these same patterns translated directly to using Git, where a "shared branch" meant the same thing as above, and a "shared workspace/sandbox env" translated to the same local (git) repo. *If using "Gerrit"*: - there was no such thing as PRs. They were "code-reviews", and they were less about getting signoff and more about getting code reviews - Gerrit required review PER Commit to be merged (so you might "squash" some to collapse them into the same review) - Gerrit also had an configurable signoff "scoring" system to make it easy to auto-signofff (i.e. synchronous, instead of async/blocking) - Gerrit also had configurable review-to-merge workflow states (this was less well known), which also made it easier to automate streamline reviews/signoffs *When using Gitolite, or the equivalent *(i.,e., no formal code-review mechanism) : - team-members working on a* shared-branch* from a *private (non-shared) local-repo* would simply to a pull-then-push to create a "diff/review package" in the shared repository so others could see/view it - (like a "pushmi-pullyu", from Dr. Doolittle [the book, or the Rex Harrison movie] -- only it was really the other order "pull-you ==> push-me) *Facebook's "Phabricator <https://en.wikipedia.org/wiki/Phabricator>" toolset *made this easiest (see intro/overview <https://secure.phabricator.com/book/phabricator/article/introduction/> -- it is called Graphite <https://graphite.dev/> these days) - it gave a way to do the code-review in local (or central) repo BEFORE "submitting" for integration - using pre-integration code-reviews, eliminated handoff/waiting (the 'primary' dev was responsible for making sure any necessary code-review was logged BEFORE attempting to integrate - at integration-time, when initiating the merge to mainline, it would check to see if any required code-review was logged/stamped - if not, there was no queuing/async handoff, it just aborted, telling you to try again AFTER code-review (i.e. "go away and dont come back until you have the broom of the wicked witch of the west") I believe *NetFlix *created/used a similar suite, until they settled on GitHub and its PRs. *RhodeCode *also facilitated this (without requiring PRs) because you could do code-reviews of *commits* (without creating a PR). [You could also do PRs if you wanted to] *With PRs:* The rest of us that were stuck using systems that had PRs (GitHub, GitLab, Bitbucket, etc.), if we wanted to do TRUE Continuous Integration (or even just frequent incremental integration) would automate this ourselves with scripting (perl, python, ruby, groovy) to "*Hijack the PR mechanism*"! 1. First off, for any non-negligible-sized codebase (or well populated Git-Server), you did NOT want to do "polling" on the integration branch the way your typical CruiseControl/Hudson/Jenkins setup did it. This was a performance/resource hog - particularly if every team (much less every repo) using that GitHub/GitLab/Bitbucket "instance" was going to "poll" all their integration branches 2. Instead, you would follow the guidance from the "Continuous Integration" book (2007, Duvall. et.al.) and implement the *"Integration Button" pattern*. - You could use pre-commit hooks for the central-repo to do this - And/Or you could (literally) implement an "integrate" (or "mainline") command (and menu-item in UI) -- but then it had to be accessible from every dev's workstation (possibly as part of an 'extended' local Git install) - And/Or you could "hijack" the implementation of PRs to get your own code to execute per/post "PR" creation, as well as to automate (or nearly automate) signoff/approval - All three of those options were actually a LOT more common than you would have guessed!!! -- Brad Appleton (he/they) ***@***.****> * "And miles to go before I sleep" --Robert Frost On Wed, Apr 2, 2025 at 10:00 AM Paul Hammant ***@***.***> wrote: > I've experienced BA and QE rolling into a pair (and out) for things > around the story that they're expert on. Same for experts on on particular > technology (DB, CSS) that the pair thinks they're NOT expert on. Good > thought, Brad. > > And yes, some teams trying to get as close to the intentions of CI are > reviewing code at a desk, without that being a PR as such. That's not quite > the "desk check" where a product owner might walk to a developer > workstation, see the app instantiated locally pre-commit and judge the > correctness of it before the commit (and push these days)... > > ... There was a horrible period of time where product owner checking of > correctness would be in a QA environment that was deployed to but otherwise > shared. And if the PO (or BA, or user) didn't like it in QA, it would be > worked on some more and deployed again. Brittle and slow loops, leading to > a long term suspect quality QA env. Un-agile, un-CI and un-CD. I digress. > > I'm not sure, but you're saying you've seen committers live the ideal of > the short-lived-feature branch cooperating on a story. Say one pair does > backend (biz logic, SQL/DML/DDL) commits to the branch, and another pair > does frontend commits (JavaScript, HTML, CSS) to the same branch. Perhaps > with lots of chat on the lew key evolution of that and pull/push before the > final declaration of "we're finished". > > — > Reply to this email directly, view it on GitHub > <#122 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ADKOCOMN5RM453OTAV5O2DD2XP3P5AVCNFSM6AAAAAB2G5QCWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONZSHA3DINJXGE> > . > You are receiving this because you commented.Message ID: > ***@***.***> > [image: paul-hammant]*paul-hammant* left a comment (paul-hammant/tbd#122) > <#122 (comment)> > > I've experienced BA and QE rolling into a pair (and out) for things > around the story that they're expert on. Same for experts on on particular > technology (DB, CSS) that the pair thinks they're NOT expert on. Good > thought, Brad. > > And yes, some teams trying to get as close to the intentions of CI are > reviewing code at a desk, without that being a PR as such. That's not quite > the "desk check" where a product owner might walk to a developer > workstation, see the app instantiated locally pre-commit and judge the > correctness of it before the commit (and push these days)... > > ... There was a horrible period of time where product owner checking of > correctness would be in a QA environment that was deployed to but otherwise > shared. And if the PO (or BA, or user) didn't like it in QA, it would be > worked on some more and deployed again. Brittle and slow loops, leading to > a long term suspect quality QA env. Un-agile, un-CI and un-CD. I digress. > > I'm not sure, but you're saying you've seen committers live the ideal of > the short-lived-feature branch cooperating on a story. Say one pair does > backend (biz logic, SQL/DML/DDL) commits to the branch, and another pair > does frontend commits (JavaScript, HTML, CSS) to the same branch. Perhaps > with lots of chat on the lew key evolution of that and pull/push before the > final declaration of "we're finished". > > — > Reply to this email directly, view it on GitHub > <#122 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ADKOCOMN5RM453OTAV5O2DD2XP3P5AVCNFSM6AAAAAB2G5QCWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONZSHA3DINJXGE> > . > You are receiving this because you commented.Message ID: > ***@***.***> >

[paul-hammant]

Thanks Brad - lots of reminders about the subtleties, there.