7 ms·

> Developers should be required to submit _clean_ PRs, that is, PR's whose git history has been organized and refactored in such a way that it removed "clean up

> Developers should be required to submit _clean_ PRs, that is, PR's whose git history has been organized and refactored in such a way that it removed "clean up commits", "typo fix", etc. A complete and utter waste of time. You spend more time messing about with rebase than solving problems. When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seemed to do you wouldn't be there.

preordained 5y ago
Could not agree more...I wish I had more of substance to add, but an upvote didn't feel sufficient
qudat 5y ago
Agreed but I also think this depends heavily on context. If the organization cares a lot of clean commit messages, have a nice git history, and want PRs to be submitted in a clean state, great! This is not always the case. I've worked on codebases where we rarely scan the git history messages for anything useful, it's just as easy for us to dig into the code to figure out what's wrong.
dyeje 5y ago
It's a prime example of engineers mistaking aesthetics for utility.
tablespoon 5y ago
> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. Eh. Not really. A clean history is a good resource for figuring out what was changed and why. Cleaning up history isn't even that much work anyway. > When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seemed to do you wouldn't be there. Only if the commit messages are shitty, which unfortunately they often are. I try to summarize what I'm changing and what I indent those changes to accomplish, because if there's one thing I hate doing, it's spending more time than necessary to puzzle out what I (or a coworker) was thinking at the time. VCS history can be a pretty powerful tool, but a messy history discourages the use of it.
- hbogert 5y ago
  amen! really, do people even use git bisect? If you do, you go through some rite of passage and I've seen people including myself make better (not perfect) commits. It remains a black art to gauge what the perfect amount of work in a commit should be.
throwaway1777 5y ago
Strongly disagree. Mainline branch commit history should be clean atomic commits that you can roll back. Otherwise you are just asking for trouble on any merge conflict or hotfix scenario.
pizza234 5y ago
> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. I'm also one of those "micro committers". Because of this attitude, many hundreds pedantic commit rebases after, I'm now a better programmer (and I also spend next to no time with that type of rebases). This is because I have now a much higher capacity (and speed) of breaking down problems into smaller, self-contained, steps. There is a common understanding that VCs are just storages. VCs actually map the mental model of the developer. Having a precise, clear, granular VC history has a bidirectional relationship with being a precise, granular, clear developer.
mrkeen 5y ago
> You spend more time messing about with rebase than solving problems Rebase should be quick and straightforward. If it's not, you've got bigger problems. By rebasing, you're taking responsibility for changing the codebase as it exists now, rather than how it used to be.
sebastialonso 5y ago
I think I'm missing something here. How valuable is to have 20 commits of "fix this error" , "fix the fix of the error", "revert all fixes", "real fix".... etc? I'd argue a PR with many commits such as these, conveys little no no useful information, when the actual change is 1-3 LOC. what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. Sure, it can take a lot of time, but it's done once, by you, after all you're supplying the changes. And if it takes a really long time, you're doing it wrong in the first place by submitting PRs with many LOC. Have you tried rebasing branches with dozens of short useless commits? That's a real waste of time, and everyone needs to do this if you want to update your main branch. So you've now multiplied the amount of wasted time for everyone.
- darknavi 5y ago
  > what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. So... squashing?
  - phone8675309 5y ago
    Yes, but the author of the code, who would presumably know their intent, gets to choose how many commits to squash to instead of just one. This means that they can preserve the development process in a logical way.
  - u801e 5y ago
    What if the change really should be split into multiple commits. Having a mega commit affecting hundreds of lines across tens of files doesn't make reviewing the change easy, and reverting it will result in conflicts.
    - Seb-C 5y ago
      I would ask such changes to be split into multiple independent PRs
