5 ms·

You have apparently never had the benefit of a properly run git repository. Our master has straight history. We always rebase to merge a PR and we have build s

You have apparently never had the benefit of a properly run git repository. Our master has straight history. We always rebase to merge a PR and we have build scripts that check this as well. You will not be able to merge a PR that tries to get more than one commit onto master, if the commits touch more than one of the modules (we have a monorepo with lots of individual services). Until the PR has been approved we leave individual commits on the branch though as that can be helpful. I have not had to "mess" with rebases ever. Most rebases apply cleanly and if they don't the conflict resolution is mostly very easy. The 'hardest' conflicts to solve were actually in the end the easiest to solve, because all you had to do was to `git rebase --skip` the appropriate commits. Of course some developers that didn't know better spent hours trying to do those conflict resolutions and complained loudly. Fortunately they were that loud, so we caught it and just did the skipping. Everything applied cleanly without any conflicts after that. It's pretty awesome to have one commit per ticket on master and especially if something goes wrong. I can easily `git bisect` to find which ticket introduced a bug or a regression and I don't ever have to mess around. All commits have to have a ticket number in the commit message. It also enables firefighters to almost mechanically solve production issues. Prod is broken after the last production deploy? There's very very likely only one new commit since the last deploy before that (or maybe a few). In any case, just revert to the commit of the previous deploy, lock down the deployments and now you have time to properly solve the issue without time pressure. If there was more than one commit and you can pinpoint which of them caused the issue it would even be very easy to just revert that one commit and let deployments continue. No dealing with 27 commits that make up one ticket. Know thy tools!

u801e 6y ago
> You will not be able to merge a PR that tries to get more than one commit onto master What value does the merge commit in the PR provide if a PR only has a single commit? Couldn't you essentially halve the number of commits in the commit history by amending the single PR commit message to contain the text that the merge commit will contain anyway?
- tharkun__ 6y ago
  In case it wasn't clear from my post, I am very all over the place with the word 'merge'. I usually use it to describe the concept. We don't ever have actual merge commits. We merge by rebasing and that then makes the 'merge' just a fast forward.
da39a3ee 6y ago
When a bug is found in production, it should usually be fixed by: 1. Make a commit supplying the test coverage that was evidently lacking. 2. Make a commit supplying the fix. Everything you say is valid. However, what I just described is the simplest and clearest example of valuable multi-commit-per-ticket git history.
- tharkun__ 6y ago
  I myself do and encourage my guys to do multiple commits to checkpoint their work. Also for bug fixes as you describe, first commit is the test proving its broken. This should turn the build red. Then another commit with the fix which makes the build green. All of this on the branch. PR it like that. It's valuable to see this history. Once the PR is approved and before merging to master, rebase abd squash. The build failing intermediate commit is no longer valuable.
  - david_allison 6y ago
    There's negative value in having a commit in the history which turns the build red; it should be removed, but the requirement that the branch is squashed onto master doesn't follow. It'd be a perfectly valid alternative in some cases to clean the commit history and rebase a number of atomic commits onto master if it would make the history more readable.
    - da39a3ee 6y ago
      Suppose (a) You suddenly realize that the production code in the main (not deployed) branch is incorrect (b) There is no test coverage for the bug (c) You have time to commit a test reproducing the bug, but a fix will take days. In this situation you think there is negative value in committing a failing test that will make the build go red? Perhaps you were forgetting that sometimes commits alter production code paths and sometimes they alter test code?
      - david_allison 6y ago
        If you turn the main branch red, you affect the workflows of all developers who pull until you turn the build green again. It may also affect git bisect depending on the bisect workflow used. The ideal would be to revert the change if known, or disable deployment on the branch until the issue is fixed with an ignored test*. I feel a workflow where you explicitly break the build [incl. tests] when a bug is discovered isn't ideal.
