6 ms·

One can ensure that every commit passes the tests suite and linting by running something like: git rebase --exec "test_cmd" --exec "lint_cmd" base_branch

by u801e 6y ago

One can ensure that every commit passes the tests suite and linting by running something like: git rebase --exec "test_cmd" --exec "lint_cmd" base_branch and any commit that doesn't pass can be fixed until you get to the point where all commits in the branch pass. Then you can find the commit that caused the issue, revert it and then make another commit that fixes the issue in the next PR (and ensure that the test suite passes for both commits with the --exec parameter to rebase. > If each single commit in your straight master history is one self contained thing it's easy to revert that whole thing and done. Not really. The more lines and files that are affected by applying the commit, the more likely there will be conflicts when trying to revert it. While that won't be the case if the commit (or merge commit) is the current head of the branch, but if other commits and merges have been made since then, then conflicts are fare more likely. On the other hand, smaller commits are much easier to revert because they tend to not change many files or many lines of code. > We are also talking SaaS here with releases going out multiple times a day. So if something is very broken it's likely to be noticed very quickly That's assuming that it's "very" broken and it's noticed quickly. Those assumptions don't always hold.

tharkun__ 5y ago
I personally think that individual commits are mine and nobody should care. They help me. They're not meant to live forever and if you make me make all of them even compile let alone build green then I will squash before ever pushing. A red build should result from the first commit on a bug fix if you ask me. I think we just fundamentally think differently. Your branches seem much longer lived. And each of your commits is more like the small commits that for us would sit directly on master. Agreed, they do not always hold. But most of the time if something really bad happens it gets noticed and we roll back to the previously deployed commit. Stuff that is only noticed later is in most cases not such a big thing and is handled as a normal bug fix. Exceptions prove the rule obviously. We also try to make individual tickets and PRs small, yes. It all plays well together and into each other. It has many advantages like easier PRs from the reviewers point of view. More likely for reviewers to actually find something useful to say as 'review fatigue' (aka 'I already read aaaaall this code and there's moooore? And teeeests? Waaaah! Whatever! ' and then they click approve) is less likely. Smaller tickets mean they get done faster and the pace that sets is more predictable than few large ones where execution time can vary considerably. Product people like predictability. You also deliver smaller chunks of changes so customers can give you more fine grained feedback vs just a large change that changes everything at once and they just go 'I hate change, undo this! Now!'. And should something really be of no use individually, feature flags are an option. Or a feature branch that doesn't live too long.
- u801e 5y ago
  > I personally think that individual commits are mine and nobody should care. They help me. They're not meant to live forever But they don't really help when reviewing because they're hard to make sense of. But if you want to make a large change in a single commit, then why not just stage the entire thing as a single commit in the first place. You can address review comments by amending the commit and force pushing it. By breaking it down into sensible commits, it makes it easier for the reviewer to review your change by filtering it by commit, so that they can see a subset of it. > You also deliver smaller chunks of changes so customers can give you more fine grained feedback Some of those changes involve some degree of refactoring. Typically, I'll separate the refactor part from the implementation part as separate commits. If I were to just make a PR that just did refactoring, then how would I justify merging it and deploying it to production? Also, making a bunch of small changes and merging them separately takes away the association between them, unlike what you get by keeping multiple related commits in a branch.
  - tharkun__ 5y ago
    I make individual commits for addressing review comments. It's sometimes helpful to see this during further review both for existing or new reviewers. They get squashed like any others afterwards. I don't see how it would be valuable after something is merged to master that I renamed a bunch of variables, extracted a method somewhere and added one test after a reviewer commented that I missed a case. Refactorings are sometimes done with a separate PR and sometimes not. Depends on the size. From our discussion we might have different philosophies on when to do that. And yes at our company merging and deploying a refactoring to master is totally acceptable and done frequently. Even if the ticket takes longer in the end, gets scrapped etc. the refactoring can probably stand on its own and be valuable if we extracted it to its own PR. Association should be there through tickets. We may have a different idea of what small is and what will be in such a commit. Let's have a made up example: You are building something that has a table view for some data. That feature is out there. But the table doesn't have sorting capabilities and no filtering and no paging. It just displays everything. There are individual tickets for adding each of these. Sorting is a small PR because the table component you use actually has that capability. You add the flag to enable sorting and the definition for the default sort and release it. Filtering is another ticket. Filtering is harder. The API this is based on doesn't support filtering and isn't owned by your team. If this had been part of the same feature branch and released together customers would have nothing yet. Instead they have sorting already. You decide to do filtering via API and not in the UI after fetching everything. So off to create a ticket for the other team and talking to them. Maybe you can help out and do it for them but in any case this will take a while. While waiting for their answer you move on to Paging! Yet another ticket. You notice that your table component doesn't support infinite scrolling. You want to have that though instead of individual paging. You create two PRs. One to the table component which adds infinite scrolling. This is an individual PR that is regression tested against all the other users of it in your code base. Once that's done you merge it. Why wait? Then you PR the actual change to use infinite scrolling with your specific table. This is at this point still based on that API that doesn't do filtering so you still always retrieve all data. But with lots of data it still helps rendering speed and the API is reasonably fast even with thousands of rows. You create a new ticket to clean this up and make it work by retrieving paged data once the API also does filtering. Or maybe you never will.