thom 5y ago
Do people here have examples of some bugs for which they had to resort to VCS history to find the cause? I'm struggling to picture a single bug in my whole career where this would have been quicker than just following the logic of the code. If there's information in commit messages that isn't evident in the code itself, that seems a terrible way to live.
- pizza234 5y ago
  As wrote in another comment, bisecting (which is for me a significant tool) relies on history (specifically, a granular one). However, it also must a disciplined history.
  - thom 5y ago
    What's an example of a bug you've had to bisect for recently? Forgive me, it just seems like such a last resort thing.
    - LocalPCGuy 5y ago
      I don't bisect regularly, but when I do, it's usually trying to figure out what the original reason for introducing the code that is problematic is. The entire point is on projects big enough, you may not be able to just "follow the logic" enough to know that your fix the to apparent bug isn't re-introducing some regression that was fixed previously. So bisect to me, is a way to figure out where and why the code was changed the way that it was, so I have a better understanding of the changes I can make going forward. That doesn't mean the change made in the past was correct and must be maintained (obviously something is broken), or that it might be that the obvious solution based on following the code logic is correct. But that doesn't mean I wasted time making sure I best understand the reasoning behind the changes made. But this is also why I don't resort to it very often, because it isn't necessary in all cases (I'd even say it isn't necessary in most cases). Bisect also allows you to see other changes made in the commit in question, and around that commit, so you get a better overall picture of the logic.
      - 5y ago
Rapzid 5y ago
I agree with you in part, depending on the project, if that means just squashing it down first. This at least can keep the integration branch somewhat sane and without a bunch of intermediate commits that are junk and won't build. For this reason I prefer people to rebase on the integration branch instead of merging it in to stay current; unless there is a reason not to. And a lot of open source projects require PRs that to be pre-squashed or be able to be squashed before merging! However, there is a time and place still for deliberately crafting the PR commits. For instance if you are wanting the reviewer to be able to review chunks of the PR in isolation/stages, or if you want each of the commits to be buildable.. I suppose also for review/testing. This all depends on the project and other factors like the wider org.
gregmac 5y ago
> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. Unless you're using a garbage client (eg, the git CLI) a rebase to get rid of the "typo" "oops" type commits (when you forgot something) takes I'd say 10-20 seconds. Most commonly I do this when I have several changes on the go at once and forget to commit a fixed unit test, new import, or something like that. I usually realize it right away, so my commit will usually be "add test" or "amend 2 commits ago" or something to remind myself which one it goes to (and it sticks out, because my other commits are usually in the format "PROJ-1234: Add new --host command-line option"). > When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seemed to do you wouldn't be there. I presume this is doing a bisect or something to narrow down where a bug was introduced? My experience comes from the opposite side. Usually I identify the line of code causing a bug, it makes me go "wtf, what is this even supposed to be doing?" so I do a git blame. Hopefully the commit message is useful and ideally leads me back to the original bug/ticket, so I can figure out what the original intent/fix was, rather than inadvertently breaking something (or re-causing a different bug it fixed). I'll also note that in code that is well-commented and well-tested, this step is rarely necessary. As an example, say I come across something like this: if (port > 443) ignoreErrors = true; Clearly, this was added to solve some some problem, but even without any context it's pretty obviously a bad solution to whatever that problem is. Git blame lets me go back to see what the original reason/bug was, evaluate if it's still relevant and then either fix it properly or safely remove this line.
- FalconSensei 5y ago
  > My experience comes from the opposite side. Usually I identify the line of code causing a bug, it makes me go "wtf, what is this even supposed to be doing?" so I do a git blame. If the commit message is something like: PROJECT-TICKETNUMBER Description, you can you know... see the ticket/issue along with full details and discussions?
- sethherr 5y ago
  > I'll also note that in code that is well-commented and well-tested, this step is rarely necessary. This is the most important line from this comment. If I'm using blame or bisect, it means I did something wrong. I merge 10 times a day - and if it takes me a minute, because I'm slower than you - that means it's ten minutes a day. Add the cognitive load of the whole process and the added time of explaining it to other team members - 10 minutes is an underestimate. I'm going to spend that time writing tests and comments, since even you note that they're the better option. Squash is good enough. I spend my energy improving the right things.
globular-toast 5y ago
> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. You won't think that when customers report a regression that was introduced some time in the last year.
tharkun__ 5y ago
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 5y 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__ 5y 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.
u801e 5y ago
> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. That can be avoided by only staging and committing changes when the change is actually completely implemented. For example, I'll make a change and then I'll stage it as follows: 1. Stage and commit a new method and associated unit tests 2. Stage and commit calls to the new method 3. Stage and commit the version update Having to go through extensive rebasing is really an issue with treating version control as a back up system rather than a way to record individual units of work with well crafted commit messages that describe what and why the change was made.