ThrustVectoring 6y ago
> if the commits touch more than one of the modules (we have a monorepo with lots of individual services) That seems wrong to me - a big advantage to having a monorepo is that when you need to make a change to how two services work together, you can do so with a single atomic commit.
- tharkun__ 6y ago
  I may not have been clear. You can merge a single commit to master (as a fast forward rebase) regardless of how many modules it touches. But you can't do it with more than one commit. It will refuse this because we don't want non squashed stuff on master and it was an easy enough hack to prevent that. Yes it's not fool proof but works well enough so far. It discourages feature branches too which we also want. All these scripts are part of the monorepo and if I need a feature branch for something I can easily exclude that branch in these checks. I've only done that once in the last few years. This stuff is also very fluid and changed and improved as we go. If the easy solution turns out to not work well enough for enough common cases _then_ we spend more time making it better. Otherwise 'good enough' does the trick for us. YMMV depending on your company size and developer culture. E.g. you might have too many cowboys that just add exceptions all the time or remove the checks entirely. Nobody can help you there. Fire then or flee ;)

u801e 6y ago

> You will not be able to merge a PR that tries to get more than one commit onto master What value does the merge commit in the PR provide if a PR only has a single commit? Couldn't you essentially halve the number of commits in the commit history by amending the single PR commit message to contain the text that the merge commit will contain anyway?

tharkun__ 6y ago
In case it wasn't clear from my post, I am very all over the place with the word 'merge'. I usually use it to describe the concept. We don't ever have actual merge commits. We merge by rebasing and that then makes the 'merge' just a fast forward.

da39a3ee 6y ago

When a bug is found in production, it should usually be fixed by: 1. Make a commit supplying the test coverage that was evidently lacking. 2. Make a commit supplying the fix. Everything you say is valid. However, what I just described is the simplest and clearest example of valuable multi-commit-per-ticket git history.

tharkun__ 6y ago
I myself do and encourage my guys to do multiple commits to checkpoint their work. Also for bug fixes as you describe, first commit is the test proving its broken. This should turn the build red. Then another commit with the fix which makes the build green. All of this on the branch. PR it like that. It's valuable to see this history. Once the PR is approved and before merging to master, rebase abd squash. The build failing intermediate commit is no longer valuable.
- david_allison 6y ago
  There's negative value in having a commit in the history which turns the build red; it should be removed, but the requirement that the branch is squashed onto master doesn't follow. It'd be a perfectly valid alternative in some cases to clean the commit history and rebase a number of atomic commits onto master if it would make the history more readable.
  - da39a3ee 6y ago
    Suppose (a) You suddenly realize that the production code in the main (not deployed) branch is incorrect (b) There is no test coverage for the bug (c) You have time to commit a test reproducing the bug, but a fix will take days. In this situation you think there is negative value in committing a failing test that will make the build go red? Perhaps you were forgetting that sometimes commits alter production code paths and sometimes they alter test code?
    - david_allison 6y ago
      If you turn the main branch red, you affect the workflows of all developers who pull until you turn the build green again. It may also affect git bisect depending on the bisect workflow used. The ideal would be to revert the change if known, or disable deployment on the branch until the issue is fixed with an ignored test*. I feel a workflow where you explicitly break the build [incl. tests] when a bug is discovered isn't ideal.

ThrustVectoring 6y ago

> if the commits touch more than one of the modules (we have a monorepo with lots of individual services) That seems wrong to me - a big advantage to having a monorepo is that when you need to make a change to how two services work together, you can do so with a single atomic commit.

tharkun__ 6y ago
I may not have been clear. You can merge a single commit to master (as a fast forward rebase) regardless of how many modules it touches. But you can't do it with more than one commit. It will refuse this because we don't want non squashed stuff on master and it was an easy enough hack to prevent that. Yes it's not fool proof but works well enough so far. It discourages feature branches too which we also want. All these scripts are part of the monorepo and if I need a feature branch for something I can easily exclude that branch in these checks. I've only done that once in the last few years. This stuff is also very fluid and changed and improved as we go. If the easy solution turns out to not work well enough for enough common cases _then_ we spend more time making it better. Otherwise 'good enough' does the trick for us. YMMV depending on your company size and developer culture. E.g. you might have too many cowboys that just add exceptions all the time or remove the checks entirely. Nobody can help you there. Fire then or flee ;)