by u801e 6y ago

tharkun__ 5y ago

I personally think that individual commits are mine and nobody should care. They help me. They're not meant to live forever and if you make me make all of them even compile let alone build green then I will squash before ever pushing. A red build should result from the first commit on a bug fix if you ask me. I think we just fundamentally think differently. Your branches seem much longer lived. And each of your commits is more like the small commits that for us would sit directly on master. Agreed, they do not always hold. But most of the time if something really bad happens it gets noticed and we roll back to the previously deployed commit. Stuff that is only noticed later is in most cases not such a big thing and is handled as a normal bug fix. Exceptions prove the rule obviously. We also try to make individual tickets and PRs small, yes. It all plays well together and into each other. It has many advantages like easier PRs from the reviewers point of view. More likely for reviewers to actually find something useful to say as 'review fatigue' (aka 'I already read aaaaall this code and there's moooore? And teeeests? Waaaah! Whatever! ' and then they click approve) is less likely. Smaller tickets mean they get done faster and the pace that sets is more predictable than few large ones where execution time can vary considerably. Product people like predictability. You also deliver smaller chunks of changes so customers can give you more fine grained feedback vs just a large change that changes everything at once and they just go 'I hate change, undo this! Now!'. And should something really be of no use individually, feature flags are an option. Or a feature branch that doesn't live too long.

u801e 5y ago
> I personally think that individual commits are mine and nobody should care. They help me. They're not meant to live forever But they don't really help when reviewing because they're hard to make sense of. But if you want to make a large change in a single commit, then why not just stage the entire thing as a single commit in the first place. You can address review comments by amending the commit and force pushing it. By breaking it down into sensible commits, it makes it easier for the reviewer to review your change by filtering it by commit, so that they can see a subset of it. > You also deliver smaller chunks of changes so customers can give you more fine grained feedback Some of those changes involve some degree of refactoring. Typically, I'll separate the refactor part from the implementation part as separate commits. If I were to just make a PR that just did refactoring, then how would I justify merging it and deploying it to production? Also, making a bunch of small changes and merging them separately takes away the association between them, unlike what you get by keeping multiple related commits in a branch.
- tharkun__ 5y ago
  I make individual commits for addressing review comments. It's sometimes helpful to see this during further review both for existing or new reviewers. They get squashed like any others afterwards. I don't see how it would be valuable after something is merged to master that I renamed a bunch of variables, extracted a method somewhere and added one test after a reviewer commented that I missed a case. Refactorings are sometimes done with a separate PR and sometimes not. Depends on the size. From our discussion we might have different philosophies on when to do that. And yes at our company merging and deploying a refactoring to master is totally acceptable and done frequently. Even if the ticket takes longer in the end, gets scrapped etc. the refactoring can probably stand on its own and be valuable if we extracted it to its own PR. Association should be there through tickets. We may have a different idea of what small is and what will be in such a commit. Let's have a made up example: You are building something that has a table view for some data. That feature is out there. But the table doesn't have sorting capabilities and no filtering and no paging. It just displays everything. There are individual tickets for adding each of these. Sorting is a small PR because the table component you use actually has that capability. You add the flag to enable sorting and the definition for the default sort and release it. Filtering is another ticket. Filtering is harder. The API this is based on doesn't support filtering and isn't owned by your team. If this had been part of the same feature branch and released together customers would have nothing yet. Instead they have sorting already. You decide to do filtering via API and not in the UI after fetching everything. So off to create a ticket for the other team and talking to them. Maybe you can help out and do it for them but in any case this will take a while. While waiting for their answer you move on to Paging! Yet another ticket. You notice that your table component doesn't support infinite scrolling. You want to have that though instead of individual paging. You create two PRs. One to the table component which adds infinite scrolling. This is an individual PR that is regression tested against all the other users of it in your code base. Once that's done you merge it. Why wait? Then you PR the actual change to use infinite scrolling with your specific table. This is at this point still based on that API that doesn't do filtering so you still always retrieve all data. But with lots of data it still helps rendering speed and the API is reasonably fast even with thousands of rows. You create a new ticket to clean this up and make it work by retrieving paged data once the API also does filtering. Or maybe you never will.