preordained 5y ago

Could not agree more...I wish I had more of substance to add, but an upvote didn't feel sufficient

qudat 5y ago

Agreed but I also think this depends heavily on context. If the organization cares a lot of clean commit messages, have a nice git history, and want PRs to be submitted in a clean state, great! This is not always the case. I've worked on codebases where we rarely scan the git history messages for anything useful, it's just as easy for us to dig into the code to figure out what's wrong.

dyeje 5y ago

It's a prime example of engineers mistaking aesthetics for utility.

tablespoon 5y ago

> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. Eh. Not really. A clean history is a good resource for figuring out what was changed and why. Cleaning up history isn't even that much work anyway. > When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seemed to do you wouldn't be there. Only if the commit messages are shitty, which unfortunately they often are. I try to summarize what I'm changing and what I indent those changes to accomplish, because if there's one thing I hate doing, it's spending more time than necessary to puzzle out what I (or a coworker) was thinking at the time. VCS history can be a pretty powerful tool, but a messy history discourages the use of it.

hbogert 5y ago
amen! really, do people even use git bisect? If you do, you go through some rite of passage and I've seen people including myself make better (not perfect) commits. It remains a black art to gauge what the perfect amount of work in a commit should be.

throwaway1777 5y ago

Strongly disagree. Mainline branch commit history should be clean atomic commits that you can roll back. Otherwise you are just asking for trouble on any merge conflict or hotfix scenario.

pizza234 5y ago

> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. I'm also one of those "micro committers". Because of this attitude, many hundreds pedantic commit rebases after, I'm now a better programmer (and I also spend next to no time with that type of rebases). This is because I have now a much higher capacity (and speed) of breaking down problems into smaller, self-contained, steps. There is a common understanding that VCs are just storages. VCs actually map the mental model of the developer. Having a precise, clear, granular VC history has a bidirectional relationship with being a precise, granular, clear developer.

mrkeen 5y ago

> You spend more time messing about with rebase than solving problems Rebase should be quick and straightforward. If it's not, you've got bigger problems. By rebasing, you're taking responsibility for changing the codebase as it exists now, rather than how it used to be.

sebastialonso 5y ago

I think I'm missing something here. How valuable is to have 20 commits of "fix this error" , "fix the fix of the error", "revert all fixes", "real fix".... etc? I'd argue a PR with many commits such as these, conveys little no no useful information, when the actual change is 1-3 LOC. what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. Sure, it can take a lot of time, but it's done once, by you, after all you're supplying the changes. And if it takes a really long time, you're doing it wrong in the first place by submitting PRs with many LOC. Have you tried rebasing branches with dozens of short useless commits? That's a real waste of time, and everyone needs to do this if you want to update your main branch. So you've now multiplied the amount of wasted time for everyone.

darknavi 5y ago
> what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. So... squashing?
- phone8675309 5y ago
  Yes, but the author of the code, who would presumably know their intent, gets to choose how many commits to squash to instead of just one. This means that they can preserve the development process in a logical way.
- u801e 5y ago
  What if the change really should be split into multiple commits. Having a mega commit affecting hundreds of lines across tens of files doesn't make reviewing the change easy, and reverting it will result in conflicts.
  - Seb-C 5y ago
    I would ask such changes to be split into multiple independent PRs

thom 5y ago

Do people here have examples of some bugs for which they had to resort to VCS history to find the cause? I'm struggling to picture a single bug in my whole career where this would have been quicker than just following the logic of the code. If there's information in commit messages that isn't evident in the code itself, that seems a terrible way to live.

