3 ms·

Curious how pre-commit code review worked, could you please elaborate a bit?

jeffbee 3y ago
Every change is reviewed by someone other than the author before it lands in the repo. At google they take this a bit further. Every change has to have been either written or reviewed by a designated owner of the code (designated by subdirectory) and one of the participants must be a qualified user of the languages used in the change ("readability"). And they have technical measures in place to ensure that programs running in production descend exclusively from reviewed and committed code. Pre-commit review is common but not universal in the industry. Some shops practice post-commit review or no review. Some believe review consists entirely of quibbling.
- dinvlad 3y ago
  Oh, I see - by "pre-commit" it doesn't really imply it in "Git commit" sense - the change is still propagated to others (presumably by committing it as a sort of "draft"), it's just not committed to the mainline - is that correct? I'm very familiar with CR at other companies, but tbh since most use Git, I wouldn't call that "pre-commit" but "pre-merge", if you will - unless I misunderstood and it really is pre-commit at Google (i.e. the changes are not even _committed_ to the repository - and then I'm confused once again at what exactly that means..)
  - joshuamorton 3y ago
    Google (for $reasons) doesn't do long lives code branches and doesn't use git, at least for the main repo. So in that context every commit is reviewed pre-commit, but you'd do the same workflow elsewhere with trunk-based development, small pull requests, and CI and automated and human review of all PRs before they're merged.
    - dinvlad 3y ago
      Right, I know it’s not on Git and hence was my question - and it sounds like this is more about terminology and less about technology. I.e. what Google does in this case is not that different from just “Code Review” in the traditional sense, as most other companies (with good engineering practices) do - reviewing code before it enters production (+CI/CD, as you mentioned). Edit: as OP mentioned, it does seem to differ in technical sense from traditional CR, in that the changes live only on developer machine, not in source control.
      - joshuamorton 3y ago
        > Edit: as OP mentioned, it does seem to differ in technical sense from traditional CR, in that the changes live only on developer machine, not in source control. Yes and no. There's a decent whitepaper on Piper and citc you can find by searching for it (or actually I will :P [0]), as far as piper is concerned they aren't checked into source control, but the vast majority of development happens in "citc" workspaces, which aren't source control, but also are source control in a sense that every save is snapshotted and you can do point in time or point in history recovery, and I can open up a readonly view of someone else's workspace if I know the right incantations, and most of the developer tools treat the local citc workspace enough-like a commit that it's transparent ish. [0]: https://cacm.acm.org/magazines/2016/7/204032-why-google-stores-billions-of-lines-of-code-in-a-single-repository/fulltext https://cacm.acm.org/magazines/2016/7/204032-why-google-stor...
        dinvlad 3y ago
        OK, thanks for these details and the link! Somehow I remember the title, but not the content :-) That'll be an interesting read.
        nonethewiser 3y ago
        > and I can open up a readonly view of someone else's workspace if I know the right incantations So thats not just a basic feature?
        UncleMeat 3y ago
  - jeffbee 3y ago
    Yeah, there are no direct translations between git and perforce concepts. The right term within Google would in fact be "pre-submit" not "pre-commit". Before a change is submitted in the Perforce-derived flow it exists only in the author's client and isn't really part of source control in the way that git users are accustomed to pushing their branch to origin. NB: At that company there are also users of git-compatible and hg-compatible tools, but I am discussing the main perforce-derived flow.
    - dinvlad 3y ago
      Oh I see, thanks! I was under impression Google has migrated away from Perforce towards an in-house system a while ago, but looks like I was mistaken (or do you mean that system is derived from Perforce?). Edit: I guess its name is Piper.. It’s quite interesting/mind-bending to think of work-in-progress that’s still somehow synced between peers (in fact this is one of those “missing nice-to-haves” I wish Git had, and can only be approximated with wip branches..)
      - jeffbee 3y ago
        The synced-between-peers features are built atop a thing call CitC, or Client in the Cloud. An author's client isn't on their machine, it's hosted in prod.
        dinvlad 3y ago
        OK got it, thanks for clarifying it
- klooney 3y ago
  The real problem with code review is that if people don't do it/just hit sign off, it's worthless. Your whole company has to believe.
benlwalker 3y ago
Pre-commit means before committed to the canonical repo, not before commit locally. The SPDK project has an elaborate pre-commit review and test system all in public. See https://spdk.io/development https://spdk.io/development . I wouldn't want to work on a project that doesn't have infrastructure like this. Even mailing lists with patches are really a pre-commit review system, as are GitHub pull requests. Pre-commit testing seems more elusive though.