pizza234 5y ago
As wrote in another comment, bisecting (which is for me a significant tool) relies on history (specifically, a granular one). However, it also must a disciplined history.
- thom 5y ago
  What's an example of a bug you've had to bisect for recently? Forgive me, it just seems like such a last resort thing.
  - LocalPCGuy 5y ago
    I don't bisect regularly, but when I do, it's usually trying to figure out what the original reason for introducing the code that is problematic is. The entire point is on projects big enough, you may not be able to just "follow the logic" enough to know that your fix the to apparent bug isn't re-introducing some regression that was fixed previously. So bisect to me, is a way to figure out where and why the code was changed the way that it was, so I have a better understanding of the changes I can make going forward. That doesn't mean the change made in the past was correct and must be maintained (obviously something is broken), or that it might be that the obvious solution based on following the code logic is correct. But that doesn't mean I wasted time making sure I best understand the reasoning behind the changes made. But this is also why I don't resort to it very often, because it isn't necessary in all cases (I'd even say it isn't necessary in most cases). Bisect also allows you to see other changes made in the commit in question, and around that commit, so you get a better overall picture of the logic.
    - 5y ago

Rapzid 5y ago

I agree with you in part, depending on the project, if that means just squashing it down first. This at least can keep the integration branch somewhat sane and without a bunch of intermediate commits that are junk and won't build. For this reason I prefer people to rebase on the integration branch instead of merging it in to stay current; unless there is a reason not to. And a lot of open source projects require PRs that to be pre-squashed or be able to be squashed before merging! However, there is a time and place still for deliberately crafting the PR commits. For instance if you are wanting the reviewer to be able to review chunks of the PR in isolation/stages, or if you want each of the commits to be buildable.. I suppose also for review/testing. This all depends on the project and other factors like the wider org.

gregmac 5y ago

> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. Unless you're using a garbage client (eg, the git CLI) a rebase to get rid of the "typo" "oops" type commits (when you forgot something) takes I'd say 10-20 seconds. Most commonly I do this when I have several changes on the go at once and forget to commit a fixed unit test, new import, or something like that. I usually realize it right away, so my commit will usually be "add test" or "amend 2 commits ago" or something to remind myself which one it goes to (and it sticks out, because my other commits are usually in the format "PROJ-1234: Add new --host command-line option"). > When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seemed to do you wouldn't be there. I presume this is doing a bisect or something to narrow down where a bug was introduced? My experience comes from the opposite side. Usually I identify the line of code causing a bug, it makes me go "wtf, what is this even supposed to be doing?" so I do a git blame. Hopefully the commit message is useful and ideally leads me back to the original bug/ticket, so I can figure out what the original intent/fix was, rather than inadvertently breaking something (or re-causing a different bug it fixed). I'll also note that in code that is well-commented and well-tested, this step is rarely necessary. As an example, say I come across something like this: if (port > 443) ignoreErrors = true; Clearly, this was added to solve some some problem, but even without any context it's pretty obviously a bad solution to whatever that problem is. Git blame lets me go back to see what the original reason/bug was, evaluate if it's still relevant and then either fix it properly or safely remove this line.

FalconSensei 5y ago
> My experience comes from the opposite side. Usually I identify the line of code causing a bug, it makes me go "wtf, what is this even supposed to be doing?" so I do a git blame. If the commit message is something like: PROJECT-TICKETNUMBER Description, you can you know... see the ticket/issue along with full details and discussions?
sethherr 5y ago
> I'll also note that in code that is well-commented and well-tested, this step is rarely necessary. This is the most important line from this comment. If I'm using blame or bisect, it means I did something wrong. I merge 10 times a day - and if it takes me a minute, because I'm slower than you - that means it's ten minutes a day. Add the cognitive load of the whole process and the added time of explaining it to other team members - 10 minutes is an underestimate. I'm going to spend that time writing tests and comments, since even you note that they're the better option. Squash is good enough. I spend my energy improving the right things.

globular-toast 5y ago

> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. You won't think that when customers report a regression that was introduced some time in the last year.

tharkun__ 5y ago

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 5y 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__ 5y 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.

u801e 5y ago

> A complete and utter waste of time. You spend more time messing about with rebase than solving problems. That can be avoided by only staging and committing changes when the change is actually completely implemented. For example, I'll make a change and then I'll stage it as follows: 1. Stage and commit a new method and associated unit tests 2. Stage and commit calls to the new method 3. Stage and commit the version update Having to go through extensive rebasing is really an issue with treating version control as a back up system rather than a way to record individual units of work with well crafted commit messages that describe what and why the change was made.