Every change is reviewed by someone other than the author before it lands in the repo. At google they take this a bit further. Every change has to have been either written or reviewed by a designated owner of the code (designated by subdirectory) and one of the participants must be a qualified user of the languages used in the change ("readability"). And they have technical measures in place to ensure that programs running in production descend exclusively from reviewed and committed code. Pre-commit review is common but not universal in the industry. Some shops practice post-commit review or no review. Some believe review consists entirely of quibbling.

dinvlad 3y ago
Oh, I see - by "pre-commit" it doesn't really imply it in "Git commit" sense - the change is still propagated to others (presumably by committing it as a sort of "draft"), it's just not committed to the mainline - is that correct? I'm very familiar with CR at other companies, but tbh since most use Git, I wouldn't call that "pre-commit" but "pre-merge", if you will - unless I misunderstood and it really is pre-commit at Google (i.e. the changes are not even _committed_ to the repository - and then I'm confused once again at what exactly that means..)
- joshuamorton 3y ago
  Google (for $reasons) doesn't do long lives code branches and doesn't use git, at least for the main repo. So in that context every commit is reviewed pre-commit, but you'd do the same workflow elsewhere with trunk-based development, small pull requests, and CI and automated and human review of all PRs before they're merged.
  - dinvlad 3y ago
    Right, I know it’s not on Git and hence was my question - and it sounds like this is more about terminology and less about technology. I.e. what Google does in this case is not that different from just “Code Review” in the traditional sense, as most other companies (with good engineering practices) do - reviewing code before it enters production (+CI/CD, as you mentioned). Edit: as OP mentioned, it does seem to differ in technical sense from traditional CR, in that the changes live only on developer machine, not in source control.
    - joshuamorton 3y ago
      > Edit: as OP mentioned, it does seem to differ in technical sense from traditional CR, in that the changes live only on developer machine, not in source control. Yes and no. There's a decent whitepaper on Piper and citc you can find by searching for it (or actually I will :P [0]), as far as piper is concerned they aren't checked into source control, but the vast majority of development happens in "citc" workspaces, which aren't source control, but also are source control in a sense that every save is snapshotted and you can do point in time or point in history recovery, and I can open up a readonly view of someone else's workspace if I know the right incantations, and most of the developer tools treat the local citc workspace enough-like a commit that it's transparent ish. [0]: https://cacm.acm.org/magazines/2016/7/204032-why-google-stores-billions-of-lines-of-code-in-a-single-repository/fulltext https://cacm.acm.org/magazines/2016/7/204032-why-google-stor...
      - dinvlad 3y ago
        OK, thanks for these details and the link! Somehow I remember the title, but not the content :-) That'll be an interesting read.
      - nonethewiser 3y ago
        > and I can open up a readonly view of someone else's workspace if I know the right incantations So thats not just a basic feature?
        UncleMeat 3y ago
- jeffbee 3y ago
  Yeah, there are no direct translations between git and perforce concepts. The right term within Google would in fact be "pre-submit" not "pre-commit". Before a change is submitted in the Perforce-derived flow it exists only in the author's client and isn't really part of source control in the way that git users are accustomed to pushing their branch to origin. NB: At that company there are also users of git-compatible and hg-compatible tools, but I am discussing the main perforce-derived flow.
  - dinvlad 3y ago
    Oh I see, thanks! I was under impression Google has migrated away from Perforce towards an in-house system a while ago, but looks like I was mistaken (or do you mean that system is derived from Perforce?). Edit: I guess its name is Piper.. It’s quite interesting/mind-bending to think of work-in-progress that’s still somehow synced between peers (in fact this is one of those “missing nice-to-haves” I wish Git had, and can only be approximated with wip branches..)
    - jeffbee 3y ago
      The synced-between-peers features are built atop a thing call CitC, or Client in the Cloud. An author's client isn't on their machine, it's hosted in prod.
      - dinvlad 3y ago
        OK got it, thanks for clarifying it
klooney 3y ago
The real problem with code review is that if people don't do it/just hit sign off, it's worthless. Your whole company has to believe.

benlwalker 3y ago

Pre-commit means before committed to the canonical repo, not before commit locally. The SPDK project has an elaborate pre-commit review and test system all in public. See https://spdk.io/development https://spdk.io/development . I wouldn't want to work on a project that doesn't have infrastructure like this. Even mailing lists with patches are really a pre-commit review system, as are GitHub pull requests. Pre-commit testing seems more elusive though.